You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[cr-test] sandbox-store: refuse post-action stamps over another intent's lifecycle - #1480
A lifecycle action's post-action stamp no longer overwrites the lifecycle a concurrent delete or suspend wrote while the action's substrate call was in flight. stamp_action_lifecycle, stamp_started_after_start and the pooled update_and_fetch_post_wake_sandbox refused only terminal lifecycles. So a start that finished while a delete held the row deleting stamped it started/on_node.
Each stamp is now a typed ActionStamp (crates/l30_storage/sandbox/store/src/action_stamp.rs) that writes only while the row is in a named in-flight set: every lifecycle an action may begin from that no other intent owns, plus the results a concurrent completion of the same action leaves. deleting, stopping, suspending, suspended, terminal and unknown values are refused (module docs name each row owner). Two stamps still write over suspended:
a suspend's own;
every Direct start (StartSubstrate::LeavesSuspended). A Direct node cold-boots a suspended sandbox inside the start, and a Direct suspend writes no suspending, so the stamp cannot tell whose suspended it sees. A wrong started needs a suspend to finish between the start's node call and its stamp, and the node's suspended report does not correct it; a wrong suspended would need one to finish between the entry read and the node call, a longer window, and would stay until the next wake. A Kubernetes start of a suspended row claims restoring first, so a suspended its stamp finds is a newer suspend's, and the stamp refuses it.
A start makes at most one post-result write; the dispatcher's shared stamp no longer repeats it, so it cannot undo a write that lands in between. A refusal matches zero rows, returns false and is logged at info; the action still succeeds. The pooled post-wake update keeps its existing CAS-miss error, so that create fails.
Changed test expectations
post_action_stamps_write_only_over_an_in_flight_lifecycle (renamed): the 15 known-defect cells (suspending, suspended, stopping, deleting, _Suspended_, all three columns) flip ! to .; KNOWN_DEFECTS 15 to 0; bogus flips A to ..
Direct completed_actions_keep_a_terminal_lifecycle_that_landed_mid_flight also lands deleting, stopping, suspending, suspended: all kept except (Start, suspended), which ends started/on_node. A new test, a_start_keeps_a_rewrite_that_landed_after_its_stamp, lands a synthetic stopped/unplaced rewrite right after a Direct start's stamp and expects it kept.
Its Kubernetes twin: the start_after_wake_ack case also lands those four; each is kept and the start succeeds.
a_successful_start_drops_a_disk_pin_and_keeps_a_suspend_pin: inputs pass the start substrate; expectations unchanged.
New action_stamp::tests, per stamp.
Validation
At this head, rebased onto main after #1426 turned the changed Direct and Kubernetes tests into rstest cases: SPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --all-features -p pplx-space-api -p pplx-space-sandbox-store passes 2553 (1 ignored benchmark). Each of this PR's tests takes under 0.4 s (ci/scripts/check-test-durations.py passes). With main's non-test code, the matrix test, the six Direct cases, the Kubernetes start_after_wake_ack case and the rewrite test fail. fmt, workspace clippy -D warnings and ci/scripts/run-archcheck.sh are clean.
On an earlier revision of this change, before #1426 reshaped its tests, with the same non-test code:
Before the fix: on origin/main code the matrix fails on the 15 cross-intent cells and the three bogus cells, and the Direct test with Start with Some("deleting"). With a start stamp that refused suspended and a second, lifecycle-only start write, (Start, suspended) ended suspended/unplaced and the rewrite case started/unplaced. All pass here. A mutation putting every start on the Direct stamp fails the Kubernetes test at suspended.
SPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --all-features -p pplx-space-sandbox-store -p pplx-space-api: 2260 passed, Postgres and Redis running.
fmt, scripts/check-rust-error-policy.sh, workspace clippy -D warnings and cargo arch: clean.
Kind, fresh cluster (up.sh, otel, 494 s): a native :pause whose stamp waited on an uncommitted deleting write succeeded, logged sandbox lifecycle stamp refused, and the row stayed deleting.
scripts/tests/smoke-kind-typed.sh all, CI's admin-token and cold-receipts setup: ECW 91 and native 42 passed, 0 failed, 3998 s. Six console scenarios skipped (no Chromium).
Not run end to end: the Direct start race (Kind runs Kubernetes); the Direct sim test covers it.
Severity and risk
Severity: medium. It needs a delete or suspend to begin mid-action, and corrupts the row that teardown or restore then trusts.
Risk: a refused stamp leaves the row to its owner, and the row keeps the owner's state if it never finishes. If that owner is a delete or a drain evacuation that then fails, its compensation puts back the lifecycle it read before the action finished, so the row can describe the sandbox as it was before the action. A fence for those compensations is a follow-up. Values outside the stored vocabulary are now refused; no writer stores one. Rollback: revert.
Rollout
No schema change. Old replicas keep the old guard, so the race stays possible until every API and cluster-controller replica runs this version.
Overlaps
Open #559 restructures update_and_fetch_post_wake_sandbox in responses.rs; whichever lands second carries the = ANY($3::TEXT[]) guard and POOLED_POST_WAKE_FROM across.
AI assistance: Claude Code wrote the code, the tests and this description.
Why: These changes govern which concurrent sandbox lifecycle transitions can write the row. No specific failure was established, but the lifecycle-write changes carry high merge risk and warrant owner acceptance before merging.
Commit: a7491f97634a.
No actionable comments were generated in the recent review.
Walkthrough
Lifecycle writers now use typed stamps with substrate-specific source-state allowlists. The store normalizes lifecycle values before guarded updates and reports whether a row changed. API action mapping and pooled post-wake compare-and-set use allowed in-flight states. Tests cover accepted and refused states, start placement, concurrent lifecycle landings, and a start rewrite race.
Possibly related PRs
perplexityai/SPACE#1412: Changes the same action-stamp types, allowed lifecycle states, and write-result behavior.
perplexityai/SPACE#1420: Guards node-publication lifecycle writes against non-canonical or unknown stored states.
perplexityai/SPACE#1399: Defines ownership rules for lifecycle projections onto suspended rows.
Merge Risk: High · up to a7491
These changes govern which concurrent sandbox lifecycle transitions can write the row. No specific failure was established, but the lifecycle-write changes carry high merge risk and warrant owner acceptance before merging.
Security architecture risk: Moderate · up to a7491
The guards reduce conflicting lifecycle writes, but recovery can restore an older lifecycle after a concurrent action has already succeeded. This can leave runtime state and recorded state inconsistent. No new authorization bypass was identified in the inspected paths.
Retained concerns
Medium · reliability · inferred: A failed competing owner can restore stale lifecycle state after a successful action's result was discarded. For example, delete captures stopped and writes deleting; an in-flight start succeeds but its new guard refuses deleting; failed-delete compensation can then restore stopped. The successful start's disk-pin cleanup is also skipped. This changes the compensation interaction relative to the base and can impair runtime ownership accuracy and later drain recovery. The exact runtime interleaving and comprehensive repair behavior are not proven.
Security review details
Security Blast Radius
inferred — The demonstrated failure mode concerns the addressed sandbox's persisted state, runtime ownership, and artifact metadata. It can affect operational drain recovery, but inspected evidence does not establish cross-tenant access, credential exposure, or increased privileges.
Trust Boundaries and Controls
observed — The inspected E2B callers retain authenticated organization or federated-subject access scopes and scoped sandbox lookup before controller dispatch. The changed stamp mapping operates after that lookup and derives its substrate choice internally; no new network entrypoint or scope widening was identified there.
Resilience and Maintainability Implications
observed — The inspected stopped-evacuation path checks node-reported presence, stopped status, and VM liveness. It defers when the runtime is alive or reported running, providing counterevidence against an immediate destructive evacuation consequence from a stale stopped lifecycle.
inferred — Direct's inability to distinguish an original suspended row from a newer completed suspend remains an existing limitation. The base already allowed stamping over suspended; the head retains that exception and removes the second start stamp. No increased exposure from this exception was established.
Hardening Proposals
proposed — Use operation or execution identity to fence compensation, or reconcile against the current runtime before restoring a prior lifecycle. Define how refused successful starts transfer artifact-cleanup responsibility, and validate that handoff across owner failure and repetition.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title starts with a bracketed tag "[cr-test]" and then "sandbox-store:", so the area prefix is present in the form the requirements allow. The summary that follows, "refuse post-action stamps over…
Description Follows The Pr Template
The description contains all required sections and content: ## Summary states what the change does; ## Validation names commands and results; ## Severity and risk gives severity and risk; and `A…
Bug Fixes Name A Regression Test
The pull request fixes application behavior under crates/. Its Validation section states that the matrix test, the six Direct cases, the Kubernetes start_after_wake_ack case, and the rewrite test …
Weakened Safeguards Are Justified
No unreasoned safeguard weakening is present. The diff changes only Rust source and tests; it deletes no files and changes no CI, lint, nextest, allowlist, baseline, or repository-guard configuration.…
Public-Safe Content
The title, description, and reviewed diff contain no credentials, token values, private keys, internal URLs or hostnames, private tracker links, cloud account IDs, ARNs, personal workstation paths, or…
Full details: Title check
Explanation
The title starts with a bracketed tag "[cr-test]" and then "sandbox-store:", so the area prefix is present in the form the requirements allow. The summary that follows, "refuse post-action stamps over another intent's lifecycle", starts with a lowercase word, has no trailing period, and contains no ticket ID. The title describes the main change: post-action lifecycle stamps now refuse rows in another intent's lifecycle. The "[cr-test]" tag is an extra bracketed prefix that the requirements do not list, but the format still matches the area-prefix rule. The title is 82 characters, which is long but not a stated requirement.
Full details: Description Follows The Pr Template
Explanation
The description contains all required sections and content: ## Summary states what the change does; ## Validation names commands and results; ## Severity and risk gives severity and risk; and AI assistance: identifies Claude Code and says it wrote the code, tests, and description.
Full details: Bug Fixes Name A Regression Test
Explanation
The pull request fixes application behavior under crates/. Its Validation section states that the matrix test, the six Direct cases, the Kubernetes start_after_wake_ack case, and the rewrite test fail with the unfixed code and pass at the pull-request head. The diff also adds or updates regression tests for refused cross-intent lifecycle stamps.
Full details: Weakened Safeguards Are Justified
Explanation
No unreasoned safeguard weakening is present. The diff changes only Rust source and tests; it deletes no files and changes no CI, lint, nextest, allowlist, baseline, or repository-guard configuration. The characterization matrix removes 15 known-defect marks by changing them to refused writes and sets KNOWN_DEFECTS from 15 to 0, with new cross-intent cases and a rewrite-race test. The new store tests assert accepted and refused stamps. The PR description and adjacent comments give specific reasons for the substrate-specific exceptions and the changed test cases.
Full details: Public-Safe Content
Explanation
The title, description, and reviewed diff contain no credentials, token values, private keys, internal URLs or hostnames, private tracker links, cloud account IDs, ARNs, personal workstation paths, or real cluster names. The operational details are lifecycle documentation and test/CI results, not raw production logs, query output, or production measurements. The described lifecycle race is a code-correctness issue, not an undisclosed security vulnerability.
Recent review info
Automatic approval policy
Commit: a7491f97634a.
Setting
Effective value
Source
Automatic approvals
Enabled
Global configuration
Merge risk
Low
Override 2
Security risk
Not set
Global configuration
Matching overrides (numbered as in the resolved configuration):
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.
Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 26 minutes.
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
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
A lifecycle action's post-action stamp no longer overwrites the lifecycle a concurrent delete or suspend wrote while the action's substrate call was in flight.
stamp_action_lifecycle,stamp_started_after_startand the pooledupdate_and_fetch_post_wake_sandboxrefused only terminal lifecycles. So a start that finished while a delete held the rowdeletingstamped itstarted/on_node.Each stamp is now a typed
ActionStamp(crates/l30_storage/sandbox/store/src/action_stamp.rs) that writes only while the row is in a named in-flight set: every lifecycle an action may begin from that no other intent owns, plus the results a concurrent completion of the same action leaves.deleting,stopping,suspending,suspended, terminal and unknown values are refused (module docs name each row owner). Two stamps still write oversuspended:StartSubstrate::LeavesSuspended). A Direct node cold-boots a suspended sandbox inside the start, and a Direct suspend writes nosuspending, so the stamp cannot tell whosesuspendedit sees. A wrongstartedneeds a suspend to finish between the start's node call and its stamp, and the node'ssuspendedreport does not correct it; a wrongsuspendedwould need one to finish between the entry read and the node call, a longer window, and would stay until the next wake. A Kubernetes start of asuspendedrow claimsrestoringfirst, so asuspendedits stamp finds is a newer suspend's, and the stamp refuses it.A start makes at most one post-result write; the dispatcher's shared stamp no longer repeats it, so it cannot undo a write that lands in between. A refusal matches zero rows, returns
falseand is logged at info; the action still succeeds. The pooled post-wake update keeps its existing CAS-miss error, so that create fails.Changed test expectations
post_action_stamps_write_only_over_an_in_flight_lifecycle(renamed): the 15 known-defect cells (suspending,suspended,stopping,deleting,_Suspended_, all three columns) flip!to.;KNOWN_DEFECTS15 to 0;bogusflipsAto..completed_actions_keep_a_terminal_lifecycle_that_landed_mid_flightalso landsdeleting,stopping,suspending,suspended: all kept except(Start, suspended), which endsstarted/on_node. A new test,a_start_keeps_a_rewrite_that_landed_after_its_stamp, lands a syntheticstopped/unplacedrewrite right after a Direct start's stamp and expects it kept.start_after_wake_ackcase also lands those four; each is kept and the start succeeds.a_successful_start_drops_a_disk_pin_and_keeps_a_suspend_pin: inputs pass the start substrate; expectations unchanged.action_stamp::tests, per stamp.Validation
At this head, rebased onto
mainafter #1426 turned the changed Direct and Kubernetes tests into rstest cases:SPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --all-features -p pplx-space-api -p pplx-space-sandbox-storepasses 2553 (1 ignored benchmark). Each of this PR's tests takes under 0.4 s (ci/scripts/check-test-durations.pypasses). Withmain's non-test code, the matrix test, the six Direct cases, the Kubernetesstart_after_wake_ackcase and the rewrite test fail. fmt, workspace clippy-D warningsandci/scripts/run-archcheck.share clean.On an earlier revision of this change, before #1426 reshaped its tests, with the same non-test code:
origin/maincode the matrix fails on the 15 cross-intent cells and the threeboguscells, and the Direct test withStart with Some("deleting"). With a start stamp that refusedsuspendedand a second, lifecycle-only start write,(Start, suspended)endedsuspended/unplacedand the rewrite casestarted/unplaced. All pass here. A mutation putting every start on the Direct stamp fails the Kubernetes test atsuspended.SPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --all-features -p pplx-space-sandbox-store -p pplx-space-api: 2260 passed, Postgres and Redis running.scripts/check-rust-error-policy.sh, workspace clippy-D warningsandcargo arch: clean.up.sh, otel, 494 s): a native:pausewhose stamp waited on an uncommitteddeletingwrite succeeded, loggedsandbox lifecycle stamp refused, and the row stayeddeleting.scripts/tests/smoke-kind-typed.sh all, CI's admin-token and cold-receipts setup: ECW 91 and native 42 passed, 0 failed, 3998 s. Six console scenarios skipped (no Chromium).Severity and risk
Severity: medium. It needs a delete or suspend to begin mid-action, and corrupts the row that teardown or restore then trusts.
Risk: a refused stamp leaves the row to its owner, and the row keeps the owner's state if it never finishes. If that owner is a delete or a drain evacuation that then fails, its compensation puts back the lifecycle it read before the action finished, so the row can describe the sandbox as it was before the action. A fence for those compensations is a follow-up. Values outside the stored vocabulary are now refused; no writer stores one. Rollback: revert.
Rollout
No schema change. Old replicas keep the old guard, so the race stays possible until every API and cluster-controller replica runs this version.
Overlaps
Open #559 restructures
update_and_fetch_post_wake_sandboxinresponses.rs; whichever lands second carries the= ANY($3::TEXT[])guard andPOOLED_POST_WAKE_FROMacross.AI assistance: Claude Code wrote the code, the tests and this description.