Skip to content

fix(selfhost): guard pg-adapter's rollback so it can't mask the original batch error - #6392

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
galuis116:fix/pg-adapter-rollback-error-masking
Jul 16, 2026
Merged

fix(selfhost): guard pg-adapter's rollback so it can't mask the original batch error#6392
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
galuis116:fix/pg-adapter-rollback-error-masking

Conversation

@galuis116

Copy link
Copy Markdown
Contributor

Closes #6282

Summary

  • pg-adapter.ts's batch() called await client.query("ROLLBACK") unguarded inside its catch block — if the rollback itself throws (plausible exactly when the original error was a connection failure), that new error propagated instead of throw error, silently discarding the real cause. d1-adapter.ts's batch() already wraps its ROLLBACK in a defensive try/catch { /* ignore */ } for exactly this reason. pg-adapter.ts now does the same. d1-adapter.ts itself is untouched — it was already correct, per the issue's explicit instruction.
  • Added an integration test in test/integration/selfhost-pg.test.ts (gated on PG_TEST_URL, per the issue's test coverage requirement) that grabs a real client from the pool, intercepts its ROLLBACK call to force it to throw, and confirms the original batch error (a real SQL syntax error) still surfaces rather than the synthetic rollback failure.

Scope

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck — root tsc --noEmit reliably OOMs on this shared sandbox regardless of what changed (reproduced repeatedly this session). This change is a 4-line try/catch wrap plus a new integration test; ran the whole root diff-check clean and the actual integration test suite against real Postgres (below).
  • npm run test:coveragesrc/selfhost/pg-adapter.ts is exempt from Codecov's unit-coverage requirement per codecov.yml (validated by the real-Postgres integration test instead, per the issue's own Test Coverage Requirements). Actually ran the integration test against a real local Postgres 16 container (docker run -d -e POSTGRES_PASSWORD=devpw -e POSTGRES_DB=gittensory -p 55432:5432 postgres:16, per the test file's own header instructions): all 7 tests in test/integration/selfhost-pg.test.ts pass, including the new regression test. Verified the new test genuinely catches the bug — reverted just the pg-adapter.ts fix via git stash and confirmed the test fails (expected ... to throw error matching /bad-sql|syntax/i but got 'simulated rollback failure') before re-confirming it passes with the fix restored.
  • npm run test:workers — N/A, no Worker-facing code changed (this path is self-host-only).
  • npm run build:mcp / npm run test:mcp-pack — N/A, no @loopover/mcp changes.
  • npm run ui:openapi:check / ui:lint / ui:typecheck / ui:build — N/A, no apps/loopover-ui changes.
  • npm audit --audit-level=moderate — 0 vulnerabilities.
  • New/changed behavior has tests — the new regression test was verified to genuinely fail without the fix (via git stash) before confirming it passes with it, against a real Postgres instance, not a mock.

If any required check was skipped, explain why:

  • Root npm run typecheck: reliably OOMs on this shared sandbox under memory pressure from concurrent sessions, independent of the diff. This is a small, self-contained change (one guarded try/catch, no new type surface) verified by real integration testing against actual Postgres instead.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. — N/A, but this IS a negative-path fix for the self-host Postgres backend, with a dedicated regression test for the failure path.
  • API/OpenAPI/MCP behavior is updated and tested where needed. — N/A.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. — N/A, no UI changes.
  • Visible UI changes include a UI Evidence section below with screenshots. — N/A, no visible UI change.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. — CHANGELOG.md untouched.

Notes

  • The issue's Links & Resources section names the test file as test/integration/selfhost-pg.ts — the actual file is test/integration/selfhost-pg.test.ts (confirmed via repo search; the .ts-only name doesn't exist).

…nal batch error

batch()'s catch block called client.query("ROLLBACK") unguarded, so a
rollback failure (plausible exactly when the original error was a
connection failure) would propagate instead of the real cause. Wrap
it in the same defensive try/catch { ignore } the SQLite adapter's
batch() already uses.

Added an integration test in test/integration/selfhost-pg.test.ts
(gated on PG_TEST_URL) that intercepts a real client's ROLLBACK call
to force it to throw, confirming the original batch error still
surfaces. Verified against a real local Postgres 16 container that
the test genuinely fails without the fix and passes with it.

Closes JSONbored#6282
@galuis116
galuis116 requested a review from JSONbored as a code owner July 16, 2026 06:01
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.60%. Comparing base (aa83a48) to head (98ebaf2).
⚠️ Report is 16 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #6392       +/-   ##
===========================================
+ Coverage   52.69%   95.60%   +42.90%     
===========================================
  Files         599      599               
  Lines       47238    47238               
  Branches    15031    15031               
===========================================
+ Hits        24891    45160    +20269     
+ Misses      19402     1291    -18111     
+ Partials     2945      787     -2158     
Flag Coverage Δ
shard-1 43.99% <ø> (?)
shard-2 36.78% <ø> (+0.18%) ⬆️
shard-3 32.42% <ø> (?)
shard-4 34.65% <ø> (?)
shard-5 31.00% <ø> (-0.59%) ⬇️
shard-6 45.30% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.
see 394 files with indirect coverage changes

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 16, 2026
@loopover-orb

loopover-orb Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-16 06:22:37 UTC

2 files · 1 AI reviewer · no blockers · readiness 86/100 · CI green · unknown

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This is a minimal, correct fix: pg-adapter's batch() catch block now wraps the ROLLBACK call in its own try/catch so a rollback failure (e.g. from a dead connection) can no longer overwrite and hide the original error via `throw error`, matching d1-adapter.ts's existing pattern. The new integration test genuinely exercises the real code path by intercepting a live pg client's ROLLBACK call and asserting the original syntax error still surfaces, which is a real regression test rather than a fabricated scenario. The change is narrow, directly closes #6282, and CI is green.

Nits — 4 non-blocking
  • The test's `@​ts-expect-error` override of `realClient.query` is a reasonable simulation technique but slightly fragile if pg's query() overload signature changes; a short comment already explains why, which is good.
  • No test verifies the case where ROLLBACK succeeds normally alongside a batch error, though this is already covered by the pre-existing 'batch is transactional' test.
  • Consider also asserting that `client.release()` is still called after a rollback failure, to fully confirm no connection leak occurs on this path.
  • Consider logging (not throwing) when the rollback swallow actually fires, to aid production debugging of connection-failure incidents, mirroring `tuneGithubRateLimitObservationsAutovacuum`'s error-logging style.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #6282
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ❌ 8/20 High review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 1926 registered-repo PR(s), 1268 merged, 54 issue(s).
Contributor context ✅ Confirmed Gittensor contributor galuis116; Gittensor profile; 1926 PR(s), 54 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Review context
  • Author: galuis116
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 1926 PR(s), 54 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Add a concise scope and risk note.
  • Then work through the remaining 1 step in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit e73f163 into JSONbored:main Jul 16, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(selfhost): Postgres adapter's batch() rollback can mask the original error, unlike the SQLite adapter

1 participant