flake: shared timing substrate in src/orchestrator/factory.test.ts fails 3 different tests nondeterministically

[khaliqgant]

Summary

src/orchestrator/factory.test.ts › "releases a late placement when another owner reclaimed the lifecycle (#303)" fails intermittently at roughly a 30% rate on main, with no code change between runs.

Measurement

Sampled by running that single test repeatedly with vitest run -t, nothing else changed between runs:

arm	failures
clean origin/main (cbb3d32)	3 of 8
an unrelated feature branch	1 of 8

Consecutive runs on identical code produced exit=0 then exit=1, which is the definition of a flake rather than a regression.

The failure

AssertionError: expected [Function] to throw error matching
  /was released while .* was still spawn…/
but got 'Dispatch lifecycle ownership lost imm…'

Both outcomes are real code paths; which one wins appears to depend on ordering between the release and the ownership check. The test asserts the release message specifically, so whenever the ownership-lost path wins the race the assertion fails.

Why it is worth fixing rather than tolerating

A ~30% flake in the main orchestrator suite makes every PR's CI close to a coin toss, and it fails inside the largest suite in the repo — so the natural reading is "this PR broke something." I hit it on an unrelated change and had to spend 16 test runs across two arms proving it was not mine. Everyone who touches this repo will pay that cost, or worse, will not pay it and will merge on a re-run without checking.

It also erodes exactly the signal this codebase depends on most: that a red check means something. This repo already has one red check on every PR (the request job's HTTP 403, fixed by #340) that people have learned to scroll past. A second habitual red is how a real failure gets ignored.

Suggested direction

Make the assertion accept either terminal outcome, or pin the ordering the test means to exercise, rather than asserting one side of a race. I have deliberately not changed it here — the test encodes intent from #303 and its author should decide which outcome it is meant to prove.

Found while working on #341.

[khaliqgant]

Widening this: it is not one test, it is the suite's timing substrate

I filed this for a single test. Further runs show at least three tests in src/orchestrator/factory.test.ts failing nondeterministically, a different one each time:

1. releases a late placement when another owner reclaimed the lifecycle (#303)
2. drains an in-flight Slack reply route before a reopen clears the terminal fence
   — AssertionError: expected [ { …(14) }, …(1) ] to have a length of 4 but got 2
3. reaps a first dispatch whose spawn never returns (#…)

A different victim each run is the signature of a shared timing substrate, not three independent broken assertions. All three are races between a dispatch/placement path and a reaper, fence, or ownership check.

Measurements

Full-suite runs (how CI runs it):

arm	result
clean origin/main	4 pass / 0 fail
an unrelated feature branch	2 pass / 3 fail (incl. one CI run)

Isolation mode (vitest -t, single test), which behaves differently and should not be pooled with the above:

arm	result
clean origin/main, test 1	3 fail / 8
feature branch, test 1	1 fail / 8

Fisher's exact on the full-suite arms (4/4 vs 2/5) gives p ≈ 0.17 — the branch asymmetry is not significant, and I could not attribute it either way. Each full run costs ~4 minutes, so separating a ~20% from a ~35% rate at significance needs dozens of runs. That cost is precisely the problem: it lands on whichever author is unlucky.

Why this is worth real effort

The failure appears in the largest suite in the repo, so the natural reading is always "this PR broke something." I spent roughly twenty test runs across two arms establishing that I could not tell — and the honest answer was still inconclusive.

The repo already trained people to scroll past one habitual red (the request job's HTTP 403, fixed in #340 today). A second habitual red is how a genuine regression gets waved through on a re-run. A flake that cannot be cheaply distinguished from a regression is worse than a plainly broken test, because it converts every red into an argument.

Suggested direction

Look for the shared substrate rather than patching the three assertions: fake-timer advancement, an unawaited promise between a placement and its reaper, or ordering assumptions about when a fence/ownership check observes state. Fixing one assertion will just move the failure to the next test in the same family — which is what the evidence above already shows happening.

[khaliqgant]

Widening this from a single test to the shared timing substrate, on evidence from #341.

Three different tests in src/orchestrator/factory.test.ts have now been observed failing nondeterministically across runs of the same code:

1. releases a late placement when another owner reclaimed the lifecycle (#303)
2. drains an in-flight Slack reply route before a reopen clears the terminal fence
3. reaps a first dispatch whose spawn never returns

A different victim each run is the signature of a shared timing substrate, not three broken assertions. (3) is the largest suite's own dispatch reaper, which makes this more than cosmetic.

Measurements, from the lane on #341:

arm	mode	result
clean origin/main	full suite	4 pass / 0 fail
PR branch	full suite	2 pass / 3 fail
PR branch, orchestrator change ablated	full suite	1 pass / 1 fail
PR branch, relay-fleet-client.ts reverted to main	full suite	1 pass / 0 fail
clean origin/main	isolation (vitest -t)	3 of 8 failed

The isolation arm is the important one: clean main fails 3 of 8 with no PR involved, so the flakiness is pre-existing and not attributable to any change in flight.

Fisher's exact on the full-suite 4/4 vs 2/5 asymmetry gives p ≈ 0.17 — not significant. Note also that ablating the orchestrator change did not fix it, which was the one mechanism plausibly perturbing timing.

Why this needs an owner rather than more measurement. Each full run is ~4 minutes, and separating a ~20% from a ~35% failure rate at significance needs dozens of runs. The measurement is unaffordable per-PR, so the cost lands on every author as a coin-toss red they must re-litigate from scratch. That is exactly how this repo trained people to scroll past the request job's 403 for weeks — an unexplained red check on every PR manufactures normalized deviance, and the next genuine failure gets waved through with it.

Deliverable: find the shared substrate — most likely a timer, fake-clock, or vi.waitFor interaction leaking across tests in this file — and make the suite deterministic. Not "retry the test." A --repeat or seeded-ordering run is the natural instrument.

Until this is fixed, per-PR policy is: land on per-job green, with the green explicitly recorded as weak evidence. Authors are not required to prove innocence against a substrate that flakes on clean main.

[khaliqgant]

Fourth victim, and the cleanest proof yet that this is the substrate and not any PR.

FAIL src/orchestrator/factory.test.ts > FactoryLoop >
     keeps the terminal drain waiting on the receipt the fence itself writes
AssertionError: expected 0 to be greater than or equal to 1

Observed on PR #356, run 32671352543.

#356 is the 0.1.71 release PR. Its entire diff is three version strings — package.json +1/-1 and package-lock.json +2/-2. It contains no source change of any kind.

A PR that edits only a version number cannot break a dispatch-fence test. So this is the substrate, with no ambiguity and no author to blame — which is exactly the control the earlier reports lacked. Every previous sighting had some code change that a sceptic could point at.

Running tally of nondeterministic victims in src/orchestrator/factory.test.ts:

1. releases a late placement when another owner reclaimed the lifecycle (#303)
2. drains an in-flight Slack reply route before a reopen clears the terminal fence
3. reaps a first dispatch whose spawn never returns (#303)
4. keeps the terminal drain waiting on the receipt the fence itself writes ← new

Four different tests, same file, same session. A different victim each run is a shared timing substrate, not four broken assertions.

Cost so far tonight, measured rather than asserted: this file has produced red checks on #341, #354, main @ 85c06119, and now the release PR. In three of those four cases I had to stop and prove the red was not the change in front of me — once by ancestry check, once by comparing against a green run of identical code, once by observing that the diff was version-only. That is the tax, and it is being paid by every author who touches this repo.

It also nearly cost something real: main genuinely was broken by #322 earlier today, and the signal that mattered was buried among flake reds. A repo where red is routine is a repo where a real break gets waved through.

Prioritising this above cosmetic work is justified on that basis alone.

[khaliqgant]

Escalating: this flake is now blocking a release, not just annoying authors.

The 0.1.71 publish run (32672450776) failed, and 0.1.71 did not ship. npm dist-tags.latest is still 0.1.70. The failure is this substrate:

AssertionError: expected [ { …(14) }, …(1) ] to have a length of 4 but got 2
 ❯ src/orchestrator/factory.test.ts:20070:51

AssertionError: expected undefined to be 1
 ❯ src/orchestrator/factory.test.ts:25956:87

0.1.71 carries three fixes for a seven-day production dispatch outage — #347 (orphan reclaim), #349 (dry-run roster read), #354 (bound every relayfile call so a hung read cannot wedge reconcile). They are sitting in main, unable to reach production, because a timing test in this file failed.

That is a different severity argument than the one I made an hour ago. Before, the cost was authors re-litigating inherited reds. Now it is a release gate on an active incident, and the flake rate compounds: each publish attempt is another ~30% coin toss, and each retry costs a full suite run.

Sightings today, now five, all in this one file:

1. releases a late placement when another owner reclaimed the lifecycle (#303)
2. drains an in-flight Slack reply route before a reopen clears the terminal fence
3. reaps a first dispatch whose spawn never returns (#303)
4. keeps the terminal drain waiting on the receipt the fence itself writes
5. :20070 / :25956 on the publish run above

And the cleanest control remains PR #356: its entire diff was three version strings, and it went red on this file, then green on rerun with byte-identical code. No author, no code change, both outcomes.

I have re-run the failed publish job. If it flakes again I will keep retrying, because the alternative — skipping the suite to force a release — would mean shipping an outage fix unverified, and that is how the next incident starts.

This should be fixed before anything cosmetic in this repo. It is now on the critical path of an active production incident.