Skip to content

Thread request and task context through the DB layer - #49

Merged
Aloento merged 2 commits into
mainfrom
refactor/thread-request-context
Oct 4, 2026
Merged

Aloento merged 2 commits into
mainfrom
refactor/thread-request-context

Conversation

@Aloento

@Aloento Aloento commented Oct 4, 2026

Copy link
Copy Markdown
Member

Summary

Adds a ctx context.Context parameter as the first argument to the DB methods that previously hardcoded context.Background(), and passes the request or task context down from all callers.

Changes

  • DB layer (30 spots): db.go (28), event_types.go (1), notification_ops.go (1) — each method that hardcoded context.Background() now accepts ctx context.Context as its first parameter.
  • API handlers (internal/api/v2): createEvent and publishMaintenanceChange now receive ctx; all db.* calls in the incident creation/patch path use c.Request.Context().
  • Checker (internal/checker): CheckMaintenance / CheckInfoEvents / processMaintenance thread the task ctx from Check(ctx) down to WithTx and PublishTx.
  • RSS feeds, middleware, and other callers: updated to pass the request context.
  • Tests: signature updates only; no new tests added.

Verification

  • go build ./... — clean
  • go vet ./... — clean
  • go test -count=1 ./internal/... — all pass
  • go test -c -o <tmp> ./tests/ — integration suite compiles
  • rg -n "context.Background()" internal/db internal/api/v2 internal/checker — zero matches

Behavior

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.

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.
ecosquad-autoreview[bot]
ecosquad-autoreview Bot previously approved these changes Oct 4, 2026

@ecosquad-autoreview ecosquad-autoreview Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 ctx only 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.

@ecosquad-autoreview ecosquad-autoreview Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@Aloento
Aloento merged commit 844ad25 into main Oct 4, 2026
14 checks passed
@Aloento
Aloento deleted the refactor/thread-request-context branch October 4, 2026 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant