feat(): adds Media field IsMissing for persistent media#551
feat(): adds Media field IsMissing for persistent media#551BossRighteous wants to merge 4 commits intoZaparooProject:mainfrom
Conversation
BossRighteous
changed the title
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR introduces a persistent Media.IsMissing flag intended to support rescans without truncating media tables, by marking existing media as missing before scanning and clearing the flag when media is encountered again.
Changes:
- Add
IsMissingcolumn toMedia(migration + Go struct/interface updates). - Replace full/selective truncation in the media scanner with “mark missing” maintenance operations.
- Add UPSERT-style behavior and batch inserter support intended to clear
IsMissingon conflict.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/database/database.go | Adds IsMissing to database.Media and new methods to MediaDBI. |
| pkg/database/mediadb/migrations/20260304011734_media_ismissing.sql | Adds the IsMissing column to the Media table. |
| pkg/database/mediadb/sql_media.go | Updates Media insert SQL to include IsMissing and attempts UPSERT behavior. |
| pkg/database/mediadb/sql_maintenance.go | Adds SQL helpers to mark media missing (all or per-system). |
| pkg/database/mediadb/mediadb.go | Wires new maintenance methods + adds conflict clause support to batch insert for Media. |
| pkg/database/mediadb/batch_inserter.go | Extends batch inserter to append a custom onConflict clause. |
| pkg/database/mediadb/batch_inserter_test.go | Updates test call site for new batch inserter signature. |
| pkg/database/mediascanner/mediascanner.go | Switches scanner prep from truncation to “mark missing” and adjusts scan-state seeding flow. |
| pkg/database/mediascanner/indexing_pipeline.go | Sets IsMissing=0 on inserts; attempts to clear missing on “found” media. |
| pkg/testing/helpers/db_mocks.go | Adds mock methods for the new maintenance API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return handleCancellation( | ||
| ctx, db, "Media indexing cancelled during selective scan state population", | ||
| ) | ||
| } |
There was a problem hiding this comment.
Since data isn't truncated anymore, PopulateScanStateForSelectiveIndexing leaves TitleIDs/MediaIDs maps empty — so AddMediaPath will re-insert titles with new DBIDs each rescan. MediaTitles doesn't have a unique constraint so they'd duplicate. Is that a problem or is this handled somewhere I'm missing?
| IsMissing: 0, | ||
| }) | ||
| // Don't process tags if found | ||
| return titleIndex, mediaIndex, nil |
There was a problem hiding this comment.
Related to the above — since MediaIDs is empty, this else branch never actually runs right now. Every path takes the !ok branch and does a fresh insert (which upserts via ON CONFLICT). So the early return skipping tags is dead code until the maps are populated. Worth keeping in mind when addressing the maps question.
| return 0, fmt.Errorf("failed to truncate database: %w", err) | ||
| } | ||
| log.Info().Msg("database truncation completed")*/ | ||
| err := db.MarkAllMediaMissing() |
There was a problem hiding this comment.
What's the plan for rows that stay IsMissing=1 after a scan finishes? Right now search queries don't filter on it, so removed files would still show up in results. Is the idea to add a cleanup step later, filter at query time, or leave them for a future orphan management UI?
There was a problem hiding this comment.
@BossRighteous we agreed that there really wouldn't be that many and it can be worried about later right?
Fix unchecked error return from InsertMedia, variable shadowing, stale comments, import shadowing, and test mock expectations. Remove commented-out truncation code blocks.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Fix gosec G118 (unused context cancel), G117 (secret pattern), G703 (path traversal), noctx (httptest.NewRequest), and prealloc warnings.
Fixes GO-2026-4601 (net/url IPv6 parsing) and GO-2026-4602 (os.Root FileInfo escape) flagged by CI govulncheck.
| const insertMediaSQL = `INSERT INTO Media | ||
| (DBID, MediaTitleDBID, SystemDBID, Path, IsMissing) | ||
| VALUES (?, ?, ?, ?, ?) | ||
| ON CONFLICT (SystemDBID, Path) | ||
| DO UPDATE SET | ||
| IsMissing = 0 | ||
| ON CONFLICT (DBID) | ||
| DO UPDATE SET | ||
| IsMissing = 0;` |
There was a problem hiding this comment.
Bug: During a selective rescan, the ON CONFLICT clause for Media updates IsMissing but fails to update MediaTitleDBID, leaving a stale reference and creating orphaned records.
Severity: HIGH
Suggested Fix
Modify the ON CONFLICT (SystemDBID, Path) clause in the InsertMedia function. The DO UPDATE statement should also update the MediaTitleDBID to the new value, for example: ... DO UPDATE SET IsMissing = 0, MediaTitleDBID = excluded.MediaTitleDBID.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: pkg/database/mediadb/sql_media.go#L33-L41
Potential issue: During a selective rescan, the logic in `AddMediaPath` creates a new
`MediaTitle` record for a file, but the subsequent `InsertMedia` call uses an `ON
CONFLICT` clause that only updates the `IsMissing` flag on the existing `Media` row. It
fails to update the `MediaTitleDBID` to point to the newly created title record. This
results in the `Media` row retaining a stale reference to an old `MediaTitle`, leading
to incorrect metadata associations and orphaned `MediaTitle` entries in the database.
This PR takes advantage of all of the existing scan state routines and adds a simple IsMissing flag to Media.
Instead of truncating tables in scanning, we can selectively mark
Media.IsMissing = 1as an inexpensive query before scan.The AddMediaPath routine then performs an insert with
ON CONFLICT DO UPDATE SET IsMissing = 0to allow existing records to update on conflict or insert as new.Truncate operations can be moved to a new UI, and records may be pivoted from the
WHERE Media.IsMissing = 1as a starting point for orphans.I was able to scan from scratch, rescan, and selective rescan with file modifications. Please test accordingly on your side as well.