fix(events): daily run ceiling is not held when two events arrive together - #33
Open
levelscorner wants to merge 1 commit into
Open
levelscorner wants to merge 1 commit into
levelscorner wants to merge 1 commit into
Conversation
`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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects in
s16code/events/. The first is the important one: the daily runceiling 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_dayis not a ceiling under concurrency.admit_runreads the window count, the run is then awaited, and the counter isonly written after the run returns. Two deliveries in flight at once both read
the same stale count. A subscription with
max_runs_per_day=1starts 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) andonto the event's decision (
refused_by).morning_reportreads both and addsthem, so a single control firing once is presented to a human as two.
How to reproduce
Both tests fail.
git checkout HEAD -- s16code/eventsrestores the fix and theypass. That is exactly how the red output below was produced.
Why it happens
Bug 1 —
s16code/events/engine.py:127and:147(upstreammain).admit_run(s16code/events/governor.py:124) decides onself.store.window_count(subscription.id, day, kind="run"). The write that wouldmake that count true is on the far side of an
await. Coroutine A suspends at:137; coroutine B runs:127, still reads0, and is admitted. The engine'sself._slotssemaphore 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.recordalwaysincremented
countby one and added the spend, so the slot could not be takenbefore the run without the spend being known.
Bug 2 —
s16code/events/report.py:97and:105.blockedis built at:77from decisions carryingrefused_by;refusalsisthe ledger. Every decision-level refusal in
engine.pycallsrecord_refusalimmediately before it sets
refused_by, so the two collections overlapcompletely 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_recordgrows acountparameter;count=0settles the cost ofa slot already taken, so admission and billing are two separate writes.
governor.recordpassescountthrough.engine.pyrecords the slot immediately afteradmit_runsucceeds, and thepost-run call becomes
count=0.There is no
awaitbetweenadmit_run's read and the new write, so on a singleevent 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
EventStorelockacquisitions, 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_controland buildrefusedfrom the ledgeralone. The ledger is the correct source of truth, not the decisions: intake
refusals (
self_trigger,source_rate_limit) return before any decision iswritten, so they appear only in the ledger.
blockedis kept solely to stop arefused decision falling into
ignored.Proof
Baseline, unmodified
main:Red — new tests added,
s16code/events/untouched: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:
Green — fix restored:
No regression — full suite, 355 before, 357 after, nothing lost:
ruff checkon the five touched files is clean. The repo currently has onepre-existing
I001intests/test_channel_connections.py, which this branchdoes not touch and does not fix.