Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
PR Summary by QodoBound terminal sessions to prevent resource leaks
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
![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: 36c1589d03
ℹ️ 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".
| // Reap idle/expired sessions first so the cap reflects live sessions only. | ||
| purgeIdleSessions(); | ||
| if (sessions.size >= MAX_SESSIONS) { | ||
| throw new Error( | ||
| `Terminal session limit reached (${MAX_SESSIONS}). Close an existing session before opening a new one.` |
There was a problem hiding this comment.
When many requests create sessions simultaneously while the map has room, every call performs this size check and then suspends at await resolveInitialCwd before any session is inserted. Once those awaits complete, every call spawns and inserts a child, so a burst can exceed 50 by an arbitrary amount and bypass the resource cap. Reserve capacity before the await, or perform the check after it.
Useful? React with 👍 / 👎.
| }): Promise<TerminalSessionSnapshot> => { | ||
| // Reap idle/expired sessions first so the cap reflects live sessions only. | ||
| purgeIdleSessions(); | ||
| if (sessions.size >= MAX_SESSIONS) { |
There was a problem hiding this comment.
After 50 shells terminate naturally, their entries remain in sessions for the 60-second output grace period because purgeIdleSessions retains exited and error sessions until then. This sessions.size check still counts those non-running entries, so every new session creation fails during that period even when no terminal is concurrent. Count only running sessions for this limit or release capacity when a child exits.
Useful? React with 👍 / 👎.
Code Review by Qodo
1. Session limits never reach users
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| const EXPIRED_OUTPUT_GRACE_MS = 60 * 1000; // 60 s — after exit, retain snapshot for this long before deletion | ||
| const PURGE_INTERVAL_MS = 30 * 1000; // 30 s — background sweep cadence | ||
| const SIGKILL_TIMEOUT_MS = 5 * 1000; // 5 s — SIGKILL escalation if SIGTERM is ignored | ||
| const MAX_SESSIONS = 50; // hard cap on concurrent sessions to bound resource use |
There was a problem hiding this comment.
1. Session limits never reach users 🐞 Bug ≡ Correctness
terminal-sessions.ts is absent from the package exports and has no production import; only its colocated test imports it directly. The user-facing terminal panel updates local display state instead, so the new guard is never executed through shipped terminal behavior.
Agent Prompt
## Issue description
The new terminal-session limit is implemented in a module that is neither exported nor invoked by production code, so it cannot remediate shipped terminal behavior.
## Fix Focus Areas
- packages/codeflow-store/src/shared/terminal-sessions.ts[40-40]
- packages/codeflow-store/package.json[8-52]
- packages/Codeflow_master/src/components/panels/TerminalPanel.tsx[79-92]
## Recommended Fix
Apply the lifecycle and limit enforcement in the terminal implementation actually invoked by users, then expose and call that implementation through a supported package export and production route rather than importing test-only source directly.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| purgeIdleSessions(); | ||
| if (sessions.size >= MAX_SESSIONS) { |
There was a problem hiding this comment.
2. Concurrent requests bypass the cap 🐞 Bug ☼ Reliability
createTerminalSession checks sessions.size without reserving a slot, then yields while awaiting resolveInitialCwd before registering the new session. Concurrent calls can therefore all pass below 50 and subsequently spawn and retain more than the intended maximum number of child processes.
Agent Prompt
## Issue description
The session limit check is separated from registration by asynchronous work, allowing concurrent creations to exceed the hard resource cap.
## Fix Focus Areas
- packages/codeflow-store/src/shared/terminal-sessions.ts[249-257]
- packages/codeflow-store/src/shared/terminal-sessions.ts[259-304]
## Recommended Fix
Atomically reserve a session slot before the first await, include reservations in the cap calculation, and release the reservation on every validation or spawn failure. Convert the reservation into the registered session only after successful initialization.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Reap idle/expired sessions first so the cap reflects live sessions only. | ||
| purgeIdleSessions(); | ||
| if (sessions.size >= MAX_SESSIONS) { |
There was a problem hiding this comment.
3. Idle sessions block new terminals 🐞 Bug ≡ Correctness
createTerminalSession invokes purgeIdleSessions but immediately counts every remaining map entry, while that purge only signals idle running sessions and deliberately leaves them mapped. At 50 idle sessions, creation still throws until child closure and the subsequent output-grace expiry, even though all sessions have already been selected for reaping.
Agent Prompt
## Issue description
Sessions already selected for idle termination remain counted by the newly added capacity check and temporarily prevent replacement sessions from opening.
## Fix Focus Areas
- packages/codeflow-store/src/shared/terminal-sessions.ts[188-221]
- packages/codeflow-store/src/shared/terminal-sessions.ts[249-255]
## Recommended Fix
Base process capacity on running sessions that have not been selected for termination, or remove reaped entries from the capacity accounting immediately while retaining any required output snapshots separately. Keep pending creation reservations in the same capacity calculation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Original task: task_1780281408931_2vxzzbu7l
PR branch: claude-lane/task_1780281408931_2vxzzbu7l-20260602125813
CI failure: merge-conflict
Strategy: rebase
Hint: PR has merge conflicts with main. Run: git fetch origin && git rebase origin/main && git push --force-with-lease. The watcher's open_pr_for_task will detect the existing PR.
Failure log (last 3000 chars)
Workflow
git log origin/main..claude-lane/task_1780281408931_2vxzzbu7l-20260602125813)claude-lane/task_1780281408931_2vxzzbu7l-20260602125813) — the worktree is already set up by the dispatcherfix(ci): <one-line summary>(single line)Automated by DevPulse dispatcher.