Skip to content

fix(scanner): fence stale scan reconciliation - #801

Open
Quick104 wants to merge 1 commit into
mainfrom
codex/fix-scan-reconcile-fence
Open

fix(scanner): fence stale scan reconciliation#801
Quick104 wants to merge 1 commit into
mainfrom
codex/fix-scan-reconcile-fence

Conversation

@Quick104

Copy link
Copy Markdown
Contributor

Problem

Related issue: N/A — audit-validated catalog correctness fix

A full scan could snapshot a media_files row, 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

  • Add a narrow media_file_scan_generations table keyed by media_files.id. An AFTER INSERT OR UPDATE trigger creates or advances the generation for every media-row write, including identity-only upserts.
  • Capture the generation with pre-walk scan state for full, subtree, exact-file, audiobook, ebook, manga, podcast, and removed-root reconciliation.
  • Refresh generations for paths a completed scan observed, including unchanged-file fast paths.
  • Lock media_files first, 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.
  • Keep missing grace cleanup unchanged. Genuine missing rows are still marked and swept after the configured grace period.

The side table avoids rewriting wide media_files rows 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-stub
  • go build ./...
  • gofmt -l . (no output)
  • go vet ./...
  • golangci-lint run --new-from-merge-base=origin/main ./... (0 issues)
  • make test-go on dev-builder. The first run hit an unrelated transient ETXTBSY in TestProbeFileKeepsMetadataWhenPacketFallbackFails; the full retry passed.
  • Database-backed go test ./internal/scanner -count=1
  • Database-backed race run covering stale mark/delete, grace, caller snapshots, repeated upserts, and overlap cases
  • Database-backed go test ./internal/libraryingest -count=1
  • make migrate-validate
  • pnpm install --frozen-lockfile
  • pnpm run lint (0 errors; 167 existing warnings)
  • pnpm run format:check
  • pnpm run build
  • make test-web (294 files, 2,184 tests)
  • make verify-settings-bindings-all
  • make verify-playback-fixtures
  • make verify-local-paths
  • Isolated dev-builder doctor: container, database, API, and frontend healthy
  • Independent read-only review of the final patch and deterministic PostgreSQL lock-barrier test

The database-backed catalog package still has its pre-existing TestEpisodeSearchPostgresAndDocumentSource failure because the document contains silo_recommendations: nil. The ordinary catalog suite passes under make 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

  • Tool(s): OpenAI Codex desktop
  • Model(s): gpt-5.6-sol (ultra)
  • Involvement: Fully AI-generated, human verified
  • Adversarial review: A separate read-only agent reviewed row identity, snapshot timing, MVCC predicate rechecks, lock ordering, migration installation races, clock assumptions, grace behavior, caller coverage, index use, WAL, and false-negative missing detection. It found gaps in confirmed-empty deletion, unchanged-path refreshes, literary scan timing, removed-root timing, the original wide-row design, migration ordering, non-atomic generation checks, and lock-barrier coverage. Each finding was resolved and the relevant tests and performance probe were rerun. The final review found no actionable issues.

Checklist

  • I read and can explain the complete diff.
  • This pull request addresses one concern.

@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 2 minutes.

View limit details

Limit 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.
You're only billed for reviews past your plan's rate limits ($0.25/file).

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3128b6d6-39ec-4bea-8e60-57bc2950f027

📥 Commits

Reviewing files that changed from the base of the PR and between 8164fd5 and f59af6a.

📒 Files selected for processing (10)
  • internal/scanner/audiobook_scan.go
  • internal/scanner/ebook_scan.go
  • internal/scanner/ebook_test.go
  • internal/scanner/file_repo.go
  • internal/scanner/manga_scan.go
  • internal/scanner/podcast_scan.go
  • internal/scanner/scan_reconcile_fence_test.go
  • internal/scanner/scan_state.go
  • internal/scanner/scanner.go
  • migrations/sql/20260827201436_add_media_file_scan_generation.sql

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant