Skip to content

fix: handle non-positive timeout and nil cancel in RequestTimeoutMiddleware #197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Siddh2024 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: handle non-positive timeout and nil cancel in RequestTimeoutMiddleware #197

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions gateway/middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,25 +161,26 @@ func RequestTimeoutMiddleware(timeout time.Duration) gin.HandlerFunc {
var cancel context.CancelFunc
var ctx context.Context
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
Comment on lines 163 to +167

@coderabbitai coderabbitai Bot Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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.go

Repository: 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 || true

Repository: 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 || true

Repository: AnkanMisra/MicroAI-Paygate

Length of output: 5729

Add missing tests for RequestTimeoutMiddleware timeout<=0 and earlier parent-deadline behavior

  • gateway/timeout_test.go only tests the positive-timeout path (504) and AI timeout (~1s) plus provider context deadline; it has no coverage for the timeout <= 0 branch (zero/negative) or for “earlier parent deadline” vs middleware timeout behavior.
  • Existing “negative” tests in gateway/redis_receipt_store_test.go / gateway/config_test.go cover 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.

Comment on lines 163 to +167

@AnkanMisra AnkanMisra Jun 5, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
if d, ok := c.Request.Context().Deadline(); ok {
desired := time.Now().Add(timeout)
// If an earlier deadline already exists, keep it. Otherwise set
// a new deadline at the desired point.
if d.Before(desired) {
ctx = c.Request.Context()
cancel = func() {} // no-op to avoid nil cancel panic in defer
} else {
ctx, cancel = context.WithDeadline(c.Request.Context(), desired)
}
} else {
ctx, cancel = context.WithTimeout(c.Request.Context(), timeout)
}
}
if cancel != nil {
defer cancel()
}
defer cancel()
c.Request = c.Request.WithContext(ctx)

origWriter := c.Writer
Expand Down
Loading