Skip to content

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

Merged
sodre merged 10 commits into
mainfrom
docs/222-scheduled-limits-design
Sep 14, 2026
Merged

sodre merged 10 commits into
mainfrom
docs/222-scheduled-limits-design

Conversation

@sodre

@sodre sodre commented Sep 14, 2026

Copy link
Copy Markdown
Member

Summary

Docs-only. Four commits adding the settled design for scheduled (cron) limits and the three implementation plans it decomposes into, all under docs/plans/.

Caveats recorded in the plans themselves

  • Each plan ends with a Self-Review section that names exactly which tasks still need expanding against the then-current code, rather than inventing bodies now. Most notably the surface plan's Tasks 3–11, which depend on signatures the core plan has yet to produce (encode, _sync_bucket_params's final shape, LimitStatus construction) and on CFN/CLI code not yet read line by line. Expansion happens against post-♻️ Make Repository._now_ms() the single injectable clock seam #430 code — ♻️ Make Repository._now_ms() the single injectable clock seam #430 (Repository._now_ms() as the single injectable clock seam) is a hard prerequisite for testing anything time-varying, and is now milestoned v0.14.0 alongside this work.
  • Notable findings folded into the plans: config items store whole tokens while bucket items store millitokens (a 1000x conversion the provisioner must make); manifest.py validates nothing but namespace, so capacity: 0 is accepted today and would newly crash inside the Lambda; refill_amounts is already a delta, so the slow path clamps for free once refill_bucket does.

Test plan

  • Docs-only — no code changes, no source or test files touched (git diff --stat main...HEAD is four files, all docs/plans/*.md)
  • No CI impact beyond markdown; lint, type check and the test suite are unaffected
  • Design and plans read for internal consistency (cross-plan ordering, spec-section coverage, type signatures across plan boundaries)

Refs #222 — design and plans only; #222 stays open until the three plans land.
Scopes #481. Subsumes #469, #471, #473 (closed on implementation, not by this PR).
Blocked on #430.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QdVj8nPhUwTz2aNJzMFqt5

sodre and others added 4 commits September 13, 2026 18:30
Sections 0-1 are settled and approved: schedule rides on Limit as
ScheduleEntry(cron, tz, scale|absolute), stored as l_{name}_sched on
config items and b_{name}_sched on bucket items; cron is a match
pattern with first match winning; per-entry IANA tz defaulting to UTC;
no inheritance across config levels. Section 2 records the decided
shape — a vu (valid-until) stamp so the speculative fast path never
evaluates a schedule — plus the composition rule for the rest of
v1.3.0 and eight open questions to resume from.

Written now because the design was interrupted mid-brainstorm and the
decisions lived only in a session transcript.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QdVj8nPhUwTz2aNJzMFqt5
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
The provisioner's _apply_set never syncs bucket params, so `limits apply`
writes config that never reaches live buckets. It now lands first, as a
standalone commit before any scheduling code: it predates #222, fixes
plain limit numbers as well as schedules, and is testable on its own.

Scope is entity-level only — resource and system defaults deliberately
rely on bucket TTL (#271, #296), and the mirror inherits that rule.

Also records the matching known limitation: a resource- or system-level
schedule change reaches existing buckets only on TTL expiry, since a vu
boundary re-materialises from the item's own stamped sched.

Refs #222, #468, #271, #296

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
@sodre sodre added this to the v0.14.0 milestone Sep 14, 2026
@sodre sodre added api-design API surface changes area/limiter Core rate limiting logic labels Sep 14, 2026
@codecov

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.42%. Comparing base (d6bd456) to head (20effd8).
⚠️ Report is 9 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #482      +/-   ##
==========================================
- Coverage   93.47%   93.42%   -0.06%     
==========================================
  Files          47       47              
  Lines        9562     9562              
==========================================
- Hits         8938     8933       -5     
- Misses        624      629       +5     
Flag Coverage Δ
doctest 30.50% <ø> (ø)

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.

mkdocs.yml set neither docs_dir nor exclude_docs and loads no exclusion
plugin, so MkDocs built every markdown file under docs/ whether or not
it appeared in `nav`. docs/plans/ was therefore live and search-indexed
on the public site — design docs and implementation plans presented
alongside user documentation.

Verified: site/plans/ no longer builds, site/guide/ is unaffected,
`mkdocs build --strict` still passes.

This PR adds four more files under docs/plans/, so the exclusion belongs
with them.

Refs #222
#430 gives one acquire() one clock on the FAST path only; _do_acquire
and _try_parent_only_acquire keep their own reading, which is correct —
inheriting the fast path's instant across a BatchGetItem and a
transaction would stamp rf in the past and under-refill.

What Task 12 must guarantee is internal consistency: one reading drives
effective_params, next_boundary and the rf stamp.

Also records a non-hazard so it is not re-derived: a boundary crossed
between the fast-path rejection and the slow-path read is harmless,
because the slow path reads later and so evaluates the new window.

Refs #222, #430
@sodre
sodre marked this pull request as ready for review September 14, 2026 03:52
sodre and others added 4 commits September 14, 2026 00:29
Found by the code review on #484. config_cache.py:99/:103 still call
time.time(), so advancing _now_ms to cross a schedule boundary returns
pre-boundary limits — the 60s cache TTL has not expired in real time.

Costs a confusing debugging session in exactly the tasks that test
boundary crossings (core 11-13, surface 3), so it belongs in the plan's
Global Constraints rather than being rediscovered there.

Refs #222, #430
Core plan Tasks 8-14 and surface plan Tasks 6-9 now carry full TDD
cycles with real fixture setup, verified against the tree rather than
recalled. Surface Tasks 3, 4, 5, 10 and 11 stay compressed and each
carries an explicit note to expand it once schedule.py exists — writing
detailed code against invented signatures is the mistake these plans'
own reviews call out.

Ten factual errors corrected along the way:

- BucketState requires entity_id/resource/limit_name; every construction
  in the old Tasks 9-10 omitted two and would have raised TypeError
- BucketRefillState requires namespace_id; Task 7's three constructions
  omitted it (fixed in place though Task 7 was nominally complete)
- Task 11 called self._now_ms() inside the condition build, which would
  re-introduce the reading #430 just removed and break TestClockSeam
- capacity_counter is a moto/sync fixture needing sync_limiter, with no
  .rcu/.wcu/.reset(); Task 13's assertion is now in test_capacity.py
  using capacity_counter.counting() and the real counters
- the Lambda builders return bytes, not paths; packaging assertions now
  go through zipfile.namelist()
- unit tests use the moto `repo` fixture, not `test_repo`
- differ.py compares nothing, so surface Task 6's change-detection tests
  were false by construction; it now pins carry-through in Change.data
- the CFN generator is a Click command over raw dicts, not
  generate_cfn_template(manifest); Task 7 targets _limits_to_cfn and
  _cfn_limits_to_manifest and parses the emitted YAML
- CLI tests mock Repository, and get_entity_disabled must be stubbed
- Task 14's :rd_rpm == 500_000 is now traced rather than flagged, and
  the unscheduled control case is shown to discriminate

Boundary tests now patch Repository._now_ms following TestClockSeam's
_counting_clock; none sleep or patch global time.

Refs #222, #430, #481, #487

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QdVj8nPhUwTz2aNJzMFqt5
The plan shipped as #485 (d22199b). Annotating it as the record of how
the fix was specified rather than pending work, and naming the gap it
left: discovery is scoped `BUCKET#{resource}#`, so an entity-wide
`_default_` change matches no bucket and silently does nothing.

Also records why the obvious fix is wrong — an unscoped query
over-applies against the config precedence and would clobber
resource-specific entity configs — so a reader of this plan does not
"correct" it into a worse bug.

Refs #481, #487
@sodre
sodre merged commit 836fe18 into main Sep 14, 2026
8 of 9 checks passed
@sodre
sodre deleted the docs/222-scheduled-limits-design branch September 14, 2026 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-design API surface changes area/limiter Core rate limiting logic

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant