Skip to content

Commit c6a7acf

Browse files
icecrasher321claude
andcommitted
fix(memory): close app-service leak paths behind the per-task memory ramp
Prod app tasks climb from ~3.0 GB to 9+ GB average (20.6 GB worst task) with uptime and reset only on deploy. The growth lives in the main Next.js process — the isolated-vm worker is a separate, bounded child and its disposal already runs in finally on every path. Four fixes at the sites that can actually accumulate there: - copilot stream teardown (lib/copilot/request/lifecycle/start.ts): the activeStreams registration, the 250ms Redis abort poller, and the SSE keepalive were acquired outside the try whose finally releases them, so a throw before the lifecycle started (e.g. resetBuffer on a Redis blip) or a throw inside the ordered teardown orphaned two immortal intervals and the registration. Add an idempotent backstop in the orchestration's outer finally; the ordered teardown sets a flag so the normal path pays nothing. - large-value cache (lib/execution/payloads/cache.ts): expiry was enforced only inside later cache calls, so on a quieting instance the last entries — parsed object graphs worth a multiple of their JSON-byte accounting — sat indefinitely instead of for the 15-minute TTL. Add a self-retiring, unref'd sweep interval, and export occupancy stats. - memory telemetry (lib/monitoring/memory-telemetry.ts): add the large-value cache occupancy and detached-context count to the periodic snapshot so the JSON-bytes-vs-heap amplification and context retention are readable from the same log line as heapUsedMB. - BYOK rotation cursors (lib/api-key/byok.ts): the tenant-keyed cursor Map had no delete, TTL, or ceiling. Bound it with an LRU; evicting an idle pool's cursor just restarts its rotation at index 0. - collab-doc converter (lib/collab-doc/converter.ts): the DOM guard read the bundled module's `window` binding while the install wrote `globalThis.window` — the same bundler mismatch documented for TipTap in next.config.ts — so a runtime where the two disagree re-allocated a multi-MB jsdom window on every conversion. Guard and install now go through globalThis, and the jsdom window is a module singleton either way. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 7b761ba commit c6a7acf

7 files changed

Lines changed: 186 additions & 13 deletions

File tree

apps/sim/lib/api-key/byok.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { db } from '@sim/db'
22
import { organizationBYOKKeys, workspace, workspaceBYOKKeys } from '@sim/db/schema'
33
import { createLogger } from '@sim/logger'
44
import { and, asc, eq, notExists } from 'drizzle-orm'
5+
import { LRUCache } from 'lru-cache'
56
import { isOrganizationBYOKEntitledCached } from '@/lib/api-key/byok-entitlement'
67
import { getRotatingApiKey } from '@/lib/core/config/api-keys'
78
import { env } from '@/lib/core/config/env'
@@ -27,7 +28,13 @@ export interface BYOKKeyResult {
2728

2829
export type BYOKKeyScopeName = 'workspace' | 'organization'
2930

30-
const rotationCounters = new Map<string, number>()
31+
/**
32+
* Bounded so tenant-keyed cursors cannot accumulate for the life of the
33+
* process (one entry per workspace/organization × provider that ever rotated).
34+
* Evicting an idle pool's cursor just restarts its rotation at index 0, which
35+
* the per-instance, approximate-rotation contract already tolerates.
36+
*/
37+
const rotationCounters = new LRUCache<string, number>({ max: 10_000 })
3138

3239
interface EncryptedBYOKKey {
3340
id: string

apps/sim/lib/collab-doc/converter.ts

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,8 @@ function markdownSchema(): Schema {
4545
return cachedSchema
4646
}
4747

48+
let cachedJsdomWindow: import('jsdom').DOMWindow | null = null
49+
4850
/**
4951
* Ensure a DOM exists for the TipTap editor the markdown engine constructs. In a `jsdom`/browser
5052
* environment `window` + `document` already exist and this is a no-op; in a plain Node server it
@@ -56,20 +58,28 @@ function markdownSchema(): Schema {
5658
* `document`-only guard (plus a sticky flag) skipped this setup — leaving TipTap to throw "there is no
5759
* window object available". Re-checking the globals every call means a partial stub can never wedge it.
5860
* When `window` is missing we install a coherent jsdom window+document pair, overwriting any such stub.
61+
*
62+
* Both the guard and the install go through `globalThis` explicitly, and the jsdom window itself is a
63+
* module-level singleton. The server bundler can give a bundled module a `window` binding that does
64+
* NOT read `globalThis` (the documented reason TipTap/Yjs sit in `serverExternalPackages` — see
65+
* `next.config.ts`); a bare-`window` guard paired with a `globalThis.window` install can therefore
66+
* disagree forever, re-entering the install on every call. Reading and writing the same object makes
67+
* the guard self-consistent, and the singleton caps this module at ONE jsdom window (megabytes each)
68+
* per process even if some runtime still defeats the guard.
5969
*/
6070
function ensureDomForTipTap(): void {
61-
if (typeof window !== 'undefined' && typeof document !== 'undefined') return
62-
// Lazy require so the client bundle never pulls jsdom in. Bind to `jsdomWindow`, NOT `window` — a
63-
// local `const window` would shadow the global and put the `typeof window` guard above in its
64-
// temporal dead zone ("Cannot access 'window' before initialization").
65-
const { JSDOM } = require('jsdom') as typeof import('jsdom')
66-
const { window: jsdomWindow } = new JSDOM('<!doctype html><html><body></body></html>')
71+
if (typeof globalThis.window !== 'undefined' && typeof globalThis.document !== 'undefined') return
72+
if (!cachedJsdomWindow) {
73+
// Lazy require so the client bundle never pulls jsdom in.
74+
const { JSDOM } = require('jsdom') as typeof import('jsdom')
75+
cachedJsdomWindow = new JSDOM('<!doctype html><html><body></body></html>').window
76+
}
6777
// double-cast-allowed: assigning the jsdom shims onto the global needs an
6878
// index-signature view of `globalThis`, whose declared type has none.
6979
const g = globalThis as unknown as Record<string, unknown>
70-
g.window = jsdomWindow
71-
g.document = jsdomWindow.document
72-
g.navigator ??= jsdomWindow.navigator
80+
g.window = cachedJsdomWindow
81+
g.document = cachedJsdomWindow.document
82+
g.navigator ??= cachedJsdomWindow.navigator
7383
}
7484

7585
/** Convert a file's markdown to a fresh collaborative {@link Y.Doc} (cold-start seed). */

apps/sim/lib/copilot/request/lifecycle/start.test.ts

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@ const {
2525
appendEvent,
2626
cleanupAbortMarker,
2727
hasAbortMarker,
28+
registerActiveStream,
2829
releasePendingChatStream,
30+
unregisterActiveStream,
2931
fetchGo,
3032
} = vi.hoisted(() => ({
3133
runCopilotLifecycle: vi.fn(),
@@ -39,7 +41,9 @@ const {
3941
appendEvent: vi.fn(),
4042
cleanupAbortMarker: vi.fn(),
4143
hasAbortMarker: vi.fn(),
44+
registerActiveStream: vi.fn(),
4245
releasePendingChatStream: vi.fn(),
46+
unregisterActiveStream: vi.fn(),
4347
fetchGo: vi.fn(),
4448
}))
4549

@@ -77,8 +81,8 @@ vi.mock('@/lib/copilot/request/session', () => ({
7781
cleanupAbortMarker,
7882
hasAbortMarker,
7983
releasePendingChatStream,
80-
registerActiveStream: vi.fn(),
81-
unregisterActiveStream: vi.fn(),
84+
registerActiveStream,
85+
unregisterActiveStream,
8286
startAbortPoller: vi.fn().mockReturnValue(setInterval(() => {}, 999999)),
8387
isExplicitStopReason: vi.fn().mockReturnValue(false),
8488
SSE_RESPONSE_HEADERS: {},
@@ -325,6 +329,32 @@ describe('createSSEStream terminal error handling', () => {
325329
expect(lifecycleTraceparent).toMatch(/^00-[0-9a-f]{32}-[0-9a-f]{16}-0[0-9a-f]$/)
326330
})
327331

332+
it('releases the stream registration and pollers when the session reset fails before the lifecycle starts', async () => {
333+
resetBuffer.mockRejectedValue(new Error('redis down'))
334+
335+
const stream = createSSEStream({
336+
requestPayload: { message: 'hello' },
337+
userId: 'user-1',
338+
streamId: 'stream-leak',
339+
executionId: 'exec-leak',
340+
runId: 'run-leak',
341+
chatId: 'chat-leak',
342+
currentChat: null,
343+
isNewChat: false,
344+
message: 'hello',
345+
titleModel: 'gpt-5.4',
346+
requestId: 'req-leak',
347+
orchestrateOptions: {},
348+
})
349+
350+
await expect(drainStream(stream)).rejects.toThrow('redis down')
351+
352+
expect(runCopilotLifecycle).not.toHaveBeenCalled()
353+
expect(registerActiveStream).toHaveBeenCalledWith('stream-leak', expect.any(AbortController))
354+
expect(unregisterActiveStream).toHaveBeenCalledWith('stream-leak')
355+
expect(releasePendingChatStream).toHaveBeenCalledWith('chat-leak', 'stream-leak')
356+
})
357+
328358
it('does not scan manually authored title input against unrelated active secrets', async () => {
329359
runCopilotLifecycle.mockResolvedValue({
330360
success: true,

apps/sim/lib/copilot/request/lifecycle/start.ts

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,17 @@ export function createSSEStream(params: StreamingOrchestrationParams): ReadableS
122122

123123
const publisher = new StreamWriter({ streamId, chatId, requestId })
124124

125+
// Declared at function scope (same rationale as `cancelReason` below) so the
126+
// leak backstop in the orchestration's outer finally can always reach them:
127+
// the stream registration above, the abort poller, and the keepalive are
128+
// process-held resources, and a throw that bypasses the inner finally's
129+
// ordered teardown (e.g. `resetBuffer` failing on a Redis blip before the
130+
// lifecycle starts) previously orphaned them — the poller and keepalive
131+
// intervals then ran, and the activeStreams entry sat, for the life of the
132+
// process.
133+
let abortPoller: ReturnType<typeof startAbortPoller> | undefined
134+
let processResourcesReleased = false
135+
125136
// Classify cancel: signal.reason (explicit-stop set) wins, then
126137
// clientDisconnected, else Unknown (latent contract bug — log it).
127138
const recordCancelled = (errorMessage?: string): CopilotRequestCancelReasonValue => {
@@ -220,7 +231,7 @@ export function createSSEStream(params: StreamingOrchestrationParams): ReadableS
220231
})
221232
}
222233

223-
const abortPoller = startAbortPoller(streamId, abortController, {
234+
abortPoller = startAbortPoller(streamId, abortController, {
224235
requestId,
225236
chatId,
226237
})
@@ -347,6 +358,7 @@ export function createSSEStream(params: StreamingOrchestrationParams): ReadableS
347358
if (chatId) {
348359
await releasePendingChatStream(chatId, streamId)
349360
}
361+
processResourcesReleased = true
350362
await scheduleBufferCleanup(streamId)
351363
await scheduleFilePreviewSessionCleanup(streamId)
352364
await cleanupAbortMarker(streamId)
@@ -371,6 +383,24 @@ export function createSSEStream(params: StreamingOrchestrationParams): ReadableS
371383
rootError = error
372384
throw error
373385
} finally {
386+
// Leak backstop for throws that bypassed the inner finally's
387+
// ordered teardown (a session reset failing before the lifecycle
388+
// started, or the teardown itself throwing before its release
389+
// lines). Every step is idempotent — clearInterval and
390+
// stopKeepalive no-op when already stopped, unregister is a keyed
391+
// delete, and the chat-stream release is ownership-guarded against
392+
// a successor stream — and none of them throw, so the otel finish
393+
// below always still runs. On the normal path the flag set by the
394+
// ordered teardown skips this entirely.
395+
if (!processResourcesReleased) {
396+
processResourcesReleased = true
397+
clearInterval(abortPoller)
398+
publisher.stopKeepalive()
399+
unregisterActiveStream(streamId)
400+
if (chatId) {
401+
await releasePendingChatStream(chatId, streamId)
402+
}
403+
}
374404
// `finish` is idempotent, so it's safe whether the POST
375405
// handler started the root (and may also call finish on an
376406
// error path before the stream ran) or we did. The cancel
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
5+
import {
6+
cacheLargeValue,
7+
clearLargeValueCacheForTests,
8+
getLargeValueCacheStats,
9+
} from '@/lib/execution/payloads/cache'
10+
11+
describe('large value cache sweep', () => {
12+
beforeEach(() => {
13+
vi.useFakeTimers()
14+
clearLargeValueCacheForTests()
15+
})
16+
17+
afterEach(() => {
18+
clearLargeValueCacheForTests()
19+
vi.useRealTimers()
20+
})
21+
22+
it('drains expired entries without further cache traffic', () => {
23+
expect(
24+
cacheLargeValue('lv_sweep', { data: 'x'.repeat(64) }, 64, { executionId: 'exec-1' })
25+
).toBe(true)
26+
expect(getLargeValueCacheStats()).toEqual({ entries: 1, trackedBytes: 64 })
27+
28+
vi.advanceTimersByTime(16 * 60 * 1000)
29+
30+
expect(getLargeValueCacheStats()).toEqual({ entries: 0, trackedBytes: 0 })
31+
})
32+
33+
it('retires the sweep timer once the cache drains and re-arms on the next insert', () => {
34+
cacheLargeValue('lv_a', { data: 1 }, 8, { executionId: 'exec-1' })
35+
expect(vi.getTimerCount()).toBe(1)
36+
37+
vi.advanceTimersByTime(16 * 60 * 1000)
38+
expect(vi.getTimerCount()).toBe(0)
39+
40+
cacheLargeValue('lv_b', { data: 2 }, 8, { executionId: 'exec-1' })
41+
expect(vi.getTimerCount()).toBe(1)
42+
})
43+
44+
it('keeps unexpired entries readable across sweep ticks', () => {
45+
cacheLargeValue('lv_live', { data: 'live' }, 16, { executionId: 'exec-1' })
46+
47+
vi.advanceTimersByTime(5 * 60 * 1000)
48+
49+
expect(getLargeValueCacheStats()).toEqual({ entries: 1, trackedBytes: 16 })
50+
})
51+
})

apps/sim/lib/execution/payloads/cache.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66

77
const FALLBACK_TTL_MS = 15 * 60 * 1000
88
const MAX_IN_MEMORY_BYTES = 256 * 1024 * 1024
9+
const SWEEP_INTERVAL_MS = 60 * 1000
910

1011
interface LargeValueCacheScope {
1112
workspaceId?: string
@@ -28,9 +29,26 @@ const inMemoryValues = new Map<
2829
>()
2930
let inMemoryBytes = 0
3031

32+
let sweepTimer: ReturnType<typeof setInterval> | null = null
33+
3134
export function clearLargeValueCacheForTests(): void {
3235
inMemoryValues.clear()
3336
inMemoryBytes = 0
37+
if (sweepTimer) {
38+
clearInterval(sweepTimer)
39+
sweepTimer = null
40+
}
41+
}
42+
43+
/**
44+
* Point-in-time occupancy of the in-memory large-value cache, for the periodic
45+
* memory-telemetry snapshot. `trackedBytes` is the JSON-serialized accounting
46+
* the admission/eviction budget uses — the retained heap of the parsed values
47+
* is a multiple of it, which is exactly what comparing this line against
48+
* `heapUsedMB` in the same snapshot is meant to expose.
49+
*/
50+
export function getLargeValueCacheStats(): { entries: number; trackedBytes: number } {
51+
return { entries: inMemoryValues.size, trackedBytes: inMemoryBytes }
3452
}
3553

3654
function cleanupExpiredValues(now = Date.now()): void {
@@ -42,6 +60,27 @@ function cleanupExpiredValues(now = Date.now()): void {
4260
}
4361
}
4462

63+
/**
64+
* Keeps the fallback TTL honest on quiet instances. Expiry was previously
65+
* enforced only inside `cacheLargeValue`/`materializeLargeValueRefSync`, so on
66+
* an instance that stopped storing large values the last entries — up to the
67+
* full budget, held as parsed object graphs — sat in memory indefinitely
68+
* instead of for `FALLBACK_TTL_MS`. The timer is unref'd so it never holds the
69+
* process open, and retires itself once the cache drains (the next insert
70+
* restarts it), so an idle process carries no interval at all.
71+
*/
72+
function ensureSweepTimer(): void {
73+
if (sweepTimer) return
74+
sweepTimer = setInterval(() => {
75+
cleanupExpiredValues()
76+
if (inMemoryValues.size === 0 && sweepTimer) {
77+
clearInterval(sweepTimer)
78+
sweepTimer = null
79+
}
80+
}, SWEEP_INTERVAL_MS)
81+
sweepTimer.unref()
82+
}
83+
4584
export function cacheLargeValue(
4685
id: string,
4786
value: unknown,
@@ -87,6 +126,7 @@ export function cacheLargeValue(
87126
expiresAt: Date.now() + FALLBACK_TTL_MS,
88127
})
89128
inMemoryBytes += size
129+
ensureSweepTimer()
90130
return true
91131
}
92132

apps/sim/lib/monitoring/memory-telemetry.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
import v8 from 'node:v8'
77
import { createLogger } from '@sim/logger'
8+
import { getLargeValueCacheStats } from '@/lib/execution/payloads/cache'
89

910
const logger = createLogger('MemoryTelemetry', { logLevel: 'INFO' })
1011

@@ -19,6 +20,7 @@ export function startMemoryTelemetry(intervalMs = 60_000) {
1920
const timer = setInterval(() => {
2021
const mem = process.memoryUsage()
2122
const heap = v8.getHeapStatistics()
23+
const largeValueCache = getLargeValueCacheStats()
2224

2325
logger.info('Memory snapshot', {
2426
heapUsedMB: Math.round(mem.heapUsed / MB),
@@ -28,10 +30,13 @@ export function startMemoryTelemetry(intervalMs = 60_000) {
2830
arrayBuffersMB: Math.round(mem.arrayBuffers / MB),
2931
heapSizeLimitMB: Math.round(heap.heap_size_limit / MB),
3032
nativeContexts: heap.number_of_native_contexts,
33+
detachedContexts: heap.number_of_detached_contexts,
3134
activeResources:
3235
typeof process.getActiveResourcesInfo === 'function'
3336
? process.getActiveResourcesInfo().length
3437
: -1,
38+
largeValueCacheEntries: largeValueCache.entries,
39+
largeValueCacheTrackedMB: Math.round(largeValueCache.trackedBytes / MB),
3540
uptimeMin: Math.round(process.uptime() / 60),
3641
})
3742
}, intervalMs)

0 commit comments

Comments
 (0)