Skip to content

fix(events): daily run ceiling is not held when two events arrive together - #33

Open
levelscorner wants to merge 1 commit into
theschoolofai:mainfrom
levelscorner:fix/event-governor-and-store
Open

levelscorner wants to merge 1 commit into
theschoolofai:mainfrom
levelscorner:fix/event-governor-and-store

Conversation

@levelscorner

Copy link
Copy Markdown

Two defects in s16code/events/. The first is the important one: the daily run
ceiling does not hold when two events arrive together. The second is a smaller,
independent bug in the morning report that happens to live in the same module.
They are separate defects and can be reviewed separately — each has its own test
and its own red line below.


What is broken

1. max_runs_per_day is not a ceiling under concurrency.

admit_run reads the window count, the run is then awaited, and the counter is
only written after the run returns. Two deliveries in flight at once both read
the same stale count. A subscription with max_runs_per_day=1 starts two runs,
and because neither was refused, no refusal is recorded either — so the
operator's report shows a ceiling that was honoured and a night in which nothing
was blocked. This is precisely the failure the governor exists to prevent: the
module docstring says an unattended agent "is bounded by the controls around it
and by nothing else", and this control does not bound it.

2. One refusal is reported as two blocked pieces of work.

A refusal is written to both the refusal ledger (store.record_refusal) and
onto the event's decision (refused_by). morning_report reads both and adds
them, so a single control firing once is presented to a human as two.

How to reproduce

git checkout <this branch>
git checkout origin/main -- s16code/events   # keep the new tests, drop the fix
uv run pytest tests/test_autonomy_bounds_regressions.py -q

Both tests fail. git checkout HEAD -- s16code/events restores the fix and they
pass. That is exactly how the red output below was produced.

Why it happens

Bug 1 — s16code/events/engine.py:127 and :147 (upstream main).

admit = self.governor.admit_run(subscription, now=now)   # :127  reads window_count
...
result = await self.runtime.run(...)                     # :137  <-- suspension point
...
self.governor.record(subscription.id, kind="run", ...)   # :147  writes window_count

admit_run (s16code/events/governor.py:124) decides on
self.store.window_count(subscription.id, day, kind="run"). The write that would
make that count true is on the far side of an await. Coroutine A suspends at
:137; coroutine B runs :127, still reads 0, and is admitted. The engine's
self._slots semaphore does not help — its default is 4, and it bounds fan-out,
not the window ledger.

The read and the write were also fused into one call: governor.record always
incremented count by one and added the spend, so the slot could not be taken
before the run without the spend being known.

Bug 2 — s16code/events/report.py:97 and :105.

"blocked_by_a_control": len(blocked) + len(refusals),   # :97
"refused": blocked + [ ... for item in refusals],       # :105

blocked is built at :77 from decisions carrying refused_by; refusals is
the ledger. Every decision-level refusal in engine.py calls record_refusal
immediately before it sets refused_by, so the two collections overlap
completely and the sum double-counts.

The fix

Bug 1. Take the window slot before the first await, then settle the spend
afterwards as a second write that moves money but not the counter:

  • store.window_record grows a count parameter; count=0 settles the cost of
    a slot already taken, so admission and billing are two separate writes.
  • governor.record passes count through.
  • engine.py records the slot immediately after admit_run succeeds, and the
    post-run call becomes count=0.

There is no await between admit_run's read and the new write, so on a single
event loop the check-and-take is now atomic. A run that raises keeps its slot,
which fails closed — the conservative direction for a spend control.

Scope note: this closes the async interleaving window, which is the one the
engine actually has. It is still a check-then-act across two EventStore lock
acquisitions, so a genuinely multi-process deployment would want the take pushed
down into the store as one locked operation. That is a larger change than this
bug needs and is not attempted here.

Bug 2. Count blocked_by_a_control and build refused from the ledger
alone. The ledger is the correct source of truth, not the decisions: intake
refusals (self_trigger, source_rate_limit) return before any decision is
written, so they appear only in the ledger. blocked is kept solely to stop a
refused decision falling into ignored.

Proof

Baseline, unmodified main:

$ uv run pytest -q
355 passed, 1 warning in 38.68s

Red — new tests added, s16code/events/ untouched:

$ uv run pytest tests/test_autonomy_bounds_regressions.py -q
FF                                                                       [100%]
=================================== FAILURES ===================================
_______ test_the_daily_run_ceiling_holds_when_two_events_arrive_together _______

        await asyncio.gather(
            engine.process(_event(id="a"), llm=_relevance_llm(stall=True)),
            engine.process(_event(id="b"), llm=_relevance_llm(stall=True)),
        )

>       assert len(runtime.runs) == 1, "the second event must not become a run"
E       AssertionError: the second event must not become a run
E       assert 2 == 1
E        +  where 2 = len(['Assess the increase.', 'Assess the increase.'])
E        +    where ['Assess the increase.', 'Assess the increase.'] = <test_autonomy_bounds_regressions._Runtime object at 0x10815f8c0>.runs

tests/test_autonomy_bounds_regressions.py:72: AssertionError
_________________ test_one_refusal_is_reported_once_not_twice __________________

        assert len(store.refusals()) == 1, "exactly one control fired"

        report = morning_report(store)
>       assert report["totals"]["blocked_by_a_control"] == 1
E       assert 2 == 1

tests/test_autonomy_bounds_regressions.py:92: AssertionError
=========================== short test summary info ============================
FAILED tests/test_autonomy_bounds_regressions.py::test_the_daily_run_ceiling_holds_when_two_events_arrive_together
FAILED tests/test_autonomy_bounds_regressions.py::test_one_refusal_is_reported_once_not_twice
2 failed in 0.11s

Note the second assertion in the ceiling test never even runs: with two runs
admitted, store.refusals() is empty, so the breach leaves no trace.

Full suite in that state — the 355 that passed before still pass, and the two
new tests fail:

$ uv run pytest -q
FAILED tests/test_autonomy_bounds_regressions.py::test_the_daily_run_ceiling_holds_when_two_events_arrive_together
FAILED tests/test_autonomy_bounds_regressions.py::test_one_refusal_is_reported_once_not_twice
2 failed, 355 passed, 1 warning in 37.72s

Green — fix restored:

$ uv run pytest tests/test_autonomy_bounds_regressions.py -v
collecting ... collected 2 items

tests/test_autonomy_bounds_regressions.py::test_the_daily_run_ceiling_holds_when_two_events_arrive_together PASSED [ 50%]
tests/test_autonomy_bounds_regressions.py::test_one_refusal_is_reported_once_not_twice PASSED [100%]

============================== 2 passed in 0.09s ===============================

No regression — full suite, 355 before, 357 after, nothing lost:

$ uv run pytest -q
357 passed, 1 warning in 37.78s

ruff check on the five touched files is clean. The repo currently has one
pre-existing I001 in tests/test_channel_connections.py, which this branch
does not touch and does not fix.

`admit_run` reads the window count, then the run is awaited, and the
counter is only written after it returns — so two deliveries in flight at
once both read the same stale count and a `max_runs_per_day` of one admits
two runs, recording no refusal at all. The slot is now taken before the
first await and the spend is settled afterwards with `count=0`, which
needed a `count` passthrough on `AutonomyGovernor.record` and
`EventStore.window_record`.

Also fixes a separate double-count in the morning report: a refusal is
written both to the refusal ledger and onto the event's decision, and the
report added the two, so one refusal was shown to an operator as two
blocked pieces of work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant