docs(operations): §8 Known limitations of the reference implementation

[surpradhan]

What does this PR do?

Adds OPERATIONS.md §8 — Known limitations (reference implementation) + its TOC entry. Five limitations documented matter-of-factly with the operational consequence and workaround for each:

1. Single-process runtime state — SSE fan-out/tickets, rate-limit windows, quota cache, and metrics are per-process in-memory; running >1 replica silently breaks real-time and multiplies rate limits. Postgres removes only the DB-layer constraint.
2. Webhook delivery is at-most-once — fire-and-forget microtask, no durable outbox, no startup re-drive of pending rows; restart symptoms documented.
3. Postgres schema is hand-mirrored — no PG migration runner; every SQLite migration must be mirrored into SCHEMA_DDL by hand; parity CI catches behavioural drift only.
4. Dashboard token can appear in URLs — ?token= convenience vs browser history/proxy logs; mitigations listed.
5. Graceful shutdown waits out SSE clients — 30s hard timeout, exit code 1; dashboards auto-reconnect.

Every claim was verified against the cited source files before writing (e.g. the quota cache's own drift comment, the stuck-pending restart symptom, the dashboard's ticket re-exchange on EventSource error).

Why?

CTO review Wave 1, item 8 — these constraints existed only in code and review transcripts; operators deserve them stated plainly. This is the documentation counterpart to the parked-reference-implementation direction: document honestly rather than engineer around.

How to test?

Docs only; anchors verified against the file's existing TOC style.

Checklist

• Docs only
• All claims source-verified

🤖 Generated with Claude Code

[surpradhan]

Independent validation report (validator context — separate from author and code-reviewer)

Verdict: Approve. Docs-only PR; every load-bearing claim in §8 was verified against source, and the two boldest claims (webhook at-most-once, 30s shutdown timeout) were verified empirically by running the code from the PR head (32f8f5b).

Gates (run in a clean worktree at the PR head)

• npm run lint — clean.
• npm test — 494 unit + 264 integration, 0 failures.
• CI: 14/14 required checks green (waited out the 4 Python matrix jobs).
• Scope: gh pr diff 163 --name-only = OPERATIONS.md only; worktree HEAD == PR headRefOid, no uncommitted drift.

Probe results

1. At-most-once webhooks — confirmed in source. scheduleDelivery (src/webhookDelivery.js:422) is Promise.resolve().then(...) fire-and-forget; the webhook_deliveries row is first written inside deliverToWebhook with status: "pending" (line 372) only when an attempt starts, and updated only after deliverWithRetries resolves — so a crash mid-retry leaves a row stuck in pending, and a queued-but-unstarted delivery leaves no row, exactly as documented. Grep across src/ finds no startup re-drive of pending rows; scheduleDelivery is called only from the ingest route (src/server.js:1606).
2. Shutdown claim — confirmed empirically. Booted the server from the worktree (temp DB, port 8899). Control: SIGTERM with no SSE client → exit 0s, code 0. With one open /stream connection (ticket minted via POST /auth/sse-ticket in dev mode): SIGTERM → process hung on httpServer.close(), logged "shutdown timeout exceeded — forcing exit", exited after exactly 30s with code 1. Matches the doc verbatim, including the quoted log line.
3. Single-process state — all named maps exist: sseTickets (src/server.js:101), sseClients (src/server.js:214, one-time-use ticket delete at :121), windows (src/middleware/rateLimit.js:35), cache (src/middleware/quota.js:43) whose header comment (lines 15–24) does document the per-process drift as claimed; src/metrics.js header states "All storage is in-process memory."
4. Anchors — 13/13 internal links resolve against GitHub-style generated anchors for the file's headings, including the new TOC entry #8-known-limitations-reference-implementation and §8's cross-refs to §1 and §6.
5. Secondary claims spot-checked: extractBearerOrQuery does accept ?token= (src/auth.js:134–136); both the http request log (src/server.js — path: req.path) and the access log (src/middleware/accessLog.js:56) record path only, never the query string; Postgres applies a single idempotent SCHEMA_DDL with no migration tracking (src/db/backends/postgres.js:60,264) while SQLite uses schema_migrations (src/db/migrate.js); dashboard re-exchanges a fresh SSE ticket in sseConn.onerror (src/public/dashboard.html:1476–1479).

Findings

Zero actionable items. The section's tone matches the rest of OPERATIONS.md ("every claim is verified against the cited source file") and, for once, that sentence is literally true — I could not find a single overstated or inaccurate claim.

(All scratch files, temp DB, and the node_modules symlink were removed; worktree left clean.)

[surpradhan]

Independent code review — PR #163 (docs-only: OPERATIONS.md §8)

Every technical claim in §8 was traced against current source in a worktree of this branch (which includes #156/#160/#162). Gates run read-only: npm run lint clean, npm run test:unit 494/494 pass.

Findings

Suggestion — OPERATIONS.md:777, 790–791 vs §2:147–152 and §3:279–282: §8's "do not run more than one instance" contradicts §2's multi-instance guidance.
§8 says "do not run more than one instance" / "Run one instance and scale vertically", but §2 "Connection pooling & sizing" plans max_connections as "(server instances × 10) + headroom" and its Multi-instance bullet recommends PgBouncer "past a handful of replicas" — i.e. it presents multi-replica as a supported shape. §3's quota bullet likewise frames multi-replica as a soft-limit caveat, not a prohibition. After this PR the same guide gives operators opposite directions. The facts support the softer position: ingest + read API are DB-backed and do scale out; what breaks is SSE real-time (sticky sessions + cross-replica blind spots), per-key rate-limit multiplication, quota drift, and per-replica metrics. Suggested fix (either direction works, pick one):

• Soften §8: retitle "Single-process runtime state — replicas degrade real-time, limits, and metrics" and close with "If you run replicas for ingest throughput (see §2 for pool sizing), accept: sticky sessions for the ticket → /stream two-step, dashboards blind to other replicas' events, per-key limits × N, quota as a soft limit (§3), and metrics aggregated at the scraper." — or
• Keep §8's prohibition and add "(but see §8)" cross-refs to §2's Multi-instance bullet and §3's soft-limit bullet.

Nit — OPERATIONS.md:795: "at-most-once" is not strictly accurate from the receiver's perspective.
deliverAttempt (src/webhookDelivery.js) classifies a thrown httpPost — including a timeout (timeout after Nms) — as transient and retries with backoff. A timed-out attempt that actually reached and was processed by the receiver is therefore re-sent, so a receiver can process the same delivery twice (same X-AEP-Delivery-Id, same body bytes). The property the section actually documents is non-durability ("not at-least-once; deliveries can be lost across restart"). Suggested fix: add a parenthetical, e.g. "at-most-once per process lifetime — and within a retry chain a timed-out attempt may already have reached the receiver, so idempotent receivers should de-dupe on X-AEP-Delivery-Id" (or retitle "Webhook delivery is best-effort, not at-least-once").

Nit — OPERATIONS.md:835: "with any dashboard connected" understates the blocker.
Any open /stream connection prevents httpServer.close() from completing — including API-key SSE consumers, not only dashboards. Suggested fix: "with any SSE client connected (typically a dashboard), …".

Verified accurate (claim-by-claim)

• #156 fail-closed staleness check (the key question): the dashboard-token subsection is orthogonal and nothing in it is now inaccurate. It describes ?token= acceptance when a token IS set (extractBearerOrQuery, src/auth.js:134, used by requireReadAccess step 1 and requireDashboardAuth); it makes no claim about unset-token behaviour, so the new production fail-closed path (src/auth.js:311–321 dashboard 503; :282–298 reads 401, API-key reads unaffected) neither contradicts nor needs to be restated here. Fine as-is — no cross-ref required (.env.example and §5 already cover it).
• Single-process state: sseTickets Map src/server.js:101, sseClients src/server.js:214, ticket-then-/stream two-step (POST /auth/sse-ticket src/server.js:1352 mints into the in-process Map → sticky sessions genuinely required); per-key windows Map src/middleware/rateLimit.js:35, mounted ingest-only (src/server.js:1499) so "per-key ingest rate-limit" is precise; quota cache src/middleware/quota.js whose header comment does document the drift verbatim; src/metrics.js header: "All storage is in-process memory".
• Webhooks: scheduleDelivery is Promise.resolve().then(...) fire-and-forget (src/webhookDelivery.js:422–431); the pending row is created at the top of deliverToWebhook (:366–378) and updated only after deliverWithRetries returns (:404) — so interrupted = stuck pending, never-started (queued on the microtask/semaphore) = no row, exactly as written; no startup re-drive exists anywhere in src/.
• Postgres: SQLite uses versioned migrations recorded in schema_migrations (src/db/migrate.js:68,106); Postgres applies one idempotent SCHEMA_DDL at every init() (src/db/backends/postgres.js:60,264) with no migration table of its own; CI job name Postgres parity tests matches .github/workflows/ci.yml:191 exactly.
• Shutdown: src/server.js:1951–1969 — httpServer.close(), 30_000 ms hard timeout, process.exit(1), and the quoted log message "shutdown timeout exceeded — forcing exit" is byte-exact; no closeAllConnections/SSE teardown exists. Dashboard reconnect: src/public/dashboard.html:1476–1485 onerror re-initialises with a fresh ticket when the EventSource is CLOSED, matching "exchanges a fresh SSE ticket on error".
• Token-in-URL support claims: access log records req.path only, never the query string (src/middleware/accessLog.js:26,56); no pino-http/req.url request logging anywhere in src/server.js or src/logger.js, so "the application never logs query strings" holds; the SSE-ticket rationale comment (src/server.js:96–98) matches the stated purpose.
• Anchors all resolve: #8-known-limitations-reference-implementation, #6-webhooks--alerts-phase-16 (heading line 513), #1-choosing-a-storage-backend (line 30); TOC entry matches the existing numbered style.
• Consistency: no contradiction with §5's checklist or the intro's /v1 note (§8 uses unversioned example paths per the stated convention); tone ("verified against the cited source file") matches the guide.

What looks good

Genuinely careful work: every file citation is correct (including src/auth.js vs src/middleware/, a distinction easy to get wrong), the quoted shutdown log line is byte-exact, and the pending-row-timing distinction (stuck pending vs no row at all) reflects the actual code order in deliverToWebhook. The workaround guidance (reconcile via GET /webhooks/:id/deliveries or the read API; migration-review checklist item for SCHEMA_DDL) is actionable rather than hand-wavy.

Verdict

Approve — zero Critical findings; 1 Suggestion (reconcile §8 with §2/§3 multi-instance text) + 2 Nits, all with concrete fixes above, to be addressed before merge per project convention.

Gates: lint clean; 494/494 unit tests; PR CI green except the still-running Python SDK matrix at review time (docs-only diff, unrelated).

[surpradhan]

Round-1 findings applied in ceac730: §8's single-instance prohibition softened to the precise truth — ingest/read correctness scales on Postgres (§2's pool-sizing guidance stands, now cross-referenced both ways); what degrades with replicas is SSE real-time, per-key limit precision, quota drift, and metrics locality. Webhook subsection retitled 'best-effort, not durable' with the duplicate-on-timeout-retry case documented and de-dup guidance on X-AEP-Delivery-Id (header verified in src/webhookDelivery.js). 'Any dashboard connected' → 'any SSE client connected (typically a dashboard)'.

[surpradhan]

Independent code review — round 2 (delta ceac730)

Narrow re-review of the round-1 findings. All three are resolved; verification notes below.

1. Suggestion (§8 vs §2/§3 multi-replica contradiction) — RESOLVED.

• §8 retitled "Single-process runtime state — replicas degrade several features" and no longer forbids replicas; it now distinguishes what is unaffected (ingest/read correctness on Postgres) from what degrades (SSE real-time delivery, limit/quota precision, metrics locality). This matches §2's PgBouncer multi-replica guidance instead of contradicting it.
• Two-way cross-refs verified: §2's Multi-instance bullet links #8-known-limitations-reference-implementation and §8 links #2-postgres-production-deployment — both anchors resolve against the actual headings (## 8. Known limitations (reference implementation), ## 2. Postgres production deployment).
• §3's quota bullet ("Soft, single-node limit … drift by up to one refresh window × instance count") now reads consistently: §8 describes quota as precision drift (matching §3) while correctly reserving "multiplies by the instance count" for the rate limiter — an accurate distinction, since the rate-limit windows are purely in-memory with no DB reconciliation while the quota cache re-counts against the DB every QUOTA_REFRESH_MS. No new contradiction introduced.

2. Nit (at-most-once precision) — RESOLVED. Retitled "best-effort, not durable" with the duplicate case spelled out. Verified against source: a per-attempt timeout surfaces as a thrown httpPost error and is classified transient → retried (src/webhookDelivery.js — req.destroy(new Error(...)) on timeout, transient classification in attemptDelivery), so a timed-out attempt that reached the receiver is indeed re-sent. X-AEP-Delivery-Id is set at src/webhookDelivery.js:389 and is minted once per delivery in deliverToWebhook with the same headers object flowing through all attempts in deliverWithRetries — the id is stable across retries, so the de-dup guidance is sound.

3. Nit ("any dashboard") — RESOLVED. Now "with any SSE client connected (typically a dashboard)", which is what httpServer.close() actually waits on.

Gates: docs-only diff (OPERATIONS.md only). CI green on all completed checks at head ceac730 (Python SDK matrix still running at review time; unaffected by this diff and green on round 1).

Zero actionable items.

Verdict: APPROVE.

(Reviewer shares the author's gh identity, so this is a COMMENT review standing in for a formal approval — per project convention.)