Review Agent PRs That Hold Row Locks Across Fetch

A checkout worker locked inventory rows during a Tuesday deploy. The agent pull request wrapped charge and email calls inside one transaction. Unit tests stayed green because payment mocks returned at once.

This review pattern appears in many agent-generated diffs. The patch reads like careful consistency for stock and payment. Production then waits on payment, mail, or a webhook.

The failure class

Agents reach for transactions when they notice races. They open BEGIN, then call fetch, then COMMIT. Row locks stay held for the whole network wait.

Other requests pile up on the same SKU row. Timeouts present as database load in dashboards. The real cause still sits in the pull request diff.

Agents also wrap SDK retries inside the same BEGIN block. Each retry extends the lock by another round trip. A flaky payment provider then serializes checkout for one SKU.

What the Tuesday PR contained

The next snippet is an unexecuted example. It mirrors a common agent-generated checkout patch.

// unexecuted example: agent-generated checkout handler
async function checkout(req, db) {
  const { sku, userId, paymentMethod } = req.body;
  await db.query("BEGIN");
  try {
    await db.query(
      "SELECT qty FROM inventory WHERE sku = $1 FOR UPDATE",
      [sku]
    );
    const charge = await fetch("https://payments.example/charge", {
      method: "POST",
      headers: { "content-type": "application/json" },
      body: JSON.stringify({ userId, sku, paymentMethod }),
    });
    if (!charge.ok) throw new Error("charge failed");
    await db.query(
      "UPDATE inventory SET qty = qty - 1 WHERE sku = $1",
      [sku]
    );
    await fetch("https://notify.example/email", {
      method: "POST",
      body: JSON.stringify({ userId, sku }),
    });
    await db.query("COMMIT");
    return { ok: true };
  } catch (err) {
    await db.query("ROLLBACK");
    throw err;
  }
}

The SELECT FOR UPDATE looks responsible in review. The two fetch calls do not belong inside that transaction. Charge latency now owns the inventory lock.

What to trust, revert, and test

Reviewers can split the diff with a hard rule. Database reads and writes may stay in the transaction. Network, disk, and queue I/O must leave it.

1. Trust the local invariant

Trust a short transaction that touches one aggregate. Trust SELECT FOR UPDATE followed only by local SQL. Trust COMMIT that happens before any fetch await.

2. Revert the outbound calls

Revert fetch, SDK, and mailer calls from the transaction body. Revert retries placed between BEGIN and COMMIT. Revert awaited remote logging inside the same block.

3. Test lock duration, not mock counts

Tests that stub fetch in one millisecond miss this class. A harness must keep the transaction open. A second client must probe the same row.

Review procedure

Follow these five steps on every agent PR that touches SQL.

1. Collect the transaction span from the diff hunks. Note BEGIN, COMMIT, and ORM transaction callbacks.
2. Mark every await that is not a database call. Include fetch, queues, object storage, and email SDKs.
3. Revert those awaits to after COMMIT or to a worker. Keep only local writes in the critical section.
4. Add a two-client lock test on the same key. Fail the build when payment I/O blocks the second session.
5. Require an outbox row or explicit compensation path. Do not return charged before the worker finishes.

Reviewers should cite file and line for each mark. Review comments without line numbers get ignored later.

Commands that surface the span

Start with the merge diff, not the whole tree. Network tokens beside BEGIN are the first signal.

git diff origin/main...HEAD -- '*.js' '*.ts' \
  | rg -n "BEGIN|startTransaction|\.transaction\(|fetch\(|COMMIT|commit\("

On a staging Postgres, inspect ungranted locks during a dry run. This command is an unexecuted example for a disposable database.

psql "$DATABASE_URL" -c "\
SELECT pid, mode, granted, relation::regclass \
FROM pg_locks \
WHERE NOT granted;"

A blocked pid during a mocked payment is enough evidence. Merge should wait until that wait disappears.

A reproducible scan artifact

The Node script below is a review aid. It scans a source file for BEGIN plus network calls. Treat it as a heuristic, not a full ORM parser.

// review-tx-fetch.js — run against a patch or source file
const fs = require("fs");

const NETWORK = /\b(fetch|axios|got|stripe|twilio|s3|sns|sqs)\b/i;
const BEGIN = /\b(BEGIN|startTransaction|transaction\()/i;
const COMMIT = /\b(COMMIT|commit\()/i;

function windows(src) {
  const lines = src.split(/\n/);
  const hits = [];
  let open = null;
  for (let i = 0; i < lines.length; i++) {
    if (BEGIN.test(lines[i])) open = { start: i + 1, net: [] };
    if (open && NETWORK.test(lines[i])) open.net.push(i + 1);
    if (open && COMMIT.test(lines[i])) {
      if (open.net.length) hits.push({ ...open, end: i + 1 });
      open = null;
    }
  }
  return hits;
}

const file = process.argv[2];
const src = fs.readFileSync(file, "utf8");
const hits = windows(src);
if (!hits.length) {
  console.log("no begin/fetch overlap found");
  process.exit(0);
}
for (const h of hits) {
  console.log(
    `transaction ${h.start}-${h.end} calls network at ${h.net.join(",")}`
  );
}
process.exit(1);

Run it on the checkout file before approval.

node review-tx-fetch.js checkout.js

A non-zero exit from the scanner should block merge. Humans still confirm wrappers the regex cannot see.

A lock-duration test plan

The next test is a labeled proposal for local Postgres. It uses two clients and a deferred stand-in for fetch.

// proposal: lock-duration.test.js
const { Pool } = require("pg");

const slowFetch = () =>
  new Promise((resolve) => setTimeout(() => resolve({ ok: true }), 1500));

test("second buyer is not blocked by payment I/O", async () => {
  const pool = new Pool({ connectionString: process.env.DATABASE_URL });
  const a = await pool.connect();
  const b = await pool.connect();
  await a.query("BEGIN");
  await a.query("SELECT qty FROM inventory WHERE sku = $1 FOR UPDATE", [
    "sku-1",
  ]);
  const blocked = b.query(
    "SELECT qty FROM inventory WHERE sku = $1 FOR UPDATE NOWAIT",
    ["sku-1"]
  );
  await slowFetch();
  await a.query("COMMIT");
  await expect(blocked).rejects.toMatchObject({ code: "55P03" });
  a.release();
  b.release();
  await pool.end();
});

The proposed test documents the failure first. After the fix, client B must not hit lock_not_available during payment I/O. Mock timers alone are not evidence of lock release.

The safer handler shape

Move side effects until after COMMIT. Persist an outbox row in the same transaction. A worker then sends payment and email without holding row locks.

// unexecuted example: local transaction, later side effects
async function checkout(req, db, queue) {
  const { sku, userId, paymentMethod } = req.body;
  const orderId = crypto.randomUUID();
  await db.query("BEGIN");
  try {
    const { rows } = await db.query(
      "SELECT qty FROM inventory WHERE sku = $1 FOR UPDATE",
      [sku]
    );
    if (!rows[0] || rows[0].qty < 1) throw new Error("sold out");
    await db.query("UPDATE inventory SET qty = qty - 1 WHERE sku = $1", [
      sku,
    ]);
    await db.query(
      "INSERT INTO outbox (id, kind, payload) VALUES ($1, $2, $3)",
      [
        orderId,
        "charge_and_email",
        JSON.stringify({ sku, userId, paymentMethod }),
      ]
    );
    await db.query("COMMIT");
  } catch (err) {
    await db.query("ROLLBACK");
    throw err;
  }
  await queue.publish("outbox", { orderId });
  return { accepted: true, orderId };
}

The handler now returns accepted, not charged. Payment failures retry without holding inventory locks during HTTP. Compensation still needs an explicit, tested design.

Decision table for the review thread

Paste this table into the agent PR. Each row needs a file and line citation.

Diff signal	Trust	Revert	Test
SELECT FOR UPDATE then SQL only	yes	no	qty invariant
fetch or SDK between BEGIN and COMMIT	no	yes	second session NOWAIT
retry wrapper around charge inside tx	no	yes	lock wait on pg_locks
outbox insert inside tx	yes	no	row visible after COMMIT
email SDK after COMMIT	yes	no	at-least-once worker

Reviewers should reject a green CI that never opened a second session. Mock call counts do not measure lock duration.

Running the scanner on a spare host

Teams can run the regex scanner on any laptop. A spare Linux box helps when the repo already expects remote CI.

Disclosure: This article was prepared as part of MonkeyCode's product outreach. MonkeyCode is an open-source project with operator-stated free model access and a free server option. The scanner and lock test can run on that free server when local Postgres is awkward. The method above stays useful without that host.

Limitations

The regex misses Knex, Prisma, and TypeORM callback transactions. It also misses SQL assembled at runtime. A green scan is not proof of safety.

The lock test needs real row locking behavior. SQLite and some pooled setups will not reproduce NOWAIT. Do not treat fake clocks as lock evidence.

Outbox designs create at-least-once side effects. Double charge is a different incident class. Idempotency keys still belong on the payment worker.

Who should not use this approach

Do not apply the revert rule to single-writer scripts. Do not force outbox infrastructure onto a one-user prototype. Do not enable the scanner as a linter without ORM review.

Skip any remote server path for production database dumps. Review data must stay synthetic and disposable.

After merge

Agent PRs often hide latency inside BEGIN. The durable fix is a shorter transaction and a later worker. Cite the network lines, run the two-client test, then merge.