This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git
The following commit(s) were added to refs/heads/main by this push:
new 802a388d28 test(frontend): cover aborting a dataset upload and its
conflict retry (#7416)
802a388d28 is described below
commit 802a388d28befb53becb764aee1e0af5fff01117
Author: Xinyuan Lin <[email protected]>
AuthorDate: Fri Aug 7 22:03:54 2026 -0700
test(frontend): cover aborting a dataset upload and its conflict retry
(#7416)
### What changes were proposed in this PR?
`onClickAbortUploadProgress` was the largest uncovered block in
`DatasetDetailComponent` and the one with the most ways to go wrong.
Aborting an in-flight upload has to survive the backend still finalizing
a previous attempt, so the abort is retried on 409:
| Response | Behaviour |
|---|---|
| success | notify, report the abort |
| 404 | already gone — report the abort, no error |
| 409, attempt < `ABORT_RETRY_MAX_ATTEMPTS` | retry after
`ABORT_RETRY_BACKOFF_BASE_MS * (attempt + 1)` |
| 409 at the limit, or any other status | give up, but still report the
abort |
Adds 9 tests over that ladder plus the surrounding bookkeeping: the
abort flag on the request, the task moving to `aborted`, the progress
subscription being dropped so a late event cannot resurrect it, the
concurrency slot being released so a queued upload starts, and
`cancelExistingUpload` delegating here for an upload still running. Both
constants are exported, so the backoff growth and the attempt bound are
asserted rather than hard-coded.
**Verified by mutation**, all reverted (production diff empty):
| Mutation | Result |
|---|---|
| never retry on conflict | red |
| make the retry unbounded | red |
| use a constant backoff instead of a growing one | red |
| skip the unsubscribe | red |
| leave the task unmarked | red |
| send the abort flag as false | red |
| drop the `onUploadComplete()` that frees the slot | red |
| remove the 404 early return | **survived** |
| remove the `doneCalled` idempotence guard | **survived** |
The two survivors are reported rather than papered over, because they
are informative:
- **The 404 early return is behaviourally redundant.** Without it a 404
falls past the 409 check to the same `done()` at the bottom, so no input
distinguishes the two. The test still earns its place — it fails if 404
is ever turned into an error path — but it does not pin the branch
itself.
- **The `doneCalled` guard is not reachable.** Exactly one of the
`next`/404/fallback paths fires per response, and each retry replaces
the subscription, so `done()` is never invoked twice. It is defensive
code with no observable behaviour at this level.
The slot-release mutation survived my first pass too; unlike the other
two that was a genuine gap, so I added the test that covers it.
No production file is touched.
### Any related issues, documentation, discussions?
Closes #7413
### How was this PR tested?
```
npx ng test --watch=false --include="**/dataset-detail.component.spec.ts"
```
```
Test Files 1 passed (1)
Tests 104 passed (104)
```
9 new on top of the existing 95. `yarn format:ci` passes.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
---
.../dataset-detail.component.spec.ts | 146 ++++++++++++++++++++-
1 file changed, 145 insertions(+), 1 deletion(-)
diff --git
a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts
b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts
index 88e0937ac8..43d9481167 100644
---
a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts
+++
b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts
@@ -22,7 +22,11 @@ import { ActivatedRoute, Router } from "@angular/router";
import { of, Subject, throwError } from "rxjs";
import { NzModalService } from "ng-zorro-antd/modal";
import { MarkdownService } from "ngx-markdown";
-import { DatasetDetailComponent } from "./dataset-detail.component";
+import {
+ DatasetDetailComponent,
+ ABORT_RETRY_BACKOFF_BASE_MS,
+ ABORT_RETRY_MAX_ATTEMPTS,
+} from "./dataset-detail.component";
import { DatasetService, MultipartUploadProgress } from
"../../../../service/user/dataset/dataset.service";
import { NotificationService } from
"../../../../../common/service/notification/notification.service";
import { DownloadService } from
"../../../../service/user/download/download.service";
@@ -131,6 +135,146 @@ describe("DatasetDetailComponent upload queue", () => {
fixture.detectChanges();
});
+ /**
+ * Aborting an in-flight upload has to survive the backend still finalizing
the previous attempt:
+ * the abort call is retried on 409 up to ABORT_RETRY_MAX_ATTEMPTS, a 404
means it is already gone,
+ * and the caller's callback must fire exactly once down every one of those
paths.
+ */
+ describe("aborting an upload", () => {
+ let finalize: ReturnType<typeof vi.fn>;
+
+ beforeEach(() => {
+ vi.useFakeTimers();
+ finalize = TestBed.inject(DatasetService).finalizeMultipartUpload as
unknown as ReturnType<typeof vi.fn>;
+ });
+
+ afterEach(() => {
+ vi.useRealTimers();
+ });
+
+ /** Starts an upload and reports progress, leaving one task in flight. */
+ function inFlight(name = "a.txt") {
+ dropFiles(name);
+ uploadSubjects[0].next({ filePath: name, percentage: 10, status:
"uploading", totalTime: 0 });
+ return component.uploadTasks.find(t => t.filePath === name)!;
+ }
+
+ const conflict = () => throwError(() => ({ status: 409 }) as any);
+ const gone = () => throwError(() => ({ status: 404 }) as any);
+
+ it("marks the task aborted and tells the caller once", () => {
+ const task = inFlight();
+ const onAborted = vi.fn();
+
+ component.onClickAbortUploadProgress(task as any, onAborted);
+
+ expect(finalize).toHaveBeenCalledWith("[email protected]",
"test-dataset", "a.txt", true);
+ expect(component.uploadTasks.find(t => t.filePath ===
"a.txt")!.status).toBe("aborted");
+ expect(onAborted).toHaveBeenCalledTimes(1);
+ });
+
+ it("stops listening to the upload it aborted", () => {
+ const task = inFlight();
+
+ component.onClickAbortUploadProgress(task as any);
+
+ // The progress stream is unsubscribed, so a late event cannot resurrect
the task.
+ uploadSubjects[0].next({ filePath: "a.txt", percentage: 100, status:
"finished", totalTime: 1 });
+ expect(component.uploadTasks.find(t => t.filePath ===
"a.txt")!.status).toBe("aborted");
+ });
+
+ it("treats a 404 as already aborted rather than an error", () => {
+ finalize.mockReturnValueOnce(gone());
+ const task = inFlight();
+ const onAborted = vi.fn();
+
+ component.onClickAbortUploadProgress(task as any, onAborted);
+
+ expect(onAborted).toHaveBeenCalledTimes(1);
+ expect(finalize).toHaveBeenCalledTimes(1);
+ });
+
+ it("retries a 409 after a backoff and finishes once the server catches
up", () => {
+ // The server is still finalizing the previous attempt; the abort has to
wait it out.
+ finalize.mockReturnValueOnce(conflict());
+ const task = inFlight();
+ const onAborted = vi.fn();
+
+ component.onClickAbortUploadProgress(task as any, onAborted);
+ expect(onAborted).not.toHaveBeenCalled();
+
+ vi.advanceTimersByTime(ABORT_RETRY_BACKOFF_BASE_MS);
+
+ expect(finalize).toHaveBeenCalledTimes(2);
+ expect(onAborted).toHaveBeenCalledTimes(1);
+ });
+
+ it("backs off further on each successive conflict", () => {
+ finalize.mockReturnValue(conflict());
+ const task = inFlight();
+
+ component.onClickAbortUploadProgress(task as any);
+ expect(finalize).toHaveBeenCalledTimes(1);
+
+ // First wait is BASE * 1, the second BASE * 2, so BASE alone is not
enough for the third call.
+ vi.advanceTimersByTime(ABORT_RETRY_BACKOFF_BASE_MS);
+ expect(finalize).toHaveBeenCalledTimes(2);
+
+ vi.advanceTimersByTime(ABORT_RETRY_BACKOFF_BASE_MS);
+ expect(finalize).toHaveBeenCalledTimes(2);
+
+ vi.advanceTimersByTime(ABORT_RETRY_BACKOFF_BASE_MS);
+ expect(finalize).toHaveBeenCalledTimes(3);
+ });
+
+ it("gives up after the attempt limit but still reports the abort", () => {
+ // Without the bound this would retry forever against a permanently
conflicted server.
+ finalize.mockReturnValue(conflict());
+ const task = inFlight();
+ const onAborted = vi.fn();
+
+ component.onClickAbortUploadProgress(task as any, onAborted);
+ vi.advanceTimersByTime(ABORT_RETRY_BACKOFF_BASE_MS *
ABORT_RETRY_MAX_ATTEMPTS * (ABORT_RETRY_MAX_ATTEMPTS + 1));
+
+ expect(finalize).toHaveBeenCalledTimes(ABORT_RETRY_MAX_ATTEMPTS + 1);
+ expect(onAborted).toHaveBeenCalledTimes(1);
+ });
+
+ it("reports the abort once even on an error the retry does not cover", ()
=> {
+ finalize.mockReturnValueOnce(throwError(() => ({ status: 500 }) as any));
+ const task = inFlight();
+ const onAborted = vi.fn();
+
+ component.onClickAbortUploadProgress(task as any, onAborted);
+
+ expect(onAborted).toHaveBeenCalledTimes(1);
+ expect(finalize).toHaveBeenCalledTimes(1);
+ });
+
+ it("frees the concurrency slot so a queued upload can start", () => {
+ // Aborting has to release the slot as an ordinary completion would;
otherwise the queue
+ // stalls behind an upload that is no longer running.
+ dropFiles("a.txt", "b.txt", "c.txt", "d.txt");
+ expect(uploadedPaths).toEqual(["a.txt", "b.txt", "c.txt"]);
+ uploadSubjects[0].next({ filePath: "a.txt", percentage: 10, status:
"uploading", totalTime: 0 });
+ const task = component.uploadTasks.find(t => t.filePath === "a.txt")!;
+
+ component.onClickAbortUploadProgress(task as any);
+
+ expect(uploadedPaths).toContain("d.txt");
+ });
+
+ it("cancelExistingUpload aborts an upload that is still running", () => {
+ inFlight("b.txt");
+ const onCanceled = vi.fn();
+
+ component.cancelExistingUpload("b.txt", onCanceled);
+
+ expect(finalize).toHaveBeenCalledWith("[email protected]",
"test-dataset", "b.txt", true);
+ expect(onCanceled).toHaveBeenCalledTimes(1);
+ });
+ });
+
describe("contributor cards", () => {
const full: Contributor = {
name: "Contributor A",
