Repository navigation
perf(metadata): batch artwork reconcile bulk resets - #818
Conversation
bulkResetSurface and bulkResetChapterThumbnails issued single full-table UPDATEs. On a ~600k-item library that is 603k media_items rows (1.32M media_files) locked by one statement; one observed run held locks for 1h51m, during which 12 of 16 pool connections sat blocked behind it and ordinary playback and metadata writes stalled. Both now update in batches of 5000 through a FOR UPDATE CTE over the surface's unique key order, committing per batch, so concurrent writers interleave instead of queueing behind a table-wide writer. Consistent ordering keeps batches from deadlocking against each other; retryOnDeadlock (added here) covers deadlocks against unrelated writers that touch the same rows in a different order, an observed 40P01 source. SKIP LOCKED is deliberately not used: skipping a contended row would end the loop early and silently leave rows unreset. The loops terminate because both SET clauses falsify the predicate that selected the row — resetSet writes the provider URL into pathCol, which cachedPredicate excludes via NOT LIKE '%://%', and clearSet empties it. Ported from RXWatcher/silo-server@3b377f5c2 (batch-size const made a var so tests can exercise the multi-batch path). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Exercises bulkResetSurface and bulkResetChapterThumbnails through multiple batches against a real database (batch size shrunk via the new var), verifying requeue-vs-clear routing, the JSONB chapter rewrite, loop termination, and that elements without thumbnails survive untouched. retryOnDeadlock gets unit coverage for retry-until-success, attempt exhaustion, non-retryable errors, and cancellation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 1 minute. View limit detailsLimit details: You’ve used all 2 included reviews currently available. 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 (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughBulk artwork resets now use bounded, ordered database batches with row locking, PostgreSQL deadlock and serialization retry handling, context cancellation, accumulated statistics, and database-backed coverage for poster and chapter-thumbnail resets. ChangesArtwork reset reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This PR changes artwork reconciliation to process resets in bounded batches, reducing prolonged database locking while preserving the existing reconciliation behavior. No actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Context
participant ArtworkReconciliation
participant PostgreSQL
Context->>ArtworkReconciliation: Provide cancellation state
ArtworkReconciliation->>PostgreSQL: Lock and update one ordered batch
PostgreSQL-->>ArtworkReconciliation: Return success or retryable error
ArtworkReconciliation->>PostgreSQL: Retry failed batch with backoff
Context-->>ArtworkReconciliation: Cancel between batches or retries
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 967073b822
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The bulk resets under test sweep their whole table, and the shared test database may hold rows from other tests or a populated snapshot. Each test now snapshots every pre-existing row its reset would touch and restores it on cleanup, so only the seeded fixtures change durably. Verified by seeding decoy cached rows before the run and checking their poster path, last_refreshed, chapter thumbnail path, and retry timestamp all survive the tests byte-for-byte. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb342ce41d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bulkUpdateInBatches returns the rows already committed alongside an error, but bulkResetSurface returned before adding them to stats, so an interrupted bulk reset serialized zero requeued/cleared rows despite having durably modified thousands. Counts are now recorded before the error check in both phases, and a regression test interrupts the clear phase to prove the requeue phase's committed rows stay counted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18ba29738e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Each batch restarted its ordered scan at the smallest key, so the database rechecked every previously reset row — still in the key index but no longer matching — before reaching the next batch, making the sweep O(N²/batchSize); on 1.32M chapter-thumbnail rows that is hundreds of millions of repeated predicate checks including jsonb_array_elements evaluation. Both loops now carry the batch's last key into the next batch's WHERE, so each key range is scanned once, termination no longer depends on predicate falsification alone, and a row re-cached by a concurrent writer behind the cursor is left for the next reconcile instead of being reset twice in one sweep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d577183d04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Inserting media_items rows with local cached poster paths fires the reopen_image_ladder_backfill_v2 trigger; on a database that has completed ladder v2 that lowers the image_ladder_backfill_state singleton, and deleting the fixture rows does not restore it. Both tests that seed such rows now snapshot the singleton and restore it last (t.Cleanup runs LIFO, and the poster-row restores themselves re-fire the trigger). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0143495ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Displacing a cached poster path ending in /original.<ext> fires queue_displaced_artwork_revision, which inserts an artwork_revision_gc_candidates row or resets an existing candidate's schedule, attempts, lease, and error state. The media_items row restore alone does not undo that. Both poster tests now snapshot the candidates for every displaceable path before running, delete candidates the reset or the fixtures created, and restore pre-existing candidates column-for-column. Verified with decoys: a candidate with distinctive attempt/lease/error state survives a test run byte-for-byte, and a displaced row that had no candidate ends with none. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Restore the original GC candidate timestamp
When a populated test database already has a dormant GC candidate for a poster touched by this reset, the cleanup restores its other fields but sets updated_at to the current time. sweepDormant only considers candidates whose timestamp is more than 24 hours old (internal/metadata/artwork_revision_gc.go:398-408), so merely running this test can postpone an already-due candidate's reference check and cleanup for another day. Include updated_at in the snapshot and restore its original value rather than writing NOW().
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Problem
bulkResetSurfaceandbulkResetChapterThumbnailsininternal/metadata/artwork_reconcile.goissue single full-table UPDATEs. On a ~600k-item library that is 603kmedia_itemsrows (1.32Mmedia_files) locked by one statement. One observed run held row locks for 1h51m, during which 12 of 16 pool connections sat blocked behind it and ordinary playback and metadata writes stalled.Change
Ported from RXWatcher/silo-server@3b377f5 (credited as commit author), plus tests written here.
Both resets now update in batches of 5000 through a
FOR UPDATECTE over the surface's unique key order, committing per batch, so concurrent writers interleave instead of queueing behind a table-wide writer. AretryOnDeadlockhelper covers deadlocks against unrelated writers that touch the same rows in a different order (an observed 40P01 source).Design points, verified against the code rather than taken from the source commit:
resetSetwrites the provider URL into the path column, whichcachedPredicateexcludes viaNOT LIKE '%://%';clearSetwrites'', excluded byNOT IN ('', '-'). The chapter rewrite empties everythumbnail_pathelement theEXISTSpredicate looks for.SKIP LOCKED. Skipping a contended row would end the loop early and silently leave rows unreset; a blocked batch waits instead.Deviation from the source commit: the batch size is a
varinstead of aconst, so tests can shrink it to drive the multi-batch path without seeding thousands of rows.Tests
New in this PR (the source commit had none):
TestBulkResetSurfaceBatches— drivesbulkResetSurfaceacross multiple batches against a real database, verifying requeue-vs-clear routing, final row state, and stats.TestBulkResetChapterThumbnailsBatches— multi-batch JSONB rewrite: cached elements emptied and stripped of retry state, elements without thumbnails untouched,chapter_thumbnail_retry_afternulled.TestRetryOnDeadlock— retry-until-success, attempt exhaustion, non-retryable errors returned immediately, cancellation honored between attempts.The DB tests gate on
SILO_TEST_DATABASE_URLlike the other*_db_test.gofiles; both pass against a migrated pgvector/pg18 database.Validation
go build ./...,go vet ./...,gofmt -lcleangolangci-lint run --new-from-merge-base=origin/main ./...— 0 issuesmake test-go(CI-equivalent; DB tests skip withoutSILO_TEST_DATABASE_URL) — passesgo test ./internal/metadata/...withSILO_TEST_DATABASE_URLpointing at a migrated pgvector/pg18 database — the new DB tests pass; the only failure isTestImageLadderBackfillLateOldArtworkReopensCompletedVersion, which fails identically onorigin/main(pre-existing ambiguousimage_typecolumn reference, unrelated; being filed separately). Other packages' DB-gated tests also carry pre-existing failures onorigin/main; CI never sets the variable, so they are not maintained.Related issue: N/A — narrow fix (batching an existing maintenance operation; no API, client, or jellycompat surface changes)
AI Disclosure
(a, b) IN (SELECT ...)composite-key form and per-statement autocommit behavior were checked, and the loss of whole-reset atomicity was assessed against reconciler idempotence. Findings: the source commit shipped without tests and with a const batch size that made the multi-batch path untestable; both addressed here.🤖 Generated with Claude Code
Summary by CodeRabbit