π fix(contribute): make a dropped SSE event observable β seq on every frame, a gap frame when events are lost - #6219
Open
Danathar wants to merge 1 commit into
Open
Conversation
/api/contribute/events discards an event for a subscriber whose channel is full and leaves the connection open. Dropping rather than blocking is deliberate and stays β a slow observer must never back-pressure the hub's event path, which is worker assignment. What hivecommons#6218 reported is that the loss was UNOBSERVABLE: the connection stayed open, the 25-second heartbeat kept it looking healthy, and a client could not tell "nothing happened" from "I missed events". The `hello` frame's replay repairs a gap only on a FUTURE reconnect, so a monitor that never reconnects never learns. For an unattended observer that is the dangerous failure: stale data rendered as current. Two additive fields, no behaviour change to the drop itself. - Every frame carries `seq`, a monotonic stream position assigned ONCE at fan-out, so the same event has the same number for every subscriber. A jump from 41 to 45 means three events never arrived. `hello` reports the position the subscriber registered at β captured under the same lock as the registration β so a client has a baseline and its first `activity` should be seq+1. - A discarded event bumps a per-subscriber counter, and the HTTP writer turns a non-zero counter into a `gap` frame naming how many went and where the stream has reached. The reporter offered three options and preferred (3), closing the subscriber. This does (1)+(2) instead. Closing turns a cheap discard into a reconnect, and a reconnect here is not cheap: the hello frame runs a full admission sweep and a 150-item queue snapshot, so a flapping slow client would trade silent data loss for a sweep storm on the hub β and the existing comment's "a momentary drop is self-healing" would stop being true. A signal leaves the choice with the client, which is the one that knows whether it can afford to re-sync. Mechanics worth noting: - The counter is atomic, not registry-locked, because the writer must never take the registry lock β the whole point of the non-blocking send is that a slow client cannot reach the hub's event path. - Swap-to-zero, so a gap is reported exactly once and a drop landing between the read and the write is carried into the next frame rather than lost. - The check runs on both loop branches. In practice the EVENT branch fires: a drop implies a full channel, so ~32 queued events sit behind it and the check runs before each pop. The heartbeat branch is the guarantee β it makes "reported" unconditional rather than contingent on another event ever arriving, which is exactly the quiet-stream case the report is about. - Ordering is stated honestly in the code: the frame means "you are missing events", not "the ones after this". The discarded event was newer than the queued ones, so it fires while the client is still draining good ones. That is the right side to err on, and it is why `seq` matters β the numbers locate the discontinuity, the frame is the prompt to go looking. Existing clients ignore both fields; `omitempty` keeps a stream that has broadcast nothing serialising exactly as before. The /contribute page, which already polled as a hedge, now uses the signal to re-sync at once instead of waiting out its 6s timer. Tests: six cases covering the numbering, the recorded drop, the end-to-end gap frame over a still-open connection, the idle-stream heartbeat path, and a healthy stream gaining neither a gap nor a discontinuity. Mutation-checked β removing the drop counter, the numbering, the start position, or either gap check turns the suite red. An earlier draft of the idle test drove broadcast and silently exercised the event path instead; the surviving mutant is what caught it, and the test now marks the pending drop directly with that reasoning recorded. sseHeartbeatInterval becomes a var solely so that branch is reachable without a 25-second wait. Race detector clean. Closes hivecommons#6218 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UQTim25GU8Yk2HrCh39i1u Signed-off-by: Douglas Baggett <doug.baggett@gmail.com>
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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.
Summary
A client of
/api/contribute/eventscan now tell "nothing happened" from"I missed events".
Dropping an event for a subscriber whose channel is full stays exactly as it was
β a slow observer must never back-pressure the hub's event path, which is worker
assignment. What changes is that the drop is no longer invisible:
seq, a monotonic stream position, andgapframe naming how manywent.
The connection stays open and reconnect-with-replay is still the recovery path.
What was missing was any way for the client to know it should take it.
Closes #6218.
Why this shape
@castrojo offered three options and preferred (3), close the subscriber.
This implements (1) + (2) instead, and I want to be explicit about the
trade-off rather than quietly picking a different one.
Closing turns a cheap discard into a reconnect, and a reconnect here is not
cheap: the
helloframe runs a full admission sweep plus a 150-item queuesnapshot. A flapping slow client would trade silent data loss for a sweep storm
on the hub, and the existing "a momentary drop is self-healing" comment would
stop being true. A signal leaves the decision with the client β the one that
knows whether it can afford to re-sync β and a client that wants option (3)'s
semantics can implement them in three lines on top of the
gapframe.What a client sees
seqis assigned once at fan-out, so the same event carries the same numberfor every subscriber β a per-subscriber counter would be useless for comparing
two clients or reasoning about the stream.
hello.seqis captured under thesame lock as the registration, so the position and the set of events the
subscriber will receive cannot disagree.
Both fields are
omitempty, existing clients ignore them, and a stream that hasbroadcast nothing serialises exactly as before.
Mechanics worth review
registry lock β the entire point of the non-blocking send is that a slow
client cannot reach the hub's event path.
the read and the write is carried into the next frame rather than lost.
the event branch fires: a drop implies a full channel, so ~32 queued events
sit behind it and the check runs before each pop. The heartbeat branch is
the guarantee β it makes "reported" unconditional rather than contingent on
another event ever arriving, which is precisely the quiet-stream case the
report describes. The code says this plainly rather than implying both are
equally load-bearing.
missing events", not "the ones after this are what you missed" β the discarded
event was newer than the queued ones, so it fires while the client is still
draining good ones. Telling a client early that it is behind beats telling it
late, and it is exactly why
seqmatters: the numbers locate thediscontinuity, the frame is the prompt to go looking.
Testing
Six cases: the numbering (monotonic, and identical across subscribers), the
recorded start position, the recorded drop, the end-to-end
gapframe over astill-open connection, the idle-stream heartbeat path, and a healthy stream
gaining neither a gap nor a discontinuity.
Mutation-checked β removing the drop counter, the numbering, the start
position, or either gap check turns the suite red:
That table cost me a rewrite, and it is the reason I trust it. My first
idle-stream test drove
broadcastto create the drop β which fills the channel,so the writer reported the gap on the event path while the test's name claimed
it covered the heartbeat. Removing the heartbeat check left it green. The
surviving mutant is what surfaced that; the test now marks the pending drop
directly on the subscriber, with the reasoning recorded in the test so the next
person does not "simplify" it back.
sseHeartbeatIntervalbecomes avarsolely so that branch is reachablewithout a 25-second wait β the same seam
CACertPathuses inpkg/proxy;production never reassigns it.
Also run: full
pkg/dashboardpackage green;-raceover the SSE tests with-count=2clean (this adds concurrent state, so that felt mandatory);golangci-lint0 issues for the package. No new routes, so the OpenAPIroute-parity guard is untouched β I updated the existing
/api/contribute/eventsdescription and response text to document the frameshape and the gap contract.
Client
The
/contributepage β which already polled/fleetas a hedge, one of thetwo workarounds the issue names β now handles
type:"gap"by re-reading thereliable endpoints immediately instead of waiting out its 6s timer. It is a
small win there; the clients this actually rescues are the headless ones like
projectbluefin/reviewthat trusted the stream and had no way to learn theywere behind.
@castrojo β you offered to test against your downstream monitor, and the two
fields are exactly what that needs: track
seqfor precise detection, or justwatch for
type:"gap"and re-sync. Happy to adjust the shape if it does not fit.β hive: backend=claude model=claude-opus-5