fix: close the last four known gaps, and the two deploy bugs found closing them - #95
Merged
Merged
Conversation
…osing them `CLAUDE.md` named four known gaps. Two were already fixed, two were open. All four are closed now, and none of them was quite what the paperwork said. **`resolveEnvironment` existed twice.** seo's is deleted; core's is the one reader of `ULTIMATE_ENV`, and `'preview'` is now core's `'staging'`. The premise I briefed this on was wrong and the agent said so: `ULTIMATE_ENV` is **not in the env schema**, so nothing validates it at boot and a `robots.txt` render can genuinely be its first reader. A typo would have 500'd the one response whose body was already going to be `Disallow: /`. Hence `tryResolveEnvironment()` in core — `undefined` instead of a throw, no fallback of its own — rather than a second resolver in seo. Every unrecognised value still resolves non-indexable. **Compose paired a published host port with `replicas: 3`.** Reproduced against real Docker: the second replica dies with `Bind for 0.0.0.0:3000 failed: port is already allocated`. `web` and `sync` are `replicas: 1` in all four files. That is the rung, not a retreat — Compose is one box, the chart is where horizontal scaling lives, and the header names both ways up. A proxy in the framework's compose loses on its own bar: a second answer to "how does traffic reach a role", beside the chart's Ingress. Two more deploy bugs surfaced while fixing it, neither previously known: - **`sync` published a port nothing listened on.** The role binds `PORT + 1`, so `PORT: 3001` opened 3002 behind `ports: ['3001:3001']`. - **The chart's sync rollout could never complete.** `roles.sync.port` was rendered into both `PORT` and `containerPort`, so the readiness probe polled a socket the process never bound. The Ingress also routed `/_sync` while the node serves `/_x/sync`, so every websocket fell through to `web`, which answers no upgrade. Milestone 11's two-platform proof could not have passed. **`x build --target binary` is now proven, not just fixed.** `docker/Dockerfile` compiled without `--define ULTIMATE_FRAMEWORK_VERSION` — so the target was fixed everywhere except in the artifact the framework ships. It passes it now, and the image build ends in `/out/app --version`, so a binary that cannot answer fails the build instead of the first command an operator runs. Verified outside the checkout: 1.2.0. Also, from the review of the last PR: - `isolateTiers()` and `isolateGraph()` join `isolateDeclaredTags()`. From inside the owning module they restore what a test file cannot reach — the revalidator and both logs included. Adopted in cache, action and render. - `X_TEST_REGISTRY_LEAK`'s own `fix:` told a leaker to add `afterAll(resetTiers)` — which drops what a NEIGHBOUR registered, and the guard reports additions only. An error whose instruction causes the next defect is worse than none. - Five source files carried literal NUL bytes, so git classified them binary and their diffs were unreviewable. Replaced with the escape sequence; identical at runtime. This commit's own diff of those five is still binary — HEAD's side is — but every later one is text. BREAKING: `@ultimat3/seo` no longer exports `resolveEnvironment` or `SeoEnvironment`; `'preview'` is `'staging'`. Migration in CHANGELOG.md. The next release is a major (#87) already. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 51 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 79 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. 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: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (58)
Comment |
sebyx07
added a commit
that referenced
this pull request
Aug 17, 2026
…or the life of the process (#107) * fix(realtime)!: a socket closing mid-subscribe leaked a query entry for the life of the process Slices 02 and 06 of the deep-dive audit, realtime half — eighteen findings, four of them Critical, plus the benchmark claim that could not have caught any of them. **Every subscribe path attached to the book after its awaits.** That one shape is three of the four Criticals. A socket closing during `authorize`/`prepare`/`#read` strands a `QueryEntry` — matcher, shared row window, retained change buffer — for the process lifetime, because `teardown` walks a book the in-flight subscribe has not written to yet. Two concurrent subscribes to one topic open two transport subscriptions; the orphan is unreachable by `#release` and survives socket close, `teardown` and `hub.close()`. And N subscribe frames in one WebSocket write walk past `maxPerSocket`, `maxPerTenant` and `maxTopicsPerSocket`, because each reads a count the registration has not yet grown. Fixed with synchronous **reservations** — the sid claim and both caps decided in one step before the first await — plus per-key FIFO **frame lanes** (`mutate` per socket, `subscribe` per sid). The lanes are not what closes the caps: the per-tenant cap spans sockets, where no lane can see it. A global per-socket lane was rejected — it would put every frame behind a snapshot read, one DB round trip per reconnecting client, which is the restart storm this package is measured on. The fourth Critical: `drain()` marked a mutation `acked` when a fire-and-forget `send()` returned. A browser `WebSocket.send` on a CLOSING socket discards silently, so every in-flight mutation was lost on exactly the event the durable queue exists for. A drained mutation is now `inflight` until the server settles it or a lost connection returns it. Also closed, each with a failing-first test: - `onOpen` replayed live queries but never `#topics`, so `client.subscribe(topic, …)` was dead after the first reconnect — every channel message and presence frame lost, with `online === true` and no error. - A **successful** ack retired nothing. The journal row and rebase entry lived for the session, and a later rebase read `seq >= 0` as "everything in the log" and replayed committed mutations on top of server truth: an acked `+10` rolled back to what it saw before it ran and re-applied over a landed 99, giving 109. The pairing is now ordered — the rebase carries the state, the ack is the receipt, and the receipt goes last. - `startRead` cleared `entry.stale` before issuing the read, so a rejecting snapshot left the window unmarked and `#resnapshot` re-snapshotted desynced subscribers out of a divergent one. Permanent silent divergence, the one thing `stale` exists to prevent. - The sync node's shutdown hook had no phase, so it kept accepting websocket upgrades between SIGTERM and the close phase. Now `stopAccepting()` in `accept`, drain in `close`. - `qidOf` was a 32-bit FNV over client-controlled input, used as the sharing key for a cross-subscriber row window. Now SHA-256 truncated to 16 hex, the width `entity` chose. - A channel guard that *failed* dropped the topic, so a database timeout looked like a revoked grant. Only a denial drops now; a failure keeps the topic and ticks a counter. - The client had no heartbeat, so a subscribed client was swept from every presence room within one 30s TTL and a half-open socket was never detected. - `HelloFrame.resume` was filled by every client and read by nobody — each reconnect shipped every cursor twice, up to 512 ids each. Deleted rather than wired: a qid's digest half is not invertible, so a node reading a resume list cannot recover `input`, cannot run `authorize`, and could only answer from a pre-policy window for a subscription that does not exist yet. **Dead mechanisms deleted, not documented.** Bun's native pub/sub was subscribed and never published to; `FRAME_LIMITS.resume` bounded a field that no longer exists. Dropped channel frames are now counted, logged and exported as `channel_frames_dropped_total`. **The 50k benchmark claim is restated, not retracted.** The harness recorded `lastSeenSeq` and read it nowhere, so "49,981 received a channel patch, p50 54.0s" timed reconnect + resubscribe + one delivery — reachability. The timings are unchanged and still stand. A delivery run now exists: 10,000 clients, a probe every 200ms, 1,666,882 patches received, **0 lost**. The counter anchors to the connection epoch, because the publisher's sequence resets per process and a naive counter reports the restart itself as mass loss. Sixteen sites across `CLAUDE.md`, `README.md`, the wiki and `docs/idea/` said "time-to-consistent". **`CHANGELOG.md` had not been touched since #95** — six merged PRs of this sweep were unrecorded, three of them breaking. Backfilled, with twelve breaking changes named and their migrations; four of the twelve no commit message had called breaking, including the money `<p>_scale` column, which needs an `alter table` on every existing app. Breaking: `HelloFrame.resume` and `FRAME_LIMITS.resume` removed from the public types; `OfflineQueue.drain` no longer marks `acked`; `SyncNode` declares `stopAccepting()` and drops `publishToSelf`; `qidOf`'s value changes, so an old cursor names a ring entry the new node never held — one snapshot per subscription across a rolling deploy. `PROTOCOL_VERSION` deliberately not bumped: `decode` is a whitelist, so both skews are readable, and a bump would refuse every in-flight client for no gain. bun run verify: 14 of 17 green, 3 skipped (drift, contract-diff, budgets). bun run scripts/reference-app-gate.ts: every pin holds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBwWKBJkiogA4mDaJiJf3D * fix(realtime): CodeRabbit round — close() could not reach a bridge still opening, and a parked drain stranded every mutation behind it 22 review comments. Two of them are the defect classes this PR exists to close, one state later than the originals. **`ChannelHub.close()` could not reach a bridge whose transport subscription was still opening.** A `subscribe()` parked in `#authorize` holds a reservation with `sub === null`, so `unsubscribeWhenOpen` finds nothing to close and `#bridges.clear()` drops the entry. `#open` then hands a live transport subscription to a detached `Bridge` that nothing can name — a later `#release` looks the topic up, misses, and returns, while the handler keeps calling `deliver` for the life of the process. Exactly the orphan the `Bridge` header claims this shape prevents, at shutdown instead of at subscribe. Proven first: `expect(transport.live).toBe(0)` / `Received: 1` with the hub already closed. A `#closed` flag now precedes the walk, and `#open` closes a subscription that lands after it — plus one addition CodeRabbit did not propose: `#open` drops its own map entry when it is still the seated bridge, so a second post-close subscribe closes its own handle rather than double-unsubscribing this one's. **A connection lost while a drain pass was parked stranded every mutation behind it `inflight` forever.** Only the first was resent; the rest sat in a status nothing moves without a server settle that can never arrive. The pass now re-checks a drain epoch before claiming each remaining mutation and abandons the rest as `pending`. `#persist()` also handed `QueueStore.save()` the live mutable array. CodeRabbit proposed chaining `drain()` after `requeueInflight()` in `client.ts`; that narrows one window in a file that does not own the bug, so the fix went to `offline-queue.ts` instead, which now saves a snapshot. `client.ts` is unchanged. Also: a node that stopped accepting mid-authentication now sheds rather than upgrades; the tenant cap's cross-socket behaviour is pinned by a test that was missing; `live-fanout.ts` and `json.ts` gained the adjacent suites they never had; `stableDigest`'s direct tests moved to `json.test.ts`, where its source lives. **Two rejected, with the convention that contradicts them.** CodeRabbit asked for two test fixtures extending `Error` to become `UltimateError`s. They stand in for *foreign* errors — a driver pool timeout and an app's `onMutate` — and the repo does this deliberately at nine sites across three packages. `isPolicyDenial`, `stringField` and `renderThrowable` exist because such values arrive; rebuilding the fixture as an `UltimateError` would prove only that the framework handles its own errors. Recorded in `packages/realtime/CLAUDE.md` so the next round gets the answer without re-deriving it. **`sync-node.ts` was 495 of a 500-line ceiling** and the fixes pushed it over, so the HTTP surface — `/healthz`, `/readyz`, load shedding, the authenticated upgrade — moved to `sync-upgrade.ts`. Public API unchanged; the websocket handler stayed put. **Two claims corrected, both ours, both overstatements of the kind this PR is about.** "1,666,882 patches received, 0 lost" states a bounded measurement as an absolute: `missing` is a lower bound, because a hole is only visible between two frames one connection received. It now reads "0 observed sequence gaps" with the bound stated, in eleven files. And the milestone-6 risk row still named DB queries and replicator CPU as measured — the bench server has no database and no replication slot, so those were never measured at all. **`@ultimat3/flags` has never been published.** Verified against the registry, not inferred: it answers 404, no package at all, while the other 28 answer 200 at 1.2.0. It is not opting out — its `package.json` declares the same `publishConfig` as the rest — and nothing in the repo notices because every consumer resolves it through the workspace. Root `CLAUDE.md` said "29 in all — on npm in lockstep"; it now says versioned in lockstep, 28 published, with the gap named. A `wiki/Known-Gaps.md` row carries the workaround. My two `Known-Gaps.md` rows each carried a third cell under a two-column header, so GitHub dropped the workaround silently. Both folded to two. bun run verify: 14 of 17 green, 3 skipped (drift, contract-diff, budgets). bun run scripts/reference-app-gate.ts: every pin holds. bun test packages/realtime/src: 675 pass, 0 fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBwWKBJkiogA4mDaJiJf3D --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CLAUDE.mdnamed four known gaps. Two were already fixed, two were open. All four are closed now, and none of them was quite what the paperwork said.The two open gaps
resolveEnvironmentexisted in bothcoreandseoseo's is deleted. Core's is the one reader of
ULTIMATE_ENV, and'preview'is now core's'staging'.The premise I briefed this on was wrong, and the agent said so. I assumed a typo'd
ULTIMATE_ENVcould never reach a render because boot would have refused it. It doesn't:ULTIMATE_ENVis not in the env schema, socheckEnv()never validates it, and a grep of every non-test call site found none unconditional in a web boot. Arobots.txtrender genuinely can be its first reader — and a throw there would 500 the one response whose body was already going to beDisallow: /.So the answer is
tryResolveEnvironment()in core —undefinedinstead of a throw, naming no fallback of its own — rather than a second resolver in seo. Every unrecognised value still resolves non-indexable:ULTIMATE_ENV/NODE_ENVproductionproductionproductionstagingpreviewstagingprod(typo), unset,NODE_ENV=cipreviewdevelopmentCompose paired a published host port with
replicas: 3Reproduced against real Docker rather than argued from the spec:
webandsyncarereplicas: 1in all four files — framework, both tracked apps, andx new's scaffold. That is the rung, not a retreat.docs/ops/already says Compose is the single-node rung and the box is the availability story; the file has never once run atreplicas: 3. A reverse proxy in the framework's compose was the tempting answer and loses on the framework's own bar: it is a new image every app inherits and a second answer to "how does traffic reach a role", beside the chart's Ingress.Two deploy bugs found while fixing that one — neither previously known
syncpublished a port nothing listened on. The role bindsPORT + 1, soPORT: 3001opened 3002 behindports: ['3001:3001'].roles.sync.portwas rendered into bothPORTandcontainerPort, so the readiness probe polled a socket the process never bound — the pod never goes ready. The Ingress also routed/_syncwhile the node serves/_x/sync, so every websocket fell through toweb, which answers no upgrade. Milestone 11's two-platform proof could not have passed.The binary target is now proven, not just fixed
docker/Dockerfilecompiled without--define ULTIMATE_FRAMEWORK_VERSION— the target was fixed everywhere except in the artifact the framework ships, so any version read in that image exitsX_INVARIANT. It passes the define now, and the image build ends in/out/app --version, so a binary that cannot answer fails the build rather than the first command an operator runs. Verified outside the checkout:✓ 1.2.0.From the review of #94
isolateTiers()andisolateGraph()joinisolateDeclaredTags(). From inside the owning module they restore what a test file cannot reach — the revalidator and both logs included, which the in-file stopgap had to document as unrestorable. Adopted incache,actionandrender.X_TEST_REGISTRY_LEAK's ownfix:gave destructive advice — it told a leaker to addafterAll(resetTiers), which drops what a neighbour registered, and the guard reports additions only. An error whose instruction causes the next defect is worse than no instruction.Gate
bun run verify— 14 of 17 in 103s, 3 skipped (drift, contract-diff, budgets).bun run scripts/reference-app-gate.ts— every pin holds;examples/dummy10/17, demo 14/17.Breaking
@ultimat3/seono longer exportsresolveEnvironmentorSeoEnvironment;'preview'is'staging'. Migration table inCHANGELOG.md. The next release is a major (#87) already, which is what made this affordable now.Deferred, named
api/surface does not partly exist —modes.ts:167refuses everyapi/route unconditionally,RouteConfighas no handler field, anddev-render.tsmounts GET-only HTML. It also found a security requirement for the eventual mount: thecache-headersstage stampscache-control: public, s-maxage=60+vary: cookieon the OAuth 302, and the first leg carries no cookie — so for 60s every anonymous visitor would share one browser's PKCE state and handshake cookie. Any mount must declarecache: NO_STORE. Next PR.METRICS_PORTis honoured underx serveand silently ignored underx dev—cmd-dev.tspasses nometricsPort, so it binds 9090 whatever the env says. Found in the same investigation; goes withappRouteTable().rateLimitdoes not project to the MCP edge.from-action.ts:90callsprimitive.run(...), so policy projects correctly, butrateLimitis applied by the HTTP pipeline. An action declaringrateLimit: { limit: 5, windowMs: 600_000 }is throttled over HTTP and only verb-class-throttled over MCP. Axiom-2 violation with a security consequence.packages/cli;resetRegistries()among them cannot be fixed until@ultimat3/jobsexposes a re-registration path it deliberately withholds.X_MFA_REQUIREDstill namesPOST /auth/mfa/verify, mounted nowhere.docs/architecture/12-generated-app.md:151-166documents anapi/route.tsmechanism that does not exist — flagged once before and expanded rather than fixed. Goes with the route seam.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.