Skip to content

Commit f2438d7

Browse files
committed
fix(knowledge): record dispatch time in its own column instead of overloading the start time
Two review findings on this PR traced to the same root: the queue grace was reading processingStartedAt, a column that means something else. Externally, a pending row carrying a dispatch timestamp reported a processing start time for work that had not started. Internally, updateDocument sets a document pending and refreshes uploadedAt while leaving the previous run's processingStartedAt in place, and completion never clears it — so the sweep aged a re-dispatched document from a leftover stamp rather than its actual dispatch, and could reclaim it inside the grace window while the earlier queue entry was still live. That reopens the duplicate-billing race this PR closes. processing_queued_at is written on every re-dispatch and read by the sweep; processingStartedAt goes back to meaning what its name says, so its external contract is byte-identical to before this PR. Not stamped on first dispatch: the row is created and dispatched inside the same sync run, so uploadedAt is already accurate there and a guarded write per upload would buy no behavior. The internal document route returns a full table row through a passthrough schema, so the new column would have shipped as an undeclared raw Date. Declared and serialized explicitly instead.
1 parent b5947fe commit f2438d7

11 files changed

Lines changed: 20149 additions & 30 deletions

File tree

apps/sim/lib/api/contracts/knowledge/documents.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,8 @@ export const documentDataSchema = z
267267
tokenCount: z.number(),
268268
characterCount: z.number(),
269269
processingStatus: z.enum(['pending', 'processing', 'completed', 'failed']),
270+
/** When indexing was last dispatched to a worker, which precedes a worker starting it. */
271+
processingQueuedAt: nullableWireDateSchema.optional(),
270272
processingStartedAt: nullableWireDateSchema.optional(),
271273
processingCompletedAt: nullableWireDateSchema.optional(),
272274
processingError: z.string().nullable().optional(),

apps/sim/lib/knowledge/api/internal-route.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ function serializeNullableDate(date: Date | string | null): string | null {
6060
export function toInternalKnowledgeDocument<
6161
T extends {
6262
uploadedAt: Date | string
63+
processingQueuedAt?: Date | string | null
6364
processingStartedAt?: Date | string | null
6465
processingCompletedAt?: Date | string | null
6566
date1?: Date | string | null
@@ -69,6 +70,7 @@ export function toInternalKnowledgeDocument<
6970
return documentDataSchema.parse({
7071
...document,
7172
uploadedAt: serializeDate(document.uploadedAt),
73+
processingQueuedAt: serializeNullableDate(document.processingQueuedAt ?? null),
7274
processingStartedAt: serializeNullableDate(document.processingStartedAt ?? null),
7375
processingCompletedAt: serializeNullableDate(document.processingCompletedAt ?? null),
7476
date1: serializeNullableDate(document.date1 ?? null),

apps/sim/lib/knowledge/connectors/sync-engine.test.ts

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -710,9 +710,14 @@ describe('isStuckDocumentSweepEligible', () => {
710710

711711
const candidate = (
712712
processingStatus: string,
713-
overrides: { processingStartedAt?: Date | null; uploadedAt?: Date } = {}
713+
overrides: {
714+
processingQueuedAt?: Date | null
715+
processingStartedAt?: Date | null
716+
uploadedAt?: Date
717+
} = {}
714718
) => ({
715719
processingStatus,
720+
processingQueuedAt: overrides.processingQueuedAt ?? null,
716721
processingStartedAt: overrides.processingStartedAt ?? null,
717722
uploadedAt: overrides.uploadedAt ?? minutesBefore(5),
718723
})
@@ -727,7 +732,7 @@ describe('isStuckDocumentSweepEligible', () => {
727732
expect(
728733
isStuckDocumentSweepEligible(
729734
candidate('pending', {
730-
processingStartedAt: minutesBefore(90),
735+
processingQueuedAt: minutesBefore(90),
731736
uploadedAt: minutesBefore(60 * 48),
732737
}),
733738
now
@@ -742,7 +747,7 @@ describe('isStuckDocumentSweepEligible', () => {
742747
expect(
743748
isStuckDocumentSweepEligible(
744749
candidate('pending', {
745-
processingStartedAt: minutesBefore(241),
750+
processingQueuedAt: minutesBefore(241),
746751
uploadedAt: minutesBefore(60 * 48),
747752
}),
748753
now
@@ -785,6 +790,19 @@ describe('isStuckDocumentSweepEligible', () => {
785790
expect(isStuckDocumentSweepEligible(candidate('processing'), now)).toBe(true)
786791
})
787792

793+
it('ignores a start time a worker left on a document that was requeued', () => {
794+
expect(
795+
isStuckDocumentSweepEligible(
796+
candidate('pending', {
797+
processingQueuedAt: minutesBefore(90),
798+
processingStartedAt: minutesBefore(60 * 48),
799+
uploadedAt: minutesBefore(60 * 72),
800+
}),
801+
now
802+
)
803+
).toBe(false)
804+
})
805+
788806
it('never reclaims a completed document', () => {
789807
expect(
790808
isStuckDocumentSweepEligible(

apps/sim/lib/knowledge/connectors/sync-engine.ts

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ const MAX_CONSECUTIVE_FAILURES = 10
8282
/** The processing state the stuck-document sweep decides on, one row at a time. */
8383
export interface StuckDocumentSweepCandidate {
8484
processingStatus: string
85+
processingQueuedAt: Date | null
8586
processingStartedAt: Date | null
8687
uploadedAt: Date
8788
}
@@ -98,16 +99,12 @@ export interface StuckDocumentSweepCandidate {
9899
* pass, so queued documents get {@link QUEUED_DISPATCH_GRACE_MINUTES} before
99100
* they are considered lost.
100101
*
101-
* No column records dispatch time. `uploadedAt` is the row's creation time,
102-
* which for a document dispatched by the sync that created it is within that
103-
* sync's own runtime — an over-estimate of the wait bounded by the one-hour
104-
* sync ceiling, and the best available proxy. A document the sweep itself
105-
* re-dispatched has no such proxy at all, its `uploadedAt` being arbitrarily
106-
* old, so the sweep stamps `processingStartedAt` with the re-dispatch time and
107-
* this reads it back. That is an overload of the column, but a safe one: every
108-
* other reader of `processingStartedAt` gates on `processingStatus` being
109-
* `processing` first, and a worker overwrites the stamp with its own the moment
110-
* it starts.
102+
* Queue wait is measured from `processingQueuedAt`, written by every path that
103+
* re-dispatches an existing document — this sweep and the user-facing retry.
104+
* It falls back to `uploadedAt` when NULL, which covers a document dispatched
105+
* by the sync that created it (`uploadedAt` then sits within that sync's own
106+
* runtime, an over-estimate bounded by the one-hour sync ceiling) and rows
107+
* written before the column existed.
111108
*
112109
* `failed` gets no grace. It is a terminal state: the run that produced it has
113110
* ended, so re-dispatching cannot duplicate live work, and it is the state the
@@ -132,7 +129,7 @@ export function isStuckDocumentSweepEligible(doc: StuckDocumentSweepCandidate, n
132129
case 'failed':
133130
return true
134131
case 'pending': {
135-
const queuedAt = doc.processingStartedAt ?? doc.uploadedAt
132+
const queuedAt = doc.processingQueuedAt ?? doc.uploadedAt
136133
return now.getTime() - queuedAt.getTime() > QUEUED_DISPATCH_GRACE_MINUTES * 60 * 1000
137134
}
138135
case 'processing': {
@@ -1474,6 +1471,7 @@ export async function executeSync(
14741471
fileSize: document.fileSize,
14751472
mimeType: document.mimeType,
14761473
processingStatus: document.processingStatus,
1474+
processingQueuedAt: document.processingQueuedAt,
14771475
processingStartedAt: document.processingStartedAt,
14781476
uploadedAt: document.uploadedAt,
14791477
})
@@ -1542,7 +1540,8 @@ export async function executeSync(
15421540
* tell a document still waiting for a worker from one whose
15431541
* dispatch was lost. See {@link isStuckDocumentSweepEligible}.
15441542
*/
1545-
processingStartedAt: sweepEvaluatedAt,
1543+
processingQueuedAt: sweepEvaluatedAt,
1544+
processingStartedAt: null,
15461545
processingCompletedAt: null,
15471546
processingError: null,
15481547
chunkCount: 0,

apps/sim/lib/knowledge/documents/retry-processing-grace.test.ts

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,18 +42,24 @@ describe('retryDocumentProcessing requeue stamp', () => {
4242
resetDbChainMock()
4343
})
4444

45-
it('stamps the requeue time instead of clearing it', async () => {
45+
it('stamps the requeue time on the dispatch column', async () => {
4646
const before = Date.now()
4747
const values = await captureRequeueValues()
4848
const after = Date.now()
4949

50-
expect(values.processingStartedAt).toBeInstanceOf(Date)
51-
const stamp = values.processingStartedAt as Date
50+
expect(values.processingQueuedAt).toBeInstanceOf(Date)
51+
const stamp = values.processingQueuedAt as Date
5252
expect(stamp.getTime()).toBeGreaterThanOrEqual(before)
5353
expect(stamp.getTime()).toBeLessThanOrEqual(after)
5454
expect(values.processingCompletedAt).toBeNull()
5555
})
5656

57+
it('leaves processingStartedAt null so the API reports no start time', async () => {
58+
const values = await captureRequeueValues()
59+
60+
expect(values.processingStartedAt).toBeNull()
61+
})
62+
5763
it('leaves the requeued document outside the reach of the next connector sync', async () => {
5864
const values = await captureRequeueValues()
5965
const uploadedAt = new Date(Date.now() - 30 * 24 * 60 * 60 * 1000)
@@ -63,6 +69,7 @@ describe('retryDocumentProcessing requeue stamp', () => {
6369
isStuckDocumentSweepEligible(
6470
{
6571
processingStatus: values.processingStatus as string,
72+
processingQueuedAt: values.processingQueuedAt as Date | null,
6673
processingStartedAt: values.processingStartedAt as Date | null,
6774
uploadedAt,
6875
},
@@ -72,7 +79,12 @@ describe('retryDocumentProcessing requeue stamp', () => {
7279

7380
expect(
7481
isStuckDocumentSweepEligible(
75-
{ processingStatus: 'pending', processingStartedAt: null, uploadedAt },
82+
{
83+
processingStatus: 'pending',
84+
processingQueuedAt: null,
85+
processingStartedAt: null,
86+
uploadedAt,
87+
},
7688
sweptAt
7789
)
7890
).toBe(true)

apps/sim/lib/knowledge/documents/service.ts

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2498,17 +2498,12 @@ export async function retryDocumentProcessing(
24982498
*
24992499
* The document sits at `pending` until a worker claims it, and for a
25002500
* connector-owned document the connector sweep
2501-
* (`isStuckDocumentSweepEligible`) reads `processingStartedAt ?? uploadedAt`
2502-
* to judge how long it has been queued. Clearing the column would fall the
2503-
* sweep back on `uploadedAt`, which for a document synced days ago is
2504-
* arbitrarily old — so the next sync would reclaim the document out from
2501+
* (`isStuckDocumentSweepEligible`) measures queue wait from
2502+
* `processingQueuedAt`, falling back to `uploadedAt`. Leaving it unset would
2503+
* fall the sweep back on `uploadedAt`, which for a document synced days ago
2504+
* is arbitrarily old — so the next sync would reclaim the document out from
25052505
* under this very retry, duplicating its work and billing a second indexing
2506-
* pass. Stamping the requeue time puts it back inside the grace period.
2507-
*
2508-
* Safe against the processing claim: `processDocumentAsync` writes its own
2509-
* `processingStartedAt` unconditionally when it starts and compares every
2510-
* later write against that value, so it never requires the column to be null
2511-
* to claim a document.
2506+
* pass.
25122507
*/
25132508
const requeuedAt = new Date()
25142509
await db.transaction(async (tx) => {
@@ -2518,7 +2513,8 @@ export async function retryDocumentProcessing(
25182513
.update(document)
25192514
.set({
25202515
processingStatus: 'pending',
2521-
processingStartedAt: requeuedAt,
2516+
processingQueuedAt: requeuedAt,
2517+
processingStartedAt: null,
25222518
processingCompletedAt: null,
25232519
processingError: null,
25242520
chunkCount: 0,
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
ALTER TABLE "document" ADD COLUMN "processing_queued_at" timestamp;

0 commit comments

Comments
 (0)