Pull requests / #3590

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

open · @justindressler · 0 comentários · No GitHub

Server & APIAMD / HIPNVIDIA / CUDAWindowsLinux

Descrição

## 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

- #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.

No site

Links install, modelos, releases.