Repository navigation
fix(native-chat): a chat you return to drops background tasks that finished while it was hidden - #24305
Conversation
…nished while it was hidden A background command that finished while its chat pane was hidden stayed in the strip as running, its timer counting for hours, and Stop failed with "The background task wasn't stopped." The pane stops listening while hidden, so it missed the "no tasks" update; once the idle sweep stopped the agent, the host reopened the pane's subscription without any roster at all, and the client reads a missing roster as "unchanged". The roster now rides each subscriber's frames the way the slash-command list and the message queue already do. The subscriber registry reads it from the host's child records on a subscriber's first frame and on every snapshot and reset, and re-sends it to each subscriber whose last copy differs when the records change. The channel always answers: a conversation with no running child is "none" whether or not an agent holds it. Ordinary journal batches never read it. This also fixes the reverse case: a snapshot or reset sent to a live pane carried no roster, which cleared a still-running task from the strip until the roster next changed. The client treats a resumed subscription's first batch as stating the roster, so a pane reconnecting to an older host that still omits it drops its stale copy too. Fixes #24227
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const views = structuredRunningChildWork((session && this.readChildWork(sessionId)) ?? []) | ||
| if (!session || views.length === 0) { | ||
| return null |
There was a problem hiding this comment.
If the child-work store read fails temporarily when a pane subscribes or resyncs, readChildWork returns undefined. This code treats that as an empty roster and sends backgroundTasks: null, hiding tasks that may still be running. Ordinary transcript batches do not read the roster again, so the strip can stay empty after the store recovers until another child-work change or resync. An unavailable read needs to remain distinct from a confirmed empty roster.
Knowledge Base Used: Agent workspaces and sessions
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe background-task channel now provides roster state through a reader. Subscriber frames select and fingerprint rosters, and subscribers republish roster changes only when their saved fingerprint differs. The read transport marks the first batch of each subscription. The reducer clears a previously saved roster when that marked batch omits it. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The supplied evidence identifies no issue that needs correction before merge. The reported checks remain subject to normal CI completion. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change refreshes task status when returning to a chat, without an identified expansion of access or command privileges. Remaining uncertainty concerns failure recovery rather than a demonstrated security flaw. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧰 Additional context used📚 Code guidelines (1)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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one small cleanup inline.
Reviewed changes
- Per-subscriber roster delivery. The background-task roster now rides each subscriber's frames: read from the host's child records on a subscriber's first frame and every snapshot/reset, deduplicated against that subscriber's last sent fingerprint, and re-sent on child-record changes (
agent-session-subscriber-frame-fields.ts,structured-agent-session-subscribers.ts). - The host always answers.
StructuredAgentSessionBackgroundTaskChannel.readreturnsnullfor "none" instead ofundefinedfor "unknown", whether or not a provider holds the session;publishnow delegates torepublishBackgroundTasksand the session-widepublishedcache is gone. - Client clears a resumed roster. The reducer clears a held roster when the first batch of a subscription omits it, and the read transport applies that first batch alone and flags it
opensSubscription, so a hidden pane that missed the host's "no tasks" update drops its stale row (new client against both new and old hosts).
Verified by reading the changed tests, the full agent-session-wire suite (148 files), tc:node, and tc:web. The client snapshot/reset paths replace state wholesale, so only batch frames need the marker; the host's roster attach covers snapshots/resets. No correctness issues found.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…hat-resumed-strip-drops-finished-tasks
…op the unused close observers The host's delivery layer now requires every hook it is built with, so production wiring cannot leave out the roster read (an opening frame without it reads as "no tasks" to current clients). The conversations map's close observers lost their only caller in this PR and are removed. The reducer test comment states the old-host omission rule precisely.
There was a problem hiding this comment.
✅ No new issues found. The prior review's only note is addressed and the delta is a clean tightening of the same design.
Reviewed changes
- Made the subscriber-delivery callbacks required.
StructuredAgentSessionClientDeliverynow takesonJournalActivity,onAgentStarted,onChildWorkChanged, andreadBackgroundTasksas required parameters, with the status feed and subscriber hooks wired unconditionally; the sole host call site already passed all four. - Removed the dead close-observer API.
StructuredAgentSessionConversations.observeClose, itscloseObserversset, and the loop indelete()are gone — the background-task channel was their only caller, and this PR removed that call, so the hook had nothing left to do. - Reworded a reducer-test comment to match the new "old host omits the roster only when its records hold no running child" rule.
The merge of main (551c0c3) contributed the injected logger, agentStart namespace import, wakeDelivery, and finishOwedStop changes in structured-agent-session-host.ts; those are base branch, not authored here.
Technical details
# Verification of the incremental cleanup
## Affected sites
- `src/main/native-chat/agent-session-wire/structured-agent-session-client-delivery.ts:36-41,63` — required params; only construction is `structured-agent-session-host.ts:77`, which passes all.
- `src/main/native-chat/agent-session-wire/structured-agent-session-conversations.ts:63-67` — `delete()` reduced to `activity.delete` + `super.delete`.
## Required outcome
- No call site omits a now-required arg, and nothing still references `observeClose`/`closeObservers`/`onDeliveryError`.
## Evidence
- `grep` over `src/` finds zero references to `observeClose`, `closeObservers`, or `onDeliveryError`; one constructor call site, fully supplied.
- 6 affected test files (37 tests) pass; `pnpm tc:node` passes.deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
…he line limit Main brought the host file to the 300-line lint limit, and this PR's roster wiring adds one line. Name the lease reconciler's type by its factory, as the neighbouring fields do.
…hat-resumed-strip-drops-finished-tasks # Conflicts: # src/main/native-chat/agent-session-wire/structured-agent-session-subscribers.test.ts # src/main/native-chat/agent-session-wire/structured-agent-session-subscribers.ts
…hat-resumed-strip-drops-finished-tasks
…hat-resumed-strip-drops-finished-tasks
…h claudeProviderHandle Main made the provider handle opaque (#24991); build it with the existing helper, as main's own tests now do.
ELI5
If a chat was off screen when its background command finished, coming back to it still showed the command as running. The timer kept counting for hours, and Stop failed. Now the chat shows what is actually running when you come back.
What Changed
The bug (#24227). A Claude chat starts a background command (here a 10-minute CI watcher), then you switch to another tab or workspace. The command finishes while the chat is hidden. Later, the chat has been idle for 30 minutes, so Orca stops its Claude process. When you return, the strip above the composer still says
1 shell command — 2h 21mwith a spinner. Clicking Stop shows "The background task wasn't stopped," because nothing is left to stop.Why it happened. A hidden chat pane stops listening for updates, so it missed the host's "no tasks running" update. When the pane came back, the host reopened its subscription from where the pane had left off and sent no task list at all, because with Claude stopped it reported "I can't say" instead of "none." The client reads a missing task list as "unchanged," so it kept the stale "running" row forever. The host's own records (its journal and its child-task store) already had the task as done. Only the delivery to the returning pane lost it.
After this change. When you return to a chat, the strip shows exactly what the host's records say is running. A task that finished while you were away is gone.
Mechanism. The task list now travels in the frames the host sends each open chat pane (a frame is one update message), the way the slash-command list and the message queue already do:
null), whether or not a Claude process is attached. Its records are the source of truth either way. The old "no live agent → say nothing" rule is gone, along with the session-wide "last sent" cache that remembered what it had sent but not who received it.Also fixed, same cause in the other direction (found by reading the code, not reproduced in the app). When the host re-sent a snapshot to a pane that was already open (after recovering from an interrupted rewind, or after the agent's process exited on its own), the frame carried no task list. The client treats a snapshot without one as "none," so a task that was still running vanished from the strip. The host's "already sent" cache then blocked a re-send until the task list changed again. Snapshots now carry the current list.
Why
The bug isn't specific to one path. Every live field a host publishes (commands, queue, activity) is stated explicitly when a subscription opens, except the task list, which the host could leave out. "Left out" meant "unchanged" to the client, so any gap where the client wasn't listening could leave it stuck. Tracking the list separately for each open pane, and putting it in every opening and resync frame, removes that gap for every way a pane can miss an update: hidden pane, dropped connection, host resync.
Alternatives considered:
nullinstead of nothing when the subscription opens (the approach in fix(native-chat): report empty background tasks on at-rest resume #24239). It fixes the reported path. It leaves the "already sent" cache shared by the whole session rather than tracked per subscriber, and it leaves snapshots without a list, so a running task can still disappear after a resync. It also doesn't help a new client talking to an older remote host.How this differs from recording task progress in the conversation log itself. The common pattern records each background task's start and finish in the conversation log, so a client resuming from its saved position replays the finish. This PR keeps the task list derived from the host's child-task records (the single store everything reads since #22614) and restates it at every subscription open and resync. That is intended: writing task lifecycle into the conversation log would create a second copy of facts the child-task store already owns. Restating the full list on open delivers the same guarantee, that a resuming client sees the current truth, without that second copy. The common pattern also explicitly marks a stopped agent's tasks finished; here the provider's own end handling settles the child records and the task list is derived from them, which is intended because a separate "mark finished on stop" step would be a second writer of facts those records already hold.
Linked Issue
Fixes #24227
Visual Proof
Not captured. Reproducing in the app needs a background task to finish while the chat is hidden, then 30 idle minutes before the agent is stopped. The issue's own reproduction, run through the real subscriber registry and the real client reducer, is now a regression test:
a pane resuming from its cursor drops a task that ended while it was away (#24227)instructured-agent-session-subscriber-background-tasks.test.ts.Testing
New tests, each confirmed to fail with its half of the fix removed:
structured-agent-session-subscriber-background-tasks.test.ts:structured-agent-session-background-task-channel.test.ts: the channel answers "none" for a conversation no agent holds (it used to answer "unknown").structured-agent-session-reducer.test.ts: a resumed subscription's first batch that leaves the list out clears the held copy, and an ordinary batch keeps it.structured-agent-session-read-transport.test.ts: each subscription's first batch is applied alone and marked, and later batches are still coalesced.Updated tests: the subscriber, command-publication and outbound-admission tests now use the per-subscriber re-send instead of the removed push method.
structured-agent-session-child-work-selection.test.tsasserted the old "no agent → no answer" rule and now expects "none."Runs (macOS):
src/main,src/rendererandsrc/shared, the Claude/Codex adapter tests that touch background tasks (real-CLI files excluded), and the end-to-end folder's read-owner unit test. 8039 passed.tests/e2e/cross-version-wire/cross-version-agent-session-wire.unit.test.ts: 17/17.pnpm tc:nodeandpnpm tc:webpass.oxlinton the changed files: 0 findings.check:code-quality:changedpasses.mainand the review follow-ups: 62 related test files (544 tests, including the cross-version file above) pass,tc:nodeandtc:webpass, and lint is clean.Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
nullas "no tasks," which already means hiding the strip, so its stale row clears too.Checklist
N/Awith reason. Not captured; reason under Visual Proof.pnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred). Ran: changed-file lint,tc:nodeandtc:web, and 778 related test files. CI covers the full suite and build.