From 1e9f89121d06ab5ef59606b626d46c0979653b79 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Mon, 17 Aug 2026 11:57:46 -0700 Subject: [PATCH 1/7] feat(notebook-migration, frontend): upload notebooks under a per-workflow filename --- .../jupyter-notebook-panel.component.spec.ts | 22 +++++---------- .../jupyter-notebook-panel.component.ts | 6 ++-- .../jupyter-panel.service.spec.ts | 20 ++++++++++++- .../jupyter-panel/jupyter-panel.service.ts | 24 ++++++++++++++-- .../notebook-migration.service.spec.ts | 28 +++++++++++++++---- .../notebook-migration.service.ts | 28 +++++++++++-------- 6 files changed, 89 insertions(+), 39 deletions(-) diff --git a/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.spec.ts b/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.spec.ts index 74d2e702983..f6991c2cea7 100644 --- a/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.spec.ts +++ b/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.spec.ts @@ -20,7 +20,6 @@ import { ComponentFixture, fakeAsync, TestBed, tick } from "@angular/core/testing"; import { JupyterNotebookPanelComponent } from "./jupyter-notebook-panel.component"; import { JupyterPanelService } from "../../service/jupyter-panel/jupyter-panel.service"; -import { NotebookMigrationService } from "../../service/notebook-migration/notebook-migration.service"; import { Subject } from "rxjs"; import { ElementRef } from "@angular/core"; import { By, DomSanitizer } from "@angular/platform-browser"; @@ -30,7 +29,6 @@ describe("JupyterNotebookPanelComponent", () => { let fixture: ComponentFixture; let mockJupyterPanelService: any; - let mockNotebookMigrationService: any; let bypassSpy: ReturnType; beforeEach(async () => { @@ -39,18 +37,12 @@ describe("JupyterNotebookPanelComponent", () => { setIframeRef: vi.fn(), deleteJupyterNotebook: vi.fn(), minimizeJupyterNotebookPanel: vi.fn(), - }; - - mockNotebookMigrationService = { - getJupyterIframeURL: vi.fn().mockResolvedValue("http://localhost:8888"), + getJupyterIframeURLForWorkflow: vi.fn().mockResolvedValue("http://localhost:8888"), }; await TestBed.configureTestingModule({ imports: [JupyterNotebookPanelComponent], - providers: [ - { provide: JupyterPanelService, useValue: mockJupyterPanelService }, - { provide: NotebookMigrationService, useValue: mockNotebookMigrationService }, - ], + providers: [{ provide: JupyterPanelService, useValue: mockJupyterPanelService }], }).compileComponents(); }); @@ -98,7 +90,7 @@ describe("JupyterNotebookPanelComponent", () => { await fixture.whenStable(); fixture.detectChanges(); - expect(mockNotebookMigrationService.getJupyterIframeURL).toHaveBeenCalled(); + expect(mockJupyterPanelService.getJupyterIframeURLForWorkflow).toHaveBeenCalled(); expect(bypassSpy).toHaveBeenCalledWith("http://localhost:8888"); expect(component.jupyterUrl).toBe(bypassSpy.mock.results[0].value); }); @@ -144,20 +136,20 @@ describe("JupyterNotebookPanelComponent", () => { it("should not update jupyterUrl when the iframe URL fetch rejects", async () => { vi.spyOn(component, "checkIframeRef").mockImplementation(() => {}); vi.spyOn(console, "error").mockImplementation(() => {}); - mockNotebookMigrationService.getJupyterIframeURL.mockRejectedValueOnce(new Error("network error")); + mockJupyterPanelService.getJupyterIframeURLForWorkflow.mockRejectedValueOnce(new Error("network error")); mockJupyterPanelService.jupyterNotebookPanelVisible$.next(true); await fixture.whenStable(); - expect(mockNotebookMigrationService.getJupyterIframeURL).toHaveBeenCalled(); + expect(mockJupyterPanelService.getJupyterIframeURLForWorkflow).toHaveBeenCalled(); expect(component.jupyterUrl).toBeNull(); }); it("should keep handling visibility emissions after a failed fetch", async () => { vi.spyOn(component, "checkIframeRef").mockImplementation(() => {}); vi.spyOn(console, "error").mockImplementation(() => {}); - mockNotebookMigrationService.getJupyterIframeURL + mockJupyterPanelService.getJupyterIframeURLForWorkflow .mockRejectedValueOnce(new Error("network error")) .mockResolvedValueOnce("http://localhost:9999"); @@ -168,7 +160,7 @@ describe("JupyterNotebookPanelComponent", () => { await fixture.whenStable(); fixture.detectChanges(); - expect(mockNotebookMigrationService.getJupyterIframeURL).toHaveBeenCalledTimes(2); + expect(mockJupyterPanelService.getJupyterIframeURLForWorkflow).toHaveBeenCalledTimes(2); expect(bypassSpy).toHaveBeenCalledWith("http://localhost:9999"); expect(component.jupyterUrl).toBe(bypassSpy.mock.results[0].value); }); diff --git a/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.ts b/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.ts index 4f3d957b515..71b0411fd6b 100644 --- a/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.ts +++ b/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.ts @@ -22,7 +22,6 @@ import { JupyterPanelService } from "../../service/jupyter-panel/jupyter-panel.s import { from, of, Subject } from "rxjs"; import { catchError, switchMap, takeUntil } from "rxjs/operators"; import { DomSanitizer, SafeResourceUrl } from "@angular/platform-browser"; -import { NotebookMigrationService } from "../../service/notebook-migration/notebook-migration.service"; import { CommonModule } from "@angular/common"; import { DragDropModule } from "@angular/cdk/drag-drop"; import { NzButtonModule } from "ng-zorro-antd/button"; @@ -45,8 +44,7 @@ export class JupyterNotebookPanelComponent implements OnInit, AfterViewInit, OnD constructor( private jupyterPanelService: JupyterPanelService, - private sanitizer: DomSanitizer, - private notebookMigrationService: NotebookMigrationService + private sanitizer: DomSanitizer ) {} ngOnInit(): void { @@ -59,7 +57,7 @@ export class JupyterNotebookPanelComponent implements OnInit, AfterViewInit, OnD return of(null); } - return from(this.notebookMigrationService.getJupyterIframeURL()).pipe( + return from(this.jupyterPanelService.getJupyterIframeURLForWorkflow()).pipe( catchError(() => { console.error("Failed to fetch Jupyter iframe URL."); return of(null); diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts index 43347dfdf1c..2cf0a4d9528 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts @@ -266,7 +266,8 @@ describe("JupyterPanelService", () => { // The mapping is stored before the notebook is handed to Jupyter, ... expect(mockNotebook.setMapping).toHaveBeenCalledWith("mapping_wid_1", mapping); // ... and the 0 came from Jupyter's own answer, not from a thrown error. - expect(mockNotebook.sendNotebookToJupyter).toHaveBeenCalledWith(notebook); + // Upload uses the wid-derived filename. + expect(mockNotebook.sendNotebookToJupyter).toHaveBeenCalledWith(notebook, "notebook_1.ipynb"); expect(consoleError).not.toHaveBeenCalled(); }); @@ -284,6 +285,16 @@ describe("JupyterPanelService", () => { expect(mockNotebook.sendNotebookToJupyter).not.toHaveBeenCalled(); }); + // Iframe URL must use the same wid-derived filename as the upload. + it("getJupyterIframeURLForWorkflow requests the current workflow's per-workflow filename", async () => { + mockNotebook.getJupyterIframeURL = vi.fn().mockResolvedValue("http://iframe"); + + const url = await service.getJupyterIframeURLForWorkflow(); + + expect(url).toBe("http://iframe"); + expect(mockNotebook.getJupyterIframeURL).toHaveBeenCalledWith("notebook_1.ipynb"); + }); + // jupyterNotebookExists$ starts false and flips true once init()'s fetch finds // a notebook for the workflow; the toolbar's expand button binds to this. it("sets jupyterNotebookExists$ true after a workflow's notebook is fetched", async () => { @@ -705,6 +716,13 @@ describe("JupyterPanelService", () => { expect(mockNotification.warning).not.toHaveBeenCalled(); }); + it("getJupyterIframeURLForWorkflow resolves null without calling the migration service", async () => { + mockNotebook.getJupyterIframeURL = vi.fn(); + const url = await service.getJupyterIframeURLForWorkflow(); + expect(url).toBeNull(); + expect(mockNotebook.getJupyterIframeURL).not.toHaveBeenCalled(); + }); + it("onWorkflowComponentClick does not postMessage to the iframe", async () => { const mockIframe = { contentWindow: { postMessage: vi.fn() }, diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts index 6a14b98efed..d338da9fd4d 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts @@ -25,7 +25,11 @@ import { HttpClient, HttpHeaders } from "@angular/common/http"; import { NotificationService } from "src/app/common/service/notification/notification.service"; import { distinctUntilChanged, switchMap } from "rxjs/operators"; import { AppSettings } from "../../../common/app-setting"; -import { NotebookMigrationService, notebookMappingKey } from "../notebook-migration/notebook-migration.service"; +import { + NotebookMigrationService, + notebookMappingKey, + notebookFileName, +} from "../notebook-migration/notebook-migration.service"; import { GuiConfigService } from "../../../common/service/gui-config.service"; @Injectable({ @@ -139,7 +143,11 @@ export class JupyterPanelService { if (response.exists) { this.notebookMigrationService.setMapping(notebookMappingKey(workflowID), response.mapping); - if ((await this.notebookMigrationService.sendNotebookToJupyter(response.notebook)) == 1) { + const sent = await this.notebookMigrationService.sendNotebookToJupyter( + response.notebook, + this.currentNotebookFileName() + ); + if (sent == 1) { return 1; } else { return 0; @@ -205,6 +213,18 @@ export class JupyterPanelService { this.iframeRef = iframe; } + // Single source for the current workflow's notebook filename, used by both the upload + // and the iframe fetch so they can't derive different names. + private currentNotebookFileName(): string { + return notebookFileName(this.workflowActionService.getWorkflow().wid); + } + + // Iframe URL for the current workflow's notebook + public getJupyterIframeURLForWorkflow(): Promise { + if (!this.enabled) return Promise.resolve(null); + return this.notebookMigrationService.getJupyterIframeURL(this.currentNotebookFileName()); + } + // Open the Jupyter Notebook panel public openPanel(panelName: string): void { if (!this.enabled) return; diff --git a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts index 2b47325fdbc..05699cfb218 100644 --- a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts +++ b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts @@ -18,7 +18,7 @@ */ import { TestBed } from "@angular/core/testing"; -import { NotebookMigrationService, notebookMappingKey } from "./notebook-migration.service"; +import { NotebookMigrationService, notebookMappingKey, notebookFileName } from "./notebook-migration.service"; import { HttpClient } from "@angular/common/http"; import { HttpClientTestingModule, HttpTestingController } from "@angular/common/http/testing"; import { NotificationService } from "src/app/common/service/notification/notification.service"; @@ -100,11 +100,12 @@ describe("NotebookMigrationService", () => { it("should send notebook successfully and return 1", async () => { const mockNotebook: any = { cells: [] }; - const promise = service.sendNotebookToJupyter(mockNotebook); + const promise = service.sendNotebookToJupyter(mockNotebook, "notebook_1.ipynb"); const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/set-notebook")); expect(req.request.method).toBe("POST"); + expect(req.request.body.notebookName).toBe("notebook_1.ipynb"); req.flush({ success: true }); @@ -117,7 +118,7 @@ describe("NotebookMigrationService", () => { it("should handle error when sending notebook and return 0", async () => { const mockNotebook: any = { cells: [] }; - const promise = service.sendNotebookToJupyter(mockNotebook); + const promise = service.sendNotebookToJupyter(mockNotebook, "notebook_1.ipynb"); const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/set-notebook")); @@ -135,7 +136,7 @@ describe("NotebookMigrationService", () => { // Error` branch. No request reaches the testing backend, so verify() stays happy. vi.spyOn(TestBed.inject(HttpClient), "post").mockReturnValue(throwError(() => new Error("network down"))); - const result = await service.sendNotebookToJupyter({ cells: [] } as any); + const result = await service.sendNotebookToJupyter({ cells: [] } as any, "notebook_1.ipynb"); expect(result).toBe(0); expect(mockNotificationService.error).toHaveBeenCalledWith(expect.stringContaining("network down")); @@ -175,6 +176,18 @@ describe("NotebookMigrationService", () => { const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/get-jupyter-iframe-url")); expect(req.request.method).toBe("GET"); + // No name given, so no notebookName query param is sent. + expect(req.request.params.has("notebookName")).toBe(false); + req.flush({ success: true, url: "http://iframe" }); + + expect(await promise).toBe("http://iframe"); + }); + + it("sends the notebookName as a query param when one is given", async () => { + const promise = service.getJupyterIframeURL("notebook_1.ipynb"); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/get-jupyter-iframe-url")); + expect(req.request.params.get("notebookName")).toBe("notebook_1.ipynb"); req.flush({ success: true, url: "http://iframe" }); expect(await promise).toBe("http://iframe"); @@ -235,6 +248,11 @@ describe("NotebookMigrationService", () => { expect(notebookMappingKey(42)).toBe("mapping_wid_42"); }); + it("notebookFileName builds a per-workflow filename from the wid, defaulting when absent", () => { + expect(notebookFileName(42)).toBe("notebook_42.ipynb"); + expect(notebookFileName(undefined)).toBe("notebook.ipynb"); + }); + // deleteNotebookAndMapping it("should call deleteNotebookAndMapping API with the wid", () => { let result: any; @@ -318,7 +336,7 @@ describe("NotebookMigrationService", () => { }); it("sendNotebookToJupyter returns 0 with no HTTP call or notification", async () => { - const result = await service.sendNotebookToJupyter({ cells: [] } as any); + const result = await service.sendNotebookToJupyter({ cells: [] } as any, "notebook_1.ipynb"); expect(result).toBe(0); expect(mockNotificationService.success).not.toHaveBeenCalled(); expect(mockNotificationService.error).not.toHaveBeenCalled(); diff --git a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts index 5c636240f36..6fb73ba3162 100644 --- a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts +++ b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts @@ -61,6 +61,12 @@ export function notebookMappingKey(wid: number | undefined): string { return "mapping_wid_" + wid; } +// Per-workflow notebook filename so workflows don't overwrite each other's notebook. +// Falls back to the default when there's no wid. +export function notebookFileName(wid: number | undefined): string { + return wid ? `notebook_${wid}.ipynb` : "notebook.ipynb"; +} + @Injectable({ providedIn: "root", }) @@ -132,16 +138,12 @@ export class NotebookMigrationService { return new NotebookMigrationLLM(this.config, this.workflowUtilService); } - public async sendNotebookToJupyter(notebookData: Notebook) { + public async sendNotebookToJupyter(notebookData: Notebook, notebookName: string) { if (!this.enabled) return 0; const jupyterAPIUrl = `${AppSettings.getApiEndpoint()}/notebook-migration/set-notebook`; const requestBody = { - // Fixed filename is intentional for the v1 per-user-pod design: each user runs - // their own notebook-migration-service and Jupyter, so a single notebook.ipynb - // never collides. A shared multi-user (global) service would need per-user or - // per-workflow keying here and for the backend's process-global jupyterIframeURL. - notebookName: "notebook.ipynb", + notebookName: notebookName, notebookData: notebookData, }; @@ -182,14 +184,16 @@ export class NotebookMigrationService { } } - public async getJupyterIframeURL(): Promise { + public async getJupyterIframeURL(notebookName?: string): Promise { if (!this.enabled) return null; try { - const data = await firstValueFrom( - this.http.get<{ success: boolean; url?: string }>( - `${AppSettings.getApiEndpoint()}/notebook-migration/get-jupyter-iframe-url` - ) - ); + const url = `${AppSettings.getApiEndpoint()}/notebook-migration/get-jupyter-iframe-url`; + // Send notebookName when given; otherwise the backend uses its default. + const params: Record = {}; + if (notebookName) { + params["notebookName"] = notebookName; + } + const data = await firstValueFrom(this.http.get<{ success: boolean; url?: string }>(url, { params })); if (!data.success || !data.url) { console.error("Jupyter server unavailable"); From d73efb99a302aafe332867d648a159ffac4ce6a8 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Mon, 17 Aug 2026 14:30:52 -0700 Subject: [PATCH 2/7] fix(notebook-migration, frontend): upload under the fetched workflow's filename --- .../jupyter-panel/jupyter-panel.service.spec.ts | 17 +++++++++++++++++ .../jupyter-panel/jupyter-panel.service.ts | 5 ++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts index 2cf0a4d9528..cd4abcdabc2 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts @@ -285,6 +285,23 @@ describe("JupyterPanelService", () => { expect(mockNotebook.sendNotebookToJupyter).not.toHaveBeenCalled(); }); + it("uploads under the fetched workflow's filename even if the current workflow changed", async () => { + // Stale-fetch guard: a fetch for wid 2 that resolves after the user switched to wid 1 + // must still upload as notebook_2.ipynb, not overwrite wid 1's file. + mockNotebook.sendNotebookToJupyter = vi.fn().mockResolvedValue(1); + mockWorkflow.getWorkflow.mockReturnValue({ wid: 1 }); + const mapping = { cell_to_operator: {}, operator_to_cell: {} }; + const notebook = { cells: [] }; + + const resultPromise = firstValueFrom((service as any).fetchNotebookAndMapping(2, 1)); + httpMock + .expectOne(r => r.url.includes("/notebook-migration/fetch-notebook-and-mapping")) + .flush({ exists: true, mapping, notebook }); + + expect(await resultPromise).toBe(1); + expect(mockNotebook.sendNotebookToJupyter).toHaveBeenCalledWith(notebook, "notebook_2.ipynb"); + }); + // Iframe URL must use the same wid-derived filename as the upload. it("getJupyterIframeURLForWorkflow requests the current workflow's per-workflow filename", async () => { mockNotebook.getJupyterIframeURL = vi.fn().mockResolvedValue("http://iframe"); diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts index d338da9fd4d..ecb5c90eca5 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts @@ -145,7 +145,7 @@ export class JupyterPanelService { const sent = await this.notebookMigrationService.sendNotebookToJupyter( response.notebook, - this.currentNotebookFileName() + notebookFileName(workflowID) ); if (sent == 1) { return 1; @@ -213,8 +213,7 @@ export class JupyterPanelService { this.iframeRef = iframe; } - // Single source for the current workflow's notebook filename, used by both the upload - // and the iframe fetch so they can't derive different names. + // Notebook filename for the workflow currently shown, used by the iframe fetch. private currentNotebookFileName(): string { return notebookFileName(this.workflowActionService.getWorkflow().wid); } From a077e3290e2515d0e21622c298f896b63657da39 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Mon, 17 Aug 2026 15:02:50 -0700 Subject: [PATCH 3/7] feat(notebook-migration): remove a deleted notebook's file from the jupyter pod (only delete notebook button) --- .../jupyter-panel.service.spec.ts | 18 ++- .../jupyter-panel/jupyter-panel.service.ts | 13 ++- .../notebook-migration.service.spec.ts | 32 +++++ .../notebook-migration.service.ts | 18 +++ .../resource/NotebookMigrationResource.scala | 109 ++++++++++++------ .../NotebookMigrationResourceSpec.scala | 84 +++++++++++++- 6 files changed, 229 insertions(+), 45 deletions(-) diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts index cd4abcdabc2..99ee28a15d9 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts @@ -88,6 +88,9 @@ describe("JupyterPanelService", () => { setMapping: vi.fn(), getJupyterURL: vi.fn().mockResolvedValue("http://jupyter"), deleteNotebookAndMapping: vi.fn().mockReturnValue(of({ success: true, deleted: 1 })), + // In the base mock because every successful delete fires it; a missing stub would + // throw inside the delete subscription rather than failing a targeted assertion. + deleteNotebookFromJupyter: vi.fn().mockResolvedValue(1), }; mockGuiConfig = { env: { pythonNotebookMigrationEnabled: true } }; @@ -146,6 +149,14 @@ describe("JupyterPanelService", () => { expect(mockWorkflow.unhighlightLinks).toHaveBeenCalled(); }); + it("deleteJupyterNotebook removes the pod's copy under the workflow's filename", () => { + service.deleteJupyterNotebook(); + + // The file cleanup keys off the same wid-derived name the upload uses, so the + // notebook_.ipynb left in the pod is the one being deleted. + expect(mockNotebook.deleteNotebookFromJupyter).toHaveBeenCalledWith("notebook_1.ipynb"); + }); + it("deleteJupyterNotebook keeps the panel open and notifies on failure", () => { mockNotebook.deleteNotebookAndMapping.mockReturnValueOnce(throwError(() => new Error("boom"))); let visible: boolean | null = null; @@ -157,6 +168,8 @@ describe("JupyterPanelService", () => { expect(mockNotification.error).toHaveBeenCalled(); expect(visible).toBe(true); expect(mockNotebook.deleteMapping).not.toHaveBeenCalled(); + // The notebook is still stored, so its file must stay in the pod. + expect(mockNotebook.deleteNotebookFromJupyter).not.toHaveBeenCalled(); }); it("deleteJupyterNotebook only resets local state for the default wid 0 (no backend call)", () => { @@ -170,8 +183,10 @@ describe("JupyterPanelService", () => { service.deleteJupyterNotebook(); - // wid 0 is the unsaved default workflow, so no backend delete should fire. + // wid 0 is the unsaved default workflow, so neither backend delete should fire: + // nothing is stored and no notebook file was ever uploaded for it. expect(mockNotebook.deleteNotebookAndMapping).not.toHaveBeenCalled(); + expect(mockNotebook.deleteNotebookFromJupyter).not.toHaveBeenCalled(); expect(visible).toBe(false); expect(exists).toBe(false); }); @@ -715,6 +730,7 @@ describe("JupyterPanelService", () => { service.deleteJupyterNotebook(); expect(mockNotebook.deleteNotebookAndMapping).not.toHaveBeenCalled(); expect(mockNotebook.deleteMapping).not.toHaveBeenCalled(); + expect(mockNotebook.deleteNotebookFromJupyter).not.toHaveBeenCalled(); }); it("minimizeJupyterNotebookPanel does not flip visibility", () => { diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts index ecb5c90eca5..17800b611f9 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts @@ -237,26 +237,27 @@ export class JupyterPanelService { } } - // Delete the current workflow's stored notebook from the backend, then hide the - // panel and clear all local notebook state. This is the user-initiated action - // behind the panel's delete button, and is distinct from the workflow-switch - // cleanup (hideAndClearLocalState), which must never touch the backend. + // Delete the current workflow's stored notebook from the migration database and its file + // from the Jupyter pod, then hide the panel and clear all local notebook state. public deleteJupyterNotebook(): void { if (!this.enabled) return; const wid = this.workflowActionService.getWorkflow().wid; - // Unsaved workflow (wid undefined or the default wid 0): nothing is persisted, - // and a delete POST with such a wid would 500, so just reset local state. + // Unsaved workflow (wid undefined or the default wid 0): nothing is persisted and no + // notebook file was uploaded for it (the upload path needs a wid) if (!wid) { this.hideAndClearLocalState(); this.jupyterNotebookExists.next(false); this.clearHighlights(); return; } + // Resolve the filename up front + const notebookName = notebookFileName(wid); this.notebookMigrationService.deleteNotebookAndMapping(wid).subscribe({ next: () => { this.hideAndClearLocalState(); this.jupyterNotebookExists.next(false); this.clearHighlights(); + void this.notebookMigrationService.deleteNotebookFromJupyter(notebookName); }, error: (err: unknown) => { // Keep the panel open on failure so the user sees the notebook wasn't removed. diff --git a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts index 05699cfb218..344146b185c 100644 --- a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts +++ b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts @@ -142,6 +142,32 @@ describe("NotebookMigrationService", () => { expect(mockNotificationService.error).toHaveBeenCalledWith(expect.stringContaining("network down")); }); + // deleteNotebookFromJupyter + it("posts the notebook name to delete-notebook and returns 1", async () => { + const promise = service.deleteNotebookFromJupyter("notebook_1.ipynb"); + + const req = httpMock.expectOne(req => req.url.endsWith("/notebook-migration/delete-notebook")); + + expect(req.request.method).toBe("POST"); + expect(req.request.body).toEqual({ notebookName: "notebook_1.ipynb" }); + + req.flush({ success: true, deleted: 1 }); + + expect(await promise).toBe(1); + }); + + it("returns 0 and shows no notification when the notebook file delete fails", async () => { + // Pod cleanup is best effort, so a failure is logged rather than surfaced: the + // database delete it follows has already succeeded. + const promise = service.deleteNotebookFromJupyter("notebook_1.ipynb"); + + const req = httpMock.expectOne(req => req.url.endsWith("/notebook-migration/delete-notebook")); + req.error(new ErrorEvent("Server error")); + + expect(await promise).toBe(0); + expect(mockNotificationService.error).not.toHaveBeenCalled(); + }); + // jupyter URL methods (HttpClient so the JwtModule interceptor attaches the auth token) it("should return Jupyter URL when the request succeeds", async () => { const promise = service.getJupyterURL(); @@ -343,6 +369,12 @@ describe("NotebookMigrationService", () => { httpMock.expectNone(req => req.url.includes("/notebook-migration/set-notebook")); }); + it("deleteNotebookFromJupyter returns 0 with no HTTP call", async () => { + const result = await service.deleteNotebookFromJupyter("notebook_1.ipynb"); + expect(result).toBe(0); + httpMock.expectNone(req => req.url.endsWith("/notebook-migration/delete-notebook")); + }); + it("getJupyterURL returns null without making an HTTP call", async () => { const result = await service.getJupyterURL(); expect(result).toBeNull(); diff --git a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts index 6fb73ba3162..410d2fe0253 100644 --- a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts +++ b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts @@ -163,6 +163,24 @@ export class NotebookMigrationService { } } + // Remove a notebook file from the Jupyter pod, the counterpart to sendNotebookToJupyter. + // Best effort by design: the database rows are the source of truth for whether a workflow + // has a notebook, so a failure here is logged and not surfaced to the user. Returns 1 when + // Jupyter reported the delete (including a file that was already gone), 0 otherwise. + public async deleteNotebookFromJupyter(notebookName: string): Promise { + if (!this.enabled) return 0; + const jupyterAPIUrl = `${AppSettings.getApiEndpoint()}/notebook-migration/delete-notebook`; + const headers = new HttpHeaders({ "Content-Type": "application/json" }); + + try { + await firstValueFrom(this.http.post(jupyterAPIUrl, { notebookName }, { headers })); + return 1; + } catch (error) { + console.error("Error deleting notebook from pod: ", error); + return 0; + } + } + public async getJupyterURL(): Promise { if (!this.enabled) return null; try { diff --git a/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala b/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala index 17a0a989d72..bb7f8453468 100644 --- a/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala +++ b/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala @@ -56,6 +56,19 @@ object NotebookMigrationResource extends LazyLogging { mapper.createObjectNode().put("success", true).put("deleted", deleted) ) + private def jupyterUnavailableResponse: Response = + Response + .status(500) + .entity( + mapper.writeValueAsString( + mapper + .createObjectNode() + .put("success", false) + .put("message", "Cannot connect to Jupyter server") + ) + ) + .build() + // Read the required integer `wid` from a request body. Returns Left(400) when the field is // missing or not an integer so the caller can short-circuit. Without this a missing wid NPEs // into a 500 and a non-integer wid silently coerces to 0 via asInt(). @@ -117,17 +130,7 @@ object NotebookMigrationResource extends LazyLogging { } if (!isJupyterAvailable(jupyterUrl)) { - return Response - .status(500) - .entity( - """ - { - "success": false, - "message": "Cannot connect to Jupyter server" - } - """ - ) - .build() + return jupyterUnavailableResponse } Response @@ -138,17 +141,7 @@ object NotebookMigrationResource extends LazyLogging { // Returns the URL of Jupyter def getJupyterURL(): Response = { if (!isJupyterAvailable(jupyterUrl)) { - return Response - .status(500) - .entity( - """ - { - "success": false, - "message": "Cannot connect to Jupyter server" - } - """ - ) - .build() + return jupyterUnavailableResponse } Response.ok(successUrlJson(jupyterUrl)).build() @@ -174,17 +167,7 @@ object NotebookMigrationResource extends LazyLogging { } if (!isJupyterAvailable(jupyterUrl)) { - return Response - .status(500) - .entity( - """ - { - "success": false, - "message": "Cannot connect to Jupyter server" - } - """ - ) - .build() + return jupyterUnavailableResponse } // Construct Jupyter API URL @@ -251,6 +234,59 @@ object NotebookMigrationResource extends LazyLogging { } } + // Delete the notebook file from Jupyter's work/ directory: + def deleteNotebook(body: String): Response = { + var conn: HttpURLConnection = null + try { + val json = mapper.readTree(body) + + // Read the name defensively + val notebookName = + Option(json.get("notebookName")).filter(_.isTextual).map(_.asText()).getOrElse("") + + if (!notebookName.matches("[A-Za-z0-9._-]+\\.ipynb")) { + return Response + .status(Response.Status.BAD_REQUEST) + .entity(errorJson(s"Invalid notebook name: $notebookName")) + .build() + } + + if (!isJupyterAvailable(jupyterUrl)) { + return jupyterUnavailableResponse + } + + val url = new URL(s"$jupyterUrl/api/contents/work/$notebookName") + conn = url.openConnection().asInstanceOf[HttpURLConnection] + + conn.setRequestMethod("DELETE") + conn.setRequestProperty("Authorization", s"token $jupyterToken") + + val status = conn.getResponseCode + + // Jupyter answers 204 on a successful delete. A 404 means the file is already gone, + // which is the requested end state, so report it as a no-op (deleted=0) rather than + // an error: a workflow whose notebook was never uploaded must still delete cleanly. + if (status != 204 && status != 200 && status != 404) { + return Response + .status(500) + .entity(errorJson(s"Failed to delete notebook from Jupyter (status $status)")) + .build() + } + + Response.ok(successDeletedJson(if (status == 404) 0 else 1)).build() + + } catch { + case NonFatal(e) => + logger.error("Error deleting notebook from Jupyter", e) + Response + .status(Response.Status.INTERNAL_SERVER_ERROR) + .entity(errorJson(e.getMessage)) + .build() + } finally { + if (conn != null) conn.disconnect() + } + } + // Store notebook + mapping in database def storeNotebookAndMapping(body: String, uid: java.lang.Integer): Response = { try { @@ -491,6 +527,13 @@ class NotebookMigrationResource extends LazyLogging { NotebookMigrationResource.setNotebook(body) } + @POST + @Path("/delete-notebook") + def deleteNotebook(body: String, @Auth user: SessionUser): Response = { + logger.info("Deleting notebook from Jupyter") + NotebookMigrationResource.deleteNotebook(body) + } + @POST @Path("/store-notebook-and-mapping") def storeNotebookAndMapping(body: String, @Auth user: SessionUser): Response = { diff --git a/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala b/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala index 15ad27eff16..d5b56cd214f 100644 --- a/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala +++ b/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala @@ -73,6 +73,10 @@ class NotebookMigrationResourceSpec private var writerUid: Integer = _ // holds WRITE access to testWid private var readerUid: Integer = _ // holds READ access to testWid + // Method and path of the last /api/contents request the fake Jupyter saw, so a test can pin + // the verb and URL a Jupyter call uses. A var is safe here because the spec runs sequentially. + private var lastContentsRequest: Option[(String, String)] = None + private val sampleNotebook = """{"cells":[{"cell_type":"code","metadata":{},"source":"print(1)"}]}""" private val sampleMapping = @@ -87,6 +91,7 @@ class NotebookMigrationResourceSpec workflowVersionDao = new WorkflowVersionDao(cfg) userDao = new UserDao(cfg) workflowUserAccessDao = new WorkflowUserAccessDao(cfg) + lastContentsRequest = None cleanup() val workflow = new Workflow @@ -163,6 +168,9 @@ class NotebookMigrationResourceSpec private def deletePayload(): String = s"""{"wid": $testWid}""" + private def deleteNotebookPayload(name: String = "notebook.ipynb"): String = + s"""{"notebookName": "$name"}""" + private val resource = new NotebookMigrationResource() private def sessionUser(uid: Integer): SessionUser = { @@ -194,11 +202,17 @@ class NotebookMigrationResourceSpec "/api/contents", (exchange: com.sun.net.httpserver.HttpExchange) => { exchange.getRequestBody.readAllBytes() - val body = "{}".getBytes("UTF-8") - exchange.sendResponseHeaders(contentsStatus, body.length) - val os = exchange.getResponseBody - os.write(body) - os.close() + lastContentsRequest = Some((exchange.getRequestMethod, exchange.getRequestURI.getPath)) + if (contentsStatus == 204) { + // 204 carries no body, so send the headers with a -1 length. + exchange.sendResponseHeaders(contentsStatus, -1) + } else { + val body = "{}".getBytes("UTF-8") + exchange.sendResponseHeaders(contentsStatus, body.length) + val os = exchange.getResponseBody + os.write(body) + os.close() + } } ) server.start() @@ -450,6 +464,7 @@ class NotebookMigrationResourceSpec resource.setNotebook(validNotebook, user).getStatus shouldBe 500 resource.getJupyterURL(user).getStatus shouldBe 500 resource.getJupyterIframeURL(null, user).getStatus shouldBe 500 + resource.deleteNotebook(deleteNotebookPayload(), user).getStatus shouldBe 500 } it should "return 500 when the request body is malformed JSON" in { @@ -545,6 +560,65 @@ class NotebookMigrationResourceSpec } } + // -- deleteNotebook (Jupyter file) ------------------------------------------ + + "deleteNotebook" should "DELETE the notebook's contents path and report deleted=1" in { + withFakeJupyter(contentsStatus = 204) { + val name = s"notebook_$testWid.ipynb" + val resp = resource.deleteNotebook(deleteNotebookPayload(name), sessionUser(writerUid)) + resp.getStatus shouldBe Response.Status.OK.getStatusCode + resp.getEntity.toString should include("\"deleted\":1") + // Pins the verb and the work/ path, the two things that make this the counterpart + // of setNotebook's PUT rather than a delete of some other file. + lastContentsRequest shouldBe Some(("DELETE", s"/api/contents/work/$name")) + } + } + + it should "treat a 404 from Jupyter as a no-op, reporting deleted=0" in { + // A workflow whose notebook was never uploaded must still delete cleanly. + withFakeJupyter(contentsStatus = 404) { + val resp = resource.deleteNotebook(deleteNotebookPayload(), sessionUser(writerUid)) + resp.getStatus shouldBe Response.Status.OK.getStatusCode + resp.getEntity.toString should include("\"deleted\":0") + } + } + + it should "return 500 when Jupyter rejects the delete" in { + withFakeJupyter(contentsStatus = 500) { + resource + .deleteNotebook(deleteNotebookPayload(), sessionUser(writerUid)) + .getStatus shouldBe 500 + } + } + + it should "reject a notebook name that is not a plain .ipynb filename with 400" in { + // Validated before any Jupyter call, so no server is needed. Covers path traversal, + // a wrong extension, and an embedded subpath. + Seq("../../etc/evil.ipynb", "notebook.txt", "work/notebook.ipynb").foreach { name => + withClue(s"name=$name: ") { + NotebookMigrationResource + .deleteNotebook(deleteNotebookPayload(name)) + .getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode + } + lastContentsRequest shouldBe None + } + } + + it should "return 400 when 'notebookName' is missing or not a string" in { + // A missing name must be a client error, not a 500 from null.asText(). + Seq("""{}""", """{"notebookName": 7}""").foreach { body => + withClue(s"body=$body: ") { + NotebookMigrationResource + .deleteNotebook(body) + .getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode + } + } + } + + it should "return 500 when the request body is malformed JSON" in { + resource.deleteNotebook("not json", sessionUser(writerUid)).getStatus shouldBe 500 + } + // -- setNotebook ------------------------------------------------------------ "setNotebook" should "reject a notebook name that is not a plain .ipynb filename with 400" in { From 26dba963ba116125150cd75db29c6d719769b457 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Mon, 17 Aug 2026 15:13:41 -0700 Subject: [PATCH 4/7] feat(notebook-migration, frontend): remove a workflow's jupyter notebook file on workflow delete --- .../user-workflow.component.spec.ts | 19 ++++++++++++++--- .../user-workflow/user-workflow.component.ts | 21 ++++++++++++++++--- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.spec.ts b/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.spec.ts index db2c2317b8c..3771b94d242 100644 --- a/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.spec.ts @@ -654,9 +654,12 @@ describe("SavedWorkflowSectionComponent", () => { }); describe("deleteWorkflow", () => { - it("deletes an entry with a wid and removes it from the results", () => { + it("deletes an entry with a wid, removes it from the results, and cleans up its pod notebook", () => { const persist = TestBed.inject(WorkflowPersistService) as any; persist.deleteWorkflow = vi.fn().mockReturnValue(of(null)); + const cleanup = vi + .spyOn(TestBed.inject(NotebookMigrationService), "deleteNotebookFromJupyter") + .mockResolvedValue(1); const target = makeEntry(5, "to delete"); setEntries([target, makeEntry(6, "keep")]); @@ -664,15 +667,18 @@ describe("SavedWorkflowSectionComponent", () => { expect(persist.deleteWorkflow).toHaveBeenCalledWith([5]); expect(component.searchResultsComponent.entries.map(e => e.name)).toEqual(["keep"]); + expect(cleanup).toHaveBeenCalledWith("notebook_5.ipynb"); }); it("does nothing when the entry has no wid", () => { const persist = TestBed.inject(WorkflowPersistService) as any; persist.deleteWorkflow = vi.fn(); + const cleanup = vi.spyOn(TestBed.inject(NotebookMigrationService), "deleteNotebookFromJupyter"); component.deleteWorkflow(makeEntry(undefined, "no wid")); expect(persist.deleteWorkflow).not.toHaveBeenCalled(); + expect(cleanup).not.toHaveBeenCalled(); }); }); @@ -732,9 +738,12 @@ describe("SavedWorkflowSectionComponent", () => { }); describe("handleConfirmDeleteSelectedWorkflows", () => { - it("deletes checked wids and keeps undefined-wid entries", () => { + it("deletes checked wids, keeps undefined-wid entries, and cleans up each pod notebook", () => { const persist = TestBed.inject(WorkflowPersistService) as any; persist.deleteWorkflow = vi.fn().mockReturnValue(of(null)); + const cleanup = vi + .spyOn(TestBed.inject(NotebookMigrationService), "deleteNotebookFromJupyter") + .mockResolvedValue(1); setEntries([ makeEntry(1, "a", true), makeEntry(2, "b", true), @@ -746,6 +755,7 @@ describe("SavedWorkflowSectionComponent", () => { expect(persist.deleteWorkflow).toHaveBeenCalledWith([1, 2]); expect(component.searchResultsComponent.entries.map(e => e.name)).toEqual(["c", "d"]); + expect(cleanup.mock.calls.map(c => c[0])).toEqual(["notebook_1.ipynb", "notebook_2.ipynb"]); }); it("early-returns when a checked entry has no wid", () => { @@ -758,15 +768,18 @@ describe("SavedWorkflowSectionComponent", () => { expect(persist.deleteWorkflow).not.toHaveBeenCalled(); }); - it("alerts on a deletion error", () => { + it("alerts on a deletion error and does not touch the pod", () => { const persist = TestBed.inject(WorkflowPersistService) as any; persist.deleteWorkflow = vi.fn().mockReturnValue(throwError(() => "delfail")); const alertSpy = vi.spyOn(window, "alert").mockImplementation(() => {}); + const cleanup = vi.spyOn(TestBed.inject(NotebookMigrationService), "deleteNotebookFromJupyter"); setEntries([makeEntry(1, "a", true)]); component.handleConfirmDeleteSelectedWorkflows(); expect(alertSpy).toHaveBeenCalledWith("delfail"); + // The backend delete failed, so the pod file must be left in place. + expect(cleanup).not.toHaveBeenCalled(); }); }); diff --git a/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.ts b/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.ts index 4bd6acda2b9..d8f16deaadc 100644 --- a/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.ts +++ b/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.ts @@ -49,6 +49,7 @@ import { GuiConfigService } from "../../../../common/service/gui-config.service" import { MappingContent, NotebookMigrationService, + notebookFileName, } from "../../../../workspace/service/notebook-migration/notebook-migration.service"; import { LlmRequestTimeoutError, Notebook } from "../../../../workspace/service/notebook-migration/migration-llm"; import { @@ -504,19 +505,32 @@ export class UserWorkflowComponent implements AfterViewInit, OnDestroy { */ public deleteWorkflow(entry: DashboardEntry): void { - if (entry.workflow.workflow.wid == undefined) { + const wid = entry.workflow.workflow.wid; + if (wid == undefined) { return; } this.workflowPersistService - .deleteWorkflow([entry.workflow.workflow.wid]) + .deleteWorkflow([wid]) .pipe(untilDestroyed(this)) .subscribe(_ => { this.searchResultsComponent.entries = this.searchResultsComponent.entries.filter( - workflowEntry => workflowEntry.workflow.workflow.wid !== entry.workflow.workflow.wid + workflowEntry => workflowEntry.workflow.workflow.wid !== wid ); + this.cleanupNotebookFiles([wid]); }); } + // Best-effort removal of the deleted workflows' notebook files from the Jupyter pod. + // The workflow delete already cascades the notebook DB rows, but the pod's per-workflow + // notebook_.ipynb only the frontend can reach, so clean it up here. Not awaited and + // never surfaced: a workflow with no notebook is a harmless 404, and an unreachable pod + // must not affect a delete that already succeeded. + private cleanupNotebookFiles(wids: number[]): void { + for (const wid of wids) { + void this.notebookMigrationService.deleteNotebookFromJupyter(notebookFileName(wid)); + } + } + /** * Verify Uploaded file name and upload the file */ @@ -696,6 +710,7 @@ export class UserWorkflowComponent implements AfterViewInit, OnDestroy { // Check if wid is defined and if it's not included in targetWids return entryWid === undefined || !targetWids.includes(entryWid); }); + this.cleanupNotebookFiles(targetWids); }, // TODO: fix this with notification component error: (err: unknown) => alert(err), From 6d7f2df8b3f69c3ca04641d990c89eb53a207e3d Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Tue, 18 Aug 2026 13:18:18 -0700 Subject: [PATCH 5/7] test(notebook-migration): cover the 200 delete-response branch in deleteNotebook --- .../service/resource/NotebookMigrationResourceSpec.scala | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala b/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala index d5b56cd214f..98eb9a43788 100644 --- a/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala +++ b/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala @@ -574,6 +574,15 @@ class NotebookMigrationResourceSpec } } + it should "treat a 200 from Jupyter as a successful delete, reporting deleted=1" in { + // Some Jupyter versions answer 200 instead of 204 on a delete; both mean success. + withFakeJupyter(contentsStatus = 200) { + val resp = resource.deleteNotebook(deleteNotebookPayload(), sessionUser(writerUid)) + resp.getStatus shouldBe Response.Status.OK.getStatusCode + resp.getEntity.toString should include("\"deleted\":1") + } + } + it should "treat a 404 from Jupyter as a no-op, reporting deleted=0" in { // A workflow whose notebook was never uploaded must still delete cleanly. withFakeJupyter(contentsStatus = 404) { From 355a77145a3ae884c52c04cb34208cb0e137d145 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Tue, 18 Aug 2026 13:46:03 -0700 Subject: [PATCH 6/7] refactor(notebook-migration, frontend): centralize notebook-file cleanup on a wid-keyed seam --- .../user-workflow.component.spec.ts | 16 ++++++++-------- .../user-workflow/user-workflow.component.ts | 3 +-- .../jupyter-panel.service.spec.ts | 15 +++++++-------- .../jupyter-panel/jupyter-panel.service.ts | 5 ++--- .../notebook-migration.service.spec.ts | 19 +++++++++---------- .../notebook-migration.service.ts | 16 +++++++--------- 6 files changed, 34 insertions(+), 40 deletions(-) diff --git a/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.spec.ts b/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.spec.ts index 3771b94d242..25f544da39f 100644 --- a/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.spec.ts @@ -658,8 +658,8 @@ describe("SavedWorkflowSectionComponent", () => { const persist = TestBed.inject(WorkflowPersistService) as any; persist.deleteWorkflow = vi.fn().mockReturnValue(of(null)); const cleanup = vi - .spyOn(TestBed.inject(NotebookMigrationService), "deleteNotebookFromJupyter") - .mockResolvedValue(1); + .spyOn(TestBed.inject(NotebookMigrationService), "deleteNotebookForWorkflow") + .mockResolvedValue(undefined); const target = makeEntry(5, "to delete"); setEntries([target, makeEntry(6, "keep")]); @@ -667,13 +667,13 @@ describe("SavedWorkflowSectionComponent", () => { expect(persist.deleteWorkflow).toHaveBeenCalledWith([5]); expect(component.searchResultsComponent.entries.map(e => e.name)).toEqual(["keep"]); - expect(cleanup).toHaveBeenCalledWith("notebook_5.ipynb"); + expect(cleanup).toHaveBeenCalledWith(5); }); it("does nothing when the entry has no wid", () => { const persist = TestBed.inject(WorkflowPersistService) as any; persist.deleteWorkflow = vi.fn(); - const cleanup = vi.spyOn(TestBed.inject(NotebookMigrationService), "deleteNotebookFromJupyter"); + const cleanup = vi.spyOn(TestBed.inject(NotebookMigrationService), "deleteNotebookForWorkflow"); component.deleteWorkflow(makeEntry(undefined, "no wid")); @@ -742,8 +742,8 @@ describe("SavedWorkflowSectionComponent", () => { const persist = TestBed.inject(WorkflowPersistService) as any; persist.deleteWorkflow = vi.fn().mockReturnValue(of(null)); const cleanup = vi - .spyOn(TestBed.inject(NotebookMigrationService), "deleteNotebookFromJupyter") - .mockResolvedValue(1); + .spyOn(TestBed.inject(NotebookMigrationService), "deleteNotebookForWorkflow") + .mockResolvedValue(undefined); setEntries([ makeEntry(1, "a", true), makeEntry(2, "b", true), @@ -755,7 +755,7 @@ describe("SavedWorkflowSectionComponent", () => { expect(persist.deleteWorkflow).toHaveBeenCalledWith([1, 2]); expect(component.searchResultsComponent.entries.map(e => e.name)).toEqual(["c", "d"]); - expect(cleanup.mock.calls.map(c => c[0])).toEqual(["notebook_1.ipynb", "notebook_2.ipynb"]); + expect(cleanup.mock.calls.map(c => c[0])).toEqual([1, 2]); }); it("early-returns when a checked entry has no wid", () => { @@ -772,7 +772,7 @@ describe("SavedWorkflowSectionComponent", () => { const persist = TestBed.inject(WorkflowPersistService) as any; persist.deleteWorkflow = vi.fn().mockReturnValue(throwError(() => "delfail")); const alertSpy = vi.spyOn(window, "alert").mockImplementation(() => {}); - const cleanup = vi.spyOn(TestBed.inject(NotebookMigrationService), "deleteNotebookFromJupyter"); + const cleanup = vi.spyOn(TestBed.inject(NotebookMigrationService), "deleteNotebookForWorkflow"); setEntries([makeEntry(1, "a", true)]); component.handleConfirmDeleteSelectedWorkflows(); diff --git a/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.ts b/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.ts index d8f16deaadc..fad38708e05 100644 --- a/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.ts +++ b/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.ts @@ -49,7 +49,6 @@ import { GuiConfigService } from "../../../../common/service/gui-config.service" import { MappingContent, NotebookMigrationService, - notebookFileName, } from "../../../../workspace/service/notebook-migration/notebook-migration.service"; import { LlmRequestTimeoutError, Notebook } from "../../../../workspace/service/notebook-migration/migration-llm"; import { @@ -527,7 +526,7 @@ export class UserWorkflowComponent implements AfterViewInit, OnDestroy { // must not affect a delete that already succeeded. private cleanupNotebookFiles(wids: number[]): void { for (const wid of wids) { - void this.notebookMigrationService.deleteNotebookFromJupyter(notebookFileName(wid)); + void this.notebookMigrationService.deleteNotebookForWorkflow(wid); } } diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts index 99ee28a15d9..256e8f2cba1 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts @@ -90,7 +90,7 @@ describe("JupyterPanelService", () => { deleteNotebookAndMapping: vi.fn().mockReturnValue(of({ success: true, deleted: 1 })), // In the base mock because every successful delete fires it; a missing stub would // throw inside the delete subscription rather than failing a targeted assertion. - deleteNotebookFromJupyter: vi.fn().mockResolvedValue(1), + deleteNotebookForWorkflow: vi.fn().mockResolvedValue(undefined), }; mockGuiConfig = { env: { pythonNotebookMigrationEnabled: true } }; @@ -149,12 +149,11 @@ describe("JupyterPanelService", () => { expect(mockWorkflow.unhighlightLinks).toHaveBeenCalled(); }); - it("deleteJupyterNotebook removes the pod's copy under the workflow's filename", () => { + it("deleteJupyterNotebook removes the pod's copy for the current workflow", () => { service.deleteJupyterNotebook(); - // The file cleanup keys off the same wid-derived name the upload uses, so the - // notebook_.ipynb left in the pod is the one being deleted. - expect(mockNotebook.deleteNotebookFromJupyter).toHaveBeenCalledWith("notebook_1.ipynb"); + // The service derives the filename from the wid, so the panel just passes the wid. + expect(mockNotebook.deleteNotebookForWorkflow).toHaveBeenCalledWith(1); }); it("deleteJupyterNotebook keeps the panel open and notifies on failure", () => { @@ -169,7 +168,7 @@ describe("JupyterPanelService", () => { expect(visible).toBe(true); expect(mockNotebook.deleteMapping).not.toHaveBeenCalled(); // The notebook is still stored, so its file must stay in the pod. - expect(mockNotebook.deleteNotebookFromJupyter).not.toHaveBeenCalled(); + expect(mockNotebook.deleteNotebookForWorkflow).not.toHaveBeenCalled(); }); it("deleteJupyterNotebook only resets local state for the default wid 0 (no backend call)", () => { @@ -186,7 +185,7 @@ describe("JupyterPanelService", () => { // wid 0 is the unsaved default workflow, so neither backend delete should fire: // nothing is stored and no notebook file was ever uploaded for it. expect(mockNotebook.deleteNotebookAndMapping).not.toHaveBeenCalled(); - expect(mockNotebook.deleteNotebookFromJupyter).not.toHaveBeenCalled(); + expect(mockNotebook.deleteNotebookForWorkflow).not.toHaveBeenCalled(); expect(visible).toBe(false); expect(exists).toBe(false); }); @@ -730,7 +729,7 @@ describe("JupyterPanelService", () => { service.deleteJupyterNotebook(); expect(mockNotebook.deleteNotebookAndMapping).not.toHaveBeenCalled(); expect(mockNotebook.deleteMapping).not.toHaveBeenCalled(); - expect(mockNotebook.deleteNotebookFromJupyter).not.toHaveBeenCalled(); + expect(mockNotebook.deleteNotebookForWorkflow).not.toHaveBeenCalled(); }); it("minimizeJupyterNotebookPanel does not flip visibility", () => { diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts index 17800b611f9..2a1681155d5 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts @@ -250,14 +250,13 @@ export class JupyterPanelService { this.clearHighlights(); return; } - // Resolve the filename up front - const notebookName = notebookFileName(wid); this.notebookMigrationService.deleteNotebookAndMapping(wid).subscribe({ next: () => { this.hideAndClearLocalState(); this.jupyterNotebookExists.next(false); this.clearHighlights(); - void this.notebookMigrationService.deleteNotebookFromJupyter(notebookName); + // wid is captured above, so a mid-flight workflow switch can't retarget this. + void this.notebookMigrationService.deleteNotebookForWorkflow(wid); }, error: (err: unknown) => { // Keep the panel open on failure so the user sees the notebook wasn't removed. diff --git a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts index 344146b185c..f464e09abce 100644 --- a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts +++ b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts @@ -142,9 +142,9 @@ describe("NotebookMigrationService", () => { expect(mockNotificationService.error).toHaveBeenCalledWith(expect.stringContaining("network down")); }); - // deleteNotebookFromJupyter - it("posts the notebook name to delete-notebook and returns 1", async () => { - const promise = service.deleteNotebookFromJupyter("notebook_1.ipynb"); + // deleteNotebookForWorkflow + it("posts the wid-derived notebook name to delete-notebook", async () => { + const promise = service.deleteNotebookForWorkflow(1); const req = httpMock.expectOne(req => req.url.endsWith("/notebook-migration/delete-notebook")); @@ -153,18 +153,18 @@ describe("NotebookMigrationService", () => { req.flush({ success: true, deleted: 1 }); - expect(await promise).toBe(1); + await promise; }); - it("returns 0 and shows no notification when the notebook file delete fails", async () => { + it("swallows the failure and shows no notification when the notebook file delete fails", async () => { // Pod cleanup is best effort, so a failure is logged rather than surfaced: the // database delete it follows has already succeeded. - const promise = service.deleteNotebookFromJupyter("notebook_1.ipynb"); + const promise = service.deleteNotebookForWorkflow(1); const req = httpMock.expectOne(req => req.url.endsWith("/notebook-migration/delete-notebook")); req.error(new ErrorEvent("Server error")); - expect(await promise).toBe(0); + await promise; expect(mockNotificationService.error).not.toHaveBeenCalled(); }); @@ -369,9 +369,8 @@ describe("NotebookMigrationService", () => { httpMock.expectNone(req => req.url.includes("/notebook-migration/set-notebook")); }); - it("deleteNotebookFromJupyter returns 0 with no HTTP call", async () => { - const result = await service.deleteNotebookFromJupyter("notebook_1.ipynb"); - expect(result).toBe(0); + it("deleteNotebookForWorkflow makes no HTTP call", async () => { + await service.deleteNotebookForWorkflow(1); httpMock.expectNone(req => req.url.endsWith("/notebook-migration/delete-notebook")); }); diff --git a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts index 410d2fe0253..96be84c2261 100644 --- a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts +++ b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts @@ -163,21 +163,19 @@ export class NotebookMigrationService { } } - // Remove a notebook file from the Jupyter pod, the counterpart to sendNotebookToJupyter. - // Best effort by design: the database rows are the source of truth for whether a workflow - // has a notebook, so a failure here is logged and not surfaced to the user. Returns 1 when - // Jupyter reported the delete (including a file that was already gone), 0 otherwise. - public async deleteNotebookFromJupyter(notebookName: string): Promise { - if (!this.enabled) return 0; + // Remove a workflow's notebook file from the Jupyter pod. + // Best effort by design: the database rows are the source of truth for + // whether a workflow has a notebook, so a failure here is logged, not surfaced, and + // nothing acts on the outcome. + public async deleteNotebookForWorkflow(wid: number | undefined): Promise { + if (!this.enabled) return; const jupyterAPIUrl = `${AppSettings.getApiEndpoint()}/notebook-migration/delete-notebook`; const headers = new HttpHeaders({ "Content-Type": "application/json" }); try { - await firstValueFrom(this.http.post(jupyterAPIUrl, { notebookName }, { headers })); - return 1; + await firstValueFrom(this.http.post(jupyterAPIUrl, { notebookName: notebookFileName(wid) }, { headers })); } catch (error) { console.error("Error deleting notebook from pod: ", error); - return 0; } } From b7aaf3f97ef8091d99af2d68b7b81c31a3e8e8a7 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Tue, 18 Aug 2026 13:57:13 -0700 Subject: [PATCH 7/7] fix(notebook-migration): bound the jupyter delete timeout and tighten the cleanup wid type --- .../notebook-migration/notebook-migration.service.ts | 11 ++++++----- .../service/resource/NotebookMigrationResource.scala | 11 +++++++---- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts index 96be84c2261..5de5ffb03e3 100644 --- a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts +++ b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts @@ -163,11 +163,12 @@ export class NotebookMigrationService { } } - // Remove a workflow's notebook file from the Jupyter pod. - // Best effort by design: the database rows are the source of truth for - // whether a workflow has a notebook, so a failure here is logged, not surfaced, and - // nothing acts on the outcome. - public async deleteNotebookForWorkflow(wid: number | undefined): Promise { + // Remove a workflow's notebook file from the Jupyter pod. Takes a concrete wid so it can + // never fall back to the shared default filename and delete the wrong file; callers guard + // out unsaved workflows before calling. Best effort by design: the database rows are the + // source of truth for whether a workflow has a notebook, so a failure here is logged, not + // surfaced, and nothing acts on the outcome. + public async deleteNotebookForWorkflow(wid: number): Promise { if (!this.enabled) return; const jupyterAPIUrl = `${AppSettings.getApiEndpoint()}/notebook-migration/delete-notebook`; const headers = new HttpHeaders({ "Content-Type": "application/json" }); diff --git a/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala b/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala index bb7f8453468..8956e506459 100644 --- a/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala +++ b/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala @@ -259,16 +259,19 @@ object NotebookMigrationResource extends LazyLogging { conn = url.openConnection().asInstanceOf[HttpURLConnection] conn.setRequestMethod("DELETE") + conn.setConnectTimeout(2000) + conn.setReadTimeout(2000) conn.setRequestProperty("Authorization", s"token $jupyterToken") val status = conn.getResponseCode - // Jupyter answers 204 on a successful delete. A 404 means the file is already gone, - // which is the requested end state, so report it as a no-op (deleted=0) rather than - // an error: a workflow whose notebook was never uploaded must still delete cleanly. + // Jupyter answers 204 on a successful delete, or 200 when it echoes the deleted entry. + // A 404 means the file is already gone, which is the requested end state, so report it + // as a no-op (deleted=0) rather than an error: a workflow whose notebook was never + // uploaded must still delete cleanly. if (status != 204 && status != 200 && status != 404) { return Response - .status(500) + .status(Response.Status.INTERNAL_SERVER_ERROR) .entity(errorJson(s"Failed to delete notebook from Jupyter (status $status)")) .build() }