Repository navigation
Thread request and task context through the DB layer - #49
Conversation
Add a ctx context.Context parameter as the first argument to the DB methods that previously hardcoded context.Background() (28 in db.go, plus getEventsByType and EnsureNotificationSchema), and pass the request or task context down from the API handlers, the RSS feeds, the middleware, and the checker. Behavior is unchanged: no new timeouts, no new configuration, no changes to business logic or API output. Request cancellation and timeouts can now propagate to database queries.
There was a problem hiding this comment.
Review — PR #49: Thread ctx through DB layer
Summary: Mechanical, well-scoped refactor that replaces hardcoded context.Background() in the DB facade with a caller-supplied ctx, and threads the request/task context down from API handlers, RSS, middleware, and the checker. The visible portion is internally consistent: every DB method that took a context already propagates it to all internal Ent queries (e.g. GetEventsWithCount uses ctx for Count, query.All, and statusesByIncident; GetIncident uses it for First and statusesByIncident). No correctness bugs, security issues, or data-loss risk found in the code I could see.
Note: the PR diff is large and only the leading portion was available for reading (db.go was truncated after SaveIncidentTx). CI (build, go-test, go-test-acc, golangci-lint) is still queued/pending at the time of this review, so compilation and tests were not independently confirmed — the author's reported go build / go vet / go test results should be corroborated by the green CI runs before merge.
Findings
Suggestion — misleading comment now that the context is threaded (internal/checker/checker.go:27-30):
The doc comment on Check now reads "Cancellation is observed only before the round starts, so a caller must not close the pool while Check is running." Before this PR that was accurate because the two scans used context.Background(). Now CheckMaintenance(ctx) / CheckInfoEvents(ctx) pass the task ctx into GetMaintenances, GetInfoEvents, GetIncident, WithTx, and ModifyIncidentTx. If the scheduler's task context carries a deadline/cancel, cancellation is now observed during the round, not just before it. The comment is stale and the behavioral contract has actually changed.
- Fix: Update the comment to reflect that cancellation is now observed throughout the round, and document the intent for the pool-lifetime caveat.
Suggestion — confirm mid-round cancellation is acceptable for the checker (internal/checker/checker.go:42-59):
Check spawns two goroutines that share the single task ctx, each running a loop that does a per-event refetch + write transaction. Previously these loops ran to completion on context.Background(). If the scheduler wraps each scan in a context with a timeout, a slow round can now be aborted mid-loop, leaving some maintenances/info-events partially processed for that round. That may be the desired behavior (bounded work), but it is a behavioral change worth an explicit decision.
- Fix: Verify how the scheduler builds the task context (deadline or not). If scans are meant to always run to completion, derive a non-cancelling context for the loop work and use the task
ctxonly for the pre-check; otherwise keep the current behavior and add a short comment documenting the bounded-scan trade-off.
No blocking issues found. Recommend the verdict be confirmed against the pending CI runs; if they go green, this is approvable with the two doc/behavior items above addressed.
Adding the ctx parameter pushed several signatures past the 120-column limit; wrap them. The Check comment claimed cancellation is only observed before the round starts, but ctx is now threaded into the scan, so state that cancellation is observed throughout the round.
There was a problem hiding this comment.
Summary
Mechanical refactor that threads a ctx context.Context as the first argument through the DB layer (db.go, event_types.go, notification_ops.go) and updates all callers (API v2 handlers, middleware, RSS, checker) to pass c.Request.Context() or the checker's task context instead of hardcoded context.Background(). Reviewed v2.go, db.go, middleware.go, rss.go, checker.go and grepped for residual context.Background() in the touched packages (none remain in internal/db, internal/api/v2, internal/checker).
The change is internally consistent: every signature change is matched by a caller update, and the checker comment was correctly updated to reflect that cancellation is now observed mid-round. I could not run the build or tests (CI build/go-test/go-test-acc are still queued), so this is a read-only review of the diff only.
Findings
Suggestion — "No behavior change" is not strictly true
The PR description claims "No behavior change," but threading c.Request.Context() (and the checker task ctx) in place of context.Background() does change runtime behavior: a client disconnect, cancellation, or gateway timeout will now abort in-flight DB queries, and any WithTx (e.g. incident creation/patch at internal/api/v2/v2.go:1084) will be rolled back mid-request. Previously those operations ran to completion regardless of the client. This is almost certainly the desired outcome (the PR says so explicitly), but the "No behavior change" wording is misleading and could hide the fact that slow writes + client timeout can now drop a partially-built incident. Worth correcting the description; no code fix required.
Suggestion — checker mid-round abort is intentional but worth noting
internal/checker/checker.go now passes the task ctx to CheckMaintenance/CheckInfoEvents. If the scheduler cancels a round, a scan can stop halfway, leaving remaining events to the next tick (the updated comment documents this). This is fine, but confirm the scheduler's per-round context actually carries a timeout/cancel — if it's a plain context.Background() with no deadline, the behavior is identical to before and the new comment is the only real change.
Notes
- I did not verify compilation or test results; CI is pending at review time. The description's stated verification (
go build,go vet,go test) should be confirmed once checks complete. - No security, data-loss, or obvious correctness defects found in the reviewed diff.
Verdict: approve — the change is a consistent context-propagation refactor; the only real concern is the inaccurate "no behavior change" claim in the description.
Summary
Adds a
ctx context.Contextparameter as the first argument to the DB methods that previously hardcodedcontext.Background(), and passes the request or task context down from all callers.Changes
db.go(28),event_types.go(1),notification_ops.go(1) — each method that hardcodedcontext.Background()now acceptsctx context.Contextas its first parameter.internal/api/v2):createEventandpublishMaintenanceChangenow receivectx; alldb.*calls in the incident creation/patch path usec.Request.Context().internal/checker):CheckMaintenance/CheckInfoEvents/processMaintenancethread the taskctxfromCheck(ctx)down toWithTxandPublishTx.Verification
go build ./...— cleango vet ./...— cleango test -count=1 ./internal/...— all passgo test -c -o <tmp> ./tests/— integration suite compilesrg -n "context.Background()" internal/db internal/api/v2 internal/checker— zero matchesBehavior
No behavior change. No new timeouts, no new configuration, no changes to business logic or API output. Request cancellation and timeouts can now propagate to database queries.