From b25f2badd459a76ebac0f8ea17374e03aa5ec78f Mon Sep 17 00:00:00 2001 From: Chris Lee Date: Wed, 3 Jun 2026 20:39:22 +1000 Subject: [PATCH] refactor(idempotency): remove dead in-memory Map and isAlreadyProcessed from router Co-Authored-By: Claude Opus 4.8 --- CLAUDE.md | 4 +- docs/build/architecture.md | 4 +- docs/use/invoking.md | 8 +- docs/use/safety.md | 2 +- src/app.ts | 2 +- src/core/tracking-comment.ts | 51 +++--------- src/mcp/servers/comment.ts | 6 +- src/webhook/idempotency.ts | 8 +- src/webhook/router.ts | 45 +++------- test/core/tracking-comment.test.ts | 129 ----------------------------- test/webhook/router.test.ts | 74 ++++++----------- test/workflows/dispatcher.test.ts | 4 + 12 files changed, 73 insertions(+), 264 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 112f1c2b..06732276 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -32,7 +32,7 @@ Single HTTP server (`src/app.ts`) using `octokit` App class. Webhook events arri **Event handler** (`src/webhook/events/`): parse event → unified `BotContext` → check for `@chrisleekr-bot` trigger → fire-and-forget `processRequest()` -**Router** (`src/webhook/router.ts`): idempotency (in-memory `Map` + durable tracking-comment check), owner allowlist, concurrency guard, triage, and scale-up decision. On heavy/overflow it spawns an ephemeral daemon K8s Pod (`src/k8s/ephemeral-daemon-spawner.ts`). The job is then enqueued for any daemon in the fleet to claim over WebSocket. The webhook server never executes the pipeline in-process. +**Router** (`src/webhook/router.ts`): owner allowlist, concurrency guard, triage, and scale-up decision (the `processRequest` entry point is dev-test-only; production handlers in `events/` own dispatch + idempotency). On heavy/overflow it spawns an ephemeral daemon K8s Pod (`src/k8s/ephemeral-daemon-spawner.ts`). The job is then enqueued for any daemon in the fleet to claim over WebSocket. The webhook server never executes the pipeline in-process. **Pipeline** (`src/core/pipeline.ts`, executed by the daemon): @@ -62,7 +62,7 @@ Single HTTP server (`src/app.ts`) using `octokit` App class. Webhook events arri ## Key Concepts - **Async processing**: Webhook must respond within 10 seconds. All heavy work runs asynchronously after 200 OK. -- **Idempotency**: GitHub webhooks are at-least-once, a delivery (auto-retry or operator redelivery) replays with the SAME `X-GitHub-Delivery` header for up to 3 days. The four side-effecting event handlers (`events/issue-comment.ts`, `events/review-comment.ts`, and the label branches of `events/issues.ts` + `events/pull-request.ts`) call `claimDelivery(deliveryId)` (`src/webhook/idempotency.ts`) at the very top of their dispatch path, before any LLM call, `workflow_runs` insert, or GitHub write. `claimDelivery` is a Valkey `SET key 1 NX EX 259200` claim: it returns `true` exactly once per `deliveryId` within the 3-day window (the redelivery gets `false` and the handler returns early). It is **fail-OPEN**, when Valkey is unconfigured or errors it returns `true`, degrading to at-least-once rather than dropping webhooks. `events/review.ts` is intentionally NOT gated: it fires only an idempotent reactor wake (no dispatch/write). Durable backstop behind the best-effort Valkey layer: the `idx_workflow_runs_inflight` partial-unique index, which makes the dispatcher reject a second in-flight run for the same workflow+target even if the Valkey claim was skipped (fail-open). `claimDelivery` also fails open when Valkey is configured-but-disconnected (gated on `isValkeyHealthy()` so a down connection skips the SET rather than blocking on Bun's offline queue). (The legacy in-memory `Map` + `isAlreadyProcessed()` tracking-comment scan lives on the `router.ts` `processRequest` path, which the production handlers bypass, issue #202.) +- **Idempotency**: GitHub webhooks are at-least-once, a delivery (auto-retry or operator redelivery) replays with the SAME `X-GitHub-Delivery` header for up to 3 days. The four side-effecting event handlers (`events/issue-comment.ts`, `events/review-comment.ts`, and the label branches of `events/issues.ts` + `events/pull-request.ts`) call `claimDelivery(deliveryId)` (`src/webhook/idempotency.ts`) at the very top of their dispatch path, before any LLM call, `workflow_runs` insert, or GitHub write. `claimDelivery` is a Valkey `SET key 1 NX EX 259200` claim: it returns `true` exactly once per `deliveryId` within the 3-day window (the redelivery gets `false` and the handler returns early). It is **fail-OPEN**, when Valkey is unconfigured or errors it returns `true`, degrading to at-least-once rather than dropping webhooks. `events/review.ts` is intentionally NOT gated: it fires only an idempotent reactor wake (no dispatch/write). Durable backstop behind the best-effort Valkey layer: the `idx_workflow_runs_inflight` partial-unique index, which makes the dispatcher reject a second in-flight run for the same workflow+target even if the Valkey claim was skipped (fail-open). `claimDelivery` also fails open when Valkey is configured-but-disconnected (gated on `isValkeyHealthy()` so a down connection skips the SET rather than blocking on Bun's offline queue). (The legacy in-memory `Map` + `isAlreadyProcessed()` tracking-comment scan was retired in issue #211; it only ever guarded the dev-test-only `router.ts` `processRequest` path, which production handlers bypass.) - **Repo checkout**: Each request clones the repo to a unique temp dir. Claude operates on local files via `cwd`. - **MCP servers**: Comment updates, inline reviews, and Context7 for library docs. Git changes are made via git CLI (Bash tool) on the cloned repo. - **Scheduled actions**: a repo may ship a `.github-app.yaml` at its default-branch root declaring prompt-based actions on a cron schedule. The internal scheduler (`src/scheduler/`, gated by `SCHEDULER_ENABLED` + `DATABASE_URL` + non-empty `ALLOWED_OWNERS`) enqueues a `scheduled-action` job, a new job kind on the scoped-job rail, that the daemon runs as one agent session via `src/daemon/scheduled-action-executor.ts`. Missed cron slots are skipped, not backfilled. The prompt is owner-trusted config. Cron parsing uses the `cron-parser` dependency. The bot-provided `merge_readiness` MCP tool is exposed only when `SCHEDULER_ALLOW_AUTO_MERGE` env AND per-action `auto_merge` are both true; `allowed_tools` is owner-trusted config, so an action granted a merge-capable Bash tool can still merge regardless. `resolve.ts` FR-017 is untouched. diff --git a/docs/build/architecture.md b/docs/build/architecture.md index 670d117a..7612ac28 100644 --- a/docs/build/architecture.md +++ b/docs/build/architecture.md @@ -43,8 +43,8 @@ flowchart TD ## Key concepts -- **Async processing.** The webhook handler responds within ten seconds, so the router fires `processRequest` with fire-and-forget semantics after the 200 OK is queued. Every box downstream of `ACK` runs after the HTTP response is on the wire. -- **Webhook delivery idempotency (issue #202).** GitHub is at-least-once: a delivery (auto-retry or operator redelivery) replays with the same `X-GitHub-Delivery` for up to 3 days. The four side-effecting handlers (`events/issue-comment.ts`, `events/review-comment.ts`, the label branches of `events/issues.ts` + `events/pull-request.ts`) call `claimDelivery(deliveryId)` (`src/webhook/idempotency.ts`) at the top of their dispatch path, before any LLM call, `workflow_runs` insert, or GitHub write. It is a Valkey `SET key 1 NX EX 259200` claim: `true` exactly once per delivery, `false` (and an early return) on a redelivery. It is **fail-open**, when Valkey is unconfigured or disconnected (gated on `isValkeyHealthy()`) it returns `true`, degrading to at-least-once rather than dropping or blocking webhooks. `events/review.ts` is exempt (idempotent reactor wake only). The durable backstop behind the best-effort Valkey layer is the `idx_workflow_runs_inflight` partial-unique index: the dispatcher rejects a second in-flight run for the same workflow+target even when the Valkey claim was skipped. The legacy in-memory `Map` + `isAlreadyProcessed` tracking-comment scan (`src/core/tracking-comment.ts`) now runs only on the `router.ts processRequest` path, which production handlers bypass. `DATABASE_URL` is required to persist execution / dispatch history and the in-flight guard across restarts. +- **Async processing.** The webhook handler responds within ten seconds, so the side-effecting `events/*` handlers fire their dispatch with fire-and-forget semantics after the 200 OK is queued. Every box downstream of `ACK` runs after the HTTP response is on the wire. (`router.ts processRequest` is the equivalent path for the dev-only `/api/test/webhook` endpoint, not production.) +- **Webhook delivery idempotency (issue #202).** GitHub is at-least-once: a delivery (auto-retry or operator redelivery) replays with the same `X-GitHub-Delivery` for up to 3 days. The four side-effecting handlers (`events/issue-comment.ts`, `events/review-comment.ts`, the label branches of `events/issues.ts` + `events/pull-request.ts`) call `claimDelivery(deliveryId)` (`src/webhook/idempotency.ts`) at the top of their dispatch path, before any LLM call, `workflow_runs` insert, or GitHub write. It is a Valkey `SET key 1 NX EX 259200` claim: `true` exactly once per delivery, `false` (and an early return) on a redelivery. It is **fail-open**, when Valkey is unconfigured or disconnected (gated on `isValkeyHealthy()`) it returns `true`, degrading to at-least-once rather than dropping or blocking webhooks. `events/review.ts` is exempt (idempotent reactor wake only). The durable backstop behind the best-effort Valkey layer is the `idx_workflow_runs_inflight` partial-unique index: the dispatcher rejects a second in-flight run for the same workflow+target even when the Valkey claim was skipped. The legacy in-memory `Map` + `isAlreadyProcessed` tracking-comment scan was retired in issue #211 (it only ever guarded the dev-test-only `router.ts processRequest` path, which production handlers bypass). `DATABASE_URL` is required to persist execution / dispatch history and the in-flight guard across restarts. - **One request, one clone.** Each delivery clones the repo into a unique temp directory under `CLONE_BASE_DIR` **on the daemon host**. Claude operates on local files via `cwd`. On PR events the checkout supplementally fetches `origin/` (when it differs from the head ref) so the agent's `git diff origin/...HEAD` and `git rebase origin/` directives resolve first try. A sibling `${workDir}-artifacts` directory is created outside the checkout and exposed to the agent as `BOT_ARTIFACT_DIR`: workflow summary files (IMPLEMENT.md / REVIEW.md / RESOLVE.md) are written there so they can never be picked up by a `git add` inside the clone. Both directories are removed in the pipeline's `finally` block regardless of outcome. - **GitHub credential resolution.** `src/core/github-token.ts:resolveGithubToken()` is the single source of the GitHub credential the daemon uses. Default is an App installation token minted just-in-time from the cached `App` singleton in `src/orchestrator/connection-handler.ts`. When `GITHUB_PERSONAL_ACCESS_TOKEN` is set, the helper short-circuits and returns the PAT instead, API/git authentication runs as the PAT owner. Commit author/committer metadata is **not** affected; `src/core/checkout.ts` hard-pins git `user.name`/`user.email` to `chrisleekr-bot[bot]` so commit objects still carry the bot identity. The git credential helper, executor `GH_TOKEN`/`GITHUB_TOKEN` env vars, and MCP server env all consume the resolved string without caring about its source. - **The webhook server never runs the pipeline.** Only daemons execute `runPipeline`. The webhook server is the orchestrator: it enqueues jobs and optionally spawns ephemeral daemons. diff --git a/docs/use/invoking.md b/docs/use/invoking.md index d01c2615..70607755 100644 --- a/docs/use/invoking.md +++ b/docs/use/invoking.md @@ -51,12 +51,12 @@ flowchart TD ## Idempotency -A duplicate webhook delivery never spawns a duplicate job. The router checks two layers: +A duplicate webhook delivery never spawns a duplicate job (issue #202). Two layers: -1. **Fast in-memory**: a `Map` keyed by the `X-GitHub-Delivery` header, swept every 60 minutes (`src/webhook/router.ts`). Lost on restart. -2. **Durable**, `isAlreadyProcessed` looks for the hidden delivery marker that the bot embeds in its tracking comment. Survives pod restarts, OOM kills, and crash loops; works without `DATABASE_URL`. +1. **Best-effort claim**: the side-effecting event handlers call `claimDelivery(deliveryId)` (`src/webhook/idempotency.ts`), a Valkey `SET key 1 NX EX 259200` that returns `true` exactly once per `X-GitHub-Delivery` within GitHub's 3-day redelivery window; a redelivery gets `false` and returns early. Fail-open: a Valkey outage degrades to at-least-once rather than dropping webhooks. +2. **Durable backstop**: the `idx_workflow_runs_inflight` partial-unique index makes the dispatcher reject a second in-flight run for the same workflow+target even if the Valkey claim was skipped. -If both miss, the request proceeds to the allowlist + concurrency guard. +The legacy in-memory `Map` + tracking-comment marker scan was retired in issue #211. ## What you see while it runs diff --git a/docs/use/safety.md b/docs/use/safety.md index 7366f7e7..1a823599 100644 --- a/docs/use/safety.md +++ b/docs/use/safety.md @@ -42,7 +42,7 @@ The shepherding probe detects when a non-bot principal has pushed to the PR's he ## Idempotency -A duplicate webhook delivery, same `X-GitHub-Delivery` header or same tracking-comment marker, is dropped before any work runs. The fast in-memory `Map` is lost on restart; the durable check (looking for the bot's hidden delivery marker in existing tracking comments) survives crash loops. +A duplicate webhook delivery (same `X-GitHub-Delivery` header) is dropped before any work runs: the side-effecting handlers claim each delivery once via a Valkey `SET NX` with a 3-day TTL (`claimDelivery`), and the `idx_workflow_runs_inflight` index is the durable backstop that rejects a second in-flight run for the same workflow+target if the claim was skipped (issue #202). ## Fork PRs diff --git a/src/app.ts b/src/app.ts index ad606069..fe794335 100644 --- a/src/app.ts +++ b/src/app.ts @@ -290,7 +290,7 @@ async function handleTestWebhook( const deliveryId = `test-${randomUUID()}`; // Mock Octokit that logs instead of calling GitHub API. - // Only used by the router path (isAlreadyProcessed, concurrency comment). + // Only used by the router path (capacity / spawn-failed comments). // The daemon creates its own real Octokit from the installation token. const mockOctokit = buildMockOctokit(); diff --git a/src/core/tracking-comment.ts b/src/core/tracking-comment.ts index ed5358f1..75b5ddcb 100644 --- a/src/core/tracking-comment.ts +++ b/src/core/tracking-comment.ts @@ -75,45 +75,16 @@ export function renderTriageSection(triage: TriageCommentSection): string { } /** - * Build the hidden HTML marker used for durable idempotency. - * Embedded in the tracking comment body so the marker survives pod restarts - * and can be detected on webhook retries. + * Build the hidden HTML marker embedded in the tracking comment body so the + * bot's own tracking comment can be located and updated in place (see the + * `comment` MCP server). Redelivery idempotency lives elsewhere now + * (`claimDelivery` + `idx_workflow_runs_inflight`, issue #202); the in-memory + * Map + marker-scan idempotency check were retired in issue #211. */ export function deliveryMarker(deliveryId: string): string { return ``; } -/** - * Check if a bot comment for this delivery already exists. - * Used as a durable idempotency check that survives pod restarts, complementing - * the in-memory processed Map which only covers the current process lifetime. - * - * Per: https://docs.github.com/en/webhooks/using-webhooks/best-practices-for-using-webhooks - */ -export async function isAlreadyProcessed(ctx: BotContext): Promise { - const { octokit, owner, repo, entityNumber, deliveryId, triggerTimestamp } = ctx; - - const marker = deliveryMarker(deliveryId); - - // Scope the scan with `since=triggerTimestamp` so we only see comments at-or-after the - // webhook trigger. The per-issue listComments endpoint orders strictly by ascending ID - // and accepts only `since`, `per_page`, `page`, `direction`/`sort` are silently dropped - // (only the repo-level sibling endpoint honours them). Without `since`, page 1 would be - // the OLDEST 100 comments, leaving the tracking marker stranded on page 2+ for any - // PR/issue with >100 prior comments and breaking the durable idempotency check on retry. - // The tracking comment is posted seconds after the trigger, so even ~24h of retries land - // inside the `since` window and the marker is guaranteed to be on page 1. - const comments = await octokit.rest.issues.listComments({ - owner, - repo, - issue_number: entityNumber, - per_page: 100, - since: triggerTimestamp, - }); - - return comments.data.some((c) => c.body?.includes(marker) === true); -} - /** * Create the initial tracking comment ("Working..."). * Returns the comment ID for future updates. @@ -123,9 +94,10 @@ export async function isAlreadyProcessed(ctx: BotContext): Promise { export async function createTrackingComment(ctx: BotContext): Promise { const { octokit, owner, repo, entityNumber, log } = ctx; - // Embed the deliveryId marker for durable idempotency, survives pod restarts. - // The in-memory processed Map in router.ts is the fast-path check; - // this marker is the durable fallback. + // Embed the deliveryId marker so the bot can locate and update its own tracking + // comment in place (see the `comment` MCP server). Not an idempotency mechanism + // anymore (claimDelivery + idx_workflow_runs_inflight own that, #202; the Map + + // marker-scan check were retired in #211). const body = `${deliveryMarker(ctx.deliveryId)}\n${SPINNER_HTML} **${config.triggerPhrase}** is working on this...\n\n_Analyzing your request..._`; const guarded = await safePostToGitHub({ @@ -245,8 +217,9 @@ export async function finalizeTrackingComment( const errorSection = error !== undefined && error !== "" ? `\n\n---\n**Error:** ${error}` : ""; - // Re-prepend the delivery marker so the durable idempotency check survives even if - // Claude's update_claude_comment call (which runs sanitizeContent) previously stripped it. + // Re-prepend the delivery marker so the tracking comment keeps its stable hidden marker + // even if Claude's update_claude_comment call (which runs sanitizeContent) previously + // stripped it. The marker locates the bot's comment, not idempotency (#202/#211). const finalBody = `${deliveryMarker(ctx.deliveryId)}\n${header}\n\n---\n${cleanedBody}${errorSection}`; await updateTrackingComment(ctx, trackingCommentId, finalBody); diff --git a/src/mcp/servers/comment.ts b/src/mcp/servers/comment.ts index 9a923f55..42a340d9 100644 --- a/src/mcp/servers/comment.ts +++ b/src/mcp/servers/comment.ts @@ -94,8 +94,10 @@ server.tool( "secret redacted from comment body", ); } - // Re-prepend the delivery marker after sanitizeContent strips it (stripHtmlComments). - // The marker is required for the durable idempotency check in isAlreadyProcessed(). + // Re-prepend the delivery marker after sanitizeContent strips it (stripHtmlComments) + // so the tracking comment keeps the stable hidden marker createTrackingComment wrote + // (see `deliveryMarker`). The idempotency check that used to read it was retired in + // issue #211; the marker is preserved for tracking-comment format consistency. // DELIVERY_ID is validated non-empty at process startup so the cast is safe. const markerPrefix = ``; const bodyWithMarker = `${markerPrefix}\n${guarded.body}`; diff --git a/src/webhook/idempotency.ts b/src/webhook/idempotency.ts index 31de559b..383a307a 100644 --- a/src/webhook/idempotency.ts +++ b/src/webhook/idempotency.ts @@ -28,10 +28,10 @@ const KEY_PREFIX = "idemp:webhook:"; * degrades to at-least-once processing rather than dropping every webhook. The * `idx_workflow_runs_inflight` partial-unique index remains the durable backstop * against duplicate work when this best-effort layer is skipped: the dispatcher - * rejects a second in-flight run for the same workflow+target. (The - * tracking-comment marker scan via `isAlreadyProcessed` is NOT a backstop here: - * it runs only on the legacy `router.ts processRequest` path that production - * handlers bypass, issue #202.) + * rejects a second in-flight run for the same workflow+target. (The legacy + * tracking-comment marker scan that used to guard the `router.ts processRequest` + * path was retired in issue #211; that path is dev-test-only and production + * handlers never used it, issue #202.) */ export async function claimDelivery(deliveryId: string, log: Logger): Promise { const client = getValkeyClient(); diff --git a/src/webhook/router.ts b/src/webhook/router.ts index 9437f50e..8e86f35c 100644 --- a/src/webhook/router.ts +++ b/src/webhook/router.ts @@ -1,5 +1,4 @@ import { config } from "../config"; -import { isAlreadyProcessed } from "../core/tracking-comment"; import { getDb } from "../db"; import { EphemeralSpawnError, spawnEphemeralDaemon } from "../k8s/ephemeral-daemon-spawner"; import { getActiveCount, isAtCapacity } from "../orchestrator/concurrency"; @@ -42,25 +41,6 @@ export interface DispatchDecision { spawnError?: string; } -const processed = new Map(); - -const IDEMPOTENCY_TTL_MS = 60 * 60 * 1000; - -export function cleanupStaleIdempotencyEntries(entries: Map, ttlMs: number): void { - const cutoff = Date.now() - ttlMs; - for (const [id, ts] of entries) { - if (ts < cutoff) { - entries.delete(id); - } - } -} - -const cleanupInterval = setInterval( - cleanupStaleIdempotencyEntries.bind(null, processed, IDEMPOTENCY_TTL_MS), - IDEMPOTENCY_TTL_MS, -); -cleanupInterval.unref(); - export function logDispatchDecision(ctx: BotContext, decision: DispatchDecision): void { const triage = decision.triage; ctx.log.info( @@ -86,20 +66,19 @@ export function logDispatchDecision(ctx: BotContext, decision: DispatchDecision) ); } +/** + * Triage + dispatch a single request through the orchestrator → daemon flow. + * + * Reachable ONLY via the dev-only `/api/test/webhook` endpoint (`app.ts`, + * 404 in production); production webhook handlers in `webhook/events/*` + * bypass this path entirely. It carries no idempotency layer of its own: + * the dev endpoint mints a fresh synthetic delivery id per call so there + * is nothing to dedup, and real redelivery dedup belongs to the production + * handlers (`claimDelivery` + the `idx_workflow_runs_inflight` backstop, + * issue #202). The legacy in-memory Map + `isAlreadyProcessed()` durable + * scan that used to guard this entry point were retired in issue #211. + */ export async function processRequest(ctx: BotContext): Promise { - if (processed.has(ctx.deliveryId)) { - ctx.log.info("Skipping duplicate delivery (in-memory)"); - return; - } - - // Reserve BEFORE the async durable check, closes a retry race. - processed.set(ctx.deliveryId, Date.now()); - - if (await isAlreadyProcessed(ctx)) { - ctx.log.info("Skipping duplicate delivery (durable marker found)"); - return; - } - // Owner allowlist check, MUST run before any GitHub side effects to // preserve the "silent skip" guarantee for non-allowlisted repos. const authResult = isOwnerAllowed(ctx.owner, ctx.log); diff --git a/test/core/tracking-comment.test.ts b/test/core/tracking-comment.test.ts index be1216f8..9a822d97 100644 --- a/test/core/tracking-comment.test.ts +++ b/test/core/tracking-comment.test.ts @@ -5,7 +5,6 @@ import { createTrackingComment, deliveryMarker, finalizeTrackingComment, - isAlreadyProcessed, renderDispatchReasonLine, renderTriageSection, type TriageCommentSection, @@ -24,134 +23,6 @@ describe("deliveryMarker", () => { }); }); -// ─── isAlreadyProcessed ─────────────────────────────────────────────────────── - -describe("isAlreadyProcessed", () => { - it("returns true when the delivery marker is found in comments", async () => { - const ctx = makeBotContext({ deliveryId: DELIVERY_ID }); - ctx.octokit = { - rest: { - issues: { - listComments: mock(() => - Promise.resolve({ - data: [ - { body: `\nWorking...` }, - { body: "Some other comment" }, - ], - }), - ), - }, - }, - } as unknown as Octokit; - - const result = await isAlreadyProcessed(ctx); - expect(result).toBe(true); - }); - - it("calls listComments with since=triggerTimestamp and per_page:100, and does NOT pass direction/sort", async () => { - // The per-issue listComments REST endpoint accepts only `since`, `per_page`, `page`. - // `direction`/`sort` are silently dropped by GitHub (only the repo-level sibling - // endpoint honours them), so passing them would be a no-op that hides an idempotency - // bug on threads with >100 prior comments. Lock the call shape against regression. - const triggerTs = "2026-04-27T10:00:00Z"; - const ctx = makeBotContext({ deliveryId: DELIVERY_ID, triggerTimestamp: triggerTs }); - const listComments = mock(() => Promise.resolve({ data: [] })); - ctx.octokit = { rest: { issues: { listComments } } } as unknown as Octokit; - - await isAlreadyProcessed(ctx); - - const callArgs = (listComments.mock.calls[0] as [Record])[0]; - expect(callArgs["since"]).toBe(triggerTs); - expect(callArgs["per_page"]).toBe(100); - expect(callArgs["direction"]).toBeUndefined(); - expect(callArgs["sort"]).toBeUndefined(); - }); - - it("hot PR: returns false when 100 old unrelated comments come back without the marker", async () => { - // Simulates the bug scenario fixed by switching from the bogus `direction:"desc"` to - // `since=triggerTimestamp`: a thread with >100 prior comments returns the oldest 100 - // ascending; with `since` narrowing the window, page 1 contains zero unrelated comments - // and the marker-less response correctly resolves to false (no duplicate-run trigger). - const ctx = makeBotContext({ deliveryId: DELIVERY_ID }); - const oldComments = Array.from({ length: 100 }, (_, i) => ({ - body: `\nUnrelated old comment ${String(i)}`, - })); - ctx.octokit = { - rest: { - issues: { listComments: mock(() => Promise.resolve({ data: oldComments })) }, - }, - } as unknown as Octokit; - - expect(await isAlreadyProcessed(ctx)).toBe(false); - }); - - it("hot PR + retry: returns true when the only comment in the since-bounded window carries the marker", async () => { - // Webhook-retry path: on a fresh pod the in-memory Map is empty, so the durable check - // is the only guard. The previously-posted tracking comment sits inside the - // `since=triggerTimestamp` window and must be discoverable on page 1. - const ctx = makeBotContext({ deliveryId: DELIVERY_ID }); - ctx.octokit = { - rest: { - issues: { - listComments: mock(() => - Promise.resolve({ - data: [ - { body: "@chrisleekr-bot trigger comment that fired this delivery" }, - { - body: `\nWorking on this...`, - }, - ], - }), - ), - }, - }, - } as unknown as Octokit; - - expect(await isAlreadyProcessed(ctx)).toBe(true); - }); - - it("returns false when no comment contains the delivery marker", async () => { - const ctx = makeBotContext({ deliveryId: DELIVERY_ID }); - ctx.octokit = { - rest: { - issues: { - listComments: mock(() => - Promise.resolve({ - data: [ - { body: "\nWorking..." }, - { body: "Normal comment" }, - ], - }), - ), - }, - }, - } as unknown as Octokit; - - const result = await isAlreadyProcessed(ctx); - expect(result).toBe(false); - }); - - it("returns false when comment list is empty", async () => { - const ctx = makeBotContext({ deliveryId: DELIVERY_ID }); - ctx.octokit = { - rest: { issues: { listComments: mock(() => Promise.resolve({ data: [] })) } }, - } as unknown as Octokit; - - expect(await isAlreadyProcessed(ctx)).toBe(false); - }); - - it("returns false for a comment with undefined body", async () => { - const ctx = makeBotContext({ deliveryId: DELIVERY_ID }); - ctx.octokit = { - rest: { - issues: { listComments: mock(() => Promise.resolve({ data: [{ body: undefined }] })) }, - }, - } as unknown as Octokit; - - expect(await isAlreadyProcessed(ctx)).toBe(false); - }); -}); - // ─── createTrackingComment ──────────────────────────────────────────────────── describe("createTrackingComment", () => { diff --git a/test/webhook/router.test.ts b/test/webhook/router.test.ts index a865709f..f1b7bffe 100644 --- a/test/webhook/router.test.ts +++ b/test/webhook/router.test.ts @@ -1,12 +1,17 @@ /** * Router tests: post dispatch-collapse surface. * - * The router now exposes three public functions plus telemetry helpers: - * - cleanupStaleIdempotencyEntries (pure utility) - * - logDispatchDecision (pure telemetry) - * - decideDispatch (triage → scaler → spawn? verdict) - * - dispatch (routes spawn-failed vs persistent-daemon) - * - processRequest (top-level: idempotency + auth + capacity) + * The router now exposes these functions: + * - logDispatchDecision (pure telemetry) + * - decideDispatch (triage → scaler → spawn? verdict) + * - dispatch (routes spawn-failed vs persistent-daemon) + * - processRequest (dev-test-endpoint-only: auth + capacity + dispatch) + * + * The in-memory idempotency Map + `isAlreadyProcessed` marker scan that + * `processRequest` used to carry were retired in issue #211 (the path is + * reachable only from the dev `/api/test/webhook` endpoint, which mints a + * fresh delivery id per call; production handlers own redelivery dedup via + * `claimDelivery` + `idx_workflow_runs_inflight`, issue #202). * * We mock every downstream surface that reaches over the network or hits * Valkey/Postgres: the router is a pure orchestrator. @@ -19,11 +24,6 @@ import { makeBotContext, makeOctokit } from "../factories"; // ─── Mocked downstream surfaces (persist across this process run) ───────── -const mockIsAlreadyProcessed = mock(() => Promise.resolve(false)); -void mock.module("../../src/core/tracking-comment", () => ({ - isAlreadyProcessed: mockIsAlreadyProcessed, -})); - const mockGetDb = mock(() => null as unknown); void mock.module("../../src/db", () => ({ getDb: mockGetDb, @@ -104,13 +104,8 @@ void mock.module("../../src/webhook/triage-client-factory", () => ({ // Import router AFTER mocks are set up. -const { - cleanupStaleIdempotencyEntries, - decideDispatch, - dispatch, - logDispatchDecision, - processRequest, -} = await import("../../src/webhook/router"); +const { decideDispatch, dispatch, logDispatchDecision, processRequest } = + await import("../../src/webhook/router"); const { getActiveCount, decrementActiveCount } = await import("../../src/orchestrator/concurrency"); const { config } = await import("../../src/config"); @@ -150,7 +145,6 @@ function withConfig(patch: Record, fn: () => T | Promise) // ─── Tests ──────────────────────────────────────────────────────────────── beforeEach(() => { - mockIsAlreadyProcessed.mockClear(); mockSpawnEphemeralDaemon.mockClear(); mockSpawnEphemeralDaemon.mockImplementation(() => Promise.resolve("ephemeral-daemon-xyz")); mockMarkSpawn.mockClear(); @@ -175,18 +169,6 @@ beforeEach(() => { while (getActiveCount() > 0) decrementActiveCount(); }); -describe("cleanupStaleIdempotencyEntries", () => { - it("removes entries older than the TTL and keeps fresh ones", () => { - const m = new Map(); - const now = Date.now(); - m.set("old", now - 10_000); - m.set("fresh", now); - cleanupStaleIdempotencyEntries(m, 5_000); - expect(m.has("old")).toBe(false); - expect(m.has("fresh")).toBe(true); - }); -}); - describe("logDispatchDecision", () => { it("emits the core dispatch fields", () => { const ctx = makeCtx(); @@ -421,23 +403,21 @@ describe("dispatch", () => { }); }); -describe("processRequest: idempotency + auth + capacity", () => { - it("skips duplicate delivery (in-memory map)", async () => { - const ctx = makeCtx({ deliveryId: "dup-delivery" }); - // First pass runs, stub allowlist via ambient env, else allowlist check - // silently skips. We deliberately use the same deliveryId twice. - await processRequest(ctx); - mockIsAlreadyProcessed.mockClear(); - await processRequest(makeCtx({ deliveryId: "dup-delivery" })); - // Second call bailed at the in-memory map, so isAlreadyProcessed is not called. - expect(mockIsAlreadyProcessed).toHaveBeenCalledTimes(0); +describe("processRequest: auth + dispatch wiring", () => { + it("dispatches an allowlisted request through decideDispatch → dispatch", async () => { + await withConfig({ allowedOwners: undefined }, async () => { + await processRequest(makeCtx()); + // Reached dispatchDaemon: executions row written + daemon dispatch attempted. + expect(mockCreateExecution).toHaveBeenCalledTimes(1); + expect(mockDispatchJob).toHaveBeenCalledTimes(1); + }); }); - it("skips request when the durable marker is already present", async () => { - mockIsAlreadyProcessed.mockImplementation(() => Promise.resolve(true)); - const ctx = makeCtx(); - await processRequest(ctx); - expect(mockDispatchJob).toHaveBeenCalledTimes(0); - expect(mockCreateExecution).toHaveBeenCalledTimes(0); + it("skips a non-allowlisted owner without any side effects", async () => { + await withConfig({ allowedOwners: ["someone-else"] }, async () => { + await processRequest(makeCtx({ owner: "chrisleekr" })); + expect(mockCreateExecution).toHaveBeenCalledTimes(0); + expect(mockDispatchJob).toHaveBeenCalledTimes(0); + }); }); }); diff --git a/test/workflows/dispatcher.test.ts b/test/workflows/dispatcher.test.ts index c7bcce74..75c9084f 100644 --- a/test/workflows/dispatcher.test.ts +++ b/test/workflows/dispatcher.test.ts @@ -199,6 +199,10 @@ describe("dispatchByLabel", () => { } expect(mockPostRefusalComment).toHaveBeenCalledTimes(1); expect(mockEnqueueJob).not.toHaveBeenCalled(); + // #211 Part 2: a collision surfaces "in-flight" (refused), it must NOT be + // marked failed. markFailed is reserved for post-insert enqueue failures + // (next test), where it deliberately clears the in-flight guard for retry. + expect(mockMarkFailed).not.toHaveBeenCalled(); }); it("rethrows non-collision insertQueued errors without refusing", async () => {