Five Deceptive Node.js Bugs That Pass Code Review Unnoticed

Each of the five code samples below executes without throwing, and each would likely slip past a quick code review unnoticed. Yet every one of them has caused a genuine outage in some production system, more than once. Before reading the explanation under each snippet, try to work out for yourself what's wrong.

Bug 1

async function notifyAllUsers(userIds) {
  userIds.forEach(async (id) => {
    const user = await getUser(id);
    await sendNotification(user);
  });
  console.log("All notifications sent!");
}

Pause here and think through what actually happens when this runs.

The bug: the line console.log("All notifications sent!") fires before any notification has actually gone out, and the outer function itself resolves without ever waiting for the individual sends to complete.

The reason is that Array.prototype.forEach has no concept of promises. It invokes the callback for each element and immediately moves to the next one, completely disregarding whatever value the callback returns. Declaring the callback async changes nothing about how forEach behaves — it simply means each call now produces a promise that forEach discards without inspection. The notifications still get sent eventually, just asynchronously in the background, with no guaranteed ordering and no mechanism for the caller to detect completion or failure.

async function notifyAllUsers(userIds) {
  await Promise.all(userIds.map(async (id) => {
    const user = await getUser(id);
    await sendNotification(user);
  }));
  console.log("All notifications sent!");
}

Switching to map means the promises are collected into an array rather than thrown away, and wrapping that array in Promise.all forces the function to genuinely wait until every send has finished. Now the log statement is telling the truth.

Bug 2

app.post("/orders", async (req, res) => {
  const order = await createOrder(req.body);
  sendConfirmationEmail(order.customerEmail);
  res.status(201).json(order);
});

The route appears to work fine — orders are created, emails go out, and the response comes back quickly. So what's the problem?

The bug: sendConfirmationEmail is invoked without await, so its returned promise is left completely unmanaged. If that promise rejects, nothing catches it. This pattern is known as a floating promise, and it isn't merely a stylistic issue — it's an operational hazard. On current versions of Node.js, an unhandled promise rejection can bring down the entire process, taking every other request in flight down with it, rather than simply failing the email quietly.

There's also a real architectural question hiding here, beyond the missing error handling: should a failed confirmation email prevent the order from succeeding, or should the order go through regardless? In most cases you'd want the latter — the order really did succeed even if the notification didn't. But there's a difference between choosing not to block on something and failing to handle its errors at all, and this code has accidentally done the second while probably intending the first.

app.post("/orders", async (req, res) => {
  const order = await createOrder(req.body);

 sendConfirmationEmail(order.customerEmail).catch((err) => {
    logger.error({ orderId: order.id, err }, "Failed to send confirmation email");
  });

  res.status(201).json(order);
});

With this version, the email genuinely doesn't hold up the HTTP response, but a failure is now logged rather than disappearing silently or crashing the server.

Bug 3

function applyDiscount(cart) {
  const updatedCart = { ...cart };
  updatedCart.items.forEach((item) => {
    item.price = item.price * 0.9;
  });
  return updatedCart;
}

At first glance this resembles the standard "copy instead of mutate" idiom you'd see in any guide about avoiding side effects. It's actually a subtler trap.

The bug: the spread { ...cart } only performs a shallow copy. It builds a new object at the top level, but updatedCart.items still points to the very same array — and the same item objects — as cart.items. So when the forEach loop mutates item.price, it's mutating the original cart's line items too, invisibly, because the spread operator never touched anything beyond the first level of the structure.

console.log(cart.items[0].price);        // already discounted, unintentionally
console.log(updatedCart.items[0].price); // same object, same value

Anyone who assumed the original cart would remain untouched — a reasonable expectation given the function's name implies it returns something new — ends up working with silently corrupted data instead.

function applyDiscount(cart) {
  return {
    ...cart,
    items: cart.items.map((item) => ({ ...item, price: item.price * 0.9 })),
  };
}

Each layer of nesting that needs to change must be copied explicitly at that layer. A shallow spread only protects the level it directly operates on, not anything nested underneath it.

Bug 4

function updateUserSettings(user, updates) {
  return Object.assign({}, user, updates);
}

app.patch("/settings", (req, res) => {
  const updated = updateUserSettings(req.user, req.body);
  saveUser(updated);
  res.json(updated);
});

This looks like an ordinary "merge an update object into an existing one" routine. So where's the danger hiding?

The bug: req.body comes straight from the client, and nothing here limits which keys are allowed to flow into the user object. If a request body contains something like "role": "admin" or "isVerified": true, those properties get merged in just as readily as any legitimate settings field, since Object.assign has no concept of which keys are supposed to be writable — it merges whatever it's given.

This falls under a well-known category of vulnerability called mass assignment, and it shows up often in APIs that push request bodies straight into database models without first filtering them through an allowlist. The risk actually grows as the merging function becomes more generic and reusable — which is exactly what makes this kind of code feel trustworthy in the first place.

function updateUserSettings(user, updates) {
  const allowedFields = ["displayName", "timezone", "emailNotifications"];
  const safeUpdates = {};
  for (const field of allowedFields) {
    if (field in updates) safeUpdates[field] = updates[field];
  }
  return { ...user, ...safeUpdates };
}

By defining an explicit allowlist, you guarantee that a request can never affect a field it wasn't specifically cleared to modify, regardless of what extra keys someone slips into the payload.

Bug 5

async function getUserWithPosts(userId) {
  const user = await db.query("SELECT * FROM users WHERE id = $1", [userId]);
  const posts = await db.query("SELECT * FROM posts WHERE user_id = $1", [userId]);
  return { ...user, posts };
}

Nothing about this code is technically wrong. But what does it cost you to write it this way?

The bug — or really, the missed opportunity — is that these two queries don't depend on one another at all. The second query doesn't need any result from the first before it can run. By chaining them with sequential await calls, the total wait time becomes the sum of both queries' durations, with one blocking the other for no real reason.

async function getUserWithPosts(userId) {
  const [user, posts] = await Promise.all([
    db.query("SELECT * FROM users WHERE id = $1", [userId]),
    db.query("SELECT * FROM posts WHERE user_id = $1", [userId]),
  ]);
  return { ...user, posts };
}

Using Promise.all lets both queries execute concurrently rather than one after another, so the overall time collapses to roughly whichever query is slower, instead of the combined total of both. This isn't a bug in the sense that it produces incorrect results — the sequential version returns perfectly accurate data. It's a bug in the sense that it silently wastes available performance, and it's the kind of pattern that gets written out of habit and rarely gets questioned, since nothing about the code looks broken on a casual read.

What All Five Have in Common

Each of these snippets ran without error, produced output that looked reasonable, and would sail through a quick skim. None of them would surface if your only test was "does it work when I try it once." Catching them requires a particular kind of skepticism: does this asynchronous code genuinely wait for everything it appears to be waiting for; does this copy operation actually duplicate every layer it needs to; does this merge logic trust input it has no business trusting. That kind of instinct isn't built from memorizing more syntax. It comes from having been burned by each of these five exact patterns at least once before.