Repository navigation
fix: harden scanner, jobs and viewer; speed up media lists and tag autocomplete - #10
Merged
Merged
Conversation
bun audit reported 20 advisories (1 critical, 11 high). Bump sharp to 0.35.5 (libvips, libheif and librsvg CVEs), drizzle-orm to 0.45.3 (identifier escaping SQL injection) and adm-zip to 0.6.1 (zip bomb and symlink extraction, reachable through ugoira zips). In-range lockfile updates pick up proxy-addr 2.0.8, react-router 6.30.6, nanoid 3.3.18 and source-map-js 1.2.2. 12 advisories remain. They need major upgrades (express 5 for qs, react-router 7) or come from onnxruntime-node's install-time adm-zip.
The prune guard still deleted a library's rows in two cases. Any fs.access error counted as a deleted file, so a folder the server could not read (EACCES, EIO, a stale network mount) lost all its media. A folder nested inside another folder of the same library had its rows pruned through the outer folder before its own guard ran. Count a row as gone only on ENOENT or ENOTDIR. Give each row to the deepest library folder that contains it, and walk nested folders once through their outer folder, which also stops a nested scan from indexing every file twice.
transcodeVideo writes to <id>.mp4.part, and ffmpeg picks the muxer from the extension, so every transcode exited 234 with "Unable to choose an output format". Pass -f mp4, and delete the temp file when ffmpeg fails.
The library route, the scheduler and download batch completion all enqueue scans, and with the default concurrency of 2 two scans of the same library ran side by side. They raced on inserts (UNIQUE constraint failed: media.path), generated thumbnails twice and left the folder marked as errored. enqueueJob takes an optional key. Jobs that share a key run one at a time, and a request for a key that is already queued returns the queued job. Scans use scan:<libraryId>.
getSession cached the finished session, not the load. Two tag jobs that started together each created an ~800 MB ONNX session, and the first was overwritten without release. Cache the load promise, and drop it if the load fails. selectModel released the session even while an inference was running on it. Refuse a model switch while images are being tagged, as it already does during a download.
A media row keeps its path in the unique media.path index even after its file is deleted from disk. Renaming another file to that name passed the on-disk existence check, moved the file, then hit the unique index on the row update: a 500 with the file moved and the row pointing at the old path. In rename-batch it was worse. All row updates ran in one transaction after every file had already moved, so one such collision rolled back every row in the batch and left all of them stale. Both routes now refuse a target path any row already owns (409 for the single rename, a per-item failure in the batch). The batch also updates each row right after its own file moves, and moves the file back if that update fails, so disk and database can no longer drift apart.
Adding "Blue Hair" stores the tag as blue_hair, but removing it and the ?tags= filter only lowercased the name, so they looked up "blue hair", found nothing, and the remove silently did nothing while the filter returned an empty grid. Setting a tag's category had the same mismatch. Every user-typed tag name in the media routes now goes through one normalizeTagName helper next to upsertTag.
…bad cursors
Three request-validation holes:
- POST /api/uploads with a libraryId that does not exist saved the file
and indexed it into that missing library, leaving rows no library
owns. It now answers 404 before writing anything.
- PATCH /api/libraries/:id with an empty body reached the database as an
update with nothing to set and came back as a 500 "No values to set".
It is now a 400, and thumbnailMediaId must be an integer.
- A pagination cursor decoding to [{...}, 1] was bound straight into SQL
and failed with a 500 datatype mismatch, while one decoding to the
string "ab" was spread into ['a', 'b'] and served a nonsense page.
decodeCursor now requires an array of strings, numbers or nulls, and
/api/discover also requires its three parts to be numbers.
In the Docker image SAKUYA_AUTH_ENABLED and SAKUYA_HTTPS were only true for the exact string 'true'. SAKUYA_AUTH_ENABLED=1, TRUE or yes all quietly became false, so someone who switched login on got a server with no login at all. The legacy AUTH_ENABLED guard that refuses to start with auth off had the same exact-match blind spot. true/false, 1/0 and yes/no are now accepted in any case. Any other value is passed through unchanged, so the existing validation refuses to start with "must be true or false" instead of guessing.
Boot already marks jobs interrupted by a restart as errored, but the scan had also set its folders to 'scanning', and nothing reset those. After a crash or restart mid-scan the folder showed as scanning indefinitely, until someone happened to scan that library again. Folders still marked 'scanning' at boot are now set to 'error', next to the job reset.
Scheduled and manual dispatches skip a job type that is already queued or running for the library, but the after-scan path did not. A scan that finished while an earlier transcode, tag or hash job for the same library was still working queued a second one over the same untranscoded, untagged or unhashed ids. Two transcodes of one id write the same .part file. dispatchAfterScan now applies the same active-job check per follow-up type, including transcode.
…y or folder A scan reads its library's folders once, walks them, then indexes the files one by one. Deleting the library or detaching a folder while that was under way removed the existing rows, but the scan kept going and inserted fresh rows for every file it had not reached yet, under a library or folder that no longer existed. indexFile now checks, right before it writes, that the file still has an owner: an attached folder of the library containing it for a scanned file, or the library itself for an upload. The check sits after the last await so a removal cannot slip in between the check and the write.
…ated runOne took an item off the queue, then awaited detectGalleryDl, which spawns gallery-dl --version (a cold start of the standalone binary takes a noticeable moment), then set the item to running unconditionally. Pausing, skipping or removing the item during that wait was overwritten and the download ran anyway. The --version spawn also happened for every single item in a batch. runOne now re-reads the item after the wait and stops if it is no longer queued. The located binary is remembered per configured gallery_dl_path setting, so a batch spawns one version check instead of one per item; a failed lookup is not remembered, so a later install is still found. startConsoleSession had the same check-then-await shape: two starts in quick succession both saw no running session, both awaited detection, and both spawned. A pending flag now covers the wait, so the second start gets the 409.
listJobs ran on every /api/jobs call and every job-stream connect, and ORDER BY created_at had no index, so SQLite scanned the whole append-only jobs table into a temp b-tree to return 50 rows. Job ids are AUTOINCREMENT and assigned in the same insert that stamps created_at, so id order is the same order and walks the rowid backwards. GET /api/jobs on a synthetic DB with 20k jobs: 7.97ms -> 1.28ms (median), identical response.
With only media(type), a ?type=video listing on the default recent sort used that index to find the videos and then sorted all of them in a temp b-tree before taking 60. The composite index hands them over in created_at order. Existing databases get it from the CREATE block and drop the old index in migration 22; a fresh database only ever has the new one. Synthetic DB, 34k media (~8.5k videos), medians: - GET /api/media?type=video: 32.90ms -> 1.20ms - 5 cursor pages of ?type=video: 184.99ms -> 5.45ms Responses identical. Disabling the index with +m.type instead measured 2.90ms and 8.23ms, so the index wins and keeps the query untouched.
The list query selected m.* and a per-row tag_count subquery in the same select it sorted, so for any sort an index can't serve (size, name, random, search, tag filters) SQLite built every matching row before LIMIT kept 60. Rank narrow (id, sort_key) rows first and join the full rows for the page only, the shape discover.ts already uses. Ordering and cursors are unchanged; the new tests walk every sort and direction page by page against /api/media/ids. Synthetic DB, 34k media, best of 3 interleaved runs of 21-run medians: - sort=size: 9.24ms -> 6.49ms - sort=random: 7.87ms -> 4.90ms - q=hair (first page): 60.21ms -> 21.21ms - tags filter (first page): 98.89ms -> 51.35ms - 5 pages sort=size: 41.69ms -> 27.61ms - 5 pages sort=random: 47.18ms -> 32.57ms Index-ordered pages (recent, type=video) pay about 0.3ms for the extra join: 1.17ms -> 1.57ms and 1.14ms -> 1.41ms. Every response is identical.
Library-scoped autocomplete counted every tag link in the library on every keystroke. Those counts only change when a media gains or loses a tag, and every such write goes through refreshUsageCounts (tag PATCH, bulk tags, AI tagging, and removeMediaRows for deletes, scan prunes and library removal). refreshUsageCounts now bumps a version, and the route keeps the ordered (tag id, count) list per library until that version moves. Names and categories are not cached: each request filters the tags table live with the same LIKE and category clauses, then walks the cached order, so results and ordering are unchanged. Synthetic DB, 24k-item library, best of 3 interleaved runs, medians: - q=hair: 153.34ms -> 1.27ms - q=a: 279.72ms -> 2.30ms - no q: 324.00ms -> 2.57ms - category=rating: 123.35ms -> 0.69ms The first request after a tag change rebuilds the whole library's counts (the count is built before joining tags, ~30% cheaper than the old shape), so it costs about one unfiltered lookup: q=hair right after an edit is 150.09ms -> 216.04ms.
tags-batch built and ran a Drizzle insert or delete per (media, tag) link. The links now go through two statements prepared once per request. Same SQL semantics: INSERT ... ON CONFLICT DO NOTHING and a keyed DELETE. POST /api/media/tags-batch, 5000 items, one tag, alternating add and remove on a synthetic 34k-item DB: 770.83ms -> 493.07ms (best of 3 interleaved runs of 8-run medians), identical response. What remains is mostly the media_tags_fts_* triggers, which rebuild each item's search-index tag text per link: 5000 link writes measured ~370-450ms with the triggers and ~11ms without. Cutting that needs a trigger redesign, so it is left as is.
Watching a video to 60%, closing it and opening it again restarted it from the beginning and saved that over the 60%. Two things caused it. The view ping sent the list row's viewProgress, and the list is not refetched after a progress save, so it wrote the stale 0 back. The player also resumed from that same stale row. Progress is now optional on PATCH /api/media/:id/progress and is only written when sent. The view and dwell pings no longer send it. Leaving a video writes the last saved progress into the cached media lists so the next open resumes from it.
The viewer held a list index, so any refetch of the media list moved it onto a different item. Removing the tag Explore was filtered by from inside the viewer, or a scan finishing and adding a newer file, swapped the open image for its neighbour without any input. Callers now hold the open item's id and the viewer finds its index. If the item leaves the list it stays on screen, and stepping picks up from the slot it left.
With the "Add to board" picker focused, one ArrowRight stepped to the next item and added that item, not the one on screen, to a board. Alt+Arrow (browser back/forward) also stepped the viewer, and Escape while typing a tag closed the whole viewer. The window handler now ignores keys with alt, meta or ctrl held, and keys aimed at an input, textarea or contenteditable. Arrows on a select no longer step. Escape on the picker still closes the viewer, so picking a board and then pressing Escape keeps working.
… belong to The success handlers wrote the response into ['media-detail', id] using whichever item was open when it arrived. Adding a tag to one image and stepping to the next before the response landed put the first image's tags on the second. The heart button did the same with likes and also marked the wrong item liked in every cached list. The cache key now comes from the response's own id.
Every input tick of the slider pushed a history entry, so dragging it across the range left a dozen entries and Back stepped through them one 5% notch at a time. The slider also kept its own state after Back, so it showed 70% while the URL said 65%. The slider now replaces the current entry, and its state follows the surprise param when the URL changes under it.
The suggestion list is fetched for the debounced segment but was shown for whatever was in the box. Typing "red", seeing "redtag", then typing "x" and pressing Tab within the debounce window added "redtag" as a filter and threw away "redx". The list now shows only once the debounced segment matches the typed one.
…icate tile Ticking one checkbox in Settings > Duplicates re-rendered every tile on the page, 18 of 18 in a six-group test library, because each group card was handed the whole selection Set and a fresh toggle function. All of those thumbnails, and the board, library-card and cover-picker grids, also loaded and decoded eagerly whether on screen or not. Each duplicate tile is now memoized and receives a boolean selected prop with a stable toggle, so a click re-renders only the tile that changed. The grid thumbnails get loading="lazy" and decoding="async". The dashboard hero's floating thumbnails stay eager.
A library folder nested inside another folder of the same library is skipped by the walk, on the assumption that the outer walk reaches it. The walk does not follow symlinked directories or enter dot-directories, so files in such a nested folder are never indexed. This test fails on the current scanner.
…er walk 1abce5e skipped a folder nested inside another folder of the same library and relied on the outer walk to cover it. walk() skips dot-directories and does not follow symlinked directories, so a nested folder reached only through a symlink (a network share linked into the library root, for example) never had new files indexed, and its status still read "indexed". Walk every folder and drop duplicate paths with a Set. The subtree a nested folder shares with its outer folder is read twice, but each file is indexed once.
db94107 skips an after-scan follow-up when a job of the same type is already queued or running for the library. That job got its media ids when it was queued, so it never sees what the later scan indexed. These tests replace the one that pinned the skip and cover both cases: the earlier transcode is running, or it is still waiting in the queue. Both fail on the current scheduler because the second video is never transcoded.
A per-library tag, hash or transcode job got its media ids when it was queued. db94107 then skipped the after-scan follow-up while such a job was active, so media indexed by a later scan waited for the next scan to be tagged, hashed or transcoded. The scheduler now queues these jobs through enqueueCatchUp. The job reads the pending ids when it starts, and it runs under the key <type>:<libraryId>, as scans do since ed66401. A follow-up that finds a job still waiting for the same library joins it. A follow-up that finds one running waits for it and then picks up only the ids that job left. Two transcodes of one id still never run at once. runTagJob, runHashJob and runTranscodeJob are the job bodies, pulled out of their enqueue functions. enqueueHashJob and enqueueTranscodeJob drop the libraryId parameter, which only the scheduler used.
The similar-media comment named media_type_idx, which migration 22 drops. The transcode test added a jobEvents listener and never removed it, so it stayed attached for the rest of the file.
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.
Why
This branch comes out of a full pass over the codebase. It fixes bugs that lose or hide data, makes the most-used queries cheaper, and fixes viewer bugs that showed or saved the wrong item. The scanner changes follow the September library wipe. They make pruning more conservative, so a scan can remove fewer rows than before, never more.
Scope
enqueueJobkeyscan:<id>). Folders leftscanningby a crash reset at boot. After-scan tag, hash and transcode jobs run under<type>:<id>keys and read their pending ids when they start (enqueueCatchUp).1,yesandTRUE. The downloader keeps paused or skipped items that way while it locates gallery-dl.media_type_created_idxreplacesmedia_type_idx(migration v22). List pages are ranked on ids first. The jobs list orders by id. Per-library tag counts are cached. Bulk tag statements are prepared once per batch.Tradeoffs
A nested library folder is walked a second time instead of being skipped. The extra
readdircalls cost far less than indexing, and skipping it missed folders the outer walk can't reach.Blast Radius
The change touches every scan, every scheduled job and the media list query on every page load. The risk is mostly in scanner pruning, so the new tests cover unreadable, unmounted, nested and removed-mid-scan folders. Migration v22 runs once at boot and only adds and drops an index.
Verification
bun test --isolate: 157 pass, 0 fail. Each fix in the last four commits lands after a test that fails without it.bun run typecheck,bun run lint(0 errors) andbun run buildpass.<type>:<id>key fails the "transcode runs" test with two overlapping transcodes. Reading ids at queue time fails the "transcode waits in the queue" test.