fix: handle non-positive timeout and nil cancel in RequestTimeoutMiddleware#197
fix: handle non-positive timeout and nil cancel in RequestTimeoutMiddleware#197Siddh2024 wants to merge 1 commit into
Conversation
…leware - When timeout <= 0, use the existing context directly instead of calling context.WithTimeout with a non-positive duration (which cancels immediately). - When an earlier deadline is kept, assign a no-op cancel func so defer cancel() does not panic on a nil function call. Fixes AnkanMisra#196
|
@Siddh2024 is attempting to deploy a commit to the ankanmisra's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR fixes ChangesTimeout Middleware Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
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 |
![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 `@gateway/middleware.go`:
- Around line 163-167: Add unit tests for RequestTimeoutMiddleware to cover the
two missing branches: (1) timeout <= 0 behavior — create a request with timeout
set to 0 and a handler that intentionally sleeps longer than a positive timeout
would; assert the middleware does NOT cancel the request (handler completes and
response is returned), verifying the middleware uses c.Request.Context() and the
no-op cancel (referencing RequestTimeoutMiddleware and the branch where ctx =
c.Request.Context(); cancel = func() {}); (2) earlier parent-deadline behavior —
create a parent request Context with an earlier deadline than the middleware
timeout and wrap it through RequestTimeoutMiddleware, then assert the request is
canceled based on the parent deadline (handler sees context.Deadline exceeded)
rather than the middleware timeout, verifying middleware respects existing
parent deadlines. Ensure tests live alongside gateway/timeout_test.go and use
the same test utilities for injecting timeouts and inspecting cancellation
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if timeout <= 0 { | ||
| // Preserve the existing behavior for zero/negative values. | ||
| ctx, cancel = context.WithTimeout(c.Request.Context(), timeout) | ||
| // Zero/negative timeout means no timeout — use the existing context | ||
| // without wrapping it, so requests don't cancel immediately. | ||
| ctx = c.Request.Context() | ||
| cancel = func() {} // no-op to avoid nil cancel panic in defer |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify test coverage for zero/negative timeout and earlier deadline cases.
# Search for test functions covering zero, negative, or deadline scenarios
rg -n "func Test.*Timeout" gateway/timeout_test.go
# Look for zero/negative timeout test cases
rg -n -C3 "timeout.*<=.*0|timeout.*0|timeout.*-|negative|zero.*timeout|earlier.*deadline|parent.*deadline" gateway/timeout_test.goRepository: AnkanMisra/MicroAI-Paygate
Length of output: 276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the timeout tests file
echo "---- gateway/timeout_test.go ----"
if [ -f gateway/timeout_test.go ]; then
nl -ba gateway/timeout_test.go | sed -n '1,220p'
else
echo "Missing gateway/timeout_test.go"
fi
# Search across gateway tests for zero/negative timeout and parent-deadline behavior
echo "---- rg across gateway for zero/negative timeout / parent deadline ----"
rg -n --hidden --glob 'gateway/**/*_test.go' \
"timeout.*(<|<=).?0|<=\s*0|<\s*0|timeout.*0|zero.*timeout|negative|earlier.*deadline|parent.*deadline|context.*deadline.*(exceed|before|earlier)" \
gateway || trueRepository: AnkanMisra/MicroAI-Paygate
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- gateway/timeout_test.go (first 220 lines) ----"
if [ -f gateway/timeout_test.go ]; then
cat -n gateway/timeout_test.go | sed -n '1,220p'
else
echo "Missing gateway/timeout_test.go"
fi
echo "---- rg across gateway tests for zero/negative timeout / parent deadline ----"
rg -n --glob 'gateway/**/*_test.go' \
"(timeout.*(<|<=).?0|<=\s*0|<\s*0|negative|zero.*timeout|earlier.*deadline|parent.*deadline|context.*deadline.*(exceed|before|earlier))" \
gateway || trueRepository: AnkanMisra/MicroAI-Paygate
Length of output: 5729
Add missing tests for RequestTimeoutMiddleware timeout<=0 and earlier parent-deadline behavior
gateway/timeout_test.goonly tests the positive-timeout path (504) and AI timeout (~1s) plus provider context deadline; it has no coverage for thetimeout <= 0branch (zero/negative) or for “earlier parent deadline” vs middleware timeout behavior.- Existing “negative” tests in
gateway/redis_receipt_store_test.go/gateway/config_test.gocover receipt TTL/config fallback, not the middleware timeout logic.
🤖 Prompt for 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.
In `@gateway/middleware.go` around lines 163 - 167, Add unit tests for
RequestTimeoutMiddleware to cover the two missing branches: (1) timeout <= 0
behavior — create a request with timeout set to 0 and a handler that
intentionally sleeps longer than a positive timeout would; assert the middleware
does NOT cancel the request (handler completes and response is returned),
verifying the middleware uses c.Request.Context() and the no-op cancel
(referencing RequestTimeoutMiddleware and the branch where ctx =
c.Request.Context(); cancel = func() {}); (2) earlier parent-deadline behavior —
create a parent request Context with an earlier deadline than the middleware
timeout and wrap it through RequestTimeoutMiddleware, then assert the request is
canceled based on the parent deadline (handler sees context.Deadline exceeded)
rather than the middleware timeout, verifying middleware respects existing
parent deadlines. Ensure tests live alongside gateway/timeout_test.go and use
the same test utilities for injecting timeouts and inspecting cancellation
behavior.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| if timeout <= 0 { | ||
| // Preserve the existing behavior for zero/negative values. | ||
| ctx, cancel = context.WithTimeout(c.Request.Context(), timeout) | ||
| // Zero/negative timeout means no timeout — use the existing context | ||
| // without wrapping it, so requests don't cancel immediately. | ||
| ctx = c.Request.Context() | ||
| cancel = func() {} // no-op to avoid nil cancel panic in defer |
There was a problem hiding this comment.
Add regression tests for the new timeout branches
Issue #196 explicitly requires coverage for timeout == 0, negative durations, and the earlier-parent-deadline path, but this change only updates gateway/middleware.go
the current timeout tests still cover only the positive timeout, fasthandler, and panic cases
That leaves the exact branches introduced here unprotected, so the immediate-504 regression or deadline-preservation behavior can change again without any failing test
AnkanMisra
added
bug
2 participants
Fixes #196
Bug 1 — Immediate cancellation with timeout <= 0:
context.WithTimeout with a non-positive duration creates an already-expired context, cancelling every request immediately.
Bug 2 — nil cancel causes panic in defer cancel():
When an earlier deadline is shorter than desired, cancel was left nil. The nil guard was fragile.
Fix: Both paths assign cancel = func(){} and the nil guard is removed.
Summary by CodeRabbit
Bug Fixes