fix(webhook): shutdown kills in-flight processRequest work; orphans tracking comments and leaks clones

[chrisleekr]

Finding

The graceful-shutdown handler in src/app.ts:181-195 drains HTTP connections only — it does not wait for the fire-and-forget async pipeline started by processRequest(). When Kubernetes sends SIGTERM (rolling deploy, HPA scale-down, node drain):

1. isReady = false flips /readyz to 503 (src/app.ts:183) — correct.
2. server.close(cb) at src/app.ts:185 fires cb as soon as all open HTTP connections close. Per the Node.js docs, the callback waits on connection lifecycle only — it does not know about module-level promises.
3. Every active webhook handler — handleIssueComment (src/webhook/events/issue-comment.ts:35-37) and handleReviewComment (src/webhook/events/review-comment.ts:35-37) — calls processRequest(ctx).catch(...) fire-and-forget after the webhook has already returned 200 OK in milliseconds. By the time SIGTERM arrives, the HTTP request is long closed, so the close() callback fires almost instantly.
4. The callback then calls process.exit(0) (src/app.ts:187), killing the process before the 290 s force-exit timer at src/app.ts:191-194 can fire. That timer is effectively dead code: it only triggers if server.close hangs, which never happens in this topology because fire-and-forget promises are not HTTP connections.

The router already tracks the right signal: activeCount (src/webhook/router.ts:29, incremented at :161, decremented at :261 inside a finally) counts exactly the in-flight processRequest invocations. Nothing awaits it at shutdown. processRequest has 10 sequential steps — tracking-comment creation, GraphQL fetch, repo clone, Claude Agent SDK execution, finalize comment, temp-dir cleanup — any of which can be interrupted.

User-visible failure modes on every pod restart:

• Orphaned "Working..." tracking comments — createTrackingComment (src/core/tracking-comment.ts:48-66) posted the spinner, but finalizeTrackingComment never ran. The durable <!-- delivery:... --> marker (src/core/tracking-comment.ts:12-14) sits in a permanently unresolved comment.
• Leaked temp clone dirs and credential helpers — checkoutRepo's cleanup() runs in the finally block of src/webhook/router.ts:204-241, which never executes after process.exit. The startup sweep at src/app.ts:139-165 removes stale *.cred.sh files but not the clone directories.
• Silent money loss — agent turns already billed by executeAgent (src/core/executor.ts:83-96) deliver no output. No retry happens because GitHub does not automatically redeliver failed webhooks — the user must re-@mention manually.

Diagram

flowchart TD
    sigterm["SIGTERM<br/>rolling deploy / HPA / node drain"]:::signal
    ready["isReady = false<br/>readyz -> 503"]:::ok
    fire["Fire-and-forget processRequest<br/>webhook already returned 200 OK"]:::warn
    noconn["Zero open HTTP connections<br/>activeCount = N &gt; 0"]:::warn
    close["server.close callback<br/>waits ONLY on HTTP conns"]:::warn
    cbfast["callback fires within ms<br/>290s force-exit never reached"]:::bad
    exit["process.exit 0<br/>aborts in-flight processRequest"]:::bad
    orphan["Orphaned Working... comment<br/>with delivery marker"]:::bad
    leak["Leaked temp clone dir"]:::bad
    wallet["API cost billed<br/>no output delivered"]:::bad
    noretry["GitHub does NOT auto-redeliver<br/>user must re-@mention"]:::bad
    fix["MISSING: await activeCount == 0<br/>before process.exit"]:::fix

    sigterm --> ready --> close
    fire -.-> noconn
    close --> noconn --> cbfast --> exit
    exit --> orphan --> noretry
    exit --> leak
    exit --> wallet
    exit -.->|should wait for| fix

    classDef signal fill:#1e3a8a;color:#ffffff;stroke:#0c2050
    classDef ok fill:#166534;color:#ffffff;stroke:#052e16
    classDef warn fill:#92400e;color:#ffffff;stroke:#451a03
    classDef bad fill:#7f1d1d;color:#ffffff;stroke:#450a0a
    classDef fix fill:#0f766e;color:#ffffff;stroke:#042f2e

Loading

Rationale

Single-pod Claude Agent SDK runs routinely take 30-120 s (multi-turn conversation + repo clone + MCP tool calls, bounded at maxTurns: 50 per src/core/executor.ts:72). A rolling deploy on every merge to main, Kubernetes HPA scale-down on every traffic dip, and node-drain on every infra rotation all truncate an arbitrary slice of in-flight requests. Conservatively — one deploy/day, one in-flight request at deploy time, mean duration 60 s, two extra SIGTERM events/day from other churn — that is roughly 3 orphaned tracking comments per day per pod. After a week the PR timeline is polluted with "Working..." stubs that require admin GC.

The setTimeout(..., 290_000) at src/app.ts:191-194 is documented as "Force exit after terminationGracePeriodSeconds if server.close hangs", but server.close never hangs here — its callback fires as soon as HTTP connections drain (immediate). The 290 s budget the operator paid for with terminationGracePeriodSeconds is thrown away. Draining on activeCount instead of HTTP-connection count recovers that budget and delivers the graceful behaviour operators already expect.

activeCount is the correct signal: the invariant is preserved across success (src/webhook/router.ts:229-237) and failure (src/webhook/router.ts:242-262) branches via finally. No new state is required; it simply needs to be awaited. The pattern matches the Node.js production shutdown contract — "stop accepting new work → drain in-flight work → close external deps → exit".

References

Internal:

• src/app.ts:181-195 — shutdown() drains HTTP only, not background promises
• src/app.ts:185-188 — server.close callback immediately calls process.exit(0)
• src/app.ts:191-194 — 290 s force-exit timer unreachable in this topology
• src/webhook/router.ts:29 — activeCount module-level counter (never awaited at shutdown)
• src/webhook/router.ts:161,261 — increment/decrement in try/finally
• src/webhook/events/issue-comment.ts:35-37 — fire-and-forget processRequest(ctx).catch(...)
• src/webhook/events/review-comment.ts:35-37 — same pattern
• src/core/tracking-comment.ts:12-14,48-66 — delivery marker + "Working..." comment creation
• src/core/executor.ts:83-96 — agent loop bounded by internal timeout only, not shutdown
• CLAUDE.md — "Async processing: Webhook must respond within 10 seconds. All heavy work runs asynchronously after 200 OK."

External:

• Node.js http.Server.close() docs — callback waits on HTTP connections only
• GitHub Docs — Handling failed webhook deliveries — no automatic redelivery
• Node.js Graceful Shutdown in Production (SIGTERM, draining, zero-downtime deploys)
• RisingStack — Graceful shutdown with Node.js and Kubernetes
• GitHub Webhooks best practices

Suggested Next Steps

1. Export waitForDrain(timeoutMs): Promise<void> from src/webhook/router.ts that polls activeCount === 0 on a short interval (e.g. 250 ms) and resolves early or rejects on timeout. Keep the pure-function style used for cleanupStaleIdempotencyEntries so the logic is unit-testable.
2. In src/app.ts shutdown(), after isReady = false, await waitForDrain(270_000) (≈20 s headroom below the assumed terminationGracePeriodSeconds). Then call server.close() and process.exit(0). Move process.exit out of the server.close callback so drain gates exit, not HTTP connection closure.
3. Add a defensive startup sweep that finalizes stale "Working..." comments older than ~10 min (symmetric with the *.cred.sh sweep at src/app.ts:139-165). The durable marker <!-- delivery:... --> already exists; isAlreadyProcessed at src/core/tracking-comment.ts:23-40 gives the lookup primitive.
4. Add a regression test in test/webhook/router.test.ts that asserts waitForDrain resolves only after all active processRequest invocations complete (use a fake executeAgent bound to a controlled promise). A grep of test/ for shutdown|SIGTERM|server.close|drain returns zero hits today.
5. Document the new shutdown contract in CLAUDE.md under "How It Runs" so contributors know fire-and-forget work is explicitly drained, not implicitly abandoned.

Areas Evaluated

• src/webhook/router.ts — idempotency fast path, durable check ordering, activeCount lifecycle, owner allowlist gate, concurrency limit, retry wrappers, error path
• src/webhook/events/ — all 5 handlers; issue-comment.ts + review-comment.ts active, rest are placeholders
• src/webhook/authorize.ts — isOwnerAllowed ordering relative to durable idempotency
• src/app.ts — signal handlers, readiness, startup checks, stale credential sweep
• src/core/tracking-comment.ts — delivery marker, create/finalize flow
• src/core/executor.ts — agent-loop wall-clock timeout
• test/webhook/router.test.ts — confirmed zero shutdown/drain coverage
• gh issue list --label research --state all — empty; no duplicate risk

Generated by scheduled research workflow run #24312419817 on 2026-04-12

[chrisleekr]

@chrisleekr-bot-dev please triage this issue and reproduce the bug

[chrisleekr-bot-dev]

@chrisleekr-bot — I'm not sure which workflow you'd like me to run (confidence 0.00).

Reply with one of the bot:* labels (e.g. bot:triage, bot:plan, bot:implement, bot:review, bot:resolve, bot:ship) or rephrase so the ask is explicit.

Rationale: classifier fallback — response could not be parsed

[chrisleekr]

@chrisleekr-bot-dev please triage this issue and reproduce the bug

[chrisleekr-bot-dev]

bot workflow triage — failed

🛑 Invalid — chain halted (stop).

Triage: fix(webhook): shutdown kills in-flight processRequest work; orphans tracking comments and leaks clones

Verdict

INVALID (confidence: 0.92)

The issue is built on a code map that no longer matches the repository. It claims (a) the webhook event handlers fire processRequest(ctx).catch(...) fire-and-forget, (b) activeCount lives in src/webhook/router.ts:29 and counts in-flight processRequest invocations, and (c) the consequences of an interrupted webhook-server SIGTERM include orphaned Working... tracking comments and leaked temp clone dirs. None of these hold today: the active comment handlers call dispatchByIntent (not processRequest); activeCount lives in src/orchestrator/concurrency.ts:22 and counts daemon-side job offers, not webhook in-flight work; and the entire runPipeline pathway (clone, agent turns, tracking comment create/finalize) executes inside the daemon process, not the webhook server. CLAUDE.md and docs/ARCHITECTURE.md:49 both state explicitly: "The webhook server never executes the pipeline in-process." The user-visible failure modes the issue predicts cannot be caused by the webhook server's SIGTERM because the webhook server never creates a tracking comment, never clones a repo, and never invokes executeAgent. There is a separate, much smaller shutdown concern about dispatchByIntent's async work (intent classification + DB row + Valkey enqueue), but that is not the issue as filed and the recommended fix (waitForDrain on activeCount) targets the wrong counter.

What was inspected

• src/app.ts — shutdown handler, signal wiring, startup sweep. Shutdown is at lines 377-409 (issue cited 181-195 — stale). Force-exit timer at lines 405-408 (issue cited 191-194). Credential sweep at lines 285-311 (issue cited 139-165). The basic server.close() → process.exit(0) pattern the issue describes is still present, but the file:line citations no longer match.
• src/webhook/router.ts — module has no activeCount and no try/finally around heavy work. processed map for in-memory idempotency is at line 39. processRequest is at line 83 and only awaits idempotency, auth, capacity, and dispatch — it does NOT clone, create tracking comments, run the agent, or finalize comments. All heavy work is enqueued to Valkey or dispatched via WebSocket to a daemon (lines 335-393).
• src/webhook/events/issue-comment.ts:46-60 — calls dispatchByIntent, NOT processRequest.
• src/webhook/events/review-comment.ts:41-56 — calls dispatchByIntent, NOT processRequest.
• src/orchestrator/concurrency.ts:22-42 — the real activeCount lives here; documented as counting "in-flight work AT daemons (offered+accepted), not enqueued work" (line 15-16). Owner is connection-handler.ts (line 8).
• src/core/tracking-comment.ts — createTrackingComment/finalizeTrackingComment definitions; only invoked from src/core/pipeline.ts.
• src/core/pipeline.ts — runPipeline is invoked only from src/daemon/job-executor.ts:275, src/workflows/handlers/review.ts:116, src/workflows/handlers/implement.ts:93 (all daemon-side).
• src/workflows/dispatcher.ts — what the active event handlers actually call. Does intent classification, posts refusal/clarify comments, inserts a workflow_runs row, enqueues a Valkey job. Returns. No clone, no agent, no tracking comment.
• docs/ARCHITECTURE.md:46-49 — "The webhook server never runs the pipeline. Only daemons execute runPipeline."
• CLAUDE.md "How It Runs" section — same invariant: pipeline runs in the daemon, never in-process in the webhook server.
• git log recent commits — feat(workflows): add label-dispatched bot workflow foundation (#49) collapsed the comment handlers onto the dispatcher path; the issue (dated 2026-04-12) predates that.

Reproduction

Bug-class claim, but reproduction by SIGTERM-to-webhook-server is logically moot here: the cited code paths and consequences cannot occur in the current code, so no execution will demonstrate the claimed behaviour. I verified by inspection only:

1. Grep processRequest src/webhook/events/ — zero matches (handlers use dispatchByIntent).
2. Grep activeCount src/webhook/router.ts — only one hit, line 108, READING getActiveCount() from the orchestrator module for capacity gating. No module-level counter, no increment/decrement in try/finally.
3. Grep createTrackingComment — only called from src/core/pipeline.ts. runPipeline is only called from daemon-side files (src/daemon/job-executor.ts, src/workflows/handlers/*). The webhook server never creates a tracking comment, so a webhook-server SIGTERM cannot leave one orphaned.
4. The terminationGracePeriodSeconds budget complaint stands (the setTimeout(290_000) is unreachable when server.close callback fires fast), but that's a structural shutdown observation, not a "kills in-flight processRequest work" bug. Waiting on activeCount from router.ts would not fix it because router.ts has no such counter, and the orchestrator-side activeCount is daemon-side accounting that the webhook server doesn't own.

attempted=true, reproduced=false: the claimed code paths and consequences do not exist in the current code; therefore the bug as written cannot be reproduced.

Findings

• Issue cites src/webhook/events/issue-comment.ts:35-37 and src/webhook/events/review-comment.ts:35-37 calling processRequest(ctx).catch(...). Actual code at src/webhook/events/issue-comment.ts:46-60 and src/webhook/events/review-comment.ts:41-56 calls dispatchByIntent (introduced by PR feat(workflows): add label-dispatched bot workflow foundation #49, post-dating the 2026-04-12 issue).
• Issue cites src/webhook/router.ts:29 as the location of activeCount. Actual file at line 39 declares const processed = new Map<...> (idempotency cache), and there is no activeCount declaration anywhere in router.ts. The real activeCount is src/orchestrator/concurrency.ts:22.
• Issue cites src/webhook/router.ts:161,261 as increment/decrement sites in try/finally around heavy work. No such pattern exists in the current router.ts — processRequest ends at line 129; the entire file is 393 lines and contains no try/finally wrapping any local heavy execution. Capacity is mutated by connection-handler.ts on daemon offer/accept, not by the router.
• runPipeline (the function that creates the tracking comment, clones, runs the agent, finalizes) is invoked only from daemon-side code (src/daemon/job-executor.ts:275, src/workflows/handlers/review.ts:116, src/workflows/handlers/implement.ts:93). Webhook-server SIGTERM cannot orphan tracking comments because the webhook server never creates them.
• CLAUDE.md "How It Runs" and docs/ARCHITECTURE.md:46-49 both record the invariant: "The webhook server never executes the pipeline in-process."
• Cleanup of clone dirs and *.cred.sh lives in the daemon's runPipeline finally blocks; the startup sweep at src/app.ts:285-311 (cited as 139-165) only sweeps stale .cred.sh files. Leaks the issue describes would have to come from daemon SIGKILL, not webhook SIGTERM.
• The structural observation that setTimeout(290_000) is unreachable when server.close callback fires immediately is technically accurate, but the proposed fix — awaiting activeCount === 0 from router.ts — targets a counter that does not exist in router.ts and tracks something different than the issue assumes.
• A real, distinct shutdown concern exists for dispatchByIntent fire-and-forget work in the webhook server (intent classifier LLM call, DB insert, Valkey enqueue). If interrupted mid-flight, you could see partial states (e.g. workflow_run row inserted but Valkey job never enqueued). But: it's not what the issue describes, the consequences are different, the fix is different, and markFailed/Valkey reconciliation already covers part of it.

Reasoning

A bug report that names file:line locations is making a claim those locations exist and behave as described. Three of the issue's load-bearing citations — src/webhook/events/issue-comment.ts:35-37, src/webhook/events/review-comment.ts:35-37, src/webhook/router.ts:29,161,261 — do not match the current code. The architectural premise — that webhook-server SIGTERM kills the heavy pipeline (clone + agent + finalize) — contradicts the documented invariant in two places (CLAUDE.md, docs/ARCHITECTURE.md:49) that the pipeline runs only in the daemon. The user-visible failure modes (orphaned Working... comments, leaked clone dirs, billed-but-undelivered agent turns) cannot be caused by webhook-server SIGTERM because the webhook server does not own any of those resources.

This looks like a research-bot finding generated against a stale snapshot of the codebase (the issue is dated 2026-04-12; PR #49 collapsed the comment handlers onto the workflow dispatcher after that). Acting on it as written would (a) implement waitForDrain against the wrong counter and (b) not actually fix the symptoms described, because those symptoms are the daemon's problem, not the webhook server's. A separate, narrower issue about webhook-server dispatchByIntent shutdown drain might be worth filing — but that's a different bug, with a different cited counter and different consequences, and it should be researched on the current code, not retrofitted onto this issue.

Recommended next step

stop — close as out-of-date / based on stale architecture; if a webhook-server-side dispatch drain concern is real, it should be filed fresh against current code with current line citations.

cost: $1.1174 · turns: 15 · duration: 202s