refactor(cli): unify CLI process execution behind one vscode-free seam - #2148
Open
rugpanov wants to merge 4 commits into
Open
refactor(cli): unify CLI process execution behind one vscode-free seam#2148rugpanov wants to merge 4 commits into
rugpanov wants to merge 4 commits into
Conversation
*Why* The `cli` and `python-setup` packages each carried their own child-process plumbing: `CliWrapper` had `execFile`/`waitForProcess`/`runBundleCommand` (buffered exec + bundle streaming, cancelled via an AbortController that only SIGTERMs the direct child) while `PythonSetupCliClient` had a separate, better spawn core (detached process group, whole-tree kill, buffer-then-decode). Two implementations of the same concern, and the older one leaked grandchildren (`terraform`, `uv`) on cancel. *What* - Add `src/cli/cliProcess.ts`: a single, vscode-free execution seam (`run(command, args, options)`) that every request/response CLI call flows through. Injectable spawn/terminate seams make it fully unit-testable. It spawns detached on POSIX + kills the whole process tree on cancel (taskkill /T /F on Windows), buffers-then-decodes output (no split-code-point corruption), streams decoded chunks to callbacks, and resolves on any exit (exit-code policy is the caller's). - `CliWrapper`'s `execFile`/`cancellableExecFile`/`runBundleCommand` are now thin adapters over `run()`; the Node-`execFile` error surface (stderr in `.message`, `.code`/`.stdout`/`.stderr`) is preserved so `isFileNotFound` and the profile-parse checks keep working. `waitForProcess` and the duplicated Windows escaping are removed. - `PythonSetupCliClient` delegates its spawning to `run()` and keeps only the setup-local concern (argv, JSON parse, onLog, cancel mapping); the shared process primitives (`terminateProcessTree`, `CancellationLike`, seams) now live in `cliProcess` and are re-exported for existing consumers. - The interactive pty terminals (`bundle run`, `bundle sync --watch`) are deliberately left out — they render into a live Pseudoterminal, a different execution model. - `ProcessError`/`CancellationError` stay in `CliWrapper` (they touch `vscode` for their toast), keeping `cliProcess` free of any `vscode` import. Bonus: POSIX cancel now tears down the whole tree (no orphaned terraform/uv), and the buffered path no longer has Node execFile's 1 MB stdout cap. *Verification* - New `cliProcess.test.ts` (21 cases): buffered capture, exit codes, streaming, UTF-8 straddle, partial-byte flush, closeStdin, shell passthrough, spawn error, cancel→tree-kill, Windows escaping — all via injected fakes, run under plain mocha (no extension host). - `yarn test:unit` → 930 passing, 0 failing (the one `shellUtils` real-shell timeout was a load flake; green on rerun). `yarn test:lint` → clean. Co-authored-by: Isaac <no-reply@databricks.com>
rugpanov
requested a deployment
to
test-trigger-is
August 24, 2026 11:21 — with 
GitHub Actions
Queued
GitHub Actions
Queued
Contributor
Author
|
🤖 Integration tests triggered for |
*Why* Review of the cliProcess seam surfaced two real robustness gaps introduced by routing every call through spawn + a cancellation token: - cancel could hang: the run resolved only on "close", so a child that ignored SIGTERM (or a failed taskkill) left bundle/aitools/python-setup operations pending forever. The old buffered execFile path settled promptly on cancel. - a stdout/stderr stream error rejected without terminating the child; since POSIX children are now spawned detached, the tree kept running after the caller saw a failure. *What* - On cancel, `run()` now fires the terminate and settles immediately as cancelled instead of waiting for "close"; a late "close" is ignored. Removes the hang while keeping the whole-tree kill. - On a stdout/stderr "error", `run()` terminates the process tree before rejecting (spawn-level "error" still just rejects — there is no process yet). - Forward-declared the cancellation subscription so a synchronously-firing token can't hit a temporal-dead-zone in `finish`. *Verification* - New `cliProcess.test.ts` cases: "settles promptly on cancel even if the process never closes" and "terminates the process tree when a stdout stream error occurs". New `CliWrapper.test.ts` case pins the buffered non-zero-exit error surface (`.code`/`.stderr`/`.stdout` + stderr in `.message`) that `isFileNotFound` and the profile parser depend on. - `yarn test:unit` -> 933 passing, 0 failing. `yarn test:lint` -> clean. Co-authored-by: Isaac <no-reply@databricks.com>
rugpanov
requested a deployment
to
test-trigger-is
August 24, 2026 11:39 — with
GitHub Actions
Queued
GitHub Actions
Queued
Contributor
Author
|
🤖 Integration tests triggered for |
*Why* Second review pass flagged two issues in the cancellation path: - settling on cancel without confirming teardown could leave a detached CLI alive (and its capture listeners buffering) after the caller moved on, while waiting only for "close" could hang if the child ignored SIGTERM. - a token that fires its callback synchronously on subscription settled the run before the subscription handle was assigned, so it was never disposed (a listener leak); the synchronous terminate could also close the child before the "close" listener was even attached, dropping the event. *What* - Cancel now terminates gracefully (SIGTERM tree), waits for "close", and after a bounded grace escalates to SIGKILL (which the process cannot ignore). The run always settles, and only after a confirmed teardown, so no orphaned tree. Grace period and both terminators are injectable seams. - Subscribe to the cancellation token only after the child listeners (especially "close") are attached, and dispose the subscription explicitly when a synchronous token already settled the run. *Verification* - New cliProcess.test.ts cases: escalates to force kill when the graceful terminate is ignored (then settles on close); does not escalate when the graceful terminate closes the process; disposes the subscription when the token fires synchronously; terminateProcessTree honours a SIGKILL signal. - yarn test:unit -> 936 passing, 0 failing. yarn test:lint -> clean. Co-authored-by: Isaac <no-reply@databricks.com>
Contributor
Author
|
🤖 Integration tests triggered for |
rugpanov
requested a deployment
to
test-trigger-is
August 24, 2026 11:52 — with
GitHub Actions
Queued
GitHub Actions
Queued
…t loop *Why* The fire-and-forget taskkill spawned to tear down the process tree on Windows was never unreferenced, so it could keep the event loop (and the test runner) alive across repeated cancellations. *What* - `unref()` the spawned taskkill helper in the real terminate primitives; still swallow its async spawn error as before. *Verification* - `yarn test:unit` -> 936 passing, 0 failing. `yarn test:lint` -> clean. (The helper is the real OS primitive; tests drive termination through the injected fake, so there is no unit-level behavior change to assert.) Co-authored-by: Isaac <no-reply@databricks.com>
rugpanov
marked this pull request as ready for review
August 24, 2026 12:14
Contributor
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
Contributor
Author
|
🤖 Integration tests ✅ all 41 test jobs passed for |
rugpanov
requested a deployment
to
test-trigger-is
August 24, 2026 12:15 — with
GitHub Actions
Queued
GitHub Actions
Queued
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
1 participant
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
Both the
cliandpython-setupfolders carried their own child-process plumbing for the same job. This unifies them behind one vscode-free execution seam that every request/response CLI call flows through.src/cli/cliProcess.ts—run(command, args, options), carrying zerovscodeimports so it is fully unit-testable via injectedspawn/terminateseams. It:taskkill /T /Fon Windows;SIGTERMto the negated pid on POSIX),dataevents can't corrupt the payload, and streams decoded chunks toonStdout/onStderr,CliWrapper—execFile/cancellableExecFile/runBundleCommandare now thin adapters overrun(). The Node-execFileerror surface (stderr in.message, plus.code/.stdout/.stderr) is preserved so the SDK'sisFileNotFoundand the profile-parse substring checks keep working.waitForProcessand the duplicated Windows-escaping helper are removed.PythonSetupCliClient— delegates its spawning torun()and keeps only the setup-local concern (argv, JSON parse,onLog, cancel→PythonSetupCancelledError). The shared primitives (terminateProcessTree,CancellationLike, seams) now live incliProcessand are re-exported for existing consumers.Scope boundary
The interactive pty terminals (
bundle runviaCustomOutputTerminal,bundle sync --watchviaSyncTasks) are deliberately left out — they render into a livePseudoterminal(ANSI, session-long), a fundamentally different execution model from request/response.ProcessError/CancellationErrorstay inCliWrapperbecause their toast touchesvscode, which keepscliProcessimport-free.Bonus correctness (picked up by the unification)
terraform/uvgrandchildren (the oldCliWrapperpathSIGTERM'd only the direct child).execFile's 1 MB stdout cap.Backward compatibility
No user-facing change and no new setting/state/telemetry/when-clause. Every existing exec contract is preserved: buffered error surface,
closeStdin,{shell:true}passthrough, bundle logger narration +commands.executeCommand("…showLogs"), andPythonSetupCliClient's resolve-on-both-exits + cancel semantics. Windows behavior is unchanged per caller: the buffered/bundle paths keepcmd.exeescaping; setup-local keeps spawning the resolved binary directly.Verification
cliProcess.test.ts(21 cases): buffered capture, exit codes, streaming, UTF-8 straddle, partial-byte flush,closeStdin,shellpassthrough, spawn error, cancel→tree-kill, and Windows escaping — all via injected fakes, run under plain mocha (no extension host).PythonSetupCliClient(16 cases) andCliWrappersuites stay green with the adapters unchanged.yarn test:unit→ 930 passing, 0 failing.yarn test:lint→ clean.This pull request and its description were written by Isaac.