Pause and preserve checkpoint on deployment timeout, add manual resume#428
Merged
Conversation
A deployment that exceeds the wall-clock timeout previously transitioned
to Failed and had its checkpoint deleted, discarding every completed
batch and forcing a restart from scratch. It now pauses (Paused) with
its checkpoint intact and can be resumed from the last completed batch
via POST tasks/{id}/resume, reusing the existing checkpoint-resume
machinery (StartExecutingAsync Paused->Executing + ResumeCheckpointPhase).
The historical fail-fast behaviour (Failed + checkpoint deleted) stays
available behind SQUID_DEPLOYMENT_TIMEOUT_RESUMABLE=false for operators
who alert on the terminal Failed state rather than the resumable Paused
state.
This was referenced Jun 11, 2026
ppXD
added a commit
that referenced
this pull request
Jun 11, 2026
* Pause (resumable) on a transient infra failure instead of failing A Halibut RPC drop that outlives the library's own retries, or an agent that goes unreachable mid-script, used to fail the deployment terminally AND — via the strategy's finally that cleared the in-flight pointer on any exit — destroy the reattach pointer, so a resume re-dispatched a duplicate of the still-running script. Classify a transient infrastructure failure (HalibutClientException / AgentUnreachableException, or an AggregateException of only those) as a PAUSE: the task transitions to Paused with its checkpoint AND in-flight pointer preserved, and a resume re-attaches to the still-running script. Reuses the #428 pause/resume machinery; env-gated by SQUID_DEPLOYMENT_TRANSIENT_RESUMABLE (default true) so operators can opt back into fail-fast. - HalibutMachineExecutionStrategy: clear the in-flight pointer ONLY on a definitive observation (return), preserve it on a throw. - TargetCatchClassifier: a transient infra failure leaves the target in-flight (not terminal) and does NOT fail-fast healthy peers — they finish, then the deployment pauses. Owns the shared IsTransientInfraFailure. - DeploymentPipelineRunner: a transient failure routes to OnTransientPauseAsync (Paused + checkpoint preserved), not OnFailureAsync; does not rethrow. - DeploymentCompletionHandler: OnTransientPauseAsync (mirrors OnTimedOutAsync). A genuine (non-transient) exception, and a transient drop racing a real cancel/timeout, still fail terminally. Tests: - Unit: TargetCatchClassifier transient cases + IsTransientInfraFailure matrix; DeploymentPipelineRunner transient->pause vs fail-fast vs genuine failure, aggregate-of-transients, env-var pin + parse matrix - Integration: HalibutResumeReattach observe-throws-transient preserves the in-flight pointer for resume * Harden transient-pause classification (adversarial review fixes) Adversarial review of the transient-pause change found five reachable paths that defeated the resumable-pause guarantee. Fixes: - Centralise the transient definition in TransientFailureClassifier (Squid.Core.Halibut.Resilience) so the pipeline runner, per-target classifier, per-action catch, and the re-attach probe all agree. - NARROW the predicate: a permanent Halibut protocol/invocation failure (ServiceInvocation / NoMatchingServiceOrMethod / MethodNotFound / ServiceNotFound / AmbiguousMethodMatch) is NOT transient — classifying it so would pause-loop forever. Base/transport HalibutClientException, AgentUnreachableException, and CircuitOpenException ARE transient. - Non-required step: a transient now propagates (rethrow before the per-target/terminal failure recording) regardless of step.IsRequired — previously it was swallowed into FailureEncountered -> OnFailureAsync, which deleted the checkpoint AND the preserved in-flight pointer, orphaning the still-running script. - Re-attach probe: a TRANSIENT probe failure on resume now preserves the pointer and propagates (was: type-blind catch cleared it and dispatched fresh -> duplicate-execution risk when the agent recovers). - Runner transient catch is guarded by !timeoutCts && !registryCts && !ct so a user-cancel / wall-clock timeout racing a transient RPC drop is not reclassified as a pause (cancel/timeout win). - CircuitOpenException is treated as transient (its own contract) so a tripped breaker on resume pauses + preserves the pointer instead of forcing a terminal Failed. Tests: - Unit: TransientFailureClassifier matrix (permanent subtypes excluded, CircuitOpen/AgentUnreachable/base included, aggregate all/mixed/empty); runner transient-during-user-cancel does NOT pause - Integration: reattach-probe transient failure preserves the pointer and does not dispatch fresh * Make transient-pause unconditional (drop env-var toggle) + real-phase tests Remove the SQUID_DEPLOYMENT_TRANSIENT_RESUMABLE escape hatch added earlier in this PR: pausing-resumable on a transient infra blip is the only correct behaviour (failing fast would discard already-completed progress and risk a duplicate run — no legitimate use case), so it needs no opt-out. The env var never shipped, so removing it is non-breaking. The wall-clock-timeout toggle (SQUID_DEPLOYMENT_TIMEOUT_RESUMABLE, shipped in 1.8.18) is unaffected. The runner's transient catch keeps its cancel/timeout guard (!timeoutCts && !registryCts && !ct) so cancel/timeout still win. Tests: - Drop the env-var pin / parse-matrix / fail-fast tests (no longer apply) - Add real-ExecuteStepsPhase propagation tests (DeploymentExecutionLoggingTests.TransientStrategyFailure_PropagatesOutOfPhase_RegardlessOfRequired): a throwing strategy on a step proves a transient PROPAGATES out of the phase for BOTH required AND non-required steps (closing the review's non-required-swallow gap end-to-end), and FailureEncountered stays false; the existing StepFailed_DoesNotLogCompleted pins the contrast that a non-required GENUINE failure is still swallowed * Exclude CircuitOpenException from transient (keep fail-fast-on-open-breaker) The K8s E2E HalibutCircuitBreakerE2ETests.BreakerForcedOpen_* asserts an open breaker fails the deployment fast (Failed) — a deliberate production-safety invariant. Classifying CircuitOpenException as transient (from the review's HIGH #5 suggestion) flipped that to Paused and broke it. Excluding it is the more correct design, by the same principle as the permanent-Halibut-subtype exclusion: the per-machine breaker only opens after 3+ consecutive failures — a SUSTAINED agent problem, not a one-off blip — and is raised BEFORE any script is dispatched (fail-fast), so there is no in-flight script to re-attach to. Pausing on it would loop on a dead agent. The narrow resume-with-open-breaker orphan #5 described is better addressed by probe-before-breaker (a separate focused change), not by this blunt reclassification. The other four review HIGH fixes (non-required propagation, narrowed predicate, reattach-probe preserve, cancel/timeout guard) are unchanged. Test flipped: CircuitOpen_IsNotTransient.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Paused) with its checkpoint preserved instead of transitioning toFailedand deleting the checkpoint. The already-completed batches are no longer thrown away.POST /api/tasks/{id}/resume→ResumeServerTaskCommand→ServerTaskControlService.ResumeTaskAsync— that re-dispatches the paused deployment. It reuses the existing, proven resume machinery:StartExecutingAsyncalready transitionsPaused → Executing(IsResumed), and the state-agnosticResumeCheckpointPhase(Order 50) restores progress, so resume skips completed batches.Failed+ checkpoint deleted) stays available behindSQUID_DEPLOYMENT_TIMEOUT_RESUMABLE=falsefor operators who alert on the terminalFailedstate.Design notes
Pausedis the only resumable state (Paused → Executingis the lone valid resume transition;Failed/TimedOutare terminal). So a resumable timeout must land inPaused— henceOnTimedOutAsynctransitionsExecuting → Paused, keeps the checkpoint (it is the resume point), and writes noDeploymentCompletionrecord (a paused deployment hasn't completed).ResumeTaskAsyncis the strict, operator-facing sibling of the opportunisticTryAutoResumeAsync(which fires after an interruption response). Both now share oneEnqueueResumeAsync. Unlike the auto path,ResumeTaskAsyncsurfaces every precondition failure as a typed exception —ServerTaskNotFoundException,ServerTaskStateTransitionException(task not paused),ServerTaskAwaitingInterruptionException(resume a guided-failure pause by submitting the interruption, not here) — so the API returns an actionable error rather than a silent 200.Failed → Paused) is the requested safe-default flip and is reversible via the env var.Test plan
DeploymentCompletionHandlerTests:OnTimedOutAsynctransitionsExecuting → Paused, does not delete the checkpoint, does not record a completion, does not trigger auto-deploys.DeploymentPipelineRunnerTimeoutTests: timeout routes toOnTimedOutAsyncby default andOnFailureAsyncwhen fail-fast;DeploymentTimedOutEventemitted in both modes; cancel still wins over timeout;SQUID_DEPLOYMENT_TIMEOUT_RESUMABLEconst pinned + 15-case parse matrix + env→property wiring.ServerTaskControlServiceResumeTests: paused+no-interruption enqueues + persists new job id; not-found / not-paused (7 states) / pending-interruption all throw and never enqueue;TryAutoResumeAsyncregression after the shared-enqueue refactor.IntegrationTimeoutResume:OnTimedOutAsync→ taskPaused+ checkpoint row preserved (with progress); fail-fast contrast →Failed+ checkpoint deleted.KubernetesResumeCheckpointE2ETests.ResumeAfterTimeoutPause_CompletesDeploymentFromCheckpoint: plant checkpoint at batch 0 + setPaused(post-timeout state) →ProcessAsyncruns only batches 1+2, endsSuccess, checkpoint cleaned up. (Runs on the CI Kind-cluster E2E gate.)Follow-up (out of scope)
DeploymentFailedaudit-event category (pre-existing mapping;EventCategoryhas noTimedOut/Pausedvalue). With the resumable default this is mildly imprecise — a dedicated category + the operator-facing "Resume" button belong with the frontend Events work.