fix(editor): clean up orphaned local asset:// files from IndexedDB - #854
fix(editor): clean up orphaned local asset:// files from IndexedDB#854Zhao0335 wants to merge 12 commits into
Conversation
Deleting a scan or guide node only routed http(s) URLs to onDeleteAsset. Local asset:// Files (up to 200 MB per scan) stayed in IndexedDB forever. - Add deleteAsset + sweepOrphanAssets to @pascal-app/core asset storage - Schedule delayed local deletes on node removal (undo-safe grace period) - Sweep unreferenced assets after applySceneGraphToEditor - Cover storage, lifecycle, undo-restore, and cancel paths with tests Fixes pascalorg#733
|
I hit an error while handling your request (Model unavailable on AI Gateway free tier: Free tier users do not have access to this model. Upgrade to paid credits at https://vercel.com/d?to=%2F%5Bteam%5D%2F%7E%2Fai%3Fmodal%3Dtop-up for unrestricted…). Please try again, rephrase, or reach out if it keeps failing. Error id: 92c31b27-f9c4-4ce1-a2fb-ce2e00b06d8f |
IndexedDB is origin-global and not scoped by project/scene. Sweeping every asset_data: entry missing from the just-loaded graph deleted local Files still referenced by other scenes, blank canvases, and previews. Keep only the undo-safe delayed delete when a node is removed. Fixes the orphaned-File leak from pascalorg#733 without a multi-scene wipe. Addresses review on pascalorg#854.
|
Addressed the Bugbot finding in d44b09d: removed the scene-load orphan sweep. IndexedDB is origin-global, so sweeping entries missing from the just-loaded graph could wipe assets still referenced by other scenes. Cleanup is now only the undo-safe delayed delete of a specific �sset:// File after its last live node is removed. |
The grace-period re-check only inspected the active useScene graph. Switching projects during the window could delete an origin-global IndexedDB File that another saved scene still referenced. Bump a scene epoch in applySceneGraphToEditor; timers scheduled under a previous epoch refuse to delete. Addresses review on pascalorg#854.
|
Addressed the latest Bugbot finding: delayed deletes now capture a scene epoch at schedule time. �pplySceneGraphToEditor bumps the epoch; timers from a previous epoch refuse to deleteAsset. Switching projects during the grace window no longer deletes origin-global IndexedDB Files that another saved scene still references. Covered by a new unit test. |
|
This finding is already fixed on the current HEAD (cb97731), which is ahead of the reviewed 7ae66a2.
No further code change needed for this comment. |
Aymericr
left a comment
There was a problem hiding this comment.
Blocker: cleanup is attached to two UI handlers rather than the deletion lifecycle, so the bug still reproduces through supported delete paths.
For example, packages/editor/src/hooks/use-keyboard.ts deletes a selected guide/scan directly with useScene.getState().deleteNode(...), and the generic selection/delete surfaces in selection-routing.ts and group-actions.ts also bypass scheduleLocalAssetDelete. Deleting the same local reference with the Delete key therefore still leaves its asset:// File in IndexedDB.
Please move the scheduling to a single path that observes all committed node removals/replacements, or route every supported guide/scan deletion path through one shared helper. Add a regression test for keyboard deletion (and ideally delete-mode/generic selection deletion) so this cannot remain UI-entry-point dependent. The 120-second grace period, live-reference recheck, cache revocation, and scene-epoch guard are otherwise sensible.
Cleanup was attached to the Site panel and reference-panel handlers, so keyboard Delete (use-keyboard.ts), selection delete, MCP, and group actions still orphaned asset:// Files in IndexedDB (pascalorg#733). - Move grace-period scheduling into @pascal-app/core - Hook deleteNodesAction and applyNodeChanges delete/update paths - Cover keyboard-style deleteNode with a core regression test Fixes the remaining delete paths called out in review.
|
Addressed the deletion-lifecycle blocker in $(git rev-parse --short HEAD):
Grace period, live-reference recheck, cache revocation, and scene-epoch guard are unchanged. |
Aymericr
left a comment
There was a problem hiding this comment.
The central delete lifecycle now covers the missing keyboard/generic paths, but the current head still has three correctness blockers before cleanup is safe: (1) URL replacement schedules deletion without a fire-time scan of all live nodes, so undo or another node sharing the same asset:// URL can lose its File; (2) the reference-panel replacement schedules before updateNode commits, so a no-op/throw can delete the still-current asset; and (3) the scene epoch only advances in applySceneGraphToEditor, while unload/import/reset paths can replace the graph through setScene/clearScene and leave an old timer valid. Please make the timer predicate query the current graph for the URL at fire time, schedule replacements only after a committed change, and put epoch invalidation at the shared graph-replacement boundary. CI also needs the three Biome organize-import fixes shown by quality. Add regressions for shared-reference replacement, undo restoration, failed/no-op replacement, and a clear/set scene transition.
…Scene Address review on pascalorg#733 cleanup: - URL-replacement timers re-check the live graph at fire time (undo / shared asset:// handles) - Guide replace no longer schedules delete before updateNode commits - Epoch invalidation lives on unloadScene/setScene, not only applySceneGraphToEditor - organizeImports on the three CI-flagged files Regressions: shared-reference replace, undo restore, unreferenced replace, setScene epoch.
|
Addressed the three correctness blockers and Biome organizeImports on $head:
Regressions added for shared-reference replacement, undo restoration, unreferenced replacement, and setScene epoch. packages/core: 1545 tests pass. |
Aymericr
left a comment
There was a problem hiding this comment.
The new fire-time live-graph checks fix the reported undo/shared-node cases, but there is still a cross-scene data-loss blocker. asset:// storage is origin-global, and scene/project duplication preserves those URLs. If scene A and an unopened saved scene B reference the same File, deleting the last reference in the currently loaded A schedules deleteAsset; staying in A for 120 seconds means the timer sees no current reference and deletes B’s File. The epoch only protects a scene switch during the grace window, not references in persisted scenes that were never loaded. This is the same ownership problem that made the original sweep unsafe.
Please do not physically delete origin-global Files based only on the active graph. The cleanup path needs a durable cross-scene reference index/refcount (updated atomically with scene persistence), per-scene asset ownership/copying, or a conservative explicit garbage-collection pass that can inspect every saved graph. Until one of those exists, this PR trades an orphan leak for silent corruption of another project. Add a regression with two persisted scenes sharing one asset URL; deleting from one must not break the other.
Move bumpLocalAssetSceneEpoch import before material-library and put events/bus value export before its type export.
IndexedDB asset:// Files are origin-global and survive scene duplication, so deleting based on the loaded graph alone corrupts other saved scenes (pascalorg#733 review). - Remove automatic deleteAsset from deleteNodes/updateNodes timers - Core exposes sweepLocalAssetsExcept(keepUrls) for explicit GC - apps/editor GC unions current + localStorage + every server scene; skips the sweep when any source cannot be enumerated - Regression: two persisted scenes sharing one URL — delete from one must not break the other Fixes the cross-scene data-loss blocker.
|
Addressed the cross-scene data-loss blocker in No longer deletes origin-global Files from the active graph.
Orphan leak may remain until a successful full enumeration runs — that is preferred over silent corruption of another project. |
- Treat a full /api/scenes page (limit=500, no cursor) as incomplete — never sweep when older scenes may still reference a File - Re-read useScene nodes immediately before sweepLocalAssetsExcept so uploads during the long per-scene fetch stay in the keep-set - Keep initialScene urls in the extra keep-set Addresses Bugbot findings on 55e04ae.
|
Fixed the two High findings on
Unit tests cover truncated list, failed list, and mid-GC live-graph growth. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3bea691. Configure here.
| for (const url of extraKeepUrls) { | ||
| if (typeof url === 'string' && url.startsWith('asset://')) finalKeep.add(url) | ||
| } | ||
| return sweepLocalAssetsExcept(finalKeep) |
There was a problem hiding this comment.
GC deletes in-flight local Files
High Severity
runLocalAssetGc builds its keep-set from the server inventory plus this tab's live nodes, then sweepLocalAssetsExcept immediately deletes everything else. SceneLoader starts that sweep on mount, in parallel with the previous editor's keepalive flush and before a just-written asset:// node is guaranteed to be persisted. A project switch, a second tab, or an upload that has hit IndexedDB but not yet landed on a node can remove a File another saved or in-flight graph still references.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3bea691. Configure here.
| } catch { | ||
| return null | ||
| } | ||
| } |
There was a problem hiding this comment.
GC exhausts scene API rate limit
Medium Severity
collectAllPersistedAssetUrls issues one list request plus a sequential GET for every scene, on the same client rate bucket as autosave and SSE (120 requests/minute by default). Opening a /scene/[id] page with tens of scenes can burn the budget so later PUTs return 429, and a full page of fetches also stalls the editor for the whole inventory walk.
Reviewed by Cursor Bugbot for commit 3bea691. Configure here.


What does this PR do?
Fixes #733.
Deleting a local scan or guide node only routed
http(s)URLs toonDeleteAsset. Localasset://Files — including scans up to ~200 MB — stayed in IndexedDB forever after the referencing node was gone.This PR schedules a delayed delete of the specific File when the last live node that pointed at it is removed:
deleteAssetin@pascal-app/coreremoves the IndexedDB entry and revokes any cached object URL.applySceneGraphToEditor. Timers scheduled under a previous epoch refuse to delete, so switching projects cannot wipe a File another saved scene still uses.It does not sweep "all unreferenced assets" on scene load. IndexedDB is origin-global and not scoped by scene.
How to test
bun dev), open a project, and upload a local guide image.Application -> IndexedDBin DevTools).Screenshots / screen recording
N/A — storage lifecycle only, no visual change.
Checklist
bun devbun checkto verify)mainbranchValidation
packages/core: 1532 tests pass (bun run test)packages/editor:local-asset-lifecycle.test.ts6/6 pass@pascal-app/coretsc --buildclean@pascal-app/editorcheck-typescleanReview follow-ups
applySceneGraphToEditorepoch are skipped, so a project switch during the grace window cannot delete a File another saved scene still references.Note
Medium Risk
IndexedDB sweeps are gated on a complete scene inventory, but incorrect keep-set logic or API edge cases could still delete blobs shared across scenes or leave large orphans until the next successful GC.
Overview
Adds origin-wide garbage collection for local
asset://blobs in IndexedDB (#733).@pascal-app/corenow exposesdeleteAsset,listLocalAssetUrls,sweepLocalAssetsExcept, and helpers to collectasset://references from nodeurl/srcfields—physical deletes only happen through an explicit sweep with a full keep-set, not from UI delete/replace paths.The editor app builds that keep-set from the live scene, localStorage draft, and every server scene graph (via
/api/scenes), and aborts if listing fails or hits the 500-scene cap so a partial inventory cannot wipe assets still used elsewhere (e.g. after duplication).SceneLoaderruns this GC once after hydration, re-reading live nodes immediately before sweeping so uploads during the fetch stay protected.Reference and site panels stop doing local IndexedDB cleanup themselves (comments point lifecycle at core/GC instead);
http(s)asset deletes viaonDeleteAssetare unchanged.Reviewed by Cursor Bugbot for commit 3bea691. Bugbot is set up for automated code reviews on this repo. Configure here.