docs: concurrency chapter + live verification record for smoke probe - #9
Conversation
Replace the README's "Concurrency Limitations & Scaling Up" section with a tighter framing built around what actually keeps the pattern safe: the S3 conditional write (If-Match on the hydrated ETag) as optimistic concurrency control over a whole database file. Move the full treatment into a new docs/10-concurrency.md covering the master/sub-copy topology, 412 handling, rebase-and-retry, the SQS single-writer queue, and why EFS is not a multi-writer escape hatch. Also record the live post-deploy verification of the IAM-protected status probe (spec 2026-08-09-smoke-status-iam-design, new section 9.1), checking the last open acceptance box. Verified against the deployed us-east-1 stack: - Pre-deploy, the URL was still AuthType: NONE and smoke.sh failed correctly with the "not enforcing AWS_IAM" message -- the regression gate firing against a genuinely public URL rather than a stub. - Post-deploy: unsigned -> 403 (Forbidden at the AWS layer, never reaching the handler), signed -> 200, exit 0. - 127 consecutive smoke runs across three live rate(5 minutes) ticks: 127/127 unsigned 403, all signed runs ended 200, zero non-zero exits. - The 429 retry path was caught live at 16:57:14Z (Attempt 1: 429, Attempt 2: 200), correlating with the tick at 16:57:17Z that ran 2311 ms while holding the reservedConcurrentExecutions: 1 mutex. - Read-only invariant held: exactly three fetch-length invocations in the window, one per scheduled tick, none from the 127 smoke runs. The empty-state branch (snapshotVersion: null) remains harness-only -- the deploy landed on a bucket that already held memory.db, so the live runs exercised the populated branch. Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR documents S3 ETag-based optimistic concurrency for SQLite snapshots, including conflict behavior, retry and queue-based scaling options, storage limitations, and live smoke-test verification. ChangesS3 concurrency documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/10-concurrency.md`:
- Around line 23-42: Add the text language identifier to the fenced code blocks
containing the ASCII diagrams in the concurrency documentation, including both
diagram sections referenced by the comment. Keep the diagram contents unchanged.
- Around line 50-63: Update the documented conflict contract so the concurrency
explanation treats HTTP 404 and 409 responses as equivalent abort conditions
alongside 412, matching the PreconditionFailedError mapping in src/store/s3.ts.
Apply this clarification in docs/10-concurrency.md lines 50-63 and README.md
lines 150-152; both sites require direct documentation updates.
- Around line 76-90: Revise the concurrency guidance around “Adding
rebase-and-retry” to state that moving Bedrock and Discord side effects before
or after S3 publication does not provide atomicity and only changes the failure
mode. Require idempotency keys or a durable outbox with idempotent consumers
when coordinating these effects, while preserving the existing retry guidance.
In `@docs/superpowers/specs/2026-08-09-smoke-status-iam-design.md`:
- Line 227: Update the smoke-status acceptance criterion near the
live-verification note to limit the claim to observed populated-response and
429-retry paths. Explicitly state that the snapshotVersion: null empty-state
branch remains harness-only, and remove any wording implying live verification
covered that branch.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a576383-e271-4673-a97d-90270b975cb7
📒 Files selected for processing (3)
README.mddocs/10-concurrency.mddocs/superpowers/specs/2026-08-09-smoke-status-iam-design.md
- Add text language to fenced ASCII diagrams (markdownlint MD040) - Document 404 NoSuchKey and 409 ConditionalRequestConflict alongside 412 as equivalent abort conditions (matches src/store/s3.ts mapping) - Replace 'move side effects' wording with a coordinator/outbox requirement so it does not read as an atomicity guarantee - Limit live-verification claim on spec §9 to the populated-response and 429-retry paths; the snapshotVersion: null empty-state branch stays harness-only per §9.1 caveat
/fix-pr follow-upCommit: Review resolution
Verification
|
There was a problem hiding this comment.
Pull request overview
Updates project documentation to better explain the tutorial’s concurrency model (optimistic concurrency control via S3 conditional writes) and records a live, post-deploy verification log for the IAM-protected smoke status probe.
Changes:
- Rewrites the README concurrency section to emphasize S3 conditional writes (ETag-based
If-Match) and links to the new extended chapter. - Adds a new
docs/10-concurrency.mdchapter covering topology, 412 handling, rebase-and-retry, and a single-writer queue approach. - Extends the smoke probe IAM design spec with a live verification record and updates the implementation status line.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| README.md | Reframes concurrency guidance around S3 conditional writes and points to the new chapter. |
| docs/10-concurrency.md | Adds a detailed concurrency/scaling chapter for the S3-rehydrated SQLite pattern. |
| docs/superpowers/specs/2026-08-09-smoke-status-iam-design.md | Updates spec status and appends live verification record / acceptance checklist updates. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- README: distinguish bootstrap conditional (If-None-Match: "*") from update conditional (If-Match: <ETag>) so the concurrency story is accurate from the first write, matching src/store/s3.ts. - Spec: merge the two overlapping 'npm run smoke' acceptance bullets into one, scoping live verification to populated-response and 429-retry paths and keeping the empty-state branch clearly harness-only. Co-Authored-By: Claude <noreply@anthropic.com>
/fix-pr follow-upCommit: Review resolution
Verification
|
Docs-only change. No source, infra, or test files are touched.
Concurrency chapter
The README's "Concurrency Limitations & Scaling Up" section led with the split-brain risk and left the impression that the pattern is unsafe by default. It is not — the S3 conditional write (
If-Matchon the ETag we hydrated from) is what makes it safe, and that is a named, well-worn technique: optimistic concurrency control applied to a whole database file.412as an abort rather than rebasing — the Bedrock call and Discord post a losing tick already made are not rolled back. Unreachable on the fixed schedule withreservedConcurrentExecutions: 1; reachable the moment a second write path can race the loop.docs/10-concurrency.mdcarries the full treatment: master/sub-copy topology,412handling, rebase-and-retry, the SQS single-writer queue for high contention, and why EFS is not the multi-writer escape hatch it appears to be.Live verification record
Closes the last open acceptance box in
docs/superpowers/specs/2026-08-09-smoke-status-iam-design.md(new §9.1), verified against the deployedus-east-1stack.The deployed Function URL turned out to still be
AuthType: NONE— the merged IAM change had never been deployed. That made for a better test than intended:smoke.shwas run against it first and failed correctly with the "not enforcing AWS_IAM" message. The §4 regression row fired against a genuinely public URL, not a stub.After deploying:
403, body{"Message":"Forbidden"}— rejected at the AWS layer, never reaches the handler200, exit 0403; all signed runs ended200; zero non-zero exits429retry path16:57:14Z—Attempt 1: 429,Attempt 2: 200, correlating with the tick at16:57:17Zthat ran 2311 ms holding thereservedConcurrentExecutions: 1mutexThe contention window is only ~2.3 s per 300 s tick, so observing a
429took repeated back-to-back runs; §9.1 notes this for anyone repeating the check.Caveat, recorded in the spec: the empty-state branch (
snapshotVersion: null) remains harness-only. The deploy landed on a bucket that already heldmemory.db, so the live runs exercised the populated branch. Observing empty state live would mean deleting the snapshot.The EventBridge rule was left
DISABLEDafter verification.🤖 Generated with Claude Code