fix(tests): remove four shared-state and lifetime faults at high concurrency

The suite now runs as one job with high per-file concurrency. Four tests
depend on state that they share with their siblings, or on a timer that
outlives them. That was safe at 8 workers. It is not safe at 96 or more.
Runs 32547184159 and 32551746525 show them.

1. Every pytest subprocess shared one temp root.

pytest puts tmp_path under <temproot>/pytest-of-<user>/. At the end of a
session it walks that directory with cleanup_dead_symlinks(). The walk lists
the directory. Then it asks whether the `pytest-current` symlink resolves.
Then it unlinks the symlink. A second process replaces that symlink between
the question and the unlink. The first process then raises FileNotFoundError
after all of its tests passed. Two files failed this way and passed on retry.

scripts/run_tests_parallel.py now gives each subprocess its own temp root
through PYTEST_DEBUG_TEMPROOT, and deletes it after the attempt. No two
processes share a directory. The race has no shared object to act on.

Proof: a direct driver of _pytest.pathlib.cleanup_dead_symlinks against one
root, with a second thread that replaces the symlink, raises the same
FileNotFoundError on 'pytest-current' as CI. A private root for each
subprocess removes that condition. A separate check confirms that 5
subprocesses receive 5 distinct roots, that tmp_path lands inside the private
root, and that no root survives the attempt.

2. The config read guard walked directories that other tests were writing.

tests/hermes_cli/test_config_read_guard.py scanned the tree with rglob. rglob
descends into every directory and filters after that, so it calls scandir() on
__pycache__ trees that the guard never inspects. Sibling processes create and
delete those entries during the run. A directory that disappears in the middle
of a walk raises FileNotFoundError out of rglob.

The scan now uses os.walk. It prunes excluded directories before it descends,
and it ignores a directory that disappears. __pycache__ joins the excluded
set, because bytecode is not source.

The guard still catches what it exists to catch. With a planted raw
yaml.safe_load of config.yaml in hermes_cli/, the test fails and names the
planted file. With a clean tree it passes.

3. A PTY test waited for a file to exist, and not for its content.

tests/tools/test_process_registry_write_stdin_surrogates.py spawns a child
that runs open(out,'wb').write(sys.stdin.buffer.readline()). open() creates
the file empty. The bytes arrive only after the PTY delivers the line. The
wait stopped at out.exists(), which the empty file already satisfies, so the
read returned b'' when the parent won that gap. This test failed both attempts
in CI, and did not pass on retry.

The test now waits for the expected bytes, with a bounded deadline.

Proof: the old wait loses 6 times in 25 runs on an idle 16-core machine. The
new wait loses 0 times in 25.

4. A dialog close timer outlived the test that started it.

ConfirmDialog holds the "done" beat for 600ms after a successful confirm, then
calls onClose. The timer had no cleanup, so an unmount inside that window left
it armed. It then called onClose on a tree that is gone, which reaches
setState in the parent. vitest can tear the environment down first, and React
then reads `window` during the update:

    ReferenceError: window is not defined
     at resolveUpdatePriority (react-dom-client.development.js:1308)
     at dispatchSetState
     at Timeout.t4 [as _onTimeout] session-actions-menu.tsx:574

The frame at session-actions-menu.tsx:574 is the `onClose` prop of
DeleteSessionDialog. The owner of the timer is ConfirmDialog, which now keeps
the handle in a ref and clears it on unmount.

Zoomable had the same fault, with a 1500ms timer that clears a "copied" flag.
copy-button.tsx and tooltip.tsx already clear their timers.

Proof: a new test confirms, unmounts inside the 600ms window, then advances
the clock. Against the old code it fails with "expected onClose to not be
called at all, but actually been called 1 times". Against the new code it
passes.

Verification:
- The affected Python files and the tests of the runner itself pass under
  scripts/run_tests.sh.
- The desktop ui suite passes: 566 files, 5382 tests, and no
  "window is not defined".
- eslint reports 0 errors on apps/desktop. The 118 warnings are the state
  before this change. The two cleanup effects carry an eslint-disable line for
  the ref-mirror rule. They write a timer handle, and not a mirror of a
  reactive value. The rule permits this, and its own comment names the case.
- The PTY test cannot run on the NixOS development machine. That machine has
  no python3 outside the nix store, and the test uses the literal `python3`.
  The child exits 127 there. The fix rests on the 25-run measurement above and
  on CI.
This commit is contained in:
ethernet
2026-08-22 01:14:14 -04:00
parent 10f99bc15e
commit 969094e4d2
6 changed files with 185 additions and 13 deletions

View File

@@ -0,0 +1,65 @@
import { cleanup, fireEvent, render, screen } from '@testing-library/react'
import { afterEach, expect, test, vi } from 'vitest'
import { ConfirmDialog } from '@/components/ui/confirm-dialog'
afterEach(cleanup)
vi.mock('@/i18n', () => ({
useI18n: () => ({
t: {
common: { cancel: 'Cancel', confirm: 'Confirm', delete: 'Delete', done: 'Done', loading: 'Working' },
errors: { genericFailure: 'Something failed' }
}
})
}))
// ConfirmDialog schedules window.setTimeout(onClose, 600) after a successful
// confirm. The timer had no cleanup, so an unmount inside that window left it
// pending. In CI it came due after the environment was gone. The setState
// path of React then touched `window`:
//
// ReferenceError: window is not defined
// at resolveUpdatePriority (react-dom-client.development.js:1308)
// at dispatchSetState
// at Timeout.t4 [as _onTimeout] session-actions-menu.tsx:574
//
// The frame at session-actions-menu.tsx:574 is the `onClose` prop of
// DeleteSessionDialog. The owner of the timer is this component.
//
// This test confirms, unmounts inside the 600ms window, and then lets the
// timer come due on the dead tree.
test('the close timer does not fire after unmount', async () => {
vi.useFakeTimers()
const onClose = vi.fn()
const onConfirm = vi.fn()
render(
<ConfirmDialog
confirmLabel="Delete"
onClose={onClose}
onConfirm={onConfirm}
open
title="Delete session"
/>
)
fireEvent.click(screen.getByRole('button', { name: 'Delete' }))
// Not waitFor: it polls on real timers, and the fake timers of this test
// never let it advance. onConfirm runs synchronously inside the click, and
// one microtask turn is enough for the await in run() to settle and reach
// the setTimeout.
await Promise.resolve()
await Promise.resolve()
expect(onConfirm).toHaveBeenCalled()
// Unmount while the close timer is still pending.
cleanup()
// Let the timer come due on the unmounted tree.
vi.advanceTimersByTime(1000)
expect(onClose).not.toHaveBeenCalled()
vi.useRealTimers()
})

View File

@@ -58,6 +58,7 @@ export function ConfirmDialog({
}: ConfirmDialogProps) {
const { t } = useI18n()
const confirmRef = useRef<HTMLButtonElement>(null)
const closeTimerRef = useRef<null | number>(null)
const [status, setStatus] = useState<'done' | 'idle' | 'saving'>('idle')
const [error, setError] = useState<null | string>(null)
const busy = status === 'saving' || status === 'done'
@@ -73,6 +74,24 @@ export function ConfirmDialog({
}
}, [open])
// Cancel the pending close timer on unmount. The timer below holds the
// "done" beat visible for 600ms, and an unmount inside that window used to
// leave it armed. It then called onClose on a tree that is gone, which
// reaches setState in the parent. Under vitest the environment can be torn
// down first, and React then reads `window` during the update and throws
// ReferenceError.
// The write below is a timer handle, and not a mirror of a reactive value.
// It happens on unmount only, and it clears the handle this component owns.
// eslint-disable-next-line no-restricted-syntax
useEffect(() => {
return () => {
if (closeTimerRef.current !== null) {
window.clearTimeout(closeTimerRef.current)
closeTimerRef.current = null
}
}
}, [])
async function run() {
if (busy) {
return
@@ -96,7 +115,10 @@ export function ConfirmDialog({
try {
await onConfirm()
setStatus('done')
window.setTimeout(onClose, 600)
closeTimerRef.current = window.setTimeout(() => {
closeTimerRef.current = null
onClose()
}, 600)
} catch (err) {
setStatus('idle')
setError(err instanceof Error ? err.message : t.errors.genericFailure)

View File

@@ -1,6 +1,6 @@
'use client'
import { type ReactNode, useEffect, useState } from 'react'
import { type ReactNode, useEffect, useRef, useState } from 'react'
import { Dialog, DialogContent } from '@/components/ui/dialog'
import { Tip } from '@/components/ui/tooltip'
@@ -117,6 +117,22 @@ function Toolbar({
zoomOut: () => void
}) {
const [copied, setCopied] = useState(false)
const resetRef = useRef<null | number>(null)
// Same reason as the close timer of ConfirmDialog. An unmount inside the
// 1500ms window used to leave this armed. The callback then called setState
// on a tree that is gone.
// The write below is a timer handle, and not a mirror of a reactive value.
// It happens on unmount only, and it clears the handle this component owns.
// eslint-disable-next-line no-restricted-syntax
useEffect(() => {
return () => {
if (resetRef.current !== null) {
window.clearTimeout(resetRef.current)
resetRef.current = null
}
}
}, [])
const copy = async () => {
if (!onCopy) {
@@ -125,7 +141,13 @@ function Toolbar({
await onCopy()
setCopied(true)
window.setTimeout(() => setCopied(false), 1500)
if (resetRef.current !== null) {
window.clearTimeout(resetRef.current)
}
resetRef.current = window.setTimeout(() => {
resetRef.current = null
setCopied(false)
}, 1500)
}
return (

View File

@@ -45,8 +45,10 @@ import argparse
import json
import os
import re
import shutil
import subprocess
import sys
import tempfile
import threading
import time
from concurrent.futures import ThreadPoolExecutor, Future
@@ -379,7 +381,27 @@ def _run_one_file_once(
) -> Tuple[Path, int, str, dict[str, int], float]:
"""Single attempt of a per-file pytest subprocess (see _run_one_file)."""
cmd = [sys.executable, "-m", "pytest", str(file), *pytest_args]
# Give this subprocess its own pytest temp root.
#
# pytest builds its tmp_path root as <temproot>/pytest-of-<user>/. At the
# end of a session it walks that directory with cleanup_dead_symlinks().
# The walk lists the directory. Then it asks whether the `pytest-current`
# symlink resolves. Then it unlinks the symlink.
#
# Every file shared one root. A second process replaced that symlink
# between the question and the unlink. The first process then died with
# FileNotFoundError after all of its tests passed.
#
# The risk grows with the number of processes that finish together. At 8
# workers it never occurred. At 144 workers it occurs.
#
# One root for each subprocess removes the shared directory that the race
# needs. The parent deletes the root after the attempt.
env = os.environ.copy()
temproot = tempfile.mkdtemp(prefix="hermes-pytest-tmproot-")
env["PYTEST_DEBUG_TEMPROOT"] = temproot
subproc_start = time.monotonic()
# launch the pytest process
proc = subprocess.Popen(
@@ -388,7 +410,7 @@ def _run_one_file_once(
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
text=True, encoding="utf-8", errors="replace",
env=os.environ,
env=env,
# POSIX: place the child at the head of its own process group so
# _kill_tree can SIGKILL the group atomically.
# Windows: this maps to CREATE_NEW_PROCESS_GROUP in CPython 3.12+;
@@ -432,6 +454,11 @@ def _run_one_file_once(
_kill_tree(proc, pgid=pgid)
output += "\n"
finally:
# Delete the temp root for this attempt. Nothing reads it after the
# subprocess exits. More than 3000 of them fill the disk of the
# runner over one suite.
shutil.rmtree(temproot, ignore_errors=True)
if rc == 5:
# No tests collected in THIS file — legitimate per-file: a

View File

@@ -24,6 +24,7 @@ file to the allowlist without a reason of the same class.
from __future__ import annotations
import os
import re
from pathlib import Path
@@ -49,6 +50,9 @@ ALLOWLIST = {
EXCLUDED_DIR_PARTS = {
"tests", ".venv", ".git", ".worktrees", "node_modules", "website",
"docs", "scripts", "examples", "apps",
# Compiled bytecode is not source. Sibling test processes also create
# and delete these directories while this scan walks the tree.
"__pycache__",
}
# A safe_load within this many lines of a config.yaml reference is treated
@@ -60,11 +64,27 @@ CONFIG_YAML_RE = re.compile(r"""["']config\.yaml["']""")
def _iter_source_files():
for path in REPO_ROOT.rglob("*.py"):
rel = path.relative_to(REPO_ROOT)
if any(part in EXCLUDED_DIR_PARTS for part in rel.parts):
continue
yield rel, path
# This uses os.walk with a pruned dirnames, and not rglob. rglob descends
# into every directory and filters after that, so it calls scandir() on
# __pycache__ trees that this guard never inspects. Sibling test processes
# create and delete those entries during the run.
#
# A directory that disappears in the middle of a walk raises
# FileNotFoundError out of rglob. The test then fails for a reason that it
# does not assert.
#
# The prune skips those trees. The onerror callback ignores a directory
# that disappears anyway.
for dirpath, dirnames, filenames in os.walk(REPO_ROOT, onerror=lambda _e: None):
dirnames[:] = [d for d in dirnames if d not in EXCLUDED_DIR_PARTS]
for name in filenames:
if not name.endswith(".py"):
continue
path = Path(dirpath) / name
rel = path.relative_to(REPO_ROOT)
if any(part in EXCLUDED_DIR_PARTS for part in rel.parts):
continue
yield rel, path
def test_no_raw_config_yaml_reads_outside_owner_modules():

View File

@@ -30,9 +30,25 @@ def test_write_stdin_pty_surrogateescape_roundtrip(tmp_path):
session.id, b"\xff".decode("utf-8", "surrogateescape") + "\n"
)
assert result["status"] == "ok", result
deadline = time.monotonic() + 10
while time.monotonic() < deadline and not out.exists():
# Wait for the CONTENT, and not for the file to exist. The child runs
# open(out,'wb').write(...). open() creates the file empty, and the
# bytes arrive only after the PTY delivers the line. The previous wait
# stopped at out.exists(), which the empty file already satisfies, so
# the read returned b'' when the parent won that gap.
#
# On a 144-worker runner the gap is wide enough to lose every time.
# This test failed both attempts in CI, and not one time only. It also
# loses 6 times in 25 runs on an idle 16-core machine.
deadline = time.monotonic() + 30
got = b""
while time.monotonic() < deadline:
try:
got = out.read_bytes()
except FileNotFoundError:
got = b""
if got == b"\xff\n":
break
time.sleep(0.05)
assert out.read_bytes() == b"\xff\n"
assert got == b"\xff\n"
finally:
registry.kill_process(session.id)