fix(redis): bound the three unbudgeted stream writers by bytes - #7568
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR adds byte-aware Redis bounds to the three stream writers that previously relied primarily on entry counts:
Confidence Score: 5/5The PR appears safe to merge; no outstanding correctness, security, or repository-rule violations remain. The current implementation atomically couples replay writes with budget reservations, preserves atomic cleanup, prunes table streams using retained-byte accounting, and compacts file-document streams without dropping unintegrated or boundary deltas. All previous actionable findings are fixed and resolved, while the legacy table-stream migration concern was explicitly withdrawn after its bounded behavior was accepted.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/core/redis/byte-budget.server.ts | Centralizes Redis budget limits, keys, atomic Lua reservation logic, refusal parsing, and telemetry. |
| apps/sim/lib/copilot/request/session/buffer.ts | Adds byte-budgeted replay persistence, UTF-8-aware chunking, non-fatal refusals, and atomic data-plus-owner cleanup. |
| apps/sim/lib/copilot/request/session/writer.ts | Propagates user budget scope and stops only replay persistence after a budget refusal. |
| apps/sim/lib/realtime/event-log.ts | Adds atomic retained-byte accounting and oldest-first pruning for table event streams. |
| apps/realtime/src/handlers/file-doc-store.ts | Adds byte-triggered Yjs compaction with per-entry accounting, inclusive-boundary handling, peer coverage, and retry cooldown. |
| apps/sim/lib/execution/event-buffer.ts | Migrates execution budgeting to the consolidated shared Redis-budget module. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Stream write] --> B{Writer}
B -->|Copilot replay| C[Measure UTF-8 batch bytes]
C --> D[Atomic budget reservation and append]
D --> E{Budget accepted?}
E -->|Yes| F[Persist replay events]
E -->|No| G[Stop replay persistence; live stream continues]
B -->|Table event log| H[Atomic append and byte accounting]
H --> I[Prune oldest entries above ceiling]
B -->|File-document delta| J[Track tailed delta bytes by stream ID]
J --> K{Foldable bytes above threshold?}
K -->|Yes| L[Append compacted Yjs snapshot]
L --> M[Trim only entries before fold boundary]
K -->|No| N[Continue retaining deltas]
Reviews (8): Last reviewed commit: "refactor(redis): drop the clear-buffer s..." | Re-trigger Greptile
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 16 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 16 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 16 files
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Fix all with cubic | Re-trigger cubic
|
@cubic-dev-ai review this PR |
There was a problem hiding this comment.
All reported issues were addressed across 16 files
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Fix all with cubic | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 16 files
Confidence score: 2/5
- The takeover logic in
apps/realtime/src/handlers/file-doc-store.tsexcludes everyAGENT_FIELDentry, conflating ordinary agent/preview updates with compaction snapshots; large preview deltas can therefore be omitted frompendingDeltasand mishandled during takeover. Separate these markers or otherwise preserve preview deltas in the takeover accounting. - The tailer in
apps/realtime/src/handlers/file-doc-store.tsapplies large markerless deltas without adding their bytes topendingDeltas, so the size check can report zero and allow a read-only room to retain an oversized stream until the 400-entry limit; account for markerless delta bytes before enforcing the limit.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/realtime/src/handlers/file-doc-store.ts">
<violation number="1" location="apps/realtime/src/handlers/file-doc-store.ts:402">
P1: During takeover, this excludes all `AGENT_FIELD` entries from `pendingDeltas`, but ordinary agent/preview updates use the same marker as agent-only compaction snapshots. A stream with large preview deltas can therefore evade the byte compaction trigger and remain oversized; distinguish actual compaction snapshots from ordinary agent deltas when adopting entries.</violation>
<violation number="2" location="apps/realtime/src/handlers/file-doc-store.ts:786">
P2: When another task publishes a large markerless delta, this tailer applies it but never adds its bytes to `pendingDeltas`, so this check sees zero. A read-only room can therefore retain an oversized stream until 400 entries or TTL; record and deduplicate tailer-applied deltas before forcing compaction.</violation>
</file>
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Fix all with cubic | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
The copilot stream buffer, the Tables event log and the realtime file-doc streams were each bounded by entry count and nothing else. An entry cap bounds how many entries a key holds and says nothing about how large each one is, so a single writer emitting large entries reaches gigabytes well inside its cap — which is how one file-edit stream filled a shared Redis under a 100,000-entry cap and evicted the whole keyspace. Each writer gets the bound its read semantics allow: - Copilot's replay buffer is read from a cursor and must stay contiguous, so it now reserves bytes against per-stream and per-user ceilings inside the same Lua that appends, and the writer soft-stops persistence on refusal rather than failing the live stream. - The Tables event log is a live feed whose readers already refetch on a prune, so it drops oldest-first once past a byte ceiling — the existing `pruned` path carries it, with the running total kept in meta under the same TTL as the bytes it counts. - The file-doc streams are Yjs deltas replayed in full by a task attaching later, so dropping the oldest would lose edits and a native MAXLEN bound is unsafe. Compaction, which folds deltas into a snapshot first, is lossless — it now triggers on appended bytes as well as entry count. Also folds `lib/execution/redis-budget.server.ts` into the shared module rather than leaving two definitions of the same prefix and ceilings writing the same Redis keys. Key layout and every execution limit are unchanged, and pinned by test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-seeding the counter with the snapshot's own size left any document larger than the ceiling permanently over it, forcing a full snapshot append on every subsequent keystroke — the write amplification the threshold exists to prevent. The counter measures edit churn since the last fold, so a stream settles at one snapshot plus that much churn. Also self-corrects the Tables byte counter whenever its buffer trims to a single entry, so an independently evicted events key cannot leave the accumulator over-reporting and pin the buffer at one entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Measure ceilings in UTF-8 bytes on both the copilot and Tables paths, so the TypeScript checks bound a stream the same way the Lua's `string.len` does rather than under-reporting every non-ASCII frame. - Split an oversized copilot batch on the per-write ceiling instead of refusing it. A flush carries whatever accumulated since the last one, so a run of large frames can exceed the ceiling collectively while each frame is individually writable; refusing that stopped replay for the rest of the stream over a batching artefact. A single frame past the ceiling is still refused. - Re-check the copilot soft stop when an in-flight append resolves, not only at enqueue, so a batch queued behind a refusal cannot land and leave replay holding later events but not the refused ones. - Deduct rather than zero the file-doc compaction counter, and only once the trim succeeds, so a failed fold leaves the trigger armed and a concurrent publish's bytes survive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…unters
The compaction counter was a single total, so a fold deducted bytes for entries
`XTRIM MINID` had retained — anything published past the fold boundary the
tailer had not yet integrated. Those bytes are still in Redis, so the trigger
disarmed while the stream kept growing. Deltas are now tracked as `{id, bytes}`
and dropped only once a trim provably removed them.
Arming the trigger on retained bytes would be the opposite fault: a fold that
reclaims nothing would re-arm immediately and force a full snapshot append per
publish. Only bytes at or before the fold boundary arm it, and because that
boundary moves in the tailer rather than on publish, the tailer re-checks it —
otherwise a burst of large edits followed by silence would sit unfolded until
the next keystroke.
Also releases the copilot owner counter when the buffer is cleared, crediting
the user counter by exactly what the owner held. Those keys are deleted rather
than expired, so the counter otherwise outlived its data and a retry reusing the
streamId would be refused against bytes that no longer exist.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…undary Deleting a copilot buffer and releasing its reservation were two round trips, so a concurrent append landing between them kept its events stored with its reservation already erased. Both now run in one script, composed from a rendered release fragment the same way the reservation is. `XTRIM MINID upTo` is inclusive — it keeps the entry whose id equals the boundary. Accounting treated that entry as folded, so its bytes stopped counting while they were still in Redis, and a large paste landing exactly on the boundary could leave the trigger disarmed. Both directions now use the same strict/inclusive split: only entries strictly before the boundary arm the trigger, and only those are dropped once a trim removes them. Replaces the tests' `any` casts with a typed accessor, per the repository's TypeScript rule. This immediately caught injected test rooms missing `pendingDeltas`, which made compaction throw into its catch while the assertions still passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An owner id is not proof of who wrote the bytes, so crediting the user counter on clear let anyone able to name a stream decrement a ceiling they never charged. That is the one direction that must not be possible: a counter driven down grants writes rather than denying them. The clear now drops the owner counter only. The user counter's fixed window settles it instead — it already tolerates accruing bytes Redis has dropped, and this is the same over-count bounded by the same window. The scope threading that existed only to credit it is removed with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d retrying hot The copilot owner counter used a fixed one-hour window while the stream TTL is configurable and defaults to exactly that. Raising COPILOT_STREAM_TTL_SECONDS would have let the counter expire under live data, and the next write would see zero reserved and grant another full ceiling. The window is now the larger of the two, so a counter can never expire before what it accounts for. A failed fold deliberately leaves the trigger armed, but the snapshot XADD lands before the XTRIM — so a persistent trim failure retried immediately, appending a full-document snapshot every time and turning a Redis blip into the write amplification the threshold exists to prevent. A forced fold now waits out a cooldown after a failure. The entry-count path is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A room attaching to an existing stream started from an empty ledger, so a multi-megabyte stream under the entry threshold stayed unfolded while that room's own heartbeat kept refreshing its TTL — a restart or handoff could hold one open indefinitely. `catchUp` already reads every entry to rebuild the doc, so adopting their bytes costs no extra work. Plain deltas only: a compaction snapshot is the result of a fold rather than something a fold can reclaim, so counting one would arm the trigger against itself. The trigger is re-checked once, after catch-up, since nothing else re-checks until the next local publish and a read-only participant never makes one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…iler A fold of an agent-only stream is stamped with the agent marker to preserve the no-persist guarantee, which made it indistinguishable from an ordinary agent preview frame. Excluding that marker from accounting therefore dropped preview deltas — the largest payloads there are, and the ones that caused the incident — while including it would let a snapshot arm the trigger against its own output. A dedicated field settles it without touching origin selection. With the ambiguity gone, accounting moves from the publish path to `applyEntry`, which observes every entry the room tails: this task's appends, a peer task's, and one published with no room attached anywhere. Publish-side accounting could only ever see local writes, and contributed nothing to the trigger before the tailer caught up regardless, since only entries at or before the fold boundary arm it. The ledger is now a Map keyed by entry id, so an entry observed twice is recorded once. Entries written before this field carry no marker and count as deltas, which over-arms by at most one fold that then trims them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A variadic DEL is already a single atomic command, so the Lua script and the render function that built it achieved nothing a plain `del(events, seq, abort, ownerBudget)` does not. The keys carry no hash tag either, so the script had the same cluster-slot constraint it appeared to avoid. Also deletes `RedisBudgetExceededError`, which was defined and never thrown, and consolidates four rounds of stacked comments in the fold down to the one that still describes the code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a049852 to
235c87c
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Why
Three Redis writers were bounded by entry count and nothing else. An entry cap bounds how many entries a key holds and says nothing about how large each one is — a writer emitting large entries reaches gigabytes well inside its cap. That is what filled the shared Redis and evicted the entire keyspace: one stream under a 100,000-entry cap.
Redis has no per-tenant memory limit, so the bound has to be applied in the application. This adds one, shaped to what each writer's readers can tolerate.
What
Copilot stream buffer — a replay buffer read from a cursor, so it must stay contiguous. Bytes are now reserved against per-stream and per-user ceilings inside the same Lua that appends (a budget checked in a separate round-trip is one two concurrent writers both pass). A refusal is returned, never thrown: the writer stops persisting for replay and the live stream is unaffected.
Tables event log (
table:stream:*) — a live feed whose readers already handle a prune by refetching and resuming from latest. So it drops oldest-first past a byte ceiling rather than refusing, riding the existingprunedpath. The running total lives in the meta hash under the same TTL as the bytes it counts.File-doc streams (
filedoc:stream:*) — Yjs deltas, replayed in full by a task attaching later. Dropping the oldest loses edits outright, so a nativeMAXLENretention bound is not safe here. Compaction is, because it folds deltas into a snapshot before trimming — it now triggers on appended bytes as well as entry count, checked every publish since one entry can cross the ceiling alone.Consolidation —
lib/execution/redis-budget.server.tsfolded into the shared module. Two files were defining the same key prefix and the same ceilings while writing the same Redis keys; a change to one would have silently disagreed with the other. Key layout and every execution limit are unchanged, and now pinned by test.Verification
apps/simlib suites (execution, uploads, copilot, realtime, table, core/redis): 4,991 passed.apps/realtime: 289 passed.bun run lint,bun run check:audits(45 audits),bun run type-checkacross all workspaces: clean.