From d121904910629b7762bc151abe2496dcf0536cc8 Mon Sep 17 00:00:00 2001 From: Lucas Carlson Date: Mon, 10 Aug 2026 09:52:37 -0700 Subject: [PATCH 1/2] fix: batch component refreshes on reconnect Mapping the reconnect path for browser coverage turned up a defect. A reconnecting subscription refreshed every stale component individually and ignored the batches those components declared: refreshes_for partitioned by batch and grouped, reconnect_refreshes did not. A page with twenty batched components therefore issued twenty requests where a live invalidation issues one. That is the worst possible moment for request amplification. Reconnects cluster: a server restart or a network blip reconnects every client at once, so the amplification lands as a burst rather than spread over time. Both paths now share refresh_streams, so a reconnecting client pays what a connected one pays. The browser suite gains the reconnect burst it was missing: convergence of batched and unbatched components, an inert replay of an already-applied revision, cancellation of the request left in flight by the drop, incarnation ordering after a destroy and recreate, and payload delivery exactly once per revision. Each was verified to fail against a mutated module rather than only to pass against the current one. --- CHANGELOG.md | 10 + docs/realtime.md | 5 + docs/roadmap.md | 12 +- lib/solid_objects/component_subscriptions.rb | 30 +- .../solid_objects/component_subscriptions.rbs | 5 + test/browser/browser_test_helper.mjs | 17 +- test/browser/reconnect.test.mjs | 320 ++++++++++++++++++ test/integration/component_batch_test.rb | 70 ++++ 8 files changed, 453 insertions(+), 16 deletions(-) create mode 100644 test/browser/reconnect.test.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 06ef394..d13807e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,16 @@ Retention runs on its own thread, so a slow pass cannot delay replacing a crashed role, and a failed pass retries at monitor cadence with a doubling backoff rather than deferring for the whole interval. +- Batch component refreshes on reconnect. A reconnecting subscription refreshed + every stale component individually, ignoring the batches those components + declared, so a page with twenty batched components issued twenty requests + instead of one. That happens at the worst moment: a server restart reconnects + every client at once. Reconnect now shares the batching the live invalidation + path uses. +- Cover the reconnect burst in the browser suite: convergence of batched and + unbatched components, an inert replay of an already-applied revision, + cancellation of the request left in flight by the drop, incarnation ordering + after a destroy and recreate, and payload delivery exactly once per revision. - Add Ruby 4.0 to the compatibility matrix, which now covers Ruby 3.3, 3.4, and 4.0 against Rails 8.0 and 8.1. diff --git a/docs/realtime.md b/docs/realtime.md index 3da2e4d..d141623 100644 --- a/docs/realtime.md +++ b/docs/realtime.md @@ -393,6 +393,11 @@ ordered commits within one incarnation. Out-of-order invalidations at or below the last transmitted pair are ignored. The durable state row remains source of truth. +Stale components that share a `batch:` are refreshed together, exactly as a +live invalidation refreshes them, so reconnecting costs one request per batch +rather than one per component. That matters most on a restart, when every +client reconnects at once. + The component endpoint rejects a requested revision newer than the committed snapshot. This is a final server-side guard; browser safety primarily comes from monotonic channel filtering plus replace-frame detachment or morph diff --git a/docs/roadmap.md b/docs/roadmap.md index 0720e27..3293881 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -52,7 +52,10 @@ - A JavaScript suite covering every browser module, run in CI with Node's test runner and jsdom, plus a browser suite running the same modules against real Chromium and a real Turbo build, with every GitHub Actions reference pinned to - a commit SHA + a commit SHA. The browser suite covers the reconnect burst: convergence of + batched and unbatched components, an inert replay of an applied revision, + cancellation of the request left in flight by the drop, incarnation ordering + after a destroy and recreate, and payload delivery exactly once per revision ## Partially implemented @@ -73,7 +76,10 @@ implemented; application-directed Turbo append intents are not. Batch coalescing happens in the browser rather than the broadcast executor, so one commit still sends one Action Cable message per changed observable even - though it costs one browser request. + though it costs one browser request. Reconnect convergence previously + bypassed batching entirely, issuing one request per stale component at the + moment a restart reconnects every client at once; it now shares the batching + the live invalidation path uses. - Backpressure: mailbox/payload/state/result caps and fair yields exist; distributed per-actor rate limits and global admission control do not. - Administration: actor and dead-letter views plus policy hooks exist; richer @@ -84,7 +90,7 @@ ## Next milestones 1. Add result lookup by request ID and broader deadlock retry classification. -2. Add Turbo append intents and expand reconnect coverage in a full browser. +2. Add Turbo append intents. 3. Add distributed rate limits, global admission hooks, and cache-capacity eviction. 4. Expand security scanning beyond the Brakeman scan, such as dependency diff --git a/lib/solid_objects/component_subscriptions.rb b/lib/solid_objects/component_subscriptions.rb index 5dc502a..077bb6a 100644 --- a/lib/solid_objects/component_subscriptions.rb +++ b/lib/solid_objects/component_subscriptions.rb @@ -52,26 +52,19 @@ def refreshes_for(invalidation) registration.dependencies.include?(observable_name) && newer_revision?(registration.dom_id, instance_id, revision) end - batched, individual = changed.partition(&:batch) - streams = individual.map { |registration| refresh(registration, instance_id, revision) } - batched.group_by(&:batch).each_value do |group| - group.each { |registration| record_revision(registration, instance_id, revision) } - streams << TurboStreamRenderer.batch_refresh(group, instance_id, revision) - end - streams + refresh_streams(changed, instance_id, revision) end # @rbs (ActorSnapshot) -> Array[String] def reconnect_refreshes(snapshot) - registrations.filter_map do |registration| - next unless newer_revision?( + stale = registrations.select do |registration| + newer_revision?( registration.dom_id, snapshot.instance_id, snapshot.revision ) - - refresh(registration, snapshot.instance_id, snapshot.revision) end + refresh_streams(stale, snapshot.instance_id, snapshot.revision) end class << self @@ -91,6 +84,21 @@ def validate_identity!(registration, reference) attr_reader :registrations, :revisions + # Live invalidations and reconnect replays share this, so a reconnecting + # client pays the same number of requests a connected one does. + # @rbs (Array[ComponentRegistration], Integer, Integer) -> Array[String] + def refresh_streams(changed, instance_id, revision) + batched, individual = changed.partition(&:batch) + streams = individual.map do |registration| + refresh(registration, instance_id, revision) + end + batched.group_by(&:batch).each_value do |group| + group.each { |registration| record_revision(registration, instance_id, revision) } + streams << TurboStreamRenderer.batch_refresh(group, instance_id, revision) + end + streams + end + # @rbs (ComponentRegistration, Integer, Integer) -> void def record_revision(registration, instance_id, revision) revisions[registration.dom_id] = [ instance_id, revision ] diff --git a/sig/generated/lib/solid_objects/component_subscriptions.rbs b/sig/generated/lib/solid_objects/component_subscriptions.rbs index 8b4746c..ff0557d 100644 --- a/sig/generated/lib/solid_objects/component_subscriptions.rbs +++ b/sig/generated/lib/solid_objects/component_subscriptions.rbs @@ -31,6 +31,11 @@ module SolidObjects attr_reader revisions: untyped + # Live invalidations and reconnect replays share this, so a reconnecting + # client pays the same number of requests a connected one does. + # @rbs (Array[ComponentRegistration], Integer, Integer) -> Array[String] + def refresh_streams: (Array[ComponentRegistration], Integer, Integer) -> Array[String] + # @rbs (ComponentRegistration, Integer, Integer) -> void def record_revision: (ComponentRegistration, Integer, Integer) -> void diff --git a/test/browser/browser_test_helper.mjs b/test/browser/browser_test_helper.mjs index 1d3380d..329584f 100644 --- a/test/browser/browser_test_helper.mjs +++ b/test/browser/browser_test_helper.mjs @@ -64,7 +64,20 @@ export async function openPage(origin) { const browser = await chromium.launch() const context = await browser.newContext() const page = await context.newPage() - await page.goto(`${origin}/`) - await page.waitForFunction(() => Boolean(customElements.get("solid-objects-refresh"))) + await loadPage(page, origin) return { browser, page } } + +// The browser modules keep applied revisions in module scope, so a test that +// needs to start from a lower revision than the previous one left behind has to +// reload rather than only reset the DOM. +export async function loadPage(page, origin) { + await page.goto(`${origin}/`) + await page.waitForFunction(() => + Boolean( + customElements.get("solid-objects-refresh") && + customElements.get("solid-objects-batch-refresh") && + customElements.get("solid-objects-payload") + ) + ) +} diff --git a/test/browser/reconnect.test.mjs b/test/browser/reconnect.test.mjs new file mode 100644 index 0000000..c99e94f --- /dev/null +++ b/test/browser/reconnect.test.mjs @@ -0,0 +1,320 @@ +import assert from "node:assert/strict" +import { test, before, after, beforeEach } from "node:test" +import { startServer, openPage, loadPage, frameHtml } from "./browser_test_helper.mjs" + +// A reconnecting subscription replays the current state as a burst of refresh +// and payload elements. These cover what the channel tests cannot: whether that +// burst converges the live document, and whether a replay of something already +// applied leaves the page alone. +let server +let origin +let browser +let page + +const batches = new Map() +const components = new Map() +const cancelled = new Set() + +before(async () => { + const started = await startServer({ + routes: { + "/solid_objects/components/batch": async (request, response, url) => { + const token = url.searchParams.get("token") + // A client abort closes the socket before the response is written, so + // this is where a cancelled request becomes observable to a test. + response.on("close", () => { + if (!response.writableEnded) cancelled.add(token) + }) + const entry = batches.get(token) + if (entry) entry.requested = true + await entry?.gate + if (response.writableEnded || response.destroyed) return + + response.writeHead(200, { "Content-Type": "application/json" }) + response.end(JSON.stringify({ frames: entry?.frames ?? [] })) + }, + "/solid_objects/components": (request, response, url) => { + response.writeHead(200, { "Content-Type": "text/html" }) + response.end(components.get(url.searchParams.get("token")) ?? "") + } + } + }) + server = started.server + origin = started.origin + const opened = await openPage(origin) + browser = opened.browser + page = opened.page +}) + +after(async () => { + await browser?.close() + server?.close() +}) + +beforeEach(async () => { + batches.clear() + components.clear() + cancelled.clear() + await loadPage(page, origin) +}) + +async function waitFor(condition, message) { + const deadline = Date.now() + 5000 + while (Date.now() < deadline) { + if (condition()) return + await new Promise((resolve) => setTimeout(resolve, 20)) + } + assert.fail(message) +} + +function gate() { + let release + const promise = new Promise((resolve) => { + release = resolve + }) + return { promise, release } +} + +function frame(target, revision, body) { + return { + target, + revision, + refresh_method: "morph", + html: frameHtml(target, revision, body) + } +} + +// Mirrors what ComponentSubscriptions transmits: one element per batch, +// carrying every stale target in that batch. +async function replayBatch({ revision, token, targets }) { + await page.evaluate(({ revision, token, targets }) => { + const element = document.createElement("solid-objects-batch-refresh") + element.dataset.batch = "playmat" + element.dataset.revision = revision + element.dataset.targets = targets.join(" ") + const tokens = targets.map((target) => `tokens[]=${target}`).join("&") + element.dataset.source = + `/solid_objects/components/batch?token=${token}&${tokens}` + document.getElementById("solid-objects-scope").append(element) + }, { revision, token, targets }) +} + +async function replayComponent({ target, token }) { + await page.evaluate(({ target, token }) => { + const element = document.createElement("solid-objects-refresh") + element.dataset.target = target + element.dataset.source = `/solid_objects/components?token=${token}` + element.dataset.refreshMethod = "morph" + document.getElementById("solid-objects-scope").append(element) + }, { target, token }) +} + +async function replayPayload({ name, revision, payload }) { + await page.evaluate(({ name, revision, payload }) => { + const element = document.createElement("solid-objects-payload") + element.dataset.name = name + element.dataset.revision = revision + element.textContent = JSON.stringify(payload) + document.getElementById("solid-objects-scope").append(element) + }, { name, revision, payload }) +} + +function textOf(target) { + return page.evaluate( + (target) => document.getElementById(target).textContent, + target + ) +} + +function revisionOf(target) { + return page.evaluate( + (target) => document.getElementById(target).dataset.solidObjectsRevision, + target + ) +} + +test("a reconnect converges every stale component in one request", async () => { + batches.set("reconnect", { + frames: [ + frame("player", "1:12", "current player"), + frame("controls", "1:12", "current controls") + ] + }) + + await replayBatch({ + revision: "1:12", + token: "reconnect", + targets: [ "player", "controls" ] + }) + await page.waitForFunction( + () => + document.getElementById("player").dataset.solidObjectsRevision === "1:12" && + document.getElementById("controls").dataset.solidObjectsRevision === "1:12", + null, + { timeout: 5000 } + ) + + assert.match(await textOf("player"), /current player/) + assert.match(await textOf("controls"), /current controls/) +}) + +test("a reconnect converges an unbatched component", async () => { + components.set("single", frameHtml("player", "1:12", "current player")) + + await replayComponent({ target: "player", token: "single" }) + await page.waitForFunction( + () => document.getElementById("player").dataset.solidObjectsRevision === "1:12", + null, + { timeout: 5000 } + ) + + assert.match(await textOf("player"), /current player/) +}) + +// A redundant replay must be inert. Morphing a frame that is already current +// would still walk the DOM, discarding anything the page put there since. +test("replaying a revision already applied leaves the frame untouched", async () => { + batches.set("applied", { frames: [ frame("player", "1:12", "current player") ] }) + await replayBatch({ revision: "1:12", token: "applied", targets: [ "player" ] }) + await page.waitForFunction( + () => document.getElementById("player").dataset.solidObjectsRevision === "1:12", + null, + { timeout: 5000 } + ) + await page.evaluate(() => { + const marker = document.createElement("span") + marker.id = "marker" + document.getElementById("player").append(marker) + }) + + batches.set("replay", { frames: [ frame("player", "1:12", "current player") ] }) + await replayBatch({ revision: "1:12", token: "replay", targets: [ "player" ] }) + await page.waitForTimeout(300) + + const marker = await page.evaluate(() => Boolean(document.getElementById("marker"))) + assert.equal(marker, true, "a replay at the applied revision should not morph") +}) + +// A request issued before the drop is still in flight when the reconnect +// arrives. Leaving it running risks its older response landing first and +// flashing stale content, so the reconnect has to cancel it outright. +test("a reconnect cancels the request left in flight by the drop", async () => { + const stalled = gate() + batches.set("before-drop", { + gate: stalled.promise, + frames: [ frame("player", "1:9", "state from before the drop") ] + }) + batches.set("after-reconnect", { + frames: [ frame("player", "1:12", "current player") ] + }) + + await replayBatch({ revision: "1:9", token: "before-drop", targets: [ "player" ] }) + await waitFor( + () => batches.get("before-drop").requested, + "the pre-drop request should reach the server" + ) + await replayBatch({ revision: "1:12", token: "after-reconnect", targets: [ "player" ] }) + + await waitFor( + () => cancelled.has("before-drop"), + "the reconnect should cancel the request from before the drop" + ) + await page.waitForFunction( + () => document.getElementById("player").dataset.solidObjectsRevision === "1:12", + null, + { timeout: 5000 } + ) + stalled.release() + await page.waitForTimeout(300) + + assert.equal(await revisionOf("player"), "1:12") + assert.match(await textOf("player"), /current player/) +}) + +// Destroy and recreate resets the state revision, so the incarnation has to +// decide the ordering. Comparing revisions alone would reject 2:1 after 1:12. +test("a reconnect after a destroy and recreate applies the new incarnation", async () => { + batches.set("first", { frames: [ frame("player", "1:12", "first incarnation") ] }) + await replayBatch({ revision: "1:12", token: "first", targets: [ "player" ] }) + await page.waitForFunction( + () => document.getElementById("player").dataset.solidObjectsRevision === "1:12", + null, + { timeout: 5000 } + ) + + batches.set("second", { frames: [ frame("player", "2:1", "second incarnation") ] }) + await replayBatch({ revision: "2:1", token: "second", targets: [ "player" ] }) + await page.waitForFunction( + () => document.getElementById("player").dataset.solidObjectsRevision === "2:1", + null, + { timeout: 5000 } + ) + + assert.match(await textOf("player"), /second incarnation/) +}) + +test("a reconnect delivers each payload revision once", async () => { + await page.evaluate(() => { + window.payloads = [] + document.addEventListener("solid-objects:payload", (event) => { + window.payloads.push([ event.detail.instanceId, event.detail.revision ]) + }) + }) + + await replayPayload({ name: "playmat_state", revision: "1:12", payload: { turn: 4 } }) + await replayPayload({ name: "playmat_state", revision: "1:12", payload: { turn: 4 } }) + await replayPayload({ name: "playmat_state", revision: "1:13", payload: { turn: 5 } }) + await page.waitForFunction(() => window.payloads.length >= 2, null, { timeout: 5000 }) + await page.waitForTimeout(200) + + const delivered = await page.evaluate(() => window.payloads) + assert.deepEqual(delivered, [ [ 1, 12 ], [ 1, 13 ] ]) +}) + +test("a stale payload replayed after a newer one is dropped", async () => { + await page.evaluate(() => { + window.payloads = [] + document.addEventListener("solid-objects:payload", (event) => { + window.payloads.push(event.detail.payload.turn) + }) + }) + + await replayPayload({ name: "playmat_state", revision: "1:13", payload: { turn: 5 } }) + await page.waitForFunction(() => window.payloads.length === 1, null, { timeout: 5000 }) + await replayPayload({ name: "playmat_state", revision: "1:12", payload: { turn: 4 } }) + await page.waitForTimeout(200) + + assert.deepEqual(await page.evaluate(() => window.payloads), [ 5 ]) +}) + +test("a reconnect burst applies alongside a payload in the same scope", async () => { + await page.evaluate(() => { + window.payloads = [] + document.addEventListener("solid-objects:payload", (event) => { + window.payloads.push(event.detail.revision) + }) + }) + batches.set("burst", { + frames: [ + frame("player", "1:12", "current player"), + frame("controls", "1:12", "current controls") + ] + }) + + await replayBatch({ + revision: "1:12", + token: "burst", + targets: [ "player", "controls" ] + }) + await replayPayload({ name: "playmat_state", revision: "1:12", payload: { turn: 4 } }) + await page.waitForFunction( + () => + window.payloads.length === 1 && + document.getElementById("controls").dataset.solidObjectsRevision === "1:12", + null, + { timeout: 5000 } + ) + + assert.equal(await revisionOf("player"), "1:12") + assert.deepEqual(await page.evaluate(() => window.payloads), [ 12 ]) +}) diff --git a/test/integration/component_batch_test.rb b/test/integration/component_batch_test.rb index 51bb399..52d11f9 100644 --- a/test/integration/component_batch_test.rb +++ b/test/integration/component_batch_test.rb @@ -147,6 +147,67 @@ class BoardActor < SolidObjects::Actor end end + # A dropped connection is the worst moment for request amplification: a + # server restart reconnects every client at once. Reconnect must cost what a + # live invalidation costs. + test "a reconnect batches the components that share a batch" do + subscriptions = subscriptions_for( + player: "playmat", + controls: "playmat", + library: "playmat" + ) + + streams = subscriptions.reconnect_refreshes(advanced_snapshot) + + assert_equal 1, streams.length, + "a reconnect should cost one request per batch, not one per component" + assert_includes streams.first, "solid-objects-batch-refresh" + assert_equal 3, streams.first[/data-targets="([^"]+)"/, 1].split.length + end + + test "a reconnect keeps unbatched components on their own refresh" do + subscriptions = SolidObjects::ComponentSubscriptions.new( + registrations(player: "playmat", controls: "playmat", chat: nil) + ) + + streams = subscriptions.reconnect_refreshes(advanced_snapshot) + + assert_equal 2, streams.length + assert_equal 1, streams.count { |stream| + stream.include?("solid-objects-batch-refresh") + } + end + + test "a reconnect refreshes distinct batches separately" do + subscriptions = SolidObjects::ComponentSubscriptions.new( + registrations(player: "playmat", chat: "sidebar") + ) + + streams = subscriptions.reconnect_refreshes(advanced_snapshot) + + batches = streams.map { |stream| stream[/data-batch="([^"]+)"/, 1] } + assert_equal %w[playmat sidebar], batches.sort + end + + test "a reconnect records what it transmitted for every batched component" do + subscriptions = subscriptions_for(player: "playmat", controls: "playmat") + current = advanced_snapshot + + subscriptions.reconnect_refreshes(current) + replayed = subscriptions.refreshes_for( + invalidation("player", revision: current.revision) + ) + + assert_empty replayed, + "an invalidation already covered by the reconnect must not refresh again" + end + + test "a reconnect leaves current components alone" do + subscriptions = subscriptions_for(player: "playmat", controls: "playmat") + + assert_empty subscriptions.reconnect_refreshes(snapshot) + end + test "the batch url requests every changed component once" do group = registrations(player: "playmat", controls: "playmat") @@ -185,6 +246,15 @@ def registrations(**batches) end end + # Models a client whose registrations were signed before the state moved on, + # which is what a reconnect after a dropped connection looks like. + def advanced_snapshot + current = snapshot + Struct + .new(:instance_id, :revision) + .new(current.instance_id, current.revision + 1) + end + def subscriptions_for(**batches) SolidObjects::ComponentSubscriptions.new(registrations(**batches)) end From f393121724d489ee50f4b93c8389f7594d105906 Mon Sep 17 00:00:00 2001 From: Lucas Carlson Date: Mon, 10 Aug 2026 09:58:23 -0700 Subject: [PATCH 2/2] test: release every gated response on teardown A gate the test never releases, because an assertion failed before it got there, leaves its route awaiting forever. Gates now register themselves and an afterEach hook drains them, so this cannot be reintroduced by a future test that forgets. The reported hang does not reproduce: closing the browser destroys the client socket, so the suspended route holds no live handle and the runner exits. Forcing the assertion to fail reported the failure and exited in seven seconds. The cleanup is worth having regardless. --- test/browser/reconnect.test.mjs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/test/browser/reconnect.test.mjs b/test/browser/reconnect.test.mjs index c99e94f..d1befe1 100644 --- a/test/browser/reconnect.test.mjs +++ b/test/browser/reconnect.test.mjs @@ -1,5 +1,5 @@ import assert from "node:assert/strict" -import { test, before, after, beforeEach } from "node:test" +import { test, before, after, beforeEach, afterEach } from "node:test" import { startServer, openPage, loadPage, frameHtml } from "./browser_test_helper.mjs" // A reconnecting subscription replays the current state as a burst of refresh @@ -14,6 +14,11 @@ let page const batches = new Map() const components = new Map() const cancelled = new Set() +const gates = [] + +function releaseGates() { + while (gates.length) gates.pop()() +} before(async () => { const started = await startServer({ @@ -47,10 +52,15 @@ before(async () => { }) after(async () => { + releaseGates() await browser?.close() server?.close() }) +afterEach(() => { + releaseGates() +}) + beforeEach(async () => { batches.clear() components.clear() @@ -67,11 +77,15 @@ async function waitFor(condition, message) { assert.fail(message) } +// A gate a test forgets to release, because an assertion failed before it got +// there, leaves its route awaiting forever and turns a reported failure into a +// hung suite. Every gate is released when the test that made it finishes. function gate() { let release const promise = new Promise((resolve) => { release = resolve }) + gates.push(release) return { promise, release } }