fix(datasets): return early when deleteDatasetRuns matches no runs - #17788
MoonlightAndSunlight wants to merge 2 commits into
Conversation
| if (datasetRuns.length === 0) { | ||
| return datasetRuns; | ||
| } |
There was a problem hiding this comment.
Cleanup retries are skipped
If PostgreSQL deletion succeeds but enqueueing the asynchronous ClickHouse cleanup fails, retrying this mutation finds no relational runs. This new return then exits before recreating the cleanup job, even when input.datasetId supplies the required dataset context, so the run's related data can remain undeleted. Limit the early return to cases where no dataset ID is available, or otherwise preserve cleanup retries.
Knowledge Base Used: Datasets and experiments
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/features/datasets/server/dataset-router.ts
Line: 2113-2115
Comment:
**Cleanup retries are skipped**
If PostgreSQL deletion succeeds but enqueueing the asynchronous ClickHouse cleanup fails, retrying this mutation finds no relational runs. This new return then exits before recreating the cleanup job, even when `input.datasetId` supplies the required dataset context, so the run's related data can remain undeleted. Limit the early return to cases where no dataset ID is available, or otherwise preserve cleanup retries.
**Knowledge Base Used:** [Datasets and experiments](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse/-/docs/datasets-and-experiments.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.greptile pointed out that returning early on an empty result skips the async cleanup enqueue. A retry after a committed delete whose enqueue failed would then never re-enqueue, leaving the ClickHouse side undeleted. The early return now only skips the enqueue when the caller gave no dataset id to enqueue with.
|
Addressed in Correct — returning early skipped the enqueue, so a retry after a committed delete whose enqueue Re-enqueueing is safe: the queue handler deletes by run id. |
What does this PR do?
deleteDatasetRunsfalls back todatasetRuns[0].datasetIdwhen filling in the queue payload,but
datasetIdis optional in the input schema. If the caller omits it and none of the supplieddatasetRunIdsmatch — the runs were already deleted, or they belong to another project —findManycomes back empty,datasetRuns[0]isundefined, and reading.datasetIdthrows. Therequest fails instead of reporting that there was nothing to delete.
The handler now returns as soon as the lookup comes back empty. Nothing is deleted, queued or
audited either way, so the early return only removes the crash; the response shape is unchanged.
Why return early instead of raising
The mutation is already idempotent in the other direction: it returns the runs it found, and its
audit entries — which are correct as written, since they are built from the
findManyresult ratherthan from the input — simply do not exist when nothing matched. Turning "nothing matched" into a 404
would be a behaviour change beyond the crash this PR is fixing, so the existing contract is kept.
Type of change
Mandatory Tasks
Checklist
Left empty on purpose: every bullet in the template is phrased as something that was not done, and
I removed the ones that did not apply.
No test added: nothing in the suite exercises
deleteDatasetRuns, and pinning this would need aseeded dataset run plus a tRPC caller. Happy to add one if you want it.
The PR is not safe to merge until the empty-result guard preserves cleanup retries when
datasetIdis supplied.Summary
This PR prevents
deleteDatasetRunsfrom dereferencing an absent first result when no runs match. However, the new unconditional return also bypasses a valid asynchronous-cleanup retry path when the caller supplied a dataset ID.datasetRuns[0]access.Reviews (1) · Last reviewed commit: "fix(datasets): return early when deleteD..."