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
cluster-controller: move the transient sweeper's candidate read into sandbox-store - #1666
The transient lifecycle sweeper built its aged-transient candidate select inline, although pplx-space-sandbox-store owns the sandboxes row. It also parsed the node drain's evacuation_restore_target marker by hand as a SandboxLifecycle, so no type said which values the marker holds.
Merging this:
moves the select, as the same statement builder, into sandbox-store's sweeper module as list_aged_transient. It runs on the caller's executor and returns the same columns;
adds EvacuationRestoreTarget there for the marker's stored values (started, paused, stopped), parsed exactly as a SandboxLifecycle is, without trimming or case folding, and has the read return it parsed. A value that doesn't parse fails neither the row nor the select;
drops the cluster-controller's pplx-space-sandbox-routability dependency and its ARCHITECTURE.toml user entry: the moved select was the controller's only use of it;
leaves every decision with the sweeper: it logs the same warning, under the same target, and treats the row as unset, and probe and classify still take the SandboxLifecycle view.
Behaviour-free: apart from its sql! locator, the statement text and its binds are unchanged, and every value the column's CHECK admits maps to the lifecycle it parsed to before. A lifecycle outside that set would now warn instead of being carried, but the validated CHECK has never admitted one.
Validation
On a Linux build host. The branch was rebased onto current main after most runs below. Main's only overlap was an import: the controller's routability predicate, which this change deletes, and the stored lifecycle's new home in sandbox-store, which a sandbox-store test imports. At the rebased head, fmt, clippy, cargo arch and the sandbox-store and cluster-controller tests (1276 passed) were rerun.
Statement identity: the sandbox-store suite and the sweeper's tests, run against a Postgres logging every statement, issue the same 221 distinct statements and the same 5536 executions at the base and here, once locators, generated ids and database names are normalized. The comparison covers the tests both commits have, so it leaves out the two new tests. The moved statement runs 123 times on both sides, with identical binds.
New tests pin the parse against the column's CHECK (every stored lifecycle and other spellings) and the unparseable-marker path, including the marker's projection, which no existing test covered.
Mutation check: changing a parse arm, dropping the marker from the select's projection, mapping an unparseable value to a lifecycle, trimming before the parse, or failing the row on an unparseable value each fails a test.
fmt and workspace clippy through ci/scripts/run-rust-checks.sh, and cargo arch: pass, no required findings.
sandbox-store and cluster-controller tests at CI's test profile (opt-level 0), with Postgres running: 1267 passed, 0 failed, 0 skipped.
The full test inventory, both groups: every touched crate passed. Tests in untouched crates that need operations the host's container or filesystem doesn't support failed there, as they do on the base commit.
Kind, before the rebase, with statement logging on the stack's Postgres: local.forced_drain_evacuates_sandbox, native.forced_drain_re_places_stopped_sandbox and local.leader_election_brownout_keeps_one_leader pass, and the deployed controller ran the moved select (under its new pplx_space_sandbox_store::sweeper locator) 24 to 36 times in each. The unparseable-marker warning appeared nowhere.
Severity and risk
Severity: does not apply; a move.
Risk: if a stored marker held a lifecycle other than started, paused or stopped, the sweeper would now treat it as unset instead of carrying it; the column's validated CHECK rules that out. Even then no sweep decision would change: the classifier reads the marker only for a restoring candidate, and the select never lists a marked restoring row. Roll back by reverting this commit.
Not covered
On Kind no scenario wedges a transient row long enough to age out, and none has the restoring select tick while a drain-marked row is restoring, so the age-out and the drain-marked exemption ran only in the Postgres tests. The same holds for reading a non-NULL marker into a candidate and the unparseable path.
The node drain still parses the marker itself, as a SandboxLifecycle checked against EVACUABLE_LIFECYCLES, and its module docs still describe reusing SandboxLifecycle instead of a closed marker type. This PR leaves the drain as it is.
The moved statement is still assembled from fragments, as its sibling sweeper statements in sandbox-store are: the store's guard builders and the routable-execution predicate of pplx-space-sandbox-routability, neither of which the data-store rule on whole statements exempts. This PR moves it unchanged.
AI assistance: Claude Code made the change, ran the validation and drafted this description. A human reviews before merge.
No actionable comments were generated in the recent review.
Walkthrough
The aged-transient candidate query and row parsing now reside in sandbox-store. The cluster controller calls the shared listing API instead of constructing and executing its own query. Tests cover evacuation-target parsing against the database constraint and handling absent, valid, and unparseable markers.
Possibly related PRs
perplexityai/SPACE#1645: Moves StoredSandboxLifecycle into sandbox-store, which this PR uses in tests of evacuation-target parsing.
Merge Risk: Low · up to ced7d
This moves the candidate query into a shared crate and is reported to produce identical SQL and results. Tests cover the new marker parsing. Residual risk is limited to the moved SQL staying equivalent in production.
Security architecture risk: Minimal · up to ced7d
The shared API preserves the existing read-only query, database scope, and lifecycle safeguards. Its narrower marker type accepts every value allowed by the existing database constraint. 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 production listing retains backend-wide selection within the caller's database pool, managed_by substrate label, and requested lifecycle; managed_by is not a tenant authorization boundary. Neither the pool nor this scope expands in the inspected caller.
Trust Boundaries and Controls
inferred — The new Rust-public API broadens reusable code access, but the traced production consumer remains the controller adapter. Caller-supplied executors and scope labels remain trusted application inputs; the inspected path does not introduce a request-facing route or grant additional database authority.
Resilience and Maintainability Implications
observed — The read move leaves transition authority in the existing action paths. Lifecycle and ownership rechecks, sandbox and execution locking, pod-identity fences, rollback on failed retirement, and write-token-bound compensation continue to contain stale observations and concurrent replacement.
Pre-merge checks: 6
Check name
Status
Explanation
Title check
The title identifies the affected area and summarizes the main change. It meets the specified format.
Description Follows The Pr Template
The description includes all four required items: a summary, validation and results, severity and risk, and an AI assistance line.
Bug Fixes Name A Regression Test
This is not an application bug fix. The diff moves the candidate query and adds a typed marker parser. The controller already treated unparseable markers as unset.
Weakened Safeguards Are Justified
The diff does not weaken an existing test, check, or repository guard. It removes an unused architecture allowlist entry. The new test drops a database constraint in its isolated test database, with a…
Agent Tooling Runs No New Commands
The diff changes Rust source and tests, plus dependency declarations. It does not change agent tooling manifests, skills, guideline files, or executable agent configuration.
Public-Safe Content
The diff, title, and description contain no identified public-safety violations. The reported measurements describe test runs, not production.
Full details: Weakened Safeguards Are Justified
Explanation
The diff does not weaken an existing test, check, or repository guard. It removes an unused architecture allowlist entry. The new test drops a database constraint in its isolated test database, with an adjacent explanation.
Recent review info
Automatic approval policy
Commit: ced7d1c8408a.
Setting
Effective value
Source
Automatic approvals
Enabled
Global configuration
Merge risk
Moderate
Global configuration
Security risk
Not set
Global configuration
No overrides matched. The global limits apply.
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; 2 remain after this review.
…sandbox-store
The transient lifecycle sweeper still built its aged-transient candidate
select inline, although pplx-space-sandbox-store owns the sandboxes row.
It also parsed the node drain's evacuation_restore_target marker by hand
as a SandboxLifecycle, so no type said which values the marker holds.
Move the select, as the same statement builder, into sandbox-store's
sweeper module as list_aged_transient. It runs on the caller's executor
and returns the same columns. Add EvacuationRestoreTarget there for the
marker's stored values (started, paused, stopped), parsed exactly as a
SandboxLifecycle is, without trimming or case folding, and have the read
return it parsed. A value that doesn't parse fails neither the row nor
the select. The sweeper keeps every decision: it logs the same warning,
under the same target, and treats the row as unset, and probe and
classify still take the SandboxLifecycle view.
The moved select was the cluster controller's only use of
pplx-space-sandbox-routability, so the controller drops that dependency
and its user entry in ARCHITECTURE.toml.
This is behaviour-free. Apart from its sql! locator, the statement text
and its binds are unchanged, and every value the column's CHECK admits
maps to the lifecycle it parsed to before. A lifecycle outside that set
would now warn instead of being carried, but the validated CHECK has
never admitted one. New tests pin the parse against the CHECK and the
unparseable-marker path, including the marker's projection, which no
existing test covered.
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
The transient lifecycle sweeper built its aged-transient candidate select inline, although
pplx-space-sandbox-storeowns thesandboxesrow. It also parsed the node drain'sevacuation_restore_targetmarker by hand as aSandboxLifecycle, so no type said which values the marker holds.Merging this:
sweepermodule aslist_aged_transient. It runs on the caller's executor and returns the same columns;EvacuationRestoreTargetthere for the marker's stored values (started,paused,stopped), parsed exactly as aSandboxLifecycleis, without trimming or case folding, and has the read return it parsed. A value that doesn't parse fails neither the row nor the select;pplx-space-sandbox-routabilitydependency and itsARCHITECTURE.tomluser entry: the moved select was the controller's only use of it;SandboxLifecycleview.Behaviour-free: apart from its
sql!locator, the statement text and its binds are unchanged, and every value the column's CHECK admits maps to the lifecycle it parsed to before. A lifecycle outside that set would now warn instead of being carried, but the validated CHECK has never admitted one.Validation
On a Linux build host. The branch was rebased onto current main after most runs below. Main's only overlap was an import: the controller's routability predicate, which this change deletes, and the stored lifecycle's new home in sandbox-store, which a sandbox-store test imports. At the rebased head, fmt, clippy,
cargo archand the sandbox-store and cluster-controller tests (1276 passed) were rerun.ci/scripts/run-rust-checks.sh, andcargo arch: pass, no required findings.local.forced_drain_evacuates_sandbox,native.forced_drain_re_places_stopped_sandboxandlocal.leader_election_brownout_keeps_one_leaderpass, and the deployed controller ran the moved select (under its newpplx_space_sandbox_store::sweeperlocator) 24 to 36 times in each. The unparseable-marker warning appeared nowhere.Severity and risk
Severity: does not apply; a move.
Risk: if a stored marker held a lifecycle other than
started,pausedorstopped, the sweeper would now treat it as unset instead of carrying it; the column's validated CHECK rules that out. Even then no sweep decision would change: the classifier reads the marker only for arestoringcandidate, and the select never lists a markedrestoringrow. Roll back by reverting this commit.Not covered
On Kind no scenario wedges a transient row long enough to age out, and none has the restoring select tick while a drain-marked row is
restoring, so the age-out and the drain-marked exemption ran only in the Postgres tests. The same holds for reading a non-NULL marker into a candidate and the unparseable path.The node drain still parses the marker itself, as a
SandboxLifecyclechecked againstEVACUABLE_LIFECYCLES, and its module docs still describe reusingSandboxLifecycleinstead of a closed marker type. This PR leaves the drain as it is.The moved statement is still assembled from fragments, as its sibling sweeper statements in sandbox-store are: the store's guard builders and the routable-execution predicate of
pplx-space-sandbox-routability, neither of which the data-store rule on whole statements exempts. This PR moves it unchanged.AI assistance: Claude Code made the change, ran the validation and drafted this description. A human reviews before merge.