🐛 fix(repository): clamp bucket tokens when set_limits shrinks capacity by mrohr · Pull Request #469 · zeroae/zae-limiter

mrohr · 2026-09-11T13:44:41Z

Parked — pending the scheduled-limits ADR (#222)

Moved to the v1.0.0 milestone. This is a sequencing decision, not a rejection of the work.

This PR, #471, and #473 implement primitives for an out-of-tree cron job that swaps an
entity/resource's capacity at each tick. Issue #222 (time-based dynamic rate limits with cron
scheduling) is being pulled forward so the library does this natively, and #222's own
"Alternatives Considered" already rejects "external scheduler calling set_limits() on schedule"
as adding operational complexity with no atomicity. So: settle the #222 ADR first, then decide
whether each of these three is still needed, needs narrowing, or can be closed.

On this PR specifically. The goal is sound: the speculative fast path is a pure ADD with no
cap math, and refill_bucket() returns early without clamping when tokens_to_add == 0
(bucket.py), so a surplus over a lowered cap can persist indefinitely. Two objections to
address on rebase:

Read-then-SET is racy, and the race over-admits. Every concurrent ADD -consumed that
lands between the read and the write is handed back to the caller. Use
SET b_{l}_tk = :cp with ConditionExpression b_{l}_tk > :cp instead: it is atomic, needs no
read (0 RCU instead of 1), and concurrent ADDs either precede it or apply to the already
clamped value.
It clamps to the undivided cp, but a shard holds cp // shard_count. And
_sync_bucket_params() now fans out to every shard as of 🔒 security(repository): sync limit changes to every bucket shard #476 — the ground under this PR has
changed, so it must clamp each shard to that shard's effective capacity, not the
table-level one. The "known limitation" note below is stale for the same reason.

Also worth folding in: make refill_bucket() clamp unconditionally, so every slow-path and
aggregator pass self-heals a surplus rather than relying on the set_limits() write alone.

Summary

set_limits() already synced a bucket's cp/ra/rp (ADR-120/⚡ Eagerly reconcile bucket limit fields and TTL on config changes #327) but never touched its current token count (tk), so an entity holding tokens accumulated under a higher capacity kept admitting requests above a newly lowered limit until the bucket happened to drain on its own
Neither the speculative fast path (a pure ADD with no cap math) nor lazy refill (which only re-applies the cap once a nonzero refill is computed) touch tk otherwise, so the old ceiling could persist indefinitely
_sync_bucket_params() now reads the bucket's current tk for each limit in the same call and clamps it down to the new cp when the new capacity is smaller than what's currently held; capacity increases are unaffected — tokens still refill in normally (the intended "expand via refill" behavior)
Applied to both repository.py (async) and the generated sync_repository.py

Test plan

test_bucket_tokens_clamped_when_capacity_shrinks (async + sync): a bucket sitting near a high capacity gets tk clamped to the new, lower cp on set_limits(), and a subsequent acquire() for the old surplus is rejected
test_bucket_tokens_untouched_when_capacity_grows (async + sync): tk is left alone when capacity increases — no free tokens are granted
uv run pytest tests/unit/ -q

Known limitation (tracked separately): like the rest of _sync_bucket_params(), only the shard-0 bucket item is read/clamped; shards N>0 are unaffected until they drain and get recreated. This is the same pre-existing gap as #468.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TDVZ82R2Z5sYKsGZziBpgc

Generated by Claude Code

set_limits() synced a bucket's capacity/refill_amount/refill_period (ADR-120) but never touched its current token count, so an entity that already held tokens under a higher capacity kept spending above a newly lowered limit until it happened to drain on its own — neither the speculative fast path nor lazy refill re-apply the capacity cap without a refill event. Read the bucket's current tokens in the same sync call and clamp them down to the new capacity when it shrinks below what's held; capacity increases still refill in normally, unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TDVZ82R2Z5sYKsGZziBpgc

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.

Benchmark suite	Current: `bc1b629`	Previous: `6160884`	Ratio
`tests/benchmark/test_localstack.py::TestCascadeOptimizationBenchmarks::test_cascade_multiple_resources`	`20.03456534938818` iter/sec (`stddev: 0.06219358482685482`)	`31.349289481169496` iter/sec (`stddev: 0.004782968691985447`)	`1.56`

This comment was automatically generated by workflow using github-action-benchmark.

codecov · 2026-09-11T13:50:06Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.95%. Comparing base (6160884) to head (bc1b629).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #469      +/-   ##
==========================================
+ Coverage   92.94%   92.95%   +0.01%     
==========================================
  Files          37       37              
  Lines        8357     8371      +14     
==========================================
+ Hits         7767     7781      +14     
  Misses        590      590

Flag	Coverage Δ
doctest	`29.97% <53.33%> (+0.03%)`	⬆️
integration	`53.78% <100.00%> (+0.07%)`	⬆️
unit	`92.84% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Resumes the #222 design at Section 2 and settles every open item. Decided: - tk is the only materialised quantity; base cp/ra/rp on the bucket item are never rewritten, so schedules compose with actor-driven changes - refill_bucket clamps unconditionally, and the aggregator's `refill_delta <= 0` guard must go — this is what replaces #469 - the actor fan-out writes vu = 0 rather than a computed boundary - cronsim parses, we match; croniter is a dev-only oracle (21,888 comparisons agree across both DST transitions) - next_boundary is our own scan: no cron library computes window *ends*, and croniter.match() re-parses per call at 362us - reset_schedule delivers calendar-aligned quota reset, replacing #471 - boundary-aware retry_after_seconds, replacing #473 Corrected from the earlier draft: - 2.1 "write params" contradicted 1.4; resolved in 1.4's favour - 1.4's JSON encoding crosses the 1 KB WCU boundary at 3 limits x 2 entries (measured 917 B, and 1337 B at 4x3). A compact storage form is 4.9x smaller and keeps the worst shared case at 805 B - "retry_after is at most this long" is false when a boundary *lowers* a limit, which is the headline use case Found: the provisioner's _apply_set never syncs bucket params, so `limits apply` writes config that never reaches live buckets. Likely a pre-existing bug; blocking for schedules via the manifest. Refs #222, #468, #469, #471, #473, #475 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QdVj8nPhUwTz2aNJzMFqt5

Three plans, each producing working, testable software on its own: 1. provisioner-bucket-sync (6 tasks) — the pre-existing bug where `limits apply` writes config that never reaches live buckets. No scheduling content; ships and is verifiable alone. 2. scheduled-limits-core (14 tasks) — schedule.py, the compact storage encoding, the unconditional clamp, vu on the fast path, slow-path materialisation, aggregator awareness. 3. scheduled-limits-surface (12 tasks) — reset_schedule, manifest/CFN/ CLI, boundary-aware retry_after, E2E, ADR-135. Ordering is 1 → 2 → 3. next_boundary takes its final two-tuple signature in plan 2 even though reset_sched is unused there, so plan 3 lands as a behaviour change in one function rather than a signature change across lease.py and processor.py. Notable findings folded in: config items store whole tokens while bucket items store millitokens (a 1000x conversion the provisioner must make); manifest.py validates nothing but `namespace`; refill_amounts is already a delta, so the slow path clamps for free once refill_bucket does. Refs #222, #468, #469, #471, #473

…limits (#482) Refs #222. Settles the scheduled (cron) limits design end to end — Sections 0-9, no open questions — and adds three implementation plans, one per independently shippable piece. Design highlights: `tk` is the only materialised quantity, so the bucket item keeps its undivided base params and the fast path gains exactly one `vu` comparison while never evaluating a schedule or reading config. cronsim parses and we match, with croniter as a dev-only oracle (~22,000 comparisons across both DST transitions). A compact storage encoding keeps bucket items under the 1 KB WCU boundary that the obvious JSON form crossed at 3 limits x 2 entries. Subsumes rather than lands the three parked PRs: #469 via the unconditional clamp (3.3), #471 via reset_schedule (3.6), #473 via boundary-aware retry_after (7). Also stops publishing docs/plans/ — mkdocs set neither docs_dir nor exclude_docs and loads no exclusion plugin, so internal planning artefacts were live and search-indexed on the public docs site. Plans were expanded against post-#430 code, which caught ten factual errors in them, including a snippet that would have re-introduced the second clock reading #484 had just removed. Surface-plan Tasks 3, 4, 5, 10 and 11 remain deliberately thin — they target schedule.py, which does not exist yet — and each says so inline. Docs-only: four markdown files and mkdocs.yml, no Python. codecov/patch reports "Coverage not affected"; codecov/project's -0.06% is measurement jitter against a check with no configured tolerance.

Two errors in the merged scheduled-limits planning documents, both of which would send whoever implements surface Task 5 into building a query surface that disagrees with enforcement. §0 said the goal was to close all three parked PRs. That is right for #469 and #471 -- native scheduling supplies those mechanisms directly -- but not for #473, and its replacement row made a category error: `acquire()` cannot stand in for it, because `acquire()` is a write that consumes and the caller is a display. #473 is adopted and landed; the row now names the non-consuming query as what has no native replacement. §7 wired `retry_after_with_schedule()` into the two places that build a `LimitStatus`, both of which run only after a rejection. The non-consuming query builds statuses too and is reached by neither, so the display would have kept the flat estimate -- the exact thing §7 opens by calling wrong in the direction that matters. §7's own headline example is the counterexample: a daily quota where `acquire()` would say "at midnight" while `check_availability()` said "in eleven hours" about the same bucket at the same instant. Both documents now list the third call site, and the "subsumed rather than dropped" claim is gone from each -- it was not true either way. Surface Task 5 additionally flags that `check_availability()`'s capacity clamp and its missing-bucket branch both use the **base** `limit.capacity`, so inside a `scale: 0.5` window they over-report by 2x. Core plan Task 9 makes `calculate_available` schedule-aware and Task 10 fixes `Limit.from_bucket_state`, but neither reaches those two: they work from the `Limit` resolved out of config, not from a `BucketState`. Refs #222, #472 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QdVj8nPhUwTz2aNJzMFqt5

…ionally (#496) Refs #222 — core plan tasks 6 and 7 of 14, the first implementation PR for scheduled limits. refill_bucket() capped tokens at capacity only on the path that adds them. Its two early returns — elapsed_ms <= 0, and tokens_to_add == 0 — returned the input untouched, so whenever a bucket held MORE than its ceiling, every pass computing no refill preserved the surplus. The clamp was unreachable in exactly the case that needed it. A bucket exceeds its ceiling whenever the ceiling drops beneath it: set_limits shrinking a capacity, a shard doubling halving each shard's share, or a schedule boundary once #222 lands. The aggregator half is the one that matters. processor.py had `if refill_delta <= 0: continue`, so fixing bucket.py alone would make the delta negative on a surplus and the aggregator would skip it — and the aggregator exists to keep hot buckets topped up so the client slow path never runs, meaning the trim would never land on precisely the buckets where over-admission is worth anything. The guard is gone; the write is an ADD of a negative delta, safe for the same commutativity reason the positive case is. 35 insertions / 13 deletions in src/ across two files, no generated code. Alone in a PR because ~40 source lines change refill semantics for every bucket in the system. Supersedes parked PR #469 by making trimming universal on every refill path rather than a special case on one call path. #469 is not closed by this merge. Findings worth keeping: - Zero existing tests changed. The predicted fallout — shard tests asserting the old 1.5x transient — did not materialise; those assertions were always about shares (sum(cp // count) <= cp), which remain correct. Only an explanatory comment was stale. - The slow path needs nothing and got nothing. lease.py sends refill_amounts as a delta from the already-refilled state, so repository.py's tk_delta = r - c reduces to an unconditional ADD that is negative on a surplus by construction. Its guard floor = max(0, c - r) with condition tk >= floor was also verified: with r negative the condition reduces to eff_cp >= consumed, already established by try_consume, so it cannot spuriously reject. - A fourth aggregator test was added beyond the plan. All three of the plan's own tests use tc_delta=0, which the consumption threshold waves through regardless, so nothing pinned the ungating of that threshold — and hot buckets, the entire justification for the change, are exactly the ones with a large tc_delta. ADR-133 was reviewed and deliberately left unedited. Its Capacity-bound paragraph remains accurate: the 1.5x bound, its one-time nature, and "not a reconciliation scheme" all still hold. Only its "decaying as shard 0 drains" clause is now incomplete, since the surplus is also trimmed on the next refill pass — the old mechanism still operates.

@mrohr

…ot (#473) Closes #472. Opened by @mrohr, whose read-amplification analysis surfaced both bugs fixed here. Their commit is preserved at the branch base; three commits were added on top rather than rewriting their work. 11 of their 13 tests are unchanged, along with the method name, the Availability type name, the signature, the locust mirror and the docs entry. Driven by a UI that displays a user's limits per limit. That needs one snapshot — two separate calls are two reads at two instants and can disagree, letting a UI render "0 remaining, available now" — plus per-limit detail, and a pure read. acquire() is a consuming write and is not a substitute for that caller. Fixes two bugs live on main: - time_until_available() issued one get_bucket() per limit against a single ADR-114 composite item, an N+1 where one read suffices. - get_bucket() defaults to shard 0, so time_until_available() computed its wait from one shard while available() sums across all shards via GSI3 (fixed for available() by #466). With two shards holding 20 tokens each on a 100/min limit, a shard-0 read reports 48s where the true answer is 12s — four times wrong, for an entity not even at its limit. LimitStatus fit the per-limit UI row unmodified, so the original's parallel dicts were deleted rather than a second model invented. The one new field is checked_at_ms, the instant the snapshot was taken, so a client can tick its countdown locally instead of re-polling. available() and time_until_available() are now thin wrappers over one read path, and available() still sums every shard. The merge commit resolves to main's bodies verbatim, repairing only two mechanical breakages, so the shard-0 regression exists in no commit on this branch and the redesign reviews on its own. Also corrects three errors in the merged scheduled-limits planning docs that this work exposed: §0 claimed the goal was to close all three parked PRs (#469 and #471 remain superseded; this one is adopted); §0 cited acquire() as already answering "can I proceed, and when", comparing a consuming write to a non-consuming read; and §7 wired boundary-aware retry_after into only two post-rejection call sites, so the non-consuming surface would have kept the flat estimate §7 itself opens by calling wrong. check_availability() is now the third call site in the design and in surface-plan Task 5. CI note: unit (3.12) failed once on test_sync_limiter.py:: TestRateLimiterCascade::test_cascade_consumes_parent (assert 100 == 99) and passed on rerun with no code change. That test is untouched by this PR and its async twin passed in the same run. Non-deterministic, and worth investigating before the scheduled-limits boundary tests land.

863bff8 (cherry-picked here as a54c700) removed the `vu = 0` write from inside `if scheduled:` and added prose saying it belongs outside the block, but never added the code back. Task 13's sample ended on a dangling "Force exactly one materialising pass" comment and wrote `vu` on no path at all — strictly worse than the nesting it was correcting. Appends the three lines after both branches, with the comment moved down to sit with them and a note that `#vu` is SET there and so must never join `remove_parts` (#488). Task 13's own test contradicted the new rule too: `test_removing_a_schedule_removes_the_stamps` asserted `vu not in item` straight after an unscheduled `set_limits`. Under "`vu = 0` on every fan-out" that item carries `vu = 0`; the self-clearing happens on the next materialising pass, not during the fan-out. Renamed to `..._removes_sched_but_still_expires_vu` and corrected, since `sched`/`sched_tz` removal and `vu = 0` are different events. Adds `test_a_never_scheduled_fan_out_still_expires_vu`: every existing test in that class either carries a schedule or is removing one, so nothing covered a bucket that never had one and is simply shrinking a capacity — #469's scenario, and the shape most `set_limits` calls take. Also qualifies Task 11's "unscheduled buckets carry no `vu` at all" docstring, which is now true of that helper but not universally, and sharpens Task 13's Step 2 expected-failure line. Refs #222, #469, #488 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QdVj8nPhUwTz2aNJzMFqt5

## Summary Task 3 of 14 in the scheduled-limits core plan. Adds `effective_params(cp_milli, ra_milli, rp_ms, sched, now_ms) -> tuple[int, int, int]` to `schedule.py`: the capacity, refill amount and refill period in force at a given instant. - **Pure by construction** — plain ints in and out, no `models` import, no clock injection, no config read. Every test calls it directly with an explicit `now_ms`. - **An empty schedule returns the base unchanged**, so the unscheduled path costs one tuple check. - **First matching entry wins**; no match returns the base. - **`scale` multiplies capacity AND refill amount together**, so time-to-fill is preserved. Scaling only the ceiling would silently double refill speed relative to bucket size. - Truncates rather than rounds (rounding a limit *up* admits more than the window allows), with a floor of 1 milli-unit since a zero capacity is unadmittable. - Fixes a plan omission: Step 3 never added `effective_params` to `__all__`. No generated code is involved — the generated-sync hook reported no files to check, confirming `schedule.py` is outside the transformed set. ## Test strengthening (the substance of this change) A mutation harness was run against the new function: **9 mutants, all caught**. Getting there exposed four places where the plan's own tests would have passed under a broken implementation. **1. `test_first_matching_entry_wins` was satisfied by "pick the LARGEST match."** The plan pairs scale `0.5` first with `0.1` second and asserts `500_000` — which `max()` also returns. Added the same pair **reversed** (expecting `100_000`), and mutation-verified that a max-wins implementation fails *only* that added test. Without it, max-wins ships green. Task 4's `next_boundary` scan assumes first-match semantics, so this would have surfaced as an inexplicable boundary bug two tasks later. **2. The `scale` fixture used a base where capacity equalled refill amount**, so the ratio check could not distinguish "scaled capacity only" from "scaled neither". Changed to `(1_000_000, 200_000, 60_000)` with an exact-tuple assertion; each broken variant now fails 8 tests. **3. The floor test asserted `cp >= 1 and ra >= 1`**, which returning the base untouched also satisfies — i.e. not applying `scale` at all. Now an exact `== (1, 1, 60_000)` on base `(1000, 500, 60_000)`. **4. `test_absolute_refill_fields_override_individually` set all three fields at once**, so nothing was individual. Split into capacity-only, refill_amount-only, refill_period-only, and all-three. Mutation-verified that wiring the `refill_amount` override to `entry.capacity` is caught, as is dropping the `x1000` on `refill_period`. Also added: per-entry timezone coverage (parametrised NY/UTC over one instant that is 14:00 in one and 18:00 in the other, pinning that `entry.tz` actually reaches `parse_cron`); `scale > 1` as a multiplier rather than a discount; and truncation-not-rounding. ## The config-cache trap did not apply here — and is not gone Recording this because it matters for later tasks. `effective_params` is pure and no `Repository` is touched, so the trap is simply out of reach at Task 3. The trap: `config_cache.py:99` and `:103` still use `time.time()`, so advancing the injectable `_now_ms` across a window boundary returns pre-boundary limits until the 60s cache TTL expires. It first becomes reachable at **Task 9 or Task 12**, whichever first resolves limits through a `Repository` across a boundary. "Task 3 didn't need it" must not be read as "the trap is gone." ## Cherry-picked commit a54c700 Folded in at the owner's request rather than getting its own PR. It is a planning-doc correction to **Task 13** of the same plan file: `vu = 0` must be written on **every** fan-out, rather than nested inside `if scheduled:`. As originally written, an entity with no schedule got no forced materialising pass, so a `set_limits` capacity shrink left a surplus the speculative fast path could spend — parked **PR #469**'s gap surviving #222 intact. Since most entities have no schedule, that was the common case rather than an edge. It also removes `vu` from the `else` branch's REMOVE list, because `SET` and `REMOVE` on one attribute in a single `UpdateExpression` is the `ValidationException` that #488 hit. This is what lets #222 subsume #469 **completely** rather than partially. **#469 should not be closed until Task 13 lands.** ## Third commit fac8ccc — the cherry-picked correction was itself broken The PR now carries three commits: **a54c7008** (the cherry-picked planning-doc correction above), **9c858cbd** (the Task 3 code, unchanged), and **fac8ccc1**, which repairs a54c700. a54c700 removed the `vu = 0` write that was nested inside `if scheduled:` and added prose saying the write belongs *outside* the block — but never added the code back. `rg -c vu_zero` across the plan file returned **zero**: Task 13's sample wrote `vu` on **no** path at all, strictly worse than the nesting it was correcting. It also left Task 13's own test asserting `BUCKET_FIELD_VU not in item` after an *unscheduled* `set_limits`, which contradicts the new rule, since that item now carries `vu = 0` — self-clearing happens on the next materialising pass, not during the fan-out. fac8ccc fixes both, plus several things the sweep turned up: | Fix | Why | |-----|-----| | Restores the `vu = 0` write after **both** branches | The rule the correction stated but never implemented. The #488 SET-and-REMOVE hazard is noted **inline**, where someone editing the `else` branch will see it, rather than only in prose eighty lines up | | Renames `test_removing_a_schedule_removes_the_stamps` → `test_removing_a_schedule_removes_sched_but_still_expires_vu` | Correcting the assertion alone would have left the name arguing the opposite. `sched`/`sched_tz` removal and `vu = 0` are different events | | Corrects Step 2's expected-failure line | It named only `KeyError: 'sched'`; it now also names `KeyError: 'vu'` on the unscheduled test — deliberately, since that is the assertion that reappears if the write is ever moved back inside the conditional | | Qualifies Task 11's `test_absent_vu_does_not_affect_the_fast_path` docstring | Its claim that unscheduled buckets carry no `vu` is still true *for that test* but no longer universal; added as a parenthetical | | Adds `test_a_never_scheduled_fan_out_still_expires_vu` | Covers a gap that **predates** the edit: every existing test in that class either carries a schedule or is removing one. Nothing covered a bucket that never had one and is simply shrinking a capacity — #469's literal scenario, and the shape most `set_limits` calls take | All 48 `vu` references in the plan were swept for other collateral; nothing else was broken. **For reviewers:** Task 3's code commit (9c858cb) is untouched by all of this and remains green — 3418 unit, 26 gevent, 100% patch coverage. ## Test plan - [x] `tests/unit/test_schedule.py` — 48 passed (35 pre-existing + 13 new), after first failing with `cannot import name 'effective_params'` - [x] Full unit suite — 3418 passed - [x] Gevent — 26 passed with `-n 0` - [x] `mypy` clean on 58 files - [x] `ruff` and `pre-commit` clean - [x] Pre-push patch coverage 100% — 11 new lines, 0 missing - [x] Mutation harness — 9 mutants, all caught ## Deferred - No `parse_cron` memoisation. Task 4 owns that; recorded as a deferred minor. Refs #222 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01QdVj8nPhUwTz2aNJzMFqt5

…alising pass (#526) ## Summary Task 13 of the #222 scheduled-limits core plan — the last task in the core plan. The `set_limits` / `delete_limits` fan-out (`_sync_bucket_params`) now carries `sched` / `sched_tz` / `b_{name}_sched` to every shard of every resource it reaches, and SETs **`vu = 0` on every fan-out, scheduled or not**. The unconditional `vu = 0` is what lets #222 subsume parked PR #469 completely rather than partially. #496 made `refill_bucket` clamp on its early-return paths, but the speculative fast path is a pure `ADD` with no cap maths, so after a `set_limits` capacity shrink a bucket can spend its surplus before any refiller trims it. `vu = 0` forces exactly one materialising pass, which clamps. ## Changes - `repository.py` — `_encode_item_schedule()`, shared by Task 12's `_stamp_schedule()` (bucket create) and the new fan-out encoder, so the two writers of these attributes cannot drift. `_build_bucket_param_update` stamps the schedule and SETs `vu = 0`. `build_composite_normal(clear_vu=...)`. - `lease.py` — `_commit_initial()` passes `clear_vu=not boundaries`. - `zae_limiter_aggregator/processor.py` — `vu` pinned in `try_refill_bucket`'s condition (#508). - `zae_limiter_provisioner/bucket_sync.py` — the mirror gets `vu = 0` and the stale-limit `sched` removal. - `repository_protocol.py` + regenerated `sync_*` twins; CLAUDE.md writer table and the #468 / aggregator-refill sections. ## #508: pinned `vu`, not bumped `rf` Task 13 requires resolving #508: the aggregator's `rf` optimistic lock cannot see a `_sync_bucket_params` fan-out, because that fan-out rewrites `cp`/`ra`/`rp`/`sched` on every shard and never touches `rf`. A stream image captured before it passes the lock and refills toward the **old, larger** capacity. The plan offered two options. I took a third that has option 2's completeness at option 1's cost, by generalising the mechanism #506 already introduced. | | Covers | Cost per refill | Refill forfeited | |---|---|---|---| | 1. Pin per-limit `cp` | `cp` only (not `ra`/`rp`) | 1 term per limit | none | | 2. Fan-out bumps `rf` | everything | none | **all accrued since last stamp, every admin write** | | **3. Pin `vu` (taken)** | **everything the fan-out writes, present and future** | **1 term** | **none** | Since this task the fan-out SETs `vu = 0` on *every* call, so `vu` is precisely the marker for "the operator changed something here". Pinning it closes the whole class for one condition term. Option 2 was rejected on a cost I could not justify: `rf` is the refill clock, so moving it discards everything accrued since the last stamp — up to a full bucket — and the `vu = 0` pass cannot restore it, because that pass would then compute zero elapsed and add nothing. `differ.py` re-asserts every manifest resource on every apply, so a CI pipeline running `zae-limiter limits apply` would silently starve buckets refilling slower than its cadence. Option 1 is narrow: it leaves `ra`/`rp` unguarded and covers nothing added later. The pin adds no false failures the `rf` lock was not already going to catch — the only other writers of `vu` are the client slow path and the aggregator itself, and both move `rf` in the same write. #506's `#sched` pin is kept and still rides only with the `vu` re-stamp: it makes the *boundary* trustworthy, a different claim from "the item has not moved". `test_a_pre_shrink_image_cannot_refill_toward_the_old_capacity` runs the real fan-out against moto and then the real aggregator against the same table, and fails against the previous behaviour. `test_a_current_image_still_refills` and `test_an_untouched_bucket_refills_with_no_vu_at_all` discriminate it. ## Four defects found beyond the plan's text **1. `vu` could never be cleared, so `vu = 0` was a permanent fast-path demotion — not self-clearing.** Filed as #525, fixed in its own commit. The plan asserts "that pass computes `next_boundary(()) -> None` and removes `vu` again, so it is self-clearing and leaves no residue". It does not. `_commit_initial()` computes `vu=None` when nothing is scheduled, and `build_composite_normal`'s contract for `None` is "leave the attribute untouched" — by design, so a pass with nothing to say about the boundary cannot clobber a live one. The aggregator cannot clear it either (it re-stamps only when a boundary exists). The speculative condition is `(attribute_not_exists(#vu) OR #vu > :vu_now)`, so `vu = 0` fails it on **every** acquire, forever: 1 round trip becomes 3, $0.625/M becomes $1.375/M, and an entity-config bucket carries no TTL to recycle the item. Shipping Task 13 as written would have done this to every unscheduled entity touched by any `set_limits` — most of them. Fixed with an explicit `clear_vu` flag. **2. A superseded per-limit `b_{name}_sched` override survives a narrowing change.** The plan's snippet SETs an override only where a limit differs from the item default, and REMOVEs overrides only in the `else` (nothing-scheduled) branch. So two limits that diverge and then converge leave the second one enforcing the superseded schedule forever — absence means "inherit the item default", and the override is still there. Same trap in the other direction when a limit loses its schedule while another keeps one, which never reaches the `else` branch at all. Both branches now SET and REMOVE symmetrically. **3. A dropped limit's schedule override outlived the limit.** `sched` was not in the stale-limit REMOVE field list, so `b_{name}_sched` stayed behind as orphan state that re-attaches the moment a limit of that name is configured again. Added on both the async path and the provisioner mirror. **4. The provisioner mirror had #469's exposure and no `vu = 0`.** Task 13 names only `repository.py`, but `zae_limiter_provisioner/bucket_sync.py` is a sync mirror of the same function and a manifest apply that shrinks a capacity has exactly the same unclamped-surplus window. `differ.py` emits a change for every manifest resource on every apply, so this is the *common* path for manifest users. Schedules are not manifest-expressible, so the mirror deliberately does not touch `sched`/`sched_tz` — that write must not strip a schedule set through the Python API — but it now writes `vu = 0`. This also matters for the #508 decision above: the `vu` pin only protects manifest-driven changes if the mirror stamps `vu` too. ## N-surfaces checked `_sync_bucket_params` is every shard × every resource × every limit, writes serially, and raises `FanoutIncomplete` with a progress count on partial failure. | Surface | Covered by | |---|---| | Every shard stamped and expired | `test_every_shard_is_stamped_and_expired` (4 shards) | | Entity-wide `_default_` reaches every resource (#487) | `test_the_entity_wide_scope_reaches_every_resource` | | A resource resolving at a different level keeps **its own** schedule | `test_a_resource_with_its_own_config_gets_its_own_schedule` | | Many limits, converging / diverging / dropped schedules | defects 2 and 3 above | | `vu` vs the stale REMOVE list (#488) | `test_vu_is_never_set_and_removed_in_one_expression`, `test_no_alias_is_both_set_and_removed` | | Partial fan-out still reports progress | `test_a_partial_fan_out_reports_how_many_buckets_were_stamped` | | Each shard clears its own stamp independently | `test_every_shard_clears_its_own_stamp` | | Cross-limit timezone conflict | `test_rejects_limits_that_disagree_on_timezone` (matches the create path) | ## The load-bearing claim `tests/benchmark/test_capacity.py::TestScheduledFastPathCapacity` asserts a scheduled bucket's fast path reads no config, no batch, no query — one conditional `UpdateItem`, identical to unscheduled — and that the fan-out's forced pass costs **one** demoted acquire, not every acquire. ## Test plan - 54 new tests (unit) + 3 benchmark capacity tests - **13 mutations, all killed, zero survivors** — `vu = 0` nested inside `if scheduled:` (the plan's shape), `vu = 0` dropped, item-level `sched` SET dropped, `else`-branch REMOVE dropped, per-limit override SET-only (the plan's snippet), stale `sched` dropped, timezone check disabled, `clear_vu` branch dropped, lease never asks to clear, aggregator `vu` pin dropped, pin ignores the value, provisioner mirror `vu = 0` dropped, fan-out stamps `now_ms` instead of `0` - The three highest-value mutations were re-verified against their **behavioural** tests individually, not just the first condition-string test to fail - `uv run pytest tests/unit/ -q` → **3867 passed** (baseline 3813 + 54) - `uv run pytest tests/unit/ -m gevent -n 0 -q` → **26 passed** - `uv run pytest tests/benchmark/test_capacity.py -o "addopts=" -q` → **21 passed** (baseline 18 + 3) - `pre-commit run` green on every changed file (ruff, ruff-format, mypy, Verify Generated Sync Code); `git diff --exit-code` clean after regeneration Refs #222 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01QdVj8nPhUwTz2aNJzMFqt5

sodre · 2026-09-15T07:38:50Z

Closing this as superseded — the behaviour it asks for is now on main, arrived at by a different mechanism.

What supersedes it. Task 13 of the #222 scheduled-limits core plan, merged in #526 as bd450ab10120c8ad8ec83e748f9915c2f809a6fb (feature commit 53f162ec). _sync_bucket_params — the set_limits / delete_limits fan-out — now SETs vu = 0 on every fan-out, scheduled or not. vu is the "next boundary" marker the speculative fast-path condition tests, so vu = 0 fails that condition once and routes the very next acquire() through the slow path. That pass materialises the bucket, and materialising trims it to its new ceiling, then clears vu again so the demotion is a single pass rather than a permanent one (that self-clearing was itself a defect found during Task 13 and fixed in the same PR, #525).

In other words: the unconditional vu = 0 supplies exactly the trigger this PR was written to provide. A capacity shrink can no longer leave a surplus sitting on a bucket to be spent by the pure-ADD fast path before a refiller gets to it.

Both review objections on this PR are satisfied by other means.

The read-then-write race. The trim is not a read-modify-write. It is an atomic ADD delta, computed per shard against that shard's effective capacity (capacity_milli // shard_count), and applied to every shard of every resource the fan-out reaches — not just shard 0, not just the keyed resource. No read, therefore no race to lose.
The paths that skip the clamp. refill_bucket clamps to min(capacity_milli, tokens_milli) unconditionally on every path, including the two early returns that add no tokens, since 🐛 fix(bucket,aggregator): clamp tokens on every refill path, unconditionally #496. That was the gap that made a trigger-only fix incomplete; it is closed independently.

Thank you. The diagnosis here was correct, and it is what motivated the unconditional form of the vu = 0 write — the plan's original text only stamped vu when something was actually scheduled, which would have left precisely the hole you identified for every unscheduled bucket. Widening it to fire on every fan-out is what lets #222 subsume this completely rather than partially, and that widening traces directly back to this PR.

Note for anyone arriving here from search: #471 (reset bucket usage for an entity/resource pair) is not superseded and stays open. It is a separate operator primitive — an explicit, on-demand reset — which vu = 0 does not provide.

mrohr added this to the v0.13.0 milestone Sep 11, 2026

mrohr added the area/limiter Core rate limiting logic label Sep 11, 2026

github-actions Bot reviewed Sep 11, 2026

View reviewed changes

sodre mentioned this pull request Sep 12, 2026

🎯 Close out v0.13.0 and the sharding follow-ons #478

Closed

8 tasks

sodre modified the milestones: v0.13.0, v1.0.0 Sep 12, 2026

This was referenced Sep 12, 2026

✨ feat(limiter): reset bucket usage for an entity/resource pair #471

Open

✨ feat(limiter): add check_availability() combining capacity and wait time #473

Merged

This was referenced Sep 14, 2026

📝 docs(plans): settled design and implementation plans for scheduled limits (#222) #482

Merged

🐛 Provisioner never syncs bucket params, so manifest-applied entity limits never reach live buckets #481

Closed

This was referenced Sep 14, 2026

✨ Support time-based dynamic rate limits with scheduling #222

Closed

🐛 fix(bucket,aggregator): clamp tokens on every refill path, unconditionally #496

Merged

sodre mentioned this pull request Sep 14, 2026

✨ feat(models): resolve effective limit params from a schedule #499

Merged

7 tasks

This was referenced Sep 15, 2026

🐛 A bucket stamped vu = 0 can never clear it, permanently demoting the entity to the slow path #525

Closed

✨ feat(repository): stamp schedules onto buckets and force one materialising pass #526

Merged

sodre closed this Sep 15, 2026

Navigation Menu

🐛 fix(repository): clamp bucket tokens when set_limits shrinks capacity - #469

🐛 fix(repository): clamp bucket tokens when set_limits shrinks capacity#469
mrohr wants to merge 1 commit into
mainfrom
claude/beautiful-planck-bqdvfa

mrohr commented Sep 11, 2026 •

edited by sodre

Loading

Uh oh!

github-actions Bot left a comment

Uh oh!

codecov Bot commented Sep 11, 2026 •

edited

Loading

Uh oh!

sodre commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mrohr commented Sep 11, 2026 • edited by sodre Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Parked — pending the scheduled-limits ADR (#222)

Summary

Test plan

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

codecov Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sodre commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mrohr commented Sep 11, 2026 •

edited by sodre

Loading

codecov Bot commented Sep 11, 2026 •

edited

Loading