Fix connection pool leak in RunChecks when table validation fails

[mason-sharp]

At v1.8.0, TableDiffTask.RunChecks() created a connection pool per node but only closed it on the success path (line 941). Any error before that — missing table, no primary key, schema mismatch, or insufficient privileges — returned without closing the pool. Because pgxpool keeps a background health-check goroutine alive, leaked pools and their connections persisted for the lifetime of the process.

In a scheduled repset-diff, the leaks accumulated across runs: if N tables consistently failed RunChecks, each 10-minute tick leaked N more connections. A user with 100 tables saw 79 leaked connections after their ACE user lost table privileges.

Wrap the per-node work in a closure so defer conn.Close() scopes to each loop iteration and covers all error paths.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4ed883f8-69ea-4069-a875-4149fcbe3869

📥 Commits

Reviewing files that changed from the base of the PR and between 55fdce4 and 09650f8.

📒 Files selected for processing (1)

• tests/integration/repset_diff_connleak_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/integration/repset_diff_connleak_test.go

---

📝 Walkthrough

Walkthrough

Refactors per-node connection handling in TableDiffTask.RunChecks to use inline error-returning blocks with deferred conn.Close(), and adds two integration tests that exercise diff connection behavior and verify no connection leaks under normal, concurrent, and error conditions.

Changes

Cohort / File(s)	Summary
Connection Handling Refactor internal/consistency/diff/table_diff.go	Refactored per-node connection logic into inline func() error { ... }() blocks; added defer conn.Close() immediately after acquiring connections; adjusted control flow for TableFilter and propagated errors via returns from the inner block.
Connection Leak Integration Tests tests/integration/repset_diff_connleak_test.go	Added two integration tests: TestConnLeak_100Tables_ConnectionExhaustion (checks connection peaks under single and concurrent runs) and TestConnLeak_RunChecksLeakAccumulation (repeated failing runs ensure no accumulated connection leaks).

Poem

🐰 I nibble logs and count each thread,
Deferred closes tuck connections to bed,
Concurrent hops don't leave a trail,
Errors bounce but pools prevail,
Carrots safe — no leaks ahead!

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately summarizes the primary change: fixing a connection pool leak in the RunChecks method when table validation fails.
Description check	✅ Passed	The description is directly related to the changeset, explaining the bug (connection pool leak on error paths), its impact (accumulating leaks in scheduled runs), and the fix (wrapping per-node work in a closure with deferred close).
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch connection-leak-error

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codacy-production]

Up to standards ✅

🟢 Issues 5 medium

Results:
5 new issues

Category	Results
Complexity	5 medium

View in Codacy

🟢 Metrics 50 complexity · 14 duplication

Metric	Results
Complexity	50
Duplication	14

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/integration/repset_diff_connleak_test.go`:
- Around line 135-167: The peak-connection assertion is flaky because
monitorConnections (used via monitorPools and pollInterval) only samples on
ticker edges and can miss brief spikes; update the test to ensure peaks can't be
missed by either increasing sampling frequency and/or taking continuous/final
immediate samples: modify the monitorConnections invocation or implementation to
sample more frequently (reduce pollInterval), add an immediate sample before
stopping and another right after diff.RepsetDiff returns, and/or extend
monitorConnections to record instantaneous max from pg_stat_activity on stop;
reference monitorConnections, pollInterval, snapshots, stopMonitor and the
helper in repset_diff_conntrack_test.go to locate and change the sampling
behavior so the peak assertion against maxConnections is reliable.
- Around line 380-387: The test currently ignores the result of
diff.RepsetDiff(task); change the loop to capture and assert that it returns an
error (or a specific error type/message) so the bad-table RunChecks path is
exercised. Specifically, call err := diff.RepsetDiff(task) and add an assertion
(e.g., require.Error/Assert.Error and optionally
require.Contains/Assert.Contains against the expected "missing table" message)
for each invocation created by newTestRepsetDiffTask(repsetName) with
SkipDBUpdate=true and MaxConnections set; this ensures the broken-table failure
path (RunChecks) was actually hit rather than silently discarded.

🪄 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

Run ID: d7cd68f6-2628-4692-abe9-5bb66a3deef7

📥 Commits

Reviewing files that changed from the base of the PR and between 779f8ee and 55fdce4.

📒 Files selected for processing (2)

• internal/consistency/diff/table_diff.go
• tests/integration/repset_diff_connleak_test.go

[ibrarahmad]

LGTM