Keep one bad custom list from blowing up the entries sweep (PP-4787)

[dbernstein]

Description

Two fixes to update_custom_list_entries, both aimed at the same failure: a single custom list with a malformed auto_update_query currently takes down the entire custom-list entries sweep.

1. Catch QueryParseException alongside RequestError.

The per-list task already intends to log-and-skip a list whose query is malformed, so that one bad list doesn't abort the sweep chord. But it only caught RequestError (an OpenSearch 400). A query can also be rejected by our own JSONQuery parser, before any request reaches OpenSearch — that raises QueryParseException, which sailed straight past the handler and failed the task.

This restores the behavior of the CustomListUpdateEntriesScript this pipeline replaced, which wrapped each list in a bare except Exception: self.log.exception(...).

One other note here: even though we are validating custom list queries when they are entered by users, it is still possible that query validation logic could tighten down the road causing existing queries to start failing. That is why I believe it is worth in submitting this PR along with #3551.

2. Release the sweep lock when a per-list task fails.

update_custom_list_entries is a chord header, and Celery runs a chord's body only if every header task succeeded. The body is finalize_custom_list_entries_sweep, which is what releases the sweep-level Redis lock — so any header failure left that lock held for its full 2-hour TTL. The sweep is beat-scheduled hourly, so the next one or two ticks would skip with "another sweep is already in progress," and then the same thing would happen again.

finalize is now registered as the chord body's error callback as well as its body. Exactly one of the two runs, so the lock is released exactly once. Infrastructure errors (database down, OpenSearch transport failure) still propagate and alarm as before — they just no longer strand the lock on their way out.

Motivation and Context

Seen in production (PP-4787). A list's stored query contained a published value of 2025>01>01 — someone typed 2025.01.01 with the shift key held down. Every sweep, that list raised:

palace.manager.search.query.QueryParseException: Could not parse 'published' value '2025>01>01'. Only use 'YYYY-MM-DD'

which failed the task, skipped the chord body, and left the sweep lock held. The net effect was that no custom list updated for any library, on a permanent, self-renewing cycle — from one bad row.

A follow-up PR (stacked on this one) adds validation of auto_update_query at save time in the admin controller, so a query that can't be parsed can't be stored in the first place.

The error was occurring on a single custom list in the California collection. I manually fixed the production issue via one off database write so we are not longer seeing the aforementioned error in production.

How Has This Been Tested?

tox -e py312-docker -- tests/manager/celery/tasks/test_custom_lists.py (24 passed).

• test_malformed_query_logged_and_swallowed is now parametrized over both RequestError and QueryParseException.
• test_finalize_registered_as_chord_error_callback asserts the error callback is wired up and carries the same lock_value as the body.
• test_failing_per_list_task_still_releases_sweep_lock is end-to-end: it runs a real sweep whose per-list task raises SQLAlchemyError, then polls until the sweep lock comes back. I confirmed this test is not vacuous by reverting the on_error line and watching it fail with "Sweep lock was not released within 10.0 seconds."

Checklist

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

🤖 Generated with Claude Code

[claude]

Claude finished @dbernstein's task in 5m 23s —— View job

---

Code Review

I reviewed both commits (c514d4e, 507a2a9) against origin/main, the surrounding task/transaction/lock machinery, and the tests.

Summary

This is a clean, well-targeted fix and I found no blocking issues. Both changes are correct and correctly scoped: QueryParseException is a BasePalaceException raised only by the JSONQuery parser, so it's genuinely a per-list config problem and belongs in the same skip-and-log path as RequestError — infrastructure errors (SQLAlchemyError, transport errors) still propagate and alarm as before. The chord error-callback is wired with .si() (immutable), which matters: it stops Celery from prepending the failed task-id as a positional arg that would otherwise clobber lock_value, and RedisLock.release() is value-checked via its Lua unlock script, so any redundant or stale release is a safe no-op that can never free a different sweep's lock. The end-to-end lock-release test runs against a real celery_session_worker (not eager mode), so it truly exercises the chord failure path rather than a simulation. I also specifically verified the Greptile P1 ("REPOPULATE can commit an emptied list"): the except (RequestError, QueryParseException) handler sits outside the with task.transaction() block, and task.transaction() is session_maker.begin(), which rolls back on exception — so the REPOPULATE bulk-delete is undone before the swallow. That invariant is now pinned by test_repopulate_parse_error_rolls_back_the_delete, so the concern is a non-issue. Nice work on the test coverage — the parametrization, the error-callback wiring assertion, and the poll-until-released end-to-end test all target the right behaviors.
· bugfix/custom-list-sweep-query-parse-error

[greptile-apps]

Greptile Summary

This PR makes the custom-list entries sweep keep running when one list has a bad query. The main changes are:

• Catches local query parse failures with malformed OpenSearch requests.
• Releases the sweep lock from the chord error path.
• Adds tests for rollback, lock release, and malformed-query handling.

Confidence Score: 5/5

This looks safe to merge.

• No blocking issues found in the changed code.

Important Files Changed

Filename	Overview
src/palace/manager/celery/tasks/custom_lists.py	Adds malformed-query handling for local parse errors and releases the sweep lock on chord failure.
tests/manager/celery/tasks/test_custom_lists.py	Adds tests for malformed-query swallowing, REPOPULATE rollback, and sweep-lock release after task failure.

_{Reviews (3): Last reviewed commit: "Pin REPOPULATE rollback on a swallowed p..." | Re-trigger Greptile}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.46%. Comparing base (65b15bf) to head (507a2a9).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3550   +/-   ##
=======================================
  Coverage   93.46%   93.46%           
=======================================
  Files         512      512           
  Lines       46611    46614    +3     
  Branches     6352     6352           
=======================================
+ Hits        43566    43570    +4     
+ Misses       1969     1968    -1     
  Partials     1076     1076           

☔ 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.

[jonathangreen]

Looks good