Skip to content

feat(account): rotate on quota exhaustion (task 25)#129

Merged
mpiton merged 6 commits intomainfrom
feat/task-25-account-rotation-quota
Apr 29, 2026
Merged

feat(account): rotate on quota exhaustion (task 25)#129
mpiton merged 6 commits intomainfrom
feat/task-25-account-rotation-quota

Conversation

@mpiton
Copy link
Copy Markdown
Owner

@mpiton mpiton commented Apr 29, 2026

Summary

Implements automatic account rotation on quota exhaustion (HTTP 429 or traffic limit). When an account hits its quota limit, rotator marks it exhausted with a configurable TTL cooldown, excludes it from selection, and automatically picks the next best account per the selection strategy (BestTraffic/RoundRobin). Enables seamless fallback across premium/debrid/free accounts without manual intervention. Closes task 25, unblocks downstream tasks (38, 51-52, 55-56, 75-76).

Why

Multi-account orchestration requires intelligent failover. Task 24 (autoselector) picks the best account; task 25 (rotator) handles graceful degradation on quota loss. Together they enable:

  • Transparent account rotation on hoster rate limits
  • Cross-service isolation (MediaFire's cooldown doesn't affect Uploaded)
  • Traffic-aware refresh (clear exhaustion only when confirmed available)
  • Zero user intervention — download retries automatically with next account

Rationale for design: in-memory cooldown map (not persisted) because window is short (5-15 min) and upstream re-sends 429 anyway on retry. Exclusive deadline semantics for expiry (now >= deadline = available).

Changes

  • New module AccountRotator in src-tauri/src/application/services/account_rotator.rs (750+ lines):

    • NextAccountOutcome enum: Picked(Account) / NoneAvailable / AllExhausted { next_eligible_at_ms } with error_message() returning PRD-frozen wording
    • mark_exhausted(account_id, service_name, ttl_secs) — inserts cooldown deadline into map, publishes AccountExhausted event
    • next_account(service, strategy) — snapshots exhausted ids (lazy prune expired), calls selector.select_best_excluding(..., &exhausted_ids), distinguishes NoneAvailable (zero accounts exist) from AllExhausted (all on cooldown) by querying repo directly
    • record_traffic_refresh(account_id, traffic_left, threshold) — clears marker ONLY if traffic_left >= threshold (no flip-flop on unknown/below-threshold)
    • is_quota_signal(http_status, traffic_left, threshold) — returns true on 429 OR traffic < threshold (threshold exclusive at equality)
    • 22 unit tests covering quota detection, rotation, all-exhausted state, cooldown expiry (exclusive deadline), cross-service isolation, traffic refresh semantics, TTL edge cases
  • Modified AccountSelector:

    • New select_best_excluding(service, strategy, exclude: &[AccountId]) — identical to select_best() but filters out ids in exclude list
    • select_best() now delegates to select_best_excluding() with empty exclude list
    • 3 new tests: step-by-step filtering, all-excluded → no event, RoundRobin over non-excluded subset
  • Added AccountExhausted event variant to domain/event.rs:

    • Carries: id: AccountId, service_name: String, exhausted_until_ms: u64 (Unix epoch deadline)
  • Added transient exhausted_until: Option<u64> field to Account:

    • Methods: mark_exhausted(until_ms), clear_exhausted(), is_exhausted(now_ms) -> bool, exhausted_until() -> Option<u64>
    • Reset to None on reconstruct() (transient state not persisted)
    • 4 unit tests: new-no-exhaustion, reconstruct resets, mark/clear lifecycle, expiry boundary
  • Wired AccountRotator into CommandBus:

    • Builder: with_account_rotator(arc), accessor: account_rotator()
    • Initialized to None in new()
  • Updated adapter bridges:

    • tauri_bridge.rs: added AccountExhausted"account-exhausted" with JSON payload (camelCase)
    • download_log_bridge.rs: added AccountExhausted arm to exhaustive match (event not download-scoped, ignored)
  • Updated CHANGELOG.md:

    • Added comprehensive entry under [Unreleased] → Added section (270+ words)
    • Lists all 4 acceptance criteria with test names
    • Explains architecture choice (in-memory, not SQLite)
    • Documents all API methods and semantics

Testing

All tests green: 1172 lib tests, 0 regressions.

cargo test --lib account_rotator       # 22 rotator + 3 selector tests
cargo test --workspace                 # 1172 total

Acceptance criteria verified:

  • ✅ Quota detection (429 + traffic_left check) → rotation to next account — test_quota_detection_to_rotation_full_flow
  • ✅ All accounts exhausted → Error state with PRD-frozen message — test_next_account_returns_all_exhausted_when_every_candidate_is_marked + test_outcome_error_message_uses_prd_wording
  • ✅ Cooldown expiry after refresh confirms availability — test_record_traffic_refresh_clears_cooldown_when_confirms_available + edge cases (None, below-threshold stay marked)
  • ✅ Cross-service isolation tested — cooldown deadline for MediaFire doesn't leak into Uploaded decision

Additional coverage:

  • Exclusive deadline semantics (now >= deadline = available)
  • Lazy pruning of expired entries from map
  • TTL zero edge case (no-op)
  • Idempotent clear_exhausted
  • Traffic threshold boundary (exclusive at equality)

Related Issues

  • Closes task 25
  • Refs task 24 (autoselector — dependency)
  • Unblocks: tasks 38, 51, 52, 55, 56, 75, 76 (all dependent on account rotation)

Notes for Reviewer

Architecture notes:

  1. In-memory state — cooldown map lives in Mutex<HashMap> (not SQLite) because:

    • Cooldown window is short (5-15 min); loss on restart is acceptable
    • Upstream hoster re-sends 429 anyway on retry → no correctness gap
    • Avoids schema migration + background purge job
    • Simpler test story (no SQLite mocking)
  2. NoneAvailable vs AllExhausted distinction — After selector.select_best_excluding(...) returns Ok(None):

    • Call repo.list_by_service() directly (not from map) to check: enabled + not expired
    • Empty set → NoneAvailable (zero credentials exist)
    • Non-empty set → AllExhausted (rotation is blocker, not absence)
    • Extract deadline only from live accounts, not map (prevents cross-service leakage)
  3. Exclusive deadline semanticsnow >= deadline = available (not exhausted):

    • Clock doesn't drift; microsecond boundaries don't matter in practice
    • Matches intuition: "expires AT this time, so available AT this time"
    • Pinned with unit test test_mark_exhausted_records_deadline_and_flips_is_exhausted
  4. Traffic refresh only clears on confirmationrecord_traffic_refresh() clears marker ONLY if traffic_left >= threshold:

    • If traffic_left == None (unknown): leave marker untouched (can't confirm availability)
    • If traffic_left < threshold (below floor): leave marker untouched (still exhausted)
    • Prevents silent undo for hosters without traffic API; prevents flapping near threshold

Checklist

  • Tests added and passing (22 rotator + 3 selector = 25 new tests, all green)
  • Docs updated (CHANGELOG.md with comprehensive feature description)
  • No secrets, debug prints, commented code
  • Self-reviewed diff
  • CI ready (cargo build, cargo clippy -D warnings, cargo fmt --check all green)

Summary by cubic

Adds automatic account rotation when a hoster quota is hit and hardens race handling so only committed picks emit AccountSelected. Prevents false “no account”/“all exhausted” states, stale selections during races, and noisy telemetry. Fulfills task 25.

  • New Features

    • AccountRotator with mark_exhausted, next_account -> Picked | NoneAvailable | AllExhausted { next_eligible_at_ms }, and error_message(service).
    • Quota detection: is_quota_signal (429 or traffic_left below threshold). record_traffic_refresh only clears cooldown when traffic_left >= threshold.
    • New DomainEvent::AccountExhausted forwarded by the Tauri bridge as account-exhausted (download log ignores it).
    • Account gains transient exhausted_until (reset on reconstruct).
    • CommandBus wiring: with_account_rotator(...) and account_rotator().
  • Bug Fixes

    • Selector: select_best_excluding(service, strategy, exclude_ids); RoundRobin respects exclusions; emit NoAccountAvailable only when the pre-exclusion eligible set is empty.
    • New select_best_excluding_quiet(...) used by the rotator; rotator emits AccountSelected only for a pick that survives the under-lock recheck.
    • Rotator race-guards: re-check picked id under lock and retry with it excluded if a concurrent mark_exhausted lands; when selection returns none, re-snapshot cooldowns and retry if any id from the full current exclude list (including race-added ids) was just cleared, avoiding false AllExhausted.
    • Preserve longer cooldowns on re-mark: mark_exhausted keeps max(existing, proposed) and publishes the committed deadline to avoid shortening an active window.

Written for commit 2aa717b. Summary will update on new commits. Review in cubic

Summary by CodeRabbit

  • New Features

    • Quota-aware account rotation with cooldowns, earliest-deadline handling, automatic recovery and exhaustion notifications per service
    • Automatic account selection with exclusion-aware strategies, round-robin behavior and strict strategy validation
    • Accounts management UI: import/export, enable/disable, validate, edit, delete, IPC wiring and i18n
    • Server-side account queries returning secret-safe results and stable selection semantics
  • Tests

    • Extensive tests covering rotation, selection, cooldown/refresh behavior, events, and query shapes

PRD §6.4 / PRD-v2 §P1.6.

New `AccountRotator` application service detects quota exhaustion
(HTTP 429 or `traffic_left` below threshold via `is_quota_signal`),
pulls the offending account out of rotation for a hoster-specific
cooldown via `mark_exhausted`, and asks the existing `AccountSelector`
for the next best candidate via `next_account` returning a
`NextAccountOutcome` enum that distinguishes `Picked(Account)`,
`NoneAvailable`, and `AllExhausted { next_eligible_at_ms }` so callers
can decide between using a credential, falling back to the free path,
or stalling the download in `Waiting` until the earliest cooldown
expires.

`NextAccountOutcome::error_message(service_name)` returns the PRD-frozen
wording ("All accounts exhausted for {service}" / "No account available
for {service}") so the UI / log copy stays uniform across hosters.

Cooldown state lives in an in-memory `Mutex<HashMap<AccountId, u64>>`
(NOT persisted in SQLite — a restart wipes the cooldown, which is the
desired behaviour for the 5-15 minute hoster reset window). A poisoned
mutex surfaces as `AppError::Validation` to mirror
`AccountSelector::pick_round_robin`'s contract. Expired entries are
pruned lazily so no background sweeper is needed.
`record_traffic_refresh(account_id, traffic_left, threshold)` clears
the marker only when the upstream confirms `traffic_left >= threshold`
— a `None` or below-threshold observation leaves the marker in place
so a hoster without a traffic counter cannot silently undo every
`mark_exhausted`.

Selector gains `select_best_excluding(service, strategy, exclude_ids)`;
`select_best` is now a thin wrapper. New
`DomainEvent::AccountExhausted { id, service_name, exhausted_until_ms }`
forwarded by the Tauri bridge as `account-exhausted`. New transient
`Account::exhausted_until` field reset to `None` by `Account::reconstruct`
so the rotator's map remains the single source of truth across SQLite
roundtrips.

Twenty-two unit tests cover the four acceptance criteria plus edge
cases: zero-TTL no-op, deadline-exclusive equality, cross-service
deadline isolation, `None`-traffic refresh keeps cooldown, `404` /
`500` ignored by `is_quota_signal`, idempotent `clear_exhausted`,
lazy cooldown expiry surfaces an account back into rotation. Total
suite: 1172 lib tests + 1 integration test green.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 29, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a quota-aware AccountRotator and exclusion-aware AccountSelector, a new DomainEvent::AccountExhausted and transient exhausted state on Account, wires an optional rotator into CommandBus, and forwards the exhausted event to Tauri adapters and logging plumbing.

Changes

Cohort / File(s) Summary
Domain Model & Events
src-tauri/src/domain/event.rs, src-tauri/src/domain/model/account.rs
Add DomainEvent::AccountExhausted { id, service_name, exhausted_until_ms }; add transient exhausted_until: Option<u64> on Account with mark/clear/is_exhausted APIs and tests.
Account Rotation Service
src-tauri/src/application/services/account_rotator.rs
New AccountRotator and NextAccountOutcome (Picked/NoneAvailable/AllExhausted), mutex-protected cooldown map, selection retry/commit logic, mark_exhausted/clear_exhausted/is_exhausted, quota-signal and traffic-refresh helpers, event emission, and unit tests.
Account Selection Extensions
src-tauri/src/application/services/account_selector.rs, src-tauri/src/application/services/mod.rs
Add select_best_excluding and select_best_excluding_quiet; refactor select_best to delegate; change emission rules for NoAccountAvailable/AccountSelected; tests for exclusion and round-robin. Re-export AccountRotator from services mod.
Command Bus Wiring
src-tauri/src/application/command_bus.rs
Add optional account_rotator: Option<Arc<AccountRotator>> field, builder-style with_account_rotator, and accessor account_rotator().
Tauri Integration & Logging
src-tauri/src/adapters/driven/event/tauri_bridge.rs, src-tauri/src/adapters/driven/logging/download_log_bridge.rs
Map/serialize AccountExhausted to "account-exhausted" payload (id, serviceName, exhaustedUntilMs) and treat AccountExhausted as unlogged/no-op in download logging.
Changelog
CHANGELOG.md
Update Unreleased → Added with account rotation, account auto-selection, Accounts view UI, and Accounts queries.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Rotator as AccountRotator
    participant Selector as AccountSelector
    participant Repo as AccountRepository
    participant Event as EventBus

    Client->>Rotator: next_account(service, strategy)
    activate Rotator
    Rotator->>Rotator: prune expired cooldowns (mutex)
    Rotator->>Selector: select_best_excluding(service, strategy, exclude)
    activate Selector
    Selector->>Repo: fetch enabled, non-expired accounts
    Repo-->>Selector: candidates
    Selector->>Selector: apply exclusions & strategy
    Selector-->>Rotator: Option<Account>
    deactivate Selector

    alt Account Picked
        Rotator->>Rotator: re-check under lock & commit selection
        Rotator-->>Client: NextAccountOutcome::Picked(account)
    else All Exhausted
        Rotator->>Repo: re-scan live candidates for service
        Repo-->>Rotator: candidates
        Rotator->>Rotator: compute earliest exhausted_until_ms
        Rotator-->>Client: NextAccountOutcome::AllExhausted{next_eligible_at_ms}
    else None Available
        Rotator-->>Client: NextAccountOutcome::NoneAvailable
    end
    deactivate Rotator

    Client->>Rotator: mark_exhausted(account_id, until_ms)
    activate Rotator
    Rotator->>Rotator: store cooldown deadline
    Rotator->>Event: emit DomainEvent::AccountExhausted{id, service_name, exhausted_until_ms}
    Event-->>Client: event dispatched
    deactivate Rotator
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

🐇 I hopped where tokens dimmed and dried,

I left a flag where buckets cried,
I skipped the nests that counted low,
I tolled the bell when quotas slow,
Now I spin the wheel and off we glide.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and specifically summarizes the main feature: account rotation when quota is exhausted, with a clear reference to the task.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/task-25-account-rotation-quota

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added documentation Improvements or additions to documentation rust labels Apr 29, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 857c169859

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src-tauri/src/application/services/account_selector.rs Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src-tauri/src/application/services/account_selector.rs (1)

97-107: ⚠️ Potential issue | 🟠 Major

Do not emit NoAccountAvailable when exclusions are the only reason the set is empty.

Right now, excluding all otherwise-eligible accounts triggers NoAccountAvailable, which collapses “all exhausted” into “none configured/eligible” and can mislead downstream behavior in rotation flows.

Proposed fix
-        let eligible: Vec<&Account> = candidates
+        let base_eligible: Vec<&Account> = candidates
             .iter()
-            .filter(|a| {
-                a.is_enabled() && !a.is_expired(now_ms) && !exclude.iter().any(|id| id == a.id())
-            })
+            .filter(|a| a.is_enabled() && !a.is_expired(now_ms))
+            .collect();
+        let eligible: Vec<&Account> = base_eligible
+            .iter()
+            .copied()
+            .filter(|a| !exclude.iter().any(|id| id == a.id()))
             .collect();
         if eligible.is_empty() {
-            self.event_bus.publish(DomainEvent::NoAccountAvailable {
-                service_name: service_name.to_string(),
-            });
+            if base_eligible.is_empty() {
+                self.event_bus.publish(DomainEvent::NoAccountAvailable {
+                    service_name: service_name.to_string(),
+                });
+            }
             return Ok(None);
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src-tauri/src/application/services/account_selector.rs` around lines 97 -
107, The current logic emits DomainEvent::NoAccountAvailable whenever the final
filtered list `eligible` is empty, which wrongly fires when exclusions are the
only cause; change the check to first compute whether there are any candidates
that satisfy `is_enabled()` and `!is_expired(now_ms)` ignoring `exclude` (e.g.,
`available_without_exclude`) and only publish `NoAccountAvailable` if that list
is empty; if `available_without_exclude` is non-empty but `eligible` is empty,
return Ok(None) without publishing the event. Update the code paths around the
`eligible` variable and the
`self.event_bus.publish(DomainEvent::NoAccountAvailable { ... })` call
accordingly.
🧹 Nitpick comments (1)
src-tauri/src/adapters/driven/event/tauri_bridge.rs (1)

73-74: Add a dedicated bridge test for AccountExhausted mapping.

The mapping looks correct, but this new frontend contract should be pinned with an explicit event_name + payload assertion.

Also applies to: 216-226

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src-tauri/src/adapters/driven/event/tauri_bridge.rs` around lines 73 - 74,
Add a dedicated unit test that pins the new frontend contract for
DomainEvent::AccountExhausted: construct a DomainEvent::AccountExhausted with
representative fields, call the adapter function that maps DomainEvent to the
Tauri event (the function which produces the event name and payload in
tauri_bridge.rs), assert the returned event_name equals "account-exhausted", and
assert the serialized payload matches the expected JSON structure/fields. Place
the test alongside the existing bridge tests (around the other event tests
referenced at the 216-226 area) so future changes will fail if the name or
payload shape changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src-tauri/src/application/services/account_rotator.rs`:
- Around line 113-136: The current next_account flow (calls: snapshot_exhausted,
selector.select_best_excluding) releases the mutex between snapshot_exhausted
and returning a picked account, allowing a concurrent mark_exhausted to race;
after selector returns an Account from select_best_excluding, re-acquire the
same lock used by snapshot_exhausted, verify the chosen AccountId is still not
in snapshot_exhausted (or otherwise not marked exhausted), and if it is now
exhausted release the lock and retry the selection loop (or re-run
snapshot_exhausted → select_best_excluding) until you can confirm under the lock
that the picked account remains valid; ensure you still perform the existing
NoneAvailable / AllExhausted checks (repo.list_by_service,
earliest_deadline_for_service) only when no valid pick is found.

---

Outside diff comments:
In `@src-tauri/src/application/services/account_selector.rs`:
- Around line 97-107: The current logic emits DomainEvent::NoAccountAvailable
whenever the final filtered list `eligible` is empty, which wrongly fires when
exclusions are the only cause; change the check to first compute whether there
are any candidates that satisfy `is_enabled()` and `!is_expired(now_ms)`
ignoring `exclude` (e.g., `available_without_exclude`) and only publish
`NoAccountAvailable` if that list is empty; if `available_without_exclude` is
non-empty but `eligible` is empty, return Ok(None) without publishing the event.
Update the code paths around the `eligible` variable and the
`self.event_bus.publish(DomainEvent::NoAccountAvailable { ... })` call
accordingly.

---

Nitpick comments:
In `@src-tauri/src/adapters/driven/event/tauri_bridge.rs`:
- Around line 73-74: Add a dedicated unit test that pins the new frontend
contract for DomainEvent::AccountExhausted: construct a
DomainEvent::AccountExhausted with representative fields, call the adapter
function that maps DomainEvent to the Tauri event (the function which produces
the event name and payload in tauri_bridge.rs), assert the returned event_name
equals "account-exhausted", and assert the serialized payload matches the
expected JSON structure/fields. Place the test alongside the existing bridge
tests (around the other event tests referenced at the 216-226 area) so future
changes will fail if the name or payload shape changes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9a214db5-0ef9-401f-ba59-5b3e7f0dbdbf

📥 Commits

Reviewing files that changed from the base of the PR and between c551d53 and 857c169.

📒 Files selected for processing (9)
  • CHANGELOG.md
  • src-tauri/src/adapters/driven/event/tauri_bridge.rs
  • src-tauri/src/adapters/driven/logging/download_log_bridge.rs
  • src-tauri/src/application/command_bus.rs
  • src-tauri/src/application/services/account_rotator.rs
  • src-tauri/src/application/services/account_selector.rs
  • src-tauri/src/application/services/mod.rs
  • src-tauri/src/domain/event.rs
  • src-tauri/src/domain/model/account.rs

Comment thread src-tauri/src/application/services/account_rotator.rs
…otator pick

- Selector: only emit NoAccountAvailable when pre-exclude eligible set is
  empty. Rotator-driven exclusion of cooled-down accounts maps to
  AllExhausted upstream and must not collapse into "no account configured".
- Rotator: re-check picked AccountId under lock and retry with id added
  to exclude list when a parallel mark_exhausted lands in the
  snapshot/select gap.

Both findings raised by Codex + CodeRabbit on PR #129. Tests added/updated.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2ceeeffdbf

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src-tauri/src/application/services/account_rotator.rs Outdated
Selector gains `select_best_excluding_quiet` (no AccountSelected
emission). Rotator's retry loop calls the quiet variant and emits
AccountSelected itself once a pick survives the under-lock recheck.

Without this, a probe discarded by a parallel `mark_exhausted` would
have leaked an `AccountSelected` event for an account never returned
to the caller, polluting UI/telemetry. Found by Codex on PR #129.

Tests added:
- selector quiet variant must not publish AccountSelected
- rotator emits exactly one AccountSelected per Picked outcome
- rotator emits zero AccountSelected on NoneAvailable
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 92d640c806

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src-tauri/src/application/services/account_rotator.rs Outdated
If a parallel `clear_exhausted` / `record_traffic_refresh` lands while
`next_account` is probing the selector, the original snapshot becomes
stale. The loop now re-snapshots when the selector exhausts options
and retries with the fresh exclude list whenever any baseline id was
cleared. Bounded by the initial snapshot size so termination is
guaranteed.

Without this, a freshly-cleared account would be reported as
`AllExhausted` even though it became selectable mid-call, sending
callers down a needless waiting path. Found by Codex on PR #129.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7f8df6462b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +158 to +159
let any_cleared = snapshot_baseline.iter().any(|id| !fresh.contains(id));
if !any_cleared {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Recheck race-added exclusions before returning AllExhausted

next_account only checks whether IDs from snapshot_baseline were cleared, but IDs appended later via exhausted_ids.push(...) (after losing the mark_exhausted race) are ignored in this any_cleared test. If such an ID is cleared before the next probe, the loop breaks anyway and can return AllExhausted even though a live account is now selectable, causing unnecessary waiting/retry behavior. The retry decision should be based on the full current exclusion set (or a final probe with fresh exclusions), not just the initial baseline snapshot.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chatgpt-codex-connector

next_account only checks whether IDs from snapshot_baseline were cleared, but IDs appended later via exhausted_ids.push(...) (after losing the mark_exhausted race) are ignored in this any_cleared test.

Fixed in 80ffd11. snapshot_baseline is gone — any_cleared now diffs the fresh snapshot against the full current exhausted_ids set, which already contains both the initial snapshot and race-pushed entries. A clear of any of those triggers a retry before classifying AllExhausted.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src-tauri/src/application/services/account_rotator.rs`:
- Around line 153-164: The loop's clearing detection only compares fresh vs
snapshot_baseline and ignores IDs appended to exhausted_ids during the
race-retry path, so update the any_cleared check (in the account rotator loop
where snapshot_exhausted(now_ms) is called) to consider both snapshot_baseline
and exhausted_ids: compute any_cleared as true if any id in snapshot_baseline or
any id in exhausted_ids is no longer present in fresh (i.e., fresh.contains(id)
is false). Then proceed to update snapshot_baseline and exhausted_ids as before.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d8cf78a7-9bc2-4fad-acaa-659da6167e0c

📥 Commits

Reviewing files that changed from the base of the PR and between 92d640c and 7f8df64.

📒 Files selected for processing (1)
  • src-tauri/src/application/services/account_rotator.rs

Comment thread src-tauri/src/application/services/account_rotator.rs
`any_cleared` previously only diffed against the initial
`snapshot_baseline`. IDs appended to `exhausted_ids` during the
mark_exhausted race-retry path were ignored, so a parallel clear of
one of those IDs would silently fall through to `AllExhausted` while
a live account was actually selectable.

Compare against the full current exclude list instead. Drop the
separate `snapshot_baseline` since `exhausted_ids` already tracks
both the initial snapshot and race-pushed entries.

Found by Codex + CodeRabbit on PR #129.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src-tauri/src/application/services/account_rotator.rs`:
- Around line 191-200: The current insert into the exhausted map unconditionally
overwrites any existing deadline; change the logic in the block around
self.lock_exhausted(), now_ms, ttl_secs and until_ms to read the current
deadline (if any) for account_id, compute committed_until =
max(existing_deadline, until_ms), store committed_until back into the guard/map
rather than unconditionally inserting until_ms, and then pass committed_until
(not until_ms) into self.event_bus.publish(DomainEvent::AccountExhausted { id:
account_id.clone(), service_name: service_name.to_string(), exhausted_until_ms:
... }) so the published event reflects the actual preserved longest cooldown.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 76b65a93-3bd3-4c0b-a917-333adff688d3

📥 Commits

Reviewing files that changed from the base of the PR and between 7f8df64 and 80ffd11.

📒 Files selected for processing (1)
  • src-tauri/src/application/services/account_rotator.rs

Comment thread src-tauri/src/application/services/account_rotator.rs Outdated
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 80ffd11585

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +124 to +125
pub fn with_account_rotator(mut self, rotator: Arc<AccountRotator>) -> Self {
self.account_rotator = Some(rotator);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Route account resolution through rotator when configured

with_account_rotator stores the rotator but no runtime path ever consumes it, so quota-based rotation is effectively disabled even when this setter is used. In this tree, resolve_account_for still selects directly via account_selector, and a repo-wide search shows account_rotator() is never called outside its own definition, so callers will never see AllExhausted/cooldown-aware behavior and will keep using the pre-rotation selection path.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chatgpt-codex-connector

with_account_rotator stores the rotator but no runtime path ever consumes it, so quota-based rotation is effectively disabled even when this setter is used.

Out of scope for task 25. This PR ships the rotator API surface plus its CommandBus builder hook; consuming it from resolve_account_for and the download retry path is downstream work tracked under tasks 38 / 51 / 52 / 55 / 56 (the first hoster plugins that actually need rotation). Wiring now would require speculative integration ahead of the consumer code that defines the retry contract — kept separate intentionally so each plugin task lands its own integration test rather than relying on synthetic glue.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +157 to +159
let any_cleared = exhausted_ids.iter().any(|id| !fresh.contains(id));
if !any_cleared {
break;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Re-probe after resnapshot before returning AllExhausted

The any_cleared decision is made from a single fresh snapshot and immediately breaks on false, but a concurrent clear_exhausted/record_traffic_refresh can clear an id right after snapshot_exhausted returns and before this check, causing a false AllExhausted classification while an account is now selectable. This race makes callers enter a wait/retry path unnecessarily; a final probe with the latest exclusions (or a second under-lock check) is needed before breaking.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chatgpt-codex-connector

The any_cleared decision is made from a single fresh snapshot and immediately breaks on false, but a concurrent clear_exhausted/record_traffic_refresh can clear an id right after snapshot_exhausted returns and before this check, causing a false AllExhausted classification.

Not a bug in practice. The window between snapshot_exhausted returning and the any_cleared boolean being computed is two synchronous statements with no I/O — microseconds. Closing it fully would require holding lock_exhausted across the selector probe (which does repo I/O), which is forbidden by the rotator's concurrency contract.

The classification is best-effort: callers treat AllExhausted { next_eligible_at_ms } as "wait until at least this deadline before retrying", and any clear that landed inside the microsecond gap is observed on the next call. Under the current cooldown windows (5–15 min for typical hoster quotas) the worst-case cost of a phantom AllExhausted is one no-op retry on the caller's side.

What would change the verdict: a measurement showing the gap inflates beyond the intended scheduler tick (e.g. due to allocator stalls under load), or a use case where the caller can't poll on next_eligible_at_ms and instead trusts the classification absolutely.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`mark_exhausted` previously overwrote the existing deadline. A short
retry-driven TTL (e.g. 60s) landing on top of a daily-cap cooldown
(e.g. 600s) would shrink the window and put the account back into
rotation early. Keep `max(existing, proposed)` and publish that
committed value in `AccountExhausted` so subscribers see the active
deadline, not a phantom shorter one.

Found by CodeRabbit on PR #129. Test
`test_mark_exhausted_keeps_existing_longer_deadline` pins the
behaviour through clock advance.
@mpiton mpiton merged commit 96fe2fe into main Apr 29, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation rust

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant