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: answer process routes by a command's process id on guests that mint their own - #1585
createCommand returns process_id, the command's session id (proc_…), and the data plane sends it to the guest as StartRequest.process_id. Guests within the supported range (>=0.1.0, <0.9.0) that predate assigned ids ignore the field and mint their own id; the data plane then binds the session to the id the guest reported (data_plane_sessions.backend_process_id). On those guests, GET /v1/sandboxes/{id}/processes/{process_id}, :signal and :kill answered 404 for the id createCommand returned, and listProcesses showed only the guest's id. docs/versioning.md requires existing operations to keep working on supported guests.
The process routes now resolve a session-shaped id through that binding:
getProcess, signalProcess and killProcess look up the session the id names in this sandbox and, when the data plane bound it to a different guest id, address the guest by that id. getProcess answers under the id that was asked for.
listProcesses lists a process bound to a session under the session id, once, in place of the guest's id. The page cursor and order follow the listed ids.
Any other id, including a guest's own id and a session id the guest kept, goes to the guest unchanged, so current guests behave as before. ECW starts processes without an assigned id and binds no session, so its processes are unaffected.
The binding is read whether or not the session has expired or been revoked, since the process can outlive it. The data-plane session retention sweep keeps it for as long: it used to delete a session row an hour after the session expired (24 h after create) or ended, so a command running past that answered 404 again. The sweep now skips a row whose backend_process_id is set and differs from its session id, which only guests that mint their own ids produce. Deleting the sandbox removes the row (the foreign key cascades). Current guests bind the session id itself, so their rows are pruned as before. The lookups go through PublicSessionStore, which owns the API's reads of data_plane_sessions: a primary-key read for each session-shaped id on get, signal and kill, and one read per list page only when the guest lists ids that are not session ids.
Retried-start protection: on current guests a repeated start of an assigned id answers AlreadyExists and the data plane reconnects instead of running the command again. Guests that predate assigned ids never answer that. For them, once the binding commits, every later attach reconnects to the bound guest id, as before. Between the start and the binding's commit, only the guest's handling of the start's idempotency key (the session id, sent on every start) keeps an attach on another replica from starting the command again. This PR does not change that; it documents it at initial_process_stream and PendingProcessBinding.
Validation
At this head (rebased on main):
New in crates/l40_api/api/src/routes/public/processes/tests.rs, against Postgres and the simulated worker:
command_process_ids_reach_processes_of_guests_that_mint_their_own: a guest that mints its own ids. getProcess, :signal and :kill on the createCommand id reach the process (each then reports signaled with the signal sent), and the list shows each command id exactly once and no guest id. It fails on main: the list is ["sim-proc-1000", "sim-proc-1001"].
processes_of_guests_that_keep_assigned_ids_answer_as_before: a guest that keeps the assigned id (with and without a committed binding) and a process no session started are listed and addressed by their own ids.
Mutation checks, each applied alone: skipping resolution for get/signal/kill, skipping the list rename, answering get under the guest id, leaving signal or kill unresolved, refusing an unbound session id, or dropping an unbound guest process from the list. Each fails one of the two tests.
The simulated worker's process table takes an optional assigned id (SimProcessSpec::process_id); its start RPC still mints every id.
New in crates/l40_api/cluster/controller/server/src/data_plane_session_retention/tests.rs, against Postgres: sweep_keeps_a_binding_to_a_guest_minted_process_id_until_its_sandbox_goes. Past retention, it keeps an expired session and a closed one that are bound to a guest-minted id, plus a live unbound session. It deletes an expired unbound session and an expired one bound to its own session id. Deleting the sandbox then removes the kept rows. With the new condition replaced by TRUE, the guest-bound rows are deleted and the test fails.
cargo nextest run --config 'profile.test.package.pplx-space-api.opt-level=0' -p pplx-space-api --all-features --locked (Postgres and Redis from the nextest setup scripts): 2595 passed, 1 skipped (#[ignore]), no dependency skips.
cargo fmt --all -- --check, cargo clippy -p pplx-space-api -p pplx-space-cluster-controller-server -p pplx-space-data-plane-server -p pplx-space-node-api-sim --all-targets --all-features --locked -- -D warnings, and cargo check --workspace --all-targets --locked pass. I did not run the suites of the other crates that use the simulated worker; they compile, and the worker's start behavior is unchanged.
Kind, at an earlier head of this PR, before the retention change (64cd810): scripts/tests/smoke-kind-typed.sh native with SPACE_SMOKE_KIND_ADMIN_TOKEN_FILE set passed 47, failed 0 on main (cd07b19) and 47 passed, 0 failed with every SPACE container at that commit. That pass includes ecw.background_process_control on the native surface, which gets, lists and kills a command by its createCommand process id on the current guest. I did not rerun Kind for the retention change: a smoke run cannot reach it, since it takes a session more than an hour past expiry.
Not validated: a guest older than the current one. The Kind seed template builds the guest from this tree, so it keeps assigned ids; running an older guest there means building and publishing a template from an older guest release, which this repository has no recipe for.
Severity and risk
Severity: medium. On supported older guests, the process id the API returns for a command could not be used with any process endpoint, and once retention removed the binding, a long-running command's id stopped working again.
Risk:
get, signal and kill add a primary-key read for session-shaped ids; a list adds one read when the guest lists ids that are not session ids. Current guests store a binding equal to the session id, so their answers are unchanged.
Retention now keeps one data_plane_sessions row per attached command or terminal on an older guest until its sandbox is deleted, so those rows grow with the sandbox's lifetime rather than a day. Apart from this PR's binding lookups, every read of the table filters out expired rows, so a kept row is invisible to them once it expires. Guest-minted ids are UUIDv7, so a kept binding cannot match a later process. A closed or revoked session on such a guest stays in session lists until its expiry, 24 h after create, rather than for one hour after it ends.
No migration. Roll back by reverting; the sweep then prunes the kept rows on its next passes.
AI assistance: Claude Code drafted the code, tests, and this description; the author reviewed them.
Why: This change alters how process endpoints answer for older guests and changes which session rows retention deletes. Tests are reported for both, but both areas are contract- and GC-sensitive, so reviewers should confirm compatibility with supported guests and the retention behavior before merging.
Commit: 03c368a499a7.
No actionable comments were generated in the recent review.
Walkthrough
Public process routes translate between session IDs and guest process IDs for listing, retrieval, signaling, and killing. Session-binding lookups include expired and revoked sessions. Retention collection excludes sessions bound to a different backend process ID. The simulator supports assigned process IDs, and integration tests cover guests that mint IDs and guests that retain assigned IDs. Documentation comments describe process-ID recovery behavior for both guest types.
Merge Risk: High · up to 03c36
This change alters how process endpoints answer for older guests and changes which session rows retention deletes. Tests are reported for both, but both areas are contract- and GC-sensitive, so reviewers should confirm compatibility with supported guests and the retention behavior before merging.
Security architecture risk: Low · up to c0355
Process-ID translation remains behind existing authentication and sandbox ownership checks. No introduced authorization bypass was established. Remaining uncertainty concerns older-guest failure recovery and the longer retention of session records.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — In the traced production routes, caller-controlled process IDs can select processes only within an already authorized sandbox. Translation does not select a different sandbox or organization, and no newly reachable cross-tenant operation was established.
Trust Boundaries and Controls
observed — The public router retains native authentication and per-operation owner scoping. Process handlers additionally check sandbox ownership before lookup or guest RPC. The existing binding writer locks the session row, accepts an unset or identical binding, and refuses a conflicting process identity.
Resilience and Maintainability Implications
observed — The unchanged startup path persists bindings independently of client disconnection. Before a binding commits, cross-replica duplicate-start protection on guests that ignore assigned IDs relies on those guests honoring the session-derived idempotency key. This is a pre-existing dependency, not an introduced vulnerability.
Hardening Proposals
proposed — Consider separating durable process bindings from expirable session payloads, or defining field-level cleanup for retained rows. This would preserve process-ID compatibility without necessarily retaining token hashes, adapter state and other session metadata for the entire sandbox lifetime. This is a data-minimization proposal, not an observed disclosure.
Pre-merge checks: 6
Check name
Status
Explanation
Title check
The title starts with the lowercase area api: and a lowercase summary. It accurately describes the main process-route change and has no trailing period, ticket ID, or internal link.
Description Follows The Pr Template
The description contains all four required items: Summary, Validation with results, Severity and risk, and an AI assistance line naming the tool and its contributions.
Bug Fixes Name A Regression Test
The Validation section names command_process_ids_reach_processes_of_guests_that_mint_their_own and reports that it fails on main and passes with the fix. The diff adds that test and changes the pr…
Weakened Safeguards Are Justified
The diff does not weaken an existing test, assertion, lint, CI check, or repository guard. The changed assertion uses the same JSON macro through an import. No review-guidance files changed.
Agent Tooling Runs No New Commands
The diff changes Rust application and test files. It does not change agent plugin manifests, skills, commands, or guideline files.
Public-Safe Content
The diff, title, and description contain no identified restricted content. The credentials and addresses in the diff are test values.
Full details: Bug Fixes Name A Regression Test
Explanation
The Validation section names command_process_ids_reach_processes_of_guests_that_mint_their_own and reports that it fails on main and passes with the fix. The diff adds that test and changes the process routes.
Recent review info
Automatic approval policy
Commit: 03c368a499a7.
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.
Reviewing files that changed from the base of the PR and between c0355de and 03c368a.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 4 remain after this review.
…int their own
createCommand returns the command's session id as its process id, and
the data plane assigns it to the guest process it starts. Supported
guests that predate assigned ids ignore it and mint their own, which the
data plane binds to the session, so on them getProcess, signalProcess
and killProcess answered 404 for the id createCommand returned, and the
list showed only the guest's id.
Resolve a session-shaped id through the session's binding before
addressing the guest, answer getProcess under the id asked for, and list
a bound process under its session id. Other ids, including a session id
the guest kept, pass through unchanged.
On such guests a repeated start is not refused as AlreadyExists; note
where the data plane relies on that, and what holds a repeated start
back there.
Guests that predate assigned process ids mint their own, and the
process routes resolve a command's process id through the session row
that binds it. Retention deleted that row an hour after the session
expired (24 h after create) or ended, after which a long-running
command's process id answered 404. The sweep now keeps a row whose
binding differs from its session id; the sandbox's deletion removes
it. Current guests bind the session id itself, so their rows are
pruned as before.
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.
Summary
createCommandreturnsprocess_id, the command's session id (proc_…), and the data plane sends it to the guest asStartRequest.process_id. Guests within the supported range (>=0.1.0, <0.9.0) that predate assigned ids ignore the field and mint their own id; the data plane then binds the session to the id the guest reported (data_plane_sessions.backend_process_id). On those guests,GET /v1/sandboxes/{id}/processes/{process_id},:signaland:killanswered 404 for the idcreateCommandreturned, andlistProcessesshowed only the guest's id. docs/versioning.md requires existing operations to keep working on supported guests.The process routes now resolve a session-shaped id through that binding:
getProcess,signalProcessandkillProcesslook up the session the id names in this sandbox and, when the data plane bound it to a different guest id, address the guest by that id.getProcessanswers under the id that was asked for.listProcesseslists a process bound to a session under the session id, once, in place of the guest's id. The page cursor and order follow the listed ids.The binding is read whether or not the session has expired or been revoked, since the process can outlive it. The data-plane session retention sweep keeps it for as long: it used to delete a session row an hour after the session expired (24 h after create) or ended, so a command running past that answered 404 again. The sweep now skips a row whose
backend_process_idis set and differs from its session id, which only guests that mint their own ids produce. Deleting the sandbox removes the row (the foreign key cascades). Current guests bind the session id itself, so their rows are pruned as before. The lookups go throughPublicSessionStore, which owns the API's reads ofdata_plane_sessions: a primary-key read for each session-shaped id on get, signal and kill, and one read per list page only when the guest lists ids that are not session ids.Retried-start protection: on current guests a repeated start of an assigned id answers
AlreadyExistsand the data plane reconnects instead of running the command again. Guests that predate assigned ids never answer that. For them, once the binding commits, every later attach reconnects to the bound guest id, as before. Between the start and the binding's commit, only the guest's handling of the start's idempotency key (the session id, sent on every start) keeps an attach on another replica from starting the command again. This PR does not change that; it documents it atinitial_process_streamandPendingProcessBinding.Validation
At this head (rebased on
main):crates/l40_api/api/src/routes/public/processes/tests.rs, against Postgres and the simulated worker:command_process_ids_reach_processes_of_guests_that_mint_their_own: a guest that mints its own ids.getProcess,:signaland:killon thecreateCommandid reach the process (each then reportssignaledwith the signal sent), and the list shows each command id exactly once and no guest id. It fails onmain: the list is["sim-proc-1000", "sim-proc-1001"].processes_of_guests_that_keep_assigned_ids_answer_as_before: a guest that keeps the assigned id (with and without a committed binding) and a process no session started are listed and addressed by their own ids.SimProcessSpec::process_id); its start RPC still mints every id.crates/l40_api/cluster/controller/server/src/data_plane_session_retention/tests.rs, against Postgres:sweep_keeps_a_binding_to_a_guest_minted_process_id_until_its_sandbox_goes. Past retention, it keeps an expired session and a closed one that are bound to a guest-minted id, plus a live unbound session. It deletes an expired unbound session and an expired one bound to its own session id. Deleting the sandbox then removes the kept rows. With the new condition replaced byTRUE, the guest-bound rows are deleted and the test fails.cargo nextest run --config 'profile.test.package.pplx-space-api.opt-level=0' -p pplx-space-api --all-features --locked(Postgres and Redis from the nextest setup scripts): 2595 passed, 1 skipped (#[ignore]), no dependency skips.cargo nextest run -p pplx-space-cluster-controller-server -p pplx-space-data-plane-server -p pplx-space-node-api-sim --all-features --locked: 1685 passed.cargo fmt --all -- --check,cargo clippy -p pplx-space-api -p pplx-space-cluster-controller-server -p pplx-space-data-plane-server -p pplx-space-node-api-sim --all-targets --all-features --locked -- -D warnings, andcargo check --workspace --all-targets --lockedpass. I did not run the suites of the other crates that use the simulated worker; they compile, and the worker's start behavior is unchanged.scripts/tests/smoke-kind-typed.sh nativewithSPACE_SMOKE_KIND_ADMIN_TOKEN_FILEset passed 47, failed 0 onmain(cd07b19) and 47 passed, 0 failed with every SPACE container at that commit. That pass includesecw.background_process_controlon the native surface, which gets, lists and kills a command by itscreateCommandprocess id on the current guest. I did not rerun Kind for the retention change: a smoke run cannot reach it, since it takes a session more than an hour past expiry.Not validated: a guest older than the current one. The Kind seed template builds the guest from this tree, so it keeps assigned ids; running an older guest there means building and publishing a template from an older guest release, which this repository has no recipe for.
Severity and risk
Severity: medium. On supported older guests, the process id the API returns for a command could not be used with any process endpoint, and once retention removed the binding, a long-running command's id stopped working again.
Risk:
data_plane_sessionsrow per attached command or terminal on an older guest until its sandbox is deleted, so those rows grow with the sandbox's lifetime rather than a day. Apart from this PR's binding lookups, every read of the table filters out expired rows, so a kept row is invisible to them once it expires. Guest-minted ids are UUIDv7, so a kept binding cannot match a later process. A closed or revoked session on such a guest stays in session lists until its expiry, 24 h after create, rather than for one hour after it ends.AI assistance: Claude Code drafted the code, tests, and this description; the author reviewed them.