`vm.fs.writeFile()` doesn't serialize writes, but the guest protocol expects them to be serialized

[maxhully]

I've noticed an issue where concurrent calls to vm.fs.writeFile() can lead to some of the writes hanging indefinitely. Here's what I think is happening.

The handleFileWrite function in the guest (guest/src/sandboxd/file_requests.zig) assumes that all the data frames after a write request belong to that request, until it sees an EOF.

But if you make concurrent calls to vm.fs.writeFile(), the guest can receive a mix of FileWriteRequests and data frames from both requests. This violates handleFileWrite's assumption. The writes can fail in two ways:

• If the guest is handling FileWriteRequest A and receives data for FileWriteRequest B, it fails with InvalidValue because the request ID doesn't match the request we're handling.
• If the guest is handling FileWriteRequest A and receives FileWriteRequest B, it fails with UnexpectedType because we expected a data frame.

In the UnexpectedType case, the guest consumes B's request while handling A, so we never actually see B as a FileWriteRequest in main.zig. So all of B's data frames produce invalid_request errors with ID 0, and we never get a file_write_done or an error with B's ID. This can leave the host-side promise for B pending indefinitely, so that the write operation seems to hang. (I think it can manifest differently, too, depending on how the concurrent stuff plays out.)

I put together a short repro script (just doing some concurrent writes) for this issue:

Script to reproduce

// host/examples/repro-concurrent-write.ts
// (written by 🤖)

import { VM } from "../src/index.ts";

const concurrency = 8;
const payloadBytes = 1024 * 1024;
const observationMs = 3000;

const vm = await VM.create({ server: { console: "none" } });
const abort = new AbortController();
const states = Array.from({ length: concurrency }, () => "pending");

try {
  await vm.start();

  const payload = Buffer.alloc(payloadBytes, 0x61);
  const writes = Array.from({ length: concurrency }, (_, index) => {
    const guestPath = `/tmp/concurrent-write-${index}.bin`;

    return vm.fs
      .writeFile(guestPath, payload, { signal: abort.signal })
      .then(() => {
        states[index] = "fulfilled";
      })
      .catch((error: unknown) => {
        states[index] = `rejected: ${error instanceof Error ? error.message : String(error)}`;
      });
  });

  await new Promise((resolve) => setTimeout(resolve, observationMs));
  console.log(`states after ${observationMs}ms:`, states);

  abort.abort();
  await new Promise((resolve) => setTimeout(resolve, 1000));
  console.log("states 1s after abort:", states);

  // Do not await pending writes: the bug is that they may never settle.
  void Promise.allSettled(writes);
} finally {
  await vm.close();
}

I think this can apply to concurrent operations beyond just writes, but this write-up focuses on writes because it was easier to wrap my head around it that way.

(Speculating about solutions:) The host code looks like it intends to serialize these operations, based on the singular activeFileOpId, and how exec requests are queued. I think there is a race between the waitForExecIdle() check and setting activeFileOpId. That section isn't protected by a mutex or queue, so concurrent callers can all waitForExecIdle() and then all go set (and/or overwrite) activeFileOpId. But I also imagine that there could be a way to adjust the guest side to properly handle a mix of write requests and data frames. Of course that decision is up to you all!

(Also, thanks for making Gondolin! It has been super fun to work with.)