Skip to content

Commit a07dd95

Browse files
committed
wsd, rpc: fix shim reverse-direction race via beforeFetch hook
Under FUSE_MOUNT=shim, a write inside an exec'd process didn't make it back to the DO unless something later triggered a fresh push/pull cycle. The shim's disk\u2192VFS path is a 250ms poll; the worker's post-exec pullOnce races against that poll, and on a fast exec the pull beats the poll \u2014 fetchChanges streams the empty pre-write state and the file stays stranded in wsd's VFS forever (or until the next exec drives another bracket). The push direction already had a symmetric escape hatch: afterApply on the SyncRPC fires after a peer batch commits and the shim flushes VFS\u2192disk before the push returns. Add the matching beforeFetch hook in the rpc server. wsd wires it to shim.reconcileNow(), a new public method on ShimMount that runs the same disk\u2192VFS reconcile the poll loop runs, serialised through the same internal mutex. With the hook in place, fetchChanges awaits the shim's reconcile before computing the change set, so a Workspace.pull issued right after shell.exec sees every file the spawned process wrote. Verified end-to-end against examples/wsd-container under wrangler dev (FUSE_MOUNT=auto resolves to shim with no /dev/fuse): the smoke script's step 4 now passes with SETTLE_SECONDS=0, and the previously-stranded single-exec scenario reads back the written file immediately. beforeFetch is documented to fire on every fetch, including ones that would otherwise stream zero entries \u2014 the hook is what produces the entries in the first place. Errors are caught and logged so a wedged shim can't take down the wire. Test coverage mirrors the afterApply set: fires once per pull, surfaces hook- materialised writes, swallows thrown hooks. The example script drops its post-exec sleep now that the contract is synchronous.
1 parent e2a7a66 commit a07dd95

6 files changed

Lines changed: 221 additions & 11 deletions

File tree

examples/wsd-container/script/run

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ BASE_URL="${BASE_URL%/}"
1818
NAME="${NAME:-demo}"
1919
PATH_VIA_API="api-wrote.txt"
2020
PATH_VIA_EXEC="exec-wrote.txt"
21-
SETTLE_SECONDS="${SETTLE_SECONDS:-3}"
2221

2322
step() { printf '\n=== %s ===\n' "$*"; }
2423
fail() { printf '\nFAIL: %s\n' "$*" >&2; exit 1; }
@@ -41,9 +40,9 @@ curl -fsS -X POST "${BASE_URL}/c/${NAME}/exec" \
4140
-H 'content-type: application/json' \
4241
-d "{\"command\":\"echo hello-from-exec > /workspace/${PATH_VIA_EXEC} && cat /workspace/${PATH_VIA_EXEC}\",\"encoding\":\"utf8\"}"
4342
printf '\n'
44-
# The shim's reverse-direction reconciler polls on a short tick.
45-
# Sleep a beat so the GET below sees the new bytes the first time.
46-
sleep "$SETTLE_SECONDS"
43+
# No settle wait needed: wsd's `beforeFetch` hook runs the shim's
44+
# disk→VFS reconcile synchronously inside `fetchChanges`, so the
45+
# GET below is guaranteed to see whatever the exec just wrote.
4746

4847
step "4. read via API: GET /c/${NAME}/file/workspace/${PATH_VIA_EXEC}"
4948
read_via_api=$(curl -fsS "${BASE_URL}/c/${NAME}/file/workspace/${PATH_VIA_EXEC}")

packages/rpc/src/server.ts

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,13 +59,28 @@ export interface ServerOptions {
5959
* push failure.
6060
*/
6161
afterApply?: () => void | Promise<void>;
62+
/**
63+
* Optional hook fired inside the SyncRPC `fetchChanges` handler,
64+
* right before the receiver computes the change set the puller
65+
* will see. Resolved before any entries stream. Used by wsd to
66+
* settle the userspace shim's disk→VFS reconcile so a
67+
* `Workspace.pull()` issued right after `shell.exec` returns the
68+
* files the exec'd process wrote, without waiting on the shim's
69+
* periodic poll.
70+
*
71+
* Fires on every fetch, including ones that would otherwise
72+
* stream zero entries — the hook is what produces the entries in
73+
* the first place. Errors are caught and logged; a hook failure
74+
* must not fail the fetch.
75+
*/
76+
beforeFetch?: () => void | Promise<void>;
6277
}
6378

6479
class SyncRPCServer extends RpcTarget implements SyncRPC {
6580
constructor(
6681
private readonly db: Database,
6782
private readonly options: Required<Pick<ServerOptions, "ignore">> &
68-
Pick<ServerOptions, "afterApply">,
83+
Pick<ServerOptions, "afterApply" | "beforeFetch">,
6984
) {
7085
super();
7186
trackStub(this);
@@ -134,6 +149,16 @@ class SyncRPCServer extends RpcTarget implements SyncRPC {
134149
appliedPushRev: number;
135150
stream: ReadableStream<ChangeEntry>;
136151
}> {
152+
if (this.options.beforeFetch !== undefined) {
153+
try {
154+
await this.options.beforeFetch();
155+
} catch (err) {
156+
// Settle hook failures must not surface as fetch failures —
157+
// we still want to stream whatever's already in the store.
158+
// Log so the operator notices a wedged shim, then carry on.
159+
console.warn("[SyncRPCServer] beforeFetch hook failed:", err);
160+
}
161+
}
137162
const sinceRev = input.sinceRev ?? 0;
138163
const ignore =
139164
input.ignore ?? (this.options.ignore.length > 0 ? this.options.ignore : DEFAULT_IGNORE);
@@ -255,7 +280,11 @@ class WorkspaceRPCServer extends RpcTarget implements WorkspaceRPC {
255280
// back the object to mount on each connection via
256281
// acceptWebSocketSession().
257282
export function createSyncServer(db: Database, options: ServerOptions = {}): SyncRPC {
258-
return new SyncRPCServer(db, { ignore: options.ignore ?? [], afterApply: options.afterApply });
283+
return new SyncRPCServer(db, {
284+
ignore: options.ignore ?? [],
285+
afterApply: options.afterApply,
286+
beforeFetch: options.beforeFetch,
287+
});
259288
}
260289

261290
// Construct a ShellRPC bound to a Runner. wsd holds the only

packages/rpc/src/sync-driver.test.ts

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,107 @@ describe("SyncRPC server — afterApply hook", () => {
234234
});
235235
});
236236

237+
describe("SyncRPC server — beforeFetch hook", () => {
238+
// Symmetric to the afterApply spy above. beforeFetch runs on the
239+
// receiver right before fetchChanges streams entries, giving the
240+
// host a chance to settle any out-of-band writes (e.g. wsd's shim
241+
// pulling disk changes into the VFS) into the store the fetch is
242+
// about to read.
243+
function makeReceiverWithSpy(): {
244+
db: Database;
245+
rpc: SyncRPC;
246+
close: () => void;
247+
calls: number;
248+
setBeforeFetch: (fn: () => void | Promise<void>) => void;
249+
} {
250+
const storage = new SQLiteTestStorage();
251+
const db = new Database(storage);
252+
initializeSchema(db, () => 1000);
253+
let hook: () => void | Promise<void> = () => {};
254+
let calls = 0;
255+
const rpc = createSyncServer(db, {
256+
beforeFetch: async () => {
257+
calls += 1;
258+
await hook();
259+
},
260+
});
261+
return {
262+
db,
263+
rpc,
264+
close: () => storage.close(),
265+
get calls() {
266+
return calls;
267+
},
268+
setBeforeFetch: (fn) => {
269+
hook = fn;
270+
},
271+
};
272+
}
273+
274+
it("fires once per fetchChanges and is awaited before entries stream", async () => {
275+
const a = makePeer();
276+
const b = makeReceiverWithSpy();
277+
try {
278+
// Stage a write in the hook itself — simulates the shim's
279+
// disk→VFS reconcile picking up a file that wasn't in the VFS
280+
// when pullOnce started.
281+
const providerB = new SQLiteWorkspaceProvider(b.db, { now: () => 2 });
282+
b.setBeforeFetch(() => {
283+
providerB.writeFileSync("/late.txt", "materialised by hook");
284+
});
285+
286+
const result = await pullOnce(a.db, b.rpc);
287+
expect(b.calls).toBe(1);
288+
// The pull must surface the entry the hook wrote — that's the
289+
// whole point of beforeFetch existing.
290+
expect(result.applied).toBeGreaterThan(0);
291+
expect(fileEntries(a.db)).toContain("late.txt");
292+
} finally {
293+
a.close();
294+
b.close();
295+
}
296+
});
297+
298+
it("fires even when there are no changes to stream", async () => {
299+
// Pulling against an empty receiver still has to call the hook,
300+
// because the hook is what produces "any changes" in the first
301+
// place — conditional firing would defeat the contract.
302+
const a = makePeer();
303+
const b = makeReceiverWithSpy();
304+
try {
305+
const result = await pullOnce(a.db, b.rpc);
306+
expect(b.calls).toBe(1);
307+
expect(result.applied).toBe(0);
308+
} finally {
309+
a.close();
310+
b.close();
311+
}
312+
});
313+
314+
it("a thrown hook does not fail the fetch", async () => {
315+
const a = makePeer();
316+
const b = makeReceiverWithSpy();
317+
try {
318+
const providerB = new SQLiteWorkspaceProvider(b.db, { now: () => 2 });
319+
providerB.writeFileSync("/already-there.txt", "x");
320+
321+
b.setBeforeFetch(() => {
322+
throw new Error("reconcile blew up");
323+
});
324+
325+
// The fetch must still succeed and return the pre-existing
326+
// entry. The receiver logs and swallows hook errors.
327+
const result = await pullOnce(a.db, b.rpc);
328+
expect(b.calls).toBe(1);
329+
expect(result.applied).toBe(1);
330+
expect(fileEntries(a.db)).toContain("already-there.txt");
331+
} finally {
332+
a.close();
333+
b.close();
334+
}
335+
});
336+
});
337+
237338
describe("sync driver — bidirectional convergence", () => {
238339
it("two peers writing in parallel converge after a few ticks", async () => {
239340
const a = makePeer();

packages/wsd/src/cli/wsd.ts

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -419,9 +419,9 @@ async function main(): Promise<void> {
419419

420420
let fuse: FuseMount | undefined;
421421
// When running on the userspace shim, capture the typed handle
422-
// so we can wire `flush()` into the SyncRPC `push` afterApply
423-
// hook below. A real FUSE mount serves reads straight from the
424-
// VFS, so it doesn't need an explicit settle.
422+
// so we can wire `flush()` and `reconcileNow()` into the SyncRPC
423+
// afterApply / beforeFetch hooks below. A real FUSE mount serves
424+
// reads straight from the VFS, so it doesn't need either settle.
425425
let shim: ShimMount | undefined;
426426
if (backend.kind !== "none") {
427427
// The VFS stores everything under `mountPoint` so capnweb pulls,
@@ -467,7 +467,19 @@ async function main(): Promise<void> {
467467
// exec()/read against the host fs after a push sees the new
468468
// files. Real FUSE doesn't need this — the kernel-FUSE driver
469469
// serves reads from the VFS directly.
470-
...(shim ? { afterApply: () => shim.flush() } : {}),
470+
// Symmetric shim settles:
471+
// - afterApply (push side): wait for the VFS→disk flush so
472+
// a subsequent `shell.exec` sees the just-pushed files.
473+
// - beforeFetch (pull side): wait for the disk→VFS reconcile
474+
// so a `Workspace.pull()` issued right after `shell.exec`
475+
// observes files the exec'd process just wrote, without
476+
// waiting on the next periodic poll tick.
477+
...(shim
478+
? {
479+
afterApply: () => shim.flush(),
480+
beforeFetch: () => shim.reconcileNow(),
481+
}
482+
: {}),
471483
});
472484
const http = createHTTPServer(info, rpc);
473485

packages/wsd/src/shim/shim.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,54 @@ test("shim.flush() resolves on an unmounted shim without throwing", async (_ctx)
172172
await shim.flush();
173173
});
174174

175+
test("shim.reconcileNow() settles disk writes into the VFS before resolving", async (_ctx) => {
176+
// Mirror of the flush() test above, in the reverse direction.
177+
// A very slow poll guarantees the periodic reconcile can't be
178+
// serving the assertion; if reconcileNow() works the file is in
179+
// the VFS as soon as the call returns.
180+
const mountPoint = await fs.mkdtemp(path.join(os.tmpdir(), "wsd-shim-reconcile-"));
181+
const { vfs } = await createNodeVirtualFileSystem();
182+
const shim = await mountShim({ vfs, mountPoint, pollIntervalMs: 60_000 });
183+
onTestFinished(async () => {
184+
await shim.unmount();
185+
await fs.rm(mountPoint, { recursive: true, force: true });
186+
});
187+
188+
await fs.mkdir(path.join(mountPoint, "proj"), { recursive: true });
189+
await fs.writeFile(path.join(mountPoint, "proj", "x.txt"), "from disk");
190+
await fs.writeFile(path.join(mountPoint, "proj", "y.txt"), "also from disk");
191+
192+
// No periodic reconcile has fired yet — the VFS is empty until
193+
// reconcileNow() walks the disk.
194+
expect(vfs.existsSync(`${mountPoint}/proj/x.txt`)).toBe(false);
195+
196+
await shim.reconcileNow();
197+
198+
expect(vfs.readFileSync(`${mountPoint}/proj/x.txt`).toString()).toBe("from disk");
199+
expect(vfs.readFileSync(`${mountPoint}/proj/y.txt`).toString()).toBe("also from disk");
200+
});
201+
202+
test("shim.reconcileNow() is idempotent and cheap on a clean tree", async (_ctx) => {
203+
const { vfs, mountPoint, shim } = await setup();
204+
await fs.writeFile(path.join(mountPoint, "stable.txt"), "steady");
205+
await shim.reconcileNow();
206+
const rev1 = vfs.statSync(`${mountPoint}/stable.txt`).mtime.getTime();
207+
await shim.reconcileNow();
208+
const rev2 = vfs.statSync(`${mountPoint}/stable.txt`).mtime.getTime();
209+
expect(rev2).toBe(rev1, "reconcileNow on an unchanged tree should not bump VFS mtime");
210+
});
211+
212+
test("shim.reconcileNow() resolves on an unmounted shim without throwing", async (_ctx) => {
213+
const mountPoint = await fs.mkdtemp(path.join(os.tmpdir(), "wsd-shim-reconcile-unmount-"));
214+
const { vfs } = await createNodeVirtualFileSystem();
215+
const shim = await mountShim({ vfs, mountPoint, pollIntervalMs: TICK_MS });
216+
onTestFinished(async () => {
217+
await fs.rm(mountPoint, { recursive: true, force: true });
218+
});
219+
await shim.unmount();
220+
await shim.reconcileNow();
221+
});
222+
175223
test("shim drops VFS writes outside the mount point", async (_ctx) => {
176224
// Pin the cross-namespace contract that backed the original bug:
177225
// a write into the VFS at `${mountPoint}/foo` lands on disk at

packages/wsd/src/shim/shim.ts

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,19 @@ export interface ShimMount {
5050
* so flushing twice in a row is cheap.
5151
*/
5252
flush(): Promise<void>;
53+
/**
54+
* Block until the VFS reflects the on-disk tree's current state.
55+
* Called by the SyncRPC `fetchChanges` handler right before it
56+
* computes the change set the puller will see, so a
57+
* `Workspace.pull()` issued after `shell.exec` returns observes
58+
* files the exec'd process wrote without waiting on the next
59+
* periodic poll tick.
60+
*
61+
* Runs the same disk→VFS reconcile the polling loop runs,
62+
* serialised through the same internal mutex so a request-time
63+
* call can't race with the tick. Idempotent on a clean tree.
64+
*/
65+
reconcileNow(): Promise<void>;
5366
}
5467

5568
export interface MountShimOptions {
@@ -155,9 +168,13 @@ export async function mountShim(options: MountShimOptions): Promise<ShimMount> {
155168

156169
// Disk -> VFS via periodic reconcile. Walks the mount point,
157170
// diffs against the shadow, applies changes to the VFS.
171+
// Shared by the periodic poll below and the on-demand
172+
// reconcileNow() hook; both go through `run` so they serialise
173+
// against the VFS watcher loop.
174+
const reconcile = (): Promise<void> => run(() => reconcileDiskToVfs(vfs, mountPoint, shadow));
158175
const pollTimer = setInterval(() => {
159176
if (stopped) return;
160-
void run(() => reconcileDiskToVfs(vfs, mountPoint, shadow)).catch((error) => {
177+
void reconcile().catch((error) => {
161178
console.error("[shim] disk reconcile failed:", error);
162179
});
163180
}, pollIntervalMs);
@@ -190,6 +207,10 @@ export async function mountShim(options: MountShimOptions): Promise<ShimMount> {
190207
await flushVfsToDisk(vfs, mountPoint, shadow);
191208
});
192209
},
210+
async reconcileNow(): Promise<void> {
211+
if (stopped) return;
212+
await reconcile();
213+
},
193214
};
194215
}
195216

0 commit comments

Comments
 (0)