test(comms): pin the ring-lag resync arms and replay-boundary exactness (RIG-3538) - #1123
Merged
trunk-io[bot] merged 1 commit intoSep 12, 2026
Conversation
…ss (RIG-3538) `internal/comms` had no test that drove either resync arm of the comms stream handler: `grep -rn Lagged --include=*_test.go` returned nothing, and of the three conditions that make `bus.Subscribe` return `ErrBufferUnderflow`, only the stale-epoch one was covered. Add five cases. - live-tail overrun (`forwardComms`, the `sub.Lagged()` arm) emits a `CommsResyncRequired` as its FINAL frame, then ends cleanly. Every frame ahead of it must be buffered content, so a degradation that dropped the live tail but still sent the terminal frame cannot pass. - a cursor at or beyond bus head resyncs at the current instance epoch. - a cursor older than the oldest retained event resyncs. A distinct branch from the one above, confirmed by mutation: disabling the eviction guard fails this case while the at/beyond-head case stays green. - replay-boundary exactness: a cursor equal to head replays nothing. Proven with a sentinel published after subscribing, not a sleep. - concurrent publishes are observed in one total seq order by two subscribers, strictly increasing. Export `events.RingCapacity` (was `ringCapacity`). The overrun counts have to exceed it, and hardcoding 1025/1100 against an unexported constant put that invariant beyond the compiler's reach: raising the capacity would not have failed these tests, it would have made one report a misleading message and the other hang. They now derive from the constant. Comments in `internal/delivery`'s busLagFloodCount had the same literal against the same cap, and its comment said the caps were unexported — no longer true, so it derives from the constant now too. The underflow tests bound their stream context. A dropped guard registers a live subscriber that tails forever, which surfaced as a whole-suite timeout; it now fails the single test in 10s with `deadline_exceeded`. The two underflow guards are also covered at the bus layer. Keeping the handler-level pair is deliberate — the handler collapses every underflow trigger into one wire frame, so these confirm each trigger reaches a client as a resync — and the doc comments say so. All five live in the untagged `subscribe_failclosed_test.go` rather than `subscribe_test.go`, which is `//go:build pgtest` and needs a live Postgres DSN. These are handler/bus semantics with no database truth in them. The two underflow cases drive `SubscribeComms` with a nil store: that arm returns before any store access. Each case was proven to bite by mutating production and watching it fail, then restoring byte-identical (5/5), re-confirmed after these changes. The design record calls the subscribe-time arm a "pre-replay lag arm"; it is an `ErrBufferUnderflow` path with no `Lagged()` involved, and it has three triggers rather than one. Record correction noted on RIG-3538. Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
added this pull request to stack #1124
September 11, 2026 23:43
rigel-mintaka
marked this pull request as ready for review
September 11, 2026 23:43
|
Compass engineering docs preview: https://compass-comms-rig3538-ring-l.compass-eng-docs.pages.dev Deployed from |
This was referenced Sep 12, 2026
Merged
mattwilkinsonn
approved these changes
Sep 12, 2026
|
This pull request was merged into |
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.
This PR is part of a stack containing 4 PRs:
maininternal/commshad no test that drove either resync arm of the commsstream handler:
grep -rn Lagged --include=*_test.goreturned nothing, andof the three conditions that make
bus.SubscribereturnErrBufferUnderflow, only the stale-epoch one was covered. Add five cases.forwardComms, thesub.Lagged()arm) emits aCommsResyncRequiredas its FINAL frame, then ends cleanly. Every frameahead of it must be buffered content, so a degradation that dropped the
live tail but still sent the terminal frame cannot pass.
from the one above, confirmed by mutation: disabling the eviction guard
fails this case while the at/beyond-head case stays green.
with a sentinel published after subscribing, not a sleep.
subscribers, strictly increasing.
Export
events.RingCapacity(wasringCapacity). The overrun counts haveto exceed it, and hardcoding 1025/1100 against an unexported constant put
that invariant beyond the compiler's reach: raising the capacity would not
have failed these tests, it would have made one report a misleading message
and the other hang. They now derive from the constant. Comments in
internal/delivery's busLagFloodCount had the same literal against the samecap, and its comment said the caps were unexported — no longer true, so it
derives from the constant now too.
The underflow tests bound their stream context. A dropped guard registers a
live subscriber that tails forever, which surfaced as a whole-suite timeout;
it now fails the single test in 10s with
deadline_exceeded.The two underflow guards are also covered at the bus layer. Keeping the
handler-level pair is deliberate — the handler collapses every underflow
trigger into one wire frame, so these confirm each trigger reaches a client
as a resync — and the doc comments say so.
All five live in the untagged
subscribe_failclosed_test.gorather thansubscribe_test.go, which is//go:build pgtestand needs a live PostgresDSN. These are handler/bus semantics with no database truth in them. The two
underflow cases drive
SubscribeCommswith a nil store: that arm returnsbefore any store access.
Each case was proven to bite by mutating production and watching it fail,
then restoring byte-identical (5/5), re-confirmed after these changes.
The design record calls the subscribe-time arm a "pre-replay lag arm"; it is
an
ErrBufferUnderflowpath with noLagged()involved, and it has threetriggers rather than one. Record correction noted on RIG-3538.
Co-authored-by: Matt Wilkinson matt@rigel.build