fix(tinytorch): tito module start/resume/view leak untracked Jupyter Lab processes - #2011
Closed
Shashank-Tripathi-07 wants to merge 4 commits into
Closed
Conversation
github-actions
Bot
added
area: tinytorch
…Lab processes Bug Every tito module start (without --no-jupyter), resume, or view call launches a brand new jupyter lab subprocess.Popen with no PID tracking, no cleanup, and no check for whether one is already running. Over a normal working session where a student opens several modules, these accumulate indefinitely: confirmed 45 leaked processes after routine module-by-module testing, and separately confirmed that a build-up of ~38 live kernels was enough to make the Jupyter Lab UI itself sluggish (menu clicks timing out). Fix Track the launched Jupyter Lab process's PID in .tito/jupyter.pid. Before launching a new one, check whether that PID is still alive and is actually still a jupyter process (guards against a recycled PID being mistaken for a live server). If so, reuse it: tell the student which notebook to open in their existing tab instead of spawning another server. Only launch a new process if no tracked server is currently running. Testing Verified against a fresh dev install with all 20 modules completed: - First tito module view 01: launches Jupyter Lab as before, records its PID. - Second tito module view 01 (simulating a student opening another module in the same session): before the fix, this would spawn a second full Jupyter Lab server. After the fix, detects the existing PID, prints "Jupyter Lab is already running (pid N)" with the notebook to open, and spawns nothing. Confirmed via process listing that the jupyter.exe count stayed at 1 across both calls.
The psutil-unavailable fallback in _running_jupyter_pid used os.kill(pid, 0), assuming POSIX signal-0 "probe only" semantics. On Windows, os.kill does not implement that: it calls TerminateProcess for any signal value, including 0. The fallback path meant to check whether a Jupyter server was still running would instead kill it. psutil is already a required project dependency (requirements.txt, tito setup's own package list), so this path should only be reached in a broken environment. Removed the unsafe probe: if psutil can't be imported, treat it the same as "can't tell" and return None, so the caller launches a fresh server instead. Worst case is a duplicate server (the same behavior this PR is fixing in the common case), never silently killing a running one.
Shashank-Tripathi-07
force-pushed
the
fix/tinytorch-jupyter-process-leak
branch
from
August 11, 2026 15:39
ef01ba5 to
2b890a5
Compare
Collaborator
Author
|
Addressed two points: Windows os.kill(pid, 0) bug (real problem, not a nit): confirmed and fixed. Removed the unsafe fallback entirely -- on ImportError, `_running_jupyter_pid` now just returns `None` (treated the same as "can't verify") instead of attempting an os.kill probe. psutil is already a required project dependency (requirements.txt, tito setup's own package list), so this path should only be hit in a broken environment, and even then the worst case is now a duplicate server spawn, never an accidental kill. Verified in isolation with psutil's import faked to raise ImportError: the function returns None and the target process (verified against my own running PID) stays alive throughout. Duplicate hunks with #2010: rebased this branch directly onto `dev` (via `git rebase --onto dev `), dropping the #2010 commit that had ended up as an ancestor here. The diff against `dev` is now clean: only the two commits that actually belong to this PR (the leak fix + the os.kill fix), 55 lines total, no overlap with #2010. Force-pushed the rewritten branch. No merge-order dependency remains for this PR specifically, though #2010 should still merge before or independently of this one since they touch the same file's nearby regions. |
… only The previous commit's fix was correct (removed the os.kill(pid, 0) Windows footgun), but its comment claimed psutil is "already a required project dependency" -- it isn't, in either pyproject.toml or requirements.txt (only tito setup's own separate package-install list includes it, which is not the same thing). Since psutil is absent by default, _running_jupyter_pid() always hit the "can't verify" path and always returned None, meaning the leak-prevention logic silently no-opped and the original leak this PR set out to fix was still present in the common case -- just no longer dangerous. Replaced the psutil dependency entirely with a stdlib-only check: - Windows: query the process via the Win32 API (OpenProcess + QueryFullProcessImageNameW through ctypes) to confirm both liveness and that it's actually a Jupyter process, without ever sending it any kind of signal. - POSIX: os.kill(pid, 0) is a real, safe liveness probe there (unlike Windows). Best-effort cmdline verification via /proc/<pid>/cmdline on Linux; falls back to "alive is good enough" on platforms without /proc (macOS/BSD), since there's no portable stdlib way to get a process's command line there. Testing Verified on this Windows machine, launching and reusing detection without any timeout-wrapper interference (an earlier test attempt using a `timeout N` wrapper around the launching command turned out to kill the detached Jupyter child once the wrapped command completed, a Windows job-object artifact of that specific test harness, not a bug in this code -- confirmed by retesting with a plain backgrounded call instead): - First tito module view 01: launches Jupyter Lab, PID confirmed alive via Get-Process independently afterward. - Second tito module view 01: correctly detects the existing PID via the new Win32-API-based check and prints "Jupyter Lab is already running (pid N)" instead of launching a second server. jupyter.exe process count for this project confirmed to stay at 1 across both calls. - Confirmed psutil is not installed as a dependency anywhere in this path (grepped pyproject.toml and requirements.txt).
… check The Windows liveness check called kernel32.OpenProcess/QueryFullProcessImageNameW/CloseHandle through ctypes without declaring their argtypes/restype. HANDLE is pointer-width (64-bit on Win64); without an explicit restype, ctypes defaults OpenProcess's return to a 32-bit signed int and truncates it. That happens to be harmless for the small handle values Windows hands out in practice, but it is not correct per the Win32 API contract. Declared the real argtypes/restype for OpenProcess, QueryFullProcessImageNameW, and CloseHandle so the kernel-level values (HANDLE, DWORD, BOOL) are marshalled correctly instead of relying on that truncation happening to be safe.
Collaborator
Author
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.
Bug
Every
tito module start(without--no-jupyter),resume, orviewcall launches a brand newjupyter labsubprocess.Popenwith no PID tracking, no cleanup, and no check for whether one is already running. Over a normal working session where a student opens several modules, these accumulate indefinitely: confirmed 45 leaked processes after routine module-by-module testing, and separately confirmed that a build-up of ~38 live kernels was enough to make the Jupyter Lab UI itself sluggish (menu clicks timing out).Fix
Track the launched Jupyter Lab process's PID in
.tito/jupyter.pid. Before launching a new one, check whether that PID is still alive and is actually still a jupyter process (guards against a recycled PID being mistaken for a live server). If so, reuse it: tell the student which notebook to open in their existing tab instead of spawning another server. Only launch a new process if no tracked server is currently running.Testing
Verified against a fresh
devinstall with all 20 modules completed:tito module view 01: launches Jupyter Lab as before, records its PID.tito module view 01(simulating a student opening another module in the same session): before the fix, this would spawn a second full Jupyter Lab server. After the fix, detects the existing PID, prints "Jupyter Lab is already running (pid N)" with the notebook to open, and spawns nothing. Confirmed via process listing that thejupyter.execount stayed at 1 across both calls.Test plan
tito module start N(without--no-jupyter), confirm Jupyter Lab launches and.tito/jupyter.pidis writtentito module view Mfor a different already-started module while the first server is still running, confirm it reuses the existing server instead of launching a second onetito module view, confirm it correctly detects the server is gone and launches a fresh one