Skip to content

fix(stop): make coli stop work on Windows — portable pid liveness probe + SIGKILL guard - #1069

Closed
sami7969 wants to merge 2 commits into
JustVugg:devfrom
sami7969:fix/windows-coli-stop
Closed

fix(stop): make coli stop work on Windows — portable pid liveness probe + SIGKILL guard#1069
sami7969 wants to merge 2 commits into
JustVugg:devfrom
sami7969:fix/windows-coli-stop

Conversation

@sami7969

Copy link
Copy Markdown

Follow-up to #1049, which you invited a PR for. Two fixes, both inside cmd_stop.

6707d1a fixed the /proc traceback I originally reported — confirmed gone on abbb8de.
Retesting on current dev turned up a different reason coli stop still doesn't work on
Windows, plus the SIGKILL issue 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 wrong

With a live serve on a listening port and a valid pidfile:

$ python coli serve --model D:/models/olmoe_merged --host 127.0.0.1 --port 8000   # running
$ type %TEMP%\coli-serve-8000.pid
692712 D:/models/olmoe_merged
$ python coli stop --port 8000
  nothing running — no serve on port 8000, no SERVE engines
$ # exit 0, port 8000 still LISTENING, ~5.2 GB of engines still resident

c/coli:1409 uses the POSIX idiom inside a bare handler:

with open(pf) as f: pid=int(f.read().split()[0])
os.kill(pid,0); targets[pid]=f"coli serve (pidfile, port {a.port})"
except (OSError,ValueError,IndexError): pass

On Windows os.kill() has no signal semantics, and for a pid the calling process did not spawn
it raises OSError [WinError 87] ("The parameter is incorrect") instead of reporting liveness.
That is swallowed, targets stays 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 (OpenProcess with
PROCESS_QUERY_LIMITED_INFORMATION + GetExitCodeProcess) and keeps os.kill(pid, 0)
everywhere else. POSIX behaviour is unchanged.

Testing note, because it cost me an hour: os.kill(pid, 0) does succeed against a child
the same interpreter spawned via subprocess.Popen — that handle already carries the access
rights OpenProcess would have to request for a foreign pid. A smoke test written that way
passes and hides the bug completely.

2. signal.SIGKILL — only reachable once the probe is fixed

try: os.kill(pid, signal.SIGKILL); print(f"  {pid}: forced (SIGKILL)")
except OSError: pass

signal.SIGKILL does not exist on win32 and AttributeError is not OSError, so this escapes
the handler and aborts the command after it has already SIGTERMed its targets.

This line is dead on Windows before the first committargets was always empty, so
cmd_stop returned before escalating. Fixing the probe is what exposes it. Worth stating
explicitly: reading the second commit alone doesn't show why it matters.

The fallback is signal.SIGTERM, not the -1 used by the SIGKILL exit-code check earlier in
this file. That difference is deliberate and commented: there the value is a sentinel compared
against an exit code, where -1 correctly never matches; here it is sent, and Windows records
the signal number as the terminated process's exit code — -1 would record 0xFFFFFFFF where
SIGTERM records 15. The status line now prints the signal actually used rather than a
hardcoded SIGKILL.

Escalation path checked, since these fixes make it reachable for the first time on Windows:
SIGTERM at :1400 then the escalation call at :1411 means two SIGTERMs there. Against an
already-dead pid the second raises OSError [WinError 5], and against a never-existed pid
OSError [WinError 87] — both caught by the existing handler. No new traceback.

Result

before:  nothing running — no serve on port 8000, no SERVE engines
         port 8000 still LISTENING, pidfile left behind, ~5.2 GB resident

after:   stopping 361356: coli serve (pidfile, port 8000)
         ✓ stopped — RAM released
         port 8000 free, pidfile removed

A stale pidfile naming a dead pid is still correctly ignored (_pid_alive returns False), so a
recycled 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, because
make check currently dies before any test runs, at
Makefile.deepseek-v4:31 → Error 2 at tests/test_deepseek_v4.exe. That is unrelated to this PR
and is what #1047 fixes.

With #1047's two commits applied on top of this branch:

Ran 574 tests in 166.913s
OK (skipped=39)

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_stop fixed, the OMP re-exec'd engine
survives a successful stop — its parent has already exited, so neither the pidfile
(written at c/coli:1355, before openai_server.serve() spawns the engine at
openai_server.py:1700) nor parent-child discovery reaches it. Fixing it means changing the
pidfile contract across a second file, or reading another process's environment block on
Windows to mirror the Linux /proc/PID/environ path. Both are larger than this PR should be and
are 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.

…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.
@sami7969 sami7969 changed the title fix(stop): make work on Windows — portable pid liveness probe + SIGKILL guard fix(stop): make coli stop work on Windows — portable pid liveness probe + SIGKILL guard Aug 17, 2026
@JustVugg

Copy link
Copy Markdown
Owner

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:

  • _pid_alive() via OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION) + GetExitCodeProcess, because os.kill(pid, 0) has no signal semantics on win32 — your diagnosis, which is what made the fix possible at all;
  • getattr(signal, "SIGKILL", signal.SIGTERM), since AttributeError is not OSError and SIGTERM is TerminateProcess on Windows anyway.

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 forced — on Windows the user then sees that it was SIGTERM, not a phantom SIGKILL, which is exactly the sort of small honesty that stops the next confused bug report. Please send that as a one-line PR and I'll merge it.

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 KILL_ON_JOB_CLOSE in openai_server.py, since job membership is inherited by child processes and therefore survives the re-exec whose parent has already exited. Your measurement with the 740652 — already exited parent column is what ruled out every pid-based approach and pointed at it.

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 olmoe.exe behind.

@JustVugg JustVugg closed this Aug 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants