fix stale account pool penalties after successful requests

[ndycode]

Summary

• heal stale account-pool penalties after a real successful request so healthy accounts do not carry expired auth-failure state forward

What Changed

• updated AccountManager.recordSuccess(...) in lib/accounts.ts to clear stale auth-failure counters and expired cooldown metadata when an account succeeds again
• persist the healed state with saveToDiskDebounced()
• added regression coverage in test/accounts.test.ts to prove success heals stale state without clearing an active newer cooldown

Validation

• npm run lint
• npm run typecheck
• npm test
• npm test -- test/documentation.test.ts
• npm run build

Docs and Governance Checklist

• README updated (if user-visible behavior changed)
• docs/getting-started.md updated (if onboarding flow changed)
• docs/features.md updated (if capability surface changed)
• relevant docs/reference/* pages updated (if commands/settings/paths changed)
• docs/upgrade.md updated (if migration behavior changed)
• SECURITY.md and CONTRIBUTING.md reviewed for alignment

Risk and Rollback

• Risk level: low to moderate; limited to success-path healing of persisted account state
• Rollback plan: revert commit adbf49a

Additional Notes

• implemented from an isolated worktree based on current origin/main

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

this pr heals stale account-pool penalty state (expired cooldown metadata and consecutiveAuthFailures) on the success path in AccountManager.recordSuccess, then persists the cleaned state via saveToDiskDebounced. the previous orphaned-cooldownReason concern is resolved: the new code calls clearAccountCooldown explicitly in the hadCooldownMetadata branch rather than relying on isAccountCoolingDown's internal side effect, so a stored account with only cooldownReason set (and coolingDownUntil absent) will also be cleaned correctly.

• lib/accounts.ts: snapshots hadCooldownMetadata and hadAuthFailures before calling isAccountCoolingDown; both healing branches are guarded by !isCoolingDown to preserve an active newer cooldown
• test/accounts.test.ts: three new regression cases — full heal, orphaned-reason-only heal, and active-cooldown guard; all verify persist behavior via flushPendingSave + mockSaveAccounts
• minor: post-construction account.consecutiveAuthFailures = 2 mutation in test cases 1 and 3 is redundant since the value is already present in the stored fixture; no isolated test exists for the consecutiveAuthFailures > 0 / no-cooldown-metadata-at-all path, though the risk is low given the branches are independent

Confidence Score: 5/5

safe to merge — logic is correct, previous concern is addressed, no P0/P1 issues found

all remaining findings are P2 style/test-hygiene only. core healing logic is sound, guard against active cooldowns is correct, debounced persist coalesces concurrent heals safely, and the orphaned-cooldownReason gap from the prior review thread is explicitly closed by the direct clearAccountCooldown call.

test/accounts.test.ts — redundant post-construction mutation on lines 1929 and 2000, and missing isolated test for pure auth-failure healing path

Important Files Changed

Filename	Overview
lib/accounts.ts	adds stale-penalty healing to recordSuccess: explicit clearAccountCooldown + clearAuthFailures guarded by isCoolingDown, then debounced persist. orphaned-cooldownReason concern from previous review is resolved by the direct clearAccountCooldown call in the hadCooldownMetadata branch.
test/accounts.test.ts	three new cases cover: full heal (expired cooldown + auth failures), orphaned-reason-only heal, and active-cooldown guard. minor: post-construction consecutiveAuthFailures mutation in cases 1 and 3 is redundant since the value is already present in stored; no isolated test for auth-failure-only healing with zero cooldown metadata.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[recordSuccess called] --> B[snapshot hadCooldownMetadata\nhadAuthFailures]
    B --> C[isAccountCoolingDown]
    C -->|coolingDownUntil future| D[isCoolingDown = true]
    C -->|coolingDownUntil expired\nside-effect: clearAccountCooldown| E[isCoolingDown = false]
    C -->|coolingDownUntil undefined| E
    D --> F[skip all healing\nhealed = false]
    E --> G{hadCooldownMetadata?}
    G -->|yes| H[clearAccountCooldown\nhealed = true]
    G -->|no| I{hadAuthFailures?}
    H --> I
    I -->|yes| J[clearAuthFailures\nhealed = true]
    I -->|no| K{healed?}
    J --> K
    K -->|yes| L[saveToDiskDebounced]
    K -->|no| M[no persist]
    F --> M

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: test/accounts.test.ts
Line: 1929

Comment:
**redundant post-construction mutation**

`stored.accounts[0].consecutiveAuthFailures` is already `2`, so `AccountManager` will deserialize that value onto the live account object. the assignment on line 1929 is a no-op and could mislead a future reader into thinking the value isn't inherited from storage.

```suggestion
      // consecutiveAuthFailures: 2 already comes from stored — no override needed
```

same pattern appears on line 2000 in the active-cooldown test.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (2): Last reviewed commit: "fix: heal stale cooldown metadata on suc..." | Re-trigger Greptile}

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[coderabbitai]

📝 Walkthrough

Walkthrough

added state-healing logic to AccountManager.recordSuccess that conditionally persists cleared auth failures and stale cooldown metadata to disk when an account is no longer cooling down. two tests verify behavior for expired versus active cooldown scenarios.

Changes

Cohort / File(s)	Summary
Account state healing lib/accounts.ts	recordSuccess now checks if account has stale cooldown metadata or prior auth failures. if not currently cooling down, it clears failed auth counts and cooldown markers, then triggers saveToDiskDebounced() to persist healed state.
Healing behavior tests test/accounts.test.ts	Two tests added: first verifies stale cooldown metadata with auth failures resets state and saves once; second confirms active cooldowns remain untouched and don't trigger saves.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Issues to consider

• missing edge case coverage: no test for windows filesystem timing on debounced save—race conditions between saveToDiskDebounced() calls and concurrent account checks could leak stale state. should verify debounce timing under contention.
• concurrency gap: recordSuccess at lib/accounts.ts mutates account state and queues a save, but no locking prevents simultaneous updates. if multiple auth attempts fire close together, concurrent healing + save logic could corrupt state.
• regression test gap: tests cover the two happy paths but miss: what happens if clearAuthFailures() throws? does saveToDiskDebounced() get called in error paths? test at test/accounts.test.ts:line should verify failure handling.

Suggested labels

bug

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	title follows conventional commits format with type 'fix', lowercase imperative summary, and is 58 characters—well within the 72-character limit.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check	✅ Passed	pull request follows template structure with complete summary, what changed, validation checklist (all items checked), and risk/rollback sections. docs checklist items appropriately marked as not applicable.

_{✏️ 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 fix/account-pool-success-healing

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch fix/account-pool-success-healing

---

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
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/accounts.ts`:
- Around line 559-561: The branch that sets healed currently leaves
cooldownReason dangling and relies on later stale-timestamp cleanup; modify the
branch where (!isCoolingDown && hadCooldownMetadata) { healed = true; } to also
call clearAccountCooldown(account) so both cooldownReason and coolingDownUntil
are normalized before save; update/extend tests (test/accounts.test.ts around
the existing expired-timestamp cases) to add a reason-only cooldown case
asserting cooldown fields are cleared, and ensure the change follows lib/**
guidelines (cite the updated test IDs) and that any queue or save path respects
EBUSY/429 retry behavior and Windows file-IO concurrency expectations.

In `@test/accounts.test.ts`:
- Around line 1906-1936: The test currently only asserts that
saveToDiskDebounced was called; instead call manager.recordSuccess(...) then
await manager.flushPendingSave() and assert the mocked saveAccounts payload
contains the normalized/healed account snapshot (consecutiveAuthFailures === 0
and no coolingDownUntil/cooldownReason) to verify the persisted state; do the
same for the active-cooldown scenario (call recordSuccess, await
flushPendingSave, and assert saveAccounts payload was left unchanged). Reference
AccountManager, recordSuccess, flushPendingSave, saveToDiskDebounced, and
saveAccounts when locating and updating the assertions.

🪄 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: ASSERTIVE

Plan: Pro

Run ID: a2b1b79b-d87c-411f-82e0-500d8ea772f9

📥 Commits

Reviewing files that changed from the base of the PR and between 53488ef and adbf49a.

📒 Files selected for processing (2)

• lib/accounts.ts
• test/accounts.test.ts

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Greptile Review

🧰 Additional context used

📓 Path-based instructions (2)

lib/**

⚙️ CodeRabbit configuration file

focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.

Files:

• lib/accounts.ts

test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

• test/accounts.test.ts