Skip to content

fix: bounded retry with exponential backoff for 403 throttle responses#1872

Merged
aaunario-keeper merged 1 commit intoKeeper-Security:releasefrom
jlima8900:fix/throttle-retry-backoff
Mar 20, 2026
Merged

fix: bounded retry with exponential backoff for 403 throttle responses#1872
aaunario-keeper merged 1 commit intoKeeper-Security:releasefrom
jlima8900:fix/throttle-retry-backoff

Conversation

@jlima8900
Copy link

Summary

execute_rest() retries 403 throttled responses every 10 seconds with no maximum retry count. This causes Commander to hang indefinitely when throttled, and the fixed 10-second interval prevents the server's cooldown from expiring — creating a permanent throttle lock-in.

Changes

  • Add max retry count (3 attempts) before raising KeeperApiError
  • Parse the server's "try again in X minutes/seconds" message to determine wait time
  • Use exponential backoff (30s, 60s, 120s) capped at the server's suggested cooldown
  • Log throttle attempts as WARNING instead of DEBUG for visibility
  • After max retries, raise KeeperApiError so callers can handle it programmatically

Behavior

Before After
Retries every 10s forever 3 retries with exponential backoff, then error
DEBUG log only WARNING log with attempt count
10s interval resets server's 1-min cooldown Respects server's indicated wait time
No way for callers to handle throttle Raises KeeperApiError after max retries

The --fail-on-throttle flag continues to work as before (immediate error, no retries).

Test plan

  • Verify normal (non-throttled) requests are unaffected
  • Verify throttled requests retry 3 times with increasing delay
  • Verify KeeperApiError is raised after max retries
  • Verify --fail-on-throttle still skips retries entirely
  • Verify server's "try again in X" message is parsed correctly

jlima8900 added a commit to jlima8900/Commander that referenced this pull request Mar 14, 2026
…ates

Validated test suite scans 445 Commander Python files across 10 categories.
Two-pass validation eliminates 96% false positives.

Confirmed findings:
- KC-001: AWS creds written without chmod 600 (awskey plugin)
- KC-002: SSH plugin logs passwd command output (5 instances)
- KC-003: AD plugin logs LDAP connection results (4 instances)
- KC-004: Record edit logs password operation details
- KC-005: Example credentials in help text

Not confirmed: SQL injection (parameterized), subprocess injection
(internal input), unbounded retry (fixed by PR Keeper-Security#1872).
@craiglurey craiglurey changed the base branch from master to release March 20, 2026 05:52
@aaunario-keeper
Copy link
Contributor

aaunario-keeper commented Mar 20, 2026

can we add a set of unit tests for this feature (verifying exactly what the test plan in your summary outlines?
For the execute_rest() throttle logic in rest_api.py, you'd test it with mocks in Python using unittest.mock:
The core idea: mock requests to return a fake 403 response, and mock time.sleep so tests don't actually wait.

@aaunario-keeper
Copy link
Contributor

Also, can you squash your commits, this should only be 1 commit so it doesn't clutter up commit history

@jlima8900 jlima8900 force-pushed the fix/throttle-retry-backoff branch 2 times, most recently from d991c9c to 4068d53 Compare March 20, 2026 16:52
The execute_rest() function previously retried throttled (403) responses
every 10 seconds with no maximum retry count. This caused Commander to
hang indefinitely when throttled, and the 10-second retry interval
prevented the server's cooldown timer from expiring.

Changes:
- Add max retry count (3 attempts) before raising KeeperApiError
- Parse the server's "try again in X minutes/seconds" message
- Use exponential backoff (30s, 60s, 120s) capped at server's suggestion
- Cap server wait time at 300s to prevent excessive delays
- Log throttle attempts as warnings instead of debug
- After max retries, raise KeeperApiError so callers can handle it

The --fail-on-throttle flag continues to work as before (immediate error).

Unit tests (9 cases):
- Normal request unaffected by throttle logic
- Throttle twice then succeed (backoff 30s, 60s)
- KeeperApiError raised after 3 retries
- --fail-on-throttle skips retries entirely
- Parses "try again in X seconds" correctly
- Parses "try again in X minutes" correctly
- Caps server wait at 300s
- Exponential backoff progression (30s, 60s, 120s)
- Missing message defaults to 60s
@jlima8900 jlima8900 force-pushed the fix/throttle-retry-backoff branch from 4068d53 to 93d9bf1 Compare March 20, 2026 19:58
@aaunario-keeper aaunario-keeper merged commit 5fbb614 into Keeper-Security:release Mar 20, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants