feat(infra): add Lambda-error alarm (+ SNS) to GitHub webhook processor (#284) - #674
feat(infra): add Lambda-error alarm (+ SNS) to GitHub webhook processor (#284)#674scottschreckengaust wants to merge 4 commits into
Conversation
8383d9a to
c510b26
Compare
#284 asked for an SQS DLQ + a CloudWatch alarm on the async screenshot processor. Most of it already landed on main (PR #241 follow-up): github-screenshot-integration.ts already defines WebhookProcessorDlq (14-day retention, enforceSSL), wires it as the Lambda async-invoke deadLetterQueue, carries the AwsSolutions-SQS3 suppression, and has a WebhookProcessorDlqDepthAlarm on the DLQ's ApproximateNumberOfMessages metric. So ACs 1 (DLQ onFailure), 3 (retention/SSE), and 4 (cdk-nag) were already satisfied. Two ACs were UNMET and are addressed here: - AC 2 — an alarm on the processor's Lambda **Errors** metric (>= 1 in 5min, 2 eval periods). This is DISTINCT from the existing DLQ-depth alarm: the depth alarm only fires once a failure has survived Lambda's async-retry ladder and landed on the queue, whereas the Errors alarm fires as soon as invocations start faulting — catching a systemic break (IAM regression, AgentCore quota exhaustion, OAuth-rotation issue, dependency outage) earlier. Mirrors the OrchestratorErrorAlarm idiom in task-orchestrator.ts (threshold, evaluationPeriods, treatMissingData=NOT_BREACHING). I did NOT duplicate the pre-existing DLQ-depth alarm. - AC 5 (docs) — DEPLOY_PREVIEW_SCREENSHOTS_GUIDE.md now has a Troubleshooting subsection explaining both alarms and how to inspect/drain the DLQ; Starlight mirror regenerated via docs:sync. SNS: AC #5 said "notify via existing alarm SNS topic if one exists, otherwise create one". There is NO SNS topic or addAlarmAction anywhere in cdk/src today — every existing alarm (FanOutConsumer.dlqDepthAlarm, OrchestratorErrorAlarm, the pre-existing WebhookProcessorDlqDepthAlarm) is a bare alarm with no notification action. To reuse-over-reinvent and stay consistent with the repo, this alarm is also bare; a stack-wide SNS notification plane is out of scope for a single construct. Closes #284 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
c510b26 to
98de40d
Compare
theagenticguy
left a comment
theagenticguy
left a comment
There was a problem hiding this comment.
Review: Request changes — one material defect, plus one scope question. The shape of this PR is right: the gap analysis is accurate (I confirmed AC 1/3/4 already landed on main), the delta is minimal, the test is real, and the SNS restraint is well argued. But the new alarm as configured does the opposite of what its own comment claims, in a way that will burn whoever is oncall.
Blocking: datapointsToAlarm is unset, so the alarm is 2-of-2 and cannot fire "sooner"
The PR asserts in three places (source comment, test comment, operator runbook) that the Errors alarm "trips one evaluation window sooner" than the DLQ-depth alarm. As configured it trips later or never. Two facts compound:
-
datapointsToAlarmis not set, and CloudWatch defaults it toevaluationPeriods: "If you omit this parameter, CloudWatch uses the same value here that you set for EvaluationPeriods, and the alarm goes to alarm state if that many consecutive periods are breaching." So this alarm needs two consecutive breaching 5-minute datapoints. The DLQ-depth alarm next to it is 1-of-1. -
Lambda
Errorsis an invocation metric: no invocation means no datapoint, not a zero. For exactly the failure class this PR names (init-time cold-start crash, bundling defect, one unguarded throw), the three attempts land at T, ~T+63s, ~T+190s — a ~3.2 minute span, usually inside a single 5-minute period. The neighbouring period has no datapoint,treatMissingData: NOT_BREACHINGfills it non-breaching, and the alarm staysOKforever. This is row 5 of the missing-data table (- - X - -):MISSING→ ALARM,BREACHING→ ALARM,NOT BREACHING→ OK. The premature-alarm rule does not rescue it, because the fill happens first.
When the retry ladder does straddle a period boundary (~60% of the time), you get two real breaching datapoints and it reaches ALARM around T+6min — the same window as the DLQ alarm, not earlier. So the best case is a tie and the common case is silence, while the DLQ alarm fires at ~T+5-6min every time.
Fix is one line: datapointsToAlarm: 1. That makes the alarm fire on the first faulting invocation, the rationale becomes true as written, and it still satisfies the AC's ">= 1 error in 5 min, 2 evaluation periods" (evaluation range stays 2). Keep NOT_BREACHING. Please add a test assertion pinning DatapointsToAlarm: 1, since that is the property carrying the semantics — the current test would pass either way.
If instead you want a sustained-error alarm, that is a defensible choice, but then all three comments and the runbook line need to say "sustained" rather than "sooner", and the complementary-not-redundant argument needs rewriting, because a 2-of-2 Errors alarm is strictly slower than the 1-of-1 DLQ alarm it sits beside.
Non-blocking: Closes #284 with no alarm action
I verified the SNS claim and it holds: there is no sns.Topic or addAlarmAction anywhere in cdk/src (only 4 bare alarms across fanout-consumer.ts, task-orchestrator.ts, and this construct). Declining to one-off a topic here is the right call, and a shared notification plane is a legitimately separate change.
But AC 5 asked for notification, and a bare alarm notifies nobody. The runbook calls these "two operator-visible signals", which is true only for an operator already reading the CloudWatch console — the exact state the issue was trying to fix. Suggest dropping Closes #284 in favour of Refs #284 and leaving the SNS AC open, or cutting the follow-up issue for the shared topic and linking it here. Also worth dropping (+ SNS) from the title, since the code deliberately does not add SNS and the title will land in the changelog.
Confirmed good
Statistic: 'Sum'pinned in the test with a comment explaining why — nice, that is the assertion most people forget.- The doc's SSE aside is correct: SSE-SQS is on by default for new queues, so omitting
encryptionis fine. - Receiver behaviour in the runbook matches
github-webhook.ts:186-211:InvocationType: 'Event', 200 on accepted dispatch, 500 only on invoke failure with dedup rollback. The "GitHub never redelivers" framing is accurate. - Both doc copies are byte-identical in the new section, so
docs:syncdid its job. - Note this is still a draft, and
build (agentcore)was pending when I looked.
| // same failure class the DLQ eventually catches, but the Errors metric | ||
| // trips one evaluation window sooner — before Lambda's async-retry | ||
| // ladder has landed a message on the DLQ. threshold 1 / 2 eval periods | ||
| // matches the AC; the metricErrors + 2-eval-period shape follows |
There was a problem hiding this comment.
This is the claim that does not survive as configured. At 2-of-2 the Errors alarm cannot beat the 1-of-1 DLQ-depth alarm 25 lines up: it either ties it (~T+6min, when the retry ladder straddles a period boundary) or stays silent. With datapointsToAlarm: 1 the claim becomes true, since Errors is stamped at invocation time roughly 3 minutes before the DLQ write.
Worth reworking this paragraph either way. As written it reads as a verified timing argument, and the next person to touch this alarm will trust it.
| - **`WebhookProcessorErrorAlarm`** — fires on the processor's Lambda `Errors` metric (`>= 1` error in a 5-minute period, sustained for 2 evaluation periods). 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 trips one evaluation window sooner (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: |
There was a problem hiding this comment.
Blocking for the runbook specifically — this is the sentence that will mislead someone at 3am. As configured the Errors alarm does not trip sooner; for a single-event crash it does not trip at all, so an operator following this section will wait on a signal that is never coming.
If you take datapointsToAlarm: 1, this line is correct as written and needs no edit. If you keep 2-of-2, it should say something like: "The DLQ-depth alarm is the reliable signal for a single failed event. The Errors alarm requires two consecutive 5-minute periods with errors, so it fires on sustained breakage rather than a one-off crash."
Same line in the Starlight mirror at docs/src/content/docs/using/Deploy-preview-screenshots-guide.md:191 — regenerate via docs:sync rather than editing both by hand.
✅ Acceptance summary (for the reviewer)#284 — GitHub webhook processor DLQ + alarm coverage, reconciled against current
🔀 Merge guidance (for the reviewer)Cluster 🤖 orchestrator note (agent) — promotion is orchestrator-driven; merge remains a human action. |
Co-authored-by: Laith Al-Saadoon <9553966+theagenticguy@users.noreply.github.com>
Co-authored-by: Laith Al-Saadoon <9553966+theagenticguy@users.noreply.github.com>
2 participants
Summary
Reconciles the remaining #284 acceptance criteria against
main. The DLQ half of the issue already landed via the PR #241 follow-up; this PR fills the two gaps: an alarm on the processor Lambda's Errors metric, and the operator doc section.Closes #284
Root cause / gap analysis
cdk/src/constructs/github-screenshot-integration.ts:175-220onmainalready:WebhookProcessorDlq(retentionPeriod14 days,enforceSSL: true) — AC 3 ✅deadLetterQueue: processorDlqonWebhookProcessorFn(async-invokeonFailure) — AC 1 ✅AwsSolutions-SQS3cdk-nag suppression — AC 4 ✅WebhookProcessorDlqDepthAlarmon the DLQ'sApproximateNumberOfMessagesVisiblemetric (threshold 1)Two ACs were UNMET:
Errorsmetric (>= 1in 5min, 2 eval periods). The pre-existing alarm is on the DLQ depth (AWS/SQS ApproximateNumberOfMessagesVisible), which is a different metric from the LambdaErrorsmetric the AC asks for. The depth alarm only fires once a failure has survived Lambda's async-retry ladder and landed on the queue; an Errors alarm fires as soon as invocations start faulting.docs/guides/DEPLOY_PREVIEW_SCREENSHOTS_GUIDE.mdexplaining how to inspect the DLQ. No such section existed.The fix (delta only)
WebhookProcessorErrorAlarmonwebhookProcessorFn.metricErrors()—threshold: 1,evaluationPeriods: 2,period: 5min,treatMissingData: NOT_BREACHING. Mirrors the existingOrchestratorErrorAlarmidiom incdk/src/constructs/task-orchestrator.ts:401. I did NOT duplicate the pre-existing DLQ-depth alarm — the two are complementary (fault-onset vs landed-on-DLQ) and the new test documents why.aws sqscommands to find, peek, and drain the DLQ. Starlight mirror regenerated viamise //docs:sync.A note on SNS (title mentions it, code does not)
AC 5 said "notify via existing alarm SNS topic if one exists; otherwise create one". There is no SNS topic or
addAlarmActionanywhere incdk/src/today — every existing alarm (FanOutConsumer.dlqDepthAlarm,OrchestratorErrorAlarm, the pre-existingWebhookProcessorDlqDepthAlarm) is a bare alarm with no notification action. To reuse-over-reinvent and stay consistent, this alarm is also bare. Standing up a stack-wide SNS notification plane is a larger cross-construct decision, out of scope for this single construct. Flagging for reviewer: if you want SNS wired now, it should probably be a shared topic across all these alarms in a follow-up, not one-off here. (Title retains "(+ SNS)" from the task brief for traceability — happy to drop it.)Testing
alarms on the processor Lambda Errors metric (>=1 in 5min, 2 eval periods)assertingMetricName: Errors,Namespace: AWS/Lambda,Period: 300,Threshold: 1,EvaluationPeriods: 2,TreatMissingData: notBreaching,FunctionNamedimension →WebhookProcessorFn. Bumped the alarmresourceCountIsfrom 1 → 2. Watched it fail (red), then added the construct code (green).mise //cdk:compile— passmise //cdk:eslint— pass (no mutations)mise //cdk:test— 136 suites / 2534 tests pass, 1 snapshot pass (no snapshot change)github-screenshotsuite — 6/6 passpre-commit run --files <changed>— all pass (incl. docs-sync, astro check, eslint)AwsSolutionsrule); existing SQS3 suppression untouched.Dependencies / related
github-screenshot-integration.ts(UA work) — whichever merges second will need a rebase; this PR touches only the alarm block + a new constant, so the conflict surface is small.aws-cdk-libmodules (aws-cloudwatch,aws-sqs) — no new dependency.aws-account-idmatches in historical docs (commit1cfa5b9f,docs/backlog/*,docs/diagrams/*) — unrelated to this branch;gitleaks git --log-opts=origin/main..HEADon my commit reports no leaks.🤖 Generated with Claude Code