fix(scanner): fence stale scan reconciliation - #801
Open
Quick104 wants to merge 1 commit into
Open
Conversation
|
Warning Review limit reachedNext included review available in 2 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 94 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
Comment |
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.
Problem
Related issue: N/A — audit-validated catalog correctness fix
A full scan could snapshot a
media_filesrow, miss its path during the walk, and later mark that row missing even after an overlapping scoped scan had re-upserted it. The stale mark hid present media and made it eligible for deletion after the missing-file grace period.The invariant this PR adds is: reconciliation may mark or delete a snapshot row only while its scan generation still matches the snapshot. Any committed upsert or completed presence observation advances the generation first.
Approach
media_file_scan_generationstable keyed bymedia_files.id. AnAFTER INSERT OR UPDATEtrigger creates or advances the generation for every media-row write, including identity-only upserts.media_filesfirst, then atomically claim the expected generation before marking or deleting. If a refresh committed first, PostgreSQL rechecks the predicate after the lock wait and the stale operation changes zero rows.The side table avoids rewriting wide
media_filesrows just to record presence. Refreshes use the existing folder/path lookup and primary keys in batches of 1,000. A synthetic 100,000-path run completed 100 batches in 3.081 seconds and generated 27,890,056 WAL bytes. The rejected wide-row prototype took 10.277 seconds and generated 192,749,136 WAL bytes on the same harness.Validation
Passed:
make embed-stubgo build ./...gofmt -l .(no output)go vet ./...golangci-lint run --new-from-merge-base=origin/main ./...(0 issues)make test-goon dev-builder. The first run hit an unrelated transientETXTBSYinTestProbeFileKeepsMetadataWhenPacketFallbackFails; the full retry passed.go test ./internal/scanner -count=1go test ./internal/libraryingest -count=1make migrate-validatepnpm install --frozen-lockfilepnpm run lint(0 errors; 167 existing warnings)pnpm run format:checkpnpm run buildmake test-web(294 files, 2,184 tests)make verify-settings-bindings-allmake verify-playback-fixturesmake verify-local-pathsdoctor: container, database, API, and frontend healthyThe database-backed catalog package still has its pre-existing
TestEpisodeSearchPostgresAndDocumentSourcefailure because the document containssilo_recommendations: nil. The ordinary catalog suite passes undermake test-go; this change does not touch catalog document construction.The deterministic sandbox matrix proves both sides of the behavior: a refreshed synthetic file remains present and visible after stale reconciliation, while a genuinely absent file is marked missing, survives a nonzero grace period, and becomes eligible for cleanup afterward. A separate barrier test holds the refresh transaction uncommitted, observes stale mark and delete blocked on its row lock, commits the refresh, and verifies both stale operations return zero.
No private fixtures were used. All sandbox media and paths were synthetic.
Risks
The migration creates and backfills one narrow row per existing media file, so apply time and WAL scale linearly with
media_files. The trigger is installed before the conflict-safe repair backfill so concurrent inserts and updates cannot miss initialization.Unrelated metadata updates conservatively invalidate the current scan snapshot and can delay missing detection until the next scan. Pathological overlapping multi-row batches can receive PostgreSQL's normal retryable deadlock error. Both cases fail without applying a stale mark or delete.
Rollback order is application first, migration second: deploy the pre-change binary, then run the migration down step to drop the trigger, function, and side table. The new binary requires the side table.
This is internal catalog-state behavior. It does not change the v1 API, capability surface, auth, playback contract, or client models, so no Apple or Android changes are needed. Jellyfin-compatible clients benefit from the same preserved catalog visibility without a jellycompat contract change.
AI Disclosure
Checklist