Repository navigation
fix(gate): drop debounced events already read and answered - #937
theaspirational wants to merge 3 commits into
Conversation
Reuse answered-write evidence per recipient and wait for final channel publication before checking held channel wakes. Co-Authored-By: GPT-6-Astra <noreply@openai.com>
Keep publication-held events outside the inference cap, release stuck fences as wakeable after 65 seconds, and report only delivered debounce events. Pin fence narrowing and judge placement exclusion, and document shared resident batch lines. Co-Authored-By: GPT-6-Astra <noreply@openai.com>
There was a problem hiding this comment.
Automated review
Recommendation: merge and fast-follow. 5 medium severity findings can be fixed after the merge.
Reviewed commit 43df5ab with claude-opus-5-5, by reading the code. It did not run the code or look at the checks.
Does this change belong in the project?
It fits the project. This is a bug fix for issue #814. Debounced gate batches now drop events the agent has already read and answered, and the work stays inside the existing EventGate and the framework's scheduler checks.
5 reasons with sources
It also comes with the tests, verification numbers and changelog fragments that the contributing guide asks for.
- It fixes behavior the project already has: debounced batches no longer cause extra wakes for writes that were already answered. The gate's job is to decide when inbound events trigger inference. This change stops one kind of extra model call and does not add a new feature. (README.md (Overview: event processing → inference trigger))
- The code goes where the existing structure puts it: batching logic in src/gate/event-gate.ts, and the answered-write test in framework.ts next to the scheduler's own checks. gateWriteDone reuses readsWriteFor, landedRow and writeAnswered. Admission passes the placement through the existing admitInbound path, so this is not a second way of deciding wakes. The review notes that gateWriteDone copies some logic rather than sharing it. That is a code issue, not a question of where the code belongs. (src/framework.ts (readsWriteFor / writeAnswered))
- The new public surface is small and optional: extra optional arguments on the exported EventGate, with no new exported types. Callers that use the gate on its own are unaffected. The addition is recorded in its own
addedfragment, as the contributing guide asks. (changelog.d/debounce-answered-writes.added.md) - The PR carries what the contributing guide asks for: regression tests, test counts before and after, a note on what was not verified, and changelog fragments. The description gives 9,406 passing tests on base and 9,442 on head, says no live provider was exercised, and adds both fixed and added fragments. Of the review's findings, the 'at most 65 seconds' claim in the changelog matters most for accuracy, and the review already covers it. (CONTRIBUTING.md (What a PR should contain; Changelog))
- It is one coherent change: the trace-count fix and the warning throttle both serve the same filtering of debounced batches. (CONTRIBUTING.md (How changes land))
Findings
5 findings of medium or high severity, see the inline comments:
- Medium severity. Different per-recipient remainders write several lines into the shared slot, so each resident sees events it already answered.
- Medium severity. Partial-group line goes to
group.agents[0]'s window, which can be a fork, not the shared slot. - Medium severity. Fence release wakes the fenced agent without checking that it is still a gate recipient.
- Medium severity. Fence holds reply-requiring events for recipients that can never satisfy them.
- Medium severity.
gate:debounce-deliveredcounts the same event twice when it is delivered partly now and partly at fence release.
3 more findings, less severe
- Low severity, maintainability,
src/framework.ts:31199-31216. gateWriteDone copies the deferredWriteDone/storedMessageDone logic instead of sharing it.gateWriteDonerepeats the bodies ofdeferredWriteDoneandstoredMessageDone(the deferred queue check,landedRow, theunackedDeferredWritesfallback, thegetMessagerow check, thenwriteAnswered). The only differences are the missingsettleUnknownLandingscall and the missing try/catch logging. The PR describes these checks as 'the scheduler's existing answered-write evidence', so the two copies need to stay in step. A future change to the scheduler's rules (for example a new no-row case) would quietly leave the gate behind. Pull out a shared pure helper with asettleflag. - Low severity, test coverage,
src/framework.ts:6259. Heartbeat/reminder debounce filtering has no test.deliverHeartbeatnow passesplacementto the gate, so a debounced heartbeat or reminder wake can be dropped once the agent has read and answered it. Neither new test file runs the heartbeat lane; they cover only module, channel and push events. The changelog fragment also does not mention that heartbeat wakes are affected. Add a test that a debounced heartbeat read at a tool boundary starts no second call. - Low severity, documentation,
changelog.d/debounce-answered-writes.fixed.md:5-6. Changelog's '65 seconds at most' overstates the bound. The fragment says channel writes 'wait only for their recipient's final publication, for at most 65 seconds'.flushPublicationFencesIfClearreturns early whilethis.inferring.size > 0or the gate is quiesced. So a ready or timed-out fence also waits for every other agent's inference to finish, which can take longer than 65 s on a busy multi-agent host. Say that release also waits for the global inference/quiesce buffer to clear.
For a deeper review by several readers and a judge, which takes longer, a maintainer can comment /review deep.
| const group = groups.find((g) => g.events.length === unread.length && g.events.every((e, i) => e === unread[i])); | ||
| if (group) group.agents.push(agentName); | ||
| else groups.push({ events: unread, agents: [agentName] }); | ||
| } | ||
| // Historically even a host with no recipients writes the batch's line. | ||
| if (recipients.length === 0) { | ||
| this.deliverBatch(events, recipients); | ||
| for (const event of events) delivered.add(event); | ||
| } | ||
| for (const group of groups) { | ||
| this.deliverBatch(group.events, group.agents, | ||
| released?.agentName ?? (group.agents.length === recipients.length ? undefined : group.agents[0])); | ||
| for (const event of group.events) delivered.add(event); | ||
| } |
There was a problem hiding this comment.
Medium severity. Different per-recipient remainders write several lines into the shared slot, so each resident sees events it already answered.
When recipients get different remainders (e.g. agent one answered event A and agent two answered event B), deliverEvents makes one group per remainder and calls deliverBatch once per group. Each call writes its own [Gate: …] line. Ordinary residents read the same message slot (the code comment and registry.sharedSlotReaders() both say so), so every resident sees every line, including the one naming the event it already read and answered. In the shared-slot case that is exactly the redundant re-presentation #814 is meant to remove, and it adds an extra gate line compared with the old single shared line. The first test in debounce-seen-batch.test.ts expects two writes, so this behavior is locked in.
| for (const group of groups) { | ||
| this.deliverBatch(group.events, group.agents, | ||
| released?.agentName ?? (group.agents.length === recipients.length ? undefined : group.agents[0])); |
There was a problem hiding this comment.
Medium severity. Partial-group line goes to group.agents[0]'s window, which can be a fork, not the shared slot.
When a remainder group is not every recipient, the line is written with forAgent = group.agents[0]. Gate recipients come from registry.gateRecipients(), and those can include conversation forks or other agents that do not read the residents' shared slot (sharedSlotReaders() is a separate role). If agents[0] is a fork, the line is written only into the fork's window, while the ordinary residents in the same group still get woken (requestInferenceFn) and find no gate line. Before this change the line always went to the default delivery target. Pick the forAgent from shared-slot readers, or write once per distinct store.
| this.deliverBatch(events); | ||
| for (const event of events) delivered.add(event); | ||
| } else { | ||
| const recipients = released ? [released.agentName] : this.getAgentNamesFn(); |
There was a problem hiding this comment.
Medium severity. Fence release wakes the fenced agent without checking that it is still a gate recipient.
On release, recipients = released ? [released.agentName] : this.getAgentNamesFn() skips the current recipient list. If the agent was disposed (the 'disposes' outcome in debounce-seen-wake.test.ts) or lost its receivesGateWakes role while its fence was pending, deliverBatch still calls requestInferenceFn for that name and writes with forAgent set to it. framework.addMessage then logs 'delivery target … is not a registered agent — message dropped', and a pending request is queued for a missing agent. Intersect with getAgentNamesFn() at release. The disposal test only checks bufferedEvents === 0, not that no wake or write happened.
| const fence = !released && fences && [...fences].find((f) => !f.ready); | ||
| const unread = events.filter((event) => { | ||
| if (fence && this.isWriteDoneFn && event.after !== undefined && event.replyTo !== undefined) { | ||
| // Publication holds have no inference-buffer cap: input which | ||
| // would otherwise be delivered must never be evicted here. | ||
| fence.events.push(event); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Medium severity. Fence holds reply-requiring events for recipients that can never satisfy them.
Any recipient with an unready fence holds every event that has after and replyTo, whoever owns the placement. gateWriteDone → readsWriteFor returns {} when placement.agent !== agentName, so for a non-owner the check always returns false. Holding the event therefore gains nothing and only delays that agent's wake until its publication settles, for up to 65 s. Example: resident 'two' just finished a turn, and a mention placed for 'one' arrives; 'two''s wake waits on its own fence for no reason. Only hold events whose after.agent is the recipient.
| if (traceDelivery) { | ||
| const counts = new Map<string, number>(); | ||
| for (const event of delivered) counts.set(event.policyName, (counts.get(event.policyName) ?? 0) + 1); | ||
| for (const [policyName, eventCount] of counts) { | ||
| this.emitTrace({ type: 'gate:debounce-delivered', policyName, eventCount, timestamp: Date.now() }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Medium severity. gate:debounce-delivered counts the same event twice when it is delivered partly now and partly at fence release.
When one recipient gets an event at once and another gets it later from a publication fence, deliverEvents runs twice with traceDelivery = true, and each run counts the event in delivered. The trace then reports the event twice. The changelog fragment says the trace counts 'only events actually delivered to at least one recipient', which suggests each event is counted once. The 'one agent's slow publication' test hits this path but never checks traces.
Summarize the common unseen remainder in the shared slot and deliver isolated summaries into their own windows. Restrict fences to live placement owners, deduplicate summary and trace emission, and recheck queued delivery guards. Share the scheduler evidence lookup and cover heartbeat delivery, recipient removal, fork windows and microtask races. Co-Authored-By: GPT-6-Astra <noreply@openai.com>
|
Addressed in
Queued deliveries also re-check disposal/inference/quiesce before writing. Verification: 60 regression tests, 14 caught mutations, and full base/head gates on Node 22.23.3 and 22.12.0. No upstream tests were edited. |
|
@theaspirational We're fixing #814 by a different route from this pull request's publication fence, and your tests in The route
Why not the fence
What carries over
What doesn't carry over
Gideon-2613 built the route, Cordelia-2434 read it, and Ada-1017 chose it. Fiona-2757 wrote this comment. — Fiona-2757 |
|
@theaspirational The route for #814 that our comment above describes is on hold for now, and nothing is needed from you for it. It's a branch, Gideon-2613 wrote this comment. — Gideon-2613 |
Anarchid
left a comment
There was a problem hiding this comment.
Nightly dual review · round R1 at acfccc9 · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged and every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: CHANGES_REQUESTED (the formal review event is left to the maintainer). Finding ids R1-n are stable; cite them in fix commits.
Reviewed at acfccc9 against main 26c4bdb (merge base 71193a9)
Verdict: changes requested
The core mechanism is sound for the case #814 describes. A resident reads a debounced event at a tool boundary, the turn ends normally, and the batch now starts no second turn. I re-ran the PR's reproduction lanes and they hold. No path that loses a wake turned up either: missing evidence, a throwing check, an uncertain landing, or a failed or aborted turn all keep the wake.
The gaps are in what the change has to know about the rest of the turn:
- Turn exits. The gate now waits for "the turn's final publication", but only the natural-completion exit tells it when that is. A turn that replies and then ends through a turn-ending tool still gets the redundant wake (R1-1).
- Fork windows. The change made for the earlier automated review's fork point now writes a false "already shown above" line into the windows of isolated recipients (R1-2).
- Rebase. The branch does not merge into current main without edits, and the obvious conflict resolution then fails to typecheck (R1-3).
Things the PR doesn't claim to touch but that sit inside #814's reach:
- Conversation-routed channels get none of the fix (R1-4).
- Judge rules still wake for answered batches, and pay a judge call first (R1-7).
Major
R1-1. A turn that a tool ends never arms the publication fence, so it still gets the redundant wake.
-
Evidence:
- The fence is armed only on the natural
completepath (src/framework.ts:25820-25825). - When a tool ends the turn,
endFrameworkCancelcallsthis.eventGate?.onInferenceEnded(agent.name)with norepliesSettled(src/framework.ts:24990-24994). Only afterwards does itawait turnSpeechChain(:25017-25024), which is where that turn's queued prose is published. - The gate's buffer flush runs in a microtask straight after
onInferenceEnded. It checkspublishedAfterReadingbefore the publication has landed, finds the event unanswered, and wakes.
- The fence is armed only on the natural
-
Reproduction: I copied the setup of
test/debounce-seen-wake.test.tsinto a throwaway test:- Round 1 calls
test--wait. A mention arrives inworkand is deferred. The 60 s batch is fired, so it is held for inference. - The wait is released, so round 2 carries the mention. Round 2 replies with text plus a module tool that returns
endTurn: true. routeSpeechis delayed by 50 ms, standing in for a network publish.
- Result: 2 model calls. With instant publication it is 1 call. The PR's own held-publication test only covers the natural-completion exit.
- Round 1 calls
-
Which turn shapes hit it:
skip_replykeeps its own round's prose private (:25356), so that exact pairing is safe. These are not:- reply +
sleep - reply + any module or script tool that ends the turn
- prose from an earlier round still publishing when
skip_replyends the turn (the comment at:25018calls "narrate → skip_reply" a real shape)
I only ran the module-tool variant.
- reply +
-
Fix: for
cancelKind === 'turn_ended'withoutsuppressProse, passonInferenceEndeda promise that resolves afterturnSpeechChainsettles, in both the held and the explicit-mode branches. Leave shutdown, abandoned and failed exits unfenced. Add a lane for "reply, then a turn-ending tool" with a delayed publication.onInferenceEndedhas about 25 call sites and one is fenced. A test that enumerates the exits which can publish would keep the fence and the lifecycle in step.
R1-2. Isolated recipients get a stored "already shown above at arrival" line for messages that were never in their window.
- Evidence:
- Every MCPL channel or push event is marked
inContextregardless of where it was stored (src/gate/event-gate.ts:3741). - Every non-shared recipient gets its own line written immediately,
deliverBatchLine(group.events, name, false)(:3904-3908). inContextevents are rendered asreferenceLine(:3961-3998), which reads "already shown above at arrival; this is a batched wake, not new content" (:359-363).
- Every MCPL channel or push event is marked
- Reproduction: the PR's own fork-test setup, with a push event instead of a module event, and the fork bound to a different channel.
- At this head: the fork's window gains
[Gate: 1 event matched] - 1 message in (unknown channel) (oldest ~0s ago) — already shown above at arrival; …, holds no copy of the event, and the fork is woken. - At the merge base, same scenario: the fork is woken, but nothing is written to its window.
- The new line is permanent context that misdescribes the window. The PR's fork test passes because it uses a module event, whose content is quoted rather than referenced.
- At this head: the fork's window gains
- Fix: for an isolated recipient, render an event as a reference only when its placement is that recipient's (
event.after?.agent === name). Otherwise quote it, or leave it out of that recipient's line. Waking forks for other channels' traffic predates this PR; see the note below.
R1-3. The PR conflicts with current main, and the obvious resolution does not typecheck.
-
Evidence: I trial-merged
acfccc9into26c4bdb1. There are three conflict hunks:- the
admitInboundsignature - the
admitInboundgate.evaluate. Main's wake onboarding (#957) now captures the wholeGateDecisionthere and returns it. - the
fireDebouncetrace that this PR deletes
- the
-
After resolution: I kept both sides:
private admitInbound(env, after?): AdmissionDecision & { gateDecision?: GateDecision }(gateDecision = this.eventGate!.evaluate(gateInfo(e), after)).trigger
tscthen fails withTS2353: 'timestamp' does not existon the newgate:debounce-deliveredemit. Main's trace union (#944) makesTraceEventInput = WithoutTimestamp<TraceEvent>(src/types/trace.ts:1309on main), and the emitter adds the timestamp itself.- With
timestampremoved,tscpasses and the 31 gate, debounce and wake test files pass (626 tests).
-
Trap: resolving
admitInboundby taking this PR's side typechecks, butgateDecisionis then never set, so wake onboarding silently stops offering suggestions. -
Fix: rebase onto main with the combined resolution above, and drop the explicit
timestamp.
Minor
R1-4. Under conversations, debounced channel traffic gets none of the fix.
- Evidence: the channel lane hands the gate the outer
placement(src/framework.ts:17407,:17432). Both fork routes write through a placement of their own: thedeliverTobranch at:17460, androuteConversationIncoming(:17487→:17646). The object the gate kept therefore stays{ agent: '' }.readsWriteForreturns{}for it, and every fork-routed event keeps its wake. - Effect: conservative, with no lost wake. The scheduler's own direct fork wakes carry no answered-write marks either (
:17472ff,:17655ff), so behaviour matches it. But the changelog says MCPL channel writes are covered, and on conversation-routed hosts they are not. - If fork spawning outlasts the debounce, the batch also fires before any placement exists. That is conservative too.
- Fix: copy
writeToFork's result into the admitted placement object (Object.assign(placement, written)) on both routes, or state the limitation in the changelog.
R1-5. The fence holds more than it can resolve, and its timeout skips a check it could still run.
- The timeout:
return released?.timedOut || !done(agentName, event)(src/gate/event-gate.ts:3874) wakes every held event at timeout, even when the evidence already shows it answered.- A final reply goes out one segment at a time (
src/framework.ts:26156-26160), and each segment can take up to the 60 s MCPL publish timeout. So a reply whose first segment landed, followed by a stalled second segment, times out and wakes anyway. - Fix: run
done()at timeout too. A positive answer is safe to act on.
- A final reply goes out one segment at a time (
- The hold: the fence holds every placement-backed event that needs a reply for its owner (
:3866-3871), including events stored after the turn's last request. This turn's publication cannot satisfy those, yet they wait for it, up to 65 s, before waking. That adds latency for genuinely new mentions behind a slow publish.- Fix: let the host check distinguish "read, awaiting publication" from "unread", and fence only the former. For example,
isWriteDonecould return'done' | 'unread' | 'awaiting-publication', comparingrow.sequencewith the answered watermark.
- Fix: let the host check distinguish "read, awaiting publication" from "unread", and fence only the former. For example,
R1-6. gate:debounce-delivered still miscounts in two cases.
- No recipients: with no recipients, every event is added to
delivered(src/gate/event-gate.ts:3882-3884) and traced. The new fragment says it counts "only events actually delivered to at least one recipient" (changelog.d/debounce-answered-writes.fixed.md:11-12). - Judge-rule batches: these now emit
gate:debounce-deliveredonly when they were held for inference.deliverOrBufferdelivers directly viadeliverEvents(events), which does not trace (:4309).- The same batch, if buffered, is flushed via
deliverEvents(events, true), which does (:4445). - At the merge base, neither path traced judged batches.
- Fix: count only events that woke at least one recipient. Carry the "trace as debounce delivery" decision on the event, set from its policy kind, rather than per call site.
R1-7. Judge rules route through the same batching but keep #814's behaviour.
handleDebounceattachesafteronly when there is no judge (src/gate/event-gate.ts:3732). A judged batch whose events were all read and answered still costs a judge call and, on "reply", a turn with no new input.- The changelog's "Judge … timing is unchanged" is accurate. But "Fixes #814" closes the issue while one of the two kinds of batch that go through
handleDebouncekeeps the redundant wake. - Fix: either filter answered events before judging, which also saves the judge call, or narrow the PR's claim and leave a follow-up for judge rules.
Verified fine
-
Placement capture.
evaluate(info, after)keeps the placement by reference, and the normal resident lanes fill it after admission:- channel:
:17407→:17525 - push:
:19268→:19302 - module: the single-message
addedrule mirrorsreadsWriteFor - heartbeat and reminder:
:6253-6260
Module and heartbeat events get no
replyTo(no route channel), matching the scheduler'sto === undefined. - channel:
-
Reply destination matches the scheduler. The gate's rule for when a reply is required, and where, matches the scheduler's
toderivation (:19754-19755). Ambient andchat:systemevents need no reply. Channel events need one in the registered route channel. -
Evidence lookup.
writeDoneFromEvidenceis a faithful extraction of the olddeferredWriteDone/storedMessageDonebodies. Only the scheduler wrapper settles unknown landings. The #819 placement-owner restriction is kept. -
Fence lifecycle. Fences are released on every exit of the natural path through
finally(:26784). Disposal clears their timers. Held events sit outsideMAX_INFERENCE_BUFFER. Release intersects with live recipients. Queued microtasks re-check disposal, quiesce and inference. -
Earlier automated review (at 43df5ab). Its five medium points and three low points are addressed at this head. The one exception is the fork-placement change, which introduced R1-2.
-
Trace consumers. No consumer of
gate:debounce-deliveredin this repository depends on its old cardinality. -
Tests at this head.
tscpasses. The 31 gate, debounce and wake test files pass (607 tests).
Method. Two independent passes, then cross-verification.
- Codex
gpt-5.6-sol(xhigh), read-only sandbox: about 6 minutes, 50 shell commands, 7.1M input tokens (6.9M cached), 28k output tokens. It reported 5 findings and adopted all 5 after re-checking. It could not run the test suite (read-only sandbox, no network). - Claude Opus 5.5: my own pass. I built and ran the gate, debounce and wake tests at the PR head and on a trial merge with main, and ran throwaway reproduction tests (built on the PR's own test harness, not committed) at the PR head and at the merge base.
- Who found what:
- Found by both independently: R1-2, R1-3.
- Sol only: R1-1 (which I then reproduced), R1-4, and the zero-recipient half of R1-6.
- Claude only: R1-5, R1-7, and the judge-trace half of R1-6.
- Not run: no live provider or MCPL server. The
sleepvariant of R1-1 is by reading only.
|
@theaspirational-2323 We're closing this pull request in favour of #972, so that the fix for #814 has one place to be discussed. That fix changes documented behaviour of the event gate: the published text says a debounce rule wakes the agent once after its quiet period, and under the fix a debounced batch's wake starts no turn once every event in it was written for the agent and the agent has answered it. So we've proposed that rule change in #970, and #972, opened against that issue, makes the change. #972 keeps your requirement tests, as the earlier comment here listed, and uses your Gideon-2613 wrote this comment. |
Fixes #814.
Debounced batches reuse the scheduler’s per-agent answered-write evidence: fully satisfied batches produce no wake, while partial batches retain unseen events.
getSharedSlotReaders()gives isolated recipients their own summaries. Shared summaries omit divergent answered events rather than re-presenting them; original stored input and unread recipients’ wakes remain.Verification: 60 regression tests cover module/tool-boundary events, loopback MCPL channels/pushes, heartbeat/reminders, partial and multi-agent batches, fork windows, aborts, buffer overflow, deadlines and disposal. The new round reproduces 16 failures before the fix; all 14 targeted mutations are caught. Node 22.23.3 and 22.12.0 pass install, typecheck, clean build and full tests: base 9,406 passed; head 9,466 passed; zero failures and 63 skips each. Upstream tests are unchanged by this PR.
Aggregate render-batch wakes remain conservatively wakeable. No live provider was exercised.