fix(desktop): invalidate artifact previews after deletion - #5394
SummerC0zyR0ck wants to merge 8 commits into
Conversation
8e384e7 to
136f20b
Compare
me2seeks
left a comment
me2seeks
left a comment
There was a problem hiding this comment.
Reviewed exact head 136f20b15d83474d9f973fc267e3fcceaf8a4058. The deletion publishers, subscription scoping, and preview quotas are internally consistent, but the transient change-feed path still leaves deleted previews readable across a reconnect. One blocking P2 is recorded inline.
| const unsubscribeSessionCatalogChanges = client.subscribeSessionCatalogChanges( | ||
| ({ sessionId }) => emitTargetSessionsChanged("updated", sessionId), | ||
| ); | ||
| const unsubscribeArtifactChanges = client.subscribeArtifactChanges((frame) => { |
There was a problem hiding this comment.
P2 — A deletion missed while reconnecting leaves the deleted preview live for the rest of its TTL. artifact.changed is a transient frame with no revision or replay, and RuntimeHostReconnectingConnection only rebinds this listener to the replacement connection. The existing preview scope is not closed when availability is lost. A reachable sequence is: Desktop prepares an HTML preview; its remote/SSH/WSL Host connection drops; the still-running Host deletes the Artifact through another Client, Deep Research rollback, or Session purge; the invalidation is emitted while no Desktop subscription exists; Desktop reconnects and receives only future frames. The local preview server therefore keeps serving the deleted snapshot for up to 30 minutes. The new reconnect test itself establishes the non-replay behavior by forwarding only frames emitted by the replacement connection, so the PR's deletion guarantee does not hold across a connection gap.
There was a problem hiding this comment.
Thanks for the careful review, @me2seeks — the concern is fair, and it sent us back through the Desktop connection model in detail. Here is what we found, and where we would value your guidance.
On the Desktop, the reconnecting path you described does not appear to exist. RuntimeHostReconnectingConnection is constructed only by the CLI/TUI clients; no Desktop (main-process) code path builds one. So the "listener is rebound to the replacement connection while the old preview scope stays open" mechanism does not apply to the Desktop.
For the connections the Desktop does use:
- libp2p-direct peer: the Host reuses the same connection session across a resume — peer-listener.ts handles the resume branch and returns without calling accept again — so the change-feed subscription is never dropped, and outbound bytes are buffered and replayed (2 MiB window, 30 s recovery). We already have tests for both the replay (resumable-peer-stream: "one-way blackhole triggers automatic recovery and preserves the pending read", "real TCP replacement preserves one Host dispatcher…") and the session reuse (peer-listener: "…resume spends no slot").
- Non-resumable transports (WSL pipe, local transport, SSH/tls/plaintext websockets): a drop closes the RuntimeHostConnection; the candidate tears down and the existing teardown calls ManagedArtifactPreview.closeScope, releasing every lease for the scope. Covered by runtime-host-desktop-candidate: "tears down the whole candidate when the Host connection closes".
- If peer recovery exceeds 30 s or the send window, the stream closes and the connection closes too — the same closeScope path.
So we could not construct a Desktop sequence where a deletion is published while no subscription exists and the connection stays open. We removed the availability-based hook we had tried, because it only applies to reconnecting connections and would never fire here.
We may well be missing a path. If you have a specific one in mind — a transport, a mount, or a client we overlooked — we would be glad to hook the release to whatever signal actually fires there. Could you point us at it?
There was a problem hiding this comment.
P1 — A normal Desktop reconnect permanently disables artifact previews for that target.
The concrete path is the candidate cleanup, not RuntimeHostReconnectingConnection: DesktopRuntimeHostCandidateImpl calls disposeClientIpc when connection.closed settles; registerHostClientIpc then calls managedArtifactPreview.closeScope(scope.targetEpoch) (runtime-host-boot.ts:1913). closeScope adds that epoch to retiredScopes (managed-artifact-preview.ts:74 rejects every retired scope). However, createDesktopRuntimeHostCandidate derives scope.targetEpoch from ipcMain.epoch (runtime-host-desktop-candidate.ts:549), and the Desktop manager creates every replacement candidate with the same target.epoch (runtime-host-desktop-manager.ts:1135). The replacement therefore reuses an already-retired scope.
I reproduced this on 9bd1819f6142d477726f8a9b5760aa5325df00f9: prepare('same-epoch') → closeScope('same-epoch') → prepare('same-epoch') returns Error: Preview owner is closed. After a normal WSL/SSH/local reconnect, existing preview leases are released but every later artifact preview for that target stays unavailable.
There was a problem hiding this comment.
Thanks for tracing the concrete path — you are right. closeScope(targetEpoch) released the existing leases but also permanently retired an epoch that Desktop reuses across replacement candidates.
I fixed this by reopening the scope only after the replacement candidate successfully registers. Teardown still retires the scope and closes all old leases, so requests cannot create previews during the reconnect gap.
The regression test now verifies the complete lifecycle: the old preview URL becomes unreachable after disconnect, and a replacement candidate using the same targetEpoch can create and serve a new preview.
There was a problem hiding this comment.
Hi @me2seeks — the P1 you found is fixed on the current head 6c444a5c.
Teardown still retires the scope and releases the old leases, and the replacement candidate reopens the scope only after it registers, so a normal reconnect can prepare previews again on the same target epoch.
The regression test now covers the full lifecycle: the old URL fails after the disconnect, and a replacement candidate on the same targetEpoch can prepare and serve a new preview.
Could you take another look at the current head when you have a moment? The required approval is the only thing left on my side.
517f3f8 to
9bd1819
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head e1aa3d3792447de41b354ceaa22baa24ea9c7126 against base 672d82731a638e45e3ec87022eabdcf70a5a90be.
The production Artifact preview lifecycle fix remains internally consistent. apps/desktop/src/main/managed-artifact-preview.ts:59-61 reopens only transiently retired scopes, while the global close state remains terminal. apps/desktop/src/main/runtime-host-boot.ts:1911-1925 opens the target epoch after candidate owner registration and closes the scope before candidate teardown completes. Together with the same-epoch successor ordering in packages/runtime-host/src/client/reconnect-lifecycle.ts:300-346, a reconnecting candidate can prepare a preview again without allowing the old connection to keep forwarding Artifact events.
The new head only adds two synchronization assertions in apps/desktop/e2e/side-chat-followups.spec.ts:115-123,179-186. They wait for the queue's draggable handles before editing or injecting the disconnect gap; packages/ui/src/composer-message-queue.tsx:159-165,207-220 confirms that this selector represents queued, reorderable entries. The exact-head required test run 35183171316 / job 105079524815 passed, including affected tests, Runtime Host, Desktop E2E, Browser WebContentsView, WorkHub browser smoke, Alignment audit, and CLI release candidate validation.
I found no P0-P3 correctness issue in this exact head. Local build/typecheck/test and real Host/Electron reconnect smoke were not independently run because this worktree lacks the complete toolchain; the hosted run is the available execution evidence.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
e1aa3d3 to
8903dfd
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head 8903dfd73dd3187c90c0f7d33a0d6c1bc23f518a against base 672d82731a638e45e3ec87022eabdcf70a5a90be.
Technical result: GO. I found no P0-P3 correctness, authorization, concurrency, or lifecycle issue in this exact head.
The production Artifact preview lifecycle is now coherent. apps/desktop/src/main/managed-artifact-preview.ts:59-61 removes only a transiently retired scope marker, while :78,184-202 continues to reject closed scopes and release every lease/server. apps/desktop/src/main/runtime-host-boot.ts:1640-1646 maps Host deleted frames to per-Artifact revoke and session_purged frames to per-Session release; :1911-1917 opens the target epoch after registration and closes it during candidate disposal. apps/desktop/src/main/runtime-host-artifacts-ipc-main.ts:82-90 also keeps the direct-delete revoke independent of feed delivery.
The earlier same-epoch reconnect issue is fixed by ordering, not by leaving stale previews alive. apps/desktop/src/main/runtime-host-desktop-candidate.ts:313-364 makes candidate closure wait for client-IPC disposal, and packages/runtime-host/src/client/reconnect-lifecycle.ts:300-324,349-366 installs a successor only after the previous candidate is closed. The candidate test at apps/desktop/src/main/__tests__/runtime-host-desktop-candidate.test.ts:503-562 verifies old URL failure and successful preview preparation on a successor using the same target epoch.
I also checked the deletion publishers and protocol boundary: successful user deletion (packages/runtime-host/src/server/artifact-coordinator.ts:471-502), successful Deep Research deletion (deep-research-coordinator.ts:94-103), Session purge (session-sidecar-purge.ts:32-50), strict frame decoding (packages/runtime-host/src/protocol/artifact-change.ts:23-62), and permission/session routing (connection-session.ts:351-409, host-change-feed.ts:161-193). Desktop candidates use raw RuntimeHostConnection (apps/desktop/src/main/runtime-host-desktop-candidate.ts:367-409,467-523), not the CLI/TUI reconnecting wrapper. For the Desktop transports, libp2p-direct retains bounded unacknowledged writes during path recovery (packages/runtime-host/src/transport/resumable-peer-stream.ts:192-224,392-410,433-464); non-resumable connection loss closes the candidate and releases its scope.
The exact-head required test run 35192164606 / job 105106945341 passed, including build, typecheck, affected workspace tests, Runtime Host tests, Desktop E2E, Browser WebContentsView, WorkHub browser smoke, Alignment audit, and CLI release-candidate validation. The PR diff is 32 files (+845/-18); the last two commits only add queue-handle waits in apps/desktop/e2e/side-chat-followups.spec.ts:115-123,179-186.
Local build/typecheck/test and real Host/Electron reconnect smoke were not independently run because this worktree lacks complete node_modules, TypeScript, Vitest, and zod. The hosted exact-head run is the available execution evidence. This review is a technical assessment only and does not approve merging.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
1b8fa8f to
6c444a5
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head 6c444a5ca72fe6483e818784cff43cd2a775089a against base 0169d0731d476e70ad2afb6cd7fc97e5980120f8.
Technical result: GO. I found no P0-P3 correctness, authorization, concurrency, or lifecycle issue in this exact head.
The preview owner and invalidation paths are coherent. apps/desktop/src/main/managed-artifact-preview.ts:59-202 reopens transient scopes, bounds leases per Session and globally, checks ownership across asynchronous reads and HTTP readiness, and releases timers, servers, and listeners. apps/desktop/src/main/runtime-host-boot.ts:1641-1647,1912-1918 maps deleted to per-artifact revoke and session_purged to per-Session release, while direct deletion also revokes locally in runtime-host-artifacts-ipc-main.ts.
The earlier same-target-epoch reconnect issue is closed by lifecycle ordering. apps/desktop/src/main/runtime-host-desktop-candidate.ts:313-364 waits for IPC and connection cleanup, and packages/runtime-host/src/client/reconnect-lifecycle.ts:300-366 installs a successor only after the prior resource has closed. apps/desktop/src/main/__tests__/runtime-host-desktop-candidate.test.ts:503-562 verifies the old URL is unusable and a successor can prepare a new preview on the same target epoch.
I also checked successful deletion publishers and Session purge wiring (packages/runtime-host/src/server/artifact-coordinator.ts:471-502, deep-research-coordinator.ts:94-103, execution-composition.ts:936-945,1578-1586,2462-2501), strict frame decoding (packages/runtime-host/src/protocol/artifact-change.ts:23-61), and permission/session routing (connection-session.ts:351-409, host-change-feed.ts:161-193). The exact-head required test run 35300099814 / job 105460752806 passed, including build, typecheck, affected workspace tests, Runtime Host tests, Desktop E2E, Browser WebContentsView, WorkHub browser smoke, Alignment audit, and CLI release-candidate validation.
The PR diff is 32 files (+845/-18) against the merge-base. Local full build/typecheck/test and real Host/Electron reconnect smoke were not independently run because this worktree lacks the complete toolchain; the hosted exact-head run is the available execution evidence. This is a technical assessment only and does not approve merging.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
6c444a5 to
caeced8
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Exact-head review: caeced84bc1dd4d94be862686905f4961d475625
Outcome: code GO. I found no P0–P3 correctness, ownership, lifecycle, or concurrency finding in this exact head.
The 32-file change (+845/-18) was reviewed across Desktop Artifact preview invalidation, Runtime Host artifact-change protocol/feed/client forwarding, candidate teardown/reconnect, deletion/session-purge callbacks, and the regression tests. In particular, managed-artifact-preview.ts:59-202 owns scope/lease/revoke/release cleanup; runtime-host-boot.ts:1641-1647,1912-1927 consumes deletion/purge events and closes the target scope during candidate cleanup; and runtime-host-desktop-candidate.ts:313-364 plus runtime-host-desktop-candidate.test.ts:503-562 cover teardown completion followed by same-epoch preview reuse. The Host protocol/feed and current-connection listener replacement paths were also checked for stale-event and permission-routing issues.
The exact-head required test check is successful (run 35334657597, job 105566598847). The merge tree is clean and git diff --check passes. No schema or migration changes are included.
Limitations: the available worktree lacks the complete local dependency/toolchain set, so I did not independently run the full local build/typecheck/dist suite or a real Electron/Runtime Host reconnect smoke test. The detailed evidence report is available as reports/pr5394-caeced84-review.md in the review workspace.
This comment is an automated review and does not replace independent human review.
Automated review notice
f6876ab to
ac37976
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
I reviewed the current head ac37976123abfdc7c240f31c498323d933bc0d77 and found no substantiated P0–P3 issue in the changed paths. The branch is not merge-ready: current main conflicts in packages/runtime-host/src/protocol/index.ts (this branch has compatibility epoch 184; main has advanced to 189). Please resolve the conflict and rerun current-head checks before reconsidering the PR.
This change adds Host artifact deletion/session-purge notifications and scoped subscriptions (packages/runtime-host/src/server/host-change-feed.ts:91, packages/runtime-host/src/server/connection-session.ts:369); Desktop consumes those notifications to revoke preview leases and opens/closes the preview scope with its Host candidate (apps/desktop/src/main/runtime-host-boot.ts:1562, apps/desktop/src/main/runtime-host-boot.ts:1840). It also bounds previews per session and globally (apps/desktop/src/main/managed-artifact-preview.ts:79). I checked delete/purge callbacks, guest session filtering, reconnect subscription cleanup, lease teardown, and adjacent tests. There is no database schema migration.
The current-head test check passes, but all earlier reviews target older commits. I could not rerun tests locally (Node 18 and no dependencies) or exercise a real Electron/Host disconnect. This is not a merge approval.
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.
ac37976 to
42b1057
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current rebased head. The Host publishes deletion/purge invalidations after the corresponding Artifact operation succeeds, routes them by the Client’s Artifact/session authority, and Desktop revokes the matching preview leases. The reconnect path closes old leases and reopens the same target scope for the replacement candidate; the latest conflict fix narrows session-scope retirement to session-catalog frames. The protocol epoch is 197, one above the current main’s 196. I found no substantiated P0–P3 issue in this head. The current test and windows_acp checks pass and the latest fetched main merges cleanly. I did not run a packaged Electron/Host disconnect smoke or the full suite locally; this is not a merge approval.
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
Astro-Han
left a comment
There was a problem hiding this comment.
Independent second review of head 42b10577 (a different model lineage from the parallel review). The deletion and purge wiring, reconnect ordering (the new candidate's openScope always follows the old closeScope), revoked-preview serving (the lease is reserved before the async read and later stages fail with 404) and protocol decoding all look right. No path traversal or cross-session access was found.
P2: revoking one guest drops live updates for every other guest on the same Session (packages/runtime-host/src/server/host-change-feed.ts:171-179).
- Why. The new second
ifin the scope-close loop matches onmask.artifact.sessionId === frame.sessionIdonly, with no principal check, and deletes the connection's whole subscription. Sinceconnection-session.ts:369-376gives everysession_guestconnection anartifact: { sessionId }mask, revoking guest-1 on Session s1 also deletes guest-2's subscription. - Effect. guest-2 stays connected but stops receiving
session.catalog.changed, so the shared view goes stale until reconnect. - Reproduced. A minimal script against the PR's feed gives guest-2 1 frame instead of 3.
- Why the existing test passes. The "other guest still receives 3 frames" contract in
host-change-feed.test.tsonly holds because its fixture has no artifact subscription. - Suggested fix. Only the owner Desktop consumes
artifact.changed(runtime-host-boot.ts:1558), so simplest is to not give guests an artifact subscription at all and drop thisif. Otherwise add the same principal check as the branch above.
P3:
- Guests learn artifact ids they can't read. Artifact events are routed by sessionId only (
host-change-feed.ts:186-190), not byisArtifactSharedSessionReadable, so guests receive ids and deletion times of artifacts they can't read. This goes away with the P2 fix. - A partial purge publishes no invalidation.
session-sidecar-purge.ts:44only publishessession_purgedwhen the purge fully succeeds, so on partial failure prepared previews stay reachable until the 30-minute TTL. Revocation is idempotent; consider publishing regardless. - The global preview cap can evict other sessions' previews. The 64-preview cap (
managed-artifact-preview.ts:85-88) silently evicts previews that other sessions are using, and eviction runs before the abort check, so an already-cancelled request can still evict. This is also scope creep relative to "invalidate after deletion". - Test gaps.
- The boot wiring (revoke/
releaseSessionon events,openScope) is untested: the candidate tests use their ownregisterClientIpc, so deleting those lines in boot still passes. - The multi-guest + artifact subscription + revoke scenario has no test.
- The boot wiring (revoke/
- Epoch coordination. #5709 also claims epoch 197. Whichever merges second must move to 198, or two builds that both say 197 will accept each other while exchanging different frames. This is a merge-coordination note, not a defect in this PR.
Verified: the 8 touched runtime-host test files, and desktop managed-artifact-preview + runtime-host-desktop-candidate 37/37, pass locally.
Not verified: the CLI stub tests, the full suite, typecheck/lint, or Electron end to end.
Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; the P2 was checked against the code, but please verify before acting.
| closeScopeFor !== undefined && | ||
| frame.kind === 'session.catalog.changed' && | ||
| subscription.mask.artifact !== true && | ||
| subscription.mask.artifact?.sessionId === frame.sessionId && |
There was a problem hiding this comment.
P2: this branch has no principal check. Every guest on the session carries an artifact: { sessionId } mask (connection-session.ts:369-376), so revoking one guest deletes the other guests' whole subscriptions and they stop receiving session.catalog.changed. Consider not giving guests an artifact subscription (only the owner Desktop consumes artifact.changed), or matching the principal as the branch above does.
There was a problem hiding this comment.
Thanks for the detailed review and for reproducing these cases. I’ve addressed all the reported P2/P3 issues:
- Guest connections no longer subscribe to
artifact.changed, so revoking one guest cannot remove other guests’ subscriptions. - Artifact invalidations are now restricted to the owning Desktop connection.
- Session purge publishes an idempotent invalidation even when cleanup partially fails.
- Removed the cross-session preview eviction behavior that was outside the scope of this fix.
- Added regression coverage for guest routing, partial purge, preview lifecycle, and reconnect behavior.
The full build succeeded, and all 162 targeted Runtime Host/Desktop tests passed. Thanks again for catching these issues.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit 855ee45. The guest-scope regression is addressed: guest connections no longer subscribe to Artifact invalidations (connection-session.ts:369-379), and host-change-feed.ts:160-169 only removes the matching principal's catalog subscription. The revised multi-guest fixture would fail the previous implementation. Session-purge invalidation also runs after partial Artifact purge failure (session-sidecar-purge.ts:39-51).
One new resource-bound finding is inline. The PR currently conflicts with fresh main and has no current-head checks, so it is not merge-ready. I inspected the incremental diff and related subscription/preview paths; I did not run local tests or packaged Desktop/Host scenarios because this checkout has no installed dependencies.
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.
| endpoints.push(await service.prepare('h', client(`preview-${index}`), `s${index}`, 'a1')); | ||
| } | ||
| const replacement = await service.prepare('h', client('replacement'), 's64', 'a1'); | ||
| assert.equal(await (await fetch(endpoints[0]!.url)).text(), 'preview-0'); |
There was a problem hiding this comment.
[P2] Keep an aggregate bound on retained previews. This new assertion explicitly keeps the first lease alive after creating a 65th session-scoped preview, while ManagedArtifactPreview.prepare() now checks only 16 leases per session (managed-artifact-preview.ts:75-84). Each lease retains up to 8 MiB in its HTTP handler (:25,104-124) for 30 minutes (:27,160-163), and creates a separate listening server. Five sessions can now retain 80 maximum-sized previews (640 MiB); more sessions have no process-wide bound. The prior 64-lease eviction was imperfect for UX, but removing it without a replacement lets repeated preview creation exhaust Desktop memory/sockets. Please keep a global resource budget (with an admission/eviction policy that does not silently starve other sessions) and test that budget.
There was a problem hiding this comment.
Thanks for catching this. You’re right that the per-Session limit alone left aggregate preview resources unbounded.
I’ve added a global limit of 64 active previews and a 128 MiB aggregate content budget. In-flight preparations reserve capacity too; requests that exceed either limit are rejected without evicting or disrupting existing previews. The regression tests cover both limits and verify that failed preparations release their reservations.
The fix is pushed in f4bb8bc71. Thanks for the careful review!
855ee45 to
76cee97
Compare
me2seeks
left a comment
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
Invalidates artifact previews after deletion and rebalances lease limits: the per-session cap is now scoped per scope+sessionId (one Session can no longer starve every other for a full TTL), and a global backstop of 64 evicts the oldest lease instead of rejecting the newest — the header comment documents why reject-newest was the starvation vector. releaseSession bulk-releases a scope's leases, and retiredScopes is cleared on reopen. The issue (deleted artifacts keeping stale previews live until TTL, and global-cap starvation) is real; the eviction-order fix is the minimal correct shape. Broad test coverage across desktop candidate, CLI ACP/context/onboarding, artifact coordinator/protocol/two-client-UDS. CI test green.
Findings
- [P3] Oldest-lease eviction calls
void this.release(oldest)without awaiting — an unlucky interleaving could admit one lease above the cap while eviction is in flight. Bounded by 1 and self-healing, but if the cap is a hard security bound (file-handle pressure), await the eviction before admitting. - [P3] Deletion→invalidation propagation relies on the artifact coordinator noticing deletes; confirm a test pins preview invalidation specifically on delete (not just TTL expiry) end to end — that's the PR's headline behavior.
Verdict
merge-ready — starvation vector closed with eviction semantics documented; two P3 robustness notes.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
I reviewed the current head. The aggregate preview fix restores a 64-lease Desktop-wide limit and reserves each Artifact’s declared size against a 128 MiB budget before streaming (managed-artifact-preview.ts:27-28,79-107). Releasing a lease returns its reservation once (:198-208); the new tests cover rejecting the 65th preview without evicting existing sessions and rejecting concurrent 8 MiB reads above the byte budget, then reusing capacity after failures (managed-artifact-preview.test.ts:161-207). I also checked that the previous cross-guest subscription issue remains addressed by excluding guest Artifact subscriptions (connection-session.ts:369-379) and principal-scoped catalog removal (host-change-feed.ts:160-169). I found no substantiated new P0–P3 in these changed paths.
The focused preview tests passed 11/11 locally. The full local build:test did not complete because of Desktop UI type errors outside this PR’s changed files; I cannot establish their cause here. Current-head hosted test and windows_acp are successful; fresh-main merge-tree and diff check are clean. I did not run a packaged Desktop/Host end-to-end flow. This is a code review, not a merge approval.
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
Astro-Han
left a comment
There was a problem hiding this comment.
Re-review at f4bb8bc7. Verdict: no P0–P2; 4 × P3.
Global preview cap: correct (managed-artifact-preview.ts). There is a single Desktop-wide instance, which enforces 16 previews per Session, 64 overall (:85-87) and a 128 MiB reserved-bytes budget (:103-107).
- There is no await between each check and its reservation, so concurrent prepares cannot overshoot.
release(:198-202) returns capacity only while the lease is still registered. Every path goes through it: expiry,releaseUrl, failure or abort,revoke,releaseSession,closeScopeandclose. A revoke followed by a late failure is therefore a no-op, not a double return.- We ran a standalone stress harness over the class: 30 × 300 randomized mixed operations, plus 200 concurrent prepares. Reserved bytes always equaled the sum over live leases, stayed within 0–128 MiB, and never exceeded 64 leases. Exactly 64 of the 200 concurrent prepares succeeded. Capacity was fully returned after release, abort,
releaseSession,closeScopeandclose. - Rejecting when full, instead of evicting the oldest preview, also resolves the earlier eviction P3.
Cross-guest fix still holds.
- Session guests no longer subscribe to artifact frames (
connection-session.ts:372-380). - Closing a scope matches only the revoked principal again (
host-change-feed.ts:152-171). - Both are covered by regressions (
host-change-feed.test.ts:56-99,connection-session.test.ts:140-197).
P3s:
- Epoch coordination (inline): this PR sets 199 and documents 198 for #5709's unmerged change.
- The per-session
artifactmask filter (host-change-feed.ts:44,179-180) is now test-only; production subscribes all-or-nothing. Consider narrowing it to a boolean. - The
continue;athost-change-feed.ts:169is the last statement in the loop and has no effect. - The capacity-return tests are weak:
- The byte-budget test re-prepares only one small preview, so a partial refund would still pass.
- The global-cap test never proves that a 65th preview succeeds after a release.
- No test covers byte refunds on revoke, expiry or disconnect.
Not verified: repo test, typecheck and lint suites were not run locally (the worktree has no deps; hosted CI is green), and there was no Electron E2E.
Automated review (Claude lineage, posted from the Astro-Han account). No approval implied.
| // 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 = 197 as const; | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 199 as const; |
There was a problem hiding this comment.
P3: epoch coordination. Main is at 197. This PR jumps to 199 and adds a 198 comment describing Usage queries can filter by model call kind, which is open, unmerged #5709 (itself 197→198).
If this lands first, main documents a 198 change that isn't present, and #5709 conflicts on this line and must renumber to 200. Several other open PRs (#4751, #5458, #5732) also claim 198.
There is no compatibility risk, because mismatched peers are still rejected. However, either use 198 here and drop the #5709 line, or coordinate the merge order.
There was a problem hiding this comment.
Thanks for the thorough re-review and the additional cleanup suggestions. I addressed the actionable findings:
- Artifact subscriptions are now owner-only at the type and routing level.
- Removed the no-op continue.
- Strengthened capacity-release and lifecycle-ordering tests.
- Removed the stale, unmerged epoch comment.
For epoch coordination, the branch had already published epoch 199, and the repository’s protocol guard prevents moving compatibility epochs backward. Therefore the Artifact invalidation change now uses epoch 200, preserving monotonic compatibility semantics and avoiding reuse of an already-published value.
The updated head is ccc1d30. Runtime Host protocol tests pass 89/89, and the Desktop/Runtime Host regression suite is green.
Thanks for the detailed reconnect and lifecycle review. I verified the two reconnect scenarios against the current implementation. Candidate teardown closes the managed preview scope, which releases all existing preview leases even if an Artifact invalidation occurs during the connection gap. The same target epoch is explicitly reopened when the replacement candidate is registered, so previews remain usable after reconnect. The resource-cap behavior now rejects new previews when the global limit is reached rather than evicting an existing preview, so the asynchronous oldest-lease eviction race does not apply to the current implementation. |
Thanks for the follow-up review. The aggregate resource bound is now enforced by both a 64-preview Desktop-wide limit and a 128 MiB reserved-content budget. In-flight preparations reserve capacity before streaming, and all failure, cancellation, revoke, expiry, scope-close, and shutdown paths return the reservation exactly once. I added regression coverage proving that capacity is reusable after the global cap is reached and after failed large-preview preparations. The owner-only Artifact subscription behavior and Guest isolation remain covered as well. The latest changes are pushed in ccc1d30; the focused build and Runtime Host/Desktop regression tests pass. |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
I re-reviewed the current head, including the incremental changes since f4bb8bc7, and found no new substantiated P0–P3. The Desktop-wide lease and reserved-byte limits remain unchanged (managed-artifact-preview.ts:25-28,79-107,198-208). The updated tests now demonstrate that releasing one of 64 leases admits a replacement and that a failed batch of 16 × 8 MiB reservations admits a new full-size preview (managed-artifact-preview.test.ts:161-210). The Host change feed now uses only the owner-wide Artifact subscription (host-change-feed.ts:43-49,174-179) and removes the redundant loop continue; guest connections still do not receive an Artifact subscription (connection-session.ts:369-379). The compatibility epoch is 200 on this head (protocol/index.ts:105-111); it must be rechecked against main if another protocol-changing PR merges first.
I ran the focused preview and Host-change-feed suites locally: 11/11 and 3/3 passed. Current-head hosted test and windows_acp are successful, and fresh-main merge-tree/diff check are clean. I did not rerun the full Desktop build or packaged Desktop/Host end-to-end flow. This is not a merge approval.
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
Astro-Han
left a comment
There was a problem hiding this comment.
Incremental re-review at ccc1d30f, relative to f4bb8bc7 (5 files, +26/−28), which we previously found clean. Verdict: no P0–P2 in the delta. It addresses our earlier P3s:
- The per-session artifact mask is narrowed to owner-wide only.
- The no-op
continueis removed. - The capacity tests now re-admit after a 64-lease release and after full 16×8 MiB failure refunds.
The epoch is now 200 with only its own comment, so it no longer documents #5709's change. It skips 198 and 199 relative to main (197). That is harmless, since equality is all that matters, and avoids colliding with the open PRs claiming 198. Whichever protocol PR merges later should still re-check.
Automated review (Claude lineage, posted from the Astro-Han account). No approval implied.
ccc1d30 to
1e42277
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head against main 2f322055. The branch has been replayed onto main; git range-diff shows the preview/invalidation implementation and tests matching the previously reviewed series, with the substantive adaptation confined to the Runtime Host protocol epoch. Main keeps executor-readiness epoch 198, and this branch assigns artifact invalidation/guest routing epoch 200 (packages/runtime-host/src/protocol/index.ts:107-111). The owner-only Artifact feed and the Desktop's 64-lease/128 MiB bounds remain in the effective PR diff (packages/runtime-host/src/server/host-change-feed.ts:173-181, apps/desktop/src/main/managed-artifact-preview.ts:27-30,76-108). I found no new substantiated P0-P3 issue in this rebase. The protocol epoch guard and its 17 focused tests pass, current-head hosted test and windows_acp succeed, and the fresh-main merge tree and diff check are clean. I did not rerun the full Desktop or packaged Host/Electron scenarios. Epoch 200 is only valid against the reviewed main: if another protocol PR merges first, reallocate and re-review this head before merging.
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.
1e42277 to
83950b8
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 83950b820ba60e655f36e1960c09e9919728d077 against current main ed38ccbb6bac474b93e4517f2a12a30209c108d5. I found no new substantiated P0–P3 issue. The Host publishes invalidations after committed Artifact deletes and Session sidecar purges; the owner Desktop revokes matching preview leases, closes them on disconnect, and can reopen the scope after reconnect. Session guests do not receive Artifact identities. Preview admission is limited to 16 per Session, 64 Desktop-wide, and 128 MiB of declared Artifact bytes. The compatibility epoch advances from 199 to 200.
The replayed feature series is behaviorally unchanged from the previously reviewed head apart from aligning the epoch with main. A clean Node 24 install and build:test passed; 230 focused tests, Biome on all 30 changed files, ASF headers, app-shell hooks, protocol-epoch guard, diff check, and a fresh-main merge-tree passed. Current-head hosted test and windows_acp are green. I did not run a packaged Electron or native Windows/macOS end-to-end test.
Please update the PR description before handoff: it still describes guest subscriptions, oldest-lease eviction, and epoch 175→176, while this head uses owner-only routing, rejects when the global bound is full, and moves 199→200. The open #4751 also proposes epoch 200; whichever lands second must rebase and reallocate its epoch. A historical CHANGES_REQUESTED review at an older head still leaves GitHub's review decision blocked despite the later fix.
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.
83950b8 to
5813aea
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 5813aea9a8548ba05934d9c7ed6acde4ec7a0873 against fresh main 5ac266b1e67972f3781907583b0b089beedce947. I found no new substantiated P0–P3 issue. The feature series was replayed on top of the merged Agent Graph change; range-diff shows the preview invalidation, owner-only routing, reconnect lifecycle, quotas, and tests unchanged from the previously reviewed series. The new final commit reconciles the compatibility epoch from main's 200 to 202 and consolidates the Artifact-feed explanation. There is no schema migration.
A clean Node 24 install and build:test passed. The 230 focused tests, Biome on all 30 changed files, ASF headers, app-shell hooks, protocol-epoch guard, diff check, and fresh-main merge-tree passed; current-head hosted test and windows_acp are green. I did not run packaged Electron or native Windows/macOS end-to-end testing.
The PR description still describes guest Artifact subscriptions, oldest-lease eviction, and epoch 175→176, none of which matches this head. Please update it before handoff. GitHub also still reports CHANGES_REQUESTED/BLOCKED from a historical review, notwithstanding the later fix; that review state needs human handling.
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.
This branch has not been deployed
4 participants
Summary
Fixes #5341
Refs #5436
ManagedArtifactPreviewserves HTML Artifacts from in-memory snapshots behind a bounded TTL. Previously, preview leases were released only by the Desktopartifacts:deleteIPC path orcloseScope(targetEpoch). Deletions performed inside the Runtime Host did not notify the Desktop preview service, so deleted Artifact bytes could remain readable until the preview expired.This change makes Artifact deletion observable by the preview owner, preserves preview lifecycle across reconnects, and bounds preview resource usage.
Host-scoped Artifact invalidation
A new closed
artifact.changedHost frame carries one of:deletedwithsessionIdandartifactIdsession_purgedwithsessionIdThe frame is published only after the corresponding deletion or Artifact purge has committed:
HostArtifactCoordinatorpublishesdeletedonly for an actual committed delete.session_purgedwhen Artifact purge succeeds, even if another sidecar cleanup fails.The existing
HostChangeFeedroutes these frames only to subscribed Clients. Session Guests receive invalidations only for their shared Session. Artifact invalidations are never exposed across unrelated Sessions or to unauthorized Guests.Desktop preview lifecycle
Desktop now consumes Artifact invalidation frames through:
RuntimeHostConnectionDesktopRuntimeHostClientruntime-host-bootThe preview manager revokes individual Artifact leases for
deletedevents and releases all leases for a Session onsession_purged.The existing direct revoke in the
artifacts:deleteIPC handler remains in place so the local caller does not depend on asynchronous Host change-feed delivery.Reconnect handling
When the Runtime Host connection closes, the candidate-scoped preview endpoints are released. When a replacement candidate reconnects with the same target epoch, the preview scope is reopened before serving requests.
This prevents a reconnect from leaving the scope permanently retired while still ensuring that endpoints from the old Host connection are no longer reachable.
Preview resource bounds
Preview admission is bounded by:
(scope, sessionId)Reservations are made before asynchronous Artifact reads begin and are released on success, failure, cancellation, revocation, expiry, Session purge, scope close, or connection teardown.
When a limit is reached, the new request is rejected. Existing previews are not evicted.
Protocol compatibility
The
artifact.changedframe and its routing semantics are wire-visible protocol changes. After rebasing onto the currentmain, the Runtime Host compatibility epoch is202.Verification
The current head was rebased onto the latest
upstream/main.Verified locally:
npm run build190/190affected tests passedgit diff --checkpassedCoverage includes:
AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex investigated the defect, implemented the Runtime Host and Desktop changes, and authored the related regression tests.
Checklist
Does this PR entail a change in behavior?