Skip to content

Commit 67d2fb7

Browse files
committed
fix(runtime): stop WASM pipe reads from stalling for maxBlockingReadMs (#1959)
A WASM pipeline stage read from a pipe via a synchronous, blocking process.fd_read on the sidecar reactor thread. While parked in that kernel read, the reactor could not run the upstream stage to completion, so the writer never closed its pipe end; the reader only unblocked when the maxBlockingReadMs watchdog fired — once per stage. A low cap turned that into a guest-visible `Would block`/EAGAIN with a nonzero exit; a high cap into a multi-second-per-stage stall (e.g. `env | sort | sed` ~20-30s). Make WASM process.fd_read non-blocking (Some(Duration::ZERO)), mirroring the sibling process.fd_write path: the runner poll+retries a logically-blocking fd on EAGAIN, the reactor stays free to run the writer, and the reader observes EOF promptly. Pipeline latency no longer depends on maxBlockingReadMs. Add a regression test asserting a low-cap pipeline succeeds without watchdog EAGAIN (with exact-output checks) and that three-stage pipeline latency is independent of the cap.
1 parent 65dc5e6 commit 67d2fb7

2 files changed

Lines changed: 116 additions & 5 deletions

File tree

crates/native-sidecar/src/execution/javascript/rpc.rs

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1181,17 +1181,31 @@ where
11811181
.map_err(|_| SidecarError::InvalidState("fd_read length is too large".into()))?;
11821182
let timeout_ms =
11831183
javascript_sync_rpc_arg_u64_optional(&request.args, 2, "fd_read timeout ms")?;
1184-
match timeout_ms {
1185-
Some(timeout_ms) => kernel
1184+
// Read non-blocking for WASM so the reactor is never parked in a
1185+
// pipe read; the runner poll+retries on EAGAIN (#1959).
1186+
if process.runtime == GuestRuntimeKind::WebAssembly {
1187+
kernel
11861188
.fd_read_with_timeout_result(
11871189
EXECUTION_DRIVER_NAME,
11881190
process.kernel_pid,
11891191
fd,
11901192
length,
1191-
Some(Duration::from_millis(timeout_ms)),
1193+
Some(Duration::ZERO),
11921194
)
1193-
.map(Option::unwrap_or_default),
1194-
None => kernel.fd_read(EXECUTION_DRIVER_NAME, process.kernel_pid, fd, length),
1195+
.map(Option::unwrap_or_default)
1196+
} else {
1197+
match timeout_ms {
1198+
Some(timeout_ms) => kernel
1199+
.fd_read_with_timeout_result(
1200+
EXECUTION_DRIVER_NAME,
1201+
process.kernel_pid,
1202+
fd,
1203+
length,
1204+
Some(Duration::from_millis(timeout_ms)),
1205+
)
1206+
.map(Option::unwrap_or_default),
1207+
None => kernel.fd_read(EXECUTION_DRIVER_NAME, process.kernel_pid, fd, length),
1208+
}
11951209
}
11961210
.map(|bytes| javascript_sync_rpc_bytes_value(&bytes))
11971211
.map_err(kernel_error)
Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
// Regression for https://github.com/rivet-dev/agentos/issues/1959
2+
//
3+
// A WASM pipeline stage reading from a pipe used to block the sidecar reactor
4+
// thread inside a synchronous kernel read (the `process.fd_read` sync RPC).
5+
// While parked, the reactor could not run the upstream stage to completion, so
6+
// the writer never closed its pipe end and the reader only unblocked when the
7+
// `maxBlockingReadMs` watchdog fired — once per pipeline stage. A low cap
8+
// turned that stall into a guest-visible `Would block` / EAGAIN and a nonzero
9+
// exit; a high cap turned it into a multi-second-per-stage stall.
10+
//
11+
// The fix makes WASM `process.fd_read` non-blocking (mirroring the sibling
12+
// `process.fd_write` path): the reactor stays free to run the writer, the
13+
// reader observes EOF promptly, and the pipeline completes with correct output
14+
// whose latency no longer depends on `maxBlockingReadMs`.
15+
import { describe, expect, test } from "vitest";
16+
import { AgentOs } from "../src/index.js";
17+
18+
describe("issue-1959: pipe reader no longer stalls for maxBlockingReadMs", () => {
19+
// A low cap is the sharpest probe: before the fix this stage read raced the
20+
// watchdog and surfaced `sed: Would block` (rc=1). After the fix the cap is
21+
// irrelevant — the read waits for the writer and observes EOF cleanly.
22+
test("a fast pipeline succeeds under a low maxBlockingReadMs (no watchdog EAGAIN)", async () => {
23+
const vm = await AgentOs.create({
24+
limits: { resources: { maxBlockingReadMs: 500 } },
25+
});
26+
try {
27+
const envSortSed = await vm.exec("env | sort | sed -n '1,2p'", {
28+
cwd: "/workspace",
29+
timeoutMs: 60_000,
30+
});
31+
expect(
32+
envSortSed.exitCode,
33+
`env|sort|sed stderr=${envSortSed.stderr}`,
34+
).toBe(0);
35+
expect(envSortSed.stderr.toLowerCase()).not.toMatch(
36+
/would block|temporarily unavailable/,
37+
);
38+
expect(envSortSed.stdout.length).toBeGreaterThan(0);
39+
40+
// Exact outputs prove no bytes are dropped across the pipe.
41+
const seq = await vm.exec("seq 1 5 | paste -sd,", {
42+
cwd: "/workspace",
43+
timeoutMs: 60_000,
44+
});
45+
expect(seq.exitCode, `seq stderr=${seq.stderr}`).toBe(0);
46+
expect(seq.stdout).toBe("1,2,3,4,5\n");
47+
48+
const large = await vm.exec("seq 1 5000 | tail -n 1", {
49+
cwd: "/workspace",
50+
timeoutMs: 60_000,
51+
});
52+
expect(large.exitCode, `large stderr=${large.stderr}`).toBe(0);
53+
expect(large.stdout).toBe("5000\n");
54+
} finally {
55+
await vm.dispose();
56+
}
57+
}, 120_000);
58+
59+
// The defining property of the fix: pipeline latency does not scale with
60+
// maxBlockingReadMs. Before the fix, raising the cap made a three-stage
61+
// pipeline pay the cap once per stage (~3x). After it, the two runs are
62+
// close regardless of the cap.
63+
test("three-stage pipeline latency is independent of maxBlockingReadMs", async () => {
64+
const timePipeline = async (maxBlockingReadMs: number) => {
65+
const vm = await AgentOs.create({
66+
limits: { resources: { maxBlockingReadMs } },
67+
});
68+
try {
69+
const startedAt = Date.now();
70+
const r = await vm.exec("env | sort | sed -n '1,2p'", {
71+
cwd: "/workspace",
72+
timeoutMs: 60_000,
73+
});
74+
expect(r.exitCode, `cap=${maxBlockingReadMs} stderr=${r.stderr}`).toBe(
75+
0,
76+
);
77+
return Date.now() - startedAt;
78+
} finally {
79+
await vm.dispose();
80+
}
81+
};
82+
83+
const lowCapMs = 500;
84+
const highCapMs = 8000;
85+
const lowElapsed = await timePipeline(lowCapMs);
86+
const highElapsed = await timePipeline(highCapMs);
87+
88+
// If the per-stage stall were still present, the high-cap run would pay
89+
// roughly an extra stage-worth of watchdog time. Allow generous slack for
90+
// unoptimized (debug) builds and VM warmup while still catching a stall:
91+
// a single extra stage at the 8s cap would blow past this bound.
92+
expect(
93+
highElapsed,
94+
`low(${lowCapMs}ms cap)=${lowElapsed}ms high(${highCapMs}ms cap)=${highElapsed}ms`,
95+
).toBeLessThan(lowElapsed + highCapMs);
96+
}, 180_000);
97+
});

0 commit comments

Comments
 (0)