From ef792e4611e336454d7f1a2682b34dc771e91bae Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 20:25:38 -0300 Subject: [PATCH 01/30] feat(core): add the constants and counters for deferred queue messages 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 --- packages/core/queues/queue-deferral.js | 79 ++++++++ packages/core/queues/queue-deferral.test.js | 192 ++++++++++++++++++++ 2 files changed, 271 insertions(+) create mode 100644 packages/core/queues/queue-deferral.js create mode 100644 packages/core/queues/queue-deferral.test.js diff --git a/packages/core/queues/queue-deferral.js b/packages/core/queues/queue-deferral.js new file mode 100644 index 000000000..3af58a536 --- /dev/null +++ b/packages/core/queues/queue-deferral.js @@ -0,0 +1,79 @@ +const MAX_DEFERRALS = 10; +const MAX_DEFERRED_MS = 24 * 60 * 60 * 1000; +const MAX_DELAY_SECONDS = 900; +const MAX_VISIBILITY_TIMEOUT_SECONDS = 43_200; +const MAX_DEFERRALS_ENV = 'FRIGG_QUEUE_MAX_DEFERRALS'; +const MAX_DEFERRED_MS_ENV = 'FRIGG_QUEUE_MAX_DEFERRED_MS'; + +const toPositiveInteger = (value) => { + const number = Number(value); + return Number.isInteger(number) && number > 0 ? number : undefined; +}; + +/** + * @typedef {Object} DeferralLimits + * @property {number} maxDeferrals Times one message may be put back for a rate limit. + * @property {number} maxDeferredMs Longest total time from the first deferral to the last retry time. + */ + +/** + * @param {Object} [env=process.env] + * @returns {DeferralLimits} + */ +const readDeferralLimits = (env = process.env) => ({ + maxDeferrals: toPositiveInteger(env[MAX_DEFERRALS_ENV]) ?? MAX_DEFERRALS, + maxDeferredMs: + toPositiveInteger(env[MAX_DEFERRED_MS_ENV]) ?? MAX_DEFERRED_MS, +}); + +/** + * The deferral counters a message body carries in `_frigg`. + * @returns {{deferrals: number, firstDeferredAt: string|undefined}} + */ +const readDeferral = (body) => { + const frigg = body?._frigg; + const deferrals = + Number.isInteger(frigg?.deferrals) && frigg.deferrals > 0 + ? frigg.deferrals + : 0; + const firstDeferredAt = + typeof frigg?.firstDeferredAt === 'string' && + !Number.isNaN(Date.parse(frigg.firstDeferredAt)) + ? frigg.firstDeferredAt + : undefined; + return { deferrals, firstDeferredAt }; +}; + +const nextDeferral = (body, now = Date.now()) => { + const current = readDeferral(body); + return { + deferrals: current.deferrals + 1, + firstDeferredAt: current.firstDeferredAt ?? new Date(now).toISOString(), + }; +}; + +const withDeferral = (body, deferral) => ({ + ...body, + _frigg: { ...body._frigg, ...deferral }, +}); + +const isDeferralCapped = ( + { deferrals, firstDeferredAt, retryAt }, + { maxDeferrals, maxDeferredMs } +) => + deferrals > maxDeferrals || + retryAt.getTime() - Date.parse(firstDeferredAt) > maxDeferredMs; + +module.exports = { + MAX_DEFERRALS, + MAX_DEFERRALS_ENV, + MAX_DEFERRED_MS, + MAX_DEFERRED_MS_ENV, + MAX_DELAY_SECONDS, + MAX_VISIBILITY_TIMEOUT_SECONDS, + isDeferralCapped, + nextDeferral, + readDeferral, + readDeferralLimits, + withDeferral, +}; diff --git a/packages/core/queues/queue-deferral.test.js b/packages/core/queues/queue-deferral.test.js new file mode 100644 index 000000000..76c3ca517 --- /dev/null +++ b/packages/core/queues/queue-deferral.test.js @@ -0,0 +1,192 @@ +const { + MAX_DEFERRALS, + MAX_DEFERRALS_ENV, + MAX_DEFERRED_MS, + MAX_DEFERRED_MS_ENV, + MAX_DELAY_SECONDS, + MAX_VISIBILITY_TIMEOUT_SECONDS, + isDeferralCapped, + nextDeferral, + readDeferral, + readDeferralLimits, + withDeferral, +} = require('./queue-deferral'); + +const NOW = Date.parse('2026-09-28T12:00:00.000Z'); +const HOUR = 60 * 60 * 1000; + +describe('queue-deferral', () => { + it('exposes the SQS limits and the default caps', () => { + expect(MAX_DELAY_SECONDS).toBe(900); + expect(MAX_VISIBILITY_TIMEOUT_SECONDS).toBe(43_200); + expect(MAX_DEFERRALS).toBe(10); + expect(MAX_DEFERRED_MS).toBe(24 * HOUR); + expect(MAX_DEFERRALS_ENV).toBe('FRIGG_QUEUE_MAX_DEFERRALS'); + expect(MAX_DEFERRED_MS_ENV).toBe('FRIGG_QUEUE_MAX_DEFERRED_MS'); + }); + + describe('readDeferralLimits', () => { + it('returns the defaults when the environment sets nothing', () => { + expect(readDeferralLimits({})).toEqual({ + maxDeferrals: 10, + maxDeferredMs: 24 * HOUR, + }); + }); + + it('reads both limits from the environment', () => { + expect( + readDeferralLimits({ + [MAX_DEFERRALS_ENV]: '3', + [MAX_DEFERRED_MS_ENV]: '7200000', + }) + ).toEqual({ maxDeferrals: 3, maxDeferredMs: 7_200_000 }); + }); + + it.each(['0', '-1', '1.5', 'many', ''])( + 'ignores the value %j', + (value) => { + expect( + readDeferralLimits({ + [MAX_DEFERRALS_ENV]: value, + [MAX_DEFERRED_MS_ENV]: value, + }) + ).toEqual({ maxDeferrals: 10, maxDeferredMs: 24 * HOUR }); + } + ); + + it('reads process.env when no environment is given', () => { + const previous = process.env[MAX_DEFERRALS_ENV]; + process.env[MAX_DEFERRALS_ENV] = '4'; + try { + expect(readDeferralLimits().maxDeferrals).toBe(4); + } finally { + if (previous === undefined) + delete process.env[MAX_DEFERRALS_ENV]; + else process.env[MAX_DEFERRALS_ENV] = previous; + } + }); + }); + + describe('readDeferral', () => { + it('reads zero deferrals from a body with no _frigg', () => { + expect(readDeferral({ event: 'X' })).toEqual({ + deferrals: 0, + firstDeferredAt: undefined, + }); + expect(readDeferral(undefined)).toEqual({ + deferrals: 0, + firstDeferredAt: undefined, + }); + }); + + it('reads the counters of a deferred body', () => { + const firstDeferredAt = new Date(NOW).toISOString(); + expect( + readDeferral({ _frigg: { deferrals: 2, firstDeferredAt } }) + ).toEqual({ deferrals: 2, firstDeferredAt }); + }); + + it.each([ + [{ deferrals: -1 }], + [{ deferrals: 1.5 }], + [{ deferrals: 'two' }], + [{ firstDeferredAt: 'yesterday' }], + [{ firstDeferredAt: 5 }], + ])('ignores an invalid counter %j', (frigg) => { + expect(readDeferral({ _frigg: frigg })).toEqual({ + deferrals: 0, + firstDeferredAt: undefined, + }); + }); + }); + + describe('nextDeferral', () => { + it('starts at 1 and stamps the first deferral time', () => { + expect(nextDeferral({ event: 'X' }, NOW)).toEqual({ + deferrals: 1, + firstDeferredAt: new Date(NOW).toISOString(), + }); + }); + + it('adds one and keeps the first deferral time', () => { + const firstDeferredAt = new Date(NOW - HOUR).toISOString(); + expect( + nextDeferral({ _frigg: { deferrals: 2, firstDeferredAt } }, NOW) + ).toEqual({ deferrals: 3, firstDeferredAt }); + }); + }); + + describe('withDeferral', () => { + it('puts the counters in _frigg and keeps the rest of the body', () => { + const body = { + event: 'X', + data: { processId: 4 }, + _frigg: { other: true }, + }; + const deferral = { + deferrals: 1, + firstDeferredAt: new Date(NOW).toISOString(), + }; + expect(withDeferral(body, deferral)).toEqual({ + event: 'X', + data: { processId: 4 }, + _frigg: { other: true, ...deferral }, + }); + expect(body._frigg).toEqual({ other: true }); + }); + }); + + describe('isDeferralCapped', () => { + const limits = { maxDeferrals: 3, maxDeferredMs: 2 * HOUR }; + const first = new Date(NOW).toISOString(); + + it('does not cap a deferral inside both limits', () => { + expect( + isDeferralCapped( + { + deferrals: 3, + firstDeferredAt: first, + retryAt: new Date(NOW + HOUR), + }, + limits + ) + ).toBe(false); + }); + + it('caps a deferral past the count', () => { + expect( + isDeferralCapped( + { + deferrals: 4, + firstDeferredAt: first, + retryAt: new Date(NOW + 1_000), + }, + limits + ) + ).toBe(true); + }); + + it('caps a deferral whose retry time is past the total time, counted from the first deferral', () => { + expect( + isDeferralCapped( + { + deferrals: 2, + firstDeferredAt: first, + retryAt: new Date(NOW + 2 * HOUR + 1), + }, + limits + ) + ).toBe(true); + expect( + isDeferralCapped( + { + deferrals: 2, + firstDeferredAt: first, + retryAt: new Date(NOW + 2 * HOUR), + }, + limits + ) + ).toBe(false); + }); + }); +}); From 288b760a8a420e22cbf68ca3367c977a90c1162f Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 20:28:54 -0300 Subject: [PATCH 02/30] feat(core): keep the rate limit on a failed token refresh 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 --- packages/core/modules/requester/oauth-2.js | 6 +++ .../core/modules/requester/oauth-2.test.js | 50 +++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/packages/core/modules/requester/oauth-2.js b/packages/core/modules/requester/oauth-2.js index 2bafb9d84..b1ae7b667 100644 --- a/packages/core/modules/requester/oauth-2.js +++ b/packages/core/modules/requester/oauth-2.js @@ -398,6 +398,12 @@ class OAuth2Requester extends Requester { ); transportError.statusCode = status; transportError.isTokenRefreshTransportFailure = true; + if (error?.isRateLimited) { + transportError.isRateLimited = true; + transportError.retryAt = error.retryAt; + transportError.waitMs = error.waitMs; + transportError.reason = error.reason; + } return transportError; } diff --git a/packages/core/modules/requester/oauth-2.test.js b/packages/core/modules/requester/oauth-2.test.js index 9e55a0059..41b9cf3aa 100644 --- a/packages/core/modules/requester/oauth-2.test.js +++ b/packages/core/modules/requester/oauth-2.test.js @@ -1,4 +1,5 @@ const { OAuth2Requester } = require('./oauth-2'); +const { RateLimitError } = require('../../errors'); describe('OAuth2Requester', () => { describe('constructor', () => { @@ -175,6 +176,55 @@ describe('OAuth2Requester', () => { ); }); + it('keeps the rate-limit fields on the failure of a throttled token endpoint', async () => { + const requester = new OAuth2Requester({ + grant_type: 'authorization_code', + refresh_token: 'test-refresh-token', + credentialReloadBackoffMs: [], + }); + const retryAt = new Date(Date.now() + 3_600_000); + requester.refreshAccessToken = jest.fn().mockRejectedValue( + new RateLimitError({ + resource: 'https://auth.example.com/token', + response: { status: 429, bodyUsed: true }, + hint: { retryAt, reason: 'burst', source: 'header' }, + }) + ); + requester.notify = jest.fn(); + + const error = await requester.refreshAuth().catch((e) => e); + + expect(error).toMatchObject({ + isRateLimited: true, + statusCode: 429, + reason: 'burst', + }); + expect(error.retryAt).toEqual(retryAt); + expect(error.waitMs).toBeGreaterThan(0); + expect(error.message).not.toContain('auth.example.com'); + expect(requester.notify).not.toHaveBeenCalledWith( + requester.DLGT_INVALID_AUTH, + expect.anything() + ); + }); + + it('does not flag an ordinary transport failure as rate limited', async () => { + const requester = new OAuth2Requester({ + grant_type: 'authorization_code', + refresh_token: 'test-refresh-token', + credentialReloadBackoffMs: [], + }); + requester.refreshAccessToken = jest + .fn() + .mockRejectedValue(new Error('socket hang up')); + requester.notify = jest.fn(); + + const error = await requester.refreshAuth().catch((e) => e); + + expect(error.isRateLimited).toBeUndefined(); + expect(error.retryAt).toBeUndefined(); + }); + it('should return false and notify DLGT_INVALID_AUTH on error during client_credentials refresh', async () => { const requester = new OAuth2Requester({ grant_type: 'client_credentials', From f9c3b5df9ebbb1875f4bed220f2296f753762cf8 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 20:33:12 -0300 Subject: [PATCH 03/30] feat(core): put a rate-limited queue message back at its retry time 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 --- package-lock.json | 1 + packages/core/core/Worker.deferral.test.js | 734 +++++++++++++++++++++ packages/core/core/Worker.js | 296 ++++++++- packages/core/package.json | 1 + packages/core/queues/queuer-util.js | 2 +- 5 files changed, 1028 insertions(+), 6 deletions(-) create mode 100644 packages/core/core/Worker.deferral.test.js diff --git a/package-lock.json b/package-lock.json index 1af7843c1..10af5cf83 100644 --- a/package-lock.json +++ b/package-lock.json @@ -34029,6 +34029,7 @@ "@aws-sdk/client-kms": "^3.588.0", "@aws-sdk/client-lambda": "^3.714.0", "@aws-sdk/client-s3": "^3.588.0", + "@aws-sdk/client-scheduler": "^3.588.0", "@aws-sdk/client-sqs": "^3.588.0", "@aws-sdk/client-ssm": "^3.588.0", "@aws-sdk/s3-request-presigner": "^3.588.0", diff --git a/packages/core/core/Worker.deferral.test.js b/packages/core/core/Worker.deferral.test.js new file mode 100644 index 000000000..285362916 --- /dev/null +++ b/packages/core/core/Worker.deferral.test.js @@ -0,0 +1,734 @@ +const { mockClient } = require('aws-sdk-client-mock'); +const { + ChangeMessageVisibilityCommand, + GetQueueUrlCommand, + SQSClient, + SendMessageCommand, +} = require('@aws-sdk/client-sqs'); +const { + CreateScheduleCommand, + SchedulerClient, +} = require('@aws-sdk/client-scheduler'); +const { Worker } = require('./Worker'); +const { RateLimitError } = require('../errors'); +const { createMemorySink } = require('../logs'); +const { + MAX_DEFERRALS_ENV, + MAX_DEFERRED_MS_ENV, +} = require('../queues/queue-deferral'); + +const NOW = Date.parse('2026-09-28T12:00:00.000Z'); +const SECOND = 1_000; +const MINUTE = 60 * SECOND; +const HOUR = 60 * MINUTE; + +const QUEUE_NAME = 'app--dev-ExampleQueue'; +const QUEUE_ARN = `arn:aws:sqs:us-east-1:123456789012:${QUEUE_NAME}`; +const QUEUE_URL = `https://sqs.us-east-1.amazonaws.com/123456789012/${QUEUE_NAME}`; +const SCHEDULER_ROLE_ARN = + 'arn:aws:iam::123456789012:role/app-dev-scheduler-role'; +const BODY = { event: 'SYNC_PAGE', data: { processId: 7 } }; + +const ENV_KEYS = [ + 'AWS_ENDPOINT', + 'FRIGG_QUEUE_MAX_RECEIVE_COUNT', + 'IS_OFFLINE', + 'SCHEDULE_GROUP_NAME', + 'SCHEDULER_PROVIDER', + 'SCHEDULER_ROLE_ARN', + MAX_DEFERRALS_ENV, + MAX_DEFERRED_MS_ENV, +]; + +function rateLimited(waitMs, { reason = 'burst', module = 'hubspot' } = {}) { + return new RateLimitError({ + resource: 'https://api.example.com/v1/items', + response: { status: 429, bodyUsed: true }, + hint: { waitMs, reason, source: 'header' }, + waitMs, + module, + now: NOW, + }); +} + +function sqsRecord(overrides = {}) { + return { + messageId: 'msg-1', + receiptHandle: 'receipt-1', + eventSourceARN: QUEUE_ARN, + attributes: { ApproximateReceiveCount: '1' }, + body: JSON.stringify(BODY), + ...overrides, + }; +} + +describe('Worker rate-limit deferral (ADR-049)', () => { + let sqsMock; + let schedulerMock; + let sink; + let worker; + let savedEnv; + + beforeEach(() => { + savedEnv = {}; + for (const key of ENV_KEYS) { + savedEnv[key] = process.env[key]; + delete process.env[key]; + } + jest.spyOn(Date, 'now').mockReturnValue(NOW); + jest.spyOn(console, 'log').mockImplementation(() => {}); + jest.spyOn(console, 'warn').mockImplementation(() => {}); + sqsMock = mockClient(SQSClient); + schedulerMock = mockClient(SchedulerClient); + sqsMock.on(GetQueueUrlCommand).resolves({ QueueUrl: QUEUE_URL }); + sqsMock.on(SendMessageCommand).resolves({ MessageId: 'new-message' }); + sqsMock.on(ChangeMessageVisibilityCommand).resolves({}); + schedulerMock.on(CreateScheduleCommand).resolves({ + ScheduleArn: + 'arn:aws:scheduler:us-east-1:123456789012:schedule/g/n', + }); + sink = createMemorySink(); + worker = new Worker(); + worker._run = jest.fn(); + }); + + afterEach(() => { + jest.restoreAllMocks(); + sqsMock.restore(); + schedulerMock.restore(); + for (const key of ENV_KEYS) { + if (savedEnv[key] === undefined) delete process.env[key]; + else process.env[key] = savedEnv[key]; + } + }); + + const sent = () => + sqsMock + .commandCalls(SendMessageCommand) + .map((call) => call.args[0].input); + const sentBody = (index = 0) => JSON.parse(sent()[index].MessageBody); + const visibility = () => + sqsMock + .commandCalls(ChangeMessageVisibilityCommand) + .map((call) => call.args[0].input); + const schedules = () => + schedulerMock + .commandCalls(CreateScheduleCommand) + .map((call) => call.args[0].input); + const logged = (name) => + sink.records.filter((r) => r.eventName === `frigg.worker.${name}`); + const run = (record = sqsRecord()) => worker.run({ Records: [record] }); + const failure = (id = 'msg-1') => ({ + batchItemFailures: [{ itemIdentifier: id }], + }); + + describe('a wait of up to 15 minutes', () => { + it('puts the message back with DelaySeconds and a deferral counter, and reports it handled', async () => { + worker._run.mockRejectedValue(rateLimited(60 * SECOND)); + + const result = await run(); + + expect(result).toEqual({ batchItemFailures: [] }); + expect(sent()).toHaveLength(1); + expect(sent()[0]).toMatchObject({ + QueueUrl: QUEUE_URL, + DelaySeconds: 60, + }); + expect(sentBody()).toEqual({ + ...BODY, + _frigg: { + deferrals: 1, + firstDeferredAt: new Date(NOW).toISOString(), + }, + }); + expect(worker._run).toHaveBeenCalledTimes(1); + }); + + it('rounds the delay up to whole seconds', async () => { + worker._run.mockRejectedValue(rateLimited(1_500)); + await run(); + expect(sent()[0].DelaySeconds).toBe(2); + }); + + it('sends with a delay of 900 seconds for a wait of exactly 15 minutes', async () => { + worker._run.mockRejectedValue(rateLimited(15 * MINUTE)); + await run(); + expect(sent()[0].DelaySeconds).toBe(900); + expect(schedules()).toEqual([]); + }); + + it('sends with no delay when the retry time has passed', async () => { + const error = new RateLimitError({ + resource: 'https://api.example.com/v1/items', + response: { status: 429, bodyUsed: true }, + hint: { retryAt: new Date(NOW - 5 * SECOND), source: 'header' }, + now: NOW, + }); + worker._run.mockRejectedValue(error); + + await run(); + + expect(sent()[0].DelaySeconds).toBe(0); + }); + + it('counts up and keeps the first deferral time on a message that was deferred before', async () => { + const firstDeferredAt = new Date(NOW - 10 * MINUTE).toISOString(); + worker._run.mockRejectedValue(rateLimited(60 * SECOND)); + + await run( + sqsRecord({ + body: JSON.stringify({ + ...BODY, + _frigg: { deferrals: 2, firstDeferredAt }, + }), + }) + ); + + expect(sentBody()._frigg).toEqual({ + deferrals: 3, + firstDeferredAt, + }); + }); + + it('looks the queue URL up from the event source ARN, once per queue', async () => { + worker._run.mockRejectedValue(rateLimited(60 * SECOND)); + + await worker.run({ + Records: [sqsRecord(), sqsRecord({ messageId: 'msg-2' })], + }); + + const lookups = sqsMock.commandCalls(GetQueueUrlCommand); + expect(lookups).toHaveLength(1); + expect(lookups[0].args[0].input).toEqual({ QueueName: QUEUE_NAME }); + expect(sent()).toHaveLength(2); + }); + + it('tries the queue URL lookup again after it failed', async () => { + sqsMock + .on(GetQueueUrlCommand) + .rejectsOnce(new Error('throttled')) + .resolves({ QueueUrl: QUEUE_URL }); + worker._run.mockRejectedValue(rateLimited(60 * SECOND)); + + const result = await worker.run({ + Records: [sqsRecord(), sqsRecord({ messageId: 'msg-2' })], + }); + + expect(result).toEqual(failure('msg-1')); + expect(sent()).toHaveLength(1); + }); + + it('hands the deferred message to _run with the counters and a fresh delivery', async () => { + const firstDeferredAt = new Date(NOW - MINUTE).toISOString(); + const body = { ...BODY, _frigg: { deferrals: 1, firstDeferredAt } }; + worker._run.mockResolvedValue(undefined); + + await run(sqsRecord({ body: JSON.stringify(body) })); + + expect(worker._run).toHaveBeenCalledWith( + body, + {}, + expect.objectContaining({ receiveCount: 1 }) + ); + }); + }); + + describe('a wait over 15 minutes', () => { + beforeEach(() => { + process.env.SCHEDULER_ROLE_ARN = SCHEDULER_ROLE_ARN; + }); + + it('schedules the message at the retry time with the one-time scheduler', async () => { + process.env.SCHEDULE_GROUP_NAME = 'app-dev-schedules'; + worker._run.mockRejectedValue(rateLimited(20 * MINUTE)); + + const result = await run(); + + expect(result).toEqual({ batchItemFailures: [] }); + expect(sent()).toEqual([]); + expect(schedules()).toHaveLength(1); + const [schedule] = schedules(); + expect(schedule).toMatchObject({ + Name: 'frigg-defer-msg-1', + GroupName: 'app-dev-schedules', + ScheduleExpression: 'at(2026-09-28T12:20:00)', + ActionAfterCompletion: 'DELETE', + Target: { Arn: QUEUE_ARN, RoleArn: SCHEDULER_ROLE_ARN }, + }); + expect(JSON.parse(schedule.Target.Input)).toEqual({ + ...BODY, + _frigg: { + deferrals: 1, + firstDeferredAt: new Date(NOW).toISOString(), + }, + }); + }); + + it('takes a schedule that exists already as scheduled', async () => { + schedulerMock + .on(CreateScheduleCommand) + .rejects( + Object.assign(new Error('exists'), { + name: 'ConflictException', + }) + ); + worker._run.mockRejectedValue(rateLimited(20 * MINUTE)); + + const result = await run(); + + expect(result).toEqual({ batchItemFailures: [] }); + expect(visibility()).toEqual([]); + }); + + it('extends the visibility timeout when the schedule cannot be created, and reports the record failed', async () => { + schedulerMock + .on(CreateScheduleCommand) + .rejects(new Error('scheduler is down')); + worker._run.mockRejectedValue(rateLimited(20 * MINUTE)); + + const result = await run(); + + expect(result).toEqual(failure()); + expect(visibility()).toEqual([ + { + QueueUrl: QUEUE_URL, + ReceiptHandle: 'receipt-1', + VisibilityTimeout: 1200, + }, + ]); + expect(logged('record_visibility_extended')).toEqual([ + expect.objectContaining({ + scheduleError: expect.stringContaining('scheduler is down'), + }), + ]); + }); + + it('never uses the mock scheduler, even when the environment names it', async () => { + process.env.SCHEDULER_PROVIDER = 'mock'; + worker._run.mockRejectedValue(rateLimited(20 * MINUTE)); + + const result = await run(); + + expect(result).toEqual(failure()); + expect(schedules()).toEqual([]); + expect(visibility()).toHaveLength(1); + }); + }); + + describe('a wait over 15 minutes with no scheduler', () => { + it('extends the visibility timeout to the retry time and reports the record failed', async () => { + worker._run.mockRejectedValue(rateLimited(20 * MINUTE)); + + const result = await run(); + + expect(result).toEqual(failure()); + expect(sent()).toEqual([]); + expect(visibility()).toEqual([ + { + QueueUrl: QUEUE_URL, + ReceiptHandle: 'receipt-1', + VisibilityTimeout: 1200, + }, + ]); + }); + + it('caps the visibility timeout at 12 hours', async () => { + process.env[MAX_DEFERRED_MS_ENV] = String(48 * HOUR); + worker._run.mockRejectedValue(rateLimited(20 * HOUR)); + + await run(); + + expect(visibility()[0].VisibilityTimeout).toBe(43_200); + }); + + it('leaves the message to SQS when the visibility change fails', async () => { + sqsMock + .on(ChangeMessageVisibilityCommand) + .rejects(new Error('gone')); + worker._run.mockRejectedValue(rateLimited(20 * MINUTE)); + + const result = await run(); + + expect(result).toEqual(failure()); + expect(logged('record_failed')).toEqual([ + expect.objectContaining({ + deferral: { + skipped: 'visibility_failed', + cause: expect.stringContaining('gone'), + }, + }), + ]); + }); + }); + + describe('the caps', () => { + it('reports the record failed, and sends nothing, past the deferral count', async () => { + process.env[MAX_DEFERRALS_ENV] = '2'; + const firstDeferredAt = new Date(NOW - MINUTE).toISOString(); + worker._run.mockRejectedValue(rateLimited(60 * SECOND)); + + const result = await run( + sqsRecord({ + body: JSON.stringify({ + ...BODY, + _frigg: { deferrals: 2, firstDeferredAt }, + }), + }) + ); + + expect(result).toEqual(failure()); + expect(sent()).toEqual([]); + expect(logged('record_deferral_capped')).toEqual([ + expect.objectContaining({ + level: 'WARN', + deferrals: 3, + maxDeferrals: 2, + }), + ]); + expect(logged('record_failed')).toEqual([]); + }); + + it('reports the record failed when the retry time is past the total time', async () => { + worker._run.mockRejectedValue(rateLimited(25 * HOUR)); + + const result = await run(); + + expect(result).toEqual(failure()); + expect(sent()).toEqual([]); + expect(visibility()).toEqual([]); + expect(logged('record_deferral_capped')).toHaveLength(1); + }); + + it('counts the total time from the first deferral', async () => { + const firstDeferredAt = new Date(NOW - 23 * HOUR).toISOString(); + worker._run.mockRejectedValue(rateLimited(2 * HOUR)); + + const result = await run( + sqsRecord({ + body: JSON.stringify({ + ...BODY, + _frigg: { deferrals: 1, firstDeferredAt }, + }), + }) + ); + + expect(result).toEqual(failure()); + expect(logged('record_deferral_capped')).toHaveLength(1); + }); + + it('writes an ERROR when a capped message is on its last delivery', async () => { + process.env.FRIGG_QUEUE_MAX_RECEIVE_COUNT = '3'; + worker._run.mockRejectedValue(rateLimited(25 * HOUR)); + + await run( + sqsRecord({ attributes: { ApproximateReceiveCount: '3' } }) + ); + + expect(logged('record_lost_rate_limited')).toEqual([ + expect.objectContaining({ + level: 'ERROR', + receiveCount: 3, + reason: 'burst', + }), + ]); + }); + + it('writes no lost-message ERROR before the last delivery', async () => { + process.env.FRIGG_QUEUE_MAX_RECEIVE_COUNT = '3'; + worker._run.mockRejectedValue(rateLimited(25 * HOUR)); + + await run( + sqsRecord({ attributes: { ApproximateReceiveCount: '2' } }) + ); + + expect(logged('record_lost_rate_limited')).toEqual([]); + }); + }); + + describe('a rate-limit error that cannot be deferred', () => { + it('goes back to SQS as before when it has no retry time', async () => { + worker._run.mockRejectedValue( + Object.assign(new Error('limited'), { isRateLimited: true }) + ); + + const result = await run(); + + expect(result).toEqual(failure()); + expect(sent()).toEqual([]); + expect(logged('record_failed')).toEqual([ + expect.objectContaining({ + deferral: { skipped: 'no_retry_at' }, + }), + ]); + }); + + it.each([ + ['no event source ARN', { eventSourceARN: undefined }], + ['a FIFO queue', { eventSourceARN: `${QUEUE_ARN}.fifo` }], + ])('goes back to SQS as before with %s', async (_label, overrides) => { + worker._run.mockRejectedValue(rateLimited(60 * SECOND)); + + const result = await run(sqsRecord(overrides)); + + expect(result).toEqual(failure()); + expect(sent()).toEqual([]); + expect(logged('record_failed')).toEqual([ + expect.objectContaining({ + deferral: { skipped: 'no_event_source' }, + }), + ]); + }); + + it('goes back to SQS, and is not lost, when the send fails', async () => { + sqsMock.on(SendMessageCommand).rejects(new Error('SQS is down')); + worker._run.mockRejectedValue(rateLimited(60 * SECOND)); + + const result = await run(); + + expect(result).toEqual(failure()); + expect(logged('record_failed')).toEqual([ + expect.objectContaining({ + deferral: { + skipped: 'send_failed', + cause: expect.stringContaining('SQS is down'), + }, + }), + ]); + }); + + it('goes back to SQS when the message body is not JSON', async () => { + worker._run.mockRejectedValue(rateLimited(60 * SECOND)); + + const result = await run(sqsRecord({ body: 'not json' })); + + expect(result).toEqual(failure()); + expect(sent()).toEqual([]); + }); + + it('does not touch an error that is not a rate limit', async () => { + worker._run.mockRejectedValue(new Error('boom')); + + const result = await run(); + + expect(result).toEqual(failure()); + expect(sent()).toEqual([]); + expect(logged('record_failed')[0].deferral).toBeUndefined(); + }); + }); + + describe('logging', () => { + it('writes one WARN record_deferred with the wait, the times and the counters', async () => { + worker._run.mockRejectedValue( + rateLimited(60 * SECOND, { reason: 'daily' }) + ); + + await run( + sqsRecord({ attributes: { ApproximateReceiveCount: '2' } }) + ); + + expect(logged('record_deferred')).toEqual([ + expect.objectContaining({ + level: 'WARN', + logger: 'frigg.worker', + messageId: 'msg-1', + receiveCount: 2, + mechanism: 'delay', + waitMs: 60_000, + delaySeconds: 60, + retryAt: new Date(NOW + 60_000).toISOString(), + deferrals: 1, + firstDeferredAt: new Date(NOW).toISOString(), + reason: 'daily', + module: 'hubspot', + statusCode: 429, + error: expect.objectContaining({ type: 'RateLimitError' }), + }), + ]); + expect(logged('record_failed')).toEqual([]); + }); + + it('names the schedule when the message was scheduled', async () => { + process.env.SCHEDULER_ROLE_ARN = SCHEDULER_ROLE_ARN; + worker._run.mockRejectedValue(rateLimited(20 * MINUTE)); + + await run(); + + expect(logged('record_deferred')).toEqual([ + expect.objectContaining({ + mechanism: 'schedule', + scheduleName: 'frigg-defer-msg-1', + }), + ]); + }); + + it('writes one WARN record_visibility_extended and no record_failed', async () => { + worker._run.mockRejectedValue(rateLimited(20 * MINUTE)); + + await run(); + + expect(logged('record_visibility_extended')).toEqual([ + expect.objectContaining({ + level: 'WARN', + mechanism: 'visibility', + visibilityTimeout: 1200, + waitMs: 20 * MINUTE, + }), + ]); + expect(logged('record_failed')).toEqual([]); + }); + }); + + describe('the run state hooks', () => { + beforeEach(() => { + worker.recordRateLimitWait = jest.fn().mockResolvedValue(undefined); + worker.clearRateLimitWait = jest.fn().mockResolvedValue(undefined); + }); + + it('records the wait after the message was put back', async () => { + worker._run.mockRejectedValue(rateLimited(60 * SECOND)); + + await run(); + + expect(worker.recordRateLimitWait).toHaveBeenCalledTimes(1); + const [body, error, state] = + worker.recordRateLimitWait.mock.calls[0]; + expect(body).toEqual(BODY); + expect(error).toBeInstanceOf(RateLimitError); + expect(state).toEqual({ + status: 'WAITING', + mechanism: 'delay', + deferrals: 1, + retryAt: new Date(NOW + 60_000), + }); + }); + + it('records nothing when the message could not be put back', async () => { + sqsMock.on(SendMessageCommand).rejects(new Error('SQS is down')); + worker._run.mockRejectedValue(rateLimited(60 * SECOND)); + + await run(); + + expect(worker.recordRateLimitWait).not.toHaveBeenCalled(); + }); + + it('records EXHAUSTED when a cap ended the deferrals', async () => { + worker._run.mockRejectedValue(rateLimited(25 * HOUR)); + + await run(); + + expect(worker.recordRateLimitWait.mock.calls[0][2]).toMatchObject({ + status: 'EXHAUSTED', + }); + }); + + it('records EXHAUSTED for a visibility change on the last delivery', async () => { + process.env.FRIGG_QUEUE_MAX_RECEIVE_COUNT = '3'; + worker._run.mockRejectedValue(rateLimited(20 * MINUTE)); + + await run( + sqsRecord({ attributes: { ApproximateReceiveCount: '3' } }) + ); + + expect(worker.recordRateLimitWait.mock.calls[0][2]).toMatchObject({ + status: 'EXHAUSTED', + mechanism: 'visibility', + }); + }); + + it('clears the wait after a deferred message succeeds', async () => { + worker._run.mockResolvedValue(undefined); + const body = { + ...BODY, + _frigg: { + deferrals: 1, + firstDeferredAt: new Date(NOW).toISOString(), + }, + }; + + await run(sqsRecord({ body: JSON.stringify(body) })); + + expect(worker.clearRateLimitWait).toHaveBeenCalledWith(body); + }); + + it('clears the wait after a redelivered message succeeds', async () => { + worker._run.mockResolvedValue(undefined); + + await run( + sqsRecord({ attributes: { ApproximateReceiveCount: '2' } }) + ); + + expect(worker.clearRateLimitWait).toHaveBeenCalledWith(BODY); + }); + + it('does not clear anything after a first delivery', async () => { + worker._run.mockResolvedValue(undefined); + + await run(); + + expect(worker.clearRateLimitWait).not.toHaveBeenCalled(); + }); + + it('logs a failing hook and still reports the message handled', async () => { + worker.recordRateLimitWait.mockRejectedValue( + new Error('database is down') + ); + worker._run.mockRejectedValue(rateLimited(60 * SECOND)); + + const result = await run(); + + expect(result).toEqual({ batchItemFailures: [] }); + expect(logged('rate_limit_state_failed')).toEqual([ + expect.objectContaining({ + level: 'WARN', + operation: 'record', + error: expect.objectContaining({ + message: 'database is down', + }), + }), + ]); + }); + + it('logs a failing clear and still counts the message a success', async () => { + worker.clearRateLimitWait.mockRejectedValue( + new Error('database is down') + ); + worker._run.mockResolvedValue(undefined); + + const result = await run( + sqsRecord({ attributes: { ApproximateReceiveCount: '2' } }) + ); + + expect(result).toEqual({ batchItemFailures: [] }); + expect(logged('rate_limit_state_failed')).toEqual([ + expect.objectContaining({ operation: 'clear' }), + ]); + }); + }); + + describe('the SQS client', () => { + it('is built with the offline and endpoint options', () => { + process.env.AWS_ENDPOINT = 'http://localhost:4566'; + process.env.IS_OFFLINE = 'true'; + const constructed = []; + + jest.isolateModules(() => { + jest.doMock('@aws-sdk/client-sqs', () => ({ + ...jest.requireActual('@aws-sdk/client-sqs'), + SQSClient: jest.fn((config) => { + constructed.push(config); + return { send: jest.fn() }; + }), + })); + require('./Worker'); + }); + + expect(constructed.length).toBeGreaterThan(0); + for (const config of constructed) { + expect(config).toMatchObject({ + endpoint: 'http://localhost:4566', + region: 'us-east-1', + }); + } + }); + }); +}); diff --git a/packages/core/core/Worker.js b/packages/core/core/Worker.js index 27c20e864..d15b33532 100644 --- a/packages/core/core/Worker.js +++ b/packages/core/core/Worker.js @@ -1,14 +1,39 @@ -const { SQSClient, GetQueueUrlCommand, SendMessageCommand } = require('@aws-sdk/client-sqs'); +const { randomUUID } = require('node:crypto'); +const { + SQSClient, + GetQueueUrlCommand, + SendMessageCommand, + ChangeMessageVisibilityCommand, +} = require('@aws-sdk/client-sqs'); const _ = require('lodash'); const { RequiredPropertyError } = require('../errors'); const { get } = require('../assertions'); const { readQueueDelivery } = require('../queues/queue-delivery'); +const { awsConfigOptions } = require('../queues/queuer-util'); +const { + MAX_DELAY_SECONDS, + MAX_VISIBILITY_TIMEOUT_SECONDS, + isDeferralCapped, + nextDeferral, + readDeferral, + readDeferralLimits, + withDeferral, +} = require('../queues/queue-deferral'); const { runMessageScope } = require('./invocation-scope'); -const { getLogger } = require('../logs'); +const { getLogger, serializeError } = require('../logs'); -const sqs = new SQSClient({ region: process.env.AWS_REGION }); +const sqs = new SQSClient({ + region: process.env.AWS_REGION, + ...awsConfigOptions(), +}); const log = getLogger('frigg.worker'); +const validDate = (value) => + value instanceof Date && !Number.isNaN(value.getTime()) ? value : null; +const queueNameOf = (arn) => + typeof arn === 'string' && arn ? arn.split(':').pop() : undefined; +const causeOf = (error) => serializeError(error).message; + class Worker { async getQueueURL(params) { // Passing params in because there will be multiple QueueNames @@ -33,12 +58,31 @@ class Worker { // messageId, receiveCount and the event come from the scope. log.debug('Record started', { eventName: 'frigg.worker.record_started' }); + const delivery = readQueueDelivery(record); + let runParams; try { - const runParams = JSON.parse(record.body); + runParams = JSON.parse(record.body); this._validateParams(runParams); - await this._run(runParams, context, readQueueDelivery(record)); + await this._run(runParams, context, delivery); + await this._clearDeferredState(runParams, delivery); log.debug('Record succeeded', { eventName: 'frigg.worker.record_succeeded' }); } catch (error) { + let deferral; + if (error.isRateLimited && runParams) { + deferral = await this.defer( + record, + runParams, + error, + delivery + ); + if (deferral.outcome === 'acked') return; + if (deferral.outcome === 'failed') { + batchItemFailures.push({ + itemIdentifier: record.messageId, + }); + return; + } + } if (error.isHaltError) { // HaltError means "discard this message, don't retry". // Treat as success so SQS deletes it from the queue. @@ -55,6 +99,14 @@ class Worker { log.warn('Record failed, returned for retry', { eventName: 'frigg.worker.record_failed', error, + ...(deferral && { + deferral: { + skipped: deferral.skipped, + ...(deferral.cause && { + cause: causeOf(deferral.cause), + }), + }, + }), }); batchItemFailures.push({ itemIdentifier: record.messageId }); } @@ -75,6 +127,240 @@ class Worker { // parameters } + /** + * Puts a rate-limited message back so it runs at `error.retryAt`, without + * spending a receive. The message is sent again with a delay of up to 900 s, + * or scheduled with the one-time scheduler, or has its visibility timeout + * extended. The new message goes out before the old one is acknowledged, so + * delivery stays at least once. + * + * @returns {Promise<{outcome: 'acked'|'failed'|'skipped', skipped?: string, cause?: Error}>} + * `acked`: the message is handled. `failed`: report the record failed and + * let SQS redeliver it. `skipped`: nothing was done; fail the record as + * any other error. + */ + async defer(record, body, error, delivery) { + const retryAt = validDate(error.retryAt); + if (!retryAt) return { outcome: 'skipped', skipped: 'no_retry_at' }; + const queueName = queueNameOf(record.eventSourceARN); + if (!queueName || queueName.endsWith('.fifo')) { + return { outcome: 'skipped', skipped: 'no_event_source' }; + } + + const now = Date.now(); + const waitMs = Math.max(0, retryAt.getTime() - now); + const deferral = nextDeferral(body, now); + const details = { + waitMs, + retryAt: retryAt.toISOString(), + deferrals: deferral.deferrals, + firstDeferredAt: deferral.firstDeferredAt, + reason: error.reason, + module: error.module, + statusCode: error.statusCode, + }; + const state = (status, mechanism) => ({ + status, + mechanism, + deferrals: deferral.deferrals, + retryAt, + }); + + const limits = readDeferralLimits(); + if (isDeferralCapped({ ...deferral, retryAt }, limits)) { + log.warn('Rate-limit deferral capped', { + eventName: 'frigg.worker.record_deferral_capped', + ...details, + maxDeferrals: limits.maxDeferrals, + maxDeferredMs: limits.maxDeferredMs, + deferredMs: + retryAt.getTime() - Date.parse(deferral.firstDeferredAt), + error, + }); + await this._recordWait(body, error, state('EXHAUSTED', 'none')); + this._logIfLost(delivery, error, details); + return { outcome: 'failed' }; + } + + const next = withDeferral(body, deferral); + + if (waitMs <= MAX_DELAY_SECONDS * 1000) { + const delaySeconds = Math.ceil(waitMs / 1000); + try { + const queueUrl = await this._queueUrl(queueName); + await this.send({ ...next, QueueUrl: queueUrl }, delaySeconds); + } catch (cause) { + return { outcome: 'skipped', skipped: 'send_failed', cause }; + } + await this._recordWait(body, error, state('WAITING', 'delay')); + log.warn('Record deferred', { + eventName: 'frigg.worker.record_deferred', + mechanism: 'delay', + delaySeconds, + ...details, + error, + }); + return { outcome: 'acked' }; + } + + let scheduleError; + const scheduler = this.getSchedulerService(); + if (scheduler) { + const scheduleName = `frigg-defer-${ + record.messageId || randomUUID() + }`; + try { + await scheduler.scheduleOneTime({ + scheduleName, + scheduleAt: retryAt, + queueResourceId: record.eventSourceARN, + payload: next, + }); + } catch (cause) { + if (cause?.name !== 'ConflictException') scheduleError = cause; + } + if (!scheduleError) { + await this._recordWait( + body, + error, + state('WAITING', 'schedule') + ); + log.warn('Record deferred', { + eventName: 'frigg.worker.record_deferred', + mechanism: 'schedule', + scheduleName, + ...details, + error, + }); + return { outcome: 'acked' }; + } + } + + const visibilityTimeout = Math.min( + Math.ceil(waitMs / 1000), + MAX_VISIBILITY_TIMEOUT_SECONDS + ); + try { + const queueUrl = await this._queueUrl(queueName); + await sqs.send( + new ChangeMessageVisibilityCommand({ + QueueUrl: queueUrl, + ReceiptHandle: record.receiptHandle, + VisibilityTimeout: visibilityTimeout, + }) + ); + } catch (cause) { + return { outcome: 'skipped', skipped: 'visibility_failed', cause }; + } + await this._recordWait( + body, + error, + state( + delivery.isLastAttempt ? 'EXHAUSTED' : 'WAITING', + 'visibility' + ) + ); + log.warn('Record visibility extended', { + eventName: 'frigg.worker.record_visibility_extended', + mechanism: 'visibility', + visibilityTimeout, + ...details, + ...(scheduleError && { scheduleError: causeOf(scheduleError) }), + error, + }); + this._logIfLost(delivery, error, details); + return { outcome: 'failed' }; + } + + /** + * The one-time scheduler, or null when the stack has none. The mock + * scheduler is never used: a schedule kept in memory would be lost with + * the message. + */ + getSchedulerService() { + if (this._schedulerService !== undefined) return this._schedulerService; + this._schedulerService = null; + if ( + process.env.SCHEDULER_ROLE_ARN && + process.env.SCHEDULER_PROVIDER !== 'mock' + ) { + const { + createSchedulerService, + SCHEDULER_PROVIDERS, + } = require('../infrastructure/scheduler/scheduler-service-factory'); + this._schedulerService = createSchedulerService({ + provider: SCHEDULER_PROVIDERS.EVENTBRIDGE, + }); + } + return this._schedulerService; + } + + /** + * Hook: the message was put back for a rate limit. Runs after it was sent. + * @param {Object} body The parsed message body. + * @param {Error} error The RateLimitError. + * @param {{status: 'WAITING'|'EXHAUSTED', mechanism: string, deferrals: number, retryAt: Date}} state + */ + async recordRateLimitWait() {} + + /** + * Hook: a message that was put back for a rate limit ran without error. + * @param {Object} body The parsed message body. + */ + async clearRateLimitWait() {} + + _queueUrl(queueName) { + this._queueUrls ??= new Map(); + if (!this._queueUrls.has(queueName)) { + const lookup = this.getQueueURL({ QueueName: queueName }).catch( + (error) => { + this._queueUrls.delete(queueName); + throw error; + } + ); + this._queueUrls.set(queueName, lookup); + } + return this._queueUrls.get(queueName); + } + + async _recordWait(body, error, state) { + await this._runStateHook('record', () => + this.recordRateLimitWait(body, error, state) + ); + } + + async _clearDeferredState(body, delivery) { + if (readDeferral(body).deferrals === 0 && !(delivery.receiveCount > 1)) + return; + await this._runStateHook('clear', () => this.clearRateLimitWait(body)); + } + + async _runStateHook(operation, hook) { + try { + await hook(); + } catch (error) { + log.warn('Rate-limit run state not written', { + eventName: 'frigg.worker.rate_limit_state_failed', + operation, + error, + }); + } + } + + _logIfLost(delivery, error, details) { + if (!delivery.isLastAttempt) return; + log.error( + 'Rate-limited record lost: the message goes to the dead-letter queue next', + { + eventName: 'frigg.worker.record_lost_rate_limited', + retryAt: details.retryAt, + reason: error.reason, + module: error.module, + error, + } + ); + } + // returns the message id async send(params, delay = 0) { this._validateParams(params); diff --git a/packages/core/package.json b/packages/core/package.json index dbbed0af0..27513db7e 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -7,6 +7,7 @@ "@aws-sdk/client-kms": "^3.588.0", "@aws-sdk/client-lambda": "^3.714.0", "@aws-sdk/client-s3": "^3.588.0", + "@aws-sdk/client-scheduler": "^3.588.0", "@aws-sdk/client-sqs": "^3.588.0", "@aws-sdk/client-ssm": "^3.588.0", "@aws-sdk/s3-request-presigner": "^3.588.0", diff --git a/packages/core/queues/queuer-util.js b/packages/core/queues/queuer-util.js index 0c9f502ca..d2f19ad6e 100644 --- a/packages/core/queues/queuer-util.js +++ b/packages/core/queues/queuer-util.js @@ -126,4 +126,4 @@ const QueuerUtil = { }, }; -module.exports = { QueuerUtil }; +module.exports = { QueuerUtil, awsConfigOptions }; From e0b75ddec7d78329dee18b5f9b7965e07474e99e Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 20:34:23 -0300 Subject: [PATCH 04/30] feat(core): write the rate-limit wait on the process of a deferred message 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 --- packages/core/handlers/backend-utils.js | 26 ++++++ .../integration-defined-workers.test.js | 93 +++++++++++++++++++ 2 files changed, 119 insertions(+) diff --git a/packages/core/handlers/backend-utils.js b/packages/core/handlers/backend-utils.js index 6cf32616c..97366a090 100644 --- a/packages/core/handlers/backend-utils.js +++ b/packages/core/handlers/backend-utils.js @@ -165,6 +165,32 @@ const createQueueWorker = (integrationClass) => { const integrationName = integrationClass.Definition.name; class QueueWorker extends Worker { + async recordRateLimitWait(body, error, state) { + const processId = body.data?.processId; + if (!processId) return; + await createProcessRepository().applyProcessUpdate(processId, { + set: { + 'context.rateLimit': { + status: state.status, + mechanism: state.mechanism, + retryAt: state.retryAt.toISOString(), + reason: error.reason, + module: error.module, + deferrals: state.deferrals, + updatedAt: new Date().toISOString(), + }, + }, + }); + } + + async clearRateLimitWait(body) { + const processId = body?.data?.processId; + if (!processId) return; + await createProcessRepository().applyProcessUpdate(processId, { + set: { 'context.rateLimit': null }, + }); + } + async _run(params, context, delivery) { const logCtx = { integration: integrationName, diff --git a/packages/core/handlers/workers/integration-defined-workers.test.js b/packages/core/handlers/workers/integration-defined-workers.test.js index 6a8eedd64..d0913f837 100644 --- a/packages/core/handlers/workers/integration-defined-workers.test.js +++ b/packages/core/handlers/workers/integration-defined-workers.test.js @@ -336,6 +336,99 @@ describe('Webhook Queue Worker', () => { }); }); + describe('Rate-limit run state', () => { + const retryAt = new Date('2026-09-28T12:20:00.000Z'); + const state = { + status: 'WAITING', + mechanism: 'delay', + deferrals: 1, + retryAt, + }; + const rateLimitError = Object.assign(new Error('limited'), { + isRateLimited: true, + reason: 'burst', + module: 'hubspot', + }); + + function buildWorker() { + const applyProcessUpdate = jest.fn().mockResolvedValue({ id: 7 }); + let QueueWorker; + jest.isolateModules(() => { + jest.doMock( + '../../integrations/repositories/process-repository-factory', + () => ({ + createProcessRepository: () => ({ applyProcessUpdate }), + }) + ); + QueueWorker = require('../backend-utils').createQueueWorker( + TestWebhookIntegration + ); + }); + return { worker: new QueueWorker(), applyProcessUpdate }; + } + + it('writes context.rateLimit on the process of a deferred message', async () => { + const { worker, applyProcessUpdate } = buildWorker(); + + await worker.recordRateLimitWait( + { data: { processId: 7 } }, + rateLimitError, + state + ); + + expect(applyProcessUpdate).toHaveBeenCalledWith(7, { + set: { + 'context.rateLimit': { + status: 'WAITING', + mechanism: 'delay', + retryAt: '2026-09-28T12:20:00.000Z', + reason: 'burst', + module: 'hubspot', + deferrals: 1, + updatedAt: expect.any(String), + }, + }, + }); + }); + + it('clears context.rateLimit on the process of a message that ran', async () => { + const { worker, applyProcessUpdate } = buildWorker(); + + await worker.clearRateLimitWait({ data: { processId: 7 } }); + + expect(applyProcessUpdate).toHaveBeenCalledWith(7, { + set: { 'context.rateLimit': null }, + }); + }); + + it('writes nothing for a message with no processId', async () => { + const { worker, applyProcessUpdate } = buildWorker(); + + await worker.recordRateLimitWait( + { data: { integrationId: 3 } }, + rateLimitError, + state + ); + await worker.clearRateLimitWait({ data: {} }); + await worker.clearRateLimitWait({}); + + expect(applyProcessUpdate).not.toHaveBeenCalled(); + }); + + it('does not fail when the process is gone', async () => { + const { worker, applyProcessUpdate } = buildWorker(); + applyProcessUpdate.mockResolvedValue(null); + + await expect( + worker.recordRateLimitWait( + { data: { processId: 7 } }, + rateLimitError, + state + ) + ).resolves.toBeUndefined(); + }); + }); + describe('Integration Hydration for webhooks with integrationId', () => { it('should attempt to load integration when integrationId present', async () => { // This test verifies the logic path - full integration test From 3518b148659effbe4adfe6e8a1553e28b942c322 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 20:36:29 -0300 Subject: [PATCH 05/30] feat(devtools): provision what a deferred queue message needs 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 `---*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 --- .../domains/scheduler/scheduler-builder.js | 11 +++- .../scheduler/scheduler-builder.test.js | 55 +++++++++++++++++++ .../utilities/base-definition-factory.js | 1 + .../utilities/base-definition-factory.test.js | 12 ++++ 4 files changed, 77 insertions(+), 2 deletions(-) diff --git a/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.js b/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.js index daaf5c03b..721375fc8 100644 --- a/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.js +++ b/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.js @@ -19,6 +19,11 @@ const { getAdminFunctionNames, } = require('../shared/function-environments'); +const declaresRateLimit = (integration) => + Object.values(integration?.Definition?.modules ?? {}).some((module) => + Boolean(module?.definition?.API?.rateLimit) + ); + class SchedulerBuilder extends InfrastructureBuilder { constructor() { super(); @@ -32,12 +37,14 @@ class SchedulerBuilder extends InfrastructureBuilder { return true; } - // Check if any integration has webhooks enabled + // Check if any integration has webhooks enabled, or a module that + // declares a rate limit (the queue worker schedules a deferred message) if (Array.isArray(appDefinition.integrations)) { return appDefinition.integrations.some( (integration) => integration?.Definition?.webhooks?.enabled === true || - integration?.Definition?.webhooks === true + integration?.Definition?.webhooks === true || + declaresRateLimit(integration) ); } diff --git a/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.test.js b/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.test.js index 7dfeeda36..588c91257 100644 --- a/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.test.js +++ b/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.test.js @@ -38,6 +38,61 @@ describe('SchedulerBuilder', () => { ).toBe(true); }); + it('runs when any module declares a rate limit', () => { + class Api { + static rateLimit = { minRetryAfterMs: 1_000 }; + } + expect( + schedulerBuilder.shouldExecute({ + integrations: [ + { Definition: { name: 'a' } }, + { + Definition: { + name: 'b', + modules: { crm: { definition: { API: Api } } }, + }, + }, + ], + }) + ).toBe(true); + }); + + it('skips a module whose API declares no rate limit', () => { + class Api {} + expect( + schedulerBuilder.shouldExecute({ + integrations: [ + { + Definition: { + name: 'a', + modules: { crm: { definition: { API: Api } } }, + }, + }, + ], + }) + ).toBe(false); + }); + + it.each([ + ['no modules', { name: 'a' }], + ['an empty modules object', { name: 'a', modules: {} }], + [ + 'a module with no definition', + { name: 'a', modules: { crm: {} } }, + ], + ['a null module', { name: 'a', modules: { crm: null } }], + [ + 'a definition with no API', + { name: 'a', modules: { crm: { definition: {} } } }, + ], + ])('skips an integration with %s', (_label, Definition) => { + expect( + schedulerBuilder.shouldExecute({ + integrations: [{ Definition }], + }) + ).toBe(false); + }); + it('skips otherwise', () => { expect( schedulerBuilder.shouldExecute({ diff --git a/packages/devtools/infrastructure/domains/shared/utilities/base-definition-factory.js b/packages/devtools/infrastructure/domains/shared/utilities/base-definition-factory.js index fb4776ff2..4aafa0756 100644 --- a/packages/devtools/infrastructure/domains/shared/utilities/base-definition-factory.js +++ b/packages/devtools/infrastructure/domains/shared/utilities/base-definition-factory.js @@ -195,6 +195,7 @@ function createBaseDefinition( 'sqs:SendMessageBatch', 'sqs:GetQueueUrl', 'sqs:GetQueueAttributes', + 'sqs:ChangeMessageVisibility', ], Resource: [ { 'Fn::GetAtt': ['InternalErrorQueue', 'Arn'] }, diff --git a/packages/devtools/infrastructure/domains/shared/utilities/base-definition-factory.test.js b/packages/devtools/infrastructure/domains/shared/utilities/base-definition-factory.test.js index 8fcb111fd..fd3f0a366 100644 --- a/packages/devtools/infrastructure/domains/shared/utilities/base-definition-factory.test.js +++ b/packages/devtools/infrastructure/domains/shared/utilities/base-definition-factory.test.js @@ -135,6 +135,18 @@ describe('Base Definition Factory', () => { expect(sqsPermission).toBeDefined(); }); + it('should let a function extend the visibility timeout of a message on an integration queue', () => { + const result = createBaseDefinition({}, {}, {}); + + const sqsPermission = result.provider.iamRoleStatements.find( + (stmt) => stmt.Action.includes('sqs:SendMessage') + ); + expect(sqsPermission.Action).toContain( + 'sqs:ChangeMessageVisibility' + ); + expect(JSON.stringify(sqsPermission.Resource)).toContain('*Queue'); + }); + it('should include required plugins', () => { const result = createBaseDefinition({}, {}, {}); From 7bafa353295b68c5d457fb6b1a208157318a48fe Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 20:37:17 -0300 Subject: [PATCH 06/30] docs(core): document the rate-limit deferral in the queue worker 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 --- docs/guides/INTEGRATION-PATTERNS.md | 33 +++++++++++- .../api-module-definition-and-functions.md | 4 +- packages/core/CLAUDE.md | 52 +++++++++++++++++++ packages/core/core/CLAUDE.md | 19 +++++++ 4 files changed, 105 insertions(+), 3 deletions(-) diff --git a/docs/guides/INTEGRATION-PATTERNS.md b/docs/guides/INTEGRATION-PATTERNS.md index aab7fe206..a987b2fde 100644 --- a/docs/guides/INTEGRATION-PATTERNS.md +++ b/docs/guides/INTEGRATION-PATTERNS.md @@ -397,6 +397,34 @@ only when SQS will move the message to the DLQ if this attempt fails, and `false` whenever that is unknown. Use it to end a run or count lost work on the final try instead of leaving the run in progress. +### Rate-Limit Deferral + +When a provider throttles a queue handler for longer than the Requester sleeps +in process, the Requester throws `RateLimitError` with `retryAt`. Let it reach +the queue worker. Core puts the message back so it runs at `retryAt`, and does +not spend one of the message's three receives: it sends the body again with a +delay (up to 15 minutes), or schedules it, or extends its visibility timeout. +With a `processId` in the message, `Process.context.rateLimit` says the run is +waiting and until when, and is cleared when the message runs. + +Check `delivery.isLastAttempt` first, then rethrow: + +```javascript +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 + } +} +``` + +A deferral past `FRIGG_QUEUE_MAX_DEFERRALS` (default 10) or +`FRIGG_QUEUE_MAX_DEFERRED_MS` (default 24 hours) fails the record like any other +retryable error, and `delivery` applies. Do not catch a `RateLimitError` to +write a retry of your own. + --- ## Sync Orchestration @@ -801,8 +829,9 @@ Put what you know about a provider's limits in its API module: declare [API module reference](../reference/api-module-definition-and-functions.md#rate-limits-static-ratelimit)). The Requester then waits as the provider says. When the wait is too long to sleep, it throws `RateLimitError` with `retryAt`. Let it reach the queue -handler's caller: the queue worker does not halt it and SQS redelivers the -message. Do not catch it to write a retry of your own. +worker: it puts the message back to run at `retryAt` (see +[Rate-Limit Deferral](#rate-limit-deferral)). Do not catch it to write a retry +of your own. For pacing in your own loop, respect API rate limits: diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index 0c049cffd..cd4161213 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -294,7 +294,9 @@ class Api extends ApiKeyRequester { `policy`, `source`, `module` and `scopeKey`. `Retry-After` is read on a 429 only. The queue worker does not halt a -`RateLimitError`, even when its status is 403: the message goes back to SQS. +`RateLimitError`, even when its status is 403. It puts the message back to run +at `retryAt` (see the Rate-Limit Deferral section of the +[Integration Patterns Guide](/docs/guides/INTEGRATION-PATTERNS.md)). ### A client that is not the Requester diff --git a/packages/core/CLAUDE.md b/packages/core/CLAUDE.md index 440ffcb84..306f94953 100644 --- a/packages/core/CLAUDE.md +++ b/packages/core/CLAUDE.md @@ -543,6 +543,58 @@ The single source of the max receive count is devtools integration builder uses it for the queue's `RedrivePolicy` and sets it as `FRIGG_QUEUE_MAX_RECEIVE_COUNT` on the queue worker function. +**Rate-limit deferral** (ADR-049): when a queue handler throws an error with +`isRateLimited` and a `retryAt` (a `RateLimitError` from the Requester, or one a +module throws), `Worker.run` does not give the message back to SQS for a 30 +minute redelivery that spends one of its three receives. It 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, and `SCHEDULER_ROLE_ARN` is 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 message's visibility timeout to `retryAt` (12 h at most). This uses one receive | reported failed | + +- The new message starts at receive count 1. A deferred body carries + `_frigg: { deferrals, firstDeferredAt }`; `_frigg` is reserved and the handler + never sees it. +- Two caps bound it: `FRIGG_QUEUE_MAX_DEFERRALS` (default 10) and + `FRIGG_QUEUE_MAX_DEFERRED_MS` (default 24 h, from the first deferral to the + retry time). Past a cap, with no `retryAt`, with no event source ARN or on a + FIFO queue, the record fails like any retryable error and `delivery` + applies. +- The mock scheduler is never used for a deferral: a schedule kept in memory + would be lost with the acknowledged message. +- With a `processId` in the message, the worker writes + `Process.context.rateLimit = { status: 'WAITING' | 'EXHAUSTED', mechanism, + retryAt, reason, module, deferrals, updatedAt }`, and `null` after the message + ran. `EXHAUSTED` means a cap ended the deferrals, or a visibility change came + on the last delivery. +- Delivery stays at least once. The new message is sent before the old one is + acknowledged, so a crash between the two can run the work twice. +- Handlers check `delivery.isLastAttempt` first, then rethrow. 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 one the worker writes an + ERROR (`frigg.worker.record_lost_rate_limited`), and the handler has this one + chance to end the run. + +```javascript +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 + } +} +``` + +Records: `frigg.worker.record_deferred` (`mechanism` `delay` or `schedule`), +`record_visibility_extended`, `record_deferral_capped`, +`record_lost_rate_limited` and `rate_limit_state_failed`. A queue in a stack +that does not own it (`ownership.queue: 'external'`) needs its own +`sqs:ChangeMessageVisibility` and scheduler access. + ### 8. Error Handling (`/errors`) **Purpose**: Standardized error types with proper HTTP semantics. diff --git a/packages/core/core/CLAUDE.md b/packages/core/core/CLAUDE.md index 0c9d79a44..4b4f83292 100644 --- a/packages/core/core/CLAUDE.md +++ b/packages/core/core/CLAUDE.md @@ -79,6 +79,25 @@ await worker.send({ }, delaySeconds); ``` +**Rate-limit deferral** (ADR-049): `run()` handles an error with `isRateLimited` +and a `retryAt` through `defer(record, body, error, delivery)`, which returns +`{ outcome }`: +- `acked`: the message was sent again with `DelaySeconds` (wait up to 900 s) or + scheduled with the one-time scheduler (longer wait). The record is handled. +- `failed`: the visibility timeout was extended to `retryAt`, or a cap + (`FRIGG_QUEUE_MAX_DEFERRALS`, `FRIGG_QUEUE_MAX_DEFERRED_MS`) ended the + deferrals. The record is reported failed. +- `skipped`: no `retryAt`, no `eventSourceARN`, a FIFO queue, or a failed send. + The record fails as any error does, and `record_failed` carries + `deferral: { skipped }`. + +Subclasses override two no-op hooks to keep run state: `recordRateLimitWait(body, +error, state)` runs after the message was put back, and `clearRateLimitWait(body)` +runs after a deferred or redelivered message succeeded. A failing hook logs +`frigg.worker.rate_limit_state_failed` and never changes the outcome. The queue +worker of an integration (`createQueueWorker`) writes `Process.context.rateLimit` +in them. See `packages/core/CLAUDE.md` section 7. + ### Delegate Pattern System (`Delegate.js:3-27`) **Purpose**: Observer/delegation pattern for decoupled component communication From ebe5c602d3aac76850912c731dd91851772856b3 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 20:38:16 -0300 Subject: [PATCH 07/30] feat(core): type the Worker rate-limit hooks Declares recordRateLimitWait, clearRateLimitWait and RateLimitWaitState so a TypeScript Worker subclass can override the hooks. Refs ADR-049. Co-Authored-By: Claude Sonnet 5.5 --- packages/core/types/core/index.d.ts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/packages/core/types/core/index.d.ts b/packages/core/types/core/index.d.ts index 3627558b7..9c9a045e8 100644 --- a/packages/core/types/core/index.d.ts +++ b/packages/core/types/core/index.d.ts @@ -47,8 +47,28 @@ declare module "@friggframework/core" { send(params: object & { QueueUrl: any }, delay?: number): Promise; sendAsyncSQSMessage(params: SendSQSMessageParams): Promise; + + /** + * Hook: a message was put back for a rate limit. Runs after the message + * was sent. A failure is logged and never changes the outcome. + */ + recordRateLimitWait( + body: object, + error: Error, + state: RateLimitWaitState + ): Promise; + + /** Hook: a message that was put back for a rate limit ran without error. */ + clearRateLimitWait(body: object): Promise; } + export type RateLimitWaitState = { + status: "WAITING" | "EXHAUSTED"; + mechanism: "delay" | "schedule" | "visibility" | "none"; + deferrals: number; + retryAt: Date; + }; + interface IWorker { getQueueURL(params: GetQueueURLParams): Promise; run(params: { Records: any }, context?: object): Promise; From b14221fca51737284a61407b42e003637930465e Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 20:59:31 -0300 Subject: [PATCH 08/30] fix(core): address the review of the queue worker deferral - 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 --- docs/guides/INTEGRATION-PATTERNS.md | 3 +- packages/core/CLAUDE.md | 9 +- packages/core/core/Worker.js | 151 +++++++---- packages/core/handlers/backend-utils.js | 23 +- .../integration-defined-workers.test.js | 249 ++++++++++++++---- 5 files changed, 324 insertions(+), 111 deletions(-) diff --git a/docs/guides/INTEGRATION-PATTERNS.md b/docs/guides/INTEGRATION-PATTERNS.md index a987b2fde..83746add8 100644 --- a/docs/guides/INTEGRATION-PATTERNS.md +++ b/docs/guides/INTEGRATION-PATTERNS.md @@ -405,7 +405,8 @@ the queue worker. Core puts the message back so it runs at `retryAt`, and does not spend one of the message's three receives: it sends the body again with a delay (up to 15 minutes), or schedules it, or extends its visibility timeout. With a `processId` in the message, `Process.context.rateLimit` says the run is -waiting and until when, and is cleared when the message runs. +waiting and until when. It holds the latest known wait of the process, and is +cleared when a message runs after that wait is over. Check `delivery.isLastAttempt` first, then rethrow: diff --git a/packages/core/CLAUDE.md b/packages/core/CLAUDE.md index 7c3eaa765..6993b8dd2 100644 --- a/packages/core/CLAUDE.md +++ b/packages/core/CLAUDE.md @@ -570,9 +570,12 @@ back so it runs at `retryAt`: would be lost with the acknowledged message. - With a `processId` in the message, the worker writes `Process.context.rateLimit = { status: 'WAITING' | 'EXHAUSTED', mechanism, - retryAt, reason, module, deferrals, updatedAt }`, and `null` after the message - ran. `EXHAUSTED` means a cap ended the deferrals, or a visibility change came - on the last delivery. + retryAt, reason, module, deferrals, updatedAt }`. `EXHAUSTED` means 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, a `WAITING` value never hides an `EXHAUSTED` one, + and after a deferred or redelivered message ran the worker sets it to `null` + only when it is `WAITING` and its `retryAt` is past or within 5 s. - Delivery stays at least once. The new message is sent before the old one is acknowledged, so a crash between the two can run the work twice. - Handlers check `delivery.isLastAttempt` first, then rethrow. The delay and diff --git a/packages/core/core/Worker.js b/packages/core/core/Worker.js index d15b33532..1823653de 100644 --- a/packages/core/core/Worker.js +++ b/packages/core/core/Worker.js @@ -182,60 +182,101 @@ class Worker { return { outcome: 'failed' }; } - const next = withDeferral(body, deferral); - - if (waitMs <= MAX_DELAY_SECONDS * 1000) { - const delaySeconds = Math.ceil(waitMs / 1000); - try { - const queueUrl = await this._queueUrl(queueName); - await this.send({ ...next, QueueUrl: queueUrl }, delaySeconds); - } catch (cause) { - return { outcome: 'skipped', skipped: 'send_failed', cause }; - } - await this._recordWait(body, error, state('WAITING', 'delay')); - log.warn('Record deferred', { - eventName: 'frigg.worker.record_deferred', - mechanism: 'delay', - delaySeconds, - ...details, - error, - }); - return { outcome: 'acked' }; + const tier = { + record, + queueName, + body, + next: withDeferral(body, deferral), + error, + delivery, + retryAt, + waitMs, + details, + state, + }; + if (waitMs <= MAX_DELAY_SECONDS * 1000) + return this._deferWithDelay(tier); + const scheduled = await this._deferWithSchedule(tier); + if (scheduled?.outcome) return scheduled; + return this._deferWithVisibility({ + ...tier, + scheduleError: scheduled?.scheduleError, + }); + } + + async _deferWithDelay({ + queueName, + body, + next, + error, + waitMs, + details, + state, + }) { + const delaySeconds = Math.ceil(waitMs / 1000); + try { + const queueUrl = await this._queueUrl(queueName); + await this.send({ ...next, QueueUrl: queueUrl }, delaySeconds); + } catch (cause) { + return { outcome: 'skipped', skipped: 'send_failed', cause }; } + await this._recordWait(body, error, state('WAITING', 'delay')); + log.warn('Record deferred', { + eventName: 'frigg.worker.record_deferred', + mechanism: 'delay', + delaySeconds, + ...details, + error, + }); + return { outcome: 'acked' }; + } - let scheduleError; + /** `acked`, `{ scheduleError }` when the schedule failed, null with no scheduler. */ + async _deferWithSchedule({ + record, + body, + next, + error, + retryAt, + details, + state, + }) { const scheduler = this.getSchedulerService(); - if (scheduler) { - const scheduleName = `frigg-defer-${ - record.messageId || randomUUID() - }`; - try { - await scheduler.scheduleOneTime({ - scheduleName, - scheduleAt: retryAt, - queueResourceId: record.eventSourceARN, - payload: next, - }); - } catch (cause) { - if (cause?.name !== 'ConflictException') scheduleError = cause; - } - if (!scheduleError) { - await this._recordWait( - body, - error, - state('WAITING', 'schedule') - ); - log.warn('Record deferred', { - eventName: 'frigg.worker.record_deferred', - mechanism: 'schedule', - scheduleName, - ...details, - error, - }); - return { outcome: 'acked' }; - } + if (!scheduler) return null; + const scheduleName = `frigg-defer-${record.messageId || randomUUID()}`; + try { + await scheduler.scheduleOneTime({ + scheduleName, + scheduleAt: retryAt, + queueResourceId: record.eventSourceARN, + payload: next, + }); + } catch (cause) { + if (cause?.name !== 'ConflictException') + return { scheduleError: cause }; } + await this._recordWait(body, error, state('WAITING', 'schedule')); + log.warn('Record deferred', { + eventName: 'frigg.worker.record_deferred', + mechanism: 'schedule', + scheduleName, + ...details, + error, + }); + return { outcome: 'acked' }; + } + async _deferWithVisibility({ + record, + queueName, + body, + error, + delivery, + waitMs, + details, + state, + scheduleError, + }) { const visibilityTimeout = Math.min( Math.ceil(waitMs / 1000), MAX_VISIBILITY_TIMEOUT_SECONDS @@ -301,13 +342,17 @@ class Worker { * @param {Error} error The RateLimitError. * @param {{status: 'WAITING'|'EXHAUSTED', mechanism: string, deferrals: number, retryAt: Date}} state */ - async recordRateLimitWait() {} + async recordRateLimitWait() { + // A subclass keeps the run state; nothing to do by default. + } /** * Hook: a message that was put back for a rate limit ran without error. * @param {Object} body The parsed message body. */ - async clearRateLimitWait() {} + async clearRateLimitWait() { + // A subclass keeps the run state; nothing to do by default. + } _queueUrl(queueName) { this._queueUrls ??= new Map(); @@ -330,8 +375,8 @@ class Worker { } async _clearDeferredState(body, delivery) { - if (readDeferral(body).deferrals === 0 && !(delivery.receiveCount > 1)) - return; + const redelivered = delivery.receiveCount > 1; + if (readDeferral(body).deferrals === 0 && !redelivered) return; await this._runStateHook('clear', () => this.clearRateLimitWait(body)); } diff --git a/packages/core/handlers/backend-utils.js b/packages/core/handlers/backend-utils.js index 97366a090..672b62fbd 100644 --- a/packages/core/handlers/backend-utils.js +++ b/packages/core/handlers/backend-utils.js @@ -22,6 +22,15 @@ const { } = require('../integrations/utils/map-integration-dto'); const { processNotFound } = require('../integrations/use-cases/process-errors'); +const CLEAR_RATE_LIMIT_TOLERANCE_MS = 5_000; + +const keepsCurrentRateLimit = (current, next) => { + if (!current) return false; + if (next.status === 'EXHAUSTED') return false; + if (current.status === 'EXHAUSTED') return true; + return Date.parse(current.retryAt) > next.retryAt.getTime(); +}; + const loadRouterFromObject = (IntegrationClass, routerObject) => { const router = Router(); const { path, method, event } = routerObject; @@ -168,7 +177,11 @@ const createQueueWorker = (integrationClass) => { async recordRateLimitWait(body, error, state) { const processId = body.data?.processId; if (!processId) return; - await createProcessRepository().applyProcessUpdate(processId, { + const processRepository = createProcessRepository(); + const process = await processRepository.findById(processId); + if (keepsCurrentRateLimit(process?.context?.rateLimit, state)) + return; + await processRepository.applyProcessUpdate(processId, { set: { 'context.rateLimit': { status: state.status, @@ -186,7 +199,13 @@ const createQueueWorker = (integrationClass) => { async clearRateLimitWait(body) { const processId = body?.data?.processId; if (!processId) return; - await createProcessRepository().applyProcessUpdate(processId, { + const processRepository = createProcessRepository(); + const process = await processRepository.findById(processId); + const current = process?.context?.rateLimit; + if (current?.status !== 'WAITING') return; + const remainingMs = Date.parse(current.retryAt) - Date.now(); + if (remainingMs > CLEAR_RATE_LIMIT_TOLERANCE_MS) return; + await processRepository.applyProcessUpdate(processId, { set: { 'context.rateLimit': null }, }); } diff --git a/packages/core/handlers/workers/integration-defined-workers.test.js b/packages/core/handlers/workers/integration-defined-workers.test.js index d0913f837..a08b53b8b 100644 --- a/packages/core/handlers/workers/integration-defined-workers.test.js +++ b/packages/core/handlers/workers/integration-defined-workers.test.js @@ -337,95 +337,240 @@ describe('Webhook Queue Worker', () => { }); describe('Rate-limit run state', () => { - const retryAt = new Date('2026-09-28T12:20:00.000Z'); - const state = { - status: 'WAITING', + const NOW = Date.parse('2026-09-28T12:00:00.000Z'); + const MINUTE = 60_000; + const at = (ms) => new Date(NOW + ms); + const stored = (retryAt, status = 'WAITING') => ({ + status, + mechanism: 'delay', + deferrals: 1, + retryAt: retryAt.toISOString(), + reason: 'burst', + module: 'hubspot', + }); + const next = (retryAt, status = 'WAITING') => ({ + status, mechanism: 'delay', deferrals: 1, retryAt, - }; + }); const rateLimitError = Object.assign(new Error('limited'), { isRateLimited: true, reason: 'burst', module: 'hubspot', }); + const message = { data: { processId: 7 } }; + let nowSpy; + + beforeEach(() => { + nowSpy = jest.spyOn(Date, 'now').mockReturnValue(NOW); + }); + + afterEach(() => nowSpy.mockRestore()); - function buildWorker() { + function buildWorker(rateLimit) { const applyProcessUpdate = jest.fn().mockResolvedValue({ id: 7 }); + const findById = jest.fn().mockResolvedValue({ + id: 7, + context: rateLimit === undefined ? {} : { rateLimit }, + }); let QueueWorker; jest.isolateModules(() => { jest.doMock( '../../integrations/repositories/process-repository-factory', () => ({ - createProcessRepository: () => ({ applyProcessUpdate }), + createProcessRepository: () => ({ + applyProcessUpdate, + findById, + }), }) ); QueueWorker = require('../backend-utils').createQueueWorker( TestWebhookIntegration ); }); - return { worker: new QueueWorker(), applyProcessUpdate }; + return { worker: new QueueWorker(), applyProcessUpdate, findById }; } - it('writes context.rateLimit on the process of a deferred message', async () => { - const { worker, applyProcessUpdate } = buildWorker(); + describe('recording a wait', () => { + it('writes context.rateLimit on the process of a deferred message', async () => { + const { worker, applyProcessUpdate } = buildWorker(); - await worker.recordRateLimitWait( - { data: { processId: 7 } }, - rateLimitError, - state - ); + await worker.recordRateLimitWait( + message, + rateLimitError, + next(at(20 * MINUTE)) + ); - expect(applyProcessUpdate).toHaveBeenCalledWith(7, { - set: { - 'context.rateLimit': { - status: 'WAITING', - mechanism: 'delay', - retryAt: '2026-09-28T12:20:00.000Z', - reason: 'burst', - module: 'hubspot', - deferrals: 1, - updatedAt: expect.any(String), + expect(applyProcessUpdate).toHaveBeenCalledWith(7, { + set: { + 'context.rateLimit': { + status: 'WAITING', + mechanism: 'delay', + retryAt: '2026-09-28T12:20:00.000Z', + reason: 'burst', + module: 'hubspot', + deferrals: 1, + updatedAt: expect.any(String), + }, }, - }, + }); }); - }); - it('clears context.rateLimit on the process of a message that ran', async () => { - const { worker, applyProcessUpdate } = buildWorker(); + it('replaces an earlier wait', async () => { + const { worker, applyProcessUpdate } = buildWorker( + stored(at(5 * MINUTE)) + ); - await worker.clearRateLimitWait({ data: { processId: 7 } }); + await worker.recordRateLimitWait( + message, + rateLimitError, + next(at(20 * MINUTE)) + ); - expect(applyProcessUpdate).toHaveBeenCalledWith(7, { - set: { 'context.rateLimit': null }, + expect(applyProcessUpdate).toHaveBeenCalledTimes(1); }); - }); - it('writes nothing for a message with no processId', async () => { - const { worker, applyProcessUpdate } = buildWorker(); + it('keeps a later wait that another message recorded', async () => { + const { worker, applyProcessUpdate } = buildWorker( + stored(at(40 * MINUTE)) + ); - await worker.recordRateLimitWait( - { data: { integrationId: 3 } }, - rateLimitError, - state - ); - await worker.clearRateLimitWait({ data: {} }); - await worker.clearRateLimitWait({}); + await worker.recordRateLimitWait( + message, + rateLimitError, + next(at(20 * MINUTE)) + ); - expect(applyProcessUpdate).not.toHaveBeenCalled(); - }); + expect(applyProcessUpdate).not.toHaveBeenCalled(); + }); + + it('shows a cap even when a later wait is recorded', async () => { + const { worker, applyProcessUpdate } = buildWorker( + stored(at(40 * MINUTE)) + ); - it('does not fail when the process is gone', async () => { - const { worker, applyProcessUpdate } = buildWorker(); - applyProcessUpdate.mockResolvedValue(null); + await worker.recordRateLimitWait( + message, + rateLimitError, + next(at(20 * MINUTE), 'EXHAUSTED') + ); + + expect(applyProcessUpdate).toHaveBeenCalledTimes(1); + }); + + it('never hides a cap behind a later wait', async () => { + const { worker, applyProcessUpdate } = buildWorker( + stored(at(5 * MINUTE), 'EXHAUSTED') + ); - await expect( - worker.recordRateLimitWait( - { data: { processId: 7 } }, + await worker.recordRateLimitWait( + message, rateLimitError, - state - ) - ).resolves.toBeUndefined(); + next(at(20 * MINUTE)) + ); + + expect(applyProcessUpdate).not.toHaveBeenCalled(); + }); + + it('writes nothing for a message with no processId', async () => { + const { worker, applyProcessUpdate, findById } = buildWorker(); + + await worker.recordRateLimitWait( + { data: { integrationId: 3 } }, + rateLimitError, + next(at(MINUTE)) + ); + + expect(findById).not.toHaveBeenCalled(); + expect(applyProcessUpdate).not.toHaveBeenCalled(); + }); + + it('does not fail when the process is gone', async () => { + const { worker, findById, applyProcessUpdate } = buildWorker(); + findById.mockResolvedValue(null); + applyProcessUpdate.mockResolvedValue(null); + + await expect( + worker.recordRateLimitWait( + message, + rateLimitError, + next(at(MINUTE)) + ) + ).resolves.toBeUndefined(); + }); + }); + + describe('clearing a wait', () => { + it('clears a wait that is over', async () => { + const { worker, applyProcessUpdate } = buildWorker( + stored(at(-MINUTE)) + ); + + await worker.clearRateLimitWait(message); + + expect(applyProcessUpdate).toHaveBeenCalledWith(7, { + set: { 'context.rateLimit': null }, + }); + }); + + it('clears a wait that ends within a few seconds', async () => { + const { worker, applyProcessUpdate } = buildWorker( + stored(at(3_000)) + ); + + await worker.clearRateLimitWait(message); + + expect(applyProcessUpdate).toHaveBeenCalledTimes(1); + }); + + it('keeps a wait that other messages still wait for', async () => { + const { worker, applyProcessUpdate } = buildWorker( + stored(at(10 * MINUTE)) + ); + + await worker.clearRateLimitWait(message); + + expect(applyProcessUpdate).not.toHaveBeenCalled(); + }); + + it('leaves a cap in place', async () => { + const { worker, applyProcessUpdate } = buildWorker( + stored(at(-MINUTE), 'EXHAUSTED') + ); + + await worker.clearRateLimitWait(message); + + expect(applyProcessUpdate).not.toHaveBeenCalled(); + }); + + it('writes nothing when no wait is recorded', async () => { + const { worker, applyProcessUpdate } = buildWorker(); + + await worker.clearRateLimitWait(message); + + expect(applyProcessUpdate).not.toHaveBeenCalled(); + }); + + it('writes nothing for a message with no processId', async () => { + const { worker, applyProcessUpdate, findById } = buildWorker(); + + await worker.clearRateLimitWait({ data: {} }); + await worker.clearRateLimitWait({}); + + expect(findById).not.toHaveBeenCalled(); + expect(applyProcessUpdate).not.toHaveBeenCalled(); + }); + + it('does not fail when the process is gone', async () => { + const { worker, findById, applyProcessUpdate } = buildWorker(); + findById.mockResolvedValue(null); + + await expect( + worker.clearRateLimitWait(message) + ).resolves.toBeUndefined(); + expect(applyProcessUpdate).not.toHaveBeenCalled(); + }); }); }); From 3df9ca15a01a20fb6b8a13b52a7a8861418b449d Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 14:20:55 -0300 Subject: [PATCH 09/30] refactor(core): drop the comments of the rate-limit deferral 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 --- packages/core/core/Worker.js | 36 +++----------------------- packages/core/queues/queue-deferral.js | 14 ---------- packages/core/types/core/index.d.ts | 5 ---- 3 files changed, 4 insertions(+), 51 deletions(-) diff --git a/packages/core/core/Worker.js b/packages/core/core/Worker.js index 1823653de..eb6d559ca 100644 --- a/packages/core/core/Worker.js +++ b/packages/core/core/Worker.js @@ -127,18 +127,6 @@ class Worker { // parameters } - /** - * Puts a rate-limited message back so it runs at `error.retryAt`, without - * spending a receive. The message is sent again with a delay of up to 900 s, - * or scheduled with the one-time scheduler, or has its visibility timeout - * extended. The new message goes out before the old one is acknowledged, so - * delivery stays at least once. - * - * @returns {Promise<{outcome: 'acked'|'failed'|'skipped', skipped?: string, cause?: Error}>} - * `acked`: the message is handled. `failed`: report the record failed and - * let SQS redeliver it. `skipped`: nothing was done; fail the record as - * any other error. - */ async defer(record, body, error, delivery) { const retryAt = validDate(error.retryAt); if (!retryAt) return { outcome: 'skipped', skipped: 'no_retry_at' }; @@ -231,7 +219,6 @@ class Worker { return { outcome: 'acked' }; } - /** `acked`, `{ scheduleError }` when the schedule failed, null with no scheduler. */ async _deferWithSchedule({ record, body, @@ -313,11 +300,6 @@ class Worker { return { outcome: 'failed' }; } - /** - * The one-time scheduler, or null when the stack has none. The mock - * scheduler is never used: a schedule kept in memory would be lost with - * the message. - */ getSchedulerService() { if (this._schedulerService !== undefined) return this._schedulerService; this._schedulerService = null; @@ -336,22 +318,12 @@ class Worker { return this._schedulerService; } - /** - * Hook: the message was put back for a rate limit. Runs after it was sent. - * @param {Object} body The parsed message body. - * @param {Error} error The RateLimitError. - * @param {{status: 'WAITING'|'EXHAUSTED', mechanism: string, deferrals: number, retryAt: Date}} state - */ - async recordRateLimitWait() { - // A subclass keeps the run state; nothing to do by default. + recordRateLimitWait() { + return Promise.resolve(); } - /** - * Hook: a message that was put back for a rate limit ran without error. - * @param {Object} body The parsed message body. - */ - async clearRateLimitWait() { - // A subclass keeps the run state; nothing to do by default. + clearRateLimitWait() { + return Promise.resolve(); } _queueUrl(queueName) { diff --git a/packages/core/queues/queue-deferral.js b/packages/core/queues/queue-deferral.js index 3af58a536..e432b5efb 100644 --- a/packages/core/queues/queue-deferral.js +++ b/packages/core/queues/queue-deferral.js @@ -10,26 +10,12 @@ const toPositiveInteger = (value) => { return Number.isInteger(number) && number > 0 ? number : undefined; }; -/** - * @typedef {Object} DeferralLimits - * @property {number} maxDeferrals Times one message may be put back for a rate limit. - * @property {number} maxDeferredMs Longest total time from the first deferral to the last retry time. - */ - -/** - * @param {Object} [env=process.env] - * @returns {DeferralLimits} - */ const readDeferralLimits = (env = process.env) => ({ maxDeferrals: toPositiveInteger(env[MAX_DEFERRALS_ENV]) ?? MAX_DEFERRALS, maxDeferredMs: toPositiveInteger(env[MAX_DEFERRED_MS_ENV]) ?? MAX_DEFERRED_MS, }); -/** - * The deferral counters a message body carries in `_frigg`. - * @returns {{deferrals: number, firstDeferredAt: string|undefined}} - */ const readDeferral = (body) => { const frigg = body?._frigg; const deferrals = diff --git a/packages/core/types/core/index.d.ts b/packages/core/types/core/index.d.ts index 9c9a045e8..7034b8624 100644 --- a/packages/core/types/core/index.d.ts +++ b/packages/core/types/core/index.d.ts @@ -48,17 +48,12 @@ declare module "@friggframework/core" { sendAsyncSQSMessage(params: SendSQSMessageParams): Promise; - /** - * Hook: a message was put back for a rate limit. Runs after the message - * was sent. A failure is logged and never changes the outcome. - */ recordRateLimitWait( body: object, error: Error, state: RateLimitWaitState ): Promise; - /** Hook: a message that was put back for a rate limit ran without error. */ clearRateLimitWait(body: object): Promise; } From d20b32f6801d932e1d9bf5e5ac9f8f298ddce25c Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 14:20:55 -0300 Subject: [PATCH 10/30] refactor(devtools): keep the scheduler builder comment as it was 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 --- .../infrastructure/domains/scheduler/scheduler-builder.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.js b/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.js index 721375fc8..0e247c2f4 100644 --- a/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.js +++ b/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.js @@ -37,8 +37,7 @@ class SchedulerBuilder extends InfrastructureBuilder { return true; } - // Check if any integration has webhooks enabled, or a module that - // declares a rate limit (the queue worker schedules a deferred message) + // Check if any integration has webhooks enabled if (Array.isArray(appDefinition.integrations)) { return appDefinition.integrations.some( (integration) => From 4536c1706baf51b56da3d00b4a05a31906edaebf Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 14:23:17 -0300 Subject: [PATCH 11/30] test(core): build the deferred message body of the Worker tests in one helper deferredBody() and deferredRecord() replace the six inline copies of a body with _frigg counters. Co-Authored-By: Claude Opus 5.5 --- packages/core/core/Worker.deferral.test.js | 74 +++++++--------------- 1 file changed, 24 insertions(+), 50 deletions(-) diff --git a/packages/core/core/Worker.deferral.test.js b/packages/core/core/Worker.deferral.test.js index 285362916..7519887bf 100644 --- a/packages/core/core/Worker.deferral.test.js +++ b/packages/core/core/Worker.deferral.test.js @@ -62,6 +62,19 @@ function sqsRecord(overrides = {}) { }; } +function deferredBody( + deferrals, + firstDeferredAt = new Date(NOW).toISOString() +) { + return { ...BODY, _frigg: { deferrals, firstDeferredAt } }; +} + +function deferredRecord(deferrals, firstDeferredAt) { + return sqsRecord({ + body: JSON.stringify(deferredBody(deferrals, firstDeferredAt)), + }); +} + describe('Worker rate-limit deferral (ADR-049)', () => { let sqsMock; let schedulerMock; @@ -134,13 +147,7 @@ describe('Worker rate-limit deferral (ADR-049)', () => { QueueUrl: QUEUE_URL, DelaySeconds: 60, }); - expect(sentBody()).toEqual({ - ...BODY, - _frigg: { - deferrals: 1, - firstDeferredAt: new Date(NOW).toISOString(), - }, - }); + expect(sentBody()).toEqual(deferredBody(1)); expect(worker._run).toHaveBeenCalledTimes(1); }); @@ -175,14 +182,7 @@ describe('Worker rate-limit deferral (ADR-049)', () => { const firstDeferredAt = new Date(NOW - 10 * MINUTE).toISOString(); worker._run.mockRejectedValue(rateLimited(60 * SECOND)); - await run( - sqsRecord({ - body: JSON.stringify({ - ...BODY, - _frigg: { deferrals: 2, firstDeferredAt }, - }), - }) - ); + await run(deferredRecord(2, firstDeferredAt)); expect(sentBody()._frigg).toEqual({ deferrals: 3, @@ -220,13 +220,12 @@ describe('Worker rate-limit deferral (ADR-049)', () => { it('hands the deferred message to _run with the counters and a fresh delivery', async () => { const firstDeferredAt = new Date(NOW - MINUTE).toISOString(); - const body = { ...BODY, _frigg: { deferrals: 1, firstDeferredAt } }; worker._run.mockResolvedValue(undefined); - await run(sqsRecord({ body: JSON.stringify(body) })); + await run(deferredRecord(1, firstDeferredAt)); expect(worker._run).toHaveBeenCalledWith( - body, + deferredBody(1, firstDeferredAt), {}, expect.objectContaining({ receiveCount: 1 }) ); @@ -255,13 +254,7 @@ describe('Worker rate-limit deferral (ADR-049)', () => { ActionAfterCompletion: 'DELETE', Target: { Arn: QUEUE_ARN, RoleArn: SCHEDULER_ROLE_ARN }, }); - expect(JSON.parse(schedule.Target.Input)).toEqual({ - ...BODY, - _frigg: { - deferrals: 1, - firstDeferredAt: new Date(NOW).toISOString(), - }, - }); + expect(JSON.parse(schedule.Target.Input)).toEqual(deferredBody(1)); }); it('takes a schedule that exists already as scheduled', async () => { @@ -367,14 +360,7 @@ describe('Worker rate-limit deferral (ADR-049)', () => { const firstDeferredAt = new Date(NOW - MINUTE).toISOString(); worker._run.mockRejectedValue(rateLimited(60 * SECOND)); - const result = await run( - sqsRecord({ - body: JSON.stringify({ - ...BODY, - _frigg: { deferrals: 2, firstDeferredAt }, - }), - }) - ); + const result = await run(deferredRecord(2, firstDeferredAt)); expect(result).toEqual(failure()); expect(sent()).toEqual([]); @@ -403,14 +389,7 @@ describe('Worker rate-limit deferral (ADR-049)', () => { const firstDeferredAt = new Date(NOW - 23 * HOUR).toISOString(); worker._run.mockRejectedValue(rateLimited(2 * HOUR)); - const result = await run( - sqsRecord({ - body: JSON.stringify({ - ...BODY, - _frigg: { deferrals: 1, firstDeferredAt }, - }), - }) - ); + const result = await run(deferredRecord(1, firstDeferredAt)); expect(result).toEqual(failure()); expect(logged('record_deferral_capped')).toHaveLength(1); @@ -637,17 +616,12 @@ describe('Worker rate-limit deferral (ADR-049)', () => { it('clears the wait after a deferred message succeeds', async () => { worker._run.mockResolvedValue(undefined); - const body = { - ...BODY, - _frigg: { - deferrals: 1, - firstDeferredAt: new Date(NOW).toISOString(), - }, - }; - await run(sqsRecord({ body: JSON.stringify(body) })); + await run(deferredRecord(1)); - expect(worker.clearRateLimitWait).toHaveBeenCalledWith(body); + expect(worker.clearRateLimitWait).toHaveBeenCalledWith( + deferredBody(1) + ); }); it('clears the wait after a redelivered message succeeds', async () => { From 897a1f7508519b0c10dda27abcf133f1fb87582d Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 18:10:02 -0300 Subject: [PATCH 12/30] feat(core): allow 30 deferrals over 26 hours by default 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 --- packages/core/core/Worker.deferral.test.js | 13 ++++++++----- packages/core/queues/queue-deferral.js | 4 ++-- packages/core/queues/queue-deferral.test.js | 10 +++++----- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/packages/core/core/Worker.deferral.test.js b/packages/core/core/Worker.deferral.test.js index 7519887bf..edb67ba22 100644 --- a/packages/core/core/Worker.deferral.test.js +++ b/packages/core/core/Worker.deferral.test.js @@ -14,6 +14,7 @@ const { RateLimitError } = require('../errors'); const { createMemorySink } = require('../logs'); const { MAX_DEFERRALS_ENV, + MAX_DEFERRED_MS, MAX_DEFERRED_MS_ENV, } = require('../queues/queue-deferral'); @@ -375,7 +376,7 @@ describe('Worker rate-limit deferral (ADR-049)', () => { }); it('reports the record failed when the retry time is past the total time', async () => { - worker._run.mockRejectedValue(rateLimited(25 * HOUR)); + worker._run.mockRejectedValue(rateLimited(MAX_DEFERRED_MS + HOUR)); const result = await run(); @@ -386,7 +387,9 @@ describe('Worker rate-limit deferral (ADR-049)', () => { }); it('counts the total time from the first deferral', async () => { - const firstDeferredAt = new Date(NOW - 23 * HOUR).toISOString(); + const firstDeferredAt = new Date( + NOW - (MAX_DEFERRED_MS - HOUR) + ).toISOString(); worker._run.mockRejectedValue(rateLimited(2 * HOUR)); const result = await run(deferredRecord(1, firstDeferredAt)); @@ -397,7 +400,7 @@ describe('Worker rate-limit deferral (ADR-049)', () => { it('writes an ERROR when a capped message is on its last delivery', async () => { process.env.FRIGG_QUEUE_MAX_RECEIVE_COUNT = '3'; - worker._run.mockRejectedValue(rateLimited(25 * HOUR)); + worker._run.mockRejectedValue(rateLimited(MAX_DEFERRED_MS + HOUR)); await run( sqsRecord({ attributes: { ApproximateReceiveCount: '3' } }) @@ -414,7 +417,7 @@ describe('Worker rate-limit deferral (ADR-049)', () => { it('writes no lost-message ERROR before the last delivery', async () => { process.env.FRIGG_QUEUE_MAX_RECEIVE_COUNT = '3'; - worker._run.mockRejectedValue(rateLimited(25 * HOUR)); + worker._run.mockRejectedValue(rateLimited(MAX_DEFERRED_MS + HOUR)); await run( sqsRecord({ attributes: { ApproximateReceiveCount: '2' } }) @@ -591,7 +594,7 @@ describe('Worker rate-limit deferral (ADR-049)', () => { }); it('records EXHAUSTED when a cap ended the deferrals', async () => { - worker._run.mockRejectedValue(rateLimited(25 * HOUR)); + worker._run.mockRejectedValue(rateLimited(MAX_DEFERRED_MS + HOUR)); await run(); diff --git a/packages/core/queues/queue-deferral.js b/packages/core/queues/queue-deferral.js index e432b5efb..cbaec63c8 100644 --- a/packages/core/queues/queue-deferral.js +++ b/packages/core/queues/queue-deferral.js @@ -1,5 +1,5 @@ -const MAX_DEFERRALS = 10; -const MAX_DEFERRED_MS = 24 * 60 * 60 * 1000; +const MAX_DEFERRALS = 30; +const MAX_DEFERRED_MS = 26 * 60 * 60 * 1000; const MAX_DELAY_SECONDS = 900; const MAX_VISIBILITY_TIMEOUT_SECONDS = 43_200; const MAX_DEFERRALS_ENV = 'FRIGG_QUEUE_MAX_DEFERRALS'; diff --git a/packages/core/queues/queue-deferral.test.js b/packages/core/queues/queue-deferral.test.js index 76c3ca517..643d7bf22 100644 --- a/packages/core/queues/queue-deferral.test.js +++ b/packages/core/queues/queue-deferral.test.js @@ -19,8 +19,8 @@ describe('queue-deferral', () => { it('exposes the SQS limits and the default caps', () => { expect(MAX_DELAY_SECONDS).toBe(900); expect(MAX_VISIBILITY_TIMEOUT_SECONDS).toBe(43_200); - expect(MAX_DEFERRALS).toBe(10); - expect(MAX_DEFERRED_MS).toBe(24 * HOUR); + expect(MAX_DEFERRALS).toBe(30); + expect(MAX_DEFERRED_MS).toBe(26 * HOUR); expect(MAX_DEFERRALS_ENV).toBe('FRIGG_QUEUE_MAX_DEFERRALS'); expect(MAX_DEFERRED_MS_ENV).toBe('FRIGG_QUEUE_MAX_DEFERRED_MS'); }); @@ -28,8 +28,8 @@ describe('queue-deferral', () => { describe('readDeferralLimits', () => { it('returns the defaults when the environment sets nothing', () => { expect(readDeferralLimits({})).toEqual({ - maxDeferrals: 10, - maxDeferredMs: 24 * HOUR, + maxDeferrals: 30, + maxDeferredMs: 26 * HOUR, }); }); @@ -50,7 +50,7 @@ describe('queue-deferral', () => { [MAX_DEFERRALS_ENV]: value, [MAX_DEFERRED_MS_ENV]: value, }) - ).toEqual({ maxDeferrals: 10, maxDeferredMs: 24 * HOUR }); + ).toEqual({ maxDeferrals: 30, maxDeferredMs: 26 * HOUR }); } ); From 208336f3a6f2905dc03c36309b1aa26608e6ad53 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 18:57:37 -0300 Subject: [PATCH 13/30] refactor(core): define toPositiveInteger once for the queue modules queue-deferral.js had a copy of the helper from queue-delivery.js. It now imports it. Co-Authored-By: Claude Opus 5.5 --- packages/core/queues/queue-deferral.js | 7 ++----- packages/core/queues/queue-delivery.js | 1 + 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/core/queues/queue-deferral.js b/packages/core/queues/queue-deferral.js index cbaec63c8..0f9ac07d9 100644 --- a/packages/core/queues/queue-deferral.js +++ b/packages/core/queues/queue-deferral.js @@ -1,3 +1,5 @@ +const { toPositiveInteger } = require('./queue-delivery'); + const MAX_DEFERRALS = 30; const MAX_DEFERRED_MS = 26 * 60 * 60 * 1000; const MAX_DELAY_SECONDS = 900; @@ -5,11 +7,6 @@ const MAX_VISIBILITY_TIMEOUT_SECONDS = 43_200; const MAX_DEFERRALS_ENV = 'FRIGG_QUEUE_MAX_DEFERRALS'; const MAX_DEFERRED_MS_ENV = 'FRIGG_QUEUE_MAX_DEFERRED_MS'; -const toPositiveInteger = (value) => { - const number = Number(value); - return Number.isInteger(number) && number > 0 ? number : undefined; -}; - const readDeferralLimits = (env = process.env) => ({ maxDeferrals: toPositiveInteger(env[MAX_DEFERRALS_ENV]) ?? MAX_DEFERRALS, maxDeferredMs: diff --git a/packages/core/queues/queue-delivery.js b/packages/core/queues/queue-delivery.js index 8b5bebb9f..5e2e02299 100644 --- a/packages/core/queues/queue-delivery.js +++ b/packages/core/queues/queue-delivery.js @@ -38,4 +38,5 @@ module.exports = { INTEGRATION_QUEUE_MAX_RECEIVE_COUNT, QUEUE_MAX_RECEIVE_COUNT_ENV, readQueueDelivery, + toPositiveInteger, }; From b24ecef3c24cca4e32c3e2171286dbcbc34a0186 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 18:57:48 -0300 Subject: [PATCH 14/30] fix(core): keep the visibility timeout under the 12 hours that SQS counts 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 --- packages/core/core/Worker.deferral.test.js | 6 +++--- packages/core/core/Worker.js | 4 ++-- packages/core/queues/queue-deferral.js | 7 +++++++ 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/packages/core/core/Worker.deferral.test.js b/packages/core/core/Worker.deferral.test.js index edb67ba22..2dd3a735e 100644 --- a/packages/core/core/Worker.deferral.test.js +++ b/packages/core/core/Worker.deferral.test.js @@ -326,13 +326,13 @@ describe('Worker rate-limit deferral (ADR-049)', () => { ]); }); - it('caps the visibility timeout at 12 hours', async () => { + it('keeps the visibility timeout 20 minutes under the 12 hours that SQS counts from the receive', async () => { process.env[MAX_DEFERRED_MS_ENV] = String(48 * HOUR); - worker._run.mockRejectedValue(rateLimited(20 * HOUR)); + worker._run.mockRejectedValue(rateLimited(13 * HOUR)); await run(); - expect(visibility()[0].VisibilityTimeout).toBe(43_200); + expect(visibility()[0].VisibilityTimeout).toBe(42_000); }); it('leaves the message to SQS when the visibility change fails', async () => { diff --git a/packages/core/core/Worker.js b/packages/core/core/Worker.js index eb6d559ca..693afe223 100644 --- a/packages/core/core/Worker.js +++ b/packages/core/core/Worker.js @@ -11,8 +11,8 @@ const { get } = require('../assertions'); const { readQueueDelivery } = require('../queues/queue-delivery'); const { awsConfigOptions } = require('../queues/queuer-util'); const { + MAX_DEFERRED_VISIBILITY_SECONDS, MAX_DELAY_SECONDS, - MAX_VISIBILITY_TIMEOUT_SECONDS, isDeferralCapped, nextDeferral, readDeferral, @@ -266,7 +266,7 @@ class Worker { }) { const visibilityTimeout = Math.min( Math.ceil(waitMs / 1000), - MAX_VISIBILITY_TIMEOUT_SECONDS + MAX_DEFERRED_VISIBILITY_SECONDS ); try { const queueUrl = await this._queueUrl(queueName); diff --git a/packages/core/queues/queue-deferral.js b/packages/core/queues/queue-deferral.js index 0f9ac07d9..b28e7aafe 100644 --- a/packages/core/queues/queue-deferral.js +++ b/packages/core/queues/queue-deferral.js @@ -4,6 +4,12 @@ const MAX_DEFERRALS = 30; const MAX_DEFERRED_MS = 26 * 60 * 60 * 1000; const MAX_DELAY_SECONDS = 900; const MAX_VISIBILITY_TIMEOUT_SECONDS = 43_200; +const MAX_LAMBDA_RUN_SECONDS = 900; +const MAX_BATCHING_WINDOW_SECONDS = 300; +const MAX_DEFERRED_VISIBILITY_SECONDS = + MAX_VISIBILITY_TIMEOUT_SECONDS - + MAX_LAMBDA_RUN_SECONDS - + MAX_BATCHING_WINDOW_SECONDS; const MAX_DEFERRALS_ENV = 'FRIGG_QUEUE_MAX_DEFERRALS'; const MAX_DEFERRED_MS_ENV = 'FRIGG_QUEUE_MAX_DEFERRED_MS'; @@ -52,6 +58,7 @@ module.exports = { MAX_DEFERRALS_ENV, MAX_DEFERRED_MS, MAX_DEFERRED_MS_ENV, + MAX_DEFERRED_VISIBILITY_SECONDS, MAX_DELAY_SECONDS, MAX_VISIBILITY_TIMEOUT_SECONDS, isDeferralCapped, From 6a66f4e3a245d8546188235217e03cb546133e31 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 18:57:56 -0300 Subject: [PATCH 15/30] refactor(core): pass one deferral object through the deferral tiers 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 --- packages/core/core/Worker.js | 247 ++++++++++++------------- packages/core/queues/queue-deferral.js | 12 +- 2 files changed, 122 insertions(+), 137 deletions(-) diff --git a/packages/core/core/Worker.js b/packages/core/core/Worker.js index 693afe223..d0a4f0808 100644 --- a/packages/core/core/Worker.js +++ b/packages/core/core/Worker.js @@ -1,4 +1,3 @@ -const { randomUUID } = require('node:crypto'); const { SQSClient, GetQueueUrlCommand, @@ -13,6 +12,7 @@ const { awsConfigOptions } = require('../queues/queuer-util'); const { MAX_DEFERRED_VISIBILITY_SECONDS, MAX_DELAY_SECONDS, + deferredMs, isDeferralCapped, nextDeferral, readDeferral, @@ -67,16 +67,16 @@ class Worker { await this._clearDeferredState(runParams, delivery); log.debug('Record succeeded', { eventName: 'frigg.worker.record_succeeded' }); } catch (error) { - let deferral; + let deferralResult; if (error.isRateLimited && runParams) { - deferral = await this.defer( + deferralResult = await this.defer( record, runParams, error, delivery ); - if (deferral.outcome === 'acked') return; - if (deferral.outcome === 'failed') { + if (deferralResult.outcome === 'acked') return; + if (deferralResult.outcome === 'failed') { batchItemFailures.push({ itemIdentifier: record.messageId, }); @@ -99,11 +99,11 @@ class Worker { log.warn('Record failed, returned for retry', { eventName: 'frigg.worker.record_failed', error, - ...(deferral && { + ...(deferralResult && { deferral: { - skipped: deferral.skipped, - ...(deferral.cause && { - cause: causeOf(deferral.cause), + skipped: deferralResult.skipped, + ...(deferralResult.cause && { + cause: causeOf(deferralResult.cause), }), }, }), @@ -136,134 +136,79 @@ class Worker { } const now = Date.now(); - const waitMs = Math.max(0, retryAt.getTime() - now); - const deferral = nextDeferral(body, now); - const details = { - waitMs, - retryAt: retryAt.toISOString(), - deferrals: deferral.deferrals, - firstDeferredAt: deferral.firstDeferredAt, - reason: error.reason, - module: error.module, - statusCode: error.statusCode, - }; - const state = (status, mechanism) => ({ - status, - mechanism, - deferrals: deferral.deferrals, - retryAt, - }); - - const limits = readDeferralLimits(); - if (isDeferralCapped({ ...deferral, retryAt }, limits)) { - log.warn('Rate-limit deferral capped', { - eventName: 'frigg.worker.record_deferral_capped', - ...details, - maxDeferrals: limits.maxDeferrals, - maxDeferredMs: limits.maxDeferredMs, - deferredMs: - retryAt.getTime() - Date.parse(deferral.firstDeferredAt), - error, - }); - await this._recordWait(body, error, state('EXHAUSTED', 'none')); - this._logIfLost(delivery, error, details); - return { outcome: 'failed' }; - } - - const tier = { + const counters = nextDeferral(body, now); + const deferral = { record, queueName, body, - next: withDeferral(body, deferral), + deferredBody: withDeferral(body, counters), error, delivery, retryAt, - waitMs, - details, - state, + waitMs: Math.max(0, retryAt.getTime() - now), + counters, }; - if (waitMs <= MAX_DELAY_SECONDS * 1000) - return this._deferWithDelay(tier); - const scheduled = await this._deferWithSchedule(tier); + + const limits = readDeferralLimits(); + if (isDeferralCapped({ ...counters, retryAt }, limits)) { + await this._recordWait(deferral, 'EXHAUSTED', 'none'); + this._logUnacked( + deferral, + 'record_deferral_capped', + 'Rate-limit deferral capped', + { + maxDeferrals: limits.maxDeferrals, + maxDeferredMs: limits.maxDeferredMs, + deferredMs: deferredMs({ ...counters, retryAt }), + } + ); + return { outcome: 'failed' }; + } + + if (deferral.waitMs <= MAX_DELAY_SECONDS * 1000) + return this._deferWithDelay(deferral); + const scheduled = await this._deferWithSchedule(deferral); if (scheduled?.outcome) return scheduled; - return this._deferWithVisibility({ - ...tier, - scheduleError: scheduled?.scheduleError, - }); + return this._deferWithVisibility(deferral, scheduled?.scheduleError); } - async _deferWithDelay({ - queueName, - body, - next, - error, - waitMs, - details, - state, - }) { - const delaySeconds = Math.ceil(waitMs / 1000); + async _deferWithDelay(deferral) { + const delaySeconds = Math.ceil(deferral.waitMs / 1000); try { - const queueUrl = await this._queueUrl(queueName); - await this.send({ ...next, QueueUrl: queueUrl }, delaySeconds); + const queueUrl = await this._queueUrl(deferral.queueName); + await this.send( + { ...deferral.deferredBody, QueueUrl: queueUrl }, + delaySeconds + ); } catch (cause) { return { outcome: 'skipped', skipped: 'send_failed', cause }; } - await this._recordWait(body, error, state('WAITING', 'delay')); - log.warn('Record deferred', { - eventName: 'frigg.worker.record_deferred', - mechanism: 'delay', - delaySeconds, - ...details, - error, - }); - return { outcome: 'acked' }; + return this._acknowledgeDeferral(deferral, 'delay', { delaySeconds }); } - async _deferWithSchedule({ - record, - body, - next, - error, - retryAt, - details, - state, - }) { + async _deferWithSchedule(deferral) { + const { record, retryAt, deferredBody } = deferral; const scheduler = this.getSchedulerService(); if (!scheduler) return null; - const scheduleName = `frigg-defer-${record.messageId || randomUUID()}`; + const scheduleName = `frigg-defer-${record.messageId}`; try { await scheduler.scheduleOneTime({ scheduleName, scheduleAt: retryAt, queueResourceId: record.eventSourceARN, - payload: next, + payload: deferredBody, }); } catch (cause) { if (cause?.name !== 'ConflictException') return { scheduleError: cause }; } - await this._recordWait(body, error, state('WAITING', 'schedule')); - log.warn('Record deferred', { - eventName: 'frigg.worker.record_deferred', - mechanism: 'schedule', + return this._acknowledgeDeferral(deferral, 'schedule', { scheduleName, - ...details, - error, }); - return { outcome: 'acked' }; } - async _deferWithVisibility({ - record, - queueName, - body, - error, - delivery, - waitMs, - details, - state, - scheduleError, - }) { + async _deferWithVisibility(deferral, scheduleError) { + const { record, queueName, delivery, waitMs } = deferral; const visibilityTimeout = Math.min( Math.ceil(waitMs / 1000), MAX_DEFERRED_VISIBILITY_SECONDS @@ -281,25 +226,35 @@ class Worker { return { outcome: 'skipped', skipped: 'visibility_failed', cause }; } await this._recordWait( - body, - error, - state( - delivery.isLastAttempt ? 'EXHAUSTED' : 'WAITING', - 'visibility' - ) + deferral, + delivery.isLastAttempt ? 'EXHAUSTED' : 'WAITING', + 'visibility' + ); + this._logUnacked( + deferral, + 'record_visibility_extended', + 'Record visibility extended', + { + mechanism: 'visibility', + visibilityTimeout, + ...(scheduleError && { scheduleError: causeOf(scheduleError) }), + } ); - log.warn('Record visibility extended', { - eventName: 'frigg.worker.record_visibility_extended', - mechanism: 'visibility', - visibilityTimeout, - ...details, - ...(scheduleError && { scheduleError: causeOf(scheduleError) }), - error, - }); - this._logIfLost(delivery, error, details); return { outcome: 'failed' }; } + async _acknowledgeDeferral(deferral, mechanism, fields) { + await this._recordWait(deferral, 'WAITING', mechanism); + log.warn('Record deferred', { + eventName: 'frigg.worker.record_deferred', + mechanism, + ...fields, + ...this._deferralLogFields(deferral), + error: deferral.error, + }); + return { outcome: 'acked' }; + } + getSchedulerService() { if (this._schedulerService !== undefined) return this._schedulerService; this._schedulerService = null; @@ -340,9 +295,14 @@ class Worker { return this._queueUrls.get(queueName); } - async _recordWait(body, error, state) { + async _recordWait({ body, error, counters, retryAt }, status, mechanism) { await this._runStateHook('record', () => - this.recordRateLimitWait(body, error, state) + this.recordRateLimitWait(body, error, { + status, + mechanism, + deferrals: counters.deferrals, + retryAt, + }) ); } @@ -364,17 +324,42 @@ class Worker { } } - _logIfLost(delivery, error, details) { - if (!delivery.isLastAttempt) return; + _deferralLogFields({ error, retryAt, waitMs, counters }) { + return { + waitMs, + retryAt: retryAt.toISOString(), + deferrals: counters.deferrals, + firstDeferredAt: counters.firstDeferredAt, + reason: error.reason, + module: error.module, + statusCode: error.statusCode, + }; + } + + _logUnacked(deferral, eventName, message, fields) { + const logFields = { + ...fields, + ...this._deferralLogFields(deferral), + error: deferral.error, + }; + log.warn(message, { + eventName: `frigg.worker.${eventName}`, + ...logFields, + }); + if (deferral.delivery.isLastAttempt) { + this._logLost({ + retryAt: logFields.retryAt, + reason: logFields.reason, + module: logFields.module, + error: deferral.error, + }); + } + } + + _logLost(fields) { log.error( 'Rate-limited record lost: the message goes to the dead-letter queue next', - { - eventName: 'frigg.worker.record_lost_rate_limited', - retryAt: details.retryAt, - reason: error.reason, - module: error.module, - error, - } + { eventName: 'frigg.worker.record_lost_rate_limited', ...fields } ); } diff --git a/packages/core/queues/queue-deferral.js b/packages/core/queues/queue-deferral.js index b28e7aafe..a3f5183ce 100644 --- a/packages/core/queues/queue-deferral.js +++ b/packages/core/queues/queue-deferral.js @@ -46,12 +46,11 @@ const withDeferral = (body, deferral) => ({ _frigg: { ...body._frigg, ...deferral }, }); -const isDeferralCapped = ( - { deferrals, firstDeferredAt, retryAt }, - { maxDeferrals, maxDeferredMs } -) => - deferrals > maxDeferrals || - retryAt.getTime() - Date.parse(firstDeferredAt) > maxDeferredMs; +const deferredMs = ({ firstDeferredAt, retryAt }) => + retryAt.getTime() - Date.parse(firstDeferredAt); + +const isDeferralCapped = (deferral, { maxDeferrals, maxDeferredMs }) => + deferral.deferrals > maxDeferrals || deferredMs(deferral) > maxDeferredMs; module.exports = { MAX_DEFERRALS, @@ -61,6 +60,7 @@ module.exports = { MAX_DEFERRED_VISIBILITY_SECONDS, MAX_DELAY_SECONDS, MAX_VISIBILITY_TIMEOUT_SECONDS, + deferredMs, isDeferralCapped, nextDeferral, readDeferral, From efd2dcf5043261628afc6dd8e3be42291c4f214c Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 18:58:04 -0300 Subject: [PATCH 16/30] fix(core): write one log record for each deferral outcome 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 --- packages/core/core/Worker.deferral.test.js | 45 ++++++++++++++++++++ packages/core/core/Worker.js | 49 ++++++++++++---------- 2 files changed, 73 insertions(+), 21 deletions(-) diff --git a/packages/core/core/Worker.deferral.test.js b/packages/core/core/Worker.deferral.test.js index 2dd3a735e..96ed81114 100644 --- a/packages/core/core/Worker.deferral.test.js +++ b/packages/core/core/Worker.deferral.test.js @@ -411,8 +411,11 @@ describe('Worker rate-limit deferral (ADR-049)', () => { level: 'ERROR', receiveCount: 3, reason: 'burst', + maxDeferrals: 30, + maxDeferredMs: MAX_DEFERRED_MS, }), ]); + expect(logged('record_deferral_capped')).toEqual([]); }); it('writes no lost-message ERROR before the last delivery', async () => { @@ -424,6 +427,48 @@ describe('Worker rate-limit deferral (ADR-049)', () => { ); expect(logged('record_lost_rate_limited')).toEqual([]); + expect(logged('record_deferral_capped')).toHaveLength(1); + }); + + it('writes one ERROR, and no WARN, for a visibility change on the last delivery', async () => { + process.env.FRIGG_QUEUE_MAX_RECEIVE_COUNT = '3'; + worker._run.mockRejectedValue(rateLimited(20 * MINUTE)); + + await run( + sqsRecord({ attributes: { ApproximateReceiveCount: '3' } }) + ); + + expect(logged('record_lost_rate_limited')).toEqual([ + expect.objectContaining({ + level: 'ERROR', + mechanism: 'visibility', + visibilityTimeout: 1200, + }), + ]); + expect(logged('record_visibility_extended')).toEqual([]); + }); + + it('writes one ERROR, and no record_failed, when a deferral fails on the last delivery', async () => { + process.env.FRIGG_QUEUE_MAX_RECEIVE_COUNT = '3'; + sqsMock.on(SendMessageCommand).rejects(new Error('SQS is down')); + worker._run.mockRejectedValue(rateLimited(60 * SECOND)); + + const result = await run( + sqsRecord({ attributes: { ApproximateReceiveCount: '3' } }) + ); + + expect(result).toEqual(failure()); + expect(logged('record_lost_rate_limited')).toEqual([ + expect.objectContaining({ + level: 'ERROR', + reason: 'burst', + deferral: { + skipped: 'send_failed', + cause: expect.stringContaining('SQS is down'), + }, + }), + ]); + expect(logged('record_failed')).toEqual([]); }); }); diff --git a/packages/core/core/Worker.js b/packages/core/core/Worker.js index d0a4f0808..ae74b0b57 100644 --- a/packages/core/core/Worker.js +++ b/packages/core/core/Worker.js @@ -95,19 +95,30 @@ class Worker { }); return; } - // The message goes back to SQS, so WARN (ADR-048 §4). - log.warn('Record failed, returned for retry', { - eventName: 'frigg.worker.record_failed', - error, - ...(deferralResult && { - deferral: { - skipped: deferralResult.skipped, - ...(deferralResult.cause && { - cause: causeOf(deferralResult.cause), - }), - }, - }), - }); + const skippedDeferral = deferralResult && { + deferral: { + skipped: deferralResult.skipped, + ...(deferralResult.cause && { + cause: causeOf(deferralResult.cause), + }), + }, + }; + if (skippedDeferral && delivery.isLastAttempt) { + this._logLost({ + retryAt: validDate(error.retryAt)?.toISOString(), + reason: error.reason, + module: error.module, + ...skippedDeferral, + error, + }); + } else { + // The message goes back to SQS, so WARN (ADR-048 §4). + log.warn('Record failed, returned for retry', { + eventName: 'frigg.worker.record_failed', + error, + ...skippedDeferral, + }); + } batchItemFailures.push({ itemIdentifier: record.messageId }); } }); @@ -342,18 +353,14 @@ class Worker { ...this._deferralLogFields(deferral), error: deferral.error, }; + if (deferral.delivery.isLastAttempt) { + this._logLost(logFields); + return; + } log.warn(message, { eventName: `frigg.worker.${eventName}`, ...logFields, }); - if (deferral.delivery.isLastAttempt) { - this._logLost({ - retryAt: logFields.retryAt, - reason: logFields.reason, - module: logFields.module, - error: deferral.error, - }); - } } _logLost(fields) { From 6e10d41d96ecae4ed2c32537037f2422aaa9e87b Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 18:58:11 -0300 Subject: [PATCH 17/30] fix(core): log the cause of a failed deferral as an error 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 --- packages/core/core/Worker.deferral.test.js | 14 ++++++++++---- packages/core/core/Worker.js | 7 +++---- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/packages/core/core/Worker.deferral.test.js b/packages/core/core/Worker.deferral.test.js index 96ed81114..c043f4c67 100644 --- a/packages/core/core/Worker.deferral.test.js +++ b/packages/core/core/Worker.deferral.test.js @@ -292,7 +292,9 @@ describe('Worker rate-limit deferral (ADR-049)', () => { ]); expect(logged('record_visibility_extended')).toEqual([ expect.objectContaining({ - scheduleError: expect.stringContaining('scheduler is down'), + scheduleError: expect.objectContaining({ + message: 'scheduler is down', + }), }), ]); }); @@ -348,7 +350,7 @@ describe('Worker rate-limit deferral (ADR-049)', () => { expect.objectContaining({ deferral: { skipped: 'visibility_failed', - cause: expect.stringContaining('gone'), + cause: expect.objectContaining({ message: 'gone' }), }, }), ]); @@ -464,7 +466,9 @@ describe('Worker rate-limit deferral (ADR-049)', () => { reason: 'burst', deferral: { skipped: 'send_failed', - cause: expect.stringContaining('SQS is down'), + cause: expect.objectContaining({ + message: 'SQS is down', + }), }, }), ]); @@ -517,7 +521,9 @@ describe('Worker rate-limit deferral (ADR-049)', () => { expect.objectContaining({ deferral: { skipped: 'send_failed', - cause: expect.stringContaining('SQS is down'), + cause: expect.objectContaining({ + message: 'SQS is down', + }), }, }), ]); diff --git a/packages/core/core/Worker.js b/packages/core/core/Worker.js index ae74b0b57..c5a2e075e 100644 --- a/packages/core/core/Worker.js +++ b/packages/core/core/Worker.js @@ -20,7 +20,7 @@ const { withDeferral, } = require('../queues/queue-deferral'); const { runMessageScope } = require('./invocation-scope'); -const { getLogger, serializeError } = require('../logs'); +const { getLogger } = require('../logs'); const sqs = new SQSClient({ region: process.env.AWS_REGION, @@ -32,7 +32,6 @@ const validDate = (value) => value instanceof Date && !Number.isNaN(value.getTime()) ? value : null; const queueNameOf = (arn) => typeof arn === 'string' && arn ? arn.split(':').pop() : undefined; -const causeOf = (error) => serializeError(error).message; class Worker { async getQueueURL(params) { @@ -99,7 +98,7 @@ class Worker { deferral: { skipped: deferralResult.skipped, ...(deferralResult.cause && { - cause: causeOf(deferralResult.cause), + cause: deferralResult.cause, }), }, }; @@ -248,7 +247,7 @@ class Worker { { mechanism: 'visibility', visibilityTimeout, - ...(scheduleError && { scheduleError: causeOf(scheduleError) }), + ...(scheduleError && { scheduleError }), } ); return { outcome: 'failed' }; From e4d6dec9854315265a8b5b5c976eac7b6bc0304a Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 18:58:20 -0300 Subject: [PATCH 18/30] fix(core): treat a scheduler that cannot be built like a failed schedule 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 --- packages/core/core/Worker.deferral.test.js | 29 ++++++++++++++++++++++ packages/core/core/Worker.js | 25 ++++++++++--------- 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/packages/core/core/Worker.deferral.test.js b/packages/core/core/Worker.deferral.test.js index c043f4c67..839004c19 100644 --- a/packages/core/core/Worker.deferral.test.js +++ b/packages/core/core/Worker.deferral.test.js @@ -299,6 +299,35 @@ describe('Worker rate-limit deferral (ADR-049)', () => { ]); }); + it('extends the visibility timeout when the scheduler cannot be built, and tries to build it again', async () => { + const factory = require('../infrastructure/scheduler/scheduler-service-factory'); + jest.spyOn(factory, 'createSchedulerService').mockImplementation( + () => { + throw new Error('bad scheduler config'); + } + ); + worker._run.mockRejectedValue(rateLimited(20 * MINUTE)); + + const first = await run(); + const second = await run(sqsRecord({ messageId: 'msg-2' })); + + expect(first).toEqual(failure()); + expect(second).toEqual(failure('msg-2')); + expect(visibility()).toHaveLength(2); + expect(logged('record_visibility_extended')).toEqual([ + expect.objectContaining({ + scheduleError: expect.objectContaining({ + message: 'bad scheduler config', + }), + }), + expect.objectContaining({ + scheduleError: expect.objectContaining({ + message: 'bad scheduler config', + }), + }), + ]); + }); + it('never uses the mock scheduler, even when the environment names it', async () => { process.env.SCHEDULER_PROVIDER = 'mock'; worker._run.mockRejectedValue(rateLimited(20 * MINUTE)); diff --git a/packages/core/core/Worker.js b/packages/core/core/Worker.js index c5a2e075e..d0ca1c0c6 100644 --- a/packages/core/core/Worker.js +++ b/packages/core/core/Worker.js @@ -198,10 +198,10 @@ class Worker { async _deferWithSchedule(deferral) { const { record, retryAt, deferredBody } = deferral; - const scheduler = this.getSchedulerService(); - if (!scheduler) return null; const scheduleName = `frigg-defer-${record.messageId}`; try { + const scheduler = this.getSchedulerService(); + if (!scheduler) return null; await scheduler.scheduleOneTime({ scheduleName, scheduleAt: retryAt, @@ -267,19 +267,20 @@ class Worker { getSchedulerService() { if (this._schedulerService !== undefined) return this._schedulerService; - this._schedulerService = null; if ( - process.env.SCHEDULER_ROLE_ARN && - process.env.SCHEDULER_PROVIDER !== 'mock' + !process.env.SCHEDULER_ROLE_ARN || + process.env.SCHEDULER_PROVIDER === 'mock' ) { - const { - createSchedulerService, - SCHEDULER_PROVIDERS, - } = require('../infrastructure/scheduler/scheduler-service-factory'); - this._schedulerService = createSchedulerService({ - provider: SCHEDULER_PROVIDERS.EVENTBRIDGE, - }); + this._schedulerService = null; + return null; } + const { + createSchedulerService, + SCHEDULER_PROVIDERS, + } = require('../infrastructure/scheduler/scheduler-service-factory'); + this._schedulerService = createSchedulerService({ + provider: SCHEDULER_PROVIDERS.EVENTBRIDGE, + }); return this._schedulerService; } From c01804e50347b24aeff15a5ffc6db3d2127dee13 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 18:32:09 -0300 Subject: [PATCH 19/30] refactor(core): move the rate-limit run state into two use cases 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 --- packages/core/handlers/backend-utils.js | 49 +++---- .../integration-defined-workers.test.js | 126 +---------------- .../use-cases/clear-rate-limit-wait.js | 23 +++ .../use-cases/clear-rate-limit-wait.test.js | 117 ++++++++++++++++ packages/core/integrations/use-cases/index.js | 6 +- .../use-cases/record-rate-limit-wait.js | 35 +++++ .../use-cases/record-rate-limit-wait.test.js | 132 ++++++++++++++++++ 7 files changed, 332 insertions(+), 156 deletions(-) create mode 100644 packages/core/integrations/use-cases/clear-rate-limit-wait.js create mode 100644 packages/core/integrations/use-cases/clear-rate-limit-wait.test.js create mode 100644 packages/core/integrations/use-cases/record-rate-limit-wait.js create mode 100644 packages/core/integrations/use-cases/record-rate-limit-wait.test.js diff --git a/packages/core/handlers/backend-utils.js b/packages/core/handlers/backend-utils.js index 672b62fbd..e7880bb4c 100644 --- a/packages/core/handlers/backend-utils.js +++ b/packages/core/handlers/backend-utils.js @@ -21,15 +21,12 @@ const { getModulesDefinitionFromIntegrationClasses, } = require('../integrations/utils/map-integration-dto'); const { processNotFound } = require('../integrations/use-cases/process-errors'); - -const CLEAR_RATE_LIMIT_TOLERANCE_MS = 5_000; - -const keepsCurrentRateLimit = (current, next) => { - if (!current) return false; - if (next.status === 'EXHAUSTED') return false; - if (current.status === 'EXHAUSTED') return true; - return Date.parse(current.retryAt) > next.retryAt.getTime(); -}; +const { + RecordRateLimitWait, +} = require('../integrations/use-cases/record-rate-limit-wait'); +const { + ClearRateLimitWait, +} = require('../integrations/use-cases/clear-rate-limit-wait'); const loadRouterFromObject = (IntegrationClass, routerObject) => { const router = Router(); @@ -177,37 +174,23 @@ const createQueueWorker = (integrationClass) => { async recordRateLimitWait(body, error, state) { const processId = body.data?.processId; if (!processId) return; - const processRepository = createProcessRepository(); - const process = await processRepository.findById(processId); - if (keepsCurrentRateLimit(process?.context?.rateLimit, state)) - return; - await processRepository.applyProcessUpdate(processId, { - set: { - 'context.rateLimit': { - status: state.status, - mechanism: state.mechanism, - retryAt: state.retryAt.toISOString(), - reason: error.reason, - module: error.module, - deferrals: state.deferrals, - updatedAt: new Date().toISOString(), - }, - }, + const recordRateLimitWait = new RecordRateLimitWait({ + processRepository: createProcessRepository(), + }); + await recordRateLimitWait.execute(processId, { + ...state, + reason: error.reason, + module: error.module, }); } async clearRateLimitWait(body) { const processId = body?.data?.processId; if (!processId) return; - const processRepository = createProcessRepository(); - const process = await processRepository.findById(processId); - const current = process?.context?.rateLimit; - if (current?.status !== 'WAITING') return; - const remainingMs = Date.parse(current.retryAt) - Date.now(); - if (remainingMs > CLEAR_RATE_LIMIT_TOLERANCE_MS) return; - await processRepository.applyProcessUpdate(processId, { - set: { 'context.rateLimit': null }, + const clearRateLimitWait = new ClearRateLimitWait({ + processRepository: createProcessRepository(), }); + await clearRateLimitWait.execute(processId); } async _run(params, context, delivery) { diff --git a/packages/core/handlers/workers/integration-defined-workers.test.js b/packages/core/handlers/workers/integration-defined-workers.test.js index a08b53b8b..2f430eb54 100644 --- a/packages/core/handlers/workers/integration-defined-workers.test.js +++ b/packages/core/handlers/workers/integration-defined-workers.test.js @@ -340,16 +340,16 @@ describe('Webhook Queue Worker', () => { const NOW = Date.parse('2026-09-28T12:00:00.000Z'); const MINUTE = 60_000; const at = (ms) => new Date(NOW + ms); - const stored = (retryAt, status = 'WAITING') => ({ - status, + const stored = (retryAt) => ({ + status: 'WAITING', mechanism: 'delay', deferrals: 1, retryAt: retryAt.toISOString(), reason: 'burst', module: 'hubspot', }); - const next = (retryAt, status = 'WAITING') => ({ - status, + const next = (retryAt) => ({ + status: 'WAITING', mechanism: 'delay', deferrals: 1, retryAt, @@ -417,62 +417,6 @@ describe('Webhook Queue Worker', () => { }); }); - it('replaces an earlier wait', async () => { - const { worker, applyProcessUpdate } = buildWorker( - stored(at(5 * MINUTE)) - ); - - await worker.recordRateLimitWait( - message, - rateLimitError, - next(at(20 * MINUTE)) - ); - - expect(applyProcessUpdate).toHaveBeenCalledTimes(1); - }); - - it('keeps a later wait that another message recorded', async () => { - const { worker, applyProcessUpdate } = buildWorker( - stored(at(40 * MINUTE)) - ); - - await worker.recordRateLimitWait( - message, - rateLimitError, - next(at(20 * MINUTE)) - ); - - expect(applyProcessUpdate).not.toHaveBeenCalled(); - }); - - it('shows a cap even when a later wait is recorded', async () => { - const { worker, applyProcessUpdate } = buildWorker( - stored(at(40 * MINUTE)) - ); - - await worker.recordRateLimitWait( - message, - rateLimitError, - next(at(20 * MINUTE), 'EXHAUSTED') - ); - - expect(applyProcessUpdate).toHaveBeenCalledTimes(1); - }); - - it('never hides a cap behind a later wait', async () => { - const { worker, applyProcessUpdate } = buildWorker( - stored(at(5 * MINUTE), 'EXHAUSTED') - ); - - await worker.recordRateLimitWait( - message, - rateLimitError, - next(at(20 * MINUTE)) - ); - - expect(applyProcessUpdate).not.toHaveBeenCalled(); - }); - it('writes nothing for a message with no processId', async () => { const { worker, applyProcessUpdate, findById } = buildWorker(); @@ -485,20 +429,6 @@ describe('Webhook Queue Worker', () => { expect(findById).not.toHaveBeenCalled(); expect(applyProcessUpdate).not.toHaveBeenCalled(); }); - - it('does not fail when the process is gone', async () => { - const { worker, findById, applyProcessUpdate } = buildWorker(); - findById.mockResolvedValue(null); - applyProcessUpdate.mockResolvedValue(null); - - await expect( - worker.recordRateLimitWait( - message, - rateLimitError, - next(at(MINUTE)) - ) - ).resolves.toBeUndefined(); - }); }); describe('clearing a wait', () => { @@ -514,44 +444,6 @@ describe('Webhook Queue Worker', () => { }); }); - it('clears a wait that ends within a few seconds', async () => { - const { worker, applyProcessUpdate } = buildWorker( - stored(at(3_000)) - ); - - await worker.clearRateLimitWait(message); - - expect(applyProcessUpdate).toHaveBeenCalledTimes(1); - }); - - it('keeps a wait that other messages still wait for', async () => { - const { worker, applyProcessUpdate } = buildWorker( - stored(at(10 * MINUTE)) - ); - - await worker.clearRateLimitWait(message); - - expect(applyProcessUpdate).not.toHaveBeenCalled(); - }); - - it('leaves a cap in place', async () => { - const { worker, applyProcessUpdate } = buildWorker( - stored(at(-MINUTE), 'EXHAUSTED') - ); - - await worker.clearRateLimitWait(message); - - expect(applyProcessUpdate).not.toHaveBeenCalled(); - }); - - it('writes nothing when no wait is recorded', async () => { - const { worker, applyProcessUpdate } = buildWorker(); - - await worker.clearRateLimitWait(message); - - expect(applyProcessUpdate).not.toHaveBeenCalled(); - }); - it('writes nothing for a message with no processId', async () => { const { worker, applyProcessUpdate, findById } = buildWorker(); @@ -561,16 +453,6 @@ describe('Webhook Queue Worker', () => { expect(findById).not.toHaveBeenCalled(); expect(applyProcessUpdate).not.toHaveBeenCalled(); }); - - it('does not fail when the process is gone', async () => { - const { worker, findById, applyProcessUpdate } = buildWorker(); - findById.mockResolvedValue(null); - - await expect( - worker.clearRateLimitWait(message) - ).resolves.toBeUndefined(); - expect(applyProcessUpdate).not.toHaveBeenCalled(); - }); }); }); diff --git a/packages/core/integrations/use-cases/clear-rate-limit-wait.js b/packages/core/integrations/use-cases/clear-rate-limit-wait.js new file mode 100644 index 000000000..bb1f41f5a --- /dev/null +++ b/packages/core/integrations/use-cases/clear-rate-limit-wait.js @@ -0,0 +1,23 @@ +const CLEAR_RATE_LIMIT_TOLERANCE_MS = 5_000; + +class ClearRateLimitWait { + constructor({ processRepository }) { + if (!processRepository) { + throw new Error('processRepository is required'); + } + this.processRepository = processRepository; + } + + async execute(processId) { + const process = await this.processRepository.findById(processId); + const current = process?.context?.rateLimit; + if (current?.status !== 'WAITING') return; + const remainingMs = Date.parse(current.retryAt) - Date.now(); + if (remainingMs > CLEAR_RATE_LIMIT_TOLERANCE_MS) return; + await this.processRepository.applyProcessUpdate(processId, { + set: { 'context.rateLimit': null }, + }); + } +} + +module.exports = { ClearRateLimitWait }; diff --git a/packages/core/integrations/use-cases/clear-rate-limit-wait.test.js b/packages/core/integrations/use-cases/clear-rate-limit-wait.test.js new file mode 100644 index 000000000..7fc33298f --- /dev/null +++ b/packages/core/integrations/use-cases/clear-rate-limit-wait.test.js @@ -0,0 +1,117 @@ +const { ClearRateLimitWait } = require('./clear-rate-limit-wait'); + +describe('ClearRateLimitWait', () => { + const NOW = Date.parse('2026-09-28T12:00:00.000Z'); + const MINUTE = 60_000; + const processId = 'process-123'; + const at = (ms) => new Date(NOW + ms); + const stored = (retryAt, status = 'WAITING') => ({ + status, + mechanism: 'delay', + deferrals: 1, + retryAt: retryAt.toISOString(), + reason: 'burst', + module: 'hubspot', + }); + let clearRateLimitWait; + let mockProcessRepository; + let nowSpy; + + const storeOnProcess = (rateLimit) => + mockProcessRepository.findById.mockResolvedValue({ + id: processId, + context: { rateLimit }, + }); + + beforeEach(() => { + nowSpy = jest.spyOn(Date, 'now').mockReturnValue(NOW); + mockProcessRepository = { + findById: jest + .fn() + .mockResolvedValue({ id: processId, context: {} }), + applyProcessUpdate: jest.fn().mockResolvedValue({ id: processId }), + }; + clearRateLimitWait = new ClearRateLimitWait({ + processRepository: mockProcessRepository, + }); + }); + + afterEach(() => nowSpy.mockRestore()); + + describe('constructor', () => { + it('should require processRepository', () => { + expect(() => new ClearRateLimitWait({})).toThrow( + 'processRepository is required' + ); + }); + + it('should initialize with processRepository', () => { + expect(clearRateLimitWait.processRepository).toBe( + mockProcessRepository + ); + }); + }); + + describe('execute', () => { + it('clears a wait that is over', async () => { + storeOnProcess(stored(at(-MINUTE))); + + await clearRateLimitWait.execute(processId); + + expect( + mockProcessRepository.applyProcessUpdate + ).toHaveBeenCalledWith(processId, { + set: { 'context.rateLimit': null }, + }); + }); + + it('clears a wait that ends within a few seconds', async () => { + storeOnProcess(stored(at(3_000))); + + await clearRateLimitWait.execute(processId); + + expect( + mockProcessRepository.applyProcessUpdate + ).toHaveBeenCalledTimes(1); + }); + + it('keeps a wait that other messages still wait for', async () => { + storeOnProcess(stored(at(10 * MINUTE))); + + await clearRateLimitWait.execute(processId); + + expect( + mockProcessRepository.applyProcessUpdate + ).not.toHaveBeenCalled(); + }); + + it('leaves a cap in place', async () => { + storeOnProcess(stored(at(-MINUTE), 'EXHAUSTED')); + + await clearRateLimitWait.execute(processId); + + expect( + mockProcessRepository.applyProcessUpdate + ).not.toHaveBeenCalled(); + }); + + it('writes nothing when no wait is recorded', async () => { + await clearRateLimitWait.execute(processId); + + expect( + mockProcessRepository.applyProcessUpdate + ).not.toHaveBeenCalled(); + }); + + it('does not fail when the process is gone', async () => { + mockProcessRepository.findById.mockResolvedValue(null); + + await expect( + clearRateLimitWait.execute(processId) + ).resolves.toBeUndefined(); + expect( + mockProcessRepository.applyProcessUpdate + ).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/packages/core/integrations/use-cases/index.js b/packages/core/integrations/use-cases/index.js index d7ce7a7fc..9c33015f7 100644 --- a/packages/core/integrations/use-cases/index.js +++ b/packages/core/integrations/use-cases/index.js @@ -6,6 +6,8 @@ const { CreateProcess } = require('./create-process'); const { UpdateProcessState } = require('./update-process-state'); const { UpdateProcessMetrics } = require('./update-process-metrics'); const { GetProcess } = require('./get-process'); +const { RecordRateLimitWait } = require('./record-rate-limit-wait'); +const { ClearRateLimitWait } = require('./clear-rate-limit-wait'); module.exports = { GetIntegrationsForUser, @@ -16,4 +18,6 @@ module.exports = { UpdateProcessState, UpdateProcessMetrics, GetProcess, -}; \ No newline at end of file + RecordRateLimitWait, + ClearRateLimitWait, +}; \ No newline at end of file diff --git a/packages/core/integrations/use-cases/record-rate-limit-wait.js b/packages/core/integrations/use-cases/record-rate-limit-wait.js new file mode 100644 index 000000000..b9bf9c787 --- /dev/null +++ b/packages/core/integrations/use-cases/record-rate-limit-wait.js @@ -0,0 +1,35 @@ +const keepsCurrentRateLimit = (current, next) => { + if (!current) return false; + if (next.status === 'EXHAUSTED') return false; + if (current.status === 'EXHAUSTED') return true; + return Date.parse(current.retryAt) > next.retryAt.getTime(); +}; + +class RecordRateLimitWait { + constructor({ processRepository }) { + if (!processRepository) { + throw new Error('processRepository is required'); + } + this.processRepository = processRepository; + } + + async execute(processId, wait) { + const process = await this.processRepository.findById(processId); + if (keepsCurrentRateLimit(process?.context?.rateLimit, wait)) return; + await this.processRepository.applyProcessUpdate(processId, { + set: { + 'context.rateLimit': { + status: wait.status, + mechanism: wait.mechanism, + retryAt: wait.retryAt.toISOString(), + reason: wait.reason, + module: wait.module, + deferrals: wait.deferrals, + updatedAt: new Date().toISOString(), + }, + }, + }); + } +} + +module.exports = { RecordRateLimitWait }; diff --git a/packages/core/integrations/use-cases/record-rate-limit-wait.test.js b/packages/core/integrations/use-cases/record-rate-limit-wait.test.js new file mode 100644 index 000000000..b84860543 --- /dev/null +++ b/packages/core/integrations/use-cases/record-rate-limit-wait.test.js @@ -0,0 +1,132 @@ +const { RecordRateLimitWait } = require('./record-rate-limit-wait'); + +describe('RecordRateLimitWait', () => { + const NOW = Date.parse('2026-09-28T12:00:00.000Z'); + const MINUTE = 60_000; + const processId = 'process-123'; + const at = (ms) => new Date(NOW + ms); + const stored = (retryAt, status = 'WAITING') => ({ + status, + mechanism: 'delay', + deferrals: 1, + retryAt: retryAt.toISOString(), + reason: 'burst', + module: 'hubspot', + }); + const wait = (retryAt, status = 'WAITING') => ({ + status, + mechanism: 'delay', + deferrals: 1, + retryAt, + reason: 'burst', + module: 'hubspot', + }); + let recordRateLimitWait; + let mockProcessRepository; + + const storeOnProcess = (rateLimit) => + mockProcessRepository.findById.mockResolvedValue({ + id: processId, + context: { rateLimit }, + }); + + beforeEach(() => { + mockProcessRepository = { + findById: jest + .fn() + .mockResolvedValue({ id: processId, context: {} }), + applyProcessUpdate: jest.fn().mockResolvedValue({ id: processId }), + }; + recordRateLimitWait = new RecordRateLimitWait({ + processRepository: mockProcessRepository, + }); + }); + + describe('constructor', () => { + it('should require processRepository', () => { + expect(() => new RecordRateLimitWait({})).toThrow( + 'processRepository is required' + ); + }); + + it('should initialize with processRepository', () => { + expect(recordRateLimitWait.processRepository).toBe( + mockProcessRepository + ); + }); + }); + + describe('execute', () => { + it('writes the wait as context.rateLimit on the process', async () => { + await recordRateLimitWait.execute(processId, wait(at(20 * MINUTE))); + + expect( + mockProcessRepository.applyProcessUpdate + ).toHaveBeenCalledWith(processId, { + set: { + 'context.rateLimit': { + status: 'WAITING', + mechanism: 'delay', + retryAt: '2026-09-28T12:20:00.000Z', + reason: 'burst', + module: 'hubspot', + deferrals: 1, + updatedAt: expect.any(String), + }, + }, + }); + }); + + it('replaces an earlier wait', async () => { + storeOnProcess(stored(at(5 * MINUTE))); + + await recordRateLimitWait.execute(processId, wait(at(20 * MINUTE))); + + expect( + mockProcessRepository.applyProcessUpdate + ).toHaveBeenCalledTimes(1); + }); + + it('keeps a later wait that another message recorded', async () => { + storeOnProcess(stored(at(40 * MINUTE))); + + await recordRateLimitWait.execute(processId, wait(at(20 * MINUTE))); + + expect( + mockProcessRepository.applyProcessUpdate + ).not.toHaveBeenCalled(); + }); + + it('shows a cap even when a later wait is recorded', async () => { + storeOnProcess(stored(at(40 * MINUTE))); + + await recordRateLimitWait.execute( + processId, + wait(at(20 * MINUTE), 'EXHAUSTED') + ); + + expect( + mockProcessRepository.applyProcessUpdate + ).toHaveBeenCalledTimes(1); + }); + + it('never hides a cap behind a later wait', async () => { + storeOnProcess(stored(at(5 * MINUTE), 'EXHAUSTED')); + + await recordRateLimitWait.execute(processId, wait(at(20 * MINUTE))); + + expect( + mockProcessRepository.applyProcessUpdate + ).not.toHaveBeenCalled(); + }); + + it('does not fail when the process is gone', async () => { + mockProcessRepository.findById.mockResolvedValue(null); + mockProcessRepository.applyProcessUpdate.mockResolvedValue(null); + + await expect( + recordRateLimitWait.execute(processId, wait(at(MINUTE))) + ).resolves.toBeUndefined(); + }); + }); +}); From 775b1b8c853a8f67ddcc9978bf806c3343575de1 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 18:40:23 -0300 Subject: [PATCH 20/30] test(core): clear the rate-limit run state after a deferred message runs 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 --- .../integration-defined-workers.test.js | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/packages/core/handlers/workers/integration-defined-workers.test.js b/packages/core/handlers/workers/integration-defined-workers.test.js index 2f430eb54..ff954033e 100644 --- a/packages/core/handlers/workers/integration-defined-workers.test.js +++ b/packages/core/handlers/workers/integration-defined-workers.test.js @@ -385,6 +385,24 @@ describe('Webhook Queue Worker', () => { }), }) ); + jest.doMock( + '../../integrations/repositories/integration-repository-factory', + () => ({ createIntegrationRepository: () => ({}) }) + ); + jest.doMock( + '../../modules/repositories/module-repository-factory', + () => ({ createModuleRepository: () => ({}) }) + ); + jest.doMock( + '../../integrations/use-cases/get-integration-instance', + () => ({ + GetIntegrationInstance: class { + async execute() { + return new TestWebhookIntegration(); + } + }, + }) + ); QueueWorker = require('../backend-utils').createQueueWorker( TestWebhookIntegration ); @@ -432,6 +450,47 @@ describe('Webhook Queue Worker', () => { }); describe('clearing a wait', () => { + const sqsRecord = (frigg) => ({ + messageId: 'msg-1', + body: JSON.stringify({ + event: 'ON_WEBHOOK', + data: { processId: 7 }, + ...(frigg && { _frigg: frigg }), + }), + attributes: { ApproximateReceiveCount: '1' }, + }); + + it('clears the wait after a deferred message runs', async () => { + const { worker, applyProcessUpdate } = buildWorker( + stored(at(-MINUTE)) + ); + + const result = await worker.run({ + Records: [ + sqsRecord({ + deferrals: 1, + firstDeferredAt: at(-20 * MINUTE).toISOString(), + }), + ], + }); + + expect(result.batchItemFailures).toEqual([]); + expect(applyProcessUpdate).toHaveBeenCalledWith(7, { + set: { 'context.rateLimit': null }, + }); + }); + + it('does not clear the wait after a first delivery runs', async () => { + const { worker, applyProcessUpdate } = buildWorker( + stored(at(-MINUTE)) + ); + + const result = await worker.run({ Records: [sqsRecord()] }); + + expect(result.batchItemFailures).toEqual([]); + expect(applyProcessUpdate).not.toHaveBeenCalled(); + }); + it('clears a wait that is over', async () => { const { worker, applyProcessUpdate } = buildWorker( stored(at(-MINUTE)) From 910d5a8d9e46e35c33d5501c346e47004ffd2369 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 18:46:30 -0300 Subject: [PATCH 21/30] docs(core): describe the deferral caps, the visibility margin and the 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 --- .claude/skills/frigg-scheduled-jobs/SKILL.md | 9 ++-- .../skills/frigg/references/infrastructure.md | 2 +- docs/guides/INTEGRATION-PATTERNS.md | 21 ++++---- docs/guides/LOGGING-IN-INTEGRATIONS.md | 13 ++++- packages/core/CLAUDE.md | 51 ++++++++++++------- packages/core/core/CLAUDE.md | 12 +++-- 6 files changed, 71 insertions(+), 37 deletions(-) diff --git a/.claude/skills/frigg-scheduled-jobs/SKILL.md b/.claude/skills/frigg-scheduled-jobs/SKILL.md index 7d5a736c7..47ebe5d84 100644 --- a/.claude/skills/frigg-scheduled-jobs/SKILL.md +++ b/.claude/skills/frigg-scheduled-jobs/SKILL.md @@ -1,6 +1,6 @@ --- name: frigg-scheduled-jobs -description: "Scheduling one-time deferred jobs in a Frigg integration via AWS EventBridge Scheduler — e.g. webhook subscription renewals or delayed tasks. Covers the scheduler command API (createSchedulerCommands: scheduleJob, deleteJob, getJobStatus), how the scheduler infrastructure is auto-provisioned (enabled by scheduler.enable or any integration with webhooks), the production EventBridge vs local mock provider, auto-cleanup after execution, and the required environment variables. Use when an integration needs to run a job at a future time, renew a webhook subscription, or schedule a deferred action." +description: "Scheduling one-time deferred jobs in a Frigg integration via AWS EventBridge Scheduler — e.g. webhook subscription renewals or delayed tasks. Covers the scheduler command API (createSchedulerCommands: scheduleJob, deleteJob, getJobStatus), how the scheduler infrastructure is auto-provisioned (enabled by scheduler.enable, any integration with webhooks, or any API module that declares static rateLimit), the production EventBridge vs local mock provider, auto-cleanup after execution, and the required environment variables. Use when an integration needs to run a job at a future time, renew a webhook subscription, or schedule a deferred action." --- # Frigg Scheduled Jobs @@ -56,9 +56,10 @@ STAGE=local ## Infrastructure provisioning -The scheduler infrastructure is generated by the scheduler domain builder and is **auto-enabled** when either: +The scheduler infrastructure is generated by the scheduler domain builder and is **auto-enabled** when any of these is true: -- `appDefinition.scheduler.enable === true`, or -- any integration declares webhooks (`Definition.webhooks.enabled === true` or `Definition.webhooks === true`) — webhooks commonly need renewal scheduling. +- `appDefinition.scheduler.enable === true`, +- any integration declares webhooks (`Definition.webhooks.enabled === true` or `Definition.webhooks === true`) — webhooks commonly need renewal scheduling, or +- any API module of an integration declares `static rateLimit` — the queue worker schedules a rate-limited message that must wait more than 15 minutes. When enabled, the builder creates an `AWS::Scheduler::ScheduleGroup` (`FriggScheduleGroup`) and an IAM role for EventBridge → SQS, and exposes `SCHEDULER_ROLE_ARN` to the functions. For the builder's place in the wider infra pipeline, see the `frigg` skill's `references/infrastructure.md`. Webhook-renewal scheduling pairs with the webhook patterns in the `frigg-extensions` skill. diff --git a/.claude/skills/frigg/references/infrastructure.md b/.claude/skills/frigg/references/infrastructure.md index a11714c3d..2e9a989da 100644 --- a/.claude/skills/frigg/references/infrastructure.md +++ b/.claude/skills/frigg/references/infrastructure.md @@ -50,7 +50,7 @@ Each builder implements: ## Scheduler -The scheduler builder (`scheduler/scheduler-builder.js`) auto-enables when any integration has `webhooks.enabled = true` (or explicitly via `appDefinition.scheduler.enable = true`). It creates an EventBridge Scheduler `ScheduleGroup` + an IAM role for EventBridge → SQS and exposes `SCHEDULER_ROLE_ARN`, enabling one-time scheduled jobs (e.g., webhook subscription renewals). +The scheduler builder (`scheduler/scheduler-builder.js`) auto-enables when any integration has `webhooks.enabled = true`, or when any API module declares `static rateLimit` (or explicitly via `appDefinition.scheduler.enable = true`). It creates an EventBridge Scheduler `ScheduleGroup` + an IAM role for EventBridge → SQS and exposes `SCHEDULER_ROLE_ARN`, enabling one-time scheduled jobs (e.g., webhook subscription renewals, or a rate-limited queue message that must wait more than 15 minutes). For scheduling jobs from integration code (`createSchedulerCommands`: `scheduleJob` / `deleteJob` / `getJobStatus`), the production-vs-mock provider, auto-cleanup, and env vars, see the **frigg-scheduled-jobs** skill. diff --git a/docs/guides/INTEGRATION-PATTERNS.md b/docs/guides/INTEGRATION-PATTERNS.md index 0f6af2a01..2cd3c7e89 100644 --- a/docs/guides/INTEGRATION-PATTERNS.md +++ b/docs/guides/INTEGRATION-PATTERNS.md @@ -401,9 +401,11 @@ the final try instead of leaving the run in progress. When a provider throttles a queue handler for longer than the Requester sleeps in process, the Requester throws `RateLimitError` with `retryAt`. Let it reach -the queue worker. Core puts the message back so it runs at `retryAt`, and does -not spend one of the message's three receives: it sends the body again with a -delay (up to 15 minutes), or schedules it, or extends its visibility timeout. +the queue worker. Core puts the message back so it runs at `retryAt`: it sends +the body again with a delay (up to 15 minutes), or schedules it, or extends its +visibility timeout (12 hours less 20 minutes at most). The delay and the +schedule send a new message, so they do not use one of the message's three +receives. The visibility timeout and a cap (see below) do use one. With a `processId` in the message, `Process.context.rateLimit` says the run is waiting and until when. It holds the latest known wait of the process, and is cleared when a message runs after that wait is over. @@ -421,10 +423,11 @@ async processBatch({ data, delivery }) { } ``` -A deferral past `FRIGG_QUEUE_MAX_DEFERRALS` (default 10) or -`FRIGG_QUEUE_MAX_DEFERRED_MS` (default 24 hours) fails the record like any other -retryable error, and `delivery` applies. Do not catch a `RateLimitError` to -write a retry of your own. +A deferral past `FRIGG_QUEUE_MAX_DEFERRALS` (default 30) or +`FRIGG_QUEUE_MAX_DEFERRED_MS` (default 26 hours) fails the record like any other +retryable error, and `delivery` applies. The defaults cover one full day of +hourly waits, for a module that probes a daily limit every hour. Do not catch a +`RateLimitError` to write a retry of your own. --- @@ -831,8 +834,8 @@ Put what you know about a provider's limits in its API module: declare The Requester then waits as the provider says. When the wait is too long to sleep, it throws `RateLimitError` with `retryAt`. A queue handler checks `delivery.isLastAttempt` first (see [Queue Handler Delivery](#queue-handler-delivery)) -and ends the run on the last attempt, then rethrows: the queue worker puts the -message back to run at `retryAt` (see [Rate-Limit Deferral](#rate-limit-deferral)). +and ends the run on the last attempt. Otherwise it rethrows: the queue worker +puts the message back to run at `retryAt` (see [Rate-Limit Deferral](#rate-limit-deferral)). Do not catch it to write a retry of your own. For pacing in your own loop, respect API rate limits: diff --git a/docs/guides/LOGGING-IN-INTEGRATIONS.md b/docs/guides/LOGGING-IN-INTEGRATIONS.md index 6b65771b5..3f37ef917 100644 --- a/docs/guides/LOGGING-IN-INTEGRATIONS.md +++ b/docs/guides/LOGGING-IN-INTEGRATIONS.md @@ -202,6 +202,16 @@ try { the last retry, the DLQ processor logs `frigg.queue.dlq.message_failed` at `ERROR`. A `HaltError` stops the retries and logs `frigg.worker.record_halted` at `ERROR`. +- A `RateLimitError` (an error with `isRateLimited` and `retryAt`) is + different. The `Worker` puts the message back to run at `retryAt`, and logs + one record: + - `frigg.worker.record_deferred` at `WARN` when it sent the message again + with a delay, or scheduled it. The new message starts with all its retries. + - One `WARN` when the message returns to SQS: + `frigg.worker.record_deferral_capped`, `record_visibility_extended`, or + `record_failed` with `deferral.skipped`. + - On the last attempt, one `ERROR`, `frigg.worker.record_lost_rate_limited`, + with the details, and no `WARN`. The message goes to the DLQ next. ## Ids that you learn later @@ -577,7 +587,8 @@ What each record carries without extra code: - The `listContacts` DEBUG records carry `entityId` and `credentialId` of the HubSpot module. - If `processBatch` throws, the `Worker` logs `frigg.worker.record_failed` with - the same ids, and the message returns to SQS. + the same ids, and the message returns to SQS. For a `RateLimitError` it logs + one of the deferral records instead (see [Errors](#errors)). ## Common mistakes diff --git a/packages/core/CLAUDE.md b/packages/core/CLAUDE.md index 09fddbb22..ea9f424b0 100644 --- a/packages/core/CLAUDE.md +++ b/packages/core/CLAUDE.md @@ -483,21 +483,22 @@ handlers (`{ req, res, next }`) and `this.on` events do not. `isLastAttempt` is `false` when either count is unknown: a local or non-SQS invocation, or a queue whose redrive policy the stack does not own -(`ownership.queue: 'external'`). The value is information only: core still -rethrows retryable errors and discards halt errors (4xx except 408, 429 and -errors with `isRateLimited`). +(`ownership.queue: 'external'`). The value does not change what core does: a +retryable error returns the message to SQS, a rate-limited error puts it back +to run at `retryAt` (see **Rate-limit deferral** below), and a halt error (4xx +except 408, 429 and errors with `isRateLimited`) discards it. -Use it to end a run or count lost work on the final try: when a retryable -error (429, 5xx, network) is about to be rethrown and `isLastAttempt` is -`true`, the message goes to the DLQ next, so mark the run failed or count the -message's records as failed before rethrowing. +Use it to end a run or count lost work on the final try. Check it first: when +it is `true`, mark the run failed or count the message's records as failed, and +return. Otherwise rethrow. Do not end the run and then rethrow: core can still +send a `RateLimitError` again, so the message would run again on an ended run. ```javascript async processBatch({ data, delivery }) { try { await this.syncPage(data); } catch (error) { - if (delivery?.isLastAttempt) await this.failRun(data.processId, error); + if (delivery?.isLastAttempt) return this.failRun(data.processId, error); throw error; } } @@ -511,23 +512,27 @@ it as `FRIGG_QUEUE_MAX_RECEIVE_COUNT` on the queue worker function. **Rate-limit deferral** (ADR-049): when a queue handler throws an error with `isRateLimited` and a `retryAt` (a `RateLimitError` from the Requester, or one a module throws), `Worker.run` does not give the message back to SQS for a 30 -minute redelivery that spends one of its three receives. It puts the message -back so it runs at `retryAt`: +minute redelivery. It 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, and `SCHEDULER_ROLE_ARN` is 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 message's visibility timeout to `retryAt` (12 h at most). This uses one receive | reported failed | +| Over 900 s, no scheduler | Extends the message's visibility timeout to `retryAt` (12 h less 20 minutes at most). This uses one receive | reported failed | - The new message starts at receive count 1. A deferred body carries `_frigg: { deferrals, firstDeferredAt }`; `_frigg` is reserved and the handler never sees it. -- Two caps bound it: `FRIGG_QUEUE_MAX_DEFERRALS` (default 10) and - `FRIGG_QUEUE_MAX_DEFERRED_MS` (default 24 h, from the first deferral to the - retry time). Past a cap, with no `retryAt`, with no event source ARN or on a - FIFO queue, the record fails like any retryable error and `delivery` - applies. +- The visibility tier sets a timeout of at most 12 h less 20 minutes + (42,000 s). SQS counts its 12 h maximum from the receive, and an extension + does not reset it. The margin covers the longest Lambda run (900 s) plus the + longest batching window (300 s). +- 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 defaults cover one full day of hourly waits, for a module + that probes a daily limit every hour. Past a cap, with no `retryAt`, with no + event source ARN or on a FIFO queue, the record fails like any retryable + error and `delivery` applies. - The mock scheduler is never used for a deferral: a schedule kept in memory would be lost with the acknowledged message. - With a `processId` in the message, the worker writes @@ -557,9 +562,15 @@ async processBatch({ data, delivery }) { } ``` -Records: `frigg.worker.record_deferred` (`mechanism` `delay` or `schedule`), -`record_visibility_extended`, `record_deferral_capped`, -`record_lost_rate_limited` and `rate_limit_state_failed`. A queue in a stack +Records: a rate-limited SQS record gets one log record, by its outcome. + +| Outcome | Log record | +|---|---| +| Acknowledged | WARN `frigg.worker.record_deferred` (`mechanism` `delay` or `schedule`) | +| Not acknowledged, not the last attempt | One WARN: `record_deferral_capped`, `record_visibility_extended`, or `record_failed` with `deferral.skipped` | +| Not acknowledged, last attempt | One ERROR, `record_lost_rate_limited`, with the details, and no WARN | + +A failing run state hook adds `rate_limit_state_failed`. A queue in a stack that does not own it (`ownership.queue: 'external'`) needs its own `sqs:ChangeMessageVisibility` and scheduler access. @@ -814,6 +825,8 @@ Use test doubles from `@friggframework/test` package for consistent mocking. - `SECRET_ARN` - AWS Secrets Manager ARN for auto-injection - `FRIGG_LOG_LEVEL` - Minimum log level (`TRACE` … `FATAL`, default `INFO`; `DEBUG` on local runs) - `FRIGG_QUEUE_MAX_RECEIVE_COUNT` - Set by devtools on integration queue workers whose queue the stack owns; feeds `delivery.maxReceiveCount` +- `FRIGG_QUEUE_MAX_DEFERRALS` - Most rate-limit deferrals of one message (default 30) +- `FRIGG_QUEUE_MAX_DEFERRED_MS` - Most time in ms from a message's first rate-limit deferral to its retry time (default 26 h) ## Version Information diff --git a/packages/core/core/CLAUDE.md b/packages/core/core/CLAUDE.md index 4b4f83292..07495dd5a 100644 --- a/packages/core/core/CLAUDE.md +++ b/packages/core/core/CLAUDE.md @@ -87,9 +87,15 @@ and a `retryAt` through `defer(record, body, error, delivery)`, which returns - `failed`: the visibility timeout was extended to `retryAt`, or a cap (`FRIGG_QUEUE_MAX_DEFERRALS`, `FRIGG_QUEUE_MAX_DEFERRED_MS`) ended the deferrals. The record is reported failed. -- `skipped`: no `retryAt`, no `eventSourceARN`, a FIFO queue, or a failed send. - The record fails as any error does, and `record_failed` carries - `deferral: { skipped }`. +- `skipped`: no `retryAt` (`no_retry_at`), no `eventSourceARN` or a FIFO queue + (`no_event_source`), a failed send (`send_failed`), or a failed visibility + change (`visibility_failed`). The record fails as any error does. + +Each outcome writes one log record. `acked` writes WARN `record_deferred`. +`failed` and `skipped` write one WARN (`record_visibility_extended`, +`record_deferral_capped`, or `record_failed` with `deferral: { skipped }`). On +the last attempt they write one ERROR, `record_lost_rate_limited`, with the +details, and no WARN. Subclasses override two no-op hooks to keep run state: `recordRateLimitWait(body, error, state)` runs after the message was put back, and `clearRateLimitWait(body)` From 5575ec4090ae29309effd5ad2fa8faf5ff47f26e Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 18:54:34 -0300 Subject: [PATCH 22/30] fix(core): keep the module name on a rate-limited token refresh error 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 --- packages/core/modules/requester/oauth-2.js | 1 + .../core/modules/requester/oauth-2.test.js | 20 +++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/packages/core/modules/requester/oauth-2.js b/packages/core/modules/requester/oauth-2.js index b1ae7b667..352df70e4 100644 --- a/packages/core/modules/requester/oauth-2.js +++ b/packages/core/modules/requester/oauth-2.js @@ -403,6 +403,7 @@ class OAuth2Requester extends Requester { transportError.retryAt = error.retryAt; transportError.waitMs = error.waitMs; transportError.reason = error.reason; + transportError.module = error.module; } return transportError; } diff --git a/packages/core/modules/requester/oauth-2.test.js b/packages/core/modules/requester/oauth-2.test.js index 41b9cf3aa..799cc7216 100644 --- a/packages/core/modules/requester/oauth-2.test.js +++ b/packages/core/modules/requester/oauth-2.test.js @@ -208,6 +208,26 @@ describe('OAuth2Requester', () => { ); }); + it('keeps the module name on the failure of a throttled token endpoint', async () => { + const requester = new OAuth2Requester({ + grant_type: 'authorization_code', + refresh_token: 'test-refresh-token', + credentialReloadBackoffMs: [], + }); + requester.refreshAccessToken = jest.fn().mockRejectedValue( + new RateLimitError({ + resource: 'https://auth.example.com/token', + response: { status: 429, bodyUsed: true }, + waitMs: 3_600_000, + module: 'crm', + }) + ); + + const error = await requester.refreshAuth().catch((e) => e); + + expect(error).toMatchObject({ isRateLimited: true, module: 'crm' }); + }); + it('does not flag an ordinary transport failure as rate limited', async () => { const requester = new OAuth2Requester({ grant_type: 'authorization_code', From 4b25a44784355d68c5a1a0254f559f36eb3c6589 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 19:06:36 -0300 Subject: [PATCH 23/30] test(devtools): pin the template of an app that declares no rate limit 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 --- .../integration/integration-builder.test.js | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/packages/devtools/infrastructure/domains/integration/integration-builder.test.js b/packages/devtools/infrastructure/domains/integration/integration-builder.test.js index cc0f1c71b..57099a539 100644 --- a/packages/devtools/infrastructure/domains/integration/integration-builder.test.js +++ b/packages/devtools/infrastructure/domains/integration/integration-builder.test.js @@ -10,6 +10,8 @@ const { } = require('@friggframework/core/queues/queue-delivery'); const { IntegrationBuilder } = require('./integration-builder'); const { ValidationResult } = require('../shared/base-builder'); +const { BuilderOrchestrator } = require('../shared/builder-orchestrator'); +const { SchedulerBuilder } = require('../scheduler/scheduler-builder'); describe('IntegrationBuilder', () => { let integrationBuilder; @@ -259,6 +261,37 @@ describe('IntegrationBuilder', () => { ).toBeUndefined(); }); + it('adds no resource, function or env var when no module declares rateLimit', async () => { + class Api {} + const { merged } = await new BuilderOrchestrator([ + new IntegrationBuilder(), + new SchedulerBuilder(), + ]).buildAll({ + integrations: [ + { + Definition: { + name: 'test', + modules: { crm: { definition: { API: Api } } }, + }, + }, + ], + }); + const before = await integrationBuilder.build( + { integrations: [{ Definition: { name: 'test' } }] }, + {} + ); + + expect(Object.keys(merged.resources)).toEqual( + Object.keys(before.resources) + ); + expect(Object.keys(merged.functions)).toEqual( + Object.keys(before.functions) + ); + expect(Object.keys(merged.environment)).toEqual( + Object.keys(before.environment) + ); + }); + it('should create queue worker function', async () => { const appDefinition = { integrations: [ From 6a81b603e102e18dd02eaca5cb89bf04f43b18a0 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 19:08:22 -0300 Subject: [PATCH 24/30] chore(core): declare aws-sdk-client-mock as a dev dependency 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 --- package-lock.json | 1 + packages/core/package.json | 1 + 2 files changed, 2 insertions(+) diff --git a/package-lock.json b/package-lock.json index 10af5cf83..66fb19d4f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -34064,6 +34064,7 @@ "@prisma/client": "^6.19.3", "@types/lodash": "4.17.15", "@typescript-eslint/eslint-plugin": "^8.0.0", + "aws-sdk-client-mock": "^4.1.0", "chai": "^4.3.6", "eslint": "^8.22.0", "eslint-plugin-import": "^2.29.1", diff --git a/packages/core/package.json b/packages/core/package.json index 27513db7e..bd599c6a0 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -54,6 +54,7 @@ "@prisma/client": "^6.19.3", "@types/lodash": "4.17.15", "@typescript-eslint/eslint-plugin": "^8.0.0", + "aws-sdk-client-mock": "^4.1.0", "chai": "^4.3.6", "eslint": "^8.22.0", "eslint-plugin-import": "^2.29.1", From 07aea3197703a6bc5116bdb6c52308c0ac9b50db Mon Sep 17 00:00:00 2001 From: d-klotz Date: Wed, 30 Sep 2026 11:21:39 -0300 Subject: [PATCH 25/30] fix(core): point the scheduler client at the local endpoint in a local 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 --- .../eventbridge-scheduler-adapter.js | 2 + .../eventbridge-scheduler-adapter.test.js | 46 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 packages/core/infrastructure/scheduler/eventbridge-scheduler-adapter.test.js diff --git a/packages/core/infrastructure/scheduler/eventbridge-scheduler-adapter.js b/packages/core/infrastructure/scheduler/eventbridge-scheduler-adapter.js index c06c46375..d7ac3b1ea 100644 --- a/packages/core/infrastructure/scheduler/eventbridge-scheduler-adapter.js +++ b/packages/core/infrastructure/scheduler/eventbridge-scheduler-adapter.js @@ -20,12 +20,14 @@ const { } = require('@aws-sdk/client-scheduler'); const { SchedulerServiceInterface } = require('./scheduler-service-interface'); +const { awsConfigOptions } = require('../../queues/queuer-util'); class EventBridgeSchedulerAdapter extends SchedulerServiceInterface { constructor({ region } = {}) { super(); this.client = new SchedulerClient({ region: region || process.env.AWS_REGION || 'us-east-1', + ...awsConfigOptions(), }); this.scheduleGroupName = process.env.SCHEDULE_GROUP_NAME || 'frigg-integration-schedules'; diff --git a/packages/core/infrastructure/scheduler/eventbridge-scheduler-adapter.test.js b/packages/core/infrastructure/scheduler/eventbridge-scheduler-adapter.test.js new file mode 100644 index 000000000..d3b2f85a6 --- /dev/null +++ b/packages/core/infrastructure/scheduler/eventbridge-scheduler-adapter.test.js @@ -0,0 +1,46 @@ +const { + EventBridgeSchedulerAdapter, +} = require('./eventbridge-scheduler-adapter'); + +const ENV_KEYS = ['IS_OFFLINE', 'AWS_ENDPOINT', 'AWS_REGION']; + +describe('EventBridgeSchedulerAdapter', () => { + let savedEnv; + + beforeEach(() => { + savedEnv = {}; + for (const key of ENV_KEYS) { + savedEnv[key] = process.env[key]; + delete process.env[key]; + } + }); + + afterEach(() => { + for (const key of ENV_KEYS) { + if (savedEnv[key] === undefined) delete process.env[key]; + else process.env[key] = savedEnv[key]; + } + }); + + it('uses the offline credentials and the local endpoint, as the queue clients do', async () => { + process.env.IS_OFFLINE = 'true'; + process.env.AWS_ENDPOINT = 'http://localhost:4566'; + + const { client } = new EventBridgeSchedulerAdapter(); + + const endpoint = await client.config.endpoint(); + expect(endpoint).toMatchObject({ hostname: 'localhost', port: 4566 }); + await expect(client.config.credentials()).resolves.toMatchObject({ + accessKeyId: 'test-aws-key', + }); + }); + + it('keeps the AWS endpoint and the region when no local endpoint is set', async () => { + process.env.AWS_REGION = 'eu-west-1'; + + const { client } = new EventBridgeSchedulerAdapter(); + + expect(client.config.isCustomEndpoint).toBe(false); + await expect(client.config.region()).resolves.toBe('eu-west-1'); + }); +}); From e372ef6b74e5bb76673a99181fed6d54d097dc99 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Wed, 30 Sep 2026 18:27:01 -0300 Subject: [PATCH 26/30] fix(core): let the message that set a rate-limit wait clear it at once 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 --- packages/core/CLAUDE.md | 21 +++++---- packages/core/core/CLAUDE.md | 6 ++- packages/core/core/Worker.deferral.test.js | 47 ++++++++++++++++--- packages/core/core/Worker.js | 16 +++++-- packages/core/handlers/backend-utils.js | 4 +- .../integration-defined-workers.test.js | 35 ++++++++++++-- .../use-cases/clear-rate-limit-wait.js | 11 +++-- .../use-cases/clear-rate-limit-wait.test.js | 26 ++++++++++ .../use-cases/record-rate-limit-wait.js | 1 + .../use-cases/record-rate-limit-wait.test.js | 13 +++++ packages/core/queues/queue-deferral.js | 9 +++- packages/core/types/core/index.d.ts | 6 ++- 12 files changed, 160 insertions(+), 35 deletions(-) diff --git a/packages/core/CLAUDE.md b/packages/core/CLAUDE.md index ea9f424b0..de17afb62 100644 --- a/packages/core/CLAUDE.md +++ b/packages/core/CLAUDE.md @@ -521,8 +521,9 @@ minute redelivery. It puts the message back so it runs at `retryAt`: | Over 900 s, no scheduler | Extends the message's visibility timeout to `retryAt` (12 h less 20 minutes at most). This uses one receive | reported failed | - The new message starts at receive count 1. A deferred body carries - `_frigg: { deferrals, firstDeferredAt }`; `_frigg` is reserved and the handler - never sees it. + `_frigg: { deferrals, firstDeferredAt, deferralId }`; `_frigg` is reserved and + the handler never sees it. `deferralId` is the SQS message id of the first + message that was put back, and every later deferral of that work keeps it. - The visibility tier sets a timeout of at most 12 h less 20 minutes (42,000 s). SQS counts its 12 h maximum from the receive, and an extension does not reset it. The margin covers the longest Lambda run (900 s) plus the @@ -537,12 +538,16 @@ minute redelivery. It puts the message back so it runs at `retryAt`: would be lost with the acknowledged message. - With a `processId` in the message, the worker writes `Process.context.rateLimit = { status: 'WAITING' | 'EXHAUSTED', mechanism, - retryAt, reason, module, deferrals, updatedAt }`. `EXHAUSTED` means 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, a `WAITING` value never hides an `EXHAUSTED` one, - and after a deferred or redelivered message ran the worker sets it to `null` - only when it is `WAITING` and its `retryAt` is past or within 5 s. + retryAt, reason, module, deferrals, deferralId, updatedAt }`. `EXHAUSTED` means + 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, the worker + sets a `WAITING` value to `null` at once when that message set it (same + `deferralId`, or the message's own id for a visibility change), 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 the wait of + another message. - Delivery stays at least once. The new message is sent before the old one is acknowledged, so a crash between the two can run the work twice. - Handlers check `delivery.isLastAttempt` first, then rethrow. The delay and diff --git a/packages/core/core/CLAUDE.md b/packages/core/core/CLAUDE.md index 07495dd5a..4dd61e5e8 100644 --- a/packages/core/core/CLAUDE.md +++ b/packages/core/core/CLAUDE.md @@ -98,8 +98,10 @@ the last attempt they write one ERROR, `record_lost_rate_limited`, with the details, and no WARN. Subclasses override two no-op hooks to keep run state: `recordRateLimitWait(body, -error, state)` runs after the message was put back, and `clearRateLimitWait(body)` -runs after a deferred or redelivered message succeeded. A failing hook logs +error, state)` runs after the message was put back, and `clearRateLimitWait(body, +{ deferralId })` runs after a deferred or redelivered message succeeded. +`state.deferralId` and `deferralId` name the message that was first put back +(`_frigg.deferralId`, or the message's own id). A failing hook logs `frigg.worker.rate_limit_state_failed` and never changes the outcome. The queue worker of an integration (`createQueueWorker`) writes `Process.context.rateLimit` in them. See `packages/core/CLAUDE.md` section 7. diff --git a/packages/core/core/Worker.deferral.test.js b/packages/core/core/Worker.deferral.test.js index 839004c19..057d6feb9 100644 --- a/packages/core/core/Worker.deferral.test.js +++ b/packages/core/core/Worker.deferral.test.js @@ -65,9 +65,10 @@ function sqsRecord(overrides = {}) { function deferredBody( deferrals, - firstDeferredAt = new Date(NOW).toISOString() + firstDeferredAt = new Date(NOW).toISOString(), + deferralId = 'msg-1' ) { - return { ...BODY, _frigg: { deferrals, firstDeferredAt } }; + return { ...BODY, _frigg: { deferrals, firstDeferredAt, deferralId } }; } function deferredRecord(deferrals, firstDeferredAt) { @@ -179,6 +180,27 @@ describe('Worker rate-limit deferral (ADR-049)', () => { expect(sent()[0].DelaySeconds).toBe(0); }); + it('marks the new message with the id of the message it replaces', async () => { + worker._run.mockRejectedValue(rateLimited(60 * SECOND)); + + await run(); + + expect(sentBody()._frigg.deferralId).toBe('msg-1'); + }); + + it('keeps the id of the first message when the message is put back again', async () => { + worker._run.mockRejectedValue(rateLimited(60 * SECOND)); + + await run( + sqsRecord({ + messageId: 'msg-2', + body: JSON.stringify(deferredBody(1, undefined, 'msg-1')), + }) + ); + + expect(sentBody()._frigg.deferralId).toBe('msg-1'); + }); + it('counts up and keeps the first deferral time on a message that was deferred before', async () => { const firstDeferredAt = new Date(NOW - 10 * MINUTE).toISOString(); worker._run.mockRejectedValue(rateLimited(60 * SECOND)); @@ -188,6 +210,7 @@ describe('Worker rate-limit deferral (ADR-049)', () => { expect(sentBody()._frigg).toEqual({ deferrals: 3, firstDeferredAt, + deferralId: 'msg-1', }); }); @@ -660,6 +683,7 @@ describe('Worker rate-limit deferral (ADR-049)', () => { status: 'WAITING', mechanism: 'delay', deferrals: 1, + deferralId: 'msg-1', retryAt: new Date(NOW + 60_000), }); }); @@ -694,27 +718,36 @@ describe('Worker rate-limit deferral (ADR-049)', () => { expect(worker.recordRateLimitWait.mock.calls[0][2]).toMatchObject({ status: 'EXHAUSTED', mechanism: 'visibility', + deferralId: 'msg-1', }); }); - it('clears the wait after a deferred message succeeds', async () => { + it('clears the wait after a deferred message succeeds, naming the message that set it', async () => { worker._run.mockResolvedValue(undefined); - await run(deferredRecord(1)); + await run( + sqsRecord({ + messageId: 'msg-2', + body: JSON.stringify(deferredBody(1)), + }) + ); expect(worker.clearRateLimitWait).toHaveBeenCalledWith( - deferredBody(1) + deferredBody(1), + { deferralId: 'msg-1' } ); }); - it('clears the wait after a redelivered message succeeds', async () => { + it('clears the wait after a redelivered message succeeds, naming that message', async () => { worker._run.mockResolvedValue(undefined); await run( sqsRecord({ attributes: { ApproximateReceiveCount: '2' } }) ); - expect(worker.clearRateLimitWait).toHaveBeenCalledWith(BODY); + expect(worker.clearRateLimitWait).toHaveBeenCalledWith(BODY, { + deferralId: 'msg-1', + }); }); it('does not clear anything after a first delivery', async () => { diff --git a/packages/core/core/Worker.js b/packages/core/core/Worker.js index d0ca1c0c6..c392e2d26 100644 --- a/packages/core/core/Worker.js +++ b/packages/core/core/Worker.js @@ -63,7 +63,7 @@ class Worker { runParams = JSON.parse(record.body); this._validateParams(runParams); await this._run(runParams, context, delivery); - await this._clearDeferredState(runParams, delivery); + await this._clearDeferredState(record, runParams, delivery); log.debug('Record succeeded', { eventName: 'frigg.worker.record_succeeded' }); } catch (error) { let deferralResult; @@ -146,7 +146,7 @@ class Worker { } const now = Date.now(); - const counters = nextDeferral(body, now); + const counters = nextDeferral(body, now, record.messageId); const deferral = { record, queueName, @@ -312,15 +312,21 @@ class Worker { status, mechanism, deferrals: counters.deferrals, + deferralId: counters.deferralId, retryAt, }) ); } - async _clearDeferredState(body, delivery) { + async _clearDeferredState(record, body, delivery) { const redelivered = delivery.receiveCount > 1; - if (readDeferral(body).deferrals === 0 && !redelivered) return; - await this._runStateHook('clear', () => this.clearRateLimitWait(body)); + const { deferrals, deferralId } = readDeferral(body); + if (deferrals === 0 && !redelivered) return; + await this._runStateHook('clear', () => + this.clearRateLimitWait(body, { + deferralId: deferralId ?? record.messageId, + }) + ); } async _runStateHook(operation, hook) { diff --git a/packages/core/handlers/backend-utils.js b/packages/core/handlers/backend-utils.js index e7880bb4c..3f678a6d7 100644 --- a/packages/core/handlers/backend-utils.js +++ b/packages/core/handlers/backend-utils.js @@ -184,13 +184,13 @@ const createQueueWorker = (integrationClass) => { }); } - async clearRateLimitWait(body) { + async clearRateLimitWait(body, { deferralId } = {}) { const processId = body?.data?.processId; if (!processId) return; const clearRateLimitWait = new ClearRateLimitWait({ processRepository: createProcessRepository(), }); - await clearRateLimitWait.execute(processId); + await clearRateLimitWait.execute(processId, { deferralId }); } async _run(params, context, delivery) { diff --git a/packages/core/handlers/workers/integration-defined-workers.test.js b/packages/core/handlers/workers/integration-defined-workers.test.js index ff954033e..e915dfd28 100644 --- a/packages/core/handlers/workers/integration-defined-workers.test.js +++ b/packages/core/handlers/workers/integration-defined-workers.test.js @@ -414,11 +414,10 @@ describe('Webhook Queue Worker', () => { it('writes context.rateLimit on the process of a deferred message', async () => { const { worker, applyProcessUpdate } = buildWorker(); - await worker.recordRateLimitWait( - message, - rateLimitError, - next(at(20 * MINUTE)) - ); + await worker.recordRateLimitWait(message, rateLimitError, { + ...next(at(20 * MINUTE)), + deferralId: 'msg-1', + }); expect(applyProcessUpdate).toHaveBeenCalledWith(7, { set: { @@ -429,6 +428,7 @@ describe('Webhook Queue Worker', () => { reason: 'burst', module: 'hubspot', deferrals: 1, + deferralId: 'msg-1', updatedAt: expect.any(String), }, }, @@ -480,6 +480,31 @@ describe('Webhook Queue Worker', () => { }); }); + it('clears at once the wait that the finished message set, before its retry time', async () => { + const { worker, applyProcessUpdate } = buildWorker({ + ...stored(at(60 * MINUTE)), + deferralId: 'msg-1', + }); + + const result = await worker.run({ + Records: [ + { + ...sqsRecord({ + deferrals: 1, + firstDeferredAt: at(-MINUTE).toISOString(), + deferralId: 'msg-1', + }), + messageId: 'msg-2', + }, + ], + }); + + expect(result.batchItemFailures).toEqual([]); + expect(applyProcessUpdate).toHaveBeenCalledWith(7, { + set: { 'context.rateLimit': null }, + }); + }); + it('does not clear the wait after a first delivery runs', async () => { const { worker, applyProcessUpdate } = buildWorker( stored(at(-MINUTE)) diff --git a/packages/core/integrations/use-cases/clear-rate-limit-wait.js b/packages/core/integrations/use-cases/clear-rate-limit-wait.js index bb1f41f5a..b243c574b 100644 --- a/packages/core/integrations/use-cases/clear-rate-limit-wait.js +++ b/packages/core/integrations/use-cases/clear-rate-limit-wait.js @@ -1,5 +1,11 @@ const CLEAR_RATE_LIMIT_TOLERANCE_MS = 5_000; +const isOwnWait = (current, deferralId) => + Boolean(deferralId) && current.deferralId === deferralId; + +const isOver = (current) => + Date.parse(current.retryAt) - Date.now() <= CLEAR_RATE_LIMIT_TOLERANCE_MS; + class ClearRateLimitWait { constructor({ processRepository }) { if (!processRepository) { @@ -8,12 +14,11 @@ class ClearRateLimitWait { this.processRepository = processRepository; } - async execute(processId) { + async execute(processId, { deferralId } = {}) { const process = await this.processRepository.findById(processId); const current = process?.context?.rateLimit; if (current?.status !== 'WAITING') return; - const remainingMs = Date.parse(current.retryAt) - Date.now(); - if (remainingMs > CLEAR_RATE_LIMIT_TOLERANCE_MS) return; + if (!isOwnWait(current, deferralId) && !isOver(current)) return; await this.processRepository.applyProcessUpdate(processId, { set: { 'context.rateLimit': null }, }); diff --git a/packages/core/integrations/use-cases/clear-rate-limit-wait.test.js b/packages/core/integrations/use-cases/clear-rate-limit-wait.test.js index 7fc33298f..2d9b8ea95 100644 --- a/packages/core/integrations/use-cases/clear-rate-limit-wait.test.js +++ b/packages/core/integrations/use-cases/clear-rate-limit-wait.test.js @@ -85,6 +85,32 @@ describe('ClearRateLimitWait', () => { ).not.toHaveBeenCalled(); }); + it('clears at once the wait that the finished message set', async () => { + storeOnProcess({ ...stored(at(60 * MINUTE)), deferralId: 'msg-1' }); + + await clearRateLimitWait.execute(processId, { + deferralId: 'msg-1', + }); + + expect( + mockProcessRepository.applyProcessUpdate + ).toHaveBeenCalledWith(processId, { + set: { 'context.rateLimit': null }, + }); + }); + + it('keeps a wait that another message set until its retry time', async () => { + storeOnProcess({ ...stored(at(60 * MINUTE)), deferralId: 'msg-1' }); + + await clearRateLimitWait.execute(processId, { + deferralId: 'msg-2', + }); + + expect( + mockProcessRepository.applyProcessUpdate + ).not.toHaveBeenCalled(); + }); + it('leaves a cap in place', async () => { storeOnProcess(stored(at(-MINUTE), 'EXHAUSTED')); diff --git a/packages/core/integrations/use-cases/record-rate-limit-wait.js b/packages/core/integrations/use-cases/record-rate-limit-wait.js index b9bf9c787..05b83f4da 100644 --- a/packages/core/integrations/use-cases/record-rate-limit-wait.js +++ b/packages/core/integrations/use-cases/record-rate-limit-wait.js @@ -25,6 +25,7 @@ class RecordRateLimitWait { reason: wait.reason, module: wait.module, deferrals: wait.deferrals, + deferralId: wait.deferralId, updatedAt: new Date().toISOString(), }, }, diff --git a/packages/core/integrations/use-cases/record-rate-limit-wait.test.js b/packages/core/integrations/use-cases/record-rate-limit-wait.test.js index b84860543..43bef1cbe 100644 --- a/packages/core/integrations/use-cases/record-rate-limit-wait.test.js +++ b/packages/core/integrations/use-cases/record-rate-limit-wait.test.js @@ -77,6 +77,19 @@ describe('RecordRateLimitWait', () => { }); }); + it('keeps the id of the message that set the wait', async () => { + await recordRateLimitWait.execute(processId, { + ...wait(at(20 * MINUTE)), + deferralId: 'msg-1', + }); + + const [, update] = + mockProcessRepository.applyProcessUpdate.mock.calls[0]; + expect(update.set['context.rateLimit']).toMatchObject({ + deferralId: 'msg-1', + }); + }); + it('replaces an earlier wait', async () => { storeOnProcess(stored(at(5 * MINUTE))); diff --git a/packages/core/queues/queue-deferral.js b/packages/core/queues/queue-deferral.js index a3f5183ce..a5d0c35af 100644 --- a/packages/core/queues/queue-deferral.js +++ b/packages/core/queues/queue-deferral.js @@ -30,14 +30,19 @@ const readDeferral = (body) => { !Number.isNaN(Date.parse(frigg.firstDeferredAt)) ? frigg.firstDeferredAt : undefined; - return { deferrals, firstDeferredAt }; + const deferralId = + typeof frigg?.deferralId === 'string' && frigg.deferralId !== '' + ? frigg.deferralId + : undefined; + return { deferrals, firstDeferredAt, deferralId }; }; -const nextDeferral = (body, now = Date.now()) => { +const nextDeferral = (body, now = Date.now(), messageId) => { const current = readDeferral(body); return { deferrals: current.deferrals + 1, firstDeferredAt: current.firstDeferredAt ?? new Date(now).toISOString(), + deferralId: current.deferralId ?? messageId, }; }; diff --git a/packages/core/types/core/index.d.ts b/packages/core/types/core/index.d.ts index d161e8e4b..efe9284ac 100644 --- a/packages/core/types/core/index.d.ts +++ b/packages/core/types/core/index.d.ts @@ -54,13 +54,17 @@ declare module "@friggframework/core" { state: RateLimitWaitState ): Promise; - clearRateLimitWait(body: object): Promise; + clearRateLimitWait( + body: object, + options?: { deferralId?: string } + ): Promise; } export type RateLimitWaitState = { status: "WAITING" | "EXHAUSTED"; mechanism: "delay" | "schedule" | "visibility" | "none"; deferrals: number; + deferralId?: string; retryAt: Date; }; From 3c0bea9725aa641e60782b3f4a30151c7d1b3801 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 9 Nov 2026 13:05:31 -0300 Subject: [PATCH 27/30] test(core): pin the Worker for a RateLimitError with no retry time #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 --- packages/core/core/Worker.deferral.test.js | 27 ++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/packages/core/core/Worker.deferral.test.js b/packages/core/core/Worker.deferral.test.js index 057d6feb9..841faa94a 100644 --- a/packages/core/core/Worker.deferral.test.js +++ b/packages/core/core/Worker.deferral.test.js @@ -529,15 +529,34 @@ describe('Worker rate-limit deferral (ADR-049)', () => { }); describe('a rate-limit error that cannot be deferred', () => { - it('goes back to SQS as before when it has no retry time', async () => { - worker._run.mockRejectedValue( - Object.assign(new Error('limited'), { isRateLimited: true }) - ); + it.each([ + [ + 'an error with isRateLimited and no retryAt', + () => + Object.assign(new Error('limited'), { + isRateLimited: true, + }), + ], + [ + 'a RateLimitError for a non-429 limit with no time', + () => + new RateLimitError({ + resource: 'https://api.example.com/v1/items', + response: { status: 403, bodyUsed: true }, + hint: { reason: 'daily' }, + module: 'hubspot', + now: NOW, + }), + ], + ])('goes back to SQS as before for %s', async (_label, makeError) => { + worker._run.mockRejectedValue(makeError()); const result = await run(); expect(result).toEqual(failure()); expect(sent()).toEqual([]); + expect(schedules()).toEqual([]); + expect(visibility()).toEqual([]); expect(logged('record_failed')).toEqual([ expect.objectContaining({ deferral: { skipped: 'no_retry_at' }, From 18fb0eec33ecfe46362d17ff8660d06a929ae9ae Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 9 Nov 2026 13:05:31 -0300 Subject: [PATCH 28/30] docs(core): say a RateLimitError with no retryAt returns to SQS 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 --- docs/guides/INTEGRATION-PATTERNS.md | 3 ++- docs/guides/LOGGING-IN-INTEGRATIONS.md | 6 +++--- docs/reference/api-module-definition-and-functions.md | 3 ++- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/guides/INTEGRATION-PATTERNS.md b/docs/guides/INTEGRATION-PATTERNS.md index 2cd3c7e89..f9650b838 100644 --- a/docs/guides/INTEGRATION-PATTERNS.md +++ b/docs/guides/INTEGRATION-PATTERNS.md @@ -425,7 +425,8 @@ async processBatch({ data, delivery }) { A deferral past `FRIGG_QUEUE_MAX_DEFERRALS` (default 30) or `FRIGG_QUEUE_MAX_DEFERRED_MS` (default 26 hours) fails the record like any other -retryable error, and `delivery` applies. The defaults cover one full day of +retryable error, and `delivery` applies. So does a `RateLimitError` with no +`retryAt`: a non-429 limit that `classify` names without a time. The defaults cover one full day of hourly waits, for a module that probes a daily limit every hour. Do not catch a `RateLimitError` to write a retry of your own. diff --git a/docs/guides/LOGGING-IN-INTEGRATIONS.md b/docs/guides/LOGGING-IN-INTEGRATIONS.md index 3f37ef917..e9345dcc6 100644 --- a/docs/guides/LOGGING-IN-INTEGRATIONS.md +++ b/docs/guides/LOGGING-IN-INTEGRATIONS.md @@ -202,9 +202,9 @@ try { the last retry, the DLQ processor logs `frigg.queue.dlq.message_failed` at `ERROR`. A `HaltError` stops the retries and logs `frigg.worker.record_halted` at `ERROR`. -- A `RateLimitError` (an error with `isRateLimited` and `retryAt`) is - different. The `Worker` puts the message back to run at `retryAt`, and logs - one record: +- A `RateLimitError` (an error with `isRateLimited`) is different. When it + has a `retryAt`, the `Worker` puts the message back to run at that time. It + logs one record: - `frigg.worker.record_deferred` at `WARN` when it sent the message again with a delay, or scheduled it. The new message starts with all its retries. - One `WARN` when the message returns to SQS: diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index 79b3d37b1..b4ea9fb57 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -331,7 +331,8 @@ response that `classify` recognises, and of no other response: a 503 with `Retry-After` keeps the 5xx ladder unless `classify` recognises it. The queue worker does not halt a `RateLimitError`, even when its status is 403. It puts the message back to run at `retryAt` (see the Rate-Limit Deferral section of -the [Integration Patterns Guide](/docs/guides/INTEGRATION-PATTERNS.md)). +the [Integration Patterns Guide](/docs/guides/INTEGRATION-PATTERNS.md)). With +no `retryAt`, the message returns to SQS like any retryable error. ### A client that is not the Requester From 798cee9c975be131b5446e7d35fbfcceb5329a04 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 9 Nov 2026 15:54:28 -0300 Subject: [PATCH 29/30] fix(core): write the rate-limit wait before the delayed send 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 --- packages/core/core/CLAUDE.md | 9 ++-- packages/core/core/Worker.deferral.test.js | 52 +++++++++++++++++++++- packages/core/core/Worker.js | 14 ++++-- 3 files changed, 66 insertions(+), 9 deletions(-) diff --git a/packages/core/core/CLAUDE.md b/packages/core/core/CLAUDE.md index 7ed17a380..3ef4fbab3 100644 --- a/packages/core/core/CLAUDE.md +++ b/packages/core/core/CLAUDE.md @@ -97,9 +97,12 @@ Each outcome writes one log record. `acked` writes WARN `record_deferred`. the last attempt they write one ERROR, `record_lost_rate_limited`, with the details, and no WARN. -Subclasses override two no-op hooks to keep run state: `recordRateLimitWait(body, -error, state)` runs after the message was put back, and `clearRateLimitWait(body, -{ deferralId })` runs after a deferred or redelivered message succeeded. +Subclasses override two no-op hooks to keep run state. +`recordRateLimitWait(body, error, state)` runs before a delayed message is sent, +because with a short delay the new message can run first; a failed send clears +the wait again. For a schedule or a visibility change it runs after the change. +`clearRateLimitWait(body, { deferralId })` runs after a deferred or redelivered +message succeeded. `state.deferralId` and `deferralId` name the message that was first put back (`_frigg.deferralId`, or the message's own id). A failing hook logs `frigg.worker.rate_limit_state_failed` and never changes the outcome. The queue diff --git a/packages/core/core/Worker.deferral.test.js b/packages/core/core/Worker.deferral.test.js index 841faa94a..b755b4ea9 100644 --- a/packages/core/core/Worker.deferral.test.js +++ b/packages/core/core/Worker.deferral.test.js @@ -688,7 +688,42 @@ describe('Worker rate-limit deferral (ADR-049)', () => { worker.clearRateLimitWait = jest.fn().mockResolvedValue(undefined); }); - it('records the wait after the message was put back', async () => { + const keepWaitInMemory = () => { + const store = { wait: null }; + worker.recordRateLimitWait.mockImplementation( + async (body, error, state) => { + store.wait = state; + } + ); + worker.clearRateLimitWait.mockImplementation( + async (body, { deferralId }) => { + if (store.wait?.deferralId === deferralId) { + store.wait = null; + } + } + ); + return store; + }; + + it('records the wait before the delayed message is sent, so a new message that runs at once clears it', async () => { + const store = keepWaitInMemory(); + sqsMock.on(SendMessageCommand).callsFake(async (input) => { + const body = JSON.parse(input.MessageBody); + await worker.clearRateLimitWait(body, { + deferralId: body._frigg.deferralId, + }); + return { MessageId: 'new-message' }; + }); + worker._run.mockRejectedValue(rateLimited(0)); + + const result = await run(); + + expect(result).toEqual({ batchItemFailures: [] }); + expect(sent()).toHaveLength(1); + expect(store.wait).toBeNull(); + }); + + it('records the wait of a delayed message', async () => { worker._run.mockRejectedValue(rateLimited(60 * SECOND)); await run(); @@ -707,10 +742,23 @@ describe('Worker rate-limit deferral (ADR-049)', () => { }); }); - it('records nothing when the message could not be put back', async () => { + it('leaves no wait when the delayed message could not be sent', async () => { + const store = keepWaitInMemory(); sqsMock.on(SendMessageCommand).rejects(new Error('SQS is down')); worker._run.mockRejectedValue(rateLimited(60 * SECOND)); + const result = await run(); + + expect(result).toEqual(failure()); + expect(store.wait).toBeNull(); + }); + + it('records nothing when the visibility change fails', async () => { + sqsMock + .on(ChangeMessageVisibilityCommand) + .rejects(new Error('gone')); + worker._run.mockRejectedValue(rateLimited(20 * MINUTE)); + await run(); expect(worker.recordRateLimitWait).not.toHaveBeenCalled(); diff --git a/packages/core/core/Worker.js b/packages/core/core/Worker.js index c392e2d26..11f962a7e 100644 --- a/packages/core/core/Worker.js +++ b/packages/core/core/Worker.js @@ -184,6 +184,9 @@ class Worker { async _deferWithDelay(deferral) { const delaySeconds = Math.ceil(deferral.waitMs / 1000); + // Write the wait before the send: with a short delay the new message + // can run, and clear the wait, before a later write lands. + await this._recordWait(deferral, 'WAITING', 'delay'); try { const queueUrl = await this._queueUrl(deferral.queueName); await this.send( @@ -191,6 +194,7 @@ class Worker { delaySeconds ); } catch (cause) { + await this._clearWait(deferral.body, deferral.counters.deferralId); return { outcome: 'skipped', skipped: 'send_failed', cause }; } return this._acknowledgeDeferral(deferral, 'delay', { delaySeconds }); @@ -212,6 +216,7 @@ class Worker { if (cause?.name !== 'ConflictException') return { scheduleError: cause }; } + await this._recordWait(deferral, 'WAITING', 'schedule'); return this._acknowledgeDeferral(deferral, 'schedule', { scheduleName, }); @@ -254,7 +259,6 @@ class Worker { } async _acknowledgeDeferral(deferral, mechanism, fields) { - await this._recordWait(deferral, 'WAITING', mechanism); log.warn('Record deferred', { eventName: 'frigg.worker.record_deferred', mechanism, @@ -322,10 +326,12 @@ class Worker { const redelivered = delivery.receiveCount > 1; const { deferrals, deferralId } = readDeferral(body); if (deferrals === 0 && !redelivered) return; + await this._clearWait(body, deferralId ?? record.messageId); + } + + async _clearWait(body, deferralId) { await this._runStateHook('clear', () => - this.clearRateLimitWait(body, { - deferralId: deferralId ?? record.messageId, - }) + this.clearRateLimitWait(body, { deferralId }) ); } From f16d20b91d76dd1c98f7e0dc91eb4026bafba2a3 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 9 Nov 2026 18:45:42 -0300 Subject: [PATCH 30/30] fix(devtools): let the scheduler send to an external integration queue A rate-limit policy turns the scheduler on. Its execution role used Fn::GetAtt on 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 --- .../domains/scheduler/scheduler-builder.js | 13 +++++- .../scheduler/scheduler-builder.test.js | 44 +++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.js b/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.js index 0e247c2f4..63778e9b3 100644 --- a/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.js +++ b/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.js @@ -13,12 +13,19 @@ */ const { InfrastructureBuilder, ValidationResult } = require('../shared/base-builder'); +const { ResourceOwnership } = require('../shared/types/resource-ownership'); const { isScopedEnvironmentActive, getIntegrationFunctionNames, getAdminFunctionNames, } = require('../shared/function-environments'); +const queueArnFromUrl = (queueUrl) => { + const { hostname, pathname } = new URL(queueUrl); + const [accountId, queueName] = pathname.split('/').filter(Boolean); + return `arn:aws:sqs:${hostname.split('.')[1]}:${accountId}:${queueName}`; +}; + const declaresRateLimit = (integration) => Object.values(integration?.Definition?.modules ?? {}).some((module) => Boolean(module?.definition?.API?.rateLimit) @@ -113,7 +120,11 @@ class SchedulerBuilder extends InfrastructureBuilder { if (Array.isArray(appDefinition.integrations)) { appDefinition.integrations.forEach((integration) => { const integrationName = integration?.Definition?.name; - if (integrationName) { + if ( + integration.ownership?.queue === ResourceOwnership.EXTERNAL + ) { + queueArns.push(queueArnFromUrl(integration.queue.url)); + } else if (integrationName) { const capitalizedName = integrationName.charAt(0).toUpperCase() + integrationName.slice(1); diff --git a/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.test.js b/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.test.js index 588c91257..8adf1b0de 100644 --- a/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.test.js +++ b/packages/devtools/infrastructure/domains/scheduler/scheduler-builder.test.js @@ -170,4 +170,48 @@ describe('SchedulerBuilder', () => { expect(result.functionEnvironments).toBeUndefined(); }); }); + + describe('build() execution role', () => { + const sendableQueues = (result) => + result.resources.SchedulerExecutionRole.Properties.Policies[0] + .PolicyDocument.Statement[0].Resource; + + it('allows sending to a queue the stack creates', async () => { + const result = await schedulerBuilder.build( + { + scheduler: { enable: true }, + integrations: [{ Definition: { name: 'hubspot' } }], + }, + {} + ); + + expect(sendableQueues(result)).toEqual([ + { 'Fn::GetAtt': ['HubspotQueue', 'Arn'] }, + ]); + }); + + it('allows sending to an external queue by its ARN, not a resource the stack lacks', async () => { + const result = await schedulerBuilder.build( + { + scheduler: { enable: true }, + integrations: [ + { Definition: { name: 'hubspot' } }, + { + Definition: { name: 'salesforce' }, + ownership: { queue: 'external' }, + queue: { + url: 'https://sqs.us-east-1.amazonaws.com/123456789012/acme-salesforce', + }, + }, + ], + }, + {} + ); + + expect(sendableQueues(result)).toEqual([ + { 'Fn::GetAtt': ['HubspotQueue', 'Arn'] }, + 'arn:aws:sqs:us-east-1:123456789012:acme-salesforce', + ]); + }); + }); });