The interface lied, so the catch never fired

#The question

Upload a photo against a defect. Delete the file from disk. Reload the page.

The API process exited.

Not a 500. Not a broken image. The process. One missing object took the service down for every tenant on it, repeatably, for anyone still holding the link.

I was testing the opposite thing. The requirement was handle a missing photo gracefully, and I had gone looking for a confirmation that we already did, because the code plainly said so.

#What I thought was happening

Every download route in the service looked like this:

try {
  const stream = await storage.getStream(key);
  return reply.send(stream);
} catch {
  throw new StorageUnavailable();   // 502
}

Five routes, all written that way, all reviewed, all obviously correct. My model was that getStream either returns a readable stream or throws, and the catch turns a throw into a 502.

My first guess at the crash was therefore that something else entirely was wrong, because this code could not possibly be the problem. I spent a while looking at the wrong file.

#What I found

LocalDiskDriver.getStream was, in substance, this:

getStream(path: string) {
  return createReadStream(path);   // does not throw for a missing file
}

createReadStream does not stat the file. It returns a stream object immediately and opens the file asynchronously. If the file is not there, the ENOENT arrives later, as an error event on the stream.

By then the route has already returned the stream to reply.send(). The try block completed successfully. The catch is not merely unfired; it is unfirable, because the failure happens after control has left the block it guards.

The rest follows mechanically. Fastify has an in-flight reply it has already committed to. An error surfaces on the stream it is piping. It tries to serialise an error response for a reply it can no longer change, and produces FST_ERR_REP_INVALID_PAYLOAD_TYPE outside any request context, which is an uncaught exception, which ends the process.

Five correct call sites, and the bug was in none of them. They were written correctly against a driver that did not honour the contract they assumed.

#The options

Catch on the stream and destroy the reply. Attach an error handler at each call site and tear down the response. This works, and it means every route now has two error paths for one condition: the catch that handles a driver that throws, and a stream handler for a driver that does not. Every future route needs both, and forgetting the second one is silent.

Stat before streaming. Check the file exists, then open it. This is racy, the file can vanish in the gap, and it leaves the same trap set for the next driver anyone writes.

Make the driver honour the contract its callers already assume. getStream must reject if the object cannot be read.

#The decision

The third one. The interface now states it, and the drivers implement it:

• the local driver open()s the file and streams from the handle, which also closes the time-of-check-to-time-of-use window, because the descriptor is held;
• the object-storage driver checks exists() first.

storageFailure.test.ts asserts the behaviour, and I kept a revert probe: undo the driver change and the test reproduces the exact FST_ERR_REP_INVALID_PAYLOAD_TYPE. A test for a crash is worth much less if you have not watched it fail.

The reason this is the right one rather than just the tidiest: the five routes were not wrong. Rewriting them would be changing correct code to accommodate an incorrect dependency, and the sixth route, written next month by someone who read the other five, would have the same bug. Fixing the driver fixes all of them and every future one, and it keeps the failure where the code already reads as though it happens.

#What it costs

One extra round trip per object-storage download, for the exists() check. A download is not a hot path, and a 502 beats a process exit.

The window is narrowed rather than closed. An object deleted between the open() and the stream draining will still surface late. That is a much smaller target than "the file was never there", which was the case that actually occurred, and closing it entirely would mean buffering, which costs more than it saves.

#The model that made it click

When every call site has the same correct-looking guard and the bug survives all of them, the contract is wrong, not the call sites.

The sharper version, which is the thing I actually use now: an operation that can fail asynchronously, after you have handed its result to someone else, cannot be guarded by its caller. The try/catch is scoped to the synchronous call. Once the stream is passed on, the caller has no frame left to catch anything in. Any API that returns a handle which may fail later has moved the error out of the caller's reach, whatever the caller writes.

#What I would remember in six months

Two habits came out of this.

When reviewing an interface, ask when it can fail, not just whether. "Returns a stream" and "returns a stream that is known to be readable" are different contracts, and the difference is invisible at the call site, which is exactly why it has to be stated in the interface rather than assumed by everyone who uses it.

And: a failure that takes down a process deserves a revert probe, not just a passing test. Anyone can write an assertion that goes green. Watching it go red against the old code is the only thing that proves it is testing what you think.