Repository navigation
fix(orchestration): dedupe a bridge read until it settles, not for 250 ms - #1101
Conversation
…0 ms Closes #925. `CachedValue` combined the promise with an `expiresAt` set when the request was CREATED. A read that took longer than 250 ms was therefore "expired" before it had answered, the opportunistic prune deleted it, and the next identical poll enqueued a DUPLICATE. That is not an edge case. The bridge serialises every orchestration request behind ONE in-flight slot with no timer on the queue, and `wait_agents` polls these keys every 250 ms to 1 s by design — the cache's own comment describes twenty children polling the same key in the same quarter-second. So a read waiting its turn collected one duplicate per poll, each queued behind the last: the cache built to prevent a thundering herd produced one. Two questions were conflated in one field, and they are now separate states of it. A PENDING entry is a dedup handle with no freshness to lose, so it is never pruned and is always joinable; a SETTLED one is a value, and its window starts when it arrived. Republishing on settlement is conditional, which is the half that is easy to get wrong: if a mutation invalidated the key while the read was in flight, the entry is gone and must not come back — that answer describes the world before the change, and a caller polling after the mutation has to reach the renderer. Tests drive the real bridge over its real queue. They count requests PER PARENT rather than by the length of the module-global array, because the bridge dispatches from a queue and a request another test enqueued can land after that test reset it — a cross-test leak made one of these mutations look dead before the counting was fixed. Mutations, all killed: prune pending reads again 2 failed; never join a pending entry 2; freshness starts at request time again 2; republish after an invalidation 1; a settled read never becomes a value 2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round-one review findings on #1101. The implementation survived every attack; 8 of 19 mutations did not die, and three of them let you silently revert half the fix. - The identity-guarded `.catch` delete is now the ONLY thing that removes a pending entry, because prune skips `expiresAt === null` by design. Four mutations of it survived, and one is not transient: with the delete removed a single renderer error poisons that key until the app restarts, where before #925 prune healed it in 250 ms. Four tests now pin it — a failed read is retried rather than joined, and a late rejection from a superseded generation leaves the newer entry alone — on both caches, because the two catch blocks are separate code. - Every readRunOutputs mutation survived: its pending expiry, its join condition and its prune guard could each be reverted with the suite green. It is also the read that matters most — transcript slices, and one at the end of every wait_agents. - The cap-param key carries an emphatic comment about one caller silently receiving another's differently-truncated payload, and had no test. - #925 asked for the pending-key count to be measured rather than a speedup claimed: 500 polls across 20 keys leave 20 entries, not 500. Also, three things the review was right to call out in prose: - The `requestsFor` rationale asserted a cross-test leak as fact. The reviewer instrumented the mock and could not reproduce it — no test in this file ends with a queued request. Rewritten as what it is: a guard against a reachable shape. - `startFreshnessOnSettle` did not mention that a joined caller inherits the original dispatch's 30s deadline. It is better than the 60s it replaces, but wait_agents shows that error text to the model. - `closeRun` takes its tombstone snapshot through the cached read, so the join window it can fall into grew from 250ms to the whole flight of an earlier read. Narrow, and cheaper than the alternative — now written down. Nine mutations re-measured individually, all killed: list catch guard dropped, list catch removed, rro join refuses pending, rro pending expires at creation, prune kills pending (rro / list), key ignores char caps, rro catch guard dropped, rro catch removed. `npx tsc -b` clean on Node 24. src/main + src/mcp, unit + system: 1854 passed, 1 failed — src/main/workflows/control.system.test.ts, untouched by this branch and failing identically on main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review round one — addressed in F1 — error path load-bearing, permanent, untested. Four tests, two per cache, because the two
F2 — readRunOutputs half untested. Mirrored the in-flight dedup test. M1b, M2b and M3c all die now. F3 — cap-param keying. Five reads differing only in caps produce four distinct pending entries; a repeat of the first set joins. M6/M6b die. F7 — #925's measurement. Landed as your probe E: 500 polls across 20 keys → exactly 20 pending entries, one request in flight. That is the number that answers "a pending entry is never pruned", so it belongs in the file rather than in a comment. F4 — the F5 — inherited deadline. Documented on F6 — The M10. Agreed and not touched — Nine mutations re-measured individually against a verified-clean tree (all killed):
🤖 Generated with Claude Code |
…erge Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #925 (planning finding F12 from #918).
The defect
CachedValuecombined the promise with anexpiresAtset when the request was created:So a read that took longer than 250 ms was "expired" before it had answered, the opportunistic prune deleted it, and the next identical poll enqueued a duplicate.
That is not an edge case. The bridge serialises every orchestration request behind one in-flight slot with no timer on the queue, and
orchestration_wait_agentspolls these keys every 250 ms–1 s by design — this cache's own comment describes twenty children polling the same key in the same quarter-second. A read waiting its turn collected one duplicate per poll, each queued behind the last: the cache built to prevent a thundering herd produced one.It is also invisible from outside:
MAX_ACTIVE_RENDERER_REQUESTS = 1means the duplicates queue rather than fly, so nothing is observable until the first read settles and the queue drains into them.The fix
Two questions were conflated in one field, and they are now two states of it:
expiresAt === null) — a dedup handle. It has no freshness to lose, so it is never pruned and is always joinable. It leaves the cache on settlement, as a value, or on failure (the existing.catchremoves it so the next caller retries rather than joining a rejection).Republishing on settlement is conditional, which is the half that is easy to get wrong: if a mutation invalidated the key while the read was in flight, the entry is gone and must not come back. That answer describes the world before the change, and a caller polling after the mutation has to reach the renderer.
Tests
Three new cases drive the real bridge over its real queue: a read still in flight across four polling intervals (one renderer request, not four), freshness starting at settlement rather than request time, and a mutation invalidating a read that is still in flight (its settlement must not refill the cache).
They count requests per parent session rather than by the length of the module-global array — the bridge dispatches from a queue, so a request another test enqueued can land after that test reset it. That leak made one of these mutations look dead until the counting was fixed, which is recorded at the helper.
Mutations, all killed:
npx tsc -bclean;src/main+src/mcp219 files / 1847 tests pass. The one failure,workflows/control.system.test.ts, reproduces identically on unrelated branches offmainand is green in CI.Not covered here: #926 (retaining mutation authority after a response timeout) is the sibling finding and a different mechanism — this PR does not touch the timeout path.
🤖 Generated with Claude Code