fix(knowledge): give failed documents a grace while their retries are still scheduled - #6924
Conversation
… still scheduled The sweep treats failed as terminal — "the run that produced it has ended" — and that was true when processing ran inline. Since dispatch became asynchronous the worker writes failed and then rethrows, and the processing task retries up to three times, so failed is a resting state between attempts rather than a final one. The sweep gave it no grace at all, so it reclaimed a document mid-retry chain, deleted its embeddings, and re-dispatched with a fresh pass id: two live runs, two indexing passes, two bills, through the one state deliberately left unguarded. Aged from processingCompletedAt, which every failure write stamps, so it reads exactly when the last attempt ended rather than when the document was dispatched. Sized at the queue grace rather than a run-duration bound. What is being waited on between attempts is a re-queue behind the same global concurrency limit, not another run, so on a large backlog the next attempt starts hours after the previous one ended and a run-duration bound would still reclaim live work. The grace itself is now an expression rather than a constant — corpus over queue concurrency, times occupancy, times a contention factor — with each input documented where it can be re-measured. The previous 240 was derived from a 2,600-document corpus and sat below the drain time of the 7,730-document connector it was written for. Rounding is up: too small silently re-bills live work, too large only delays recovery of documents nothing is processing. Also withdraws the queue stamp when every dispatch in a batch fails, so a provably-undispatched document keeps next-sync recovery instead of waiting out a grace it never earned. Scoped to the batch's own ids, to rows still pending, and by compare-and-set on the exact stamp that call wrote, so a concurrent batch that re-stamped a document keeps its grace. Best-effort by design, while the stamp write still throws: a failed stamp means the grace cannot be promised and dispatching anyway is the unsafe direction, whereas a failed withdrawal only delays recovery and must not mask the dispatch error underneath it.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
When every dispatch in Reviewed by Cursor Bugbot for commit 8383208. Configure here. |
Greptile SummaryThis PR prevents connector sweeps from reclaiming failed documents while Trigger retries may still be queued, and raises the queue grace using the documented production backlog estimate.
Confidence Score: 5/5The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking defect established in the changed paths. The failed-state grace follows the worker’s completion timestamp, and total dispatch failures withdraw only the exact pending-row stamps written by that dispatch attempt.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/knowledge/connectors/sync-engine.ts | Extends stuck-document grace to failed retrying rows, derives a longer queue grace, and selects the completion timestamp needed by the eligibility check. |
| apps/sim/lib/knowledge/documents/service.ts | Adds guarded, best-effort withdrawal of queue timestamps when no document dispatch succeeds. |
| apps/sim/lib/knowledge/connectors/sync-engine.test.ts | Covers pending and failed grace boundaries, timestamp precedence, and the pinned 773-minute derivation. |
| apps/sim/lib/knowledge/documents/retry-processing-grace.test.ts | Verifies total-failure withdrawal and its document-ID, pending-status, and timestamp compare-and-set guards. |
Sequence Diagram
sequenceDiagram
participant Sync as Connector sync
participant DB as Document row
participant Queue as Processing queue
participant Worker as Processing worker
Sync->>DB: Stamp processingQueuedAt
Sync->>Queue: Dispatch document
Queue->>Worker: Start attempt
Worker->>DB: "Set processingStatus=failed and processingCompletedAt"
Worker-->>Queue: Throw for retry
Note over DB,Queue: Sweep preserves failed row during queue grace
Queue->>Worker: Start retry
alt Every dispatch fails
Sync->>DB: Clear matching stamp while row remains pending
Sync-->>Sync: Throw dispatch error
end
Reviews (1): Last reviewed commit: "fix(knowledge): give failed documents a ..." | Re-trigger Greptile
1 participant
Live on staging. Found by an independent audit of the merged connector work — an interaction between #6920 and #6921, each correct alone.
The bug
The sweep treats
failedas terminal, documented as "the run that produced it has ended." That was true when processing ran inline. Since #6920 made dispatch asynchronous:processingStatus: 'failed'and then rethrowsmaxAttempts(default 3)So between attempts the row reads
failedwhile a live run is scheduled to retry it. The sweep gavefailedzero grace, so it reclaimed the document, deleted its embeddings, and re-dispatched with a fresh pass id — two live runs, two indexing passes, two bills. Exactly what #6911 and #6921 exist to prevent, reached through the one state deliberately left unguarded.Backoff between attempts is short, so the window is seconds per document — but it is per document across a 7,730-document corpus, and an OOM-kill reschedule is far longer than the configured backoff.
The fix
Aged from
processingCompletedAt, which every failure write stamps, so it reads exactly when the last attempt ended rather than when the document was dispatched.Sized at the queue grace, not a run-duration bound. This was the one wrong turn available:
STALE_PROCESSING_MINUTESbounds a run, but what is being waited on between attempts is a re-queue behind the same global concurrency limit. On a large backlog the next attempt starts hours after the previous one ended, so a 45-minute grace would still reclaim documents mid-retry-chain — just less often, which is the worst kind of fix.The grace is now an expression, not a constant:
Each input is documented with where to re-measure it. The previous 240 was derived from a 2,600-document corpus and sat below the drain time of the 7,730-document connector it was written for. Rounding is deliberately up: too small silently re-bills live work; too large only delays recovery of documents nothing is processing. The test pins
773so changing an input fails loudly rather than being absorbed.Recovery for a genuinely lost dispatch becomes grace + sync interval — free at the 1,440-minute default interval, and still an order of magnitude inside
RETRY_WINDOW_DAYS.Also: withdrawing an unearned stamp
The queue stamp is written before dispatch, so it records intent rather than fact. That ordering is deliberate and stays — a chunked batch can half-succeed, and stamping after dispatch would strand the documents that genuinely are executing. Before-dispatch fails safe; after-dispatch fails unsafe.
But in the total-failure branch we can prove nothing dispatched, so the stamp is withdrawn there before it throws. Scoped three ways: to the batch's own ids, to rows still
pending, and by compare-and-set on the exact stamp that call wrote — so a concurrent batch that has already re-stamped a document keeps its grace.The withdrawal is best-effort while the stamp write still throws. Asymmetric on purpose: a failed stamp means the grace cannot be promised and dispatching anyway is the unsafe direction, whereas a failed withdrawal only delays recovery by one grace and must not mask the dispatch error underneath it.
Verification
type-checkclean ·check:audits32/32 · 754 tests acrosslib/knowledgeand all four knowledge API suites.Ten mutations, all red. The flagship — restoring
failedto unconditionally sweepable, i.e. exactly today's staging behavior — turns four tests red. Three more separately kill each scoping guarantee on the withdrawal (own ids,pendingguard, own-stamp CAS), and two kill the derivation inputs.One thing the tests cannot cover, stated rather than implied: that the withdrawal fires only on total failure. Under test
isTriggerAvailable()is false, so every path goes in-process and fails — there is no way to make a dispatch succeed, so a mutation moving the clear outside thedispatched === 0branch would not be caught. That branch is three lines and reviewable by eye.Follow-up, deliberately not here
A
processing_attemptscolumn would convert the deterministic-OOM case from an unbounded seven-day re-sweep loop into a real resolution: increment inside the funnel's existing guarded write, reset on successful completion, sweep skips above N, leaving the document atfailedwith its error visible and the user's retry button still working.The same counter doubles as a dispatch generation — a worker carrying the value it was dispatched with can decline to bill when the row has moved past it, which is the state-based guarantee a timing grace structurally cannot give. Those two belong in one change rather than two columns a month apart, and both deserve their own review rather than riding on a hotfix.