Skip to content

Commit 1270bcb

Browse files
committed
revert(files): drop the concurrent-render coalescing
The coalescing was an optional efficiency win — rendering is content-addressed and idempotent, so duplicate concurrent renders produced the same artifact and cost only extra sandbox time on an artifact miss. It bought that at the price of the most intricate code in the change set, and produced three concurrency findings across two review rounds: a shared render inheriting one caller's cancellation, an E2B/isolated-vm asymmetry in how the signal was raced, and orphaned rejections once every caller could race away. Removing it also restores true cancellation on the isolated-vm path: the caller's signal now reaches runSandboxTask again, so an abort cancels the sandbox work rather than only abandoning the wait for it.
1 parent 5d3107f commit 1270bcb

2 files changed

Lines changed: 8 additions & 89 deletions

File tree

apps/sim/lib/copilot/tools/server/files/doc-compile.ts

Lines changed: 8 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -511,45 +511,6 @@ const unrenderableSources = new Map<string, number>()
511511
*/
512512
const UNRENDERABLE_TTL_MS = 5 * 60 * 1000
513513

514-
/**
515-
* Renders in flight, keyed identically to {@link compiledDocCache}. An artifact
516-
* miss is the same for every concurrent reader — a freshly forked workspace whose
517-
* document several viewers open at once, or one request whose blocks read the same
518-
* file — and rendering is the expensive step, so they share one run instead of each
519-
* paying for it. The entry is dropped as soon as the render settles, so a later read
520-
* re-renders normally rather than replaying a stale result.
521-
*/
522-
const inFlightRenders = new Map<string, Promise<{ buffer: Buffer; contentType: string }>>()
523-
524-
/** Rejects when `signal` aborts, so a caller can abandon a shared render without cancelling it. */
525-
function rejectOnAbort(signal: AbortSignal): Promise<never> {
526-
return new Promise((_resolve, reject) => {
527-
if (signal.aborted) {
528-
reject(signal.reason ?? new Error('Aborted'))
529-
return
530-
}
531-
signal.addEventListener('abort', () => reject(signal.reason ?? new Error('Aborted')), {
532-
once: true,
533-
})
534-
})
535-
}
536-
537-
function coalesceRender(
538-
key: string,
539-
run: () => Promise<{ buffer: Buffer; contentType: string }>
540-
): Promise<{ buffer: Buffer; contentType: string }> {
541-
const existing = inFlightRenders.get(key)
542-
if (existing) return existing
543-
const started = run().finally(() => inFlightRenders.delete(key))
544-
// Every caller races this against its own signal, so all of them can walk away
545-
// before it settles. Attach a terminal handler so a later rejection with no
546-
// waiters left is not reported as an unhandled rejection — callers still observe
547-
// it through their own reference.
548-
started.catch(() => {})
549-
inFlightRenders.set(key, started)
550-
return started
551-
}
552-
553514
function markUnrenderable(key: string): void {
554515
if (unrenderableSources.size >= MAX_COMPILED_DOC_CACHE) {
555516
unrenderableSources.delete(unrenderableSources.keys().next().value as string)
@@ -680,11 +641,7 @@ export async function resolveServableDocBytes(args: {
680641
// (content-addressed), so racing a still-running write-time compile is wasteful
681642
// but correct.
682643
try {
683-
// Same shape as the isolated-vm branch below: the shared run carries no
684-
// caller's signal, and each caller races its own so an aborting reader gives
685-
// up promptly without cancelling the render for everyone else.
686-
const shared = coalesceRender(renderKey, () => compileDoc({ source, fileName, workspaceId }))
687-
return await (signal ? Promise.race([shared, rejectOnAbort(signal)]) : shared)
644+
return await compileDoc({ source, fileName, workspaceId })
688645
} catch (error) {
689646
// Only a script error is deterministic — the same bytes will never render, so
690647
// remembering that is safe. Infra failures (sandbox create/timeout, S3, an
@@ -706,22 +663,13 @@ export async function resolveServableDocBytes(args: {
706663
}
707664

708665
try {
709-
// The shared run deliberately carries no caller's signal: it is one piece of
710-
// work several readers are waiting on, so letting whoever happened to start it
711-
// cancel it would reject every other waiter with an AbortError they did not
712-
// ask for. Each caller instead races its own signal, so an aborting reader
713-
// gives up promptly while the render continues for the rest and still lands in
714-
// the cache.
715-
const shared = coalesceRender(renderKey, async () => {
716-
const compiled = await runSandboxTask(
717-
format.taskId,
718-
{ code: source, workspaceId: workspaceId || '' },
719-
{ ownerKey }
720-
)
721-
compiledCacheSet(renderKey, compiled)
722-
return { buffer: compiled, contentType: format.contentType }
723-
})
724-
return await (signal ? Promise.race([shared, rejectOnAbort(signal)]) : shared)
666+
const compiled = await runSandboxTask(
667+
format.taskId,
668+
{ code: source, workspaceId: workspaceId || '' },
669+
{ ownerKey, signal }
670+
)
671+
compiledCacheSet(renderKey, compiled)
672+
return { buffer: compiled, contentType: format.contentType }
725673
} catch (error) {
726674
// Unlike the E2B engine, the isolated-vm task does not distinguish a script
727675
// error from an infra one, so the only signal available here is cancellation —

apps/sim/lib/copilot/tools/server/files/doc-servable.test.ts

Lines changed: 0 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -137,35 +137,6 @@ describe('resolveServableDocBytes', () => {
137137
expect(result.contentType).toBe('application/octet-stream')
138138
})
139139

140-
it('coalesces concurrent reads of the same missing artifact into one render', async () => {
141-
// A freshly forked workspace can have several viewers open the same document at
142-
// once; each would otherwise pay for its own compile of identical bytes.
143-
const source = uniqueSource('from reportlab.pdfgen import canvas')
144-
mockLoadCompiledDoc.mockResolvedValue(null)
145-
setEnvFlags({ isDocSandboxEnabled: true })
146-
mockExecuteInSandbox.mockImplementation(
147-
() =>
148-
new Promise((resolve) =>
149-
setTimeout(
150-
() => resolve({ exportedFileContent: Buffer.from('%PDF-once').toString('base64') }),
151-
5
152-
)
153-
)
154-
)
155-
156-
const args = { rawBuffer: source, fileName: 'report.pdf', workspaceId: WORKSPACE_ID }
157-
const [a, b, c] = await Promise.all([
158-
resolveServableDocBytes(args),
159-
resolveServableDocBytes(args),
160-
resolveServableDocBytes(args),
161-
])
162-
163-
expect(mockExecuteInSandbox).toHaveBeenCalledTimes(1)
164-
expect(a.buffer.toString()).toBe('%PDF-once')
165-
expect(b.buffer.toString()).toBe('%PDF-once')
166-
expect(c.buffer.toString()).toBe('%PDF-once')
167-
})
168-
169140
it('does not re-run the sandbox for bytes that already failed to render', async () => {
170141
const notReallyAPdf = uniqueSource('<html>still not a pdf</html>')
171142
mockLoadCompiledDoc.mockResolvedValue(null)

0 commit comments

Comments
 (0)