Defer tentacle upgrade when a deployment script is in flight on the machine#443
Merged
Conversation
…achine A tentacle upgrade restarts the agent. If a deployment currently has a script in flight on that agent (recorded before the StartScript RPC), the restart kills the running script. The upgrade now consults the deployment checkpoint's in-flight state and defers (Failed, retry-later) when the machine is busy, instead of dispatching. Asymmetric by design: only the upgrade — rare, operator-triggered, already holding the per-machine upgrade lock — checks for an active deployment. The deployment path is unchanged: no new lock, no self-contention on parallel same-machine steps, no extra dependency. The check sits inside the existing upgrade lock immediately before dispatch, so the TOCTOU window is minimal, and a deploy that begins in that window is itself resilient — a killed in-flight script pauses and re-attaches on resume. IInFlightScriptStore.IsMachineBusyAsync scans only checkpoints that still carry in-flight entries (a checkpoint exists only for an active/paused deployment, deleted on success), so the scan set stays small.
…DI wiring
Adversarial review of the change surfaced three fix-optional items, all
addressed here:
- IsMachineBusyAsync excluded only "[]" but not "{}", the idle value
EnsureExistsAsync seeds and the column default. Idle checkpoints leaked
past the DB filter (still correct via the parse-fallback, but wasteful
and contrary to the doc-comment). Exclude both idle shapes.
- Nothing pinned that the optional IInFlightScriptStore ctor param is
actually injected in production — the all-mock unit suite would not catch
the IScopedDependency scan dropping the store, silently disabling the
guard. Add an integration test asserting the real SquidModule resolves
IInFlightScriptStore to the real store. (A full UpgradeAsync drive-through
is infeasible in integration: the guard runs inside the Redis lock and
RedisSafeRunner eagerly connects, which the integration DI config lacks.)
- The deferral shares MachineUpgradeStatus.Failed with genuine failures and
is distinguished only by Detail text, which a unit test pins. Note that
the text is a load-bearing contract at the guard so it is reworded in
lockstep.
Plus an integration test that a seeded-but-never-dispatched checkpoint
("{}") reports no machine busy.
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
StartScriptRPC, per the in-flight checkpoint), the restart kills the running script. The upgrade now consults the deployment checkpoint's in-flight state and defers (returnsFailedwith a retry-later detail) when the machine is busy, instead of dispatching.StartWithPrevioussteps, no new dependency on the deploy hot path. (The earlier machine-exclusive-mutex approach was abandoned precisely because a symmetric lock over-serialized coexisting deploys.)IInFlightScriptStore.IsMachineBusyAsync(machineId)scans only checkpoints that still carry in-flight entries — a checkpoint exists only for an active/paused deployment and is deleted on success — so the scan set stays small. Backed by a pureInFlightScriptMap.ContainsMachinehelper.Placement & residual window
The check sits inside the existing per-machine upgrade Redis lock, immediately before dispatch and before the H4 metadata write — so a deferred upgrade leaves no metadata stragglers, and the cheap up-to-date / downgrade short-circuits still run first (a no-op upgrade never scans). The TOCTOU window between "machine is free" and "agent restarts" is minimal and accepted: a deploy that starts a script in that sub-second window is itself resilient — a killed in-flight script is classified transient and pauses/re-attaches on resume (#434), rather than failing the deployment.
Failed(not a new enum value) matches the established H4 contention convention for "couldn't dispatch right now, retry later" — non-breaking.Test plan
InFlightScriptMap.ContainsMachine: matches any slot for the machine regardless of step/action; other-machine-only → false; empty/malformed/legacy JSON → false (never falsely blocks).MachineUpgradeService: defers without dispatching when the store reports busy (asserts the strategy is never invoked + the operator-facing detail); proceeds to dispatch when free.InFlightScriptStore.IsMachineBusyAsync: in-flight script → busy (unrelated machine not); cleared slot → free; scan spans tasks (two tasks' in-flight machines both reported); untouched machine → free.InFlightScriptStoreintegration suite green (13).