ubiquity-os / ubiquity-os-kernel

1 stars 19 forks source link

fix: correct manifest decoding logic #202

Closed gentlementlegen closed 2 days ago

gentlementlegen commented 2 days ago

Refactor manifest decoding to store intermediate JSON data.

What is this change

It appears that sometimes we encounter the following error:

Error: Cannot perform I/O on behalf of a different request. I/O objects (such as streams, request/response bodies, and others) created in the context of one request handler cannot be accessed from a different request's handler. This is a limitation of Cloudflare Workers which allows us to improve overall performance. (I/O type: SpanParent)

This is specific to a Cloudflare environment and would most likely not happen on a local one. The reason is that the await result.json() is done through the function call (most likely). It is safer to store it in a variable and then passing it so that cloudflare can close that handle directly, avoiding this issue. Reference in cloudflare doc: https://developers.cloudflare.com/workers/observability/errors/#cannot-perform-io-on-behalf-of-a-different-request

This is theoretical so I don't promise that it will solve our issues, but it's worth a try.

I asked Claude about this and this is the answer:

In JavaScript/TypeScript, awaiting a Promise immediately versus passing the Promise to another function that awaits it doesn't fundamentally change the variable lifespan. However, in the context of Cloudflare Workers and I/O operations, the timing of when you await and resolve I/O-bound objects is crucial.

Let's compare:

// Original approach
try {
  const result = await fetch(manifestUrl);
  const manifest = decodeManifest(await result.json()); // await inside decodeManifest
  _manifestCache[url] = manifest;
  return manifest;
}

// Modified approach
try {
  const result = await fetch(manifestUrl);
  const jsonData = await result.json();        // await outside
  const manifest = decodeManifest(jsonData);   // pass resolved data
  _manifestCache[url] = manifest;
  return manifest;
}

The key differences are:

  1. In the first approach, the Response object from fetch() and its associated I/O context are being passed into decodeManifest
  2. In the second approach, you're passing pure JSON data that's already been extracted from the I/O context

While both might work in regular JavaScript environments, in Cloudflare Workers, it's better to resolve I/O operations as early as possible to ensure the I/O context is properly closed and doesn't leak into other functions or get stored in ways that might cross request boundaries.

Think of it like this:

So while the variable lifespan itself doesn't change, the timing of when you resolve the I/O context does matter in the Workers environment.