Repository navigation
fix(terminal): retain and pace slow fallback process queries - #24640
juliusmarminge wants to merge 4 commits into
Conversation
Slow but valid Linux ps and Windows PowerShell results must be used, the poll loop must idle at least as long as a slow query took, and a hung query must end and be retried later. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…he next poll A loaded Linux host or Windows WMI can take seconds to answer the process-table fallback. The 1s/1.5s timeouts threw those valid tables away and the loop re-issued the query almost immediately. Both queries now share a 15s timeout, and the poll loop idles at least four times as long as the last fallback query took (capped at 60s), on success and on failure. Native and custom inspection are not delayed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
closeIdle queries the process table while holding the thread lock, so the 15s fallback budget meant to tolerate slow ps/WMI on the poll loop could stall resize, write, and open on that thread. The fallback timeout is now a per-call input: the poll loop keeps 15s, closeIdle keeps ps 1s and PowerShell 1.5s. Native process tables and custom inspectors are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
vouch:trusted
ApprovabilityVerdict: Needs Human Approval Macroscope's review found this PR requires human approval — The server terminal manager now uses longer fallback query budgets and duration-based polling delays, changing production activity-detection and idle-cleanup timing. The change is tested and localized, but it modifies hard-coded product defaults and existing runtime behavior. You can add or adjust custom eligibility rules. Learn more. |
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Timed process-table snapshots apps/server/src/terminal/Manager.ts |
POSIX and Windows process-table queries accept supplied timeouts. Fallback snapshots return success and elapsed-time metadata. |
Query-aware polling schedule apps/server/src/terminal/Manager.ts, apps/server/src/terminal/Manager.test.ts |
The polling loop uses snapshot success and query duration to schedule the next poll. Tests cover activity reporting after slow queries, query spacing, and retries after timeouts. |
Bounded closeIdle queries apps/server/src/terminal/Manager.ts, apps/server/src/terminal/Manager.test.ts |
closeIdle uses a 1-second POSIX timeout or a 1.5-second Windows timeout. Tests check that close and concurrent resize complete while the process remains alive. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix · Severity of issue fixed: Medium
Suggested labels: size:M
Suggested reviewers: t3dotgg, tiliakoos
Merge Risk: 
cca46
-
The fallback polling change has no identified merge-blocking issue; it can proceed with normal checks.
Pre-merge checks: 
-
Check name Status Explanation Title check The title "fix(terminal): retain and pace slow fallback process queries" matches the main change in the changeset. The diff raises the fallback process-table query timeout to 15 seconds on POSIX and W… Description check The description follows the template's sections: Problem, Change, Scope and approval, and Verification. The Problem section states the 1.5-second timeout discards slow but valid results and gives the … Linked Issues check The only linked issue is #9476. It is closed and marked as historical context only. No active directly linked target remains, so no linked-issue coding requirements apply. The PR objectives also state…Out of Scope Changes check The whole-PR diff of apps/server/src/terminal/Manager.ts (merge base 945fd1d to head cca4625) matches the stated objective of fallback process-table pacing. The changes are: (1) PROCESS_TABLE_QUER… Full details: Title check
-
Explanation
The title "fix(terminal): retain and pace slow fallback process queries" matches the main change in the changeset. The diff raises the fallback process-table query timeout to 15 seconds on POSIX and Windows, applies successful snapshots before waiting, and paces the next poll by the greater of failure backoff and four times the query duration, capped at 60 seconds. The title is concise, follows conventional commit form, and names the primary behavior. It does not mention the closeIdle timeout split or the tests, which is acceptable.
Full details: Description check
-
Explanation
The description follows the template's sections: Problem, Change, Scope and approval, and Verification. The Problem section states the 1.5-second timeout discards slow but valid results and gives the environment (Windows fallback). The Change section explains the 15-second timeout, duration-based pacing, and the closeIdle split. Scope links the related issues and PR and credits contributors. Verification lists the tests, lint, typecheck, and review results and clearly states what was not verified (Windows, real macOS, native clients, CI on the published head). Minor issues: the Problem section gives no explicit reproduction steps, and the Verification section is long and includes a large amount of archive and log detail that a reviewer must sort through. The template also asks for the agent/model used at the end, which the description provides. These gaps are non-critical, so the description is mostly complete.
Full details: Linked Issues check
-
Explanation
The only linked issue is
#9476. It is closed and marked as historical context only. No active directly linked target remains, so no linked-issue coding requirements apply. The PR objectives also state that no active linked targets remain. Listener discovery in#16865is a separate item and is not assessed here.
Full details: Out of Scope Changes check
-
Explanation
The whole-PR diff of apps/server/src/terminal/Manager.ts (merge base 945fd1d to head cca4625) matches the stated objective of fallback process-table pacing. The changes are: (1) PROCESS_TABLE_QUERY_TIMEOUT of 15 seconds passed to the POSIX ps and Windows PowerShell fallbacks; (2) CLOSE_IDLE_QUERY_TIMEOUT of 1 second (POSIX) and 1500 ms (Windows) for closeIdle; (3) Effect.result and Effect.timed wrapping of fallback queries, which keeps the TerminalSubprocessCheckError as the failure value and records elapsed time; (4) subprocessSnapshotPollDelayMs taking max(backoff, 4 x queryElapsedMs), capped at 60 seconds; (5) the poll loop applying successful snapshots before sleeping. Native processTable and custom inspector paths report queryElapsedMs 0, so they receive no fallback delay. The Windows parser body is moved into a function with a timeout parameter. Its logic is unchanged apart from formatting. The PR description and objectives cite these changes. Manager.test.ts was not read directly. Its described tests cover the same polling path and are consistent with the changes. This is an inference from the summary, not a direct read of the test diff.
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
juliusmarminge
added
bugfix
Problem
When native process snapshots are unavailable, Windows terminal activity falls back to a PowerShell process-table query. The previous 1.5-second timeout discards slow but valid results, while repeated queries keep doing expensive work on affected hosts.
Change
Allow the Windows fallback 15 seconds and measure each fallback query's elapsed time on success or failure. Apply successful snapshots before waiting, then delay the next poll by the larger of the existing failure backoff and four times the fallback duration, capped at 60 seconds. Preserve the underlying error cause when adding timing information.
Native snapshots and custom inspectors contribute no fallback delay. The POSIX
pstimeout remains one second; its fallback uses the same elapsed-time pacing. On slow hosts, subsequent activity updates intentionally arrive less often.Scope and approval
This is the separate terminal fallback follow-up requested in the closure of #9520, addressing the fallback problem reported on #9476. Listener discovery remains separate in #16865. These links establish scope; they do not imply merge approval.
Credit to UtkarshUsername for the original implementation and to alhadeff-automaton for this contribution. Contributor commits are preserved. The change is in the shared server terminal manager, with no client or wire-contract changes.
Verification
TestClock,Deferredmilestones and a controlled process runner: a five-second successful query publishes activity before waiting and starts the next query at 25 seconds; a 15-second timeout starts the next query at 75 seconds.Prepared through T3 Code: Opus 5.5 test reduction and review, Fable 5.1 implementation, and GPT-6.1 Sol coordination and independent review, using Claude Code and Codex harnesses.
Takeover
Rebuilt from current main, crediting @alhadeff-automaton and #16874. The original description above is preserved as historical context; this section supersedes its implementation and verification claims.
Both background fallbacks now allow 15 seconds; close-idle retains POSIX 1s / PowerShell 1.5s. A timed
Resultcarries success/failure without changing errors; existing polling waits for max(backoff, 4× query duration), capped at 60s, after applying successful results. No parser changes.Original tests were replaced with shared-path regression tests and cleanup responsiveness coverage. This replacement does not preserve contributor commits. Fixes #9476.
Evidence
Before: 5a969c17e7; after: a77f9be73a; final main merge: cca46259f5 (terminal manager and runner unchanged from captured after).
Measured matrix, 12-case transcripts, reproduction scripts.
Original close fixtures leaked a background sleep child on both revisions; those invalid cleanup rows and exact-PID manual cleanup remain in the archive. No general child-tree cleanup claim is made. Supplemental close proof, transcripts, and cleanup verification.
Verification log