Close the julia oneShot worker on failed runs (fixes #649) - #655
Merged
Conversation
Running the julia tests stranded a QuartoNotebookRunner worker process per failed render, accumulating indefinitely. `executeJulia` in the fixture's engine sent the oneShot post-run close only on the SUCCESS path, so a run that threw — a cell raising under the default `error: false` — skipped the close and left that file's worker open on the control server forever, since nothing ever runs that (temp-dir) file again to reclaim it. Open workers also hold the server past its `serve(; timeout = 300)` idle exit, so the server never exited either: it accumulated children and was reparented to init. Measured before the fix: one orphaned control server (ppid=1, up 14h29m) holding 35 workers and 6.02 GB RSS. `j4` alone reproduced +1 leaked worker per invocation, deterministically over three runs; `j1` (success path) leaked none. Note this refines the diagnosis in #649: the defect is in the engine's TypeScript, not in QuartoNotebookRunner. QNR behaves correctly — it was never told to close the worker. The fix (taken from the upstream-bound `q2-close-busy-fix` branch of quarto-julia-engine, not yet in PumasAI main) wraps the run so the error path closes the worker best-effort via a new `errorRunClose`: plain close first, busy -> forceclose, and any remaining failure warns rather than throwing, so a cleanup failure cannot mask the run error already in flight. New regression row `j7_failed_run_does_not_leak_worker` runs against an ISOLATED control server under a temp HOME — seeded from the ambient runtime dir so the Julia environment is already instantiated — making the assertion an exact `workers == 0` on a server the test owns rather than a racy delta on the shared server. A guard kills that server's process group on scope exit. Verified RED before the fix ("leaked 1 QNR worker process") and GREEN after; the full julia suite now strands zero processes where it previously added one.
Follow-on to the #649 leak fix, which took only the error-path close. This brings the rest of the fixture up to upstream main, so the fixture stops straddling two upstream states. All of it comes from PumasAI main at the v0.2.1 tag: - QuartoNotebookRunner 0.17.4 -> 0.18.2 - writeAll for the socket payload (landed upstream as PR #12), fixing large documents failing with a bare "Internal Error" (quarto-cli#14834). `conn.write` is not guaranteed to write the whole buffer: a payload exceeding the socket send buffer short-writes, and the old code turned that into the bare error. The fixture carried that buggy form. Unrelated to the worker leak, and never observed here — our julia fixtures are a few hundred bytes. - keep-ipynb support - extension version 0.1.0 -> 0.2.1 Two q2-local deviations are preserved deliberately: - `_extension.yml` keeps its `author` and the `name`/`claims`/ `file-extensions` engine block. Those static declarations are q2-only schema with no upstream equivalent — Quarto 1's `external-engine` schema is `closed: true` and rejects them. - `start_quartonotebookrunner_detached.jl` keeps the q2-specific comment wording; its code lines are byte-identical to upstream. Separated from the leak fix so #649 can be reviewed — or reverted — on its own: this commit is droppable without affecting the fix. No assertion churn from the QNR bump; the julia rows also got roughly 3x faster, as 0.18.x caches worker environments across runs.
Research write-up off the back of the worker-leak fix. Records the findings that shape the sequencing — chiefly that Quarto 1's external-engine schema is closed:true and rejects all four static keys (verified empirically against the dev build), so the declarations cannot land upstream without a quarto-cli schema change first. Also records that q2 deliberately inverts Q1's jupyter/julia default, so the schema change should be accept-and-ignore rather than implemented in Q1; that file-extensions is not a transcription of validExtensions(); and a confirmed q2 bug where static claim lookup is case-sensitive while dynamic claiming is not. Draft only: not scoped into tasks, not started, six open questions.
Three changes from the first draft. Target a full stable Quarto release rather than a prerelease: a published extension whose quarto-required names a prerelease would refuse to install for ordinary users on stable Quarto, which is not something to ask PumasAI to ship. This makes the critical path a release cycle rather than weeks. Add Step 0 (discuss with Julius before anything else — the q2-only keys and the quarto-required bump are changes to their extension, and the conversation may reshape the plan) and Step 2c (offer a q2 branch on PumasAI/quarto-julia-engine so people can try Julia in q2 before the schema change ships, and so q2 has a stable remote to subtree from). Add Step 4: bundle the julia engine in q2 as a vendored subtree. Research (F8) finds q2 already has the discovery and embedding machinery — builtin_extensions_path() embeds resources/extensions via include_dir! and discover_extensions already scans a builtin dir first — so the port is mostly the maintenance command, Q1's dev-call pull-git-subtree, rehomed as cargo xtask pull-extension-subtree. Payload is 68K but the full repo is 14M, so embed only the _extensions subdir. Strengthen the provisional banner: nothing here is agreed or started.
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
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
2 participants
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.
Fixes #649.
@jkrumbiegel, we have experimental support for Julia as a TS Engine Extension in Quarto 2!
Right now
quarto-julia-engineis only included in a test fixture. This PR fixes a leak in error handling as well as updating the fixture toPumasAI/quarto-julia-engine0.2.1.We will bundle Julia in Quarto 2. If you're interested in the path forward, I've attached a very rough draft of a plan to this PR.
There are a few additions to
_extension.ymlto statically declare language and file claiming.Unfortunately, the new fields are currently rejected by Quarto 1, so it will take a Quarto 1 stable release before we have engines compatible with both versions of Quarto.
But maybe we can have a q2-compatible branch on
PumasAI/quarto-julia-enginebefore then.I'm out until Sept 15; will file PRs when I get back, or happy to discuss earlier.
The bug
Running the julia tests stranded a QuartoNotebookRunner worker process per failed render, accumulating indefinitely.
executeJuliasent the oneShot post-run close only on the success path. A run that threw — a cell raising under the defaulterror: false— skipped the close, leaving that file's worker open on the control server forever, since nothing ever runs that (temp-dir) file again to reclaim it. Open workers also hold the server past itsserve(; timeout = 300)idle exit, so the server never exited either: it accumulated children and was reparented to init.Measured on a dev machine before the fix:
ppid=1, up 14h29m) holding 35 workers / 6.02 GB RSS, launched from a long-deleted test temp dir;j4(the error-handling row) reproduced +1 leaked worker per invocation, deterministically over three runs;j1(success path) leaked none.This refines the diagnosis in the issue. #649 guesses "a bug in error handling in their
QuartoNotebookRunner". It isn't — the defect is in the engine's TypeScript. QNR behaves correctly; it was simply never told to close the worker.The fix (commit 1)
Taken from the
q2-close-busy-fixbranch ofquarto-julia-engine. This fix is not in PumasAImain— it is ours, and contributing it upstream is tracked in the plan below.The run is wrapped so the error path closes the worker best-effort via a new
errorRunClose: plain close first, busy →forceclose, and any remaining failure warns rather than throwing, so a cleanup failure cannot mask the run error already in flight.New regression row
j7_failed_run_does_not_leak_workerruns against an isolated control server under a tempHOME— seeded from the ambient runtime dir so the Julia environment is already instantiated — making the assertion an exactworkers == 0on a server the test owns, rather than a racy delta on the developer's shared server. A guard kills that server's process group on scope exit, so the row cannot itself strand anything.Verified RED before the fix (
leaked 1 QNR worker process) and GREEN after.Fixture catch-up (commit 2) — droppable
Separate commit so the fix above can be reviewed, or reverted, on its own. Brings the rest of the fixture up to upstream
mainat the v0.2.1 tag, so it stops straddling two upstream states: QNR0.17.4 → 0.18.2,writeAllfor the socket payload (upstream PR #12, fixing large documents failing with a bareInternal Error, quarto-cli#14834 — a latent bug the fixture carried, unrelated to the leak and never observed here),keep-ipynb, and the extension version bump.Two q2-local deviations are preserved deliberately: the
name/claims/file-extensionsengine block in_extension.yml(q2-only schema — Quarto 1'sexternal-engineschema isclosed: trueand rejects it), and the q2-specific comments instart_quartonotebookrunner_detached.jl(code lines byte-identical to upstream).Plan (commits 3–4)
claude-notes/plans/2026-09-03-julia-engine-static-declarations-epic.md— a provisional, needs-review epic draft covering the follow-on work: contributing the leak fix upstream, loosening Quarto 1'sexternal-engineschema so the static declarations are legal, offering a q2 branch on PumasAI so people can try Julia in q2 before that ships, fixing a confirmed q2 bug where static claim lookup is case-sensitive while dynamic claiming is not, and bundling the julia engine into q2 as a vendored subtree.Nothing in it is agreed or started — it is included here so the context lives with the fix.
Verification
cargo xtask verify --skip-hub-build --skip-hub-tests— 13676 passed, 199 skipped, 0 failures, clippy-D warningsclean, no snapshot files touched. Test count moves by exactly +1, accounted for byj7.The julia suite now strands zero processes where it previously added one; after the run the control server self-terminates via its idle timeout, leaving no julia processes at all — the lifecycle the leak had disabled.