fix(indexing): serialize pause with index attempt creation

Merged
EV
13h ago
+351-53

Description

Observation. test_mock_connector_checkpoint_recovery timed out once in 20 local runs. It timed out while it waited for the recovery attempt to start, before any batch reissue.

Cause. The first attempt fails. Before the test pauses the cc_pair, check_for_indexing can start an automatic retry. When the test calls run-once, that retry can still be in progress, because the pause has not cancelled it yet. run-once (trigger_indexing_for_cc_pair) skips a cc_pair that has an unfinished attempt. It still returns success: true with data: 0. So no trigger was set, and no recovery attempt ran.

Product gap. A pause was not serialized with attempt creation. The creator (_try_creating_attempt and the backfill and waiting-attempt creators) read the cc_pair as ACTIVE under the Redis creation lock. The pause (update_cc_pair_status) did not take that lock or any row lock. So a pause could read the active attempts, the creator could then commit a new attempt and send its task, and the pause could set PAUSED. The pause never cancelled that attempt.

Fix (product). A row lock on the cc_pair (SELECT ... FOR UPDATE):

  • New lock_connector_credential_pair_row in db/connector_credential_pair.py. It replaces the private copy in db/connector_edit_requests.py, which now uses it.
  • The three creators in task_creation_utils.py take the row lock inside the Redis creation lock, check the status under it, and hold it until the attempt commits. Every return path commits, which releases the lock.
  • The pause route takes the row lock first, before it reads the attempts to cancel, and holds it until the status commit. The cancel no longer commits once per attempt (that released the lock early). The route revokes the tasks after the commit. The single-attempt cancel route also revokes after its commit.
  • The deletion route (POST /admin/deletion-attempt) takes the same lock before it cancels attempts and sets DELETING.
  • Lock order: the Redis creation lock, then the cc_pair row, then its attempt rows. The connector edit apply already uses the pair row before the attempt rows.

Fix (test).

  • New IndexAttemptManager.wait_for_no_unfinished_attempts: it waits until every attempt of the cc_pair is terminal, and returns the attempts. With the row lock, no attempt is created after the pause commits, so the helper no longer waits out a scheduler tick.
  • The test calls it after the pause, before it resets the mock server. This also stops a late retry from reading the recovery behavior.
  • CCPairManager.run_once now returns the trigger count (data). The test asserts that it is 1.

I checked the other integration tests that call run-once. None calls it while an automatic retry can still be running. test_repeated_error_state enters the repeated error state only when the latest attempts are all FAILED, and that state stops automatic retries. The other callers use a connector without retries, or call it after a successful attempt.

Product note. run-once reports success when it skips every cc_pair. The count is in data and the message says "Marked 0 index attempts". A caller that only checks the status does not see the skip. This PR does not change that behavior.

This does not overlap with #16377, which fixes the batch reissue race in the same test.

How Has This Been Tested?

  • New external dependency unit test test_pause_serializes_with_attempt_creation.py: real threads stop the creator between its status check and its insert, start the pause, and assert that the pause cancels the new attempt and revokes its task. It fails on the old code (the attempt stays NOT_STARTED with no cancel request) and passes 5 runs out of 5 with the fix.
  • Existing tests on these paths pass (233): docfetching priority, first indexing hold (includes pause), backfill attempts, pending backfills, connector edit requests, cc_pair run history, connector deletion, external_dependency_unit/server/documents, unit/onyx/server/documents.
  • Earlier commits: tests/integration/tests/indexing/test_checkpointing.py passed 3 runs out of 3 against an isolated Docker stack. ty check and pre-commit pass on the changed files.

Additional Options

  • [Optional] Please cherry-pick this PR to the latest release version.
  • [Optional] Override Linear Check

🤖 Generated with Claude Code

Summary by cubic

Fixes a race where pausing or deleting a cc-pair could miss an index attempt created at the same time, leaving it running, and fixes the flaky checkpoint recovery test that timed out because run-once skips a cc_pair with an unfinished attempt while still returning success.

  • Attempt creation (new attempt, pending backfill, dispatch of a waiting attempt) now takes the cc-pair row lock inside the Redis creation lock and holds it until the attempt commits.
  • Pause and delete take the same row lock before cancelling attempts, so a creation runs entirely before or after the cancellation; the pause now commits once and revokes Celery tasks only after the status commit.
  • A new unit test asserts that a pause starting during attempt creation cancels the new attempt.
  • The checkpoint recovery test now waits until all the pair's attempts are terminal before calling run-once, and run_once returns the number of triggers set, which the test asserts is 1.

Summary by CodeRabbit

  • Bug Fixes

    • Improved coordination between indexing, pausing, and deletion to prevent new indexing attempts from starting during status changes.
    • Ensured active indexing tasks are canceled and revoked consistently when a connector is paused or deleted.
    • Improved checkpoint recovery handling so only the expected recovery trigger is created.

  • Tests

    • Added validation for concurrent pause and indexing-attempt creation.
    • Added polling and timeout handling to confirm indexing attempts reach a terminal state.