This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7416-85c5fb2ec52b75a91d4915a61717ced425458a5e in repository https://gitbox.apache.org/repos/asf/texera.git
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",
