My GitHub App takes a pull request, asks an LLM to review it, and posts the comments back. The first time I ran it end to end against a real repository, the review showed up on the PR exactly as intended. Then I opened GitHub's webhook delivery log and found the delivery marked as failed.
Both things were true at once: the feature worked, and the delivery had failed. That contradiction is the interesting part, and the fix I reached for first created a worse problem than the one it solved. This post walks through both, with the code, because the second problem is the kind that doesn't announce itself.
The stack is Next.js (App Router) on Vercel, Drizzle and Postgres, but nothing below depends on those. If you run slow work behind a webhook on a serverless platform, you will meet the same shape.
The symptom: a successful review and a failed delivery
The handler was the obvious one. Verify the signature, parse the payload, do the work, return 200:
case "pull_request":
await handlePullRequestEvent(payload);
break;
handlePullRequestEvent fetches the changed files, calls the model, and posts a review. The model call alone regularly takes longer than ten seconds.
GitHub's webhook best-practices page says a delivery should respond within 10 seconds, and in my notes from this incident GitHub does not automatically redeliver a failed one. So the sequence was:
- GitHub sends the
pull_requestevent. - My handler is still waiting on the LLM when GitHub gives up at about ten seconds and records the delivery as failed.
- My function keeps running anyway, because the serverless runtime doesn't know the caller hung up. It finishes, posts the review, and writes the result to the database.
So the user-visible outcome was fine, and the only evidence of a problem was a red mark in a log that nobody looks at until something else goes wrong. That is a bad state to leave a system in even when the feature technically works: the next time a delivery fails for a real reason, it will look exactly like this one.
The cause is simple. I was doing work whose duration I don't control inside the request/response cycle of a caller that gives me a fixed budget.
The obvious fix
Acknowledge immediately, do the work afterwards. On Vercel the primitive for "keep running after the response is sent" is waitUntil from @vercel/functions:
import { waitUntil } from "@vercel/functions";
// ...
case "pull_request":
// Generating the review often takes longer than 10 seconds, which is longer
// than GitHub's delivery timeout (about 10s, one attempt). ACK immediately
// and keep working in the background.
waitUntil(
handlePullRequestEvent(payload).catch((error) => {
console.error("PR review background processing failed:", { deliveryId }, error);
})
);
break;
Everything before this point is cheap and synchronous: verify the HMAC signature, parse the JSON. Then the response goes out, and the review is generated in the background.
It type-checked and the change was small, so I was pleased with it. Before shipping, I ran a security review pass over the diff, and it pointed at the exact line I was pleased with. After I addressed that finding (below), I deployed, re-ran the end-to-end test, and the delivery log showed green.
Why this is worse than it looks
Before the change, a failure had a natural place to show up. If the handler threw, GitHub got a 500, and the delivery was red. The work and the acknowledgement were the same event, so a problem with one was a problem with the other.
After the change, the acknowledgement says only "I received your payload and the signature was valid." It says nothing about whether the work will finish. The failure modes of the background job are now invisible to the caller by construction:
- The function hits its platform time limit and is killed.
- A deploy replaces the running instance.
- The process runs out of memory.
- An upstream call hangs and never returns.
None of those run your .catch. A killed process doesn't get to log anything or update any row. The delivery log stays green, because the delivery succeeded. What's left is whatever state you wrote before you started the work.
In my case that state was a row in the reviews table. I create it with status = 'pending' before returning, and the background job moves it to completed or failed at the end. If the job dies in the middle, the row stays pending for good.
That sounds like a cosmetic problem, a spinner that never stops. It isn't, for a reason specific to how I count usage. A pending row counts toward the user's monthly quota, deliberately:
// Rows that count toward the quota:
// pending (in progress) counts too. If only completed rows were counted,
// concurrent webhooks that arrive before an earlier review finishes would all
// be judged to be within the limit, making the free tier effectively unlimited.
// failed rows are excluded in principle, but a row that failed after the
// Claude API was called (tokens_used > 0) still counts...
function countsTowardQuota() {
return or(ne(reviews.status, "failed"), gt(reviews.tokensUsed, 0));
}
I count pending rows because otherwise a burst of simultaneous events all look like they fit under the limit. That's correct, and it means a row that is stuck as pending is not just an ugly record. It permanently consumes someone's allowance. A killed background job turns into a user being told they've used reviews they never received.
So the second lesson, after "don't hold the caller hostage," is that moving work out of the request cycle doesn't remove the failure modes. It moves them to a place where nothing is watching.
Layer 1: make the work end before the platform ends it
The first thing to do about a job that can be killed silently is to make sure it gets a chance to fail loudly instead. That means giving it an explicit time budget that is shorter than the platform's.
Without an explicit limit, the route depends on the platform default, and my comment in the code says what I found out about that:
// Give the background processing (PR file fetch + Claude API review generation +
// GitHub post) enough time. Leaving this unset depends implicitly on the Vercel
// default and fails silently when interrupted (reviews stay "pending" and become
// undetectable).
export const maxDuration = 120;
And the model client gets a timeout that is strictly below it:
// Keep this shorter than maxDuration (the webhook background limit) so a hung
// API call surfaces as an error before the platform timeout (silent-fail guard).
return new Anthropic({ apiKey, timeout: 90_000 });
The ordering matters more than the specific numbers. If the client timeout is longer than the function limit, the platform wins the race, kills the process, and nothing runs catch. If the client timeout is shorter, the SDK throws, my catch block runs, and the row is marked failed with an error message. A timeout I control turns into a status I can query. A timeout the platform controls turns into silence.
There is one caveat I want to be upfront about, because it's the kind of thing I'd want to know if I were copying this. A 90-second timeout on the client is a per-request limit. As far as I know, the Anthropic SDK retries failed requests by default, a couple of times. I have not set that option in this codebase and I have not tested what happens when a request times out and is retried. If retries do apply to timeouts, then the total time spent could be several multiples of 90 seconds, which is well beyond the function limit and would put the platform back in charge. I'm flagging that as something I still need to check rather than as a finding. The general point holds either way: when you stack a timeout under a deadline, budget the total across all attempts, not the single attempt.
Layer 2: assume the first layer will sometimes fail anyway
Timeouts catch the hung call. They don't catch a deploy that replaces the instance mid-flight, or an out-of-memory kill, because in those cases no code of mine gets to run at all.
So the cleanup can't live in the dying process. It has to be done by something else, later. I didn't want to add a scheduler and a new moving part for a problem this small, so the reaper runs lazily: the next time the same user's webhook arrives, it sweeps their stale rows before counting anything.
// Turn stuck pending rows (function cut off, OOM, replaced by a deploy, never
// reaching completed/failed) into failed. Rows that died before the Claude call
// (tokens_used=0) drop out of the quota; rows that died after it (the webhook
// writes a sentinel tokens_used>=1) stay in the quota even once failed.
// The threshold is 10 minutes, comfortably longer than maxDuration (120s).
const STALE_PENDING_MINUTES = 10;
async function sweepStalePending(
tx: Pick<typeof db, "update">,
userId: string
): Promise<void> {
const cutoff = new Date(Date.now() - STALE_PENDING_MINUTES * 60_000);
await tx
.update(reviews)
.set({
status: "failed",
errorMessage: `stale pending swept (older than ${STALE_PENDING_MINUTES}m)`,
completedAt: new Date(),
})
.where(
and(eq(reviews.userId, userId), eq(reviews.status, "pending"), lt(reviews.createdAt, cutoff))
);
}
Three decisions in there are worth spelling out.
The threshold is derived from the limit. Ten minutes is not a round number I liked. It has to be comfortably larger than maxDuration, otherwise the sweeper could mark a healthy, still-running job as failed while it is still working. The sweep is only safe because "older than ten minutes" means "the platform has already killed it", and that holds only as long as the threshold stays above the function limit.
It runs inside the same transaction and the same lock as the check it protects. The sweep is called from the function that reserves a review slot, after taking a per-user advisory lock:
await tx.execute(sql`select pg_advisory_xact_lock(${LOCK_NS_USER}, hashtext(${userId}))`);
// Reap stale pending rows under the lock, then count (serialized against
// concurrent runs for the same user)
await sweepStalePending(tx, userId);
The point of doing the sweep under the lock is that the count which decides "is this user over the limit" is computed on cleaned-up data, and no other request for that user can interleave between the cleanup and the count.
Whether a swept row still costs the user depends on how far it got. This is the part I like least to have to think about, and the part I'm happiest I did think about. A job that died before it called the model has cost nothing, so its row should fall out of the quota. A job that died after the model call started has cost real money, and if it were free it would be an exploit: anyone who could make the post-generation step fail on purpose would get unlimited generations. So the webhook writes a sentinel immediately before calling the model:
// Write a sentinel (tokens_used=1) just before the Claude call. If the call
// times out or the function is cut off before completing, the row still counts
// toward the quota as "generation started, cost may have been incurred". With
// tokens_used=0, a failed/swept row would free the quota and let someone abort
// generations to burn Claude cost for free.
await db.update(reviews).set({ tokensUsed: 1 }).where(eq(reviews.id, reviewRecord.id));
tokensUsed = 1;
The sweeper then doesn't need to know anything about what the job was doing. It just flips pending to failed, and the previously written tokens_used decides whether that row still counts. The state machine carries the accounting.
The reaper created a new problem: a retry budget it can exhaust
Once stuck rows get turned into failed, something else has to decide whether that commit may be reviewed again. A webhook can be redelivered, and a user can close and reopen a PR, so the slot reservation does two checks before it allows another run for the same (repository, PR, commit):
// 1-a: already delivered (completed / pending) -> reject immediately
const delivered = await tx
.select({ count: count() })
.from(reviews)
.where(and(sameCommit, ne(reviews.status, "failed")));
if ((delivered[0]?.count ?? 0) > 0) {
return { allowed: false, plan, used: 0, limit, record: null, reason: "duplicate" as const };
}
Note that pending counts as "delivered" here. That is what stops a redelivered webhook from running the same review twice while the first is still going. It also means that a stuck pending row blocks the commit entirely until the sweep clears it, which is one more reason the sweep has to exist.
The second check caps total attempts, because without a cap a close/reopen loop could trigger paid calls forever:
const attemptsSince = new Date(Date.now() - ATTEMPT_WINDOW_MS);
const attempts = await tx
.select({ count: count() })
.from(reviews)
.where(and(sameCommit, notQuotaNotice(), gte(reviews.createdAt, attemptsSince)));
if ((attempts[0]?.count ?? 0) >= MAX_ATTEMPTS_PER_COMMIT) {
return { allowed: false, plan, used: 0, limit, record: null, reason: "attempt-cap" as const };
}
The gte(reviews.createdAt, attemptsSince) window is the interesting line, and it exists because of the reaper. A review caught by the sweep is a failed row, and failed rows count as attempts. My own comment in the code explains the consequence of leaving the window out: the "zero-cost abort" rows the sweeper creates would pile up, and after a few infrastructure accidents of my own making, such as deploys replacing a running instance, that commit could never be reviewed again. The user would have done nothing wrong, and my cleanup logic would have permanently blocked them.
I only found that by thinking about the reaper as a producer of data, not just a consumer. Every row it writes feeds the other rules in the system. If you add a janitor, trace what its output does to every check that reads the same table.
What this still doesn't cover
I'd rather list the gaps than have you find them.
- The sweep is lazy. It runs when that user's next webhook arrives. If they never send another, their stuck row stays pending. The place that displays the monthly count is read-only, so a stuck row keeps showing as used until the next event cleans it up. I decided that was acceptable for the volumes involved, and I wrote the decision down in a comment, but it is a real gap, and a scheduled sweep would close it.
- A swept review is simply gone. The user's PR doesn't get a review, and nothing retries it. A new push produces a new event with a new commit, which is a fresh attempt. I left it that way because automatic retries on a billable operation are their own can of worms.
- The time budget is reasoning, not measurement. The 120, the 90 and the 10 come from how I reason about the pieces interacting. I haven't load-tested a run where several of those limits are approached together.
The general lesson
If you only take one thing away: acknowledging a webhook early doesn't make the work reliable, it makes the work unobserved. Before the change, a slow job turned the delivery red, which was annoying but honest. After it, the delivery is always green, and the only truth left is the state you wrote to your own database. So that state has to be designed:
- Write a durable record before you acknowledge. The row exists before the response goes out, so there is something to inspect if the job vanishes.
- Give the work a deadline shorter than the platform's, and order the timeouts. Make your own timeout fire first, so failure arrives as an exception you can handle rather than a kill you can't.
-
Put the cleanup outside the process that might die. A reaper that runs from somewhere else, later, is the only thing that works when
catchdoesn't. - Derive the reaper's threshold from the limit, and decide in advance what a reaped row costs. If "failed" means "free," someone will find out how to fail on purpose.
The webhook stayed fast, the delivery log stayed green, and the failure mode that used to be loud became a row with an error message and a timestamp. That is a better place for it to live. But I only got there after a review pass pointed at the one line I thought I'd finished with, which is a good reminder that "the red thing turned green" is not the same as "the problem is solved."
Top comments (0)