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] api: render what callers see from the stored row for terminal sandboxes - #1538
When the control plane marks a sandbox failed (for example the lost-started reconciler) while its node's observed record still says started, ECW GET /sandboxes/{id} answered "state":"running" and connect went ahead. ECW read the observed status before the stored lifecycle and reads any status it doesn't name as running. ECW list reads the stored lifecycle, so the two disagreed: a failed row was hidden from GET /v2/sandboxes but running in GET. The tombstones deleted and cleanup_required read as running on every ECW surface, list included. Separately, native GET /v1/sandboxes/{id} answered 500 for a cleanup_required row.
ECW reads the stored row (SandboxStateInput::rendered_lifecycle, routes/e2b/translate.rs).
The stored lifecycle decides every terminal and at-rest row: failed, quarantined, stopped, suspended, deleting, deleted, cleanup_required.
The observed status refines a live row only when it was published by the row's active node (Sandbox.active_node_id, already the direct route's node when there is one). The observed record has no execution generation to compare.
ECW reads both tombstones as deleted: hidden from list, 404 from GET and from the gate that connect, timeout and refresh share.
Admin lifecycle/status and native state are unchanged.
Native GET answers cleanup_required exactly as deleted. The by-id read excludes both, so the answer is deleted's on main: 404 {"code":"not_found","message":"sandbox not found","retryable":false,"details":[]}.
Changed test expectations
Render table, commit 1: 203 existing rows change, all with stored failed, quarantined, stopped, suspended, deleting, deleted, cleanup_required or padded Stopped/Suspended.
Their ECW get, gate, wake, pod and touch-recovery cells now equal the no-observation row.
ECW list changes only for the tombstones (running → hidden).
No admin or native cell, and no live-lifecycle row, changes.
Render table, commit 1: 63 new rows, observations started/paused/failed from a node other than the active one.
Render table, commit 2: the native cell of all 31 cleanup_required rows goes from 500 to 404.
connect_route_never_bad_gateways_for_db_state_shapes: the cleanup_required row expects 404, as deleted does, not 409.
New: get_answers_a_cleanup_required_tombstone_as_deleted compares the two tombstones' status and body through the /v1 router.
Validation
Both table tests and the connect test fail on main's code (stored failed + observed started: get=running gate=pass). The native test fails on commit 1 (500 vs 404).
SPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --all-features -p pplx-space-api -p pplx-space-api-svc -p pplx-space-cluster-controller-server: 3475 passed, Postgres running. The only skip is an ignored benchmark.
Kind, fresh cluster from this head (SPACE_KIND_TELEMETRY_MODE=otel up.sh, 9.4 min). Created an ECW sandbox, then set its row to failed with SQL while its node still reported started:
ECW GET failed, list hid it, connect 500.
Set to cleanup_required: ECW GET and connect 404, native GET 404.
On main's API, rebuilt on the same cluster: ECW GET running for both, list running for cleanup_required, native GET 500.
SKIP_BOOTSTRAP=1 scripts/tests/smoke-kind-typed.sh all (admin token, SPACE_SMOKE_KIND_COLD_RECEIPTS): ECW 91 passed, native 42 passed, 0 failed or inconclusive, 66 min, no reruns. Console scenarios skipped without Playwright, as CI withholds them.
Severity and risk
Severity: medium. ECW clients polled and connected to a dead sandbox as running, and native GET returned 500.
Risk:
A live row no longer follows an observation from a node it has left.
Native :set_timeout reads the same gate after its own state check: a suspended row whose node still reports failed/stopped is no longer refused, and a cleanup_required row answers 404, not 500.
deleting keeps its stored ECW reading, running.
Rollback: revert either commit. There are no migrations.
Mixed versions: old and new replicas read the same rows. During a rollout, ECW GET for an affected row can differ by replica. No stored state changes.
Stack
Commit 1 is ac/lifecycle-ecw-render-from-row and can merge alone. Commit 2 builds on its render table.
Not covered
Native GET /v1/sandboxes (list) still answers 500 for any page holding a cleanup_required row (seen on Kind). It is left unchanged here.
AI assistance: Claude Code drafted the code, tests and this description.
Why: Cleanup-required sandboxes now produce different native read and timeout responses. Check client compatibility before merging.
Commit: ad6e71c38766.
No actionable comments were generated in the recent review.
Walkthrough
Sandbox lifecycle rendering now uses observed status only when the observation comes from the active node and the stored lifecycle permits it. ECW routes treat tombstones as hidden or stopped, and connect returns not found for cleanup-required sandboxes. Public sandbox reads and lists exclude cleanup-required rows, and timeout updates reject them.
perplexityai/SPACE#1416: Changes SandboxStateInput, ECW translation, tombstone handling, and timeout checks for the same lifecycle behavior.
perplexityai/SPACE#1411: Changes lifecycle publication and stored lifecycle inputs that this PR uses during response rendering.
perplexityai/SPACE#1420: Changes stored lifecycle and observation handling that affect the rendering behavior defined here.
Merge Risk: High · up to ad6e7
Cleanup-required sandboxes now produce different native read and timeout responses. Check client compatibility before merging.
Security architecture risk: Low · up to ad6e7
The change makes stored terminal states authoritative and prevents stale reports from another node from deciding a sandbox’s visible state. No new authorization bypass was established. Remaining uncertainty concerns transitional lifecycle behavior and recovery under concurrent operations.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
observed — The inspected externally reachable effects concern sandbox visibility, reconnect credentials, and lifecycle activity for an authorization-scoped sandbox. Native query predicates retain organization and owner restrictions; ECW connect and refresh resolve authenticated access scope before consuming observed state.
Trust Boundaries and Controls
observed — Observation identity influences rendering, not the authorized sandbox identity. Successful connect builds credentials using the persisted sandbox’s credential epoch. Runtime lifecycle RPCs also authorize the service peer; explicit deadline updates validate execution generation and local Started state.
Resilience and Maintainability Implications
observed — Refresh does not share connect’s explicit stored teardown guard. Ignoring an other-node terminal observation can therefore admit refresh for a stopping or deleting row. The controller sends a legacy access touch without an execution-generation update; the runtime serializes it under its lifecycle lock and applies activity only while locally Started. This does not establish a control-plane teardown fence. Similar overlap was already reachable from running or absent observations before this PR.
observed — The normal timeout-update path locks and rereads sandbox ownership state before committing policy intent; runtime application follows afterward. A flag-controlled bypass and partial-failure/idempotency limitations remain pre-existing. Their production activation and complete recovery convergence were not established, so the added terminal guard should not be interpreted as proving end-to-end atomicity.
Hardening Proposals
proposed — Future hardening could carry execution generation in lifecycle observations and define a common stored-state eligibility rule for live activity mutations. This would address same-node freshness ambiguity and make teardown behavior explicit rather than relying on rendering semantics.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title uses the required "area: " prefix ("api: ") with a lowercase summary that starts with "render". It also keeps the "[cr-test]" bracket tag, which the requirement does not mention. The title m…
Description Follows The Pr Template
The description includes all required content: a substantive ## Summary, ## Validation with commands and results, ## Severity and risk with severity and risk, and an AI assistance: line identi…
Bug Fixes Name A Regression Test
The pull request fixes application behavior under crates/. Its Validation section states that both render table tests and the connect test fail on the base behavior, then reports the full `cargo nex…
Weakened Safeguards Are Justified
No unjustified safeguard weakening found. The diff changes no Cargo, CI, nextest, lint, allowlist, or baseline configuration. Existing test functions remain. The snapshot test keeps its exact `insta::…
Public-Safe Content
No public-safety violation is present. The diff contains only source, tests, snapshots, lifecycle names, and an obviously fake test key. It contains no credentials, private keys, internal hostnames or…
Full details: Title check
Explanation
The title uses the required "area: " prefix ("api: ") with a lowercase summary that starts with "render". It also keeps the "[cr-test]" bracket tag, which the requirement does not mention. The title matches the main change: terminal and at-rest sandbox rows now render from the stored lifecycle. The bracket tag is an extra prefix that may break the strict format, but the area-prefix rule is still met. The PR summary states the stored-row rendering change is the core intent, so the title is related to the main change. No trailing period and no ticket ID are present.
Full details: Description Follows The Pr Template
Explanation
The description includes all required content: a substantive ## Summary, ## Validation with commands and results, ## Severity and risk with severity and risk, and an AI assistance: line identifying Claude Code and the drafted 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 both render table tests and the connect test fail on the base behavior, then reports the full cargo nextest run with 3,475 tests passed. The diff adds regression coverage for node observations, tombstone connect handling, and native cleanup_required reads.
Full details: Weakened Safeguards Are Justified
Explanation
No unjustified safeguard weakening found. The diff changes no Cargo, CI, nextest, lint, allowlist, or baseline configuration. Existing test functions remain. The snapshot test keeps its exact insta::assert_snapshot! assertion, retains all 588 prior cases, and adds 63 other-node cases. The description gives the reason for the expected-output changes. The diff also adds targeted tests for node matching, tombstone reads, connect status, and timeout validation.
Full details: Public-Safe Content
Explanation
No public-safety violation is present. The diff contains only source, tests, snapshots, lifecycle names, and an obviously fake test key. It contains no credentials, private keys, internal hostnames or URLs, cloud identifiers, workstation paths, raw production logs, or private tracker links. The description reports a fixed API defect and test results, not an undisclosed security vulnerability or identifiable incident details.
Recent review info
Automatic approval policy
Commit: ad6e71c38766.
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.
Reviewing files that changed from the base of the PR and between 6aaa9ee and ad6e71c.
Files ignored due to path filters: 1
crates/libs/api/src/routes/e2b/snapshots/pplx_space_api__routes__e2b__state_render_tests__each_surface_renders_stored_lifecycle_and_observed_status.snap is excluded by !**/*.snap
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 8 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
When the control plane marks a sandbox
failed(for example the lost-started reconciler) while its node's observed record still saysstarted, ECWGET /sandboxes/{id}answered"state":"running"and connect went ahead. ECW read the observed status before the stored lifecycle and reads any status it doesn't name as running. ECW list reads the stored lifecycle, so the two disagreed: afailedrow was hidden fromGET /v2/sandboxesbutrunningin GET. The tombstonesdeletedandcleanup_requiredread asrunningon every ECW surface, list included. Separately, nativeGET /v1/sandboxes/{id}answered 500 for acleanup_requiredrow.SandboxStateInput::rendered_lifecycle,routes/e2b/translate.rs).failed,quarantined,stopped,suspended,deleting,deleted,cleanup_required.Sandbox.active_node_id, already the direct route's node when there is one). The observed record has no execution generation to compare.lifecycle/statusand native state are unchanged.cleanup_requiredexactly asdeleted. The by-id read excludes both, so the answer isdeleted's onmain: 404{"code":"not_found","message":"sandbox not found","retryable":false,"details":[]}.Changed test expectations
failed,quarantined,stopped,suspended,deleting,deleted,cleanup_requiredor paddedStopped/Suspended.running→hidden).started/paused/failedfrom a node other than the active one.cleanup_requiredrows goes from 500 to 404.connect_route_never_bad_gateways_for_db_state_shapes: thecleanup_requiredrow expects 404, asdeleteddoes, not 409.get_answers_a_cleanup_required_tombstone_as_deletedcompares the two tombstones' status and body through the/v1router.Validation
main's code (storedfailed+ observedstarted:get=running gate=pass). The native test fails on commit 1 (500 vs 404).cargo fmt --check,scripts/check-rust-error-policy.sh,cargo clippy --workspace --locked --all-targets --all-features -- -D warnings: clean.cargo archagainstmain's baseline: 0 required findings.SPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --all-features -p pplx-space-api -p pplx-space-api-svc -p pplx-space-cluster-controller-server: 3475 passed, Postgres running. The only skip is an ignored benchmark.SPACE_KIND_TELEMETRY_MODE=otel up.sh, 9.4 min). Created an ECW sandbox, then set its row tofailedwith SQL while its node still reportedstarted:failed, list hid it, connect 500.cleanup_required: ECW GET and connect 404, native GET 404.main's API, rebuilt on the same cluster: ECW GETrunningfor both, listrunningforcleanup_required, native GET 500.SKIP_BOOTSTRAP=1 scripts/tests/smoke-kind-typed.sh all(admin token,SPACE_SMOKE_KIND_COLD_RECEIPTS): ECW 91 passed, native 42 passed, 0 failed or inconclusive, 66 min, no reruns. Console scenarios skipped without Playwright, as CI withholds them.Severity and risk
Severity: medium. ECW clients polled and connected to a dead sandbox as running, and native GET returned 500.
Risk:
:set_timeoutreads the same gate after its own state check: asuspendedrow whose node still reportsfailed/stoppedis no longer refused, and acleanup_requiredrow answers 404, not 500.deletingkeeps its stored ECW reading,running.Rollback: revert either commit. There are no migrations.
Mixed versions: old and new replicas read the same rows. During a rollout, ECW GET for an affected row can differ by replica. No stored state changes.
Stack
Commit 1 is
ac/lifecycle-ecw-render-from-rowand can merge alone. Commit 2 builds on its render table.Not covered
Native
GET /v1/sandboxes(list) still answers 500 for any page holding acleanup_requiredrow (seen on Kind). It is left unchanged here.AI assistance: Claude Code drafted the code, tests and this description.