fix(workspace-server): bounded retry on delete-path EC2 stop + durable leak event #1932

Merged

hongming merged 1 commits from fix/workspace-ec2-leak-delete-retry into main 2026-05-27 06:14:21 +00:00

Conversation 7 Commits 1 Files Changed 5

+224 -6

hongming commented 2026-05-27 04:51:49 +00:00

Owner

Copy Link

Copy Source

Summary

The DELETE path's StopWorkspaceAuto → cpProv.Stop had no retry, while the restart path used cpStopWithRetry (bounded exp backoff). A transient CP/AWS hiccup on delete left the workspace row at status='removed' with instance_id populated, returned a 500, and relied entirely on the 60s CP-orphan-sweeper to re-drive the terminate. For a cascade descendant, the "client retries → replays terminate" recovery is defeated by CascadeDelete's status != 'removed' CTE filter (the descendant row is already removed, so a retry walks zero descendant rows).

This PR:

• Extracts the retry loop into cpStopWithRetryErr (returns the terminal error); cpStopWithRetry keeps its void contract for the restart paths via a one-line delegate.
• Adds stopWorkspaceForDelete: retries the CP terminate, and on exhaustion persists a durable workspace.delete.terminate_retry_exhausted row to structure_events (§Persistent structured logging gate) so the leak/pending decision is queryable.
• Deliberately does NOT clear status='removed' — the existing CP-orphan-sweeper keys on exactly that state; the row stays removed+instance_id so the backstop still re-drives it. The error is still returned so the HTTP Delete surfaces the retryable 500.

Paired with controlplane PR fix/workspace-ec2-leak-reaper (the row-gone leak class). Refs task #15.

Phase 1 (re-verified, not assumed)

• Brief H1 (no retry): confirmed.
• Brief H3 (status != 'removed' retry-defeat): partially falsified — scoped to descendants only; the root id is always re-stopped (stopAndRemove(id) runs unconditionally and cpProv.Stop resolves instance_id with no status gate).
• Brief H4 (orphan sweeper doesn't reap workspace EC2s): substantially falsified — registry/cp_orphan_sweeper.go (PR #2989, wired main.go:325) already reaps status='removed'+instance_id workspace EC2s every 60s. The real gap is the row-gone class → handled by the paired controlplane PR.

Phase 4 self-review (five-axis)

Correctness: No blocking finding — fail-direction proof discriminates new behavior (CPRetriesTransientThenSucceeds: 3 calls, no event) from pre-fix bare Stop. FYI: the per-workspace retry runs inside CascadeDelete's shared 30s cleanupCtx; a large cascade of failing descendants can truncate retries — degrades gracefully (ctx.Err → stopErr → 500 + cp-orphan-sweeper backstop).
Readability: No finding — verb-noun names match siblings; comments explain WHY (the CTE-defeat).
Architecture: No finding — stopWorkspaceForDelete in workspace_dispatchers.go and the loop in workspace_restart.go are both allowedFiles for the bare-Stop AST gate; TestRestart_CPStopOnlyInsideRetryHelper updated to recognize cpStopWithRetryErr as the relocated loop home (not loosened).
Security: No finding — no untrusted input, no secret write.
Performance: No finding — adds ≤3s backoff to an already-500 path.

Test plan

• go vet ./internal/handlers/ clean
• go test ./internal/handlers/ ./internal/registry/ green (no regressions)
• golangci-lint run ./internal/handlers/... → 0 issues
• Fail-direction proof: retry-success (no event) vs retry-exhaust (event + error)
• Stage B/C deferred — HOLD for merge. Staging needs: trigger a workspace delete with a forced CP-Stop 5xx, confirm workspace.delete.terminate_retry_exhausted row appears and the cp-orphan-sweeper subsequently clears the EC2.

tier:medium (touches the delete path + structured-logging; revert is git revert-clean).

🤖 Generated with Claude Code

---

SOP checklist (RFC#351)

• Comprehensive testing performed: Test-first with fail-direction proofs. Delete-path: CPRetriesTransientThenSucceeds (3 Stop calls, NO event) vs CPExhausts (durable structure_events INSERT + terminal error). No-backend no-op. AST gate TestRestart_CPStopOnlyInsideRetryHelper updated for the relocated retry loop. Edge: ctx-cancel mid-retry returns ctx.Err() without LEAK-SUSPECT (existing test preserved).
• Local-postgres E2E run: N/A for this PR's changed branches — the new logic is unit-covered via sqlmock + scripted CP-stop stub at the dispatcher layer; the full CascadeDelete row-state path is exercised by the existing handlers Postgres integration job in CI. Stage A (local platform boot) deferred to staging (HOLD).
• Staging-smoke verified or pending: Pending (HOLD for merge). Staging must: trigger a workspace delete with a forced CP-Stop 5xx, confirm the workspace.delete.terminate_retry_exhausted row appears, and the cp-orphan-sweeper subsequently clears the EC2.
• Root-cause not symptom: The delete path's cpProv.Stop had no retry (unlike restart's cpStopWithRetry); a transient failure 500'd with no inline recovery, and cascade-descendant replay is defeated by the status != 'removed' CTE filter. Fix = bounded retry + durable leak event, not a status-ordering hack.
• Five-Axis review walked: See the "Phase 4 self-review (five-axis)" section above — no Critical/Required; one FYI (30s cleanupCtx truncation under large failing cascades, degrades gracefully).
• No backwards-compat shim / dead code added: No. cpStopWithRetry keeps its void contract via a one-line delegate to the new cpStopWithRetryErr; no shim, no dead code, no removed-comment cruft.
• Memory/saved-feedback consulted: feedback_gitea_reviews_api_event_value (event=APPROVED), feedback_ci_status_check_combined_state (combined .statuses[].status), feedback_passwords_in_chat_are_burned/token-scrub (push URL scrubbed from .git/config), the dev-sop §Persistent-structured-logging-gate + §Staging-E2E-gate.

## Summary The DELETE path's `StopWorkspaceAuto` → `cpProv.Stop` had **no retry**, while the restart path used `cpStopWithRetry` (bounded exp backoff). A transient CP/AWS hiccup on delete left the workspace row at `status='removed'` with `instance_id` populated, returned a 500, and relied entirely on the 60s CP-orphan-sweeper to re-drive the terminate. For a cascade **descendant**, the "client retries → replays terminate" recovery is defeated by `CascadeDelete`'s `status != 'removed'` CTE filter (the descendant row is already `removed`, so a retry walks zero descendant rows). This PR: - Extracts the retry loop into `cpStopWithRetryErr` (returns the terminal error); `cpStopWithRetry` keeps its void contract for the restart paths via a one-line delegate. - Adds `stopWorkspaceForDelete`: retries the CP terminate, and on exhaustion persists a durable `workspace.delete.terminate_retry_exhausted` row to `structure_events` (§Persistent structured logging gate) so the leak/pending decision is queryable. - Deliberately does NOT clear `status='removed'` — the existing CP-orphan-sweeper keys on exactly that state; the row stays `removed`+`instance_id` so the backstop still re-drives it. The error is still returned so the HTTP Delete surfaces the retryable 500. Paired with controlplane PR `fix/workspace-ec2-leak-reaper` (the row-gone leak class). Refs task #15. ## Phase 1 (re-verified, not assumed) - Brief H1 (no retry): **confirmed**. - Brief H3 (`status != 'removed'` retry-defeat): **partially falsified** — scoped to *descendants* only; the root id is always re-stopped (`stopAndRemove(id)` runs unconditionally and `cpProv.Stop` resolves `instance_id` with no status gate). - Brief H4 (orphan sweeper doesn't reap workspace EC2s): **substantially falsified** — `registry/cp_orphan_sweeper.go` (PR #2989, wired `main.go:325`) already reaps `status='removed'`+`instance_id` workspace EC2s every 60s. The real gap is the **row-gone** class → handled by the paired controlplane PR. ## Phase 4 self-review (five-axis) **Correctness:** No blocking finding — fail-direction proof discriminates new behavior (`CPRetriesTransientThenSucceeds`: 3 calls, no event) from pre-fix bare Stop. _FYI:_ the per-workspace retry runs inside `CascadeDelete`'s shared 30s `cleanupCtx`; a large cascade of failing descendants can truncate retries — degrades gracefully (ctx.Err → stopErr → 500 + cp-orphan-sweeper backstop). **Readability:** No finding — verb-noun names match siblings; comments explain WHY (the CTE-defeat). **Architecture:** No finding — `stopWorkspaceForDelete` in `workspace_dispatchers.go` and the loop in `workspace_restart.go` are both `allowedFiles` for the bare-Stop AST gate; `TestRestart_CPStopOnlyInsideRetryHelper` updated to recognize `cpStopWithRetryErr` as the relocated loop home (not loosened). **Security:** No finding — no untrusted input, no secret write. **Performance:** No finding — adds ≤3s backoff to an already-500 path. ## Test plan - [x] `go vet ./internal/handlers/` clean - [x] `go test ./internal/handlers/ ./internal/registry/` green (no regressions) - [x] `golangci-lint run ./internal/handlers/...` → 0 issues - [x] Fail-direction proof: retry-success (no event) vs retry-exhaust (event + error) - [ ] **Stage B/C deferred — HOLD for merge.** Staging needs: trigger a workspace delete with a forced CP-Stop 5xx, confirm `workspace.delete.terminate_retry_exhausted` row appears and the cp-orphan-sweeper subsequently clears the EC2. `tier:medium` (touches the delete path + structured-logging; revert is `git revert`-clean). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- ## SOP checklist (RFC#351) - **Comprehensive testing performed:** Test-first with fail-direction proofs. Delete-path: `CPRetriesTransientThenSucceeds` (3 Stop calls, NO event) vs `CPExhausts` (durable `structure_events` INSERT + terminal error). No-backend no-op. AST gate `TestRestart_CPStopOnlyInsideRetryHelper` updated for the relocated retry loop. Edge: ctx-cancel mid-retry returns `ctx.Err()` without LEAK-SUSPECT (existing test preserved). - **Local-postgres E2E run:** N/A for this PR's changed branches — the new logic is unit-covered via sqlmock + scripted CP-stop stub at the dispatcher layer; the full CascadeDelete row-state path is exercised by the existing handlers Postgres integration job in CI. Stage A (local platform boot) deferred to staging (HOLD). - **Staging-smoke verified or pending:** Pending (HOLD for merge). Staging must: trigger a workspace delete with a forced CP-Stop 5xx, confirm the `workspace.delete.terminate_retry_exhausted` row appears, and the cp-orphan-sweeper subsequently clears the EC2. - **Root-cause not symptom:** The delete path's `cpProv.Stop` had no retry (unlike restart's `cpStopWithRetry`); a transient failure 500'd with no inline recovery, and cascade-descendant replay is defeated by the `status != 'removed'` CTE filter. Fix = bounded retry + durable leak event, not a status-ordering hack. - **Five-Axis review walked:** See the "Phase 4 self-review (five-axis)" section above — no Critical/Required; one FYI (30s cleanupCtx truncation under large failing cascades, degrades gracefully). - **No backwards-compat shim / dead code added:** No. `cpStopWithRetry` keeps its void contract via a one-line delegate to the new `cpStopWithRetryErr`; no shim, no dead code, no removed-comment cruft. - **Memory/saved-feedback consulted:** `feedback_gitea_reviews_api_event_value` (event=APPROVED), `feedback_ci_status_check_combined_state` (combined `.statuses[].status`), `feedback_passwords_in_chat_are_burned`/token-scrub (push URL scrubbed from `.git/config`), the dev-sop §Persistent-structured-logging-gate + §Staging-E2E-gate.

hongming added 1 commit 2026-05-27 04:51:49 +00:00

fix(workspace-server): bounded retry on delete-path EC2 stop + durable leak event ... e058137fbf

The DELETE path's StopWorkspaceAuto → cpProv.Stop had no retry, while the
restart path used cpStopWithRetry (bounded exp backoff). A transient CP/AWS
hiccup on delete left the workspace row at status='removed' with instance_id
populated, returned a 500, and relied entirely on the 60s CP-orphan-sweeper
to re-drive the terminate. For a cascade *descendant* the "client retries →
replays terminate" recovery is defeated by CascadeDelete's status != 'removed'
CTE filter — so the only inline recovery is a bounded retry.

This extracts the retry loop into cpStopWithRetryErr (cpStopWithRetry keeps
its void contract for the restart paths) and adds stopWorkspaceForDelete,
which retries the CP terminate and, on exhaustion, persists a durable
workspace.delete.terminate_retry_exhausted row to structure_events (the
§Persistent structured logging gate) so the leak/pending decision is
queryable. The row deliberately stays status='removed' + instance_id so the
existing CP-orphan-sweeper backstop still re-drives it; the error is still
returned so the HTTP Delete surfaces the retryable 500.

Test-first, fail-direction proof: CPRetriesTransientThenSucceeds (3 calls, no
event) vs CPExhausts (event + error) discriminate the new behavior from the
pre-fix bare Stop. AST gate updated to recognize cpStopWithRetryErr as the
relocated home of the retry loop.

Refs task #15 (workspace-ec2-leak). Paired with the controlplane workspace-
EC2 reaper PR for the row-gone leak class.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

hongming added the tier:medium label 2026-05-27 04:52:35 +00:00

core-qa approved these changes 2026-05-27 04:53:21 +00:00

core-qa left a comment

Member

Copy Link

Copy Source

core-qa review (correctness + test coverage axis):

Verified the changed-branch coverage maps to discriminating tests:

• stopWorkspaceForDelete CP-retry-then-success: asserts 3 Stop calls + NO terminate_retry_exhausted event (would fail against the pre-fix bare one-shot Stop, which makes 1 call).
• CP-retry-exhaust: asserts the durable structure_events INSERT fires AND the terminal error is returned wrapping the LAST attempt error — discriminates exhaustion from transient.
• no-backend no-op covered.
• cpStopWithRetry void contract preserved (delegates to cpStopWithRetryErr); existing restart-path retry/ctx-cancel/LEAK-SUSPECT tests still green.
• The status='removed' ordering is deliberately unchanged so the existing cp_orphan_sweeper backstop keeps working — confirmed the comment matches the behavior.

go test ./internal/handlers ./internal/registry green, golangci-lint 0 issues, AST gate updated correctly (relocated, not loosened). Stage B/C correctly deferred (HOLD). Correctness + test axes pass.

APPROVED.

core-qa review (correctness + test coverage axis): Verified the changed-branch coverage maps to discriminating tests: - stopWorkspaceForDelete CP-retry-then-success: asserts 3 Stop calls + NO terminate_retry_exhausted event (would fail against the pre-fix bare one-shot Stop, which makes 1 call). - CP-retry-exhaust: asserts the durable structure_events INSERT fires AND the terminal error is returned wrapping the LAST attempt error — discriminates exhaustion from transient. - no-backend no-op covered. - cpStopWithRetry void contract preserved (delegates to cpStopWithRetryErr); existing restart-path retry/ctx-cancel/LEAK-SUSPECT tests still green. - The status='removed' ordering is deliberately unchanged so the existing cp_orphan_sweeper backstop keeps working — confirmed the comment matches the behavior. go test ./internal/handlers ./internal/registry green, golangci-lint 0 issues, AST gate updated correctly (relocated, not loosened). Stage B/C correctly deferred (HOLD). Correctness + test axes pass. APPROVED.

core-security approved these changes 2026-05-27 04:53:31 +00:00

core-security left a comment

Member

Copy Link

Copy Source

core-security review (trust-boundary axis):

Delete-path change touches no new untrusted input: workspaceID values in CascadeDelete are already UUID-validated (validateWorkspaceID), and the new structure_events INSERT is parameterized ($1/$2/$3) — no string concat into SQL. workspace_id flows into a UUID column; lib/pq casts the validated-UUID string safely. No secret/token written to disk or logs — emitDeleteTerminateRetryExhausted payload carries only the workspace_id, attempt count, and the CP error string (no creds). Telemetry is log-and-swallow so an INSERT failure can never 500 the caller or leak via panic. event_type is a constant, not user-controlled. No authz change — the Delete handler's existing gates are untouched. Trust boundary intact.

APPROVED.

core-security review (trust-boundary axis): Delete-path change touches no new untrusted input: workspaceID values in CascadeDelete are already UUID-validated (validateWorkspaceID), and the new structure_events INSERT is parameterized ($1/$2/$3) — no string concat into SQL. workspace_id flows into a UUID column; lib/pq casts the validated-UUID string safely. No secret/token written to disk or logs — emitDeleteTerminateRetryExhausted payload carries only the workspace_id, attempt count, and the CP error string (no creds). Telemetry is log-and-swallow so an INSERT failure can never 500 the caller or leak via panic. event_type is a constant, not user-controlled. No authz change — the Delete handler's existing gates are untouched. Trust boundary intact. APPROVED.

core-qa commented 2026-05-27 04:55:38 +00:00

Member

Copy Link

Copy Source

/sop-ack comprehensive-testing

/sop-ack comprehensive-testing

core-qa commented 2026-05-27 04:55:39 +00:00

Member

Copy Link

Copy Source

/sop-ack local-postgres-e2e

/sop-ack local-postgres-e2e

core-qa commented 2026-05-27 04:55:40 +00:00

Member

Copy Link

Copy Source

/sop-ack staging-smoke

/sop-ack staging-smoke

core-qa commented 2026-05-27 04:55:40 +00:00

Member

Copy Link

Copy Source

/sop-ack root-cause

/sop-ack root-cause

core-security commented 2026-05-27 04:55:41 +00:00

Member

Copy Link

Copy Source

/sop-ack five-axis-review

/sop-ack five-axis-review

core-security commented 2026-05-27 04:55:41 +00:00

Member

Copy Link

Copy Source

/sop-ack no-backwards-compat

/sop-ack no-backwards-compat

core-security commented 2026-05-27 04:55:42 +00:00

Member

Copy Link

Copy Source

/sop-ack memory-consulted

/sop-ack memory-consulted

hongming merged commit fdd3f52bc8 into main 2026-05-27 06:14:21 +00:00

hongming referenced this issue from a commit 2026-05-27 06:14:22 +00:00

fix(workspace-server): retry EC2 terminate on delete (#1932)

Sign in to join this conversation.

Reviewers

No Reviewers

core-qa

core-security

Labels

Clear labels

area/ci

CI/CD pipeline issues

do-not-auto-merge

Opt out of autonomous merge-queue merging

kind/infrastructure

Infrastructure-related issues

merge-queue

Ready for serialized Gitea merge queue

merge-queue-hold

Temporarily hold PR in merge queue

platform/go

Go platform test issues

release-blocker

Blocks the staging→main promotion / a release

release-test

security

test-label-sre

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

triage-test

test

wip

Work in progress — do not auto-merge

No Label tier:medium

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent-dev-a agent-dev-b agent-pm agent-researcher agent-reviewer agent-reviewer-1 agent-reviewer-cr2 app-fe (Molecule AI · app-fe) app-lead (Molecule AI · app-lead) app-qa (Molecule AI · app-qa) claude-ceo-assistant claude-ci-reader claude-status-reaper core-be (Molecule AI · core-be) core-devops (Molecule AI · core-devops) core-fe (Molecule AI · core-fe) core-lead (Molecule AI · core-lead) core-offsec (Molecule AI · core-offsec) core-qa (Molecule AI · core-qa) core-security (Molecule AI · core-security) core-uiux (Molecule AI · core-uiux) cp-be (Molecule AI · cp-be) cp-lead (Molecule AI · cp-lead) cp-qa (Molecule AI · cp-qa) cp-security (Molecule AI · cp-security) cui (Zhanlin Cui) dev-lead (Molecule AI · dev-lead) devops-engineer documentation-specialist (Molecule AI · documentation-specialist) fullstack-engineer (Molecule AI · fullstack-engineer) hongming hongming-ceo-delegated hongming-codex-laptop hongming-kimi-laptop hongming-pc2 infra-lead (Molecule AI · infra-lead) infra-runtime-be (Molecule AI · infra-runtime-be) infra-sre (Molecule AI · infra-sre) integration-tester (Molecule AI · integration-tester) mc-drift-bot molecule-code-reviewer plugin-dev (Molecule AI · plugin-dev) pm publish-runtime-bot pypi-publisher (Molecule AI PyPI Publisher (RFC#596)) release-manager (Molecule AI · release-manager) sdk-dev (Molecule AI · sdk-dev) sdk-lead (Molecule AI · sdk-lead) sop-drift-bot sop-tier-bot (SOP Tier-Check Bot) technical-writer (Molecule AI · technical-writer) triage-operator (Molecule AI · triage-operator)

No Assignees

3 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#1932