Skip to content

Commit 3de3602

Browse files
waleedlatif1claude
andcommitted
fix(redis): address review findings on the byte bounds
- 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>
1 parent 329eab9 commit 3de3602

7 files changed

Lines changed: 219 additions & 50 deletions

File tree

‎apps/realtime/src/handlers/file-doc-store.test.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -470,6 +470,28 @@ describe('FileDocStore', () => {
470470
doc.destroy()
471471
})
472472

473+
it('keeps the byte trigger armed when compaction fails', async () => {
474+
const a = await newStore()
475+
const doc = new Y.Doc()
476+
await a.attachRoom(NAME, doc)
477+
const room = (a as any).rooms.get(NAME)
478+
room.appendedBytes = 9 * 1024 * 1024
479+
room.realEdited = true
480+
481+
const write = (a as any).write
482+
const original = write.xTrim.bind(write)
483+
write.xTrim = async () => {
484+
throw new Error('redis blip')
485+
}
486+
await (a as any).maybeCompact(NAME, true)
487+
write.xTrim = original
488+
489+
// A failed fold must not disarm the trigger — otherwise the stream stays oversized until
490+
// this task happens to append another full threshold's worth of deltas.
491+
expect(room.appendedBytes).toBe(9 * 1024 * 1024)
492+
doc.destroy()
493+
})
494+
473495
it('stamps a compaction snapshot of an agent-ONLY stream as an agent frame (never persisted)', async () => {
474496
const streamKey = `filedoc:stream:${NAME}`
475497
const noop = Buffer.from(Y.encodeStateAsUpdate(new Y.Doc())).toString('base64')

‎apps/realtime/src/handlers/file-doc-store.ts‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -787,12 +787,10 @@ export class FileDocStore {
787787
// appended snapshot id instead would silently drop those un-integrated peer entries.
788788
const upTo = room.lastId
789789
const snapshot = Buffer.from(Y.encodeStateAsUpdate(room.doc)).toString('base64')
790-
// Counts deltas appended SINCE this fold, so it must not carry the snapshot's own size: a
791-
// document whose snapshot already exceeds the ceiling would otherwise re-breach it the instant
792-
// compaction finished and force a full snapshot append on every subsequent keystroke — the
793-
// write amplification this threshold exists to prevent. Reset before the appends so a
794-
// concurrent publish is counted against the new baseline rather than the one being retired.
795-
room.appendedBytes = 0
790+
// Bytes this fold is accountable for. Deducted only once the trim succeeds, so a failed
791+
// compaction leaves the trigger armed instead of silently disarming it — and deducting
792+
// rather than zeroing preserves whatever a concurrent publish added while it ran.
793+
const foldedBytes = room.appendedBytes
796794
// Stamp the snapshot by what it folds: a real edit → SNAPSHOT_FIELD (a fresh catch-up treats it
797795
// as edited content, not a bare seed). An agent-ONLY stream (no real edit yet) → AGENT_FIELD, so a
798796
// peer catching up applies it as REDIS_AGENT_ORIGIN and never marks the doc edited — preserving
@@ -805,6 +803,10 @@ export class FileDocStore {
805803
// MINID keeps entries with id >= upTo: the snapshot, any un-integrated peer entries, and
806804
// `upTo` itself (redundant with the snapshot, harmless); it drops only the folded older deltas.
807805
await this.write.xTrim(streamKey(name), 'MINID', upTo)
806+
// Never the snapshot's own size: a document whose snapshot already exceeds the ceiling
807+
// would re-breach it the instant compaction finished and force a full snapshot append on
808+
// every subsequent keystroke — the write amplification this threshold exists to prevent.
809+
room.appendedBytes = Math.max(0, room.appendedBytes - foldedBytes)
808810
} finally {
809811
await this.releaseLock(key, token)
810812
}

‎apps/sim/lib/copilot/request/session/buffer.test.ts‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
MothershipStreamV1TextChannel,
1010
} from '@/lib/copilot/generated/mothership-stream-v1'
1111
import { createEvent } from '@/lib/copilot/request/session/event'
12+
import { getRedisBudgetLimits } from '@/lib/core/redis/byte-budget.server'
1213

1314
type StoredEnvelope = {
1415
score: number
@@ -136,6 +137,18 @@ import {
136137
scheduleBufferCleanup,
137138
} from '@/lib/copilot/request/session/buffer'
138139

140+
async function makeEnvelope(text: string) {
141+
const cursor = await allocateCursor('stream-1')
142+
return createEvent({
143+
streamId: 'stream-1',
144+
cursor: cursor.cursor,
145+
seq: cursor.seq,
146+
requestId: 'req-1',
147+
type: MothershipStreamV1EventType.text,
148+
payload: { channel: MothershipStreamV1TextChannel.assistant, text },
149+
})
150+
}
151+
139152
describe('mothership-stream-outbox', () => {
140153
beforeEach(() => {
141154
mockRedis = createRedisStub()
@@ -346,4 +359,43 @@ describe('mothership-stream-outbox', () => {
346359
expect(replayed).toHaveLength(1)
347360
expect(replayed[0]?.payload.text).toBe('hello')
348361
})
362+
363+
it('splits an oversized batch instead of refusing it', async () => {
364+
const limits = getRedisBudgetLimits('copilot_stream')
365+
// Individually writable frames that collectively exceed the per-write ceiling. Refusing the
366+
// whole batch would stop replay persistence for the rest of the stream over a batching artefact.
367+
const envelopes = await Promise.all(
368+
Array.from({ length: 3 }, () =>
369+
makeEnvelope('x'.repeat(Math.floor(limits.maxSingleWriteBytes * 0.45)))
370+
)
371+
)
372+
373+
const result = await appendEvents(envelopes, { streamId: 'stream-1' })
374+
375+
expect(result.persisted).toBe(true)
376+
expect(mockRedis.eval).toHaveBeenCalledTimes(2)
377+
})
378+
379+
it('refuses a single frame that can never land, without splitting', async () => {
380+
const limits = getRedisBudgetLimits('copilot_stream')
381+
const oversized = await makeEnvelope('x'.repeat(limits.maxSingleWriteBytes + 10))
382+
const result = await appendEvents([oversized], { streamId: 'stream-1' })
383+
384+
expect(result.persisted).toBe(false)
385+
expect(mockRedis.eval).not.toHaveBeenCalled()
386+
})
387+
388+
it('measures the ceiling in UTF-8 bytes, not UTF-16 units', async () => {
389+
const limits = getRedisBudgetLimits('copilot_stream')
390+
// Each astral char is 2 UTF-16 units but 4 UTF-8 bytes, so `String.length` under-reports by 2x
391+
// and would call this batch writable when Redis will not.
392+
const chars = Math.floor(limits.maxSingleWriteBytes / 3)
393+
const astral = await makeEnvelope('\u{1D306}'.repeat(chars))
394+
expect(JSON.stringify(astral).length).toBeLessThan(limits.maxSingleWriteBytes)
395+
396+
const result = await appendEvents([astral], { streamId: 'stream-1' })
397+
398+
expect(result.persisted).toBe(false)
399+
expect(mockRedis.eval).not.toHaveBeenCalled()
400+
})
349401
})

‎apps/sim/lib/copilot/request/session/buffer.ts‎

Lines changed: 62 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -236,50 +236,77 @@ export async function appendEvents(
236236
}
237237
const budgetKeys = getRedisBudgetKeys(budgetScope)
238238

239-
const zaddArgs: Array<number | string> = []
240-
let batchBytes = 0
241-
for (const envelope of envelopes) {
239+
/*
240+
Redis measures a member in UTF-8 bytes, so the ceiling has to be measured the same
241+
way — `String.length` counts UTF-16 units and under-reports every non-ASCII frame,
242+
which would let a batch past a check the Lua then applies differently.
243+
*/
244+
const members = envelopes.map((envelope) => {
242245
const member = JSON.stringify(envelope)
243-
batchBytes += member.length
244-
zaddArgs.push(envelope.seq, member)
245-
}
246+
return { seq: envelope.seq, member, bytes: Buffer.byteLength(member, 'utf8') }
247+
})
246248

247249
/*
248-
A single batch past the per-write ceiling can never land, and retrying it would
249-
stall every later batch behind it. Refuse it the same way the budget would.
250+
Split on the per-write ceiling rather than refusing the whole batch: a flush carries
251+
whatever accumulated since the last one, so an ordinary run of large frames can exceed
252+
the ceiling collectively while every frame is individually writable. Refusing that
253+
batch would stop replay persistence for the rest of the stream over a batching
254+
artefact. Chunks are written in sequence order, so the stored cursor stays monotonic.
250255
*/
251-
if (batchBytes > limits.maxSingleWriteBytes) {
252-
const refusal: RedisBudgetRefusal = {
253-
resource: 'owner_redis_bytes',
254-
currentBytes: 0,
255-
limitBytes: limits.maxSingleWriteBytes,
256-
attemptedBytes: batchBytes,
256+
const chunks: Array<{ members: typeof members; bytes: number }> = []
257+
for (const entry of members) {
258+
const last = chunks[chunks.length - 1]
259+
if (!last || last.bytes + entry.bytes > limits.maxSingleWriteBytes) {
260+
chunks.push({ members: [entry], bytes: entry.bytes })
261+
} else {
262+
last.members.push(entry)
263+
last.bytes += entry.bytes
257264
}
258-
logRedisBudgetRefusal(refusal, { operation: 'append_event', scope: budgetScope, logger })
259-
return { persisted: false, refusal }
260265
}
261266

262-
const result = await withRedisRetry({ operation: 'append_event', streamId }, async (redis) =>
263-
redis.eval(
264-
APPEND_EVENTS_SCRIPT,
265-
2 + budgetKeys.length,
266-
getEventsKey(streamId),
267-
getSeqKey(streamId),
268-
...budgetKeys,
269-
config.ttlSeconds,
270-
config.eventLimit,
271-
limits.maxOwnerBytes,
272-
limits.maxUserBytes,
273-
limits.ttlSeconds,
274-
String(envelopes[envelopes.length - 1].seq),
275-
...zaddArgs
267+
for (const chunk of chunks) {
268+
/*
269+
A single frame past the ceiling can never land, and retrying it would stall every
270+
later batch behind it. Refuse it the same way the budget would.
271+
*/
272+
if (chunk.bytes > limits.maxSingleWriteBytes) {
273+
const refusal: RedisBudgetRefusal = {
274+
resource: 'owner_redis_bytes',
275+
currentBytes: 0,
276+
limitBytes: limits.maxSingleWriteBytes,
277+
attemptedBytes: chunk.bytes,
278+
}
279+
logRedisBudgetRefusal(refusal, { operation: 'append_event', scope: budgetScope, logger })
280+
return { persisted: false, refusal }
281+
}
282+
283+
const zaddArgs: Array<number | string> = []
284+
for (const entry of chunk.members) {
285+
zaddArgs.push(entry.seq, entry.member)
286+
}
287+
288+
const result = await withRedisRetry({ operation: 'append_event', streamId }, async (redis) =>
289+
redis.eval(
290+
APPEND_EVENTS_SCRIPT,
291+
2 + budgetKeys.length,
292+
getEventsKey(streamId),
293+
getSeqKey(streamId),
294+
...budgetKeys,
295+
config.ttlSeconds,
296+
config.eventLimit,
297+
limits.maxOwnerBytes,
298+
limits.maxUserBytes,
299+
limits.ttlSeconds,
300+
String(chunk.members[chunk.members.length - 1].seq),
301+
...zaddArgs
302+
)
276303
)
277-
)
278304

279-
const refusal = parseRedisBudgetRefusal(result, batchBytes, limits)
280-
if (refusal) {
281-
logRedisBudgetRefusal(refusal, { operation: 'append_event', scope: budgetScope, logger })
282-
return { persisted: false, refusal }
305+
const refusal = parseRedisBudgetRefusal(result, chunk.bytes, limits)
306+
if (refusal) {
307+
logRedisBudgetRefusal(refusal, { operation: 'append_event', scope: budgetScope, logger })
308+
return { persisted: false, refusal }
309+
}
283310
}
284311

285312
return { persisted: true }

‎apps/sim/lib/copilot/request/session/writer.test.ts‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,4 +322,58 @@ describe('StreamWriter', () => {
322322
userId: 'user-7',
323323
})
324324
})
325+
326+
it('does not persist a batch queued while an earlier append was already refusing', async () => {
327+
vi.useFakeTimers()
328+
let releaseFirst: () => void = () => {}
329+
appendEvents
330+
.mockImplementationOnce(
331+
() =>
332+
new Promise((resolve) => {
333+
releaseFirst = () =>
334+
resolve({
335+
persisted: false,
336+
refusal: {
337+
resource: 'owner_redis_bytes',
338+
currentBytes: 1,
339+
limitBytes: 1,
340+
attemptedBytes: 1,
341+
},
342+
})
343+
})
344+
)
345+
.mockResolvedValue({ persisted: true })
346+
347+
const writer = new StreamWriter({
348+
streamId: 'stream-1',
349+
chatId: 'chat-1',
350+
requestId: 'req-1',
351+
})
352+
const controller = {
353+
enqueue: vi.fn(),
354+
close: vi.fn(),
355+
} as unknown as ReadableStreamDefaultController
356+
writer.attach(controller)
357+
358+
await writer.publish({
359+
type: MothershipStreamV1EventType.text,
360+
payload: { channel: MothershipStreamV1TextChannel.assistant, text: 'one' },
361+
})
362+
await vi.advanceTimersByTimeAsync(15)
363+
364+
// Queued while the first append is still in flight, so the enqueue-time check cannot see the
365+
// refusal about to latch. Persisting it would leave replay holding a later event but not the
366+
// refused one — a hole a resuming client cannot detect.
367+
await writer.publish({
368+
type: MothershipStreamV1EventType.text,
369+
payload: { channel: MothershipStreamV1TextChannel.assistant, text: 'two' },
370+
})
371+
await vi.advanceTimersByTimeAsync(15)
372+
373+
releaseFirst()
374+
await writer.close()
375+
376+
expect(writer.persistenceStopped).toBe(true)
377+
expect(appendEvents).toHaveBeenCalledTimes(1)
378+
})
325379
})

‎apps/sim/lib/copilot/request/session/writer.ts‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@ export interface StreamWriterOptions {
2222
keepaliveMs?: number
2323
}
2424

25+
/** Result used when the soft stop is already latched, so no further append is attempted. */
26+
const PERSISTENCE_ALREADY_STOPPED = { persisted: true } as const
27+
2528
export class StreamWriter {
2629
private readonly streamId: string
2730
private readonly chatId: string | undefined
@@ -191,10 +194,18 @@ export class StreamWriter {
191194
this.persistenceTail = this.persistenceTail
192195
.catch(() => undefined)
193196
.then(() =>
194-
appendEvents(batch, {
195-
streamId: this.streamId,
196-
...(this.userId ? { userId: this.userId } : {}),
197-
})
197+
/*
198+
Re-checked here, not only at enqueue: a batch queued while an earlier append was
199+
in flight would otherwise land after that append had already stopped persistence,
200+
leaving a replay that holds later events but not the refused ones — a hole a
201+
resuming client cannot detect.
202+
*/
203+
this._persistenceStopped
204+
? PERSISTENCE_ALREADY_STOPPED
205+
: appendEvents(batch, {
206+
streamId: this.streamId,
207+
...(this.userId ? { userId: this.userId } : {}),
208+
})
198209
)
199210
.then((result) => {
200211
this.lastPersistenceError = null

‎apps/sim/lib/realtime/event-log.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -198,12 +198,13 @@ export async function appendEvent<E extends EventLogEntry>(
198198
stream.events = stream.events.slice(-config.cap)
199199
}
200200
if (config.maxBytes > 0) {
201-
let bytes = stream.events.reduce(
202-
(total, event) => total + JSON.stringify(event).length,
203-
0
204-
)
201+
// UTF-8 bytes, so this path bounds a stream identically to the Lua's `string.len`;
202+
// `String.length` counts UTF-16 units and under-reports every non-ASCII event.
203+
const entryBytes = (event: EventLogEntry) =>
204+
Buffer.byteLength(JSON.stringify(event), 'utf8')
205+
let bytes = stream.events.reduce((total, event) => total + entryBytes(event), 0)
205206
while (bytes > config.maxBytes && stream.events.length > 1) {
206-
bytes -= JSON.stringify(stream.events[0]).length
207+
bytes -= entryBytes(stream.events[0])
207208
stream.events = stream.events.slice(1)
208209
}
209210
}

0 commit comments

Comments
 (0)