Repository navigation
relay: a resume outlives the downstream that asked for it - #123
Merged
Merged
Conversation
propagateForwardUpstream ran the §7.2 REQUEST_UPDATE FORWARD=1 under the triggering downstream's context. A subscriber that left while the relay awaited REQUEST_UPDATE_OK cancelled the wait, so SetForwardState(1) was skipped although the publisher had already resumed. The relay then recorded the upstream as paused, and the pause the leaving subscriber scheduled (Config.PauseUnwatchedPublishes, 85049b6) found nothing to do: the publisher kept forwarding a track nobody watched. Run the resume detached from ctx, as the pause already is; each round trip stays bounded by UpstreamSub.Update's own timeout. The explicit WithoutCancel at the late-publisher call site is now redundant. This is what made TestRelay_UnwatchedPublishPaused fail ~15% of runs (36/200 on draft-22). TestRelay_UnwatchedPublishPausedAfterAnInterruptedResume holds the publisher's REQUEST_UPDATE_OK until the subscriber has left; verified red before the fix (20/20), green after (100/100). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Fixes the flaky
TestRelay_UnwatchedPublishPaused, which failed 36 of 200 runs ondraft-22and caused #122's first CI failure. The flake was a real bug in 85049b6.Bug.
propagateForwardUpstream(the §7.2 resume, REQUEST_UPDATE FORWARD=1) ran under the context of the downstream that triggered it. If that subscriber left while the relay waited for REQUEST_UPDATE_OK,Updatereturnedcontext.CanceledandSetForwardState(1)was skipped, even though the publisher had already resumed. The relay then recorded the upstream as paused. The pause that the leaving subscriber had scheduled saw "already 0" and never sent FORWARD=0, so the publisher kept forwarding a track nobody watched. I confirmed this with a temporary diagnostic log: each failing run loggedwant=1 err="context canceled".Fix. Run the resume with
context.WithoutCancel(ctx), the same way the pause path already runs. Each round trip is still bounded byUpstreamSub.Update's own 5s timeout. I didn't add an outer timeout, because one shared budget across several upstreams would bring the same mismatch back. The explicitWithoutCancelat thehandler_namespace.gocall site is now redundant, so I removed it.Test.
TestRelay_UnwatchedPublishPausedAfterAnInterruptedResumeholds the publisher's REQUEST_UPDATE_OK (by wrappingPublication.ApplyUpdateinHandleUpdates) until the subscriber has left. It fails 20/20 before the fix and passes 100/100 after. The existing test also passes 100/100. golangci-lint v2.14.0 reports 0 issues, andgo test -race ./pkg/relay/...passes. This isn't a hot path, so I didn't run benchmarks.Known gap (not addressed here). The relay's recorded Forward State can still drift when a publisher answers a REQUEST_UPDATE after
Update's 5s timeout.Updatewrites the REQUEST_UPDATE before it gives up, so the publisher may apply it anyway. After a pause, that leaves a watched track paused, which breaks §7.2's MUST. A full fix needs a design decision: either record the state once the write succeeds, or treat a timed-out update as unknown and re-send it.Reviewed by moqt-reviewer. Its finding about a redundant outer timeout is applied.
The test's
time.Sleep(50ms), which waits for the relay to see the subscriber leave, is timing-based. It can only cause a missed regression, never a false failure.🤖 Generated with Claude Code