Skip to content

Commit 232fb15

Browse files
committed
fix(copilot): stream append previews as deltas instead of a snapshot per token
An `append` preview is `existingContent + streamed`, so forcing a full snapshot on every emission re-sent the entire file once per streamed token. Cost is `O(file x tokens)` into the stream buffer, whose only trim is a 100,000-member rank cap — a count cap does not bound a member whose size is the file. One 250 KB file cost gigabytes. The forced snapshot was correct when it was written: the preview consumer could not merge a delta. It gained that ability two months later and the producer was never relaxed, so the branch has been dead weight since — `update` has shipped deltas through the same reducer ever since, and `deriveFilePreviewSession` is the only thing in the app that reads `contentMode`. Removing it changes no rendered text. The two conditions that make a snapshot necessary still force one: a base that diverges mid-stream fails `startsWith`, and the checkpoint interval still emits a recoverable full snapshot so no delta chain runs longer than a second. `file-preview-append-roundtrip.test.ts` drives the real producer into the real consumer and asserts the reconstructed text is byte-identical, including at token-scale chunking over a 250 KB base, across a diverging base, and under duplicate delivery. Both halves were verified to fail when the behaviour they pin is reverted. Measured against the session that prompted this: 2.29 GiB -> 55 MB.
1 parent b385131 commit 232fb15

4 files changed

Lines changed: 303 additions & 3 deletions

File tree

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
# Redis Payloads
2+
3+
Rules for anything written to the shared Redis. `sim-caching.md` governs *in-process*
4+
caches; this governs the cluster every process shares.
5+
6+
Redis is one fixed-size box with `maxmemory-policy allkeys-lru` and no AOF. There is no
7+
per-feature quota and no isolation: a key that grows without a bound does not fail its
8+
own feature, it evicts *somebody else's* keys. Idempotency records, usage reservations,
9+
cancellation flags and distributed locks all live there, so an unbounded write is not a
10+
performance problem — it is a correctness problem in a feature you did not touch.
11+
12+
## Cap the bytes, never the count
13+
14+
A count cap is not a memory bound. `ZREMRANGEBYRANK(key, 0, -limit-1)` bounds how many
15+
members a key holds; it says nothing about how large each member is. A key holding 400
16+
members of 250 KB is 100 MB and passes a 100,000-member cap untouched.
17+
18+
Cap the dimension you are actually short of:
19+
20+
```ts
21+
// ✗ Bad — bounds cardinality, not memory
22+
pipeline.zadd(key, seq, JSON.stringify(envelope))
23+
pipeline.zremrangebyrank(key, 0, -EVENT_LIMIT - 1)
24+
25+
// ✓ Good — refuse the write that would exceed the byte budget
26+
const bytes = Buffer.byteLength(payload)
27+
const allowed = await reserveRedisBudget({ ownerId, category, bytes, operation })
28+
if (!allowed) return // drop, offload, or degrade — but do not write
29+
```
30+
31+
A count cap on top of a byte cap is fine and often useful. A count cap *instead of* one
32+
is the bug this rule exists to prevent — it reads like a guard in review and holds
33+
nothing.
34+
35+
## Use the budget that already exists
36+
37+
`lib/execution/redis-budget.server.ts` is the sanctioned implementation. Three tiers,
38+
enforced atomically in the same Lua as the write:
39+
40+
| Limit | Value | Stops |
41+
|---|---|---|
42+
| `maxSingleWriteBytes` | 8 MB | one pathological value |
43+
| `maxExecutionBytes` | 64 MB | one runaway session |
44+
| `maxUserBytes` | 256 MB | one runaway tenant |
45+
46+
Pair it with `lib/execution/payloads/large-value-ref.ts`: above the threshold the value
47+
goes to blob storage and Redis holds a typed reference instead. Do not write a second
48+
budget module — extend this one to your owner kind.
49+
50+
Any new multi-MB Redis writer must go through it. Today only `event-buffer.ts` and
51+
`user-file-base64.server.ts` do, which is why the copilot stream buffer, the table event
52+
log and the file-doc stream each independently grew a key with no byte bound.
53+
54+
## Never store a value whose size scales with something you do not control
55+
56+
Before writing, name what bounds the value. If the answer is "the size of a user's file",
57+
"the number of tokens the model emits", or "how long the session runs", it is unbounded
58+
and belongs in blob storage behind a reference.
59+
60+
The failure mode to watch for is **re-serialization per event**: emitting a full snapshot
61+
of a growing document on every streamed chunk makes total bytes `O(document × chunks)`
62+
quadratic in the document, and the client already has every prior byte. Send the delta the
63+
producer usually already computed. A snapshot is a recovery affordance; send it on a
64+
timer, not per chunk.
65+
66+
```ts
67+
// ✗ Bad — every chunk carries the whole document
68+
emit({ content: nextText, contentMode: 'snapshot' })
69+
70+
// ✓ Good — snapshot on an interval, delta in between
71+
const isCheckpoint = now - lastSnapshotAt >= CHECKPOINT_INTERVAL_MS
72+
emit(
73+
isCheckpoint || !nextText.startsWith(previousText)
74+
? { content: nextText, contentMode: 'snapshot' }
75+
: { content: nextText.slice(previousText.length), contentMode: 'delta' }
76+
)
77+
```
78+
79+
Throttles must cover every branch that can emit. A throttle written for one operation and
80+
keyed on that operation's name silently exempts every sibling added later — bound the
81+
emission, not the operation.
82+
83+
## Sliding TTLs do not expire
84+
85+
`EXPIRE` on every write means the key dies a TTL after the *last* write, not after the
86+
first. For an append-only key under continuous traffic that is never. A sliding TTL is
87+
correct for a session that should outlive its own idle gaps; it is wrong as the only
88+
bound on a key that grows. Pair it with a byte budget, or set the TTL once on creation.
89+
90+
Cleanup that runs only on a clean close is not a bound either — the process that dies
91+
mid-stream is exactly the one holding the largest key.
92+
93+
## Reviewer checklist
94+
95+
For any new or changed Redis write:
96+
97+
- What is the largest this value can be? If you cannot state it in bytes, it is unbounded.
98+
- Is the cap on bytes, or only on entries?
99+
- Does the TTL slide? If so, what stops the key growing between writes?
100+
- Does the write go through `redis-budget.server.ts`? If not, why is this one exempt?
101+
- If it appends per streamed event, does it send deltas, or re-send the whole state?
102+
- If this key grew 100× tomorrow, which *other* feature breaks first?

apps/sim/lib/copilot/request/go/file-preview-adapter.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,6 @@ export function buildPreviewContentUpdate(
326326
previousText.length === 0 ||
327327
!nextText.startsWith(previousText) ||
328328
operation === 'patch' ||
329-
operation === 'append' ||
330329
now - lastSnapshotAt >= DELTA_PREVIEW_CHECKPOINT_INTERVAL_MS
331330

332331
if (shouldForceSnapshot) {
Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it } from 'vitest'
5+
import { buildPreviewContentUpdate } from '@/lib/copilot/request/go/file-preview-adapter'
6+
import type { FilePreviewSession } from '@/lib/copilot/request/session/file-preview-session-contract'
7+
import { deriveFilePreviewSession } from '@/app/workspace/[workspaceId]/home/hooks/preview/apply-file-preview-phase'
8+
9+
const CHECKPOINT_MS = 1_000
10+
11+
/**
12+
* Producer -> consumer round trip for an `append` preview.
13+
*
14+
* `buildPreviewContentUpdate` decides snapshot vs delta; `deriveFilePreviewSession` is
15+
* the only thing in the app that reads `contentMode`. Emitting deltas instead of a full
16+
* snapshot per chunk is only safe if replaying what the producer emits reconstructs the
17+
* text exactly — this drives the real functions against each other and checks that,
18+
* rather than reasoning about it.
19+
*/
20+
function roundTrip(
21+
chunks: string[],
22+
base: string,
23+
msPerChunk: number
24+
): { rendered: string; expected: string; snapshots: number; deltas: number } {
25+
let lastEmitted = ''
26+
let lastSnapshotAt = 0
27+
let now = 0
28+
let streamed = ''
29+
let session: FilePreviewSession | undefined
30+
let version = 0
31+
let snapshots = 0
32+
let deltas = 0
33+
34+
for (const chunk of chunks) {
35+
streamed += chunk
36+
now += msPerChunk
37+
const nextText = base.length > 0 ? `${base}\n${streamed}` : streamed
38+
const update = buildPreviewContentUpdate(lastEmitted, nextText, lastSnapshotAt, now, 'append')
39+
lastEmitted = nextText
40+
lastSnapshotAt = update.lastSnapshotAt
41+
version += 1
42+
if (update.contentMode === 'snapshot') snapshots++
43+
else deltas++
44+
45+
session = deriveFilePreviewSession(
46+
session,
47+
{
48+
previewPhase: 'file_preview_content',
49+
content: update.content,
50+
contentMode: update.contentMode,
51+
previewVersion: version,
52+
toolCallId: 'tc_1',
53+
toolName: 'prepare_file_edit',
54+
fileName: 'notes.md',
55+
operation: 'append',
56+
} as never,
57+
'stream_1',
58+
new Date(now).toISOString()
59+
)
60+
}
61+
62+
return {
63+
rendered: session?.previewText ?? '',
64+
expected: base.length > 0 ? `${base}\n${streamed}` : streamed,
65+
snapshots,
66+
deltas,
67+
}
68+
}
69+
70+
function chunksOf(text: string, size: number): string[] {
71+
const out: string[] = []
72+
for (let i = 0; i < text.length; i += size) out.push(text.slice(i, i + size))
73+
return out
74+
}
75+
76+
describe('append preview round trip', () => {
77+
it('reconstructs the exact text the user should see, and does it with deltas', () => {
78+
const base = 'Existing file body.\nSecond line.'
79+
const r = roundTrip(chunksOf('The appended paragraph goes here.', 4), base, 20)
80+
81+
expect(r.rendered).toBe(r.expected)
82+
expect(r.deltas).toBeGreaterThan(0)
83+
})
84+
85+
it('holds for realistic token-scale chunking on a large base file', () => {
86+
const base = 'x'.repeat(250 * 1024)
87+
const r = roundTrip(chunksOf('y'.repeat(4096), 10), base, 20)
88+
89+
expect(r.rendered).toBe(r.expected)
90+
expect(r.rendered.length).toBe(250 * 1024 + 1 + 4096)
91+
})
92+
93+
it('still emits a recoverable full snapshot on the checkpoint interval', () => {
94+
// One chunk per 400ms crosses the 1s checkpoint repeatedly.
95+
const r = roundTrip(chunksOf('abcdefghij', 1), 'base', 400)
96+
97+
expect(r.rendered).toBe(r.expected)
98+
expect(r.snapshots).toBeGreaterThan(1)
99+
expect(r.snapshots * CHECKPOINT_MS).toBeGreaterThan(0)
100+
})
101+
102+
it('recovers exactly when the base file changes underneath the stream', () => {
103+
// A divergent base must fall back to a snapshot, not a delta on stale text.
104+
const first = buildPreviewContentUpdate('Old base\nabc', 'New base\nabcd', 100, 200, 'append')
105+
expect(first.contentMode).toBe('snapshot')
106+
expect(first.content).toBe('New base\nabcd')
107+
108+
const session = deriveFilePreviewSession(
109+
undefined,
110+
{
111+
previewPhase: 'file_preview_content',
112+
content: first.content,
113+
contentMode: first.contentMode,
114+
previewVersion: 1,
115+
toolCallId: 'tc_1',
116+
toolName: 'prepare_file_edit',
117+
fileName: 'notes.md',
118+
operation: 'append',
119+
} as never,
120+
'stream_1',
121+
new Date().toISOString()
122+
)
123+
expect(session.previewText).toBe('New base\nabcd')
124+
})
125+
126+
it('ignores a replayed event rather than double-appending its delta', () => {
127+
const base = 'Base.'
128+
const chunks = chunksOf('hello world', 3)
129+
let lastEmitted = ''
130+
let lastSnapshotAt = 0
131+
let now = 0
132+
let streamed = ''
133+
let session: FilePreviewSession | undefined
134+
let version = 0
135+
const emitted: Array<{ content: string; contentMode: string; version: number }> = []
136+
137+
for (const chunk of chunks) {
138+
streamed += chunk
139+
now += 20
140+
const u = buildPreviewContentUpdate(
141+
lastEmitted,
142+
`${base}\n${streamed}`,
143+
lastSnapshotAt,
144+
now,
145+
'append'
146+
)
147+
lastEmitted = `${base}\n${streamed}`
148+
lastSnapshotAt = u.lastSnapshotAt
149+
version += 1
150+
emitted.push({ content: u.content, contentMode: u.contentMode, version })
151+
}
152+
153+
// Deliver every event twice, out of order for the duplicates.
154+
for (const e of [...emitted, ...emitted]) {
155+
session = deriveFilePreviewSession(
156+
session,
157+
{
158+
previewPhase: 'file_preview_content',
159+
content: e.content,
160+
contentMode: e.contentMode,
161+
previewVersion: e.version,
162+
toolCallId: 'tc_1',
163+
toolName: 'prepare_file_edit',
164+
fileName: 'notes.md',
165+
operation: 'append',
166+
} as never,
167+
'stream_1',
168+
new Date().toISOString()
169+
)
170+
}
171+
172+
expect(session?.previewText).toBe(`${base}\n${streamed}`)
173+
})
174+
})

apps/sim/lib/copilot/request/go/stream.test.ts

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -193,14 +193,39 @@ describe('copilot go stream helpers', () => {
193193
expect(extractEditContent('{"content":"tab\\tvalue"}')).toBe('tab\tvalue')
194194
})
195195

196-
it('emits full snapshots for append (sidebar viewer uses replace mode; no delta merge)', () => {
196+
/**
197+
* Append extends its own text, so it deltas like `update` does.
198+
*
199+
* It forced a snapshot per emission until the only consumer that could not merge a
200+
* delta was gone — `apply-file-preview-phase.ts` has accumulated them since #4923.
201+
* Because an append preview is `existingContent + streamed`, a snapshot per chunk
202+
* re-sent the whole file on every streamed token, which is `O(file x tokens)` into
203+
* the stream buffer: one 250 KB file cost gigabytes of Redis.
204+
*/
205+
it('emits deltas for append when the preview extends the previous text', () => {
197206
expect(buildPreviewContentUpdate('hello', 'hello world', 100, 200, 'append')).toEqual({
198-
content: 'hello world',
207+
content: ' world',
208+
contentMode: 'delta',
209+
lastSnapshotAt: 100,
210+
})
211+
})
212+
213+
it('still snapshots an append whose base changed underneath it', () => {
214+
expect(buildPreviewContentUpdate('hello', 'HELLO world', 100, 200, 'append')).toEqual({
215+
content: 'HELLO world',
199216
contentMode: 'snapshot',
200217
lastSnapshotAt: 200,
201218
})
202219
})
203220

221+
it('still checkpoints an append with a full snapshot on the interval', () => {
222+
expect(buildPreviewContentUpdate('hello', 'hello world', 0, 1_000, 'append')).toEqual({
223+
content: 'hello world',
224+
contentMode: 'snapshot',
225+
lastSnapshotAt: 1_000,
226+
})
227+
})
228+
204229
it('emits deltas for update when the preview extends the previous text', () => {
205230
expect(buildPreviewContentUpdate('hello', 'hello world', 100, 200, 'update')).toEqual({
206231
content: ' world',

0 commit comments

Comments
 (0)