Skip to content

fix: three correctness bugs (withRetry delay, history log label, cron null-deref) - #205

Closed
dylanneve1 wants to merge 2 commits into
mainfrom
claude/eager-sagan-D4qs9
Closed

fix: three correctness bugs (withRetry delay, history log label, cron null-deref)#205
dylanneve1 wants to merge 2 commits into
mainfrom
claude/eager-sagan-D4qs9

Conversation

@dylanneve1

Copy link
Copy Markdown
Owner

Summary

Deep code review of the full repository surfaced three correctness bugs:

  • src/core/gateway.ts β€” withRetry silently ignored rate-limit retry-after hints (HIGH)
    delayMs was computed from classified.retryAfterMs and logged, but p-retry's own minTimeout: 1000 / factor: 2 backoff was the actual wait β€” capping delays at 1–2 s regardless of what the API instructed (often 60 s for rate limits). Fixed by inserting await sleep(delayMs) before rethrowing and setting p-retry's own timeout to 0 to avoid double-waiting.

  • src/storage/history.ts β€” wrong log label in corrupt-backup error path (LOW)
    loadHistory emitted logError("sessions", ...) instead of logError("history", ...) when both the primary store and its backup were unreadable β€” a copy-paste error from sessions.ts. Fixed.

  • src/storage/cron-store.ts β€” unsafe Date cast on Cron#nextRun() (MEDIUM)
    validateCronExpression did (nextDate as Date).toISOString() without checking for null. Cron#nextRun() returns Date | null (e.g. for schedules whose window has already passed), so this throws a TypeError at runtime. Fixed with a null guard.

  • src/__tests__/gateway-retry.test.ts β€” exhaustion tests had unhandled rejections
    The fake-timer approach (vi.useFakeTimers + vi.runAllTimersAsync) left p-retry's internal promise chains unsettled after assertions completed, producing 3 unhandled-rejection warnings and a non-zero Errors count in Vitest output. Replaced with retryAfterMs: 1 so real timers fire in ~1 ms β€” no fake timers needed.

Test plan

  • npx vitest run src/__tests__/gateway-retry.test.ts β€” 26 tests pass, 0 errors
  • Manual: trigger a rate-limited call and confirm the bot waits the full retry-after duration before retrying
  • Manual: create a cron expression with no upcoming run dates and confirm /cron add returns a valid response rather than a 500

https://claude.ai/code/session_01ENgnJb7py1ZCKA9ET7EMxm


Generated by Claude Code

claude added 2 commits May 18, 2026 02:19
- gateway: withRetry ignored rate-limit retry-after hints β€” p-retry's own
  minTimeout/factor backoff (1–60 s) silently overrode the computed delayMs.
  Now we sleep(delayMs) manually before rethrowing, and set p-retry's own
  timeout to 0 so delays aren't applied twice.

- history: loadHistory logged under the "sessions" label instead of "history"
  when both the primary store and its backup were corrupt (copy-paste error).

- cron-store: validateCronExpression unsafely cast Cron#nextRun() to Date
  before calling .toISOString(). nextRun() can return null (e.g. for
  expressions whose schedule has already ended), causing a TypeError at
  runtime. Now guarded with a null check.

- tests: gateway-retry exhaustion tests used vi.useFakeTimers() +
  vi.runAllTimersAsync(), which left p-retry's internal promise chains
  unsettled and produced three unhandled-rejection warnings. Replaced with
  retryAfterMs: 1 so real timers fire in ~1 ms β€” no fake timers needed.

https://claude.ai/code/session_01ENgnJb7py1ZCKA9ET7EMxm
Regenerated by npm install in the CI container environment.

https://claude.ai/code/session_01ENgnJb7py1ZCKA9ET7EMxm

@claudiusthebot claudiusthebot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

34/34 CI green βœ… β€” LGTM.

Three genuine correctness bugs, each cleanly fixed. A few notes on overlaps with older open PRs:

Bug 1 (withRetry) β€” also fixed in PR #162 Bug 1, using the differential approach (retryAfterMs - pRetryDelay). This PR's approach β€” set p-retry minTimeout: 0 and sleep the full delayMs before rethrowing β€” is arguably cleaner since it doesn't depend on knowing p-retry's internal backoff arithmetic. Either works; this is simpler. If this lands first, #162 will conflict on gateway.ts and will need to drop its Bug 1. Flagging on #162.

Bug 2 (history.ts log label) β€” same one-line fix as PR #168 Bug 1. No semantic conflict, just a mechanical rebase need.

Bug 3 (cron-store.ts null guard) β€” same fix as PR #168 Bug 7. Same story β€” clean but will require a rebase on #168.

The test cleanup (replacing fake timers + vi.runAllTimersAsync with retryAfterMs: 1 real timers) is the right call. Fake timers + p-retry's internal promise chains interact poorly; real 1ms sleeps sidestep the whole problem.

Impact on older PRs after this lands:

  • #162: needs Bug 1 dropped (Bugs 2 + 3 β€” sticker rate-limit + chatId null check β€” remain unique and worth landing)
  • #168: needs Bugs 1 + 7 dropped, then a rebase; Bugs 2–6 (5 bugs) remain unique

I'll note the overlap on both.

@dylanneve1

Copy link
Copy Markdown
Owner Author

Superseded by #246. I reviewed the old changes and ported the valid/current fixes onto current main in the consolidated PR, while leaving out stale removed-backend changes and broad churn that no longer applies.

@dylanneve1 dylanneve1 closed this May 22, 2026
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.

3 participants