Skip to content

[Engine] stabilise the serve test job: lifeline identity by start time, sampler that survives a bad source - #3590

Closed
justindressler wants to merge 5 commits into
Niko1221:mainfrom
justindressler:fix/flaky-serve-tests
Closed

justindressler wants to merge 5 commits into
Niko1221:mainfrom
justindressler:fix/flaky-serve-tests

Conversation

@justindressler

@justindressler justindressler commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Problem and result

The Minimal serving runtime job fails intermittently on changes that do not touch serve/ at all,
which makes every PR's check look broken for reasons that have nothing to do with the PR. Two different
failures were observed on this account's fork, on the same 929-test run:

  1. test_responses.NoLeakedSampler.test_server_close_ends_the_sampler — AssertionError: 1 != 2, twice
    in a row on one branch and green on the next run of the same commit.
  2. test_lifeline.Lifeline.test_the_portable_watcher_ends_the_child_when_the_server_is_killed —
    AssertionError: False is not true : the child outlived a killed server (watcher), on main with no
    local changes, and on v0.2.2's own run.

Result: both are fixed at the cause, not by loosening the assertion.

The lifeline one was a real bug, and it reproduced here 6 times out of 8. serve/lifeline.py built the
child's identity with ps -o lstart=,stat= and kept the whole line, so the token changed whenever the
process changed state — measured on one live child: ... R once, ... S eleven times inside 150 ms.
The portable watcher compares its own reading against the token taken at start, concluded "that is not my
child any more", and exited without ending it. Identity is now the start time alone: field 22 of
/proc/<pid>/stat on Linux (the same rule strata_control/procs.py::start_token already used), and
ps -o lstart= elsewhere; a zombie has no identity, so an already-dead child is skipped rather than
signalled. After the change the same test passed 8 out of 8.

The sampler one turned out not to be a race in the test. The count-based assertion was ambiguous
(1 != 2), so it was rewritten to compare thread identities and say which half failed — and the next CI run
on that rewritten test reported 0 != 1 : the server did not start its hardware sampler. The thread was
being started and killed immediately: Telemetry._loop had no exception handling, so the first failing
sample() on a machine whose GPU/psutil source is not readable ended the sampler for good. GET /metrics
then silently stops updating. _loop now catches a failing sample, prints one line, and keeps sampling once
a second ("a monitor must outlive its source"), and the test asserts on thread identity so it can only
fail for the real reason.

Related discussion

  • [Engine] serve/lifeline.py: the portable watcher does not end the child when the child's ps state changes (test_lifeline fails 5/5 on Linux) #3407, [Engine] serve/lifeline.py: the portable watcher does not end the child when the child's ps state
    changes (test_lifeline fails 5/5 on Linux)
    — the same root cause, reported independently a day earlier
    with the same ps -o lstart=,stat= diagnosis. That report proposes dropping the state column from the
    ps output; this PR takes the equivalent idea further and removes the ps call: on Linux the identity is
    field 22 of /proc/<pid>/stat, which is what strata_control/procs.py::start_token already uses, so no
    subprocess and no locale-dependent date parsing. The sampler half has no existing thread.
  • Scope agreement is not needed for either half: both are focused fixes to an existing test job, which
    CONTRIBUTING.md allows straight to a PR.

Approach

Three small changes, no behaviour change beyond the watcher and the sampler doing what they were meant to
do:

  • serve/lifeline.py: _start_token() and the inline start_of() inside _WATCHER use the process start
    time as identity instead of a ps line that includes the volatile state column. Linux reads
    /proc/<pid>/stat directly (no subprocess call, which also removes a ps dependency from the watcher
    on that path); other POSIX uses ps -o lstart= only. Zombies return no identity.
  • serve/test_responses.py: NoLeakedSampler uses a set of threading.enumerate() idents instead of a
    count, and asserts on the difference, with a message that names which half failed.
  • serve/telemetry.py: Telemetry._loop survives a failing sample() instead of dying on the first one,
    and complains once rather than once a second.

Both fixes are additive to the existing behaviour: the pdeathsig and Windows job-object paths are
untouched, and release()/end_children() are unchanged.

Validation

Revision: main at 011c9e60 (0.2.2). Environment: WSL2 on x86-64, Python 3.12, no GPU (none needed).

Passed:

  • python -m unittest serve.test_lifeline.Lifeline.test_the_portable_watcher_ends_the_child_when_the_server_is_killed
    — 8/8 runs green after the change; the same command was 6 failures out of 8 before it, so the
    repro and the fix are both measured, not argued.
  • python -m unittest serve.test_lifeline — 6 tests, OK.
  • python -m unittest serve.test_responses.NoLeakedSampler — 6/6 runs green (and green again after the
    telemetry change).
  • A sampler whose sample() always raises: thread still alive after 2.4 s, 3 sample attempts, one complaint
    line, and it still stops cleanly.
  • python -m unittest discover -s serve -p 'test_*.py' (the job's own command) — 929 tests, OK (skipped=14) in 307 s, 0 failures — the first fully green run of
    this suite in this campaign; the same command failed one or both of the tests above on every previous
    attempt..

Failed:

  • None observed after the change.

Not run:

  • Windows (job-object path) and macOS (ps -o lstart= fallback): no hardware here. The Linux path is
    exercised on every run of these tests; the other two are reasoned about, not measured.
  • CUDA, HIP and SYCL builds: these files are Python process supervision and a test, nothing the engine
    compiles.

Limits

  • Identity on the non-Linux POSIX path is ps -o lstart=, which is second-granular. A PID recycled within
    the same second could alias — the same limitation this code had before, and the Linux path (where the
    watcher runs in practice) is exact.
  • The watcher still gives the child SIGTERM, waits up to 10 s, then SIGKILLs; a child that ignores SIGTERM
    is unchanged in behaviour.

Update 2026-10-23 — rebased, and the lifeline half now has a test for its cause

Rebased onto main 7ddc17a3 (was 011c9e60); clean, 3 commits, 4 files, +87 -9.

Added test_the_watcher_token_does_not_change_with_the_child_state, which samples the identity token twice on one live child (while it spins as R, then while it sleeps as S). The existing behavioural test cannot catch this class of regression on its own: it passes whenever the spawn is sampled after the child settles, which is exactly the shape of the flake.

Validation at 02db1cf4, Debian 13 container, Python 3.11.15, no GPU (none needed):

  • python -m unittest discover -s serve -p 'test_*.py' — 975 tests, OK (skipped=14)
  • python -m unittest serve.test_lifeline — 7 tests, OK; the watcher case 5/5 consecutive runs
  • python -m unittest serve.test_responses.NoLeakedSampler — 3/3 consecutive runs
  • new test against base 7ddc17a3 — fails, so it pins the defect rather than restating the fix

Not run: Windows job-object path, macOS, and any CUDA/HIP/SYCL build — Python only, no compiled path touched.

@justindressler justindressler changed the title [Engine] stabilise the serve test job: lifeline identity by start time, race-free sampler test [Engine] stabilise the serve test job: lifeline identity by start time, sampler that survives a bad source Oct 21, 2026
@i-Vladimir-i

Copy link
Copy Markdown
Contributor

@justindressler Independently confirmed while validating #2435 on main 011c9e600a93.

A controlled S→R transition changes the old ps-based token for the same live child. Closing the watcher pipe then leaves that child alive on both unchanged main and #2435 (de511d394a8f). With this PR at acb05a7fe948, the token stays stable and the child is ended. Mismatched-start-time and release guards still preserve the child.

The patch applies cleanly to #2435. On Linux/Python 3.12.3, CPU-only, we measured 10/10 repeated portable-watcher tests on main plus this PR and 10/10 on the combined tree. The combination also passes 27/27 focused tests and full server discovery: 915 passed, 0 failures/errors, 18 skipped out of 933.

This matches #3407. The original GitHub failure on #2435 is still visible; no CI rerun or Windows/macOS validation is claimed.

@i-Vladimir-i

Copy link
Copy Markdown
Contributor

@justindressler @Niko1221 Fresh independent confirmation on October 23, against current main 5c8dd322528c and the updated vision-only #2435 (d7059ebe0a43).

New GitHub CI evidence: #2435 / Minimal serving runtime / failing job, Python 3.11.17:

  • Runtime-only smoke: 43 passed, 4 skipped, 47 total.
  • Full server discovery: 964 passed, 14 skipped, 1 failure, 979 total, exit 1.
  • The only failure is test_lifeline.Lifeline.test_the_portable_watcher_ends_the_child_when_the_server_is_killed: the child outlived a killed server (watcher). The vision regressions pass. No CI rerun was requested.

Dev reproduction and control: on Linux/Python 3.12.3, a controlled S-to-R transition changes the old ps-based identity token for the same live child. After watcher-pipe EOF, the child survives on both unchanged main and #2435. With this PR unchanged at acb05a7fe948, identity stays stable and the watcher ends the child. Wrong-identity and explicit-release guards preserve the child in all three arms.

Full python -m unittest discover -s serve -p 'test_*.py' -v, with no test exclusions:

Dev tree Passed Skipped Failures/errors Total Exit
main + #3590 948 26 0 974 0
#2435 + #3590 953 26 0 979 0

The standalone #2435 suite also passed locally this time (953 passed / 26 skipped), but the deterministic probe still reproduced the bug; that green run does not establish a fix. These are CPU/fake-engine tests, not Windows/macOS or real CUDA OOM validation.

Following CONTRIBUTING's one-problem-per-PR rule, #2435 remains a Draft and explicitly names #3590 as a CI-stability prerequisite, not a functional dependency of the HTTP 503 change. Your fix is not copied into our branch, nor are assertions weakened or tests skipped to hide the failure. Once this lands, we can update #2435 from main and rerun it standalone.

_start_token() kept ps's whole 'lstart,stat' line, so the token changed whenever the
child changed state (measured: R once, S eleven times inside 150 ms). The portable
watcher compared its own reading with the token from start time, concluded it was a
different process, and exited without ending the child - the 'child outlived a killed
server' failure. Identity is now the start time alone (/proc/<pid>/stat field 22 on
Linux, as strata_control.procs.start_token already did; ps -o lstart= elsewhere), and a
zombie has no identity.

test_server_close_ends_the_sampler compared thread counts, so an unrelated sampler
finishing between the two readings read as 1 != 2; it now compares thread idents.

Measured: watcher test 8/8 after (6 failures in 8 before), sampler test 6/6, and the
job's own command (unittest discover -s serve) 929 tests OK, 0 failures.
The new identity-based assertion in NoLeakedSampler reported the real reason on CI:
'0 != 1 - the server did not start its hardware sampler'. It had started and died:
_loop had no exception handling, so the first failing sample() on a machine whose GPU
or psutil source is not readable ended the sampler for good, and GET /metrics stopped
updating silently.

One failing sample now prints one line and the loop keeps its once-a-second cadence.

Measured: a Telemetry whose sample() always raises stays alive past 2.4 s (3 attempts,
one complaint) and still stops cleanly; the sampler test is green and the job's own
command (unittest discover -s serve) is 929 tests OK, 0 failures.
… state

The lifeline fix in the previous commit is only as good as the test that keeps it
in place.  `test_the_portable_watcher_ends_the_child_when_the_server_is_killed`
proves the behaviour but cannot prove the CAUSE: it passes whenever the spawn
happens to be sampled after the child has settled to S, which is exactly why the
bug read as a flake (it failed 3/3 in one container and passed 3/3 in another
without any change).

This samples the identity token twice on one live child - while it spins (R) and
again while it sleeps (S) - and requires the same value both times.  That is the
property the watcher depends on, and the property the old `ps -o lstart=,stat=`
token broke.

    base 7ddc17a : FAILED (failures=1)
    with the fix  : OK

serve.test_lifeline is 7/7 with it, the watcher case 5/5 on repeated runs, and
NoLeakedSampler 3/3.
@justindressler

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (7ddc17a3), and added the regression test that was missing for the lifeline half.

Thanks @i-Vladimir-i — your controlled S→R probe and the #2435 CI log are the part I could not produce myself (Windows/macOS and a second card), and they agree with the diagnosis exactly.

What changed since acb05a7f

  1. Rebase onto 7ddc17a3 (the branch was based on 011c9e60). Clean, no conflicts. Nothing in main touched serve/lifeline.py in between, so the lifeline commit is byte-identical in effect.

  2. A test for the cause, not just the behaviour. test_the_portable_watcher_ends_the_child_when_the_server_is_killed proves the child dies, but it cannot prove why — it passes whenever the spawn happens to be sampled after the child has settled to S, which is exactly why this looked like a flake. The new test_the_watcher_token_does_not_change_with_the_child_state samples lifeline._start_token() twice on one live child: while it spins (R) and again while it sleeps (S). Same value both times or the test fails.

    base 7ddc17a3, new test only : FAILED (failures=1)
    with this branch            : OK
    

Validation at 02db1cf4

Revision 02db1cf4, Debian 13 container, Python 3.11.15, no GPU (none needed for these paths).

Passed:

  • python -m unittest discover -s serve -p 'test_*.py' (the job's own command) — 975 tests, OK (skipped=14), 314.9 s.
  • python -m unittest serve.test_lifeline — 7 tests, OK.
  • ... test_the_portable_watcher_ends_the_child_when_the_server_is_killed — 5 of 5 consecutive runs.
  • python -m unittest serve.test_responses.NoLeakedSampler — 3 of 3 consecutive runs.
  • The new token test against base 7ddc17a3 — fails, which is how I know it pins the defect.

Failed: none observed after the change. One item worth recording, because it is the same cause and not a new failure: in a container where the spawn was sampled as R the old code made the watcher exit without signalling — watcher gone at t+2 s, child still alive at t+20 s, hand-run. In another container the same unpatched code passed 3/3. That is the flake's shape, and it is why the CI evidence in #2435 and the local 3/3 can both be true at once.

Not run: Windows job-object path, macOS, and any CUDA/HIP/SYCL build — this is Python only (serve/lifeline.py, serve/telemetry.py, serve/test_*.py) and touches no compiled path. #2435's failing job is the same test_lifeline case as this thread.

Limits: the ps -o lstart= fallback still depends on ps existing on non-Linux POSIX; Linux now needs no subprocess at all. release() and end_children() are unchanged, so an explicitly released child is still spared.

@i-Vladimir-i

Copy link
Copy Markdown
Contributor

@justindressler @Niko1221 Independently rechecked the updated head 02db1cf4b77649c66f6b0c3053ed295e1c44f330 against unchanged main 7ddc17a38fe7f31637e895ecfba58c2cd43d65ec on October 23. The new regression distinguishes the fix from main in 3/3 separate runs.

Environment: Linux 6.8.0-147-generic x86_64, glibc 2.39, Python 3.12.3, existing dev venv. Separate clean detached worktrees, isolated process-state directories, no dependency installation or GPU/model execution.

Passed / independent results:

Check Unchanged main 7ddc17a3 This PR 02db1cf4
New test_the_watcher_token_does_not_change_with_the_child_state Expected failure 3/3, exit 1 Passed 3/3, exit 0
python -m unittest serve.test_lifeline -v 6/6, exit 0 7/7, exit 0
Full server discovery, no exclusions 974 total / 948 passed / 26 skipped, exit 0, 333.018 s 975 total / 949 passed / 26 skipped, exit 0, 336.145 s
Separate Node/web supplement on the same revisions 12/12, exit 0 12/12, exit 0

Full discovery command: python -m unittest discover -s serve -p 'test_*.py'. Both full suites ran concurrently in separate processes; timings are not a performance comparison.

For the negative control, main's source was not modified: importlib loaded the exact new test module from this PR while serve.lifeline came from the unchanged main worktree, with an assertion checking its actual file path. Every control failed specifically on the token's state suffix for the same live PID:

... R != ... S
the token moved with the child's state, so the watcher would skip the kill
FAILED (failures=1)

The old full main suite passing this time does not contradict that failure: it lacks the new regression, and the portable watcher case remains timing-sensitive.

Twelve initial skips were because Node was outside the process PATH. With the already-installed Node v24.21.0 added only to the validation process's PATH, python -m unittest serve.test_web_api_key serve.test_web_sampling -v passed 12/12 on each revision. This is a supplement, not a claim that the original full run had only 14 skips. The remaining 14 were Anthropic SDK (3), pack tokenizer (5), jsonschema (3), and Windows (3).

Failed: no unexpected candidate failures; only the three intended negative-control failures on main.

Not run: Python 3.11 / clean runtime-only CI venv, minimal_server_smoke.py, Windows job objects, macOS fallback, or GPU/backend builds and inference. No CI rerun or production changes. #2435 remains separate; these dev results do not turn its existing failing CI job green.

@justindressler

Copy link
Copy Markdown
Contributor Author

Thanks — that is a stronger control than the one I used. Loading the new test module with importlib against an unmodified serve/lifeline.py and asserting the file path is a cleaner separation than my stash-based check, and it removes the obvious objection that the negative control and the fix came from the same tree.

Two things worth recording from your numbers:

Discovery parity. Our run and yours both report 975 tests total on this head, from separate machines and Python builds. The skip counts differ (yours 26, ours 14) and your breakdown accounts for all twelve of the extra ones — Node off the PATH, Anthropic SDK, pack tokenizer, jsonschema, Windows. So neither run added or silently dropped a test; the suites are the same suite.

The gaps you listed as not run are now covered at the same head. I dispatched the repository's own runtime-only CI job against 02db1cf4 in the fork, which is the clean venv you were missing:

run contents result
40031380554 — Minimal serving runtime Python 3.11.17, pinned runtime requirements only, no terminal packages success

Inside that job: minimal_server_smoke.py → "passed": true; the fake-engine serving check → 47 tests, OK (skipped=4); and the job's own full command python -m unittest discover -s serve -p 'test_*.py' → 975 tests, OK (skipped=14), 314.4 s.

That closes Python 3.11, the runtime-only venv and the smoke test from your "not run" list. Still not run by either of us, and I am not claiming otherwise: Windows job objects, the macOS ps fallback, and any GPU/backend build — this PR is Python only and touches no compiled path.

@Niko1221 — the two commits are unchanged since the review above, and the branch is now based on current main. The check suites on this PR sit at action_required because it comes from a fork, so a maintainer approval is what would put CI on the thread itself; the runs linked here are from the fork at the same commit.

One limit I want stated plainly rather than buried: the new test pins the token's stability, not the watcher's end-to-end timing. The behavioural case remains timing-sensitive on a loaded machine — it passes because the token is now correct, not because the wait got longer.

Cross-model review of the branch (GLM-5.3 and DeepSeek-V4-Flash; MiniMax-M3
stalled on its API call and was abandoned) returned APPROVE from both, with the
same MINORs found independently. This commit closes the ones that were real.

1. Linux without procfs could never match.  `_start_token()` returned "" on any
   /proc read failure, while the watcher's own `start_of()` falls back to
   `ps -o lstart=` and returns "p:...".  On a Linux box with no readable procfs
   the two strings can never be equal, so no signal is ever sent and the child
   outlives the server - the failure this branch removes, restored for that
   environment.  The token side now falls through to `ps` when procfs is absent,
   exactly as the watcher does; with procfs present but the entry unreadable it
   still returns "" (the process is gone or is not ours).

2. The watcher's `/proc/<pid>/stat` read now uses a `with` block.  start_of()
   runs up to 42 times per child while the 10 s grace elapses, so a leaked
   handle there would be a real fd leak rather than a ResourceWarning.

3. `strata_control/procs.py::start_token`'s ps fallback kept the state column
   too ("s:" + the whole line).  That is the same defect on the path used by
   POSIX without psutil or without /proc, so the runtime had two contradictory
   definitions of process identity.  Both now use the start time alone; zombies
   are still detected from the state column before it is dropped.

4. The new test no longer depends on luck.  It read the token twice and required
   equality, which on the OLD code passes whenever the spawn sample happens to
   land on `S` - the same timing dependence as the bug.  It now reads the
   child's state independently (`ps -o stat=`) and asserts it went R -> S, so a
   runner where the child never reaches those states fails loudly instead of
   passing quietly.  It also keeps its launch record in the test's temp dir:
   popen() runs in-process, so procs.write_record() was writing under the real
   per-user state dir, where a dead child's record would be left behind.

Validation, revision before this commit, Debian 13 container, Python 3.11.15:

  serve.test_lifeline                 7/7 OK
  new test vs base 7ddc17a           FAILED (failures=1) - state assertion passed
                                      (R then S observed), token assertion failed
  full `unittest discover -s serve`   975 tests, OK (skipped=14), 323.7 s
  procs.start_token                    'l:22527756' stable across a state change
  ps fallback expression               'R' and 'S' now give the same token
@justindressler

Copy link
Copy Markdown
Contributor Author

Put the branch through a cross-model review before adding anything else. Two of the three reviewers returned; both APPROVE, and they found the same things independently.

reviewer verdict notes
GLM-5.3 APPROVE verified the /proc indices against a naive whole-line split on a live process, and reproduced the base failure by execution
DeepSeek-V4-Flash APPROVE reproduced base failure 3/3, cross-checked field 22 against btime in /proc/stat
MiniMax-M3 no verdict stalled on its API call with 23 s of CPU in 35 min and was abandoned. Not counted.

Everything below is a finding that both of the returned reviewers raised; none is single-source.

1625e97f — what changed

1. Linux without procfs could never match (the one that mattered). _start_token() returned "" on any /proc read failure, while the watcher's own start_of() falls back to ps -o lstart= and returns "p:…". On a Linux box with no readable procfs the two strings can never be equal, so no signal is ever sent — the failure this PR removes, restored for that environment. The token side now falls through to ps when procfs is absent, exactly as the watcher does; with procfs present but the entry unreadable it still returns "".

2. Unclosed handle in the watcher. start_of() runs up to 42 times per child while the 10 s grace elapses, so the leak was real rather than a ResourceWarning. Now a with block.

3. strata_control/procs.py::start_token had the same defect on its ps fallback ("s:" + the whole line, state column included) — the path used by POSIX without psutil or without /proc. Two contradictory definitions of process identity in one runtime; both now use the start time alone, with the zombie check still reading the state column before it is dropped.

4. The new test no longer depends on luck. It required the two token reads to be equal, which on the old code passes whenever the spawn sample lands on S — the same timing dependence as the bug itself. It now reads the child's state independently (ps -o stat=) and asserts R → S first, so a runner where the child never reaches those states fails loudly. It also keeps its launch record in the test's temp dir: popen() runs in-process, so procs.write_record() was writing under the real per-user state directory and leaving a dead child's record behind.

Validation at 1625e97f

Debian 13 container, Python 3.11.15, no GPU needed.

Passed: serve.test_lifeline 7/7 · full python -m unittest discover -s serve -p 'test_*.py' → 975 tests, OK (skipped=14), 323.7 s · procs.start_token returns l:22527756 and holds it across a state change · the ps fallback now yields the same token for R and S.

Failed (deliberate): the new test against base 7ddc17a3 → FAILED (failures=1), and the state assertion passed first (R then S observed), so the failure is the token, not a missed state transition.

Not run: Windows job objects, macOS, GPU/backend builds — unchanged from before, Python only.

Limit I am leaving in place: the identity rule now exists in three places (procs.start_token, lifeline._start_token, and the inline start_of() inside the -c watcher script, which cannot import a helper). The prefixed tokens (l:/p:) keep them from silently disagreeing — that was the reviewers' suggested mitigation — but a future edit to one copy is still the way this regresses.

@justindressler

Copy link
Copy Markdown
Contributor Author

Before/after, measured both at the test level and outside the test suite on real processes. The point of the second half: a failing test is not evidence that the product is broken, and a passing test is not evidence that it works.

Test level — the shipped test, five consecutive runs each

base 7ddc17a3 head 1625e97f
test_the_portable_watcher_ends_the_child_when_the_server_is_killed 5/5 FAILED, 15.2–15.3 s each 5/5 OK, 0.27–0.30 s each

The run time is itself the fault: on base the test burns its entire 15 s wait_gone grace before failing, because the child never goes away. On head it returns in 0.28 s because the child is gone in about 0.1 s.

Product level — no unittest, the shipped watcher program, real processes

Method: a server process starts a sleep(600) child through lifeline.popen(..., "engine") with STRATA_LIFELINE=watcher; the parent then SIGKILLs the server and polls the child's /proc/<pid>/stat directly (not the test's alive() helper) for 20 s. In every trial the child's state before the kill was S — asleep, genuinely running, not a zombie.

base 7ddc17a3 head 1625e97f
children ended after killing the server 0 / 5 5 / 5
children still running 20 s later 5 / 5 0 / 5
time for the child to go never 0.1 s

So on base the fault is real: kill the server and the engine child keeps running, with the watcher process already gone. The test was telling the truth.

The sampler half — is that test invalid?

This is where the original assertion deserved the criticism. Base, outside the suite, with a sample() that always raises (the machine whose GPU/psutil source cannot be read):

base 7ddc17a3 head 1625e97f
sampler thread after 2.5 s of failing samples DEAD — killed by the first exception, sample() attempted once alive, 2 attempts, one [strata] metrics: a sample failed … keeps trying line
thread gone after close() yes yes

So the fault the old test was pointing at was real — the sampler died on the first bad sample and GET /metrics stopped updating. What was invalid was the test's message, not its premise: AssertionError: 1 != 2 counted threads instead of identifying them, so it could not tell "the sampler never started" from "another test's sampler finished", and it read as a flake in the test rather than a dead monitor in the product. The rewrite keeps the same strength (exactly one new sampler identity is still required, so zero threads still fails) and names the half that failed.

CI at the head

Two runs, same commit: 40037822044 success, 40037823106 success.

kingdonb added a commit to kingdonb/Strata that referenced this pull request Oct 25, 2026
…upstream PR Niko1221#3590)

Cherry-pick and squashed adoption of upstream PR Niko1221#3590
by @justindressler resolving issue Niko1221#3407:
'[Engine] stabilise the serve test job: lifeline identity by start time, sampler that survives a bad source'

Key changes:
- serve/lifeline.py: identify child processes strictly by start time (via
  /proc/<pid>/stat starttime on Linux and ps -o lstart= on Darwin/BSD),
  dropping the transient scheduler state letter (R/S) that caused the
  watcher to erroneously misidentify children and exit without killing them.
- serve/telemetry.py: Telemetry._loop catches sample errors so a failing
  or transiently unreadable GPU/psutil telemetry source does not crash the
  sampling thread.
- serve/test_lifeline.py: added test_the_watcher_token_does_not_change_with_the_child_state.
- serve/test_responses.py: NoLeakedSampler tracks thread identities rather
  than raw thread counts, avoiding race assertions in concurrent test suites.
- strata_control/procs.py: strip trailing state token.

Validated on Darwin arm64: serve.test_lifeline passes 7/7 in 4.7s;
serve.test_responses passes 41/41 in 13.2s.
kingdonb added a commit to kingdonb/Strata that referenced this pull request Oct 25, 2026
…upstream PR Niko1221#3590)

Cherry-pick and squashed adoption of upstream PR Niko1221#3590
by @justindressler resolving issue Niko1221#3407:
'[Engine] stabilise the serve test job: lifeline identity by start time, sampler that survives a bad source'

Key changes:
- serve/lifeline.py: identify child processes strictly by start time (via
  /proc/<pid>/stat starttime on Linux and ps -o lstart= on Darwin/BSD),
  dropping the transient scheduler state letter (R/S) that caused the
  watcher to erroneously misidentify children and exit without killing them.
  Also guard _watch(child) against mock Popen test doubles lacking pid.
- serve/telemetry.py: Telemetry._loop catches sample errors so a failing
  or transiently unreadable GPU/psutil telemetry source does not crash the
  sampling thread.
- serve/test_lifeline.py: added test_the_watcher_token_does_not_change_with_the_child_state.
- serve/test_responses.py: NoLeakedSampler tracks thread identities rather
  than raw thread counts, avoiding race assertions in concurrent test suites.
- strata_control/procs.py: strip trailing state token.

Validated on Darwin arm64: serve.test_lifeline passes 7/7 in 4.7s;
serve.test_responses passes 41/41 in 13.2s; serve.test_server passes 100%.
@kingdonb

kingdonb commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor
  • Validated on macOS (Darwin arm64): before this PR, 3 out of 6 tests in serve.test_lifeline failed or timed out due to the watcher state token flip (ps -o lstart=,stat=). With PR [Engine] stabilise the serve test job: lifeline identity by start time, sampler that survives a bad source #3590, all 7/7 tests pass cleanly in 4.7s.
  • Bonus Darwin finding: In serve/lifeline.py, _watch(child) is executed on macOS where how == "watcher". Unit tests that mock subprocess.Popen (such as VisionArgs in test_server.py) return mock proc objects lacking .pid. Adding if not isinstance(child, _REAL_POPEN) or getattr(child, "pid", None) is None: return at the top of _watch() avoids an AttributeError on non-Linux hosts.

(Wait a second, back up, there's still a flake... about to push a new change to my fork that I'd propose you include in this PR, if it looks like the flake is solved in https://github.com/kingdonb/Strata/actions/runs/40216254644/job/121237909467)

kingdonb added a commit to kingdonb/Strata that referenced this pull request Oct 25, 2026
…test isolation (cherry-pick upstream PR Niko1221#3590)

Cherry-pick and refined adoption of upstream PR Niko1221#3590
by @justindressler resolving issue Niko1221#3407:
'[Engine] stabilise the serve test job: lifeline identity by start time, sampler that survives a bad source'

Key refinements over PR Niko1221#3590:
- serve/lifeline.py:
  * identify child processes strictly by start time (via /proc/<pid>/stat
    starttime on Linux and ps -o lstart= on Darwin/BSD), dropping the transient
    scheduler state letter (R/S) that caused the watcher to erroneously
    misidentify children and exit without killing them.
  * guard _watch(child) against mock Popen test doubles that lack a pid
    (fixing AttributeError in VisionArgs on Darwin).
- serve/telemetry.py:
  * Telemetry._loop catches sample errors so a failing or transiently unreadable
    telemetry source does not crash the sampling thread.
  * Telemetry retains a reference to its thread (self._thread).
- serve/test_responses.py:
  * NoLeakedSampler directly asserts on Telemetry._thread lifecycle and joins it
    on server_close, avoiding thread ident recycling race collisions in full-suite CI.
- serve/test_lifeline.py: added test_the_watcher_token_does_not_change_with_the_child_state.
- strata_control/procs.py: strip trailing state token.

Validated:
- Linux CI ('Minimal serving runtime'): 1,036/1,036 tests pass in 355s (0 failures).
- Darwin arm64: 7/7 lifeline tests pass in 4.7s; 40/40 responses tests pass.
@kingdonb

kingdonb commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Tested this branch against the full 1,036-test suite in CI and on Apple Silicon macOS, and found two small edge cases worth folding in:

  1. Thread ID recycling flake in NoLeakedSampler: In full-suite runs, samplers() - before occasionally flaked with 0 != 1 because the OS recycled a dying thread's ident for the new sampler. Retaining self._thread on Telemetry and asserting/joining sampler._thread directly makes the test completely immune to lingering background threads and thread-ID reuse.
  2. Darwin mock guard in _watch(): On macOS (where how == "watcher" rather than pdeathsig), unit tests using duck-typed Popen doubles without .pid (e.g. VisionArgs) raised AttributeError. Adding if not isinstance(child, _REAL_POPEN) or getattr(child, "pid", None) is None: return keeps it clean across non-Linux platforms.

Full suite is 100% green with both included (reference commit a32edec on kingdonb/Strata@fix/lifeline-watcher-token-race).

@i-Vladimir-i

Copy link
Copy Markdown
Contributor

@justindressler @kingdonb @Niko1221 Fresh independent dev results, including a current-main control and both edge cases raised above. Runs completed on October 25 at 20:57 UTC.

Tested revisions:

  • unchanged main 4b9e05a2a806a3fe24f7b2560d98626c37dade35;
  • this PR unchanged at 1625e97faf1be0e7ad6dfa662c684c0c1b849165;
  • the same main plus this PR's exact five-file diff from its base 7ddc17a3 (applies cleanly, no additional fixes).

Linux x86_64, kernel 6.8.0-147, glibc 2.39, Python 3.12.3, existing dev runtime venv; isolated worktrees/process-state directories, GPU visibility disabled, no dependency installation. Node v24.21.0 was on the test processes' PATH.

Passed

Check Unchanged main PR head Current main + PR
Full server discovery, no exclusions 1035 total / 1020 passed / 15 skipped, exit 0 975 / 961 / 14, exit 0 1036 / 1021 / 15, exit 0
serve.test_lifeline 6/6, exit 0 7/7, exit 0 7/7, exit 0
New R-to-S identity regression, three separate runs Expected FAIL 3/3, exit 1 PASS 3/3, exit 0 PASS 3/3, exit 0
Four focused identity/fallback checks Expected 3 assertion failures, no errors 4/4, exit 0 4/4, exit 0
Terminal runtime discovery, with normal SIGINT/SIGQUIT dispositions 48 total / 44 passed / 4 skipped, exit 0 47 / 43 / 4, exit 0 48 / 44 / 4, exit 0

Commands: python -m unittest discover -s serve -p 'test_*.py' -v, python -m unittest serve.test_lifeline -v, and python -m unittest discover -s tests/terminal -p test_runtime.py -v.

For the negative control, importlib loaded this PR's exact new test module while serve.lifeline came from unchanged main, with an assertion checking its file path. All three failures were the token changing after an independently observed R-to-S transition, not setup errors or a missed transition. Main's old full suite passing does not contradict that control. The fallback checks cover absent procfs, a missing child entry with procfs present, the POSIX procs.start_token ps fallback across R/S, and zombie rejection; these are controlled branch tests, not native macOS execution.

Both reported edge cases independently confirmed

1. Numeric thread-ID reuse in NoLeakedSampler: reproduced 3/3 on each arm using two real Telemetry instances and a controlled interleaving: the previous sampler is in the test's initial snapshot, then exits just before the new sampler starts. The OS actually reused its numeric ident; the retained Thread objects were different. The unchanged shipped test falsely reports 0 != 1: the server did not start its hardware sampler on PR head and main+PR; old main's count-based test falsely reports 1 != 2. An independent object-identity check confirms the new thread is alive after start and stopped after server close/join. This supports retaining and checking the owned Thread object. It demonstrates the possibility, not a measured natural flake rate.

2. Portable watcher with a Popen double lacking pid: on Linux, forcing the portable MODE=watcher path and running the shipped VisionArgs reproduces AttributeError: 'Proc' object has no attribute 'pid' on all three arms. An in-memory version of the suggested guard makes that test pass; a real synthetic Popen child still gets a watcher and can be released. We did not run on real macOS. This checks the portable branch used there, not Darwin compatibility.

Both cases also occur on unchanged main; neither is established as a newly introduced regression. We did not apply or validate kingdonb's full a32edec patch, and the tested PR sources remain unchanged.

Separate baseline failure — not an all-green claim

python -m unittest discover -s tests/terminal -p test_procs.py -v gives 6 passed / 1 failed out of 7, exit 1 on all three arms. The same failure is Procs.test_an_engine_orphan_of_this_executable_is_found_and_a_live_one_is_not, at its final assertIsNone(procs.start_token(orphan_pid)). It is present on unchanged main; we have not diagnosed its cause or attributed it to this PR. No assertion was weakened or test excluded.

Harness limits recorded rather than hidden:

  • Initial runtime runs inherited ignored SIGINT/SIGQUIT from the background launcher and timed out on the SIGINT subtest on all arms. Normalizing only our test runner's dispositions made the unchanged full runtime sets pass as shown above.
  • The standalone minimal_server_smoke.py entry failed setup because an existing llama.cpp/gguf-py tests package shadows tests.terminal in this venv. Discovery ran the intended runtime file successfully; the smoke script itself is not claimed green. Earlier harness setup failures were retained, not counted as product regressions.

Server skips: Anthropic SDK 3, pack tokenizer 5, jsonschema 3, Windows 3; current main/main+PR additionally skip one DeepSeek tokenizer case. Runtime skips are four Windows-specific checks.

Not run: real macOS/Darwin, Windows job objects, a fresh Python 3.11 runtime-only CI venv, GPU/backend builds or inference. These are dev results, not a GitHub CI rerun; no production changes, installs, or source pushes.

Both independently reproduced by @i-Vladimir-i on Oct 25 (3/3 each, on main
and on this PR alike - pre-existing flakes, not regressions of this branch):

- serve/lifeline.py: guard _watch() against Popen test doubles without .pid
  (AttributeError in test_server.VisionArgs on the Darwin watcher path).
- serve/telemetry.py + serve/test_responses.py: retain Telemetry._thread and
  have NoLeakedSampler assert on the owned Thread object. The OS can recycle
  a dying thread's numeric ident for the new sampler, which flaked
  'samplers() - before' as 0 != 1 in full-suite runs; object identity is
  immune to thread-ID reuse and to other tests' lingering samplers.

Diff is byte-identical to kingdonb's a32edec hunks on these files,
cherry-picked by hand because his branch is based on a later main.
@justindressler

Copy link
Copy Markdown
Contributor Author

@kingdonb's two fixes from his fork are folded in at 0010a908, credited in the commit message. The diff is byte-identical to the a32edec8 hunks on these three files (hand-applied, because his branch is based on a later main than this PR):

  • serve/lifeline.py — _watch() early-returns for Popen test doubles without .pid (the VisionArgs AttributeError on the Darwin watcher path).
  • serve/telemetry.py + serve/test_responses.py — Telemetry retains self._thread, and NoLeakedSampler asserts on the owned Thread object instead of samplers() - before. This is the thread-ID-recycling flake @i-Vladimir-i reproduced 3/3: the OS reused a dying sampler's numeric ident for the new one, so the set difference came back empty (0 != 1). Object identity is immune to that and to other tests' lingering samplers — and it keeps the same strength: a server that never starts its sampler still fails, and a stopped server whose sampler survives still fails.

Validation at 0010a908 (the repo's own pinned requirements.txt venv, Linux/WSL2, Python 3.14.9, CPU only — no GPU needed for these paths):

Passed:

  • serve.test_lifeline — 7/7 OK, including test_the_watcher_token_does_not_change_with_the_child_state
  • serve.test_responses.NoLeakedSampler — OK
  • serve.test_server — 352/352 OK (includes the VisionArgs mock-double case behind the new guard)
  • The CI job's own full command python -m unittest discover -s serve -p 'test_*.py' — 975 tests, OK (skipped=14), 321.6 s — same total and skip count as the previous head 1625e97f, so nothing was added or silently dropped.

Not run: real macOS/Darwin and Windows job objects (the two paths the guard protects are exercised via the mock double and the forced-watcher reproduction @i-Vladimir-i already ran 3/3); GPU/backend builds — unchanged from before, this PR is Python only.

Also noting for the record @i-Vladimir-i's separate baseline finding on current main — tests/terminal/test_procs.py::test_an_engine_orphan_of_this_executable_is_found_and_a_live_one_is_not fails on all arms including unchanged main. That is unrelated to this PR's five files and deserves its own issue rather than a rider here.

Niko1221 pushed a commit that referenced this pull request Oct 27, 2026
…urvives a bad source (#3590, #3407)

The portable lifeline watcher built a child's identity from `ps -o lstart=,stat=` and kept the whole line, so the token
changed with the process state (R while it starts, S asleep). The watcher compared its own reading with the token taken
at start, decided it was not the child it was told about and exited without ending it (test_lifeline "the child outlived
a killed server (watcher)", #3407). The identity is now the start time alone: field 22 of /proc/<pid>/stat on Linux
(a zombie has none), `ps -o lstart=` elsewhere; strata_control/procs.start_token drops the state column the same way.
A launch that is not a real Popen is not watched.

Telemetry._loop no longer ends on the first failing sample (a machine whose GPU or psutil source is unreadable): it
prints one line and keeps sampling, and NoLeakedSampler checks the thread itself instead of counting threads.

New test: the token is the same while the child runs (R) and sleeps (S); on the old code it fails on Linux
('...R' != '...S').

Ported from PR #3590.
@Niko1221

Copy link
Copy Markdown
Owner

Thank you. Fixed in 0.2.7: The lifeline identity is the child's start time, the sampler survives a bad source (#3590, #3407). Your name is on the commit.

Release: https://github.com/Niko1221/Strata/releases/tag/v0.2.7

@Niko1221 Niko1221 closed this Oct 27, 2026
MistVVK added a commit to MistVVK/XeStrata that referenced this pull request Oct 30, 2026
GPU や psutil の値が読めない機械では、最初の例外で測りのスレッドが終わり、GET /metrics が黙って
更新されなくなっていた。1 行だけ伝えて測り続ける。スレッドは _thread に持つ。lifeline の部分
(serve/lifeline.py と strata_control)は XeStrata に無いので移さない。upstream Niko1221#3590。
ruc1j added a commit to ruc1j/XeStrata-890M that referenced this pull request Oct 31, 2026
…の 1)

serve の変更
- 宣言していない名前の tool call を文のまま返すとき、その名前を strata_rejected_tool_calls で答えに添え、/metrics/prometheus で数える(upstream 805ebe1、Niko1221#3796)
- 名前空間つきの tool は namespace=member、namespace_member、member だけの綴りでも受け、`__` を `_` に潰した綴りも宣言した tool として読む(upstream fd8d37e、805ebe17b、Niko1221#3972)
- Chat Completions の tool の結果を、答えた call の順に並べ直す(upstream 11237f8、Niko1221#3395)
- message のない chat の要求は 400 にする(upstream 8a3ca07)
- --host :: で IPv6 で待ち受ける(upstream eba3106、Niko1221#3212)
- /v1/responses の max_output_tokens は 0 と -1 を上限なしとして受け、答えに timings を付ける(upstream 9c2d35d、a0fc88abd、Niko1221#4036、Niko1221#3291)
- response_format の repair_latex_escapes で、LaTeX のエスケープで壊れた JSON を直してから検査する(upstream 2cc6ea1、Niko1221#2893)
- 1 トークンの答えには decode の速さを出さない(upstream 5d5c26d)
- elastic な画像エンコーダーが起動できなければ 503 を返す(upstream 092c7d1、Niko1221#2435)
- lifeline の識別は子プロセスの開始時刻にし、sampler は壊れた source に耐える(upstream b3db5a1、Niko1221#3590、Niko1221#3407)
- web ページ: 答えを追うのは読み手が離れていないあいだだけ(upstream 89a68d8)、広い窓を使い狭い画面に収める(upstream ad1a310、Niko1221#3176)

setup の変更
- cmake と ninja は venv のものを先に取り、CMakeLists.txt が求める 3.24 より古い cmake は使わない(upstream 5bfce4b、Niko1221#3822)
- 別の generator で configure された build フォルダーは作り直す(upstream bf994ca、Niko1221#4038)
- ダウンロードの長さが取れなければ公開されている大きさを使う(upstream a1d2fd7、Niko1221#2287)
- 2 分データが来ないダウンロードは止まっていると言う(upstream ed0c893、Niko1221#3567)
- --rope-scale は有限で 1 以上の値だけ受ける(upstream a332cc9、Niko1221#3393)
- pip の記録はどの OS が書いたかを覚え、別の OS の記録は信じない(upstream fb3bae2、Niko1221#3634)

tools の変更
- gguf_reader.py はヘッダーの読みに上限を置き、壊れた metadata を ValueError にする(upstream 409f246、Niko1221#2288、Niko1221#1919)
- strata_bench.py は amd-smi しかない ROCm 7 以降でも AMD のカードの名前を書く(upstream c471588、Niko1221#3630)
- calibrate.py は計測の途中でエンジンが落ちても、その候補だけを落として続ける(upstream 41188ec、Niko1221#3317)
- chat.py は端末で fenced code block に色を付ける(upstream 3e81173、PR Niko1221#1895)
- 試験: llama.cpp の gguf-py がなければ test_iq_pack は skip、ほか試験の穴 3 つ(upstream 5b14fe2、Niko1221#3236)

fork に合わせて変えたこと: upstream の docs/DETAILS.md の差分は取らず、文書は別に書く。logprobs(upstream 9ea0449)は fork にすでにあり、allowed_token_ids は見送り。Control(strata_control)に依る setup の変更と、fork にない機能(presets の警告、Delete a model、queue timeout の badge)は取らない。
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.

4 participants