Repository navigation
fix(omni-bridge): harden session lifecycle + liveness check + collision-resistant window names - #1051
Conversation
…cleanup, concurrency guard - Log warning + send buffer-full reply when MAX_BUFFER_PER_CHAT exceeded (was silent drop) - Shut down all active executor sessions in stop() before clearing (was resource leak) - Count spawning entries in concurrency limit to prevent oversubscription - Re-queue buffered messages on spawn failure instead of losing them Closes #1035
The function is only used within the module — no need to export it. Fixes dead-code check (knip) warning.
…roup 1 dependencies - Re-export sanitizeWindowName (needed by test file) - Add isPaneProcessRunning to tmux.ts (used by claude-code executor liveness check) - Include claude-code.test.ts for sanitizeWindowName collision tests
|
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 several improvements to the session management and process monitoring in the omni-bridge service. Key changes include the addition of a process-checking utility for tmux panes, an updated window name sanitization function with collision prevention, and enhanced session lifecycle management, including concurrent shutdown handling and buffer overflow protection. I have provided feedback regarding the use of synchronous process execution and suggested using Promise.all for more efficient session shutdowns.
| const { execSync } = await import('node:child_process'); | ||
| // Check direct children and grandchildren for the target process name | ||
| const output = execSync( | ||
| `pgrep -la -P ${panePid} 2>/dev/null; for cpid in $(pgrep -P ${panePid} 2>/dev/null); do pgrep -la -P "$cpid" 2>/dev/null; done; true`, | ||
| { encoding: 'utf-8', timeout: 5000 }, | ||
| ); | ||
| return output.toLowerCase().includes(processName.toLowerCase()); |
There was a problem hiding this comment.
Using execSync inside an asynchronous function blocks the Node.js event loop. This can significantly degrade the performance and responsiveness of the bridge, especially when managing multiple concurrent sessions or during the periodic liveness checks. It is highly recommended to use an asynchronous alternative like child_process.exec wrapped in a Promise. This implementation utilizes dynamic imports, which aligns with the repository guideline to use dynamic imports to prevent circular dependency cycles.
const { exec } = await import('node:child_process');
const { promisify } = await import('node:util');
const execAsync = promisify(exec);
// Check direct children and grandchildren for the target process name
const { stdout } = await execAsync(
'pgrep -la -P ' + panePid + ' 2>/dev/null; for cpid in $(pgrep -P ' + panePid + ' 2>/dev/null); do pgrep -la -P "$cpid" 2>/dev/null; done; true',
{ encoding: 'utf-8', timeout: 5000 },
);
return stdout.toLowerCase().includes(processName.toLowerCase());References
- Use dynamic imports (require() or import()) to break circular dependency cycles that would otherwise occur at module load time.
| for (const [key, entry] of this.sessions) { | ||
| if (entry.idleTimer) clearTimeout(entry.idleTimer); | ||
| if (!entry.spawning && entry.session) { | ||
| try { | ||
| await this.executor.shutdown(entry.session); | ||
| } catch (err) { | ||
| console.warn(`[omni-bridge] Error shutting down session ${key}:`, err); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Shutting down sessions sequentially using await inside a loop can be inefficient and slow down the bridge's shutdown process, especially with a high number of active sessions. Using Promise.all to perform these shutdowns concurrently would be much more efficient.
| for (const [key, entry] of this.sessions) { | |
| if (entry.idleTimer) clearTimeout(entry.idleTimer); | |
| if (!entry.spawning && entry.session) { | |
| try { | |
| await this.executor.shutdown(entry.session); | |
| } catch (err) { | |
| console.warn(`[omni-bridge] Error shutting down session ${key}:`, err); | |
| } | |
| } | |
| } | |
| const shutdownPromises: Promise<void>[] = []; | |
| for (const [key, entry] of this.sessions) { | |
| if (entry.idleTimer) clearTimeout(entry.idleTimer); | |
| if (!entry.spawning && entry.session) { | |
| shutdownPromises.push( | |
| this.executor.shutdown(entry.session).catch((err) => { | |
| console.warn('[omni-bridge] Error shutting down session ' + key + ':', err); | |
| }), | |
| ); | |
| } | |
| } | |
| await Promise.all(shutdownPromises); |
![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: 7414d1c608
ℹ️ 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".
| console.warn( | ||
| `[omni-bridge] Re-queuing ${lostMessages.length} buffered message(s) from failed spawn for ${key}`, | ||
| ); | ||
| this.messageQueue.push(...lostMessages); |
There was a problem hiding this comment.
When spawnSession fails, buffered messages are moved to messageQueue, but no retry path is triggered here. In this file, drainQueue() is only invoked from the idle-timeout callback in resetIdleTimer, so a failure during initial spawn (for example, transient tmux startup issues) can leave these requeued messages stuck indefinitely with no active timer event to flush them. This creates message loss-by-stall and unbounded queue growth under repeated spawn failures.
Useful? React with 👍 / 👎.
| if (entry.idleTimer) clearTimeout(entry.idleTimer); | ||
| if (!entry.spawning && entry.session) { | ||
| try { | ||
| await this.executor.shutdown(entry.session); |
There was a problem hiding this comment.
stop() now awaits executor.shutdown() for existing sessions while the NATS subscription is still active, and only unsubscribes afterward. During that wait, processSubscription() can still route new incoming messages and create new sessions; those entries are then dropped by this.sessions.clear() without a corresponding shutdown call, which can leave tmux windows/processes running after stop under live traffic.
Useful? React with 👍 / 👎.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 18760271 | Triggered | Generic Password | a506587 | src/lib/test-setup.ts | View secret |
| 18760271 | Triggered | Generic Password | a506587 | src/lib/test-setup.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Tests now run against a dedicated pgserve spawned in --ram mode (native postgres with data in /dev/shm) on port 20900+, isolated from the production daemon at port 19642 and from ~/.genie/data/pgserve. Eliminates PG deadlock flakiness in team.test.ts that was blocking the pre-push hook. Root cause: tests and the live genie serve daemon shared one PostgreSQL database, and their asymmetric catalog-lock orderings tripped PG's deadlock detector under concurrent load. Full analysis in .genie/wishes/test-pg-ram-isolation/TRACE.md. Changes: - bunfig.toml: [test] preload = ["./src/lib/test-setup.ts"] - src/lib/test-setup.ts: spawns pgserve --ram on 20900-20999, reaps orphaned wrappers from previous runs at startup (bun test hard-exits without firing exit hooks). - src/lib/db.ts: ensurePgserve() honors GENIE_TEST_PG_PORT, production paths unchanged when unset. - src/lib/test-setup.test.ts: proves preload wiring and port binding. Validation: - 5 consecutive full-suite runs: 2052 pass, 0 fail, ~51s each. - bun run check passes on first attempt in 61s. - Orphan reaper keeps steady state of 1 pgserve between runs.
Summary
sanitizeWindowNamevia MD5 hash suffix (prevents window collisions for JIDs that sanitize to the same prefix)isPaneProcessRunning(walks pane pid tree 2 levels deep)sanitizeWindowNamecovering collision resistance, truncation, and edge casesChanges
Test plan