Skip to content

Commit eef7c02

Browse files
committed
fix: prefix the start-budget env var, reject inf, bound the CI job (ROADMAP.md #72)
DOCX_SESSION_START_TIMEOUT becomes DOCX_EDITOR_SESSION_START_TIMEOUT, matching the repo's one existing env var before the name ships. The budget now rejects inf (an unbounded wait defeats the diagnostic), CI sets 60 instead of 120 and bounds the job at 45 minutes so a runner where every kernel start times out fails in bounded time. The three fake-kernel tests that only checked the error text now also prove the stand-in died, the stop-session kill spy filters on the kernel's pid, and the second-phase budget is computed at its use so the report cannot total more than the budget.
1 parent 8cfa3ac commit eef7c02

5 files changed

Lines changed: 55 additions & 24 deletions

File tree

‎.github/workflows/main.yml‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ jobs:
2727

2828
tests-and-type-check:
2929
runs-on: ubuntu-latest
30+
# A wedged runner must fail in bounded time: with every kernel start
31+
# timing out, the session tests alone would otherwise grind for hours.
32+
timeout-minutes: 45
3033
name: tests-and-type-check (${{ matrix.python-version }}, shard ${{ matrix.shard }})
3134
strategy:
3235
matrix:
@@ -56,10 +59,12 @@ jobs:
5659
env:
5760
# A kernel start on this runner (2 vCPUs, two xdist workers each
5861
# spawning kernels, coverage tracing on 3.11) can take well over the
59-
# 30 s library default. A start that needs more than this is a broken
60-
# runner and should fail loudly — the error explains which phase
61-
# timed out — rather than be retried.
62-
DOCX_SESSION_START_TIMEOUT: "120"
62+
# 30 s library default. Doubling covers that; going higher only
63+
# multiplies the wall time of a broken runner, where every start in
64+
# the module times out in sequence. A start that needs more than
65+
# this should fail loudly — the error explains which phase timed
66+
# out — rather than be retried.
67+
DOCX_EDITOR_SESSION_START_TIMEOUT: "60"
6368
run: >-
6469
uv run python -m pytest tests -n auto
6570
--splits 2 --group ${{ matrix.shard }}

‎docx_editor/session.py‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import argparse
1313
import ast
1414
import json
15+
import math
1516
import os
1617
import re
1718
import signal
@@ -36,7 +37,7 @@
3637
# loaded machine (a 2-vCPU CI runner with parallel test workers, coverage
3738
# tracing on) that import alone can take longer than the unloaded 30 s default.
3839
DEFAULT_START_TIMEOUT = 30.0
39-
START_TIMEOUT_ENV = "DOCX_SESSION_START_TIMEOUT"
40+
START_TIMEOUT_ENV = "DOCX_EDITOR_SESSION_START_TIMEOUT"
4041

4142
# How the kernel is launched. Tests substitute a stand-in kernel here to
4243
# exercise the start-failure paths deterministically; the "-f <file>" argument
@@ -147,7 +148,7 @@ def _read_pid(connection_file: Path) -> int | None:
147148

148149

149150
def _start_timeout() -> float:
150-
"""start_session's budget: DOCX_SESSION_START_TIMEOUT, else the 30 s default.
151+
"""start_session's budget: DOCX_EDITOR_SESSION_START_TIMEOUT, else the 30 s default.
151152
152153
Raises:
153154
SessionError: If the variable is set to anything but a positive number.
@@ -157,16 +158,21 @@ def _start_timeout() -> float:
157158
return DEFAULT_START_TIMEOUT
158159
try:
159160
value = float(raw)
160-
if not value > 0: # also rejects nan
161+
if not (math.isfinite(value) and value > 0): # rejects nan and inf too
161162
raise ValueError
162163
except ValueError:
163164
raise SessionError(f"{START_TIMEOUT_ENV} must be a positive number of seconds, got {raw!r}") from None
164165
return value
165166

166167

167-
def _drain_stderr(proc: "subprocess.Popen[bytes]") -> str:
168+
def _drain_stderr(proc: subprocess.Popen[bytes]) -> str:
168169
"""Tail of what a (killed and reaped) kernel wrote to stderr, without blocking.
169170
171+
Invariant: nothing may read ``proc.stderr`` before this runs — the raw-fd
172+
read below bypasses the ``BufferedReader``, so any bytes already buffered
173+
there would be silently dropped. (Today the only other reader, the
174+
exited-during-startup path, always raises without reaching ``_abort``.)
175+
170176
The pipe is switched to non-blocking first: every writer is dead, so the
171177
read hits EOF at once — but if a grandchild ever inherited the write end,
172178
EAGAIN ends the loop instead of hanging a diagnostic that exists to explain
@@ -221,7 +227,7 @@ def start_session(connection_file: Path = DEFAULT_CONNECTION_FILE, timeout: floa
221227
Args:
222228
connection_file: Where to write the kernel connection file
223229
timeout: Seconds to wait for the kernel to become ready. ``None`` (the
224-
default) reads ``DOCX_SESSION_START_TIMEOUT`` and falls back to
230+
default) reads ``DOCX_EDITOR_SESSION_START_TIMEOUT`` and falls back to
225231
30 s when it is unset or blank; an explicit argument always wins.
226232
The variable exists for loaded machines — CI runners, parallel
227233
test workers — where a fresh interpreter importing IPython takes
@@ -234,7 +240,7 @@ def start_session(connection_file: Path = DEFAULT_CONNECTION_FILE, timeout: floa
234240
Raises:
235241
SessionError: If a session is already running, the kernel fails to
236242
start, the connection file it wrote is unreadable, or
237-
``DOCX_SESSION_START_TIMEOUT`` is not a positive number.
243+
``DOCX_EDITOR_SESSION_START_TIMEOUT`` is not a positive number.
238244
A start that runs out of budget explains itself: which phase
239245
timed out (connection file vs. control-channel reply), whether
240246
the kernel was still alive, the load average, what it wrote to
@@ -298,7 +304,6 @@ def _abort(summary: str) -> SessionError:
298304
)
299305
time.sleep(0.1)
300306
t_file = time.monotonic() - t0
301-
remaining = max(1.0, deadline - time.monotonic())
302307

303308
kc = None
304309
try:
@@ -309,6 +314,7 @@ def _abort(summary: str) -> SessionError:
309314
kc = _client(connection_file)
310315
except (ValueError, OSError) as e:
311316
raise SessionError(f"Kernel wrote an unreadable connection file ({connection_file}): {e}") from e
317+
remaining = max(1.0, deadline - time.monotonic())
312318
if not _kernel_alive(kc, timeout=remaining):
313319
raise _abort(
314320
f"Kernel did not become ready within {timeout}s: kernel.json appeared after {t_file:.1f}s, "

‎skills/docx/SKILL.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1176,7 +1176,7 @@ scripts. The document (and all your variables) stays open between commands:
11761176
```bash
11771177
# Requires: pip install "docx-editor[session]"
11781178
docx-session start
1179-
# On a loaded machine (CI, parallel workers) set DOCX_SESSION_START_TIMEOUT=120
1179+
# On a loaded machine (CI, parallel workers) set DOCX_EDITOR_SESSION_START_TIMEOUT=60
11801180
# to raise the 30 s budget `start` gives the kernel to answer.
11811181

11821182
# Use ABSOLUTE paths: the kernel keeps the cwd it was started in, which is not

‎tests/conftest.py‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -180,10 +180,12 @@ def counting(root, tags, **kwargs):
180180
def _connection_file_from_argv(args) -> Path | None:
181181
"""Return the ``-f <path>`` of an ipykernel_launcher command line, else None.
182182
183-
``start_session`` always spawns the kernel as
183+
``start_session`` spawns the kernel as
184184
``[python, "-m", "ipykernel_launcher", "-f", <connection file>]``, so the
185185
connection file can be recovered from the argv alone — no cooperation from
186-
the code under test required.
186+
the code under test required. Tests that substitute a stand-in kernel (via
187+
``session_mod._KERNEL_COMMAND`` or a fake ``subprocess``) are invisible to
188+
this reaper and must reap their own processes.
187189
188190
The path is resolved eagerly: cwd is correct now, but two tests chdir
189191
(``test_workspace.py``, ``test_document.py``), so a relative path stored

‎tests/test_session.py‎

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,11 @@
4242
)
4343
from docx_editor.workspace import _pid_alive # noqa: E402
4444

45-
# Every wait in this module is bounded by the kernel start budget, so a loaded
46-
# machine raises them all through DOCX_SESSION_START_TIMEOUT (CI sets 120).
45+
# Start waits and busy-kernel handshakes in this module are bounded by the
46+
# kernel start budget, so a loaded machine raises them all through
47+
# DOCX_EDITOR_SESSION_START_TIMEOUT (CI sets 60). A few fixed windows remain
48+
# where the tested behaviour is itself wall-clock (the 10 s silence probe in
49+
# TestKernelDeath, the stdin and silent-exec bounds).
4750
START_BUDGET = _start_timeout()
4851
# For the one behaviour that is inherently wall-clock — "my own code overran":
4952
# an idle kernel must dequeue a request within this long. 2 s locally, 12 s on CI.
@@ -395,7 +398,11 @@ def test_request_discarded_behind_a_raising_command(tmp_path):
395398
assert res.started is False # the tell: never dequeued
396399
assert res.stdout == ""
397400

398-
_wait_for_reply(hog) # the failing command is done; the kernel is idle
401+
# The reply lands just before ipykernel's post-error abort turn
402+
# (_abort_queues, one event-loop turn at the default
403+
# stop_on_error_timeout of 0); the eval below builds a fresh
404+
# client first, which comfortably outlasts it.
405+
_wait_for_reply(hog) # the failing command is done
399406
assert eval_code("globals().get('discarded')", connection_file=conn).value is None
400407
finally:
401408
stop_session(conn)
@@ -970,7 +977,9 @@ def test_stop_session_honours_the_shutdown_ack(tmp_path, monkeypatch):
970977
real_kill = os.kill
971978

972979
def spying_kill(target: int, sig: int) -> None:
973-
if sig != 0: # _pid_alive probes with signal 0; only real signals count
980+
# _pid_alive probes with signal 0, and unrelated components may signal
981+
# other pids; only real signals aimed at this kernel count.
982+
if sig != 0 and target == pid:
974983
signalled.append((target, sig))
975984
real_kill(target, sig)
976985

@@ -1053,6 +1062,14 @@ def test_module_entrypoint_runs():
10531062
"""
10541063

10551064

1065+
def _assert_reported_kernel_dead(report: str) -> None:
1066+
"""The stand-in kernel's argv has no ``ipykernel_launcher``, so conftest's
1067+
reaper cannot see it: every test must prove ``_abort`` killed it."""
1068+
pid_match = re.search(r"Kernel pid (\d+)", report)
1069+
assert pid_match is not None, report
1070+
assert not _pid_alive(int(pid_match.group(1)), reap=True)
1071+
1072+
10561073
class TestStartFailureDiagnosis:
10571074
"""A start that exhausts its budget must say why, and the budget must be
10581075
tunable for loaded machines (ROADMAP.md #72).
@@ -1083,21 +1100,21 @@ def test_start_timeout_explains_itself(self, tmp_path, fake_kernel):
10831100
# Nothing left behind: no state files, and the fake is dead.
10841101
assert not conn.exists()
10851102
assert not conn.with_suffix(".pid").exists()
1086-
pid_match = re.search(r"Kernel pid (\d+)", message)
1087-
assert pid_match is not None, message
1088-
assert not _pid_alive(int(pid_match.group(1)), reap=True)
1103+
_assert_reported_kernel_dead(message)
10891104

10901105
def test_start_budget_comes_from_env(self, tmp_path, fake_kernel, monkeypatch):
10911106
monkeypatch.setenv(START_TIMEOUT_ENV, "2")
1092-
with pytest.raises(SessionError, match="did not become ready within 2.0s"):
1107+
with pytest.raises(SessionError, match="did not become ready within 2.0s") as excinfo:
10931108
start_session(tmp_path / "kernel.json")
1109+
_assert_reported_kernel_dead(str(excinfo.value))
10941110

10951111
def test_explicit_timeout_beats_env(self, tmp_path, fake_kernel, monkeypatch):
10961112
monkeypatch.setenv(START_TIMEOUT_ENV, "2")
1097-
with pytest.raises(SessionError, match="did not become ready within 1.0s"):
1113+
with pytest.raises(SessionError, match="did not become ready within 1.0s") as excinfo:
10981114
start_session(tmp_path / "kernel.json", timeout=1.0)
1115+
_assert_reported_kernel_dead(str(excinfo.value))
10991116

1100-
@pytest.mark.parametrize("value", ["soon", "0", "-5", "nan"])
1117+
@pytest.mark.parametrize("value", ["soon", "0", "-5", "nan", "inf"])
11011118
def test_invalid_start_budget_raises(self, tmp_path, monkeypatch, value):
11021119
monkeypatch.setenv(START_TIMEOUT_ENV, value)
11031120
# Were a kernel spawned anyway, this exits at once with a distinct error.
@@ -1123,6 +1140,7 @@ def test_main_start_prints_the_diagnosis(self, tmp_path, fake_kernel, monkeypatc
11231140
assert "did not become ready within 2.0s" in err
11241141
assert "fake kernel: sockets bound, never served" in err
11251142
assert START_TIMEOUT_ENV in err
1143+
_assert_reported_kernel_dead(err)
11261144

11271145

11281146
class TestKernelReaping:

0 commit comments

Comments
 (0)