🐛 fix(timeout): reclaim abandoned fiber.Ctx via ScheduleReclaim latch (#4359)#4400
Conversation
The timeout middleware abandoned the fiber.Ctx on every timeout and never called ForceRelease, so timed-out requests leaked their context permanently even after the handler eventually completed. Under sustained timeouts this is an unbounded memory leak (the code's own TODO acknowledged it). Add an opt-in reclamation latch on *DefaultCtx. ScheduleReclaim arms a single goroutine that returns the context to the pool via ForceRelease only after BOTH the handler goroutine finishes AND the request handler releases the context (signalled from ReleaseCtx/releaseDefaultCtx). This keeps reclamation race-free and adds zero overhead to the non-abandoned hot path. SSE's use of Abandon() without arming the latch is unchanged: such contexts stay unpooled as before. The timeout middleware now closes a handlerDone channel when its handler goroutine exits (covering normal and panic paths) and arms reclamation via a narrow type assertion, falling back to the previous behavior for custom Ctx implementations. Tests (run with -race) verify the context is not reclaimed while the handler is still running, is reclaimed once it finishes (normal and panic paths), and that an abandoned-but-unarmed context is left unpooled.
|
@claude please pick up this PR and drive it to merge. Context: this branch was created earlier as the deterministic alternative to PR #4390's Next steps for you:
Verify before requesting review:
If anything in the design needs revisiting, raise it here rather than silently changing the contract. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4400 +/- ##
==========================================
- Coverage 91.33% 91.33% -0.01%
==========================================
Files 132 132
Lines 13110 13161 +51
==========================================
+ Hits 11974 12020 +46
- Misses 717 724 +7
+ Partials 419 417 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a race-free reclamation mechanism for abandoned contexts to resolve memory leaks in timed-out requests. It implements a reclaimLatch and ScheduleReclaim method on DefaultCtx to coordinate safe pool reclamation once both the handler goroutine and the request handler have finished. The timeout middleware is updated to use this mechanism, and comprehensive tests are added. The review feedback highlights a potential leak when custom contexts wrap *DefaultCtx, recommending an interface check instead of a concrete type assertion to safely invoke signalReleased().
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a reclaim latch and ChangesAbandoned Context Reclamation for Timeout Middleware
Sequence Diagram(s)sequenceDiagram
participant Handler
participant TimeoutMiddleware
participant DefaultCtx
participant Reclaimer
TimeoutMiddleware->>DefaultCtx: Abandon() + ScheduleReclaim(handlerDone, cancel)
TimeoutMiddleware->>Handler: return timeout error
Reclaimer->>Handler: wait for handlerDone close
Reclaimer->>TimeoutMiddleware: optional cancel() (if provided)
Handler->>DefaultCtx: ReleaseCtx -> signalReleased()
Reclaimer->>DefaultCtx: wait releasedCh then ForceRelease()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@ctx.go`:
- Line 776: Spelling fix: update the comment that references
ReleaseCtx/releaseDefaultCtx by changing the US English spelling "signalled" to
"signaled" so the linter stops flagging it; locate the comment near the handler
release notice (mentions ReleaseCtx/releaseDefaultCtx) and replace the word
only, leaving the rest of the comment intact.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c50a07ac-b3a3-4430-9b1a-f1b3a4f47159
📒 Files selected for processing (4)
ctx.goctx_interface.gomiddleware/timeout/timeout.gomiddleware/timeout/timeout_test.go
There was a problem hiding this comment.
Pull request overview
This PR addresses the timeout middleware’s abandoned fiber.Ctx leak by adding a safe, opt-in reclamation mechanism to *fiber.DefaultCtx and wiring the timeout path to schedule pool reclamation once it is race-free to do so.
Changes:
- Add a
DefaultCtx“reclaim latch” (ScheduleReclaim+signalReleased) to coordinate safeForceRelease()after both the handler goroutine exits and Fiber releases the ctx. - Update timeout middleware to close a
handlerDonesignal and schedule reclamation on timeout instead of leaking abandoned contexts. - Add unit tests validating reclamation behavior (normal + panic paths) and preserving the “abandon without reclaim stays unpooled” contract.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| middleware/timeout/timeout.go | Adds handlerDone and schedules ctx reclamation on timeout to avoid leaking abandoned contexts. |
| middleware/timeout/timeout_test.go | Adds tests ensuring abandoned timed-out contexts are reclaimed only after handler exit; keeps SSE-style abandon semantics. |
| ctx.go | Introduces reclaimLatch, ScheduleReclaim, and signalReleased on DefaultCtx, plus clears latch on release. |
| ctx_interface.go | Ensures ReleaseCtx / releaseDefaultCtx signal the reclaimer when releasing an abandoned *DefaultCtx. |
|
@claude pls solve the review hints and linting issues |
|
@claude[agent] pls solve the review hints and linting issues |
- Change type assertion in timeout.go from interface to concrete *fiber.DefaultCtx to prevent accidental matches with custom Ctx implementations that lack the signalReleased wiring, which would cause goroutine leaks - Make ScheduleReclaim call Abandon() internally to prevent goroutine leaks if callers forget to call Abandon first - Update timeout middleware to rely on ScheduleReclaim's internal Abandon call for DefaultCtx path, but still call Abandon explicitly for custom context fallback - Fix misspelling: "signalled" → "signaled" Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/478a8456-02af-4103-97a0-e848a364daee Co-authored-by: ReneWerner87 <7063188+ReneWerner87@users.noreply.github.com>
Fixed both review issues in commit f20d73b:
All tests pass with
Completed. See commit f20d73b which addresses both review issues and fixes the linting error. |
Status check after
|
| Issue criterion | Status |
|---|---|
(1) Implement a finalizer goroutine pool or sync.WaitGroup-based cleanup that eventually calls ForceRelease after the handler goroutine completes |
Implemented via ScheduleReclaim + reclaimLatch (handler-done channel plus released-channel from ReleaseCtx / releaseDefaultCtx, fused with sync.Once). |
| (2) Add a secondary timeout for the cleanup goroutine itself (e.g. 2x the original timeout) | Not implemented. Documented design choice in ctx.go: "If handlerDone never closes - a handler that never returns - the context is intentionally never reclaimed, because the handler still owns it." This matches the issue's own note ("Note this part is partly inherent to the goroutine-per-request timeout pattern") and #4347's folded-in concern. |
| (3) Consider a bounded pool of reclaimer goroutines rather than spawning one per timeout | Not implemented. Still one goroutine per timed-out request. The goroutine's lifetime is bounded by handler lifetime, so steady-state goroutine count is bounded by handler latency, not by timeout count. |
Items (2) and (3) are deliberate non-goals of this PR. They address the never-returning-handler edge case, which the issue itself acknowledges is "partly inherent to the goroutine-per-request timeout pattern". Suggest tracking them in a follow-up issue if we want to chase a bounded-pool / hard-cap design (e.g. preemption via runtime.Goexit is not possible in Go, but capping the in-flight reclaimer count and shedding load when saturated is).
Net
If we accept that (2)/(3) are a follow-up, the only remaining gate is the codecov/patch signal, which is purely a coverage-collection artifact. Once a direct unit test in package fiber is added (or -coverpkg is set in CI), this PR is ready for human review and merge.
The reclaim path was already exercised end-to-end by the timeout middleware tests, but the root-package coverage profile (test.yml runs `go test ./...` without -coverpkg) only credits coverage of statements in the package being tested, so codecov/patch saw 37% on the changes in ctx.go and ctx_interface.go. Add direct unit tests in package fiber that drive ScheduleReclaim, the release/handler-done ordering both ways, the cancel hook, the nil-cancel branch, signalReleased's no-op and sync.Once semantics, and both ReleaseCtx and releaseDefaultCtx release entry points. All run under -race. Coverage after this change (root package run): ScheduleReclaim, signalReleased, ForceRelease, Abandon, IsAbandoned, ReleaseCtx, releaseDefaultCtx all 100%.
Summary
Deterministic fix for the timeout context leak reported in #4359 (and the related goroutine-survival aspect of #4347). Replaces the existing TODO in
middleware/timeout/timeout.gothat documented exactly this design.Closes #4359.
Approach
Adds an opt-in reclamation latch on
*DefaultCtx:ScheduleReclaim(handlerDone <-chan struct{}, cancel context.CancelFunc)arms a single goroutine that waits for BOTH:handlerDoneclose (the handler goroutine has fully exited, including panic-path), ANDReleaseCtx/releaseDefaultCtx).ForceRelease, returning the ctx to the pool. Race-free by construction: every concurrent holder (handler goroutine,requestHandlerrunningErrorHandler) is guaranteed done.The timeout middleware closes
handlerDonein adefercovering both the normal and panic exit paths, then callsScheduleReclaimvia a narrow type assertion. CustomCtximplementations fall back to the prior behavior (no auto-reclaim).Why not
runtime.SetFinalizer(PR #4390)Documented in detail at #4390 (comment). Short version:
DefaultCtxshell, not the ~10 - 20 KiB fasthttpRequestCtxgraph that fasthttp itself permanently abandons afterTimeoutErrorWithResponse. About 90 % of the leak would survive.GOGCvalues.sync.Pool) has zero stdlib or production-library precedent; the Go team is migrating away fromSetFinalizereven for safety-net uses.Changes introduced
reclaim *reclaimLatchis nil unless armed; reads are nil-checks). Timeout path replaces the existing cleanup goroutine with the reclamation goroutine, same goroutine count.fiber.Ctx. TheAbandon()contract is unchanged for existing callers (SSE); a new opt-inScheduleReclaimarms automatic safe reclamation.ScheduleReclaim) on*DefaultCtxand one private signal hook (signalReleased) called fromReleaseCtx/releaseDefaultCtx. No breaking change.Type of change
Checklist
-race.Test plan
TestTimeout_AbandonedCtxReclaimed: confirms the ctx is NOT reclaimed while the handler is still running, and IS reclaimed (viaIsAbandoned()flipping to false on the same pointer) once it exits.TestTimeout_PanicAfterTimeoutReclaimed: same assertion for the panic exit path.TestTimeout_AbandonWithoutReclaimNotPooled: guards the SSE-styleAbandon()contract: an abandoned-but-unarmed ctx stays unpooled.Refs: #4359, #4347, #4390.
🤖 Generated with Claude Code