fix(hooks): skip db boot for dispatch — Mac CPU fix C - #1476
Conversation
Every `genie hook dispatch` bun fork was running runMigrations() + needsSeed()+runSeed() on first PG connect. needsSeed() loops all 92 ~/.claude/teams entries on every call. With hundreds of hook forks per minute on a busy Mac dev machine, this was the second-largest contributor to the .18 100%-CPU regression (after fix A — the retention DELETEs). Fix: - dispatch-command.ts:dispatchAction() sets GENIE_SKIP_DB_BOOT=1 BEFORE calling dispatch(stdin), so the first getConnection() inside any handler skips migrations + seed. - db.ts:runPostConnectSetup() honors GENIE_SKIP_DB_BOOT alongside isTestMode via a unified `skipBoot` guard. Correctness: - The long-lived `genie serve` daemon owns migrations + seed at startup; short-lived hook forks must not re-run them. Schema is already there. - Handlers that need DB still get a working connection — they just skip the boot-time setup the daemon already handled. - Daemon and CLI code paths never enter dispatchAction(), so the env flag scope is correctly hook-only. Validation: 45/45 src/lib/db.test.ts pass (43 prior + 2 new for the new contract). tsc --noEmit clean. Fix C of the 5-step .19 Mac-CPU root-cause plan (A→E): - A: ✓ #1475 — drop runRetention from getConnection - B: pending — cache needsSeed via teams-mtime marker (separately) - C: this PR - D: pending — narrow hook matchers (PostToolUse=SendMessage only) - E: pending — persist session-sync cache file Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
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 a performance optimization for Mac systems by introducing the GENIE_SKIP_DB_BOOT environment variable, which allows short-lived hook dispatch forks to skip database migrations and seeding. The changes include updating the dispatch entrypoint to set this variable and modifying the database setup logic to respect it. A review comment suggests refining a test case by tightening a regular expression to more precisely verify the environment variable check.
| // The skipBoot guard must combine isTestMode + the env var so the | ||
| // hook-dispatch entrypoint can short-circuit migrations + seed | ||
| expect(source).toContain("process.env.GENIE_SKIP_DB_BOOT === '1'"); | ||
| expect(source).toMatch(/skipBoot\s*=\s*isTestMode\s*\|\|\s*process\.env\.GENIE_SKIP_DB_BOOT/); |
There was a problem hiding this comment.
The regular expression used here is too broad. It matches the assignment prefix but does not verify the actual condition value (=== '1'). A change that incorrectly checks for a different value (e.g., === '0') would still pass this test. Consider tightening the regex to include the full comparison logic.
| expect(source).toMatch(/skipBoot\s*=\s*isTestMode\s*\|\|\s*process\.env\.GENIE_SKIP_DB_BOOT/); | |
| expect(source).toMatch(/skipBoot\s*=\s*isTestMode\s*\|\|\s*process\.env\.GENIE_SKIP_DB_BOOT\s*===\s*'1'/); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c96f83234
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // | ||
| // Set unconditionally for the dispatch entrypoint; daemon and CLI | ||
| // code paths never enter this function. | ||
| process.env.GENIE_SKIP_DB_BOOT = '1'; |
There was a problem hiding this comment.
Prevent GENIE_SKIP_DB_BOOT from leaking to child CLI runs
Setting process.env.GENIE_SKIP_DB_BOOT = '1' here changes the entire hook process environment, and hook handlers like auto-spawn forward process.env into spawnSync('genie', ...) child commands. That means non-hook subcommands (for example genie spawn) can inherit this flag and skip runMigrations/runSeed in runPostConnectSetup, which can break those commands on fresh installs or immediately after schema changes before bootstrapping has completed.
Useful? React with 👍 / 👎.
Per-process Map<executorId, sessionId> in session-sync.ts caches NOTHING across `genie hook dispatch` bun forks (each fork starts with empty Map). Result: every cold-start hook fork did 3 DB round-trips (getAgentByName + getExecutor + audit) even when the (executorId, sessionId) pair was already-reconciled by a previous fork. Fix: - Disk-backed cache at ~/.genie/cache/session-sync.json (overridable via __GENIE_SESSION_SYNC_CACHE_FILE for tests) - loadDiskCache() runs once per fork BEFORE any DB call, replacing empty Map with previously-persisted (executorId, sessionId) pairs - persistDiskCache() writes after every cache mutation (atomic via write-temp + rename) - MAX_CACHE_ENTRIES=1000 with insertion-order trim — bounded growth - Concurrency: rename races can lose one fork's writes for OTHER executor entries (benign: at most occasional redundant DB syncs from later forks) - Corrupt cache file is tolerated — falls through to DB After this PR, hook forks have ZERO DB calls in the steady state for already-reconciled sessions (only on actual rotation or first capture). Validation: 18/18 src/hooks/__tests__/session-sync.test.ts pass (14 prior + 4 new: persist-after-reconcile, cold-start-loads-from-disk, cache-miss- falls-through, corrupt-file-tolerated). tsc --noEmit clean. biome clean. Fix E of the 5-step .19 Mac-CPU root-cause plan (A→E): - A: shipped #1475 — drop runRetention from getConnection - filewatch: shipped #1474 — chokidar replacement - C: shipped #1476 — GENIE_SKIP_DB_BOOT for hook dispatch - D: open #1479 — narrow inject matchers - E: this PR - B: in flight (twin) — needsSeed mtime cache (lower priority post-C) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Per-process Map<executorId, sessionId> in session-sync.ts caches NOTHING across `genie hook dispatch` bun forks (each fork starts with empty Map). Result: every cold-start hook fork did 3 DB round-trips (getAgentByName + getExecutor + audit) even when the (executorId, sessionId) pair was already-reconciled by a previous fork. Fix: - Disk-backed cache at ~/.genie/cache/session-sync.json (overridable via __GENIE_SESSION_SYNC_CACHE_FILE for tests) - loadDiskCache() runs once per fork BEFORE any DB call, replacing empty Map with previously-persisted (executorId, sessionId) pairs - persistDiskCache() writes after every cache mutation (atomic via write-temp + rename) - MAX_CACHE_ENTRIES=1000 with insertion-order trim — bounded growth - Concurrency: rename races can lose one fork's writes for OTHER executor entries (benign: at most occasional redundant DB syncs from later forks) - Corrupt cache file is tolerated — falls through to DB After this PR, hook forks have ZERO DB calls in the steady state for already-reconciled sessions (only on actual rotation or first capture). Validation: 18/18 src/hooks/__tests__/session-sync.test.ts pass (14 prior + 4 new: persist-after-reconcile, cold-start-loads-from-disk, cache-miss- falls-through, corrupt-file-tolerated). tsc --noEmit clean. biome clean. Fix E of the 5-step .19 Mac-CPU root-cause plan (A→E): - A: shipped #1475 — drop runRetention from getConnection - filewatch: shipped #1474 — chokidar replacement - C: shipped #1476 — GENIE_SKIP_DB_BOOT for hook dispatch - D: open #1479 — narrow inject matchers - E: this PR - B: in flight (twin) — needsSeed mtime cache (lower priority post-C) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(hooks): persist session-sync cache to disk — Mac CPU fix E Per-process Map<executorId, sessionId> in session-sync.ts caches NOTHING across `genie hook dispatch` bun forks (each fork starts with empty Map). Result: every cold-start hook fork did 3 DB round-trips (getAgentByName + getExecutor + audit) even when the (executorId, sessionId) pair was already-reconciled by a previous fork. Fix: - Disk-backed cache at ~/.genie/cache/session-sync.json (overridable via __GENIE_SESSION_SYNC_CACHE_FILE for tests) - loadDiskCache() runs once per fork BEFORE any DB call, replacing empty Map with previously-persisted (executorId, sessionId) pairs - persistDiskCache() writes after every cache mutation (atomic via write-temp + rename) - MAX_CACHE_ENTRIES=1000 with insertion-order trim — bounded growth - Concurrency: rename races can lose one fork's writes for OTHER executor entries (benign: at most occasional redundant DB syncs from later forks) - Corrupt cache file is tolerated — falls through to DB After this PR, hook forks have ZERO DB calls in the steady state for already-reconciled sessions (only on actual rotation or first capture). Validation: 18/18 src/hooks/__tests__/session-sync.test.ts pass (14 prior + 4 new: persist-after-reconcile, cold-start-loads-from-disk, cache-miss- falls-through, corrupt-file-tolerated). tsc --noEmit clean. biome clean. Fix E of the 5-step .19 Mac-CPU root-cause plan (A→E): - A: shipped #1475 — drop runRetention from getConnection - filewatch: shipped #1474 — chokidar replacement - C: shipped #1476 — GENIE_SKIP_DB_BOOT for hook dispatch - D: open #1479 — narrow inject matchers - E: this PR - B: in flight (twin) — needsSeed mtime cache (lower priority post-C) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks): skip disk cache I/O in test mode (post-rebase fix-up) After rebasing fix E onto current dev (post #1479/#1482/#1483/#1484 merges), the pre-existing session-sync (Gap 2) tests started failing because loadDiskCache() on first call would read the production ~/.genie/cache/session-sync.json — populated by earlier real-mode runs of the handler. Stale (executorId, sessionId) entries caused the in-memory cache to short-circuit BEFORE the test fixtures' mocked DB calls were made, so audit-event emissions never fired. Fix: loadDiskCache() and persistDiskCache() now skip ALL disk I/O when: - ANY _deps field is non-null (test installed mocks), OR - NODE_ENV=test or BUN_ENV=test UNLESS the test explicitly set __GENIE_SESSION_SYNC_CACHE_FILE via _setCacheFileForTest (the new fix-E tests opt in this way). Mirrors the existing test-mode skip in shouldSkipSync. Validation: 18/18 src/hooks/__tests__/session-sync.test.ts pass. Both the 14 pre-existing tests and the 4 new fix-E tests work — pre-existing tests get clean cache state; fix-E tests use their isolated tmp-dir cache file. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…ok entry (#1489) * docs(brainstorm): hookify-third-party-absorption design + wish (delivery #2) Crystallized brainstorm + scaffolded wish for delivery #2 of the genie hookify umbrella (delivery #1 shipped as PR #1485). Delivery #2 makes genie the only Claude Code hook entry: absorbs existing foreign hooks (Token Optimizer, ultratoken, future plugins) via a one-time settings.json rewrite + subprocess-passthrough handlers, and lets operators deploy custom hooks (e.g. rlmx run on every Bash) as plain TS code in .genie/hooks/. Three-tier scoping (per-team > per-repo > global), trust- allowlisted, with reload + test for inner-loop iteration. Council reviewed (sentinel + architect + ergonomist + operator) — all four push-backs folded into the design + wish: - Sentinel: trust allowlist required (filesystem presence is not consent), versioned absorb snapshots, two-mode env capture (probe + offline) with denylist, threat model documented (single-operator machine). - Architect: Handler interface gains version/source/manifest_path discriminated union; registry migrates const handlers to let registryRef ReadonlyArray Handler; loud shadowing instead of silent. - Ergonomist: defineHook() config-object scaffold; genie hook reload + test ship with this delivery; rename absorb to import; broken hooks loud in list. - Operator: versioned snapshots last 10; per-team archive lifecycle; 5 ms passthrough is a measured SLO with bench + alert template. Plan-review fix-loop 1 closed all 6 reviewer gaps (trust threat model, probe offline fallback, registry mutation contract, per-team archive lifecycle, Handler vNext strategy, broken-hook recovery). WISH.md plan-reviewed SHIP. 4 execution groups, 3 waves: - Wave 1: G1 foundation (registry + Handler v1 + loader + trust gate) - Wave 2 parallel: G2 operator inner loop + G3 foreign-hook absorption - Wave 3: G4 lifecycle wiring + telemetry/microbench + docs + delivery report Files: - .genie/brainstorms/hookify-third-party-absorption/DRAFT.md - .genie/brainstorms/hookify-third-party-absorption/DESIGN.md - .genie/wishes/hookify-third-party-absorption/WISH.md Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(brainstorm): patch hookify-third-party-absorption wish for Mac CPU follow-ups Three Mac CPU follow-up PRs landed on dev after delivery #1 merged (PR #1475 retention, #1476 GENIE_SKIP_DB_BOOT, #1481 narrowed matchers). Wish plan shape unchanged but two implementation realities now apply: 1. DISPATCHED_EVENTS renamed to DISPATCHED_EVENT_MATCHERS in src/hooks/types.ts and shrunk from 6 events to 2 (PreToolUse plus PostToolUse:SendMessage). Group 3's import logic must consult the new constant; foreign hooks on un-wired events (PreCompact, SessionStart, etc.) cannot be absorbed and must be left in place with explicit reporting in --dry-run. 2. Tools that invoke hook code outside genie serve set GENIE_SKIP_DB_BOOT=1 to mirror the bun-fallback path's behavior (genie hook test, scaffold validation). Added [left-in-place] reporting requirement to import --dry-run plus a new acceptance criterion covering the three-status output. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Fix C of the 5-step .19 Mac-CPU root-cause plan. Second-largest single CPU win after fix A.
Every
genie hook dispatchbun fork was runningrunMigrations()+needsSeed()+runSeed()on first PG connect.needsSeed()loops all 92~/.claude/teamsentries on every call. With hundreds of hook forks per minute on a busy Mac dev machine, this was the second-largest contributor to the .18 100%-CPU regression (after fix A which dropped retention DELETEs from the same path).Fix
src/hooks/dispatch-command.ts:dispatchAction()setsGENIE_SKIP_DB_BOOT=1before callingdispatch(stdin), so the firstgetConnection()inside any handler skips migrations + seedsrc/lib/db.ts:runPostConnectSetup()honorsGENIE_SKIP_DB_BOOTalongsideisTestModevia a unifiedskipBootguardCorrectness
genie servedaemon owns migrations + seed at startup — short-lived hook forks must not re-run them. The schema is already there.codex-inbox-deliver) still get a working connection — they just skip the boot-time setup the daemon already handled.dispatchAction(), so the env-flag scope is correctly hook-only (no risk of leaking to other genie binary subcommands).Validation
bun test src/lib/db.test.ts— 45/45 pass (43 prior + 2 new contract tests)bun x tsc --noEmitcleanWhere this fits in the .19 Mac-CPU sprint
needsSeedvia teams-mtime marker (orthogonal speedup; this PR makes it lower priority since hooks now skip seed entirely)PostToolUse=SendMessageonly, drop empty events) — eliminates wasted bun cold-starts