Keep temp-directory assets visible while their files exist - #15510
Keep temp-directory assets visible while their files exist#15510synap5e wants to merge 2 commits into
Conversation
Assets written to the temp directory were flagged as missing and dropped from GET /api/assets, even with the file sitting on disk. One list of directories was answering two different questions -- where the scanner looks for new files, and which files ComfyUI considers its own -- and temp belongs only in the second, so every temp reference was disowned by the prune that runs at startup and on POST /api/assets/prune. Ownership now covers temp. Discovery still does not: the temp directory is wiped before the scan runs, and assets written there are already registered with a hash, mime type and dimensions, so walking it would find nothing. Temp references are instead reconciled against the filesystem directly, so a temp file that really is gone is still retired rather than lingering as a broken entry. get_prefixes_for_root becomes get_scan_prefixes_for_root so the two questions are told apart by name rather than by comment.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a90aa2d21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| all_prefixes = get_owned_prefixes() | ||
| marked = mark_missing_outside_prefixes_safely(all_prefixes) |
There was a problem hiding this comment.
When /api/assets/prune is called after a temp file has disappeared, adding the temp directory to get_owned_prefixes() prevents the outside-prefix update from marking that reference missing, but this explicit prune method never calls sync_temp_references_safely() as _run_scan does. Because temp is intentionally excluded from every scan root, the nonexistent reference remains active and listed by GET /api/assets until ComfyUI restarts; reconcile temp references in this path as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deliberate. Reconciliation runs on the startup scan instead — see the description.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @synap5e.
Found 4 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🟢 Low | 2 |
Panel: 8/8 reviewers contributed findings.
| marked = mark_missing_outside_prefixes_safely(all_prefixes) | ||
| if marked > 0: | ||
| logging.info("Marked %d refs as missing before scan", marked) | ||
| sync_temp_references_safely() |
There was a problem hiding this comment.
🟡 Medium — get_owned_prefixes() now includes the temp directory, so mark_missing_outside_prefixes_safely treats temp references as owned and never prunes them. Temp is only stat'd by sync_temp_references_safely(), which runs solely inside this _prune_first branch; non-prune scans (API / /object_info-triggered) and the standalone mark-missing method at line 417 skip it, so temp files deleted during a session stay listed as present until the next startup prune. Raised by 4 of 8 reviewers (kimi-k2.7-code adversarial, gpt-5.6-sol-max edge-case, gemini-3.1-pro edge-case, kimi-k2.7-code edge-case).
There was a problem hiding this comment.
Deliberate. Reconciliation runs on the startup scan instead — see the description.
| """Retire temp references whose file is gone; temp is never scanned, so nothing else stats them.""" | ||
| try: | ||
| with create_session() as sess: | ||
| sync_prefixes_with_filesystem(sess, get_temp_prefixes()) |
There was a problem hiding this comment.
🟢 Low — This calls sync_prefixes_with_filesystem without update_missing_tags=True. The is_missing flag is still toggled, but the 'missing' metadata tag isn't maintained for temp refs, and already-missing rows are excluded from the reconciliation query — so a temp reference once marked missing is never restored if its file reappears, since nothing else scans temp. Raised by 3 of 8 reviewers (gpt-5.6-sol-max edge-case, kimi-k2.7-code adversarial, kimi-k2.7-code edge-case).
There was a problem hiding this comment.
Registration clears the flag, so it is restored. Not changing.
|
|
||
|
|
||
| def get_temp_prefixes() -> list[str]: | ||
| return [os.path.abspath(folder_paths.get_temp_directory())] |
There was a problem hiding this comment.
🟢 Low — get_temp_prefixes() feeds folder_paths.get_temp_directory() straight into the owned-prefix set with no bounds check. A broadly configured temp directory would make mark_missing_outside_prefixes_safely treat everything beneath it as owned, silently disabling the missing-reference prune for those paths. Raised by 1 of 8 reviewers (kimi-k2.7-code adversarial).
There was a problem hiding this comment.
Same for input, output and models. Not specific to this change.
The existing temp tests all registered hashed assets, so they never exercised the path an unhashed asset takes when its file is gone: the orphaned rows are removed rather than kept as missing, exactly as under any other root.
|
@coderabbitai review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
2 participants
Assets written to the temp directory were flagged as missing and dropped from
GET /api/assets, even with the file sitting on disk. Anything that registers a temp asset — previews, intermediate outputs — was affected.One hardcoded list of directories was answering two different questions:
collect_paths_for_rootsget_all_known_prefixesTemp belongs only to the second, but it was in neither, so
mark_references_missing_outside_prefixesdisowned every temp reference. That prune runs on startup and onPOST /api/assets/prune, so a temp asset vanished from the listing within moments of being created.What changed
Ownership now covers temp, so a temp file that exists on disk stays visible across a prune.
Discovery deliberately does not. The temp directory is wiped before the scan runs, and temp assets are already fully populated at registration time — hashed, with mime type and image dimensions — so adding temp as a scan root would walk an empty directory on behalf of assets that need nothing from it.
Because temp is never scanned, nothing else ever stats those files. Temp references are therefore reconciled against the filesystem directly, alongside the prune, so a temp file that really is gone is retired rather than lingering as a broken entry once the next restart wipes the directory. This is the same reconciliation the scan already runs for the other directories, and it is not purely a soft mark: a reference whose file is gone is flagged missing, and rows left with nothing pointing at them are dropped. That reconciliation deliberately does not apply the
missingtag — a temp file disappearing is routine, unlike a vanished model. A reference already marked missing is restored if a file reappears at that path, since registering it clears the flag.That reconciliation runs as part of the startup scan, which is the point at which it is needed: ComfyUI wipes the temp directory on the way up, immediately before the scan, so that is precisely when stale temp references exist. A temp file deleted mid-session keeps its reference until the next restart — ComfyUI leaves temp files alone until shutdown, so there is nothing to reconcile in between.
That includes
POST /api/assets/prune, which deliberately does not reconcile temp. Its contract is to mark references missing when they fall outside the known prefixes, and temp is now inside them, so it correctly leaves them alone. Before this change the endpoint appeared to clean temp up, but only as a side effect of disowning every temp reference including the live ones — the bug this PR fixes, rather than a behaviour worth preserving.get_prefixes_for_rootis renamedget_scan_prefixes_for_rootso the two questions are told apart by name rather than by comment.Testing
New
tests-unit/assets_test/test_temp_assets.pycovers both directions: temp is owned, discovery skips it, a live temp reference survives the prune while a file outside every owned directory is still disowned, and a wiped temp file is retired — hashed and unhashed alike, since the two are retired differently. A new seeder test covers the wiring, so removing the reconciliation call fails the build. Confirmed the new tests fail against the unfixed code.Existing suites pass unchanged:
tests-unit/assets_test(includingtest_sync_references.py's 22 tests),tests-unit/seeder_test, andtests/test_asset_seeder.py.Also driven end to end through a real scan against a SQLite database, with one live temp asset and one whose file had been deleted: the live one stays listed, the deleted one is retired.
Review coverage
Codex and the Cursor panel (8/8 reviewers) both reviewed
6a90aa2d, the commit carrying the change itself;f2c2288bon top of it adds test coverage only. Findings raised on the prune endpoint and on how unhashed temp assets are retired are answered above, in What changed.CodeRabbit approved
f2c2288bwith no findings.🤖 Generated with Claude Code