2026-09-05 · 8 min read

JavaScript code review exercise: the import succeeds before it finishes

I have seen this bug in more than one codebase, and it shows up roughly the same way every time. A user import is "working" in production. The admin dashboard shows a green checkmark. But the actual table is short a handful of rows. Nobody notices until someone queries the database and a few accounts are just gone. No error in the logs, no failed HTTP response, no alert. The import function returned success, the response was sent to the browser, and then, a few hundred milliseconds later, a few inserts quietly failed because of a duplicate-key constraint. The promises that carried those failures had nobody listening. The function had already left the building.

The root cause was forEach with an async callback. This is a JavaScript code review exercise built around that exact bug: an async/await mistake that passes every linter, every type checker, and most test suites without a whisper. It is a worked example with the full solution, the fix, and the test that catches it. If you review Node.js pull requests, whether the code was written by a colleague or generated by an AI assistant, this is the pattern that will burn you the most quietly, because the keywords async and await are both present, the logic reads sequentially, and nothing about it screams "fire and forget." But that is exactly what it does.

The requirements

An admin needs a bulk import endpoint. The spec is four lines:

Add an admin endpoint that imports a list of users.
Every user must be inserted into the database.
The endpoint must only report success after all inserts have completed.
A failed insert must not be silently swallowed.

Context: this runs as an admin-triggered batch job. Imports can be several thousand users.

The PR

The AI assistant opened a two-file PR: a new service and a route that wires it up.

ai-agent wants to merge · src/services/importUsers.js +9

1+ async function importUsers(users) {

2+ users.forEach(async (user) => {

3+ await db.insert(user);

4+ });

5+ console.log("All users imported ✅");

6+ return { imported: users.length };

7+ }

9+ module.exports = { importUsers };

ai-agent wants to merge · src/routes/admin.js +4 -1

1 const express = require("express");

2 const { listUsers } = require("../services/listUsers");

3+ const { importUsers } = require("../services/importUsers");

5 const router = express.Router();

7 router.get("/users", listUsers);

8+ router.post("/users/import", async (req, res) => {

9+ res.json(await importUsers(req.body.users));

10+ });

12 module.exports = router;

Nine lines of new service code, three lines of routing. Clean, readable. The PR description says "verified with a small test file, all rows appeared in the table." And they probably did, because with three test users and a local database, every insert finishes before you can blink. The bug only bites at scale, or when one insert is slow, or when one fails.

Try it yourself

This is a real exercise. Try reviewing it cold before reading the walkthrough.

Same PR, no annotations, your call. You get the canonical review after your verdict.

Review this PR →

Here be spoilers

Eight characters that change everything

The entire bug lives on line 2 of src/services/importUsers.js:

2+ users.forEach(async (user) => {

3+ await db.insert(user);

4+ });

Array.prototype.forEach calls its callback once per element, synchronously, in order. It does not look at the return value. It does not know what a Promise is. If you hand it an async function, it calls that function, receives the Promise it returns, and throws it on the floor. The await db.insert(user) on line 3 does pause execution of that particular callback, but nobody is waiting for that callback to finish. forEach is not, importUsers is not, the route handler is not.

So here is what actually happens at runtime. importUsers is called. forEach fires off N async callbacks. Each one starts executing and hits its first await, at which point it yields. forEach returns undefined. Execution continues to line 5: console.log("All users imported ✅"). Line 6: return { imported: users.length }. The route handler gets the result. Express sends a 200 with {"imported": 3000}. The HTTP response is on the wire. The admin sees a green checkmark.

Meanwhile, back in the event loop, those N promises are still resolving. Some inserts have not even started yet. If one of them rejects, say a duplicate key or a connection timeout, that rejection has nowhere to go. There is no .catch() on the promise, no try/catch that wraps the whole batch, no caller awaiting it. In Node.js 15 and later, an unhandled promise rejection terminates the process by default. So a single bad row in a 3,000-user import can crash the server, and the admin already got their success response thirty milliseconds ago.

To be precise about the runtime behavior: Node.js 15+ (released November 2020) changed the default for --unhandled-rejections from warn to throw. On Node 14 and earlier, or if the flag is set to warn, the rejection prints a warning to stderr and keeps running. Either way, the failure is invisible to the caller and breaks requirements 3 and 4.

Three requirements, three failures

Let's walk through what this code violates:

Requirement 2: "Every user must be inserted into the database." The function returns { imported: users.length } unconditionally. It does not check whether any insert actually succeeded. It returns the count of users it was asked to import, not the count it actually imported. If the database is under load and half the inserts time out, the response still says all of them went through.

Requirement 3: "The endpoint must only report success after all inserts have completed." This is the core failure. The endpoint reports success before the inserts complete. Not after some of them, not after most of them. Before any of them, potentially, because forEach fires off all the callbacks synchronously and then the function falls through to the return statement on the same tick.

Requirement 4: "A failed insert must not be silently swallowed." Every failed insert becomes an unhandled promise rejection. On modern Node, that crashes the process. On older Node, it logs a deprecation warning to stderr that nobody reads. Neither of these surfaces the error to the caller.

The comment a reviewer would actually leave

1+ async function importUsers(users) {

2+ users.forEach(async (user) => {

Yyou commented

forEach ignores the promises returned by an async callback. None of these inserts are awaited by importUsers, so the function returns { imported: N } and the endpoint responds with 200 before any insert has finished. If an insert fails, the rejection is unhandled. Replace with for...of and await each insert, or map to promises and await Promise.all(...) if order does not matter.

3+ await db.insert(user);

4+ });

Verdict: Request changes. The endpoint does not wait for the work it claims to have done. This is not a style issue or a nice-to-have. The function literally lies about its own completion. One comment, one blocker, and a clear path to the fix.

The fix, and why there are two options

The minimal fix is a for...of loop:

2- users.forEach(async (user) => {

3- await db.insert(user);

4- });

2+ for (const user of users) {

3+ await db.insert(user);

4+ }

This inserts users one at a time, in order, and if any insert throws, the error propagates to the caller. The route handler's await importUsers(...) catches it, Express returns a 500, and the admin knows something went wrong. Requirements 3 and 4 are now met.

If you want parallelism (and your database can handle it), the alternative is:

2+ await Promise.all(users.map((user) => db.insert(user)));

Promise.all takes an array of promises, waits for all of them, and rejects if any one rejects. The key difference from the forEach version: someone is actually holding the promises. Promise.all collects them, await suspends importUsers until they all settle, and a failure propagates upward.

For a several-thousand-user import, you might worry about firing 3,000 concurrent database inserts. That is a reasonable concern, and if you hit it, the answer is batching (chunking the array and processing each chunk with Promise.all). But that is an optimization discussion for the next round of review, not a reason to keep the current code that does not wait at all.

A test that catches the old code and passes the fix

The trick is making the insert take a measurable amount of time so you can observe whether the function actually waits:

1+ const { importUsers } = require("../services/importUsers");

3+ test("importUsers waits for all inserts", async () => {

4+ const inserted = [];

5+ db.insert = (user) =>

6+ new Promise((resolve) =>

7+ setTimeout(() => { inserted.push(user); resolve(); }, 50)

8+ );

10+ await importUsers([{ name: "a" }, { name: "b" }]);

11+ expect(inserted).toEqual([{ name: "a" }, { name: "b" }]);

12+ });

13+

14+ test("importUsers surfaces a failed insert", async () => {

15+ db.insert = () => Promise.reject(new Error("duplicate key"));

16+ await expect(importUsers([{ name: "x" }])).rejects.toThrow("duplicate key");

17+ });

The first test stubs db.insert with a 50ms delay. With the forEach version, importUsers returns immediately, await resolves, and inserted is still empty. The assertion fails. With the for...of fix, the function waits for both inserts, and the array contains both users.

The second test checks requirement 4. With forEach, the rejection becomes unhandled. With for...of, it propagates and the test passes.

The reflex worth building

Every time you see an async callback passed to a higher-order function, ask one question: who awaits the promise this callback returns? For for...of, it is the loop. For Promise.all(arr.map(async ...), it is Promise.all. For forEach, filter, reduce, map (without wrapping in Promise.all), and most event-handler registrations, the answer is nobody. That single question would have caught the bug in this exercise, and it catches the same bug in migration scripts, webhook dispatchers, queue consumers, and every other place where someone writes .forEach(async and moves on because the keywords look right.