fix(stop): make coli stop work on Windows — portable pid liveness probe + SIGKILL guard - #1069
fix(stop): make coli stop work on Windows — portable pid liveness probe + SIGKILL guard#1069sami7969 wants to merge 2 commits into
coli stop work on Windows — portable pid liveness probe + SIGKILL guard#1069Conversation
…ness check on Windows
On Windows os.kill() has no signal semantics. For a pid the calling process did
not spawn it raises OSError [WinError 87] ("The parameter is incorrect") instead
of reporting liveness, and cmd_stop's pidfile probe swallows that in its bare
except (OSError, ValueError, IndexError). The pidfile target is silently dropped,
targets stays empty, and the command reports success having stopped nothing:
$ python coli serve --model ... --port 8000 # running, port listening
$ python coli stop --port 8000
nothing running - no serve on port 8000, no SERVE engines
$ # serve alive, port 8000 still LISTENING, ~5.2 GB of engines resident
Add _pid_alive(): OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION) +
GetExitCodeProcess on Windows, os.kill(pid, 0) everywhere else. POSIX behaviour
is unchanged.
Note the probe is easy to mis-test: os.kill(pid, 0) DOES succeed against a child
the same interpreter spawned, because that handle already carries the access
rights OpenProcess would have to request for a foreign pid.
cmd_stop's escalation step references signal.SIGKILL inside a handler that only
catches OSError. On Windows signal.SIGKILL is absent, so this raises
AttributeError, which escapes and aborts the command after it has already sent
SIGTERM to every target.
>>> import signal, sys; sys.platform, hasattr(signal, 'SIGKILL')
('win32', False)
os.kill() maps every signal to TerminateProcess on Windows, so SIGTERM is the
equivalent forced kill there; getattr mirrors the guard already used for the
SIGKILL exit-code check earlier in this file.
This line is unreachable on Windows before the preceding commit: the pidfile
probe left targets empty, so cmd_stop returned at the 'nothing running' branch
without ever escalating. Fixing the probe is what exposes it.
coli stop work on Windows — portable pid liveness probe + SIGKILL guard
|
I owe you an apology for this one, and I'd rather be precise about it than vague. On #1049 you wrote "I've opened a PR for both of these" at 16:06. Twenty-eight minutes later I opened #1059 doing the same two fixes, and merged it within the hour. Your PR landed the next morning. So I didn't overtake an existing PR — but you told me you were writing one and I built it anyway without checking, which is the part that wasted your evening. That was careless of me and I'm sorry. For the record, we independently reached the same design, which is some consolation about the design being right:
Your version has one thing mine doesn't, and it's a genuine improvement: print(f" {pid}: forced ({_sigkill.name})")Naming the signal in the output is better than my bare What #1059 added beyond the scope of yours was the third defect from your issue — the orphaned OMP re-exec holding 2.6 GB. That needed a Windows Job Object with Closing this as superseded by #1059 — superseded in fact, not in merit. And the ask from #1049 still stands and is still yours: CI proved the job gets created and kills a child on Windows, but only you can confirm a real serve/stop cycle leaves no |
Follow-up to #1049, which you invited a PR for. Two fixes, both inside
cmd_stop.6707d1afixed the/proctraceback I originally reported — confirmed gone onabbb8de.Retesting on current
devturned up a different reasoncoli stopstill doesn't work onWindows, plus the
SIGKILLissue you identified, which that first bug was masking.1.
os.kill(pid, 0)is not a liveness probe on Windows — this is the one that makes the command wrongWith a live serve on a listening port and a valid pidfile:
c/coli:1409uses the POSIX idiom inside a bare handler:On Windows
os.kill()has no signal semantics, and for a pid the calling process did not spawnit raises
OSError [WinError 87] ("The parameter is incorrect")instead of reporting liveness.That is swallowed,
targetsstays empty, and the command returns at its "nothing running"branch having stopped nothing. So the adjacent comment — "native Windows has no /proc; the
pidfile still works" — does not hold in practice.
_pid_alive()queries the OS directly on Windows (OpenProcesswithPROCESS_QUERY_LIMITED_INFORMATION+GetExitCodeProcess) and keepsos.kill(pid, 0)everywhere else. POSIX behaviour is unchanged.
2.
signal.SIGKILL— only reachable once the probe is fixedsignal.SIGKILLdoes not exist on win32 andAttributeErroris notOSError, so this escapesthe handler and aborts the command after it has already SIGTERMed its targets.
This line is dead on Windows before the first commit —
targetswas always empty, socmd_stopreturned before escalating. Fixing the probe is what exposes it. Worth statingexplicitly: reading the second commit alone doesn't show why it matters.
The fallback is
signal.SIGTERM, not the-1used by theSIGKILLexit-code check earlier inthis file. That difference is deliberate and commented: there the value is a sentinel compared
against an exit code, where
-1correctly never matches; here it is sent, and Windows recordsthe signal number as the terminated process's exit code —
-1would record0xFFFFFFFFwhereSIGTERMrecords 15. The status line now prints the signal actually used rather than ahardcoded
SIGKILL.Escalation path checked, since these fixes make it reachable for the first time on Windows:
SIGTERMat:1400then the escalation call at:1411means twoSIGTERMs there. Against analready-dead pid the second raises
OSError [WinError 5], and against a never-existed pidOSError [WinError 87]— both caught by the existing handler. No new traceback.Result
A stale pidfile naming a dead pid is still correctly ignored (
_pid_alivereturns False), so arecycled pid is not killed by mistake.
Test evidence
c/tests/test_stop_scope.py— your own coverage of this code — passes: 8 tests,OK (skipped=1).On
make check, honestly: this branch alone cannot go green on Windows/MSYS2, becausemake checkcurrently dies before any test runs, atMakefile.deepseek-v4:31 → Error 2 at tests/test_deepseek_v4.exe. That is unrelated to this PRand is what #1047 fixes.
With #1047's two commits applied on top of this branch:
So these changes introduce no regression; the branch is red on its own only because of that
separately-filed blocker. Taking #1047 first would make this one verifiable in CI on Windows.
Not included: an orphaned engine holding ~2.6 GB
Documented with measurements in #1049. Even with
cmd_stopfixed, the OMP re-exec'd enginesurvives a successful stop — its parent has already exited, so neither the pidfile
(written at
c/coli:1355, beforeopenai_server.serve()spawns the engine atopenai_server.py:1700) nor parent-child discovery reaches it. Fixing it means changing thepidfile contract across a second file, or reading another process's environment block on
Windows to mirror the Linux
/proc/PID/environpath. Both are larger than this PR should be andare your design call — happy to implement whichever you prefer.
Environment
Windows 11 Enterprise 26200, native (no WSL) · MSYS2 20260611 (MINGW64) · mingw-w64 gcc 16.2.0 ·
Python 3.12.10 · OLMoE int8 engine. Base:
origin/dev@abbb8de.