Scope the in-flight reattach store to the dispatch unit, not the machine#433
Merged
Conversation
StepBatcherParallelE2ETests proved StartWithPrevious parallelism by stop-watching the whole ProcessAsync and asserting < 3.5s. That measured pipeline overhead (Halibut handshake, 1s polling, DB, Kind latency) as much as the step overlap, so a genuinely-parallel run could breach the ceiling on a loaded CI runner — it false-failed 4+ times across unrelated PRs, including a unit-test-only change. Replace it with an agent-side interval-overlap proof: each step echoes an epoch timestamp (date +%s.%N) around its sleep, and the test asserts the two execution intervals overlap (min(end) - max(start) > 1s). This measures only the overlap and is immune to pipeline/CI overhead. HalibutCircuitBreakerE2ETests used the same stopwatch-around-pipeline pattern (< 5s). Its real proof is non-temporal (INVARIANT 3: the stub never ran the script); widen the wall-clock to a generous 20s anti-hang guard (the regression it catches is a full 30-min script timeout) so DB-bound prepare phases can't breach it under CI load.
…itution The interval-overlap rewrite emitted each step's epoch with echo "STEPTIME <id> START $(date +%s.%N)" whose double quotes + command substitution did not survive the JSON action-property round-trip into the agent — the body failed to parse and the script exited 1 before the sleep, so the deployment never reached Success. Bake the marker into date's own format string instead: date +STEPTIME_<id>_START_%s.%N This emits the same STEPTIME line with no shell quoting and no command substitution, so it round-trips verbatim. Parser regex updated to the underscore-delimited form.
A parallel StartWithPrevious batch dispatches its steps concurrently via
Task.WhenAll. When two steps in the batch share a target machine, both
call DispatchOrReattachAsync for the same (task, machine) at once. The
in-flight reattach store was keyed by (serverTaskId, machineId) only, so
the second dispatch's reattach probe found the first's just-recorded
ticket and "skipped duplicate dispatch" — silently running the first
step's script instead of its own, with the losing observer returning
exit -1 and failing the deployment. Record-before-RPC (P1a) widened the
collision window by persisting the ticket before StartScript.
Key the in-flight slot by the dispatch unit (machine, step, action)
instead. Concurrent dispatches to one machine now stay independent both
live and on resume — each re-attaches only to its OWN ticket. The
checkpoint JSON moves from a machine-keyed object to a list of
{m,s,a,t} entries; an older machine-keyed row deserialises to empty
(the documented re-dispatch fail-safe), so it is non-breaking.
This is the real bug behind the flaky StepBatcherParallel E2E (two
StartWithPrevious steps on one agent): with the slot fix both steps run
their own script, so the deployment succeeds and the execution
intervals overlap deterministically.
Tests:
- Unit: InFlightScriptMap same-machine/different-step independence +
legacy-shape tolerance
- Integration: InFlightScriptStore dispatch-scoped independence +
concurrent same-machine record; HalibutResumeReattach deterministic
two-concurrent-dispatch regression (gate parks step A's RPC until
step B has dispatched) proving no cross-reattach
Adversarial review found the prior slot keyed by (machineId, StepName,
ActionName) still re-opened the collision for duplicate names: step and
action names have NO uniqueness constraint (the schema only enforces
unique step_order / action_order), so two distinct identically-named
steps targeting one machine in a parallel batch collapse to the same
slot and re-attach to each other — the exact bug, merely narrowed.
Key DispatchSlot by the stable, process-unique (MachineId, StepId,
ActionId) instead. The ids come from the frozen process snapshot, so
they are identical across a crash→resume. Threaded onto
ScriptExecutionRequest and attached post-render in ExecuteStepsPhase
(single injection point, alongside the existing Masker attach — the
renderer carries only display names). The checkpoint JSON now stores
{m,s,a,t} with integer s/a; the interim name-keyed list and the older
machine-keyed object both deserialise to empty (re-dispatch fail-safe).
The concurrent-dispatch regression now uses TWO identically-named steps
('deploy'/'deploy') differing only by id, proving the slot keys on ids
not names — it would re-collide under the name key.
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
StartWithPreviousbatch dispatches its steps concurrently (Task.WhenAll). When two steps share a target machine, both callDispatchOrReattachAsyncfor the same(task, machine)at once. The in-flight reattach store (InFlightScriptStore) was keyed by(serverTaskId, machineId)only, so the second dispatch's reattach probe found the first's just-recorded ticket and "skipped duplicate dispatch" — silently running the first step's script instead of its own, with the losing observer returning exit-1and failing the deployment. Record-before-RPC (Record the in-flight ScriptTicket before the StartScript RPC #431) widened the window by persisting the ticket beforeStartScript.DispatchSlot(MachineId, StepId, ActionId). It keys on the stable, process-unique step/action ids — NOT display names, which carry no uniqueness constraint (the schema enforces uniquestep_order/action_orderonly, so two identically-named steps would otherwise re-collapse to one slot). The ids come from the frozen process snapshot, so they are identical across a crash→resume; threaded ontoScriptExecutionRequestand attached post-render at the single dispatch site (alongside the existing masker attach).{m,s,a,t}list; older/interim shapes deserialise to empty (the documented re-dispatch fail-safe), so it is non-breaking.StepBatcherParallelE2ETests(twoStartWithPrevioussteps on one agent): with the slot fix both steps run their own script, the deployment succeeds, and the agent-side execution intervals overlap deterministically. The E2E also now proves overlap from agent-clock timestamps emitted via adateformat string (immune to the JSON action-property round-trip) instead of a flaky total-wall-clock ceiling.Test plan
InFlightScriptMapTests— same-machine/different-action-id independence, sibling-slot non-match, legacy machine-keyed + interim name-keyed shapes tolerated as emptyInFlightScriptStoreTestsdispatch-scoped independence + concurrent same-machine record;HalibutResumeReattachTests.ConcurrentDispatch_SameMachineIdenticallyNamedSteps_…— deterministic two-concurrent-dispatch regression with TWO identically-named steps differing only by id (a gate parks step A's RPC until step B has dispatched), proving no cross-reattach,StartScriptCalls == 2StepBatcherParallelE2ETeststwoStartWithPrevioussteps on one agent → Success + overlapping intervals