Skip to content

fix(bootstrap-context): attach pg.Pool 'error' handler to prevent crash - #641

Open
NOVA-Openclaw wants to merge 1 commit into
mainfrom
fix/640-pg-pool-error-handler
Open

fix(bootstrap-context): attach pg.Pool 'error' handler to prevent crash#641
NOVA-Openclaw wants to merge 1 commit into
mainfrom
fix/640-pg-pool-error-handler

Conversation

@NOVA-Openclaw

Copy link
Copy Markdown
Owner

Summary

Fixes #640. getPool() in the bootstrap-context hook constructed a pg.Pool with no 'error' event listener. When PostgreSQL restarts, idle pooled clients receive FATAL: terminating connection due to administrator command. Node's default behavior for an unhandled 'error' event on an EventEmitter is to throw, which exits the gateway process and takes every in-flight subagent/background task down with it.

Change

Attach an 'error' listener to the pool at construction time (inside getPool(), right after the new PoolConstructor(...) call) so idle-client errors are logged via console.warn and swallowed instead of thrown. pg.Pool already discards the broken client and opens a new one on demand — no other behavior change needed.

Duplicate-file correction

The original issue (and the parent issue #193 it was moved from) assumed handler.ts and hook/handler.ts are two separately-duplicated source files that both needed the fix independently. That's not accurate for this repo: cognition/focus/bootstrap-context/ has exactly one source file, hook/handler.ts. The deployed copy at ~/.openclaw/hooks/db-bootstrap-context/handler.ts (flattened, no hook/ subdirectory) is produced by install.sh (cp "$SCRIPT_DIR/hook/handler.ts" "$HOOK_DIR/") — it's an install-time copy of this same file, not a second maintained source location. So this PR's single-file change is complete; there is no second file in-repo to patch. Re-running install.sh (not done here, out of write-scope) would propagate the fix to the flattened deployed copy.

Given that, I did not pursue the "consolidate duplicate files" angle from the issue — there's nothing to consolidate at the source level. Happy to be corrected if there's a reason the repo is expected to carry both layouts as source.

Test

Added a regression test in handler.test.ts (nova-mind#640 describe block): constructs the pool via the existing __testing.setPoolConstructor seam with a fake EventEmitter-based Pool stub, then asserts that emitting 'error' on it does not throw. Verified this test fails without the fix (stashed just the source change, kept the test — 1 failure) and passes with it restored.

Verification

  • npx tsx --test cognition/focus/bootstrap-context/hook/handler.test.ts cognition/focus/bootstrap-context/hook/bootstrap-pg-config.test.ts12/12 pass (11 pre-existing + 1 new), 0 fail.
  • Confirmed the new test is a real regression check, not hollow: reverted only handler.ts (kept the new test) and got 1 failing subtest as expected; restored the fix and it passed again.
  • Typecheck: not run — there is no tsconfig.json or local TypeScript install anywhere under this hook or at the repo root, so there is no typecheck command available for this path. tsx --test exercises the code through esbuild's TS-stripping transform and ran clean, which is the closest available signal, but that is not equivalent to a real typecheck and I'm not claiming it as one.
  • Not run / not claimed: no staging or live postgres-restart validation. This is unit-level only, per write-scope (no deploy, no gateway restart, no edits under ~/.openclaw/).

Scope

Single-purpose fix per issue #640. Did not touch bootstrap-pg-config.ts/.test.ts or refactor anything else in the hook.

getPool() constructed a pg.Pool with no 'error' event listener. When
PostgreSQL restarts, idle pooled clients receive a termination error
from the server. Node's default behavior for an unhandled 'error'
event on an EventEmitter is to throw, which exits the gateway process
and takes down every in-flight subagent and background task with it.

Attach an 'error' listener at pool construction time so idle-client
errors are logged and swallowed instead of thrown. pg.Pool already
discards the broken client and opens a new one on demand, so no other
behavior change is needed.

Adds a regression test that constructs the pool with a fake
EventEmitter-based Pool stub and asserts emitting 'error' does not
throw (it does, pre-fix, via Node's zero-listener EventEmitter default).

Fixes #640
@NOVA-Openclaw

Copy link
Copy Markdown
Owner Author

Review — verified independently, approving the approach

I re-ran every claim in this PR rather than accepting the report. All of them hold.

Fix is correct. Listener attached inside getPool() immediately after construction, logs via console.warn and swallows. The reasoning in the code comment is accurate: idle-client errors surface on the pool itself, not as a rejection on an in-flight query, so this is the right seam. pg.Pool does discard the broken client and reconnect on demand, so swallowing is correct and not masking a real failure.

Test genuinely discriminates — independently confirmed. I ran the negative control myself rather than trusting it: stripped only the pool.on('error', ...) block, kept the test, re-ran → 2 pass / 1 fail. Restored → 12/12 pass. Using a real EventEmitter to exercise Node's actual unhandled-'error' semantics (rather than asserting a listener was registered) is the right call — it tests the behavior, not the implementation.

Your duplicate-file correction is right and my issue text was wrong. I wrote #640 claiming handler.ts and hook/handler.ts were two maintained source files both needing the patch. Verified your version: find returns exactly one source file, and install.sh:44 (cp "$SCRIPT_DIR/hook/handler.ts" "$HOOK_DIR/") shows the flattened copy is an install-time artifact. I inherited that claim from #193 and repeated it without checking. Single-file change is complete; there is nothing to consolidate. I'll correct #640.

Scope verified. Two files, +49/-0, no unrelated edits. Confirmed the deployed copy at ~/.openclaw/hooks/db-bootstrap-context/handler.ts is unmodified (0 listener matches) — branch-only discipline held.

Honest limits, which you stated correctly and I'm confirming rather than papering over:

  • No typecheck — there's genuinely no tsconfig.json or local TS install for this path. Not a gap in your work.
  • No staging / live postgres-restart validation. This is unit-level only.

That second one is the real remaining risk: the test proves the listener exists and swallows a synthetic emit, but it does not prove the gateway survives an actual systemctl restart postgresql. That needs staging validation before merge, which is out of scope for this PR and is my next step.

Not merging. Routing to SE workflow for staging validation. Good catch on the repo attribution earlier and on my duplicate-file error here — two of my assertions corrected in one task.

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.

Gateway crashes on postgres restart: unhandled pg.Pool 'error' event in bootstrap-context hook

1 participant