Conversation
Adds a way to reset an entity's token bucket state for one resource back to a blank slate without touching its stored limit config, for use right after an operator adjusts an entity's limits. Deletes the bucket item(s) for (entity_id, resource) across all shards (GHSA-76rv), so the next acquire() recreates the bucket at full capacity on the slow path. Repository-level only, mirroring where disable_entity/enable_entity live. Exposed via CLI as `zae-limiter entity reset-bucket ENTITY_ID --resource RESOURCE`. Closes #470 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dnre1GeQwf423EPtZNKitp
Adds a CLAUDE.md reference section, an entity reset-bucket section in docs/cli.md, a config-hierarchy tip pointing at it from the "changing limits" discussion, the bucket_reset audit action in docs/infra/auditing.md, and rewrites the docs/operations/rate-limits.md runbook (previously only deleted shard 0 by hand, which is incomplete under write sharding and left no audit trail). Refs #470 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dnre1GeQwf423EPtZNKitp
mrohr
added
area/cli
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| Returns: | ||
| Number of bucket items deleted (0 if there was nothing to reset). | ||
| """ | ||
| ... |
| Returns: | ||
| Number of bucket items deleted (0 if there was nothing to reset). | ||
| """ | ||
| ... |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
⚠️ 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: 7447dc4 | Previous: 6160884 | Ratio |
|---|---|---|---|
tests/benchmark/test_localstack.py::TestCascadeOptimizationBenchmarks::test_cascade_multiple_resources |
19.555884617951854 iter/sec (stddev: 0.05186278461799603) |
31.349289481169496 iter/sec (stddev: 0.004782968691985447) |
1.60 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #471 +/- ##
==========================================
+ Coverage 92.94% 92.96% +0.02%
==========================================
Files 37 37
Lines 8357 8389 +32
==========================================
+ Hits 7767 7799 +32
Misses 590 590
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
The `<entity_id>`/`<resource>` placeholders in the Python doctest block are real values passed to reset_bucket(), which validates resource names and rejects angle brackets. Use concrete example identifiers matching the surrounding sections' convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dnre1GeQwf423EPtZNKitp
mrohr
marked this pull request as ready for review
September 11, 2026 15:32
Verifies acquire() behavior after Repository.reset_bucket(): usage does not carry forward, debt from lease.adjust() is cleared, the reset is a no-op for a never-acquired bucket, every shard is deleted under write sharding, and a reset is scoped to one resource. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dnre1GeQwf423EPtZNKitp
This was referenced Sep 12, 2026
12 tasks
sodre
added a commit
that referenced
this pull request
Sep 14, 2026
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
sodre
added a commit
that referenced
this pull request
Sep 14, 2026
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
3 tasks
sodre
added a commit
that referenced
this pull request
Sep 14, 2026
…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.
5 tasks
sodre
added a commit
that referenced
this pull request
Sep 14, 2026
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
sodre
added a commit
that referenced
this pull request
Sep 14, 2026
…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.
sodre
added a commit
that referenced
this pull request
Sep 15, 2026
## Summary Surface-plan Task 3 for #222 — the task that makes `reset_schedule` actually do something. Tasks 1, 2 and 4 built the parts (`Limit.quota`, `prev_reset_edge`, reset-aware `next_boundary`, the `rsched` encoding); nothing yet applied a reset to a bucket. A crossed reset edge sets the balance to the shard's share of the capacity **in force at that instant**, **before** admission — so the request that crosses midnight is gated against the restored quota, not the one after it. Detection is backwards (`prev_reset_edge(reset_sched, now) > rf`), which makes an idle bucket correct for free and two missed edges idempotent. `tc` is never touched, so the total-consumed counter stays monotonic — the reason this is safer than the `reset_bucket()` parked PR #471 proposed. Both materialising writers apply it, using the same `> rf` comparison against the same stored `rf` and stamping `rf` past the edge in the same write, so whichever gets there first wins and the other skips: - client: `RateLimiter._apply_reset_edge()`, called at both slow-path entry-building seams (`_do_acquire`, `_try_parent_only_acquire`); - aggregator: `try_refill_bucket()`, expressing it as `ADD (eff_cp - tk_observed)` — the identical delta shape the unconditional clamp already uses. `vu` now folds the reset tuple in on both sides (`RateLimiter._materialisation_stamps()`, `_item_next_boundary()`): a limit with a reset schedule and **no** parameter schedule would otherwise never expire `vu`, and the daily quota is exactly that shape. ## Defects found beyond the plan's text 1. **The plan's aggregator ordering is wrong.** Its Step 7 snippet puts the reset check "after the effective params are computed and before `refill_bucket` runs" — but the merged loop opens with `if info.rp_ms <= 0 or not is_accrual_rate(info.ra_milli): continue`, and a quota's stored rate is 0 since ADR-137. That guard skips exactly the limits a reset exists for, so the plan's own Step 5 tests fail against its own Step 7 code. The reset is now evaluated **before** the accrual guard. Pinned by mutation: restoring the plan's ordering fails 9 tests. 2. **A reset edge crossed between the slow path's two clock readings was silently lost.** `_do_acquire` reads the clock before the config resolve and the `BatchGetItem`; `_commit_initial` takes a second reading a round trip later, and that is what stamps `rf`. An edge in that gap is invisible to `_apply_reset_edge` (which ran at the earlier instant), yet `rf` lands past it — so the *next* pass compares the edge against an `rf` already past it and skips it too, and the aggregator, reading the same `rf` off the stream image, skips it as well. A whole period's quota disappears with no error anywhere. Core Task 12's `vu <= rf` remedy ("one extra pass") is sufficient for a *level*-triggered parameter boundary and not for an *edge*-triggered reset. `_commit_initial` now re-expresses the delta from `LeaseEntry._reset_edge_ms`; it cannot double-apply, because the acquire path covers every edge at or before its own reading and `_reset_edge_ms` is strictly after it. 3. **The plan's `lease.py` `vu` snippet is stale.** It patches a `next_boundary(entry.limit.schedule, ...)` call "in lease.py"; on merged `main` that computation lives in `limiter.py` as `LeaseEntry._boundary_ms`. Handled there instead, via a helper that computes the parameter and reset halves separately so neither cron scan runs twice and the commit can tell them apart (defect 2 needs the reset half alone). 4. **The plan's cascade test does not reach the seam it claims.** `test_a_parent_only_cascade_acquire_also_resets` never enters `_try_parent_only_acquire`: both items' `vu` expire at the edge, and a boundary-expired parent is routed to the *full* slow path by design. Deleting the parent-only seam left it green. Split into an honest end-to-end cascade test plus a direct `_try_parent_only_acquire` test that does discriminate. ## Test plan - `uv run pytest tests/unit/ -q` → **4313 passed** (7:59) - `uv run pytest tests/unit/ -m gevent -n 0 -q` → **26 passed** - New: 21 client tests (`TestApplyResetEdge`, `TestResetMaterialisationThroughAcquire` in `tests/unit/test_limiter.py`) + their generated sync twins, 22 aggregator tests (`TestAggregatorAppliesResets`, `TestResetSchedIsCarriedFromTheStreamImage` in `tests/unit/test_processor.py`) - Every branch mutation-checked: 13 mutations run (reset call removed at each seam, `>` → `>=`, shard share → undivided capacity, `wcu` exemption removed, per-limit override ignored, reset tuple dropped from `vu` on both sides, the plan's aggregator ordering, the commit-time re-expression removed). Each fails at least one test; none is a no-op. - `hatch run generate-sync` clean, `git diff --exit-code` clean, `pre-commit` (ruff / ruff-format / mypy / sync verify) green. Refs #222 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01QdVj8nPhUwTz2aNJzMFqt5
9 tasks
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
3 participants
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.
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, #469, 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. Resetting by deleting the bucket item loses more than tokens:
total_consumed_milli, the counter the aggregator diffs to build usage snapshots — whichexists precisely because deriving consumption from token deltas was wrong (🐛 Snapshot aggregator fails to capture usage with high rate limits (10M+ TPM) #179).
disabledstamp.shard_count.Worse,
_commit_adjustments()and_rollback()are unconditionalADDs viawrite_each(), andADDon a missing item creates it. So any lease open across a reset leaves behind a skeletonitem with no
cp/ra/rp/rfand no GSI3 keys — which the speculative fast path then treats asa valid bucket.
Suggested alternative, at identical cost: keep the same resource-scoped GSI3 discovery, but per
shard issue
SET b_{l}_tk = :effective_cp, rf = :nowfor each resolved limit. The item neverstops existing, nothing is lost, and the next
acquire()stays on the fast path instead of payinga full slow-path recreate. The resource-scoped discovery helper itself is worth keeping either
way.
Summary
Repository.reset_bucket(entity_id, resource, principal=None) -> int, which deletes the bucket item(s) for(entity_id, resource)across every write-sharding shard (discovered via a resource-scoped_discover_entity_bucket_pksGSI3 query), so the nextacquire()recreates the bucket on the slow path at full capacity under whatever limits are configured now, withshard_countcollapsed back to 1. A missing bucket is a no-op (0, no error).SyncRepository.reset_bucket(...)from the async source (ADR-121) and add the method toRepositoryProtocol/SyncRepositoryProtocol.AuditAction.BUCKET_RESETaudit event (withentity_id,resource,buckets_deleted) on every reset.zae-limiter entity reset-bucket ENTITY_ID --resource RESOURCE [--namespace NS], following the existingentity disable/entity enablecommand shape, printing the number of buckets reset.CLAUDE.md,docs/cli.md,docs/guide/config-hierarchy.md,docs/infra/auditing.md, and rewrite thedocs/operations/rate-limits.mdrunbook (previously only deleted shard 0 by hand, which was incomplete under write sharding and left no audit trail).Test plan
tests/unit/test_repository.py(+ generatedtests/unit/test_sync_repository.py): no-op on missing bucket, deletion of all shards whenshard_count > 1, cascading child's reset leaves the parent bucket untouched — 9 tests passtests/unit/test_cli.py:entity reset-bucketCLI commandacquire()afterreset_bucket()observes full capacity rather than pre-reset consumption/debt — not yet added, tracked as follow-up if reviewers want it before mergeCloses #470
🤖 Generated with Claude Code
https://claude.ai/code/session_01Dnre1GeQwf423EPtZNKitp
Generated by Claude Code