DEV Community

Daniel Pertu
Daniel Pertu

Posted on

We had three copies of a nine-line timeout helper, so a stall in a job, a route and a cache were three different errors

Promise.race against a setTimeout is the kind of utility everybody writes inline, which is why Munchable's backend had written it three times:

  • once in the job runner, for a database step
  • once in the label-reading API route, for a step on the request path
  • once in the taxonomy cache, for a Redis write

Identical implementations. Different rejection values. The comment at the top of the file that replaced all three says what that cost:

There were three: one in lib/jobs/util.ts, one in app/api/ocr/route.ts and one in lib/taxonomy/store.ts, identical but for the error they rejected with, so a stall in a job, a stall in the OCR route and a stall in the cache were three different error shapes and nothing keyed on "timed out" could see all of them.

That is the actual bug in duplicated utilities, and it is not the duplication. Three copies of the same nine lines is a rounding error in a codebase. Three different error shapes for the same failure mode means you cannot ask the question "what timed out this week" in one place, and you cannot write a single piece of handling that covers all of them.

The whole file

// apps/web/lib/promise.ts

/** A step that did not finish inside its budget. Carries the step's name. */
export class StepTimeout extends Error {
  constructor(
    public readonly step: string,
    public readonly ms: number,
  ) {
    super(`step "${step}" timed out after ${ms} ms`);
    this.name = 'StepTimeout';
  }
}

/** Reject after `ms` with a StepTimeout naming the step. */
export function withTimeout<T>(p: Promise<T>, ms: number, step: string): Promise<T> {
  return new Promise<T>((resolve, reject) => {
    const t = setTimeout(() => reject(new StepTimeout(step, ms)), ms);
    p.then(
      (v) => (clearTimeout(t), resolve(v)),
      (e) => (clearTimeout(t), reject(e)),
    );
  });
}
Enter fullscreen mode Exit fullscreen mode

Three things in there are worth more than they look.

The step name is a required argument. Not optional, not defaulted. A timeout error that says timeout tells you nothing you did not already know from the stack being empty. step "load-overlay" timed out after 3000 ms tells you which of the eight awaits in the request was the slow one, and it survives into a log line that gets grepped weeks later.

ms is a field on the error, not only text in the message. When the budget is computed rather than constant, which it is in one of our three callers, the message is the only record of what the budget actually was on that request.

clearTimeout on both paths. This is the one that bites in Node rather than in a browser. A pending timer keeps the event loop alive. Forget the clear in the resolve path and a 30 second budget means a process that finished its work in 200 ms sits there for another 29.8 seconds before it is allowed to exit, which in a short-lived job or a CLI shows up as "it hangs at the end sometimes". Clearing in the reject path matters too: the promise can reject for its own reasons long before the timer fires.

Three callers, three budgets, three reactions

The helper deliberately knows nothing about how long anything should take. Every caller brings its own number, and the numbers differ by a factor of ten because the callers are in different worlds.

The job runner wraps every step and re-throws:

async step<T>(name: string, fn: () => Promise<T>, opts: { timeoutMs?: number; data?: Record<string, unknown> } = {}): Promise<T> {
  const t0 = Date.now();
  try {
    const result = await withTimeout(fn(), opts.timeoutMs ?? DB_STEP_MS, name);
    this.log.info(`step=${name} ms=${Date.now() - t0}`, opts.data);
    return result;
  } catch (e) {
    this.log.warn(`step=${name} ms=${Date.now() - t0} failed: ${String(e).slice(0, 300)}`, opts.data);
    throw e;
  }
}
Enter fullscreen mode Exit fullscreen mode

Every step in a background job is named, timed and logged on both paths, which means the log is a timing profile of the run whether or not it succeeded. The default budget there is 30 seconds, with the justification in a one-line comment: "A database round trip (or a small batch of them) has no business taking longer." A model call in the same file gets its own constant at 120 seconds, and the per-batch clock is deliberately longer still than that.

The taxonomy cache swallows it entirely:

export async function publishVersion(version: string): Promise<void> {
  memory = null;
  memoryVersion = { version, at: Date.now() };
  if (!redis) return;
  try {
    await withTimeout(
      Promise.all([redis.set(VERSION_KEY, version, { ex: REDIS_TTL_S }), redis.del(OVERLAY_KEY)]),
      CACHE_OP_MS,
      'taxonomy cache publish',
    );
  } catch {
    // The TTLs bound how long a stale cache can live; a client that has seen
    // the new version bypasses it anyway.
  }
}
Enter fullscreen mode Exit fullscreen mode

CACHE_OP_MS is 3 seconds and the catch block is empty on purpose. A cache publish that fails has a bounded consequence, because the entries carry a TTL and a reader that already knows about a newer version skips the cache anyway. The right response to a slow optional write is to stop waiting and carry on, and the comment is there so nobody later decides the empty catch is an oversight and "fixes" it into a throw.

The request path makes the budget a function of the time already spent:

const remaining = Math.max(1000, Math.min(INLINE_CALL_MS, INLINE_START_BUDGET_MS - (Date.now() - args.startedAt)));

const outcome = await withTimeout(
  resolveIngredientTags({
    ...
    deadline: Date.now() + remaining,
  }),
  remaining + DB_STEP_MS,
  'resolve-ingredient-tags',
);
Enter fullscreen mode Exit fullscreen mode

Three things are happening in those two expressions.

The budget is what is left of an overall allowance for the request, floored so it is never absurdly small and capped so one step cannot eat the whole thing. And DB_STEP_MS in this file is 3 seconds, not the job runner's 30, because here it is "a cache or database read on the request path, where the budget is tight". Same identifier, different file, an order of magnitude apart, each with its own comment. I used to think that was a smell. I now think a timeout constant belongs next to the caller precisely because the right number is a property of the context and not of the operation: 30 seconds for a database step is sensible in a nightly job and absurd while somebody waits in a shop.

The third thing is the one I would actually argue for. The inner call gets a deadline, and the outer withTimeout is set to later than that deadline, remaining + 3000. That is not sloppiness about which number wins. The component holding the deadline knows how to stop gracefully: it can return what it has resolved so far, skip the optional part of its work, and hand back a usable result. The wrapper can only reject. So the deadline is given room to fire first, and the timeout exists purely as a backstop for the case where something ignores its deadline entirely, which is exactly the case where you want a hard reject with a name on it.

That whole block then sits inside a try whose catch returns the unresolved mapping. A stall in an optional enrichment step degrades the answer, it does not fail the request. The person photographing a label still gets a reply.

What I would take from this

  1. Name the step, and make it mandatory. The cost is one argument at the call site. The benefit is that every timeout in your logs is self-describing.
  2. One error class, one name. Nothing in our code keys on instanceof StepTimeout yet, and the class is still worth it, because the day something wants to count timeouts, or retry only timeouts, or page on timeouts in one subsystem, the thing it needs to key on already exists and already covers all three callers. Three ad-hoc new Error('timeout')s is a migration you have to do first.
  3. Clear the timer on both paths.
  4. Put the budget at the call site, not in the helper. Different contexts legitimately deserve different numbers for the same operation.
  5. When something downstream holds a deadline, set your timeout later than it. A graceful partial result beats a rejection, so let the component that can produce one have the first chance.

For context on why any of this matters here: the app's job is to answer, in a supermarket aisle, whether a packaged food suits the conditions somebody has told it about. The conditions it covers are at munchable.app/conditions, and the static version of its answers for single ingredients is at munchable.app/answers. The answer itself is computed on the phone, so a stalled server step is never allowed to be the thing that decides whether you get one. That is a design rule, and withTimeout with a named error is how a 31-line file helps keep it true.

Top comments (0)