Repository navigation
fix(desktop): invalidate artifact previews after deletion #5394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
20db8b7
3664854
4615a6b
7e0a17c
3128ab6
14b9605
061be7c
5813aea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1562,6 +1562,13 @@ function registerHostClientIpc( | |
| const unsubscribeSessionCatalogChanges = client.subscribeSessionCatalogChanges( | ||
| ({ sessionId }) => emitTargetSessionsChanged("updated", sessionId), | ||
| ); | ||
| const unsubscribeArtifactChanges = client.subscribeArtifactChanges((frame) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 — A deletion missed while reconnecting leaves the deleted preview live for the rest of its TTL.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1 — A normal Desktop reconnect permanently disables artifact previews for that target. The concrete path is the candidate cleanup, not I reproduced this on
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for tracing the concrete path — you are right. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @me2seeks — the P1 you found is fixed on the current head 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. |
||
| if (frame.reason === 'deleted') { | ||
| void managedArtifactPreview.revoke(scope.targetEpoch, frame.sessionId, frame.artifactId); | ||
| } else { | ||
| void managedArtifactPreview.releaseSession(scope.targetEpoch, frame.sessionId); | ||
| } | ||
| }); | ||
| const unsubscribeProjectCatalogChanges = client.subscribeProjectCatalogChanges(() => { | ||
| sendToRenderer("projects:changed"); | ||
| }); | ||
|
|
@@ -1835,12 +1842,14 @@ function registerHostClientIpc( | |
| }); | ||
| registerOnboardingIpc({ onboardingService, ipcMain: scopedIpc }); | ||
| registerTaskSubmissionReadinessIpc(taskSubmissionReadinessService, scopedIpc); | ||
| managedArtifactPreview.openScope(scope.targetEpoch); | ||
| return async () => { | ||
| clientPluginTransport.release(client); | ||
| unsubscribeConfigurationChanges(); | ||
| await managedArtifactPreview.closeScope(scope.targetEpoch); | ||
| unsubscribeConnectionCatalogChanges(); | ||
| unsubscribeSessionCatalogChanges(); | ||
| unsubscribeArtifactChanges(); | ||
| unsubscribeProjectCatalogChanges(); | ||
| unsubscribeScheduledTaskChanges(); | ||
| runtimePolicyTargets.delete(target); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!