Remove the orphaned v5/v6/v7 search indexes

[jonathangreen]

Description

Stacked on top of #3523. Cleans up the orphaned circulation-works-v5, -v6 and -v7 OpenSearch indexes that earlier schema revisions left behind in the cluster.

When the search schema migrates to a new revision, the reindex flow creates the new index and re-points the read/write aliases at it, but it never deletes the index it replaced — so every retired revision leaves an orphaned {base}-v{n} index sitting in the cluster, consuming shards and disk. Nothing in the codebase has ever removed them. #3523 removes the v5–v7 revision code; this PR removes the leftover indexes.

This adds:

• SearchService.index_remove(name) — deletes an index if it exists and reports whether it did. The service had no delete-index capability before.
• remove_search_indices(service, versions, *, log) helper — deletes the named old versions, with a safety guard that never deletes an index a read or write alias still points at, so it cannot drop the live index even if asked to. It's idempotent (delete-if-exists) and returns the names it removed.
• A one-shot startup task (startup_tasks/2026_06_30_remove_old_search_indexes.py) that calls the helper inline for versions [5, 6, 7] on the next boot.

Removing a handful of indexes is a fast metadata operation, so this runs inline in the startup task rather than via a background job. If OpenSearch is briefly unavailable the helper raises, the startup runner declines to record the task as done, and it simply retries on the next boot.

Motivation and Context

Removing the v5–v7 revision modules (#3523) stops the code from referencing those schemas, but the actual indexes remain in the cluster forever because the migration flow only swaps aliases and never drops the old index. This reclaims that cluster state (shards, disk) as the final step of retiring those revisions.

How Has This Been Tested?

New tests, all passing under the docker tox environment:

• tests/manager/search/test_service.py::TestService::test_index_remove — index_remove deletes a real index and is a no-op (returns False) when it's absent.
• tests/manager/search/test_service.py::TestRemoveSearchIndices — three cases against the real OpenSearch fixture: the happy path (old indexes removed, the live index preserved), the alias guard (an index a read alias points at is skipped), and a no-op when the indexes don't exist.

Also ran the startup-task and initialization suites to confirm the new task file is discovered and the boot wiring is unaffected.

Checklist

• I have updated the documentation accordingly.
• All new and existing tests passed.

[claude]

Claude finished @jonathangreen's task in 3m 34s —— View job

---

Summary

Nicely scoped change — the new index_remove is minimal and correctly delegates existence handling, the alias guard is the right instinct, and the three integration tests against the real OpenSearch fixture cover the paths that matter. My one substantive concern is that the guard's "skip" is permanent: the startup-task runner records the task as RUN even when the guard skipped an index, so on a deployment that is mid-migration at boot the orphan this PR exists to remove is left behind forever.

Details

startup_tasks/2026_06_30_remove_old_search_indexes.py:26

A skipped index is never retried. InstanceInitializationScript.run() calls initialize_search() immediately before run_startup_tasks() (initialization.py:200-203), and on a deployment booting at v7 that call runs migrate_search(): it creates v8, moves the write pointer, and dispatches search_reindex → update_read_pointer asynchronously — so the read pointer is still {base}-v7 when this task runs moments later. The guard correctly skips v7, but _run_tasks then records the task as RUN (startup.py:218) and it never runs again, so v7 stays orphaned on exactly the deployments that were migrating. Consider making a guard-skip raise, so the runner declines to record the task and it retries on the next boot the same way a transient OpenSearch failure does:

removed, skipped = remove_search_indices(services.search.service(), [5, 6, 7], log=log)
if skipped:
    raise SearchServiceException(f"Aliases still point at {skipped}; will retry next boot.")

circulation/startup_tasks/2026_06_30_remove_old_search_indexes.py

Line 26 in ca017c8

remove_search_indices(services.search.service(), [5, 6, 7], log=log)

Minor: src/palace/manager/search/service.py:399-403

The guard silently protects nothing when a pointer can't be resolved. _get_pointer returns None both when the alias is absent and when it maps to more than one index (service.py:209-214) — the latter logs an error and is filtered out here, leaving protected empty, so a live index an alias still points at would be deleted. That state needs manual intervention to reach, but it's the exact scenario the guard is for. Bailing out when either pointer is None costs nothing in practice, since initialize_search() guarantees both pointers exist by the time this runs.

circulation/src/palace/manager/search/service.py

Lines 399 to 403 in ca017c8

	protected = {
	pointer.index
	for pointer in (service.read_pointer(), service.write_pointer())
	if pointer is not None
	}

---

Lint and mypy are green on ca017c8; the test workflow was still running when I looked.
| Branch: chore/cleanup-old-search-indexes

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.51%. Comparing base (2b54d93) to head (ca017c8).

Additional details and impacted files

@@                            Coverage Diff                            @@
##           chore/remove-old-search-schema-revisions    #3524   +/-   ##
=========================================================================
  Coverage                                     93.50%   93.51%           
=========================================================================
  Files                                           509      509           
  Lines                                         46663    46685   +22     
  Branches                                       6378     6382    +4     
=========================================================================
+ Hits                                          43633    43656   +23     
+ Misses                                         1959     1958    -1     
  Partials                                       1071     1071           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[greptile-apps]

Greptile Summary

This PR adds SearchService.index_remove(name), a remove_search_indices helper with an alias-safety guard, and a one-shot startup task that removes the orphaned circulation-works-v5/v6/v7 OpenSearch indexes left behind by previous schema migrations.

• index_remove(name) – new abstract method on SearchService + SearchServiceOpensearch1 implementation; uses ignore=[404] to handle the TOCTOU window between exists() and delete(), and is propagated to SearchServiceFake.
• remove_search_indices – module-level helper that builds a protected set from current read/write alias targets before iterating, ensuring the live index is never deleted even if it appears in the requested versions list.
• Startup task 2026_06_30_remove_old_search_indexes.py – runs inline (no Celery dispatch); any OpenSearchException propagates to the startup runner, which leaves the task unrecorded and retries it on the next boot.

Confidence Score: 5/5

Safe to merge — the alias guard reliably blocks deletion of any live index, and the startup runner correctly retries on failure.

The alias-safety guard in remove_search_indices is sound: it queries both read and write pointers before touching any index, so it cannot accidentally remove the live index. index_remove uses ignore=[404] to handle concurrent deletes without spurious retries. The startup runner's try/except means any OpenSearch outage at boot simply defers the task to the next startup without affecting other tasks. No logic issues were found across the four changed files.

Files Needing Attention: No files require special attention.

Important Files Changed

Filename	Overview
src/palace/manager/search/service.py	Adds abstract index_remove(name) -> bool to SearchService and its implementation in SearchServiceOpensearch1; adds module-level remove_search_indices helper with alias-safety guard. Logic is correct: exists() + delete(ignore=[404]) handles the TOCTOU window; the protected-set construction correctly prevents deletion of any index an alias still targets.
startup_tasks/2026_06_30_remove_old_search_indexes.py	One-shot startup task that calls remove_search_indices inline for v5-v7. Exceptions propagate to the startup runner which leaves the task unrecorded and retries on the next boot — consistent with the framework's error-handling contract.
tests/manager/search/test_service.py	Adds test_index_remove for the new method and TestRemoveSearchIndices with three cases (happy path, alias guard, no-op). All use a live OpenSearch fixture, providing real integration coverage.
tests/mocks/search.py	Adds _created_indices tracking to SearchServiceFake so index_remove can correctly return True/False; base_revision_name returns the same base_name used in index_create, so the fake is internally consistent with remove_search_indices.

Sequence Diagram

sequenceDiagram
    participant SR as StartupRunner
    participant ST as StartupTask (run)
    participant RSI as remove_search_indices
    participant SVC as SearchServiceOpensearch1
    participant OS as OpenSearch

    SR->>ST: run(services, session, log)
    ST->>RSI: remove_search_indices(service, [5,6,7], log)
    RSI->>SVC: read_pointer()
    SVC->>OS: get_alias(read)
    OS-->>SVC: alias → index name
    RSI->>SVC: write_pointer()
    SVC->>OS: get_alias(write)
    OS-->>SVC: alias → index name
    note over RSI: Build protected set (live index names)
    loop for each version in [5,6,7]
        RSI->>SVC: index_remove(name)
        SVC->>OS: indices.exists(name)
        OS-->>SVC: True / False
        alt exists and not protected
            SVC->>OS: "indices.delete(name, ignore=[404])"
            OS-->>SVC: OK
            SVC-->>RSI: True
        else
            SVC-->>RSI: False
        end
    end
    RSI-->>ST: removed names
    ST-->>SR: None
    SR->>SR: record task as DONE

Loading

_{Reviews (5): Last reviewed commit: "Address review feedback on the search in..." | Re-trigger Greptile}

[jonathangreen]

This is ready for review, but I'll keep it in draft until its ready to be merged

[tdilauro]

This looks good! 🏁🔥