mirror of
https://github.com/onyx-dot-app/onyx.git
synced 2026-10-22 03:14:06 -05:00
Evan LohnandClaude Opus 5.5
0c0ba3bb58
## 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.
- [x] [Optional] Override Linear Check
🤖 Generated with [Claude Code](https://claude.com/claude-code)
<!-- This is an auto-generated description by cubic. -->
---
## 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.
<sup>Written for commit ad461953e2.
Summary will update on new commits.</sup>
<a
href="https://cubic.dev/pr/onyx-dot-app/onyx/pull/16391?utm_source=github"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img
alt="View guided diff"
src="https://www.cubic.dev/buttons/review-in-cubic-light.svg"></picture></a>
<a
href="https://www.cubic.dev/action/auto-fix/pr/onyx-dot-app/onyx/16391?returnTo=https%3A%2F%2Fgithub.com%2Fonyx-dot-app%2Fonyx%2Fpull%2F16391&source=description"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/turn-on-auto-fix-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/turn-on-auto-fix-light.svg"><img
alt="Turn on auto-fix"
src="https://www.cubic.dev/buttons/turn-on-auto-fix-light.svg"></picture></a>
<!-- End of auto-generated description by cubic. -->
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
<!-- coderabbit-ai-generated:v1 -->
## 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.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
