Advance the search read pointer from the reindex instead of a chain (PP-4908) - #3621
Advance the search read pointer from the reindex instead of a chain (PP-4908)#3621jonathangreen wants to merge 16 commits into
Conversation
A schema migration created the new index, pointed writes at it, and queued `chain(search_reindex, update_read_pointer)`. A chain only reaches its second link if the first one succeeds, and `search_reindex` raises LockNotAcquired when a beat-scheduled reindex is already running - which, on managers where a full pass takes days, is most of the time. The chain then dies at the first link and nothing ever advances the read pointer again: writes go to the new index, reads stay on the old one, and no later run repairs it because the nightly beat task queues a bare `search_reindex` with no second link. Fold the advance into the end of `search_reindex`, so whichever run first completes a full pass over the latest index publishes it. A run that dies partway through leaves the pointer where it was, so a failed migration degrades to "reads stay on the old index" rather than half-migrating. To keep that safe, `search_reindex` records the index it started filling and carries it across requeues. If the write pointer moved partway through - a deploy landing mid-run - the documents are split across two indexes and neither got a complete pass, so the pointer is left alone. `update_read_pointer` stays as the manual override for the cases the automatic advance declines to handle. `RebuildSearchIndexScript` loses its `--migration` flag, which now describes the only behaviour it has.
|
Claude finished @jonathangreen's task in 9m 56s —— View job Code Review
SummaryThe design is sound and the guards are the right ones. I traced the production scenario from the description ( DetailsMinor:
|
|
Pairs with #3622, which raises the |
Greptile SummaryThe PR moves search read-pointer advancement into the completion of a full
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/palace/manager/celery/tasks/search.py | Integrates guarded read-pointer publication into completed reindex passes, preserves the target across task replacements, and relocates retry handling into task bodies. |
| src/palace/manager/scripts/initialization.py | Queues a bare search reindex after creating a new schema index because the reindex now completes pointer publication itself. |
| src/palace/manager/scripts/search.py | Simplifies manual rebuilds to use the same reindex path and removes the obsolete migration mode. |
| tests/manager/celery/tasks/test_search.py | Adds coverage for successful publication, retries, moved pointers, partial runs, and document-conversion omissions. |
Sequence Diagram
sequenceDiagram
participant Init as Initialization/Rebuild
participant Reindex as search_reindex
participant Write as Write pointer
participant Index as Target index
participant Read as Read pointer
Init->>Reindex: Queue bare reindex
Reindex->>Write: Resolve target_index at offset 0
loop Each work-ID batch
Reindex->>Index: Submit search documents
Reindex->>Reindex: Replace task with target_index
end
Reindex->>Write: Verify target is still current
alt Target is latest and still writable
Reindex->>Read: Publish completed target
else Pointer moved or target is stale
Reindex-->>Read: Leave existing pointer unchanged
end
Reviews (13): Last reviewed commit: "Cover an index with no read pointer, and..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
jonathangreen
changed the title
The reindex now reads the search pointers on every run, which the existing tests weren't set up for: - The tests that exercise a reindex against a mocked search service got MagicMocks back from `read_pointer`/`write_pointer`, which can't be compared by version or serialized into the requeue. `mock_search_pointers` gives them a coherent service pointed at a single current index. - The tests that exercise a reindex end to end need a real Redis for the task lock, which they were only getting incidentally from the lock fixture. `test_do_run_migration_flag_removed` asserted an unknown argument exits, but `Script.parse_command_line` uses `parse_known_args` and ignores it. Drop the test rather than assert argparse behaviour for a flag that no longer exists.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3621 +/- ##
==========================================
- Coverage 93.52% 93.51% -0.02%
==========================================
Files 512 509 -3
Lines 46760 46698 -62
Branches 6379 6386 +7
==========================================
- Hits 43731 43668 -63
- Misses 1958 1959 +1
Partials 1071 1071 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A run started at a non-zero offset skips every work before it, so it has not filled the index it is writing to and must not publish it. Resolve target_index only when a run starts from the beginning; a resumed run leaves it unset, which can never match the latest index, so the advance declines on its own. The test for a reindex that dies partway through held the task lock, so the task raised LockNotAcquired before its body ran and the assertion held no matter what the advance did. Let the run index its first batch and fail the second through every retry instead, which is the case the test describes. Lock contention is already covered by test_search_reindex_lock. Note in update_read_pointer's docstring how to queue it, since it is the manual override and no longer has a CLI entry point.
The two alias reads at the end of a pass were the only OpenSearch calls in this module without retry handling, so a transient failure there threw away a pass that takes days on a large collection. Wrap them the way the other calls are wrapped. Cover the write-pointer-moved guard, in both the moved and the missing arm, and the new retry. Document set_read_pointer.
A reindex can't tell which index it is filling until it reads the write pointer, so a transient OpenSearch failure there ended the run instead of backing off like every other search call in the module. Move that read into resolve_target_index, which retries. The read pointer test indexed ten works with the default batch size, so it finished in one batch and never crossed a requeue -- dropping target_index from the requeue signature would have broken every real migration with the suite still green. Give it a batch size that forces the requeues.
A beat-scheduled run that can't take the lock is now the benign case: the incumbent run is doing the work and will publish the index itself. Declare LockNotAcquired in throws so it logs without a traceback, the way search_indexing already does, instead of an ERROR that no longer distinguishes anything. Document advance_read_pointer's task parameter.
A run that started partway through records no index to fill, and reporting that as "this reindex filled None, but the latest revision is X" reads like it filled the wrong one. Give the two causes their own messages, since this warning is what an operator reads when asking why the pointer didn't move.
The index a run is filling has to survive task.retry as well as task.replace, and no test exercised the two together: the requeue test never retried, and the retry tests were single batch, so their retry re-entered at offset 0 and simply re-resolved the target.
Only the offset-0 branch reads it, and advance_read_pointer resolves the same singleton for itself at the end.
gen_task_name strips the palace.manager.celery.tasks. prefix, so the registered name is search.update_read_pointer. The documented command used the import path, which celery call sends verbatim: it prints a task id and exits 0 while the worker discards the message as NotRegistered. Drop the suggestion to run bin/repair/search_index alongside it, since a manager that needs this repair usually has a reindex running already and a second one dies on the lock.
task.retry reschedules the whole task, so a retry raised from inside a helper reads as if only that helper is retried. The helpers now do their work and raise, and each task decides what to retry: search_reindex retries the batch it is on, from inside the lock so it keeps holding it, and index_works and update_read_pointer retry their one operation. resolve_target_index no longer needs the task at all, and the blanket catch logs with log.exception so the traceback still says which call failed.
With the retries gone the helpers only needed the task to log, and advance_read_pointer to resolve two singletons. They now take a LoggerType and their actual dependencies, so nothing outside a task touches Task. Passing task.log rather than a module-level logger keeps the per-task logger name that LoggerMixin derives from the task class.
Nothing queues it since the reindex started advancing the pointer itself: no beat entry, no chain, no CLI. It only ever existed for the case the reindex declines to handle, publishing an index that no run filled end to end, and doing that means serving reads from an index nobody verified is complete. The repair is a reindex that finishes, and PP-4908 makes the blocking rebuild run a real pass in-process, so that is a couple of hours rather than the days a queued pass takes. An escape hatch that publishes an unverified index is not worth keeping next to one that publishes a verified one.
|
|
||
| def add_documents_to_index( | ||
| task: Task, index: ExternalSearchIndex, documents: Sequence[dict[str, Any]] | ||
| log: LoggerType, index: ExternalSearchIndex, documents: Sequence[dict[str, Any]] |
There was a problem hiding this comment.
This is just a bit of refactoring to make add_documents_to_index take a log parameter instead of task, so its aligned with the rest of the helper functions defined in this PR. The retry logic here really shouldn't have lived in this function, as it applies to the whole task.
| task.services.search.revision_directory().highest(), | ||
| target_index, | ||
| ) | ||
| except (FailedToIndex, OpenSearchException) as e: |
There was a problem hiding this comment.
This except was moved here from where it was buried in add_documents_to_index before.
Work.to_search_documents leaves out any work whose document it cannot build, logging the failure and returning a shorter list than it was given. The reindex read that short list as the end of the works and stopped, so one work with a broken presentation edition ended a pass at whatever batch it landed in - and now that a completed pass publishes the index, the truncated index became the one reads are served from. Split the id query out as get_presentation_ready_work_ids, so the batch that decides whether there is more to do is the batch of works we asked for, not the documents we managed to build from them.
…pair one A missing read alias is a state advance_read_pointer handles on purpose: an index serving no reads is not already current, so a completed pass publishes it. test_update_read_pointer was what covered that, and it went with the task, so give SearchMigrationFixture a way to drop the alias and pin the behaviour where the rest of the pointer logic is tested. The startup message for a read pointer behind the write pointer sent operators to repair the index by hand. That was right when nothing advanced the pointer on its own; now the first reindex to complete a full pass publishes it, so say that instead.
1 participant
Description
Folds the search read-pointer advance into the end of
search_reindex, and deletes bothget_migrate_search_chain()and theupdate_read_pointertask.A schema migration used to create the new index, point writes at it, and queue
chain(search_reindex.si(), update_read_pointer.si()). Nowsearch_reindexadvances the read pointer itself once it has completed a full pass, so whichever run first fills the latest index publishes it.To make that safe,
search_reindexrecords the index it started filling (target_index, resolved from the write pointer at offset 0) and carries it across requeues viasignature_with. The pointer only advances when that index is both the latest revision and still the write pointer at the end. If the write pointer moved partway through — a deploy landing mid-run — the documents are split across two indexes and neither received a complete pass, so the pointer is left alone.A running search reindex that happens during a deploy will error out due to this added kwarg, but this is fine, its a one time thing during deploy and a new reindex will be rescheduled the next night anyway. I think this is better trade-off then dealing with the complexity of a task that can handle this kwarg or not.
InstanceInitializationScript.migrate_searchandRebuildSearchIndexScriptnow queue a baresearch_reindex.RebuildSearchIndexScriptloses its--migrationflag, which no longer distinguishes anything.update_read_pointeris deleted. It was the chain's second link and had no other caller — no beat entry, no CLI — and the only thing it could still do was publish an index that no single run filled end to end, which means serving reads from an index nobody verified is complete. The repair for that is a reindex that finishes, and Make the blocking search rebuild actually run in this process (PP-4908) #3623 makesRebuildSearchIndexScript --blockingrun a real pass in-process, in hours rather than the days a queued pass takes.task.retryreschedules the whole task, so a retry raised from inside a helper reads as if only that helper is retried; the helpers now raise and each task decides what to retry.search_reindexretries inside its lock, so a run that is waiting to retry keeps holding it.LoggerTyperather than aTask, so nothing outside a task body touchesTask. Passingtask.logkeeps the per-task logger name thatLoggerMixinderives from the task class.Work.to_search_documentsomits any work whose document it cannot build, so a full batch of works can come back as a short batch of documents — which the reindex read as the end of the works, stopping the pass there. That was survivable when a short pass just meant an incomplete index; now that a completed pass publishes the index, one work with a broken presentation edition would have published a truncated one.get_work_search_documentsis split intoget_presentation_ready_work_idsplus aWork.to_search_documentscall, so the batch that decides whether there is more to do is the batch of works we asked for.Motivation and Context
JIRA: PP-4908
Three production circulation managers —
ca-california,ct-connecticut,nj-newjersey— have been serving search from the v7 index since v45.1.0 rolled out on 2026-07-20, two weeks ago. Writes go to v8, reads come from v7, and nothing is repairing it.A chain only reaches its second link if the first one succeeds.
search_reindexraisesLockNotAcquiredwhen a reindex is already running, and on these managers a full pass takes 3-5 days against a dailyfull_search_reindexbeat, so a run is almost always already in flight. At deploy time the migration chain lost that race and died at the first link:The chain's second link was never received for any of the three (it ran and succeeded for
ks-kansasandwa-washington, whose nightly reindex had already finished and so left the lock free).Nothing recovers from that. The nightly beat queues a bare
search_reindexwith no second link, so the reindex has since completed several times —nj-newjersey07-21,ct-connecticut07-23 / 07-29 / 08-02,ca-california07-27 — with zero read-pointer updates in two weeks of logs. Theca-californiaandct-connecticutv8 indexes are now fully populated and still unpublished.With this change, any one of those completed runs would have finished the migration on its own.
The failure also degrades in the right direction: a run that dies partway through leaves the pointer where it was, so a stalled migration means "reads stay on the old index" rather than a half-migrated one.
How Has This Been Tested?
Eleven new tests in
tests/manager/celery/tasks/test_search.py, most built on a newSearchMigrationFixture(new revision created, write pointer moved, read pointer still on the old index). Two of them cover states nothing else reaches: a batch whose documents come back short of its works still pages to the end, and an index with no read pointer at all gets published rather than treated as current.Checklist