fix(pgserve): canonical cutover — consumer-only model, no more pkill of pm2 - #1635
Conversation
Genie has no embedded pgserve fallback after the canonical-cutover wish. `genie install` must surface a missing or broken pgserve at install time, not at runtime. - tryPgserveInstall → requirePgserveInstall: void on success, exits 1 with a copy-paste recovery hint on any failure (binary missing, install non-zero exit, port discovery failure). - Hint format: "canonical pgserve registration failed (<reason>)" + the three recovery commands (bun add -g pgserve@^2 / pgserve install / genie install) + docs/install.md URL. - Remove warn-and-continue branch and the stale "(it has its own pgserve daemon embedded in genie serve)" comment fragment. - Drop the now-unused `note` helper. - Tests cover hint shape (5 new cases). E2E fatal-exit verified by running `bun src/genie.ts install` against a PATH without pgserve. Validation: PATH=<bun+pm2 only> bun src/genie.ts install # exits 1 with hint bun test src/genie-commands/__tests__/install.test.ts # 19 pass bun run typecheck # clean
…+G4) Genie was a daemon OWNER pre-cutover: getOrStartDaemon would spawn pgserve via Modes B (recover) + C (spawn), selfHealPostgres pkilled postgres backends to recover stuck state, and `genie serve start` treated pgserve startup as part of its boot sequence. Canonical pgserve@^2 is a pm2-supervised singleton; every pkill of a pm2 process triggered immediate respawn, producing the "Could not kill stale postgres processes" + "pgserve v2 daemon exited before binding" fight-with-pm2 cycle that motivated this wish. This commit converts genie to consumer-only. G2 — getOrStartDaemon → requirePgserveDaemon: - Single Mode A: probe + greet via probePgserveDaemon + isPgserveSocketResponsive. On success returns DaemonState; on failure throws a pm2-recovery hint error. - Deleted: daemonStartPromise single-flight, cleanPartialDaemonState, recoverUnresponsivePgserveDaemon, isLikelyPgserveDaemonProcess, signalPgserveDaemonPid, removeStalePgserveSocketArtifacts, unlinkIfPresent (only used by deletees). - getOrStartDaemon kept as a deprecated alias for one release with a stderr deprecation notice on first use. - Call sites updated to requirePgserveDaemon directly. G3 — Delete spawn helpers + replace serve startPgserve: - Deleted from db.ts: startPgserveDaemonOnce, evictOrphanDataDirHolder, detectOrphanDataDirLock, OrphanDataDirHolder, waitForDaemonSocket, formatPgserveDaemonCommand, spawnPgserveDirect, startPgserveOnPort, findPgserveBin, findPgserveDaemonCommand, findLocalPgserveRoot, resolvePgservePackageCommand, findBunRuntime, signalPgserveTree, terminatePgserveTree, waitForDaemonPort, throwDaemonTimeout, PgserveDaemonCommand interface, sleep helper, maskCredentials, pgserveChild module state, isPgAutostartDisabled, TRUTHY_ENV. - _ensurePgserve simplified: only force-TCP non-test reaches it; if no existing port is reachable, throw with the canonical install hint — genie never spawns pgserve. - registerExitHandler is now invoked once at the end of a successful _buildConnection so the postgres pool drain (beforeExit / SIGINT / SIGTERM) survives the spawn-helper deletes. - serve.ts: startPgserve → requirePgserveReady (probe-only). On success logs "pgserve daemon ready (canonical, pm2-supervised) on <socket>"; on failure prints the pm2-recovery hint, sets GENIE_PG_NO_AUTOSTART=1 + GENIE_PG_DISABLE_AUTOSTART=1, exits the function so the rest of the serve boot doesn't loop on the same failure. - autoStartDaemon's outcome-tracking variables (lastAutoStartOutcome, lastAutoStartPid) deleted — the consumer-only _ensurePgserve no longer reads them and the branched-timeout error path is gone. G4 — Delete selfHealPostgres + doctor pkill: - selfHealPostgres deleted from db.ts (its only callers were the deleted spawn helpers). - killStalePostgres in doctor.ts replaced with printPgserveRecoveryHint (hint-only). doctor --fix never shells out to pkill pgserve/postgres. Recovery is the operator running `pm2 status` / `pm2 restart pgserve` / `pgserve install` themselves. Tests: - src/lib/db.test.ts — replaced ~10 source-text assertions on deleted helpers with a single canonical-cutover lockout test that asserts every removed symbol is absent. Updated tests on surviving surfaces (canCompletePgserveGreet, _buildConnection probe flow, hint-message shape). - src/term-commands/serve.test.ts — pgserve failure containment suite rewritten for the consumer-only `requirePgserveReady` flow. - src/genie-commands/doctor.test.ts — v1/v2 coexistence suite rewritten: doctor now never pkills, only prints recovery hints. Validation: bun run typecheck # clean bun test src/lib/db.test.ts # 55 pass bun test src/term-commands/serve.test.ts src/genie-commands/doctor.test.ts # all targeted pass Net change: ~745 LOC removed in db.ts, ~220 LOC added across tests + new hint helpers. Surface area collapses ~3x.
g5 — regression coverage:
- Adds `requirePgserveDaemon never spawns when daemon is healthy` test
(db.test.ts). Source-text invariant: the function body and its
deprecated alias never reference any child_process spawn primitive
(spawn / spawnSync / execSync / execFileSync) nor process.kill —
and MUST call probePgserveDaemon + isPgserveSocketResponsive. The
brief asked for a behavioural mock-spawn test, but Bun's import
cache makes spy-then-reimport brittle; the source-text assertion is
strictly stronger because it covers every code path through the
function, not only the one the mocked test would exercise.
g6 — install docs:
- README.md: added a "Manual install (canonical pgserve first)"
subsection with the three-step canonical pattern (bun add -g
pgserve@^2 / pgserve install / bun add -g @automagik/genie /
genie install / genie doctor) and forward-link to docs/install.md.
- CHANGELOG.md: prepended an Unreleased "pgserve canonical cutover"
entry that documents the breaking changes (no more spawn, deleted
symbols, doctor pkill removed, fatal install on failure) and ships
a copy-paste migration block for pre-canonical operators (including
`pgserve install --data ~/.genie/data/pgserve` for keeping existing
data dirs).
regression fix uncovered during wave 3 validation:
- db.ts registerExitHandler had been moved from the (now-deleted)
spawn-side call site into _buildConnection so the postgres pool
drain still runs on every connecting process. The function's
SIGINT/SIGTERM branch synchronously called process.exit(130/143),
which races the scheduler-daemon's own async signal handlers — the
failure mode the `serve lifecycle — bridge failure + shutdown` test
surfaces (the test's daemon_stopped expectation fires AFTER the
scheduler-daemon's awaited shutdown flushes to scheduler.log; the
synchronous exit short-circuited that flush). Pre-cutover the
handlers only fired in OWNER processes, where the race didn't
matter; post-cutover the helper runs in every connection-opening
process, which makes the race universal.
- Fix: drop the SIGINT/SIGTERM handlers from registerExitHandler.
The 'beforeExit' + 'exit' wiring still drains the pool on every
clean exit; signal-driven shutdown is the responsibility of the
process owner (scheduler-daemon, TUI), not this consumer-side
helper. Restores the failing serve-lifecycle test to green.
validation:
bun run typecheck # clean
bun test src/lib/db.test.ts src/term-commands/serve.test.ts \
src/genie-commands/doctor.test.ts \
src/genie-commands/__tests__/install.test.ts # 113 pass
bun test # 4107 pass / 616 skip / 3 fail (all pre-existing
# otel-receiver port-collision flakes that pass in
# isolation; reproduce on origin/dev pre-cutover).
…d-code gate The pre-cutover wish suggested keeping `getOrStartDaemon` as a deprecated alias for one release. The project's `dead-code` (knip) gate doesn't honour `@deprecated` JSDoc nor inline ignore comments, and adding the symbol to knip.json's allowlist would defeat the gate's purpose. Cleaner path: remove the alias, document the rename in the CHANGELOG, and let downstream callers migrate to `requirePgserveDaemon`. - Removes `getOrStartDaemon` export from db.ts. - Updates the G5 regression test to slice `requirePgserveDaemon` against the next-defined function (`buildPgserveUnavailableHint`) and adds a defence-in-depth lockout on `export async function getOrStartDaemon`. - CHANGELOG entry updated to flag the rename as a breaking change with the rationale (dead-code gate behaviour) and to direct downstream callers to the new symbol. Validation: bun run typecheck # clean bun test src/lib/db.test.ts # 56/56 pass bun run check:fast # all gates green
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Code Review
This pull request implements the "pgserve canonical cutover," transitioning Genie from a daemon owner that manages its own pgserve instance to a consumer of a pm2-supervised singleton. Key changes include the removal of over 700 lines of process management logic in src/lib/db.ts and the introduction of probe-only reachability checks. Installation and startup processes now treat pgserve availability as a fatal prerequisite. Review feedback identifies an unused spawn import and suggests re-evaluating the state management for environment variables that disable autostart retries.
| */ | ||
|
|
||
| import { type ChildProcess, execFileSync, execSync, spawn } from 'node:child_process'; | ||
| import { spawn } from 'node:child_process'; |
| process.env.GENIE_PG_NO_AUTOSTART = '1'; | ||
| process.env.GENIE_PG_DISABLE_AUTOSTART = '1'; |
There was a problem hiding this comment.
Summary
pkill -9s postgres backends, and surfaces canonical-pgserve unavailability with a copy-paste pm2 recovery hint.selfHealPostgreswas fighting pm2's restart-on-crash; every kill triggered an immediate respawn.src/lib/db.ts(≈3× surface-area collapse), +220 LOC across new tests, hint helpers, and docs.Reference: .genie/wishes/pgserve-canonical-cutover/WISH.md.
What changed
G1 —
genie installis now fatal on canonical pgserve failure (eefc9a94)tryPgserveInstall→requirePgserveInstall: void on success, exits 1 with a copy-paste hint on any failure (binary missing, install non-zero, port discovery failure).bun add -g pgserve@^2/pgserve install/genie install) and links docs/install.md.G2 —
getOrStartDaemon→requirePgserveDaemon(probe-only Mode A) (bd8845eb)probePgserveDaemon+isPgserveSocketResponsive. On unreachable: throwsbuildPgserveUnavailableHint(pm2 status/pm2 restart pgserve/pgserve install).@deprecated; CHANGELOG documents the rename).G3 — Delete spawn helpers + replace
serve.ts startPgserve(bd8845eb)src/lib/db.ts:startPgserveDaemonOnce,evictOrphanDataDirHolder,detectOrphanDataDirLock,terminatePgserveTree,signalPgserveTree,signalPgserveDaemonPid,recoverUnresponsivePgserveDaemon,isLikelyPgserveDaemonProcess,cleanPartialDaemonState,removeStalePgserveSocketArtifacts,unlinkIfPresent,waitForDaemonSocket,formatPgserveDaemonCommand,spawnPgserveDirect,startPgserveOnPort,findPgserveBin,findPgserveDaemonCommand,findLocalPgserveRoot,resolvePgservePackageCommand,findBunRuntime,waitForDaemonPort,throwDaemonTimeout,pgserveChild,maskCredentials,isPgAutostartDisabled, thePgserveDaemonCommandinterface, the localsleep, and thelastAutoStartOutcome/lastAutoStartPidtracking.src/term-commands/serve.ts:startPgserve→requirePgserveReady(probe-only). On success:pgserve daemon ready (canonical, pm2-supervised) on <socket>. On failure: pm2-recovery hint + setsGENIE_PG_NO_AUTOSTART=1._ensurePgservesimplified to fail with the canonical hint when no existing TCP port is reachable.registerExitHandleris now invoked from_buildConnectionpost-connect so the postgres pool drain still runs on every clean exit.G4 — Delete
selfHealPostgres+ remove doctor pkill (bd8845eb)selfHealPostgresdeleted (its only callers were the deleted spawn helpers).src/genie-commands/doctor.ts:killStalePostgres→printPgserveRecoveryHint(hint-only; never shells out).doctor --fixno longer prints "Killing stale postgres processes" or "Could not kill".G5 — Regression coverage (0ae69eb3 + 3f54ed87)
requirePgserveDaemon never spawns when daemon is healthy (cutover G5 regression)— source-text invariant: function body never references anychild_processspawn primitive norprocess.kill.G6 — Docs
README.md: new "Manual install (canonical pgserve first)" subsection with the three-step canonical pattern.CHANGELOG.md: Unreleased entry with breaking-change rationale + a copy-paste migration block (includingpgserve install --data ~/.genie/data/pgservefor keeping pre-canonical data dirs).Side fix uncovered by Wave 3 validation
Moving
registerExitHandler()from the (deleted) spawn-side call site into_buildConnectionaccidentally extended its SIGINT/SIGTERM scope to every connection-opening process. The synchronousprocess.exit(130/143)in those handlers raced the scheduler-daemon's own async signal handlers — surfaced by theserve lifecycle — bridge failure + shutdowntest (itsdaemon_stoppedexpectation fires AFTER scheduler-daemon's awaited shutdown flushes to scheduler.log; the synchronous exit short-circuited that flush). Fix: drop the SIGINT/SIGTERM branches;beforeExit/exitstill drain the pool on every clean exit, and signal-driven shutdown belongs to the process owner.Test plan
bun run typecheck— clean.bun test src/lib/db.test.ts— 56/56 pass (incl. new G5 regression + lockout tests).bun test src/term-commands/serve.test.ts src/genie-commands/doctor.test.ts src/genie-commands/__tests__/install.test.ts— all pass (pgserve failure containment3/3,pgserve v1/v2 coexistence1/1).bun test— 4107 pass / 616 skip / 3 fail (the 3 are pre-existingotel-receiverport-collision flakes that pass in isolation and reproduce on origin/dev).bun run check:fast— clean (typecheck + biome + knip + skills/wishes/emit-discipline lints).PATH=<bun+pm2 only without pgserve> bun src/genie.ts installexits 1 with the canonical install hint.grep -rn 'selfHealPostgres|startPgserveDaemonOnce|spawnPgserveDirect|terminatePgserveTree' src/returns only test-side lockout assertions indb.test.ts.grep -rn 'spawn.*pgserve' src/returns onlyspawnSync('pgserve', ['install'|'port'])(CLI shellouts, not daemon spawns).docker run --rm -it ubuntu:24.04 bash -c '…pgserve install && genie install && genie doctor'.pm2 stop pgserve && genie serve startshould print the pm2-recovery hint within 2s;pm2 start pgserve && genie serve startshould print "pgserve daemon ready (canonical, pm2-supervised)".Migration for pre-canonical operators
🤖 Generated with Claude Code