Skip to content

refactor(coding-agent): delete unreachable empty-selector auto-cancel timers - #1828

Closed
snimu wants to merge 146 commits into
mainfrom
snimu/remove-empty-selector-timers
Closed

refactor(coding-agent): delete unreachable empty-selector auto-cancel timers#1828
snimu wants to merge 146 commits into
mainfrom
snimu/remove-empty-selector-timers

Conversation

@snimu

@snimu snimu commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

What was wrong

Both TUI selectors (fork-message and session-tree) carried a 100ms auto-cancel timer for the empty-collection case — but their only construction sites already return early with a user-facing notice when the collection is empty. Unreachable defensive timers, plus the stale-callback hazard they imply (audit: timeouts.md finding 8).

The fix

Pure deletion of both blocks (+1/−8). No replacement assertions — the callers own the precondition.

How it's verified

Reviewer independently enumerated all construction sites (production + tests) confirming the empty case cannot reach the components, and that the deleted blocks held no listener registrations — nothing leaks. tsgo/biome clean, tree-selector 18/18, full CI-style suite failing set matches stack base. Two-model implement/review loop, approved first pass.

Stacked on #1744 (test the whole stack at the leaf; merge base-first). This completes the deletion-only wave of the audit stack.

Note: intentionally no Linear ticket for this cleanup stack, so that check stays red.


Note

Medium Risk
Medium risk from rewritten post-compaction continuation, single-shot remote messaging, and RPC clients waiting indefinitely without per-command timeouts; telemetry may run in tests unless DO_NOT_TRACK is set.

Overview
This PR bundles several reliability and UX fixes across the coding agent, daemon, RPC client, and interactive TUI, plus small API removals in pi-ai.

Session loop after compaction no longer waits on a 100ms timer. Post-compaction continuation is driven by idle/retry/refine gates, queued-work pauses, and an explicit continueAfterSessionInput flag so compaction can resume the turn immediately when safe.

RLM child visibility is unified on AgentSession.getRlmChildSnapshots() (duration, activity, previews, recap, tokens). Retained children store { session, run? } so reattached sessions show queued runs correctly; daemon attach snapshots delegate to the parent session and only add activeSessionId for resident children. Legacy rlm-subagents.jsonl parsing is centralized in readLegacyRlmSubagentRegistry, deriving sessionDir when missing.

Daemon / worker behavior: remote agent messages connect once, send once, and do not retry on timeout or lost responses; supervised workers apply renames via setStateSessionNameForCommand without re-validating names the supervisor already approved. Python kernel graceful dispose interrupts in-flight work and skips a final snapshot if the execution queue does not settle within SNAPSHOT_EXECUTION_TIMEOUT_MS (removes the separate dispose timeout cap).

Interactive TUI: the message queue is read only from connectionState.sessionActions (no local mirror or optimistic queue patches); browse/edit/move reconcile via refreshQueueSelectionAt. “New chat” hints and shortcut tray use messageCount (and streaming) instead of a local sessionHasMessages flag. Empty tree/message selectors no longer auto-cancel after 100ms.

RPC client drops fixed request/refine timeouts and the 100ms startup sleep; transport failures latch and fail pending requests and event waiters. Telemetry no longer treats NODE_ENV=test as off—tests stub DO_NOT_TRACK explicitly. Misc: shared looksLikeSessionPath, removed test-only clearConfigValueCache / daemon lookup override / getOverflowPatterns export.

Reviewed by Cursor Bugbot for commit 88dbffa. Bugbot is set up for automated code reviews on this repo. Configure here.

Linear ticket: ENG-5663
(ticket linked above)

Note

Remove empty-selector timers, RPC client timeouts, and post-compaction delay

  • TreeSelectorComponent and UserMessageSelectorComponent no longer start 100ms auto-cancel timers when their input is empty; selectors stay open until explicitly cancelled
  • RpcClient removes all per-request timeouts (REFINE_REQUEST_TIMEOUT_MS deleted, send no longer takes timeoutMs); instead it latches the first transport error in transportError, fails all pending requests and event waiters via failPendingOperations, and waits for the child spawn event on startup instead of a fixed 100ms delay
  • AgentSession post-compaction continuation replaces setTimeout/clearTimeout scheduling with async waits on agent idle, queued-work pauses, and session input checkpoints; a new continueAfterSessionInput flag controls whether continuation proceeds after session input
  • InteractiveMode removes the local connectionQueue mirror and sessionHasMessages flag; queue data is now derived from connectionState.sessionActions via getConnectionQueue, model lists via getAvailableConnectionModels, and isNewChat keys off messageCount/isStreaming
  • daemon-mode supervised renames route through setStateSessionNameForCommand, supervisor send_message performs a single request/response cycle, and legacy RLM subagent registry parsing is centralized in rlm-ledger.readLegacyRlmSubagentRegistry
  • Behavioral Change: RpcClient.send no longer auto-times out — requests wait indefinitely unless a transport error occurs or the process responds; waitForIdle and collectEvents only time out when an explicit timeout is passed. isTelemetryEnabled no longer returns false for NODE_ENV=test; tests now set DO_NOT_TRACK=1 via vitest.config.ts. clearApiKeyCache/clearConfigValueCache and getOverflowPatterns exports are removed. findActiveDaemonSessionSummaryForInteractiveStartup is no longer exported from main.ts.
📊 Macroscope summarized 2033172. 33 files reviewed, 9 issues evaluated, 6 issues filtered, 3 comments posted

🗂️ Filtered Issues

packages/coding-agent/src/core/agent-session.ts — 0 comments posted, 1 evaluated, 1 filtered
  • line 8605: Passing true makes the post-compaction worker call agent.continue() after it has drained queued session input. If a follow-up/steering action runs first, that action completes with an assistant message as the transcript tail; Agent.continue() then rejects with nothing-to-continue, so the threshold-compacted tool loop is never resumed. Trigger this by queueing a prompt while a threshold compaction that set _continueAfterThresholdCompaction is in progress. [ Failed validation ]
packages/coding-agent/src/modes/interactive/interactive-mode.ts — 1 comment posted, 3 evaluated, 2 filtered
  • line 2840: Resetting the shared pendingQueueMove flag lets a move from the old session race a newly started move after a session replacement. The old moveQueueSelection coroutine still reaches its unconditional finally and sets the flag to false, so a session_action_update for the new in-flight move calls refreshQueueSelectionFromState() prematurely. That can reset/retarget the selection and restore the stashed draft while the new mutation is still pending; when its response arrives, the UI is browsing a queued item but the editor contains the unrelated draft, so submitting edits the wrong queued message. [ Failed validation ]
  • line 7123: When the selected queue entry is changed or consumed while its edit RPC is in flight, the corresponding session_action_update skips selection refresh because pendingQueueEdit is set. On a returned rejected/invalid status, this branch restores the editor text but never refreshes or resets queueSelection; it remains pointed at the vanished/stale (lane,index,text). Subsequent Enter or follow-up attempts repeatedly target that stale entry and cannot submit the restored text as a normal prompt until the user manually leaves browse mode. [ Failed validation ]
packages/coding-agent/src/modes/rpc/rpc-client.ts — 1 comment posted, 4 evaluated, 3 filtered
  • line 550: waitForIdle now creates no timer when callers use its documented/default invocation. A live daemon that never emits agent_end leaves the registered waiter pending forever, so callers cannot recover from a stuck run; the former default rejected after 60 seconds. Process-error handling only covers process/stream termination, not this live-but-stalled path. [ Out of scope (triage) ]
  • line 582: collectEvents() no longer has its 60-second default timeout, so a caller using it without an explicit timeout waits forever if the agent remains live but never produces agent_end. This also makes the default promptAndWait path unbounded because it delegates to this collection logic. [ Out of scope (triage) ]
  • line 687: send no longer installs any timeout for a pending RPC request. If the child stays alive but fails to emit a matching response (for example, a stalled daemon command), the promise added to pendingRequests at this call never settles and the public operation hangs indefinitely; previously every command was rejected after 30 seconds. Transport-close handling cannot recover this live-but-unresponsive case. [ Out of scope (triage) ]

Supersedes #1746 (recreated as a plain PR against main; GitHub's stack lock prevented retargeting the stacked PR).

snimu added 30 commits August 24, 2026 11:17
snimu added 18 commits August 27, 2026 11:01
if (this.process.exitCode !== null) {
throw new Error(`Agent process exited immediately with code ${this.process.exitCode}. Stderr: ${this.stderr}`);
try {
await once(child, "spawn");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium rpc/rpc-client.ts:140

start() resolves as soon as the node process emits spawn, so a missing or broken cliPath is reported as a successful startup even though the child exits immediately afterward. Restore a short post-spawn exit check so callers can detect initialization failures from await start().

		await once(child, "spawn");
+		await new Promise((resolve) => setTimeout(resolve, 100));
+		if (child.exitCode !== null) {
+			throw new Error(`Agent process exited immediately with code ${child.exitCode}. Stderr: ${this.stderr}`);
+		}
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/modes/rpc/rpc-client.ts around line 140:

`start()` resolves as soon as the `node` process emits `spawn`, so a missing or broken `cliPath` is reported as a successful startup even though the child exits immediately afterward. Restore a short post-spawn exit check so callers can detect initialization failures from `await start()`.

Evidence trail:
packages/coding-agent/src/modes/rpc/rpc-client.ts:112-143 (REVIEWED_COMMIT); git diff MERGE_BASE..REVIEWED_COMMIT -- packages/coding-agent/src/modes/rpc/rpc-client.ts

/** Best-effort final snapshot before a graceful dispose, bounded by a timeout. */
private async flushSnapshotForDispose(): Promise<void> {
if (!this.options.snapshot || !this.isRunning) return;
const pendingExecutions = this.executionQueue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High kernel/repl-manager.ts:1104

dispose() and shutdown({ snapshot: true }) can wait indefinitely for the final snapshot when a concurrent execute() queues a non-terminating request after pendingExecutions is captured. captureSnapshot() is queued behind that request, and enqueueRequest() does not start its timeout until after await prev; block new executions while flushing or apply the timeout to the queue wait as well.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/core/kernel/repl-manager.ts around line 1104:

`dispose()` and `shutdown({ snapshot: true })` can wait indefinitely for the final snapshot when a concurrent `execute()` queues a non-terminating request after `pendingExecutions` is captured. `captureSnapshot()` is queued behind that request, and `enqueueRequest()` does not start its timeout until after `await prev`; block new executions while flushing or apply the timeout to the queue wait as well.

Evidence trail:
packages/coding-agent/src/core/kernel/repl-manager.ts:454-497, 897-910, 1002-1020, 1102-1117 at 20331723e1b5ba29e48f2d185214f2b4529a204b; packages/coding-agent/src/core/agent-session.ts:1376 at 20331723e1b5ba29e48f2d185214f2b4529a204b

if (selected) {
try {
status = await this.agentConnection.mutateQueuedMessage(
selected.lane,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium interactive/interactive-mode.ts:7095

applyQueueSelection can report a rejected mutation and restore the edit even though the preceding reorder already succeeded, so quickly editing a moved queued message loses the requested replacement or deletion. It re-reads this.queueSelection.selected after waiting behind serialized mutations; an earlier session_action_update can reset that selection while the queue mirror is still stale. Preserve the submitted selection or otherwise synchronize the queue state before deciding that selected is absent.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/modes/interactive/interactive-mode.ts around line 7095:

`applyQueueSelection` can report a rejected mutation and restore the edit even though the preceding reorder already succeeded, so quickly editing a moved queued message loses the requested replacement or deletion. It re-reads `this.queueSelection.selected` after waiting behind serialized mutations; an earlier `session_action_update` can reset that selection while the queue mirror is still stale. Preserve the submitted selection or otherwise synchronize the queue state before deciding that `selected` is absent.

Evidence trail:
packages/coding-agent/src/modes/interactive/interactive-mode.ts:7005-7046, 7059-7137 @ 20331723e1b5ba29e48f2d185214f2b4529a204b; packages/coding-agent/src/modes/interactive/queue-selection.ts:63-83 @ 20331723e1b5ba29e48f2d185214f2b4529a204b; git diff MERGE_BASE..REVIEWED_COMMIT -- packages/coding-agent/src/modes/interactive/interactive-mode.ts

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 88dbffa. Configure here.

clearTimeout(timeout);
resolve();
});
child.kill("SIGTERM");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RPC stop can hang forever

High Severity

stop() now resolves only on the child close event. The 1s timer sends SIGKILL but no longer finishes the waiter, so a missed or missing close (process already exited, or spawn failed with only an error event) leaves stop() pending indefinitely and blocks restart.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 88dbffa. Configure here.

@snimu

snimu commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Superseded: the chained stack was restructured into independent PRs (byte-identical combined tree). A fresh standalone PR for this change follows on the same branch name.

@snimu snimu closed this Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant