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
layers: move common-svc's SQL into postgres-pool and sandbox-residency - #1574
Second of three changes that delete pplx-space-common-svc.
Its sql module (the managed-identity Postgres pools) moves into a new l30 store, pplx-space-postgres-pool
(crates/l30_storage/postgres/pool), which keeps common-svc's sqlx features. Its own contention module (is_lock_contention, is_lock_not_available, is_query_canceled, each taking &sqlx::Error) classifies errors from its connections by calling the l10 helper pplx-space-postgres-contention, so callers call the store rather than the helper.
The sandbox residency fragments move verbatim into a new l30 store, pplx-space-sandbox-residency
(crates/l30_storage/sandbox/residency). Both stores declare postgres = [].
Users of the stores common-svc re-exported depend on them directly. common-svc drops sql, its migration-checksums feature, sqlx and common-core/sqlx. db-migrate declares the sqlx uuid feature it used to
get through common-svc.
ARCHITECTURE.toml store users: postgres-pool lists api, cluster-controller-server, credmgr and internal-api-server;
sandbox-residency lists api and cluster-controller-server (internal-api-server uses it only in tests, through a
dev-dependency); api joins sandbox-quarantine-store's users (its debug route reads it); api and internal-api-server
leave sandbox-routability's, which they no longer use.
CI's isolated-crates job lints postgres-pool alone (it joins ALSO in ci/scripts/lib/rust-isolated-crates.sh).
The space-data-store-development skill lists sandbox-residency's fragments as a named exception: the single
definition of node residency for node-release decisions.
Baseline: common-svc's codec_features entry (common-core/sqlx), four direction edges (to the stores it
re-exported) and its storage_access entries for sqlx and postgres-contention are removed. Nothing is added.
Changed text: SQL statement bodies, metric names and wire formats are unchanged. sql! locator comments change in the
moved pool (/*pplx_space_common_svc::sql:208*/ and :283 become /*pplx_space_postgres_pool:207*/ and :282) and
where imports shift lines: internal-api published_state.rs (27 statements, −2), federated-grant-store lib.rs
(7, −4), cluster-controller drained_node_terminator/store/resident_census_gate_tests.rs (6, −2) and node_drain/store/residency_tests.rs (5, −2), node-registration-store lib.rs (1, −1). Two warning events change
target from pplx_space_common_svc::sql to pplx_space_postgres_pool.
Overlap: open #1553 edits sandbox_residency.rs and its tests at the old path. Whichever lands second carries that
edit into crates/l30_storage/sandbox/residency.
Validation
Workspace check with default and all features; every dependent checked alone (--all-targets and --lib).
The CI entrypoints on this head against current main: clippy and isolated-crates pass; docs and Python tests pass;
Rust tests fail only 40 tests that need process permissions the validation host lacks and that fail the same way on
main.
cargo arch: no required findings; the baseline only shrinks.
Severity and risk
Severity: does not apply (structural). Risk: a crate built alone missing a feature it used to get through common-svc;
the per-crate and isolated-crates builds cover that. sql! locator lines shift in the files listed above. Roll back by reverting.
Automatic approval is disabled by the configured approval policy for this pull request. Review comments and requests for changes continue normally.
Commit: c564a253ed3e.
No actionable comments were generated in the recent review.
Walkthrough
The change adds dedicated PostgreSQL pool and sandbox residency crates. API services and controllers switch pool, contention, residency, and store imports to dedicated crates. The migration runner loads accepted checksums from a direct dependency. The common service removes related dependencies and its local SQL module. Workspace records, CI scripts, and documentation reflect the updated crate boundaries. The plugin version changes from 0.2.47 to 0.2.48.
Possibly related PRs
perplexityai/SPACE#1515: Moves PostgreSQL pool metrics into the common-service SQL namespace, while this change moves pool implementation ownership out of that namespace.
Merge Risk: Minimal · up to c564a
The storage crates and their consumers appear to preserve existing behavior. No actionable merge-blocking issue remains after normal checks.
Security architecture risk: Low · up to c564a
The inspected changes preserve managed database identities, credential separation, authorization predicates and transaction behavior. No new access path or privilege increase was identified. The remaining risk is coordinating shared dependency changes across several services.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The security-sensitive scope is the existing control-plane database access shared by API, controller, internal-API and credential-manager processes. The credential lane includes user and organization credentials and master-key records. These authorities predate the extraction; the checked consumers retain their previous identity selection, without an identified increase in tenant or database reach.
Trust Boundaries and Controls
observed — Changed API-key, console and session-authentication error paths retain their existing denial or error outcomes; only the contention-classifier provider changes. That provider delegates to the previous helper, so the inspected changes do not turn a database lookup failure into successful authentication.
Resilience and Maintainability Implications
observed — The residency policy retains lifecycle-only behavior as its default, including for unrecognized configuration values. Its shared occupancy predicate remains the policy branch used for node-release decisions, and the checked terminator consumer still imports that common definition rather than introducing a separate release policy.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title meets the format requirements and describes the main change.
Description Follows The Pr Template
The description contains all four required items: Summary, Validation, Severity and risk, and AI assistance. Each has content.
Bug Fixes Name A Regression Test
The PR is a storage-layer refactor, not an application bug fix. The diff moves residency code unchanged and redirects pool and contention calls to the same implementations.
Weakened Safeguards Are Justified
The description explains that common-svc removes the migration-checksums feature, so its isolated Clippy check no longer applies. It also explains the new store-user entries. The moved tests retain …
Public-Safe Content
The reviewed diff, title, and description contain no identified content prohibited by this check. Credential references are code identifiers, not credential values.
Full details: Weakened Safeguards Are Justified
Explanation
The description explains that common-svc removes the migration-checksums feature, so its isolated Clippy check no longer applies. It also explains the new store-user entries. The moved tests retain their assertions and test module registrations. The SQL-fragment exception has an adjacent reason.
Recent review info
Automatic approval policy
Commit: c564a253ed3e.
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; 3 remain after this review.
Moves common-svc's `sql` module into two l30_storage crates, as part of
deleting common-svc:
- pplx-space-postgres-pool (crates/l30_storage/postgres/pool): the
managed-identity pools (`sql/mod.rs`, `sql/identity.rs` and their
tests), with svc's sqlx features. It exposes pplx-space-postgres-
contention as its `contention` module, so callers that wrote
`common_svc::sql::contention::…` write
`pplx_space_postgres_pool::contention::…` and contention stays an
l10 storage helper.
- pplx-space-sandbox-residency (crates/l30_storage/sandbox/residency):
the residency SQL fragments (`sql/sandbox_residency.rs`), unchanged.
Both declare `postgres = []`. The code moves unchanged apart from
paths: the pool takes `sql!` from pplx-space-sql-locator,
`DatabaseIdentity` from pplx-space-db-identity and `ReconcileReason`
from pplx-space-state-reconciliation instead of through common-core
and common-svc.
The users of the stores common-svc re-exported (federated-grant-store,
node-registration-store, sandbox-quarantine-store,
sandbox-routability) and of migration-checksums depend on them
directly. common-svc drops `sql`, its `migration-checksums` feature
and its sqlx, rand and store dependencies, and no longer enables
common-core's `sqlx` feature. db-migrate declares the sqlx `uuid`
feature it used to get through common-svc.
ARCHITECTURE.toml: the two new stores list the crates whose production
code uses them (internal-api-server reads sandbox-residency only in
tests, through a dev-dependency); api joins sandbox-quarantine-store's
users (its debug route reads quarantine rows); api and
internal-api-server leave sandbox-routability's, which neither uses. A
comment at the storage helpers records postgres-pool's `contention`
re-export as an edge archcheck does not see. The baseline loses common-svc's codec_features entry,
its direction edges to the four stores, and its storage_access
entries for sqlx and postgres-contention.
The `sql!` locators of the pool's two statements change from
`/*pplx_space_common_svc::sql:208*/` and `:283` to
`/*pplx_space_postgres_pool:207*/` and `:282`. The pool's two warning
events change target from `pplx_space_common_svc::sql` to
`pplx_space_postgres_pool`.
CI drops the common-svc migration-checksums clippy step; the
isolated-crates job lints pplx-space-migration-checksums alone, and
lints postgres-pool alone through its ALSO list. The data-store skill
lists sandbox-residency's fragments as an interpolation exception (the
plugin goes to 0.2.48), and the docs that named common-svc's SQL
re-exports follow.
postgres-pool re-exported pplx-space-postgres-contention as `contention`,
so api, credmgr and internal-api-server reached a storage helper through an
edge archcheck can't see. A `contention` module now defines the three
predicates those call sites use (is_lock_contention, is_lock_not_available,
is_query_canceled), each delegating to the helper. Call sites are unchanged.
Drops the ARCHITECTURE.toml comment and the archcheck README example that
described the re-export.
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.
shehan-pplx
left a comment
Summary
Second of three changes that delete
pplx-space-common-svc.sqlmodule (the managed-identity Postgres pools) moves into a new l30 store,pplx-space-postgres-pool(
crates/l30_storage/postgres/pool), which keeps common-svc's sqlx features. Its owncontentionmodule (is_lock_contention,is_lock_not_available,is_query_canceled, each taking&sqlx::Error) classifies errors from its connections by calling the l10 helperpplx-space-postgres-contention, so callers call the store rather than the helper.pplx-space-sandbox-residency(
crates/l30_storage/sandbox/residency). Both stores declarepostgres = [].sql, itsmigration-checksumsfeature, sqlx andcommon-core/sqlx. db-migrate declares the sqlxuuidfeature it used toget through common-svc.
ARCHITECTURE.tomlstore users: postgres-pool lists api, cluster-controller-server, credmgr and internal-api-server;sandbox-residency lists api and cluster-controller-server (internal-api-server uses it only in tests, through a
dev-dependency); api joins sandbox-quarantine-store's users (its debug route reads it); api and internal-api-server
leave sandbox-routability's, which they no longer use.
ALSOinci/scripts/lib/rust-isolated-crates.sh).definition of node residency for node-release decisions.
codec_featuresentry (common-core/sqlx), fourdirectionedges (to the stores itre-exported) and its
storage_accessentries for sqlx and postgres-contention are removed. Nothing is added.Changed text: SQL statement bodies, metric names and wire formats are unchanged.
sql!locator comments change in themoved pool (
/*pplx_space_common_svc::sql:208*/and:283become/*pplx_space_postgres_pool:207*/and:282) andwhere imports shift lines: internal-api
published_state.rs(27 statements, −2), federated-grant-storelib.rs(7, −4), cluster-controller
drained_node_terminator/store/resident_census_gate_tests.rs(6, −2) andnode_drain/store/residency_tests.rs(5, −2), node-registration-storelib.rs(1, −1). Two warning events changetarget from
pplx_space_common_svc::sqltopplx_space_postgres_pool.Overlap: open #1553 edits
sandbox_residency.rsand its tests at the old path. Whichever lands second carries thatedit into
crates/l30_storage/sandbox/residency.Validation
--all-targetsand--lib).Rust tests fail only 40 tests that need process permissions the validation host lacks and that fail the same way on
main.
cargo arch: no required findings; the baseline only shrinks.Severity and risk
Severity: does not apply (structural). Risk: a crate built alone missing a feature it used to get through common-svc;
the per-crate and isolated-crates builds cover that.
sql!locator lines shift in the files listed above. Roll back by reverting.Stack
AI assistance: Claude Code made the changes and drafted this description.