Commit Graph

1 Commits

Author SHA1 Message Date
Aakash Kattelu ccdb8ba113
fix(deriver): fix create_documents deadlock (#1033)
* fix(deriver): eliminate create_documents deadlock and stop silently burning batches on transient errors

Two concurrent work units writing the same (workspace, observer, observed)
collection deadlocked on times_derived reinforcement UPDATEs issued in
batch order (DEV-1975, 682 events in 90 days). The deadlock was swallowed
per-document, the loop cascaded PendingRollbackErrors against the dead
session, the whole batch was lost, and the queue item was marked processed.

- serialize writers per collection with a transaction-scoped advisory lock
  (pg_advisory_xact_lock + SET LOCAL lock_timeout), skipped for insert-only
  batches; covers all three row-lock sites in one move
- hoist external-vector-store dup-candidate resolution ahead of the first
  DB statement so the lock's critical section contains no network calls
- abort the batch on SQLAlchemyError instead of continuing through an
  aborted transaction; per-document skip semantics kept for non-DB errors
- classify transient errors (new src/utils/retryable_errors.py) and retry
  them via a bounded in-process counter instead of marking items errored

* fix(deriver): replace create_documents advisory lock with id-ordered row locks

Advisory locks are database-scoped and would serialize every writer to a
collection, including across Groudon tenants that share names. Collect
reinforcement and replace ops during the loop, lock target rows with
SELECT ... ORDER BY id FOR UPDATE, then apply. populate_existing reloads
times_derived so a prefetched identity-map row cannot lose a concurrent
increment.

* fix(deriver): harden create_documents candidate hoist and test isolation

Skip empty embeddings on the external-store path, isolate per-document
resolve failures, and keep replacement times_derived in the in-batch
ledger. Patch get_external_vector_store in the hoist test and cover
in-loop SQLAlchemyError abort.

* fix(deriver): address CodeRabbit findings on create_documents deadlock fix

- Distinguish external resolve failure ([] skip) from pgvector fallback (None)
  so _semantic_dup_decision never re-enters external I/O under an open session
- Bound external candidate hoist concurrency with a semaphore
- Map in-loop IntegrityError to ValidationException for a uniform contract
- Persist transient retry attempts on the oldest unprocessed queue item so
  every deriver instance shares one MAX_RETRYABLE_ATTEMPTS budget
- Cover resolve-failure skip and multi-manager reclaim of the retry budget

* fix(deriver): harden retry metadata cleanup and stale reinforce fallback

- Strip _retry_attempts from payloads in the same transaction as
  mark_queue_items_as_processed / mark_queue_item_as_errored
- Clear shared retry metadata only after a successful terminal mark
- On reinforce, if the locked target is gone or soft-deleted, insert the
  incoming document instead of dropping it
- Skip pgvector semantic lookup when embedding is empty so query_documents
  cannot embed under an open session

* fix(deriver): address review on deadlock retry and row-lock apply

Strip _retry_attempts before payload validation so non-representation
tasks are not burned as extra_forbidden. Re-raise retryable observer
save errors after telemetry so the queue actually retries. Skip
same-batch reinforce fallbacks after a replace. Revert unordered
FOR UPDATE on mark processed/errored and drop post-commit retry
cleanup from the success path.

* fix: add test and simplify queue query

---------

Co-authored-by: Vineeth Voruganti <13438633+VVoruganti@users.noreply.github.com>
2026-09-02 11:07:50 -04:00