Add hourly cron to garbage-collect ingest jobs with no documents#143
Add hourly cron to garbage-collect ingest jobs with no documents#143tifa2UP wants to merge 2 commits into
Conversation
Deleting documents never deletes the parent ingest job, so jobs whose documents were all deleted linger forever (and keep squatting on their namespace-unique externalId). A new hourly trigger.dev cron sweeps terminal (COMPLETED/FAILED/CANCELLED) jobs that have had no documents for 24 hours and deletes them. - keyset-paginated scan, one deleteMany + one grouped counter decrement per namespace instead of per-job operations - full filter re-checked inside the delete transaction so concurrent deleteIngestJob runs, re-ingests (fresh updatedAt), and namespace teardowns (status != ACTIVE) are never raced or double-decremented - counters and ingest_job.deleted webhooks are driven by the exact set of ids the transaction deleted - new emitBulkIngestJobWebhooks helper fetches the org webhook list once per namespace group (mirrors emitBulkDocumentWebhooks)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| deletedIds = await db.$transaction(async (tx) => { | ||
| // re-check the FULL filter at delete time: a job that gained a | ||
| // concurrent deleteIngestJob run (status -> DELETING), was | ||
| // touched since the scan (fresh updatedAt resets its grace), or | ||
| // sits in a namespace that started tearing down is skipped | ||
| const candidates = await tx.ingestJob.findMany({ | ||
| where: { id: { in: ids }, ...emptyJobFilter }, | ||
| select: { id: true }, | ||
| }); | ||
| if (candidates.length === 0) return []; | ||
|
|
||
| const candidateIds = candidates.map((candidate) => candidate.id); | ||
| const { count } = await tx.ingestJob.deleteMany({ | ||
| where: { id: { in: candidateIds }, ...emptyJobFilter }, | ||
| }); | ||
|
|
||
| // rare: a row changed between the two statements — re-derive | ||
| // exactly which ids this transaction deleted, so webhooks are | ||
| // emitted for precisely those and counters match reality | ||
| let deleted = candidateIds; | ||
| if (count !== candidateIds.length) { | ||
| const survivors = await tx.ingestJob.findMany({ | ||
| where: { id: { in: candidateIds } }, | ||
| select: { id: true }, | ||
| }); | ||
| const surviving = new Set( | ||
| survivors.map((survivor) => survivor.id), | ||
| ); | ||
| deleted = candidateIds.filter((id) => !surviving.has(id)); | ||
| } | ||
|
|
||
| if (deleted.length > 0) { | ||
| await tx.namespace.update({ | ||
| where: { id: namespaceId }, | ||
| data: { | ||
| totalIngestJobs: { decrement: deleted.length }, | ||
| organization: { | ||
| update: { totalIngestJobs: { decrement: deleted.length } }, | ||
| }, | ||
| }, | ||
| select: { id: true }, |
There was a problem hiding this comment.
After deleteMany returns a count mismatch, the survivors query runs under READ COMMITTED isolation, meaning it sees rows deleted by any committed transaction — including a concurrent deleteIngestJob that committed between the re-check findMany and the deleteMany. A job that deleteIngestJob deleted in that narrow window will appear absent from survivors and be added to deleted, causing the cron to decrement totalIngestJobs a second time and emit a duplicate ingest_job.deleted webhook for it.
The protection against deleteIngestJob relies on deleteIngestJob updating status to DELETING (which is not in DELETABLE_STATUSES) before the cron's re-check findMany. If that status update commits after the re-check findMany but the full deletion (status update + delete + counter) commits before the cron's deleteMany, the row disappears from survivors even though our deleteMany got 0 for it — and the counter and webhook double-fire.
A reliable fix is to use the count from deleteMany as the authoritative total (using Prisma $queryRaw with RETURNING id is the cleanest approach), so only rows our transaction actually removed drive counter and webhook logic. The 24-hour grace period makes this race very rare in practice, but the code comment on line 148 explicitly claims correctness in this scenario.
| }; | ||
|
|
||
| let totalDeleted = 0; | ||
| let totalSkipped = 0; |
There was a problem hiding this comment.
The while (batches < MAX_BATCHES) guard exits once batches === MAX_BATCHES, after which the warning "Cleanup stopped at the batch cap with jobs remaining" fires unconditionally. However, if the total number of eligible jobs is exactly MAX_BATCHES × BATCH_SIZE, the last batch fills and batches reaches the cap — but the next iteration would return zero rows and stop naturally. The warning fires claiming there are remaining jobs when there are none.
Consider checking whether the final batch was full (jobs.length < BATCH_SIZE) before emitting the warning, to avoid a spurious alert that could mask real cap-hits.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if (deleted.length > 0) { | ||
| await tx.namespace.update({ | ||
| where: { id: namespaceId }, | ||
| data: { | ||
| totalIngestJobs: { decrement: deleted.length }, | ||
| organization: { | ||
| update: { totalIngestJobs: { decrement: deleted.length } }, | ||
| }, | ||
| }, | ||
| select: { id: true }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
needs to update page counters / other counters too too
There was a problem hiding this comment.
Dug into this before changing it, because I initially expected the same thing — turns out for jobs that can reach this cron, the remaining page/doc delta is provably zero, and decrementing here would double-count:
- Pages/documents are document-lifecycle counters: they're incremented per-document during ingest (
ingest.tsincrementstotalDocumentsat doc creation,totalPagesat job end for completed docs), and every path that deletes a document settles them at that moment —delete-document.ts(updateCounters: true) decrementsnamespace.totalPages/totalDocuments+org.totalDocumentsand creditsorg.deletedPages;delete-ingest-job.tsapplies the same shape once, aggregated. Job creation only ever incrementstotalIngestJobs— so that's the only counter an empty job still owes, which this cron settles. - The two ways a terminal job ends up empty: (1) all its docs were deleted through the paths above → their pages/docs were already settled per-document (a second decrement here would double-count them); (2) it never produced documents (empty crawl, failed before doc creation) → nothing was ever counted, so the only correct delta is zero.
- Mechanically there's also nothing to decrement by: the
ingest_jobrow stores no page/doc totals, and by definition there are no document rows left to sum.
That said — the underlying instinct is right that page counters do drift, just via other paths, and the magnitude is unknowable by the time the doc rows are gone (so no per-job decrement here can repair it):
delete-ingest-job.ts— a child doc-delete that fails all retries gets cascade-deleted by the parent'singestJob.deletewith zero counter adjustment; a parent crash after children deleted rows strands their counters permanently (job stuck inDELETING).delete-namespace.ts:63-74— child results are never inspected; failed children's jobs/docs get cascade-deleted while onlytotalNamespacesis decremented →org.totalPages/totalDocuments/totalIngestJobsinflated forever.re-ingest.ts:108-119— setscompletedAt: nullon already-counted docs without touching counters; if the resync fails and the doc is later deleted,delete-document.ts:141credits 0 and the pages leak (this is the likely mechanism behind the stuck 33ktotalPagesin the customer report).
Happy to do a follow-up PR with the source fixes for those three + a reconciliation pass (recompute namespace.totalPages/totalDocuments from live docs for quiescent namespaces and push any org-side excess into deletedPages so the existing billing-cycle release clears it — keeps the org.totalPages == Σ namespace.totalPages + deletedPages invariant intact). If you're seeing a specific path where docs vanish without counter updates that isn't one of these three, point me at it and I'll trace it.
- Replace the candidates-select + deleteMany + survivors-select with a single DELETE ... RETURNING inside the transaction. The survivors re-select ran on a fresh READ COMMITTED snapshot, so a job deleted by a concurrent deleteIngestJob (its status flip and final delete are milliseconds apart for empty jobs) could be misattributed to the cron's own deleted set, double-decrementing totalIngestJobs and double-emitting the webhook. RETURNING reports exactly the rows this statement removed. Raw SQL because Prisma has no deleteManyAndReturn; identifiers validated with EXPLAIN against the dev schema. - Only warn about the batch cap when the final page was full (a sweep landing exactly on the cap is a natural finish). - Drop unused status from the scan select.
2 participants
Why
Deleting documents (individually or "delete all") never deletes the parent ingest job — only
deleteIngestJobremoves job rows. So jobs whose documents were all deleted linger forever. Besides the stale rows, they permanently squat on their@@unique([namespaceId, externalId])slot, blocking that externalId from ever being reused.What
A new hourly trigger.dev cron (
cleanup-empty-ingest-jobs,0 * * * *) sweeps ingest jobs that have no documents and deletes them, keepingnamespace.totalIngestJobs/organization.totalIngestJobsin sync and emittingingest_job.deletedwebhooks.Eligibility — a job is only collected when all of these hold:
COMPLETED/FAILED/CANCELLED). In-flight jobs legitimately have no documents yet;DELETING/CANCELLING/QUEUED_FOR_DELETEare owned by another workflow that deletes the row and updates counters itself.documents: { none: {} }→NOT EXISTS, probed via the existingdocument(ingestJobId, …)index).updatedAtolder than 24h. The grace keeps zero-result crawls and failed jobs (the user's error surface) visible for a day, and gives API consumers polling a terminal job a day to read its final status before the id 404s.ACTIVE— a namespace mid-teardown owns its jobs (and deliberately suppresses webhooks for them).Efficiency / correctness on critical infra:
id > cursor, PK order) in batches of 100, hard cap of 1000 batches/run with a warning log — the cap and any skips are logged, never silent.deleteMany+ one grouped counter decrement (namespace + nested org) per group, instead of per-job writes.deleteIngestJob(status →DELETING), touched since the scan (freshupdatedAtresets its grace), or whose namespace started tearing down is skipped — no double-decrements, no deleting under an active workflow.emitBulkIngestJobWebhookshelper that fetches the org's webhook list once per namespace group (mirrors the existingemitBulkDocumentWebhooks), early-exits when the org has no webhooks, and is best-effort after commit (failure logs instead of re-sweeping already-deleted rows).queue: { concurrencyLimit: 1 }so hourly runs never overlap; the loop is idempotent under retries regardless.Notes / follow-ups
statusindex has poor selectivity; the dominant cost is theNOT EXISTSprobes, which no ingest_job index removes. In steady state each run costs one pass of index probes over terminal jobs — fine at current table sizes. If it ever shows up in db metrics, the better shape is event-driven: check-and-delete the parent job in the document-deletion path and keep this cron as a backstop.GRACE_MS) — easy to tune if we want empties gone faster.packages/emails--jsxerrors, identical onmain.Greptile Summary
This PR adds an hourly Trigger.dev cron job that garbage-collects ingest jobs whose documents were all deleted — a gap where the parent job row lingered indefinitely and permanently occupied its
@@unique([namespaceId, externalId])slot. It also introducesemitBulkIngestJobWebhooksas a clean mirror of the existingemitBulkDocumentWebhookshelper.DELETE ... RETURNINGthat re-checks every eligibility condition atomically inside a transaction, and per-namespace batched counter decrements — making it correct under concurrentdeleteIngestJobruns.ACTIVE, preventing races with in-flight workflows and namespace teardown.queue: { concurrencyLimit: 1 }ensures runs never overlap; the task directory is already included intrigger.config.tsso no registration change is needed.Confidence Score: 5/5
Safe to merge; the cron is idempotent, isolated behind a concurrency limit, and carefully guards against double-decrements via RETURNING-based accounting.
The concurrency design is thorough: the DELETE … RETURNING re-check eliminates the double-decrement/double-webhook class of bugs, keyset pagination guarantees forward progress even when jobs are skipped, and the catch block correctly absorbs mid-sweep namespace teardown. The one new finding is a minor maintainability nit (raw 'ACTIVE' string in SQL instead of the imported constant). All previously identified concerns have been addressed in the current code.
No files require special attention; cleanup-empty-ingest-jobs.ts has the most logic but its concurrency guards are well-reasoned.
Important Files Changed
status: "DELETING"regardless of the actual terminal statusemitBulkIngestJobWebhookswrapper mirroring the existingemitBulkDocumentWebhookspattern; thin delegation to the base implementation, no issuesemitBulkIngestJobWebhooksas a direct mirror ofemitBulkDocumentWebhooks; fetches webhooks once per namespace group, filters and fans out viaPromise.all, consistent with existing patternsReviews (2): Last reviewed commit: "Fix deleted-set attribution race and rev..." | Re-trigger Greptile