Skip to content

feat(notebook-migration): remove a workflow's notebook file from the Jupyter pod on delete - #7741

Draft
zyratlo wants to merge 5 commits into
apache:mainfrom
zyratlo:migration-tool-delete-notebook-in-jupyter
Draft

feat(notebook-migration): remove a workflow's notebook file from the Jupyter pod on delete#7741
zyratlo wants to merge 5 commits into
apache:mainfrom
zyratlo:migration-tool-delete-notebook-in-jupyter

Conversation

@zyratlo

@zyratlo zyratlo commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Removes a workflow's notebook file from the Jupyter pod when the notebook is deleted, so the pod's work/ directory no longer accumulates notebooks for workflows the user has removed.

Before #7671 this was self limiting: every workflow uploaded to the same work/notebook.ipynb, so there was only ever one file and it was overwritten on reuse. Now that each workflow uploads to work/notebook_<wid>.ipynb, the file survives both a notebook delete and a workflow delete, because those paths only touched the database. deleteNotebookAndMapping removed the notebook rows (cascading to workflow_notebook_mapping), and deleting a workflow cascades those same rows through the notebook.wid foreign key, but nothing removed the file from the pod.

The database is authoritative for whether a notebook exists. The pod file is a per user artifact only the frontend can reach, since the notebook migration service targets one Jupyter per process under the per user pod model. So the file cleanup is best effort from the frontend after the authoritative database delete, and a failure is logged rather than surfaced.

NotebookMigrationResource (new delete-notebook endpoint)

  • Adds POST /notebook-migration/delete-notebook, the counterpart to set-notebook: it takes a notebookName, validates it with the same [A-Za-z0-9._-]+\.ipynb pattern (blocking path traversal before any network call), and issues DELETE /api/contents/work/<name> against the Jupyter Contents API.
  • A 204 or 200 reports deleted: 1. A 404 is treated as a no op with deleted: 0, so a workflow whose notebook was never uploaded still deletes cleanly, consistent with how the database delete reports deleted: 0 when nothing was stored. Any other status is a 500.
  • Extracts the shared jupyterUnavailableResponse so the four endpoints that need a reachable Jupyter cannot drift in status or body.

NotebookMigrationService (frontend)

  • Adds deleteNotebookFromJupyter(notebookName). It posts the name to the new endpoint, returns 1 or 0, logs failures, and shows no notification because pod cleanup is best effort.

JupyterPanelService (panel delete button)

  • deleteJupyterNotebook() resolves notebookFileName(wid) up front (so a mid flight workflow switch cannot retarget the delete), then fires the pod cleanup after the database delete succeeds. The unsaved workflow path (wid undefined or the default 0) still resets local state only, since no file was ever uploaded for it.

UserWorkflowComponent (dashboard delete)

  • Adds a private cleanupNotebookFiles(wids) that fires the pod cleanup per wid, and calls it from the success handler of both single delete (deleteWorkflow) and bulk delete (handleConfirmDeleteSelectedWorkflows). It runs only after the backend delete succeeds, so a failed delete leaves the pod file in place. A deleted workflow with no notebook produces a harmless 404.

Any related issues, documentation, discussions?

Closes #7737
Parent issue #4301

Stacked on #7738, which introduces the notebook_<wid>.ipynb filename scheme this cleanup depends on. Until that merges, this PR's diff includes its commits.

How was this PR tested?

  • NotebookMigrationResourceSpec.scala: delete-notebook issues a DELETE against the work/<name> contents path (verb and path pinned), reports deleted=1 on 204, treats 404 as deleted=0, returns 500 when Jupyter rejects the delete or is unreachable, and returns 400 on an invalid name, a missing or non string name, or a malformed body.
  • notebook-migration.service.spec.ts: deleteNotebookFromJupyter posts the name to delete-notebook and returns 1 on success, returns 0 with no notification on failure, and makes no HTTP call when the feature flag is off.
  • jupyter-panel.service.spec.ts: the panel delete removes the pod copy under the workflow's filename, does not touch the pod when the database delete fails or for the unsaved default wid, and no ops when the flag is off.
  • user-workflow.component.spec.ts: single delete cleans up notebook_5.ipynb, bulk delete cleans up each checked wid in order, and neither the no wid path nor a backend delete error touches the pod.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 4.8)

@github-actions github-actions Bot added frontend Changes related to the frontend GUI platform Non-amber Scala service paths labels Aug 17, 2026
@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • No candidates found from git blame history.

@codecov-commenter

codecov-commenter commented Aug 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.15686% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.04%. Comparing base (60300e3) to head (7ceddac).

Files with missing lines Patch % Lines
...a/service/resource/NotebookMigrationResource.scala 88.23% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7741      +/-   ##
============================================
+ Coverage     91.02%   91.04%   +0.01%     
- Complexity     4454     4460       +6     
============================================
  Files          1174     1174              
  Lines         47146    47188      +42     
  Branches       5287     5295       +8     
============================================
+ Hits          42916    42960      +44     
+ Misses         2550     2549       -1     
+ Partials       1680     1679       -1     
Flag Coverage Δ *Carryforward flag
access-control-service 81.00% <ø> (ø)
agent-service 98.62% <ø> (ø) Carriedforward from e80add4
amber 87.49% <ø> (+0.03%) ⬆️ Carriedforward from e80add4
computing-unit-managing-service 73.67% <ø> (ø)
config-service 86.73% <ø> (ø)
file-service 68.90% <ø> (ø)
frontend 92.59% <100.00%> (+<0.01%) ⬆️
notebook-migration-service 83.91% <88.23%> (+0.16%) ⬆️
pyamber 97.57% <ø> (ø) Carriedforward from e80add4
workflow-compiling-service 77.19% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend Changes related to the frontend GUI platform Non-amber Scala service paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Notebook Migration] Deleting a notebook leaves its .ipynb file in the Jupyter pod

2 participants