Skip to content

fix: bound the retry budget for two idempotency DynamoDB writes#116

Merged
timpugh merged 1 commit into
mainfrom
fix/retry-budget-idempotency-writes
Jul 7, 2026
Merged

fix: bound the retry budget for two idempotency DynamoDB writes#116
timpugh merged 1 commit into
mainfrom
fix/retry-budget-idempotency-writes

Conversation

@timpugh

@timpugh timpugh commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Addresses a Well-Architected review (Reliability + Cost).

Finding 1 — Reliability (fixed in code)

The @idempotent layer makes two sequential DynamoDB writes per request (save_inprogress PutItem before the handler, save_success UpdateItem after), and the persistence layer shares boto_config. With total_max_attempts=3 (~7.5s worst case per call), a DynamoDB brownout hits the budget twice → ~15s > the 10s function timeout → a 502 off the OpenAPI contract that feeds the CodeDeploy canary rollback alarm. The prior budget comment only reasoned about one browned-out call.

  • lambda/app.py: total_max_attempts 3 → 2 (~4s/call, ~8s for both writes, < 10s)
  • Corrected the budget comment to account for the double write
  • tests/unit/test_handler.py: assertion 3 → 2 + docstring

Finding 2 — RUM guest pool (documented, no behavior change)

The vulnerability (public unauthenticated pool → anyone can mint guest creds and call rum:PutRumEvents directly → uncapped ingestion cost) is real, but the review's proposed remediation — lowering session_sample_ratedoes not mitigate it: that knob is client-side (honored by the RUM browser client); a direct PutRumEvents caller ignores it. Lowering it only reduces legitimate traffic (already a separate cost TODO). So instead of a mislabeled sample-rate change:

  • frontend_stack.py: comment at session_sample_rate explaining it's client-side and doesn't bound the data plane
  • TODO.md: new item naming the abuse vector and the real guardrail (Budgets / RUM-volume alarm) as a fork responsibility

Validation

make pr passes end to end (tests, cdk-nag clean, OpenAPI in sync). No infra shape or IAM change.

The @idempotent layer makes two sequential DynamoDB writes per request
(save_inprogress before the handler, save_success after), so a DynamoDB
brownout hits the shared retry budget twice. With total_max_attempts=3
(~7.5s per call) the two writes could reach ~15s and exceed the 10s function
timeout — a 502 off the OpenAPI contract that feeds the canary rollback alarm.
Drop to total_max_attempts=2 (~4s/call, ~8s for both) and correct the budget
comment and the test assertion.

Also document (no behavior change) that the RUM guest identity pool lets anyone
call rum:PutRumEvents directly, and that session_sample_rate is client-side and
does not bound that vector — the real guardrail is a Budgets / RUM-volume alarm.
@timpugh timpugh enabled auto-merge (squash) July 7, 2026 23:32
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

🏗️ CDK infra diff — PR vs main

ServerlessApp-us-east-1-stage/ServerlessAppBackend-us-east-1
Stack ServerlessApp-us-east-1-stage/ServerlessAppBackend-us-east-1 (ServerlessAppBackend-us-east-1)
Resources
[-] AWS::Lambda::Version ServerlessAppBackend-us-east-1/App/ApiFunction/CurrentVersion AppApiFunctionCurrentVersion01C510AC3f1d65f272b884e62cef2387146e55b2 destroy
[+] AWS::Lambda::Version ServerlessAppBackend-us-east-1/App/ApiFunction/CurrentVersion AppApiFunctionCurrentVersion01C510ACf4955ab9db00d1307ccc3cae98f79992
[~] AWS::Lambda::Function ServerlessAppBackend-us-east-1/App/ApiFunction AppApiFunctionDE515850
 ├─ [~] Code
 │   └─ [~] .S3Key:
 │       ├─ [-] 632c9e4f0820a76d728b9764a2f84266948dc6e7a8ae3ea7eab17f983d9a1bda.zip
 │       └─ [+] 867c7f34fc34e08ab5e6d7db4be3ccf863b7b28048fba29d729892816af3e003.zip
 └─ [~] Metadata
     └─ [~] .aws:asset:path:
         ├─ [-] ../asset.632c9e4f0820a76d728b9764a2f84266948dc6e7a8ae3ea7eab17f983d9a1bda
         └─ [+] ../asset.867c7f34fc34e08ab5e6d7db4be3ccf863b7b28048fba29d729892816af3e003
[~] AWS::Lambda::Alias ServerlessAppBackend-us-east-1/App/LiveAlias AppLiveAlias3872472E
 └─ [~] FunctionVersion
     └─ [~] .Fn::GetAtt:
         └─ @@ -1,4 +1,4 @@
            [ ] [
            [-]   "AppApiFunctionCurrentVersion01C510AC3f1d65f272b884e62cef2387146e55b2",
            [+]   "AppApiFunctionCurrentVersion01C510ACf4955ab9db00d1307ccc3cae98f79992",
            [ ]   "Version"
            [ ] ]



✨  Number of stacks with differences: 1

@timpugh timpugh merged commit 4054ab9 into main Jul 7, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant