Repository navigation
[restore] retire an abandoned restore's execution in the revert that abandons it - #545
Conversation
5d1fcd1 to
a58251d
Compare
92e89ab to
70a6a43
Compare
ac-pplx
left a comment
There was a problem hiding this comment.
Requesting changes: the transactional retirement is a useful improvement, but three access paths can still return a restoring execution before restoring -> started is durable. That breaks the prerequisite for treating an abandoned restore as disposable.
Reviewed 59789a1d44cfacea3d326581ab6ce4b6766eeab4...70a6a438d043446f75809b005293a1945c45a5a0.
1. [P2] Gate successful recovery followers on the sandbox lifecycle
observe_started_recovery accepts any active execution with generation >= the joined generation and reports success when that execution is running with an IP; it does not read the sandbox lifecycle. The follower converts that observation directly into a binding, and the Joined branch returns it without the new binding_once_started gate.
A follower waiting on generation N can miss an intervening suspend and observe generation N+1 running while the sandbox is still restoring. It then hands that replacement to an access request before restore completion is committed. The new gate in SupersededBySuspend does not cover this case because the observation succeeds. Require the sandbox to be started before returning follower success, preserving the original deadline, and add a regression that skips over the intervening suspended state.
2. [P2] Recheck lifecycle after refreshing the session route row
resolve_data_plane_session_route rejects restoring on its initial row, but can then replace that row with update_route_row. The update returns fresh sandbox lifecycle and execution identity, which are used for signing without another lifecycle check.
If the first read sees a started sandbox with a stale session endpoint, and a same-node suspend/restore commits between that read and the update, the refreshed row can identify the restoring replacement. Process, terminal, and file-watch sessions skip ingress preparation, so the resolver can sign and return that replacement's current identity. Matching execution identity therefore does not reject this route. Recheck the refreshed lifecycle before signing and return a retryable result or wait through the activation path using the current lifecycle. Cover the transition between the two SQL statements in a regression.
3. [P2] Apply the started gate to the late suspend-recovery branch
The late error-recovery branch still returns redrive_started_recovery_after_suspend(...).await.map(|activation| activation.binding) directly. Unlike the earlier SupersededBySuspend branch, it never calls binding_once_started.
When a started-recovery owner fails readiness and discovers a concurrent suspend, this branch redrives the restore. If complete_owned_user_restore then encounters a database error, it logs the error and preserves the successful binding result while the row can remain restoring. Access can consequently reach a VM that a subsequent revert is now allowed to retire, risking loss of newly accepted writes. After redrive succeeds, use binding_once_started(sandbox_id, join_deadline) as in the earlier branch, with a regression for a delayed or failed started commit.
Validation: all 399 selected tests passed, including the PostgreSQL store aggregate with PostgreSQL and Redis dependencies enabled. Formatting, diff checks, and affected-package Clippy passed. These three interleavings are source-verified findings; they were not reproduced in a live VM environment. The PR's 20 checks were successful at review time.
70a6a43 to
252dce8
Compare
|
Thanks, all three hold at 70a6a43. Fixed in 252dce8, each with a test that failed before its fix:
|
252dce8 to
e70a73a
Compare
kumarpallav-pplx
left a comment
There was a problem hiding this comment.
Requesting changes at e70a73a.
[P1] Stage the routing guards before enabling abandoned-restore retirement
The new retirement relies on a restoring VM never having served user requests, but the guards establishing that invariant and the destructive cleanup ship together. During a mixed-version rollout, older API/internal-API replicas still route to a reachable execution while its sandbox is restoring.
A concrete interleaving is: the sweeper finishes its live-pod check; that restore pod becomes reachable and an older replica serves a write; then the new revert retires the same execution as replaced and cleanup deletes its pod. The next wake selects the pre-restore snapshot, losing the accepted write. This PR deliberately allows cleanup to delete a pod that recovered after the check; the old resource-version refusal could instead stop that cleanup. See the new retirement and new access guard.
Please split out and fully deploy the routing guards first, or add a default-off rollout gate covering both the API rollback retirement and the sweeper retirement. Enable retirement only after every routing replica has the guard and pre-upgrade restores that may have served traffic are accounted for. The rollback plan must disable retirement before restoring older routing binaries. Deploying #331 alone does not establish the no-user-traffic invariant.
Please cover the mixed-version/late-reachable-pod interleaving with a regression. This is a source-verified active-traffic rollout blocker, not a reproduced production incident; no local tests were run.
e70a73a to
985f698
Compare
|
Agreed, this is a real gap. The routing guards and the destructive cleanup can't share one rollout. Fixed in 985f698. Both retirements, the API rollback's and the sweeper's, now sit behind a default-off switch,
The routing guards, the owner's Turning retirement on is a separate, per-environment step, and it happens only after every API and internal-API replica runs the guards and pre-upgrade restores that may have served traffic are accounted for. Rolling back turns it off first, before older routing binaries return. That's in Tests for the interleaving:
Forcing retirement on in either writer while the switch is off fails its test. |
|
@kumarpallav-pplx, re-requesting your review on 985f698, which puts both retirements behind the default-off switch described in my reply above. The prerequisite, PR #331 (the teardown-suspend backup generation fence), is already merged. |
985f698 to
0dd3272
Compare
0dd3272 to
548cb3b
Compare
Summary
When a restore fails and is rolled back to
suspended, the rollback now retires the execution the restore created asreplaced, in the same transaction that moves the row back. Before, both rollback paths changed only the lifecycle. The restore's execution stayed active at the pod's generation on asuspendedrow: nothing revisits that shape, the next restore reused that generation and could adopt the stale pod, and nothing in the row tells it apart from a live sandbox whose pod holds the only copy of its state. With the execution retired, the leftover pod becomes a no-active-execution restore pod on asuspendedrow, which the orphaned-restore-pod GC already reaps. The next restore claims a newer generation, and the teardown-suspend backup fence refuses any capture registered against the retired one.The retirement happens only when restore selection on the reverted row is proven to pick a ready, durably stored pre-restore source (
restore_retry_suspended_revert_snapshot_exists, the proof the sweeper's revert already required). Without that proof, the rollback keeps today's lifecycle-only revert.Retirement ships disabled, behind
SPACE_ABANDONED_RESTORE_RETIREMENT_ENABLED, which both the API and the cluster controller read. Retiring is safe only once no user request can have reached arestoringsandbox's pod. The routing guards below establish that, but only on replicas that run them. During a mixed-version rollout an older API or internal-API replica still routes to a reachable restore pod, and a retirement would then delete what it served. Disabled, both rollbacks revert only the lifecycle as before, and the sweeper's cleanup refuses a restore pod that came up after its check. Everything else in this PR is active from deploy. Turning retirement on is a per-environment step, tracked separately, once every routing replica runs the guards. Rolling back to older routing binaries turns it off first.Prerequisite: PR #331 (the teardown-suspend backup generation fence) is already merged. Turning retirement on waits until #331 is deployed where it's enabled; with the switch off, this PR deletes no pod #331's fence would have to answer for. With retirement enabled, deleting a bound abandoned-restore pod drives the node's teardown suspend, and only #331's fence turns that capture of a retired generation into a refusal that releases the runtime, instead of a snapshot the next restore could select.
What changed
crates/libs/api/src/controllers/substrate/kubernetes/store/restore_lease.rs,revert_restore_claim_to_suspended). This replacesrevert_lifecycle_after_rollback_best_effort.restoringunder the claim's lease token. The tokenless evacuation call also requiresrestoringnow. The proof and the claim's execution are read after the lock, and the retirement targets the identified generation.restoringtransition that still carries the claim's activation attempt and generation. A redrive of the samerestoringrow takes a new attempt, so a delayed rollback can't retire the redrive's execution. A claim that failed before it was assigned an attempt is identified by its lease token alone, so a tokenless one retires nothing. An execution the claim reused is left alone.replaced, invalidates open port exposures, and unplaces the row (placement = 'unplaced',active_node_id = NULL). It keepstarget_node_idand the artifact marker, which name the wake source.execution.rs,revert_failed_restore_claim) then deletes that one pod, best-effort. A pod it misses has no active execution, so the GC and the next restore's lingering-pod sweep reap it.crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/store.rs). The revert CAS now runs in a transaction with the execution's retirement asreplaced.retire_executioncleanup then finds no active execution, deletes the inactive rows' pods and unplaces the row.started, and with the gate below no user request reached its VM, so nothing it holds is newer than the pre-restore snapshot the next wake restores from. Before, a retire that failed or was refused after the CAS left the rowsuspendedwith its execution active.restoringsandbox's pod. Before, the access path andruntime_clientdialed arunningexecution's pod while the row was stillrestoring, and the data-plane route resolvers routed to it. Anything that VM served was lost when a rollback or the sweeper discarded the restore. Every user path now waits for or joins the in-flight restore until the row isstarted, bounded by the existing restore deadline, and otherwise fails with a retryable error. A path that doesn't recover reports the runtime unreachable instead.ResolvedPodInstancecarriessandbox_restoring(pod_ops/read.rs). The probe that resolves a reachable pod IP returns none for a restoring row.runtime_clientandaccess_runtime_client(adapter/substrate_adapter.rs), on both the cached fast path and the fallback, calljoin_restore_before_dial(execution.rs). It joins the in-flight restore first, then dials the execution the row routes to once the row isstarted. It reclaims, within what's left of the caller's budget, only a row that's stillrestoringafter its owner's lease and execution ownership lapsed and that no drain owns. It takes the restoring lease itself and drives the wake under that claim, so a rollback or sweeper revert can't land in between and turn the reclaim into a fresh restore. A request that finds an owner back keeps waiting on the shared restore poller; a reclaim left with under a second of budget, or one whose row moved on, hands the lease back. A request whose owner rolled back tosuspendedgetsRestoreInFlightinstead of starting a fresh restore that its caller's access-wake policy and admission never approved. A row that's alreadystartedwhen the join re-reads it is dialed at once. A drain's evacuation restore owns itsrestoringrow through the evacuation marker, not a lease, so the row reads as unowned. The request waits for the drain to move it tostartedand drives no wake of its own. A restore that doesn't reachstartedin time fails asRestoreInFlight(retryable 503). A row that leaves for a state that can't be woken (failed,deleting, …) gets that state's error.started. The wake that owns a user restore (it holds the restore lease token and targetsstarted) runsrestoring → starteditself before it returns (K8sStore::complete_owned_user_restore,store/restore_lease.rs). It's the cancelled-wake redrive's fenced CAS (wake_completion.rs): the row must still berestoringwith a routable active execution. The redrive's lease-expiry guard is replaced by the owner's own lease token, since the owner is the live attempt that guard protects. Before, on Kubernetes,startedcame from the lifecycle controller's completion CAS, the cancelled-wake redrive, the node's published state, or the transient-lifecycle sweeper's converge, and the access path runs no completion CAS. So a restore driven from an access request stayedrestoringuntil the node publication converged it, and with the gate its request couldn't be served until then. Once the row isstarted, neither the API rollback nor the sweeper's revert, bothrestoring-guarded, can move it back. The access andruntime_clientpaths that wake asuspendedsandbox dial only once the row isstartedtoo. The lifecycle controller's completion CAS still runs and finds the rowstarted. When the commit lands, the restore's detached task emitsRestoreCompletedfrom the substrate, so a caller that hung up can't drop it. The node's published state used to emit it when it converged the row, and now finds the rowstarted. The lifecycle controller's Wake, Restore and Start actions still emit their own completion events from the API. So an explicit Restore now reportsRestoreCompletedtwice, once from each source, as wakes already reportWakeCompletedfrom both. A started recovery superseded by a suspend redrives a restore, at its claim or after its readiness fails, and both hand out the binding only once the row isstarted(redrive_suspended_recovery_for_access), so an owner whose commit failed doesn't hand out its restoring pod. As before that convergence, the access path doesn't re-enqueue open port exposures itself; the per-node reconcile picks them up.observe_started_recovery,store/started_recovery.rs) reports success only for a running execution on astartedsandbox. If a suspend and a restore land while it waits, the generation it finds running is that restore's, and it keeps waiting while the row isrestoring.native_kept_worker_client(wait_ready.rs) doesn't follow a re-resolved pod whose sandbox isrestoring. It returns the first dial's error, and the caller's recovery joins the restore.reachable_runtime_client, which doesn't recover, returns no client for a restoring sandbox, without waiting. Its callers are best-effort (the ECW keepalive refresh, the loopback bridge close) or turn an unreachable runtime into a retryable 503 (stats, snapshot listing, fork capture, and Pause and Checkpoint on one of their two paths).restoringsandbox, the calls that resolve the runtime throughruntime_client(Stop, Kill, Resume, and Pause and Checkpoint on their other path) now wait for a live owner's restore, within their 120-second budget, instead of dialing the restore pod at once.data_plane_sessions/{service_route,session_route}.rs) wait on a restoring row through the activation owner and refuse it with a retryableUnavailableotherwise, on every transport and route kind. A session route whose stored endpoint is stale re-reads the sandbox when it refreshes the row (update_route_row), and refuses it retryably if a suspend and a restore committed since the first read. The refresh runs in a transaction that a refusal rolls back, so a refused route changes nothing.crates/libs/api/src/controllers/sandbox/lifecycle.rs).suspendedaftersuspend_executionreturned, guarded only against terminal lifecycles. A wake that claimed the row in between, and reachedstarted, was put back tosuspendedwith its new execution active.SubstrateCapabilities::suspend_commits_lifecycle(Kubernetes: true, Direct: false) skips that write where the substrate's suspend already committedsuspendedunder its execution fence. Direct still relies on the write and keeps it.orphaned_restore_pod_gc/classify.rs,reconciler.rs). A same-generation pod on asuspendedrow is still never reaped. It's now its own skip class,suspended_same_generation, instead ofactive_runtime, and the tick-complete log reports its count. The count sizes the rows left in this shape by rollbacks from before this change.placement_changed_atpast the pre-restore snapshot.durably_restorable, which gates a fenced release of the abandoned pod's runtime, so the release waited for the next suspend and the pod counted againstteardown_stuck.placement_changed_atunder the proof, as the markerless restoring recovery inpod_tracking.rsalready does. That happens only when no active execution remains: an execution the API rollback's claim reused, rather than created, keeps the clock, since nothing proves its pod holds no state of its own.restoringrow keeps the clock its retry window reads.AbandonedRestoreRetirement,crates/libs/api/src/controllers/substrate/mod.rs).SPACE_ABANDONED_RESTORE_RETIREMENT_ENABLED, default off; only an explicit truthy value turns it on.KubernetesStateand passes it torevert_restore_claim_to_suspended. Disabled, the rollback reverts only the lifecycle whatever the proof says: the execution, placement and placement clock stay, and no pod is deleted. The outcome isexecution_left_active.ReconcilerConfig, andSubstrateSweepSink::with_abandoned_restore_retirementtakes it. Disabled, the revert runs the lifecycle-only CAS (revert_to_suspended_lifecycle_only_query). The cleanup then retires the still-active execution under the candidate's fence, which refuses a pod that came up since, and the revert is undone. The undo puts back a placement clock only where a retiring revert cleared it, so a lifecycle-only revert's clock, and a bind's since, are kept.sandbox.restore.claim_revert{outcome}, one per failed fresh restore claim the API rolls back (execution_retired,lifecycle_only,execution_left_active,not_reverted).architecture-baseline.toml: thesandbox_writescaps forrestore_lease.rs(5 to 8) and the sweeper'sstore.rs(10 to 12). The new writes are guarded lifecycle transitions, which stay out of the sandbox store.Test plan
cargo fmt --all -- --checkcargo clippy --workspace --locked --all-targets --all-features -- -D warnings--run-ignored all --ignore-default-filter --retries 1) on a Linux host:group(privileged)tests as root from an archive of the same build, insideci/scripts/with-nocow-test-filesystem.sh;node-runtime-corefs-verity tests need a kernel with fs-verity, which this host's kernel lacks, so they fail there as they do atmain. CI's privileged lane runs them.failed_restore_revert_retires_only_its_own_proven_execution(API store, Postgres): retires only the claim's own proven execution and keeps the wake source. It leaves alone reused, unproven and moved-on rows, an execution a redrive took over, and a tokenless claim with no attempt.revert_cas_retires_only_the_observed_execution(sweeper store, Postgres): each identity field, an unread fence, and the no-execution and unobserved fences.placed_then_reverted_restore_is_durably_restorable_at_onceandrevert_undo_restores_the_placement_clock(sweeper store, Postgres).sweeper_revert_cleanup_deletes_a_restore_pod_that_bound_after_its_check(Kubernetes adapter, scripted apiserver): a restore pod that binds and comes up after the sweeper's live-pod check is deleted by the cleanup. The row stayssuspended, and restore still selects the durable pre-restore snapshot.durably_restorable, after the bind moved its placement.adapter_characterization_tests/restoring_access.rs(Kubernetes adapter, scripted worker): access andruntime_clienton a restoring sandbox join the restore and dial only once it'sstarted, and are refused retryably when it lingers.reachable_runtime_clientfinds no client without waiting, a restoring sandbox without a resolved pod joins the restore, a joined restore whose owner rolls back or fails starts no restore of its own, a request on an evacuating sandbox waits for the drain without waking it, the probe finds no client for a restoring pod, and a rolled-back restore discards no request it served. A request that wakes asuspendedsandbox and joins an in-process restore run that completes before the row isstartedis dialed only once it is, and a restore that finished before the join is dialed rather than refused.a_native_request_does_not_follow_a_moved_pod_into_a_restore(Kubernetes adapter).without_retirement_a_failed_restore_revert_moves_only_the_lifecycle(API store, Postgres) andwithout_retirement_the_revert_moves_only_the_lifecycle(sweeper store, Postgres): with retirement disabled, a proven revert leaves the execution active, the row placed and the placement clock as it was.without_retirement_the_sweeper_cleanup_keeps_a_restore_pod_that_came_up_after_its_check(Kubernetes adapter, scripted apiserver): the mixed-version interleaving, from the shape the lifecycle-only revert leaves. The restore pod came up after the sweeper's check, as a pre-guard replica may have routed to it. The cleanup's fence refuses, nothing is deleted, and the execution stays active.a_failed_restore_rollback_deletes_its_pod_only_with_retirement_enabled(Kubernetes adapter): the substrate's rollback of a failed claim, with the switch on and off. Off, the execution stays active and no pod is deleted.owned_restore_completion_requires_its_lease_and_a_routable_execution(API store, Postgres): the owner's commit needs its own lease token, arestoringrow and a routable execution, and a rollback after it finds nothing to revert.an_owned_restore_commits_started_before_it_returnsanda_request_reclaims_a_lapsed_restore_and_dials_it_once_started(Kubernetes substrate, sim worker).revert_after_the_owner_commits_started_claims_nothing(sweeper store, Postgres): a revert the sweeper decided on before the owner's commit claims nothing after it, even with the observed execution's exact fence.started_recovery_followers_succeed_only_on_a_started_sandbox(API store, Postgres),a_redriven_restore_hands_out_no_binding_until_it_is_started(Kubernetes substrate, sim worker, with a test-only trigger that fails thestartedcommit) anda_route_refresh_refuses_a_sandbox_that_started_restoring(internal API, Postgres). Each failed before its fix.stored_ingress_access_joins_a_restore_whose_lease_lapsed(ingress on the Kubernetes substrate).direct_route_to_a_restoring_sandbox_waits_for_its_restoreandsession_route_to_a_restoring_sandbox_waits_for_its_restore(internal API, Postgres): with no owner, an owner that can't finish, and one that finishes.suspend_keeps_a_wake_that_claimed_after_the_suspend_committed(lifecycle controller on the Kubernetes substrate): a deferred trigger claims the row right after the suspend commits, and the row stays claimed.lifecycle = 'restoring'guard changes nothing, because the lease-clear trigger nulls the token on any row that leavesrestoring. Skipping the join's wait on the shared restore poller changes no outcome, because the reclaim's lease claim refuses a live owner and thestartedwait follows; the poller only keeps followers from polling the row one by one. Not pinned by a test: the reclaim's preclaimed wake (a rollback or sweeper revert would have to land between the reclaim's read and its claim), its lease release when the charge is refused, its one-second floor, and the switch's two environment reads (unset reads as disabled). These include the pre-fix behavior of both rollbacks (lifecycle-only revert, no retirement) and of the HTTP suspend (trailing write kept).Not exercised end to end: a restore pod that comes up after the sweeper's live-pod check. The Kind run below forces restores that fail on the worker; the late-pod interleaving is covered by the adapter tests above.
Smoke test results
Kind on a Linux/KVM devbox (1 control plane, 5 workers), 2026-10-21, commit
0dd3272871ca. The bench was brought up cold at that commit. Every SPACE container ran an image built from it, checked by the harness's image-provenance check before each scenario. Image config digests:space-api368c0438d5e7,space-cluster-controller6dc60aa4a3f7,space-workerac6a6eca6861. Both the API and controller binaries contain theSPACE_ABANDONED_RESTORE_RETIREMENT_ENABLEDkey this PR adds.Each scenario ran twice. The first arm used the switch's default (unset, retirement disabled), the state this PR ships in. The second ran with
SPACE_ABANDONED_RESTORE_RETIREMENT_ENABLED=1set onspace-apiandspace-cluster-controller; the value was read back from all four running pods before the arm and after it.smoke-kind-e2e.shecw.pause_connectecw.pause_connect_racelocal.concurrent_restore_converges_to_one_executionlocal.btrfs_send_restore_rejects_corrupted_streamlocal.btrfs_send_restore_fails_closed_without_parent_sourcelocal.suspend_restore_cross_node_from_object_storagelocal.cold_sandbox_eviction_wake_round_triplocal.orphaned_restore_pod_gc_reaps_only_supersededEvery scenario reported an empty cleanup-failure list. There was no smoke for the transient-lifecycle sweeper: its contracts are crate tests.
cargo nextest run -p pplx-space-cluster-controller-server transient_lifecycle_sweeperwithSPACE_CI_REQUIRE_TEST_DEPS=1ran 117 tests, 117 passing, 50 of them Postgres-backed.Final stack state: the switch unset again on both deployments, all six nodes
Ready, zeronode_k8s_tombstonesrows, no sandbox rows, and no sandbox pods.Docs
docs/configuration.md:SPACE_ABANDONED_RESTORE_RETIREMENT_ENABLED.orphaned_restore_pod_gc/mod.rs,suspended_execution_reaper/mod.rsand the sweeper's candidate-fence doc describe retirement as conditional on the switch.suspended_execution_reaper/mod.rs: the module doc no longer says node-side idle auto-suspend leaves the execution active. Internal API's suspended projections retire it in the statement that moves the row tosuspended. The doc now says why a present pod on such a row is out of the reaper's scope.orphaned_restore_pod_gc/mod.rs: the "one orphan this loop deliberately does NOT handle" section now describes the rollback retirement and the new skip class. The reap-path text no longer says a sweeper-reverted restore's capture is refused only for lacking a source node: its execution is nowreplaced, so the fence's superseded verdict releases it.transient_lifecycle_sweeper/store.rs: the candidate-fence doc says that the revert pass checks the live pod once, before its CAS, and treats a restore pod that binds after that check as the abandoned restore's leftover.Deleted
docs/space/diagrams/orphaned-restore-pod-gc.{mmd,svg,png}. Nothing references them, and their "same-gen orphan → suspend subsystem" label no longer matches the GC.