Skip to content

Make the plan update rate limit visible - #373

Merged
MaggieAppleton merged 1 commit into
mainfrom
design/plan-rate-limit-visibility
Oct 8, 2026
Merged

MaggieAppleton merged 1 commit into
mainfrom
design/plan-rate-limit-visibility

Conversation

@MaggieAppleton

@MaggieAppleton MaggieAppleton commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Why

submit() in apps/server/src/plan/service.ts dropped every update over 200 a second per socket. It sent no reply and logged nothing. In the Decisions audit this cost a user's typing (see #358): Yjs holds back later updates until a dropped one arrives, so nothing else that client typed got through, and nothing in the log showed why. #358 stops current clients from going over the limit and makes them resend. A tab still running an older bundle only replays its outbox when it reopens.

What changed

  • Refusal on the wire. A dropped update gets fail(ws, rid, "rate limited").
  • Log line. One warning per one-second window of drops, naming the handle and document.
  • Forced reconnect. After drops in three consecutive windows, the server closes the socket with 4429 plan updates too fast. The web client retries every close except 4404, so an older client reconnects, reopens the document and replays its unacknowledged updates.
  • Accepted updates are untouched. Queueing, the 5 ms grouping, and persistence before acknowledgement or relay work as before.

Server flow

Diagram of document updates passing through the server limit, with a rate-limited reply, warning, and reconnect after repeated over-limit windows.

The server reports dropped updates and lets an older client recover by reconnecting after sustained over-limit traffic.

Testing

  • New apps/server/src/plan/rate-limit.test.ts (3 tests) covers:

    • 205 updates in one window: 200 acknowledged, 5 refused with rate limited and their rids, one warning, no close
    • drops in three consecutive windows close the socket with 4429
    • a quiet gap resets the count

    The first two fail without the change.

  • bun test apps/server/src/plan: 191 pass.

  • bun run types: clean. bun run ci: passes. bun run fix: no further changes. Final rebased head 818564d0 passed hosted validation, E2E, and container checks.

🤖 Generated with Claude Code

@MaggieAppleton

Copy link
Copy Markdown
Collaborator Author

Hold — please don't merge yet. Server-side rate-limit/socket-close change; a review is in progress. I'll post the verdict here.

🤖 Generated with Claude Code

@MaggieAppleton MaggieAppleton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Looks good

Checked against AGENTS.md (runtime model, persistence before publication, collaboration traps) and #358's client batching and resend.

1. Can a real user trip the close? In practice, no.

  • Only browser plan:update frames reach submit() (main.ts). Planner and MCP edits are server-authored and skip the meter.
  • The meter is keyed per (plan, ws). Each tab has its own socket and its own meter.
  • A paste is one Yjs transaction. IME composition and key repeat (about 30/s) stay far below 200/s.
  • With #358, SEND_MS = 10 caps a client at about 100 flushes a second, plus merged resends with backoff. 200/s is out of reach for normal use.

2. Reconnect-storm risk: low.

  • The meter belongs to the socket, so a reconnect starts again with zero strikes.
  • An old client's reopen replays at most 1,000 outbox items in one burst. That counts as one strike, and the close needs three consecutive windows.
  • Wire retries with jittered exponential backoff, starting at 250–750 ms. #attempts resets on open, so a client that is really sustained over the limit would cycle about every 3 s. That is bounded and logged.

3. Logging: contains the handle and document ID only, with no update content or tokens. At most one line per second per socket.

4. Reply shape: fail() sends a session:error with the request's rid, which matches Protocol.Failure. Older clients never register a pending entry for plan:update rids, and on main nothing listens to session:error, so the reply is ignored. #358's listener only parks on implementation is active, so rate limited leaves the resend path in charge, which is correct.

5. Tests: meaningful. I reverted service.ts to main and the first two tests failed. The quiet-gap test covers the consecutive-window reset. bun test apps/server/src/plan: 191 pass locally. CI is green.

Non-blocking note: an old bundle that goes over the limit for only one or two windows still strands its later edits until it next reopens, because the close needs three windows. Humans can't type that fast, so I'd leave it as is.

@MaggieAppleton

Copy link
Copy Markdown
Collaborator Author

Hold lifted.

@MaggieAppleton MaggieAppleton left a comment •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate of the review above, posted twice by mistake.

An update over the per-socket limit was dropped with no reply and no log.
The server now refuses it with `rate limited`, warns once per window, and
closes a connection that stays over the limit for three consecutive windows,
so a client without resends reconnects and replays its outbox.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MaggieAppleton
MaggieAppleton force-pushed the design/plan-rate-limit-visibility branch from 8915db6 to 818564d Compare October 8, 2026 04:30
@MaggieAppleton
MaggieAppleton merged commit 1eba652 into main Oct 8, 2026
3 checks passed
@MaggieAppleton
MaggieAppleton deleted the design/plan-rate-limit-visibility branch October 8, 2026 04:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant