Skip to content

feat(core): put a rate-limited queue message back at its retry time - #657

Open
d-klotz wants to merge 34 commits into
feat/core-rate-limit-requesterfrom
feat/core-rate-limit-defer
Open

d-klotz wants to merge 34 commits into
feat/core-rate-limit-requesterfrom
feat/core-rate-limit-defer

Conversation

@d-klotz

@d-klotz d-klotz commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

A queue handler that hits a provider rate limit no longer gives its message back to SQS for a 30 minute redelivery that spends one of its three receives. The queue worker puts the message back so it runs at the time the provider said. This is phase 2 of ADR-049 (#655).

Stacked on #656. Merge #656 first. This PR needs RateLimitError and retryAt from it, and GitHub retargets this PR to next when #656 merges.

What problem this fixes

  • A long limit costs all three receives. After the Requester's in-process ladder, a throttled handler rethrows and SQS redelivers 30 minutes later. The queue allows 3 deliveries, so a limit that lasts two hours sends the message to the DLQ, which only logs.
  • A sync split into many messages then stays open forever. One lost message means the run is never completed and never failed.
  • The user sees a run that looks stuck. Nothing says "waiting for the provider limit until 23:00 UTC".

What this PR does

When a handler throws an error with isRateLimited and a retryAt (a RateLimitError from the Requester, or one a module throws), Worker.run puts the message back so it runs at retryAt:

Wait What the worker does Record
Up to 900 s Sends the same body again with DelaySeconds, then acknowledges the old message handled
Over 900 s, SCHEDULER_ROLE_ARN set Creates a one-time EventBridge schedule at retryAt that sends the body to the queue, then acknowledges handled
Over 900 s, no scheduler Extends the visibility timeout to retryAt (12 h less 20 minutes at most: SQS counts the 12 h from the receive, and an extension does not reset it). This uses one receive reported failed
  • The new message goes out before the old one is acknowledged. Delivery stays at least once, as today. A crash between the two can run the work twice, so handlers stay idempotent.

  • A deferred body carries _frigg: { deferrals, firstDeferredAt, deferralId }. It is reserved; handlers never see it. deferralId is the SQS id of the first message that was put back, and later deferrals keep it. The new message starts at receive count 1.

  • Two caps bound it: FRIGG_QUEUE_MAX_DEFERRALS (default 30) and FRIGG_QUEUE_MAX_DEFERRED_MS (default 26 h, from the first deferral to the retry time). The HubSpot and Salesforce modules probe a daily limit every hour, so the defaults cover one full day of hourly probes. Core owns both ends of these numbers, so devtools injects nothing.

  • Past a cap, with no retryAt, with no event source ARN, or on a FIFO queue, the record fails like any retryable error does, and delivery.isLastAttempt (feat(core): tell queue handlers their last delivery attempt #653) applies.

  • Run state. With a processId in the message, the worker writes, through two use cases (RecordRateLimitWait, ClearRateLimitWait), Process.context.rateLimit = { status: 'WAITING' | 'EXHAUSTED', mechanism, retryAt, reason, module, deferrals, deferralId, updatedAt }. After a deferred or redelivered message ran, a WAITING value becomes null at once when that message set it (same deferralId), and for any other message only when its retryAt is past or within 5 s. So a job that resumes before its retryAt clears its own wait, and cannot clear another message's wait. applyProcessUpdate also keeps the process updatedAt fresh while it waits.

  • A throttled token endpoint defers too. OAuth2Requester replaces non-definitive refresh errors with a fresh Error on purpose (the original message can hold the client secret). The fresh error now keeps isRateLimited, retryAt, waitMs, reason and module, none of which is secret.

  • Devtools. The one-time scheduler is provisioned when any API module declares static rateLimit. The base role gains sqs:ChangeMessageVisibility.

  • One log record per outcome. Acknowledged: WARN record_deferred. Not acknowledged before the last receive: one WARN (record_deferral_capped, record_visibility_extended, or record_failed with deferral.skipped and the cause as an error). Not acknowledged on the last receive: one ERROR record_lost_rate_limited with the same details, and no WARN.

  • A scheduler that cannot be built is handled like a failed schedule: the record falls back to the visibility tier with scheduleError, instead of failing the whole batch.

  • The scheduler client honours the local endpoint. The EventBridge adapter now takes the same awsConfigOptions() as the SQS clients. Before, a local run with SCHEDULER_ROLE_ARN set sent a long deferral to the real AWS endpoint with the machine's own credentials; the local end-to-end run found it.

Handler contract

Check the last attempt first, then rethrow:

async processBatch({ data, delivery }) {
    try { await this.syncPage(data); }
    catch (error) {
        if (delivery?.isLastAttempt) return this.failRun(data.processId, error);
        throw error; // a RateLimitError too: core puts the message back at retryAt
    }
}

The delay and schedule tiers send a new message, so they never reach the last attempt. The visibility tier and a cap use a receive. On the last receive the worker writes an ERROR, frigg.worker.record_lost_rate_limited, and the handler has that one chance to end the run. (ADR-049 Appendix A.10 rethrows a rate-limited error first; that order leaves the run open in those two cases, so the check moves first.)

Design choices worth a look

  • The mock scheduler is never used for a deferral. The factory returns it for STAGE=dev, and a schedule kept in memory would be lost with the acknowledged message. The worker builds the EventBridge adapter explicitly, and only when SCHEDULER_ROLE_ARN is set and SCHEDULER_PROVIDER is not mock.
  • The queue URL comes from record.eventSourceARN (GetQueueUrl by name, memoized per invocation). The base Worker has no integration name for a <NAME>_QUEUE_URL, and a composed URL breaks LocalStack and other partitions.
  • The Worker's SQS client now takes the offline and endpoint options that QueuerUtil has. Deferral puts GetQueueUrl, SendMessage and ChangeMessageVisibility on the normal path; without it, frigg start and LocalStack would reach real AWS on every deferral.
  • sqs:ChangeMessageVisibility is granted to every stack, not only to one with a declaring module. feat(core): honour Retry-After and let API modules declare rate limits #656 honours Retry-After for every module, so any queue worker can meet a long wait. The statement is already scoped to <service>--<stage>-*Queue; the template gains one action string, and no new resource, function or environment variable. ADR-049 §8 asks for a byte-identical template when no module declares rateLimit; this is the one deviation.
  • @aws-sdk/client-scheduler is now a declared dependency of core. Core already required it (the scheduler adapter is exported from the package root) and got it only because admin-scripts hoists it. The lockfile gains one line; npm ci --dry-run passes.

Caveats

  • Not run against real AWS. Every tier ran end to end against LocalStack, and is unit-tested with aws-sdk-client-mock. Before relying on it in production, run one throttled message per tier on a dev stack and check the queue, the schedule group and CloudTrail.
  • ReportBatchItemFailures and our own ChangeMessageVisibility. The visibility tier reports the record failed after extending its timeout. As far as I know Lambda does not reset the visibility of a reported failure, but I have not verified it. If it does, that tier falls back to today's 30 minute redelivery.
  • A stack whose queue is not its own (ownership.queue: 'external') needs sqs:ChangeMessageVisibility and scheduler access granted separately. Pre-existing and wider now: createSchedulerExecutionRole names <Name>Queue for every integration, so such an app cannot deploy once the scheduler is on.
  • A wait over 26 hours (a monthly quota) is capped at once and falls back to delivery.isLastAttempt. Raise FRIGG_QUEUE_MAX_DEFERRED_MS for an app that wants longer.
  • The handler contract ends a run on the last receive, also when core could still defer. A handler that checks delivery.isLastAttempt first ends the run and returns, so on the third receive a RateLimitError that the delay or schedule tier could still put back ends the run instead. A message reaches its third receive only after two failures that were not deferred, so this is rare.
  • The run state is read, then written. Two workers that write in the same instant can keep the earlier retryAt. It is a UI signal only, and the next write corrects it.
  • Twenty workers on one limit each defer their own copy. Nothing paces them yet.

Not in this PR

The RATE_LIMITED message for the user, pacing, and the policies of the HubSpot, Salesforce and Pipedrive modules.

Test plan

Release notes

  • Upgrade @friggframework/core and @friggframework/devtools together. Devtools grants the IAM action and switches on the scheduler; core does the deferral.
  • Redeploy the stack for the IAM action and, when a module declares rateLimit, for the scheduler resources.

🤖 Generated with Claude Code

📦 Published PR as canary version: 2.0.0--canary.657.f16d20b.0

✨ Test out this PR locally via:

npm install @friggframework/admin-scripts@2.0.0--canary.657.f16d20b.0
npm install @friggframework/core@2.0.0--canary.657.f16d20b.0
npm install @friggframework/devtools@2.0.0--canary.657.f16d20b.0
npm install @friggframework/eslint-config@2.0.0--canary.657.f16d20b.0
npm install @friggframework/prettier-config@2.0.0--canary.657.f16d20b.0
npm install @friggframework/schemas@2.0.0--canary.657.f16d20b.0
npm install @friggframework/serverless-plugin@2.0.0--canary.657.f16d20b.0
npm install @friggframework/test@2.0.0--canary.657.f16d20b.0
npm install @friggframework/ui@2.0.0--canary.657.f16d20b.0
# or 
yarn add @friggframework/admin-scripts@2.0.0--canary.657.f16d20b.0
yarn add @friggframework/core@2.0.0--canary.657.f16d20b.0
yarn add @friggframework/devtools@2.0.0--canary.657.f16d20b.0
yarn add @friggframework/eslint-config@2.0.0--canary.657.f16d20b.0
yarn add @friggframework/prettier-config@2.0.0--canary.657.f16d20b.0
yarn add @friggframework/schemas@2.0.0--canary.657.f16d20b.0
yarn add @friggframework/serverless-plugin@2.0.0--canary.657.f16d20b.0
yarn add @friggframework/test@2.0.0--canary.657.f16d20b.0
yarn add @friggframework/ui@2.0.0--canary.657.f16d20b.0

d-klotz and others added 7 commits September 28, 2026 20:25
queue-deferral.js holds the SQS limits (900 s delay, 12 h visibility), the
two caps that bound how long a rate-limited message is put back
(FRIGG_QUEUE_MAX_DEFERRALS, default 10; FRIGG_QUEUE_MAX_DEFERRED_MS, default
24 h) and the `_frigg` counters a deferred body carries. Core owns both
sides of these numbers, so devtools injects nothing.

Refs ADR-049.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
A throttled token endpoint makes the Requester throw RateLimitError inside
refreshAuth(). OAuth2Requester replaces every non-definitive refresh error
with a fresh Error, on purpose: the original message can hold the request
body and the client secret. The fresh error now carries isRateLimited,
retryAt, waitMs and reason, none of which is secret, so the queue worker can
defer the message to the right time.

Refs ADR-049.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
When a queue handler throws an error with isRateLimited and a retryAt,
Worker.run no longer gives the message back to SQS for a 30 minute
redelivery that spends one of its three receives. It sends the same body
again with DelaySeconds (a wait up to 15 minutes), or creates a one-time
EventBridge schedule at retryAt, or extends the visibility timeout (up to
12 hours). The new message goes out before the old one is acknowledged, so
delivery stays at least once. A deferred body carries `_frigg.deferrals` and
`_frigg.firstDeferredAt`; the handler never sees them.

Two caps bound it: FRIGG_QUEUE_MAX_DEFERRALS (default 10) and
FRIGG_QUEUE_MAX_DEFERRED_MS (default 24 hours, counted from the first
deferral). Past a cap, or with no retryAt, no event source ARN, or a FIFO
queue, the record fails as any retryable error does. The visibility tier
uses one receive, so on the last delivery the worker writes an ERROR
(record_lost_rate_limited) before the message goes to the DLQ.

The mock scheduler is never used: a schedule kept in memory would be lost
with the acknowledged message. recordRateLimitWait and clearRateLimitWait
are hooks a subclass overrides to write run state.

The Worker's SQS client now takes the offline and endpoint options that
QueuerUtil has, because deferral puts GetQueueUrl, SendMessage and
ChangeMessageVisibility on the normal path. @aws-sdk/client-scheduler is now a
declared dependency of core; it was only hoisted from admin-scripts.

Refs ADR-049.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ssage

The queue worker now writes Process.context.rateLimit when it puts a message
back for a rate limit: { status: 'WAITING' | 'EXHAUSTED', mechanism,
retryAt, reason, module, deferrals, updatedAt }. It writes null after the
deferred message runs without error. A UI can then say "waiting for the
provider limit until 23:00 UTC" instead of showing a run that looks stuck,
and applyProcessUpdate keeps the process updatedAt fresh while it waits.

Refs ADR-049.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The one-time scheduler is now provisioned when any API module declares a
`static rateLimit`, so the queue worker can schedule a message that must wait
more than 15 minutes. The base role can now call sqs:ChangeMessageVisibility
on the integration queues, for the wait the scheduler cannot take.

The action is granted to every stack, not only to one with a declaring
module: the Requester honours Retry-After for every module, so any queue
worker can meet a long wait. The statement was already scoped to
`<service>--<stage>-*Queue`; the template gains one action string, and no
new resource, function or environment variable.

Refs ADR-049.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Adds the deferral tiers, the two caps, the `context.rateLimit` run state and
the handler contract (check delivery.isLastAttempt first, then rethrow) to
the core CLAUDE.md files and the integration patterns guide. The delay and
schedule tiers send a new message and never reach the last attempt; the
visibility tier and a cap use a receive, so the handler keeps its last chance
to end the run. The earlier text that said a RateLimitError goes back to SQS
now says the worker puts it back at retryAt.

Refs ADR-049.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Declares recordRateLimitWait, clearRateLimitWait and RateLimitWaitState so a
TypeScript Worker subclass can override the hooks.

Refs ADR-049.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@d-klotz d-klotz added release Create a release when this pr is merged prerelease This change is available in a prerelease. labels Sep 28, 2026
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-28T23:44:40.707723Z ebe5c60 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

d-klotz added a commit that referenced this pull request Sep 28, 2026
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>

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ebe5c602d3

ℹ️ 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".

Comment thread packages/core/core/Worker.js Resolved
d-klotz and others added 2 commits September 28, 2026 20:54
- context.rateLimit is one value per process, so an unrelated message that
  succeeded could wipe it while another message still waited. The queue worker
  now compares times: a new wait replaces the stored one only when its retryAt
  is later; a WAITING value never hides an EXHAUSTED one; after a deferred or
  redelivered message ran, the value is cleared only when it is WAITING and its
  retryAt is past or within 5 s (the scheduler fires up to a second early).
- defer() is split into one helper per tier (delay, schedule, visibility), which
  brings its cognitive complexity under the limit SonarCloud enforces.
- The two no-op hooks have a body, and the redelivery check no longer negates a
  comparison: `!(receiveCount > 1)` is not `receiveCount <= 1` when the count
  is unknown.

Refs ADR-049.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@d-klotz

d-klotz commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the automated review in the latest commit:

  • Shared context.rateLimit (Codex P2): the value is one per process, so an unrelated message that succeeded could clear it while another message still waited. The queue worker now compares times. A new wait replaces the stored one only when its retryAt is later, a WAITING value never hides an EXHAUSTED one, and after a deferred or redelivered message ran the worker clears the value only when it is WAITING and its retryAt is past or within 5 s (the scheduler fires up to a second early). Both hooks now read the process first; a failed read is logged and never changes the outcome. 15 tests cover it.
  • SonarCloud: defer() is split into one helper per tier, which brings its cognitive complexity under the limit. The two no-op hooks have a body. The redelivery check no longer negates a comparison, because !(receiveCount > 1) is not receiveCount <= 1 when the count is unknown.

This branch also merges the fixes made to #656 (the review of the Requester), so the stack stays in sync.

Full packages/core run: 2,769 pass, and the same 63 failures as origin/next in this environment.

@claude claude Bot 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.

Beyond the inline findings, I also checked two other candidate issues and ruled them out: context.rateLimit (backend-utils.js) is a brand-new field with no prior usage anywhere in the codebase, so there's no overwrite risk for existing integration data; and the visibility-extension deferral path for externally-owned/non-scheduler queues is still bounded by SQS's native receive-count/isLastAttempt check (independent of the body-embedded deferral counter), so it can't extend visibility forever even though that counter itself doesn't advance on redelivery.

Extended reasoning...

This PR is a large, security/reliability-sensitive change to the queue worker's rate-limit deferral logic (Worker.js, backend-utils.js, oauth-2.js, scheduler-builder.js) with three confirmed findings already flagged inline (a scheduler IAM/deploy gap, a lost module name on OAuth rate limits, and a rate-limit-state clearing race across sibling messages). I independently traced two additional candidate concerns from the ruled-out list — a potential field-overwrite in backend-utils.js and a potential unbounded-visibility-extension loop for external queues in Worker.js — and confirmed via the code (grep for prior context.rateLimit usage, and the delivery.isLastAttempt gate at Worker.js's visibility branch) that neither holds up as a bug.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Comment thread packages/core/core/Worker.js Resolved

@claude claude Bot 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.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

d-klotz and others added 11 commits September 29, 2026 14:20
The JSDoc blocks of defer(), the schedule tier, getSchedulerService(), the two
run-state hooks and the deferral limits repeat what the names and the docs
(packages/core/CLAUDE.md section 7, core/CLAUDE.md) already say.

The two default hooks return a resolved promise instead of holding a comment,
so they still have a body for the empty-function check.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The rate-limit switch is named by declaresRateLimit(), so the comment above
the integration check goes back to its original line.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e helper

deferredBody() and deferredRecord() replace the six inline copies of a body
with _frigg counters.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The HubSpot and Salesforce modules probe a daily limit every hour, because
neither provider says when it resets. With 10 deferrals, a daily limit that
starts more than about 10 hours before its reset used the cap before the
reset, and the sync failed. 30 deferrals and 26 hours cover one full day of
hourly probes, each of which can start a little late. The count cap still
stops a message that loops with very short waits.

The Worker tests now derive their over-the-cap waits from MAX_DEFERRED_MS.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts:
#	docs/guides/INTEGRATION-PATTERNS.md
#	docs/reference/api-module-definition-and-functions.md
queue-deferral.js had a copy of the helper from queue-delivery.js. It now
imports it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…unts from the receive

SQS counts the 12 hour maximum of a visibility timeout from the receive, an
extension does not reset it, and a request past the time left fails. The
worker asked for up to 43 200 s, so a wait of 12 h or more ended as
visibility_failed and fell back to the 30 minute redelivery. It now asks for
at most 43 200 s less the longest Lambda run (900 s) and the longest
batching window (300 s): 42 000 s.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
defer() built a ten-field bag and a run-state closure that each tier took
apart again. The tiers now take one deferral object: the record, the queue,
the body and the deferred body, the error, the delivery, the retry time,
the wait and the counters.

- _acknowledgeDeferral holds the hook, log and acknowledge tail that the
  delay and the schedule tier shared.
- _deferralLogFields builds the log fields once.
- deferredMs() in queue-deferral.js is the one formula for the time from
  the first deferral to the retry time.
- The schedule name no longer falls back to a random UUID: an SQS record
  always has a messageId, and defer() already needs its eventSourceARN.

No behaviour change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On the last receive, a capped or visibility outcome wrote a WARN and then
the ERROR record_lost_rate_limited, both with the error, for one outcome.
LOGGING.md asks for one record at the boundary. A deferral that failed on
the last receive (a failed send or visibility change) wrote only the WARN
record_failed, although the message goes to the dead-letter queue next.

A rate-limited record that is not acknowledged now writes one WARN before
its last receive and exactly one ERROR record_lost_rate_limited on it, with
the details the WARN would have had.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
deferral.cause and scheduleError held only the message of the error. The
logger serializes an error at any depth, so they now keep the error with its
type and stack. A missing IAM permission shows up there.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
getSchedulerService() ran outside the try of the schedule tier, so an error
from createSchedulerService left Worker.run and failed the whole batch. It
now runs inside the try, and the record falls back to the visibility tier
with scheduleError. A failed build is no longer remembered as "no
scheduler", so every record logs the error, not only the first one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d-klotz and others added 6 commits September 29, 2026 18:58
The QueueWorker hooks called the process repository directly and held
the merge rules for context.rateLimit, against the core rule that
handlers go through use cases. RecordRateLimitWait and
ClearRateLimitWait now own those rules; the hooks keep their processId
guard, build the use case and call execute. Behaviour is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
QueueWorker.run had no test through the clear path. A deferred message
whose handler succeeds now clears context.rateLimit on its process; a
first delivery leaves it in place.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… handler contract

The caps are now 30 deferrals and 26 hours by default. The visibility tier
sets at most 12 hours less 20 minutes, because SQS counts its 12 hour maximum
from the receive. A rate-limited record gets one log record per outcome, and
one ERROR on the last attempt.

The handler example ends the run and returns on the last attempt instead of
ending it and rethrowing, and the delay and schedule tiers are no longer said
to spare the receive that the visibility tier and a cap use. The scheduler
docs now say that a module with a static rateLimit provisions it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A refresh that fails for a reason other than a definitive rejection throws a
fresh Error, because the original can hold the client secret. It copied the
rate-limit fields but not the module name, so a throttled token endpoint
deferred with no module in the worker's log and in Process.context.rateLimit.
The module name is not secret, so it is copied too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The scheduler is now provisioned when an API module declares a static
rateLimit. The new test builds an app whose module declares none through the
integration and scheduler builders, and compares its resource, function and
environment keys with those of the same integration without modules. A
scheduler that turns on for such an app now fails the test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The core tests mock the AWS SDK clients with aws-sdk-client-mock, but only
devtools declared it: core resolved the copy that npm hoisted for devtools.
Core now declares the same range, ^4.1.0, and the lockfile still holds one
copy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@claude claude Bot 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.

⚠️ Code review skipped — your organization has no extra usage available to pay for this review.

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.

d-klotz and others added 2 commits September 30, 2026 11:21
…l run

The queue worker's SQS client takes awsConfigOptions() (the IS_OFFLINE
credentials and AWS_ENDPOINT), so a local run reaches LocalStack. The
EventBridge scheduler adapter built its client with the region only. In a
local run with SCHEDULER_ROLE_ARN set, a deferral over 15 minutes therefore
went to the real AWS endpoint with the machine's own credentials.

A local end-to-end run against LocalStack found it: the schedule tier failed
with CredentialsProviderError and fell back to the visibility tier. The
adapter now takes the same options as the SQS clients.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The run state (`Process.context.rateLimit`) was cleared only when its
`retryAt` was at most 5 s away, so that one message could not wipe a wait
that another message had set. A job that resumed before its retry time
therefore left its own wait behind. A local end-to-end run showed it: the
lab fired a 1 h schedule early, the sync completed, and the process kept
`status: 'WAITING'`, and nothing cleared it later. In AWS this happens
when a job runs before its `retryAt`: on the visibility tier for waits
over 11 h 40 min (SQS returns the message at 42,000 s), or when someone
sends the message again by hand.

The deferred body now carries `_frigg.deferralId`: the SQS id of the
first message that was put back, and every later deferral of that work
keeps it. The run state stores it. After a deferred or redelivered
message succeeds, the worker passes it to
`clearRateLimitWait(body, { deferralId })`; the visibility tier does not
rewrite the body, so there the message's own id is used.
`ClearRateLimitWait` clears a WAITING value at once when the ids match,
and otherwise only when its `retryAt` is past or within 5 s, as before.

Known limit, the same kind as before: the run state holds one wait per
process. With two waits at once it keeps the later one; if that job
resumes early and clears it, the earlier wait of the other message no
longer shows.

Refs ADR-049.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d-klotz added a commit that referenced this pull request Sep 30, 2026
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>
@sonarqubecloud

Copy link
Copy Markdown

@cursor

cursor Bot commented Oct 24, 2026

Copy link
Copy Markdown

Frigg 2.0.1 review (Group 2) — COMMENT only

Checklist

  • Requeue / deferral decision lives in the worker or eventing, not the API module: pass
    • Worker.js + queues/queue-deferral.js catch RateLimitError, SQS DelaySeconds ≤15m, EventBridge Scheduler beyond that, visibility fallback if scheduler unavailable.
    • API/Requester still only waits in-process or throws (from #656).

Verdict (would): would approve

Findings

None blocking. One nit: ensure scheduler IAM / SCHEDULER_ROLE_ARN failure modes are called out in operator docs (tests cover visibility fallback). Stack on #656 as intended.

d-klotz and others added 3 commits November 9, 2026 13:03
#656 now throws RateLimitError at once for a non-429 limit that classify()
names with no time, with retryAt and waitMs undefined. The Worker must not
send, schedule or extend visibility for it. The old test used a plain
error with isRateLimited; this adds the real error shape.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The deferral docs said the worker puts every RateLimitError back at
retryAt. A non-429 limit with no time has no retryAt, so the message
returns to SQS like any retryable error.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@claude claude Bot 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.

⚠️ Code review skipped — your organization's extra usage balance is too low to start another review, so this review was not started.

An organization admin can add extra usage credits at claude.ai/admin-settings/usage.

Once credits are available, someone with write access to this repository can comment @claude review on this pull request to trigger a review.

Comment thread packages/core/core/Worker.js Resolved
With a delay of 0 or a few seconds, the new message could run and call
clearRateLimitWait before the old one wrote the wait. The clear found
nothing, the late write then left the run WAITING with a past retryAt.

The delay tier now writes the wait before the send, and clears it again
when the send fails. Schedule and visibility keep writing after the
change: their message cannot run before retryAt.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d-klotz added a commit that referenced this pull request Nov 9, 2026
Phase 1 alone sent a rate-limited 403 back to SQS. With the generated
queue config (3 receives about 30 min apart) a daily limit then reached
the DLQ after about an hour, so the work was still lost. The halt
exemption moves to the deferral change (#657), which puts the message
back at its retry time. Phase 1 keeps the halt rule of next.

The reference doc now also says that a 429 reads all default parsers,
not only Retry-After.

Refs ADR-049 §7.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d-klotz and others added 2 commits November 9, 2026 18:41
…it-defer

#656 now keeps halting a rate-limited 4xx (ADR-049 §7). This branch puts
the message back at its retry time, so it keeps the halt exemption and
its test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A rate-limit policy turns the scheduler on. Its execution role used
Fn::GetAtt on <Name>Queue for every integration, but the stack creates
no such resource for a queue with ownership.queue 'external'. The
template then could not deploy. The role now takes the ARN of an
external queue from its queue.url.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sonarqubecloud

sonarqubecloud Bot commented Nov 9, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

prerelease This change is available in a prerelease. release Create a release when this pr is merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant