fix(desktop): invalidate artifact previews after deletion by SummerC0zyR0ck · Pull Request #5394 · apache/maka

SummerC0zyR0ck · 2026-09-16T10:01:12Z

Summary

ManagedArtifactPreview serves HTML Artifacts from in-memory snapshots behind a bounded TTL. Previously, preview leases were released only by the Desktop artifacts:delete IPC path or closeScope(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 owning Desktop connection, preserves preview lifecycle across reconnects, and bounds preview resource usage.

Host-scoped Artifact invalidation

A new closed artifact.changed Host frame carries one of:

deleted with sessionId and artifactId
session_purged with sessionId

The frame is published only after the corresponding deletion or Artifact purge has committed:

HostArtifactCoordinator publishes deleted only for an actual committed delete.
Protected or missing Artifacts do not publish invalidation frames.
Session retirement publishes session_purged when Artifact purge succeeds, including partial-purge cases where another sidecar cleanup fails.

Artifact invalidations are routed only to the owning Desktop connection. Session Guests do not subscribe to artifact.changed, so they neither receive Artifact identities nor affect other Guests' catalog subscriptions.

Desktop preview lifecycle

Desktop consumes Artifact invalidation frames through its Runtime Host connection and runtime-host-boot:

deleted revokes the matching Artifact preview lease.
session_purged releases all preview leases for the affected Session.

The existing direct revoke in the artifacts:delete IPC handler remains in place so the local caller does not depend on asynchronous Host change-feed delivery.

Reconnect handling

When the Runtime Host candidate closes, all candidate-scoped preview endpoints are released and the scope is retired. When a replacement candidate reconnects with the same target epoch, the scope is reopened only after successful registration.

This prevents stale endpoints from surviving a disconnected Host while allowing new previews to be created after a normal reconnect.

Preview resource bounds

Preview admission is bounded by:

16 active previews per (scope, sessionId)
64 active previews across the Desktop
128 MiB of aggregate reserved preview content
8 MiB per preview

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.changed frame and its routing semantics are wire-visible protocol changes. After coordination with the current main, the Runtime Host compatibility epoch is 213 (main: 212).

If another protocol-changing PR lands before this one, the epoch must be rechecked and moved to the next available value before merge.

Verification

The feature series was rebased onto the current upstream/main.

Verified locally:

npm run build
Runtime Host protocol, Artifact protocol, connection-session, host-change-feed, reconnecting-connection, and Session retirement tests
Desktop managed Artifact preview, startup lifetime, and Runtime Host candidate tests
204 affected tests passed
git diff --check passed
Runtime Host protocol epoch guard passed

Coverage includes:

invalidation only after committed deletion
invalidation after partial Session purge
closed and bounded Artifact change frames
owner-only Artifact routing
no Artifact subscription for Session Guests
reconnect subscription cleanup
preview cancellation while preparing
per-Session and global preview limits
aggregate memory reservation and release
reconnect reopening the same preview scope
cleanup of all previews during Session purge

AI use

Select exactly one:

No generative tool made a substantive contribution
Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex investigated the defect, implemented the Runtime Host and Desktop changes, and authored the related regression tests.

Checklist

Tests cover the change and fail without it
The project builds successfully
Affected Runtime Host and Desktop test suites pass
Formatting and whitespace checks pass
Protocol compatibility epoch was coordinated against the current main

Does this PR entail a change in behavior?

Yes — deleted or purged Artifacts now invalidate active Desktop previews, Session Guests no longer receive Artifact invalidations, reconnects reopen valid preview scopes, and preview resources are bounded as described above.
No

me2seeks

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.

me2seeks · 2026-09-16T11:24:45Z

  const unsubscribeSessionCatalogChanges = client.subscribeSessionCatalogChanges(
    ({ sessionId }) => emitTargetSessionsChanged("updated", sessionId),
  );
+  const unsubscribeArtifactChanges = client.subscribeArtifactChanges((frame) => {


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.

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?

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.

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.

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.

Astro-Han

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.

Astro-Han

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.

Astro-Han

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.

Astro-Han

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

hqhq1025

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.

hqhq1025

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.

Astro-Han

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 if in the scope-close loop matches on mask.artifact.sessionId === frame.sessionId only, with no principal check, and deletes the connection's whole subscription. Since connection-session.ts:369-376 gives every session_guest connection an artifact: { 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.ts only 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 this if. 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 by isArtifactSharedSessionReadable, 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:44 only publishes session_purged when 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/releaseSession on events, openScope) is untested: the candidate tests use their own registerClientIpc, so deleting those lines in boot still passes.
- The multi-guest + artifact subscription + revoke scenario has no test.
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.

Astro-Han · 2026-09-28T03:30:44Z

+        closeScopeFor !== undefined &&
+        frame.kind === 'session.catalog.changed' &&
+        subscription.mask.artifact !== true &&
+        subscription.mask.artifact?.sessionId === frame.sessionId &&


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.

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

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.

hqhq1025 · 2026-09-28T07:12:53Z

+      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');


[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.

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!

me2seeks

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

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.

hqhq1025

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.

hqhq1025

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.

hqhq1025

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.

Astro-Han

Incremental review from ccc1d30f (our last review) to 9cf44e3c. Verdict: no P0–P2. The only finding is one P3 about the PR description.

What changed since ccc1d30. The branch was rebased from main de4fc5ff onto 0af3d5ad. git range-diff maps all seven earlier commits one-to-one:

Commits 2, 3, 4 and 6 are identical (=).
Commits 1, 5 and 7 differ only in packages/runtime-host/src/protocol/index.ts and in surrounding context lines that changed on main (session-retirement-coordinator.ts: requestDrain became footprint).
The new commit 9cf44e3c deletes a leftover // 201: Artifact invalidation feed... comment that the rebase had put between main's 200 and 199 entries.

The feature code itself is unchanged: preview revocation, owner-only Artifact routing, reconnect close/reopen, and the 16-per-Session / 64-global / 128 MiB limits. Our earlier analysis of it still holds.

Epoch. On this head the epoch is 209 with one combined 209 comment, and main (da682aa3) is at 208. That is a single step, it has no stray history lines, and it no longer describes any other PR's change. As before, if another protocol PR merges first, this one must move to the next free number.

Interaction with main since our last review. Main added archive retention, which auto-deletes archived Sessions. Its coordinator calls HostSessionRetirementCoordinator.removeForRetention. That goes through #removeUnder and then purgeSessionSidecars, which publishes onArtifactsPurged whether or not the purge succeeded (session-sidecar-purge.ts:44). Retention deletes therefore also revoke Desktop previews. Every caller of purgeSessionSidecars (the retirement and revision coordinators) and of onArtifactDeleted is wired in execution-composition.ts, and I found no new deletion path that skips invalidation.

Prior findings. All of our earlier P3s are closed: the dead per-Session mask, the no-op continue, the weak capacity tests and the stray epoch comment. Nothing from earlier is still open.

P3: the PR description no longer matches the code.

It says "Session Guests receive invalidations only for their shared Session". On this head guests get no Artifact subscription at all (connection-session.ts, owner-only artifact?: true).
It gives the compatibility epoch as 202. The head uses 209.

Please update the description so reviewers and the merge commit describe what actually ships.

Process note. GitHub still reports reviewDecision: CHANGES_REQUESTED because of me2seeks's review at 136f20b1 (2026-09-16). The P1 that review raised (a retired scope on same-epoch reconnect) was fixed long ago. A maintainer has to dismiss or re-review it.

CI and mergeability. Both test and windows_acp pass on 9cf44e3c. git merge-tree against current main da682aa3 is clean, and git diff --check passes. GitHub's mergeable state was still UNKNOWN when I checked.

Not verified. I did not run the test suites locally: the delta is the epoch plus context only, and hosted CI is green. I did not do an Electron end-to-end run. I did not check every open PR to see whether another one also claims 209.

P3 (epoch coordination): open #5548 also claims compatibility epoch 209 (and #3700, #5969 and #5495 claim 207, 210 and 212). Whichever of #5394 and #5548 lands second must renumber to the next free epoch at merge time.

Astro-Han

Incremental review: 9cf44e3c (our last reviewed head) to 2fc0e4c2. The branch was rebased onto main 6fa48516 (after #5495, #5512, #6011–#6016). git range-diff shows five of the eight commits unchanged. The other three differ only in the protocol epoch line, its comment and the surrounding hunk context: the first and the "isolate artifact invalidations from guests" commit carry the epoch hunk, and the last commit reconciles it again after the rebase.

Epoch: the head now claims RUNTIME_HOST_COMPATIBILITY_EPOCH = 213, one above main's 212 (#5495), with a single 213: comment above the 212 entry. This resolves the P3 from our last review: the collision with #5548 at 209 is gone, because both were below main anyway. No other open PR we track currently claims 213. Whichever protocol PR lands next must still take the next free number.

Findings: None at P0-P3 in the delta. The P3 about the stale PR description (guest invalidations, epoch 202) is still open. The earlier CHANGES_REQUESTED from 2026-09-16 still blocks merge until a maintainer dismisses it or re-reviews.

Status: CI test and windows_acp pass on 2fc0e4c2. git merge-tree against current main is clean. GitHub's mergeable state was still computing.

Preserve the upstream measured and unavailable tooltip copy in the post-compaction component assertions. The branch compatibility bump was already reconciled while replaying the protocol changes: epoch 214 is ahead of main at 212 and the 213 claims in open PRs apache#5394 and apache#5969. Generated-by: Codex

Epoch 214 is also declared by open PR apache#5548. Verified all 188 current open PR heads: apache#5394 and apache#5969 use 213, apache#5548 and this PR use 214, and 215 is unused. Move WorkHub to 215; allocation must be rechecked at integration because the base-only CI guard cannot reserve numbers across open branches. Generated-by: Codex

…6051) * refactor(workhub): simplify coordination and execution configuration Use the coordinating agent directly, persist WorkHub model settings, scope delegated permissions to executions, and separate steering from stop and delegation. Preserve historical recovery records and return worker missing information through ordinary follow-up messages. Generated-by: Codex * fix(workhub): propagate delegated execution policy and steering questions Preserve delegated permissions across child and graph execution lineage, restore scoped policy from durable Host facts, and update question handling when steering an existing backend. Generated-by: Codex * fix(runtime): support frozen stores in delegated execution policy Use an independent proxy facade so bound methods and permission read overrides do not violate invariants on frozen Host stores. Exercise the production frozen facade shape in the scoped-policy regression test. Generated-by: Codex * feat(desktop): route remote chats through shared WorkHub Add remote message handling settings, durable source-aware replies, and compact interaction choice answers. Generated-by: Codex * fix(core): simplify remote message source labels Generated-by: Codex * fix(desktop): keep message copy actions visible Generated-by: Codex * fix(desktop): size WorkHub rails to message bubbles Generated-by: Codex * feat(workhub): preserve intent and show clarification handoffs Generated-by: Codex * fix(workhub): link result replies to their source task rails Result notification turns had no links unless they delegated again. Derive their Host-scoped task identity from the durable result origin so rails and filters also cover tool-free result summaries. Generated-by: Codex * feat(workhub): add wn current-work and next-prompt shortcut Generated-by: Codex * fix(workhub): show progress and next steps for three recent works Generated-by: Codex * feat(workhub): place concise wn suggestions in composer draft Generated-by: Codex * fix(workhub): simplify answer summaries and balance result spacing Generated-by: Codex * style(workhub): reduce spacing before result notices to 4px Generated-by: Codex * refactor(ui): share transcript disclosure for selected answers and completion Generated-by: Codex * style(workhub): remove handoff content disclosure Generated-by: Codex * style(workhub): remove gap before result notices Generated-by: Codex * style(ui): highlight focused choice instead of picker outline Generated-by: Codex * fix(ui): confirm question answers on option click Generated-by: Codex * fix(desktop): localize WorkHub remote handling settings through catalog Generated-by: Codex * fix(ci): refresh surface inventory for shared transcript disclosure Generated-by: Codex * test(desktop): retain pending choices with keyboard selection Generated-by: Codex * fix(desktop): align story geometry with rails and timestamp sizing Check WorkHub rails against bubble bounds after the shorter-rail design. Give localized usage timestamps 8px more room so Linux font metrics fit without weakening the overflow assertion. Generated-by: Codex * fix(workhub): preserve steering policy across physical successors Resolve the latest durable policy for each physical continuation, including steered ordinary admissions with no sourceMessages. Add production composition coverage proving successor inheritance and restoration on a later user Turn. Use compatibility epoch 214 to distinguish this contract from the open epoch-213 changes. Generated-by: Codex * test(desktop): reconcile WorkHub fixtures after main rebase Provide the shared WorkHub enablement context to the new dock backdrop fixture and refresh the merged UI inventory totals. Generated-by: Codex * fix(protocol): assign WorkHub an unused compatibility epoch Epoch 214 is also declared by open PR #5548. Verified all 188 current open PR heads: #5394 and #5969 use 213, #5548 and this PR use 214, and 215 is unused. Move WorkHub to 215; allocation must be rechecked at integration because the base-only CI guard cannot reserve numbers across open branches. Generated-by: Codex

Preserve the upstream measured and unavailable tooltip copy in the post-compaction component assertions. The branch compatibility bump was already reconciled while replaying the protocol changes: epoch 214 is ahead of main at 212 and the 213 claims in open PRs apache#5394 and apache#5969. Generated-by: Codex

Treat measurements taken before a successful compaction as stale until a newer request settles. Update the composer display, localized tooltip, tests, and changelog. Generated-by: Codex fix(ui): keep post-compaction anchor over stale snapshot Preserve the selected request anchor timestamp so the context usage resolver can order it against a retained diagnostics snapshot. Cover the post-compaction sequence in resolver tests. Generated-by: Codex fix(ui): order context usage anchors by request settlement Persist the provider request completion time on usage anchors so a later transcript write cannot displace the same request snapshot and its metered window. Preserve legacy anchors and bump the runtime-host compatibility epoch for the new field. Generated-by: Codex docs(changelog): describe the post-compaction gauge fallback The stale reading keeps the gauge chip on its localized Usage label with the compacted-context tooltip; it never rendered a question mark. Align the entry with the shipped ContextUsageAction. Generated-by: Codex docs(ui): explain context usage reading resolution Describe what resolveContextUsage consumes (live Turn snapshot and durable transcript anchors/notes) and the ordering rule it applies after a compaction. Generated-by: Codex fix(ui): order usage anchors against the compaction apply time Record context_compaction_applied when a compaction lands so the boundary carries the apply time rather than the turn's settlement time, and persist completedAt on the settlement usage anchor even without provider telemetry. A send that compacts mid-turn and never completes its retry settles the usage row after the compaction notes while the anchor still describes the pre-compaction request, so readers arbitrate the newest anchored row against a boundary found behind it by event time. The applied row is hidden from the user-facing notes; context_compacted stays the display row. Refs #5547 Generated-by: Codex test(ui): assert context usage tooltips for stale and unavailable readings The share-resolution test only checked the visible label, so a stale reading and an unavailable reading were indistinguishable. Resolve the button aria-describedby to its tooltip element and assert the copy for the stale, measured, and unavailable states so the tooltip follows the reading across transitions in the same mounted control. Generated-by: Codex docs(runtime-host): cover the compaction note kind in the epoch-206 note Generated-by: Codex docs(changelog): name the compaction apply-time boundary row The gauge entry credited context_compacted alone, but the mid-turn boundary is the context_compaction_applied row recorded when a compaction lands; context_compacted stays the settlement-time display note. Refs #5547 Generated-by: Codex fix(runtime): forward compaction boundaries while a turn is running A mid-turn compaction only reached the composer gauge once the turn settled and the transcript refreshed, so the pre-compaction figure stayed on screen for the rest of a long turn. The runtime now emits a context_compaction_applied event when a compaction lands, persists it as the hidden boundary note, and the Runtime Host forwards it over the session subscription. The live context usage tracker holds the gauge on the compacted reading until a measurement that settled after the boundary arrives. Live and transcript readings share one ContextUsageSnapshot union, and the measured reading's window is renamed to contextWindow. Refs #5547 Generated-by: Codex test(ui): align context usage assertions with shortened tooltips Preserve the upstream measured and unavailable tooltip copy in the post-compaction component assertions. The branch compatibility bump was already reconciled while replaying the protocol changes: epoch 214 is ahead of main at 212 and the 213 claims in open PRs #5394 and #5969. Generated-by: Codex

github-actions Bot added the effort/L Under 1000 readable lines label Sep 16, 2026

SummerC0zyR0ck force-pushed the fix/managed-artifact-preview-lifecycle branch from 8e384e7 to 136f20b Compare September 16, 2026 10:06

me2seeks requested changes Sep 16, 2026

View reviewed changes

SummerC0zyR0ck force-pushed the fix/managed-artifact-preview-lifecycle branch 2 times, most recently from 517f3f8 to 9bd1819 Compare September 17, 2026 02:31

SummerC0zyR0ck requested a review from me2seeks September 17, 2026 02:33

Astro-Han reviewed Sep 17, 2026

View reviewed changes

SummerC0zyR0ck mentioned this pull request Sep 17, 2026

bug(desktop): managed artifact previews stay permanently closed after a Host reconnect #5436

Closed

SummerC0zyR0ck force-pushed the fix/managed-artifact-preview-lifecycle branch from e1aa3d3 to 8903dfd Compare September 17, 2026 06:57

Astro-Han reviewed Sep 17, 2026

View reviewed changes

SummerC0zyR0ck force-pushed the fix/managed-artifact-preview-lifecycle branch 2 times, most recently from 1b8fa8f to 6c444a5 Compare September 18, 2026 02:37

Astro-Han reviewed Sep 18, 2026

View reviewed changes

SummerC0zyR0ck force-pushed the fix/managed-artifact-preview-lifecycle branch from 6c444a5 to caeced8 Compare September 18, 2026 10:25

Astro-Han reviewed Sep 18, 2026

View reviewed changes

SummerC0zyR0ck force-pushed the fix/managed-artifact-preview-lifecycle branch 7 times, most recently from f6876ab to ac37976 Compare September 24, 2026 07:32

hqhq1025 reviewed Sep 25, 2026

View reviewed changes

SummerC0zyR0ck force-pushed the fix/managed-artifact-preview-lifecycle branch from ac37976 to 42b1057 Compare September 28, 2026 01:15

hqhq1025 reviewed Sep 28, 2026

View reviewed changes

Astro-Han reviewed Sep 28, 2026

View reviewed changes

hqhq1025 reviewed Sep 28, 2026

View reviewed changes

SummerC0zyR0ck force-pushed the fix/managed-artifact-preview-lifecycle branch from 855ee45 to 76cee97 Compare September 28, 2026 07:13

me2seeks reviewed Sep 28, 2026

View reviewed changes

hqhq1025 reviewed Sep 28, 2026

View reviewed changes

hqhq1025 reviewed Sep 29, 2026

View reviewed changes

liugddx mentioned this pull request Sep 29, 2026

tracking(storage): task storage visibility, reclamation and scoped cleanup #5825

Closed

hqhq1025 mentioned this pull request Sep 30, 2026

feat(desktop): show Agent Graph output previews #4751

Merged

1 of 2 tasks

SummerC0zyR0ck force-pushed the fix/managed-artifact-preview-lifecycle branch from 1e42277 to 83950b8 Compare September 30, 2026 06:56

hqhq1025 reviewed Sep 30, 2026

View reviewed changes

SummerC0zyR0ck force-pushed the fix/managed-artifact-preview-lifecycle branch from 83950b8 to 5813aea Compare September 30, 2026 08:45

hqhq1025 reviewed Sep 30, 2026

View reviewed changes

liugddx mentioned this pull request Sep 30, 2026

feat(desktop): clean up archived tasks by age and project #5896

Merged

SummerC0zyR0ck force-pushed the fix/managed-artifact-preview-lifecycle branch 2 times, most recently from 5813aea to 9cf44e3 Compare October 9, 2026 01:47

Astro-Han reviewed Oct 9, 2026

View reviewed changes

SummerC0zyR0ck added 8 commits October 9, 2026 10:03

fix(desktop): invalidate artifact previews after deletion

83fcd50

test(desktop): verify previews close on Host disconnect

1438be4

fix(desktop): reopen artifact previews after reconnect

f0466b2

fix(runtime-host): resolve artifact preview rebase conflicts

2f96414

fix(runtime-host): isolate artifact invalidations from guests

d0cd291

fix(desktop): bound aggregate artifact previews

3bbe855

fix(runtime-host): align artifact preview review fixes

ed17409

fix(runtime-host): reconcile protocol epoch after rebase

2fc0e4c

SummerC0zyR0ck force-pushed the fix/managed-artifact-preview-lifecycle branch from 9cc77ff to 2fc0e4c Compare October 9, 2026 10:05

Astro-Han reviewed Oct 9, 2026

View reviewed changes

Astro-Han mentioned this pull request Oct 9, 2026

feat(desktop): automatically render and archive chat images #5969

Open

4 of 6 tasks

This was referenced Oct 10, 2026

fix(desktop): show unknown context usage after compaction #5548

Merged

feat(workhub): simplify coordination and unify remote task workflows #6051

Merged

feat(providers): add Azure Foundry and Bedrock API-key connections #6032

Open

Conversation

SummerC0zyR0ck commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Host-scoped Artifact invalidation

Desktop preview lifecycle

Reconnect handling

Preview resource bounds

Protocol compatibility

Verification

AI use

Checklist

Uh oh!

me2seeks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Astro-Han left a comment

Choose a reason for hiding this comment

Uh oh!

Astro-Han left a comment

Choose a reason for hiding this comment

Uh oh!

Astro-Han left a comment

Choose a reason for hiding this comment

Uh oh!

Astro-Han left a comment

Choose a reason for hiding this comment

Exact-head review: caeced84bc1dd4d94be862686905f4961d475625

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

Astro-Han left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

me2seeks left a comment

Choose a reason for hiding this comment

Summary

Findings

Verdict

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

Astro-Han left a comment

Choose a reason for hiding this comment

Uh oh!

SummerC0zyR0ck commented Sep 16, 2026 •

edited

Loading

Exact-head review: `caeced84bc1dd4d94be862686905f4961d475625`