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
placement: delete the unused crates and move the stored lifecycle to sandbox-store - #1645
pplx-space-placement and pplx-space-placement-executor were written ahead of their callers, and no service calls them: the executor's place returns a typed refusal, and PostgresPlacementStore is reached only from its own tests and the executor. They still count as sandbox-row writers and readers of the evacuation restore marker, so every lifecycle or drain change has had to account for them.
Merging this:
deletes pplx-space-placement-executor and pplx-space-placement;
moves the one part in use, StoredSandboxLifecycle (the stored vocabulary of sandboxes.lifecycle) and its writer-test matrix, unchanged into pplx-space-sandbox-store, which owns that table. The four files move as pure renames. The matrix sits behind a new test-support feature;
switches the callers' imports and dev-dependencies to sandbox-store (internal-api in production; api, cluster-controller, internal-api and sandbox-store in tests). Every one of them already depended on sandbox-store;
removes the placement crate's sandboxes co-writer grant, its dependents entry and its baseline line from the architecture rules, and its rows from scripts/move_crates.toml;
updates the comments and docs that named the deleted code as current.
The design stays in history if a caller needs it later.
Validation
Rebased onto main after the removal of pplx-space-common-svc. The conflicts were mechanical: import paths, Cargo dependency lists, the architecture store_users entry, and keeping the deletion of a placement file main had edited. The four moved files are still pure renames.
fmt and workspace clippy through ci/scripts/run-rust-checks.sh: pass. cargo arch: no required findings; its three new advisories are the removed dependents entry, store user and co-writer grant.
The six touched crates' tests at CI's test profile, with Postgres and Redis running: 4771 run, 4771 passed.
The full test inventory, both groups: every touched crate passed. Tests in 8 untouched crates that need process, namespace or cgroup operations the host's container blocks failed there, and fail the same way on the base commit.
cargo tree -e features for the internal-api, api and cluster-controller service binaries: a release build doesn't enable sandbox-store's test-support.
Mutation check on the new wiring: dropping the re-export, dropping the dev-dependency feature, or gating the matrix on cfg(test) alone breaks the build; changing a stored spelling, dropping case normalization, skipping a value in the sweep, or ignoring the lifecycle in a published-status check fails 10 to 54 tests.
Kind smoke was not run: the change deletes uncalled code and moves a type unchanged.
Severity and risk
Severity: does not apply.
Risk: a build that names either deleted crate stops compiling until it drops the name; the moved type keeps its name and behaviour. Open PR #1553 edits three of the moved files, three files this deletes (the placement crate's README.md, src/postgres.rs and src/postgres/tests/completion.rs), and the SandboxLifecycle::all doc comment in common-core that this rewrites. Git follows the renames, so its edits to the moved files carry over. Whichever lands second keeps the deletions and names pplx-space-sandbox-store in that doc comment. Open PR #1575 changes one comment in the placement crate's src/model.rs, which this deletes. Roll back by reverting this commit.
AI assistance: Claude Code made the change, ran the validation and drafted this description. A human reviews before merge.
Automatic approval is disabled by the configured approval policy for this pull request. Review comments and requests for changes continue normally.
Commit: 7e9b6431b456.
No actionable comments were generated in the recent review.
Walkthrough
The placement storage crate and executor are removed from the workspace. The sandbox store now exports StoredSandboxLifecycle and test support. API, controller, server, and test imports are updated to use the sandbox store. Architecture rules, documentation, and crate records are also revised.
Merge Risk: Moderate · up to 7e9b6
Placement claim and completion tests are removed along with the unused implementation. Confirm that loss of coverage is acceptable before merging.
Security architecture risk: Minimal · up to 7e9b6
The lifecycle implementation and test helpers move unchanged to the package that owns sandbox storage. The removed executor was not wired into production, and the inspected production consumers change only their imports. No introduced or worsened security risk was identified.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
observed — The security-relevant change concerns interpretation of sandboxes.lifecycle and repository authorization for sandbox-row writers. The removed placement grant was a repository architecture-policy exception, not a database-role revocation.
Trust Boundaries and Controls
observed — The policy comparison removes only the deleted placement writer's co-writer grant and baseline entry. The sandbox-write matcher, sandbox-store allowance, and remaining API writer caps remain in place; the PR does not broaden those repository write exceptions.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title meets the format requirements and describes the removal of the placement crates and the lifecycle move.
Description Follows The Pr Template
The description includes all four required items: Summary, Validation, Severity and risk, and an AI assistance line.
Bug Fixes Name A Regression Test
This PR is not an application bug fix. It removes unused placement crates and moves the lifecycle type unchanged. The production call sites only change their imports.
Weakened Safeguards Are Justified
The description explains that the deleted tests covered placement code with no service callers. The lifecycle tests and matrix move unchanged. The removed architecture entries apply to the deleted cra…
Public-Safe Content
The diff, title, and description contain no identified sensitive content covered by the check. The addresses in deleted test fixtures are illustrative, and the database URLs use localhost.
Full details: Weakened Safeguards Are Justified
Explanation
The description explains that the deleted tests covered placement code with no service callers. The lifecycle tests and matrix move unchanged. The removed architecture entries apply to the deleted crate.
Recent review info
Automatic approval policy
Commit: 7e9b6431b456.
Setting
Effective value
Source
Automatic approvals
Disabled
Override 4
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; 2 remain after this review.
Verified against 1f3c9da3a9d833947e7fd80811c9af3ee753c279. Everything below is from reading this head, except the CI results.
Nothing left references the deleted crates.git grep for pplx[-_]space[-_]placement, placement[-_]executor, l30_storage/placement and l40_api/placement finds nothing anywhere in the tree: code, manifests, docs, skills, CI scripts or scripts/move_crates.toml.
The move is exact.git diff -M --summary shows lifecycle.rs, lifecycle/tests.rs, test_support.rs and test_support/tests.rs as 100% renames into pplx-space-sandbox-store. StoredSandboxLifecycle is re-exported from the crate root under the same name.
The test-only matrix stays out of release builds.test_support is gated #[cfg(any(test, feature = "test-support"))], and the new test-support feature is empty and off by default. internal-api, api and cluster-controller enable it only in [dev-dependencies]; their normal dependency on sandbox-store has no features.
The architecture rules lose exactly what was the placement crate's:
its store-users row (pplx-space-internal-api-server, pplx-space-placement-executor);
its temporary sandboxes co-writer grant;
its baseline line;
the include_str! example, which now reads generically.
internal-api's production use now goes through sandbox-store, which it already depended on.
CI on this head passes: Rust tests, clippy, format, isolated crates, dependency checks, and both Kind artifact builds.
Not done: I didn't run the suites or the mutation checks myself. I relied on CI and the description's validation for those.
…sandbox-store
The placement crate's claim and completion store, and the placement
executor built on it, were written ahead of their callers, and no
service calls them: the executor's `place` returns a typed refusal, and
`PostgresPlacementStore` is reached only from its own tests and the
executor. They still count as sandbox-row writers and readers of the
evacuation restore marker, so every lifecycle or drain change has to
account for them.
Delete `pplx-space-placement-executor` and `pplx-space-placement`. The
one part in use, `StoredSandboxLifecycle` (the stored vocabulary of
`sandboxes.lifecycle`) and its writer-test matrix, moves unchanged to
`pplx-space-sandbox-store`, which owns that table. The matrix sits
behind a new `test-support` feature there. Every caller already depends
on sandbox-store. The placement crate's `sandboxes` co-writer grant
goes too.
The design stays in history if a caller needs it later.
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.
the-gigi-pplx
left a comment
Summary
pplx-space-placementandpplx-space-placement-executorwere written ahead of their callers, and no service calls them: the executor'splacereturns a typed refusal, andPostgresPlacementStoreis reached only from its own tests and the executor. They still count as sandbox-row writers and readers of the evacuation restore marker, so every lifecycle or drain change has had to account for them.Merging this:
pplx-space-placement-executorandpplx-space-placement;StoredSandboxLifecycle(the stored vocabulary ofsandboxes.lifecycle) and its writer-test matrix, unchanged intopplx-space-sandbox-store, which owns that table. The four files move as pure renames. The matrix sits behind a newtest-supportfeature;sandboxesco-writer grant, its dependents entry and its baseline line from the architecture rules, and its rows fromscripts/move_crates.toml;The design stays in history if a caller needs it later.
Validation
Rebased onto main after the removal of
pplx-space-common-svc. The conflicts were mechanical: import paths, Cargo dependency lists, the architecturestore_usersentry, and keeping the deletion of a placement file main had edited. The four moved files are still pure renames.At the rebased head, on a Linux build host:
cargo check --workspace --all-targets, andcargo check -p pplx-space-api --features kubernetes --all-targets: pass.ci/scripts/run-rust-checks.sh: pass.cargo arch: no required findings; its three new advisories are the removed dependents entry, store user and co-writer grant.cargo tree -e featuresfor the internal-api, api and cluster-controller service binaries: a release build doesn't enable sandbox-store'stest-support.cfg(test)alone breaks the build; changing a stored spelling, dropping case normalization, skipping a value in the sweep, or ignoring the lifecycle in a published-status check fails 10 to 54 tests.Kind smoke was not run: the change deletes uncalled code and moves a type unchanged.
Severity and risk
Severity: does not apply.
Risk: a build that names either deleted crate stops compiling until it drops the name; the moved type keeps its name and behaviour. Open PR #1553 edits three of the moved files, three files this deletes (the placement crate's
README.md,src/postgres.rsandsrc/postgres/tests/completion.rs), and theSandboxLifecycle::alldoc comment in common-core that this rewrites. Git follows the renames, so its edits to the moved files carry over. Whichever lands second keeps the deletions and namespplx-space-sandbox-storein that doc comment. Open PR #1575 changes one comment in the placement crate'ssrc/model.rs, which this deletes. Roll back by reverting this commit.AI assistance: Claude Code made the change, ran the validation and drafted this description. A human reviews before merge.