From 135aaaa7def88bab238ad3b8a9c54561631b55b0 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Fri, 9 Oct 2026 10:47:29 +0000 Subject: [PATCH] linux: walk for one wrap at a time, and let go of the listings before the scan Wraps walk in turns, so several interleave on the one thread, and each keeps all it has listed until it ends: peak memory, and the time the event loop is held, grew with the wraps in flight. Parallel walks gain nothing there. They now take their walks first come, first served. An attempt waits before it reads anything, so it goes by what it finds afterwards. The place is given up on every way out, and a waiter whose signal is aborted leaves at once. --- src/sandbox/sandbox-manager.ts | 82 +++++++++++- test/sandbox/glob-expand.test.ts | 214 ++++++++++++++++++++++++++++++- 2 files changed, 293 insertions(+), 3 deletions(-) diff --git a/src/sandbox/sandbox-manager.ts b/src/sandbox/sandbox-manager.ts index a35d72856..2f5dd1888 100644 --- a/src/sandbox/sandbox-manager.ts +++ b/src/sandbox/sandbox-manager.ts @@ -1876,6 +1876,47 @@ const RESTARTS_IN_TURNS = 2 /** In place of what a walk finds: the configuration was replaced under it. */ const REPLACED = Symbol('replaced') +/** Whether a wrap is walking, and the ones waiting to, first come first. */ +let walking = false +const waitingToWalk: (() => void)[] = [] + +/** + * Resolves once no other wrap is walking, with what says that this one's walk + * has ended; only the first call of it counts. Walks that take turns on one + * thread end no sooner for it, while each keeps all it has listed until it + * ends and has its turn before the event loop gets one: the memory and the + * thread held would both grow with the wraps in flight. + * Rejects with `signal`'s reason as soon as that is aborted during the wait. + * Nothing else takes a wrap out of the line, reset() included. + */ +async function waitToWalk(signal?: AbortSignal): Promise<() => void> { + if (walking) { + signal?.throwIfAborted() + await new Promise((resolve, reject) => { + const go = (): void => { + signal?.removeEventListener('abort', leave) + resolve() + } + const leave = (): void => { + waitingToWalk.splice(waitingToWalk.indexOf(go), 1) + reject(signal?.reason) + } + waitingToWalk.push(go) + signal?.addEventListener('abort', leave, { once: true }) + }) + } + walking = true + let ended = false + return () => { + if (ended) return + ended = true + // Handed on while `walking` stays set, so that no newcomer gets in first. + const next = waitingToWalk.shift() + if (next) next() + else walking = false + } +} + async function wrapWithSandbox( command: string, binShell?: string, @@ -1901,6 +1942,35 @@ async function wrapWithSandboxAgain( customConfig?: Partial, abortSignal?: AbortSignal, options?: WrapWithSandboxOptions, +): Promise { + // INVARIANT: an attempt waits before it reads anything, so that it goes by + // the configuration, the working directory and the disk it finds afterwards. + const walkEnded = await waitToWalk(abortSignal) + try { + return await wrapWithSandboxOnce( + walkEnded, + restarts, + command, + binShell, + customConfig, + abortSignal, + options, + ) + } finally { + walkEnded() + } +} + +/** One attempt of {@link wrapWithSandboxAgain}, which calls `walkEnded` once + * it walks no more. */ +async function wrapWithSandboxOnce( + walkEnded: () => void, + restarts: number, + command: string, + binShell?: string, + customConfig?: Partial, + abortSignal?: AbortSignal, + options?: WrapWithSandboxOptions, ): Promise { const platform = getPlatform() const commandId = options?.commandId @@ -1912,8 +1982,10 @@ async function wrapWithSandboxAgain( config !== startedWith || configInstalls !== startedAt || workingDirectory() !== startedIn - const startOver = (): Promise => - wrapWithSandboxAgain( + const startOver = (): Promise => { + // The next attempt waits like any other wrap, behind the ones there are. + walkEnded() + return wrapWithSandboxAgain( restarts + 1, command, binShell, @@ -1921,6 +1993,7 @@ async function wrapWithSandboxAgain( abortSignal, options, ) + } /** `steps`, left as soon as the host is seen to have moved on: what they * would go on to find is for a wrap that starts over anyway. */ function* whileCurrent(steps: Steps): Steps { @@ -2051,6 +2124,9 @@ async function wrapWithSandboxAgain( credentialRestrictions.degradeToDenyPaths, ), ) + // Emptied by hand: a runtime may keep what this function has named for as + // long as it is suspended, which is until the scan below has ended. + listings.clear() if (expandedDenyRead === REPLACED) return startOver() readConfig = { denyOnly: expandedDenyRead, @@ -2062,6 +2138,8 @@ async function wrapWithSandboxAgain( ), } } + // Nothing below walks, and what it waits for is not this thread. + walkEnded() // Check if network config is specified - this determines if we need network restrictions // Network restriction is needed when: diff --git a/test/sandbox/glob-expand.test.ts b/test/sandbox/glob-expand.test.ts index 58f2ad1be..b35770164 100644 --- a/test/sandbox/glob-expand.test.ts +++ b/test/sandbox/glob-expand.test.ts @@ -1,4 +1,14 @@ -import { describe, it, expect, beforeAll, afterAll, spyOn } from 'bun:test' +/// +import { + describe, + it, + expect, + beforeAll, + beforeEach, + afterAll, + afterEach, + spyOn, +} from 'bun:test' import * as fc from 'fast-check' // The namespace of the same module production binds (sandbox-utils.ts does // `import * as fs from 'fs'`), so a spy on it is seen by the code under test. @@ -17,6 +27,7 @@ import { tmpdir } from 'node:os' import { basename, join } from 'node:path' import { literalReadings } from '../../src/sandbox/path-entries.js' import type { FilesystemPathEntry } from '../../src/sandbox/sandbox-config.js' +import type { ISandboxManager } from '../../src/sandbox/sandbox-manager.js' import { expandGlobPattern, expandTilde, @@ -2301,6 +2312,207 @@ describe.if(isLinux)('getFsReadConfig with glob patterns on Linux', () => { }) }) +// ============================================================================ +// Tests for wraps asked for while another one walks, on Linux +// ============================================================================ + +describe.if(isLinux)('wrapWithSandbox while another wrap walks', () => { + let SandboxManager: ISandboxManager + const cwd = process.cwd() + let root: string + /** The tree each listing was in: `a`, `b` or `c`, three listings each. */ + let listed: string + /** Runs in each listing, with how many there have been and what it found. */ + let inListing: (nth: number, entries: object) => void + let readdirSpy: { mockClear(): void; mockRestore(): void } + + const configured = (denyRead: string[], allowWrite: string[]) => ({ + network: { allowedDomains: [], deniedDomains: [] }, + filesystem: { + denyRead: denyRead.map(pattern => join(root, pattern)), + allowWrite, + denyWrite: [], + }, + }) + /** A wrap that walks `tree`, whatever is configured. */ + const wrapWalking = (tree: string, signal?: AbortSignal): Promise => + SandboxManager.wrapWithSandbox( + 'true', + undefined, + { filesystem: configured([`${tree}/**/*.pem`], []).filesystem }, + signal, + ) + + beforeEach(async () => { + ;({ SandboxManager } = await import('../../src/sandbox/sandbox-manager.js')) + root = realPath(mkdtempSync(join(tmpdir(), 'glob-walk-line-'))) + for (const tree of ['a', 'b', 'c']) { + mkdirSync(join(root, tree, 'from'), { recursive: true }) + mkdirSync(join(root, tree, 'to')) + } + listed = '' + inListing = () => {} + // Each listing takes longer than a turn. + const readdirSync = fs.readdirSync + readdirSpy = spyOn(fs, 'readdirSync').mockImplementation((( + ...args: Parameters + ) => { + const entries = readdirSync(...args) + if (String(args[0]).startsWith(root)) { + listed += String(args[0])[root.length + 1] + inListing(listed.length, entries) + const until = performance.now() + 15 + while (performance.now() < until); + } + return entries + }) as typeof fs.readdirSync) + process.chdir(join(root, 'c', 'from')) + await SandboxManager.reset() + await SandboxManager.initialize(configured(['b/**/.env'], ['.', root])) + }) + + afterEach(async () => { + readdirSpy.mockRestore() + process.chdir(cwd) + await SandboxManager.reset() + rmSync(root, { recursive: true, force: true }) + }) + + it('walks for one wrap at a time, in the order they were asked for', async () => { + await Promise.all(['a', 'b', 'c'].map(tree => wrapWalking(tree))) + expect(listed).toBe('aaabbbccc') + }) + + // Aborted before it is asked for, and while it waits. + it.each([0, 2])( + 'gives up an aborted wrap before its place comes, and goes on to the next (aborted after %d listings)', + async at => { + const controller = new AbortController() + const reason = new Error('stopped') + inListing = nth => { + if (nth === at) controller.abort(reason) + } + inListing(0, []) + let listedWhenGivenUp = '' + const [, givenUp] = await Promise.all([ + wrapWalking('a'), + wrapWalking('b', controller.signal).catch((e: unknown) => { + listedWhenGivenUp = listed + return e + }), + wrapWalking('c'), + ]) + expect(givenUp).toBe(reason) + expect(listedWhenGivenUp.length).toBeLessThan(3) + expect(listed).toBe('aaaccc') + }, + ) + + it('goes on to the next when the wrap that walks is aborted, and when it throws', async () => { + const controller = new AbortController() + inListing = nth => { + if (nth === 2) controller.abort(new Error('stopped')) + } + const outcomes = await Promise.allSettled([ + wrapWalking('a', controller.signal), + // Neither a path nor a marked one. + SandboxManager.wrapWithSandbox('true', undefined, { + filesystem: { denyRead: [{} as never], allowWrite: [], denyWrite: [] }, + }), + wrapWalking('b'), + ]) + expect(outcomes.map(outcome => outcome.status)).toEqual([ + 'rejected', + 'rejected', + 'fulfilled', + ]) + expect(listed).toBe('aabbb') + }) + + // What the host does in the second listing of `a`, with a wrap by the + // configuration waiting. That one has read nothing yet, so it walks `b` once; + // the wrap that was walking starts over, behind it. + it.each([ + [ + 'the configuration installed', + () => { + writeFileSync(join(root, 'b', 'key.pem'), '') + SandboxManager.updateConfig(configured(['b/**/.env', 'b/**/*.pem'], [])) + }, + 'b/key.pem', + 'c/from', + ], + [ + 'the working directory the host moved to', + () => process.chdir(join(root, 'c', 'to')), + 'c/to', + 'c/from', + ], + ])('goes by %s while it waited', async (_, disturb, holds, lacks) => { + inListing = nth => { + if (nth === 2) disturb() + } + const [, waited] = await Promise.all([ + wrapWalking('a'), + SandboxManager.wrapWithSandbox('true'), + ]) + expect(listed).toBe('aabbbaaa') + expect(waited).toContain(join(root, holds)) + expect(waited).not.toContain(join(root, lacks)) + expect(waited).toBe(await SandboxManager.wrapWithSandbox('true')) + }) + + it('keeps the order through a reset()', async () => { + let reset: Promise | undefined + inListing = nth => { + if (nth === 2) reset = SandboxManager.reset() + } + // Either may find the network bridge gone, as any wrap during a reset(). + await Promise.allSettled([wrapWalking('a'), wrapWalking('b')]) + await reset + expect(listed).toBe('aaabbb') + }) + + it('has walked, and let go of what it listed, before the mandatory denies are looked for', async () => { + // Stands in for ripgrep: says that it runs, and goes on until told. + const scans = join(root, 'scans') + const enough = join(root, 'enough') + const slowScan = join(root, 'slow-scan') + writeFileSync( + slowScan, + `#!/bin/sh\necho ran >> ${scans}\nfor i in $(seq 300); do [ -e ${enough} ] && exit; sleep 0.01; done\n`, + { mode: 0o755 }, + ) + writeFileSync(scans, '') + SandboxManager.updateConfig({ + ...configured([], []), + ripgrep: { command: slowScan }, + }) + const listings: WeakRef[] = [] + inListing = (_, entries) => listings.push(new WeakRef(entries)) + + let wrapped = 0 + const wrapping = Promise.all( + ['a', 'b'].map(tree => wrapWalking(tree).then(() => wrapped++)), + ) + try { + while (fs.readFileSync(scans, 'utf8') !== 'ran\nran\n') await Bun.sleep(5) + // The second has walked while the first was scanning. + expect(wrapped).toBe(0) + readdirSpy.mockClear() // It keeps what each call returned. + Bun.gc(true) + // Not none: a collector that reads the stack can take a stale word for a + // reference. Held by the wraps, it would be all six. + expect(listings.filter(l => l.deref() !== undefined).length).toBeLessThan( + 6, + ) + } finally { + writeFileSync(enough, '') + await wrapping + } + }) +}) + // ============================================================================ // Tests for getLinuxGlobPatternWarnings // ============================================================================