Repository navigation
[space] fence every placement writer against the provider-terminate path - #559
the-gigi-pplx wants to merge 1 commit into
Conversation
160fcd3 to
c08e93f
Compare
c08e93f to
fde9350
Compare
b282726 to
a666308
Compare
fde9350 to
1775c65
Compare
a666308 to
918d2f3
Compare
1775c65 to
5e4129b
Compare
ac-pplx
left a comment
There was a problem hiding this comment.
[P2] Keep ambiguous receipt disappearance retryable during Direct compensation
In direct.rs, Absent with receipt_matched_before = true is treated like a successor replacing the receipt. Another attempt of the same compensation can also remove it.
If attempt A remains active after its lease becomes reclaimable, replacement B can read the same matching receipt and issue the same idempotent teardown. A can then clear the receipt but lose its claim before parent/reconcile cleanup. B observes Absent, returns Conflict, and the operation executor records a terminal failure. The sandbox row remains deleting without a retry owner; Direct has no Kubernetes orphan-cleanup backstop.
Please preserve successor protection while making this ambiguous outcome retryable, or persist evidence that the same compensation completed teardown. Add a regression covering overlapping claims and cancellation after receipt removal but before parent cleanup. Unconditionally treating absence as success would weaken successor protection.
The overlapping substrate behavior was reproduced with PostgreSQL and a simulated worker. Lease-loss cancellation and terminal error handling were separately verified in source.
6ac6790 to
aed4276
Compare
5e4129b to
c72bee1
Compare
|
Thanks — the ambiguous case was real: a second attempt of the same compensation (a stale claim reclaimed while the first still ran) clears the receipt, and the old code made that terminal with the row left After its delete RPC returns, the attempt now asks the node:
Regression coverage is Postgres-backed with a simulated worker. The same push also makes every compensation requeue spend its retry budget, so a compensation always reaches a terminal state, and adds |
the-gigi-pplx
left a comment
There was a problem hiding this comment.
Addressed in c72bee1. That head postdates the commit this review was posted against, 5e4129b.
delete_created_execution no longer treats a receipt that matched and then went missing as a successor. After its own delete RPC it probes the node. If the node reports the sandbox Missing, the teardown is confirmed and the compensation finishes and deletes the row. If the node still has the sandbox, something landed after this attempt's delete, so it stays a Conflict. That keeps successor protection, and the attempt that cleared the receipt owns the row. A node that never answers is also a Conflict, because a later attempt would find no receipt and delete by sandbox id.
Regression coverage, in controllers/sandbox/updates/compensation_tests.rs:
overlapping_compensation_attempts_finish_the_teardownforces the interleaving against Postgres and a simulated worker. It holds the receipt row so this attempt passes its receipt check and its node delete, then waits in its clear. The other attempt deletes the receipt meanwhile. Without a successor, the compensation endsTornDown, with the guest and the row gone. As a control, a successor landed on the node yieldsConflict, the guest stays and the row staysdeleting.a_compensation_cancelled_before_its_row_cleanup_is_finished_by_the_next_attemptcovers cancellation after the receipt clear but before the row cleanup: the next attempt confirms absence on the node and deletes the row.
With the 5e4129b behaviour restored (Absent after a match returns Conflict), the overlap test fails with the 409 described here. At c72bee1, both tests pass.
ac-pplx
left a comment
There was a problem hiding this comment.
[P2] Persist verification progress so failed probes retain cleanup ownership
Rechecked c72bee1. The explicit Missing response now fixes the healthy overlap case, and the new regression passes. The unanswered-probe branch in direct.rs still leaves part of the previous finding unresolved.
If the attempt that removed the receipt stops before parent cleanup, the replacement attempt must finish that cleanup. A temporary worker outage during its three state probes returns Conflict, which the executor maps to terminal failure. The parent stays deleting, and the failed operation is no longer claimable. Direct has no Kubernetes orphan-cleanup backstop. The guest can already be gone; the remaining defect is abandoned cleanup and loss of automatic recovery.
I agree that simply making this error retryable is unsafe: the next attempt sees no receipt and can issue another sandbox-ID delete against a successor. Please persist enough teardown/verification progress, or transfer ownership to an equivalent durable cleanup mechanism, so recovery can continue verification and parent cleanup without repeating that delete. Cancellation during the probe needs the same recovery contract. More in-attempt probes do not preserve this distinction across attempts.
Validation: 21 focused tests passed with PostgreSQL/Redis and simulated workers, without dependency skips. A temporary harness using the production operation store, executor, and driver reproduced three failed probes followed by terminal failed/conflict, a deleting parent, no receipt, and no reclaimable operation. It used the checked-in overlap test's locked-row stand-in for the competing attempt, then stopped the simulated worker. Aborting after the first failed probe also allowed reclaim with the original fence but no verification-progress marker. Actual simultaneous lease-loss scheduling and successor deletion were not reproduced.
Please add regression coverage for failed probes and cancellation during verification. This is an incomplete repair of the existing P2, not a new regression introduced by c72bee1.
aed4276 to
7485c7a
Compare
nikhilgupta-pplx
left a comment
There was a problem hiding this comment.
+1 to ac's latest review. The placement fencing looks sound, but the compensation handoff still loses ownership after teardown. If an overlapping attempt clears the receipt and is cancelled before parent cleanup, failed worker probes permanently fail the operation and leave the Direct row in deleting; cancellation also forgets that a retry must only verify. Please make that phase durable, keep inconclusive probes retryable, and cover failure and cancellation through the operation driver.
Separately for rollout: #558 needs to land first, and since old API replicas can fail reads once new-kind rows exist (including terminal history), the rollback boundary needs to be explicit rather than relying on controller-first ordering alone.
| }); | ||
| } | ||
| None => { | ||
| return Err(SubstrateError::Conflict { |
There was a problem hiding this comment.
[P2] Keep ownership of the verification-only cleanup durably. If an overlapping attempt clears the receipt and is cancelled before deleting the parent row, three unavailable worker probes here become Conflict, which permanently fails the compensation operation and strands the Direct row in deleting. Cancellation also loses receipt_matched_before, so a reclaimed attempt takes the sandbox-ID delete path again. Please persist the post-teardown verification phase, retry inconclusive probes, and cover both failed probes and cancellation during verification through the operation driver.
dfee18d to
8493580
Compare
nikhilgupta-pplx
left a comment
There was a problem hiding this comment.
The durable teardown marker addresses my blocker: it commits with receipt removal under the current operation claim, and reclaimed attempts only verify. The driver tests cover retry, cancellation, and preserving a successor receipt.
LGTM. Keep the documented rollout/rollback ordering, and get ac’s re-review before landing.
5651fb6 to
47c28cc
Compare
47c28cc to
1c30bc2
Compare
|
Thanks for the driver-level reproduction. This is addressed at 1c30bc2: verification progress is now durable, so a replacement attempt continues verification and parent cleanup without repeating the delete, and an unanswered probe is retryable. Persisted progress. After its node delete returns, an attempt records Recovery only verifies. The executor passes the claim and the recorded flag through (public_sandbox_operations.rs; dispatcher). An attempt that finds the record takes Cancellation during verification. The record always commits before any probe, so an attempt cancelled during its probe leaves the record, no receipt, and the row in Regression coverage, through the operation driver with the production store and executor, Postgres and simulated workers (compensation_tests.rs):
What's left: an attempt that ends between its node delete returning and that record committing leaves nothing recorded. With a receipt, the next attempt finds it still current and repeats the fenced delete; on the legacy path it repeats the sandbox-id delete, as before. It's listed in the description. |
Summary
A sandbox placement could commit onto a node that had already been moved to
drainedorterminating, so the sandbox died with the instance. The phase writers lock thenodesrow, butsandboxes.active_node_idhas no foreign key tonodes, and its writers never took that lock. The pre-terminate recount inhandle_terminating_nodenarrowed the window but couldn't close it: a placement could still land between the recount and the provider call.This PR orders the two sides on the
nodesrow. Every live writer ofactive_node_idnow takes the placement half of a termination fence, and the terminate path takes the other half, so a placement either commits first and is counted, or reads the committed phase and refuses.The fence orders database placement commits against termination. It doesn't close runtime admission: a pod or VM can already be running before its placement is recorded, which is why a refused placement has to tear down what it can't record.
The mechanism
The whole protocol lives in
crates/libs/common/svc/src/sql/node_placement_fence.rs:fence_node_for_placement)SELECT drain_phase … FOR KEY SHAREactive_node_id.lock_node_for_phase_write)SELECT 1 … FOR UPDATEin its own statement, with a transaction-locallock_timeout, then the guarded phase writeFOR KEY SHAREis the weakest lock that works. It conflicts with a phase write'sFOR UPDATEand with node deletion, but not with other placements or with heartbeat and inventory traffic.FOR KEY SHAREre-reads the lockednodestuple fresh after a wait, but evaluates the rest of its statement against the pre-wait snapshot. A carve-out subquery oversandboxesin the same statement would be stale.drained,terminating, an unrecognized phase, and a missing node row. It leavesdrainingopen: by then a pod may already exist, and recording it is what makes the sandbox visible to node-drain's evacuation.What changed
pooled_post_wake(controllers/sandbox/responses.rs),post_create_placementandcreate_finalize(controllers/sandbox/queries.rs),pooled_create(insert_pooled_sandbox, which fences before callingpplx_space_sandbox_store::insert_pooled),direct_restore(controllers/substrate/direct.rs), andpod_placement(controllers/substrate/kubernetes/store/pod_placement.rs). Writer-side policy and the refusal metric live incontrollers/placement_fence.rs.node_drain::store'smark_terminatingpath takes the shared preamble and treats lock contention as a one-tick deferral.drained_node_terminatorandorphaned_nodestake the same lock before their phase writes and deletes.RestoreInFlight, which keeps cleanup ownership with the caller.deletingbefore it returns, including a redrive that joined an abandoned activation.deletingrow to it, and doesn't offer the client a retry of that sandbox.restoring): the claim doesn't cover it. On the request path, before returning, the guard deletes the pod (UID-fenced), waits until the apiserver no longer has it, and only then retires the claim, so create cleanup deletes the dead shell. A pod that outlives its delete keeps the claim, and the create reportsRestoreInFlightinstead of the placement error, so create cleanup keeps the row with it for the transient-lifecycle sweeper rather than leaving a live pod with neither.RestoreInFlight, so create cleanup keeps the row the pod's cleanup goes through. When ownership moved on, the guard leaves the pod alone.creatingrow for a retried create.rollback_created_sandboxmoves the row todeletingand queues acompensate_create_sandboxoperation, already claimed, in one transaction, before touching the substrate. The request then runs the first attempt itself, through the driver and executor the dispatcher uses, and waits for it as main waited for its teardown. So the teardown also runs withSPACE_PUBLIC_OPERATION_DISPATCHER_ENABLED=false. The dispatcher retries a failed attempt and reclaims one whose process died.deletingbelongs to whatever took it over, and the compensation declines without touching the substrate.compensate_create_sandboxnever claims one. The newcreate_rollback_fencepayload key is skipped when absent, so user lifecycle payloads serialize as before.OperationKindlists it after aninternalmarker, which keeps it out of the published schema, so the M1 contract is unchanged.public_operation_driver. Every requeue spends it, whatever the reason, including the workflow-progress and backpressure requeues that don't for other kinds. Compensation isn't an M1 kind and has no deadline, so the budget is what guarantees it reaches a terminal state.public_operations_kind_check.orphaned_sandboxesclaims abandoned creates reached through the Forgotten guard as well as the pod-name guard.docs/configuration.md, and turned off only by a recognized false value. Set each on the API and the cluster controller together.SPACE_PLACEMENT_TERMINATION_FENCE: disabling removes the row lock as well as the check.SPACE_CREATE_COMPENSATION_QUEUE: off, a failed create claims its sandbox and tears it down immediately, with no operation to retry it, and writes nocompensate_create_sandboxoperation. A teardown that fails then leaves the rowdeleting, and on the direct substrate nothing else collects it or its guest. Both processes run creates, which is why both need it.placement_fence.refusals{writer,verdict}counts refusals, andplacement_fence.termination_contention_deferralscounts terminate-side deferrals after a bounded lock wait. Compensation's confirmed pod deletes report through the existing blocking-delete latency metric with operationcreate_compensation.DbContext, so a lost lock race there stays the retryableLockContention.sandbox_writescaps inarchitecture-baseline.toml:controllers/sandbox/queries.rs4 to 5,kubernetes/store/pod_tracking.rs5 to 7, andorphaned_sandboxes.rs5 to 7. That's the ratchet's documented exception for another write of thesandboxesrow in a listed file, and archcheck reports it as advisory. The new writes are create cleanup's shell delete, the cancelled-create claim and its row delete, andorphaned_sandboxes' claims of abandoned creates. They stay in those files rather than moving intopplx_space_sandbox_storein this PR.scripts/source_guards/placement_fence_writers.pyfinds every production SQL statement that assigns a non-NULLsandboxes.active_node_id, requires a fence call in the same function, and pins the site count at six.FENCED_BY_CALLER), and fails if a second caller appears.completion_sqlincrates/libs/placement/machine, which has no production caller.scripts/tests/test_placement_fence_writers_guard.pyin the repo's Python test discovery.Dependencies
public_operations_kind_check, which listed the operation kinds and notcompensate_create_sandbox; with it in place, every compensation insert would fail. This PR targetsmainand is rebased onto that merge.Rollout order
Deploying the API ahead of the controller opens two windows that the reverse order doesn't:
compensate_create_sandbox, so a compensation whose inline attempt fails waits indeletingwith its guest running.DELETEs don't take the phase lock first. A delete that waits behind a fenced placement then evaluates its residency check on the snapshot from before the wait.An older API can't decode a
compensate_create_sandboxrow either. On an older replica during rollout, or after a rollback, operation fetch and the admin listing fail while such a row exists, terminal rows included. Claims aren't affected, since they select by kind.Rolling back
A rollback has to drain the new operation kind before it removes the code that runs it, and PR #558's down migration refuses while any
compensate_create_sandboxrow exists. In order:SPACE_CREATE_COMPENSATION_QUEUE=falseon the API and the cluster controller. Failed creates then tear down immediately and write no new rows of the kind.gc_afteris set when the operation is inserted, 7 days after its creation, not after it completes.Rolling the cluster controller back before step 2 finishes strands the queued compensations. An older controller never claims the kind, and on the direct substrate nothing else tears down their guests.
Deliberately not covered
On a direct compensation whose route receipt disappeared during the teardown, a successor that lands on the node after the compensation's state check isn't seen. With the receipt gone and the node empty at the check, the compensation reports its teardown done.
When two attempts of one compensation overlap, the one that holds the claim clears the receipt and deletes the row, even when the stale one found a sandbox on the node and reported a conflict. The stale one's result is dropped. A successor that landed between the two deletes can be left without a row.
An attempt cancelled after its node delete returns but before its teardown record commits leaves nothing recorded. With a receipt, the next attempt finds it still current and repeats the fenced delete. Without one, the next attempt repeats the sandbox-id delete, as before this change.
A created pod whose create response carries no valid UID can't be named for a fenced delete. Its row and execution claim are kept for reconciliation, but the create returns a retryable 503.
A direct compensation's node delete names the sandbox, not the execution. A successor activation that landed on the same node before that call is deleted with it. The compensation then leaves the successor's receipt and row alone and stops with a conflict, so they point at a guest that's gone.
Several paths leave the last step to the orphaned-sandbox reconciler, and that reconciler acts only with
SPACE_API_RECONCILER_DRY_RUN=false; the default is dry-run. The paths:deletingrow an earlier placement already attached to a node;creatingshell;In dry-run, these stay
deletingorcreatinguntil someone acts by hand.A pod scheduled before the drain began keeps running, since cordon only stops new scheduling. Until its placement persists, the row carries a NULL
active_node_idand is invisible to every occupancy term. The fence makes the later placement refuse instead of recording a healthy sandbox on a dying node, but the guest on that node is lost either way.A create refused for a missing node row (Direct substrate) can't reach the node to tear down, so its compensation retries to budget and leaves the row in
deleting.An action already in flight can still overwrite
deletingwhen its result lands during the compensation's teardown call. Compensation checks ownership before the call, but no lock spans it. That post-result write replaces cross-intent lifecycles in general and is characterized as open inlifecycle/characterization_tests.rs.With the dispatcher disabled, a compensation gets only the attempt its request runs. A failed attempt stays queued, with its row in
deleting.A Kubernetes pod that stays
Terminating(a finalizer, or a kubelet that never reports back) fails every confirmed delete, so its compensation retries to budget and leaves the row indeletingwith its execution still tracked.An ordinary cancel guard's claim retirement is fenced on generation and attempt, not lifecycle, as on main. The lifecycle check applies only on the durable path.
The four controller-side writers don't classify a lock-timeout error (
55P03) from the fence; the two substrate writers do. This has no effect while the app role has nolock_timeout. The coupling is recorded at the statement.Test plan
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --locked -- -D warningscargo nextest run -p pplx-space-api -p pplx-space-cluster-controller-server -p pplx-space-common-svc -p pplx-space-metrics -p pplx-space-public-api -p pplx-space-db-test-fixtures: 3314 passed, 1 skipped, against Postgres with the PR [db] drop public_operations_kind_check; the app owns the kind set #558 migration applied.compensation_leaves_a_sandbox_that_left_deleting_untouched,create_rollback_tears_down_without_a_dispatcher,create_rollback_falls_back_to_a_claimed_teardown_when_queueing_fails,compensation_claim_declines_a_successor_execution,create_compensation_deletes_its_pod_by_uid_after_the_row_is_gone,create_compensation_releases_tracking_only_once_the_pod_is_gone,cancel_guard_durable_cleanup_retires_tracking_only_once_the_pod_is_gone,owned_restore_fence_contention_is_retryable,overlapping_compensation_attempts_finish_the_teardown,create_rollback_queues_nothing_with_the_queue_switch_off, andcompensation_requeues_always_spend_the_retry_budget,an_unconfirmed_teardown_stays_retryable_and_is_only_verified,a_compensation_cancelled_after_its_teardown_is_only_verified_on_reclaim,a_stale_compensation_claim_clears_no_receipt,a_legacy_teardown_records_itself,claim_renewal_does_not_wait_on_the_operations_row_lock, andcancel_guard_release_now_retires_before_returning. The second and third run through the operation driver with the production store and executor: one forces a teardown whose probes fail (the worker is stopped mid-attempt) and asserts the operation is requeued with its teardown recorded, then that a retry succeeds once the node answers; the other cancels an attempt between its teardown and its row cleanup and reclaims it. Each has a control where a successor landed on the node and must survive the retry.overlapping_compensation_attempts_finish_the_teardownforces the interleaving: it holds the route receipt row, so one attempt passes its receipt check and node delete and then waits in its receipt clear, and the test commits the other attempt's clear while it waits. Its control has the node still holding the sandbox, which must stay a conflict.a_compensation_cancelled_before_its_row_cleanup_is_finished_by_the_next_attemptcovers an attempt cancelled between its receipt clear and its row delete. That case takes the absent-receipt arm, which already handled it, so the test guards that path rather than this round's probe. The fallback's identity check is tested through the claim it shares with the queued path, not through a successor racing the fallback itself. The pod-placement fence's classification has no test: that writer locks the node row before the fence reads it, so the read doesn't wait on a lock there.node_drain/store/placement_fence_tests.rsinterleave two live transactions, withpg_blocking_pidsas the barrier. They cover a placement committing first, a phase write committing first, and a node delete committing mid-placement.controllers/sandbox/queries/placement_fence_tests.rspair each refused case with an open-node case, so a writer that stopped working can't pass as fenced.python -m pytest scripts/tests/test_placement_fence_writers_guard.py: 35 passed. Each new guard test fails with its mechanism reverted.