linux: walk for one wrap at a time, and let go of the listings before the scan

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<void>((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<SandboxRuntimeConfig>,
   abortSignal?: AbortSignal,
   options?: WrapWithSandboxOptions,
+): Promise<string> {
+  // 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<SandboxRuntimeConfig>,
+  abortSignal?: AbortSignal,
+  options?: WrapWithSandboxOptions,
 ): Promise<string> {
   const platform = getPlatform()
   const commandId = options?.commandId
@@ -1912,15 +1982,18 @@ async function wrapWithSandboxAgain(
     config !== startedWith ||
     configInstalls !== startedAt ||
     workingDirectory() !== startedIn
-  const startOver = (): Promise<string> =>
-    wrapWithSandboxAgain(
+  const startOver = (): Promise<string> => {
+    // The next attempt waits like any other wrap, behind the ones there are.
+    walkEnded()
+    return wrapWithSandboxAgain(
       restarts + 1,
       command,
       binShell,
       customConfig,
       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<T>(steps: Steps<T>): Steps<T | typeof REPLACED> {
@@ -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:

test/sandbox/glob-expand.test.ts

@@ -1,4 +1,14 @@
-import { describe, it, expect, beforeAll, afterAll, spyOn } from 'bun:test'
+/// <reference lib="es2021.weakref" />
+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.
@@ -20,6 +30,7 @@ import type {
   FilesystemPathEntry,
   SandboxRuntimeConfig,
 } from '../../src/sandbox/sandbox-config.js'
+import type { ISandboxManager } from '../../src/sandbox/sandbox-manager.js'
 import {
   expandGlobPattern,
   expandTilde,
@@ -2304,6 +2315,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<string> =>
+    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<typeof fs.readdirSync>
+    ) => {
+      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<void> | 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<object>[] = []
+    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
 // ============================================================================