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
api: admit the remaining lifecycle requests, and move node registration and the admin marker reads into their stores - #1673
Three behaviour-free consolidations, one commit each:
api: admit the remaining user lifecycle requests.admit_user_lifecycle now also runs for creates (native, admin, ECW), ECW fork (for the source), data-plane access wakes (connect_spaced_runtime_client: requests through the API, native file access grants, ingress to a suspended or restoring sandbox), the open-port wake, and the Update fallback wake. New request kinds: Create, Fork, ControlWake(verb); data-plane and open-port wakes reuse Action(ImplicitWake(activity)). On the admin route, and on the native route when it does not try a warm-pool claim or its claim attempt fails, a replay, a reused key or a create the quota check refuses is admitted for an ID it then discards; an ECW create that adopts a warm-pool slot answers with the slot's ID, not the one it was admitted for. On Kubernetes, a connection that names no activity, and control_runtime_client's first attempt, can still wake a sandbox without being admitted. The function still admits everything; a test-only recorder lets one test per call family assert the request and sandbox.
node-registration-store: register Kubernetes nodes in one store function.upsert_k8s_node_role's statements move whole, in order, into pplx_space_node_registration_store::register_k8s_node, on the caller's connection and transaction. The api adapter keeps its signature, maps each error to the same SubstrateError text and disposition, and emits the UID-change warning from a callback at the same point. Comments citing the removed bootstrap path or an internal-api registration writer are corrected.
sandbox-store: move the admin console's reads of the drain restore marker into the store. New admin_occupancy (node, fleet and node-group occupancy; every shape a whole statement, interpolating only residency and durability predicates) and runtime_population (the runtime environment population statement, unchanged). Callers keep executors, transactions and error mapping. In the unranked shapes the marker term sits under false AND and is unreachable, so no test can observe it.
Validation
At this head: fmt, clippy, cargo arch, the touched crates' tests, and the fleet statements' MATERIALIZED check failing when that choice is flipped. The full CI inventory, the statement logs, the other mutations and the Kind runs ran before the rebase onto main, which changed no code in these commits.
fmt, run-rust-checks.sh clippy and cargo arch (0 required findings) pass.
api, sandbox-store and node-registration-store at CI's test profile with ignored tests: 2630 passed, 0 failed, 0 skipped. Each earlier commit passes the same suites (2629).
Full CI inventory: every failure is in the spaced guest, node-mgr, build-mgr, credmgr and runtime-supervisor crates (process, namespace and fs-verity limits of the local container). None is in a crate this change touches.
Statement logs (log_statement=all) of the touched suites before and after commits 2 and 3: the same statements with the same binds in the same order, apart from sql! locators. Commit 2's comparison ran the node registration characterization tests unchanged.
Mutations: removing each new admission call fails its test; flipping the drain-clear, tombstone re-check and insert-only predicates fails registration tests; flipping the marker predicate fails a test for the group, ranked node, ranked fleet (both residency modes) and population statements.
Local Kind before the rebase onto main: native_contract.file_op_wakes_idle_suspended_sandbox, ecw.fork_idempotency, local.forced_drain_evacuates_sandbox (twice) and ecw.public_port_to_loopback pass. The Postgres statement log shows the moved registration statements on node sync and during the drain, and each moved occupancy and population statement from the admin node list, a node's page, :assign-org and the population route.
Severity and risk
Severity: does not apply (no behaviour change). Risk: a mismatch in the moved SQL or in the error mapping, guarded by the statement-log comparison and the existing tests. Roll back by reverting; the squash merge lands the three changes as one commit, so a revert takes all three.
Not covered
No Kind scenario reaches the ingress wake of a suspended sandbox, the open-port wake or the Update fallback wake; Postgres tests cover them. The native create ran on Kind inside native_contract.file_op_wakes_idle_suspended_sandbox; local.console_operator_suspends_another_orgs_sandbox reaches the admin create, and this change did not run it. Kind runs census-aware residency, so the ranked fleet statement's lifecycle-only shape ran only in the Postgres tests.
The moved occupancy statements still interpolate the residency and durability predicates, as the drain gate's own statements do; this PR moves them 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 change adds a shared PostgreSQL operation for Kubernetes node registration and moves sandbox occupancy and population queries into the sandbox store. Admin routes now call those shared queries. The API also records lifecycle admission for additional sandbox creates, forks, and wake paths. New tests check occupancy behavior and recorded lifecycle requests.
Possibly related PRs
perplexityai/SPACE#1676: Changes the registration interface and controller path introduced or refactored here.
perplexityai/SPACE#1574: Adds the residency crate used by the extracted sandbox occupancy queries.
The change consolidates registration and reporting and adds admission calls. No merge-blocking issue is established.
Security architecture risk: Low · up to cdf9a
The inspected changes preserve existing authorization checks and registration safeguards. Lifecycle admission remains non-enforcing, so its documented identity and wake-coverage limitations do not establish a new access bypass. Remaining uncertainty concerns incomplete coverage rather than a demonstrated security regression.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The inspected user-facing changes remain within existing organization and sandbox scopes. Registration can affect shared Kubernetes inventory and drain state, but remains an internal synchronization path; fleet reporting remains behind managing-admin checks. No newly reachable privileged outcome was identified in these paths.
Trust Boundaries and Controls
observed — Port-session entrypoints retain sandbox-owner checks. Ingress retains lifecycle, auto-wake policy, access, and drain checks before runtime activation. The added admission calls do not replace those controls or grant runtime access.
Resilience and Maintainability Implications
observed — The registration adapter preserves the existing substrate-error mapping and UID-change warning timing. Together with caller-owned rollback, this retains the inspected failure-containment behavior rather than moving recovery responsibility into a separately committing store operation.
Hardening Proposals
proposed — Before lifecycle admission becomes enforcing or supplies durable security audit records, define how final warm-pool identities and inline Kubernetes recovery enter the same admission contract. This is a future prerequisite, not an observed security regression in the current recording-only implementation.
Pre-merge checks: 6
Check name
Status
Explanation
Title check
The title names two of the three changes (lifecycle admission and node registration/admin marker reads), which the raw summary and objectives support. It follows the required format: lowercase area pr…
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. The added lifecycle admission calls still admit every request, and the other changes move existing registration and occupancy logic into stores.
Weakened Safeguards Are Justified
The description explains both architecture allowlist additions: the API delegates node registration to the store, and the sandbox store now runs occupancy queries with residency predicates. The existi…
Agent Tooling Runs No New Commands
The diff changes Rust code and project dependency configuration. It does not change agent plugin manifests, skills, guidelines, or files referenced by the plugin manifests.
Public-Safe Content
The title, description, and diff contain no identified content that meets the failure conditions. The added network addresses and credential reference are test fixtures.
Full details: Title check
Explanation
The title names two of the three changes (lifecycle admission and node registration/admin marker reads), which the raw summary and objectives support. It follows the required format: lowercase area prefix with a colon, then a lowercase summary word. The title is long and uses a comma-joined list, but it still describes what the merge does. Proper nouns and code identifiers are not present, so no casing issue arises. No trailing period, ticket ID, or link appears.
Full details: Weakened Safeguards Are Justified
Explanation
The description explains both architecture allowlist additions: the API delegates node registration to the store, and the sandbox store now runs occupancy queries with residency predicates. The existing unused-variable expectation retains its reason. No test or assertion was removed or weakened.
Recent review info
Automatic approval policy
Commit: cdf9a017c3ae.
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; 4 remain after this review.
admit_user_lifecycle runs where a route records a lifecycle operation,
on entry to SandboxController::action, and before the operation
executor runs a delete or a rollback. Creates on every surface, ECW
fork, and the wakes that reach a sandbox's runtime without action did
not pass it: data-plane connections through
connect_spaced_runtime_client (requests through the API, native file
access grants, and ingress to a suspended or restoring sandbox), the
open-port wake, and the Update fallback wake in control_runtime_client.
A rule that must hold for every user lifecycle request would miss them.
Call it once at each. connect_spaced_runtime_client, for a request that
may wake, and the open-port path admit SandboxAction::ImplicitWake of
the request's activity, the request that ingress to a paused sandbox and
the queued implicit wake already admit. The wake path already says what
the user asked, so no new kind is needed, and the one call in
connect_spaced_runtime_client covers every connection whose request
names an activity, mid-stream reconnects included, with no new
parameter. On Kubernetes, the substrate's runtime_client, which a
connection that names no activity (wait_ready) and
control_runtime_client's first attempt both call, can still wake a
sandbox whose suspend completed during the call, or recreate the pod of
a non-dormant sandbox that lost it; neither is admitted. The fallback
wake admits the new ControlWake of the caller's verb, Update.
Creates admit the new Create: in the native route's input builder, which
every native create it records passes through, so a create that claims a
warm-pool slot is admitted for the slot's reserved ID; before the admin
route's store call; and in SandboxController::create, which the ECW
create calls, for the ID it draws. The admin route's store call, and the
native route's when it does not try a warm-pool claim or its claim
attempt fails, also resolve replays and key conflicts, and on the native
route the live-sandbox quota, so a replay, a reused key or a create the
quota check refuses is admitted for an ID it then discards. A native
create that tries a claim resolves those first. An ECW create that
adopts a warm-pool slot answers with the slot's reserved ID, not the one
it was admitted for. ECW fork admits the new Fork for its source after
both replay checks, before the fork is recorded. The cancelled-wake
redrive finishes a wake that action already admitted and does not call
it. The function's doc lists every caller family.
This changes no behaviour: the function still admits every request and
nothing reads its answer, and no call adds an await, an error or a
reordering. In tests it records each request it is asked to admit; one
test per new call family asserts the request and the sandbox it was
admitted for, with a case that admits nothing where the path does not
wake.
upsert_k8s_node_role in the api's Kubernetes store is the only writer
that registers a Kubernetes node: a tombstone check, the nodes upsert
and its ownership re-check, the drain-state clear when the Node's UID
changed, the node_k8s upsert and the tombstone re-check. Its statements
lived in the api, so a change to what registration records would be a
change to the api instead of to the store that owns node registration.
Move them, whole and in the same order, into
pplx_space_node_registration_store::register_k8s_node. It runs on the
caller's connection, inside the caller's transaction, and returns
whether any write changed a row. Its error names the statement that
failed, or the refusal: tombstoned before or during registration, or a
node id that belongs to a non-Kubernetes node.
upsert_k8s_node_role keeps its signature as the api's adapter, so node
sync, the admin listing's insert-only path and pod placement call it
unchanged. It maps each error to the SubstrateError text and
lock-contention disposition it returned before, and the store runs a
callback where the UID-change warning was emitted, so the warning keeps
its target, level, text, fields and timing. NodeStateWrite and
NodeLifecycleState stay in the api, where its callers and other readers
use them. The api becomes a user of the store in ARCHITECTURE.toml, and
its dependency moves out of dev-dependencies. Comments that cited a
bootstrap path that no longer exists, or a registration writer in
internal-api, now give the current reason: two in the moved code, one in
the cluster controller's node drain store, one in the node sync tests
and one in the Kubernetes store's Postgres tests. A cluster controller
comment that pointed at the drain reset now points at register_k8s_node,
and the store's crate doc no longer mentions other node_k8s writers.
This changes no behaviour. The node registration characterization
tests pass unchanged, and a statement log of their run records every
moved statement with the same text apart from its locator, the same
binds, in the same order.
…rker into the store
The admin node list, a node's page and the node-group assignment guard
count what occupies a node or a group, and the runtime environment page
counts which environment each sandbox runs or would restore with. Each
reads sandboxes.evacuation_restore_target, the marker a drain leaves on
a sandbox it moved, and the api spelled each of those reads out in its
own SQL. The node and fleet occupancy statements were also assembled at
each call from local fragments (the ranking switch, a MATERIALIZED
keyword, a COUNT form and a filter), so none of their shapes could be
read whole.
Add pplx_space_sandbox_store::admin_occupancy with node_occupancy,
fleet_occupancy and group_occupancy. Every shape is a whole statement:
a node's and the fleet's without the drainability counters and with
them under each ResidencyEvidence, and the group's. Each is built once
and interpolates only the shared residency and durability predicates.
Move the population statement, unchanged, into
pplx_space_sandbox_store::runtime_population::sandbox_population.
pplx-space-sandbox-residency owns predicates and no statement, so none
of these belongs there.
The callers keep their executors, transactions and error handling;
group_occupancy_in_tx passes its transaction. OccupancyRow and
SandboxPopulationRow stay in the api, which other code and tests use,
converted from the store's rows of the same fields. The helpers that
only the batched oracle uses become test-only, and the old statement
builders become test-only accessors to the store's statements for the
tests that inspect their plans. The sandbox store becomes a user of the
residency crate in ARCHITECTURE.toml. With this, no admin-console
statement in the api names the marker itself; those that interpolate the
shared drain gate or durability predicates, such as the :terminate
guard, still read it through them.
A new store test pins the ranked fleet statement's marker term under
both residency modes, which no test reached: a restore the drain
marked does not hold its target node's drain open, and an unmarked one
does.
This changes no behaviour. A statement log of the admin and
sandbox-store suites on the base and on this change records the same
statements with the same binds, in the same order, apart from sql!
locators.
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
Three behaviour-free consolidations, one commit each:
admit_user_lifecyclenow also runs for creates (native, admin, ECW), ECW fork (for the source), data-plane access wakes (connect_spaced_runtime_client: requests through the API, native file access grants, ingress to a suspended or restoring sandbox), the open-port wake, and the Update fallback wake. New request kinds:Create,Fork,ControlWake(verb); data-plane and open-port wakes reuseAction(ImplicitWake(activity)). On the admin route, and on the native route when it does not try a warm-pool claim or its claim attempt fails, a replay, a reused key or a create the quota check refuses is admitted for an ID it then discards; an ECW create that adopts a warm-pool slot answers with the slot's ID, not the one it was admitted for. On Kubernetes, a connection that names no activity, and control_runtime_client's first attempt, can still wake a sandbox without being admitted. The function still admits everything; a test-only recorder lets one test per call family assert the request and sandbox.upsert_k8s_node_role's statements move whole, in order, intopplx_space_node_registration_store::register_k8s_node, on the caller's connection and transaction. The api adapter keeps its signature, maps each error to the sameSubstrateErrortext and disposition, and emits the UID-change warning from a callback at the same point. Comments citing the removed bootstrap path or an internal-api registration writer are corrected.admin_occupancy(node, fleet and node-group occupancy; every shape a whole statement, interpolating only residency and durability predicates) andruntime_population(the runtime environment population statement, unchanged). Callers keep executors, transactions and error mapping. In the unranked shapes the marker term sits underfalse ANDand is unreachable, so no test can observe it.Validation
At this head: fmt, clippy,
cargo arch, the touched crates' tests, and the fleet statements'MATERIALIZEDcheck failing when that choice is flipped. The full CI inventory, the statement logs, the other mutations and the Kind runs ran before the rebase onto main, which changed no code in these commits.run-rust-checks.sh clippyandcargo arch(0 required findings) pass.log_statement=all) of the touched suites before and after commits 2 and 3: the same statements with the same binds in the same order, apart fromsql!locators. Commit 2's comparison ran the node registration characterization tests unchanged.native_contract.file_op_wakes_idle_suspended_sandbox,ecw.fork_idempotency,local.forced_drain_evacuates_sandbox(twice) andecw.public_port_to_loopbackpass. The Postgres statement log shows the moved registration statements on node sync and during the drain, and each moved occupancy and population statement from the admin node list, a node's page,:assign-organd the population route.Severity and risk
Severity: does not apply (no behaviour change). Risk: a mismatch in the moved SQL or in the error mapping, guarded by the statement-log comparison and the existing tests. Roll back by reverting; the squash merge lands the three changes as one commit, so a revert takes all three.
Not covered
No Kind scenario reaches the ingress wake of a suspended sandbox, the open-port wake or the Update fallback wake; Postgres tests cover them. The native create ran on Kind inside
native_contract.file_op_wakes_idle_suspended_sandbox;local.console_operator_suspends_another_orgs_sandboxreaches the admin create, and this change did not run it. Kind runs census-aware residency, so the ranked fleet statement's lifecycle-only shape ran only in the Postgres tests.The moved occupancy statements still interpolate the residency and durability predicates, as the drain gate's own statements do; this PR moves them unchanged.
AI assistance: Claude Code made the change, ran the validation and drafted this description. A human reviews before merge.