githubnext/chopinPublic

Let's compose rich plans together

Keep large bracket-heavy pastes from stalling the server #389

MergedMaggieAppletonmerged 1 commit intomainfromrefs/pull/389/head
  1. MA

    ## Why

    A reviewer pasted a large block of text into a document. The server rejected it, then lost its storage writer lease and shut down.

    I reproduced this. The cause is not the rejection path or lease timing. It is two quadratic loops that run synchronously on the event loop whenever the server projects a document:

    - **micromark (parse):** at every unescaped `]`, the parser looks back through the whole paragraph for an opener. Our serializer escapes `[` but leaves `]` alone, so `room.apply` re-parses prose full of brackets in quadratic time. 32 KB of `]` takes 5.2 s to parse. 128 KB takes minutes. - **mdast-util-to-markdown (serialize):** `safe()` checks each escape with `positions.includes(position)`, so serializing gets quadratic in the number of escaped characters. 128 KB of `]` took 2.3 s per projection, and `apply` projects several times.

    During either loop the lease renewal timer can't run. After 25 s the safety watchdog fires and the server stops. The watchdog is doing its job: a process that can't prove it still holds the lease should stop.

    The `a|` paste from the report is fast on its own (about 30 ms). In the reviewer's document, the stall came from bracket-heavy content (their earlier tests pasted many `[site](…)` / `![i](…)` strings as literal text). A single 128 KB paste of `[a]` reproduces it on `main`.

    ## What changed

    - `packages/dialect/src/serialize.ts`: escape every `]` in prose, mirroring the default rule for `[`. The parser no longer runs its lookback on server-produced source. Links, references and footnotes are unchanged. - `patches/mdast-util-to-markdown@2.1.2.patch`: one line. `positions.includes(position)` becomes `Object.hasOwn(infos, position)`. Same behaviour, now linear. - Canonical output changes only for prose that contains `]` (now `\]`). Restore already accepts `serialize(parse(stored))`, so stored documents load unchanged and are rewritten on their next edit.

    ## Evidence

    Two browser contexts on a fake-GitHub server (port 8894), with a preload that logs event-loop stalls. One context pastes 128 KB of `[a]` into a document that has open questions and a comment.

    **Before (main):** ``` [dbg] apply 56725ms ok=true n=1 [lag] event loop blocked ~341070ms chopin: storage writer lease was not renewed before its safety deadline chopin: plan persistence failed - StorageError: storage lease chopin:writer is no longer held ``` The second context never received the paste, and the process exited. The reviewer's original log shows the same sequence after `rejected batch: source-too-large`.

    **After:** the paste is accepted and reaches the second context within 5 s. A second paste over the limit gets `rejected batch: source-too-large` and the room rebuilds. The longest event-loop stall is 824 ms, and the server keeps running.

    `room.apply` on a 128 KB paste, server only:

    | Paste | Before | After | |---|---|---| | `[a]` × 64 KB | 12.7 s | 0.65 s | | `]` × 128 KB (refused, too large) | minutes | 0.27 s | | `**a` × 128 KB | 2.1 s | 0.2 s |

    ## Deploy notes

    - **Drop the bun patch when upgrading to mdast-util-to-markdown 2.1.3 or later.** Upstream has the same fix, but 2.1.3 also changes canonical output, so treat that upgrade as its own migration. - **`sourceHash` changes for documents that contain `]`.** Their first projection after deploy writes `\]`, so the hash changes once. Expect a one-time regeneration of the summary and description for those documents, and a spurious `changedSinceReference` on Chat references made before the deploy. - **Anchors stored before deploy.** A block digest computed before deploy won't match a block that contains `]`. This only matters when the anchor's Yjs relative position was already unresolvable (for example after an epoch replacement). Those anchors orphan rather than recover by digest. That case is narrow.

    ## Testing

    - `bun test packages/dialect apps/server/src/plan/room.test.ts`: 148 pass. - New dialect tests check that `]` is escaped and round-trips, that a limit-sized `[a]` paragraph re-parses in under 5 s, and that links and footnotes are unchanged. - New room test runs a heartbeat timer during an accepted and a refused limit-sized bracket paste and asserts no stall over 5 s. Without the fix it fails: longest stall 134 s, test run 358 s. With the fix it passes in 2.4 s. - `bun test` (full): 4065 pass, 0 fail. One earlier full run had a single unrelated failure under heavy machine load (load average about 80) that did not recur. - `bun run ci`, `bun run types`: pass. - Hosted E2E passed on the final rebased head (`eb995b73`), alongside validation and container.

    ## Not in this PR

    - **`![a` parse is still slightly superlinear:** about 1.3 s at 256 KB. That is well inside the lease deadline. Follow-up. - **A visible message when a paste is refused for size.** A refused update still causes a silent `plan:reset` for everyone. Showing "This paste is too large for the document" only to the person who pasted needs a protocol field and new editor UI, so it should be designed separately. - **Client-side paste time.** A literal 128 KB paste takes 5–8 s inside Lexical in the browser, and up to 33 s when the document is already large. That only affects the pasting tab.

    🤖 Generated with [Claude Code](https://claude.com/claude-code)

    ## Paste and projection flow

    ![Bracket-heavy paste before and after: escaping closing brackets and keyed lookup keep document projection responsive](https://github.com/user-attachments/assets/779be14a-fe6f-43d8-8bf7-4c5855f1a70d)

  2. MA
    Author

    **Hold — please don't merge yet.** Changes canonical serialization (escapes `]`) and adds a dependency patch to fix a server event-loop stall that can drop the writer lease. A review is in progress; I'll post the verdict here.

    🤖 Generated with [Claude Code](https://claude.com/claude-code)

  3. MaggieAppletonhead-ref-force-pushed eb995b7329fblast month on GitHub

  4. MA
    Author

    **Review:** Looks good

    I checked the patch against upstream, the digest and restore risks, round-trip stability, and a wider set of pathological pastes.

    **1. Stored documents and anchors: safe.** I seeded a document with `]` in a heading, prose, task items and a long paragraph on the merge base. It had a Decision card related to two `]` blocks and two open comment passages over `]` text. I then loaded the same Postgres database under this branch. - Restore passes. Nothing was rejected, and the log has no `stored plan source does not match its Yjs checkpoint` or `session:error`. - `Service.open` re-expresses every anchor through its Yjs relative position, which still resolves, so the digests are re-minted in the new format. The anchors are not orphaned under `rebase` or `carry`. The passages still `locate`. - In two browser contexts, comment highlights render. An edit containing `]` reaches the other context and survives a reload. Afterwards the anchors still resolve, including recovery by digest alone onto a fresh epoch. - Rolling back is also safe: `main` accepts `\]` source through `serialize(parse(source))`.

    Residual, not blocking: a raw stored digest from before this change can no longer recover by digest *alone*. I confirmed this onto a fresh epoch: main recovers it, this branch orphans it. This only matters for an anchor whose position was already unresolvable when the deploy happens, which in practice means `recoverOnNextEdit` anchors waiting for a deleted `]` block to reappear. Those lose their one-edit recovery window. Narrow enough to accept, but worth a line in the PR description.

    **2. Round-trip: stable.** `serialize(parse(x))` is idempotent and leaves the tree unchanged for all of these: inline, full, collapsed and shortcut references; unresolved references; footnote refs and definitions containing `]`; `- [x]` / `- [ ]` task items; code spans (including `` x]`]y ``) and fenced code; inline and block math; tables; headings; emphasis and strikethrough; link text with `\]`; link titles and `<…>` destinations containing `]`; `![alt]]`; `\\]`; `&#93;`. Code, math, destinations and task markers keep a raw `]`.

    **3. Patch: semantically identical.** `positions` and `infos` always get the same keys at the same time, so `Object.hasOwn(infos, position)` matches `positions.includes(position)` exactly. Upstream fixed this the same way in **2.1.3** (2026-09-27): `own.call(infos, position)`. Patching is the right call for now, because 2.1.3 also changes serialization: it adds CR/LF escapes, autolink escapes, HTML-kind block joining, and a rewritten `container-phrasing` emphasis encoding. Upgrading would mean a canonical-output migration. Suggestion: add a note that this patch should be dropped once we move to ≥2.1.3.

    **4. Other pathological pastes: no remaining stalls.** I timed a single-paragraph paste through `room.apply` at 32, 64 and 128 KB for `]`, `[`, `[a]`, `*`, `_`, `` ` ``, `<`, `\`, `(`, `**a`, `_a`, `*a `, `![a`, `![`, `[a](`, `$`, `~`, `|`, `&`, `<a`, `{`, `[^`, `a](`, `[a`, `](`, `[](`, `<b>`, `` `a ``, ` ``a `, `&amp`, `&#`, and newlines. All are linear and under 300 ms at 128 KB, except `![a` / `a![`. Those still grow about 3× per doubling in **parse**: 0.66 s at 128 KB, and **1.3 s at the 256 KB source limit**. Main takes 3.6 s for `![a` and 43 s for `![a]` at 128 KB. So that path is bounded well under the lease deadline. It's a follow-up, not a blocker.

    **5. Tests: meaningful.** With the bun patch reverted, the room test fails (it times out at 5 s). With the `UNSAFE` rule removed, both new tests fail, and the room test stalls for 145 s. 148/148 pass on the branch.

    Minor: the change also alters `sourceHash` for any document containing `]`. As a one-off after deploy, in-flight summary/description commits will no-op and regenerate, and Chat references created earlier will report `changedSinceReference` for those documents. That's benign, but it could go in the "Canonical output changes" note too.

  5. MA
    Author

    Hold lifted.

  6. MaggieAppletonmerged 9800ccf6e424last month on GitHub

  7. MaggieAppletonclosedlast month on GitHub

  8. MaggieAppletonhead-ref-deletedlast month on GitHub