From 02ed23ac0850167dc0563c4674ee94bd4948a8c3 Mon Sep 17 00:00:00 2001 From: Steve Buxton Date: Fri, 7 Aug 2026 15:52:21 +0100 Subject: [PATCH 1/2] 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 2/2] 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",