Repository navigation
test(coil): auto-resume tests wait on receipts, never on scheduler turns - #143
Conversation
`Reactor.test.ts` went red in CI on `pending must be cleared: expected 1 to equal +0`. Nothing was wrong with the product: `advancePastResume` was eight clock steps each chased by a fixed ten-pump spin, a budget of scheduler turns rather than a signal, and the store persists through real filesystem I/O that completes on the Node event loop and not on TestClock — so on a loaded runner the assertion simply looked before the cancellation landed. The auto-resume reactor now announces its milestones the way the loop supervisor does: an optional receipt service resolved with `Effect.serviceOption`, a no-op `Effect.void` wherever nobody provides it (which is every production graph), and a dropping PubSub so a test that stops draining can never stall the wake fiber. `tick.completed` closes every wake pass, including the one where nothing is due, so "advance one poll and let the reactor finish" is one exact await. Every wait in the tests is now an await on a receipt. The clock still moves in poll-sized steps — advancing time is the scenario, polling for the result was the bug — and the pump helpers are gone. Closes #134 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
radroid
left a comment
There was a problem hiding this comment.
Reviewed the diff, receipts.ts against coil/loop/receipts.ts, and ran the tests. One real bug found (would use request-changes, but GitHub blocks that on your own PR); everything else checks out.
1. Possible hang: tick.completed is skipped if any fireOne fails, unlike the loop reactor it claims to mirror. Reactor.ts:329 — Effect.forEach(due, (p) => fireOne(p, nowMs), { discard: true }) has no per-item failure isolation, so if one fireOne fails, processDue fails before reaching the tick.completed emit at Reactor.ts:334. fireOne (Reactor.ts:213) calls snapshotQuery.getSnapshot(), which has a typed ProjectionRepositoryError channel — a real, expected failure, not just a defect — and only the dispatchResume call is wrapped in catchCause; nothing else in fireOne is. Compare coil/loop/Reactor.ts:700-707, where evaluateOne is deliberately wrapped per-thread in catchCause ("a single bad record must not retire it") specifically so the tick's end-of-pass receipt is unconditional. AutoResumeReactorReceipts has no such isolation.
Concrete failure: a snapshot read fails for one due thread mid-tick → processDue fails → the outer catchCause+forever wake loop logs and retries, so production is fine → but tick.completed was never published for that tick, so any test awaiting it via advanceOneTick's while (true) { PubSub.take(log) } (Reactor.test.ts) blocks forever, bounded only by vitest's default timeout instead of the old bounded-pump Effect.die with a clear message. Not hit by the current 8 scenarios (the stub getSnapshot never fails), but it's a latent hang the old code couldn't produce, and belongs in this PR since it's exactly the "same class" this PR is fixing.
Fix: wrap the fireOne call inside Effect.forEach with its own Effect.catchCause (log + continue), matching evaluateOne's pattern, so one bad record can't suppress the tick receipt.
Everything else held up:
- All turn-budget waits (
settleQuiet,advancePastResume, fixed pump loops) are gone; the two remainingTestClock.adjustcalls each pair with an exact receipt await. - Negative assertions are now N-completed-ticks-observed, not spin-and-hope.
- No production layer provides
AutoResumeReactorReceipts(coil/index.tsnever references it);receiptEmitteris a constantEffect.void/enabled:falsewhen absent, andtick.completedis gated behindreceipts.enabled. receipts.tsmatchesloop/receipts.ts's pattern (buffer 4096,PubSub.dropping, scope/Layer.effect shape) with no drift.resume.firedis emitted after the dispatch attempt (post-catchCause), so a failed dispatch still announces — correct as described.- Reactor.ts diff is additive-only around existing guards; no reordered/changed decisions. The 5 removed
storebindings are all genuinely unused after the receipt-based rewrite. - Tests:
Reactor.test.ts10/10 passing x3 runs;src/coil/autoResumefull dir 117/117 passing once. No wall-clock timeouts introduced. - Fork-owned files only, conventional commit title, PR body ends with the model/harness line,
replay/*.test.tsfollow-up explicitly deferred.
Claude Sonnet 5 via Claude Code subagent.
`processDue` ran every due arm through a bare `Effect.forEach`, so the first `fireOne` that failed took the whole pass down with it. `fireOne` reads a fresh snapshot per arm and `getSnapshot` has a typed `ProjectionRepositoryError` channel, so that is an expected failure rather than a defect — and it cost two things: the rest of the batch never ran, and the pass never reached its `tick.completed`. Production recovered on the next pass, but anything awaiting that receipt was left waiting for one that would never come. Each arm is now wrapped the way `coil/loop/Reactor.ts` wraps `evaluateOne`: log the cause, continue the batch. The failed arm itself is untouched — a failed fire reserves nothing and clears nothing — so the next pass tries it again. The new test pins both halves. Without the fix it fails on the arm: the first thread's failure starves the batch until the wake loop's own retry re-runs the whole pass, by which point the arm has been consumed out of order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Fixed in
New test: Worth recording: reverted against this test the failure is not the hang you predicted, it is an assertion. The wake loop's Verification: |
The rejected-window fixture keeps the sync's runtime.warning shape and this branch's eventId parameter. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 23 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ot a turn count CI run 34734274701 failed its CONTROL case with `expected [Array(1)] to include 'Auto-resume cancelled: thread-advanced.'`: ten quiet spins after the clock step were not enough for the cancellation to land on a loaded runner. Both cases now advance the clock and then `settleUntil` the activity they assert on has been dispatched, which is the harness's own rule for anything that must appear. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
apps/server/src/coil/autoResume/Reactor.test.tswent red in CI onpending must be cleared: expected 1 to equal +0. Nothing was wrong with the product:advancePastResumewas eightTestClock.adjuststeps each chased by a fixed ten-pump spin — a budget of scheduler turns rather than a signal — and the store persists throughwriteFileStringAtomically, real filesystem I/O that completes on the Node event loop and not onTestClock. Under load the assertion simply looked before the cancellation had landed.The auto-resume reactor now announces its milestones exactly the way the loop supervisor does (
coil/loop/receipts.ts): an optional service resolved withEffect.serviceOption, a no-opEffect.voidwherever nobody provides it — which is every production graph, sincecoil/index.tsdoes not mention the module — and a droppingPubSubso a test that stops draining can never stall the wake fiber. The variants areresume.scheduled,resume.skipped,resume.cancelled,resume.fired, and atick.completedthat closes every wake pass including the one where nothing is due, so "advance one poll and let the reactor finish" is one exact await.tick.completedis the only hot-path receipt and is assembled only whenenabled, so an install with no subscriber allocates nothing per poll.Every wait in the tests is now an await on a receipt, including the two negative ones: a disabled thread is proven by its
resume.skippedrather than by a spin, and "it must not fire yet" is N demonstrably completed passes rather than N clock steps and a hope. The clock still moves in poll-sized steps — advancing time is the scenario, polling for the result was the bug — andsettleQuiet/settleUntil/advancePastResumeare gone.Verification, all from the worktree:
vp test run src/coil/autoResume(117 tests) ran clean 6 times after the change,Reactor.test.tsalone 5 times; the package typecheck (tsgo --noEmit) andvp linton the three touched files are clean. A control run on the pristine base reproduced the flake class — 1 of 4 whole-directory runs went red onReactor.test.ts.Known and deliberately out of scope: the three
autoResume/replay/*.test.tssuites still wait onsettleQuiet/advanceStepsturn budgets fromreplay/reactorHarness.ts, and one of them (subagentFanout.test.ts) flaked once under parallel load during this work. Same class, same fix, now mechanical — worth its own change.Closes #134
Claude Opus 5 via a Claude Code subagent did the work.
🤖 Generated with Claude Code