Fix panic in runs.Replay when the consumer breaks out early - #4
Merged
Conversation
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.
Problem
Run.Replayforwarded every non-nil error fromreplayRemoteEventsintoyield,including the internal
errStopReplaysentinel. That sentinel is returned atreplay.go:69precisely becauseyieldalready returnedfalse:Calling
yieldagain after it has returnedfalseviolates Go's range-over-funccontract, so any consumer breaking out of the loop panicked:
Run.Replayis exported frompkg/productize/runs, so this is on the public APIsurface. The sibling path already gets this right —
replayHistoricalWindowexplicitlyswallows the same sentinel at
replay.go:112(case errors.Is(replayErr, errStopReplay): return false). One path handled it; the other crashed.Fix
The guard becomes
err != nil && !errors.Is(err, errStopReplay), matching the sibling.errorswas already imported — no new dependencies, nogo.modchanges. A short commentrecords the invariant.
Test
TestReplayStopsCleanlyWhenConsumerBreaksEarlyintail_test.go, table-driven witht.Parallel()per repo convention. One subtest breaks on the first event of a two-eventpage; the other breaks on the first event of a
HasMore: truepaged reader. Both assertexactly one event was yielded and that
eventCallsstayed at 1 — proving the early breakboth avoids the panic and stops paging rather than fetching the next page.
Before the fix the test panics at
replay.go:33, the exactyield(events.Event{}, err)line.
Verification
make verifypasses end to end — fmt, lint with0 issues., 3668 tests, and the build.Also verified merged together with the
runs purgecontext fix on an integration branch:3669 tests, gate green.
🤖 Generated with Claude Code