Skip to content

fix(knowledge): stop recovery from re-dispatching documents whose runs are still queued - #7992

Closed
waleedlatif1 wants to merge 2 commits into
stagingfrom
fix/knowledge-processing-redispatch-loop
Closed

waleedlatif1 wants to merge 2 commits into
stagingfrom
fix/knowledge-processing-redispatch-loop

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • Recovery (the outbox sweeper and the connector stuck-document sweep) treats a pending document older than the 4h queue grace as a lost dispatch and installs a new generation plus a new run. A queue has no bound on wait time (Trigger.dev keeps an unstarted run for 14 days), so when the backlog grew past the grace, recovery re-dispatched documents whose original run was still queued. The replacement joined the same backlog, and each sweep added another run per waiting document.
  • The token fence already stopped superseded runs from doing real work: they exit at the claim without parsing or embedding. They still started a worker, though. Every replacement also landed at the back of the queue and spent another processing attempt, so under a sustained backlog a document could burn its whole budget and be dead-lettered at pending without being processed once.
  • Every knowledge-process-document dispatch (batch dispatch, recovery relay, quota/provider continuations) now gets a Trigger.dev ttl. It expires the run at its generation's queue stamp + QUEUED_DISPATCH_START_DEADLINE_MS (grace − 30 min). By the time recovery can replace a generation, its run can no longer start, so each document has at most one startable run. The deadline comes from the stamp, so a late outbox relay only gets the time its generation has left, and a delayed run's TTL is counted from the end of its delay.
  • Replacing a never-claimed queued generation (pending, stamped, not deferred, past grace) no longer charges a second attempt. Its unused charge moves to the replacement, applied the same way in recovery, the connector sweep, and a user retry via releaseUnclaimedDispatchAttempt.
  • The connector resurrect step now only writes rows that are actually tombstoned (deleted_at IS NOT NULL). Before, every page rewrote deleted_at = NULL on every verified live document.
  • Left persistDocumentAcls as is. acl_verified_at has to be refreshed every admin sync, so the row is written anyway, and because acl_verified_at isn't indexed and an unchanged acl keeps the update HOT-eligible, skipping the acl column would save nothing measurable.

Type of Change

  • Bug fix

Testing

  • New unit tests cover the run expiry math, the TTL at the batch and continuation dispatch sites, the retry attempt release, and the resurrect guard.
  • New integration tests run on real Postgres:
    • A document whose runs keep expiring is recovered more times than MAX_PROCESSING_ATTEMPTS without spending budget, and is then indexed exactly once while the superseded generations no-op.
    • The connector sweep moves an expired generation's charge instead of adding one.
  • Checked that every new test fails with the source changes reverted, then restored the changes and confirmed the diff was byte-identical.
  • bun run type-check, lint, check:api-validation, and check:audits all pass.
  • The related unit suites (knowledge, outbox) pass, and so do these knowledge integration suites: stored-document-recovery, connector-lifecycle-locks, provider/embedding processing recovery, listing-continuation, connector-persistence-regressions, connector-deferral, ocr-input-failures, gmail/calendar member, member-document-lifecycle.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercel Bot commented Sep 18, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
docs Skipped Skipped Sep 18, 2026 11:40pm UTC

Request Review

@greptile-apps

greptile-apps Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge; no actionable correctness, security, or repository-rule violations remain.

Summary

This PR prevents recovery from repeatedly dispatching documents whose Trigger.dev runs remain queued.

  • Assigns each processing run a TTL ending before its generation becomes recoverable.
  • Transfers the attempt charge from an expired, unclaimed generation to its replacement.
  • Applies consistent attempt accounting across stored-document recovery, connector recovery, and user retries.
  • Avoids redundant connector resurrection updates for documents that are not tombstoned.
  • Adds unit and PostgreSQL integration coverage for expiry, dispatch, recovery, and attempt-budget behavior.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Document generation stamped and charged] --> B[Trigger.dev run queued with TTL]
    B --> C{Worker starts before deadline?}
    C -->|Yes| D[Claim generation and process document]
    C -->|No| E[Run expires unstarted]
    E --> F[Recovery observes pending generation after grace]
    F --> G[Release unused attempt charge]
    G --> H[Install replacement generation and dispatch new run]
    H --> B
    D --> I[Complete or retain charge on processing failure]
Loading

Reviews (2) · Last reviewed commit: "test(knowledge): prove sweep dispatches ..."

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 15 files

Confidence score: 4/5

  • apps/sim/lib/knowledge/__integration__/connector-lifecycle-locks.integration.ts does not verify replacement generation or its charge because the mocked processDocumentsWithQueue performs no database write, leaving the lifecycle behavior insufficiently covered; exercise the real queue path or use a DB-aware mock and assert the persisted replacement and charge.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/sim/lib/knowledge/__integration__/connector-lifecycle-locks.integration.ts">

<violation number="1" location="apps/sim/lib/knowledge/__integration__/connector-lifecycle-locks.integration.ts:284">
P2: This test never verifies the replacement generation or its charge because `processDocumentsWithQueue` is mocked to perform no database write. Run this case through the real queue path, or use a DB-aware mock and assert the replacement token/outbox record and final attempt count.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 15 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@waleedlatif1
waleedlatif1 deleted the fix/knowledge-processing-redispatch-loop branch September 19, 2026 05:26

This branch was previously deployed

1 inactive deployment
Preview 8f362e94 Deployed Sep 18, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant