Skip to content

fix(aws): cleanup stalls after instances disappear - #1904

Draft
vincentkoc wants to merge 5 commits into
mainfrom
fix/aws-empty-describe-cleanup-20260906
Draft

fix(aws): cleanup stalls after instances disappear#1904
vincentkoc wants to merge 5 commits into
mainfrom
fix/aws-empty-describe-cleanup-20260906

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Sep 6, 2026

Copy link
Copy Markdown
Member

What Problem This Solves

AWS cleanup could remain stuck after an instance disappeared from DescribeInstances. The previous path could not distinguish authoritative absence from a malformed response, a different account, or an interrupted provisioning attempt.

Cross-Region fallback also had a lifecycle gap: a failed Region could retain the lease's canonical SSH key after provisioning moved elsewhere.

Canonical Fix

  • Persist the authenticated 12-digit AWS account before provider mutation.
  • Use one fixed credential snapshot for each regional provisioning or cleanup operation.
  • Verify the persisted account and Region before accepting absence or mutating ingress.
  • Treat malformed or incomplete AWS inventory as retryable observation failure.
  • Treat account, Region, instance identity, and ownership contradictions as terminal authority failure.
  • Delete a failed Region's canonical SSH key with the same verified credential snapshot before fallback.
  • Stop fallback and preserve exact cleanup debt if that key cannot be deleted.
  • Keep historical leases without account authority unresolved rather than manufacturing deletion proof.
  • Verify persisted account authority before key-only cleanup; credential rotation cannot delete another account's key.
  • Reconcile legacy unscoped ingress under the authenticated account, additively, so upgrades do not revoke active CIDRs.

User Impact

Verified absence can now complete cleanup and retire access evidence. Temporary AWS failures remain scheduled for retry. Authority contradictions keep the lease and access evidence visible for operator resolution.

Successful fallback no longer leaks the lease key into abandoned Regions.

Evidence

  • Exact signed head: 680af84b34d346720dc6f646b145a5293e78d2de
  • Base: 3d739b2f1114d1924d46d70749798953f532a407
  • Independent exact-head review: no P1/P2 findings
  • Exact-head AWS fresh-PR run: run_9e9bf46104b6
  • Exact-head remote result: 2,907 passed, 3 skipped
  • Local result: 2,907 passed, 3 skipped
  • npm run format:check --prefix worker
  • npm run lint --prefix worker
  • npm run check --prefix worker
  • npm run build --prefix worker
  • node scripts/build-docs-site.mjs
  • node --test scripts/build-docs-site.test.js scripts/check-docs-links.test.js
  • git diff --check origin/main...HEAD
  • All five branch commits have valid SSH signatures

The exact-head run used a fresh checkout on a real AWS lease. It proves the published commit, build, and full test surface on the production provider path. The destructive edge cases themselves remain isolated contract tests because credential reassignment and ambiguous deletion must not be induced against shared production credentials.

Regression Coverage

Tests now cover:

  • empty canonical inventory completing cleanup;
  • malformed inventory retrying and later completing;
  • account or Region mismatch retaining unresolved evidence;
  • credential revocation and transient AWS failures retaining automatic retries;
  • one credential snapshot across STS, EC2, SSM, termination, and key deletion;
  • cancellation before and during key import;
  • fallback Region persistence across restart;
  • failed-Region key deletion before fallback;
  • key deletion transport failure stopping fallback with retryable debt;
  • key ownership contradiction stopping fallback with terminal unresolved evidence;
  • account-scoped ingress reconciliation before any write;
  • key-only cleanup refusing a different authenticated account before deletion;
  • mixed scoped and legacy-unscoped ingress remaining additive during upgrade.

Scope And Size

  • Runtime TypeScript: +1,173 / -376, net +797
  • Tests: +3,055 / -368, net +2,687
  • Docs and changelog: +46 / -9, net +37

The runtime growth represents new authority and lifecycle state that did not previously exist: exact AWS response validation, account-scoped cleanup authority, fixed credential sessions, Region fencing, retry classification, and Region-scoped key cleanup.

Post-Deploy Final-Effect Canary

After the exact merge SHA is deployed:

  1. Create one dedicated AWS lease and record its persisted account and Region.
  2. Terminate only that instance out of band.
  3. Trigger coordinator cleanup.
  4. Require cleanupStatus: complete, a valid cleanupCompletedAt, no cleanup debt, no host, and no retained access evidence.
  5. Verify the instance and canonical key are absent in the recorded Region.

Credential reassignment remains qualification-only and is not tested against shared production credentials.

@clawsweeper

clawsweeper Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@vincentkoc
vincentkoc force-pushed the fix/aws-empty-describe-cleanup-20260906 branch from 75c0136 to c35a325 Compare September 6, 2026 04:46
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 6, 2026
@clawsweeper

clawsweeper Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed September 6, 2026, 9:22 AM ET / 13:22 UTC.

ClawSweeper review

What this changes

The PR binds AWS provisioning and cleanup to recorded accounts and Regions, validates instance observations, and preserves SSH-key cleanup obligations across cancellation and regional fallback.

Merge readiness

Blocked before merge - 4 items remain

This PR remains necessary and has no newly identified blocking production-code defect. Earlier authority-proof and historical-upgrade validation gaps remain unresolved.

Priority: P2
Reviewed head: 13b8d79aaa4f2956a906f0c76ad5714ed3736ef4

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The patch has substantial regression coverage and no new blocking code finding, but authority final-effect proof and upgrade validation remain incomplete.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: Authority-chain proof required: AWSProvider uses persisted account/Region authority for deletion and Fleet cleanup completion, but the relevant allowed, reassigned-account, and revoked-credential scenarios substitute transport. The captured AWS-hosted run proves build/test execution on an earlier head; the real cleanup canary remains deferred until deployment. The member exemption from unrelated runtime proof remains intact. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: Authority-chain proof required: AWSProvider uses persisted account/Region authority for deletion and Fleet cleanup completion, but the relevant allowed, reassigned-account, and revoked-credential scenarios substitute transport. The captured AWS-hosted run proves build/test execution on an earlier head; the real cleanup canary remains deferred until deployment. The member exemption from unrelated runtime proof remains intact. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 11 items Applicable repository policy: Read the complete root AGENTS.md and checked for nested policy and matching maintainer notes; no additional applicable files were found. Provider boundaries, credential handling, and upgrade validation informed the review.
Current main still has the reported empty-response defect: On fetched main, getServer throws a generic 'aws instance not found' error for empty reservations, while findServer only accepts InvalidInstanceID.NotFound. A successful empty inventory therefore escapes as an error instead of optional absence.
Latest release retains the old lookup path: The v0.50.0 source retains getServer/findServer and lacks withLeaseOperation; the proposed account-bound operation is not already supplied by this release.
Findings None None.
Security None None.

How this fits together

Crabbox’s coordinator turns lease requests into remote machines and later cleans up their resources. Its AWS adapter checks account identity and resource ownership before deleting instances or keys and allowing the coordinator to retire access evidence.

flowchart TD
  A[Lease request or cleanup alarm] --> B[Coordinator lease record]
  B --> C[Recorded account and Region]
  C --> D[AWS credential snapshot]
  D --> E[Identity and ownership checks]
  E --> F[Observe or delete AWS resources]
  F --> G[Complete cleanup or retain cleanup debt]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: Authority-chain proof required: AWSProvider uses persisted account/Region authority for deletion and Fleet cleanup completion, but the relevant allowed, reassigned-account, and revoked-credential scenarios substitute transport. The captured AWS-hosted run proves build/test execution on an earlier head; the real cleanup canary remains deferred until deployment. The member exemption from unrelated runtime proof remains intact. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P1) - Reassigned-account and revoked-credential rejection before deletion or cleanup completion remains demonstrated only with substituted transport.
  • Resolve merge risk (P1) - Historical leases lacking account scope or Region can retain cleanup debt and require operator investigation after upgrade; the claimed unbound-present regression test still inherits a bound account.
  • Complete next step (P2) - Add isolated final-effect authority proof and correct the historical unbound-lease validation before merge. Redacted terminal traces or logs are suitable; remove credentials, private endpoints, and IP addresses. Update the PR body to trigger re-review, or ask a maintainer to comment @clawsweeper re-review.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test LOC Production +1,173 / -376; tests +3,055 / -368 The body justifies production growth through credential sessions, authority validation, Region fencing, and cleanup lifecycle tracking.

Merge-risk options

Maintainer options:

  1. Validate the final effects in isolation (recommended)
    Use an authorized isolated setup or production-path harness with real transport to establish allowed cleanup, forbidden-account rejection, credential revocation handling, and historical upgrade behavior before merge.
  2. Keep the draft pending isolated evidence
    Retain the draft if safe qualification infrastructure is unavailable rather than relying on the proposed post-deployment canary as pre-merge evidence.

Technical review

Best possible solution:

Keep account-bound cleanup and conservative historical handling, supported by isolated final-effect evidence and explicit fresh-versus-upgrade validation.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: current main throws a generic error for canonical empty DescribeInstances results that its optional lookup does not recognize as absence. No runtime reproduction was executed.

Is this the best way to solve the issue?

Yes in direction: validating absence within the AWS adapter and binding cleanup to recorded authority fits the existing lifecycle contract; merge readiness still requires final-effect and historical-upgrade evidence.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 8120119d878f.

Labels

Label justifications:

  • P2: This is a bounded AWS cleanup and fallback reliability improvement without evidence of a current broad outage.
  • merge-risk: 🚨 compatibility: Historical records without persisted account or Region evidence receive stricter cleanup handling, and their upgrade coverage is incomplete.
  • merge-risk: 🚨 security-boundary: Persisted account authority now controls deletion and cleanup completion, while forbidden-account and revoked-credential final effects lack real-transport proof.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Authority-chain proof required: AWSProvider uses persisted account/Region authority for deletion and Fleet cleanup completion, but the relevant allowed, reassigned-account, and revoked-credential scenarios substitute transport. The captured AWS-hosted run proves build/test execution on an earlier head; the real cleanup canary remains deferred until deployment. The member exemption from unrelated runtime proof remains intact. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Applicable repository policy: Read the complete root AGENTS.md and checked for nested policy and matching maintainer notes; no additional applicable files were found. Provider boundaries, credential handling, and upgrade validation informed the review. (AGENTS.md:1, 13b8d79aaa4f)
  • Current main still has the reported empty-response defect: On fetched main, getServer throws a generic 'aws instance not found' error for empty reservations, while findServer only accepts InvalidInstanceID.NotFound. A successful empty inventory therefore escapes as an error instead of optional absence. (worker/src/aws.ts:1227, 8120119d878f)
  • Latest release retains the old lookup path: The v0.50.0 source retains getServer/findServer and lacks withLeaseOperation; the proposed account-bound operation is not already supplied by this release. (worker/src/aws.ts:1227, 215115a45086)
  • Credential and deletion authority: withLeaseOperation snapshots direct-provider credentials and shares them across STS, instance observation, termination, and key deletion. AWSProvider verifies the persisted Region/account before release and rejects unbound absence. (worker/src/aws.ts:817, 13b8d79aaa4f)
  • Persisted authority reaches cleanup completion: verifyLeaseOperationAuthority and observeLeaseServer gate the AWS release path; successful release subsequently permits Fleet cleanup completion and access retirement. This is a material persisted-authority boundary, so allowed and forbidden final-effect evidence remains relevant. (worker/src/fleet.ts:28374, 13b8d79aaa4f)
  • Captured proof and discussion: The supplied body at sourceRevision 87e39845da4757542fba0828d79e9104d071dd5cf89a7d0c5c05ebdea55714ba reports AWS-hosted build/test run run_9e9bf46104b6 for head 680af84, explicitly describes destructive cases as isolated contract tests, and defers the cleanup canary until deployment. The member’s explanation at fix(aws): cleanup stalls after instances disappear #1904 (comment) appropriately avoids destructive testing with shared credentials, but does not supply final-effect proof.

Likely related people:

  • vincentkoc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Provide real-transport final-effect traces showing allowed cleanup and rejection of reassigned accounts or revoked credentials before unauthorized deletion or completion.
  • Correct the unbound-present fixture to omit providerScope and demonstrate fresh and historical cleanup behavior, including preserved legacy ingress.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (7 earlier review cycles)
  • reviewed 2026-09-06T04:51:22.443Z sha c35a325 :: needs real behavior proof before merge. :: [P1] Preserve leading zeros before validating AWS account identifiers | [P2] Enable XML syntax validation before accepting absence
  • reviewed 2026-09-06T06:45:55.276Z sha 0ac90de :: needs real behavior proof before merge. :: [P1] Bind provisioning credentials to the persisted account scope | [P2] Preserve automatic retries for transient AWS operation failures
  • reviewed 2026-09-06T08:42:00.493Z sha 8686ada :: needs real behavior proof before merge. :: [P1] [P1] Verify recorded authority before key-only cleanup | [P1] [P1] Preserve legacy lease access during account-scoped reconciliation
  • reviewed 2026-09-06T08:46:29.372Z sha 8686ada :: needs real behavior proof before merge. :: [P1] [P1] Verify recorded authority before key-only cleanup | [P1] [P1] Preserve legacy lease access during account-scoped reconciliation
  • reviewed 2026-09-06T08:53:54.689Z sha 680af84 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-06T10:08:04.644Z sha 69bbdfa :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-06T11:39:26.399Z sha 1733cd3 :: needs real behavior proof before merge. :: none

@vincentkoc
vincentkoc force-pushed the fix/aws-empty-describe-cleanup-20260906 branch 2 times, most recently from 62f6d2e to 0ac90de Compare September 6, 2026 05:36
@vincentkoc

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

Please review exact head 0ac90de2ce3a5c1523c1885e81c7018a16b828b1.

The prior findings are addressed:

  • AWS Account and ownerId values are preserved lexically, including leading zeros.
  • authoritative DescribeInstances parsing enables XML syntax validation and exact envelope checks.
  • direct STS, EC2 observation, termination confirmation, and owned key deletion share one immutable credential snapshot.
  • each fallback Region is persisted before provider mutation, with stale release/cancellation races fenced before dispatch.
  • historical unbound absence remains unresolved and its operator path is documented without an unsafe override.
  • Fleet tests cover successful retirement plus malformed XML, revocation, credential rotation, fallback restart routing, and POST/PUT pre-mutation races.

There is no reviewed isolated pre-merge AWS deployment path with separate credentials in this repository. The PR body therefore records the exact post-deployment final-effect canary instead of presenting mocked transport as live proof.

@clawsweeper

clawsweeper Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@vincentkoc

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

The exact scanner fixture repair landed on ClawSweeper main as 04e5065bfa7808d2ef60bf31cdb12e7dd6c4f5fa via openclaw/clawsweeper#1465.

Please re-review this unchanged Crabbox head: 0ac90de2ce3a5c1523c1885e81c7018a16b828b1.

@clawsweeper

clawsweeper Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@vincentkoc
vincentkoc force-pushed the fix/aws-empty-describe-cleanup-20260906 branch 2 times, most recently from 7b34f12 to 8686ada Compare September 6, 2026 08:36
@vincentkoc

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

Please review signed exact head 8686adab3c9dce500032e1ea2168836711fd148f.

The previous findings are addressed:

  • provisioning and cleanup use account-verified fixed credential snapshots;
  • malformed AWS inventory remains fail-closed but retries automatically;
  • account, Region, resource, and key-ownership contradictions remain terminal;
  • failed-Region canonical keys are deleted before fallback with the same credential snapshot;
  • failed key deletion stops fallback and preserves retryable or terminal cleanup debt as appropriate;
  • cancellation before a new Region mutates no resource and leaves no false cleanup debt.

Independent exact-head review reports no P1/P2 findings. Fresh-PR AWS run run_ea75af61dd77 checked out the exact head and passed build, docs, and all 2,905 tests.

@clawsweeper

clawsweeper Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@vincentkoc

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

Please review signed exact head 680af84b34d346720dc6f646b145a5293e78d2de.

The previous findings are addressed:

  • provisioning and cleanup use account-verified fixed credential snapshots;
  • malformed AWS inventory remains fail-closed but retries automatically;
  • account, Region, resource, and key-ownership contradictions remain terminal;
  • failed-Region canonical keys are deleted before fallback with the same credential snapshot;
  • failed key deletion stops fallback and preserves retryable or terminal cleanup debt as appropriate;
  • cancellation before a new Region mutates no resource and leaves no false cleanup debt.
  • key-only cleanup verifies persisted Region and account before deleting the canonical key;
  • legacy unscoped ingress is bound to the authenticated account and stays additive beside scoped leases.

Independent exact-head review reports no P1/P2 findings. Fresh-PR AWS run run_9e9bf46104b6 checked out the exact head and passed build, docs, and all 2,907 tests.

@clawsweeper

clawsweeper Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

@vincentkoc
vincentkoc force-pushed the fix/aws-empty-describe-cleanup-20260906 branch 2 times, most recently from 69bbdfa to 1733cd3 Compare September 6, 2026 11:34
@vincentkoc
vincentkoc force-pushed the fix/aws-empty-describe-cleanup-20260906 branch from 1733cd3 to 13b8d79 Compare September 6, 2026 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant