diff --git a/apps/desktop/src/components/ui/confirm-dialog-unmount.test.tsx b/apps/desktop/src/components/ui/confirm-dialog-unmount.test.tsx new file mode 100644 index 0000000000..e797b35abe --- /dev/null +++ b/apps/desktop/src/components/ui/confirm-dialog-unmount.test.tsx @@ -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( + + ) + + 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() +}) diff --git a/apps/desktop/src/components/ui/confirm-dialog.tsx b/apps/desktop/src/components/ui/confirm-dialog.tsx index 9e30011b8e..3792ff41ce 100644 --- a/apps/desktop/src/components/ui/confirm-dialog.tsx +++ b/apps/desktop/src/components/ui/confirm-dialog.tsx @@ -58,6 +58,7 @@ export function ConfirmDialog({ }: ConfirmDialogProps) { const { t } = useI18n() const confirmRef = useRef(null) + const closeTimerRef = useRef(null) const [status, setStatus] = useState<'done' | 'idle' | 'saving'>('idle') const [error, setError] = useState(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) diff --git a/apps/desktop/src/components/ui/zoomable.tsx b/apps/desktop/src/components/ui/zoomable.tsx index 7741022b91..46aed953c0 100644 --- a/apps/desktop/src/components/ui/zoomable.tsx +++ b/apps/desktop/src/components/ui/zoomable.tsx @@ -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) + + // 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 ( diff --git a/scripts/run_tests_parallel.py b/scripts/run_tests_parallel.py index 96bec3c56a..eb923cc9b4 100755 --- a/scripts/run_tests_parallel.py +++ b/scripts/run_tests_parallel.py @@ -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 /pytest-of-/. 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 diff --git a/tests/hermes_cli/test_config_read_guard.py b/tests/hermes_cli/test_config_read_guard.py index 02e2414650..d54ea08e4e 100644 --- a/tests/hermes_cli/test_config_read_guard.py +++ b/tests/hermes_cli/test_config_read_guard.py @@ -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(): diff --git a/tests/tools/test_process_registry_write_stdin_surrogates.py b/tests/tools/test_process_registry_write_stdin_surrogates.py index 811323d70c..539d980caf 100644 --- a/tests/tools/test_process_registry_write_stdin_surrogates.py +++ b/tests/tools/test_process_registry_write_stdin_surrogates.py @@ -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)