Cloudflare collector: degrade per-call, don't discard the whole run - #1744
Open
schmetti-dev wants to merge 1 commit into
Open
Cloudflare collector: degrade per-call, don't discard the whole run#1744schmetti-dev wants to merge 1 commit into
schmetti-dev wants to merge 1 commit into
Conversation
collect() threw on the first non-OK response from any account-level
cf() listing call (routes, dns_records, kv, r2, d1, workers/scripts,
workers/domains), discarding everything already gathered in that run
- zones, DNS, workers, KV, R2, D1, all of it - the moment any ONE of
those hit a scope gap.
Found running this against a real Cloudflare account with a narrowly-
scoped API token (zone/DNS read access, but not the account-level
Workers/KV/R2/D1 APIs): the very first sync threw on the per-zone
workers/routes call and produced zero Cloudflare assets, even though
the account/zones/DNS enumeration had already succeeded moments
before. A token scoped to only some of Cloudflare's many permission
groups is a completely ordinary setup (least-privilege tokens created
for a specific deploy pipeline, for instance) - not a misconfiguration
this collector should treat as fatal.
This contradicts the collector's own stated doctrine ("a partial page,
an API error, a rate limit -> PARTIAL run, no sweep, prior state
intact" - Store.ts's sweep gate exists precisely to make this safe),
which the bare cf() throw never actually implemented for this failure
class.
Added a cfSoft() wrapper: catches a failure on a single listing call,
logs it, marks the run `partial` (so Store.applyRun's sweep gate
correctly withholds sweeping instead of expiring real assets over
incomplete data), and returns an empty array so the rest of the
enumeration proceeds. Applied to every account/zone-scoped listing
call except /accounts and /zones themselves, which stay hard failures
- without an account or the zone list, nothing else in the collector
is meaningful anyway.
Verified against the real account described above: before the fix,
`atlas sync cloudflare` returned 0 assets on a fresh graph. After,
it returns the full zone/DNS enumeration (PARTIAL, correctly not
swept) while cleanly logging which specific calls degraded and why.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
collect()threw on the first non-OK response from any account-levelcf()listing call (workers/routes,dns_records,storage/kv/namespaces,r2/buckets,d1/database,workers/scripts,workers/domains) — discarding everything already gathered in that run (zones, DNS, workers, KV, R2, D1, all of it) the moment any ONE of those hit a permission gap.Found running this against a real Cloudflare account with a narrowly-scoped API token (zone/DNS read access, but not the account-level Workers/KV/R2/D1 APIs — an entirely ordinary least-privilege setup, e.g. a token created for a specific deploy pipeline). The very first sync threw on the per-zone
workers/routescall and produced zero Cloudflare assets, even though account + zone + DNS enumeration had already succeeded moments before.This contradicts the collector's own doctrine ("a partial page, an API error, a rate limit →
PARTIALrun, no sweep, prior state intact" —Store.ts's sweep gate exists precisely to make this safe), which the barecf()throw never actually implemented for this failure class.Fix
Added a
cfSoft()wrapper: catches a failure on a single listing call, logs which one and why, marks the runpartial(soStore.applyRun's sweep gate correctly withholds sweeping instead of expiring real assets over incomplete data), and returns an empty array so the rest of the enumeration proceeds. Applied to every account/zone-scoped listing call except/accountsand/zonesthemselves, which stay hard failures — without an account or the zone list, nothing else in the collector is meaningful anyway.Testing evidence
Verified against the real account described above:
Zones + DNS records (the calls the token does have permission for) now correctly land in the graph instead of being discarded, and the run is honestly marked
PARTIALrather than either crashing or silently claimingcomplete.🤖 Generated with Claude Code