feat(notebook-migration): remove a workflow's notebook file from the Jupyter pod on delete - #7741
Draft
zyratlo wants to merge 5 commits into
Draft
feat(notebook-migration): remove a workflow's notebook file from the Jupyter pod on delete#7741zyratlo wants to merge 5 commits into
zyratlo wants to merge 5 commits into
Conversation
…upyter pod (only delete notebook button)
…ook file on workflow delete
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is
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
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 towork/notebook_<wid>.ipynb, the file survives both a notebook delete and a workflow delete, because those paths only touched the database.deleteNotebookAndMappingremoved thenotebookrows (cascading toworkflow_notebook_mapping), and deleting a workflow cascades those same rows through thenotebook.widforeign 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(newdelete-notebookendpoint)POST /notebook-migration/delete-notebook, the counterpart toset-notebook: it takes anotebookName, validates it with the same[A-Za-z0-9._-]+\.ipynbpattern (blocking path traversal before any network call), and issuesDELETE /api/contents/work/<name>against the Jupyter Contents API.deleted: 1. A 404 is treated as a no op withdeleted: 0, so a workflow whose notebook was never uploaded still deletes cleanly, consistent with how the database delete reportsdeleted: 0when nothing was stored. Any other status is a 500.jupyterUnavailableResponseso the four endpoints that need a reachable Jupyter cannot drift in status or body.NotebookMigrationService(frontend)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()resolvesnotebookFileName(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)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>.ipynbfilename scheme this cleanup depends on. Until that merges, this PR's diff includes its commits.How was this PR tested?
NotebookMigrationResourceSpec.scala:delete-notebookissues a DELETE against thework/<name>contents path (verb and path pinned), reportsdeleted=1on 204, treats 404 asdeleted=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:deleteNotebookFromJupyterposts the name todelete-notebookand 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 upnotebook_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)