fix(aws): harden discovery scan data integrity and resume handling - #59
Merged
Merged
Conversation
Five write-path defects in AWS discovery. Four of them contradict a guarantee the codebase states in its own comments, and two destroy data that a rescan cannot recover. Activity cap no longer licenses a deletion. scanActivity discarded the row count from ListIdentities, so above activityIdentityCap it read a prefix of the account while the surface reported `reached`. Complete() stayed true and ReconcileGeneration deleted the cloud_usage history of every identity past the cap -- deterministically the same identities each scan, since the list is ordered (kind, name, id). It now reports `partial` and blocks reconciliation. Partial, not denied: nothing was refused, the read stopped short, and the operator should be told which. UpsertResource refreshes migration 021's ARN-derived columns. They were written on INSERT only, so every pre-021 resource took the conflict path forever and kept is_external = false -- a positive claim of locality about what may be a cross-account ARN, with a partial index built to query exactly that column. resource_account, is_external and object_key move together because 021's CHECKs couple them to each other and to kind. sensitivity_source and sensitivity_reason stay out: they explain a verdict this scan deliberately does not refresh. UpsertIdentity stops overwriting attrs on a partial read. iam.go says a writer "must not treat a partial role as authoritative and overwrite metadata an earlier complete read established"; it did. A throttled GetRole yields Tags=nil and boundary="", both omitempty, so a blind overwrite erased the tags that are the only ownership signal collected. The caller now says the read fell short -- detail_incomplete lives inside the attrs JSON and is omitempty, so the repository cannot infer it without parsing provider JSON. Merged rather than skipped, so the row keeps the old keys AND records that this read could not confirm them; skipping alone would leave it asserting it was fully read. One generation authority. cloud_scan_run.generation is assigned at Claim and preserved across re-claims, while Scan recomputed connector.ScanGeneration+1 per attempt. They diverge when a run is re-claimed after commitScan advanced the connector -- the crashed-worker path -- stamping entity rows a generation ahead of the observations written for the same pass, so evidence no longer joins to the inventory it explains. The worker now passes run.Generation. Unset keeps the derived behaviour for callers with no run row. ListObservations pages over a total order. observed_at alone is not one, and CloudTrail rows carry e.EventTime, which AWS reports to the second -- 12 rows share a timestamp on a real account here. id DESC matches the direction of the (workspace_id, subject, observed_at DESC) indexes. Tests: each fix has one that fails when the fix is removed, verified by removing it. Three more pin the ways a fix could be worse than the bug: a partially read role is still stamped as seen, a first sighting still lands, and an under-cap read still reconciles. Verified against a live AWS account: 13 resources degraded to pre-021 defaults were repaired by a real scan (the 2 left blank are S3 ARNs, which carry no account segment), and rows, permissions, usage and evidence all published on one generation.
adityaauthnull250401
added a commit
that referenced
this pull request
Sep 25, 2026
Kept from #59: the ARN-derived resource columns refresh on conflict, and observations page over (observed_at, id). Already covered here, so this branch's version stands: the run owns the generation (WithGeneration, T1.5); an activity read past the cap is partial and blocks usage reconciliation (D-86); and there is no per-role GetRole to half-fail -- roles come from the authorization details and D-48 keeps only the keys that read does not return -- so partialRead and its four tests are dropped.
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.
Five write-path defects in AWS discovery. Four contradict a guarantee the codebase states in its own comments, and two destroy data a rescan cannot recover.
No migrations, so rollback is a plain image revert.
The fixes
scanActivitydiscarded the count fromListIdentities, so aboveactivityIdentityCapit read a prefix while the surface reportedreached—Complete()stayed true andReconcileGenerationdeletedcloud_usagefor every identity past the cap. Ordered(kind, name, id), so the same identities were starved every scan.partial, block reconciliation. Partial rather than denied: nothing was refused, the read stopped short.is_external = false— a positive claim of locality about what may be a cross-account ARN, withidx_cloud_resource_externalbuilt to query exactly that column.resource_account,is_external,object_key. They move together because 021's CHECKs couple them to each other and tokind.iam.gosays a writer "must not treat a partial role as authoritative and overwrite metadata an earlier complete read established". It did. A throttledGetRoleyieldsTags=nil/ boundary"", bothomitempty, so a blind overwrite erased the IAM tags that are the only ownership signal collected.cloud_scan_run.generationis assigned at Claim and preserved across re-claims, whileScanrecomputedScanGeneration+1. They diverge when a run is re-claimed aftercommitScan— the crashed-worker path — stamping rows a generation ahead of the observations for the same pass.run.Generation. Unset keeps derived behaviour.observed_atalone is not a total order, and CloudTrail rows carrye.EventTime, which AWS reports to the second.id DESC, matching the direction of the(workspace_id, subject, observed_at DESC)indexes.Two deliberate non-changes
sensitivity_source/sensitivity_reasonstay out ofDoUpdates. They explain a verdict this scan deliberately does not refresh; updating them whilesensitivitystays pinned would leave a row whose reason contradicts its value.UpsertSecrethas the same unconditionalattrsoverwrite, butupsertAccessKeynever writes attrs, so the blob is always{}. Documented in place rather than fixed — a flag no caller can set is speculative API surface.Behaviour changes to know about
partialand stop reconcilingcloud_usageuntil the cap is paged. That is the safe direction — a stale row is recoverable, a deleted one is not — but it will look like a regression if unannounced.constraintStateon a throttled re-read from last-known data rather than erased data. The merge keepsdetail_incompleteaccurate so it still degrades to unknown.Verification
Eight new tests. Each fix has one that fails when the fix is removed, verified by removing it — the repo has a documented case of a test that passed with its fix removed, so this was checked rather than assumed:
erased the owner tag: map[string]string(nil)columns were not refreshed: account="" is_external=falsea truncated activity read must not license reconciliationThree tests pin the ways a fix could be worse than the bug: a partially read role is still stamped as seen, a first sighting still lands, and an under-cap read still reconciles.
Against a live AWS account: 13 resources degraded to pre-021 defaults were repaired by a real scan — the 2 left blank are S3 ARNs, which carry no account segment. A second scan triggered through the normal UI path published with identities, permissions, 1,433 usage rows and evidence all on one generation, coverage
complete.Gates:
go build,go vet, full integration suite, unit suite, migration hygiene, IGA isolation — all pass.gofmt -lflags 589 files repo-wide from CRLF line endings; these files are clean once normalised.Scope
services/cloud_gcp_scan_identities.gois a call-site update only — it passesfalseand GCP behaviour is unchanged, confirmed by the GCP suite. Two unrelated ordering fixes indiscovery_repository.goandgovernance_repository.gowere deliberately dropped to keep this PR AWS-scoped; they are still open defects worth a follow-up.