Repository navigation
api: adopt a Direct sandbox's live record even when its receipt lags - #1218
Conversation
A wake that races a Direct node's idle suspend can leave the sandbox running on that node while the row is unplaced. The API adopts such a record instead of restoring around it (#1169), but only when the route identity's execution generation and the last reserved one both equal the record's. Two reachable shapes failed that fence while the record was the newest execution: - A placed wake reserves the next generation before its Wake and persists the route receipt only after the response. If the node's suspend projection clears active_node_id and that wake's API process stops before the receipt, the record runs one generation ahead of the route identity. Every later wake restored instead: the record's node refused the restore's Start for a different execution, and the candidates moved on, running a second copy from the older durable snapshot or answering 503 without one. - A second wake or an explicit restore reserves a generation that its restore then abandons, for example when the first wake's completion writes started over restoring and clears its lease. The row is left started with no active node and the last reserved generation above the record, which no lookup adopted, so every runtime call answered 503. The adoption now fences on a route identity on the record's node whose execution generation is at most the record's, and a record generation at most the last one reserved, and equal to it unless the row holds no restore-lease token. On a tokenless row no restore owner can still persist a receipt or complete, since both fence on the token, which a claim writes and leaving restoring clears; a reservation above the record is abandoned, or a wake through the same node's Wake, which joins. A newer execution on another worker would have moved the route identity there, because a receipt moves it only to the row's active node. A restoring row that still holds a token, its lease expired, keeps the strict rule, and a record older than the route identity is still not adopted. After a wake-path adoption the Wake's receipt catches the route identity up to the record. A cross-node restore's source cleanup treated a record whose lifecycle it could not decode as not live and deleted it. sandbox-manager publishes Unspecified for a label it cannot parse, and an API older than its nodes cannot decode a newer value. The cleanup now deletes only a record whose lifecycle decodes to one that holds no VM and keeps an undecodable one with its own warning. The probe_runtime_client and ingress probe docs now say that on Direct the lookup may place a started row with no active node back on the node running it, and the delete-miss comment no longer says a suspended Direct row is never contacted. The node-api simulator's Wake of a Direct record that needs no transition now reports the record's route identity, as sandbox-manager does, so the tests see the receipt catch up.
|
Thanks for opening this pull request! A SPACE maintainer will take a look. CONTRIBUTING.md explains how we review outside contributions. |
The adoption fence relaxed the route identity's execution generation for every row, including a restoring row that still holds a restore-lease token after its lease expired. Such a row may have an owner whose Start ran and whose receipt has not landed. Adopting it writes started and clears the token that receipt fences on; an owner whose receipt is then refused after a remote-snapshot restore deletes that destination, which may be the copy the adoption just placed the row on. A row that holds a restore-lease token is now adopted only when the route identity, the record and the last reservation are one execution generation, as before the relaxation. Only a tokenless row is adopted with a route identity that trails its record. The cost is the older behavior for an owner that stopped after its Start and before its receipt: the wake restores around the record, and the cross-node cleanup keeps that copy.
A wake that adopted a Started record ahead of the route identity wrote started, active_node_id and the placement in one statement, and only the Wake after it persisted the receipt that moves the route identity to the record's execution generation. A wake that failed between the two left the row started on the node while the route identity still named the older execution. Route tokens minted from that identity, by the API's spaced bridge and by data-plane session routes, are refused by the node as a different execution generation. An implicit access does not wake a started row, so nothing caught the route identity up until a later Wake persisted a newer receipt. On the wake path, when the route identity on the record's node trails a Started record on a tokenless row that still has the lifecycle the wake read, the wake now sends that record's worker a Wake first and persists its receipt, then adopts the receipt's execution. That Wake carries no route identity, so it cannot start a new execution: a record that now needs one is restored instead, and a Wake that fails commits nothing, leaving the row unplaced for the next wake. The adoption statement itself writes started only when the route identity names the execution it adopts; otherwise it keeps the read lifecycle, as it does for a record still moving. Every caller of wake_execution gets this, including access_runtime_client and the spaced fallback wake, which do not complete the lifecycle themselves. An adoption on the lookup path issues no Wake and still places a started row whose route identity trails its record; the node's next suspend projects it and a later wake repairs the route identity. The node-api simulator can now fail the next Wake of a sandbox, and the idle-suspend tests seed the resident with the route identity's activation attempt, so a Wake that needs no transition returns the receipt sandbox-manager would.
Doc and comment wording only. The cross-node restore's source cleanup deletes any record whose lifecycle decodes outside the live set (lifecycle_holds_live_runtime), which includes Quarantined, Failed, CleanupRequired, CleanupFailed and Deleting. A quarantine after an unconfirmed pause does not establish that the VM stopped, so the docs no longer say the cleanup deletes only a copy that holds no VM. They say it deletes a record outside the live set and keeps a live, undecodable or unreadable one. The adoption's reasoning now says that only a newer execution whose receipt landed on another worker would have moved the route identity there. A restore whose receipt was refused after its lease was cleared can leave an untracked copy on another worker, and an abandoned reservation may still have started one. No route reaches such a copy, so adopting the record's own node is still right. The delete-miss comment describes Direct's local-state lookup as the first of active_node_id and target_node_id that is set, and the probe contract names the Direct adoption as the one routing-evidence change a probe may make.
zibi-pplx
left a comment
zibi-pplx
left a comment
There was a problem hiding this comment.
The adoption fence changes look right for the wake path. I found one restore path that still bypasses adoption, plus a doc question.
-
[medium] An explicit Restore still restores around a live record (pre-existing on main)
crates/libs/api/src/controllers/substrate/direct.rs:4624-4636:restore_executiongoes straight torestore_execution_with_candidatesand never reads the source record or callsadopt_live_source_record. The adoption and catch-up added here only run fromwake_executionandsandbox_node. A nativerestoreand ECWresumeboth reachrestore_execution, because ECW resume maps toSandboxAction::Restore(routes/e2b/lifecycle.rs:91).
It can still start a second copy when all of these hold:- the row is
suspended; - the source node still runs a live record from the wake-vs-idle-suspend race;
- the source refuses the restore's
StartwithFailedPrecondition, whichshould_try_next_restore_candidate(direct.rs:307) treats as "try the next candidate"; - another candidate has a usable remote snapshot.
The sandbox then restores elsewhere from the older snapshot, and the source cleanup keeps the live copy. That is the outcome this PR prevents for wakes.
Suggestion: before claiming the lease, have Direct'srestore_executionread the source record the same waywake_executiondoes, and adopt it under the same fence. Keep the fix in the substrate; the ECW resume handler should keep mapping to Restore, not to a wake. If that is out of scope here, a tracking issue linked from the "Not covered" list would do. - the row is
-
[low] Is
AccessBackend::reachablestill a read-only probe?
crates/libs/api/src/controllers/sandbox/ingress.rs:38-41: the doc still opens with "Read-only probe" and now adds that on Direct the lookup may re-place a row. That re-placement is anUPDATEofactive_node_id,target_node_idandplacement, behind a nodeStateRequestthat can take up toDIRECT_SOURCE_RECORD_READ_TIMEOUT(3 s). Should the probe skip the adoption, for example through a lookup that doesn't adopt? Or should the trait doc drop "read-only" so callers don't rely on it?
Withdrawing this approval for now. Any feedback above still stands.
mabdelraouf-pplx
left a comment
mabdelraouf-pplx
left a comment
There was a problem hiding this comment.
Approving. Please follow up on these two fixes:
-
Keep the
startedlookup fence strict unless the route identity has been caught up.sandbox_node()currently uses the relaxedroute <= record <= lastadoption without runningcatch_up_route_identity(), so astartedrow can be placed while its route identity still trails the execution. Normal access then skips Wake and presents stale route tokens. Either catch up on lookup or requireroute == recordthere, and add a PostgreSQL-backed case forroute < record. -
Bound
catch_up_route_identity()end to end by the caller's remaining deadline. The new fresh connection and Wake use the default node RPC timeout; only the capability check uses the passed budget. Carry one deadline across connect, capability check, and Wake so a stalled catch-up cannot outlive the access or wake request.
Summary
A follow-up to #1169 and #1170. When a wake races a Direct node's idle suspend, the sandbox can keep running on that node while its row has no active node. #1169 adopts that running record instead of restoring around it, but only when two generations both equal the record's: the route identity's execution generation and the last reserved one. Two reachable shapes failed that check even though the record was the newest execution:
active_node_idand that wake's API process stops before saving the receipt, the record is one generation ahead of the route identity. Every later wake then restored instead of adopting. The record's node refused the restore's Start, and the restore moved on to other candidates: it either started a second copy from the older durable snapshot, or answered 503 when there was none.startedoverrestoringand clears that restore's lease. The row is leftstartedwith no active node, and the last reserved generation is above the record's. No lookup adopted that row, so every runtime call answered 503.Separately, after a cross-node restore, the cleanup of the source node's copy deleted a record whose lifecycle the API could not decode, treating it as not live. sandbox-manager publishes
Unspecifiedfor a label it cannot parse, and an API older than its nodes cannot decode a newer value. Either record may still hold a VM.The change. Adoption now requires all of the following:
The reasoning:
restoringclears. A reservation above the record was therefore abandoned, or is a wake through the same node's Wake, which the adoption joins. An abandoned restore may still have started a VM on another worker (see Not covered).restoringrow that still holds a token (its lease expired) may have an owner whose Start ran and whose receipt has not landed. Adopting would clear the token that receipt checks, and an owner whose receipt is refused after a remote-snapshot restore deletes that destination, which may be the copy just adopted. Such a row keeps main's rule.No
startedrow with a trailing route identity on the wake path. The adoption writesstartedonly when the route identity already names the execution it adopts; otherwise it keeps the read lifecycle, as it does for a record that is still moving. When a wake finds aStartedrecord ahead of the route identity on a tokenless row, it first sends that record's worker a Wake and persists the receipt, then adopts the receipt's execution. That Wake carries no route identity, so it cannot start a new execution. A record that now needs one is restored instead. A catch-up Wake that cannot connect or answer places nothing, so the row stays unplaced and the next wake reads it again. A receipt naming the wrong sandbox or node clears the route identity, as on main's Wake path. This lives inwake_execution, so every caller gets it, includingaccess_runtime_client(spaced connect, ports) and the spaced fallback wake, which do not complete the lifecycle themselves. The catch-up is a read plus the existing receipt write; thesandbox_writesbaseline is unchanged.The cross-node cleanup now deletes the source copy only when it reads a record there whose lifecycle is outside the live set (
lifecycle_holds_live_runtime). It keeps a live, undecodable or unreadable record and logs a warning. A lifecycle outside the live set, such asQuarantinedafter an unconfirmed pause, does not prove the VM stopped; that behavior predates this change.New adoption rule, relative to #1170's description. #1170 says adoption needs "the same execution generation". That is no longer true on a tokenless row. Adoption now needs a route identity on the record's node whose generation is at most the record's, not equal to it. Adoption therefore still sets
active_node_idonly to the node the route identity names, so #1170's point stands: adoption cannot make the route identity andactive_node_iddisagree on the node. On the wake path, the route identity's generation catches up before the row readsstarted.Also in this change:
probe_runtime_clientand ingress probe docs now say the Direct lookup may put astartedrow with no active node back on the node running it. The delete-miss comment describes Direct's local-state lookup as the first ofactive_node_idandtarget_node_idthat is set.Validation
At
a9cb40c4on top of58fe107e. Main has since gained #1213, which changes the simulator andlifecycle.rswithout conflicting with this change:cargo nextest run -p pplx-space-api --all-features -E 'test(idle_suspended)'failed 2 of 10:wake_adopts_a_started_record_only_at_its_route_identity: after a wake whose Wake failed, the row wasstartedand placed while its route identity still named generation 1. This held for bothwake_executionandaccess_runtime_client; each failed when run as the table's first case. Its third case, a successful access wake, passed there and guards the access path.wake_adopts_a_live_record_on_its_artifact_node, at the case "restoring row whose expired owner reserved the record's generation": the token-holding row was adopted instead of restored around.dc8e48b3, not re-run at this head). With only the test file applied to main's code, the same command failed at the cases "live record ahead of a receipt its wake never persisted" (the wake did not reach the record's worker) and "record at the receipt, below an abandoned reservation" ("is not assigned to an active worker node"). With the cleanup's undecodable-lifecycle arm removed, the case "record whose lifecycle the API cannot decode" failed because the source copy was deleted.idle_suspendedtests pass. Disabling only the catch-up makeswake_adopts_a_started_record_only_at_its_route_identityand the lagging-receipt case fail again.cargo nextest run -p pplx-space-api -p pplx-space-node-api-sim -p pplx-space-cluster-controller-server -p pplx-space-internal-api-server --all-features: 3784 passed, 1 skipped. The skip is the ignoredbenchmark_single_node_occupancy. Postgres and Redis were started by nextest's setup scripts, with no dependency skips.cargo clippyon the same packages with--all-features --all-targets -- -D warningsis clean, andcargo fmt --all -- --checkis clean.cargo arch --base-baseline <main's architecture-baseline.toml> --base-architecture <main's ARCHITECTURE.toml>: 0 required findings. Thesandbox_writesbaseline is unchanged (direct.rsstays at 6).direct.rsgets larger; it was already over its advisory size cap on main.SPACE_KIND_SUBSTRATE=direct SPACE_KIND_STORAGE_CREDENTIALS=static infra/space/kind/scripts/up.sh, then the runs below. Environment:SPACE_SMOKE_SUBSTRATE=direct SPACE_KIND_SUBSTRATE=direct SPACE_KIND_STORAGE_CREDENTIALS=static SKIP_BOOTSTRAP=1 SPACE_SMOKE_BACKEND=kind, the admin token andSPACE_DATA_PLANE_URLset asscripts/tests/smoke-kind-typed.shdoes, and thenativesurface fornative_contract.*. Five runs each, all pass (35/35):ecw.pause_timeout_requires_connectecw.pause_connectecw.access_idle_reconnect_preserves_workspacenative_contract.command_wakes_idle_suspended_sandboxnative_contract.process_session_wakes_idle_suspended_sandboxnative_contract.session_reattach_wakes_idle_suspended_sandboxnative_contract.session_reconnect_after_idle_suspendup.sh, then one run ofecw.pause_connect: pass.The Kind runs show the idle-suspend wake paths still work. They do not reproduce the race shapes, which need a wake whose API process stops mid-wake, a Wake that fails after the record read, or a concurrent wake during the suspend. The Postgres-backed tests above cover those shapes.
Severity and risk
Severity: low to moderate, and Direct only. Hitting it takes the wake-vs-idle-suspend race plus an interrupted or concurrent wake. When it hits, the sandbox either runs a second copy from an older snapshot or answers 503 on every runtime call until someone intervenes.
Risk: narrow. The fence is a single guarded
UPDATE, and it is relaxed only where no restore owner can still record a newer execution. Rows that still hold a restore-lease token keep main's rule. The cost is main's behavior for a restore owner that stopped after its Start and before its receipt: a later wake restores around that record, and the cross-node cleanup keeps the copy. A wake that finds aStartedrecord ahead of the route identity now sends one extra Wake to that record's worker before it adopts; a failure there fails the wake without placing the row. The cleanup change only keeps more source copies than before: an undecodable record now stays on its node instead of being deleted. Kubernetes is unaffected. Rollback: revert these commits.Rollout
No migration and no flag. The change is in the API, plus a test-only simulator change; sandbox-manager is unchanged. While old and new API replicas run together, they run different
WHEREclauses. An old replica applies the strict fence and may restore around a record a new replica would adopt; that is today's behavior. Both adoptions are a singleUPDATEguarded onactive_node_id IS NULL, so the first writer wins and the other joins throughactive_sandbox_node. Old replicas keep deleting an undecodable source copy until they are replaced.Not covered
startedrow with no active node) issues no Wake, so it does not catch the route identity up. Astartedrow whose route identity trails its record can still reach the lookup, for example when a restore started the record on its own node and an earlier wake's completion cleared that restore's lease before its receipt. After the adoption, the node refuses route tokens minted from the older identity until a later Wake persists a newer receipt, such as a wake after the node next suspends it.Quarantined,Failed,CleanupRequired,CleanupFailedorDeletingsource copy, as on main. Whether to keep some of those is a follow-up.suspended. On the access paths, which do not complete the lifecycle, the row then readssuspendedwhile it runs, as it already does on main for a record that was still moving when adopted. This belongs with the concurrent-wake behavior above.AI assistance: Claude Code wrote the fix and its tests, applied it to main, ran the validation and drafted this description.