Repository navigation
fix(desktop): preserve Swarm session activity during delegated work - #5495
chinawch007 wants to merge 9 commits into
Conversation
4f575f0 to
50255b3
Compare
573dffc to
717ec29
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review summary
Current head: 717ec29b6895961e1b382005b717e138c147acd9
P2 — Owner catalog stale activity survives a failed TTL refresh
After an Owner catalog reaches an authoritative state, a failed TTL refresh does not revoke #catalogFresh. The stale Swarm backgroundActivity can therefore remain projected as running or waiting indefinitely in the UI.
Evidence:
apps/desktop/src/main/session-local-service.ts:278-293refreshes after the five-second TTL but does not clear freshness first.apps/desktop/src/main/session-local-service.ts:347-367reportslistSessions()failure without clearing#catalogFreshor emitting a cached fallback.apps/desktop/src/renderer/session-status-presentation.ts:70-78therefore does not remove the stale activity, andpackages/ui/src/session-history-list.tsx:1723-1735continues to project it.
The review reproduced this on the current head by starting with an authoritative running catalog, advancing past 6001 ms, and making subsequent listSessions() calls reject. The stale result remained authoritative and continued to report running. Please add a regression that asserts authoritative=false, localState=cached, and removal of backgroundActivity and runningTurnIds after a failed refresh, followed by recovery after a successful retry.
The reviewed change has no schema or migration changes. Hosted test is successful and the merge tree is clean. One local production Bash sandbox test could not run because this environment rejected unshare/bwrap; it was not attributed to the PR. Cross-process Guest disconnect/network-partition behavior and native Windows/macOS coverage were not exercised.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
hqhq1025
left a comment
There was a problem hiding this comment.
Finding
P2 - Expired Owner catalogs remain authoritative after a refresh failure
catalog() continues to derive authoritative only from the current connection's existing #catalogFresh entry (apps/desktop/src/main/session-local-service.ts:278-293). When the five-second TTL expires, it starts a refresh but leaves that entry intact. If the current connection's listSessions() then rejects, the catch path only reports the error (apps/desktop/src/main/session-local-service.ts:347-352); it does not revoke freshness or notify consumers that the stored catalog is now cached.
This is reachable on the current head: seed an Owner catalog whose root session has backgroundActivity: 'running', advance the mocked clock by 6001 ms, and make subsequent listSessions() calls reject. Before, during, and after the failed refresh, repeated catalog() reads still return authoritative: true with backgroundActivity: 'running'. Persistent refresh failures therefore allow a finished or unreachable Swarm session to remain shown indefinitely as running or waiting. The renderer only strips background activity after the row has been marked cached (apps/desktop/src/renderer/session-status-presentation.ts:70-78), while the session list directly renders authoritative background activity (packages/ui/src/session-history-list.tsx:1723-1735).
Please revoke the current freshness generation when a TTL refresh fails, emit the cached state, and retry from unknown state. A regression test should start from an authoritative running catalog, fail the same-connection TTL refresh, assert authoritative: false, localState: 'cached', and cleared backgroundActivity/runningTurnIds, then verify that a successful retry restores authoritative state. The existing tests cover reconnect recovery failure and successful TTL refresh, but not this same-connection failure path.
Review scope: exact head 717ec29b6895961e1b382005b717e138c147acd9; current main b62ca805e58fcd8975620112c153c76ebe515099; merge-tree clean; hosted test successful. Focused Runtime, Desktop, UI, Host catalog/IPC, typecheck, lint, format, ASF-header, renderer-architecture, E2E-budget, and protocol-epoch checks passed. One unchanged production Bash sandbox test could not run in this environment because its managed sandbox boundary was unavailable; I did not attribute that failure to this PR. I did not run packaged Electron E2E or native Windows/macOS validation.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
Thanks for catching this. Fixed in commit 0326988. |
hqhq1025
left a comment
There was a problem hiding this comment.
Re-review result
Exact head: 0326988cbdc71b5fcfb84463d6a583a6ffc5e52f
I found no new P0-P3 issue on this head. The previously reported P2 is fixed.
The current increment changes only session-local-service.ts and its tests. On a failed refresh for the current Owner connection, the service now deletes the partition's freshness entry and emits a change on the live-to-cached transition (apps/desktop/src/main/session-local-service.ts:348-374). Catalog consumers therefore receive authoritative: false cached rows, with stale execution activity removed. The added regression covers both running and waiting_for_user, verifies that repeated failures do not create a notification/retry loop, and verifies successful recovery (apps/desktop/src/main/__tests__/session-local.test.ts:468-526).
I compiled and ran those current tests against the exact parent 717ec29b6895961e1b382005b717e138c147acd9; both failed at the expected authoritative: true assertion. They pass on this head.
Validation on the exact head:
- Desktop: 2757/2757 passed; UI: 643/643 passed.
- Focused compiled
session-localsuite: 46/46 passed. - Full build, typecheck, lint, format, ASF-header, protocol-epoch, renderer-architecture, renderer stale-output, and E2E-budget checks passed.
- Hosted
testis successful. - The merge tree against current
main4d1e35898fa27bb32408a2bee7746e09ad2f85a9is clean. A synthetic merge completed the full test build and the 46-testsession-localsuite successfully. The two latest main commits touch Runtime model-adapter/scheduled-task files and do not overlap this PR's file set. - No schema or migration changes are present.
Remaining coverage gaps: I did not run packaged Electron E2E or native Windows/macOS validation. The production Owner catalog failure path is covered by the focused service tests and the broader Desktop/UI suites.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
0326988 to
3673874
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Re-review result
Exact head: 36738742d32284838e7c90c64ebc3caa07ea0611
I found no P0-P3 issue on this head. The previously reported stale Owner activity issue remains fixed.
This four-commit series derives process-local background activity from graph scheduling/claims, pending interactions, and supervisor wake delivery (packages/runtime/src/stream-graph-coordinator.ts:275-354, packages/runtime/src/agent-graph-supervisor-wake.ts:279-343, packages/runtime-host/src/server/session-background-activity.ts:22-57). Runtime Host publishes that strict protocol projection through session catalogs, and cached Guest/Owner projections strip activity once their live authority is lost (packages/runtime-host/src/protocol/session-catalog.ts:1015-1027, apps/desktop/src/shared/shared-session-catalog-projection.ts:48-53, apps/desktop/src/main/runtime-host-guest-session-mounts.ts:234-299). The renderer distinguishes background running, waiting, and blocked activity from a Session's own live turn (packages/ui/src/session-history-list.tsx:1683-1725). No database schema or migration changes are present.
The final commit revokes Owner catalog freshness after a current-connection refresh failure, emits the live-to-cached transition once, and immediately permits recovery without retaining stale execution activity (apps/desktop/src/main/session-local-service.ts:327-377). Its running and waiting_for_user regressions cover pending refresh, failure, repeated failure, no notification/retry loop, retained disk history, and successful recovery (apps/desktop/src/main/__tests__/session-local.test.ts:488-545). Both tests fail at the expected authoritative-state assertion when compiled and run against the exact parent 46a399e50aa6f24f2f31df02c729eb6e562e7410, and pass on this head.
Validation on this exact head:
- Full Runtime: 3530 passed, 13 skipped; full Desktop: 2781 passed; full UI: 659 passed.
- Focused Desktop/Guest/Owner/UI: 144 passed; graph/supervisor: 49 passed; Host protocol/catalog/activity: 95 passed; production Host Swarm/interaction: 4 passed.
- Clean install and test build, typecheck, lint, format, ASF headers, protocol epoch guard and its 17 tests, renderer architecture, renderer build/staleness, and E2E budget checks passed.
- Full Runtime Host: 2119 passed, 19 skipped, and one sandbox-boundary test failed because the local Linux sandbox could not establish the expected boundary. The same unchanged test fails at the same assertion on exact
main99cfeb7e94728dfece75e7a47677c82c7506bb39, so I did not attribute it to this PR. - Hosted
testis successful. The PR is 4 commits ahead / 0 behind currentmain;git diff --checkpasses and the merge tree is clean.
Remaining coverage gaps: I did not run packaged Electron E2E, native Windows/macOS validation, or a live cross-machine Guest transport.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
3673874 to
40567bd
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Re-review result
Exact head: 40567bdb625992a7e3cbf26269c4f0a338468187
I found no P0-P3 issue on this head. The previously reported stale Owner activity issue remains fixed.
The four-commit series projects Swarm/graph activity, pending interactions, and supervisor wakes into the Host session catalog without making the projection a durable execution authority. Runtime derives activity from durable graph/claim state and current driver generations (packages/runtime/src/stream-graph-coordinator.ts:276-444, packages/runtime/src/agent-graph-supervisor-wake.ts:280-405). Desktop now revokes catalog freshness after a failed refresh for the same live connection, so cached rows lose backgroundActivity and runningTurnIds until a successful retry restores authority (apps/desktop/src/main/session-local-service.ts:327-377). The two regressions cover both running and waiting_for_user, including retry recovery (apps/desktop/src/main/__tests__/session-local.test.ts:488-545). No storage schema or migration changes are introduced.
The rebase preserves the final three patches equivalently. Its first patch only adapts the current-main session collaboration test helper and advances the Runtime Host compatibility epoch from main's 189 to 190 (packages/runtime-host/src/protocol/index.ts:106).
Validation on the exact head:
- Clean
npm ciandnpm run build:testwith Node 24.18.1. - Runtime: 3,530 passed / 13 skipped; Desktop: 2,761/2,761; UI: 660/660.
- Runtime Host: 2,124 passed / 19 skipped / 1 failed. The sole managed-sandbox boundary failure was reproduced on exact current main at the same file, line, and assertion, so it is not attributable to this PR.
- Focused activity-path suites: Runtime graph/supervisor 49/49; Desktop Owner/Guest/UI 167/167; relevant Runtime Host paths 180 passed with the same baseline sandbox failure.
- Typecheck, lint, format, ASF headers, protocol epoch tests and guard, renderer architecture, renderer build/stale check, Windows test inventory, E2E budget,
git diff --check, and hostedtestpassed. - Current main is
87fc9f69cd11648f31048c1b633bb813aca5f515; the PR is 4 commits ahead and 0 behind, and the merge tree is conflict-free.
Residual scope: I did not run packaged Electron E2E, native Windows/macOS validation, or a live cross-machine Guest transport test.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
40567bd to
cf2ab03
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Re-review result
Exact head: cf2ab03302db899788c8453ff2cbe726c4be2cc9
I found no substantiated P0-P3 issue on this head. The previously reported stale Owner activity issue remains fixed.
The session catalog keeps background activity authoritative only while the current connection owns a fresh catalog generation; after a same-connection refresh failure, it revokes freshness and emits the live-to-cached transition once (apps/desktop/src/main/session-local-service.ts:270-297,328-377). Cached rows therefore clear backgroundActivity and runningTurnIds. The regressions cover both running and waiting_for_user, repeated failure without a notification loop, retained disk history, and successful retry recovery (apps/desktop/src/main/__tests__/session-local.test.ts:488-545). Runtime Host compatibility is explicitly advanced to epoch 193 for the catalog projection (packages/runtime-host/src/protocol/index.ts:102-107). No storage schema or migration changes are introduced.
The final commit only aligns test fixtures with the current Runtime context; the production activity path and the Owner TTL fix remain intact. Validation on this exact head included a clean install and test build, Runtime 3,662 passed / 13 skipped, Desktop 2,842/2,842, UI 684/684, and focused activity-path tests 324 passed with one managed-sandbox boundary failure. Full Runtime Host produced 2,146 passed / 19 skipped / the same single failure; that unchanged assertion also fails on exact current main, so I did not attribute it to this PR. Typecheck, lint, format, ASF headers, protocol epoch, renderer architecture/build/staleness, Windows inventory, E2E budget, locale hygiene, and git diff --check passed. Hosted test is successful.
Current main is 18827d99c5704e185398b94d9d4e1ca666d2219f. The merge tree is conflict-free, and a synthetic current-main merge completed build:test plus 285 focused activity tests. The only main commit after the earlier synthetic validation changes unrelated interrupted-resume/UI files and has no path overlap with this PR.
Residual scope: I did not run packaged Electron E2E, native Windows/macOS validation, or a live cross-machine Guest transport test.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
Freshness update after the review above:
|
me2seeks
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.
Summary
The issue is real: in base, the sidebar running indicator derives solely from session.runningTurnIds (packages/ui/src/session-history-list.tsx:1687-1691 on main), so when a Swarm parent's own Turn completes after yielding to folded children, the indicator dies while child work continues. The fix direction is right: the Host is the only authority on graph/supervisor activity, and projecting a Host-owned backgroundActivity through the catalog protocol (epoch 192→193), with live/cached downgrade on desktop owner and guest paths, fixes it at the correct layer. However, the PR is conflicting with main and one new presentation-only read now gates a canonical-continuity path.
Findings
- [P1] PR metadata —
mergeable: "CONFLICTING", and the body's "Protocol compatibility guard 164 → 165" is stale: the diff actually bumpsRUNTIME_HOST_COMPATIBILITY_EPOCH192→193 (packages/runtime-host/src/protocol/index.ts:106). The branch predates current main around exactly the protocol-guard area this PR touches, so the claimed compat-guard verification no longer applies. Rebase and re-run the guard before merge. - [P2]
packages/runtime-host/src/server/execution-composition.ts:1083—await interactionActivity.refresh(sessionId)is inserted ahead ofcontinuityCoordinator.refreshCanonical(...)andsessionAdmission.detach(...)in the Interaction coordinator's canonical-refresh callback. The newSessionInteractionActivityProjection.refresh(session-interaction-activity.ts:47-49) performs two store reads; if either rejects, the callback now throws before canonical continuity refresh and admission detach run — a failure mode that did not exist on this path before, introduced by a presentation-only projection. Failure-isolate it (catch + report) so sidebar state can never gate canonical refresh. - [P3]
packages/runtime/src/agent-graph-supervisor-wake.ts:331-332—notifyPermissionResponseclears#sessionAttention/#parkedWakeIdsbefore#runTracked, which immediately publishes (:523); the settle then re-publishes. Each permission response produces a transientwaiting_for_user → running → idle/waiting_for_userflip, and every transition fans outhostChanges.publishSessionCatalogto all clients. Suppress the intermediate publish.
Verdict
needs-changes — conflicting with main (stale epoch evidence in the body) and a non-isolated presentation read on the canonical-refresh path; core design and regression coverage are otherwise sound.
|
Thanks for the detailed review. We addressed P1 and P2 in P1 — Merge conflicts and outdated compatibility informationWe merged The resolution preserves both sets of changes: main’s live-run epoch/Host-generation fields and this PR’s Host-owned background-activity projection. Since the merged main already uses compatibility epoch 196, we advanced this PR to 197, rather than retaining the previous branch value of 193. We also updated the PR description to remove the stale 164 → 165 information and distinguish current validation from earlier results. The protocol compatibility guard passes against the merged main, and GitHub now reports the PR as conflict-free. P2 — Isolate presentation failures from canonical interaction processingAgreed. The newly added activity refresh could reject before canonical continuity refresh and WorkHub notification ran. On answer-processing paths, that could also prevent the subsequent application of an already-committed answer. We fixed this at the
The refresh remains awaited under the existing Session admission, preserving its ordering on the successful path. We did not move it into an untracked asynchronous operation or add a broad catch around canonical processing itself. Three new regression tests cover failures from each of the two pending-request stores, plus observer/diagnostic failures. They verify that canonical publication, WorkHub notification, and answer application continue, and cover cache invalidation, repeated failures, and recovery. All three fail against the pre-fix implementation and pass with the fix. P3 — Retain the intermediate
|
hqhq1025
left a comment
There was a problem hiding this comment.
Re-review result
Exact head: 01c614fe25177b8fa897300f066e7311e603534e
I found no substantiated P0-P3 issue on this head. The previous merge conflict and the presentation-read gating issue are resolved, and the earlier Owner catalog freshness fix remains intact.
The merge resolution preserves both current-main run ordering (runEpoch and runHostGeneration) and this PR's Host-owned backgroundActivity projection in the Session contract and catalog coordinator (packages/core/src/session.ts:407-430, packages/runtime-host/src/server/session-catalog-coordinator.ts). Compatibility is advanced to epoch 197 with the intervening epoch history retained (packages/runtime-host/src/protocol/index.ts:102-116).
The new interaction projection isolates both source-read failures and observer/diagnostic failures. A failed read invalidates cached counts to unknown, but cannot stop canonical continuity refresh, WorkHub notification, or answer application (packages/runtime-host/src/server/session-interaction-activity.ts:45-68, packages/runtime-host/src/server/execution-composition.ts:1067-1094). The new regressions exercise failures from both interaction sources, repeated failure, recovery, and throwing observers (packages/runtime-host/src/__tests__/interaction-coordinator.test.ts:209-315). I also checked the production call graph: these refreshes are reached through the per-Session admission coordinator, so the projection's lack of a separate async generation does not create a reachable stale-overwrite race in the current composition.
The Owner path still revokes catalog freshness after a failed current-connection refresh, clears cached execution activity, emits the live-to-cached transition once, and permits recovery (apps/desktop/src/main/session-local-service.ts:270-297,328-377). No storage schema or migration changes are introduced.
Validation on this exact head with Node 24.18.1:
- Clean install and
build:testpassed. - Runtime: 3,660 passed / 13 skipped; Desktop: 2,852/2,852; UI: 681/681.
- Runtime Host: 2,160 passed / 19 skipped / 1 failed. The sole managed Bash sandbox-boundary failure reproduced independently on exact base/main
542f04a4f328de7a49b96e5146f391fb923fa369at the same assertion, so I did not attribute it to this PR. - Focused current-head suites passed: Runtime Host activity/catalog/protocol 148/148, Runtime graph/supervisor 49/49, Desktop Owner/Guest/rail projection 134/134, and the production three-child Swarm activity scenario 1/1.
- Typecheck, lint, format, ASF headers, model metadata, renderer architecture, renderer production build/staleness, Windows test inventory, E2E budget, TUI copy, locale hygiene, and
git diff --checkpassed. - Hosted
testis successful. Current main remains542f04a4f328de7a49b96e5146f391fb923fa369; the PR is 6 commits ahead / 0 behind and the merge tree is conflict-free.
Residual scope: I did not run packaged Electron E2E, native Windows/macOS validation, or a live cross-machine Guest transport test.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Independent second review of head 01c614fe (a different model lineage from the parallel review). This head is the merge of the previously reviewed cf2ab033 with main 542f04a4. No P0/P1 found; one P2 is worth addressing.
What was checked:
- The conflict resolution with #5741 is correct.
backgroundActivitysits alongsiderunEpoch/runHostGeneration, argument order is right on both the get and list paths, and the compat epoch goes 196 → 197 with the comment chain intact. - The new failure isolation works. An interaction-read failure, or a throwing observer or diagnostic callback, no longer blocks the canonical answer commit or the WorkHub notification. The three new tests cover failure, repeated failure and recovery.
P2: the Owner catalog refresh has no progress guarantee under sustained invalidation (apps/desktop/src/main/session-local-service.ts:343, :366-370).
- Any invalidation that arrives during
listSessions()discards the whole result and issues another read. - Each invalidation also demotes rows to cached, which drops the running and background-activity indicators.
- In a Swarm with several delegated children emitting frequent events, and a full-catalog round trip longer than the gap between events (likely with a remote Host), every read is invalidated before it lands. The sidebar then never shows the running indicator, which is exactly during the delegated work this PR is meant to cover.
- The previous code saved the result regardless of invalidation. This is derived from the code; real event rates weren't measured.
- Consider saving a result that is fresher than the current cached one even if invalidated, and then re-reading. Alternatively, cap the discard-and-retry count.
P3 (non-blocking):
- Refreshes above the concurrency cap are silently dropped. The global "at most 2 catalog reads" cap (
session-local-service.ts:332) drops a refresh when three or more Owner partitions refresh together. That partition stays cached until something else triggers a read. - Some waiting states don't survive a Host restart (partly unverified). Two cases show the parent as idle first:
- a supervisor wake waiting on a permission (
agent-graph-supervisor-wake.ts:347-358only recovers retryable wakes); - the in-memory interaction counts, which are lost until the next interaction event.
- a supervisor wake waiting on a permission (
- Some "needs attention" states have no way out. Supervisor wake errors (
agent-graph-supervisor-wake.ts:316/339/481) and graph dispatch failures (stream-graph-coordinator.ts:284) keep the state until the next event, with no matching action in the UI. - The coordinator keeps a shadow copy of graph state.
stream-graph-coordinator.ts:177-192maintains a graph-state model with 8+ update sites, parallel to the read model's derivation, plus three layers of publish de-duplication. It's correct today but easy to drift. - Background-activity changes rely on an undocumented ordering assumption. They don't bump the revision or run epoch, so they aren't protected by #5741's ordering. Correctness relies on serialization in the Desktop main process, which isn't documented.
Tests run:
npm run build:testpasses.- Runtime and runtime-host suites: 236/237 pass. The one failure, the sandbox-boundary Bash test, also fails on an unrelated worktree on this machine, so it's environmental.
- Desktop: 168/168 pass.
- A temporary trace on the happy path shows running → idle with no idle or blocked flicker.
Not verified: renderer typecheck, lint and E2E locally (hosted CI is green); real Swarm event rates for the P2; child pending-interaction recovery after restart.
The AI-use disclosure and the Generated-by trailers (Codex; Maka gpt-6-astra) are consistent.
Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; key claims were checked against the code, but please verify before acting.
| .then((sessions) => { | ||
| if (!this.#current(target)) return; | ||
| if (!this.#currentCatalogConnection(target, connection)) return; | ||
| if (connection.invalidationVersion !== invalidationVersion) return; |
There was a problem hiding this comment.
P2: any invalidation during the read discards the whole result, and finally re-reads. Under a steady stream of Swarm child events, with a round trip longer than the gap between events, this never lands, so the sidebar stays cached and loses the running indicator during delegated work. Consider saving the result when it is fresher than the current cached one and then re-reading, or bounding the retries.
01c614f to
a889be9
Compare
|
Thanks for the detailed review. I reproduced the P2 and have addressed it in a889be974. Ordinary Host notifications now mark the Owner catalog as needing refresh without revoking the last successful observation from the current connection. A successful full-catalog read is published even when more notifications arrived during the read; those notifications coalesce into one trailing read. This allows progress under sustained invalidation and convergence once the notifications stop. It deliberately allows an observation to lag a concurrent Host change while the next refresh is pending. Connection/Host-generation changes, authority removal, and local-store mutation fences remain in place. Read failures still revoke live activity and notify the live-to-cached transition, including when a trailing retry is needed. The earlier failed-TTL-refresh fix is preserved. The regression test injects multiple invalidations during each of eight consecutive reads, checks that every successful observation is published, and checks that refreshing stops after the final clean read. It fails on the previous implementation. Tests also cover coalescing, failure notification, local deletion protection, connection replacement and TTL failure/recovery. For the five non-blocking P3 items, I would prefer to keep them outside this update:
I also rebased onto current Validation: 302 relevant Desktop, Runtime and Runtime Host tests passed on the final build. This update and reply were prepared with OpenAI Codex; the implementation commits include |
hqhq1025
left a comment
There was a problem hiding this comment.
Re-review result
Exact head: a889be974ef8004b4a6e1afea89622f0952e170a
I found no remaining P0-P3 issue on this head. The previously reported Owner catalog liveness P2 is fixed.
Host catalog changes now increment the live connection's invalidation generation without revoking its last successful observation (apps/desktop/src/main/session-local-service.ts:158-173). A successful in-flight read is published even when newer invalidations arrived, while the captured generation keeps that observation dirty and the finally path coalesces all intervening changes into one trailing refresh (apps/desktop/src/main/session-local-service.ts:275-303,335-385). Connection replacement and local-store revision fences still reject reads that could cross authority or restore a locally removed Session (apps/desktop/src/main/session-local-service.ts:307-333,347-356). Failed refreshes still revoke authority and notify consumers before a dirty trailing retry.
The new regressions cover successful, removed, and failed dirty observations, continuous invalidations, final convergence, and local deletion protection (apps/desktop/src/main/__tests__/session-local.test.ts:361-460,693-718). As a discrimination check, I temporarily restored the old "discard a result when its invalidation version changed" guard: the focused suite then failed three of these cases, including the continuous-invalidation progress assertion. After restoring the exact head, the focused suite passed 49/49.
Validation on this exact head with Node 24.18.1:
- Clean
npm ciandnpm run build:testpassed. - Desktop passed 2,884/2,884; the focused
session-localsuite passed 49/49. - Desktop typecheck, lint, format, ASF headers, model metadata, renderer architecture (112/112), renderer production build/staleness, Windows test inventory, E2E budget, and
git diff --checkpassed. - Hosted
testis successful. - Current main is
b39330436311f54c0512c06aabf0be4525bba645; the PR is 7 commits ahead / 2 behind and the merge tree is conflict-free. A synthetic merge with current main completed a fresh install,build:test, the focused suite 49/49, and the full Desktop suite 2,885/2,885.
The PR description still names revision 01c614fe2 and compatibility epoch 196 to 197, while the current series reaches epoch 199; please refresh that metadata before merge.
Residual scope: I did not run packaged Electron E2E, native Windows/macOS validation, or a live cross-machine Guest transport test.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
a889be9 to
88ff409
Compare
|
Thanks for taking another look. For context, these points have already been discussed in the earlier review rounds, and I’ve replied to them individually:
|
hqhq1025
left a comment
There was a problem hiding this comment.
The current head retains the Host-owned background activity projection, Owner catalog refresh handling, and the newly restored isolation of interaction-activity read and observer failures from canonical answer processing. I found no separate defect in those paths, but this revision is not merge-ready against current main.
[P1] main now uses compatibility epoch 200 for Agent Graph operator output, while this branch independently uses 200 for Session catalog background activity. A three-way merge conflicts in protocol/index.ts. Resolving the text conflict while keeping 200 would let peers with different wire contracts pass the same epoch handshake; the merge-result protocol guard would also reject an incompatible protocol change without an epoch advance. Rebase on current main, move the epoch beyond its current value, retain both compatibility notes, and rerun the merged guard and affected tests.
The hosted test run for this exact head succeeded, and local protocol-epoch guard tests passed (17/17). git merge-tree against current main fails at the epoch declaration. I did not rerun the full Desktop/Runtime suites, packaged Electron, native Windows/macOS, or cross-machine Guest transport.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| // Increment when the same protocol version no longer guarantees safe Client-Host | ||
| // interoperability. Mismatches are rejected before domain commands are admitted. | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 199 as const; | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 200 as const; |
There was a problem hiding this comment.
[P1] Current main already assigns epoch 200 to Agent Graph operator output, but this branch assigns 200 to a different strict catalog field. The merge conflicts here. Keeping 200 after conflict resolution would advertise incompatible wire contracts under the same handshake epoch. Please rebase and advance beyond the current base epoch, preserving both compatibility notes, then run the merge-result epoch guard.
88ff409 to
cae4a93
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Re-review of exact head cae4a93ec842f053422aad615708ff18bbba02e2.
What changed: The Host projects a per-root backgroundActivity (idle/running/waiting_for_user/blocked) from three sources: Agent Graph coordinator facts (schedule, claims, operator projections), supervisor wake delivery, and canonical pending interactions. It publishes the value through the Owner and Guest Session catalogs, so the sidebar keeps a parent Swarm session marked active after its own Turn yields to folded children. Owner and Guest desktop paths strip the field once they lose live authority. The compatibility epoch moves 202 -> 203. There are no storage schema or migration changes; all activity state is in process memory.
Scope checked:
- The branch is rebased directly on current
main1e80e3b88(0 behind) andgit merge-treeis clean.scripts/protocol-epoch-check.mjs --base origin/mainpasses (202 -> 203). - Prior findings:
- Fixed and still present after the rebase: the stale Owner activity after a failed TTL refresh, the merge conflict and epoch-200 collision with main, the presentation read gating canonical refresh (
session-interaction-activity.ts:45-63isolates read, observer and diagnostic failures), and the Owner refresh liveness under sustained invalidation (invalidationVersionplus a trailing refresh). - Still open: the earlier non-blocking P3s (transient flip on permission response, waiting-permission attention not recovered after a Host restart, unordered same-revision activity updates). I found nothing that raises their severity.
- Fixed and still present after the rebase: the stale Owner activity after a failed TTL refresh, the merge conflict and epoch-200 collision with main, the presentation read gating canonical refresh (
- I reviewed the activity derivation for delegated work and child sessions: epoch handover via
#currentDrivers/#activityEpochs, child-to-driver mapping, the stop, close and failure paths, and the supervisor version fencing in#runTracked. I found no new correctness defect there.
Findings:
- P3: Epoch 203 is also claimed by open #5753, so whichever PR merges second must move to 204.
- P3: A trailing Owner refresh can be dropped by the existing two-read cap. The row then stays authoritative with stale activity.
- The PR description is stale: it still cites head
88ff409fand epoch 199 -> 200.
Validation (Node, clean npm ci + patches + build:test):
- Focused Runtime graph/supervisor, Runtime Host interaction/activity/catalog/protocol/UDS/WebSocket/execution-composition, and Desktop session-local/Guest mounts/rail/projection/IPC/preload/join-dialog/startup suites: 413/414 pass. The one failure is the production Bash sandbox-boundary test. Its test body is unchanged from main, and this environment cannot establish that sandbox.
- UI: 679/679 pass.
Not exercised: full Runtime, Runtime Host and Desktop suites; packaged Electron E2E; native Windows/macOS; live cross-machine Guest transport.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
| // Increment when the same protocol version no longer guarantees safe Client-Host | ||
| // interoperability. Mismatches are rejected before domain commands are admitted. | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 202 as const; | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 203 as const; |
There was a problem hiding this comment.
P3 (merge coordination): Epoch 203 is also claimed by open #5753 (fix(transcript): recover from oversized invocations, head 4ee3709d, whose line 108 reads // 203: Transcript omission notes ...). Current main is 202, so either PR is correct on its own. Once one lands, though, the other must move to 204 and keep both history lines. If a conflict resolver keeps 203 for both, peers with different strict catalog/transcript decoders would pass the same epoch handshake. Please coordinate the merge order, and rebase and bump if #5753 lands first.
| this.deps.changed(target.scope); | ||
| if (connection.invalidationVersion !== invalidationVersion) { | ||
| // Consume all changes received during this read with one request. | ||
| this.#refreshCatalog(target, connection); |
There was a problem hiding this comment.
P3: This trailing refresh still goes through the global #catalogTasks.size >= 2 cap (line 339), which can return without scheduling anything. Before this PR, a Host change deleted #catalogFresh, so a dropped refresh left the row cached with activity stripped. Now changed() only bumps invalidationVersion, and catalog() decides authoritative from fresh.connection === connection alone. A dropped trailing refresh therefore keeps the last observation (for example backgroundActivity: 'running' after children have finished) authoritative until another renderer catalog() call finds a free slot. This needs at least three Owner partitions refreshing at once, so it is rare. Consider exempting trailing refreshes from the cap or rescheduling them, or treat fresh.invalidationVersion !== connection.invalidationVersion as non-authoritative once the read has settled.
cae4a93 to
5fb3881
Compare
|
Thanks for the follow-up review. The refresh scheduling and activity ordering issues are addressed in Refresh concurrency and stale authority I reproduced the stale-authority concern with a third Owner partition while two catalog reads occupied the available slots. The existing trailing path releases its own slot before synchronously requesting another read, so that was not the dropped-request case I could reproduce. A new refresh for another partition could be dropped, leaving its last activity authoritative until another renderer read. Refreshes now enter a deduplicated FIFO queue while retaining the two-read limit. A released slot admits a waiting partition automatically, ahead of a busy partition's next trailing read. Reads fenced by a local store revision change also requeue; queued entries are retired with their connection or authority. This preserves successful observations during continuous invalidation and convergence after events stop. Regression tests cover the stale third partition without an extra renderer read, fairness against trailing work, bounded concurrency, local mutation fences, and connection replacement. Independent activity ordering This follow-up also addresses the previously deferred ordering issue. The Host now projects Both Owner and Guest paths carry the version and strip it with cached activity. Revisions are compared only within one Host generation; a restarted Host can start from a lower counter. Tests cover both response orders, independent metadata/Turn updates, cache downgrade, and generation changes. The seven new queue/order regression scenarios all fail against the implementation before this follow-up and pass after it. Compatibility and the other observations Current main has advanced to epoch 206. This branch now uses 208, preserving the main history and distinguishing the activity field from the subsequent ordering-field change. The guard passes against current main (206 → 208) and for the follow-up commit (207 → 208). The merge result will still need the guard against whatever main contains at merge time, including changes from #5753. I retained the permission-response Validation Full build, workspace typecheck, lint, format check, both Desktop/UI knip checks, and the protocol guard passed. Desktop passed 3,337 tests, excluding the two packaged Electron cryptography tests because the binary was unavailable; UI passed 713. All 261 targeted Runtime and Runtime Host tests passed, including the production three-child Swarm yield/resume/final-summary case. Three UDS startup tests timed out during parallel execution and passed when rerun in isolation. Renderer architecture, build freshness, Windows test inventory, E2E budget and diff checks passed. GUI E2E, packaged cross-platform checks, live cross-machine Guest transport and the full Runtime/Runtime Host suites were not run in this update. This implementation and reply were prepared with OpenAI Codex; the commit includes |
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review from the last reviewed head cae4a93e to 5fb3881b0cd7c2be366fc913ee854ab0f3020576.
Delta. Commits 1-7 were rebased from 1e80e3b8 to 49e01b15. Per git range-diff, the only changes to them are rebase context (the #isTurnBusy and groupingHeading neighbours) and the epoch renumbering. The new work is all in 5fb3881b0:
- Owner catalog refreshes go through a deduplicated FIFO queue (
#catalogPending) that keeps the two-read limit. Fenced reads are requeued. - The Host projects
backgroundActivityVersion(hostGenerationplus a Host-wide monotonic revision). The renderer merges activity separately from the Session revision and Turn epoch.
Prior findings
- P3, trailing Owner refresh dropped by the two-read limit (session-local-service.ts): fixed. A request that hits a full limit is now queued instead of dropped. Every task
finally, connection change and authority removal drains the queue. A fenced or invalidated read re-enters at the tail, so waiting partitions go first. Tests cover the third-partition, fairness, local-fence and connection-replacement cases. - P3, unordered same-revision activity updates: addressed by
backgroundActivityVersionandreconcileSummary. The comparison is scoped to one Host generation, and cached rows strip the version with the activity on the Owner, Guest and display paths. - P3, epoch 203 collision with #5753: obsolete. This PR no longer uses 203.
- P3s the author declined (transient
runningon a permission response, waiting-permission attention after a Host restart): still unchanged. The author's reasoning in the follow-up reply is acceptable for non-blocking items.
Findings
- P3: merge conflict and epoch collision; the branch must be rebased before it can land.
git merge-tree --write-tree origin/mainreports a content conflict inpackages/runtime-host/src/protocol/index.ts, and GitHub shows CONFLICTING/DIRTY. The PR claims two epochs, 207 (activity field) and 208 (activity version). Current main is already at 208 from #5709, and open #3700 also claims 207.scripts/protocol-epoch-check.mjs --base origin/mainfails with "still 208, the current base parent's value". 209 (#5548) and 210 (#5969) are claimed by open PRs, and 211 is suggested for #5980. Neither field is on main yet, so the two bumps can collapse into one: rebase onto main and take the next free epoch (212 if the others hold), with one history line covering bothbackgroundActivityandbackgroundActivityVersion. - P3 (unreproduced): a fenced read can be starved across partitions. The fence at session-local-service.ts:361/369 compares
SessionLocalStore.revision, and every partition'ssaveCatalogbumps that one global counter. Suppose one partition gets frequent Host invalidations and has fast reads, and another has slower reads. Each successful save from the first partition fences the slower read in flight, which then requeues. The slow partition can keep its last authoritative observation, possibly a stalerunning, until the busy partition quiets down. The new requeue is an improvement: atcae4a93ethe fenced read was dropped. The residual comes from the fence being global when the hazard it guards (a local create/remove racing a read) is per partition. A per-partition revision would remove this. I did not reproduce it. - P3 (observation): the activity revision is Host-wide and visible to Guests.
SessionBackgroundActivityProjection.#revisionadvances on every activity transition of any Session. It is exported inSharedSessionCatalogProjection, so a Guest sees the version and can infer the rate of activity changes in Sessions not shared with it. The leak is small, but a per-Session counter (or one kept only while a Session is non-idle) would avoid it. - The PR description is still stale. It cites head
88ff409fand epoch 199 -> 200.
CI and mergeability. The test check passes on 5fb3881b. Mergeability is CONFLICTING (see the epoch finding).
Validation. Clean npm ci --ignore-scripts, dependency patches and build:test on 5fb3881b. Focused suites:
- Desktop
session-local,session-catalog-*and Guest mounts: 140/140 pass. - Runtime Host
session-catalog-protocol,session-background-activity,session-catalog-coordinator,authenticated-websocketandsession-catalog-two-client-uds: 140/140 pass.
Not run: full suites, UI, Electron E2E, Windows/macOS, live Guest transport.
| // Increment when the same protocol version no longer guarantees safe Client-Host | ||
| // interoperability. Mismatches are rejected before domain commands are admitted. | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 206 as const; | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 208 as const; |
There was a problem hiding this comment.
P3: This conflicts with main, which is already at 208 (#5709), and open #3700 claims 207. protocol-epoch-check --base origin/main fails. Neither field has landed, so please collapse 207 and 208 into one bump at the next free epoch after rebasing. 209, 210 and 211 are claimed or suggested by #5548, #5969 and #5980, so that is 212 if those hold. Keep one history line covering both backgroundActivity and backgroundActivityVersion.
| if (!this.#currentCatalogConnection(target, connection)) return; | ||
| // A late catalog cannot erase a Session created/removed while it read. | ||
| if (this.store.revision !== revision) return; | ||
| if (this.store.revision !== revision) { |
There was a problem hiding this comment.
P3 (unreproduced): store.revision is global, and every partition's saveCatalog bumps it. A partition with frequent invalidations and fast reads can therefore keep fencing a slower partition's read in flight. The slow partition requeues each time and keeps its last authoritative activity until the busy one goes quiet. The requeue is already better than dropping the read. A per-partition revision would scope the fence to the local create/remove race it is meant to guard.
| if (previous === next) return; | ||
| // One Host-wide clock orders observations even after an idle Session is | ||
| // removed from the deduplication map, without retaining per-Session clocks. | ||
| this.#revision += 1; |
There was a problem hiding this comment.
P3 (observation): This counter is Host-wide and is published in the shared (Guest) catalog projection. A Guest can therefore watch it advance for activity changes in Sessions it cannot see. The leak is small. A per-Session revision, or one kept only while a Session is non-idle, would avoid it.
Project graph and supervisor wake activity into session catalogs so the session list stays active after the parent yields and until its summary finishes. Preserve logical Turn completion and pending interaction states. Invalidate cached activity across Owner and Guest reconnects, retry failed Guest catalog refreshes, and provide a manual recovery action. Cover graph lifecycle, interaction, reconnect, sidebar, and two-client protocol behavior. Generated-by: Codex
Surface failed graph projections after child execution settles. Fence Owner catalog responses against same-connection invalidations and issue a trailing read for newer state. Add regression coverage for terminal projection failures and superseded Owner catalog results. Validated with 304 related tests, workspace builds, Desktop typechecking, and targeted lint. Generated-by: Codex
Update remote catalog assertions for the Host-owned idle projection on created and renamed Sessions. Generated-by: Codex
Downgrade failed same-connection TTL refreshes to cached state and notify consumers once. Cover running and waiting activity, repeated failures, and successful recovery. Generated-by: Codex
Treat Host catalog notifications as refresh demand without revoking a live connection's last successful observation. Publish successful in-flight reads and coalesce intervening notifications into one trailing read, so sustained activity cannot indefinitely suppress catalog progress. Retain connection, authority, and local store mutation fences. Publish failed-refresh authority loss before scheduling a trailing retry. Cover continuous invalidations, convergence, failure notification, and local deletion protection. Generated-by: Codex
Retain the activity projection failure isolation and its regression tests from the previous merge commit when replaying the branch on current main. Keep the supervisor coordinator-failure test independent of exhausted provider retries and retain the reviewed catalog field layout. Generated-by: Maka (gpt-6-astra) Generated-by: Codex
Queue Owner catalog refreshes fairly behind the existing two-read limit, retry observations fenced by local mutations, and retire queued reads with their connection or authority. Version Host background activity independently of Session revisions and Turn epochs. Preserve the newer activity across list and targeted-row races without discarding newer durable metadata or live Turn state, and strip activity versions from cached projections. Advance compatibility epoch to 208 and cover queue admission, ordering and Host restart. Generated-by: Codex
Fence Owner catalog observations with the target partition's mutation revision, preserving local create/remove races without letting a fast partition keep discarding a slower partition's successful reads. Retain the existing global revision for transcript ordering and keep partition clocks monotonic across purge and authority replacement. Give each Session its own activity observation revision so shared queries cannot reveal activity changes in private Sessions through this field. Keep idle clocks for the Host generation to prevent older running responses from outranking completed activity. Cover sustained cross-partition invalidation, mutation isolation, idle clock continuity and actual Guest shared queries. The three behavioral regressions fail with the pre-fix production implementations. Generated-by: Codex
5fb3881 to
fa11c8f
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review from our last reviewed head 5fb3881b to fa11c8f1c02bcf7d5ade0af3827847ac8f451da7.
Delta. Commits 1-8 were rebased from 49e01b15 onto current main 74094c63. Per git range-diff, the only change to them is the epoch rework: the two bumps (207 and 208) are now one bump to 212, with one history line covering both backgroundActivity and backgroundActivityVersion. The new work is all in fa11c8f1c:
- The Owner catalog fence now uses a per-partition mutation revision (
DesktopSessionLocalStore.partitionRevision). The counter is advanced byenqueue/saveSession/removeSession/purge, is kept after purge, and the global revision remains for transcript ordering. SessionBackgroundActivityProjectionkeeps one activity clock per Session for the life of the Host generation, replacing the Host-wide counter.
Prior findings
- P3, epoch collision and merge conflict: fixed. Current main is at 208. Of the open PRs, #3700 claims 207, #5548 claims 209 and #5969 claims 210. The #5980 head currently says 205, and 211 is suggested there. This PR's 212 does not collide with any of them.
scripts/protocol-epoch-check.mjs --base origin/mainpasses ("208 -> 212").git merge-treeagainst main is clean, and GitHub reports MERGEABLE. - P3, cross-partition starvation of fenced reads: fixed.
saveCatalogadvances revisions only throughsaveSession/removeSessionon its own partition, so a busy partition can no longer fence another partition's read. Same-partition create/remove races are still fenced, and keeping the counter after purge stops an in-flight read from matching an earlier revision after an authority switch. I confirmed that the new test "continuous fast Owner invalidations cannot fence a slower partition catalog" fails when the old global fence is restored. - P3, Host-wide activity revision visible to Guests: fixed. The version a Guest sees now advances only on transitions in its own Session. The renderer compares versions only between successive states of the same Session row, so per-Session clocks keep the ordering guarantee. Keeping idle clocks for the generation costs one integer per Session that has transitioned, which is acceptable. When I restored the Host-wide counter, both new tests failed: "activity versions reveal only their own Session while idle and running" and "Guest shared queries do not expose activity transitions in private Sessions".
- P3s the author declined earlier (transient
runningon a permission response, waiting-permission attention after a Host restart): unchanged and still acceptable as non-blocking.
Findings
- P3 (process): the PR description is stale. It still cites head
5fb3881b0and base49e01b15, and it says the epoch moves "from main's 206 to 208". It should say 208 -> 212 on the current base. Please also note in the description that the epoch guard must be rerun if #3700, #5548, #5969 or #5980 lands first.
I found no new defects in this delta.
CI and mergeability. The test check on fa11c8f1 was still pending when I checked. GitHub reports MERGEABLE, with merge state BLOCKED because review is required.
Validation. Clean npm ci --ignore-scripts, dependency patches and build:test on fa11c8f1, and the epoch guard against origin/main passes. Focused suites:
- Desktop
session-local,session-catalog-*and Guest mounts: 119/119 pass. - Runtime Host
session-catalog-protocol,session-background-activity,session-catalog-coordinator,authenticated-websocketandsession-catalog-two-client-uds: 142/142 pass.
I also restored the old global fence and the old Host-wide clock in the compiled output. Each regression test then fails as intended.
Not run: full suites, UI, Electron E2E, Windows/macOS, live Guest transport.
|
Thanks for the follow-up review. All actionable findings in this review have now been addressed. |
Summary
Keep a parent Swarm session's sidebar indicator active after its own Turn yields to folded children, through supervisor work and final summary generation. Show attention when child work needs input or is blocked, and clear activity when work settles.
Project Host-owned activity through Owner and Guest catalogs. Owner refreshes use a deduplicated FIFO queue with the existing two-read limit and partition-local mutation fences, so a fast Host cannot keep discarding another Host's successful observations. Activity versions are per Session and Host generation, merge independently of Session revisions and Turn epochs, and retain their clocks through idle. Cached rows discard both activity and its ordering authority. Presentation failures remain isolated from canonical answer processing.
Fixes #5334
Verification
Head:
fa11c8f1c, rebased onto officialmainat74094c632. Local toolchain: Node 24.18.0 and npm 11.16.0.session.shared.queryisolation. Coverage also checks mutation fences and monotonic clocks through idle, purge, and authority replacement.GUI E2E, packaged cross-platform checks, live cross-machine Guest transport, and the full Runtime/Runtime Host suites were not run. These are local results; hosted CI for this head must be checked before merge.
The existing component screenshots below show a completed parent Turn with three running, folded children. They were captured in an earlier revision: the before case uses the previous catalog shape; the after case receives Host-owned background activity.
Breaking change
Advance Runtime Host compatibility from main's 208 to 212. One history entry covers
backgroundActivityandbackgroundActivityVersion; main's usage-query compatibility note is retained. Older closed catalog decoders reject the new fields, so Hosts and Clients need matching epochs. Rerun the guard against the merge-time main.Each Session retains an activity counter for the current Host generation once its activity changes, including while idle. This prevents a late running response from outranking newer idle activity. The new field no longer exposes other Sessions' activity transitions; the existing Host-wide catalog-event revision remains unchanged.
AI use
Tool(s) and scope: Codex — implementation, reviewer follow-ups, rebase integration, regression tests, validation, and PR preparation. Maka (gpt-6-astra) — earlier integration, presentation-failure isolation, regression tests, and validation. Retain the
Generated-byattribution when squash-merging.Checklist
Does this PR entail a change in behavior?