A bulk-import job that fans out work over `Promise.all` and collects results by pushing onto a shared array. Looks fine. Subtle bug: any logic that depends on order, indices, or 'this is the Nth result' breaks under concurrent task completion.
The vulnerable diff
// ingestion/bulk-import.ts
async function importAll(items: Item[]) {
const results: ProcessedItem[] = [];
let okCount = 0;
let lastIndex = 0;
// BUG: side-effects to shared state from concurrent callbacks
await Promise.all(items.map(async (item) => {
const r = await process(item);
results.push(r); // non-deterministic order
okCount++; // OK — single-threaded
lastIndex = items.indexOf(item); // racy: who wins?
}));
return { results, okCount, lastIndex };
}What is wrong
JS is single-threaded so `results.push(r)` itself is atomic — no torn writes. The bug is that the *order* of pushes depends on the order of microtask completion, which is non-deterministic. Code that downstream-depends on ordering breaks. Worse, any compound 'read-then-write' to outer-scope state (like 'if results.length < N, push the result; else log overflow') is a true race because the read and write are separated by an await. The fix is to use Promise.all's return value — it preserves the input order regardless of completion order.
The attack
Symptom looks like a flaky test:
// Test sometimes passes, sometimes fails:
expect(results[0].sourceId).toBe(items[0].sourceId);
// Fails when process(items[1]) completes before process(items[0]).
// Looks intermittent. Three retries make it pass. CI labels as flake.The 'flake' is the bug — race conditions look like flakiness until you understand the model.
Mesrai's review comment
mesraipilot · Bot · reviewed 2 min ago
[mesrai] [code-review] [Logic] [Race] [medium]
Side-effects to outer-scope state from inside Promise.all callbacks
are order-dependent. Two problems:
1. results.push order is non-deterministic — depends on completion
order, not input order. Downstream code that assumes input order
breaks intermittently.
2. lastIndex = items.indexOf(item) is racy — the last callback to
complete wins. Multiple completions in the same microtask tick
give nondeterministic results.
Use Promise.all's return value, which preserves input order:
const results = await Promise.all(items.map(item => process(item)));
const okCount = results.length;
// lastIndex doesn't make sense for parallel work — drop it or
// restructure the API.
If you genuinely need backpressure / streaming results, use
p-limit or for-await with an async iterator — not Promise.all.The fix
// ingestion/bulk-import.ts — fixed
async function importAll(items: Item[]) {
// Promise.all preserves input order in its result array
const results = await Promise.all(items.map(item => process(item)));
return {
results,
okCount: results.length,
};
}
// For backpressure / partial-failure resilience:
import pLimit from "p-limit";
const limit = pLimit(8);
const results = await Promise.all(
items.map(item => limit(() => process(item)))
);`Promise.all` preserves input order in the returned array — index 0 of the result corresponds to index 0 of the input, regardless of which finished first. No shared state, no order race. For backpressure, use `p-limit` to cap concurrency without changing the order semantics.
Why human review missed it
The bug looks like working code in a single-threaded language — there is no obvious data race. The subtlety is that 'single-threaded' applies to memory operations, not to the order in which awaited work completes. Mesrai's rule pack catches every Promise.all callback that mutates outer-scope state instead of returning a value.
Related rules + further reading
Mesrai rule pack: logic/no-shared-state-in-promise-all — flags side-effecting Promise.all callbacks.
MDN: Promise.all — return-value ordering documented.
Common in async-heavy codebases; surfaces as 'flaky test'.
Takeaway
Return values, not push side-effects. Promise.all already gives you ordered results. Mesrai catches the push pattern on every PR.