fix(sandbox): keep bridge queue-directory setup on the startup step
The paperclip callback bridge reset the active startup-step store for the whole worker start. That also unparented the awaited queue-directory setup execs, which are startup work and must keep the `bridge.paperclip` step. Move the store reset to the long-lived poll loop inside the worker. The `makeDir` setup now keeps the active step, so its `sandbox.exec` spans stay parented with the correct `criticalPath` flag. The loop still runs with an empty store, so each run-time exec span opens unparented. Add a regression test that reads the active step during setup and during the first poll, and proves the boundary sits at the loop, not the whole worker. Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
parent
dd6742f7d0
commit
223068e2ff
|
|
@ -1799,13 +1799,12 @@ export async function startAdapterExecutionTargetPaperclipBridge(input: {
|
|||
// this flag is enabled. Only intended for active debugging in trusted
|
||||
// environments.
|
||||
const bridgeDebugEnabled = isBridgeDebugEnabled(process.env);
|
||||
// Start the long-lived callback-bridge worker loop outside the measured
|
||||
// bridge step store. The loop reads and writes queue files with run-time
|
||||
// execs for the whole run, not startup work, so a worker `sandbox.exec` span
|
||||
// must not parent to the ended `bridge.paperclip` step or copy its
|
||||
// `criticalPath` flag. `runWithoutActiveStep` empties the store for the loop
|
||||
// that the worker start schedules, so every later poll tick stays unparented.
|
||||
worker = await runWithoutActiveStep(() => startSandboxCallbackBridgeWorker({
|
||||
// `startSandboxCallbackBridgeWorker` keeps its awaited queue-directory
|
||||
// setup on the active `bridge.paperclip` step, and resets the store only
|
||||
// for its long-lived poll loop (see `runWithoutActiveStep` inside that
|
||||
// function). So the startup `mkdir` execs stay parented and every later
|
||||
// loop `sandbox.exec` span stays unparented with no stale `criticalPath`.
|
||||
worker = await startSandboxCallbackBridgeWorker({
|
||||
client,
|
||||
queueDir,
|
||||
maxBodyBytes,
|
||||
|
|
@ -1842,7 +1841,7 @@ export async function startAdapterExecutionTargetPaperclipBridge(input: {
|
|||
body: await readBridgeForwardResponseBody(response, maxBodyBytes),
|
||||
};
|
||||
},
|
||||
}));
|
||||
});
|
||||
server = await startSandboxCallbackBridgeServer({
|
||||
runner,
|
||||
remoteCwd: target.remoteCwd,
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@ import path from "node:path";
|
|||
import { promisify } from "node:util";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
import { getActiveStepContext, measureStartupStep } from "./acpx-engine/startup-timing.js";
|
||||
import { prepareCommandManagedRuntime } from "./command-managed-runtime.js";
|
||||
import {
|
||||
authorizeSandboxCallbackBridgeRequestWithRoutes,
|
||||
|
|
@ -471,6 +472,73 @@ describe("sandbox callback bridge", () => {
|
|||
}
|
||||
});
|
||||
|
||||
it("keeps the queue-directory setup on the startup step but resets the poll loop store", async () => {
|
||||
// The worker starts inside the measured `bridge.paperclip` step. Its awaited
|
||||
// queue-directory setup is startup work, so a `makeDir` `sandbox.exec` span
|
||||
// must keep the active step and its `criticalPath` flag. The long-lived poll
|
||||
// loop runs run-time execs for the whole run, so a loop `sandbox.exec` span
|
||||
// must open unparented with no stale flag. This test reads the active step in
|
||||
// both places and proves the boundary sits at the loop, not the whole worker.
|
||||
let setupStep: ReturnType<typeof getActiveStepContext> | "unset" = "unset";
|
||||
let loopStep: ReturnType<typeof getActiveStepContext> | "unset" = "unset";
|
||||
let resolveFirstPoll: () => void = () => {};
|
||||
const firstPoll = new Promise<void>((resolve) => {
|
||||
resolveFirstPoll = resolve;
|
||||
});
|
||||
|
||||
const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-bridge-step-store-"));
|
||||
cleanupDirs.push(rootDir);
|
||||
const queueDir = path.posix.join(rootDir, "queue");
|
||||
|
||||
const worker = await measureStartupStep(
|
||||
{},
|
||||
() => 0,
|
||||
"bridge.paperclip",
|
||||
() =>
|
||||
startSandboxCallbackBridgeWorker({
|
||||
client: {
|
||||
makeDir: async () => {
|
||||
setupStep = getActiveStepContext();
|
||||
},
|
||||
makeDirs: async () => {
|
||||
setupStep = getActiveStepContext();
|
||||
},
|
||||
listJsonFiles: async () => {
|
||||
loopStep = getActiveStepContext();
|
||||
resolveFirstPoll();
|
||||
return [];
|
||||
},
|
||||
readTextFile: async () => {
|
||||
throw new Error("unexpected readTextFile");
|
||||
},
|
||||
writeTextFile: async () => {
|
||||
throw new Error("unexpected writeTextFile");
|
||||
},
|
||||
rename: async () => {
|
||||
throw new Error("unexpected rename");
|
||||
},
|
||||
remove: async () => {},
|
||||
},
|
||||
queueDir,
|
||||
authorizeRequest: async () => null,
|
||||
handleRequest: async () => ({ status: 200, body: "ok" }),
|
||||
}),
|
||||
{ criticalPath: false },
|
||||
);
|
||||
|
||||
await firstPoll;
|
||||
await worker.stop();
|
||||
|
||||
// The setup ran on the active step, so its exec span parents to the step.
|
||||
expect(setupStep).not.toBe("unset");
|
||||
expect(setupStep).not.toBeNull();
|
||||
expect((setupStep as { criticalPath?: boolean }).criticalPath).toBe(false);
|
||||
|
||||
// The loop ran outside that store, so its exec span opens unparented with no
|
||||
// stale `criticalPath` flag.
|
||||
expect(loopStep).toBeNull();
|
||||
});
|
||||
|
||||
it("serializes remote response writes so stop does not recreate a late orphaned response", async () => {
|
||||
const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-bridge-response-lock-"));
|
||||
cleanupDirs.push(rootDir);
|
||||
|
|
|
|||
|
|
@ -3,6 +3,7 @@ import { promises as fs } from "node:fs";
|
|||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
|
||||
import { runWithoutActiveStep } from "./acpx-engine/startup-timing.js";
|
||||
import type { CommandManagedRuntimeRunner } from "./command-managed-runtime.js";
|
||||
import { preferredShellForSandbox, shellCommandArgs } from "./sandbox-shell.js";
|
||||
import type { RunProcessResult } from "./server-utils.js";
|
||||
|
|
@ -745,7 +746,13 @@ export async function startSandboxCallbackBridgeWorker(input: {
|
|||
}
|
||||
};
|
||||
|
||||
const loop = (async () => {
|
||||
// Start the long-lived poll loop outside the measured startup-step store.
|
||||
// The `makeDir` calls above are startup work and must keep the active
|
||||
// `bridge.paperclip` step. The loop runs run-time execs for the whole run,
|
||||
// so each loop `sandbox.exec` span must not parent to the ended step or copy
|
||||
// its `criticalPath` flag. `runWithoutActiveStep` empties the store for the
|
||||
// loop only; Node keeps the empty store on every later poll continuation.
|
||||
const loop = runWithoutActiveStep(() => (async () => {
|
||||
try {
|
||||
while (true) {
|
||||
const fileNames = await input.client.listJsonFiles(directories.requestsDir);
|
||||
|
|
@ -785,7 +792,7 @@ export async function startSandboxCallbackBridgeWorker(input: {
|
|||
settleResolve();
|
||||
}
|
||||
}
|
||||
})();
|
||||
})());
|
||||
|
||||
void loop;
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue