fix(workflow-workerthread): tighten post-result promptness waits back down
Review follow-up: the blanket 10s default correctly targets worker-thread cold-start races (starting, first-script-line, async child-registration messages — genuinely CPU-bound under CI contention), but four waits assert something different — that the HOST reacted PROMPTLY to an event that already happened (a settled result, an observed worker death). Those had no cold-start left to wait on, so the generous default just widened the window a real regression could hide in. Verified by injecting a 6s delay into the settle-reap's abort call: the un-overridden helper's test still passed in ~6s. The same mutation now fails in ~1s with the explicit 1000ms override restored on all four sites (the abort-on-settle test's own assertion, the two worker-death cancel/dispose reap checks, and the dispose-ack-race check). The other 12 waits keep the 10s default — they run BEFORE a result is awaited, waiting on the worker to actually start rather than on a host reaction. Doc comment corrected to describe the split instead of claiming every wait is a cold-start race.
This commit is contained in:
@@ -21,12 +21,18 @@ function fakeParent(): Agent {
|
|||||||
vi.setConfig({ testTimeout: 30_000 })
|
vi.setConfig({ testTimeout: 30_000 })
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* `vi.waitFor` with a contention-proof timeout: the 1s default flaked
|
* `vi.waitFor` with a contention-proof default timeout: the 1s default
|
||||||
* repeatedly on the CI coverage lane, where worker-thread cold start competes
|
* flaked repeatedly on the CI coverage lane, where worker-thread cold start
|
||||||
* with three sibling vitest workers for CPU. Every wait in this file is for
|
* (CPU-bound — a fresh thread compiles the runtime) competes with three
|
||||||
* something that WILL happen (a worker starting, a child registering) — a
|
* sibling vitest workers for CPU. The 10s default is for exactly those
|
||||||
* generous bound only removes the flake, it cannot mask a genuine hang (the
|
* races — waiting for a worker to start, run its first script line, or
|
||||||
* file-wide test timeout above still fences those).
|
* deliver an async child-registration message to the host. It is NOT for a
|
||||||
|
* wait that asserts the HOST reacted PROMPTLY to something that already
|
||||||
|
* happened (a settled result, an observed worker death): those keep an
|
||||||
|
* explicit tight override below, or the generous default would silently
|
||||||
|
* accept a multi-second regression in host-side reap latency as passing
|
||||||
|
* (proven by injecting a 6s delay into one such reap and watching the
|
||||||
|
* un-overridden version of this helper still pass in ~6s).
|
||||||
* @param assertion - retried until it stops throwing or the timeout elapses.
|
* @param assertion - retried until it stops throwing or the timeout elapses.
|
||||||
* @param timeout - override for a wait that must stay deliberately tight.
|
* @param timeout - override for a wait that must stay deliberately tight.
|
||||||
* @returns resolves when the assertion passes.
|
* @returns resolves when the assertion passes.
|
||||||
@@ -545,8 +551,11 @@ describe('dsh-workflow-workerthread', () => {
|
|||||||
const result = await handle.result
|
const result = await handle.result
|
||||||
expect(result.stopReason).toBe('completed')
|
expect(result.stopReason).toBe('completed')
|
||||||
// BEFORE dispose(): the settlement itself must have aborted the signal —
|
// BEFORE dispose(): the settlement itself must have aborted the signal —
|
||||||
// without it this child would stay live until dispose's terminate.
|
// without it this child would stay live until dispose's terminate. This
|
||||||
await waitFor(() => { expect(aborted).toEqual(['workflow settled']) })
|
// is a HOST-PROMPTNESS claim, not a cold-start race — a tight explicit
|
||||||
|
// bound (unlike the file default) so a multi-second reap regression
|
||||||
|
// cannot pass by outlasting the wait.
|
||||||
|
await waitFor(() => { expect(aborted).toEqual(['workflow settled']) }, 1000)
|
||||||
await handle.dispose()
|
await handle.dispose()
|
||||||
})
|
})
|
||||||
|
|
||||||
@@ -769,7 +778,9 @@ describe('dsh-workflow-workerthread', () => {
|
|||||||
// A worker death is a stop reason like any other: workflow/end fires
|
// A worker death is a stop reason like any other: workflow/end fires
|
||||||
// with the error outcome — for a bus observer it is the only obituary.
|
// with the error outcome — for a bus observer it is the only obituary.
|
||||||
expect(runEnds).toEqual([{ stopReason: 'error', error: result.error, agentsStarted: 1 }])
|
expect(runEnds).toEqual([{ stopReason: 'error', error: result.error, agentsStarted: 1 }])
|
||||||
await waitFor(() => { expect(cancelled.length).toBe(1) })
|
// Result already settled — this is the reap's promptness, not a
|
||||||
|
// cold-start race; tight explicit bound (see the helper's doc comment).
|
||||||
|
await waitFor(() => { expect(cancelled.length).toBe(1) }, 1000)
|
||||||
await handle.dispose()
|
await handle.dispose()
|
||||||
}, 15_000)
|
}, 15_000)
|
||||||
|
|
||||||
@@ -790,10 +801,12 @@ describe('dsh-workflow-workerthread', () => {
|
|||||||
expect(result.stopReason).toBe('error')
|
expect(result.stopReason).toBe('error')
|
||||||
expect(result.error).toContain('worker blew up')
|
expect(result.error).toContain('worker blew up')
|
||||||
// The reap wound the stray child down (cancel + a CLEAN dispose).
|
// The reap wound the stray child down (cancel + a CLEAN dispose).
|
||||||
|
// Result already settled — this is the reap's promptness, not a
|
||||||
|
// cold-start race; tight explicit bound (see the helper's doc comment).
|
||||||
await waitFor(() => {
|
await waitFor(() => {
|
||||||
expect(provider.runs.length).toBe(1)
|
expect(provider.runs.length).toBe(1)
|
||||||
expect(provider.runs[0]!.disposed).toBe(true)
|
expect(provider.runs[0]!.disposed).toBe(true)
|
||||||
})
|
}, 1000)
|
||||||
await handle.dispose()
|
await handle.dispose()
|
||||||
}, 15_000)
|
}, 15_000)
|
||||||
|
|
||||||
@@ -857,7 +870,10 @@ describe('dsh-workflow-workerthread', () => {
|
|||||||
const result = await handle.result
|
const result = await handle.result
|
||||||
expect(result.stopReason).toBe('error')
|
expect(result.stopReason).toBe('error')
|
||||||
expect(result.error).toContain('exit code 5')
|
expect(result.error).toContain('exit code 5')
|
||||||
await waitFor(() => { expect(provider.runs[0]!.disposed).toBe(true) })
|
// Result already settled — this is the reap's promptness (bounded
|
||||||
|
// above the mock's fixed 300ms dispose delay, not a cold-start race);
|
||||||
|
// tight explicit bound (see the helper's doc comment).
|
||||||
|
await waitFor(() => { expect(provider.runs[0]!.disposed).toBe(true) }, 1000)
|
||||||
await handle.dispose()
|
await handle.dispose()
|
||||||
}, 15_000)
|
}, 15_000)
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user