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(...).

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.

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 8m 12s —— View job

---

PR Review: Keep one bad custom list from wedging the entries sweep (PP-4506)

• Read the diff (custom_lists.py, test_custom_lists.py)
• Trace where QueryParseException / RequestError are raised in the per-list path
• Verify the chord error-callback ("exactly one of the two runs") mechanism
• Check whether other list-specific exceptions (e.g. json.JSONDecodeError) can still wedge the sweep
• Review the new tests and the _wait_for_sweep_lock_release helper
• Post review

Summary

This is a solid, well-scoped fix and the tests are genuinely convincing. Both changes are correct: QueryParseException is raised by the JSONQuery parser inside wl.search (e.g. the published transform at search/query.py:791), so it belongs in the same list-specific "log-and-skip" bucket as an OpenSearch RequestError, and catching it restores the pre-pipeline behavior. The chord-body error-callback is the right tool for the lock leak — Celery invokes a chord body's link_error when a header task fails (via chord_error_from_stack), which is exactly the abort path that used to strand the sweep lock, and using .si correctly suppresses errback arg-injection so lock_value binds cleanly. finalize releasing via _sweep_lock(...).release() is value-scoped, so even in the unlikely event both the body and its errback fired, the second release is a harmless no-op rather than a correctness problem. I especially appreciated that test_failing_per_list_task_still_releases_sweep_lock exercises the real worker end-to-end with a genuine SQLAlchemyError and that you confirmed it isn't vacuous — that's the assertion that actually pins down the fix.

I looked specifically for a remaining hole where a bad row could still abort the sweep: populate_query_pages does its own json.loads(custom_list.auto_update_query) (core/query/customlist.py:131) in INIT/REPOPULATE mode, which raises a raw json.JSONDecodeError rather than QueryParseException. That turns out to be unreachable in practice — the admin controller only ever stores json.dumps(auto_update_query) (api/admin/controller/custom_lists.py:165), so the stored string is always valid JSON — so it's not worth widening the catch here. No blocking issues found; nothing to change.
· bugfix/custom-list-sweep-query-parse-error

[greptile-apps]

Greptile Summary

This PR makes custom-list entry sweeps recover from bad per-list queries. The main changes are:

• Catch QueryParseException alongside OpenSearch RequestError.
• Register the sweep finalizer as the chord body's error callback.
• Add tests for malformed queries and lock release after failed per-list tasks.

Confidence Score: 4/5

The repopulate failure path needs a fix before merging.

• A malformed query during REPOPULATE can leave a custom list empty.
• The new lock-release behavior is covered by targeted tests.
• The parser-error tests do not cover the state that deletes entries before the parse failure.

src/palace/manager/celery/tasks/custom_lists.py

Important Files Changed

Filename	Overview
src/palace/manager/celery/tasks/custom_lists.py	Handles parser errors per list and wires finalization onto the chord error path, but the swallowed parser error can commit an already-cleared repopulating list.
tests/manager/celery/tasks/test_custom_lists.py	Adds coverage for malformed-query exception handling and lock release, but does not cover the repopulate path where entries are deleted before parsing fails.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Beat starts custom-list sweep] --> B{Acquire sweep lock?}
    B -- No --> C[Skip sweep]
    B -- Yes --> D[Run per-list update chord]
    D --> E{Per-list query malformed?}
    E -- Yes --> F[Log and skip that list]
    E -- Other task failure --> G[Chord error callback]
    E -- All tasks succeed --> H[Chord body]
    F --> H
    G --> I[Finalize releases sweep lock]
    H --> I

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[Beat starts custom-list sweep] --> B{Acquire sweep lock?}
    B -- No --> C[Skip sweep]
    B -- Yes --> D[Run per-list update chord]
    D --> E{Per-list query malformed?}
    E -- Yes --> F[Log and skip that list]
    E -- Other task failure --> G[Chord error callback]
    E -- All tasks succeed --> H[Chord body]
    F --> H
    G --> I[Finalize releases sweep lock]
    H --> I

Loading

_{Reviews (1): Last reviewed commit: "Keep one bad custom list from wedging th..." | Re-trigger Greptile}

[greptile-apps]

Repopulate Can Commit Empty Lists

When a list is in REPOPULATE, _setup_first_invocation deletes its existing entries before populate_query_pages parses and runs the query. With this new catch, a QueryParseException after that delete is logged and swallowed, so the transaction can commit with the list emptied and still stuck in REPOPULATE instead of preserving the last good entries or rolling back.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.46%. Comparing base (7061612) to head (ad810db).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3550   +/-   ##
=======================================
  Coverage   93.46%   93.46%           
=======================================
  Files         512      512           
  Lines       46614    46614           
  Branches     6352     6352           
=======================================
  Hits        43570    43570           
  Misses       1968     1968           
  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.