Skip to content

Commit 235c87c

Browse files
waleedlatif1claude
andcommitted
refactor(redis): drop the clear-buffer script and the unused error class
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>
1 parent f2f63a1 commit 235c87c

4 files changed

Lines changed: 21 additions & 88 deletions

File tree

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

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -853,7 +853,6 @@ export class FileDocStore {
853853
// appended snapshot id instead would silently drop those un-integrated peer entries.
854854
const upTo = room.lastId
855855
const snapshot = Buffer.from(Y.encodeStateAsUpdate(room.doc)).toString('base64')
856-
// Captured with `upTo` so the two agree: exactly the entries this fold will trim.
857856
// Stamp the snapshot by what it folds: a real edit → SNAPSHOT_FIELD (a fresh catch-up treats it
858857
// as edited content, not a bare seed). An agent-ONLY stream (no real edit yet) → AGENT_FIELD, so a
859858
// peer catching up applies it as REDIS_AGENT_ORIGIN and never marks the doc edited — preserving
@@ -867,15 +866,9 @@ export class FileDocStore {
867866
// MINID keeps entries with id >= upTo: the snapshot, any un-integrated peer entries, and
868867
// `upTo` itself (redundant with the snapshot, harmless); it drops only the folded older deltas.
869868
await this.write.xTrim(streamKey(name), 'MINID', upTo)
870-
// Never the snapshot's own size: a document whose snapshot already exceeds the ceiling
871-
// would re-breach it the instant compaction finished and force a full snapshot append on
872-
// every subsequent keystroke — the write amplification this threshold exists to prevent.
873-
// Drop only what the trim provably removed. An entry published past `upTo` is retained by
874-
// MINID and its bytes are still in Redis, so it stays counted; dropping it would disarm the
875-
// trigger while the stream kept growing. Done after the trim, so a failed fold changes
876-
// nothing and leaves the trigger armed.
877-
// `MINID upTo` is inclusive — it keeps the entry whose id EQUALS `upTo`, so that entry's
878-
// bytes are still in Redis and must stay counted. Keeps exactly what survived the trim.
869+
// Drop exactly what the trim removed, which `MINID upTo` being INCLUSIVE makes `id < upTo`
870+
// — the entry at the boundary survives, and its bytes are still in Redis. Run after the
871+
// trim, so a failed fold leaves the ledger intact and the trigger armed.
879872
for (const id of room.pendingDeltas.keys()) {
880873
if (isAfterStreamId(upTo, id)) room.pendingDeltas.delete(id)
881874
}

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

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -66,29 +66,17 @@ const createRedisStub = () => {
6666
}),
6767
get: vi.fn().mockImplementation((key: string) => Promise.resolve(values.get(key) ?? null)),
6868
/**
69-
* Stands in for both Lua scripts, dispatching on the leading `DEL` that only
70-
* `CLEAR_BUFFER_SCRIPT` has. It reproduces their observable
69+
* Stands in for `APPEND_EVENTS_SCRIPT`. It reproduces the script's observable
7170
* effects — dedupe, zadd, rank-trim, seq — so the read-path tests still exercise
7271
* real data, and exposes `budgetRefusal` so the refusal branch can be driven
7372
* without reimplementing the budget arithmetic here.
7473
*/
7574
budgetRefusal: null as null | [number, string, number],
7675
eval: vi.fn().mockImplementation((...args: unknown[]) => {
77-
const script = String(args[0])
7876
const numKeys = Number(args[1])
7977
const keys = args.slice(2, 2 + numKeys) as string[]
8078
const argv = args.slice(2 + numKeys) as Array<string | number>
8179

82-
// CLEAR_BUFFER_SCRIPT is the only one that opens with a DEL.
83-
if (script.trimStart().startsWith("redis.call('DEL'")) {
84-
for (const key of keys) {
85-
values.delete(key)
86-
sortedSets.delete(key)
87-
counters.delete(key)
88-
}
89-
return Promise.resolve(1)
90-
}
91-
9280
if (api.budgetRefusal) return Promise.resolve(api.budgetRefusal)
9381

9482
const [eventsKey, seqKey] = keys
@@ -419,17 +407,18 @@ describe('mothership-stream-outbox', () => {
419407
// its events stored with its reservation already erased.
420408
await clearBuffer('stream-1')
421409

422-
const evalCall = mockRedis.eval.mock.calls.at(-1)
423-
expect(evalCall?.[1]).toBe(4)
424-
expect(evalCall?.[5]).toBe('execution:redis-budget:copilot_stream:stream-1')
410+
// One variadic DEL: a single atomic command, so no script is needed for the counter to go
411+
// with the data it accounts for.
412+
expect(mockRedis.del).toHaveBeenCalledTimes(1)
413+
expect(mockRedis.del.mock.calls[0]).toContain('execution:redis-budget:copilot_stream:stream-1')
425414
})
426415

427416
it('never touches the shared user counter when clearing a buffer', async () => {
428417
// An owner id is not proof of who wrote the bytes, so crediting the user counter here would
429418
// let anyone who can name a stream decrement a ceiling they never charged.
430419
await clearBuffer('stream-1')
431420

432-
const keys = mockRedis.eval.mock.calls.at(-1)?.slice(2, 6) as string[]
421+
const keys = mockRedis.del.mock.calls[0] as string[]
433422
expect(keys.some((key) => key.includes('redis-budget:user:'))).toBe(false)
434423
})
435424
})

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

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import {
1010
parseRedisBudgetRefusal,
1111
type RedisBudgetRefusal,
1212
renderRedisBudgetLua,
13-
renderRedisBudgetReleaseLua,
1413
} from '@/lib/core/redis/byte-budget.server'
1514
import {
1615
type PersistedStreamEventEnvelope,
@@ -103,32 +102,27 @@ export async function allocateCursor(streamId: string): Promise<{
103102
return { seq, cursor: String(seq) }
104103
}
105104

106-
/** Deletes a stream's buffer and releases its reservation together. KEYS: [events, seq, abort, budget...]. */
107-
const CLEAR_BUFFER_SCRIPT = `
108-
redis.call('DEL', KEYS[1], KEYS[2], KEYS[3])
109-
${renderRedisBudgetReleaseLua(3)}
110-
return 1
111-
`
112-
113105
export async function resetBuffer(streamId: string): Promise<void> {
114106
await clearBuffer(streamId, 'reset_outbox')
115107
}
116108

117109
export async function clearBuffer(streamId: string, operation = 'clear_outbox'): Promise<void> {
118110
/*
119-
Delete and release in ONE script. The counter outlives the data it accounts for unless
120-
it is dropped here — these keys are deleted rather than expired, so a retry reusing the
121-
same streamId would be refused against bytes that no longer exist. Doing it in a second
122-
round trip would be its own hole: a concurrent append landing between the two would keep
123-
its events stored with its reservation already erased.
124-
125-
Only the owner counter, never the shared user counter — see the release fragment.
111+
The owner counter is deleted WITH the data it accounts for. These keys are deleted rather
112+
than expired, so a counter left behind would refuse a retry reusing the same streamId
113+
against bytes that no longer exist; dropping it in a second round trip would be its own
114+
hole, since a concurrent append landing between the two would keep its events stored with
115+
its reservation already erased. One variadic DEL is a single atomic command, so no script
116+
is needed to get that.
117+
118+
The shared user counter is deliberately untouched: an owner id is not proof of who wrote
119+
the bytes, so crediting it here would let anyone able to name a stream decrement a ceiling
120+
they never charged — and a counter driven down grants writes rather than denying them. Its
121+
fixed window settles it instead, over-counting in the safe direction meanwhile.
126122
*/
127123
const [ownerBudgetKey] = getRedisBudgetKeys({ kind: 'copilot_stream', id: streamId })
128124
await withRedisRetry({ operation, streamId }, async (redis) => {
129-
await redis.eval(
130-
CLEAR_BUFFER_SCRIPT,
131-
4,
125+
await redis.del(
132126
getEventsKey(streamId),
133127
getSeqKey(streamId),
134128
getAbortKey(streamId),

apps/sim/lib/core/redis/byte-budget.server.ts

Lines changed: 0 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -115,24 +115,6 @@ export interface RedisBudgetRefusal {
115115
attemptedBytes: number
116116
}
117117

118-
export class RedisBudgetExceededError extends Error {
119-
readonly resource: RedisBudgetRefusal['resource']
120-
readonly currentBytes: number
121-
readonly limitBytes: number
122-
readonly attemptedBytes: number
123-
124-
constructor(refusal: RedisBudgetRefusal) {
125-
super(
126-
`Redis byte budget exceeded (${refusal.resource}): ${refusal.attemptedBytes} bytes would take ${refusal.currentBytes} past ${refusal.limitBytes}`
127-
)
128-
this.name = 'RedisBudgetExceededError'
129-
this.resource = refusal.resource
130-
this.currentBytes = refusal.currentBytes
131-
this.limitBytes = refusal.limitBytes
132-
this.attemptedBytes = refusal.attemptedBytes
133-
}
134-
}
135-
136118
/**
137119
* Lua that reserves or releases `net_bytes` against the caller's budget keys.
138120
*
@@ -201,31 +183,6 @@ end
201183
`
202184
}
203185

204-
/**
205-
* Lua that drops an owner's counter, for data that is deleted rather than left to expire.
206-
*
207-
* Rendered into the caller's own script, the same way {@link renderRedisBudgetLua} is, so
208-
* the release commits together with the delete it accounts for. Releasing in a second
209-
* round trip would let a concurrent write land in between and keep its bytes stored with
210-
* its reservation already erased.
211-
*
212-
* The shared user counter is deliberately NOT credited here. An owner id is not proof of
213-
* who wrote the bytes — anyone who can name an owner could otherwise decrement a counter
214-
* they never charged, which is the one direction that must never be possible, since a
215-
* counter driven down grants writes rather than denying them. The user counter's fixed
216-
* window is what settles it instead: it already tolerates accruing bytes Redis has dropped
217-
* (see {@link REDIS_BUDGET_TTL_SECONDS}), and this is the same over-count, bounded by the
218-
* same window.
219-
*
220-
* Contract: the owner key is the **last** entry of `KEYS`, and `baseKeyCount` is how many
221-
* precede it.
222-
*/
223-
export function renderRedisBudgetReleaseLua(baseKeyCount: number): string {
224-
return `
225-
redis.call('DEL', KEYS[${baseKeyCount + 1}])
226-
`
227-
}
228-
229186
/** Parses the `{0, resource, current}` refusal a guarded script returns. */
230187
export function parseRedisBudgetRefusal(
231188
result: unknown,

0 commit comments

Comments
 (0)