feat(downloads): unlink imported files once *arr confirms the import - #79
Merged
Conversation
Adds `downloads.unlinkImportedFiles` (default false). When enabled, the import watcher drops jack's copy in completedPath right after a row flips to `imported` — only on a confirmed import (the destination reports the hash in its history, or the manual-import command reports completed), never on a queued or failed row. It is a plain unlink of that one file: if *arr hardlinked into the library the data lives on through the library's link; if it copied, only jack's copy goes. The path guards from DownloadsService.delete move into a shared `unlinkDownloadArtifact` (inside completedPath, still owned by the live row, not shared with a sibling, ENOENT-tolerant) and both call sites use it. Configurable from the management UI (Settings -> Downloads) via a new GET/PATCH /config/downloads; the PATCH merges into the stored block and the watcher reads the flag per import, so the toggle applies without a restart.
Contributor
|
5 tasks
The sibling guard blocked an unlink whenever any other row referenced the path, including rows that were themselves already imported. Two rows can legitimately share a destination — the in-flight duplicate guard only covers active transfers, so a re-grab that lands while the first row is still import_queued gets its own row on the same path. Both would then import, each defer to the other, and since `imported` is terminal and never revisited, the file outlived both rows despite unlinkImportedFiles being on. A sibling now only blocks while it still has a use for the file: downloading into it, awaiting import, or holding a failed import that a retry would re-trigger against that path. `imported` is the one status that doesn't qualify, so the row that imports last does the cleanup. This also applies to the delete path, which shares the helper: deleting a row now removes a shared file that only already-imported rows still point at. Reported by Greptile on #79.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds
downloads.unlinkImportedFiles(defaultfalse): once an *arr confirms it imported a download, jack drops its own copy fromcompletedPath.The motivation is that jack has no use for the file after the import — it's never re-served or re-imported. Whether the *arr hardlinks (my setup) or copies (a friend's), the library has what it needs, so jack's copy is dead weight.
It is a plain
unlinkof that one file, never a recursive delete:Only ever on a confirmed import
ImportWatcher.cleanUpImportedFileruns immediately aftermarkImported, at exactly the two points where *arr confirms the import:completed.Rows that are queued, still importing, failed, or blackhole keep their file. A failed unlink is logged and leaves the row
imported— the import already succeeded, so cleanup never fails it.Shared path guards
The guards that were inline in
DownloadsService.deletemove intounlinkDownloadArtifact(modules/downloads/artifact-cleanup.ts) and both call sites now use it: the path must resolve insidecompletedPath, the live row must still reference it, no sibling row may reference it, and ENOENT counts as success. All three are re-evaluated immediately before the syscall.Configurable from the UI
New
GET/PATCH /config/downloadson the management API, and a Settings → Downloads section with a switch. The PATCH is merged into the stored block, so the file's other download knobs are never clobbered (a first patch that would leave the merged block withoutcompletedPathis rejected with a 400). The watcher reads the flag per import rather than at boot, so the toggle applies without a restart — unlike every other key in the block. When the config has nodownloadsblock at all, the section renders an explanatory empty state instead of a dead toggle.Related issue
Checklist
mise run lint:fixwas run and its output is committed (no changes)mise run testpasses — 464 pass / 0 fail.mise run test:e2ewas not run: no Docker in the dev environment. The default isfalse, so the import path is byte-for-byte unchanged unless the option is enabled.website/reference/configuration.md,website/guide/management-ui.md,examples/config.jsoncTests
12 new tests:
importedwhen the unlink itself failscompletedPathGET/PATCH /config/downloads: merge semantics, 400 on a merged block with nocompletedPath, 400 on a non-boolean flag, 404 without aConfigServiceAlso smoke-tested against a running backend:
GET→PATCH→GETround-trips and lands correctly on disk.