Skip to content

Scope the in-flight reattach store to the dispatch unit, not the machine

Placeholder ppxd requested to merge test/harden-flaky-e2e-timing into main

Summary

  • A parallel StartWithPrevious batch dispatches its steps concurrently (Task.WhenAll). When two steps share a target machine, both call DispatchOrReattachAsync for 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 -1 and failing the deployment. Record-before-RPC (#431) widened the window by persisting the ticket before StartScript.
  • Fix: key the in-flight slot by the dispatch unit via a new 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 unique step_order/action_order only, 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 onto ScriptExecutionRequest and attached post-render at the single dispatch site (alongside the existing masker attach).
  • Concurrent dispatches to one machine now stay independent — live and on resume each re-attaches only to its OWN ticket. The checkpoint JSON moves from a machine-keyed object to a {m,s,a,t} list; older/interim shapes deserialise to empty (the documented re-dispatch fail-safe), so it is non-breaking.
  • This is the real bug behind the flaky StepBatcherParallelE2ETests (two StartWithPrevious steps 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 a date format string (immune to the JSON action-property round-trip) instead of a flaky total-wall-clock ceiling.

Test plan

  • Unit (5986/5986): InFlightScriptMapTests — same-machine/different-action-id independence, sibling-slot non-match, legacy machine-keyed + interim name-keyed shapes tolerated as empty
  • Integration (real Postgres): InFlightScriptStoreTests dispatch-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 == 2
  • E2E (K8s Pipeline): StepBatcherParallelE2ETests two StartWithPrevious steps on one agent → Success + overlapping intervals
  • Full unit suite green; non-breaking (transient checkpoint column, fail-safe handles old shapes)
  • Adversarial multi-agent review (concurrency / resume / non-breaking / completeness) — the name-vs-id finding drove the id-keying

Merge request reports

Loading