[zibi-pplx]

Summary

Merging this makes the shim decide a cleanup-owned row from SandboxInfo.cleanup.state as well as from its lifecycle, so its answers stay the same if a node later reports such a row under its teardown transition (STOPPING, DELETING or SUSPENDING) instead of CLEANUP_*.

• Legacy pod teardown applies its completion switch to any cleanup-owned row.
• A stopped restart refuses such a row, as it refuses CLEANUP_*.
• Rows without a cleanup state, including armed activations with only a completion, keep their lifecycle's answer.
• A cleanup state this shim doesn't know fails closed with FailedPrecondition, as an unknown lifecycle does, and as the Rust decode does ([sbx-mgr-protocol] decode an owning cleanup to its combined status #1558).

Compared with today's STOPPING answer, four row shapes change, each to today's CLEANUP_* answer: MARK_SUSPENDED after the task's own stop goes from destructive to release (the intended flip, pinned by its own case), and a cleanup with no completion, MARK_CREATED or QUARANTINE, each without the task's own stop, goes from release to destructive.

Shipping this before any node reports the new shape keeps running shims correct through that change.

Validation

• Tests: TestLegacyTeardownDecidesAnOwnedCleanupByItsCleanupStateNotItsLifecycle and TestStoppedRestartRefusesACleanupOwnedRowWhateverItsLifecycle.
• A sweep over both cleanup states, every completion, kill and delete, with and without the shim's own stop, checks that the answer under STOPPING, DELETING and SUSPENDING equals the CLEANUP_* answer.
• gofmt -l, go vet and go test pass for the shim.

Severity and risk

Severity: does not apply.

Risk: none while nodes keep publishing CLEANUP_*, which they do until the switch in the last PR of this stack is turned on. Roll back by reverting.

Stack

Part of a stack that moves CleanupRequired and CleanupFailed out of the sandbox lifecycle into their own cleanup state. Merge in order; this PR depends on #1556.

1. [shim] keep a Stop sbx-manager refused before admission as refused #1552 shim: keep a Stop refused before admission as refused (independent fix, merged)
2. [control-plane] drop cleanup_required from the stored lifecycle vocabulary #1553 control-plane: drop cleanup_required from the stored lifecycle vocabulary
3. [proto] publish whether a node-local cleanup owns a sandbox row #1554 proto: publish whether a node-local cleanup owns a sandbox row
4. [sbx-mgr] store cleanup ownership apart from the sandbox lifecycle #1555 sbx-mgr: store cleanup ownership apart from the sandbox lifecycle
5. [sbx-mgr] publish cleanup ownership on State, Wait and List #1556 sbx-mgr: publish cleanup ownership on State, Wait and List
6. [shim] decide a cleanup-owned row by its cleanup state, not its lifecycle #1557 shim: decide a cleanup-owned row by its cleanup state ← this PR
7. [sbx-mgr-protocol] decode an owning cleanup to its combined status #1558 sbx-mgr-protocol: decode an owning cleanup to its combined status
8. [sbx-mgr] count sandbox rows by lifecycle and cleanup state #1559 sbx-mgr: count sandbox rows by lifecycle and cleanup state
9. [events] carry a transition's cleanup state apart from its lifecycle #1560 events: carry a transition's cleanup state apart from its lifecycle
10. [sbx-mgr] keep cleanup out of the sandbox lifecycle state machine #1561 sbx-mgr: keep cleanup out of the sandbox lifecycle state machine
11. [sbx-mgr] publish the lifecycle under a cleanup behind a switch #1562 sbx-mgr: publish the lifecycle under a cleanup behind a switch

Each PR keeps today's published behaviour; only the last adds a switch, off by default, that publishes the lifecycle under a cleanup.

AI assistance: an AI coding agent drafted the code, tests and this description; the author reviewed them, and a second AI agent reviewed the change. Perplexity Computer rebased the stack, repaired integration conflicts, restored a test assertion in #1553, and updated this disclosure.

Created with Perplexity

[coderabbitai]

Automatic approval withheld

Automatic approval is disabled by the configured approval policy for this pull request. Review comments and requests for changes continue normally.

Commit: 80970604b408.

No actionable comments were generated in the recent review.

Walkthrough

Legacy teardown now recognizes cleanup-owned rows from lifecycle or cleanup state. Cleanup completion selects the stop path, releases the task, or allows later teardown handling. Unknown cleanup-state values return FailedPrecondition. Stopped-restart checkpointing rejects cleanup-owned rows. Tests cover teardown decisions and restart refusal under transition lifecycles.

Possibly related PRs

• perplexityai/SPACE#1555: Assigns transition lifecycles to cleanup-owned rows that this PR handles in shim teardown.
• perplexityai/SPACE#1556: Publishes cleanup ownership and completion used by this PR’s teardown and restart decisions.
• perplexityai/SPACE#1501: Adds cleanup-completion decisions for cleanup-required and cleanup-failed rows, which this PR extends to transition lifecycles.

Merge Risk: Moderate · previous review 9b7b8

Cleanup-owned rows can now take a different teardown path, including a sandbox Stop or Delete. Confirm the intended routing before merging, or explicitly accept the risk.

Security architecture risk: Moderate · up to 9b7b8

The change preserves cleanup ownership across different lifecycle representations and rejects unknown cleanup states. It also changes when sandbox stop or deletion is permitted, so retained-data safety depends on coordinated rollout and recovery behavior. No introduced security vulnerability was established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The reviewed destructive operations remain targeted to the sandbox ID stored on the selected task. The affected asset is that sandbox's retained runtime and rootfs state; the changed paths do not introduce a broader target selector. Deployment-wide exposure was not established.

Trust Boundaries and Controls

• observed — The ownership decision consumes sandbox-manager State, rather than a new cleanup-state parameter on Kill or Delete. Existing task lookup and identity revalidation continue to mediate the transition from a task request to a sandbox-manager operation.

Resilience and Maintainability Implications

• inferred — The test evidence supports shim decision mapping and local failure handling, but does not establish durable manager idempotency, consistent state publication, or cleanup ordering across cancellation and recovery. These remain evidence gaps, not demonstrated defects.

Hardening Proposals

• proposed — Make forward and reverse rollout prerequisites explicit, and validate mixed-version teardown with lost replies, concurrent requests, and recovery before enabling native publication. Include assertions that retained stopped and suspended data survive the rollback sequence.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title identifies the shim area and summarizes the cleanup-state decision. It meets the specified format.
Description Follows The Pr Template		The description includes a summary, validation and results, severity and risk, and an AI assistance line that identifies the drafted parts.
Bug Fixes Name A Regression Test		The Validation section names TestLegacyTeardownDecidesAnOwnedCleanupByItsCleanupStateNotItsLifecycle. The test checks that a cleanup-owned STOPPING row with MARK_SUSPENDED is released after the …
Weakened Safeguards Are Justified		The diff adds tests and changes teardown behavior. It does not remove or weaken a test, assertion, lint, CI check, or repository guard.
Public-Safe Content		The diff, title, and description contain no content covered by the failure conditions.

Full details: Bug Fixes Name A Regression Test

Explanation

The Validation section names TestLegacyTeardownDecidesAnOwnedCleanupByItsCleanupStateNotItsLifecycle. The test checks that a cleanup-owned STOPPING row with MARK_SUSPENDED is released after the task's own stop. Before the fix, that case takes the destructive path.

Recent review info

Automatic approval policy

Commit: 80970604b408.

Setting	Effective value	Source
Automatic approvals	Disabled	Override 3
Merge risk	Moderate	Global configuration
Security risk	Not set	Global configuration

Matching overrides (numbered as in the resolved configuration):

• Override 3 — any author; paths: .coderabbit.yaml, .coderabbit.yml, coderabbit.yaml, coderabbit.yml, .coderabbit.config.ts, .claude/settings*.json, **/.mcp.json, **/hooks.json, .agents/plugins/**/hooks/**, .agents/plugins/**/scripts/**, .agents/plugins/**/commands/**, .agents/plugins/**/agents/**, **/skills/**/hooks/**, **/skills/**/scripts/**, **/skills/**/commands/**, **/skills/**/agents/**, .claude/commands/**, .claude/agents/**, **/marketplace.json, .github/**, ci/**, !ci/scripts/setup-nextest-*.sh, scripts/source_guards/**, scripts/check*, clippy.toml, deny.toml, .gitleaks.toml, .pre-commit-config.yaml, lefthook.yaml, rust-toolchain.toml, .cargo/**, supply-chain/**, pyproject.toml, uv.lock, **/package.json, **/package-lock.json, **/go.mod, **/go.sum, **/requirements*.txt, **/requirements*.in, proto/**, db/**, docs/versioning.md, crates/l40_api/api/src/auth.rs, crates/l40_api/api/src/auth/**, crates/l40_api/api/src/middleware/auth*, crates/l40_api/api/src/middleware/auth/**, crates/l40_api/api/src/routes/authz.rs, crates/l10_foundation/bearer/**, crates/l10_foundation/rustls/**, crates/l10_foundation/guest/**, crates/l10_foundation/pii/**, shim/**, docker/**, !docker/Dockerfile.rust-build-kind, templates/**, infra/space/aws/**, infra/space/cluster/**, infra/space/identity/**, infra/space/observability/**, infra/space/packer/**, infra/space/kind/charts/**, LICENSE, SECURITY.md; automatic approvals: disabled.

Any matching disabled override withholds approval, regardless of risk. Otherwise, the strictest explicitly set limit wins for each score. Limits omitted by all matching overrides inherit the global configuration.

Not set means no limit was configured after inheritance. Use @coderabbitai configuration to inspect the resolved settings and their sources.

Run configuration

• Source mode: Auto
• Repository: perplexityai/SPACE
• Branch: zibi/cleanup-state-5-shim
• File: .coderabbit.yaml
• Commit: 9b7b88b
• Layers: Repository YAML; Enforced overrides: Organization
• Review profile: CHILL
• Plan: Enterprise
• Run ID: 1d7bbb3a-33ac-4d04-a902-29993f6ac35d

Commits

Reviewing files that changed from the base of the PR and between 8736bd8 and 9b7b88b.

Files selected for processing: 2

• shim/space.go
• shim/space_test.go

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 3 remain after this review.

[the-gigi-pplx]

Review — 3 findings

Medium

1. shim/space.go:1124-1131: cleanupOwned treats any cleanup-state value other than UNSPECIFIED as "a cleanup owns the row", including values this build doesn't know.
   • The conflict: protobuf-go keeps an unknown enum value as its raw int, so a newer node's value lands here as "owned". The shim's own rule (shim/lifecycle.go:9, enforced by observedSandboxLifecycle) is that unknown observations can't authorize destructive teardown.
   • The effect: a STOPPING row with an unknown state, no completion and no stop of the task's own now gets Stop/Delete under both kill and delete, where its lifecycle alone would release it.
   • The Rust side disagrees: 1558 decodes the same input to UnknownCleanupState and fails closed, so the two readers give different answers.
   • Suggested fix: return a FailedPrecondition for an unknown value, as observedSandboxLifecycle does, and add a row for it. The cost of that extreme is that teardown errors until the shim is upgraded, so the pod is stuck rather than wrongly torn down. That's the existing contract for an unknown lifecycle.

Low

2. The description names one flipped row; four row shapes change compared with today's STOPPING answer (all are shapes the I1 triggers allow):

   • the planned flip: MARK_SUSPENDED with the task's own stop goes from destructive to release;
   • a cleanup with no completion (UNSPECIFIED), MARK_CREATED and QUARANTINE, each without the task's own stop, go from release to destructive.

   Each new answer equals today's CLEANUP_* answer, and TestLegacyTeardownStillTearsDownOtherFailedCleanups pins those as destructive, so the behaviour matches the plan's §3. It's only the description that undercounts. An explicit table row for the planned flip would also help: today it's only implied by the sweep.

3. TestStoppedRestartRefusesACleanupOwnedRowWhateverItsLifecycle covers only STOPPING with FAILED, not REQUIRED, DELETING or SUSPENDING. Under a mutation it fails only when the real 30 s wait budget runs out (it took 30.00 s with the check disabled), so a short budget in the test would make it fail fast.

Verified against 8736bd893832cf735c478a129f0337ab0d0601f6.

What I ran:

• go test ./... in shim/.
• A sweep over STOPPING, DELETING and SUSPENDING, every completion, cleanup states 1, 2 and 99, kill and delete, with and without the task's own stop. Findings 1 and 2 come from that sweep.
• Mutations, each caught by a test:
  • dropping the state check;
  • recognizing only REQUIRED;
  • moving the cleanup arm after STOPPED/STOPPING;
  • disabling the stopped-restart check;
  • dropping the CLEANUP_* arm, which an old node still publishes;
  • deciding by message presence instead of state, which fails the armed-activation tests.

Not run: live behaviour on a node, or CI on this head.

[the-gigi-pplx]

Approved. 3 non-blocking findings in the review comment above.

[zibi-pplx]

Thanks @the-gigi-pplx. Rebased the stack on main (47f3aa39); this PR's head is now cee902db7d7f.

1. Unknown cleanup state. Agreed. cleanupOwned now returns FailedPrecondition for a value this build doesn't know, the same contract as observedSandboxLifecycle, and both callers propagate it. New cases: state 99 on STOPPING and CLEANUP_FAILED, under both kill and delete, gives FailedPrecondition with no Stop or Delete. That now matches [sbx-mgr-protocol] decode an owning cleanup to its combined status #1558's fail-closed decode.
2. Changed rows. The description now lists all four shapes that change against today's STOPPING answer. The planned flip (MARK_SUSPENDED after the task's own stop now releases) has its own explicit case.
3. Stopped restart. It now sweeps REQUIRED, FAILED and an unknown state over STOPPING, DELETING and SUSPENDING, each under a 2 s deadline, so a missed refusal fails fast.