Skip to content

Commit 99a5aca

Browse files
Refuse a sync overrun at guard exit even when the timer never fired (#38)
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 <noreply@anthropic.com>
1 parent 6f36c51 commit 99a5aca

2 files changed

Lines changed: 59 additions & 6 deletions

File tree

grapharc/runtime/budget.py

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -367,8 +367,8 @@ def deadline_guard(meter: BudgetMeter, *, what: str) -> Iterator[None]:
367367
What this does *not* guarantee:
368368
369369
- Mechanism 2 cannot interrupt a thread parked inside a C call: a
370-
`time.sleep(60)` sleeps out its 60 seconds and raises on return. This is
371-
not a fan-out-only weakness. Mechanism 1 needs `invoke()` to be on the
370+
`time.sleep(60)` is not interrupted mid-call, but the guard still raises
371+
on exit. This is not a fan-out-only weakness. Mechanism 1 needs `invoke()` to be on the
372372
process's main thread, so *any* run driven from a worker thread — every
373373
request handler in a threaded server, every `ThreadPoolExecutor` caller —
374374
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]:
382382
- Like any asynchronous exception, the interrupt lands wherever the node
383383
happened to be: it is as safe as Ctrl-C, no safer.
384384
385-
Short of that last case the ceiling is honoured at the node boundary: if the
386-
deadline passed and the node swallowed the exception, this guard raises on
387-
exit, so the node's writes never reach state.
385+
Short of the never-returns case the ceiling is honoured at the node
386+
boundary: if the deadline passed — whether the node swallowed the exception
387+
or no interrupt was ever delivered — this guard raises on exit, so the
388+
node's writes never reach state.
388389
"""
389390
remaining = meter.remaining_seconds()
390391
if remaining is None:
@@ -486,5 +487,11 @@ def disarm() -> None:
486487
disarm()
487488
except NodeDeadlineExceeded as exc:
488489
raise NodeDeadlineExceeded(detail()) from exc
489-
if state["fired"]:
490+
491+
# Reached only when the node returned normally. It may have swallowed the
492+
# interrupt, or the deadline may have passed without the timer firing —
493+
# the timer thread needs the GIL, which a node inside a long C call
494+
# withholds until it returns; either way its writes must not land.
495+
left = meter.remaining_seconds()
496+
if state["fired"] or (left is not None and left <= 0):
490497
raise NodeDeadlineExceeded(detail())

tests/test_budget_enforcement.py

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -542,6 +542,52 @@ def body():
542542
assert ran_for < 2.0, "the node asked for 5s and was not stopped"
543543

544544

545+
def test_an_overrun_is_refused_at_exit_even_if_the_timer_never_fired(monkeypatch):
546+
"""A node that holds the GIL through the deadline — a long C call, or plain
547+
timer-scheduling latency — denies the timer thread its turn: `fire()` never
548+
runs, the node returns normally, and an exit check that tests only
549+
`state["fired"]` lets the overrun's writes land. The contract is the node
550+
boundary, so the exit check itself must notice the spent deadline.
551+
552+
The timer is replaced with one that never fires, which makes this the
553+
deterministic statement of that contract: asserting on whether a real
554+
timer's async exception got delivered in time is a race (see
555+
`_run_swallower` above), whereas the exit check runs unconditionally.
556+
"""
557+
558+
class NeverFires:
559+
"""`threading.Timer`'s surface as the guard uses it, minus the firing."""
560+
561+
def __init__(self, interval, function):
562+
self.daemon = False
563+
564+
def start(self):
565+
pass
566+
567+
def cancel(self):
568+
pass
569+
570+
monkeypatch.setattr(threading, "Timer", NeverFires)
571+
outcome: dict[str, object] = {}
572+
573+
def body(): # a worker thread uses mechanism 2, like any threaded server
574+
meter = BudgetMeter(Budget(max_seconds=0.05))
575+
try:
576+
with deadline_guard(meter, what="node 'n'"):
577+
time.sleep(0.2) # outlast the deadline; nothing interrupts it
578+
outcome["raised"] = None
579+
except NodeDeadlineExceeded as exc:
580+
outcome["raised"] = exc
581+
582+
worker = threading.Thread(target=body)
583+
worker.start()
584+
worker.join(timeout=10)
585+
assert not worker.is_alive()
586+
assert isinstance(outcome["raised"], NodeDeadlineExceeded), (
587+
"the node overran, no interrupt fired, and the guard let its writes land"
588+
)
589+
590+
545591
def test_a_node_that_finishes_in_time_is_left_alone():
546592
def brisk(state):
547593
time.sleep(0.05)

0 commit comments

Comments
 (0)