fix(providers): D24 — close spawn-timeout race when timer fires during yield-suspension (round-2 F4)

cold-audit catch from 2026-05-24

Round-2 cold-audit Finding 4 (P2 concurrency bug). All 3 provider plugins
(anthropic / codex / mistral) had a race in spawn-timeout enforcement:

The timer's rejection branch is `if (rejectNext) { rejectNext(SPAWN_TIMEOUT) }`.
`rejectNext` is only set while the drain loop is AWAITING an empty-queue
promise. If the timer fires while the generator is suspended at a `yield`
(rejectNext === null because we're not currently awaiting), the rejection
is silently skipped. The loop then drains queued items, sees `close`
(from the SIGTERM), breaks normally. Post-loop guards on `!spawnTimedOut`
both skip: SPAWN_FAILED throw + yield-stop emit. Generator returns
normally with N partial chunks. Consumer (collectAllChunks) sees a
"successful" return. Fallback never advances. Truncated response gets
cached.

This violates BOTH:
- ADR 0004 § Trigger taxonomy bullet 4 (hard trigger never fires)
- ADR 0005 § Cache write conditions item 1 (truncated response cached)

The bug escaped FOUR reviewers: D10 diff-review, round-1 cold-audit, D11
reviewer, D14 reviewer. Round-2 cold-audit caught it because it was
unprimed and walked the timer/drain-loop race manually.

Fix (3 plugins, parallel race):

Each plugin gains an unconditional post-loop check, placed AFTER the
existing `try { drain loop } finally { clearTimeout(timer) }` block,
but BEFORE the existing `!spawnTimedOut`-gated guards:

```js
if (spawnTimedOut) {
  throw new ProviderError(
    `<cli-name> spawn timed out after ${maxSpawnTimeMs}ms`,
    'SPAWN_TIMEOUT',
  );
}
```

This closes the race surface: the inner-loop guard (unchanged, lines
~319/461/583) handles the rejectNext-set path; the new post-loop guard
handles the rejectNext-null path. Together they cover both halves —
SPAWN_TIMEOUT now reliably surfaces as a hard trigger regardless of
which path the timer fire took.

Changes (4 files, +154 / -0):
- lib/providers/anthropic.mjs: post-loop guard at lines 360-365 (after
  clearTimeout at line 348-350, before existing SPAWN_FAILED guard at 368)
- lib/providers/codex.mjs: post-loop guard at lines 521-526 (parallel)
- lib/providers/mistral.mjs: post-loop guard at lines 643-648 (parallel)
- test-features.mjs: new Suite 16 test 16e — race reproduction (109 lines)

Test 16e race reproduction (deterministic):

Exploits Node event-loop ordering. Mock spawn schedules data1 + data2 +
close via a single setImmediate. setImmediate runs before timers in the
same tick, so all 3 events queue before the timer can fire. Generator
yields data1 then suspends at yield with rejectNext null. Consumer
pauses 25ms (> 10ms timer). Timer fires during yield-suspension:
spawnTimedOut=true, rejectNext null → branch skipped, SIGTERM sent.
Consumer resumes, drain processes data2 + close, breaks normally.
Post-loop: D24 guard catches spawnTimedOut → throws SPAWN_TIMEOUT.

Pre-D24 behavior: generator returns normally (truncated, silently
cacheable). Test would FAIL.
Post-D24: SPAWN_TIMEOUT throws → test PASSES.

Reviewer empirically validated the test's diagnostic power by stripping
D24 from anthropic.mjs and observing exactly 16e fail (only 16e — 16a
through 16d still pass on the orthogonal rejectNext-set path), then
restored.

Timing flake risk: very low. Race depends only on relative ordering of
two setTimeout callbacks (10ms vs 25ms) which Node guarantees regardless
of absolute drift. Reviewer ran 7 npm test passes; 16e timing spread
26.43-27.70ms (1.3ms variance).

Tests: 334 → 335 (+1). 335/335 pass on Node 20.

Out of scope (filed in issue #3):
- SPAWN_TIMEOUT salvage parity: this fix discards partial chunks (matches
  current SPAWN_TIMEOUT semantics, doesn't introduce asymmetry vs
  SPAWN_FAILED-no-chunks). Whether to add SPAWN_TIMEOUT-with-usable-chunks
  salvage (analogous to D16's SPAWN_FAILED salvage) is a separate design
  decision tracked in issue #3.

Authority:
- ADR 0004 § Trigger taxonomy — Hard triggers bullet 4: "Provider CLI
  spawn timeout (configurable per-provider via hints.maxSpawnTimeMs)"
  https://github.com/dtzp555-max/olp/blob/main/docs/adr/0004-fallback-engine.md
- ADR 0005 § Cache write conditions item 1 (no truncation)
- engine.mjs HARD_TRIGGER_CODES['SPAWN_TIMEOUT'] = true (D10 P1.3,
  unchanged)
- CC 开发铁律 v1.6 § 10.x — Round-2 Cold Audit caught this

Reviewer (Iron Rule v1.6 § 10.x Mode A, fresh-context opus, independent
of drafter): APPROVE. Verified guard position in all 3 plugins; read
push() to confirm rejectNext-null mechanism; empirically validated test
16e's diagnostic specificity (strip-revert-restore); 7 test runs
confirmed timing stability. No blocking issues.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
2026-05-24 14:22:53 +10:00
co-authored by Claude Opus 4.7
parent 1466d3a082
commit f8348adb3b
4 changed files with 154 additions and 0 deletions
+15
View File
@@ -349,6 +349,21 @@ async function* _spawnAndStream(irRequest, authContext, spawnImpl) {
clearTimeout(spawnDeadlineTimer);
}
// D24 round-2 F4 fix: if the timer fired while the drain loop was processing
// queued items, rejectNext was null at fire-time so the rejection was skipped.
// The post-loop SPAWN_FAILED + yield-stop guards both check !spawnTimedOut, so
// the generator would otherwise return normally with partial chunks despite the
// timeout. This unconditional throw closes the race — SPAWN_TIMEOUT always
// surfaces as a hard trigger to the fallback engine regardless of which path
// the timer fire took. Note: any partial chunks already yielded are discarded
// by the caller; SPAWN_TIMEOUT salvage parity is tracked in issue #3.
if (spawnTimedOut) {
throw new ProviderError(
`claude spawn timed out after ${maxSpawnTimeMs}ms`,
'SPAWN_TIMEOUT',
);
}
// Process close
if (exitCode !== 0 && !spawnTimedOut) {
const errMsg = accumulatedStderr.slice(0, 300) || `claude exit ${exitCode}`;