Skip to content

fix(serenity): check retryOnQuota's shared deadline before every attempt#2882

Open
byteclimber wants to merge 1 commit into
mainfrom
fix/retryonquota-deadline-preattempt
Open

fix(serenity): check retryOnQuota's shared deadline before every attempt#2882
byteclimber wants to merge 1 commit into
mainfrom
fix/retryonquota-deadline-preattempt

Conversation

@byteclimber

Copy link
Copy Markdown
Contributor

Summary

Self-review follow-up to #2874. Found that the shared per-request deadline was only checked inside the catch block, after a poll attempt had already failed — so the first poll attempt always ran regardless of how long ensure() itself took (e.g. queued behind other same-child recoveries on withResourceLock's own up-to-10s safety valve).

This undermines the deadline's whole purpose: a request where multiple stacked wrap sites (e.g. createProject + createPromptsByIds, both hit in the same generateTopics: true create-market call on a freshly-activated brand) each independently hit the disguised-405 lag could still compound latency past the intended budget — a narrower recurrence of the exact stacked-call-site risk the shared deadline was built to close (round-2 SRE review on #2874).

  • Moves the deadline check to run before every attempt, including the first, not just between retries.
  • Tracks the most recent metered-quota error (lastError) so a pre-attempt bail still throws something meaningful instead of needing a fresh failure to report.
  • attempt now starts at 0 and increments only once an attempt is actually made, so a pre-attempt deadline bail reports exhausted at the attempt count actually reached (possibly 0, meaning ensure() alone consumed the whole budget).

Test plan

  • npm run lint && npm run type-check && npm test — full suite green (15424 passing)
  • Updated the two deadline-specific tests to reflect the corrected pre-attempt semantics
  • Added a new test proving a budget already blown by ensure() skips the poll attempt entirely (the exact gap this fix closes)

🤖 Generated with Claude Code

…mpt, not just between retries

Self-review of #2874 found that the deadline was only consulted inside the
catch block, after a poll attempt had already failed — so the FIRST
attempt always ran regardless of how long ensure() itself took (e.g.
queued behind other same-child recoveries on withResourceLock's own
up-to-10s safety valve). That undermines the shared per-request deadline's
whole purpose: bounding a request where multiple stacked wrap sites
(createProject + createPromptsByIds in the same generateTopics create-market
call) each hit the disguised-405 lag could still compound past budget in a
narrower but real case.

Move the deadline check to run before every attempt, including the first,
tracking the most recent metered-quota error so a pre-attempt bail still
throws something meaningful.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@MysticatBot MysticatBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey @byteclimber,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Approve - clean, well-scoped fix that closes a real gap in deadline enforcement; no blocking findings.
Complexity: LOW - small single-service bug fix.
Changes: Moves the shared per-request deadline check to run before every poll attempt (including the first), so a slow ensure() call can no longer silently blow past the budget (2 files).

Non-blocking (2): minor issues and suggestions
  • nit: The 8-line inline comment block (lines 181-192 in the new code) restates the design rationale already present in the header comment and includes process artifacts ("self-review finding", "found on self-review") that will age poorly. Consider trimming to 2-3 lines that state the invariant, not its discovery story - src/support/serenity/dynamic-allocation-active.js:181
  • suggestion: When the pre-attempt deadline check fires at attempt=0, recordQuotaRetryOutcome('exhausted', { attempt: 0, callSite }) introduces a new metric dimension value that did not exist before (minimum was previously 1). Confirm downstream dashboards/alerts that consume this metric handle attempt=0 gracefully, or consider a distinct outcome label like 'budget_exceeded' - src/support/serenity/dynamic-allocation-active.js:190

Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 7m 59s | Cost: $3.35 | Commit: de25ef5a75da64003bc84d5ba950c513053e2589
If this code review was useful, please react with 👍. Otherwise, react with 👎.

@MysticatBot MysticatBot added ai-reviewed Reviewed by AI complexity:low AI-assessed PR complexity: LOW labels Jul 22, 2026
@byteclimber
byteclimber requested a review from aliciadriani July 22, 2026 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed Reviewed by AI complexity:low AI-assessed PR complexity: LOW

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants