🩹 fix: Timeout middleware not enforced

[edvardsanta]

Description

This PR refactors the timeout middleware to properly enforce request timeouts by:

1. Wrapping the handler execution in a goroutine. 2 Using a select statement to handle three possible outcomes:

   • Handler completes successfully.
   • Handler panics (panic is propagated).
   • Timeout occurs, triggering cfg.OnTimeout if set, or returning fiber.ErrRequestTimeout..

This change guarantees that:

• The timeout is respected in real time.
• Handlers have the opportunity to observe context cancellation if needed.
• Panics are safely propagated without losing stack traces.

Fixes #3671

Changes introduced

List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.

• [ ] Benchmarks: Describe any performance benchmarks and improvements related to the changes.
• [ ] Documentation Update: Detail the updates made to the documentation and links to the changed files.
• [ ] Changelog/What's New: Include a summary of the additions for the upcoming release notes.
• [ ] Migration Guide: If necessary, provide a guide or steps for users to migrate their existing code to accommodate these changes.
• [ ] API Alignment with Express: Explain how the changes align with the Express API.
• [ ] API Longevity: Discuss the steps taken to ensure that the new or updated APIs are consistent and not prone to breaking changes.
• [ ] Examples: Provide examples demonstrating the new features or changes in action.

Type of change

Please delete options that are not relevant.

• [ ] New feature (non-breaking change which adds functionality)
• [x] Enhancement (improvement to existing features and functionality)
• [ ] Documentation update (changes to documentation)
• [ ] Performance improvement (non-breaking change which improves efficiency)
• [ ] Code consistency (non-breaking change which improves code reliability and robustness)

Checklist

Before you submit your pull request, please make sure you meet these requirements:

• [ ] Followed the inspiration of the Express.js framework for new functionalities, making them similar in usage.
• [] Conducted a self-review of the code and provided comments for complex or critical parts.
• [ ] Updated the documentation in the /docs/ directory for Fiber's documentation.
• [ ] Added or updated unit tests to validate the effectiveness of the changes or new features.
• [ ] Ensured that new and existing unit tests pass locally with the changes.
• [ ] Verified that any new dependencies are essential and have been agreed upon by the maintainers/community.
• [ ] Aimed for optimal performance with minimal allocations in the new code.
• [ ] Provided benchmarks for the new code to analyze and improve upon.

Commit formatting

Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md

[coderabbitai[bot]]

[!NOTE]

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

The timeout middleware refactoring enforces handler timeouts through concurrent execution with goroutine-based synchronization, panic capture, and context cancellation. The change replaces linear post-execution timeout checks with a select-based control flow that monitors completion, panics, and deadline exceeded conditions, unified through a single error path.

Changes

Cohort / File(s)	Change Summary
Core Timeout Handler Refactoring middleware/timeout/timeout.go	Restructures timeout enforcement from post-execution checks to concurrent execution with goroutine-driven done and panic channels. Introduces safeCall wrapper for panic capture. Integrates context cancellation, custom timeout errors via OnTimeout, and converts panics/deadline-exceeded to unified fiber.ErrRequestTimeout error.
Comprehensive Test Coverage middleware/timeout/timeout_test.go	Adds TestTimeout_Issue_3671 covering multiple timeout scenarios (panic after timeout, blocking handlers, micro/large timeouts, custom OnTimeout behaviors) and TestSafeCall_Panic validating panic-to-timeout error conversion in the safe call wrapper.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Middleware
    participant Handler Goroutine
    participant Done Channel
    participant Panic Channel
    participant Context

    Client->>Middleware: HTTP Request
    Middleware->>Context: Create cancellable context with timeout
    Middleware->>Handler Goroutine: Launch handler in goroutine
    Middleware->>Middleware: Enter select statement
    
    alt Successful Completion
        Handler Goroutine->>Done Channel: Signal completion
        Done Channel->>Middleware: Receive result
        Middleware->>Middleware: Check for custom error
        Middleware->>Client: Return response
    else Timeout/Deadline Exceeded
        Context->>Context: Deadline exceeded
        Context->>Middleware: Context cancellation detected
        Middleware->>Middleware: Invoke OnTimeout callback
        Middleware->>Client: Return fiber.ErrRequestTimeout
    else Panic in Handler
        Handler Goroutine->>Panic Channel: Capture panic
        Panic Channel->>Middleware: Receive panic signal
        Middleware->>Client: Return fiber.ErrRequestTimeout
    end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Areas requiring extra attention:

• Goroutine synchronization logic in the select statement and channel handling to ensure no race conditions or resource leaks
• Panic capture mechanism in safeCall wrapper and conversion to appropriate timeout error
• Context cancellation propagation and its interaction with deadline-exceeded conditions
• Edge case coverage in tests, particularly the TestTimeout_Issue_3671 scenarios involving concurrent panics and blocking handlers
• Validation that OnTimeout callback is invoked at the correct time and with proper state

Possibly related PRs

• #3275: Refactors middleware/timeout/timeout.go to unify timeout handling with runHandler and isCustomError patterns
• #3720: Modifies timeout.go to improve context handling (ctx.Context(), SetContext, context.WithTimeout) within the timeout middleware
• #3604: Updates timeout middleware's error handling and OnTimeout/runHandler invocation logic

Suggested labels

☢️ Bug, v3, codex

Suggested reviewers

• gaby
• sixcolons
• ReneWerner87
• efectn

Poem

🐰 A rabbit's ode to concurrent grace, Where timeouts now have their rightful place, Goroutines dance with channels so bright, Panics are caught—no more endless flight! Context cancels when time runs thin, With safeCall guards, the true flow wins! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title 'fix: Timeout middleware not enforced' directly relates to the main change—refactoring timeout middleware to properly enforce timeouts—though it uses an emoji and could be more descriptive.
Description check	✅ Passed	Description clearly outlines the refactoring (goroutine, select statement, panic handling) and the issue fixed (#3671), but most checklist items are left unchecked, indicating incomplete documentation, benchmarks, and migration guidance.
Linked Issues check	✅ Passed	The PR addresses issue #3671 requirements: enforces timeouts via goroutine and select, passes cancellable context to handlers, safely propagates panics, and includes comprehensive edge-case tests for these behaviors.
Out of Scope Changes check	✅ Passed	All changes in timeout.go and timeout_test.go are directly scoped to fixing the timeout middleware enforcement and adding required validation tests for issue #3671.

✨ Finishing touches

• [ ] 📝 Generate docstrings

🧪 Generate unit tests (beta)

• [ ] Create PR with unit tests
• [ ] Post copyable unit tests in a comment

[!TIP]

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions: | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context. Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[edvardsanta]

Hello, @ReneWerner87, @gaby ,@own2pwn, @sixcolors.

I’m exploring some edge cases around the Timeout middleware and would like your thoughts on expected behavior. Specifically, I’m wondering how Fiber should behave when a panic occurs in the handler. I have written two tests cases: Handler panics after timeout:

handler: func(_ fiber.Ctx) error {
      time.Sleep(50 * time.Millisecond)
      panic("panic after timeout")
}

Custom OnTimeout handler panics:

handler: func(_ fiber.Ctx) error {
		time.Sleep(50 * time.Millisecond)
		return nil
	},
	config: Config{
		Timeout: 10 * time.Millisecond,
		OnTimeout: func(_ fiber.Ctx) error {
			panic("custom panic on timeout")
		},
},

My questions:

1. In the case of a panic after the timeout has already triggered, should Fiber still return a 408 Request Timeout, or should the panic propagate?

2. For a panic inside a custom OnTimeout handler, do we have an expected status code, or should the panic be allowed to crash the app?

3. Any recommendations on handling these panics safely in production while maintaining the correct timeout semantics?

I’m not sure what the “correct” behavior should be here, so I’d like to align it first before to open the pr

[gaby]

@edvardsanta

1. Return 408
2. Probably 408 too
3. Add tests for them, testify has a way of testing these

[gaby]

@edvardsanta Any updates on this?

[edvardsanta]

@edvardsanta Any updates on this?

I’ll get back to this later today