Review Agent PRs That Fan Out Unbounded Parallel Calls

The pull request arrived during a quiet Tuesday standup. An agent had rewritten a slow enrichment loop. CI was green and staging felt twice as fast.

The change replaced a sequential map with Promise.all. Each identifier now hit a downstream billing API together. Nobody counted in-flight sockets before the merge.

Two hours later the payment vendor returned HTTP 429. The Node process also exhausted its HTTP agent pool. Latency for unrelated routes climbed without a clear owner.

This review treats that diff as a teaching case. It states what to keep, revert, and test. The sample code is a labeled, unexecuted example.

What the agent shipped

The agent framed the rewrite as a latency win. The old path awaited each enrich call in order. The new path fired every call in one burst.

// Proposal only: agent-generated handler (do not merge as-is)
async function enrichAccounts(accountIds, client) {
  const rows = await Promise.all(
    accountIds.map((id) => client.get(`/v1/accounts/${id}/balance`))
  );
  return rows.map((row) => row.data);
}

The unit tests stubbed client.get with instant fixtures. They never opened a real network socket. They never asserted a maximum in-flight count.

Downstream behavior stayed outside the automated test graph. Vendor rate limits never appeared in the CI log. Connection pool caps never appeared in those tests either.

What to trust

Trust the diagnosis that sequential awaits wasted idle time. Independent reads can overlap without changing the answers. The account id list can stay the input.

Trust tests that still pin the response mapping. Shape checks on a single record remain useful. Status handling for one isolated call can stay.

Do not trust green CI as capacity evidence. Stubs hide queueing, throttling, and DNS delay. Local duration is not proof of production concurrency.

What to revert

Revert unbounded Promise.all over user-controlled identifier lists. A single request may carry hundreds of ids. The fan-out then becomes an accidental load test.

Revert missing timeouts on each outbound call. A hung vendor socket pins a pool entry. Other tenants then wait on that same agent.

Revert silent omission of partial failure rules. Promise.all fails the whole batch on one reject. Operators then retry the full blast again later.

Revert new retries added beside the fan-out. Combined retries multiply traffic under HTTP 429. That pattern belongs in a later, separate review.

Review workflow

Reviewers can follow a fixed five-step pass. Each step produces a written note on the PR. Skip none of them for agent diffs.

Step 1 — Measure the cardinality

Read every list that feeds the parallel map. Record whether that length comes from the client. Record any server-side cap before the map.

A missing cap is a revert, not a nit. Note the largest production list from logs. If logs are absent, assume a hostile maximum.

Step 2 — Name the shared scarce resources

List sockets, vendor quotas, and shared database pools. List memory buffers held until all awaits finish. List any mutex the downstream service still uses.

If two resources can saturate together, revert the blast. Independent CPU work is not the same case. Network fan-out is the dangerous case here.

Step 3 — Demand an explicit concurrency limit

A hard limit belongs in the function signature. Default values should be conservative on first merge. Magic constants hidden inside helpers will drift later.

// Proposal only: bounded worker queue
async function mapLimit(items, limit, worker) {
  const out = new Array(items.length);
  let next = 0;

  async function run() {
    while (next < items.length) {
      const cur = next++;
      out[cur] = await worker(items[cur], cur);
    }
  }

  const n = Math.min(limit, items.length);
  await Promise.all(Array.from({ length: n }, run));
  return out;
}

The helper still needs timeouts inside the worker. A limit without deadlines only delays the hang. Both controls belong in the same review change.

Step 4 — Define partial failure before merge

Write the policy in the PR body as a table. All-or-nothing is valid only with a tiny batch. Mixed success needs a per-id error envelope.

Callers must not blindly retry identifiers that already succeeded. Idempotency keys belong on every follow-up write. This review does not reopen earlier write-retry guidance.

Step 5 — Prove the cap with a probe

Do not argue from intuition about event-loop overlap. Run a local server that counts in-flight calls. Fail the build when the cap is exceeded.

The next sections contain a concrete local probe. It is small enough for a free host. It does not require access to production traffic.

Reading the agent's justification

Agents often cite average latency from the stubbed suite. That number does not include vendor queue time. Reviewers should ignore latency claims without a live dependency.

Watch for comments that call the change a quick win. Quick wins that drop caps are not wins. Require a peak-in-flight number in the PR instead.

Watch the Node HTTP agent

Node reuses sockets through its default HTTP agent. The default agent does not cap parallel sockets per host. A Promise.all storm can open hundreds of connections.

Set maxSockets on a dedicated agent for that vendor. Keep that agent in module scope, not per request. Creating agents inside the handler leaks sockets over time.

// Proposal only: dedicated agent for one vendor host
import https from "node:https";

export const vendorAgent = new https.Agent({
  keepAlive: true,
  maxSockets: 8,
});

Pass { agent: vendorAgent } into the HTTP client. Fetch in Node 20 uses undici, not https.Agent. For fetch, cap concurrency inside mapLimit instead.

A reproducible concurrency probe

Save the two files under probes/fanout/. Run them with Node 20 or newer. Treat those numbers as local evidence only.

// probes/fanout/mock-vendor.mjs
import http from "node:http";

let inFlight = 0;
let peak = 0;

const server = http.createServer((req, res) => {
  inFlight += 1;
  peak = Math.max(peak, inFlight);
  setTimeout(() => {
    inFlight -= 1;
    res.writeHead(200, { "content-type": "application/json" });
    res.end(JSON.stringify({ ok: true, peak }));
  }, 80);
});

server.listen(4377, () => {
  console.log("mock vendor on 4377");
});

The mock holds every request for eighty milliseconds. Overlap then becomes visible as the peak. A sequential client should report a peak of one.

// probes/fanout/run.mjs
import assert from "node:assert/strict";

const LIMIT = Number(process.env.FANOUT_LIMIT || 4);
const N = Number(process.env.FANOUT_N || 40);
const MAX_PEAK = Number(process.env.FANOUT_MAX_PEAK || 4);

async function getBalance(id) {
  const ac = new AbortController();
  const t = setTimeout(() => ac.abort(), 2500);
  try {
    const res = await fetch(`http://127.0.0.1:4377/v1/accounts/${id}`, {
      signal: ac.signal,
    });
    if (!res.ok) throw new Error(`status ${res.status}`);
    return res.json();
  } finally {
    clearTimeout(t);
  }
}

async function mapLimit(items, limit, worker) {
  const out = new Array(items.length);
  let next = 0;
  async function run() {
    while (next < items.length) {
      const i = next++;
      out[i] = await worker(items[i], i);
    }
  }
  const n = Math.min(limit, items.length);
  await Promise.all(Array.from({ length: n }, run));
  return out;
}

const ids = Array.from({ length: N }, (_, i) => String(i + 1));
const rows = await mapLimit(ids, LIMIT, getBalance);
const peak = Math.max(...rows.map((r) => r.peak));

assert.equal(rows.length, N);
assert.ok(peak <= MAX_PEAK, `peak ${peak} exceeded ${MAX_PEAK}`);
console.log(JSON.stringify({ n: N, limit: LIMIT, peak }, null, 2));

Run the mock and the probe in two terminals.

node probes/fanout/mock-vendor.mjs
FANOUT_LIMIT=4 FANOUT_N=40 FANOUT_MAX_PEAK=4 node probes/fanout/run.mjs

Read the printed peak after a successful run. A peak of four means the cap held. A peak of forty means the helper never engaged.

Temporarily replace mapLimit with a raw Promise.all call. Keep FANOUT_MAX_PEAK at four for that run. The process should exit with an assertion error.

Record both command outputs in the PR thread. A screenshot is weaker than the JSON line. Reviewers should rerun the probe after later agent edits.

Decision table for the review note

Observation on the diff	Trust	Revert	Required test
Agent parallelizes independent GETs	Intent to overlap reads	Unbounded list	Peak in-flight cap
Vendor documents a tight route quota	Quota text as a clue	Client-side blast	429 fixture plus backoff
Handler maps a user-supplied id array	Input shape	Missing max length	Reject over cap before fetch
Tests mock fetch with instant data	Isolation	Using them as load proof	Local mock with delay
Agent adds Promise.allSettled only	Partial error objects	Still unbounded	Peak cap plus error envelope
Timeouts wrap each worker	Per-call deadline	Sharing one abort for all	Abort after 2500ms per call

The table belongs in the written review comment. It keeps later agents from reopening the same hole. It also stops reviewers from arguing from vibe.

Where a free review host fits

Agent diffs need a place that can run the probe. A laptop works, yet shared review queues still help. MonkeyCode offers free model access and a free server option for that loop.

Disclosure: This article was prepared as part of MonkeyCode's product outreach.

Free model access can apply the checklist to a new diff. It must not replace the probe or the revert list. Reviewers still paste command output before any approval.

The host does not prove vendor behavior under contract. It only proves that the client cap exists. Keep production soak tests on the real staging path.

Some teams already park review probes on a spare host. That free server option can run this fan-out check.

Limitations

The probe measures one process on one machine. Cluster-wide fan-out remains invisible to this probe. Multiple replicas can still multiply the vendor traffic.

The eighty millisecond delay is a stand-in only. Real p99 values will differ by region. Do not treat peak four as a universal default.

The helper is not a full job queue. It ignores fairness across incoming HTTP request traffic. A public API still needs admission control at the edge.

This workflow does not certify correctness of balances. It only constrains how hard the service pushes. Schema and auth reviews stay on their own checklists.

Model-written review text can miss hidden extra maps. Nested helpers may fan out after this pass. Search the diff for every Promise.all call site.

Who should skip this approach

Teams with a mesh retry budget already under test may skip it. Their gateway may already cap per-route outbound concurrency. Duplicating the probe then adds review noise.

Do not use this method as a vendor load test. Permission for that work lives with the vendor. A mock server is not their production fabric.

Do not merge because a model replied with approval. Generated approval text is not merge evidence. The probe JSON line is the evidence.

Skip the rewrite when the list is always length one. Parallelism then buys nothing and only adds code. Sequential await remains the clearer default path.

Close

Unbounded Promise.all remains a common agent shortcut. It looks like performance work in the diff. It is often a reliability defect in production.

Keep the independent-read intent when the data allows it. Revert missing caps, deadlines, and partial-failure rules together. Test peak in-flight count on a boring mock.