From 02ed23ac0850167dc0563c4674ee94bd4948a8c3 Mon Sep 17 00:00:00 2001 From: Steve Buxton Date: Fri, 7 Aug 2026 15:52:21 +0100 Subject: [PATCH 01/11] Send letters to DLQ if supplier allocation fails --- .../api/module_lambda_supplier_allocator.tf | 18 +- lambdas/supplier-allocator/README.md | 14 +- .../src/errors/supplier-config-error.ts | 9 + .../__tests__/allocate-handler.test.ts | 203 +++++++++--------- .../__tests__/allocation-config.test.ts | 18 +- .../src/handler/allocate-handler.ts | 195 +++++++++-------- .../src/handler/allocation-config.ts | 7 +- .../__tests__/supplier-config.test.ts | 59 +++-- .../src/services/supplier-config.ts | 22 +- 9 files changed, 317 insertions(+), 228 deletions(-) create mode 100644 lambdas/supplier-allocator/src/errors/supplier-config-error.ts diff --git a/infrastructure/terraform/components/api/module_lambda_supplier_allocator.tf b/infrastructure/terraform/components/api/module_lambda_supplier_allocator.tf index b0354bd88..3ab822ecc 100644 --- a/infrastructure/terraform/components/api/module_lambda_supplier_allocator.tf +++ b/infrastructure/terraform/components/api/module_lambda_supplier_allocator.tf @@ -35,8 +35,9 @@ module "supplier_allocator" { log_subscription_role_arn = local.acct.log_subscription_role_arn lambda_env_vars = merge(local.common_lambda_env_vars, { - UPSERT_LETTERS_QUEUE_URL = module.sqs_letter_updates.sqs_queue_url, - IDEMPOTENCY_TABLE_NAME = aws_dynamodb_table.idempotency.name + UPSERT_LETTERS_QUEUE_URL = module.sqs_letter_updates.sqs_queue_url, + SUPPLIER_ALLOCATOR_DLQ_URL = module.sqs_supplier_allocator.sqs_dlq_url, + IDEMPOTENCY_TABLE_NAME = aws_dynamodb_table.idempotency.name }) } @@ -83,6 +84,19 @@ data "aws_iam_policy_document" "supplier_allocator_lambda" { ] } + statement { + sid = "AllowSupplierAllocatorDLQWrite" + effect = "Allow" + + actions = [ + "sqs:SendMessage" + ] + + resources = [ + module.sqs_supplier_allocator.sqs_dlq_arn + ] + } + statement { sid = "AllowConfigDynamoDBAccess" effect = "Allow" diff --git a/lambdas/supplier-allocator/README.md b/lambdas/supplier-allocator/README.md index f33e1517c..2802dd587 100644 --- a/lambdas/supplier-allocator/README.md +++ b/lambdas/supplier-allocator/README.md @@ -12,21 +12,23 @@ Consumes `LetterRequestPrepared` events (v1 and v2) from an SQS queue, chooses a 2. Each record body is parsed and validated as either `$LetterRequestPreparedEventV2` or `$LetterRequestPreparedEvent` (v1 fallback). 3. The allocator loads the relevant supplier configuration from `SUPPLIER_CONFIG_TABLE`, including the letter variant, active volume group, candidate suppliers, and compatible pack details. 4. Candidate suppliers are filtered using pack support and daily capacity, then ranked using quota data from `SUPPLIER_QUOTAS_TABLE`. -5. On success, the handler produces an allocation with `allocationStatus.status = "PENDING"`. If allocation cannot be completed, it produces a REJECTED allocation with a failure reason instead of dropping the message. -6. Each record produces a `{ letterEvent, allocationDetails }` message sent to `UPSERT_LETTERS_QUEUE_URL`. -7. After the batch completes, allocation counters are written back to `SUPPLIER_QUOTAS_TABLE`, and only genuine processing failures are returned as `batchItemFailures`. +5. On success, the handler produces an allocation with `allocationStatus.status = "PENDING"` and sends `{ letterEvent, allocationDetails }` to `UPSERT_LETTERS_QUEUE_URL`. +6. If a `SupplierConfigError` is raised, the original record is sent directly to `SUPPLIER_ALLOCATOR_DLQ_URL` and acknowledged so it is not retried. +7. Any other processing error is returned in `batchItemFailures` so SQS retries the record based on the source queue redrive policy. +8. After the batch completes, allocation counters are written back to `SUPPLIER_QUOTAS_TABLE`. ## Key Integration Points -- **SQS**: Input from EventSub, output to the upsert-letter queue (`UPSERT_LETTERS_QUEUE`). +- **SQS**: Input from EventSub, output to the upsert-letter queue (`UPSERT_LETTERS_QUEUE`), and direct publish to the allocator DLQ (`SUPPLIER_ALLOCATOR_DLQ_URL`) for `SupplierConfigError`. - **`SupplierConfigRepository`** from `@internal/datastore` (`SUPPLIER_CONFIG_TABLE`): reads letter variants, volume groups, supplier allocations, pack specifications, and supplier packs. - **`SupplierQuotasRepository`** from `@internal/datastore` (`SUPPLIER_QUOTAS_TABLE`): reads and writes daily and overall allocation counts per volume group and supplier. - **Event schemas**: `@nhsdigital/nhs-notify-event-schemas-letter-rendering` (v2) and `@nhsdigital/nhs-notify-event-schemas-letter-rendering-v1` (v1). -- **Downstream consumer**: `upsert-letter` receives `{ letterEvent, allocationDetails }` and persists either a PENDING or REJECTED letter. +- **Downstream consumer**: `upsert-letter` receives `{ letterEvent, allocationDetails }` and persists PENDING letters. ## Nuances and Peculiarities -- **Failed allocations produce REJECTED letters, not dropped messages.** If the config lookup chain fails for any reason, the handler still sends a message to the upsert queue with `allocationStatus.status = "REJECTED"` and `supplierId = "unknown"`. No letters are silently lost. +- **`SupplierConfigError` is treated as terminal for retries.** The handler sends the original message directly to the allocator DLQ and acknowledges the source record. +- **All other failures retain normal retry semantics.** Non-`SupplierConfigError` records are returned in `batchItemFailures` and retried according to queue configuration. - **The factor algorithm is a running weighted average across the lifetime of the system, not per-batch.** The `overallAllocation` table accumulates counts since deployment. A supplier that handled a disproportionate share yesterday will have a high factor today and be deprioritised, allowing others to catch up to their target percentage. - **Daily capacity check uses London timezone.** `format(toZonedTime(new Date(), "Europe/London"), "yyyy-MM-dd")` determines the date key. Capacity resets at midnight London time, not UTC. - **Quota updates happen after the entire batch completes**, not per-record. This optimisation means concurrent Lambda invocations can transiently over-allocate a supplier before quotas are reconciled. diff --git a/lambdas/supplier-allocator/src/errors/supplier-config-error.ts b/lambdas/supplier-allocator/src/errors/supplier-config-error.ts new file mode 100644 index 000000000..90abdfefb --- /dev/null +++ b/lambdas/supplier-allocator/src/errors/supplier-config-error.ts @@ -0,0 +1,9 @@ +/** + * Error thrown when a supplier cannot be allocated due to incorrect supplier config + */ +export default class SupplierConfigError extends Error { + constructor(public readonly message: string) { + super(message); + this.name = "SupplierConfigError"; + } +} diff --git a/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts b/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts index 97d6e391a..7bf8b495e 100644 --- a/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts +++ b/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts @@ -13,6 +13,7 @@ import * as supplierConfig from "../../services/supplier-config"; import * as supplierQuotas from "../../services/supplier-quotas"; import * as allocationConfig from "../allocation-config"; import { Deps } from "../../config/deps"; +import SupplierConfigError from "../../errors/supplier-config-error"; import packageJson from "../../../package.json"; const renderingSchemaVersion: string = @@ -527,56 +528,13 @@ describe("createSupplierAllocatorHandler", () => { expect(sendCall.input.QueueUrl).toBe(queueUrl); }); - test("logs error when supplier config retrieval fails", async () => { - const preparedEvent = createPreparedV2Event(); - - const evt: SQSEvent = createSQSEvent([ - createSqsRecord("msg1", JSON.stringify(preparedEvent)), - ]); - - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; - const configError = new Error("Failed to retrieve supplier config"); - (supplierConfig.getVariantDetails as jest.Mock).mockRejectedValueOnce( - configError, - ); - - const handler = createSupplierAllocatorHandler(mockedDeps); - const result = await handler(evt, {} as any, {} as any); - if (!result) throw new Error("expected BatchResponse, got void"); - expect((mockedDeps.logger.error as jest.Mock).mock.calls).toHaveLength(1); - expect((mockedDeps.logger.error as jest.Mock).mock.calls[0][0]).toEqual( - expect.objectContaining({ - description: "Error fetching supplier from config", - err: configError, - variantId: "lv1", - }), - ); - expect(mockedDeps.sqsClient.send).toHaveBeenCalledTimes(1); - const sendCall = (mockedDeps.sqsClient.send as jest.Mock).mock.calls[0][0]; - expect(sendCall).toBeInstanceOf(SendMessageCommand); - - const messageBody = JSON.parse(sendCall.input.MessageBody); - expect(messageBody.letterEvent).toEqual(preparedEvent); - expect(messageBody.allocationDetails.supplierSpec).toEqual({ - supplierId: "unknown", - specId: "unknown", - priority: 0, - billingId: "unknown", - }); - expect(messageBody.allocationDetails.allocationStatus).toEqual({ - status: "REJECTED", - reasonCode: "NO_SUPPLIERS_AVAILABLE", - reasonText: "Failed to retrieve supplier config", - }); - }); - const rejectWith = (mock: jest.Mock, errorMessage: string) => mock.mockRejectedValueOnce(new Error(errorMessage)); const throwAny = (mock: jest.Mock) => mock.mockRejectedValueOnce("anything that is not an Error"); - const supplierConfigErrorCases = [ + const nonSupplierConfigErrorCases = [ { name: "getVolumeGroupDetails", errorMessage: "Volume group retrieval failed", @@ -639,8 +597,8 @@ describe("createSupplierAllocatorHandler", () => { }, ]; - test.each(supplierConfigErrorCases)( - "logs error when %s rejects during supplier config resolution", + test.each(nonSupplierConfigErrorCases)( + "returns batch failure when %s rejects with a non-SupplierConfigError", async ({ errorMessage, setup }) => { const preparedEvent = createPreparedV2Event(); const evt: SQSEvent = createSQSEvent([ @@ -648,6 +606,8 @@ describe("createSupplierAllocatorHandler", () => { ]); process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; + process.env.SUPPLIER_ALLOCATOR_DLQ_URL = + "https://sqs.test.queue/supplier-allocator-dlq"; setup(); const handler = createSupplierAllocatorHandler(mockedDeps); @@ -657,71 +617,118 @@ describe("createSupplierAllocatorHandler", () => { expect((mockedDeps.logger.error as jest.Mock).mock.calls).toHaveLength(1); expect((mockedDeps.logger.error as jest.Mock).mock.calls[0][0]).toEqual( expect.objectContaining({ - description: "Error fetching supplier from config", - variantId: "lv1", + description: "Error processing allocation of record", }), ); + expect(result.batchItemFailures).toHaveLength(1); + expect(result.batchItemFailures[0].itemIdentifier).toBe("msg1"); + expect(mockedDeps.sqsClient.send).not.toHaveBeenCalled(); + + expect(errorMessage).toBeDefined(); + }, + ); + + describe("Dead letter queue", () => { + it("places the record on the DLQ when no suppliers are found for pack specification", async () => { + const preparedEvent = createPreparedV2Event(); + const evt: SQSEvent = createSQSEvent([ + createSqsRecord("msg1", JSON.stringify(preparedEvent)), + ]); + + process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; + process.env.SUPPLIER_ALLOCATOR_DLQ_URL = + "https://sqs.test.queue/supplier-allocator-dlq"; + + setupDefaultMocks(); + (allocationConfig.suppliersWithValidPack as jest.Mock).mockResolvedValue( + [], + ); + + const handler = createSupplierAllocatorHandler(mockedDeps); + const result = await handler(evt, {} as any, {} as any); + if (!result) throw new Error("expected BatchResponse, got void"); + + expect((mockedDeps.logger.error as jest.Mock).mock.calls).toHaveLength(1); + expect((mockedDeps.logger.error as jest.Mock).mock.calls[0][0]).toEqual( + expect.objectContaining({ + description: "Error processing allocation of record", + }), + ); + + expect(result.batchItemFailures).toHaveLength(0); expect(mockedDeps.sqsClient.send).toHaveBeenCalledTimes(1); + const sendCall = (mockedDeps.sqsClient.send as jest.Mock).mock .calls[0][0]; expect(sendCall).toBeInstanceOf(SendMessageCommand); + expect(sendCall.input.QueueUrl).toBe( + "https://sqs.test.queue/supplier-allocator-dlq", + ); + expect(sendCall.input.MessageBody).toBe(JSON.stringify(preparedEvent)); + }); - const messageBody = JSON.parse(sendCall.input.MessageBody); - expect(messageBody.letterEvent).toEqual(preparedEvent); - expect(messageBody.allocationDetails.supplierSpec).toEqual({ - supplierId: "unknown", - specId: "unknown", - priority: 0, - billingId: "unknown", - }); - expect(messageBody.allocationDetails.allocationStatus).toEqual({ - status: "REJECTED", - reasonCode: "NO_SUPPLIERS_AVAILABLE", - reasonText: errorMessage, - }); - }, - ); + it("places the record on the DLQ when supplier config retrieval fails", async () => { + const preparedEvent = createPreparedV2Event(); - test("returns batch failure when no suppliers are found for pack specification", async () => { - const preparedEvent = createPreparedV2Event(); - const evt: SQSEvent = createSQSEvent([ - createSqsRecord("msg1", JSON.stringify(preparedEvent)), - ]); + const evt: SQSEvent = createSQSEvent([ + createSqsRecord("msg1", JSON.stringify(preparedEvent)), + ]); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; + process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; + process.env.SUPPLIER_ALLOCATOR_DLQ_URL = + "https://sqs.test.queue/supplier-allocator-dlq"; + const configError = new SupplierConfigError( + "Failed to retrieve supplier config", + ); + (supplierConfig.getVariantDetails as jest.Mock).mockRejectedValueOnce( + configError, + ); - setupDefaultMocks(); - (allocationConfig.suppliersWithValidPack as jest.Mock).mockResolvedValue( - [], - ); + const handler = createSupplierAllocatorHandler(mockedDeps); + const result = await handler(evt, {} as any, {} as any); + if (!result) throw new Error("expected BatchResponse, got void"); + expect((mockedDeps.logger.error as jest.Mock).mock.calls).toHaveLength(1); + expect((mockedDeps.logger.error as jest.Mock).mock.calls[0][0]).toEqual( + expect.objectContaining({ + description: "Error processing allocation of record", + err: configError, + }), + ); + expect(result.batchItemFailures).toHaveLength(0); + expect(mockedDeps.sqsClient.send).toHaveBeenCalledTimes(1); + const sendCall = (mockedDeps.sqsClient.send as jest.Mock).mock + .calls[0][0]; + expect(sendCall.input.QueueUrl).toBe( + "https://sqs.test.queue/supplier-allocator-dlq", + ); + expect(sendCall.input.MessageBody).toBe(JSON.stringify(preparedEvent)); + }); - const handler = createSupplierAllocatorHandler(mockedDeps); - const result = await handler(evt, {} as any, {} as any); - if (!result) throw new Error("expected BatchResponse, got void"); + it("returns batch failure when sending to DLQ fails", async () => { + const preparedEvent = createPreparedV2Event(); + const evt: SQSEvent = createSQSEvent([ + createSqsRecord("msg1", JSON.stringify(preparedEvent)), + ]); - expect((mockedDeps.logger.error as jest.Mock).mock.calls).toHaveLength(1); - expect((mockedDeps.logger.error as jest.Mock).mock.calls[0][0]).toEqual( - expect.objectContaining({ - description: "Error fetching supplier from config", - variantId: "lv1", - }), - ); - expect(mockedDeps.sqsClient.send).toHaveBeenCalledTimes(1); - const sendCall = (mockedDeps.sqsClient.send as jest.Mock).mock.calls[0][0]; - expect(sendCall).toBeInstanceOf(SendMessageCommand); + process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; + process.env.SUPPLIER_ALLOCATOR_DLQ_URL = + "https://sqs.test.queue/supplier-allocator-dlq"; - const messageBody = JSON.parse(sendCall.input.MessageBody); - expect(messageBody.letterEvent).toEqual(preparedEvent); - expect(messageBody.allocationDetails.supplierSpec).toEqual({ - supplierId: "unknown", - specId: "unknown", - priority: 0, - billingId: "unknown", - }); - expect(messageBody.allocationDetails.allocationStatus).toEqual({ - status: "REJECTED", - reasonCode: "NO_SUPPLIERS_AVAILABLE", - reasonText: "No suppliers found for pack specification spec1", + setupDefaultMocks(); + (allocationConfig.suppliersWithValidPack as jest.Mock).mockResolvedValue( + [], + ); + (mockedDeps.sqsClient.send as jest.Mock).mockRejectedValueOnce( + new Error("DLQ send failed"), + ); + + const handler = createSupplierAllocatorHandler(mockedDeps); + const result = await handler(evt, {} as any, {} as any); + if (!result) throw new Error("expected BatchResponse, got void"); + + expect(result.batchItemFailures).toHaveLength(1); + expect(result.batchItemFailures[0].itemIdentifier).toBe("msg1"); + expect((mockedDeps.logger.error as jest.Mock).mock.calls).toHaveLength(2); }); }); diff --git a/lambdas/supplier-allocator/src/handler/__tests__/allocation-config.test.ts b/lambdas/supplier-allocator/src/handler/__tests__/allocation-config.test.ts index ef3e576f2..773ece53a 100644 --- a/lambdas/supplier-allocator/src/handler/__tests__/allocation-config.test.ts +++ b/lambdas/supplier-allocator/src/handler/__tests__/allocation-config.test.ts @@ -21,6 +21,15 @@ import * as supplierQuotasService from "../../services/supplier-quotas"; jest.mock("../../services/supplier-config"); jest.mock("../../services/supplier-quotas"); +async function expectSupplierConfigError( + promise: Promise, + message: string | RegExp, +): Promise { + await expect(promise).rejects.toThrow(message); + await expect(promise).rejects.toMatchObject({ + name: "SupplierConfigError", + }); +} describe("eligibleSuppliers", () => { let mockDeps: jest.Mocked; let mockVolumeGroup: VolumeGroup; @@ -970,14 +979,14 @@ describe("selectSupplierByFactor", () => { } as SupplierAllocation, ]; - await expect( + await expectSupplierConfigError( selectSupplierByFactor( mockSuppliers, zeroAllocations, domainId, mockDeps, ), - ).rejects.toThrow( + "No valid supplier allocations found for suppliers with valid pack", ); }); @@ -1155,13 +1164,14 @@ describe("selectSupplierByFactor", () => { supplierQuotasService.calculateSupplierAllocatedFactor as jest.Mock ).mockResolvedValue([]); - await expect( + await expectSupplierConfigError( selectSupplierByFactor( mockSuppliers, mockSupplierAllocations, domainId, mockDeps, ), - ).rejects.toThrow("No supplier factors could be calculated for allocation"); + "No supplier factors could be calculated for allocation", + ); }); }); diff --git a/lambdas/supplier-allocator/src/handler/allocate-handler.ts b/lambdas/supplier-allocator/src/handler/allocate-handler.ts index 795814937..8ae953e60 100644 --- a/lambdas/supplier-allocator/src/handler/allocate-handler.ts +++ b/lambdas/supplier-allocator/src/handler/allocate-handler.ts @@ -1,4 +1,10 @@ -import { Context, SQSBatchItemFailure, SQSEvent, SQSHandler } from "aws-lambda"; +import { + Context, + SQSBatchItemFailure, + SQSEvent, + SQSHandler, + SQSRecord, +} from "aws-lambda"; import { SendMessageCommand } from "@aws-sdk/client-sqs"; import { LetterVariant, @@ -31,6 +37,7 @@ import { } from "./allocation-config"; import { Deps } from "../config/deps"; import { PreparedEventSchema, PreparedEvents, SupplierDetails } from "./types"; +import SupplierConfigError from "../errors/supplier-config-error"; const idempotencyConfig = new IdempotencyConfig({ eventKeyJmesPath: "data.domainId", @@ -81,99 +88,79 @@ async function getSupplierFromConfig( letterEvent: PreparedEvents, deps: Deps, ): Promise { - try { - const letterVariant: LetterVariant = await getVariantDetails( - letterEvent.data.letterVariantId, - deps, - ); + const letterVariant: LetterVariant = await getVariantDetails( + letterEvent.data.letterVariantId, + deps, + ); - const volumeGroup: VolumeGroup = await getVolumeGroupDetails( - letterVariant.volumeGroupId, - deps, - ); + const volumeGroup: VolumeGroup = await getVolumeGroupDetails( + letterVariant.volumeGroupId, + deps, + ); - const { supplierAllocations, suppliers: allocatedSuppliers } = - await eligibleSuppliers(volumeGroup, deps, letterVariant.supplierId); + const { supplierAllocations, suppliers: allocatedSuppliers } = + await eligibleSuppliers(volumeGroup, deps, letterVariant.supplierId); - const preferredPack: PackSpecification = await preferredSupplierPack( - letterEvent, - allocatedSuppliers, - letterVariant.packSpecificationIds, - deps, - ); + const preferredPack: PackSpecification = await preferredSupplierPack( + letterEvent, + allocatedSuppliers, + letterVariant.packSpecificationIds, + deps, + ); - const allSuppliersForPack: Supplier[] = await suppliersWithValidPack( - allocatedSuppliers, - preferredPack.id, - deps, + const allSuppliersForPack: Supplier[] = await suppliersWithValidPack( + allocatedSuppliers, + preferredPack.id, + deps, + ); + + if (allSuppliersForPack.length === 0) { + throw new SupplierConfigError( + `No suppliers found for pack specification ${preferredPack.id}`, ); + } - if (allSuppliersForPack.length === 0) { - throw new Error( - `No suppliers found for pack specification ${preferredPack.id}`, - ); - } + const suppliersForPackWithCapacity: Supplier[] = + await filterSuppliersWithCapacity(allSuppliersForPack, deps); + + // selected supplier id is determined by first calling selectSupplierByFactor for suppliers with capacity + // and if that returns nothing, try again with all suppliers for the pack + const selectedSupplierId = + (suppliersForPackWithCapacity.length > 0 + ? await selectSupplierByFactor( + suppliersForPackWithCapacity, + supplierAllocations, + letterEvent.data.domainId, + deps, + ) + : undefined) ?? + (await selectSupplierByFactor( + allSuppliersForPack, + supplierAllocations, + letterEvent.data.domainId, + deps, + )); - const suppliersForPackWithCapacity: Supplier[] = - await filterSuppliersWithCapacity(allSuppliersForPack, deps); - - // selected supplier id is determined by first calling selectSupplierByFactor for suppliers with capacity - // and if that returns nothing, try again with all suppliers for the pack - const selectedSupplierId = - (suppliersForPackWithCapacity.length > 0 - ? await selectSupplierByFactor( - suppliersForPackWithCapacity, - supplierAllocations, - letterEvent.data.domainId, - deps, - ) - : undefined) ?? - (await selectSupplierByFactor( - allSuppliersForPack, - supplierAllocations, - letterEvent.data.domainId, - deps, - )); - - deps.logger.info({ - description: "Fetched supplier details for supplier allocations", - domainId: letterEvent.data.domainId, - variantId: letterEvent.data.letterVariantId, - volumeGroupId: volumeGroup.id, - supplierAllocationIds: supplierAllocations.map((a) => a.id), - allocatedSuppliers, - allSuppliersForPack: allSuppliersForPack.map((s) => s.id), - suppliersForPackWithCapacity: suppliersForPackWithCapacity.map( - (s) => s.id, - ), - selectedSupplierId, - }); + deps.logger.info({ + description: "Fetched supplier details for supplier allocations", + domainId: letterEvent.data.domainId, + variantId: letterEvent.data.letterVariantId, + volumeGroupId: volumeGroup.id, + supplierAllocationIds: supplierAllocations.map((a) => a.id), + allocatedSuppliers, + allSuppliersForPack: allSuppliersForPack.map((s) => s.id), + suppliersForPackWithCapacity: suppliersForPackWithCapacity.map((s) => s.id), + selectedSupplierId, + }); - return buildSupplierDetails( - selectedSupplierId, - preferredPack.id, - preferredPack.billingId, - letterVariant.priority, - "PENDING", - volumeGroup.id, - ); - } catch (error) { - deps.logger.error({ - description: "Error fetching supplier from config", - err: error, - variantId: letterEvent.data.letterVariantId, - }); - return buildSupplierDetails( - "unknown", - "unknown", - "unknown", - 0, - "REJECTED", - "unknown", - "NO_SUPPLIERS_AVAILABLE", - error instanceof Error ? error.message : "Unknown error", - ); - } + return buildSupplierDetails( + selectedSupplierId, + preferredPack.id, + preferredPack.billingId, + letterVariant.priority, + "PENDING", + volumeGroup.id, + ); } type AllocationMetrics = Map>; @@ -338,6 +325,26 @@ async function processSupplierAllocation( }; } +async function placeOnDeadLetterQueue(record: SQSRecord, deps: Deps) { + const deadLetterQueueUrl = process.env.SUPPLIER_ALLOCATOR_DLQ_URL; + if (!deadLetterQueueUrl) { + throw new Error("SUPPLIER_ALLOCATOR_DLQ_URL not configured"); + } + + deps.logger.info({ + description: "Sending record to supplier allocator DLQ", + messageId: record.messageId, + deadLetterQueueUrl, + }); + + await deps.sqsClient.send( + new SendMessageCommand({ + QueueUrl: deadLetterQueueUrl, + MessageBody: record.body, + }), + ); +} + export default function createSupplierAllocatorHandler(deps: Deps): SQSHandler { const createGetSupplierIdempotently = ( perAllocationSuccess: AllocationMetrics, @@ -400,7 +407,21 @@ export default function createSupplierAllocatorHandler(deps: Deps): SQSHandler { message: record.body, }); incrementMetric(perAllocationFailure, supplier, priority); - batchItemFailures.push({ itemIdentifier: record.messageId }); + if (error instanceof SupplierConfigError) { + try { + await placeOnDeadLetterQueue(record, deps); + } catch (dlqError) { + deps.logger.error({ + description: "Failed to send record to supplier allocator DLQ", + err: dlqError, + messageId: record.messageId, + message: record.body, + }); + batchItemFailures.push({ itemIdentifier: record.messageId }); + } + } else { + batchItemFailures.push({ itemIdentifier: record.messageId }); + } } }); diff --git a/lambdas/supplier-allocator/src/handler/allocation-config.ts b/lambdas/supplier-allocator/src/handler/allocation-config.ts index f9e7f92d1..70291d76f 100644 --- a/lambdas/supplier-allocator/src/handler/allocation-config.ts +++ b/lambdas/supplier-allocator/src/handler/allocation-config.ts @@ -19,6 +19,7 @@ import { calculateSupplierAllocatedFactor } from "../services/supplier-quotas"; import { Deps } from "../config/deps"; import { PreparedEvents } from "./types"; +import SupplierConfigError from "../errors/supplier-config-error"; export async function eligibleSuppliers( volumeGroup: VolumeGroup, @@ -163,7 +164,7 @@ export async function selectSupplierByFactor( return suppliers.some((supplier) => supplier.id === alloc.supplier); }); if (supplierAllocationsForPack.length === 0) { - throw new Error( + throw new SupplierConfigError( "No valid supplier allocations found for suppliers with valid pack", ); } @@ -171,7 +172,9 @@ export async function selectSupplierByFactor( await calculateSupplierAllocatedFactor(supplierAllocationsForPack, deps); if (supplierFactors.length === 0) { - throw new Error("No supplier factors could be calculated for allocation"); + throw new SupplierConfigError( + "No supplier factors could be calculated for allocation", + ); } deps.logger.info({ diff --git a/lambdas/supplier-allocator/src/services/__tests__/supplier-config.test.ts b/lambdas/supplier-allocator/src/services/__tests__/supplier-config.test.ts index 1f6807dba..b27d11fda 100644 --- a/lambdas/supplier-allocator/src/services/__tests__/supplier-config.test.ts +++ b/lambdas/supplier-allocator/src/services/__tests__/supplier-config.test.ts @@ -31,6 +31,16 @@ function makeDeps(overrides: Partial = {}): Deps { return { ...(base as Deps), ...overrides }; } +async function expectSupplierConfigError( + promise: Promise, + message: string | RegExp, +): Promise { + await expect(promise).rejects.toThrow(message); + await expect(promise).rejects.toMatchObject({ + name: "SupplierConfigError", + }); +} + describe("supplier-config service", () => { afterEach(() => jest.resetAllMocks()); @@ -88,7 +98,8 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue(group); - await expect(getVolumeGroupDetails("g2", deps)).rejects.toThrow( + await expectSupplierConfigError( + getVolumeGroupDetails("g2", deps), /not active/, ); expect(deps.logger.error).toHaveBeenCalled(); @@ -102,7 +113,8 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue(group); - await expect(getVolumeGroupDetails("g3", deps)).rejects.toThrow( + await expectSupplierConfigError( + getVolumeGroupDetails("g3", deps), /not active/, ); expect(deps.logger.error).toHaveBeenCalled(); @@ -121,7 +133,8 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue(group); - await expect(getVolumeGroupDetails("g3", deps)).rejects.toThrow( + await expectSupplierConfigError( + getVolumeGroupDetails("g3", deps), /not active/, ); expect(deps.logger.error).toHaveBeenCalled(); @@ -183,9 +196,10 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue(allocations); - await expect( + await expectSupplierConfigError( getSupplierAllocationsForVolumeGroup("g1", deps, "missing"), - ).rejects.toThrow(/No supplier allocations found/); + /No supplier allocations found/, + ); expect(deps.logger.error).toHaveBeenCalled(); }); }); @@ -218,7 +232,8 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue([]); - await expect(getSupplierDetails(supplierIds, deps)).rejects.toThrow( + await expectSupplierConfigError( + getSupplierDetails(supplierIds, deps), /No supplier details found/, ); }); @@ -293,7 +308,8 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue(suppliers); - await expect(getSupplierDetails(supplierIds, deps)).rejects.toThrow( + await expectSupplierConfigError( + getSupplierDetails(supplierIds, deps), /No active suppliers found/, ); expect(deps.logger.error).toHaveBeenCalledWith( @@ -360,9 +376,10 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue([]); - await expect( + await expectSupplierConfigError( getPreferredSupplierPacks(["spec1"], suppliers, deps), - ).rejects.toThrow(/No preferred supplier packs found/); + /No preferred supplier packs found/, + ); expect(deps.logger.error).toHaveBeenCalledWith( expect.objectContaining({ description: @@ -409,9 +426,10 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue(supplierPacks); - await expect( + await expectSupplierConfigError( getPreferredSupplierPacks(["spec1"], suppliers, deps), - ).rejects.toThrow(/No preferred supplier packs found/); + /No preferred supplier packs found/, + ); expect(deps.logger.error).toHaveBeenCalledWith( expect.objectContaining({ description: @@ -449,7 +467,8 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue(packSpec); - await expect(getPackSpecification("spec2", deps)).rejects.toThrow( + await expectSupplierConfigError( + getPackSpecification("spec2", deps), /not active/, ); expect(deps.logger.error).toHaveBeenCalledWith( @@ -500,9 +519,8 @@ describe("supplier-config service", () => { }, } as any; - await expect( + await expectSupplierConfigError( filterPacksForLetter(letterEvent, ["spec1"], deps), - ).rejects.toThrow( "No eligible pack specifications found for letter variant id undefined and pack specification ids spec1", ); expect(deps.logger.info).toHaveBeenCalledWith({ @@ -571,9 +589,10 @@ describe("supplier-config service", () => { }, } as any; - await expect( + await expectSupplierConfigError( filterPacksForLetter(letterEvent, ["spec1"], deps), - ).rejects.toThrow(/No eligible pack specifications found/); + /No eligible pack specifications found/, + ); expect(deps.logger.info).toHaveBeenCalledWith({ description: @@ -611,9 +630,10 @@ describe("supplier-config service", () => { }, } as any; - await expect( + await expectSupplierConfigError( filterPacksForLetter(letterEvent, ["spec1"], deps), - ).rejects.toThrow(/No eligible pack specifications found/); + /No eligible pack specifications found/, + ); expect(deps.logger.info).toHaveBeenCalledWith({ description: @@ -731,9 +751,8 @@ describe("supplier-config service", () => { }, } as any; - await expect( + await expectSupplierConfigError( filterPacksForLetter(letterEvent, ["spec1"], deps), - ).rejects.toThrow( "Unsupported operator UNSUPPORTED_OP in pack specification constraints", ); }); diff --git a/lambdas/supplier-allocator/src/services/supplier-config.ts b/lambdas/supplier-allocator/src/services/supplier-config.ts index 7ed2509bd..66084fa62 100644 --- a/lambdas/supplier-allocator/src/services/supplier-config.ts +++ b/lambdas/supplier-allocator/src/services/supplier-config.ts @@ -9,6 +9,7 @@ import { import { Deps } from "../config/deps"; import { PreparedEvents } from "../handler/types"; +import SupplierConfigError from "../errors/supplier-config-error"; export async function getVariantDetails( variantId: string, @@ -46,7 +47,9 @@ export async function getVolumeGroupDetails( startDate: groupDetails.startDate, endDate: groupDetails.endDate, }); - throw new Error(`Volume group with id ${groupId} is not active`); + throw new SupplierConfigError( + `Volume group with id ${groupId} is not active`, + ); } export async function getSupplierAllocationsForVolumeGroup( @@ -68,7 +71,7 @@ export async function getSupplierAllocationsForVolumeGroup( groupId, supplierId, }); - throw new Error( + throw new SupplierConfigError( `No supplier allocations found for variant supplier id ${supplierId} in volume group ${groupId}`, ); } @@ -90,7 +93,7 @@ export async function getSupplierDetails( description: "No supplier details found for supplier allocations", supplierIds, }); - throw new Error( + throw new SupplierConfigError( `No supplier details found for supplier ids ${supplierIds.join(", ")}`, ); } @@ -113,7 +116,7 @@ export async function getSupplierDetails( description: "No active suppliers found for supplier allocations", supplierIds, }); - throw new Error( + throw new SupplierConfigError( `No active suppliers found for supplier ids ${supplierIds.join(", ")}`, ); } @@ -145,7 +148,7 @@ export async function getPreferredSupplierPacks( packSpecificationIds, supplierIds: suppliers.map((s) => s.id), }); - throw new Error( + throw new SupplierConfigError( `No preferred supplier packs found for pack specification ids ${packSpecificationIds.join(", ")} and suppliers ${suppliers.map((s) => s.id).join(", ")}`, ); } @@ -162,7 +165,9 @@ export async function getPackSpecification( packSpecId, status: packSpec.status, }); - throw new Error(`Pack specification with id ${packSpecId} is not active`); + throw new SupplierConfigError( + `Pack specification with id ${packSpecId} is not active`, + ); } return packSpec; } @@ -203,7 +208,7 @@ function evaluateContraint( return actualValue <= constraintValue; } default: { - throw new Error( + throw new SupplierConfigError( `Unsupported operator ${operator} in pack specification constraints`, ); } @@ -301,7 +306,6 @@ export async function filterPacksForLetter( if (violatedConstraints.length > 0) { deps.logger.info({ description: `Pack specification filtered out based on pageCount constraints`, - dommainId: letterEvent.data.domainId, packSpecId, pageCount, violatedConstraints, @@ -322,7 +326,7 @@ export async function filterPacksForLetter( letterVariantId: letterEvent.data.letterVariantId, packSpecificationIds, }); - throw new Error( + throw new SupplierConfigError( `No eligible pack specifications found for letter variant id ${letterEvent.data.letterVariantId} and pack specification ids ${packSpecificationIds.join(", ")}`, ); } From 13835a47323c1a232b448481e0a0e89d6a2c74d1 Mon Sep 17 00:00:00 2001 From: Steve Buxton Date: Fri, 7 Aug 2026 16:46:33 +0100 Subject: [PATCH 02/11] Simplify to improve code coverage --- .../src/handler/allocate-handler.ts | 26 +++++++------------ 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/lambdas/supplier-allocator/src/handler/allocate-handler.ts b/lambdas/supplier-allocator/src/handler/allocate-handler.ts index 8ae953e60..2b53b6eb0 100644 --- a/lambdas/supplier-allocator/src/handler/allocate-handler.ts +++ b/lambdas/supplier-allocator/src/handler/allocate-handler.ts @@ -281,20 +281,17 @@ async function processSupplierAllocation( const supplier = supplierSpec.supplierId; const priority = String(supplierSpec.priority); - if (supplierDetails.allocationDetails.allocationStatus.status === "PENDING") { - incrementMetric(perAllocationSuccess, supplier, priority); - emitDataMetrics(letterEvent, supplier, "extra_data_dimensions", deps); + incrementMetric(perAllocationSuccess, supplier, priority); + emitDataMetrics(letterEvent, supplier, "extra_data_dimensions", deps); - incrementAllocation( - volumeGroupAllocations, - supplierDetails.volumeGroupId, - supplier, - 1, - deps, - ); - } else { - incrementMetric(perAllocationFailure, supplier, priority); - } + incrementAllocation( + volumeGroupAllocations, + supplierDetails.volumeGroupId, + supplier, + 1, + deps, + ); +} // Send to allocated letters queue const queueUrl = process.env.UPSERT_LETTERS_QUEUE_URL; @@ -327,9 +324,6 @@ async function processSupplierAllocation( async function placeOnDeadLetterQueue(record: SQSRecord, deps: Deps) { const deadLetterQueueUrl = process.env.SUPPLIER_ALLOCATOR_DLQ_URL; - if (!deadLetterQueueUrl) { - throw new Error("SUPPLIER_ALLOCATOR_DLQ_URL not configured"); - } deps.logger.info({ description: "Sending record to supplier allocator DLQ", From ed65c302a174373d2b1ebb00e5f9b5ebf6b6ca34 Mon Sep 17 00:00:00 2001 From: Steve Buxton Date: Mon, 10 Aug 2026 09:02:23 +0100 Subject: [PATCH 03/11] Test coverage --- .../__tests__/allocate-handler.test.ts | 66 ++++++++----------- .../src/handler/allocate-handler.ts | 4 +- 2 files changed, 32 insertions(+), 38 deletions(-) diff --git a/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts b/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts index 7bf8b495e..adfe6a27e 100644 --- a/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts +++ b/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts @@ -219,6 +219,9 @@ describe("createSupplierAllocatorHandler", () => { } as unknown as Deps; beforeEach(() => { + process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; + process.env.SUPPLIER_ALLOCATOR_DLQ_URL = + "https://sqs.test.queue/supplier-allocator-dlq"; jest.clearAllMocks(); }); @@ -229,7 +232,6 @@ describe("createSupplierAllocatorHandler", () => { ]); setupDefaultMocks(); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; const handler = createSupplierAllocatorHandler(mockedDeps); const result = await handler(evt, {} as any, {} as any); @@ -266,7 +268,6 @@ describe("createSupplierAllocatorHandler", () => { ]); setupDefaultMocks(); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; const handler = createSupplierAllocatorHandler(mockedDeps); const result = await handler(evt, {} as any, {} as any); @@ -300,7 +301,6 @@ describe("createSupplierAllocatorHandler", () => { createSqsRecord("msg1", JSON.stringify(preparedEvent)), ]); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; setupDefaultMocks(); const handler = createSupplierAllocatorHandler(mockedDeps); const result = await handler(evt, {} as any, {} as any); @@ -331,8 +331,6 @@ describe("createSupplierAllocatorHandler", () => { createSqsRecord("invalid-event", JSON.stringify(preparedEvent)), ]); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; - const handler = createSupplierAllocatorHandler(mockedDeps); const result = await handler(evt, {} as any, {} as any); @@ -351,8 +349,6 @@ describe("createSupplierAllocatorHandler", () => { createSqsRecord("msg1", JSON.stringify(preparedEvent)), ]); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; - const handler = createSupplierAllocatorHandler(mockedDeps); await handler(evt, {} as any, {} as any); @@ -371,8 +367,6 @@ describe("createSupplierAllocatorHandler", () => { createSqsRecord("msg1", JSON.stringify(preparedEvent)), ]); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; - const handler = createSupplierAllocatorHandler(mockedDeps); const result = await handler(evt, {} as any, {} as any); @@ -396,8 +390,6 @@ describe("createSupplierAllocatorHandler", () => { ), ]); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; - const handler = createSupplierAllocatorHandler(mockedDeps); const result = await handler(evt, {} as any, {} as any); @@ -413,8 +405,6 @@ describe("createSupplierAllocatorHandler", () => { createSqsRecord("bad-json", "this-is-not-json"), ]); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; - const handler = createSupplierAllocatorHandler(mockedDeps); const result = await handler(evt, {} as any, {} as any); @@ -433,8 +423,6 @@ describe("createSupplierAllocatorHandler", () => { createSqsRecord("no-type", JSON.stringify(event)), ]); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; - const handler = createSupplierAllocatorHandler(mockedDeps); const result = await handler(evt, {} as any, {} as any); if (!result) throw new Error("expected BatchResponse, got void"); @@ -450,6 +438,7 @@ describe("createSupplierAllocatorHandler", () => { createSqsRecord("msg1", JSON.stringify(preparedEvent)), ]); + setupDefaultMocks(); delete process.env.UPSERT_LETTERS_QUEUE_URL; const handler = createSupplierAllocatorHandler(mockedDeps); @@ -472,8 +461,6 @@ describe("createSupplierAllocatorHandler", () => { createSqsRecord("msg1", JSON.stringify(preparedEvent)), ]); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; - const sqsError = new Error("SQS send failed"); (mockedDeps.sqsClient.send as jest.Mock).mockRejectedValueOnce(sqsError); @@ -499,8 +486,6 @@ describe("createSupplierAllocatorHandler", () => { ), ]); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; - const handler = createSupplierAllocatorHandler(mockedDeps); const result = await handler(evt, {} as any, {} as any); if (!result) throw new Error("expected BatchResponse, got void"); @@ -605,9 +590,6 @@ describe("createSupplierAllocatorHandler", () => { createSqsRecord("msg1", JSON.stringify(preparedEvent)), ]); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; - process.env.SUPPLIER_ALLOCATOR_DLQ_URL = - "https://sqs.test.queue/supplier-allocator-dlq"; setup(); const handler = createSupplierAllocatorHandler(mockedDeps); @@ -635,10 +617,6 @@ describe("createSupplierAllocatorHandler", () => { createSqsRecord("msg1", JSON.stringify(preparedEvent)), ]); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; - process.env.SUPPLIER_ALLOCATOR_DLQ_URL = - "https://sqs.test.queue/supplier-allocator-dlq"; - setupDefaultMocks(); (allocationConfig.suppliersWithValidPack as jest.Mock).mockResolvedValue( [], @@ -674,9 +652,6 @@ describe("createSupplierAllocatorHandler", () => { createSqsRecord("msg1", JSON.stringify(preparedEvent)), ]); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; - process.env.SUPPLIER_ALLOCATOR_DLQ_URL = - "https://sqs.test.queue/supplier-allocator-dlq"; const configError = new SupplierConfigError( "Failed to retrieve supplier config", ); @@ -704,15 +679,36 @@ describe("createSupplierAllocatorHandler", () => { expect(sendCall.input.MessageBody).toBe(JSON.stringify(preparedEvent)); }); - it("returns batch failure when sending to DLQ fails", async () => { + it("returns batch failure when DLQ not configured", async () => { const preparedEvent = createPreparedV2Event(); const evt: SQSEvent = createSQSEvent([ createSqsRecord("msg1", JSON.stringify(preparedEvent)), ]); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; - process.env.SUPPLIER_ALLOCATOR_DLQ_URL = - "https://sqs.test.queue/supplier-allocator-dlq"; + const configError = new SupplierConfigError( + "Failed to retrieve supplier config", + ); + (supplierConfig.getVariantDetails as jest.Mock).mockRejectedValueOnce( + configError, + ); + + setupDefaultMocks(); + delete process.env.SUPPLIER_ALLOCATOR_DLQ_URL; + + const handler = createSupplierAllocatorHandler(mockedDeps); + const result = await handler(evt, {} as any, {} as any); + if (!result) throw new Error("expected BatchResponse, got void"); + + expect(result.batchItemFailures).toHaveLength(1); + expect(result.batchItemFailures[0].itemIdentifier).toBe("msg1"); + expect((mockedDeps.logger.error as jest.Mock).mock.calls).toHaveLength(2); + }); + + it("returns batch failure when sending to DLQ fails", async () => { + const preparedEvent = createPreparedV2Event(); + const evt: SQSEvent = createSQSEvent([ + createSqsRecord("msg1", JSON.stringify(preparedEvent)), + ]); setupDefaultMocks(); (allocationConfig.suppliersWithValidPack as jest.Mock).mockResolvedValue( @@ -737,7 +733,6 @@ describe("createSupplierAllocatorHandler", () => { ( allocationConfig.filterSuppliersWithCapacity as jest.Mock ).mockResolvedValue([]); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; const evt: SQSEvent = createSQSEvent([ createSqsRecord("msg1", JSON.stringify(createPreparedV2Event())), @@ -761,7 +756,6 @@ describe("createSupplierAllocatorHandler", () => { (allocationConfig.selectSupplierByFactor as jest.Mock) .mockResolvedValueOnce(null) .mockResolvedValueOnce("supplier1"); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; const evt: SQSEvent = createSQSEvent([ createSqsRecord("msg1", JSON.stringify(createPreparedV2Event())), @@ -781,8 +775,6 @@ describe("createSupplierAllocatorHandler", () => { createSqsRecord("msg1", JSON.stringify(preparedEvent)), ]); - process.env.UPSERT_LETTERS_QUEUE_URL = "https://sqs.test.queue"; - setupDefaultMocks(); (makeIdempotent as jest.Mock).mockImplementationOnce((_fn) => "supplier1"); diff --git a/lambdas/supplier-allocator/src/handler/allocate-handler.ts b/lambdas/supplier-allocator/src/handler/allocate-handler.ts index 2b53b6eb0..b27a621ae 100644 --- a/lambdas/supplier-allocator/src/handler/allocate-handler.ts +++ b/lambdas/supplier-allocator/src/handler/allocate-handler.ts @@ -291,7 +291,6 @@ async function processSupplierAllocation( 1, deps, ); -} // Send to allocated letters queue const queueUrl = process.env.UPSERT_LETTERS_QUEUE_URL; @@ -324,6 +323,9 @@ async function processSupplierAllocation( async function placeOnDeadLetterQueue(record: SQSRecord, deps: Deps) { const deadLetterQueueUrl = process.env.SUPPLIER_ALLOCATOR_DLQ_URL; + if (!deadLetterQueueUrl) { + throw new Error("SUPPLIER_ALLOCATOR_DLQ_URL not configured"); + } deps.logger.info({ description: "Sending record to supplier allocator DLQ", From baf99459c54797ac54071e1c0217318a01f7720b Mon Sep 17 00:00:00 2001 From: Steve Buxton Date: Mon, 10 Aug 2026 14:02:05 +0100 Subject: [PATCH 04/11] Test fixes --- .../allocation-target-percentage.spec.ts | 17 +----- .../letter-allocation-rejected.spec.ts | 54 ++++--------------- .../supplier-allocation.spec.ts | 12 ++--- tests/helpers/aws-queue-helper.ts | 43 +++++++++++++++ tests/package.json | 1 + 5 files changed, 59 insertions(+), 68 deletions(-) create mode 100644 tests/helpers/aws-queue-helper.ts diff --git a/tests/component-tests/allocation-tests/allocation-target-percentage.spec.ts b/tests/component-tests/allocation-tests/allocation-target-percentage.spec.ts index 65bc5d6db..0a627bb4d 100644 --- a/tests/component-tests/allocation-tests/allocation-target-percentage.spec.ts +++ b/tests/component-tests/allocation-tests/allocation-target-percentage.spec.ts @@ -8,6 +8,7 @@ import { updateSupplierAllocation, updateVolumeGroupData, } from "tests/helpers/allocation-helper"; +import { pollQueueForLetterEvent } from "tests/helpers/aws-queue-helper"; import { createPreparedV1Event } from "tests/helpers/event-fixtures"; import { getLettersFromSupplierTable } from "tests/helpers/generate-fetch-test-data"; import { sendSnsEvent } from "tests/helpers/send-sns-event"; @@ -44,21 +45,7 @@ test.describe("Allocation Target Percentage Tests", () => { const response = await sendSnsEvent(preparedEvent); expect(response.MessageId).toBeTruthy(); - const allocationLog = await getAllocationLogForDomainId(domainId); - const lettersInDb = await getLettersFromSupplierTable( - "unknown", - domainId, - "REJECTED", - ); - - expect(lettersInDb.status).toBe("REJECTED"); - expect(lettersInDb.supplierId).toBe( - allocationLog.msg?.allocationDetails?.supplierSpec?.supplierId, - ); - expect(lettersInDb.reasonCode).toBe("NO_SUPPLIERS_AVAILABLE"); - expect(lettersInDb.reasonText).toBe( - `No valid supplier allocations found for suppliers with valid pack`, - ); + await pollQueueForLetterEvent("supplier-allocator-dlq", domainId); }); test("Verify that supplier with less than 100 target percentage is handled correctly", async () => { diff --git a/tests/component-tests/allocation-tests/letter-allocation-rejected.spec.ts b/tests/component-tests/allocation-tests/letter-allocation-rejected.spec.ts index bcf4a6182..3e2898243 100644 --- a/tests/component-tests/allocation-tests/letter-allocation-rejected.spec.ts +++ b/tests/component-tests/allocation-tests/letter-allocation-rejected.spec.ts @@ -9,8 +9,8 @@ import { updateLetterVariantPackSpecs, updateVolumeGroupData, } from "tests/helpers/allocation-helper"; +import { pollQueueForLetterEvent } from "tests/helpers/aws-queue-helper"; import { createPreparedV1Event } from "tests/helpers/event-fixtures"; -import { getLettersFromSupplierTable } from "tests/helpers/generate-fetch-test-data"; import { logger } from "tests/helpers/pino-logger"; import { sendSnsEvent } from "tests/helpers/send-sns-event"; @@ -27,7 +27,7 @@ test.describe("Allocator Rejected Allocation Tests", () => { { testCase: 1, testName: - "Verify that the letters are REJECTED when no pack specification is eligible", + "Verify that the letters are placed on a DLQ when no pack specification is eligible", letterVariantMapping: 1, domainIdName: "NoEligiblePackSpecs", pageCount: 100, // high page count to ensure pack specifications are filtered out based on constraints @@ -36,7 +36,7 @@ test.describe("Allocator Rejected Allocation Tests", () => { { testCase: 2, testName: - "Verify that the letters are REJECTED when no supplier pack are found for selected pack", + "Verify that the letters are placed on a DLQ when no supplier pack are found for selected pack", letterVariantMapping: 6, domainIdName: "NoSupplierPacksFound", pageCount: 2, @@ -46,7 +46,7 @@ test.describe("Allocator Rejected Allocation Tests", () => { { testCase: 3, testName: - "Verify that the letters are REJECTED when no pack specification found for letter variant", + "Verify that the letters are placed on a DLQ when no pack specification found for letter variant", letterVariantMapping: 7, domainIdName: "NoPackSpecificationFound", pageCount: 2, @@ -72,38 +72,17 @@ test.describe("Allocator Rejected Allocation Tests", () => { const supplierAllocatorLog = await getAllocationLog(expectedError); - const allocationLog = await getAllocationLogForDomainId(domainId); - const lettersInDb = await getLettersFromSupplierTable( - "unknown", - domainId, - "REJECTED", - ); + await pollQueueForLetterEvent("supplier-allocator-dlq", domainId); - expect(lettersInDb.status).toBe("REJECTED"); - expect(lettersInDb.supplierId).toBe( - allocationLog.msg?.allocationDetails?.supplierSpec?.supplierId, - ); switch (testCase) { - case 1: { - const { packSpecificationIds } = supplierAllocatorLog; - expect(packSpecificationIds).toBeTruthy(); - expect(lettersInDb.reasonText).toBe( - `No eligible pack specifications found for letter variant id ${letterVariant} and pack specification ids ${packSpecificationIds?.join(", ")}`, - ); - break; - } + case 1: case 2: { const { packSpecificationIds } = supplierAllocatorLog; expect(packSpecificationIds).toBeTruthy(); - expect(lettersInDb.reasonText).toContain( - `No preferred supplier packs found for pack specification ids ${packSpecificationIds?.join(", ")} and suppliers`, - ); break; } + case 3: { - expect(lettersInDb.reasonText).toContain( - `No pack specification found for id`, - ); await updateLetterVariantPackSpecs(letterVariant, [ "notify-c5-colour", ]); // update back to valid config for other tests @@ -119,13 +98,13 @@ test.describe("Allocator Rejected Allocation Tests", () => { for (const { fieldToUpdate, testName, volumeGroupId } of [ { testName: - "Verify that letters are rejected when volumeGroup is not active", + "Verify that letters are placed on a DLQ when volumeGroup is not active", volumeGroupId: "volumeGroup-test2", fieldToUpdate: "startDate", }, { testName: - "Verify that letters are rejected when volumeGroup is no longer active", + "Verify that letters are placed on a DLQ when volumeGroup is no longer active", volumeGroupId: "volumeGroup-test2", fieldToUpdate: "endDate", }, @@ -163,20 +142,7 @@ test.describe("Allocator Rejected Allocation Tests", () => { const response = await sendSnsEvent(preparedEvent); expect(response.MessageId).toBeTruthy(); - const allocationLog = await getAllocationLogForDomainId(domainId); - const lettersInDb = await getLettersFromSupplierTable( - "unknown", - domainId, - "REJECTED", - ); - - expect(lettersInDb.status).toBe("REJECTED"); - expect(lettersInDb.supplierId).toBe( - allocationLog.msg?.allocationDetails?.supplierSpec?.supplierId, - ); - expect(lettersInDb.reasonText).toContain( - `Volume group with id ${volumeGroupId} is not active`, - ); + await pollQueueForLetterEvent("supplier-allocator-dlq", domainId); const resolvedOriginalEndDate = originalEndDate ?? diff --git a/tests/component-tests/allocation-tests/supplier-allocation.spec.ts b/tests/component-tests/allocation-tests/supplier-allocation.spec.ts index fc1f03c27..c71ad873e 100644 --- a/tests/component-tests/allocation-tests/supplier-allocation.spec.ts +++ b/tests/component-tests/allocation-tests/supplier-allocation.spec.ts @@ -13,6 +13,7 @@ import { toZonedTime } from "date-fns-tz"; import { randomUUID } from "node:crypto"; import { createPreparedV1Event } from "tests/helpers/event-fixtures"; import { sendSnsBatchEvent } from "tests/helpers/send-sns-event"; +import { pollQueueForLetterEvent } from "tests/helpers/aws-queue-helper"; test.describe("Supplier Allocation Tests", () => { test("Verify that successful supplier allocation emits a PENDING event for the allocated supplier", async () => { @@ -41,7 +42,7 @@ test.describe("Supplier Allocation Tests", () => { ); }); - test("Verify that supplier allocator emits a rejected request for an unknown letter variant", async () => { + test("Verify that supplier allocator places the event on a DLQ for an unknown letter variant", async () => { test.setTimeout(180_000); // 3 minutes for long running polling const domainId = randomUUID(); logger.info( @@ -55,15 +56,8 @@ test.describe("Supplier Allocation Tests", () => { { id: preparedEvent.id, message: preparedEvent }, ]); expect(response.Successful).toHaveLength(1); - const allocationDetails = - await pollSupplierAllocatorForAllocationDetails(domainId); - - const supplierId = allocationDetails?.supplierSpec?.supplierId; - - const status = allocationDetails?.allocationStatus?.status; - expect(supplierId).toBe("unknown"); - expect(status).toBe("REJECTED"); + await pollQueueForLetterEvent("supplier-allocator-dlq", domainId); }); test("Verify that supplier allocations are correctly updated only once for a volume group for multiple messages", async () => { diff --git a/tests/helpers/aws-queue-helper.ts b/tests/helpers/aws-queue-helper.ts new file mode 100644 index 000000000..9dc02bcce --- /dev/null +++ b/tests/helpers/aws-queue-helper.ts @@ -0,0 +1,43 @@ +import { + DeleteMessageCommand, + ReceiveMessageCommand, + SQSClient, +} from "@aws-sdk/client-sqs"; +import { PreparedEventSchema } from "lambdas/supplier-allocator/src/handler/types"; +import { + AWS_ACCOUNT_ID, + AWS_REGION, + envName, +} from "tests/constants/api-constants"; + +export async function pollQueueForLetterEvent( + queueName: string, + domainId: string, +) { + const queueUrl = `https://sqs.${AWS_REGION}.amazonaws.com/${AWS_ACCOUNT_ID}/nhs-${envName}-supapi-${queueName}`; + const client = new SQSClient({ region: AWS_REGION }); + let matchingMessage; + setTimeout(async () => { + do { + const response = await client.send( + new ReceiveMessageCommand({ + QueueUrl: queueUrl, + VisibilityTimeout: 0, + WaitTimeSeconds: 5, + }), + ); + + matchingMessage = (response.Messages || []).find( + (message) => + PreparedEventSchema.parse(message.Body).data.domainId === domainId, + ); + } while (!matchingMessage); + + await client.send( + new DeleteMessageCommand({ + QueueUrl: queueUrl, + ReceiptHandle: matchingMessage.ReceiptHandle, + }), + ); + }, 60_000); +} diff --git a/tests/package.json b/tests/package.json index d2fab44c1..11c61f804 100644 --- a/tests/package.json +++ b/tests/package.json @@ -6,6 +6,7 @@ "@aws-sdk/client-kinesis": "^3.964.0", "@aws-sdk/client-lambda": "^3.986.0", "@aws-sdk/client-sns": "^3.1044.0", + "@aws-sdk/client-sqs": "^3.1044.0", "@aws-sdk/lib-dynamodb": "^3.1044.0", "@nhsdigital/nhs-notify-event-schemas-letter-rendering": "^2.0.2", "@nhsdigital/notify-digital-letters-consumer-contracts": "^1.0.1", From 4b9e04e15226dd68bab7e739eeeb76baeabb4299 Mon Sep 17 00:00:00 2001 From: Steve Buxton Date: Mon, 10 Aug 2026 14:21:21 +0100 Subject: [PATCH 05/11] Missed some files --- lambdas/supplier-allocator/src/services/supplier-config.ts | 1 + package-lock.json | 1 + 2 files changed, 2 insertions(+) diff --git a/lambdas/supplier-allocator/src/services/supplier-config.ts b/lambdas/supplier-allocator/src/services/supplier-config.ts index 66084fa62..d14e35f8e 100644 --- a/lambdas/supplier-allocator/src/services/supplier-config.ts +++ b/lambdas/supplier-allocator/src/services/supplier-config.ts @@ -306,6 +306,7 @@ export async function filterPacksForLetter( if (violatedConstraints.length > 0) { deps.logger.info({ description: `Pack specification filtered out based on pageCount constraints`, + domainId: letterEvent.data.domainId, packSpecId, pageCount, violatedConstraints, diff --git a/package-lock.json b/package-lock.json index ec6854450..84aac6c08 100644 --- a/package-lock.json +++ b/package-lock.json @@ -24617,6 +24617,7 @@ "@aws-sdk/client-kinesis": "^3.964.0", "@aws-sdk/client-lambda": "^3.986.0", "@aws-sdk/client-sns": "^3.1044.0", + "@aws-sdk/client-sqs": "^3.1044.0", "@aws-sdk/lib-dynamodb": "^3.1044.0", "@nhsdigital/nhs-notify-event-schemas-letter-rendering": "^2.0.2", "@nhsdigital/notify-digital-letters-consumer-contracts": "^1.0.1", From 6d02cfe7650c1b117bd2111e034d9016cd71a600 Mon Sep 17 00:00:00 2001 From: Steve Buxton Date: Mon, 10 Aug 2026 14:33:52 +0100 Subject: [PATCH 06/11] Dependency fix --- package-lock.json | 1 + tests/package.json | 1 + 2 files changed, 2 insertions(+) diff --git a/package-lock.json b/package-lock.json index 84aac6c08..c2c3f6e7d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -24630,6 +24630,7 @@ "graphql": "^16.11.0", "graphql-tag": "^2.12.6", "md5": "^2.3.0", + "nhs-notify-supplier-api-allocate-letter": "^0.0.1", "nhs-notify-supplier-api-upsert-letter": "^0.0.1", "openapi-response-validator": "^12.1.3", "pino": "^10.3.0", diff --git a/tests/package.json b/tests/package.json index 11c61f804..2482f5228 100644 --- a/tests/package.json +++ b/tests/package.json @@ -19,6 +19,7 @@ "graphql": "^16.11.0", "graphql-tag": "^2.12.6", "md5": "^2.3.0", + "nhs-notify-supplier-api-allocate-letter": "^0.0.1", "nhs-notify-supplier-api-upsert-letter": "^0.0.1", "openapi-response-validator": "^12.1.3", "pino": "^10.3.0", From 1d609e5784a23943b3780e87316194c030eb2f5d Mon Sep 17 00:00:00 2001 From: Steve Buxton Date: Mon, 10 Aug 2026 14:54:47 +0100 Subject: [PATCH 07/11] Clean up imports Please enter the commit message for your changes. Lines starting --- .../allocation-tests/letter-allocation-rejected.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/component-tests/allocation-tests/letter-allocation-rejected.spec.ts b/tests/component-tests/allocation-tests/letter-allocation-rejected.spec.ts index 3e2898243..75b53a50c 100644 --- a/tests/component-tests/allocation-tests/letter-allocation-rejected.spec.ts +++ b/tests/component-tests/allocation-tests/letter-allocation-rejected.spec.ts @@ -3,7 +3,6 @@ import test, { expect } from "playwright/test"; import { PackErrorLog, getAllocationLog, - getAllocationLogForDomainId, getVariantsForAllocation, getVolumeGroupData, updateLetterVariantPackSpecs, From 4ecff9fc99ab8b4785334a44c1c50202f81499e2 Mon Sep 17 00:00:00 2001 From: Steve Buxton Date: Tue, 11 Aug 2026 10:34:09 +0100 Subject: [PATCH 08/11] Also place on DLQ for errors thrown from repo --- .../errors/missing-supplier-config-error.ts | 9 ++ internal/datastore/src/index.ts | 1 + .../src/supplier-config-repository.ts | 19 +++-- lambdas/supplier-allocator/README.md | 8 +- lambdas/supplier-allocator/jest.config.ts | 3 + ...ts => supplier-config-validation-error.ts} | 4 +- .../__tests__/allocate-handler.test.ts | 82 ++++++++++--------- .../__tests__/allocation-config.test.ts | 8 +- .../src/handler/allocate-handler.ts | 12 +-- .../src/handler/allocation-config.ts | 6 +- .../__tests__/supplier-config.test.ts | 30 +++---- .../src/services/supplier-config.ts | 18 ++-- tests/helpers/aws-queue-helper.ts | 76 ++++++++++++----- 13 files changed, 171 insertions(+), 105 deletions(-) create mode 100644 internal/datastore/src/errors/missing-supplier-config-error.ts rename lambdas/supplier-allocator/src/errors/{supplier-config-error.ts => supplier-config-validation-error.ts} (59%) diff --git a/internal/datastore/src/errors/missing-supplier-config-error.ts b/internal/datastore/src/errors/missing-supplier-config-error.ts new file mode 100644 index 000000000..2dac18d4a --- /dev/null +++ b/internal/datastore/src/errors/missing-supplier-config-error.ts @@ -0,0 +1,9 @@ +/** + * Error thrown when a supplier cannot be allocated due to missing supplier config + */ +export default class MissingSupplierConfigError extends Error { + constructor(public readonly message: string) { + super(message); + this.name = "MissingSupplierConfigError"; + } +} diff --git a/internal/datastore/src/index.ts b/internal/datastore/src/index.ts index 10e255fbb..48e963112 100644 --- a/internal/datastore/src/index.ts +++ b/internal/datastore/src/index.ts @@ -9,3 +9,4 @@ export { default as DBHealthcheck } from "./healthcheck"; export { default as LetterAlreadyExistsError } from "./errors/letter-already-exists-error"; export { default as LetterNotFoundError } from "./errors/letter-not-found-error"; export { default as MiNotFoundError } from "./errors/mi-not-found-error"; +export { default as MissingSupplierConfigError } from "./errors/missing-supplier-config-error"; diff --git a/internal/datastore/src/supplier-config-repository.ts b/internal/datastore/src/supplier-config-repository.ts index 0d0492bab..0a8ecf6fc 100644 --- a/internal/datastore/src/supplier-config-repository.ts +++ b/internal/datastore/src/supplier-config-repository.ts @@ -19,6 +19,7 @@ import { VolumeGroup, } from "@nhsdigital/nhs-notify-event-schemas-supplier-config"; import { SupplierConfigEntity } from "./types"; +import MissingSupplierConfigError from "./errors/missing-supplier-config-error"; export type SupplierConfigRepositoryConfig = { supplierConfigTableName: string; @@ -40,7 +41,9 @@ export class SupplierConfigRepository { }), ); if (!result.Item) { - throw new Error(`No letter variant details found for id ${variantId}`); + throw new MissingSupplierConfigError( + `No letter variant details found for id ${variantId}`, + ); } return $LetterVariant.parse(result.Item); @@ -54,7 +57,9 @@ export class SupplierConfigRepository { }), ); if (!result.Item) { - throw new Error(`No volume group details found for id ${groupId}`); + throw new MissingSupplierConfigError( + `No volume group details found for id ${groupId}`, + ); } return $VolumeGroup.parse(result.Item); } @@ -81,7 +86,7 @@ export class SupplierConfigRepository { }), ); if (!result.Items || result.Items.length === 0) { - throw new Error( + throw new MissingSupplierConfigError( `No active supplier allocations found for volume group id ${groupId}`, ); } @@ -99,7 +104,9 @@ export class SupplierConfigRepository { }), ); if (!result.Item) { - throw new Error(`Supplier with id ${supplierId} not found`); + throw new MissingSupplierConfigError( + `Supplier with id ${supplierId} not found`, + ); } suppliers.push($Supplier.parse(result.Item)); } @@ -140,7 +147,9 @@ export class SupplierConfigRepository { }), ); if (!result.Item) { - throw new Error(`No pack specification found for id ${packSpecId}`); + throw new MissingSupplierConfigError( + `No pack specification found for id ${packSpecId}`, + ); } return $PackSpecification.parse(result.Item); } diff --git a/lambdas/supplier-allocator/README.md b/lambdas/supplier-allocator/README.md index 2802dd587..94d3b3bd9 100644 --- a/lambdas/supplier-allocator/README.md +++ b/lambdas/supplier-allocator/README.md @@ -13,13 +13,13 @@ Consumes `LetterRequestPrepared` events (v1 and v2) from an SQS queue, chooses a 3. The allocator loads the relevant supplier configuration from `SUPPLIER_CONFIG_TABLE`, including the letter variant, active volume group, candidate suppliers, and compatible pack details. 4. Candidate suppliers are filtered using pack support and daily capacity, then ranked using quota data from `SUPPLIER_QUOTAS_TABLE`. 5. On success, the handler produces an allocation with `allocationStatus.status = "PENDING"` and sends `{ letterEvent, allocationDetails }` to `UPSERT_LETTERS_QUEUE_URL`. -6. If a `SupplierConfigError` is raised, the original record is sent directly to `SUPPLIER_ALLOCATOR_DLQ_URL` and acknowledged so it is not retried. +6. If a `SupplierConfigValidationError` is raised, the original record is sent directly to `SUPPLIER_ALLOCATOR_DLQ_URL` and acknowledged so it is not retried. 7. Any other processing error is returned in `batchItemFailures` so SQS retries the record based on the source queue redrive policy. 8. After the batch completes, allocation counters are written back to `SUPPLIER_QUOTAS_TABLE`. ## Key Integration Points -- **SQS**: Input from EventSub, output to the upsert-letter queue (`UPSERT_LETTERS_QUEUE`), and direct publish to the allocator DLQ (`SUPPLIER_ALLOCATOR_DLQ_URL`) for `SupplierConfigError`. +- **SQS**: Input from EventSub, output to the upsert-letter queue (`UPSERT_LETTERS_QUEUE`), and direct publish to the allocator DLQ (`SUPPLIER_ALLOCATOR_DLQ_URL`) for `SupplierConfigValidationError`. - **`SupplierConfigRepository`** from `@internal/datastore` (`SUPPLIER_CONFIG_TABLE`): reads letter variants, volume groups, supplier allocations, pack specifications, and supplier packs. - **`SupplierQuotasRepository`** from `@internal/datastore` (`SUPPLIER_QUOTAS_TABLE`): reads and writes daily and overall allocation counts per volume group and supplier. - **Event schemas**: `@nhsdigital/nhs-notify-event-schemas-letter-rendering` (v2) and `@nhsdigital/nhs-notify-event-schemas-letter-rendering-v1` (v1). @@ -27,8 +27,8 @@ Consumes `LetterRequestPrepared` events (v1 and v2) from an SQS queue, chooses a ## Nuances and Peculiarities -- **`SupplierConfigError` is treated as terminal for retries.** The handler sends the original message directly to the allocator DLQ and acknowledges the source record. -- **All other failures retain normal retry semantics.** Non-`SupplierConfigError` records are returned in `batchItemFailures` and retried according to queue configuration. +- **`SupplierConfigValidationError` is treated as terminal for retries.** The handler sends the original message directly to the allocator DLQ and acknowledges the source record. +- **All other failures retain normal retry semantics.** Non-`SupplierConfigValidationError` records are returned in `batchItemFailures` and retried according to queue configuration. - **The factor algorithm is a running weighted average across the lifetime of the system, not per-batch.** The `overallAllocation` table accumulates counts since deployment. A supplier that handled a disproportionate share yesterday will have a high factor today and be deprioritised, allowing others to catch up to their target percentage. - **Daily capacity check uses London timezone.** `format(toZonedTime(new Date(), "Europe/London"), "yyyy-MM-dd")` determines the date key. Capacity resets at midnight London time, not UTC. - **Quota updates happen after the entire batch completes**, not per-record. This optimisation means concurrent Lambda invocations can transiently over-allocate a supplier before quotas are reconciled. diff --git a/lambdas/supplier-allocator/jest.config.ts b/lambdas/supplier-allocator/jest.config.ts index 872794514..174e7f7f9 100644 --- a/lambdas/supplier-allocator/jest.config.ts +++ b/lambdas/supplier-allocator/jest.config.ts @@ -9,6 +9,9 @@ export const baseJestConfig = { }, ], }, + transformIgnorePatterns: [ + "node_modules/(?!(@nhsdigital/nhs-notify-event-schemas-supplier-config)/)", + ], // Automatically clear mock calls, instances, contexts and results before every test clearMocks: true, diff --git a/lambdas/supplier-allocator/src/errors/supplier-config-error.ts b/lambdas/supplier-allocator/src/errors/supplier-config-validation-error.ts similarity index 59% rename from lambdas/supplier-allocator/src/errors/supplier-config-error.ts rename to lambdas/supplier-allocator/src/errors/supplier-config-validation-error.ts index 90abdfefb..8bde4b622 100644 --- a/lambdas/supplier-allocator/src/errors/supplier-config-error.ts +++ b/lambdas/supplier-allocator/src/errors/supplier-config-validation-error.ts @@ -1,9 +1,9 @@ /** * Error thrown when a supplier cannot be allocated due to incorrect supplier config */ -export default class SupplierConfigError extends Error { +export default class SupplierConfigValidationError extends Error { constructor(public readonly message: string) { super(message); - this.name = "SupplierConfigError"; + this.name = "SupplierConfigValidationError"; } } diff --git a/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts b/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts index adfe6a27e..cdfc6d378 100644 --- a/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts +++ b/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts @@ -8,12 +8,13 @@ import { LetterStatusChangeEvent, } from "@nhsdigital/nhs-notify-event-schemas-supplier-api/src/events/letter-events"; import { makeIdempotent } from "@aws-lambda-powertools/idempotency"; +import { MissingSupplierConfigError } from "@internal/datastore"; import createSupplierAllocatorHandler from "../allocate-handler"; import * as supplierConfig from "../../services/supplier-config"; import * as supplierQuotas from "../../services/supplier-quotas"; import * as allocationConfig from "../allocation-config"; import { Deps } from "../../config/deps"; -import SupplierConfigError from "../../errors/supplier-config-error"; +import SupplierConfigValidationError from "../../errors/supplier-config-validation-error"; import packageJson from "../../../package.json"; const renderingSchemaVersion: string = @@ -519,7 +520,7 @@ describe("createSupplierAllocatorHandler", () => { const throwAny = (mock: jest.Mock) => mock.mockRejectedValueOnce("anything that is not an Error"); - const nonSupplierConfigErrorCases = [ + const nonSupplierConfigValidationErrorCases = [ { name: "getVolumeGroupDetails", errorMessage: "Volume group retrieval failed", @@ -582,8 +583,8 @@ describe("createSupplierAllocatorHandler", () => { }, ]; - test.each(nonSupplierConfigErrorCases)( - "returns batch failure when %s rejects with a non-SupplierConfigError", + test.each(nonSupplierConfigValidationErrorCases)( + "returns batch failure when %s rejects with a non-SupplierConfigValidationError", async ({ errorMessage, setup }) => { const preparedEvent = createPreparedV2Event(); const evt: SQSEvent = createSQSEvent([ @@ -645,39 +646,44 @@ describe("createSupplierAllocatorHandler", () => { expect(sendCall.input.MessageBody).toBe(JSON.stringify(preparedEvent)); }); - it("places the record on the DLQ when supplier config retrieval fails", async () => { - const preparedEvent = createPreparedV2Event(); - - const evt: SQSEvent = createSQSEvent([ - createSqsRecord("msg1", JSON.stringify(preparedEvent)), - ]); - - const configError = new SupplierConfigError( - "Failed to retrieve supplier config", - ); - (supplierConfig.getVariantDetails as jest.Mock).mockRejectedValueOnce( - configError, - ); - - const handler = createSupplierAllocatorHandler(mockedDeps); - const result = await handler(evt, {} as any, {} as any); - if (!result) throw new Error("expected BatchResponse, got void"); - expect((mockedDeps.logger.error as jest.Mock).mock.calls).toHaveLength(1); - expect((mockedDeps.logger.error as jest.Mock).mock.calls[0][0]).toEqual( - expect.objectContaining({ - description: "Error processing allocation of record", - err: configError, - }), - ); - expect(result.batchItemFailures).toHaveLength(0); - expect(mockedDeps.sqsClient.send).toHaveBeenCalledTimes(1); - const sendCall = (mockedDeps.sqsClient.send as jest.Mock).mock - .calls[0][0]; - expect(sendCall.input.QueueUrl).toBe( - "https://sqs.test.queue/supplier-allocator-dlq", - ); - expect(sendCall.input.MessageBody).toBe(JSON.stringify(preparedEvent)); - }); + test.each([ + new SupplierConfigValidationError("Failed to retrieve supplier config"), + new MissingSupplierConfigError("Failed to retrieve supplier config"), + ])( + "places the record on the DLQ when supplier config retrieval fails", + async (configError: Error) => { + const preparedEvent = createPreparedV2Event(); + + const evt: SQSEvent = createSQSEvent([ + createSqsRecord("msg1", JSON.stringify(preparedEvent)), + ]); + + (supplierConfig.getVariantDetails as jest.Mock).mockRejectedValueOnce( + configError, + ); + + const handler = createSupplierAllocatorHandler(mockedDeps); + const result = await handler(evt, {} as any, {} as any); + if (!result) throw new Error("expected BatchResponse, got void"); + expect((mockedDeps.logger.error as jest.Mock).mock.calls).toHaveLength( + 1, + ); + expect((mockedDeps.logger.error as jest.Mock).mock.calls[0][0]).toEqual( + expect.objectContaining({ + description: "Error processing allocation of record", + err: configError, + }), + ); + expect(result.batchItemFailures).toHaveLength(0); + expect(mockedDeps.sqsClient.send).toHaveBeenCalledTimes(1); + const sendCall = (mockedDeps.sqsClient.send as jest.Mock).mock + .calls[0][0]; + expect(sendCall.input.QueueUrl).toBe( + "https://sqs.test.queue/supplier-allocator-dlq", + ); + expect(sendCall.input.MessageBody).toBe(JSON.stringify(preparedEvent)); + }, + ); it("returns batch failure when DLQ not configured", async () => { const preparedEvent = createPreparedV2Event(); @@ -685,7 +691,7 @@ describe("createSupplierAllocatorHandler", () => { createSqsRecord("msg1", JSON.stringify(preparedEvent)), ]); - const configError = new SupplierConfigError( + const configError = new SupplierConfigValidationError( "Failed to retrieve supplier config", ); (supplierConfig.getVariantDetails as jest.Mock).mockRejectedValueOnce( diff --git a/lambdas/supplier-allocator/src/handler/__tests__/allocation-config.test.ts b/lambdas/supplier-allocator/src/handler/__tests__/allocation-config.test.ts index 773ece53a..eca009e65 100644 --- a/lambdas/supplier-allocator/src/handler/__tests__/allocation-config.test.ts +++ b/lambdas/supplier-allocator/src/handler/__tests__/allocation-config.test.ts @@ -21,13 +21,13 @@ import * as supplierQuotasService from "../../services/supplier-quotas"; jest.mock("../../services/supplier-config"); jest.mock("../../services/supplier-quotas"); -async function expectSupplierConfigError( +async function expectSupplierConfigValidationError( promise: Promise, message: string | RegExp, ): Promise { await expect(promise).rejects.toThrow(message); await expect(promise).rejects.toMatchObject({ - name: "SupplierConfigError", + name: "SupplierConfigValidationError", }); } describe("eligibleSuppliers", () => { @@ -979,7 +979,7 @@ describe("selectSupplierByFactor", () => { } as SupplierAllocation, ]; - await expectSupplierConfigError( + await expectSupplierConfigValidationError( selectSupplierByFactor( mockSuppliers, zeroAllocations, @@ -1164,7 +1164,7 @@ describe("selectSupplierByFactor", () => { supplierQuotasService.calculateSupplierAllocatedFactor as jest.Mock ).mockResolvedValue([]); - await expectSupplierConfigError( + await expectSupplierConfigValidationError( selectSupplierByFactor( mockSuppliers, mockSupplierAllocations, diff --git a/lambdas/supplier-allocator/src/handler/allocate-handler.ts b/lambdas/supplier-allocator/src/handler/allocate-handler.ts index b27a621ae..93c9109d5 100644 --- a/lambdas/supplier-allocator/src/handler/allocate-handler.ts +++ b/lambdas/supplier-allocator/src/handler/allocate-handler.ts @@ -23,6 +23,7 @@ import { IdempotencyConfig, makeIdempotent, } from "@aws-lambda-powertools/idempotency"; +import MissingSupplierConfigError from "@internal/datastore/src/errors/missing-supplier-config-error"; import { getVariantDetails, getVolumeGroupDetails, @@ -37,7 +38,7 @@ import { } from "./allocation-config"; import { Deps } from "../config/deps"; import { PreparedEventSchema, PreparedEvents, SupplierDetails } from "./types"; -import SupplierConfigError from "../errors/supplier-config-error"; +import SupplierConfigValidationError from "../errors/supplier-config-validation-error"; const idempotencyConfig = new IdempotencyConfig({ eventKeyJmesPath: "data.domainId", @@ -115,7 +116,7 @@ async function getSupplierFromConfig( ); if (allSuppliersForPack.length === 0) { - throw new SupplierConfigError( + throw new SupplierConfigValidationError( `No suppliers found for pack specification ${preferredPack.id}`, ); } @@ -265,7 +266,6 @@ async function processSupplierAllocation( letterEvent: PreparedEvents, deps: Deps, perAllocationSuccess: AllocationMetrics, - perAllocationFailure: AllocationMetrics, volumeGroupAllocations: VolumeGroupAllocation, ): Promise { const supplierDetails: SupplierDetails = await getSupplierFromConfig( @@ -353,7 +353,6 @@ export default function createSupplierAllocatorHandler(deps: Deps): SQSHandler { letterEvent, depsInner, perAllocationSuccess, - perAllocationFailure, volumeGroupAllocations, ), { @@ -403,7 +402,10 @@ export default function createSupplierAllocatorHandler(deps: Deps): SQSHandler { message: record.body, }); incrementMetric(perAllocationFailure, supplier, priority); - if (error instanceof SupplierConfigError) { + if ( + error instanceof SupplierConfigValidationError || + error instanceof MissingSupplierConfigError + ) { try { await placeOnDeadLetterQueue(record, deps); } catch (dlqError) { diff --git a/lambdas/supplier-allocator/src/handler/allocation-config.ts b/lambdas/supplier-allocator/src/handler/allocation-config.ts index 70291d76f..6a31b54dd 100644 --- a/lambdas/supplier-allocator/src/handler/allocation-config.ts +++ b/lambdas/supplier-allocator/src/handler/allocation-config.ts @@ -19,7 +19,7 @@ import { calculateSupplierAllocatedFactor } from "../services/supplier-quotas"; import { Deps } from "../config/deps"; import { PreparedEvents } from "./types"; -import SupplierConfigError from "../errors/supplier-config-error"; +import SupplierConfigValidationError from "../errors/supplier-config-validation-error"; export async function eligibleSuppliers( volumeGroup: VolumeGroup, @@ -164,7 +164,7 @@ export async function selectSupplierByFactor( return suppliers.some((supplier) => supplier.id === alloc.supplier); }); if (supplierAllocationsForPack.length === 0) { - throw new SupplierConfigError( + throw new SupplierConfigValidationError( "No valid supplier allocations found for suppliers with valid pack", ); } @@ -172,7 +172,7 @@ export async function selectSupplierByFactor( await calculateSupplierAllocatedFactor(supplierAllocationsForPack, deps); if (supplierFactors.length === 0) { - throw new SupplierConfigError( + throw new SupplierConfigValidationError( "No supplier factors could be calculated for allocation", ); } diff --git a/lambdas/supplier-allocator/src/services/__tests__/supplier-config.test.ts b/lambdas/supplier-allocator/src/services/__tests__/supplier-config.test.ts index b27d11fda..c3dfb4128 100644 --- a/lambdas/supplier-allocator/src/services/__tests__/supplier-config.test.ts +++ b/lambdas/supplier-allocator/src/services/__tests__/supplier-config.test.ts @@ -31,13 +31,13 @@ function makeDeps(overrides: Partial = {}): Deps { return { ...(base as Deps), ...overrides }; } -async function expectSupplierConfigError( +async function expectSupplierConfigValidationError( promise: Promise, message: string | RegExp, ): Promise { await expect(promise).rejects.toThrow(message); await expect(promise).rejects.toMatchObject({ - name: "SupplierConfigError", + name: "SupplierConfigValidationError", }); } @@ -98,7 +98,7 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue(group); - await expectSupplierConfigError( + await expectSupplierConfigValidationError( getVolumeGroupDetails("g2", deps), /not active/, ); @@ -113,7 +113,7 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue(group); - await expectSupplierConfigError( + await expectSupplierConfigValidationError( getVolumeGroupDetails("g3", deps), /not active/, ); @@ -133,7 +133,7 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue(group); - await expectSupplierConfigError( + await expectSupplierConfigValidationError( getVolumeGroupDetails("g3", deps), /not active/, ); @@ -196,7 +196,7 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue(allocations); - await expectSupplierConfigError( + await expectSupplierConfigValidationError( getSupplierAllocationsForVolumeGroup("g1", deps, "missing"), /No supplier allocations found/, ); @@ -232,7 +232,7 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue([]); - await expectSupplierConfigError( + await expectSupplierConfigValidationError( getSupplierDetails(supplierIds, deps), /No supplier details found/, ); @@ -308,7 +308,7 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue(suppliers); - await expectSupplierConfigError( + await expectSupplierConfigValidationError( getSupplierDetails(supplierIds, deps), /No active suppliers found/, ); @@ -376,7 +376,7 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue([]); - await expectSupplierConfigError( + await expectSupplierConfigValidationError( getPreferredSupplierPacks(["spec1"], suppliers, deps), /No preferred supplier packs found/, ); @@ -426,7 +426,7 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue(supplierPacks); - await expectSupplierConfigError( + await expectSupplierConfigValidationError( getPreferredSupplierPacks(["spec1"], suppliers, deps), /No preferred supplier packs found/, ); @@ -467,7 +467,7 @@ describe("supplier-config service", () => { .fn() .mockResolvedValue(packSpec); - await expectSupplierConfigError( + await expectSupplierConfigValidationError( getPackSpecification("spec2", deps), /not active/, ); @@ -519,7 +519,7 @@ describe("supplier-config service", () => { }, } as any; - await expectSupplierConfigError( + await expectSupplierConfigValidationError( filterPacksForLetter(letterEvent, ["spec1"], deps), "No eligible pack specifications found for letter variant id undefined and pack specification ids spec1", ); @@ -589,7 +589,7 @@ describe("supplier-config service", () => { }, } as any; - await expectSupplierConfigError( + await expectSupplierConfigValidationError( filterPacksForLetter(letterEvent, ["spec1"], deps), /No eligible pack specifications found/, ); @@ -630,7 +630,7 @@ describe("supplier-config service", () => { }, } as any; - await expectSupplierConfigError( + await expectSupplierConfigValidationError( filterPacksForLetter(letterEvent, ["spec1"], deps), /No eligible pack specifications found/, ); @@ -751,7 +751,7 @@ describe("supplier-config service", () => { }, } as any; - await expectSupplierConfigError( + await expectSupplierConfigValidationError( filterPacksForLetter(letterEvent, ["spec1"], deps), "Unsupported operator UNSUPPORTED_OP in pack specification constraints", ); diff --git a/lambdas/supplier-allocator/src/services/supplier-config.ts b/lambdas/supplier-allocator/src/services/supplier-config.ts index d14e35f8e..8ff81245e 100644 --- a/lambdas/supplier-allocator/src/services/supplier-config.ts +++ b/lambdas/supplier-allocator/src/services/supplier-config.ts @@ -9,7 +9,7 @@ import { import { Deps } from "../config/deps"; import { PreparedEvents } from "../handler/types"; -import SupplierConfigError from "../errors/supplier-config-error"; +import SupplierConfigValidationError from "../errors/supplier-config-validation-error"; export async function getVariantDetails( variantId: string, @@ -47,7 +47,7 @@ export async function getVolumeGroupDetails( startDate: groupDetails.startDate, endDate: groupDetails.endDate, }); - throw new SupplierConfigError( + throw new SupplierConfigValidationError( `Volume group with id ${groupId} is not active`, ); } @@ -71,7 +71,7 @@ export async function getSupplierAllocationsForVolumeGroup( groupId, supplierId, }); - throw new SupplierConfigError( + throw new SupplierConfigValidationError( `No supplier allocations found for variant supplier id ${supplierId} in volume group ${groupId}`, ); } @@ -93,7 +93,7 @@ export async function getSupplierDetails( description: "No supplier details found for supplier allocations", supplierIds, }); - throw new SupplierConfigError( + throw new SupplierConfigValidationError( `No supplier details found for supplier ids ${supplierIds.join(", ")}`, ); } @@ -116,7 +116,7 @@ export async function getSupplierDetails( description: "No active suppliers found for supplier allocations", supplierIds, }); - throw new SupplierConfigError( + throw new SupplierConfigValidationError( `No active suppliers found for supplier ids ${supplierIds.join(", ")}`, ); } @@ -148,7 +148,7 @@ export async function getPreferredSupplierPacks( packSpecificationIds, supplierIds: suppliers.map((s) => s.id), }); - throw new SupplierConfigError( + throw new SupplierConfigValidationError( `No preferred supplier packs found for pack specification ids ${packSpecificationIds.join(", ")} and suppliers ${suppliers.map((s) => s.id).join(", ")}`, ); } @@ -165,7 +165,7 @@ export async function getPackSpecification( packSpecId, status: packSpec.status, }); - throw new SupplierConfigError( + throw new SupplierConfigValidationError( `Pack specification with id ${packSpecId} is not active`, ); } @@ -208,7 +208,7 @@ function evaluateContraint( return actualValue <= constraintValue; } default: { - throw new SupplierConfigError( + throw new SupplierConfigValidationError( `Unsupported operator ${operator} in pack specification constraints`, ); } @@ -327,7 +327,7 @@ export async function filterPacksForLetter( letterVariantId: letterEvent.data.letterVariantId, packSpecificationIds, }); - throw new SupplierConfigError( + throw new SupplierConfigValidationError( `No eligible pack specifications found for letter variant id ${letterEvent.data.letterVariantId} and pack specification ids ${packSpecificationIds.join(", ")}`, ); } diff --git a/tests/helpers/aws-queue-helper.ts b/tests/helpers/aws-queue-helper.ts index 9dc02bcce..aed546e0b 100644 --- a/tests/helpers/aws-queue-helper.ts +++ b/tests/helpers/aws-queue-helper.ts @@ -1,5 +1,6 @@ import { DeleteMessageCommand, + Message, ReceiveMessageCommand, SQSClient, } from "@aws-sdk/client-sqs"; @@ -9,35 +10,70 @@ import { AWS_REGION, envName, } from "tests/constants/api-constants"; +import { setTimeout } from "node:timers/promises"; -export async function pollQueueForLetterEvent( - queueName: string, +function messageMatchesDomainId(message: Message, domainId: string): boolean { + const result = PreparedEventSchema.safeParse(JSON.parse(message.Body!)); + return result.success && result.data.data.domainId === domainId; +} + +async function doPoll( + client: SQSClient, + queueUrl: string, domainId: string, + options: { abortSignal: AbortSignal }, ) { - const queueUrl = `https://sqs.${AWS_REGION}.amazonaws.com/${AWS_ACCOUNT_ID}/nhs-${envName}-supapi-${queueName}`; - const client = new SQSClient({ region: AWS_REGION }); let matchingMessage; - setTimeout(async () => { - do { - const response = await client.send( - new ReceiveMessageCommand({ - QueueUrl: queueUrl, - VisibilityTimeout: 0, - WaitTimeSeconds: 5, - }), - ); - - matchingMessage = (response.Messages || []).find( - (message) => - PreparedEventSchema.parse(message.Body).data.domainId === domainId, - ); - } while (!matchingMessage); + do { + const response = await client.send( + new ReceiveMessageCommand({ + QueueUrl: queueUrl, + VisibilityTimeout: 0, + WaitTimeSeconds: 5, + }), + options, + ); + matchingMessage = (response.Messages || []).find((message) => + messageMatchesDomainId(message, domainId), + ); + } while (!matchingMessage && !options.abortSignal.aborted); + + if (matchingMessage) { await client.send( new DeleteMessageCommand({ QueueUrl: queueUrl, ReceiptHandle: matchingMessage.ReceiptHandle, }), + options, ); - }, 60_000); + } else { + throw new Error("Timed out polling queue"); + } +} + +export async function pollQueueForLetterEvent( + queueName: string, + domainId: string, +) { + const queueUrl = `https://sqs.${AWS_REGION}.amazonaws.com/${AWS_ACCOUNT_ID}/nhs-${envName}-supapi-${queueName}`; + const client = new SQSClient({ region: AWS_REGION }); + + const cancelTimeout = new AbortController(); + const cancelPolling = new AbortController(); + + const timeoutPromise = setTimeout(60_000, undefined, { + signal: cancelTimeout.signal, + }).then(() => { + cancelPolling.abort(); + throw new Error("Timed out polling queue"); + }); + + const pollPromise = doPoll(client, queueUrl, domainId, { + abortSignal: cancelPolling.signal, + }).finally(() => { + cancelTimeout.abort(); + }); + + await Promise.race([timeoutPromise, pollPromise]); } From f4879f7786b9ee825d219c02bcf9d333d5bf2d36 Mon Sep 17 00:00:00 2001 From: Steve Buxton Date: Tue, 11 Aug 2026 15:05:37 +0100 Subject: [PATCH 09/11] Address Copilot review comments --- lambdas/supplier-allocator/README.md | 8 ++++---- .../src/handler/allocate-handler.ts | 4 +--- tests/helpers/aws-queue-helper.ts | 12 ++++++++++-- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/lambdas/supplier-allocator/README.md b/lambdas/supplier-allocator/README.md index 94d3b3bd9..4b104c57d 100644 --- a/lambdas/supplier-allocator/README.md +++ b/lambdas/supplier-allocator/README.md @@ -13,13 +13,13 @@ Consumes `LetterRequestPrepared` events (v1 and v2) from an SQS queue, chooses a 3. The allocator loads the relevant supplier configuration from `SUPPLIER_CONFIG_TABLE`, including the letter variant, active volume group, candidate suppliers, and compatible pack details. 4. Candidate suppliers are filtered using pack support and daily capacity, then ranked using quota data from `SUPPLIER_QUOTAS_TABLE`. 5. On success, the handler produces an allocation with `allocationStatus.status = "PENDING"` and sends `{ letterEvent, allocationDetails }` to `UPSERT_LETTERS_QUEUE_URL`. -6. If a `SupplierConfigValidationError` is raised, the original record is sent directly to `SUPPLIER_ALLOCATOR_DLQ_URL` and acknowledged so it is not retried. +6. If a failure occurs due to missing or invalid supplier config, the original record is sent directly to `SUPPLIER_ALLOCATOR_DLQ_URL` and acknowledged so it is not retried. 7. Any other processing error is returned in `batchItemFailures` so SQS retries the record based on the source queue redrive policy. 8. After the batch completes, allocation counters are written back to `SUPPLIER_QUOTAS_TABLE`. ## Key Integration Points -- **SQS**: Input from EventSub, output to the upsert-letter queue (`UPSERT_LETTERS_QUEUE`), and direct publish to the allocator DLQ (`SUPPLIER_ALLOCATOR_DLQ_URL`) for `SupplierConfigValidationError`. +- **SQS**: Input from EventSub, output to the upsert-letter queue (`UPSERT_LETTERS_QUEUE`), and direct publish to the allocator DLQ (`SUPPLIER_ALLOCATOR_DLQ_URL`) for `SupplierConfigValidationError` or `MissingSupplierConfigError`. - **`SupplierConfigRepository`** from `@internal/datastore` (`SUPPLIER_CONFIG_TABLE`): reads letter variants, volume groups, supplier allocations, pack specifications, and supplier packs. - **`SupplierQuotasRepository`** from `@internal/datastore` (`SUPPLIER_QUOTAS_TABLE`): reads and writes daily and overall allocation counts per volume group and supplier. - **Event schemas**: `@nhsdigital/nhs-notify-event-schemas-letter-rendering` (v2) and `@nhsdigital/nhs-notify-event-schemas-letter-rendering-v1` (v1). @@ -27,8 +27,8 @@ Consumes `LetterRequestPrepared` events (v1 and v2) from an SQS queue, chooses a ## Nuances and Peculiarities -- **`SupplierConfigValidationError` is treated as terminal for retries.** The handler sends the original message directly to the allocator DLQ and acknowledges the source record. -- **All other failures retain normal retry semantics.** Non-`SupplierConfigValidationError` records are returned in `batchItemFailures` and retried according to queue configuration. +- **`SupplierConfigValidationError` and `MissingSupplierConfigError` are treated as terminal for retries.** The handler sends the original message directly to the allocator DLQ and acknowledges the source record. +- **All other failures retain normal retry semantics.** Records that fail for non-supplier-config-related reasons are returned in `batchItemFailures` and retried according to queue configuration. - **The factor algorithm is a running weighted average across the lifetime of the system, not per-batch.** The `overallAllocation` table accumulates counts since deployment. A supplier that handled a disproportionate share yesterday will have a high factor today and be deprioritised, allowing others to catch up to their target percentage. - **Daily capacity check uses London timezone.** `format(toZonedTime(new Date(), "Europe/London"), "yyyy-MM-dd")` determines the date key. Capacity resets at midnight London time, not UTC. - **Quota updates happen after the entire batch completes**, not per-record. This optimisation means concurrent Lambda invocations can transiently over-allocate a supplier before quotas are reconciled. diff --git a/lambdas/supplier-allocator/src/handler/allocate-handler.ts b/lambdas/supplier-allocator/src/handler/allocate-handler.ts index 93c9109d5..c0c6f0c30 100644 --- a/lambdas/supplier-allocator/src/handler/allocate-handler.ts +++ b/lambdas/supplier-allocator/src/handler/allocate-handler.ts @@ -23,7 +23,7 @@ import { IdempotencyConfig, makeIdempotent, } from "@aws-lambda-powertools/idempotency"; -import MissingSupplierConfigError from "@internal/datastore/src/errors/missing-supplier-config-error"; +import { MissingSupplierConfigError } from "@internal/datastore"; import { getVariantDetails, getVolumeGroupDetails, @@ -344,7 +344,6 @@ async function placeOnDeadLetterQueue(record: SQSRecord, deps: Deps) { export default function createSupplierAllocatorHandler(deps: Deps): SQSHandler { const createGetSupplierIdempotently = ( perAllocationSuccess: AllocationMetrics, - perAllocationFailure: AllocationMetrics, volumeGroupAllocations: VolumeGroupAllocation, ) => { return makeIdempotent( @@ -370,7 +369,6 @@ export default function createSupplierAllocatorHandler(deps: Deps): SQSHandler { // create an idempotent function bound to this handler's global variables to track metrics and allocations const getSupplierIdempotently = createGetSupplierIdempotently( perAllocationSuccess, - perAllocationFailure, volumeGroupAllocations, ); diff --git a/tests/helpers/aws-queue-helper.ts b/tests/helpers/aws-queue-helper.ts index aed546e0b..6c75347db 100644 --- a/tests/helpers/aws-queue-helper.ts +++ b/tests/helpers/aws-queue-helper.ts @@ -13,8 +13,16 @@ import { import { setTimeout } from "node:timers/promises"; function messageMatchesDomainId(message: Message, domainId: string): boolean { - const result = PreparedEventSchema.safeParse(JSON.parse(message.Body!)); - return result.success && result.data.data.domainId === domainId; + if (!message.Body) { + return false; + } + try { + const letterEvent = PreparedEventSchema.parse(JSON.parse(message.Body)); + return letterEvent.data.domainId === domainId; + } catch { + // Allow for (and ignore) malformed messages on DLQ + return false; + } } async function doPoll( From a2febfc753cc6d55f11b69c5f6c394072c76a329 Mon Sep 17 00:00:00 2001 From: Steve Buxton Date: Fri, 14 Aug 2026 13:35:15 +0100 Subject: [PATCH 10/11] Reject letters that violate constraints for all available supplier packs --- lambdas/supplier-allocator/README.md | 8 +- .../src/errors/rejected-error.ts | 9 ++ .../__tests__/allocate-handler.test.ts | 41 ++++++ .../src/handler/allocate-handler.ts | 134 +++++++++++------- .../__tests__/supplier-config.test.ts | 16 ++- .../src/services/supplier-config.ts | 3 +- 6 files changed, 149 insertions(+), 62 deletions(-) create mode 100644 lambdas/supplier-allocator/src/errors/rejected-error.ts diff --git a/lambdas/supplier-allocator/README.md b/lambdas/supplier-allocator/README.md index 4b104c57d..a11189ae9 100644 --- a/lambdas/supplier-allocator/README.md +++ b/lambdas/supplier-allocator/README.md @@ -14,8 +14,9 @@ Consumes `LetterRequestPrepared` events (v1 and v2) from an SQS queue, chooses a 4. Candidate suppliers are filtered using pack support and daily capacity, then ranked using quota data from `SUPPLIER_QUOTAS_TABLE`. 5. On success, the handler produces an allocation with `allocationStatus.status = "PENDING"` and sends `{ letterEvent, allocationDetails }` to `UPSERT_LETTERS_QUEUE_URL`. 6. If a failure occurs due to missing or invalid supplier config, the original record is sent directly to `SUPPLIER_ALLOCATOR_DLQ_URL` and acknowledged so it is not retried. -7. Any other processing error is returned in `batchItemFailures` so SQS retries the record based on the source queue redrive policy. -8. After the batch completes, allocation counters are written back to `SUPPLIER_QUOTAS_TABLE`. +7. If supplier config is valid, but the letter request violates the contraints of all the available supplier packs (for example, too many pages), the handler produces a REJECTED allocation with a failure reason instead of dropping the message. +8. Any other processing error is returned in `batchItemFailures` so SQS retries the record based on the source queue redrive policy. +9. After the batch completes, allocation counters are written back to `SUPPLIER_QUOTAS_TABLE`. ## Key Integration Points @@ -23,11 +24,12 @@ Consumes `LetterRequestPrepared` events (v1 and v2) from an SQS queue, chooses a - **`SupplierConfigRepository`** from `@internal/datastore` (`SUPPLIER_CONFIG_TABLE`): reads letter variants, volume groups, supplier allocations, pack specifications, and supplier packs. - **`SupplierQuotasRepository`** from `@internal/datastore` (`SUPPLIER_QUOTAS_TABLE`): reads and writes daily and overall allocation counts per volume group and supplier. - **Event schemas**: `@nhsdigital/nhs-notify-event-schemas-letter-rendering` (v2) and `@nhsdigital/nhs-notify-event-schemas-letter-rendering-v1` (v1). -- **Downstream consumer**: `upsert-letter` receives `{ letterEvent, allocationDetails }` and persists PENDING letters. +- **Downstream consumer**: `upsert-letter` receives `{ letterEvent, allocationDetails }` and persists either PENDING or REJECTED letters. ## Nuances and Peculiarities - **`SupplierConfigValidationError` and `MissingSupplierConfigError` are treated as terminal for retries.** The handler sends the original message directly to the allocator DLQ and acknowledges the source record. +- **Otherwise failed allocations produce REJECTED letters, not dropped messages.** If no valid supplier packs are found, the handler sends a message to the upsert queue with `allocationStatus.status = "REJECTED"` and `supplierId = "unknown"`. - **All other failures retain normal retry semantics.** Records that fail for non-supplier-config-related reasons are returned in `batchItemFailures` and retried according to queue configuration. - **The factor algorithm is a running weighted average across the lifetime of the system, not per-batch.** The `overallAllocation` table accumulates counts since deployment. A supplier that handled a disproportionate share yesterday will have a high factor today and be deprioritised, allowing others to catch up to their target percentage. - **Daily capacity check uses London timezone.** `format(toZonedTime(new Date(), "Europe/London"), "yyyy-MM-dd")` determines the date key. Capacity resets at midnight London time, not UTC. diff --git a/lambdas/supplier-allocator/src/errors/rejected-error.ts b/lambdas/supplier-allocator/src/errors/rejected-error.ts new file mode 100644 index 000000000..022bade8c --- /dev/null +++ b/lambdas/supplier-allocator/src/errors/rejected-error.ts @@ -0,0 +1,9 @@ +/** + * Error thrown when a letter is rejected due to violating pack constraints for all possible supplier packs. + */ +export default class RejectedError extends Error { + constructor(public readonly message: string) { + super(message); + this.name = "RejectedError"; + } +} diff --git a/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts b/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts index cdfc6d378..ba541c3ec 100644 --- a/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts +++ b/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts @@ -16,6 +16,7 @@ import * as allocationConfig from "../allocation-config"; import { Deps } from "../../config/deps"; import SupplierConfigValidationError from "../../errors/supplier-config-validation-error"; import packageJson from "../../../package.json"; +import RejectedError from "../../errors/rejected-error"; const renderingSchemaVersion: string = packageJson.dependencies[ @@ -611,6 +612,46 @@ describe("createSupplierAllocatorHandler", () => { }, ); + it("rejects the letter when a RejectedError is thrown", async () => { + const preparedEvent = createPreparedV2Event(); + const evt: SQSEvent = createSQSEvent([ + createSqsRecord("msg1", JSON.stringify(preparedEvent)), + ]); + + setupDefaultMocks(); + (allocationConfig.preferredSupplierPack as jest.Mock).mockRejectedValueOnce( + new RejectedError("No eligible packs found"), + ); + + const handler = createSupplierAllocatorHandler(mockedDeps); + const result = await handler(evt, {} as any, {} as any); + if (!result) throw new Error("expected BatchResponse, got void"); + + expect((mockedDeps.logger.error as jest.Mock).mock.calls).toHaveLength(1); + expect((mockedDeps.logger.error as jest.Mock).mock.calls[0][0]).toEqual( + expect.objectContaining({ + description: "Letter request rejected", + }), + ); + expect(mockedDeps.sqsClient.send).toHaveBeenCalledTimes(1); + const sendCall = (mockedDeps.sqsClient.send as jest.Mock).mock.calls[0][0]; + expect(sendCall).toBeInstanceOf(SendMessageCommand); + + const messageBody = JSON.parse(sendCall.input.MessageBody); + expect(messageBody.letterEvent).toEqual(preparedEvent); + expect(messageBody.allocationDetails.supplierSpec).toEqual({ + supplierId: "unknown", + specId: "unknown", + priority: 0, + billingId: "unknown", + }); + expect(messageBody.allocationDetails.allocationStatus).toEqual({ + status: "REJECTED", + reasonCode: "NO_SUPPLIERS_AVAILABLE", + reasonText: "No eligible packs found", + }); + }); + describe("Dead letter queue", () => { it("places the record on the DLQ when no suppliers are found for pack specification", async () => { const preparedEvent = createPreparedV2Event(); diff --git a/lambdas/supplier-allocator/src/handler/allocate-handler.ts b/lambdas/supplier-allocator/src/handler/allocate-handler.ts index c0c6f0c30..a4dd059f0 100644 --- a/lambdas/supplier-allocator/src/handler/allocate-handler.ts +++ b/lambdas/supplier-allocator/src/handler/allocate-handler.ts @@ -39,6 +39,7 @@ import { import { Deps } from "../config/deps"; import { PreparedEventSchema, PreparedEvents, SupplierDetails } from "./types"; import SupplierConfigValidationError from "../errors/supplier-config-validation-error"; +import RejectedError from "../errors/rejected-error"; const idempotencyConfig = new IdempotencyConfig({ eventKeyJmesPath: "data.domainId", @@ -102,66 +103,89 @@ async function getSupplierFromConfig( const { supplierAllocations, suppliers: allocatedSuppliers } = await eligibleSuppliers(volumeGroup, deps, letterVariant.supplierId); - const preferredPack: PackSpecification = await preferredSupplierPack( - letterEvent, - allocatedSuppliers, - letterVariant.packSpecificationIds, - deps, - ); - - const allSuppliersForPack: Supplier[] = await suppliersWithValidPack( - allocatedSuppliers, - preferredPack.id, - deps, - ); - - if (allSuppliersForPack.length === 0) { - throw new SupplierConfigValidationError( - `No suppliers found for pack specification ${preferredPack.id}`, + try { + const preferredPack: PackSpecification = await preferredSupplierPack( + letterEvent, + allocatedSuppliers, + letterVariant.packSpecificationIds, + deps, ); - } - const suppliersForPackWithCapacity: Supplier[] = - await filterSuppliersWithCapacity(allSuppliersForPack, deps); - - // selected supplier id is determined by first calling selectSupplierByFactor for suppliers with capacity - // and if that returns nothing, try again with all suppliers for the pack - const selectedSupplierId = - (suppliersForPackWithCapacity.length > 0 - ? await selectSupplierByFactor( - suppliersForPackWithCapacity, - supplierAllocations, - letterEvent.data.domainId, - deps, - ) - : undefined) ?? - (await selectSupplierByFactor( - allSuppliersForPack, - supplierAllocations, - letterEvent.data.domainId, + const allSuppliersForPack: Supplier[] = await suppliersWithValidPack( + allocatedSuppliers, + preferredPack.id, deps, - )); + ); - deps.logger.info({ - description: "Fetched supplier details for supplier allocations", - domainId: letterEvent.data.domainId, - variantId: letterEvent.data.letterVariantId, - volumeGroupId: volumeGroup.id, - supplierAllocationIds: supplierAllocations.map((a) => a.id), - allocatedSuppliers, - allSuppliersForPack: allSuppliersForPack.map((s) => s.id), - suppliersForPackWithCapacity: suppliersForPackWithCapacity.map((s) => s.id), - selectedSupplierId, - }); + if (allSuppliersForPack.length === 0) { + throw new SupplierConfigValidationError( + `No suppliers found for pack specification ${preferredPack.id}`, + ); + } - return buildSupplierDetails( - selectedSupplierId, - preferredPack.id, - preferredPack.billingId, - letterVariant.priority, - "PENDING", - volumeGroup.id, - ); + const suppliersForPackWithCapacity: Supplier[] = + await filterSuppliersWithCapacity(allSuppliersForPack, deps); + + // selected supplier id is determined by first calling selectSupplierByFactor for suppliers with capacity + // and if that returns nothing, try again with all suppliers for the pack + const selectedSupplierId = + (suppliersForPackWithCapacity.length > 0 + ? await selectSupplierByFactor( + suppliersForPackWithCapacity, + supplierAllocations, + letterEvent.data.domainId, + deps, + ) + : undefined) ?? + (await selectSupplierByFactor( + allSuppliersForPack, + supplierAllocations, + letterEvent.data.domainId, + deps, + )); + + deps.logger.info({ + description: "Fetched supplier details for supplier allocations", + domainId: letterEvent.data.domainId, + variantId: letterEvent.data.letterVariantId, + volumeGroupId: volumeGroup.id, + supplierAllocationIds: supplierAllocations.map((a) => a.id), + allocatedSuppliers, + allSuppliersForPack: allSuppliersForPack.map((s) => s.id), + suppliersForPackWithCapacity: suppliersForPackWithCapacity.map( + (s) => s.id, + ), + selectedSupplierId, + }); + + return buildSupplierDetails( + selectedSupplierId, + preferredPack.id, + preferredPack.billingId, + letterVariant.priority, + "PENDING", + volumeGroup.id, + ); + } catch (error) { + if (error instanceof RejectedError) { + deps.logger.error({ + description: "Letter request rejected", + err: error, + variantId: letterEvent.data.letterVariantId, + }); + return buildSupplierDetails( + "unknown", + "unknown", + "unknown", + 0, + "REJECTED", + "unknown", + "NO_SUPPLIERS_AVAILABLE", + error.message, + ); + } + throw error; + } } type AllocationMetrics = Map>; diff --git a/lambdas/supplier-allocator/src/services/__tests__/supplier-config.test.ts b/lambdas/supplier-allocator/src/services/__tests__/supplier-config.test.ts index c3dfb4128..1bd5fb9c3 100644 --- a/lambdas/supplier-allocator/src/services/__tests__/supplier-config.test.ts +++ b/lambdas/supplier-allocator/src/services/__tests__/supplier-config.test.ts @@ -41,6 +41,16 @@ async function expectSupplierConfigValidationError( }); } +async function expectRejectedError( + promise: Promise, + message: string | RegExp, +): Promise { + await expect(promise).rejects.toThrow(message); + await expect(promise).rejects.toMatchObject({ + name: "RejectedError", + }); +} + describe("supplier-config service", () => { afterEach(() => jest.resetAllMocks()); @@ -519,7 +529,7 @@ describe("supplier-config service", () => { }, } as any; - await expectSupplierConfigValidationError( + await expectRejectedError( filterPacksForLetter(letterEvent, ["spec1"], deps), "No eligible pack specifications found for letter variant id undefined and pack specification ids spec1", ); @@ -589,7 +599,7 @@ describe("supplier-config service", () => { }, } as any; - await expectSupplierConfigValidationError( + await expectRejectedError( filterPacksForLetter(letterEvent, ["spec1"], deps), /No eligible pack specifications found/, ); @@ -630,7 +640,7 @@ describe("supplier-config service", () => { }, } as any; - await expectSupplierConfigValidationError( + await expectRejectedError( filterPacksForLetter(letterEvent, ["spec1"], deps), /No eligible pack specifications found/, ); diff --git a/lambdas/supplier-allocator/src/services/supplier-config.ts b/lambdas/supplier-allocator/src/services/supplier-config.ts index 8ff81245e..dcb71abc1 100644 --- a/lambdas/supplier-allocator/src/services/supplier-config.ts +++ b/lambdas/supplier-allocator/src/services/supplier-config.ts @@ -10,6 +10,7 @@ import { import { Deps } from "../config/deps"; import { PreparedEvents } from "../handler/types"; import SupplierConfigValidationError from "../errors/supplier-config-validation-error"; +import RejectedError from "../errors/rejected-error"; export async function getVariantDetails( variantId: string, @@ -327,7 +328,7 @@ export async function filterPacksForLetter( letterVariantId: letterEvent.data.letterVariantId, packSpecificationIds, }); - throw new SupplierConfigValidationError( + throw new RejectedError( `No eligible pack specifications found for letter variant id ${letterEvent.data.letterVariantId} and pack specification ids ${packSpecificationIds.join(", ")}`, ); } From fcd1a6e02781a54393e60d2bf4df21a60347e3a3 Mon Sep 17 00:00:00 2001 From: Steve Buxton Date: Fri, 14 Aug 2026 14:31:33 +0100 Subject: [PATCH 11/11] Fix test --- .../letter-allocation-rejected.spec.ts | 140 +++++++++--------- 1 file changed, 70 insertions(+), 70 deletions(-) diff --git a/tests/component-tests/allocation-tests/letter-allocation-rejected.spec.ts b/tests/component-tests/allocation-tests/letter-allocation-rejected.spec.ts index 75b53a50c..3220a348d 100644 --- a/tests/component-tests/allocation-tests/letter-allocation-rejected.spec.ts +++ b/tests/component-tests/allocation-tests/letter-allocation-rejected.spec.ts @@ -3,6 +3,7 @@ import test, { expect } from "playwright/test"; import { PackErrorLog, getAllocationLog, + getAllocationLogForDomainId, getVariantsForAllocation, getVolumeGroupData, updateLetterVariantPackSpecs, @@ -10,89 +11,88 @@ import { } from "tests/helpers/allocation-helper"; import { pollQueueForLetterEvent } from "tests/helpers/aws-queue-helper"; import { createPreparedV1Event } from "tests/helpers/event-fixtures"; +import { getLettersFromSupplierTable } from "tests/helpers/generate-fetch-test-data"; import { logger } from "tests/helpers/pino-logger"; import { sendSnsEvent } from "tests/helpers/send-sns-event"; test.describe("Allocator Rejected Allocation Tests", () => { test.setTimeout(180_000); // 3 minutes for long running polling - for (const { - domainIdName, - expectedError, - letterVariantMapping, - pageCount, - testCase, - testName, - } of [ - { - testCase: 1, - testName: - "Verify that the letters are placed on a DLQ when no pack specification is eligible", - letterVariantMapping: 1, - domainIdName: "NoEligiblePackSpecs", + + test("Verify that the letters are REJECTED when no pack specification is eligible", async () => { + const letterVariant = getVariantsForAllocation(1); + const domainId = `NoEligiblePackSpecs-${randomUUID()}`; + const preparedEvent = createPreparedV1Event({ + domainId, + letterVariantId: letterVariant, pageCount: 100, // high page count to ensure pack specifications are filtered out based on constraints - expectedError: "No eligible pack specifications found for letter", - }, - { - testCase: 2, - testName: - "Verify that the letters are placed on a DLQ when no supplier pack are found for selected pack", - letterVariantMapping: 6, - domainIdName: "NoSupplierPacksFound", - pageCount: 2, - expectedError: - "No preferred supplier packs found for pack specification ids and suppliers", - }, - { - testCase: 3, - testName: - "Verify that the letters are placed on a DLQ when no pack specification found for letter variant", - letterVariantMapping: 7, - domainIdName: "NoPackSpecificationFound", + }); + + const response = await sendSnsEvent(preparedEvent); + expect(response.MessageId).toBeTruthy(); + + const supplierAllocatorLog = await getAllocationLog( + "No eligible pack specifications found for letter", + ); + + const allocationLog = await getAllocationLogForDomainId(domainId); + const lettersInDb = await getLettersFromSupplierTable( + "unknown", + domainId, + "REJECTED", + ); + + expect(lettersInDb.status).toBe("REJECTED"); + expect(lettersInDb.supplierId).toBe( + allocationLog.msg?.allocationDetails?.supplierSpec?.supplierId, + ); + + const { packSpecificationIds } = supplierAllocatorLog; + expect(packSpecificationIds).toBeTruthy(); + }); + + test("Verify that the letters are placed on a DLQ when no supplier packs are found", async () => { + const letterVariant = getVariantsForAllocation(6); + const domainId = `NoSupplierPacksFound-${randomUUID()}`; + const preparedEvent = createPreparedV1Event({ + domainId, + letterVariantId: letterVariant, pageCount: 2, - expectedError: "No pack specification found for id", - }, - ]) { - test(testName, async () => { - const letterVariant = getVariantsForAllocation(letterVariantMapping); - const domainId = `${domainIdName}-${randomUUID()}`; - const preparedEvent = createPreparedV1Event({ - domainId, - letterVariantId: letterVariant, - pageCount, - }); + }); - if (letterVariantMapping === 7) { - await updateLetterVariantPackSpecs(letterVariant, [""]); - } + const response = await sendSnsEvent(preparedEvent); + expect(response.MessageId).toBeTruthy(); - const response = await sendSnsEvent(preparedEvent); - expect(response.MessageId).toBeTruthy(); + const supplierAllocatorLog = await getAllocationLog( + "No preferred supplier packs found for pack specification ids and suppliers", + ); - const supplierAllocatorLog = - await getAllocationLog(expectedError); + await pollQueueForLetterEvent("supplier-allocator-dlq", domainId); - await pollQueueForLetterEvent("supplier-allocator-dlq", domainId); + const { packSpecificationIds } = supplierAllocatorLog; + expect(packSpecificationIds).toBeTruthy(); + }); + + test("Verify that the letters are placed on a DLQ when no pack specification found for letter variant", async () => { + const letterVariant = getVariantsForAllocation(7); + const domainId = `NoPackSpecificationFound-${randomUUID()}`; - switch (testCase) { - case 1: - case 2: { - const { packSpecificationIds } = supplierAllocatorLog; - expect(packSpecificationIds).toBeTruthy(); - break; - } - - case 3: { - await updateLetterVariantPackSpecs(letterVariant, [ - "notify-c5-colour", - ]); // update back to valid config for other tests - break; - } - default: { - throw new Error(`Unexpected test case ${testCase}`); - } - } + await updateLetterVariantPackSpecs(letterVariant, [""]); + + const preparedEvent = createPreparedV1Event({ + domainId, + letterVariantId: letterVariant, + pageCount: 2, }); - } + + const response = await sendSnsEvent(preparedEvent); + expect(response.MessageId).toBeTruthy(); + + await getAllocationLog("No pack specification found for id"); + + await pollQueueForLetterEvent("supplier-allocator-dlq", domainId); + + await updateLetterVariantPackSpecs(letterVariant, ["notify-c5-colour"]); // update back to valid config for other tests + }); for (const { fieldToUpdate, testName, volumeGroupId } of [ {