fix(docker-mariadb-clean-arch): bound data layer calls with a request context - #5305
Conversation
… context Closes #2718. Every handler and middleware passed context.Background() straight through the service into the repository and on to database/sql. Nothing in the recipe ever set a deadline, so a slow or hung query blocked the handler for as long as the driver kept waiting, and a client that disconnected left the query running. The issue reported this against context.WithCancel, which the code used at the time. The cancel func was deferred but nothing else could trigger it, so it never cancelled anything. The call was later simplified to a plain context.Background(), which removed even that. All twelve call sites now derive from the request context and add a deadline: ctx, cancel := context.WithTimeout(c.Context(), requestTimeout) defer cancel() That covers both halves of the problem. c.Context() is cancelled when the client goes away, and the timeout bounds a query that hangs while the client is still connected. requestTimeout is a package level constant in city and user, set to 10 seconds, kept per package so the two stay independent as the clean architecture layout intends. Verified with go build ./... and go vet ./...; no context.Background() calls remain in the recipe.
There was a problem hiding this comment.
🟢 Ready to approve
The changes are localized and correctly propagate request-scoped timeouts into service calls, with only minor middleware context-cancel timing improvements suggested.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR updates the docker-mariadb-clean-arch recipe to ensure data-layer calls are bounded by request-scoped contexts with deadlines, preventing handlers from hanging indefinitely on slow/hung DB operations and allowing cancellation when the client disconnects.
Changes:
- Introduced a per-package
requestTimeoutconstant (10s) forcityanduser. - Replaced
context.Background()usage in handlers/middleware withcontext.WithTimeout(c.Context(), requestTimeout)and propagated the derived context into service calls. - Updated both
cityandusermiddleware to use request-derived contexts when performing existence checks.
File summaries
| File | Description |
|---|---|
| docker-mariadb-clean-arch/internal/user/handler.go | Adds a 10s request timeout and uses request-derived contexts for all user handler service calls. |
| docker-mariadb-clean-arch/internal/user/middleware.go | Uses a request-derived timeout context for the “user exists” lookup before continuing the chain. |
| docker-mariadb-clean-arch/internal/city/handler.go | Adds a 10s request timeout and uses request-derived contexts for all city handler service calls. |
| docker-mariadb-clean-arch/internal/city/middleware.go | Uses a request-derived timeout context for the “city exists” lookup before continuing the chain. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
📝 WalkthroughWalkthroughCity and user handlers and middleware now derive 10-second cancellable contexts from Fiber requests. City and user repositories now return database row iteration errors instead of partial results with nil errors. ChangesRequest Timeout Propagation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
🧹 Nitpick comments (1)
docker-mariadb-clean-arch/internal/city/handler.go (1)
12-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the timeout as per operation, not per request.
The existence middleware and the subsequent update or delete handler create independent 10-second contexts. A
PUTorDELETErequest can therefore spend two sequential 10-second data-layer waits. The comment says the constant bounds one request, which is not accurate. If the requirement is per operation, update the comment. If it is a 10-second total request budget, reuse one context across both calls.
- docker-mariadb-clean-arch/internal/city/handler.go#L12-L16: describe
requestTimeoutas an operation timeout, or propagate one request-wide context.- docker-mariadb-clean-arch/internal/user/handler.go#L10-L16: apply the same clarification or request-wide context design.
🤖 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 `@docker-mariadb-clean-arch/internal/city/handler.go` around lines 12 - 16, Clarify both requestTimeout comments as per-operation data-layer timeouts rather than single-request budgets; update docker-mariadb-clean-arch/internal/city/handler.go lines 12-16 and docker-mariadb-clean-arch/internal/user/handler.go lines 10-16, with no context propagation changes required.
🤖 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 `@docker-mariadb-clean-arch/internal/city/handler.go`:
- Around line 47-50: Add result-error propagation after the row-iteration loops
in GetCities and GetUsers within
docker-mariadb-clean-arch/internal/city/repository.go and
docker-mariadb-clean-arch/internal/user/repository.go, returning res.Err()
instead of the collected slice when iteration is interrupted. The handler sites
docker-mariadb-clean-arch/internal/city/handler.go lines 47-50 and
docker-mariadb-clean-arch/internal/user/handler.go lines 41-44 require no direct
changes; they will propagate the repository errors.
---
Nitpick comments:
In `@docker-mariadb-clean-arch/internal/city/handler.go`:
- Around line 12-16: Clarify both requestTimeout comments as per-operation
data-layer timeouts rather than single-request budgets; update
docker-mariadb-clean-arch/internal/city/handler.go lines 12-16 and
docker-mariadb-clean-arch/internal/user/handler.go lines 10-16, with no context
propagation changes required.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f2817a69-3e4d-4cfa-8cfd-cacb75a9e26c
📒 Files selected for processing (4)
docker-mariadb-clean-arch/internal/city/handler.godocker-mariadb-clean-arch/internal/city/middleware.godocker-mariadb-clean-arch/internal/user/handler.godocker-mariadb-clean-arch/internal/user/middleware.go
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
docker-mariadb-clean-arch/internal/city/handler.go (2)
5-10: LGTM!
77-80: LGTM!Also applies to: 110-113, 144-147, 168-171
docker-mariadb-clean-arch/internal/city/middleware.go (1)
21-24: LGTM!docker-mariadb-clean-arch/internal/user/handler.go (2)
5-8: LGTM!
71-74: LGTM!Also applies to: 103-106, 136-139, 160-163
docker-mariadb-clean-arch/internal/user/middleware.go (1)
21-24: LGTM!
Raised by CodeRabbit on review, and it is a real consequence of adding timeouts in the previous commit. GetCities and GetUsers iterate with res.Next() and return the collected slice once the loop ends, without checking res.Err(). res.Next() also returns false when the context is cancelled, so a query that times out or whose client disconnected mid-iteration produced a partial slice and a nil error, and the handler answered HTTP 200 with incomplete data. This was latent before: with context.Background() nothing ever cancelled, so the loop only ended early on a driver error. Adding real deadlines made the path reachable, so the check belongs in the same change. Also corrects the requestTimeout comment. It described the constant as bounding a request, but routes that run the existence middleware before the handler make two independent calls and can wait up to twice as long. It bounds one data layer call.
…ately Raised by Copilot on review. In Fiber middleware `return c.Next()` runs the whole downstream chain before the middleware itself returns, so a deferred cancel() only fires at the very end. The context is used for one repository lookup, but its timer stayed armed for the entire request. Calling cancel() directly after the lookup releases it as soon as it is no longer needed. Safe here because GetCity and GetUser scan the row into the struct before returning, so nothing lazily depends on the context afterwards, and placing the call before the error branches covers every return path.
There was a problem hiding this comment.
🟢 Ready to approve
The changes consistently propagate request cancellation and add per-operation deadlines without introducing API breaks, and the added Rows.Err() handling correctly avoids partial-success returns on cancellation.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The changes consistently propagate request-scoped contexts with timeouts and add correct rows.Err() handling without introducing API or behavioral inconsistencies in the touched recipe paths.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Closes #2718.
The problem
Every handler and middleware in this recipe passed
context.Background()straight through the service into the repository and on todatabase/sql. Nothing ever set a deadline, so a slow or hung query blocked the handler for as long as the driver kept waiting, and a client that disconnected left the query running.The issue reported this against
context.WithCancel, which the code used in 2023:The reporter was right, and for a sharper reason than they gave. The
cancelfunc was deferred but nothing else could ever trigger it, so it cancelled nothing. The call was later simplified to a plaincontext.Background(), which removed even that.The fix
All twelve call sites now derive from the request context and add a deadline:
This covers both halves of the problem, which a bare
WithTimeout(context.Background(), ...)would not:c.Context()is cancelled when the client disconnects, so an abandoned request stops occupying a database connectionrequestTimeoutis a package level constant incityanduser, set to 10 seconds. Kept per package rather than shared, so the two stay independent as the clean architecture layout intends.Sites touched: 5 in
city/handler.go, 1 incity/middleware.go, 5 inuser/handler.go, 1 inuser/middleware.go.On the
wontfixlabelWorth flagging, since it looks like a maintainer decision and is not one.
.github/stale.ymlsetsstaleLabel: wontfix, so the label was applied automatically by the stale bot. All five comments on the issue are bot noise, three welcome-bot and two stale-bot. Nobody ever answered the technical question.The same applies to #2704, #2705 and #2706, which carry the label for the same reason.
Verification
go build ./...passesgo vet ./...passescontext.Background()calls remain anywhere in the recipeNot exercised: the running stack against MariaDB, which needs the docker-compose environment.
Summary by CodeRabbit