fix(serve): make Omni bridge optional so serve survives NATS-less dev - #1239
Merged
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 294 files, which is 144 over the limit of 150. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (294)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements a comprehensive stabilization plan for the Changes
Sequence DiagramssequenceDiagram
participant Client as Client Process
participant FileSystem as File System
participant ProcessKernel as Process Kernel
participant Database as Database<br/>(pgserve)
rect rgba(76, 175, 80, 0.5)
Note over Client,Database: Identity-Aware Daemon Auto-Start (New)
Client->>FileSystem: read serve.pid
FileSystem-->>Client: {pid:startTime} or legacy {pid}
alt PID file missing
Client->>Client: spawn new daemon
Client->>ProcessKernel: getProcessStartTime(pid)
ProcessKernel-->>Client: startTime token
Client->>FileSystem: write {pid}:{startTime}
else PID exists, validate identity
Client->>ProcessKernel: getProcessStartTime(stored.pid)
ProcessKernel-->>Client: current startTime
alt startTime matches
Client->>Database: connect (reuse existing daemon)
else startTime mismatch (recycled PID)
Client->>FileSystem: unlink serve.pid
Client->>Client: spawn new daemon
end
end
end
sequenceDiagram
participant Scheduler as Scheduler
participant Registry as Agent Registry
participant FileSystem as File System
participant Tmux as Tmux
participant Database as Database
rect rgba(33, 150, 243, 0.5)
Note over Scheduler,Database: Zombie Agent Detection & Dead-Socket Fast Path (New)
Scheduler->>Registry: reconcileStaleSpawns()
Registry->>Database: query agents (idle/working/permission/question)
Database-->>Registry: agent list with pane_ids
loop Per tmux socket in candidates
Registry->>FileSystem: check /tmp/tmux-{uid}/{socketName}
FileSystem-->>Registry: socket exists?
alt Socket dead (missing)
Registry->>Database: bulk UPDATE agents on dead socket
Database-->>Registry: state='error', pane_id=''
else Socket alive
loop Per agent on live socket
Registry->>Tmux: isPaneAlive(pane_id)
Tmux-->>Registry: pane state
end
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e28b7f005
ℹ️ 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".
Comment on lines
+634
to
+638
| if (process.env.GENIE_OMNI_REQUIRED === '1') { | ||
| console.error(` Omni bridge: FAILED — ${msg}`); | ||
| process.exit(1); | ||
| } | ||
| console.warn(` Omni bridge: degraded — ${msg}; set GENIE_OMNI_REQUIRED=1 to make this fatal`); |
There was a problem hiding this comment.
In the non-strict fallback, we log degraded mode and continue when bridge.start() throws, but we do not run bridge.stop(). OmniBridge.start() can fail after allocating resources (for example after connecting to NATS but before full initialization, such as PG setup/schema failures), so this leaves a partially initialized bridge alive for the rest of the serve process; because handles.omniBridge is never set, normal shutdown will not clean it up. Add a best-effort await bridge.stop().catch(...) in this catch path before continuing.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/term-commands/serve.ts (1)
721-754:⚠️ Potential issue | 🟡 MinorMulti-signal race:
forceTimercan be scheduled twice.
gracefulExitguards re-entry onshutdownStarted, but that flag is only flipped insideshutdown()(Line 650), aftergracefulExithas already scheduled itsforceTimerand entered theawaitinshutdown(). A second signal arriving beforeshutdown()'s first synchronous statements runs will seeshutdownStarted === false, schedule a secondforceTimer, and re-entershutdown()— where the real guard then short-circuits. You end up with two timers, and the first one to fire wins the exit code (1instead of130/143).🔧 Local guard
+ let gracefulExitStarted = false; const gracefulExit = (exitCode: number): void => { - // Guard against multiple concurrent signals - if (shutdownStarted) return; + if (gracefulExitStarted) return; + gracefulExitStarted = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/term-commands/serve.ts` around lines 721 - 754, The gracefulExit function can schedule multiple forceTimer instances because shutdownStarted is only set inside shutdown(); set the shutdown guard immediately at the start of gracefulExit to prevent re-entry before scheduling timers—e.g., set shutdownStarted = true as the first statement in gracefulExit (before creating forceTimer) so subsequent signals return early; keep the rest of logic (forceTimer setup, shutdown().catch().finally() that clears the timer, removeServePid and process.exit) unchanged so the single timer is the only one that can fire.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/agent-registry.test.ts`:
- Around line 430-475: The test overwrites process.env.GENIE_TMUX_SOCKET and
blindly deletes it in finally, which clobbers any preexisting value; update the
dead-socket-worker-1/2 test (and the other live-socket test around the
reconcileStaleSpawns tests) to snapshot const prev =
process.env.GENIE_TMUX_SOCKET before assigning a test socket, and in the finally
restore it with process.env.GENIE_TMUX_SOCKET = prev (or delete if prev is
undefined) so existing env is preserved; touch the tests referencing
reconcileStaleSpawns and the GENIE_TMUX_SOCKET manipulation to follow this
pattern.
In `@src/lib/agent-registry.ts`:
- Around line 317-340: The socket bucketing via socketBuckets is redundant
because resolveWorkerSocketName() returns a single process-wide value; instead,
call resolveWorkerSocketName() once outside the loops and branch: if
isTmuxSocketAlive(socket) is true then directly assign liveSpawning =
staleWithPane and liveActive = activeDeadCandidates (or push all rows into those
arrays), else iterate staleWithPane and activeDeadCandidates to handle the dead
case; update uses of socketBuckets, spawning/active variables and remove the Map
scaffolding (or if you prefer to keep it for future per-row sockets, add a clear
TODO above socketBuckets explaining it is intentional scaffolding referencing
resolveWorkerSocketName, socketBuckets, staleWithPane, activeDeadCandidates,
liveSpawning, liveActive, and isTmuxSocketAlive).
- Around line 335-381: The dead-socket branch (inside the socketBuckets loop
using isTmuxSocketAlive) currently only records an aggregate
recordAuditEvent('worker', socketName, 'recovery_socket_dead', ...) and skips
emitting per-worker state_changed audit rows like the live-socket branch does;
update the code so that inside the loops iterating bucket.spawning and
bucket.active (where you already update agent rows and push resetIds) you also
call recordAuditEvent with the per-worker event (use entity_type = 'worker' and
entity_id = row.id, event type 'state_changed' and include previous/new state
metadata) for every agent actually transitioned, and change the aggregate event
to use an appropriate entity_type such as 'tmux_socket' (keep entity_id =
socketName) so the aggregate recovery_socket_dead event no longer mislabels
workers.
In `@src/lib/db.test.ts`:
- Around line 572-578: The test currently only scans db.ts source text; instead
call the function under test (autoStartDaemon) and exercise each timeout branch
by stubbing the environment/IO checks: simulate "no PID" by making
readFile/exists checks behave as if no ~/.genie/serve.pid is present and assert
the thrown message contains "genie serve not running. Run: genie serve start";
simulate "stale PID" by returning a PID file whose process check fails and
assert the thrown message contains "Stale ~/.genie/serve.pid"; simulate
"unhealthy pgserve" by stubbing the health/connect check (e.g., the method that
probes pgserve port) to return failure and assert the thrown message contains
"pgserve did not respond on port". Use the same test file (db.test.ts),
sinon/jest mocks or fixtures to replace filesystem and network checks around
autoStartDaemon, and assert on the thrown errors rather than searching source
text.
- Around line 472-480: The afterEach cleanup in the test sets
process.env.GENIE_HOME = undefined which assigns the string "undefined" and
pollutes other tests; update the afterEach block (the cleanup around
__setSpawnDaemonForTest and origGenieHome) to remove the environment variable
instead: if origGenieHome is defined restore it, otherwise use delete
process.env.GENIE_HOME so GENIE_HOME is actually removed from the environment.
In `@src/lib/db.ts`:
- Around line 345-364: Between detecting a stale identity and calling
unlinkSync(pidPath), add a re-check: read the pid file again
(fs.readFileSync(pidPath) parse to pid and startTime) and compare both values to
the pid and recordedStartTime you just used to classify it; only call
unlinkSync(pidPath) if they still match. If the re-read shows a different live
identity, treat that as "alive" (set lastAutoStartOutcome='alive',
lastAutoStartPid to the re-read pid and return) instead of removing the file; if
the re-read fails because the file is already gone, continue treating it as
stale and proceed to unlink/spawn as before. Use the same parsing logic as
earlier (the code around getProcessStartTime, pid, recordedStartTime and
spawnDaemon) and wrap filesystem ops in try/catch to preserve existing
error-handling behavior.
- Around line 261-275: Duplicate default spawn logic exists inside
__setSpawnDaemonForTest; extract the original implementation into a single
reusable constant (e.g., DEFAULT_SPAWN_DAEMON) defined next to where spawnDaemon
is declared, then have __setSpawnDaemonForTest set spawnDaemon = fn ??
DEFAULT_SPAWN_DAEMON so the default body is only maintained in one place; update
any references to use DEFAULT_SPAWN_DAEMON and ensure behavior (detached, stdio,
env, child.unref()) is preserved.
- Around line 442-459: The pidLabel fallback currently uses outcomeAtStart which
can be a state word like 'alive' and produce confusing messages; update the
logic that computes pidLabel (where pidLabel = pidAtStart ?? outcomeAtStart ??
'unknown') to prefer an actual PID source instead of outcomeAtStart — e.g., use
lastAutoStartPid or another stored PID (pidAtStart ?? lastAutoStartPid ??
'unknown') and keep the existing early-return checks around outcomeAtStart
('stale'/'missing') intact so the final thrown Error for the nonresponsive
pgserve always shows a numeric PID or 'unknown' rather than the outcome string.
In `@src/lib/tmux.ts`:
- Around line 556-559: isTmuxSocketAlive currently hard-codes `/tmp/tmux-<uid>`
so sockets placed under a custom TMUX_TMPDIR are treated as dead; update
isTmuxSocketAlive to compute the socket directory from process.env.TMUX_TMPDIR
(falling back to '/tmp') and join that with `tmux-<uid>` and the socketName
(e.g., join(tmuxTmpDir, `tmux-${uid}`, socketName)) so reconcileStaleSpawns no
longer mis-classifies live sockets; also update the JSDoc for isTmuxSocketAlive
to mention that the socket directory is determined by the TMUX_TMPDIR
environment variable.
In `@src/term-commands/serve.test.ts`:
- Around line 112-134: The spawned serve process is inheriting the test runner
cwd so it exits with "No workspace found"; update the spawn call
(spawn(BUN_PATH, [GENIE_ENTRY, 'serve', ...], { env: mergedEnv, stdio: [...] }))
to include cwd: testDir (or ensure a workspace is created and chdir to testDir
before spawning) so the child runs in the temporary workspace where GENIE_HOME
points; keep mergedEnv and stdio intact.
In `@src/term-commands/serve.ts`:
- Around line 760-771: The uncaughtException handler can call gracefulExit(1)
which may return immediately if shutdownStarted is already true, leaving the
process running; update the uncaughtException handler to call gracefulExit(1)
and if shutdownStarted is true (or if gracefulExit returns without scheduling an
exit) forcefully call process.exit(1) so the process terminates; reference the
gracefulExit function, the shutdownStarted flag, and the uncaughtException
handler and ensure removeServePid semantics remain unchanged.
---
Outside diff comments:
In `@src/term-commands/serve.ts`:
- Around line 721-754: The gracefulExit function can schedule multiple
forceTimer instances because shutdownStarted is only set inside shutdown(); set
the shutdown guard immediately at the start of gracefulExit to prevent re-entry
before scheduling timers—e.g., set shutdownStarted = true as the first statement
in gracefulExit (before creating forceTimer) so subsequent signals return early;
keep the rest of logic (forceTimer setup, shutdown().catch().finally() that
clears the timer, removeServePid and process.exit) unchanged so the single timer
is the only one that can fire.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ca1cb3f-b059-43fe-91c1-af8798a3f84e
📒 Files selected for processing (12)
.genie/wishes/genie-serve-stability/WISH.mdskills/genie-hacks/SKILL.mdskills/pm/SKILL.mdsrc/__tests__/migrate.test.tssrc/lib/agent-registry.test.tssrc/lib/agent-registry.tssrc/lib/db.test.tssrc/lib/db.tssrc/lib/process-identity.tssrc/lib/tmux.tssrc/term-commands/serve.test.tssrc/term-commands/serve.ts
Comment on lines
+430
to
+475
| test('flips workers registered on a dead socket to error (Bug 4)', async () => { | ||
| // Bug 4 repro: workers recorded on a tmux socket that no longer exists | ||
| // were stuck in 'idle'/'working' forever because reconcileStaleSpawns | ||
| // catches the TmuxUnreachableError from isPaneAlive and skips the worker. | ||
| // After the fix, a dead socket is detected once up-front and every | ||
| // worker on it is transitioned to 'error' with pane_id cleared. | ||
| // | ||
| // We pick a socket name that we can guarantee does NOT exist under | ||
| // /tmp/tmux-<uid>/ — any random UUID works. | ||
| const deadSocketName = `genie-dead-${Date.now()}-${Math.floor(Math.random() * 1e9)}`; | ||
| process.env.GENIE_TMUX_SOCKET = deadSocketName; | ||
| try { | ||
| const oldChange = new Date(Date.now() - 5_000).toISOString(); | ||
| await register( | ||
| makeAgent({ | ||
| id: 'dead-socket-worker-1', | ||
| paneId: '%9999', | ||
| state: 'idle', | ||
| startedAt: oldChange, | ||
| lastStateChange: oldChange, | ||
| }), | ||
| ); | ||
| await register( | ||
| makeAgent({ | ||
| id: 'dead-socket-worker-2', | ||
| paneId: '%9998', | ||
| state: 'working', | ||
| startedAt: oldChange, | ||
| lastStateChange: oldChange, | ||
| }), | ||
| ); | ||
|
|
||
| const reset = await reconcileStaleSpawns(2); | ||
| expect(reset).toContain('dead-socket-worker-1'); | ||
| expect(reset).toContain('dead-socket-worker-2'); | ||
|
|
||
| const a1 = await get('dead-socket-worker-1'); | ||
| expect(a1!.state).toBe('error'); | ||
| expect(a1!.paneId).toBe(''); | ||
| const a2 = await get('dead-socket-worker-2'); | ||
| expect(a2!.state).toBe('error'); | ||
| expect(a2!.paneId).toBe(''); | ||
| } finally { | ||
| // biome-ignore lint/performance/noDelete: assigning undefined would set the string "undefined" | ||
| delete process.env.GENIE_TMUX_SOCKET; | ||
| } |
There was a problem hiding this comment.
Restore the original GENIE_TMUX_SOCKET.
These tests overwrite a process-global env var and then always delete it. If the runner already had GENIE_TMUX_SOCKET set, later tests lose that value. Snapshot and restore it in both tests.
Proposed cleanup pattern
const deadSocketName = `genie-dead-${Date.now()}-${Math.floor(Math.random() * 1e9)}`;
+ const originalSocketName = process.env.GENIE_TMUX_SOCKET;
process.env.GENIE_TMUX_SOCKET = deadSocketName;
try {
const oldChange = new Date(Date.now() - 5_000).toISOString();
@@
} finally {
- // biome-ignore lint/performance/noDelete: assigning undefined would set the string "undefined"
- delete process.env.GENIE_TMUX_SOCKET;
+ if (originalSocketName === undefined) {
+ // biome-ignore lint/performance/noDelete: assigning undefined would set the string "undefined"
+ delete process.env.GENIE_TMUX_SOCKET;
+ } else {
+ process.env.GENIE_TMUX_SOCKET = originalSocketName;
+ }
}Apply the same restore pattern around the live-socket test.
Also applies to: 493-512
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/agent-registry.test.ts` around lines 430 - 475, The test overwrites
process.env.GENIE_TMUX_SOCKET and blindly deletes it in finally, which clobbers
any preexisting value; update the dead-socket-worker-1/2 test (and the other
live-socket test around the reconcileStaleSpawns tests) to snapshot const prev =
process.env.GENIE_TMUX_SOCKET before assigning a test socket, and in the finally
restore it with process.env.GENIE_TMUX_SOCKET = prev (or delete if prev is
undefined) so existing env is preserved; touch the tests referencing
reconcileStaleSpawns and the GENIE_TMUX_SOCKET manipulation to follow this
pattern.
Comment on lines
+335
to
+381
| for (const [socketName, bucket] of socketBuckets) { | ||
| if (isTmuxSocketAlive(socketName)) { | ||
| liveSpawning.push(...bucket.spawning); | ||
| liveActive.push(...bucket.active); | ||
| continue; | ||
| } | ||
| // Socket is dead — mark every candidate on it as error in one pass. | ||
| const allIds = [...bucket.spawning.map((r) => r.id), ...bucket.active.map((r) => r.id)]; | ||
| if (allIds.length === 0) continue; | ||
|
|
||
| // Per-row state-guarded UPDATE preserves the concurrent-transition | ||
| // race protection from the original code. | ||
| for (const row of bucket.spawning) { | ||
| const updated = await sql<{ id: string }[]>` | ||
| UPDATE agents | ||
| SET state = 'error', last_state_change = now(), pane_id = '' | ||
| WHERE id = ${row.id} AND state = 'spawning' | ||
| RETURNING id | ||
| `; | ||
| if (updated.length > 0) { | ||
| console.error( | ||
| `[reconcile] Reset agent ${row.id} (dead socket ${socketName}, pane ${row.pane_id}) from spawning → error`, | ||
| ); | ||
| resetIds.push(row.id); | ||
| } | ||
| } | ||
| for (const row of bucket.active) { | ||
| const prevState = row.state; | ||
| const updated = await sql<{ id: string }[]>` | ||
| UPDATE agents | ||
| SET state = 'error', last_state_change = now(), pane_id = '' | ||
| WHERE id = ${row.id} AND state = ${prevState} | ||
| RETURNING id | ||
| `; | ||
| if (updated.length > 0) { | ||
| console.error( | ||
| `[reconcile] Reset agent ${row.id} (dead socket ${socketName}, pane ${row.pane_id}) from ${prevState} → error`, | ||
| ); | ||
| resetIds.push(row.id); | ||
| } | ||
| } | ||
| recordAuditEvent('worker', socketName, 'recovery_socket_dead', 'reconciler', { | ||
| socket: socketName, | ||
| worker_ids: allIds, | ||
| worker_count: allIds.length, | ||
| }).catch(() => {}); | ||
| } |
There was a problem hiding this comment.
Dead-socket path skips per-worker state_changed audit events.
The live-socket branch at Lines 405–432 emits a per-worker state_changed audit event when a zombie is reset, matching the pattern used everywhere else in this file (see also Lines 274 and 394). The dead-socket branch records only one aggregate recovery_socket_dead event keyed by the socket name — so workers killed via the fast path are invisible to per-worker state-transition audit queries and dashboards that pivot on entity_id.
Also, using socketName as the entity_id under entity_type = 'worker' is a type/id mismatch; consider entity_type = 'tmux_socket' (or similar) for the aggregate event, and still emit per-worker state_changed rows inside the loops at Lines 347 and 361.
🔧 Suggested patch
for (const row of bucket.spawning) {
const updated = await sql<{ id: string }[]>`
UPDATE agents
SET state = 'error', last_state_change = now(), pane_id = ''
WHERE id = ${row.id} AND state = 'spawning'
RETURNING id
`;
if (updated.length > 0) {
console.error(
`[reconcile] Reset agent ${row.id} (dead socket ${socketName}, pane ${row.pane_id}) from spawning → error`,
);
+ recordAuditEvent('worker', row.id, 'state_changed', 'reconciler', {
+ state: 'error',
+ reason: 'socket_dead',
+ previous_state: 'spawning',
+ socket: socketName,
+ }).catch(() => {});
resetIds.push(row.id);
}
}
for (const row of bucket.active) {
const prevState = row.state;
const updated = await sql<{ id: string }[]>`
UPDATE agents
SET state = 'error', last_state_change = now(), pane_id = ''
WHERE id = ${row.id} AND state = ${prevState}
RETURNING id
`;
if (updated.length > 0) {
console.error(
`[reconcile] Reset agent ${row.id} (dead socket ${socketName}, pane ${row.pane_id}) from ${prevState} → error`,
);
+ recordAuditEvent('worker', row.id, 'state_changed', 'reconciler', {
+ state: 'error',
+ reason: 'socket_dead',
+ previous_state: prevState,
+ socket: socketName,
+ }).catch(() => {});
resetIds.push(row.id);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const [socketName, bucket] of socketBuckets) { | |
| if (isTmuxSocketAlive(socketName)) { | |
| liveSpawning.push(...bucket.spawning); | |
| liveActive.push(...bucket.active); | |
| continue; | |
| } | |
| // Socket is dead — mark every candidate on it as error in one pass. | |
| const allIds = [...bucket.spawning.map((r) => r.id), ...bucket.active.map((r) => r.id)]; | |
| if (allIds.length === 0) continue; | |
| // Per-row state-guarded UPDATE preserves the concurrent-transition | |
| // race protection from the original code. | |
| for (const row of bucket.spawning) { | |
| const updated = await sql<{ id: string }[]>` | |
| UPDATE agents | |
| SET state = 'error', last_state_change = now(), pane_id = '' | |
| WHERE id = ${row.id} AND state = 'spawning' | |
| RETURNING id | |
| `; | |
| if (updated.length > 0) { | |
| console.error( | |
| `[reconcile] Reset agent ${row.id} (dead socket ${socketName}, pane ${row.pane_id}) from spawning → error`, | |
| ); | |
| resetIds.push(row.id); | |
| } | |
| } | |
| for (const row of bucket.active) { | |
| const prevState = row.state; | |
| const updated = await sql<{ id: string }[]>` | |
| UPDATE agents | |
| SET state = 'error', last_state_change = now(), pane_id = '' | |
| WHERE id = ${row.id} AND state = ${prevState} | |
| RETURNING id | |
| `; | |
| if (updated.length > 0) { | |
| console.error( | |
| `[reconcile] Reset agent ${row.id} (dead socket ${socketName}, pane ${row.pane_id}) from ${prevState} → error`, | |
| ); | |
| resetIds.push(row.id); | |
| } | |
| } | |
| recordAuditEvent('worker', socketName, 'recovery_socket_dead', 'reconciler', { | |
| socket: socketName, | |
| worker_ids: allIds, | |
| worker_count: allIds.length, | |
| }).catch(() => {}); | |
| } | |
| for (const [socketName, bucket] of socketBuckets) { | |
| if (isTmuxSocketAlive(socketName)) { | |
| liveSpawning.push(...bucket.spawning); | |
| liveActive.push(...bucket.active); | |
| continue; | |
| } | |
| // Socket is dead — mark every candidate on it as error in one pass. | |
| const allIds = [...bucket.spawning.map((r) => r.id), ...bucket.active.map((r) => r.id)]; | |
| if (allIds.length === 0) continue; | |
| // Per-row state-guarded UPDATE preserves the concurrent-transition | |
| // race protection from the original code. | |
| for (const row of bucket.spawning) { | |
| const updated = await sql<{ id: string }[]>` | |
| UPDATE agents | |
| SET state = 'error', last_state_change = now(), pane_id = '' | |
| WHERE id = ${row.id} AND state = 'spawning' | |
| RETURNING id | |
| `; | |
| if (updated.length > 0) { | |
| console.error( | |
| `[reconcile] Reset agent ${row.id} (dead socket ${socketName}, pane ${row.pane_id}) from spawning → error`, | |
| ); | |
| recordAuditEvent('worker', row.id, 'state_changed', 'reconciler', { | |
| state: 'error', | |
| reason: 'socket_dead', | |
| previous_state: 'spawning', | |
| socket: socketName, | |
| }).catch(() => {}); | |
| resetIds.push(row.id); | |
| } | |
| } | |
| for (const row of bucket.active) { | |
| const prevState = row.state; | |
| const updated = await sql<{ id: string }[]>` | |
| UPDATE agents | |
| SET state = 'error', last_state_change = now(), pane_id = '' | |
| WHERE id = ${row.id} AND state = ${prevState} | |
| RETURNING id | |
| `; | |
| if (updated.length > 0) { | |
| console.error( | |
| `[reconcile] Reset agent ${row.id} (dead socket ${socketName}, pane ${row.pane_id}) from ${prevState} → error`, | |
| ); | |
| recordAuditEvent('worker', row.id, 'state_changed', 'reconciler', { | |
| state: 'error', | |
| reason: 'socket_dead', | |
| previous_state: prevState, | |
| socket: socketName, | |
| }).catch(() => {}); | |
| resetIds.push(row.id); | |
| } | |
| } | |
| recordAuditEvent('worker', socketName, 'recovery_socket_dead', 'reconciler', { | |
| socket: socketName, | |
| worker_ids: allIds, | |
| worker_count: allIds.length, | |
| }).catch(() => {}); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/agent-registry.ts` around lines 335 - 381, The dead-socket branch
(inside the socketBuckets loop using isTmuxSocketAlive) currently only records
an aggregate recordAuditEvent('worker', socketName, 'recovery_socket_dead', ...)
and skips emitting per-worker state_changed audit rows like the live-socket
branch does; update the code so that inside the loops iterating bucket.spawning
and bucket.active (where you already update agent rows and push resetIds) you
also call recordAuditEvent with the per-worker event (use entity_type = 'worker'
and entity_id = row.id, event type 'state_changed' and include previous/new
state metadata) for every agent actually transitioned, and change the aggregate
event to use an appropriate entity_type such as 'tmux_socket' (keep entity_id =
socketName) so the aggregate recovery_socket_dead event no longer mislabels
workers.
Comment on lines
+472
to
+480
| afterEach(async () => { | ||
| // Always restore the real spawn fn so other tests aren't affected. | ||
| const { __setSpawnDaemonForTest } = await import('./db.js'); | ||
| __setSpawnDaemonForTest(null); | ||
| if (origGenieHome !== undefined) { | ||
| process.env.GENIE_HOME = origGenieHome; | ||
| } else { | ||
| process.env.GENIE_HOME = undefined; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifies Node/Bun-style env assignment behavior without touching repo files.
node -e 'process.env.GENIE_HOME = undefined; console.log(String(process.env.GENIE_HOME)); delete process.env.GENIE_HOME; console.log(process.env.GENIE_HOME === undefined)'Repository: automagik-dev/genie
Length of output: 77
🏁 Script executed:
sed -n '472,480p' src/lib/db.test.tsRepository: automagik-dev/genie
Length of output: 406
Delete GENIE_HOME instead of assigning undefined to prevent test isolation failure.
Assigning process.env.GENIE_HOME = undefined sets it to the string "undefined" rather than removing it. This pollutes the global test environment, causing subsequent tests to resolve paths under a bogus GENIE_HOME value.
Use delete process.env.GENIE_HOME instead:
Fix
if (origGenieHome !== undefined) {
process.env.GENIE_HOME = origGenieHome;
} else {
- process.env.GENIE_HOME = undefined;
+ // biome-ignore lint/performance/noDelete: assigning undefined would set the string "undefined"
+ delete process.env.GENIE_HOME;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/db.test.ts` around lines 472 - 480, The afterEach cleanup in the test
sets process.env.GENIE_HOME = undefined which assigns the string "undefined" and
pollutes other tests; update the afterEach block (the cleanup around
__setSpawnDaemonForTest and origGenieHome) to remove the environment variable
instead: if origGenieHome is defined restore it, otherwise use delete
process.env.GENIE_HOME so GENIE_HOME is actually removed from the environment.
Comment on lines
+442
to
+459
|
|
||
| // Branch the timeout error so the user sees the actual failure mode. | ||
| const home = process.env.GENIE_HOME ?? GENIE_HOME; | ||
| const pidPath = join(home, 'serve.pid'); | ||
| const hasPidFile = existsSync(pidPath); | ||
| const currentPort = readLockfile() ?? getPort(); | ||
| if (outcomeAtStart === 'stale') { | ||
| throw new Error( | ||
| `Stale ~/.genie/serve.pid (PID ${pidAtStart ?? 'unknown'} was not our serve). Removed and retried — if this persists, run: genie serve start`, | ||
| ); | ||
| } | ||
| if (!hasPidFile) { | ||
| throw new Error('genie serve not running. Run: genie serve start'); | ||
| } | ||
| const pidLabel = pidAtStart ?? outcomeAtStart ?? 'unknown'; | ||
| throw new Error( | ||
| `genie serve is running (PID ${pidLabel}) but pgserve did not respond on port ${currentPort} within 16s. Try: genie serve restart, or check ~/.genie/logs/scheduler.log`, | ||
| ); |
There was a problem hiding this comment.
pidLabel can print a state word as a PID.
On Line 456, pidLabel = pidAtStart ?? outcomeAtStart ?? 'unknown' falls back to the outcome string ('missing' | 'stale' | 'alive') when pidAtStart is null. The user would then see genie serve is running (PID alive) but pgserve did not respond…, which reads as nonsense. This branch is only reached when outcomeAtStart === 'alive' (stale/missing already returned earlier), and lastAutoStartPid is set on the alive path — but the fallback exists and will fire on any future refactor where pidAtStart is null.
🔧 Proposed fix
- const pidLabel = pidAtStart ?? outcomeAtStart ?? 'unknown';
+ const pidLabel = pidAtStart ?? 'unknown';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Branch the timeout error so the user sees the actual failure mode. | |
| const home = process.env.GENIE_HOME ?? GENIE_HOME; | |
| const pidPath = join(home, 'serve.pid'); | |
| const hasPidFile = existsSync(pidPath); | |
| const currentPort = readLockfile() ?? getPort(); | |
| if (outcomeAtStart === 'stale') { | |
| throw new Error( | |
| `Stale ~/.genie/serve.pid (PID ${pidAtStart ?? 'unknown'} was not our serve). Removed and retried — if this persists, run: genie serve start`, | |
| ); | |
| } | |
| if (!hasPidFile) { | |
| throw new Error('genie serve not running. Run: genie serve start'); | |
| } | |
| const pidLabel = pidAtStart ?? outcomeAtStart ?? 'unknown'; | |
| throw new Error( | |
| `genie serve is running (PID ${pidLabel}) but pgserve did not respond on port ${currentPort} within 16s. Try: genie serve restart, or check ~/.genie/logs/scheduler.log`, | |
| ); | |
| // Branch the timeout error so the user sees the actual failure mode. | |
| const home = process.env.GENIE_HOME ?? GENIE_HOME; | |
| const pidPath = join(home, 'serve.pid'); | |
| const hasPidFile = existsSync(pidPath); | |
| const currentPort = readLockfile() ?? getPort(); | |
| if (outcomeAtStart === 'stale') { | |
| throw new Error( | |
| `Stale ~/.genie/serve.pid (PID ${pidAtStart ?? 'unknown'} was not our serve). Removed and retried — if this persists, run: genie serve start`, | |
| ); | |
| } | |
| if (!hasPidFile) { | |
| throw new Error('genie serve not running. Run: genie serve start'); | |
| } | |
| const pidLabel = pidAtStart ?? 'unknown'; | |
| throw new Error( | |
| `genie serve is running (PID ${pidLabel}) but pgserve did not respond on port ${currentPort} within 16s. Try: genie serve restart, or check ~/.genie/logs/scheduler.log`, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/db.ts` around lines 442 - 459, The pidLabel fallback currently uses
outcomeAtStart which can be a state word like 'alive' and produce confusing
messages; update the logic that computes pidLabel (where pidLabel = pidAtStart
?? outcomeAtStart ?? 'unknown') to prefer an actual PID source instead of
outcomeAtStart — e.g., use lastAutoStartPid or another stored PID (pidAtStart ??
lastAutoStartPid ?? 'unknown') and keep the existing early-return checks around
outcomeAtStart ('stale'/'missing') intact so the final thrown Error for the
nonresponsive pgserve always shows a numeric PID or 'unknown' rather than the
outcome string.
Comment on lines
+556
to
+559
| export function isTmuxSocketAlive(socketName: string | undefined | null): boolean { | ||
| if (!socketName) return false; | ||
| const uid = process.getuid?.() ?? 501; | ||
| return existsSync(join(`/tmp/tmux-${uid}`, socketName)); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
tmux default socket path TMUX_TMPDIR tmux-UID -L socket
💡 Result:
The default tmux socket path is $TMUX_TMPDIR/tmux-$UID/default, where TMUX_TMPDIR defaults to /tmp if unset. Thus, it is typically /tmp/tmux-$UID/default (e.g., /tmp/tmux-1000/default for UID 1000). TMUX_TMPDIR specifies the base directory for the tmux-UID subdirectory containing sockets. Set it in the environment before starting tmux (requires server restart to take effect). The -L option specifies a socket name (default: "default") within that directory, enabling multiple servers like tmux -L mysocket. The -S option overrides with a full custom socket path, ignoring -L and the default directory. The tmux-UID directory is created by tmux and must not be world-readable/writable/executable for security. Inside tmux panes, $TMUX contains the full socket path (first part before comma). Examples: - Default: tmux new-session → /tmp/tmux-1000/default - Custom dir: TMUX_TMPDIR=/var/tmux tmux new-session → /var/tmux/tmux-1000/default - Multiple servers: tmux -L work new-session → $TMUX_TMPDIR/tmux-$UID/work - Full path: tmux -S /path/to/socket new-session
Citations:
- 1: https://github.com/tmux/tmux/wiki/Advanced-Use
- 2: https://linux.die.net/man/1/tmux
- 3: Change tmux default temporary location
/tmp/tmux-1000tmux/tmux#1646 - 4: https://man7.org/linux/man-pages/man1/tmux.1.html
- 5: https://github.com/tmux/tmux/blob/master/tmux.1
🏁 Script executed:
# Find all usages of isTmuxSocketAlive
rg "isTmuxSocketAlive" --type ts -B 2 -A 2Repository: automagik-dev/genie
Length of output: 1065
🏁 Script executed:
# Examine agent-registry.ts to understand the context
fd agent-registryRepository: automagik-dev/genie
Length of output: 119
🏁 Script executed:
# Check the full implementation of isTmuxSocketAlive and surrounding context
sed -n '550,565p' src/lib/tmux.tsRepository: automagik-dev/genie
Length of output: 708
🏁 Script executed:
# Look for socket bucket setup and where socketName originates
sed -n '270,310p' src/lib/agent-registry.tsRepository: automagik-dev/genie
Length of output: 2122
🏁 Script executed:
# Search for where socket names are collected/stored
rg "socketName" src/lib/agent-registry.ts -B 3 -A 3Repository: automagik-dev/genie
Length of output: 1068
🏁 Script executed:
# Check if socketName could ever be a full path (from -S or $TMUX)
rg "TMUX|socketName|\-S" src/lib/ --type ts -A 2 | head -80Repository: automagik-dev/genie
Length of output: 3985
🏁 Script executed:
# Verify the exact structure of agent-registry socket grouping to confirm socketName is just the name
sed -n '200,260p' src/lib/agent-registry.ts | grep -A 50 "socketBuckets"Repository: automagik-dev/genie
Length of output: 45
🏁 Script executed:
# Check if there's any extraction/parsing of socket names that might affect them
rg "pane_id|socket" src/lib/agent-registry.ts | grep -i "split\|substring\|extract" | head -20Repository: automagik-dev/genie
Length of output: 45
🏁 Script executed:
# Find socketBuckets construction more directly
rg "socketBuckets" src/lib/agent-registry.ts -B 5 -A 10Repository: automagik-dev/genie
Length of output: 1559
🏁 Script executed:
# Check where socket names come from in the agent records
rg "socket_name|pane_id" src/lib/agent-registry.ts -B 2 -A 2 | head -60Repository: automagik-dev/genie
Length of output: 3878
🏁 Script executed:
# Find the resolveWorkerSocketName function
rg "resolveWorkerSocketName" src/lib/agent-registry.ts -B 3 -A 8Repository: automagik-dev/genie
Length of output: 1308
🏁 Script executed:
# Also check the tmux.ts file for any other socket-related functions
sed -n '1,50p' src/lib/tmux.ts | head -40Repository: automagik-dev/genie
Length of output: 1082
🏁 Script executed:
# Verify the fix is complete - check if there are any other socket path constructions
rg "tmux-\${uid}|/tmp/tmux" src/lib/ --type tsRepository: automagik-dev/genie
Length of output: 173
🏁 Script executed:
# Check if TMUX_TMPDIR is used elsewhere in the codebase
rg "TMUX_TMPDIR" src/ --type tsRepository: automagik-dev/genie
Length of output: 45
🏁 Script executed:
# Check for other socket path constructions (fixed regex)
rg "/tmp/tmux|tmux-uid|TMUX_TMPDIR" src/lib/ --type tsRepository: automagik-dev/genie
Length of output: 1537
🏁 Script executed:
# Check if socketName could ever contain a full path with slashes
rg "socketName|socket" src/lib/agent-registry.ts | grep -E "split|substring|indexOf|slice"Repository: automagik-dev/genie
Length of output: 45
🏁 Script executed:
# Look at the comment in tmux.ts about socket paths
sed -n '545,560p' src/lib/tmux.tsRepository: automagik-dev/genie
Length of output: 813
🏁 Script executed:
# Check if there are any tests for isTmuxSocketAlive that might be affected
rg "isTmuxSocketAlive" src/lib/*.test.ts -B 3 -A 5Repository: automagik-dev/genie
Length of output: 45
🏁 Script executed:
# Look for any existing tests that set TMUX_TMPDIR
rg "TMUX_TMPDIR" . --type ts --type jsonRepository: automagik-dev/genie
Length of output: 45
🏁 Script executed:
# Check the test file for isTmuxSocketAlive context
grep -r "isTmuxSocketAlive\|tmux.*socket" src/lib/*.test.ts | head -20Repository: automagik-dev/genie
Length of output: 274
Fix socket path to respect TMUX_TMPDIR.
isTmuxSocketAlive() hard-codes /tmp/tmux-<uid> for socket lookup, but tmux respects $TMUX_TMPDIR (defaults to /tmp). When TMUX_TMPDIR is set to a custom directory, live sockets there are incorrectly classified as dead, causing reconcileStaleSpawns() to mark all workers on that socket as error.
The JSDoc comment also needs updating to document this configuration option.
Fix
export function isTmuxSocketAlive(socketName: string | undefined | null): boolean {
if (!socketName) return false;
const uid = process.getuid?.() ?? 501;
- return existsSync(join(`/tmp/tmux-${uid}`, socketName));
+ const tmuxTmpDir = process.env.TMUX_TMPDIR ?? '/tmp';
+ return existsSync(join(tmuxTmpDir, `tmux-${uid}`, socketName));
}Also update the JSDoc to clarify that the socket directory is determined by $TMUX_TMPDIR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/tmux.ts` around lines 556 - 559, isTmuxSocketAlive currently
hard-codes `/tmp/tmux-<uid>` so sockets placed under a custom TMUX_TMPDIR are
treated as dead; update isTmuxSocketAlive to compute the socket directory from
process.env.TMUX_TMPDIR (falling back to '/tmp') and join that with `tmux-<uid>`
and the socketName (e.g., join(tmuxTmpDir, `tmux-${uid}`, socketName)) so
reconcileStaleSpawns no longer mis-classifies live sockets; also update the
JSDoc for isTmuxSocketAlive to mention that the socket directory is determined
by the TMUX_TMPDIR environment variable.
Comment on lines
+112
to
+134
| const proc = spawn(BUN_PATH, [GENIE_ENTRY, 'serve', 'start', '--foreground', '--headless'], { | ||
| env: mergedEnv, | ||
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| }); | ||
| proc.stdout?.on('data', (chunk: Buffer) => { | ||
| stdout.buffer += chunk.toString('utf-8'); | ||
| }); | ||
| proc.stderr?.on('data', (chunk: Buffer) => { | ||
| stderr.buffer += chunk.toString('utf-8'); | ||
| }); | ||
|
|
||
| const exit = new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve) => { | ||
| proc.on('exit', (code, signal) => resolve({ code, signal })); | ||
| }); | ||
|
|
||
| return { child: proc, stdout, stderr, exit }; | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| testDir = join(tmpdir(), `genie-serve-test-${Date.now()}-${Math.random().toString(36).slice(2)}`); | ||
| mkdirSync(testDir, { recursive: true }); | ||
| genieHome = join(testDir, '.genie'); | ||
| mkdirSync(genieHome, { recursive: true }); |
There was a problem hiding this comment.
Spawn serve from the initialized temp workspace.
The child inherits the test runner cwd while GENIE_HOME points at testDir/.genie; CI shows it exits with No workspace found before the bridge/PID assertions run. Pass the temp workspace as cwd or initialize one before spawning.
Proposed fix
const proc = spawn(BUN_PATH, [GENIE_ENTRY, 'serve', 'start', '--foreground', '--headless'], {
+ cwd: testDir,
env: mergedEnv,
stdio: ['ignore', 'pipe', 'pipe'],
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const proc = spawn(BUN_PATH, [GENIE_ENTRY, 'serve', 'start', '--foreground', '--headless'], { | |
| env: mergedEnv, | |
| stdio: ['ignore', 'pipe', 'pipe'], | |
| }); | |
| proc.stdout?.on('data', (chunk: Buffer) => { | |
| stdout.buffer += chunk.toString('utf-8'); | |
| }); | |
| proc.stderr?.on('data', (chunk: Buffer) => { | |
| stderr.buffer += chunk.toString('utf-8'); | |
| }); | |
| const exit = new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve) => { | |
| proc.on('exit', (code, signal) => resolve({ code, signal })); | |
| }); | |
| return { child: proc, stdout, stderr, exit }; | |
| } | |
| beforeEach(() => { | |
| testDir = join(tmpdir(), `genie-serve-test-${Date.now()}-${Math.random().toString(36).slice(2)}`); | |
| mkdirSync(testDir, { recursive: true }); | |
| genieHome = join(testDir, '.genie'); | |
| mkdirSync(genieHome, { recursive: true }); | |
| const proc = spawn(BUN_PATH, [GENIE_ENTRY, 'serve', 'start', '--foreground', '--headless'], { | |
| cwd: testDir, | |
| env: mergedEnv, | |
| stdio: ['ignore', 'pipe', 'pipe'], | |
| }); | |
| proc.stdout?.on('data', (chunk: Buffer) => { | |
| stdout.buffer += chunk.toString('utf-8'); | |
| }); | |
| proc.stderr?.on('data', (chunk: Buffer) => { | |
| stderr.buffer += chunk.toString('utf-8'); | |
| }); | |
| const exit = new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve) => { | |
| proc.on('exit', (code, signal) => resolve({ code, signal })); | |
| }); | |
| return { child: proc, stdout, stderr, exit }; | |
| } | |
| beforeEach(() => { | |
| testDir = join(tmpdir(), `genie-serve-test-${Date.now()}-${Math.random().toString(36).slice(2)}`); | |
| mkdirSync(testDir, { recursive: true }); | |
| genieHome = join(testDir, '.genie'); | |
| mkdirSync(genieHome, { recursive: true }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/serve.test.ts` around lines 112 - 134, The spawned serve
process is inheriting the test runner cwd so it exits with "No workspace found";
update the spawn call (spawn(BUN_PATH, [GENIE_ENTRY, 'serve', ...], { env:
mergedEnv, stdio: [...] })) to include cwd: testDir (or ensure a workspace is
created and chdir to testDir before spawning) so the child runs in the temporary
workspace where GENIE_HOME points; keep mergedEnv and stdio intact.
Comment on lines
+760
to
771
| // `exit` handler can only run synchronous code — ensure the PID file is gone | ||
| // even if we reach process exit without going through gracefulExit (e.g. the | ||
| // scheduler's `done` promise resolved normally). | ||
| process.on('exit', () => { | ||
| removeServePid(); | ||
| }); | ||
| process.on('SIGINT', () => { | ||
| forceKillShutdown(); | ||
| process.exit(130); | ||
|
|
||
| // Last-resort handler: surface the error, attempt cleanup, then exit 1. | ||
| process.on('uncaughtException', (err) => { | ||
| console.error('Uncaught exception in genie serve:', err); | ||
| gracefulExit(1); | ||
| }); |
There was a problem hiding this comment.
uncaughtException → gracefulExit can silently become a no-op.
If the uncaught exception originates inside shutdown() (e.g. from await schedulerHandle.done rejecting in a way not caught by the inner try, or from omniBridge.stop() throwing synchronously before the .catch), shutdownStarted is already true, so gracefulExit(1) early-returns at Line 723 and no exit is scheduled. The process keeps running after a fatal error, and only the forceTimer from the original signal (if any) will eventually kill it — otherwise it hangs.
The exit handler will still removeServePid, but only once the process actually exits. Consider forcing an process.exit(1) directly in the uncaughtException path if shutdown was already in flight.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/serve.ts` around lines 760 - 771, The uncaughtException
handler can call gracefulExit(1) which may return immediately if shutdownStarted
is already true, leaving the process running; update the uncaughtException
handler to call gracefulExit(1) and if shutdownStarted is true (or if
gracefulExit returns without scheduling an exit) forcefully call process.exit(1)
so the process terminates; reference the gracefulExit function, the
shutdownStarted flag, and the uncaughtException handler and ensure
removeServePid semantics remain unchanged.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces several stability fixes for genie serve, including making the Omni bridge optional, implementing PID identity verification via process start times, and improving worker reconciliation by checking for tmux socket existence. It also adds comprehensive lifecycle tests and descriptive error messages for various failure modes. Review feedback focuses on optimizing database interactions by batching updates during reconciliation and ensuring consistent state management by clearing pane_id in all recovery paths.
Comment on lines
+347
to
+375
| for (const row of bucket.spawning) { | ||
| const updated = await sql<{ id: string }[]>` | ||
| UPDATE agents | ||
| SET state = 'error', last_state_change = now(), pane_id = '' | ||
| WHERE id = ${row.id} AND state = 'spawning' | ||
| RETURNING id | ||
| `; | ||
| if (updated.length > 0) { | ||
| console.error( | ||
| `[reconcile] Reset agent ${row.id} (dead socket ${socketName}, pane ${row.pane_id}) from spawning → error`, | ||
| ); | ||
| resetIds.push(row.id); | ||
| } | ||
| } | ||
| for (const row of bucket.active) { | ||
| const prevState = row.state; | ||
| const updated = await sql<{ id: string }[]>` | ||
| UPDATE agents | ||
| SET state = 'error', last_state_change = now(), pane_id = '' | ||
| WHERE id = ${row.id} AND state = ${prevState} | ||
| RETURNING id | ||
| `; | ||
| if (updated.length > 0) { | ||
| console.error( | ||
| `[reconcile] Reset agent ${row.id} (dead socket ${socketName}, pane ${row.pane_id}) from ${prevState} → error`, | ||
| ); | ||
| resetIds.push(row.id); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The sequential UPDATE queries inside these loops can be optimized by performing a single batch update per bucket using WHERE id IN ${sql(ids)}. This significantly reduces database round-trips, which is especially important when reconciling a large number of stale agents. Additionally, resolveWorkerSocketName() should be called once outside the loops to avoid redundant environment variable lookups and object creation on every iteration.
Comment on lines
+410
to
+415
| const updated = await sql<{ id: string }[]>` | ||
| UPDATE agents | ||
| SET state = 'error', last_state_change = now() | ||
| WHERE id = ${row.id} AND state = ${prevState} | ||
| RETURNING id | ||
| `; |
Contributor
There was a problem hiding this comment.
The pane_id should be cleared here as well to maintain consistency across all recovery paths. When a pane is confirmed dead, its ID is no longer valid and should be removed from the database record.
const updated = await sql<{ id: string }[]>`
UPDATE agents
SET state = 'error', last_state_change = now(), pane_id = ''
WHERE id = ${row.id} AND state = ${prevState}
RETURNING id
`;
`genie serve` exited with code 1 whenever the Omni bridge could not
reach NATS (default on any dev machine), which made every CLI command
hang for 16s and blocked testing. Make the bridge optional: on start
failure, log `Omni bridge: degraded — <reason>` and continue. Set
`GENIE_OMNI_REQUIRED=1` to preserve strict (exit-1) behavior.
Bundles related genie-serve-stability fixes: `{pid}:{startTime}`
identity for PID staleness checks (new process-identity module),
dead-socket reconciliation for stale workers, zombie dead-pane pass
for idle/working rows, and regression tests across serve / db /
agent-registry.
Wish: .genie/wishes/genie-serve-stability/WISH.md
On macOS `os.tmpdir()` returns `/var/folders/...` but `/var` is a symlink to `/private/var`. `planMigration()` resolves symlinks on its input paths, so tests that compared against the raw tmpdir string failed with `/private/var/...` vs `/var/...`. Realpath the test directory in `beforeEach` so expected and actual paths align on macOS and Linux alike.
namastex888
force-pushed
the
fix/genie-serve-stability
branch
from
April 20, 2026 20:41
5e28b7f to
d3ea283
Compare
2 of 3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
genie serveexited with code 1 whenever the Omni bridge could not reach NATS (the default on any dev machine), which made every CLI command hang for 16s and blocked testing. The bridge is now optional: on start failure, serve logsOmni bridge: degraded — <reason>and continues. SetGENIE_OMNI_REQUIRED=1to restore strict (exit-1) behavior.genie-serve-stabilityhardening:{pid}:{startTime}process identity for correct PID staleness checks (newsrc/lib/process-identity.ts), dead-socket reconciliation for stale workers, a zombie dead-pane pass for idle/working rows, and regression tests acrossserve,db, andagent-registry.bun run checkand the pre-push hook go green:genie resetrefs inskills/pm/SKILL.mdandskills/genie-hacks/SKILL.mdwithgenie task unblock(the current command).tmpdir()insrc/__tests__/migrate.test.tsso the macOS/var→/private/varsymlink doesn't break path assertions.Root cause of the original report: the fix existed only in the working tree — the globally installed
~/.bun/bin/geniewas a stale published build that still hadprocess.exit(1)on bridge failure. Landing and publishing this PR ensures users who rungenie servepick up the optional bridge.Wish:
.genie/wishes/genie-serve-stability/WISH.mdTest plan
bun run typecheck— cleanbun run lint— only pre-existing complexity warnings inagents.ts(not touched by this PR)bun run skills:lint— OK (0 missing)bun test— 2493/2493 pass (previously 4 pre-existing macOS failures inmigrate.test.ts— fixed here)genie servewith no NATS running — printsOmni bridge: degraded — CONNECTION_REFUSED, continues, PID file present, SIGTERM shuts down cleanlyGENIE_OMNI_REQUIRED=1 genie serve start --foregroundwith no NATS still exits 1 (strict-mode regression check)Summary by CodeRabbit
New Features
GENIE_OMNI_REQUIRED=1to make it mandatory.Bug Fixes
Documentation