Pause (resumable) on a transient infra failure instead of failing#434
Merged
Conversation
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
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
… 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
…reaker) 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.
3 tasks
ppXD
added a commit
that referenced
this pull request
Jun 11, 2026
…required) (#436) * Add end-to-end ProcessAsync test for transient-pause (required + non-required) The P1b transient-pause fix (#434) was covered only compositionally: the unit tests mock the phase to throw the transient directly, and HalibutResumeReattachTests calls the strategy directly — neither runs the real ExecuteSingleActionAsync per-action catch THROUGH the runner to the pause outcome. The non-required path (no per-target catch) was the gap the adversarial review flagged. This drives the REAL pipeline: IDeploymentTaskExecutor.ProcessAsync over a seeded deployment (KubernetesAgent target, so the real HalibutMachineExecutionStrategy records the in-flight pointer), with only the Halibut RPC transport faked (GetStatus throws a transient HalibutClientException). It asserts, for BOTH required and non-required steps: task Paused (not Failed), checkpoint preserved, and the in-flight pointer preserved. Red-green verified: with the ExecuteSingleActionAsync transient catch neutered, the NON-required case fails (Paused → Failed + checkpoint deleted) — the exact swallow bug — while the required case still passes (the per-target catch covers it), confirming the test specifically guards the non-required propagation the fix added. * Tighten transient-pause E2E: typed pointer reader + permanent-failure contrast Replace the brittle Contains("\"t\"") substring with the production typed reader InFlightScriptMap.TryGet(json, DispatchSlot(...)) asserting the preserved pointer holds the EXACT ScriptTicket the strategy dispatched (captured in the fake) — pins slot identity + ticket value and survives a JsonPropertyName rename. Add a permanent-failure contrast case: the same seeded path with a permanent Halibut subtype (ServiceNotFoundHalibutClientException) must Fail terminally + delete the checkpoint, so the Paused rows read as "Paused iff transient" and the transient/permanent boundary is pinned end-to-end, not just in the predicate's own unit test.
4 tasks
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
finallythat cleared the in-flight pointer on any exit — destroy the reattach pointer, so a resume re-dispatched a duplicate of the still-running script (the P1 idempotency gap, second half).HalibutClientException,AgentUnreachableException,CircuitOpenException, or anAggregateExceptionof only those) as a PAUSE: the task transitions toPausedwith its checkpoint AND in-flight pointer preserved, and a resume re-attaches to the still-running script. Reuses the #428 pause/resume machinery.SQUID_DEPLOYMENT_TIMEOUT_RESUMABLEfrom Pause and preserve checkpoint on deployment timeout, add manual resume #428 is separate and unaffected.)TransientFailureClassifier, the single source of truth) excludes the permanent Halibut protocol/invocation subtypes (ServiceInvocation/NoMatchingServiceOrMethod/MethodNotFound/ServiceNotFound/AmbiguousMethodMatch) — classifying those as transient would pause-loop forever.ExecuteStepsPhasefor both required and non-required steps (non-required was previously swallowed intoFailureEncountered→OnFailureAsync→ checkpoint+pointer deleted). The re-attach probe preserves the pointer on a transient probe failure (was: cleared it → duplicate-dispatch risk). The runner's transient catch is guarded by!timeoutCts && !registryCts && !ctso cancel/timeout win.Test plan
TransientFailureClassifiermatrix (permanent subtypes excluded; CircuitOpen/AgentUnreachable/base included; aggregate all/mixed/empty);DeploymentPipelineRunnerTransientPauseTests(transient→pause, genuine-failure→fail+rethrow, aggregate-of-transients, transient-during-user-cancel does NOT pause);TargetCatchClassifierTeststransient casesExecuteStepsPhase:DeploymentExecutionLoggingTests.TransientStrategyFailure_PropagatesOutOfPhase_RegardlessOfRequired(a throwing strategy proves a transient propagates for BOTH required + non-required,FailureEncounteredstays false); contrast pinned byStepFailed_DoesNotLogCompleted(non-required GENUINE failure still swallowed)HalibutResumeReattachTests— observe-throws-transient preserves the in-flight pointer; reattach-probe transient failure preserves the pointer (no fresh dispatch); full reattach + timeout-resume suites green