[the-gigi-pplx]

Summary

While a runtime upgrade fences a sandbox, the State page that node-ctl publishes from reports the execution a rollback restores, not the target the boot saved. A rolled-back upgrade is never published under its target.

Terms

• Fence: an upgrade row in fenced, preserved or booting, which refuses other activations of the sandbox (FENCING_PHASES in crates/l30_storage/sbx/mgr/store/src/db/runtime_upgrades.rs).
• Consumer: the execution saved as the sandbox's execution_metadata, which the cold start boots.
• Retained execution: the consumer the upgrade restores on a rollback (previous_execution_metadata_json). For a sandbox with no saved consumer, that's the execution its disk was captured under.

The problem

The boot saves the target as the sandbox's consumer in the transaction that moves the row to booting, before the boot runs. A failed boot's cleanup stops the sandbox with the target still saved, and the rollback restores the old consumer only once its disk is back. Meanwhile the State page published the sandbox, stopped, under the target. The control plane reads a stopped sandbox published under the target as an upgrade that reached it.

How this fixes it

• Publish the retained execution while fenced. fenced_runtime_upgrade_publications reads each fenced sandbox's retained execution in one query. state_page (read/state_page.rs) returns it as the row's published_execution, beside the stored row it leaves unchanged, and the State response publishes that in place of the saved target. The rollback writes the same execution as the consumer when it ends the fence, so the publication doesn't change at the rollback.
• Charge it to the budget first. The retained execution is charged to the page's byte budget from SQLite's own buffer before it's copied or decoded, as each sandbox row the page reads is.
• Degrade one execution, not the page. A retained execution that isn't valid text or doesn't decode publishes its sandbox without an execution, as a sandbox whose own execution doesn't decode is, and logs a warning with the sandbox id. It's never published under the target, and the rest of the page is answered as usual.
• RuntimeUpgradePhase::fences_activation and both fencing queries share FENCING_PHASES, placeholders included, and both pick the earliest fencing row. A test holds UNRETIRED_UPGRADE_PHASES_SQL, which spells the phases out for other queries, to exactly the fencing phases and verified. A sql_placeholders helper replaces three copies of the IN placeholder builder, and value_raw_bytes is the one per-value cost the inventory budgets charge.

Validation

• runtime_upgrade::tests::a_rolled_back_upgrade_is_never_published_under_its_target: a failed boot whose restore fails too leaves the target saved, and the node publishes the source both after the failed boot and after the rollback.
• runtime_upgrade::tests::a_verified_upgrade_is_published_under_its_target (while the upgrade waits in verified for its retirement, and after it retires), and the_state_rpc_publishes_the_retained_execution_while_fenced, which reads the State RPC's response for a fenced sandbox whose row has the target saved.
• Store runtime_upgrades::tests::a_fencing_upgrades_retained_execution_is_charged_to_the_page_budget (an oversized one fails the page with the budget's Limit, whether it decodes or not), the_earliest_fencing_upgrade_is_the_one_published, an_unbound_sandbox_publishes_its_disks_execution_from_the_fence (in fenced and preserved), the_unretired_phase_list_is_the_fencing_phases_and_verified, and an_undecodable_retained_execution_degrades_only_its_own_sandbox (malformed JSON, a BLOB and invalid UTF-8 text each publish that sandbox without an execution and leave the others as they were).
• Mutation checks:
  • Dropping the substitution fails the rolled-back test.
  • Binding verified in place of booting in the publication query fails the rolled-back test.
  • Skipping the budget charge fails the budget test.
  • Failing the page on a non-text value fails the degradation test.
  • Keeping the target on a decode failure fails the degradation test.
  • Keeping the last fencing row, or ordering the latest first, fails the earliest-upgrade test.
  • Publishing the retained execution only in booting fails the unbound-sandbox test.
  • Dropping the State response's override fails the State RPC test.
  • Binding verified in place of preserved in the publication query fails the verified test.
  • Dropping a phase from UNRETIRED_UPGRADE_PHASES_SQL fails the phase-list test.
• The full workspace run through CI's own entry points: clippy clean, and the unprivileged lane's 12969 tests and the doctest and feature suites passed. The privileged lane passed 72 of 78. Its 6 failures are in node-runtime-core, node-runtime-supervisor and spaced-guest-worker, which this change doesn't touch, and come from the build host: no fs-verity, a test directory under non-root-owned ancestors, and no private cgroup namespace. CI's privileged lane covers them.

Not validated at this head: the Kind smoke, which ran on the eight changes together.

Not covered

• The saved consumer itself doesn't change: the cold start still boots the target the boot saved, and the rollback still restores the retained execution. Only what the State RPC reports differs. It's the only sandbox-manager response that carries an execution during a fence, since export refuses a fenced sandbox (refuse_runtime_upgrade_fence).

• A retained execution that doesn't decode is published as no execution, as a saved execution that doesn't decode already is (sandbox_row.rs). Neither marks the publication degraded, so the control plane can't tell an unknown execution from a missing one. The fence still refuses other activations meanwhile.

Dependencies

None. This merges on its own: PR #1751, which it was stacked on, has merged.

Severity and risk

Severity: medium. Without it, a rolled-back upgrade reads to the control plane as one that reached its target.

Risk: low. It acts only while an upgrade fences a sandbox. It adds one query per State page, and a fenced sandbox's retained execution is charged to the page's byte budget on top of its row. Roll back by reverting.

Docs

docs/execution-environments.md, "Booting an upgrade in its pod": what the State RPC reports while an upgrade fences the sandbox, and why the target is saved before the boot rather than when it's verified (the boot is the ordinary cold start, which reads the saved consumer).

AI assistance: Claude Code drafted the code, tests, docs and this description.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The state page now loads retained execution metadata from the earliest fencing runtime upgrade and includes it in the raw-byte budget. The State RPC publishes that execution while the upgrade fences the sandbox. If retained metadata does not decode, the RPC publishes no execution for that sandbox. The changes also add runtime-upgrade database queries, shared SQL placeholder generation, tests, and documentation updates.

Merge Risk: Moderate · up to 7f69a

The State RPC now publishes the execution a rollback restores while a runtime upgrade fences a sandbox, and named tests cover this. The cluster-level Kind smoke has not been validated at this head, so it should be run or the gap explicitly accepted before merge.

Security architecture risk: Minimal · up to 7f69a

The change aligns upgrade reporting with rollback behavior without expanding access or lifecycle authority. No introduced material security, failure-containment, or rollback risk was established in the inspected change.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The directly affected scope is execution reporting and State-call failure behavior for requested sandboxes served by one manager. The inspected change adds no lifecycle mutation authority: State still checks its existing host-observer peer policy before processing sandbox IDs.

Trust Boundaries and Controls

• observed — State delegates peer authorization before reading data. Request-selected sandbox IDs remain bound SQL values in the new fence query. The shared placeholder helper accepts only a count and emits question-mark placeholders; the fixed phase-list caller uses a three-element constant, while the variable sandbox-list query returns early on empty input.

Resilience and Maintainability Implications

• observed — Inspected test source asserts publication continuity across failed disk restoration and later rollback, target publication after verification, budget rejection before decoding, deterministic earliest-fence selection, and isolation of malformed retained metadata. These assertions support the intended containment behavior but were not executed during this review.

Pre-merge checks: 6

Check name	Status	Explanation
Title check		The title uses the bracketed area prefix "[sbx-mgr]" and then a summary starting with a lowercase word, "publish", with no trailing period or ticket ID. It names the main change: the State RPC publish…
Description Follows The Pr Template		The PR description contains all four required items. This check depends only on the description text, so no code inspection was needed. 1. "## Summary" section: Present with content. It states tha…
Bug Fixes Name A Regression Test		This PR fixes an application defect under crates/. While a runtime upgrade fences a sandbox, the State RPC published the saved target instead of the retained execution that rollback restores. The ch…
Weakened Safeguards Are Justified		The check looks for removed or weakened tests, assertions, lints, CI checks, guards, allowlists, or review-guidance text. The diff touches 10 files (PR #1752, author the-gigi-pplx, from the provider r…
Agent Tooling Runs No New Commands		The check applies to agent plugin manifests, skills, command files, and guideline files. This PR does not add or change any of them. - The changed-file inventory (git diff --name-status against the …
Public-Safe Content		The check passes. I scanned every added line in the 10 changed files, and the PR title and description, for the sensitive categories the check lists. - Credentials, tokens, private keys: none foun…

Full details: Title check

Explanation

The title uses the bracketed area prefix "[sbx-mgr]" and then a summary starting with a lowercase word, "publish", with no trailing period or ticket ID. It names the main change: the State RPC publishes the execution a rollback restores while a runtime upgrade fences a sandbox, which matches the raw summary and PR objectives.

Full details: Description Follows The Pr Template

Explanation

The PR description contains all four required items. This check depends only on the description text, so no code inspection was needed. 1. "## Summary" section: Present with content. It states that while a runtime upgrade fences a sandbox, the State page reports the execution a rollback restores, not the saved target. 2. "## Validation" section: Present with content. It names the Rust tests run (for example a_rolled_back_upgrade_is_never_published_under_its_target), mutation checks, and the CI results (clippy clean, unprivileged lane 12969 tests passed, privileged lane 72 of 78 with the six failures explained). 3. "## Severity and risk" section: Present with both items. It gives "Severity: medium" and "Risk: low", with a rationale and a rollback note. 4. "AI assistance:" line: Present. It reads "AI assistance: Claude Code drafted the code, tests, docs and this description." This names the tool and the parts it drafted. Nothing is missing or limited to template HTML comments. Per the check, presence is judged, not quality.

Full details: Bug Fixes Name A Regression Test

Explanation

This PR fixes an application defect under crates/. While a runtime upgrade fences a sandbox, the State RPC published the saved target instead of the retained execution that rollback restores. The check therefore applies. The Validation section names regression tests that cover this defect. I confirmed they exist in the reviewed range: - runtime_upgrade::tests::a_rolled_back_upgrade_is_never_published_under_its_target (crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/tests.rs:782) - runtime_upgrade::tests::a_verified_upgrade_is_published_under_its_target (same file, line 814) - the_state_rpc_publishes_the_retained_execution_while_fenced (same file, line 1753) - an_undecodable_retained_execution_degrades_only_its_own_sandbox (crates/l30_storage/sbx/mgr/store/src/db/runtime_upgrades/tests.rs:1519) The Validation section also lists mutation checks. For example, dropping the substitution fails the rolled-back test. Binding verified in place of booting also fails it. These show the tests fail without the fix. The mutation results are author-reported. I did not run them, because build tooling is unavailable here. The tests' presence and naming match the claim. The check passes because the PR names tests that fail without the fix and pass with it.

Full details: Weakened Safeguards Are Justified

Explanation

The check looks for removed or weakened tests, assertions, lints, CI checks, guards, allowlists, or review-guidance text. The diff touches 10 files (PR #1752, author the-gigi-pplx, from the provider record). None of them is a protected file. - No added #[allow], #[expect], #[ignore], skip, xfail, continue-on-error, SPACE_TEST_DEPS_OPTIONAL, nextest, [lints], deny/baseline/known-failures, or reason = text. - No CI workflow, Cargo.toml, AGENTS.md, CLAUDE.md, .agents/, CONTRIBUTING.md, or docs/rust/ file changed. - No test is deleted. The two test files are additions only (+238 and +121 lines, 0 removed). - Removed lines in production code are refactors. The removed placeholders builders are replaced by sql_placeholders. The fences_activation and fencing-query phase literals move to FENCING_PHASES. The row_raw_bytes size logic moves into value_raw_bytes, and the fenced-publication step is added. The fencing phase set is unchanged. - Removed lines in docs/execution-environments.md rewrite prose about boot and saved-consumer behavior. That file is not one of the listed guidance files, and the edit does not add review or merge-risk guidance. - Added assertions only check new behavior. No assertion was removed or loosened. Nothing in the diff matches a failure condition, so the check passes.

Full details: Agent Tooling Runs No New Commands

Explanation

The check applies to agent plugin manifests, skills, command files, and guideline files. This PR does not add or change any of them. - The changed-file inventory (git diff --name-status against the review base) lists 10 files. All are Rust sources under crates/l30_storage and crates/l40_api, plus docs/execution-environments.md. - The repository does contain agent files, such as .agents/plugins/pplx-space/**/plugin.json, plugin.yaml, SKILL.md, and several AGENTS.md files. None of them appears in the diff. - The only non-Rust changed file is docs/execution-environments.md. Its added lines are prose about the runtime-upgrade State RPC behavior. It is not an agent guideline or skill file, and it adds no inline shell substitution. - A scan of all added lines in the diff found no !` substitution, no hooks, mcpServers, lspServers, or allowed-tools keys, and no process or server entry. No item meets a failure condition, so no items are listed.

Full details: Public-Safe Content

Explanation

The check passes. I scanned every added line in the 10 changed files, and the PR title and description, for the sensitive categories the check lists. - Credentials, tokens, private keys: none found. The only keyword hits were code identifiers such as Status::internal(...) and prose such as "producer evidence". - Internal hostnames, URLs, private trackers or chats: no URLs or internal hosts in the diff or description. The description refers only to PR #1751, a same-repository reference. - Cloud account IDs or ARNs: none found. - Cluster or environment names: the description mentions "Kind smoke" and "CI's privileged lane". These are generic test-lane names, not names of real clusters or internal environments. - Personal absolute paths: none found. The paths cited are repository-relative (for example crates/l30_storage/... and docs/execution-environments.md). - Raw production logs, query output or measurements: none. The description gives test counts and mutation-check results, which are CI validation results, not production data. - Incident details: the description describes a correctness defect (a rolled-back upgrade published under its target) and states that this PR fixes it. It does not contain incident narrative. - Unfixed security vulnerability: the defect is not framed as a security issue. It is fixed by this PR, and the description does not disclose an open vulnerability. No file or line requires a FAIL.

Recent review info

Run configuration

• Source mode: Auto
• Repository: perplexityai/SPACE
• Branch: gigi/pla-7603-publish-rollback-execution-while-fenced
• File: .coderabbit.yaml
• Commit: 7f69a8d
• Layers: Repository YAML; Enforced overrides: Organization
• Review profile: CHILL
• Plan: Enterprise
• Run ID: 4e8779a1-d560-4080-a091-2a5c04eec374

Commits

Reviewing files that changed from the base of the PR and between 0f588b1 and 7f69a8d.

Files selected for processing: 5

• crates/l30_storage/sbx/mgr/store/src/db.rs
• crates/l30_storage/sbx/mgr/store/src/db/read/state_page.rs
• crates/l30_storage/sbx/mgr/store/src/db/runtime_upgrades/tests.rs
• crates/l30_storage/sbx/mgr/store/src/db/sandboxes.rs
• crates/l40_api/sbx/mgr/server/src/service/grpc_state_snapshot.rs

Files skipped from review as they are similar to previous changes: 1

• crates/l30_storage/sbx/mgr/store/src/db/read/state_page.rs

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

[shehan-pplx]

Reviewed by Shehan's Review Bot

The pull request changed after this approval. CodeRabbit approves again after it reviews the current head.

The pull request changed after this approval. CodeRabbit approves again after it reviews the current head.

The pull request changed after this approval. CodeRabbit approves again after it reviews the current head.

[coderabbitai]

Additional comments: 2

crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/suspend.rs-300-300: 1

300-300: Major · Data Integrity & Integration · Quick win

Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Check the upgrade hold before returning a completed Suspend retry.

If a stop-target upgrade fences a Suspended sandbox with a pinned full suspend artifact, suspend() returns from completed_suspend_retry_response at Line 204 before this check runs. A blocking retry therefore receives OK and the old artifact ID while the upgrade can still retire that checkpoint. Check the hold before the completed-retry return, under the existing wake and artifact locks. (raw.githubusercontent.com)

crates/l40_api/sbx/mgr/server/src/service/tests.rs-10919-10919: 1

10919-10919: Minor · Functional Correctness · Quick win

Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Use the test service’s node ID in the shim identity.

shim_route_identity supplies node ID ...0001, while SandboxManagerService::for_test uses ...00fe. Identity resolution rejects that request before runtime publication. The new test bypasses resolution and asserts that the writer accepts it. Supply the service’s node ID and check resolution before asserting publication. (raw.githubusercontent.com)

As per coding guidelines, a test needs “an independent oracle and a reachable production behavior.”

Source: Coding guidelines

Review info

Run configuration

• Source mode: Auto
• Repository: perplexityai/SPACE
• Branch: gigi/pla-7603-publish-rollback-execution-while-fenced
• File: .coderabbit.yaml
• Commit: ee041d0
• Layers: Repository YAML; Enforced overrides: Organization
• Review profile: CHILL
• Plan: Enterprise
• Run ID: bc8f1127-956b-4d7d-8a58-986aeda51c77

Commits

Reviewing files that changed from the base of the PR and between ee041d0 and ee041d0.

Files selected for processing: 16

• crates/l30_storage/sbx/mgr/store/src/db/read/runtime_upgrade.rs
• crates/l30_storage/sbx/mgr/store/src/db/runtime_upgrades.rs
• crates/l30_storage/sbx/mgr/store/src/db/runtime_upgrades/tests.rs
• crates/l30_storage/sbx/mgr/store/src/db/test_fixtures.rs
• crates/l40_api/sbx/mgr/server/src/service/create/tests.rs
• crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/backup_policy_tests.rs
• crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/suspend.rs
• crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/tests.rs
• crates/l40_api/sbx/mgr/server/src/service/mod.rs
• crates/l40_api/sbx/mgr/server/src/service/reconcile.rs
• crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade.rs
• crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/node_effects/tests.rs
• crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/stale_copy/tests.rs
• crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/tests.rs
• crates/l40_api/sbx/mgr/server/src/service/tests.rs
• docs/execution-environments.md

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

[shehan-pplx]

Reviewed by Shehan's Review Bot

The pull request changed after this approval. CodeRabbit approves again after it reviews the current head.