diff --git a/cdk/src/constructs/github-screenshot-integration.ts b/cdk/src/constructs/github-screenshot-integration.ts index b48c70864..739e5a7c3 100644 --- a/cdk/src/constructs/github-screenshot-integration.ts +++ b/cdk/src/constructs/github-screenshot-integration.ts @@ -40,6 +40,15 @@ const PROCESSOR_DLQ_RETENTION_DAYS = 14; /** DLQ-depth alarm metric period (minutes). */ const DLQ_ALARM_PERIOD_MINUTES = 5; +/** Processor Lambda-Errors alarm metric period (minutes). */ +const ERROR_ALARM_PERIOD_MINUTES = 5; + +/** Processor Lambda-Errors alarm evaluation range (periods). Paired with + * `datapointsToAlarm: 1` below, this is 1-of-2, NOT 2-of-2: any single + * breaching period alarms. Do not remove `datapointsToAlarm` — CloudWatch + * would default it back to this value and a one-off crash would never fire. */ +const ERROR_ALARM_EVALUATION_PERIODS = 2; + /** Async screenshot-processor Lambda memory (MB). */ const PROCESSOR_MEMORY_MB = 512; @@ -128,6 +137,18 @@ export class GitHubScreenshotIntegration extends Construct { * "for operator inspection" that no operator is ever told to make. */ public readonly processorDlqDepthAlarm: cloudwatch.Alarm; + /** Fires when the processor Lambda's ``Errors`` metric breaches. The + * handler swallows its per-step operational failures (log + return), + * so this metric ticks only on faults that ESCAPE the handler — + * init-time crashes (missing env at cold start, bundling defect), + * unhandled throws in an unguarded path, or the 120s hard timeout. + * For that population it fires one evaluation window sooner than the + * DLQ-depth alarm (which waits for Lambda's async-retry ladder to land + * a message). Follows the ``metricErrors`` + 2-eval-period shape of + * ``task-orchestrator.ts`` OrchestratorErrorAlarm (with threshold 1, + * since any escaped error here is significant). */ + public readonly processorErrorAlarm: cloudwatch.Alarm; + constructor(scope: Construct, id: string, props: GitHubScreenshotIntegrationProps) { super(scope, id); @@ -219,6 +240,38 @@ export class GitHubScreenshotIntegration extends Construct { treatMissingData: cloudwatch.TreatMissingData.NOT_BREACHING, }); + // Alarm on the processor's Errors metric (#284 AC). Complementary to + // the DLQ-depth alarm above, not redundant. The handler is best-effort + // by design — it catches its per-step operational failures (bad token, + // AgentCore/S3/comment-post errors) and returns success, so those + // surface as tagged log events, NOT as Lambda Errors. This alarm + // therefore fires on the faults that ESCAPE the handler: an init-time + // crash (missing env at cold start, bundling defect), an unhandled + // throw in an unguarded path, or the 120s hard timeout. That is the + // same failure class the DLQ eventually catches, but before Lambda's async-retry + // ladder has landed a message on the DLQ. The Errors metric is stamped + // at invocation time, so this alarm fires no later than — and, when the + // retry ladder straddles a period boundary, one window before — the + // DLQ-depth alarm. threshold 1 / 1-of-2 eval periods matches the AC; + // the metricErrors shape follows ``task-orchestrator.ts`` + // OrchestratorErrorAlarm (which uses threshold 3 — we use 1 because any + // escaped error here is significant). + this.processorErrorAlarm = new cloudwatch.Alarm(this, 'WebhookProcessorErrorAlarm', { + metric: this.webhookProcessorFn.metricErrors({ + period: Duration.minutes(ERROR_ALARM_PERIOD_MINUTES), + }), + threshold: 1, + evaluationPeriods: ERROR_ALARM_EVALUATION_PERIODS, + // Fire on the FIRST breaching datapoint. Unset, this defaults to + // evaluationPeriods (2 consecutive), which for a sparse invocation + // metric means a single-event crash never alarms: its retries land + // in one period and NOT_BREACHING fills the neighbour. + datapointsToAlarm: 1, + alarmDescription: + 'Screenshot webhook processor Lambda errors breached threshold — deploy-preview screenshots are failing (check IAM, AgentCore Browser quota, and dependency health)', + treatMissingData: cloudwatch.TreatMissingData.NOT_BREACHING, + }); + this.screenshotBucket.bucket.grantPut(this.webhookProcessorFn); props.githubTokenSecret.grantRead(this.webhookProcessorFn); diff --git a/cdk/test/constructs/github-screenshot-integration.test.ts b/cdk/test/constructs/github-screenshot-integration.test.ts index 3e415c87a..d519da46b 100644 --- a/cdk/test/constructs/github-screenshot-integration.test.ts +++ b/cdk/test/constructs/github-screenshot-integration.test.ts @@ -70,7 +70,12 @@ describe('GitHubScreenshotIntegration construct', () => { // crashes reach the queue — each one means the screenshot pipeline is // silently down. Without the alarm the queue is "for operator // inspection" that no operator is ever told to make. - template.resourceCountIs('AWS::CloudWatch::Alarm', 1); + // + // Two alarms total: this DLQ-depth alarm (a record LANDED) and the + // Lambda-Errors alarm below (invocations are FAILING, before Lambda's + // async retries have exhausted onto the DLQ). They are complementary, + // not redundant — see the next test. + template.resourceCountIs('AWS::CloudWatch::Alarm', 2); template.hasResourceProperties('AWS::CloudWatch::Alarm', { MetricName: 'ApproximateNumberOfMessagesVisible', Namespace: 'AWS/SQS', @@ -90,6 +95,44 @@ describe('GitHubScreenshotIntegration construct', () => { }); }); + test('alarms on the processor Lambda Errors metric (>=1 in 5min, 2 eval periods)', () => { + // #284 AC: an alarm on the processor's Errors metric. This is DISTINCT + // from the DLQ-depth alarm above — the DLQ-depth alarm only fires once + // a failure has survived Lambda's async retry ladder and LANDED on the + // queue, whereas an Errors alarm fires as soon as invocations start + // failing, catching a systemic break (IAM regression, AgentCore quota + // exhaustion, dependency outage) at the first faulting invocation. + // Mirrors `task-orchestrator.ts` OrchestratorErrorAlarm's idiom. + template.hasResourceProperties('AWS::CloudWatch::Alarm', { + MetricName: 'Errors', + Namespace: 'AWS/Lambda', + // metricErrors() defaults to Sum — the correct statistic for an + // error COUNT (vs Average/Maximum). Pin it so a future edit that + // swaps the statistic can't silently change the alarm's semantics. + Statistic: 'Sum', + Period: 300, + Threshold: 1, + // >= 1, not > 1. Relies on a CDK default otherwise, and + // GreaterThanThreshold would silently require TWO errors to fire. + ComparisonOperator: 'GreaterThanOrEqualToThreshold', + EvaluationPeriods: 2, + // 1-of-2, not 2-of-2. Unset, DatapointsToAlarm defaults to + // EvaluationPeriods, and a single-event crash would never alarm + // (its Errors datapoints land in one period; NOT_BREACHING fills + // the neighbour). Pinned so that regression is caught here. + DatapointsToAlarm: 1, + TreatMissingData: 'notBreaching', + Dimensions: Match.arrayWith([ + Match.objectLike({ + Name: 'FunctionName', + Value: Match.objectLike({ + Ref: Match.stringLikeRegexp('WebhookProcessorFn'), + }), + }), + ]), + }); + }); + test('wires the DLQ as the processor Lambda async-invoke dead-letter target', () => { // The queue existing is not enough — it must be bound to the // processor function's DeadLetterConfig or failed async invokes diff --git a/docs/guides/DEPLOY_PREVIEW_SCREENSHOTS_GUIDE.md b/docs/guides/DEPLOY_PREVIEW_SCREENSHOTS_GUIDE.md index f3c4ff40f..71a315cd1 100644 --- a/docs/guides/DEPLOY_PREVIEW_SCREENSHOTS_GUIDE.md +++ b/docs/guides/DEPLOY_PREVIEW_SCREENSHOTS_GUIDE.md @@ -175,6 +175,35 @@ Then tail the function's CloudWatch log group. Common silent skips: - `skipped_no_url` — the `success` status didn't include `environment_url`. Some providers post URL-less success events; the next push usually carries the URL. - `No open PR found for SHA after retries` — the deploy provider built and reported faster than the agent could `gh pr create` (race window > 35s). Rare; redeliver the webhook from GitHub's UI to retry. +### No screenshots at all: check the processor alarms and DLQ + +The receiver Lambda async-invokes the processor (`InvocationType: Event`) and returns `200` to GitHub as soon as that invoke is accepted, so a *processor*-side fault never propagates back — GitHub sees success and never redelivers. (Only a failure to even enqueue the invoke returns `500`.) Two operator-visible signals catch a hard processor fault that would otherwise stop screenshots silently. + +**Important:** the processor handler is best-effort by design — it catches its own per-step operational failures (bad token, AgentCore/S3/comment-post errors) and returns success, so those show up as tagged log events (see the section above), **not** as Lambda `Errors`. Both alarms below fire only on faults that *escape* the handler: an init-time crash (missing env at cold start, bundling defect), an unhandled throw in an unguarded path, or the 120s hard timeout. For a swallowed operational failure (e.g. a revoked S3/AgentCore permission), watch the processor **logs**, not these alarms. + +- **`WebhookProcessorErrorAlarm`** — fires on the processor's Lambda `Errors` metric: **any** 5-minute period with `>= 1` error alarms (evaluation range 2 periods, 1 datapoint to alarm). Early signal: an invocation is faulting *now*. +- **`WebhookProcessorDlqDepthAlarm`** — fires when such a failed invocation has survived Lambda's built-in async retries and landed on the DLQ (construct id `WebhookProcessorDlq`; 14-day retention, SSL-enforced). Backstop: a payload is now parked undelivered. (The queue also gets SSE via SQS's service-side default; the construct doesn't set an explicit `encryption`.) + +The two catch the same failure class, but the `Errors` alarm fires no later than — and, when the retry ladder straddles a period boundary, one window before — the DLQ-depth alarm (the `Errors` metric is stamped at invocation time, before retries exhaust onto the DLQ). Find and inspect the DLQ — the construct sets no explicit queue name, so its physical name is CloudFormation-generated and *contains* the `WebhookProcessorDlq` construct id: + +```bash +# Find the DLQ URL (physical name contains the construct id) +aws sqs list-queues --region us-east-1 \ + --query "QueueUrls[?contains(@, 'WebhookProcessorDlq')]" --output text + +# How many failed invocations are parked? +aws sqs get-queue-attributes --region us-east-1 \ + --queue-url \ + --attribute-names ApproximateNumberOfMessages + +# Peek at a parked event (the original async-invoke payload + Lambda error context) +aws sqs receive-message --region us-east-1 \ + --queue-url --max-number-of-messages 1 \ + --visibility-timeout 0 +``` + +The message body is the original async-invoke event; the `RequestContext`/error attributes show why Lambda gave up. Fix the root cause (re-check the processor's IAM grants, AgentCore Browser quota, and the GitHub/Linear token secrets). The event source is GitHub, not the DLQ, so recovery is to **redeliver the webhook from GitHub's UI** (a Lambda async-invoke DLQ has no automatic re-invoke path — the parked messages are event copies for diagnosis). Purge the queue (`aws sqs purge-queue`) once the alarm has cleared and you no longer need the payloads. + ### Screenshot lands on GitHub PR but not on Linear The GitHub-side post is the primary path; Linear is opt-in and best-effort. Skipping the Linear post is normal if you don't have Linear configured. If you do, look for the processor log line `Linear identifier did not resolve to an issue` — usually means: diff --git a/docs/src/content/docs/using/Deploy-preview-screenshots-guide.md b/docs/src/content/docs/using/Deploy-preview-screenshots-guide.md index cd01304e3..36c49637f 100644 --- a/docs/src/content/docs/using/Deploy-preview-screenshots-guide.md +++ b/docs/src/content/docs/using/Deploy-preview-screenshots-guide.md @@ -179,6 +179,35 @@ Then tail the function's CloudWatch log group. Common silent skips: - `skipped_no_url` — the `success` status didn't include `environment_url`. Some providers post URL-less success events; the next push usually carries the URL. - `No open PR found for SHA after retries` — the deploy provider built and reported faster than the agent could `gh pr create` (race window > 35s). Rare; redeliver the webhook from GitHub's UI to retry. +### No screenshots at all: check the processor alarms and DLQ + +The receiver Lambda async-invokes the processor (`InvocationType: Event`) and returns `200` to GitHub as soon as that invoke is accepted, so a *processor*-side fault never propagates back — GitHub sees success and never redelivers. (Only a failure to even enqueue the invoke returns `500`.) Two operator-visible signals catch a hard processor fault that would otherwise stop screenshots silently. + +**Important:** the processor handler is best-effort by design — it catches its own per-step operational failures (bad token, AgentCore/S3/comment-post errors) and returns success, so those show up as tagged log events (see the section above), **not** as Lambda `Errors`. Both alarms below fire only on faults that *escape* the handler: an init-time crash (missing env at cold start, bundling defect), an unhandled throw in an unguarded path, or the 120s hard timeout. For a swallowed operational failure (e.g. a revoked S3/AgentCore permission), watch the processor **logs**, not these alarms. + +- **`WebhookProcessorErrorAlarm`** — fires on the processor's Lambda `Errors` metric: **any** 5-minute period with `>= 1` error alarms (evaluation range 2 periods, 1 datapoint to alarm). Early signal: an invocation is faulting *now*. +- **`WebhookProcessorDlqDepthAlarm`** — fires when such a failed invocation has survived Lambda's built-in async retries and landed on the DLQ (construct id `WebhookProcessorDlq`; 14-day retention, SSL-enforced). Backstop: a payload is now parked undelivered. (The queue also gets SSE via SQS's service-side default; the construct doesn't set an explicit `encryption`.) + +The two catch the same failure class, but the `Errors` alarm fires no later than — and, when the retry ladder straddles a period boundary, one window before — the DLQ-depth alarm (the `Errors` metric is stamped at invocation time, before retries exhaust onto the DLQ). Find and inspect the DLQ — the construct sets no explicit queue name, so its physical name is CloudFormation-generated and *contains* the `WebhookProcessorDlq` construct id: + +```bash +# Find the DLQ URL (physical name contains the construct id) +aws sqs list-queues --region us-east-1 \ + --query "QueueUrls[?contains(@, 'WebhookProcessorDlq')]" --output text + +# How many failed invocations are parked? +aws sqs get-queue-attributes --region us-east-1 \ + --queue-url \ + --attribute-names ApproximateNumberOfMessages + +# Peek at a parked event (the original async-invoke payload + Lambda error context) +aws sqs receive-message --region us-east-1 \ + --queue-url --max-number-of-messages 1 \ + --visibility-timeout 0 +``` + +The message body is the original async-invoke event; the `RequestContext`/error attributes show why Lambda gave up. Fix the root cause (re-check the processor's IAM grants, AgentCore Browser quota, and the GitHub/Linear token secrets). The event source is GitHub, not the DLQ, so recovery is to **redeliver the webhook from GitHub's UI** (a Lambda async-invoke DLQ has no automatic re-invoke path — the parked messages are event copies for diagnosis). Purge the queue (`aws sqs purge-queue`) once the alarm has cleared and you no longer need the payloads. + ### Screenshot lands on GitHub PR but not on Linear The GitHub-side post is the primary path; Linear is opt-in and best-effort. Skipping the Linear post is normal if you don't have Linear configured. If you do, look for the processor log line `Linear identifier did not resolve to an issue` — usually means: