Originally created by: Akarsh-Hegde
src/llm/reset_time.rs parses that (absolute clock time or relative duration) and the resolver now waits exactly that long instead of guessing.RATE_LIMIT_BACKOFF (30 min) when the message doesn't match a recognised shape — e.g. Codex's weekly-limit message names a full date, which is deliberately out of scope.Observed live: hitting the Claude Code rate limit around 3:45pm with a reset at 4:20pm meant Meridian's worklog pipeline (which now routes through the user's own claude CLI subscription when llm_provider = "claude") was stuck on a flat 30-minute backoff — either retrying too early (still rate-limited) or waiting longer than necessary after the window actually reopened. This makes the backoff track the provider's real window.
cargo fmt / cargo clippy -- -D warnings cleancargo test — 9 new unit tests in reset_time.rs covering absolute/relative/ambiguous/past-rollover/cap/unparseable cases, plus updated resolver.rs tests for the new start_backoff(Duration) signature🤖 Generated with Claude Code
Originally posted by: coderabbitai[bot]
✨ Finishing Touches
🧪 Generate unit tests (beta)
- [ ] Create PR with unit tests - [ ] Commit unit tests in branch `feat/llm-rate-limit-reset-retry`Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
❤️ Share
- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)Comment
@coderabbitai helpto get the list of available commands.Originally posted by: adityaharishch
Review: [#452] — honor the provider's own rate-limit reset time
Overview
Adds
src/llm/reset_time.rs, a small parser that extracts a concrete wait duration from a rate-limit error message (absolute clock time like "resets 3pm", or relative like "try again in 3h 42m"), and wires it intoresolver.rs'sstart_backoffso the in-memory backoff tracks the provider's real reset window instead of a flat 30-minute guess. Falls back to the existingRATE_LIMIT_BACKOFFwhen nothing parses (e.g. Codex's weekly-limit message, which names a full date and is deliberately out of scope). Adds a 60s margin and a 6h cap to every parsed wait.Well-scoped, well-documented (module doc explains the ambiguity-resolution rule for bare hours), and backed by 12 new unit tests covering real observed strings from Claude Code, Codex, and GitHub Copilot, plus mechanics-level edge cases (past-time rollover, ambiguous bare hour, cap, unparseable).
Correctness
parse_relative/parse_absolute/take_number/parse_number_unit) is careful and each branch is exercised by a test with a real-world string. Traced through several by hand (bare-pm rollover, "It resets at" phrasing, compact3h 42m, spelled-out "2 hours 15 minutes") — all check out.parse_absoluteresolves the clock time usingchrono::Localat the call site (resolver.rspasseschrono::Local::now()), but ignores any timezone the message itself states (e.g."resets 10:40pm (Asia/Calcutta)"). This is fine as long as the machine's system timezone always matches the CLI's own local time (true today since the summariser runs on the user's own machine), but it's an implicit assumption that isn't documented inreset_time.rs's doc comment — worth a one-line note so a future reader doesn't assume the(Zone)suffix is actually being honored.parse_number_unitcomputesamount * 3600/amount * 60with plainu64multiplication on a value parsed straight out of the provider's message text. A pathological/garbled string with a very large digit run (parses fine intou64but overflows on* 3600) would panic in a debug/test build (attempt to multiply with overflow) rather than gracefully falling back like every other unparseable shape does. Since this is real subprocess-stderr text and the project's convention is to validate at system boundaries, considerchecked_mul/saturating_mulhere for defense in depth — low likelihood, cheap fix.Code quality / conventions
//!doc with# Relatedsection, doc comments on everypub fn,#[cfg(test)]unit tests in-module.resolver.rs's doc updates are consistent with the behavior change (backoff table entry, new "How long the backoff actually lasts" section).reset_timeDB-free/pure (easy to unit test, nochrono::Local::now()baked in —nowis a parameter).Test coverage
"resets 15:00"wherehour > 12) — the code path exists (None => vec![hour]incandidate_hours) but isn't exercised. Not a bug, just uncovered.resolver.rstests were mechanically updated for the newstart_backoff(Duration)signature — no new resolver-level test asserts that a parsed (non-default) duration actually gets threaded throughcomplete_innerend-to-end; coverage there stays at thereset_timeunit level. Given the size of the change this is a reasonable trade-off, just noting the gap.Other
feat/llm-provider-enum, notpre-main— presumably intentional stacking on the not-yet-merged provider-enum work rather than an oversight, but flagging since the repo's CLAUDE.md states all feature/fix PRs targetpre-main. Worth retargeting once the base lands.Verdict
Solid, well-tested, narrowly-scoped change. Nothing blocking; the two correctness notes (timezone assumption, multiplication overflow) are worth a small follow-up but don't need to hold up the merge.
Related
Tickets:
#452Originally posted by: Akarsh-Hegde
Addressed the review in f130fcb1:
reset_time.rs's module doc explaining that a parsed absolute time trusts the machine's ownchrono::Localclock and never reads the zone the provider prints (e.g. "(Asia/Calcutta)"), and why that's safe here (every backend is a subprocess on this same machine, so the CLI's own rendering andLocalcan never disagree).parse_number_unitnow useschecked_mul/checked_addinstead of*/+=on the parsed digit run. A pathological string that fits inu64but overflows once scaled to seconds now degrades toNone(the caller's flat backoff) instead of panicking. Added a regression test (a_pathological_digit_run_does_not_panic).a_24h_format_hour_outside_1_12_is_unambiguous, covering the previously-untestedhour > 12/ no-am-pm branch ("resets 14:30").Two points from the review I left as-is, since you flagged them as non-blocking/acceptable yourselves:
feat/llm-provider-enuminstead ofpre-main) — intentional stacking on the not-yet-merged provider-enum work, per direction at the start of this thread. Will retarget once that branch lands onpre-main.complete_inner— agreed this is a reasonable trade-off given the size of the change; coverage stays at thereset_timeunit level (12+ tests now, each pinned to a real observed message per provider).546 tests passing, fmt/clippy/UI build/UI tests/security audit all clean on push.
Ticket changed by: Akarsh-Hegde