From 954f5a8ea8c021f82d71fe44351fce61e7560d9b Mon Sep 17 00:00:00 2001 From: Shashank Shekhar Singh Date: Fri, 31 Jul 2026 23:20:48 +0530 Subject: [PATCH] Refuse a sync overrun at guard exit even when the timer never fired MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sync deadline_guard's exit path raised only when its interrupt had actually fired. On the threading.Timer fallback — the mechanism every worker-thread run uses — the timer thread needs the GIL to run fire(), so a node that held it through the deadline (a long C call, or plain scheduling latency) returned normally, disarm() cancelled the pending timer, and the overrun's writes committed; for a last node the run then reported success past its wall-clock ceiling. Mirror the async guard's exit check: raise NodeDeadlineExceeded when the interrupt fired or the meter shows the deadline spent, and say so in the guard's docstring. The regression test monkeypatches threading.Timer with one that never fires, testing the exit contract deterministically instead of racing the timer thread. Fixes #22 Co-Authored-By: Claude Fable 5 --- grapharc/runtime/budget.py | 19 ++++++++----- tests/test_budget_enforcement.py | 46 ++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 6 deletions(-) diff --git a/grapharc/runtime/budget.py b/grapharc/runtime/budget.py index 8ee4761..fc44af1 100644 --- a/grapharc/runtime/budget.py +++ b/grapharc/runtime/budget.py @@ -367,8 +367,8 @@ def deadline_guard(meter: BudgetMeter, *, what: str) -> Iterator[None]: What this does *not* guarantee: - Mechanism 2 cannot interrupt a thread parked inside a C call: a - `time.sleep(60)` sleeps out its 60 seconds and raises on return. This is - not a fan-out-only weakness. Mechanism 1 needs `invoke()` to be on the + `time.sleep(60)` is not interrupted mid-call, but the guard still raises + on exit. This is not a fan-out-only weakness. Mechanism 1 needs `invoke()` to be on the process's main thread, so *any* run driven from a worker thread — every request handler in a threaded server, every `ThreadPoolExecutor` caller — falls back to mechanism 2 for the whole run, nodes and fan-out alike. @@ -382,9 +382,10 @@ def deadline_guard(meter: BudgetMeter, *, what: str) -> Iterator[None]: - Like any asynchronous exception, the interrupt lands wherever the node happened to be: it is as safe as Ctrl-C, no safer. - Short of that last case the ceiling is honoured at the node boundary: if the - deadline passed and the node swallowed the exception, this guard raises on - exit, so the node's writes never reach state. + Short of the never-returns case the ceiling is honoured at the node + boundary: if the deadline passed — whether the node swallowed the exception + or no interrupt was ever delivered — this guard raises on exit, so the + node's writes never reach state. """ remaining = meter.remaining_seconds() if remaining is None: @@ -486,5 +487,11 @@ def disarm() -> None: disarm() except NodeDeadlineExceeded as exc: raise NodeDeadlineExceeded(detail()) from exc - if state["fired"]: + + # Reached only when the node returned normally. It may have swallowed the + # interrupt, or the deadline may have passed without the timer firing — + # the timer thread needs the GIL, which a node inside a long C call + # withholds until it returns; either way its writes must not land. + left = meter.remaining_seconds() + if state["fired"] or (left is not None and left <= 0): raise NodeDeadlineExceeded(detail()) diff --git a/tests/test_budget_enforcement.py b/tests/test_budget_enforcement.py index 00c6166..55dcc44 100644 --- a/tests/test_budget_enforcement.py +++ b/tests/test_budget_enforcement.py @@ -542,6 +542,52 @@ def body(): assert ran_for < 2.0, "the node asked for 5s and was not stopped" +def test_an_overrun_is_refused_at_exit_even_if_the_timer_never_fired(monkeypatch): + """A node that holds the GIL through the deadline — a long C call, or plain + timer-scheduling latency — denies the timer thread its turn: `fire()` never + runs, the node returns normally, and an exit check that tests only + `state["fired"]` lets the overrun's writes land. The contract is the node + boundary, so the exit check itself must notice the spent deadline. + + The timer is replaced with one that never fires, which makes this the + deterministic statement of that contract: asserting on whether a real + timer's async exception got delivered in time is a race (see + `_run_swallower` above), whereas the exit check runs unconditionally. + """ + + class NeverFires: + """`threading.Timer`'s surface as the guard uses it, minus the firing.""" + + def __init__(self, interval, function): + self.daemon = False + + def start(self): + pass + + def cancel(self): + pass + + monkeypatch.setattr(threading, "Timer", NeverFires) + outcome: dict[str, object] = {} + + def body(): # a worker thread uses mechanism 2, like any threaded server + meter = BudgetMeter(Budget(max_seconds=0.05)) + try: + with deadline_guard(meter, what="node 'n'"): + time.sleep(0.2) # outlast the deadline; nothing interrupts it + outcome["raised"] = None + except NodeDeadlineExceeded as exc: + outcome["raised"] = exc + + worker = threading.Thread(target=body) + worker.start() + worker.join(timeout=10) + assert not worker.is_alive() + assert isinstance(outcome["raised"], NodeDeadlineExceeded), ( + "the node overran, no interrupt fired, and the guard let its writes land" + ) + + def test_a_node_that_finishes_in_time_is_left_alone(): def brisk(state): time.sleep(0.05)