Skip to content

[restore] retire an abandoned restore's execution in the revert that abandons it - #545

Merged
the-gigi-pplx merged 1 commit into
mainfrom
gigi/pla-4949-reap-the-same-generation-interrupted-restore-orpha
Oct 21, 2026
Merged

the-gigi-pplx merged 1 commit into
mainfrom
gigi/pla-4949-reap-the-same-generation-interrupted-restore-orpha

Conversation

@the-gigi-pplx

@the-gigi-pplx the-gigi-pplx commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

When a restore fails and is rolled back to suspended, the rollback now retires the execution the restore created as replaced, 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 a suspended row: 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 a suspended row, 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 a restoring sandbox'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

  • API restore rollback (crates/libs/api/src/controllers/substrate/kubernetes/store/restore_lease.rs, revert_restore_claim_to_suspended). This replaces revert_lifecycle_after_rollback_best_effort.
    • Locks the sandbox row in its own statement and requires it to still be restoring under the claim's lease token. The tokenless evacuation call also requires restoring now. The proof and the claim's execution are read after the lock, and the retirement targets the identified generation.
    • Identifies the execution the claim created as the active one created since the claim's restoring transition that still carries the claim's activation attempt and generation. A redrive of the same restoring row 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.
    • Under the proof, retires that execution as replaced, invalidates open port exposures, and unplaces the row (placement = 'unplaced', active_node_id = NULL). It keeps target_node_id and the artifact marker, which name the wake source.
    • The caller (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.
  • Transient-lifecycle sweeper revert (crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/store.rs). The revert CAS now runs in a transaction with the execution's retirement as replaced.
    • The retirement matches the identity the candidate select recorded: pod name, generation, activation attempt, pod UID, route activation.
    • A retiring revert also unplaces the row in the same transaction.
    • A candidate observed with no execution reverts only while none exists. If the identity no longer matches, nothing reverts.
    • The existing retire_execution cleanup then finds no active execution, deletes the inactive rows' pods and unplaces the row.
    • A restore pod that binds and comes up after the sweeper's live-pod check is deleted rather than allowed to recover. Before, the cleanup's resourceVersion check refused and the revert was undone. Its row never reached 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 row suspended with its execution active.
  • No user request reaches a restoring sandbox's pod. Before, the access path and runtime_client dialed a running execution's pod while the row was still restoring, 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 is started, bounded by the existing restore deadline, and otherwise fails with a retryable error. A path that doesn't recover reports the runtime unreachable instead.
    • ResolvedPodInstance carries sandbox_restoring (pod_ops/read.rs). The probe that resolves a reachable pod IP returns none for a restoring row.
    • runtime_client and access_runtime_client (adapter/substrate_adapter.rs), on both the cached fast path and the fallback, call join_restore_before_dial (execution.rs). It joins the in-flight restore first, then dials the execution the row routes to once the row is started. It reclaims, within what's left of the caller's budget, only a row that's still restoring after 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 to suspended gets RestoreInFlight instead of starting a fresh restore that its caller's access-wake policy and admission never approved. A row that's already started when the join re-reads it is dialed at once. A drain's evacuation restore owns its restoring row through the evacuation marker, not a lease, so the row reads as unowned. The request waits for the drain to move it to started and drives no wake of its own. A restore that doesn't reach started in time fails as RestoreInFlight (retryable 503). A row that leaves for a state that can't be woken (failed, deleting, …) gets that state's error.
    • A new lifecycle writer: the restore's owner commits started. The wake that owns a user restore (it holds the restore lease token and targets started) runs restoring → started itself 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 be restoring with 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, started came 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 stayed restoring until the node publication converged it, and with the gate its request couldn't be served until then. Once the row is started, neither the API rollback nor the sweeper's revert, both restoring-guarded, can move it back. The access and runtime_client paths that wake a suspended sandbox dial only once the row is started too. The lifecycle controller's completion CAS still runs and finds the row started. When the commit lands, the restore's detached task emits RestoreCompleted from 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 row started. The lifecycle controller's Wake, Restore and Start actions still emit their own completion events from the API. So an explicit Restore now reports RestoreCompleted twice, once from each source, as wakes already report WakeCompleted from 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 is started (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.
    • A started-recovery follower (observe_started_recovery, store/started_recovery.rs) reports success only for a running execution on a started sandbox. 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 is restoring.
    • The Native failover in native_kept_worker_client (wait_ready.rs) doesn't follow a re-resolved pod whose sandbox is restoring. 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).
    • Ingress reaches the access path through the same gate.
    • On a restoring sandbox, the calls that resolve the runtime through runtime_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.
    • The internal API's service and session route resolvers (data_plane_sessions/{service_route,session_route}.rs) wait on a restoring row through the activation owner and refuse it with a retryable Unavailable otherwise, 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.
  • HTTP suspend's trailing lifecycle write (crates/libs/api/src/controllers/sandbox/lifecycle.rs).
    • The controller wrote suspended after suspend_execution returned, guarded only against terminal lifecycles. A wake that claimed the row in between, and reached started, was put back to suspended with its new execution active.
    • A new SubstrateCapabilities::suspend_commits_lifecycle (Kubernetes: true, Direct: false) skips that write where the substrate's suspend already committed suspended under its execution fence. Direct still relies on the write and keeps it.
  • Orphaned-restore-pod GC (orphaned_restore_pod_gc/classify.rs, reconciler.rs). A same-generation pod on a suspended row is still never reaped. It's now its own skip class, suspended_same_generation, instead of active_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 clock on revert (both revert writers). Binding a restore pod moves placement_changed_at past the pre-restore snapshot.
    • The revert proof accepts that source anyway.
    • Two checks take a markerless snapshot only when it's newer than the last placement:
      • the suspended restore selector, which found no remote source after a revert;
      • 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 against teardown_stuck.
    • Both reverts now clear placement_changed_at under the proof, as the markerless restoring recovery in pod_tracking.rs already 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.
    • The sweeper's undo puts back the value the revert read under the row lock, so a restored restoring row keeps the clock its retry window reads.
  • Rollout gate (AbandonedRestoreRetirement, crates/libs/api/src/controllers/substrate/mod.rs). SPACE_ABANDONED_RESTORE_RETIREMENT_ENABLED, default off; only an explicit truthy value turns it on.
    • The API reads it into KubernetesState and passes it to revert_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 is execution_left_active.
    • The cluster controller reads it into the sweeper's ReconcilerConfig, and SubstrateSweepSink::with_abandoned_restore_retirement takes 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.
  • Metric: 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: the sandbox_writes caps for restore_lease.rs (5 to 8) and the sweeper's store.rs (10 to 12). The new writes are guarded lifecycle transitions, which stay out of the sandbox store.

Test plan

  • cargo fmt --all -- --check
  • cargo clippy --workspace --locked --all-targets --all-features -- -D warnings
  • Full workspace nextest with CI's arguments (--run-ignored all --ignore-default-filter --retries 1) on a Linux host:
    • the unprivileged inventory as an ordinary user;
    • the group(privileged) tests as root from an archive of the same build, inside ci/scripts/with-nocow-test-filesystem.sh;
    • workspace doctests.
    • Three privileged node-runtime-core fs-verity tests need a kernel with fs-verity, which this host's kernel lacks, so they fail there as they do at main. CI's privileged lane runs them.
  • New tests:
    • 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_once and revert_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 stays suspended, and restore still selects the durable pre-restore snapshot.
    • The API revert test also checks that every reverted row with a proven source still selects it, and is durably_restorable, after the bind moved its placement.
    • adapter_characterization_tests/restoring_access.rs (Kubernetes adapter, scripted worker): access and runtime_client on a restoring sandbox join the restore and dial only once it's started, and are refused retryably when it lingers. reachable_runtime_client finds 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 a suspended sandbox and joins an in-process restore run that completes before the row is started is 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) and without_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, a restoring row and a routable execution, and a rollback after it finds nothing to revert.
    • an_owned_restore_commits_started_before_it_returns and a_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 the started commit) and a_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_restore and session_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.
  • Each new guard was removed one at a time, on the head that introduced it, and its test failed: 51 mutations, 24 of them on the restoring gate and the owner's commit, and 3 on the retirement switch. Two more are equivalent. Dropping the owner commit's lifecycle = 'restoring' guard changes nothing, because the lease-clear trigger nulls the token on any row that leaves restoring. Skipping the join's wait on the shared restore poller changes no outcome, because the reclaim's lease claim refuses a live owner and the started wait 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-api 368c0438d5e7, space-cluster-controller 6dc60aa4a3f7, space-worker ac6a6eca6861. Both the API and controller binaries contain the SPACE_ABANDONED_RESTORE_RETIREMENT_ENABLED key 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=1 set on space-api and space-cluster-controller; the value was read back from all four running pods before the arm and after it.

Suite Switch off Switch on What it exercised
smoke-kind-e2e.sh PASS 7/7 not rerun create, start, file API round trip, delete
ecw.pause_connect PASS PASS suspend, then a connect that restores the sandbox and reads its state back
ecw.pause_connect_race PASS PASS ten suspend and immediate-reconnect cycles, each routed into the restore it triggers
local.concurrent_restore_converges_to_one_execution PASS PASS twenty connects over three API replicas join one restore; pod POSTs refused at admission retire their generations; a delete during a refused restore leaves nothing
local.btrfs_send_restore_rejects_corrupted_stream PASS PASS a restore that fails on the worker (corrupted stream) is rolled back, with nothing left on the target
local.btrfs_send_restore_fails_closed_without_parent_source PASS PASS a restore that fails at the template lookup is rolled back, with nothing materialized
local.suspend_restore_cross_node_from_object_storage PASS PASS restore onto another worker from object storage; state and execution survive
local.cold_sandbox_eviction_wake_round_trip PASS PASS eviction of a backed suspended sandbox, then a wake that restores from object storage
local.orphaned_restore_pod_gc_reaps_only_superseded PASS PASS the GC reaps superseded restore pods and keeps the one a live restore needs

Every 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_sweeper with SPACE_CI_REQUIRE_TEST_DEPS=1 ran 117 tests, 117 passing, 50 of them Postgres-backed.

Final stack state: the switch unset again on both deployments, all six nodes Ready, zero node_k8s_tombstones rows, 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.rs and 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 to suspended. 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 now replaced, 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.

@the-gigi-pplx
the-gigi-pplx force-pushed the gigi/pla-4949-reap-the-same-generation-interrupted-restore-orpha branch from 5d1fcd1 to a58251d Compare October 15, 2026 21:32
@the-gigi-pplx
the-gigi-pplx force-pushed the gigi/pla-4949-reap-the-same-generation-interrupted-restore-orpha branch 6 times, most recently from 92e89ab to 70a6a43 Compare October 20, 2026 03:13
@the-gigi-pplx
the-gigi-pplx marked this pull request as ready for review October 20, 2026 03:33

@ac-pplx ac-pplx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@the-gigi-pplx
the-gigi-pplx force-pushed the gigi/pla-4949-reap-the-same-generation-interrupted-restore-orpha branch from 70a6a43 to 252dce8 Compare October 20, 2026 04:41
@the-gigi-pplx

Copy link
Copy Markdown
Collaborator Author

Thanks, all three hold at 70a6a43. Fixed in 252dce8, each with a test that failed before its fix:

  1. Started-recovery followers. observe_started_recovery now reads the sandbox's lifecycle alongside the execution. A running execution counts as success only when the sandbox is started. While it's restoring, the observation stays Pending, so the follower keeps waiting inside its original deadline. On any other lifecycle it's Failed, so the existing suspend-supersession handling takes over. The fix is at the observation, so it covers both callers of the join: ensure_execution_ready_for_access and the started-recovery join in drive_prepared_wake. Test: started_recovery_followers_succeed_only_on_a_started_sandbox puts a running generation 2 behind a follower of generation 1, with the sandbox started, restoring and suspended. Before the fix, the follower reported success for all three.

  2. Session route refresh. update_route_row now refuses the refreshed row with the same retryable Unavailable as the first read when it comes back restoring. That happens before anything is signed. I couldn't make the interleaving between the two statements deterministic: both read sandboxes.lifecycle, and nothing can run between them from SQL. So the test, a_route_refresh_refuses_a_sandbox_that_started_restoring, takes the first read while the sandbox is started, commits restoring, then drives update_route_row. Before the fix it returned the restoring row.

  3. Late suspend-recovery branch. Both suspend-supersession branches now go through redrive_suspended_recovery_for_access: the one at the claim and the late one after readiness fails. It runs the redrive, then returns binding_once_started(sandbox_id, join_deadline). Test: a_redriven_restore_hands_out_no_binding_until_it_is_started installs a test-only trigger that fails the restoring → started commit, the delayed or failed commit you described. Before the fix the redrive returned the binding with the row still restoring. Now it returns RestoreInFlight.

@the-gigi-pplx
the-gigi-pplx requested a review from ac-pplx October 20, 2026 05:20
@the-gigi-pplx
the-gigi-pplx force-pushed the gigi/pla-4949-reap-the-same-generation-interrupted-restore-orpha branch from 252dce8 to e70a73a Compare October 20, 2026 12:24

@kumarpallav-pplx kumarpallav-pplx left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@the-gigi-pplx
the-gigi-pplx force-pushed the gigi/pla-4949-reap-the-same-generation-interrupted-restore-orpha branch from e70a73a to 985f698 Compare October 21, 2026 03:04
@the-gigi-pplx

Copy link
Copy Markdown
Collaborator Author

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, SPACE_ABANDONED_RESTORE_RETIREMENT_ENABLED. The API and the cluster controller both read it, and only an explicit truthy value turns it on. While it's off, both writers behave as on main:

  • The API rollback (revert_restore_claim_to_suspended) reverts only the lifecycle, whatever the snapshot proof says. The execution, placement and placement clock stay as the restore left them, and no pod is deleted (execution_left_active).
  • The sweeper's revert runs the lifecycle-only CAS. Its cleanup then retires the still-active execution under the candidate's fence. A restore pod that came up after the live-pod check, at a newer resourceVersion, is refused rather than deleted, and the revert is undone, as before this PR.

The routing guards, the owner's started commit and the HTTP suspend fix are active from deploy.

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 docs/configuration.md and on the switch's doc comment.

Tests for the interleaving:

  • without_retirement_the_sweeper_cleanup_keeps_a_restore_pod_that_came_up_after_its_check (Kubernetes adapter): after the lifecycle-only revert, the restore pod came up after the check, as if a pre-guard replica had routed a write to it. The cleanup's fence refuses, nothing is deleted, and the execution stays active.
  • without_retirement_the_revert_moves_only_the_lifecycle (sweeper store) and without_retirement_a_failed_restore_revert_moves_only_the_lifecycle (API store): with the switch off, a proven revert leaves the execution active and the row placed, with its placement clock unchanged.

Forcing retirement on in either writer while the switch is off fails its test.

@the-gigi-pplx

Copy link
Copy Markdown
Collaborator Author

@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.

@the-gigi-pplx
the-gigi-pplx force-pushed the gigi/pla-4949-reap-the-same-generation-interrupted-restore-orpha branch from 985f698 to 0dd3272 Compare October 21, 2026 14:06
@the-gigi-pplx
the-gigi-pplx force-pushed the gigi/pla-4949-reap-the-same-generation-interrupted-restore-orpha branch from 0dd3272 to 548cb3b Compare October 21, 2026 20:10
@the-gigi-pplx
the-gigi-pplx added this pull request to the merge queue Oct 21, 2026
Merged via the queue into main with commit cc3848b Oct 21, 2026
27 checks passed
@the-gigi-pplx
the-gigi-pplx deleted the gigi/pla-4949-reap-the-same-generation-interrupted-restore-orpha branch October 21, 2026 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants