refactor(coding-agent): derive scoped heartbeats at render time - #1830
refactor(coding-agent): derive scoped heartbeats at render time#1830snimu wants to merge 164 commits into
Conversation
…ot-dispose-timeout
…onfig-cache-reset
…flow-patterns-export
…e-daemon-lookup-fake
…e-daemon-lookup-fake
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| if (!this.options.snapshot || !this.isRunning) return; | ||
| const pendingExecutions = this.executionQueue; | ||
| if (this.activeExecution) void this.interrupt().catch(() => undefined); | ||
| let timeout: ReturnType<typeof globalThis.setTimeout> | undefined; |
There was a problem hiding this comment.
🟠 High kernel/repl-manager.ts:1106
dispose() and shutdown({ snapshot: true }) can wait indefinitely instead of honoring SNAPSHOT_EXECUTION_TIMEOUT_MS: a concurrent non-terminating execute() can be appended after pendingExecutions, leaving the final captureSnapshot() queued behind it. The timeout passed to captureSnapshot() does not start until enqueueRequest() finishes awaiting that queue, so the timeout must cover both queue waiting and snapshot execution.
🚀 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 1106:
`dispose()` and `shutdown({ snapshot: true })` can wait indefinitely instead of honoring `SNAPSHOT_EXECUTION_TIMEOUT_MS`: a concurrent non-terminating `execute()` can be appended after `pendingExecutions`, leaving the final `captureSnapshot()` queued behind it. The timeout passed to `captureSnapshot()` does not start until `enqueueRequest()` finishes awaiting that queue, so the timeout must cover both queue waiting and snapshot execution.
Evidence trail:
packages/coding-agent/src/core/kernel/repl-manager.ts:454-496 @ e953a58fd8588288a39201899db29a7280eab230; packages/coding-agent/src/core/kernel/repl-manager.ts:1102-1117 @ e953a58fd8588288a39201899db29a7280eab230; packages/coding-agent/src/core/kernel/repl-manager.ts:897-910 @ e953a58fd8588288a39201899db29a7280eab230
| @@ -2577,7 +2588,6 @@ export class DaemonSupervisor { | |||
| worker.launchEnv = undefined; | |||
There was a problem hiding this comment.
🟡 Medium daemon/daemon-supervisor.ts:2588
After a supervisor restart adopts a pre-revision-23 worker and launches a new worker, the adopted worker's remoteAgentPeers remains stale, so its agent list and family-reachability checks omit the new peer. Because the ready path no longer sends worker_sync_agent_peers, cross-worker messaging and sibling-name admission can be incorrect during mixed-version operation; restore that synchronization after the worker becomes ready.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/modes/daemon/daemon-supervisor.ts around line 2588:
After a supervisor restart adopts a pre-revision-23 worker and launches a new worker, the adopted worker's `remoteAgentPeers` remains stale, so its agent list and family-reachability checks omit the new peer. Because the ready path no longer sends `worker_sync_agent_peers`, cross-worker messaging and sibling-name admission can be incorrect during mixed-version operation; restore that synchronization after the worker becomes ready.
Evidence trail:
packages/coding-agent/src/modes/daemon/daemon-supervisor.ts:2583-2592, 2707-2756, 1536-1553 at e953a58fd8588288a39201899db29a7280eab230; packages/coding-agent/src/modes/daemon/daemon-protocol.ts:716-722 at e953a58fd8588288a39201899db29a7280eab230; packages/coding-agent/src/modes/daemon/daemon-supervisor.ts:3420-3445 at bc0fa7606abb3b7af0f765319518d255e6ae553d; packages/coding-agent/src/modes/daemon/daemon-mode.ts:3657-3667, 5370-5476 at bc0fa7606abb3b7af0f765319518d255e6ae553d; git diff MERGE_BASE REVIEWED_COMMIT -- packages/coding-agent/src/modes/daemon/daemon-supervisor.ts
| private async flushSnapshotForDispose(): Promise<void> { | ||
| if (!this.options.snapshot || !this.isRunning) return; | ||
| const pendingExecutions = this.executionQueue; | ||
| if (this.activeExecution) void this.interrupt().catch(() => undefined); |
There was a problem hiding this comment.
🟠 High kernel/repl-manager.ts:1105
flushSnapshotForDispose aborts an active user cell immediately, so shutdown({ snapshot: true }) and dispose() can snapshot before that cell's updates are committed even when it would finish within the five-second flush window. Remove the immediate interrupt and let the queued execution finish before capturing the snapshot.
- if (this.activeExecution) void this.interrupt().catch(() => undefined);🚀 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 1105:
`flushSnapshotForDispose` aborts an active user cell immediately, so `shutdown({ snapshot: true })` and `dispose()` can snapshot before that cell's updates are committed even when it would finish within the five-second flush window. Remove the immediate interrupt and let the queued execution finish before capturing the snapshot.
Evidence trail:
packages/coding-agent/src/core/kernel/repl-manager.ts:445-497, 831-835, 897-910, 1102-1116 @ e953a58; prime-agent-runtime/src/rlm/repl.md:87-92 @ e953a58
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 11d0356. Configure here.
| clearTimeout(timeout); | ||
| resolve(); | ||
| }); | ||
| child.kill("SIGTERM"); |
There was a problem hiding this comment.
RPC stop can hang forever
Medium Severity
stop() now waits only for the child close event. The 1s timer still sends SIGKILL, but it no longer resolves the waiter. If close already fired in the race after reading this.process, or never fires after kill, stop() never returns and shutdown or tests hang.
Reviewed by Cursor Bugbot for commit 11d0356. Configure here.
| this.sessionHasMessages = hasMessages; | ||
| this.builtInHeader?.invalidate(); | ||
| this.subagentSummaryLine.invalidate(); | ||
| return (this.connectionState?.messageCount ?? 0) === 0 && this.connectionState?.isStreaming !== true; |
There was a problem hiding this comment.
New-chat state lags first message
Low Severity
isNewChat() now stays true until message_end or agent_start. The old path marked the session as having messages on user (or agent) message_start, which is also when the transcript first shows that message. Start hints and new-chat chrome can remain visible after the first prompt has already appeared.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 11d0356. Configure here.
|
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. |
1 participant
What was wrong
The TUI stored a scoped
heartbeatslist — a projection of the heartbeat catalog x connection state x subagent snapshots — kept aligned byupdateScopedHeartbeats()calls scattered across unrelated code paths. A missed call site meant stale heartbeat counts or manager rows (audit: dup-truth.md finding 5; TUI-side sibling of #1743).The fix
The stored projection, its updater, and
setHeartbeats()are deleted. The tray label, refresh scheduler, and heartbeat manager derive the scoped list from the three sources at use/render time; the manager holds only selection state by heartbeat ID (re-anchoring gracefully when the selected entry disappears). Production code is net deletion; stale test stubs of the deleted method removed.How it's verified
Reviewer accounted for all 4 former sync call sites, bounded the per-render derivation cost (tiny, overlay-scoped), traced selection survival through catalog refreshes, and confirmed the getter wiring holds no leak. Focused suites 190/190; full CI-style failing set exactly matches the stack base. Two-model implement/review loop.
Stacked on #1748 (test the whole stack at the leaf; merge base-first).
Note: intentionally no Linear ticket for this cleanup stack, so that check stays red.
Note
Medium Risk
Touches daemon protocol revision, session compaction continuation, RPC transport behavior, and RLM child lifecycle; breaking for removed
getOverflowPatternsand changed internal_rlmChildSessionsmap shape.Overview
This PR removes several mirrored state in the interactive TUI and derives queue, scoped heartbeats, “new chat” hints, and available models from
connectionStateand catalogs at read/render time. Queue browsing and edits no longer optimistically patch a local queue copy:QueueSelection.refreshAtre-anchors by lane, index, and expected text, and move/replace waits on daemon acknowledgments.Daemon protocol revision 23 replaces supervisor
worker_sync_agent_peersbroadcasts with on-demandlist_agent_peers; workers query the supervisor when building agent rosters. Supervised workers apply renames viaapplyStateSessionNameafter the supervisor reserves the name, and remote agent messages are sent once per call (connect retries no longer resend after a lost response).AgentSessiondrops the 100ms post-compaction continuation timer and runs continuation immediately behind idle/refine/queued-work gates. RLM child rosters retainRetainedRlmChildrun metadata so reattach snapshots show queued children, activity, and previews correctly; daemon child listing delegates togetRlmChildSnapshots.RpcClientwaits for process spawn instead of a fixed delay, removes default per-request timeouts (including refine), and fails all pending requests/event waiters on transport errors. Smaller cleanups: sharedlooksLikeSessionPath, legacy RLM registry parsing with defaultsessionDir, kernel dispose cancels in-flight work before final snapshot, removal of test-only cache/lookup hooks andgetOverflowPatterns, and telemetry no longer auto-off underNODE_ENV=test.Reviewed by Cursor Bugbot for commit 11d0356. Bugbot is set up for automated code reviews on this repo. Configure here.
Linear ticket: ENG-5665
(ticket linked above)
Note
Derive scoped heartbeats and interactive-mode state at render time
connectionQueue,connectionModels,sessionHasMessages,heartbeats) fromInteractiveMode; queue, models, heartbeats, and new-chat detection are now derived fromconnectionStateon demand in interactive-mode.tsHeartbeatManagerComponentnow pulls heartbeats via agetHeartbeatscallback at render time instead of receiving pushed arrays; selection tracks by heartbeat id across list mutations in heartbeat-manager.tsInteractiveMode.moveQueueSelectionandeditQueueSelectionno longer apply optimistic local patches; they await server acknowledgment and re-anchor selection viaQueueSelection.refreshAtin queue-selection.tsworker_sync_agent_peerswith a pull-basedlist_agent_peerssupervisor RPC (schema revision 22→23); workers query peers on demand and no longer cache them in daemon-mode.ts and daemon-supervisor.tsRpcClientin rpc-client.ts: removes per-request default timeouts, addsfailPendingOperationsfor centralized transport-failure rejection, and awaits the childspawnevent instead of a fixed delay on startsetTimeoutdelay, coordinating with idle/queued-work state via_runScheduledPostCompactionContinueRpcClient.sendno longer applies a default 30s timeout and will wait indefinitely unless a timeout is explicitly passed or the transport fails; callers relying on implicit timeout behavior need to pass explicit timeouts.getOverflowPatternsis removed from overflow.ts — out-of-tree imports will break.QueueSelection.refreshAtreplacessync()with strict lane/index/text matching; callers using the old API must migrate.📊 Macroscope summarized e953a58. 39 files reviewed, 10 issues evaluated, 7 issues filtered, 3 comments posted
🗂️ Filtered Issues
packages/coding-agent/src/modes/daemon/daemon-mode.ts — 0 comments posted, 2 evaluated, 2 filtered
handleCommandhas nocase "list_agent_peers". The new worker-sidelistSupervisorAgentPeers()request therefore reaches the supervisor but falls through the command switch (and, as the command allowlist also lacks it, is rejected even earlier), so workers always catch the failure and use an empty remote roster. This breaks cross-worker agent discovery/reachability and message routing introduced by the on-demand replacement. [ Out of scope (triage) ]createAgentMessageListResultcallslistSupervisorAgentPeers, but the supervisor'sDAEMON_COMMAND_TYPES/handleCommandhas nolist_agent_peerscase. Every worker query is rejected as unknown and caught as[], so workers cannot list, resolve, or enforce family reachability for agents hosted by other workers. [ Out of scope (triage) ]packages/coding-agent/src/modes/interactive/components/heartbeat-manager.ts — 0 comments posted, 1 evaluated, 1 filtered
heartbeatsgetter at line 43 has no accompanying invalidation path, so changes to the callback result are not rendered until some unrelated input/render occurs. In particular, a connection snapshot can change the session identity used bygetScopedHeartbeats()while the manager is open; its caller only schedules a catalog refresh, and if the scoped jobs are paused there is no timer to cause one. The oldsetHeartbeats()always calledrequestRender(), so the manager can now continue displaying (and allow actions on) heartbeats from the prior session after that update. [ Failed validation ]packages/coding-agent/src/modes/interactive/interactive-mode.ts — 0 comments posted, 1 evaluated, 1 filtered
liveImageMarkerIds()now reads the queue only fromconnectionState.sessionActions, which is updated asynchronously bysession_action_update. After a successful queued-message mutation or prompt admission but before that event is processed, a subsequent large image paste can run eviction without seeing the queued marker and evict its image; recalling/sending that queued prompt then silently omits the attachment. [ Failed validation ]packages/coding-agent/src/modes/rpc/rpc-client.ts — 0 comments posted, 3 evaluated, 3 filtered
waitForIdlenow creates no timer when callers use its documented/default invocation. A live daemon that never emitsagent_endleaves 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) ]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 producesagent_end. This also makes the defaultpromptAndWaitpath unbounded because it delegates to this collection logic. [ Out of scope (triage) ]sendno 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 topendingRequestsat 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 #1749 (recreated as a plain PR against
main; GitHub's stack lock prevented retargeting the stacked PR).