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: write the portability assignment renderers' statements whole and delete the renderers - #1440
pplx-space-sandbox-portability still rendered SET lists for sandboxes and for its executions and port exposures. The data-store rules forbid that: a statement stays whole at its query site, and only a whole portability boolean predicate may be interpolated. For example, repair::revert_failed_to_suspended built SET {sandbox_assignments}, restore_lease_expires_at = NULL, ..., so the text Postgres received could not be read at the query site.
Merging this:
Writes each such statement out in full at its query site:
cluster controller: pod-less drain give-up, started repair and failed-restorable repair stores.
Builds a statement once in a LazyLock<String> where its only interpolations are portability predicates, or the constant repair row-state guards.
Splits pod_tracking's started recovery into two whole statements, one per proof shape.
Leaves the orphan and repair guard fragments interpolated; the reconciler's scans compose the same fragments.
Deletes the renderers and the types only they used: clear_/write_/publish_clear_or_preserve_current_local_artifact_marker_assignments, suspended_/quarantined_k8s_finalization_sql, SuspendedK8sFinalizationSql, SuspendedTargetNodeAssignment and the execution and port-exposure assignment helpers.
Changes drain::revert_pod_less_resident_to_suspended to return bool instead of the renderer.
Updates the doc comments that named deleted functions, including node_local_backup_pending.rs. Deletes the sandbox_writes ratchet's two renderer-call patterns, which match nothing once the renderers are gone, and the comment text about renderers in ARCHITECTURE.toml and architecture-baseline.toml. The statement-head pattern is unchanged and the deleted patterns matched nothing, so no baseline cap changes.
Changed test expectations: none.
Validation
Byte-identity: I ran the touched crates and their callers against a Postgres with log_statement=all, once on origin/main and once on this head, and compared the statements by sql! module path with the locator line number removed. A temporary test, not committed, ran the two OrphanGuard::LegacyDeleting statements, which no existing test reaches. It compared 18 statement texts and all 18 match exactly:
drain pod-less revert (1);
ingest::finalize_suspended (1);
orphan recover and quarantine, each for PodName, Forgotten and LegacyDeleting (6);
pod_tracking ExecutionRetired and FailedRecovery (2);
repair started and failed (2);
suspend committed-artifact and missing-pod (2);
port-exposure reset in both repair stores (2);
give-up execution retire and port-exposure reset (2).
The nine touched modules issued the same 59 distinct statements on both sides. Across the whole log (96,614 statements on the base, 96,647 on the head, 8,042 distinct on each), the only differences were run-generated UUIDs, test database names, prepared-statement names and timestamps.
cargo fmt --all -- --check, scripts/check-rust-error-policy.sh and cargo clippy --workspace --locked --all-targets --all-features -- -D warnings pass. cargo arch reports no findings, also with --base-baseline, --renames and --base-architecture from origin/main.
SPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --all-features on sandbox-store, sandbox-portability, cluster-controller-server, internal-api-server, api, snapshot-store and snapshot-retention: 4025 passed against a real Postgres, 0 dependency skips. The one skipped test is the ignored opt-in benchmark.
Kind: a fresh cluster from an earlier revision with the same patch (infra/space/kind/scripts/up.sh, otel telemetry) came up in 514 s. scripts/tests/smoke-kind-typed.sh all, with CI's admin-token and cold-receipt setup, took 4031 s. The native surface passed 42/42. The ECW surface passed 90/91: ecw.guest_mptcp_segments_dropped hit an infrastructure error at sandbox create (guest command readiness timed out, node-mgr probe Unavailable) and passed when rerun once. Six console-UI scenarios were skipped because Playwright/Chromium isn't installed.
At this head, rebased onto main with the patch unchanged (sorted +/- lines identical): fmt, ci/scripts/run-archcheck.sh (no findings) and workspace clippy -D warnings pass. SPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --all-features on cluster-controller-server, internal-api-server, sandbox-portability and sandbox-store: 1750 passed, 0 skipped. ci/scripts/check-test-durations.py passes, and no test this PR touches takes over 2 s.
Severity and risk
Severity: does not apply; this is a structural cleanup with no behaviour change.
Risk: low. The SQL text is unchanged, so the only risk is in the Rust control flow around it, mainly bool replacing Option<SuspendedK8sFinalizationSql> in the pod-less give-up. Roll back by reverting the commit.
Mixed versions: there is no schema or wire change. Old and new binaries send the same statements, apart from sql! locator comments.
Not covered
The orphan statements (orphan::recover_orphan_to_suspended and orphan::quarantine_orphan) still interpolate the classification guard's FROM items (joined to the active execution for the pod-name guard), its predicate and its FOR UPDATE OF targets, which the reconciler's plan read composes too. quarantine_orphan also picks its artifact predicate per call (TRUE for a legacy deleting row, otherwise the negated restart eligibility). Because the guard varies per call, both statements are still built with format! on every call. Writing them as one whole statement per guard is a separate change. The repair row-state guards stay interpolated as the existing fragments docs/rust/space-conventions.md describes.
AI assistance: Claude Code drafted the code changes, the byte-identity harness and this description.
… whole and delete the renderers
pplx-space-sandbox-portability still rendered SET-list fragments for the
sandboxes row, its executions and its port exposures
(clear/write/publish_clear_or_preserve_current_local_artifact_marker_assignments,
suspended_k8s_finalization_sql and quarantined_k8s_finalization_sql with
SuspendedK8sFinalizationSql and SuspendedTargetNodeAssignment). The
data-store rules keep a statement whole at its query site and allow only a
whole portability boolean predicate to be interpolated.
Each statement that interpolated a renderer now spells it out at its query
site in sandbox-store (drain, ingest, orphan, pod_tracking, repair, suspend)
and in the cluster controller's pod-less drain give-up and started and
failed-restorable repair stores. Statements whose only interpolations are
portability predicates are built once in a LazyLock; the two proof shapes of
pod_tracking's started recovery are two whole statements. The orphan guard
and repair row-state guard fragments, which the reconciler scans also use,
stay as they are. drain::revert_pod_less_resident_to_suspended returns
whether the row flipped instead of the renderer.
The sandbox_writes ratchet drops its two renderer-call patterns, which match
nothing once the renderers are gone, and its comments about renderers; the
statement-head pattern and every baseline cap are unchanged.
Every statement text is unchanged: a Postgres statement log of the touched
crates' tests on the base and on this change records the same statements,
apart from sql! locator line numbers.
No actionable comments were generated in the recent review.
Walkthrough
Sandbox finalization and recovery paths now define SQL statements directly instead of using shared portability renderers. The statements update sandbox, execution, and port-exposure state. The sandbox-writes ratchet now matches SQL statement heads and no longer matches renderer calls.
Possibly related PRs
perplexityai/SPACE#1406: Replaces portability assignment renderers with SQL statements and changes the resident revert function to return a boolean.
perplexityai/SPACE#1360: Makes the sandbox-writes ratchet required; this PR changes the patterns that the ratchet enforces.
Merge Risk: Minimal · up to 3acb2
This is a structural cleanup that moves finalization SQL next to the code that runs it. Source comparison found the statements and guards semantically equivalent to the removed renderers, so no behavior change or user impact is expected.
Security architecture risk: Minimal · up to 3acb2
The inspected changes preserve the existing identity checks, recovery preconditions and transactional cleanup. No material security risk introduced or worsened by this change was identified.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
observed — The sensitive sinks are control-plane sandbox lifecycle and placement records, execution records, artifact-marker references and port-exposure reconciliation metadata. Repair and drain writes remain scoped to a bound sandbox identity; publication can finalize a batch selected for the publishing node.
Trust Boundaries and Controls
observed — The production publication handler obtains an authenticated node origin and passes that identity into suspended candidate selection. Selection checks active-node ownership, lifecycle and recovery evidence and locks sandbox rows. The finalizer's reliance on caller selection is pre-existing; the selector and production transaction owner are unchanged by this PR.
observed — Pod-tracking proof selection remains a private enum selecting fixed statements, not caller-provided SQL. Failed recovery requires the bound active generation to be recovery_failed with no pod identifiers and rejects a newer active generation; execution-retired recovery requires no active execution or retained pod IP.
Resilience and Maintainability Implications
inferred — The inspected transition boundaries preserve failure containment: drain retains locking, guarded transition, dependent cleanup and commit in one transaction; repair rolls back refused transitions and commits only after cleanup. Publication likewise commits after projection and dependent writes. Moving statement ownership does not create an inspected intermediate commit that could strand partially finalized state.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title uses the required lowercase area prefix with a colon ("sandbox-store:") and then a summary starting with a lowercase word ("write"). It has no trailing period and no ticket ID. The bracketed…
Description Follows The Pr Template
The description contains all required items: a substantive ## Summary with a “Merging this” explanation, a ## Validation section with commands and results, a ## Severity and risk section stating…
Bug Fixes Name A Regression Test
PASS: This PR is a structural SQL-renderer cleanup, not an application behavior bug fix. The diff replaces renderer-generated assignments with equivalent whole statements, removes renderer types, and …
Weakened Safeguards Are Justified
No unjustified safeguard weakening found. The only guard reduction removes two obsolete sandbox_writes renderer-call patterns in ARCHITECTURE.toml; the PR description specifically states that the …
Public-Safe Content
No public-safety violation found. The diff contains no credentials, private keys, private URLs, cloud IDs or ARNs, personal workstation paths, or internal hostnames. The title and description contain …
Full details: Title check
Explanation
The title uses the required lowercase area prefix with a colon ("sandbox-store:") and then a summary starting with a lowercase word ("write"). It has no trailing period and no ticket ID. The bracketed "[cr-test]" tag appears before the area prefix, which is an extra tag rather than a ticket ID or internal link. The summary matches the main change: the PR writes the portability assignment renderers' statements in full at their query sites and deletes the renderers.
Full details: Description Follows The Pr Template
Explanation
The description contains all required items: a substantive ## Summary with a “Merging this” explanation, a ## Validation section with commands and results, a ## Severity and risk section stating severity does not apply and giving low risk, and an AI assistance: line naming Claude Code and the drafted parts.
Full details: Bug Fixes Name A Regression Test
Explanation
PASS: This PR is a structural SQL-renderer cleanup, not an application behavior bug fix. The diff replaces renderer-generated assignments with equivalent whole statements, removes renderer types, and changes one return value from Option to bool; no behavior defect is claimed or introduced. The PR therefore does not require a regression test under this check.
Full details: Weakened Safeguards Are Justified
Explanation
No unjustified safeguard weakening found. The only guard reduction removes two obsolete sandbox_writes renderer-call patterns in ARCHITECTURE.toml; the PR description specifically states that the renderers were deleted and the patterns matched nothing afterward. architecture-baseline.toml changes comments only. No tests, assertions, lints, CI checks, allowlists, or failure lists were removed or loosened.
Full details: Public-Safe Content
Explanation
No public-safety violation found. The diff contains no credentials, private keys, private URLs, cloud IDs or ARNs, personal workstation paths, or internal hostnames. The title and description contain code references and test/CI results only; they do not disclose production logs, production measurements, incident details, or an unfixed security vulnerability.
Recent review info
Automatic approval policy
Commit: 3acb2e396bd4.
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.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
pplx-space-sandbox-portabilitystill rendered SET lists forsandboxesand for its executions and port exposures. The data-store rules forbid that: a statement stays whole at its query site, and only a whole portability boolean predicate may be interpolated. For example,repair::revert_failed_to_suspendedbuiltSET {sandbox_assignments}, restore_lease_expires_at = NULL, ..., so the text Postgres received could not be read at the query site.Merging this:
drain,ingest,orphan,pod_tracking,repair,suspend;LazyLock<String>where its only interpolations are portability predicates, or the constant repair row-state guards.pod_tracking's started recovery into two whole statements, one per proof shape.clear_/write_/publish_clear_or_preserve_current_local_artifact_marker_assignments,suspended_/quarantined_k8s_finalization_sql,SuspendedK8sFinalizationSql,SuspendedTargetNodeAssignmentand the execution and port-exposure assignment helpers.drain::revert_pod_less_resident_to_suspendedto returnboolinstead of the renderer.node_local_backup_pending.rs. Deletes thesandbox_writesratchet's two renderer-call patterns, which match nothing once the renderers are gone, and the comment text about renderers inARCHITECTURE.tomlandarchitecture-baseline.toml. The statement-head pattern is unchanged and the deleted patterns matched nothing, so no baseline cap changes.Changed test expectations: none.
Validation
Byte-identity: I ran the touched crates and their callers against a Postgres with
log_statement=all, once onorigin/mainand once on this head, and compared the statements bysql!module path with the locator line number removed. A temporary test, not committed, ran the twoOrphanGuard::LegacyDeletingstatements, which no existing test reaches. It compared 18 statement texts and all 18 match exactly:drainpod-less revert (1);ingest::finalize_suspended(1);orphanrecover and quarantine, each for PodName, Forgotten and LegacyDeleting (6);pod_trackingExecutionRetired and FailedRecovery (2);repairstarted and failed (2);suspendcommitted-artifact and missing-pod (2);The nine touched modules issued the same 59 distinct statements on both sides. Across the whole log (96,614 statements on the base, 96,647 on the head, 8,042 distinct on each), the only differences were run-generated UUIDs, test database names, prepared-statement names and timestamps.
Mutation: changing
ingest'starget_node_id = COALESCE(s.target_node_id, s.active_node_id)tos.target_node_idfailspublished_state_tests::lifecycle_writers::{ready_backup_projects_suspended_only_out_of_running_and_restoring_lifecycles, suspended_report_projects_only_out_of_running_and_restoring_lifecycles}.cargo fmt --all -- --check,scripts/check-rust-error-policy.shandcargo clippy --workspace --locked --all-targets --all-features -- -D warningspass.cargo archreports no findings, also with--base-baseline,--renamesand--base-architecturefromorigin/main.SPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --all-featureson sandbox-store, sandbox-portability, cluster-controller-server, internal-api-server, api, snapshot-store and snapshot-retention: 4025 passed against a real Postgres, 0 dependency skips. The one skipped test is the ignored opt-in benchmark.Kind: a fresh cluster from an earlier revision with the same patch (
infra/space/kind/scripts/up.sh, otel telemetry) came up in 514 s.scripts/tests/smoke-kind-typed.sh all, with CI's admin-token and cold-receipt setup, took 4031 s. The native surface passed 42/42. The ECW surface passed 90/91:ecw.guest_mptcp_segments_droppedhit an infrastructure error at sandbox create (guest command readiness timed out, node-mgr probe Unavailable) and passed when rerun once. Six console-UI scenarios were skipped because Playwright/Chromium isn't installed.At this head, rebased onto
mainwith the patch unchanged (sorted+/-lines identical): fmt,ci/scripts/run-archcheck.sh(no findings) and workspace clippy-D warningspass.SPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --all-featureson cluster-controller-server, internal-api-server, sandbox-portability and sandbox-store: 1750 passed, 0 skipped.ci/scripts/check-test-durations.pypasses, and no test this PR touches takes over 2 s.Severity and risk
Severity: does not apply; this is a structural cleanup with no behaviour change.
Risk: low. The SQL text is unchanged, so the only risk is in the Rust control flow around it, mainly
boolreplacingOption<SuspendedK8sFinalizationSql>in the pod-less give-up. Roll back by reverting the commit.Mixed versions: there is no schema or wire change. Old and new binaries send the same statements, apart from
sql!locator comments.Not covered
The orphan statements (
orphan::recover_orphan_to_suspendedandorphan::quarantine_orphan) still interpolate the classification guard'sFROMitems (joined to the active execution for the pod-name guard), its predicate and itsFOR UPDATE OFtargets, which the reconciler's plan read composes too.quarantine_orphanalso picks its artifact predicate per call (TRUEfor a legacydeletingrow, otherwise the negated restart eligibility). Because the guard varies per call, both statements are still built withformat!on every call. Writing them as one whole statement per guard is a separate change. The repair row-state guards stay interpolated as the existing fragmentsdocs/rust/space-conventions.mddescribes.AI assistance: Claude Code drafted the code changes, the byte-identity harness and this description.