Standardise deferred pgx Rollback() calls on a non-cancelable context - #420
Standardise deferred pgx Rollback() calls on a non-cancelable context#420dpage wants to merge 3 commits into
Conversation
The pgx v5 driver treats a rollback issued on a cancelled context as a failed rollback, and a failed rollback is unrecoverable: pgx calls conn.die(), so the pooled connection is discarded whilst its transaction is still open on the server. A client that closes a tab or times out mid-request therefore leaked a connection in an aborted-transaction state, and could also trip the close-of-closed-channel panic tracked as jackc/pgx#2470. Two call sites already guarded against this, each carrying its own copy of the explanation, whilst the remaining twenty passed the request-derived context. This sweeps every rollback in the server, collector, and alerter onto context.Background(), whether deferred directly, deferred inside a cleanup closure, or issued inline on an error path. Transaction semantics, commit paths, and error handling are untouched; only the context handed to Rollback changes. The rationale now lives once in .claude/golang-expert/ transaction-rollback.md, with a short note under "Go Code" in the contributor guide so new transactions get it right by default, and the duplicated inline explanations are reduced to a pointer. Two tests lock the behaviour in. A regression test drives UnacknowledgeAlert with a pgx QueryTracer that cancels the request context the moment the last statement completes, then asserts the ROLLBACK ran on an uncancelled context and that the pooled backend PID is unchanged; it fails on the pre-fix code for both reasons. A convention test parses every non-test Go file in all three modules and rejects any single-argument Rollback call that is not given context.Background(). Coverage of the touched transaction paths is raised alongside, including the previously untested AcknowledgeAlert, SetNodeRelationships, and RemoveServerFromCluster. Closes #381
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 28 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 458 |
| Duplication | 176 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
@coderabbitai review |
|
Every function whose rollback the sweep converted now clears the project's 90% line-coverage floor, and every modified line but one is executed by a test. The three performance-summary endpoints and the query executor were the gap: their handlers had no test at all, so the converted rollback lines were never reached. The new handler tests follow the harness pattern established in PR #416, wiring a real handler to the local Postgres over a trimmed metrics schema and driving it through httptest. Distinct helper and schema names keep the two test files independent, and no production line #416 touches is modified here. Coverage of the touched units, before and after: handlePerfSummary 0.0% -> 94.5% handleDatabaseSummaries 0.0% -> 90.0% handleTopQueries 0.0% -> 95.9% executeQuery 25.0% -> 91.7% UnacknowledgeAlert 81.0% -> 90.5% DeleteAutoDetectedCluster 76.0% -> 92.0% RemoveServerFromCluster 0.0% -> 95.2% Migrate (collector) 72.7% -> 90.9% Reaching the last few branches needed realistic failure injection rather than mocks: a query tracer that cancels the request context between statements, schema drift such as a dropped column, a migration that closes its own connection, and a monitored server on a dead port. The one modified line that remains uncovered is the rollback inside StoreMetrics in collector/src/probes/storage.go. Its guard tests an outer err that the INSERT and Commit failure paths shadow with :=, so the guard can never be true and the rollback is unreachable. That is a pre-existing defect worth its own issue; this change deliberately does not alter the behaviour.
The new executeQuery tests seeded a connection row with no password, which works against a loopback server using trust authentication but fails on CI, where Postgres authenticates. The fixture now encrypts the test connection string's password with a fixed test secret and stores it the way the production create path does, so the handler also exercises the decrypt step. A target with no password still stores NULL.
1 participant
Summary
Deferred
tx.Rollback()calls were inconsistent across the tree: twosites deliberately passed
context.Background(), each carrying its owncopy of the explanation, whilst the remaining twenty passed the
request-derived
ctx. This sweeps every rollback in the server,collector, and alerter onto a non-cancelable context, in one deliberate
pass.
The reason it matters is that pgx v5 treats a rollback on a cancelled
context as a failed rollback, and a failed rollback is unrecoverable:
conn.die()runs, so the pooled connection is discarded whilst itstransaction is still open on the server. A client that closes a tab or
times out mid-request therefore leaked a connection in an
aborted-transaction state, and could also trip the
close-of-closed-channel panic tracked as
jackc/pgx#2470.
Transaction semantics, commit paths, and error handling are untouched;
only the context handed to
Rollbackchanges.Sites converted (20)
server/src/internal/api/perf_summary_handlers.go:handlePerfSummary,handleDatabaseSummaries,handleTopQueriesserver/src/internal/api/query_handlers.go: the deferred cleanup inexecuteQueryserver/src/internal/database/cluster_queries.go:DeleteAutoDetectedCluster,DismissAutoDetectedClusterKeysserver/src/internal/database/relationship_queries.go:SetNodeRelationships,SyncAutoDetectedRelationships,RemoveServerFromClusterserver/src/internal/database/alert_queries.go:AcknowledgeAlert,UnacknowledgeAlertserver/src/internal/tools/transaction.go: seven rollbacks acrossBeginReadOnlyTxandBeginTx(panic recovery, uncommitted cleanup,and setup failure)
alerter/src/internal/database/alert_queries.go:ReactivateAlertcollector/src/probes/storage.go:StoreMetricscollector/src/database/schema.go: both migration error pathsserver/src/internal/database/connection_queries.go(DeleteConnection)was already hardened and keeps its behaviour, with the duplicated inline
rationale reduced to a pointer.
server/src/internal/auth/token_scope.gousesdatabase/sql, whoseRollback()takes no context, so it needs no change.The issue listed 10 sites; the sweep also covers the deferred cleanup
closures and inline error-path rollbacks in
query_handlers.go,tools/transaction.go,probes/storage.go, andschema.go, because ahalf-converted file cannot be enforced or explained coherently. The two
schema.gosites already ran on a background context in practice(
Migratecreates its own), so the change there makes the intentexplicit rather than altering behaviour.
Documentation
.claude/golang-expert/transaction-rollback.md, listed in thegolang-expert agent's knowledge base.
docs/developer-guide/contributing.md, so new transactions get itright by default.
docs/changelog.mdrecords the fix.Test plan
server/src/internal/database/rollback_context_integration_test.goisthe behavioural regression test. It drives
UnacknowledgeAlertover asingle-connection pool with a pgx
QueryTracerthat cancels therequest context the moment the last statement completes, then asserts
the
ROLLBACKran on an uncancelled context and that the pooledbackend PID is unchanged. Verified to fail on the pre-fix code for
both reasons: the rollback ran on the cancelled context, and the
backend PID changed because pgx discarded the connection.
server/src/internal/database/rollback_convention_test.goparses everynon-test Go file under
server/src,collector/src, andalerter/srcand rejects any single-argumentRollbackcall not givencontext.Background(). Verified to fail when one site is reverted.server/src/internal/tools/transaction_rollback_integration_test.gocovers all seven rollbacks in
BeginReadOnlyTx/BeginTx, includingthe panic-recovery and setup-failure paths.
query executor end to end, following the harness pattern established in
Fix two dashboard charts that plotted the wrong data #416: a real handler wired to Postgres over a trimmed metrics schema,
driven through
httptest. Helper and schema names are distinct fromFix two dashboard charts that plotted the wrong data #416's, and no production line that Fix two dashboard charts that plotted the wrong data #416 touches is modified here.
rather than mocks: a query tracer that cancels the request context
between statements, schema drift such as a dropped column, a migration
that closes its own connection, and a monitored server on a dead port.
Coverage of the touched units
Every function whose rollback changed now clears the 90% floor:
handlePerfSummaryhandleDatabaseSummarieshandleTopQueriesexecuteQueryBeginReadOnlyTxBeginTxAcknowledgeAlertUnacknowledgeAlertSetNodeRelationshipsRemoveServerFromClusterSyncAutoDetectedRelationshipsDeleteConnectionDeleteAutoDetectedClusterDismissAutoDetectedClusterKeysReactivateAlert(alerter)StoreMetrics(collector)Migrate(collector)Of the 22 modified lines, 21 are executed by tests, verified block by
block against the coverage profiles.
One uncoverable line, and why
The rollback in
StoreMetrics(
collector/src/probes/storage.go) cannot be reached by any test. Itsdeferred guard tests the outer
errfromconn.Begin, but both theINSERT and Commit failure paths declare their own
errwith:=, so theouter variable is never assigned and the guard is never true:
The existing
TestStoreMetrics_ErrorPathprovokes exactly that failureand the rollback still does not run. This is a pre-existing defect rather
than something the sweep introduced, and fixing it would change
transaction behaviour, so it is left alone here and is worth its own
issue.
Verification
gofmtclean andgolangci-lint runreports 0 issues in all three Gomodules. Note that
misspellenforces the US locale in Go sources, sothe new tests use "canceled" whilst the prose keeps British spelling.
make coveragefor the server, collector, and alerter run againsta local Postgres. The only failures are the two pre-existing pgvector
fixture failures in
internal/tools(
TestStoreMemoryGeneratesEmbeddingIntegration,TestRecallMemoriesGeneratesQueryEmbeddingIntegration), confirmed tofail identically on
mainand already addressed by fix(server): widen memory embedding test fixture to halfvec(4000) #382. They skip inCI, which has no pgvector.
🤖 Generated with Claude Code
https://claude.ai/code/session_01N44XgdgR2msyNydAao5vXY