Repository navigation
Make the plan update rate limit visible - #373
Conversation
|
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
left a comment
There was a problem hiding this comment.
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:updateframes reachsubmit()(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 = 10caps 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.
Wireretries with jittered exponential backoff, starting at 250–750 ms.#attemptsresets 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.
|
Hold lifted. |
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>
8915db6 to
818564d
Compare
Why
submit()inapps/server/src/plan/service.tsdropped 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
fail(ws, rid, "rate limited").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.Server flow
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:rate limitedand their rids, one warning, no closeThe 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 head818564d0passed hosted validation, E2E, and container checks.🤖 Generated with Claude Code