fix(connector-core): bound the overflow reprieve so a message cannot cycle forever (#807) - #809
Merged
Merged
Conversation
davidfarah2003
force-pushed
the
fix/807-bound-overflow-churn
branch
4 times, most recently
from
August 24, 2026 01:00
d8628cc to
31186a8
Compare
…cycle forever #793 stopped the overflow valve acking a directed message it evicts, so the broker can redeliver it once there is room instead of destroying it. That turned unrecoverable loss into a delay - but a delay only helps if it ends. An un-acked id is one the broker may hand straight back, into an inbox that is still full, to be evicted again: a cycle that spends throughput while every seat looks healthy. Same defect shape as #790, where drive() retries a failed turn with no backoff and no cap. I filed that against someone else and then shipped it one layer up. Evictions are counted per id. After five the reprieve ends: the message is acked and the drop is reported, because a message lost with a log line beats a mesh that quietly stops moving. The tally clears whenever an id is actually handled, so a message that is redelivered, evicted, then finally lands never accumulates toward the cap, and the map is bounded so tracking churn cannot become its own leak. The report goes to the existing stderr log helper, never emit("error"): an EventEmitter with no error listener turns that into an unhandled exception, so announcing a dropped message would kill the seat that was surviving the flood. A cell asserts that directly.
davidfarah2003
force-pushed
the
fix/807-bound-overflow-churn
branch
from
August 24, 2026 01:25
31186a8 to
3dd3ad8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The overflow reprieve introduced by #793 now terminates.
Why
#793 stopped the inbox overflow valve acking a directed message it evicts, so JetStream can redeliver it once there is room rather than destroying it. That turned unrecoverable loss into a delay — but a delay only helps if it ends.
An un-acked id is one the broker may hand straight back, into an inbox that is still full, to be evicted again. Nothing bounded that. Under sustained overflow the same message can cycle indefinitely, spending broker and connector throughput while every seat involved reports healthy.
Same defect shape as #790, where
drive()retries a failed turn with no backoff and no cap. I filed that against someone else's code and then shipped it one layer up.The fix
Evictions are counted per id. After five, the reprieve ends: the message is acked and the drop is reported. A message lost with a log line beats a mesh that quietly stops moving.
Three details that matter:
drainInboxIds). A message that is redelivered, evicted, then finally lands carries no history toward the cap — otherwise a busy-but-healthy seat would eventually start dropping mail.4 × MAX_INBOX). Tracking a flood must not become the leak. Evicting the oldest tally only forgives a message, never destroys one, so the failure direction is safe.log()helper, neveremit("error"). An EventEmitter with no"error"listener turns that emit into an unhandled exception, so announcing a dropped message would kill the seat that was surviving the flood. A cell asserts this directly.Proof
smoke:overflow-churn-bound, 12/12, registered inci-suites.txtwith the defect story.Cells cover: the reprieve still applying while it is plausible; the cycle stopping at the cap with the loss reported; the report not riding an error event; the tally accumulating, clearing on success, and restarting from zero; the bookkeeping staying bounded under 2000 distinct evictions.
Cell 4 re-asserts #793's own guarantee — a channel flood still cannot silence a DM, evicted channel ambient is still acked. A fix that bounded churn by weakening that would be a regression dressed as a fix.
Neighbours green:
inbox-overflow-directed17/17,history-flood10,inbox-window85,attention13,cross-path-dedup52.typecheckclean,GATE INVENTORY OK.Two mistakes worth recording
Both were caught by this suite before it was trusted:
emit("error")and crashed the process in any seat without a listener. My smoke masked it by registering one — which is exactly why it now captures stderr instead.drainInboxIdsacks handled messages by design. It failed against correct code until it asserted the eviction tally — the mechanism — instead of an ambiguous side effect.Closes #807.