Repository navigation
fix(ls): check executor state for non-tmux (SDK/omni) agents - #1167
Conversation
genie ls was rendering SDK/omni-bridge/process agents as "offline" (or
a misleading "error (0/3 resumes, auto-resume: on)") while they were
actively running. buildWorkerStatusMap gated liveness entirely on
isPaneAlive(w.paneId), which requires paneId to match /^%\d+$/.
Non-tmux transports carry synthetic pane tokens ('sdk', 'inline', '')
that fail the regex, so they were silently dropped from statusMap and
their stale agents.state field was all that survived for display.
Gate on transport type: tmux panes still use isPaneAlive (preserving
legacy behavior exactly), while non-tmux transports consult
executors.state via the new getLiveExecutorState helper. That state is
also used directly for display, since agents.state is not kept in sync
for SDK/omni agents — the claude-sdk transport updates executors.state
on every message, which is the authoritative signal.
Scope: intentionally narrow. Parallel-pattern sites flagged by tracer
(agents.ts:1291, 1835, 1886; scheduler-daemon.ts:595,667;
team-auto-spawn.ts:122; auto-spawn.ts:84) are out of scope — separate
wish.
Evidence (genie ls --json | jq '.[] | select(.name=="trace")'):
BEFORE: status = "error (0/3 resumes, auto-resume: on)"
AFTER: status = "spawning"
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces getLiveExecutorState and isExecutorAlive to accurately track the liveness of agents using non-tmux transports, ensuring their status is correctly reported in genie ls. Review feedback suggests parallelizing the liveness checks in buildWorkerStatusMap to improve performance and adjusting getLiveExecutorState to return the authoritative state for terminal executors to prevent the display of stale information.
| for (const w of workers) { | ||
| const name = w.role || w.id; | ||
| const alive = await isPaneAlive(w.paneId); | ||
| const { alive, state } = await resolveWorkerLiveness(w); |
There was a problem hiding this comment.
The loop performs sequential asynchronous calls to resolveWorkerLiveness, which involves external process executions (tmux) and database queries. This can lead to significant performance degradation when listing a large number of agents. Parallelizing these checks using Promise.all would improve responsiveness.
| for (const w of workers) { | |
| const name = w.role || w.id; | |
| const alive = await isPaneAlive(w.paneId); | |
| const { alive, state } = await resolveWorkerLiveness(w); | |
| const results = await Promise.all(workers.map(async (w) => ({ w, ...(await resolveWorkerLiveness(w)) }))); | |
| for (const { w, alive, state } of results) { | |
| const name = w.role || w.id; |
| export async function getLiveExecutorState(agentId: string): Promise<ExecutorState | null> { | ||
| const sql = await getConnection(); | ||
| const rows = await sql<{ state: ExecutorState }[]>` | ||
| SELECT e.state FROM executors e | ||
| JOIN agents a ON a.current_executor_id = e.id | ||
| WHERE a.id = ${agentId} | ||
| AND e.state IN ('spawning', 'running', 'working', 'idle', 'permission', 'question') | ||
| LIMIT 1 | ||
| `; | ||
| return rows.length > 0 ? rows[0].state : null; | ||
| } |
There was a problem hiding this comment.
The getLiveExecutorState function returns null for terminal states (done, error, terminated). However, as noted in the function's documentation, the agents.state column is stale for non-tmux transports. By returning null for dead executors, you force the caller to fall back to this stale data, which can result in genie ls showing an incorrect status (e.g., offline instead of error) for crashed agents. Consider returning the authoritative executor state regardless of liveness and letting the caller determine how to handle terminal states.
![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: 99d8ea67ca
ℹ️ 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".
| if (/^%\d+$/.test(w.paneId)) { | ||
| return { alive: await isPaneAlive(w.paneId), state: w.state }; | ||
| } | ||
| const execState = await executorRegistry.getLiveExecutorState(w.id); | ||
| return { alive: execState !== null, state: execState ?? w.state }; |
There was a problem hiding this comment.
registry.list() includes identity-only agent rows (from findOrCreateAgent) that have empty paneId but a live current_executor_id; with this new branch, those rows are treated as non-tmux and marked alive from executors.state. For tmux-backed workers this creates a duplicate entry keyed by the same role, and because buildWorkerStatusMap overwrites by name without stable ordering, genie ls can nondeterministically show the identity row’s stale executor state (often running) instead of the runtime row’s agents.state (idle/working).
Useful? React with 👍 / 👎.
Site 1 findDeadResumable: safe — provider filter excludes SDK (documented).
Site 2 rejectDuplicateRole: dispatch via resolveWorkerLivenessByTransport so
live SDK/omni/inline duplicates are detected instead of being silently
cleaned. Session-recycle check gated on tmux paneId (synthetic paneIds
never collide with a recycled pane).
Site 3 resolveSpawnIdentity: widen isAliveFn signature to {id, paneId};
default routes through the shared helper. Prevents a live SDK canonical
from being wrongly re-canonicalized and clobbering its session UUID via
ON CONFLICT UPDATE. Updates test stubs in agents.test.ts and
tui-spawn-dx.integration.test.ts to match signature.
Complements #1167.
Site 4 checkWorkerAlive (scheduler-daemon.ts): documented safe — runs.worker_id only carries real PIDs or a UUID daemonId fallback from handleTrigger; never a tmux pane or synthetic id. SDK agents live in agents/executors, not runs. Site 5 runAgentRecoveryPass (scheduler-daemon.ts): documented safe — claudeSessionId filter excludes SDK agents (SDK lacks a Claude-CLI JSONL). Site 6 isAgentAlive (team-auto-spawn.ts): dispatch via shared helper so inbox-watcher correctly routes messages to live SDK recipients instead of treating them as dead and dropping/misrouting. Site 7 executeAutoSpawn (hooks/handlers/auto-spawn.ts): dispatch via shared helper so the PreToolUse SendMessage handler stops triggering duplicate spawns on every message to a live SDK recipient. Complements #1167.
fix(v0.2): transport-aware liveness at 5 callers of isPaneAlive (complements #1167)
Summary
genie lswas rendering SDK/omni-bridge/process agents asoffline(or the misleadingerror (0/3 resumes, auto-resume: on)) while they were actively running.buildWorkerStatusMap(src/term-commands/agents.ts:2138) gated liveness entirely onisPaneAlive(w.paneId), which requires a%Ntmux pane ID. Non-tmux transports carry synthetic tokens ('sdk','inline','') that fail the regex, so they were silently dropped from the status map.isPaneAlive(legacy behavior preserved exactly). Non-tmux agents consult the newgetLiveExecutorState(agentId)helper —executors.stateis the authoritative signal (claude-sdk updates it on every message) and is also used for display sinceagents.stateisn't kept in sync for these transports.Changes
src/lib/executor-registry.ts— newgetLiveExecutorState(agentId): Promise<ExecutorState | null>+ boolean wrapperisExecutorAlive.src/term-commands/agents.ts—buildWorkerStatusMapdelegates to a newresolveWorkerLivenesshelper that picks the right liveness source.src/lib/executor-registry.test.ts— 14 new tests covering both functions across all executor states, missing-executor, and terminal transitions.Scope
Intentionally narrow. The tracer flagged parallel
isPaneAlive(w.paneId)sites inagents.ts:1291/1835/1886,scheduler-daemon.ts:595/667,team-auto-spawn.ts:122,auto-spawn.ts:84— those are out of scope and belong in a follow-up wish. This PR is a surgical UX visibility fix, not a lifecycle refactor.Evidence
Live against a running daemon, same agent (
trace) in both runs:Status distribution shift (same daemon, same instant):
offlineerror (0/3 resumes, auto-resume: on)idlespawningTmux agents (
felipe-3,fix-genie-ls-status) unchanged — stillidle— confirming the legacy path is preserved.Test plan
bun test src/lib/executor-registry.test.ts→ 79 pass, 0 fail (14 new)bun run check→ EXIT=0, 2495/0 across 128 filesbunx biome checkon changed files → no new warnings (one pre-existingprocessMessagecomplexity warning at agents.ts:1073 unrelated)🤖 Generated with Claude Code