Repository navigation
Conversation
API modules declare their limits and how to read a throttled response. The Requester waits in process for a short wait and throws RateLimitError with retryAt for a long one; the queue worker defers the message instead of spending a delivery. The last-attempt signal stays the fallback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
✅ Deploy Preview for friggframework-org canceled.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 754b69ea39
ℹ️ 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".
| - **Wait over 900 s:** schedule the same body with the one-time scheduler | ||
| at `retryAt`. Devtools provisions the scheduler when any module declares | ||
| `rateLimit`. |
There was a problem hiding this comment.
When an application uses an externally owned integration queue and one of its modules declares rateLimit, this rule enables the scheduler, but SchedulerBuilder.createSchedulerExecutionRole() currently emits Fn::GetAtt: [<IntegrationName>Queue, Arn] for every integration while IntegrationBuilder.useExternalIntegrationQueue() creates no such resource (integration-builder.js:597-612). The resulting CloudFormation template has a dangling logical ID and cannot deploy, so PR 2 needs to pass the ownership decision and external queue ARN into the scheduler rather than merely auto-enabling it.
Useful? React with 👍 / 👎.
| The worker writes `Process.context.rateLimit` with `applyProcessUpdate`: | ||
| `{ status: 'WAITING', retryAt, reason, module, deferrals }`. The next | ||
| successful dispatch of that process clears it. A UI can then show "waiting | ||
| for the provider limit until 23:00 UTC" instead of a run that looks stuck. |
There was a problem hiding this comment.
For a process split across concurrently handled messages, a sibling message can complete after another message has been deferred; clearing the single context.rateLimit field on that unrelated success then hides the wait even though deferred work is still outstanding. Concurrent limits can similarly overwrite one another, so the state must identify pending deferrals and only clear the matching entry when that deferred dispatch succeeds.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs — the change is a new ADR document plus one index-table line, with no runtime code affected. Because it proposes a large, multi-phase architecture change (Requester, queue worker, IAM, module contract, run-state, and pacing) with several open questions still unresolved, a human architectural review would still be worthwhile before this is accepted as policy.
What was reviewed: spot-checked the ADR's file:line citations against the current source (requester.js backoff ladder and 4xx handling, backend-utils.js halt logic, Worker.js/queuer-util.js DelaySeconds support, base-definition-factory.js IAM actions) and they match current behavior; also reviewed the phase-2a per-entity pacing design for cross-tenant impact (noted as a ruled-out candidate issue, not a defect in this docs-only change) and the README index-table addition, which is a simple one-line add.
Extended reasoning...
The diff is documentation-only: a new 349-line ADR (docs/architecture-decisions/049-rate-limit-awareness.md) proposing rate-limit-aware handling across the Requester, queue worker, IAM, module contract, and pacing, plus a one-line index update in README.md. No source, tests, or config changed, so there is no injection/auth/data-exposure surface exercised. Spot-checked several of the ADR's file:line evidence citations (requester.js backoff ladder, backend-utils.js halt logic, Worker.js/queuer-util.js DelaySeconds, base-definition-factory.js IAM policy) against actual source and found them accurate. The decision to defer rather than approve is driven by the size and consequence of the proposed design (multi-phase, touches core/devtools/module-library, 8 open questions) rather than any code defect — this is exactly the kind of large architectural decision a human should weigh in on even when the PR itself is just prose.
API modules (headers and body reason, docs-only limits, a non-Requester client, a module that declares nothing), the Requester throttled branch, a Retry-After parser, RateLimitError, the halt rule, Worker.run deferral, an integration handler and the RATE_LIMITED message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
This automated review ran again and found no bugs (docs-only ADR proposal, no source code changed). Since the last review, the PR grew substantially with a new appendix of code sketches (~276 added lines): Requester throttled-branch handling, RateLimitError, the queue-worker defer() fallback chain (send/scheduler/ChangeMessageVisibility), the integration handler, and the RATE_LIMITED message shape. Given the size and the number of framework-wide design decisions being proposed (retry/backoff behavior, SQS visibility/IAM changes, run-state and user messaging), a human architectural review is still worthwhile before this ADR is accepted.
Beyond the reported candidate issue, I confirmed it against the current text: in the defer() sketch (A.9), the ChangeMessageVisibility fallback branch calls changeVisibility(record, ...) on the original record rather than persisting next (which holds the incremented deferrals counter), so a redelivery via that path would not see the incremented count — but this is illustrative pseudocode in the ADR appendix, not shipped code, so it doesn't affect any running system.
Extended reasoning...
Docs-only change: a new ~625-line ADR (049-rate-limit-awareness.md) proposing rate-limit handling across the Requester, SQS queue worker, IAM permissions, and run/user messaging, plus a one-line README index update. No source code or security-sensitive surface is touched. Since the prior review, the author appended a large code-sketch section (276 lines); the size and cross-cutting design scope (not any bug) are why a human look is still warranted, and the previously flagged candidate issue is confirmed present but immaterial since it's only illustrative pseudocode.
Records the answers to open questions 1 to 4 (Retry-After for every module, a 5 minute in-process cap, a new message for a wait up to 15 minutes and the scheduler beyond, 10 deferrals and 24 hours) and brings the text and the sketches in line with the code in #656 and #657. Changes to the text: - the hint wins; the fixed ladder is used only when there is no hint, and it has no budget; - classify runs for a 429 and any other 4xx or 5xx except 401, not for a 2xx, and may name only a reason; - the invocation deadline lives in its own AsyncLocalStorage; - the handler checks delivery.isLastAttempt first, then rethrows, because the visibility tier and a cap use a receive; - the RATE_LIMITED message has no `scheduled` flag and is deduplicated; - the base role gets sqs:ChangeMessageVisibility in every stack. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The Requester throws RateLimitError only when a real hint exists, so `source` is never `backoff`. The error also carries `waitMs`. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also looked at the RATE_LIMITED notification dedup rule (line 269), which keys the 60s skip on module rather than the declared scope key (entity/credential/app, line 132). Since the delegate flows through a per-integration-instance path (module.js:210 → integration-base.js:857), this dedup does not appear to cross distinct entities/credentials in practice — it reads as an underspecified detail in a proposal doc rather than a confirmed bug, so I'm not raising it as a separate finding.
Extended reasoning...
This run's inline findings (three CONFIRMED issues in the ADR's appendix code sketches) already signal that human review is warranted, so no approval is possible. Beyond those, I independently checked the RATE_LIMITED notification dedup keying concern raised as a candidate this run and concluded it is not a clear bug given the per-instance delegate path, distinct from the already-confirmed appendix issues, so it's worth a brief note rather than silence or repeating prior feedback.
|
|
||
| ### A.5 Requester: the throttled branch | ||
|
|
||
| This branch replaces the combined 429 and 5xx ladder at `requester.js:301-310`. |
There was a problem hiding this comment.
🟡 (optional) The Requester code sketch cites the wrong location for the code it says it replaces: 'This branch replaces the combined 429 and 5xx ladder at requester.js:301-310' — but the actual backOff ladder (status === 429 || status >= 500) is at requester.js:276-284, correctly cited earlier in the same ADR's own current-state table (line 41). Lines 301-310 in requester.js are unrelated 401 token-refresh retry logic. An implementer of phase-1 (frigg#656) following this citation could edit or remove the wrong block, breaking the 401/refresh retry path while leaving the real 429/5xx ladder untouched. Fix: correct the citation to requester.js:276-284 (or re-verify against current line numbers before merge).
Why this was flagged
The ADR's appendix A.5 (line 515) says the throttled-branch sketch 'replaces the combined 429 and 5xx ladder at requester.js:301-310'. Verified against packages/core/modules/requester/requester.js: the ladder condition '(status === 429 || status >= 500) && attempt < this.backOff.length' is at line 277 (block 276-284), matching the ADR's own earlier citation at line 41 ('requester.js:276-284'). Lines 301-310 instead contain the 401/token-refresh retry branch ('This request did not try the current token...'). A contributor implementing frigg#656 from this appendix, without re-deriving the line numbers, could modify the wrong code path. No safeguard catches this because the ADR states elsewhere 'where a sketch and the code differ, the code and text of this section are right', which covers sketch content, not this citation's location.
Verification: nit. The candidate is factually correct. ADR line 515 states: "This branch replaces the combined 429 and 5xx ladder at requester.js:301-310." But in packages/core/modules/requester/requester.js the actual 429/5xx ladder is at lines 276-284: (status === 429 || status >= 500) && attempt < this.backOff.length (line 276-278) followed by the backOff sleep and retry. Lines 301-310 instead…
| // inside run(), per record | ||
| } catch (error) { | ||
| if (error.isRateLimited && body) { | ||
| const { outcome } = await this.defer(record, body, error, delivery); | ||
| if (outcome === 'acked') return; // sent again, or scheduled | ||
| if (outcome === 'failed') { // visibility extended, or a cap | ||
| batchItemFailures.push({ itemIdentifier: record.messageId }); | ||
| return; | ||
| } | ||
| } // 'skipped': fail as any error does | ||
| ... | ||
| } |
There was a problem hiding this comment.
🟡 (optional) A.9's Worker.run sketch (lines 610, 613) uses return instead of continue inside the per-record for-loop, so once any record in an SQS batch throws isRateLimited, every later record in that same batch is skipped entirely and never added to batchItemFailures. Compare to the real loop in packages/core/core/Worker.js:27-67, which never returns mid-loop and always finishes iterating every record, pushing failures as it goes. A record silently skipped this way is neither retried nor reported as failed, so SQS deletes it as if it succeeded, permanently losing that message's work. …
Why this was flagged
…Fix: replace both return statements with continue (matching the existing halt-error continue at Worker.js:62) so deferral/cap outcomes only affect the current record and the loop still visits every remaining record in the batch.
Trigger: a Lambda invocation receives an SQS batch with more than one record (default batch size up to 10, and the ADR itself notes reservedConcurrency: 20 workers sharing a queue), and one record's handler throws an error with isRateLimited. In A.9, defer() runs, then if (outcome === 'acked') return; (line 610) or the failed branch's return; (line 613) exits the whole run() method, not just the current loop iteration. Any records after that one in records are never touched: not processed, not pushed to batchItemFailures. Base behavior (Worker.js today) always finishes the for-loop over every record. Because unmentioned messageIds are treated by SQS as successfully processed and deleted, those records are lost with no retry and no error logged.
Verification: nit. The claim is factually correct. The A.9 sketch at docs/architecture-decisions/049-rate-limit-awareness.md is explicitly labeled "// inside run(), per record" (line 606) and its catch block uses bare return in two spots: line 610 if (outcome === 'acked') return; and line 613 return; (inside the failed branch, after pushing to batchItemFailures). Because the sketch belongs inside…
| ### A.4 API module: declares nothing | ||
|
|
||
| No change: the same calls, the same fixed backoff, the same redelivery. | ||
|
|
||
| ```js | ||
| class Api extends ApiKeyRequester { | ||
| static requestTimeoutMs = 30_000; | ||
| // no rateLimit: backOff [1, 3, 10, 30, 60, 180] s, then SQS redelivery, as today | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🟡 (optional) The appendix's canonical 'declares nothing' example (A.4) says a module with no static rateLimit config gets 'the same calls, the same fixed backoff, the same redelivery' unconditionally. That contradicts Decided item 1 (line 383): 'Retry-After is honoured for every module, by default', independent of any static declaration (also stated at lines 169 and 324-325). An implementer coding A.4 literally would gate default Retry-After parsing behind a static rateLimit declaration, so an undeclared module hitting a 429 with a real Retry-After header keeps the old ~284s ladder instead of the header-driven wait the decision requires. …
Why this was flagged
…Fix: qualify A.4 to say 'no change' only for a 429 with no Retry-After header, matching sections 1/2 and Decided #1, and align the phasing table's 'opt-in' label on PR1 (line 289) with the same by-default scope.
A.4 (lines 502-511) is the doc's worked example for 'API module: declares nothing' and states flatly that behaviour is unchanged: same backoff, same redelivery. Decided #1 (line 383) and section 1 (line 169) say Retry-After parsing runs 'for every module, by default' on any 429, regardless of a static rateLimit declaration; the phasing table (line 289) even labels the PR bundling this 'opt-in'. These passages disagree on what happens when a provider used by an undeclared module returns a 429 with Retry-After: A.4 implies the fixed ladder always still runs for such modules; the Decided section implies the new header-driven wait applies instead. Nothing in the doc reconciles the two, so an implementer following the appendix literally would under-scope the default Retry-After honoring the decision requires, and reviewers reading only the phasing table would not expect…
Verification: nit. Real internal contradiction in the new ADR. A.4 line 504 ("API module: declares nothing") states unconditionally: "No change: the same calls, the same fixed backoff, the same redelivery." with the code comment at line 509 reinforcing "backOff ... then SQS redelivery, as today". This directly contradicts the doc's own authoritative statements that Retry-After runs for undeclared… | nit.…
`Process.context.rateLimit` is one value per process, so it holds the latest known wait. A new wait replaces it only when its retryAt is later, a WAITING value never hides an EXHAUSTED one, and the worker clears it only when it is WAITING and its retryAt is past or within 5 s. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I checked the deferral-cap math at lines 218-222 (default maxDeferrals=10, maxDeferredMs=24h): a daily/monthly quota wait resolves via a single scheduler-tier defer at retryAt (line 210-213), not repeated 900s delay-tier cycles, so it isn't silently exhausted after ~3 cycles as the candidate issue suggested — the "no longer DLQ'd" claim holds for that scenario.
Extended reasoning...
Docs-only ADR change (no code); this run's inline findings (RateLimitError.create not overridden, defer() missing required exclusion checks, scheduler idempotency gap) already flag the appendix code sketches for human attention. I additionally verified the previously-flagged deferral-cap/DLQ concern and found the scheduler tier handles long waits in one shot rather than via repeated 900s cycles, so that particular concern doesn't hold.
Still open from earlier reviews (3):
- Unresolved: 3 minor or pre-existing.
| throw await RateLimitError.create({ | ||
| resource: encodedUrl, | ||
| init: options, | ||
| response, | ||
| hint, | ||
| waitMs, | ||
| module: this._telemetryModuleLabel(), | ||
| scopeKey: computeScopeKey(policy, this), | ||
| }); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### A.6 Requester: a built-in parser and the budget | ||
|
|
||
| ```js | ||
| function parseRetryAfter(value, { now = Date.now() } = {}) { | ||
| const text = String(value ?? '').trim(); | ||
| if (/^\d+(\.\d+)?$/.test(text)) return hintFromWait(Number(text) * 1000, now); | ||
| if (!/[A-Za-z]/.test(text)) return null; // Date.parse("5") is a valid date | ||
| const at = Date.parse(text); // HTTP-date or ISO timestamp | ||
| return Number.isNaN(at) ? null : hintFromRetryAt(new Date(at), now); | ||
| } | ||
|
|
||
| function inProcessBudgetMs({ policy, requestTimeoutMs, remainingMs, waitedMs }) { | ||
| const cap = policy?.maxInProcessWaitMs ?? 300_000; | ||
| return Math.max(0, Math.min(cap - waitedMs, remainingMs - (requestTimeoutMs || 0))); | ||
| } | ||
| ``` | ||
|
|
||
| ### A.7 `RateLimitError` | ||
|
|
||
| ```js | ||
| class RateLimitError extends FetchError { | ||
| constructor({ hint, module, ...fetchErrorArgs }) { | ||
| super(fetchErrorArgs); | ||
| this.name = 'RateLimitError'; | ||
| this.isRateLimited = true; | ||
| this.retryAt = hint.retryAt ?? new Date(Date.now() + hint.waitMs); | ||
| this.waitMs = Math.max(0, this.retryAt - Date.now()); | ||
| this.reason = hint.reason; // burst | daily | monthly | concurrency | ||
| this.policy = hint.policy; | ||
| this.source = hint.source; // header | body | static, never backoff | ||
| this.module = module; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 (optional) A future implementer following this ADR literally gets a rate-limit feature that silently never activates. A.5 (line 539) throws await RateLimitError.create({...}), but A.7's RateLimitError class (lines 572-584) never overrides the inherited static create(). The real FetchError.create() (packages/core/errors/fetch-error.js:76-81) hardcodes return new FetchError({...options, responseBody}), not new this(...), so calling it via a subclass still returns a plain FetchError with no isRateLimited/retryAt/waitMs. That breaks the Requester's throw and the A.9 defer() check (error.isRateLimited, line 609), so it silently falls back to today's ladder/redelivery. … [also at: docs/architecture-decisions/049-rate-limit-awareness.md:578 - A contributor implementing frigg#656 from A.7's RateLimitError sketch gets a self-computed waitMs that silently discards the value the Requester already worked out, so the error's reported wait can diverge from what was actually computed for it.; docs/architecture-decisions/049-rate-limit-awareness.md:582 - A contributor implementing frigg#656/#657 from A.7's RateLimitError sketch gets an error object whose scopeKey is always undefined, contradicting section 2 (line 193-194) which states RateLimitError 'adds ... scopeKey ... and module'.]
Why this was flagged
…Fix: have RateLimitError define its own static create() using new this(...), or have A.5 build the error with new RateLimitError(...) directly.
Trigger: an implementer of frigg#656/#657 codes A.5's throttled branch verbatim, calling RateLimitError.create({resource, init, response, hint, waitMs, module, scopeKey}) at line 539. RateLimitError (A.7, lines 572-584) inherits static async create from FetchError instead of defining its own. packages/core/errors/fetch-error.js:76-81 shows that method hardcodes new FetchError({...options, responseBody}), never new this(...), so the returned…
Verification: nit. Docs-only ADR sketch defect, factually accurate. A.7 (docs/architecture-decisions/049-rate-limit-awareness.md:572-584) defines class RateLimitError extends FetchError with only a constructor and no override of the static create(). A.5 (line 539) calls throw await RateLimitError.create({resource, init, response, hint, waitMs, module, scopeKey}), invoking the inherited static. The real…
| async defer(record, body, error, delivery) { | ||
| const deferral = nextDeferral(body); // _frigg.deferrals, firstDeferredAt | ||
| if (isDeferralCapped({ ...deferral, retryAt: error.retryAt }, readDeferralLimits())) { | ||
| return { outcome: 'failed' }; // and record EXHAUSTED | ||
| } | ||
| const waitMs = Math.max(0, error.retryAt - Date.now()); | ||
| const next = withDeferral(body, deferral); | ||
|
|
||
| if (waitMs <= 900_000) { | ||
| await this.send({ ...next, QueueUrl: await this._queueUrl(record) }, Math.ceil(waitMs / 1000)); | ||
| return { outcome: 'acked' }; | ||
| } | ||
| if (this.getSchedulerService()) { // never the mock scheduler | ||
| await scheduler.scheduleOneTime({ | ||
| scheduleName: `frigg-defer-${record.messageId}`, | ||
| scheduleAt: error.retryAt, | ||
| queueResourceId: record.eventSourceARN, | ||
| payload: next, | ||
| }); | ||
| return { outcome: 'acked' }; | ||
| } | ||
| await changeVisibility(record, Math.min(Math.ceil(waitMs / 1000), 43_200)); | ||
| return { outcome: 'failed' }; // back at retryAt, one delivery used | ||
| } |
There was a problem hiding this comment.
🟡 (optional) An implementer coding A.9's defer() literally gets calls to AWS send/scheduler.scheduleOneTime/changeVisibility with NaN or undefined values instead of the safe fallback section 3 requires. defer() (620-643) only checks isDeferralCapped; it never checks the other three exclusions section 3 (line 221) lists ('no retryAt, no event source ARN or a FIFO queue'). If error.retryAt is missing, waitMs at 625 is NaN, so changeVisibility(record, NaN) or scheduler.scheduleOneTime({scheduleAt: undefined}) (636) fire instead of falling back to #653's isLastAttempt. …
Why this was flagged
…Fix: defer() must check retryAt presence, event-source ARN presence, and FIFO queue before attempting any of the three send/schedule/visibility paths, returning {outcome:'skipped'} so #653 stays the fallback, per line 221/391-393.
Trigger: a rate-limited record where error.retryAt is undefined (a hint with no reset time), or the queue is FIFO, or record.eventSourceARN is absent. defer() at line 620-643 only guards the deferral cap (622); it does not implement the other exclusions section 3 (line 221) states are required before deferring. With no retryAt, waitMs (625) is NaN, so changeVisibility(record, NaN) (641) or scheduler.scheduleOneTime with scheduleAt: undefined (635) run instead of falling back cleanly. Base behavior (today) always rethrows to #653's isLastAttempt; this sketch instead makes an invalid AWS call inside an unguarded async function with no surrounding try/catch, likely throwing uncaught inside the record's catch clause.
Verification: nit (docs-only ADR; the section-3 prose already states the correct guards, so an implementer who follows the prose is safe — the defect is that the A.9 code sketch does not implement them). The gap is real in the sketch as written. Section 3 lines 206 ("defers a record that failed with an error that has isRateLimited and a retryAt") and 220-222 ("Past the cap, or with no retryAt, no…
| async defer(record, body, error, delivery) { | ||
| const deferral = nextDeferral(body); // _frigg.deferrals, firstDeferredAt | ||
| if (isDeferralCapped({ ...deferral, retryAt: error.retryAt }, readDeferralLimits())) { | ||
| return { outcome: 'failed' }; // and record EXHAUSTED | ||
| } | ||
| const waitMs = Math.max(0, error.retryAt - Date.now()); | ||
| const next = withDeferral(body, deferral); | ||
|
|
||
| if (waitMs <= 900_000) { | ||
| await this.send({ ...next, QueueUrl: await this._queueUrl(record) }, Math.ceil(waitMs / 1000)); | ||
| return { outcome: 'acked' }; | ||
| } | ||
| if (this.getSchedulerService()) { // never the mock scheduler | ||
| await scheduler.scheduleOneTime({ | ||
| scheduleName: `frigg-defer-${record.messageId}`, | ||
| scheduleAt: error.retryAt, | ||
| queueResourceId: record.eventSourceARN, | ||
| payload: next, | ||
| }); | ||
| return { outcome: 'acked' }; |
There was a problem hiding this comment.
🟡 (optional) A.9's defer() breaks the doc's own idempotency claim at line 223-224 ('Delivery stays at least once, so handlers stay idempotent as today'): the scheduler tier uses a deterministic scheduleName: frigg-defer-${record.messageId} (line 634) with no conflict handling. If the Lambda stops after scheduler.scheduleOneTime() succeeds but before the record is acked, SQS redelivers the same message; the retry calls scheduleOneTime again with the identical name, which EventBridge Scheduler rejects as a duplicate. That throw is uncaught (defer() is awaited inside run()'s catch block at line 610 with no inner try/catch), so it escapes run() and fails the whole batch instead of just this record. …
Why this was flagged
…Fix: make the scheduler branch tolerate a duplicate-name conflict as a success (or vary the name per delivery attempt), and isolate defer()'s own errors from other records in the batch.
Trigger: a rate-limited record needs the scheduler tier (wait > 900_000 ms, A.9 line 628-639) and the Lambda instance is stopped/recycled after scheduler.scheduleOneTime() at line 633 succeeds but before the outcome 'acked' propagates back through Lambda's response. SQS then redelivers the original, unmodified message (at-least-once, as the doc itself notes at line 223-224 and 602-604). The redelivered record re-enters run()'s catch and calls defer() again, which computes the same scheduleName (line 634) and calls scheduleOneTime again. EventBridge Scheduler's CreateSchedule rejects a duplicate schedule name, and this rejection is never caught inside defer() or its caller (line 608-618), so it propagates out of run() for the whole invocation, not just this record. Base code has no such call at all, so this is a new crash path the ADR's own idempotency claim does not cover.
Verification: nit (docs-only ADR; a literal implementer would ship the bug, nothing breaks at merge). The candidate's chain holds against the real code. The A.9 sketch (docs/architecture-decisions/049-rate-limit-awareness.md:632-639) schedules with a deterministic name frigg-defer-${record.messageId} (line 634) and returns { outcome: 'acked' } (639) only after scheduler.scheduleOneTime() succeeds — the…
…docs HubSpot resets a daily limit at midnight in the account time zone, which the module does not know, so the sketch probes every hour instead of computing a UTC midnight. A burst limit names only its reason and takes its time from Retry-After or the burst window. Salesforce uses one error code for the daily limit and for too many concurrent long-running requests, so the sketch reads the message to tell them apart, and tells the delegate before it throws. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
| The message has no `scheduled` flag. The Requester does not know whether the | ||
| worker will defer, and `Process.context.rateLimit` tells the UI that. Twenty | ||
| workers can meet the same limit at once, so a message for the same module | ||
| with a `retryAt` within 60 s of the last one is skipped. The status of the | ||
| integration does not change: a rate limit is not an error. |
There was a problem hiding this comment.
🟡 (optional) Section 5 (lines 270-274) says a RATE_LIMITED message is 'skipped' when its retryAt is within 60s of 'the last one' for the same module, but names no storage or lock for tracking 'the last one'. Under the doc's own '20 workers' scenario this is a check-then-act race: concurrent invocations can each see no recent message and all send, so users get several duplicate notifications instead of the single one promised. The only shared per-scope state anywhere in the ADR (rateLimitedUntil[scopeKey], section 6, line 282) ships in PR5/phase 2, one PR after this message delegate (PR4, phasing table line 296), so an implementer following PR4 alone has nothing atomic to check against. …
Why this was flagged
…Fix: specify an atomic conditional write (e.g. borrow 2b's entity-scoped state a phase early, or a per-scopeKey lock) before claiming dedup, covering every concurrent-worker path that can emit this message, not just a single-worker case.
Trigger: 20 concurrent queue workers (reservedConcurrency: 20, cited at line 45) hit the same provider limit at once, exactly the scenario the doc itself invokes at lines 271-272. The changed text at 270-274 dedupes RATE_LIMITED messages by comparing retryAt to 'the last one' but never assigns that state a home (no field, table, or lock named). Section 6 (line 282) is the only place the ADR adds shared per-scope state (rateLimitedUntil[scopeKey] on the entity), and the phasing table (line 296-297) puts it in PR5, one PR after the message delegate's own PR4. So at PR4 time there is no atomic check-and-set: concurrent invocations each read 'no recent message' and each send, yielding many duplicate messages instead of the one the doc promises. No safeguard in the doc catches this because no section assigns ownership of 'the last one' before PR5.
Verification: nit (docs-only ADR; no runtime impact at merge, but the design gap is real). The candidate accurately quotes the mechanism at docs/architecture-decisions/049-rate-limit-awareness.md:270-274: "Twenty workers can meet the same limit at once, so a message for the same module with a retryAt within 60 s of the last one is skipped." The ADR states this dedup as a guaranteed outcome ("is…
| The worker writes `Process.context.rateLimit` with `applyProcessUpdate`: | ||
| `{ status, mechanism, retryAt, reason, module, deferrals, updatedAt }`. | ||
| `status` is `WAITING`, or `EXHAUSTED` when a cap ended the deferrals or a | ||
| visibility change came on the last delivery. The value is one per process, so | ||
| it holds the latest known wait: a new wait replaces it only when its `retryAt` | ||
| is later, and a `WAITING` value never hides an `EXHAUSTED` one. After a | ||
| deferred or redelivered message ran without error, the worker sets it to | ||
| `null` only when it is `WAITING` and its `retryAt` is past or within 5 s. |
There was a problem hiding this comment.
🟡 (optional) An implementer building Phase 2 (frigg#657) from this ADR gets a Process.context.rateLimit merge rule the ADR's own atomic primitive cannot express, so the 'one per process, latest known wait' guarantee (lines 240-244) silently breaks under the 20-concurrent-workers scenario the ADR itself notes at line 272. applyProcessUpdate's only mutation is a blind whole-subtree set (process-repository-interface.js:76-78; mongo.js:117-124 issues an unconditional $set) — there is no compare/max op for 'replaces only when its retryAt is later' or 'a WAITING value never hides an EXHAUSTED one'. Phase 2's file list (line 294) lists no process-repository-*.js file. …
Why this was flagged
…Fix: either add a conditional/CAS op to applyProcessUpdate (and add the repository files to Phase 2's scope) or specify a read-compare-retry loop, not a bare 'replaces only when' rule that assumes atomicity set doesn't provide.
Trigger: Phase 2 (frigg#657) implements section 4's Process.context.rateLimit write using Worker.js and the files listed at line 294, none of which are process-repository-*.js. Section 4 (lines 240-244) requires a conditional merge ('a new wait replaces it only when its retryAt is later, and a WAITING value never hides an EXHAUSTED one') applied by 20 concurrent workers hitting the same limit (line 272). The only real primitive available, applyProcessUpdate, supports only increment/set/pushSlice/newState (process-repository-interface.js:72-86) and every backend's set is an unconditional overwrite (mongo.js:117-124: $set[path] = value; postgres uses plain jsonb_set). Implementing the ADR's rule thus requires a non-atomic findById+compare+update in Worker.js, reintroducing the exact clobber race applyProcessUpdate (packages/core/CLAUDE.md:216) exists to…
Verification: nit (docs-only ADR; nothing breaks at merge, and it describes code nobody has written yet). The internal gap is real and accurately stated. Section 4 (docs/architecture-decisions/049-rate-limit-awareness.md:237-244) prescribes writing Process.context.rateLimit "with applyProcessUpdate" and requires a conditional merge: "a new wait replaces it only when its retryAt is later, and a…
There was a problem hiding this comment.
An organization admin can add extra usage credits at claude.ai/admin-settings/usage.
Once credits are available, reopen this pull request to trigger a review.
- The in-process budget keeps a reserve for the next request of the request timeout or half the time left, whichever is smaller. With the full request timeout the budget was 0 in every 29 s HTTP Lambda. - The RATE_LIMITED message is written only for a wait longer than the in-process cap, and each stored message type keeps its newest 50 items. - The default deferral caps are 30 deferrals and 26 hours, so hourly probes of a daily limit last one full day. - The visibility tier keeps a margin: SQS counts the 12 hour maximum from the receive, and an extension does not reset it. - classify() must return a reason or a time, synchronously; Retry-After is read on a 429 and on a response that classify() recognises; the static step takes the larger of minRetryAfterMs and the window. - backOff: [] means no in-process waits for a hint either; a hinted wait logs at TRACE and the Requester does not log a throw. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
If your organization's extra usage balance is empty, an organization admin can add extra usage credits at claude.ai/admin-settings/usage. If its monthly spend limit was reached, an admin can raise it on the same page. If neither applies, contact Anthropic support.
Once extra usage is available, comment @claude review on this pull request, or push a new commit, to trigger a review.
ADR-049 section 4 said the run state is cleared only when its retryAt is past or within 5 s. A local end-to-end run showed the gap: a job that resumed before its retryAt finished, and its process kept `status: 'WAITING'`. The deferred body now carries `_frigg.deferralId`, the run state stores it, and the message that set a wait clears it at once; other messages still clear it only after its retry time (#657). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
seanspeaks
left a comment
seanspeaks
left a comment
There was a problem hiding this comment.
Proposed ADR-049 amendments that follow from the #656 review. Drafted with Claude Code at Sean's request; the direction is Sean's.
Direction: an API module describes its provider. The caller decides how long it may wait in process. A long wait becomes an event, and the worker or orchestrator decides what happens next. Most of the ADR already fits: phase 2's deferral is the right mechanism. These changes move the decisions to that shape.
§1 API-module contract
- Keep
scope,windows,maxConcurrency,minRetryAfterMs,parsers,classifyanduserHints. All of these describe the provider. - Say explicitly that a module never decides waiting, retrying or deferral.
- A
classifyhint for a status other than 429 must not fall back to thebackOffladder. If it has no time (from the parsers,windows.resets,minRetryAfterMsorperMs), the Requester throwsRateLimitErrorimmediately withretryAtunknown, and the worker handles an unknownretryAt(see §3). Today this falls through to the unbudgeted ladder (#656, finding 1).
§2 Requester
- Move
maxInProcessWaitMsout of the module policy into the invocation context, set by the entry point:runWithInvocationDeadline({ deadlineAt, maxInProcessWaitMs }). Defaults per entry type: HTTP ~10 s, queue worker small (0 once phase 2 lands, so workers always defer instead of holding one of the 20 slots), admin/script runners their own. - List every entry point that must set the deadline: HTTP, queue workers, admin script and report routers/executors, db-migration. Several don't today.
- Bound hinted retries:
max(backOff.length, N)ormaxHintedRetries, and say that hinted retries don't use up the ladder'sattemptbudget for 5xx/ECONNRESET. - Propagate the rate-limit fields through the OAuth token-endpoint path (
_transportFailureError).
§3 Queue worker → event, then a default handler
Replace "Worker.run defers a record" with:
Worker.runcatchesisRateLimitedand dispatches aRATE_LIMITEDintegration event with{ retryAt, waitMs, reason, module, scopeKey, message, delivery }.- If the integration defines a handler for it, that handler decides: defer, end the run, notify, or rethrow.
- Otherwise a framework default handler applies the tiers already written here:
DelaySecondsup to 900 s, the scheduler beyond that,ChangeMessageVisibilitywithout a scheduler, plus themaxDeferrals/maxDeferredMscaps. - An unknown
retryAtgoes to a configured probe interval (e.g. 1 h for a daily limit) instead of the visibility timeout. - ADR-047: the same
RATE_LIMITEDevent is what the orchestrator's reducer receives later, so this interim mechanism becomes the orchestrator's input rather than a parallel path. Cross-reference it in ADR-047's event types too.
§5 User-facing message
The RATE_LIMITED message should be written by the worker/default handler from the event, not by a delegate from the module up to IntegrationBase. The module still supplies the text and links as data (userHints). That also settles the open "the Requester doesn't know whether the worker will defer" problem, because the component that decides is the one that writes.
§7 Phasing
Phase 1 alone changes the behaviour for a daily-limited 403: it used to be discarded at once, and now it's lost after about an hour via the DLQ (3 receives about 30 min apart with the generated VisibilityTimeout: 1800 / maxReceiveCount: 3). That's slightly better, because it's recoverable from the DLQ, but it's still lost.
- Either land the event and the default handler (phase 2) in the same release as phase 1,
- or keep the halt exemption out of phase 1 until phase 2.
Decisions to record: who owns the in-process budget (the caller), the RATE_LIMITED event as the single seam for deferral and messaging, and the cap on hinted retries.
2 participants
Proposes rate-limit awareness for Frigg: API modules say what their provider's limits are and how to read a throttled response, so Frigg waits exactly as long as needed instead of a fixed backoff and three 30-minute redeliveries.
Why
Decision (summary)
static rateLimit(windows,minRetryAfterMs,maxConcurrency, built-in parsers, aclassify()hook for non-429 signals,userHints).RateLimitErrorwithretryAt. Modules that declare nothing keep today's behaviour.DelaySecondsup to 900 s, the one-time scheduler beyond,ChangeMessageVisibilityas a fallback; capped. feat(core): tell queue handlers their last delivery attempt #653isLastAttemptstays the fallback.context.rateLimit("waiting until X") and aRATE_LIMITEDdelegate with "retry when ready" and provider links.rateLimitedUntilon the entity); no distributed limiter for now (ADR-031).Phased into 5 PRs; the first is core-only and opt-in. Eight open questions, then an appendix of code sketches: four API-module shapes (headers + body reason, docs-only limits, a non-Requester client, a module that declares nothing), the Requester's throttled branch,
RateLimitError, the halt rule,Worker.rundeferral, an integration handler and theRATE_LIMITEDmessage.Related
ADR-031, ADR-042, ADR-047 (#648), ADR-048, #653, #633.
Implementation
RateLimitError: feat(core): honour Retry-After and let API modules declare rate limits #656RATE_LIMITEDmessage: feat(core): tell the integration when a rate limit is too long to wait for #658 (stacked on feat(core): honour Retry-After and let API modules declare rate limits #656)The decisions on open questions 1 to 4 are recorded in the ADR, and the sketches now match the code.
🤖 Generated with Claude Code