Skip to content

test(daemon): de-flake sidecar stop test on the pid-liveness check#2349

Merged
kovtcharov merged 1 commit into
mainfrom
fix/flaky-daemon-relay-stop
Jul 21, 2026
Merged

test(daemon): de-flake sidecar stop test on the pid-liveness check#2349
kovtcharov merged 1 commit into
mainfrom
fix/flaky-daemon-relay-stop

Conversation

@kovtcharov-amd

Copy link
Copy Markdown
Collaborator

Why this matters

test_connection_stopped_after_running_raises_again fails intermittently on CI (seen on the macOS smoke runner: StopFailedError: agent 'toy' sidecar pid 4321 survived the tree-kill). It's a false failure that blocks unrelated PRs and erodes trust in the suite — the production code is fine.

Root cause: the test's fake manager reports a hardcoded pid (4321) but owns no real OS process, so its shutdown() can't make that pid disappear. registry.stop() then verifies the pid is gone via psutil.pid_exists(pid) against the real process table. On most runners pid 4321 is dead → passes; when it collides with a live process on the runner → spurious StopFailedError. Test-double fidelity bug, not a product bug: a real manager tree-kills its own real pid, so the liveness check stays meaningful in production.

Fix: patch pid_exists to False for the fake so the check reflects the double's intent (the fake sidecar is terminated) instead of the host's process table. One line + a comment; production behavior untouched.

Test plan

  • pytest tests/unit/test_daemon_relay.py — 36 pass
  • Verified the mechanism: forcing pid_exists → True (simulating pid 4321 alive on the runner) reproduces the StopFailedError; the fix's pid_exists → False patch neutralizes it while stop() still succeeds
  • black/isort clean

test_connection_stopped_after_running_raises_again used a fake manager
that reports a hardcoded pid (4321) but owns no real OS process, so its
shutdown() can't make that pid disappear. registry.stop() then verifies
the pid is gone via psutil.pid_exists against the real process table —
which spuriously raises StopFailedError whenever pid 4321 happens to be a
live process on the runner (observed on macOS CI).

Patch pid_exists to False for the fake so the liveness check reflects the
double's intent (the fake sidecar is terminated) rather than the host's
process table. Production behavior is unchanged — a real manager tree-kills
its real pid, so the check stays meaningful there.
@github-actions github-actions Bot added the tests Test changes label Jul 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Approve

This de-flakes test_connection_stopped_after_running_raises_again by making the fake sidecar's shutdown believable to the liveness check. The fake reports a hardcoded pid but owns no real OS process, so stop()'s psutil.pid_exists check against the real process table would spuriously fail whenever that pid happened to be live on the runner. Patching pid_exists → False for the fake models the double's intent (the sidecar is terminated) without touching product code. Root cause and mechanism are correctly diagnosed and the fix is minimal.

🔍 Technical details

Verified

  • Patch path is correct: registry.py:16 imports psutil at module scope, and registry.stop() calls psutil.pid_exists(pid) (src/gaia/daemon/sidecars/registry.py:299), so monkeypatch.setattr("gaia.daemon.sidecars.registry.psutil.pid_exists", ...) resolves to the exact symbol under test.
  • Root cause matches the code: the fake manager hardcodes self.pid = 4321 (test_daemon_relay.py:224) and owns no real process, so its shutdown() can't clear that pid from the real process table — the source of the intermittent StopFailedError.
  • Scope is clean: only the one flaky test changes; stop()'s liveness check is untouched, so it stays meaningful in production where a real manager tree-kills its own real pid.

No concern, noting for the record

  • Forcing pid_exists → False doesn't erase coverage of the StopFailedError path — that path is exercised elsewhere (tests/integration/test_daemon_sidecars.py), and this test's assertion is that connection() raises SidecarNotRunningError after stop(), which is unrelated to the liveness branch.

Strengths

  • The inline comment names the invariant (double's intent vs. host process table) in the right length — one tight block, no history dump.
  • Fixes the flake at the test-double boundary rather than weakening the production assertion, which is the correct call.

@kovtcharov
kovtcharov added this pull request to the merge queue Jul 21, 2026
Merged via the queue into main with commit 19b1a4c Jul 21, 2026
30 checks passed
@kovtcharov
kovtcharov deleted the fix/flaky-daemon-relay-stop branch July 21, 2026 17:41
@itomek itomek mentioned this pull request Jul 22, 2026
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants