[fix]: core - recover from closeBodyStream panic in idle-timeout timer goroutine - #4616
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughA ChangesIdle timeout timer panic recovery
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…r goroutine An orphaned idle-timeout timer can fire after the stream's connection has already been released to / reused from the fasthttp pool. closeBodyStream then calls CloseWithError, which nil-derefs in (*HostClient).CloseConn and panics. Because this runs in the time.AfterFunc timer goroutine, the panic is unrecoverable by callers and crashes the whole process -- observed taking down a gateway under sustained streaming load (idle timer firing ~minutes after a stream completed). maximhq#3677 added a recover() to the idleTimeoutReader.Read() path but not to the AfterFunc's own closeBodyStream call. This adds the companion recover() in the timer callback so a stale idle timer can never crash the process. Adds TestIdleTimeoutReader_RecoversCloseStreamPanicOnTimerFire, which crashes the test process without the fix and passes with it. Affected packages: - core/providers/utils/ - the fix + regression test - core/changelog.md - changelog entry Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
d4cdd1d to
3e3aa06
Compare
Address review feedback: the bare `_ = recover()` swallowed the panic value with no diagnostics. Unlike the Read() path, the timer goroutine cannot re-panic an unexpected value (that would crash the process — the bug being fixed), so log the recovered value at debug level via getLogger(), matching the existing teardown-recover pattern in this file (EnsureStreamFinalizerCalled, ReleaseStreamingResponse). This leaves a forensic trace if a future, unrelated panic is ever introduced into closeBodyStream or its callees. Adds TestIdleTimeoutReader_LogsRecoveredTimerPanic, which installs a capturing logger and asserts the recovered value is logged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review feedback: both timer-panic tests slept a fixed 50ms then called cleanup(). On a slow runner the 10ms timer might not have fired yet, so cleanup()'s timer.Stop() returns true, the test returns without any panic occurring, and it passes while silently skipping the recover path it guards. timerPanicCloser now closes a `called` channel the instant CloseWithError is entered (just before the panic). The tests wait on that channel (with a 2s deadline) before cleanup(), so the timer is only stopped after it has demonstrably fired — a definitive signal the guarded path ran, and it also removes the arbitrary sleep. Verified stable across -race -count=20. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/providers/utils/idle_timeout_reader_test.go`:
- Around line 469-470: Replace the fixed time.Sleep(50 * time.Millisecond) call
at line 469 with a condition-based synchronization mechanism that waits
deterministically for the timer callback to complete. Instead of relying on a
hardcoded sleep duration that may be insufficient under slow CI scheduling, use
a synchronization primitive such as a sync.WaitGroup, atomic flag, or channel to
signal when the callback execution has finished. This ensures the test waits
until the actual callback (recover + log) has returned before cleanup() is
called, making the assertion deterministic and eliminating the race condition
where cleanup() might stop the timer before the callback executes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 812e0b84-9746-4c59-ab98-ca1b8b962b94
📒 Files selected for processing (2)
core/providers/utils/idle_timeout_reader_test.gocore/providers/utils/utils.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/providers/utils/utils.go
…r goroutine (#4616) * [fix]: core - recover from closeBodyStream panic in idle-timeout timer goroutine An orphaned idle-timeout timer can fire after the stream's connection has already been released to / reused from the fasthttp pool. closeBodyStream then calls CloseWithError, which nil-derefs in (*HostClient).CloseConn and panics. Because this runs in the time.AfterFunc timer goroutine, the panic is unrecoverable by callers and crashes the whole process -- observed taking down a gateway under sustained streaming load (idle timer firing ~minutes after a stream completed). #3677 added a recover() to the idleTimeoutReader.Read() path but not to the AfterFunc's own closeBodyStream call. This adds the companion recover() in the timer callback so a stale idle timer can never crash the process. Adds TestIdleTimeoutReader_RecoversCloseStreamPanicOnTimerFire, which crashes the test process without the fix and passes with it. Affected packages: - core/providers/utils/ - the fix + regression test - core/changelog.md - changelog entry Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [fix]: core - log recovered panic value in idle-timeout timer goroutine Address review feedback: the bare `_ = recover()` swallowed the panic value with no diagnostics. Unlike the Read() path, the timer goroutine cannot re-panic an unexpected value (that would crash the process — the bug being fixed), so log the recovered value at debug level via getLogger(), matching the existing teardown-recover pattern in this file (EnsureStreamFinalizerCalled, ReleaseStreamingResponse). This leaves a forensic trace if a future, unrelated panic is ever introduced into closeBodyStream or its callees. Adds TestIdleTimeoutReader_LogsRecoveredTimerPanic, which installs a capturing logger and asserts the recovered value is logged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [test]: core - make idle-timeout timer-panic tests deterministic Address review feedback: both timer-panic tests slept a fixed 50ms then called cleanup(). On a slow runner the 10ms timer might not have fired yet, so cleanup()'s timer.Stop() returns true, the test returns without any panic occurring, and it passes while silently skipping the recover path it guards. timerPanicCloser now closes a `called` channel the instant CloseWithError is entered (just before the panic). The tests wait on that channel (with a 2s deadline) before cleanup(), so the timer is only stopped after it has demonstrably fired — a definitive signal the guarded path ran, and it also removes the arbitrary sleep. Verified stable across -race -count=20. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…r goroutine (maximhq#4616) * [fix]: core - recover from closeBodyStream panic in idle-timeout timer goroutine An orphaned idle-timeout timer can fire after the stream's connection has already been released to / reused from the fasthttp pool. closeBodyStream then calls CloseWithError, which nil-derefs in (*HostClient).CloseConn and panics. Because this runs in the time.AfterFunc timer goroutine, the panic is unrecoverable by callers and crashes the whole process -- observed taking down a gateway under sustained streaming load (idle timer firing ~minutes after a stream completed). maximhq#3677 added a recover() to the idleTimeoutReader.Read() path but not to the AfterFunc's own closeBodyStream call. This adds the companion recover() in the timer callback so a stale idle timer can never crash the process. Adds TestIdleTimeoutReader_RecoversCloseStreamPanicOnTimerFire, which crashes the test process without the fix and passes with it. Affected packages: - core/providers/utils/ - the fix + regression test - core/changelog.md - changelog entry Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [fix]: core - log recovered panic value in idle-timeout timer goroutine Address review feedback: the bare `_ = recover()` swallowed the panic value with no diagnostics. Unlike the Read() path, the timer goroutine cannot re-panic an unexpected value (that would crash the process — the bug being fixed), so log the recovered value at debug level via getLogger(), matching the existing teardown-recover pattern in this file (EnsureStreamFinalizerCalled, ReleaseStreamingResponse). This leaves a forensic trace if a future, unrelated panic is ever introduced into closeBodyStream or its callees. Adds TestIdleTimeoutReader_LogsRecoveredTimerPanic, which installs a capturing logger and asserts the recovered value is logged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [test]: core - make idle-timeout timer-panic tests deterministic Address review feedback: both timer-panic tests slept a fixed 50ms then called cleanup(). On a slow runner the 10ms timer might not have fired yet, so cleanup()'s timer.Stop() returns true, the test returns without any panic occurring, and it passes while silently skipping the recover path it guards. timerPanicCloser now closes a `called` channel the instant CloseWithError is entered (just before the panic). The tests wait on that channel (with a 2s deadline) before cleanup(), so the timer is only stopped after it has demonstrably fired — a definitive signal the guarded path ran, and it also removes the arbitrary sleep. Verified stable across -race -count=20. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…r goroutine (maximhq#4616) * [fix]: core - recover from closeBodyStream panic in idle-timeout timer goroutine An orphaned idle-timeout timer can fire after the stream's connection has already been released to / reused from the fasthttp pool. closeBodyStream then calls CloseWithError, which nil-derefs in (*HostClient).CloseConn and panics. Because this runs in the time.AfterFunc timer goroutine, the panic is unrecoverable by callers and crashes the whole process -- observed taking down a gateway under sustained streaming load (idle timer firing ~minutes after a stream completed). maximhq#3677 added a recover() to the idleTimeoutReader.Read() path but not to the AfterFunc's own closeBodyStream call. This adds the companion recover() in the timer callback so a stale idle timer can never crash the process. Adds TestIdleTimeoutReader_RecoversCloseStreamPanicOnTimerFire, which crashes the test process without the fix and passes with it. Affected packages: - core/providers/utils/ - the fix + regression test - core/changelog.md - changelog entry Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [fix]: core - log recovered panic value in idle-timeout timer goroutine Address review feedback: the bare `_ = recover()` swallowed the panic value with no diagnostics. Unlike the Read() path, the timer goroutine cannot re-panic an unexpected value (that would crash the process — the bug being fixed), so log the recovered value at debug level via getLogger(), matching the existing teardown-recover pattern in this file (EnsureStreamFinalizerCalled, ReleaseStreamingResponse). This leaves a forensic trace if a future, unrelated panic is ever introduced into closeBodyStream or its callees. Adds TestIdleTimeoutReader_LogsRecoveredTimerPanic, which installs a capturing logger and asserts the recovered value is logged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [test]: core - make idle-timeout timer-panic tests deterministic Address review feedback: both timer-panic tests slept a fixed 50ms then called cleanup(). On a slow runner the 10ms timer might not have fired yet, so cleanup()'s timer.Stop() returns true, the test returns without any panic occurring, and it passes while silently skipping the recover path it guards. timerPanicCloser now closes a `called` channel the instant CloseWithError is entered (just before the panic). The tests wait on that channel (with a 2s deadline) before cleanup(), so the timer is only stopped after it has demonstrably fired — a definitive signal the guarded path ran, and it also removes the arbitrary sleep. Verified stable across -race -count=20. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Summary
An orphaned idle-timeout timer can crash the whole process. When the
time.AfterFuncregistered byNewIdleTimeoutReaderfires after thestream's connection has already been released to / reused from the fasthttp
pool, the
closeBodyStream(r.bodyStream, ...)call invokes the body'sCloseWithError, which nil-dereferences in fasthttp's(*HostClient).CloseConn. Because that runs in the timer goroutine, the panicis unrecoverable by any caller and takes the entire process down.
We observed this crashing a gateway built on bifrost under sustained streaming
load — the crash fires minutes after the originating stream has completed, when
the stale idle timer finally elapses, so it is hard to correlate with any
single request.
PR #3677 ("fix idle timeout panic") added a
recover()to theidleTimeoutReader.Read()path, but theAfterFunc's owncloseBodyStreamcall has no such guard. This PR adds the companion
recover()inside the timercallback so a stale idle timer can never crash the process.
Changes
core/providers/utils/utils.go— wrap thecloseBodyStreamcall insideNewIdleTimeoutReader'stime.AfterFuncwithdefer func() { _ = recover() }().This is the timer-goroutine counterpart to the
Read()-path recover fromfix idle timeout panic #3677. The
BifrostContextKeyConnectionClosedsignal andtimerDoneclosesemantics are unchanged — the recover only prevents an unrecoverable
cross-goroutine panic.
core/providers/utils/idle_timeout_reader_test.go— addTestIdleTimeoutReader_RecoversCloseStreamPanicOnTimerFireplus atimerPanicCloserstub whoseCloseWithErrorpanics (mimicking the fasthttpCloseConnnil-deref) and which implementsstreamCloserWithErrorsocloseBodyStreamtakes theCloseWithErrorbranch the idle timer actuallyhits.
core/changelog.md— changelog entry for the fix (perdocs/contributing/raising-a-pr.mdx).Type of change
Affected areas
How to test
The new test fails (the test process crashes with
panic: simulated fasthttp CloseConn nil-deref) without the fix, and passeswith it.
To see it fail without the fix, temporarily remove the
defer func() { _ = recover() }()line and re-run — the timer goroutine panictakes the test binary down:
Breaking changes
Related issues
Closes #4617.
Follow-up to #3677 (which guarded only the
Read()path). This closes theremaining timer-goroutine crash path.
Security considerations
None. The change only swallows a panic in a background timer goroutine; it does
not alter auth, secrets handling, or what data is read/written. The connection
is already being torn down on the timeout path, and
BifrostContextKeyConnectionClosedis still set before the recover.
Checklist
go vet+go test ./providers/utils/ -race)docs/contributing/raising-a-pr.mdx,code-conventions.mdx) and added acore/changelog.mdentrymake test-all/make lint) passes locally — only the affected package (core/providers/utils) was run + vetted + gofmt'd