fix(triggers): redis-back review-dispatch dedup to close cross-process gap

[zbigniewsobiecki]

What

Move the review-dispatch dedup state from a per-process in-memory Map to Redis (SET key value NX EX <ttl>), so the dedup holds across all processes that participate in dispatch — router, IMPL workers (post-completion-hook), and any future horizontally-scaled router replicas.

The incident this fixes — ucho/PR #194 (2026-05-01)

CI fully green, single SHA 9ed484dfcc9c95265d55ec9be449f47f813d26a7. Two review runs dispatched 29 s apart, both triggerType=ci-success, both completed normally, both burned LLM tokens.

Run	Container (process)	Trigger path	Loki signal
6e0fe8d0 started 12:52:14	IMPL worker cascade-worker-coalesce_ucho_MNG-453_*	post-completion-hook	review (post-completion) completed { reviewDispatchKey: '...:194:9ed4...', trigger: 'post-completion-hook' }
2847d5c3 started 12:52:43	cascade-router	check-suite-success	Claimed review dispatch for PR+SHA reviewDispatchKey: '...:194:9ed4...'

Identical dedup key. Both claimReviewDispatch(key, ...) calls returned true. Why? The pre-Redis dedup at src/triggers/github/review-dispatch-dedup.ts:5 was a module-scoped recentlyDispatched: Map<string, number> — per-process state. The IMPL worker process saw an empty Map and claimed; the router process saw an empty Map and also claimed. No cross-process synchronization at all.

Why PR #1246 didn't catch this

#1246's plan explicitly noted this gap as out-of-scope:

Cross-process dedup via Redis: would also fix Root cause B in isolation but doesn't help Root cause A. With the architectural shift above, the in-memory dedup is sufficient. Skip.

That was wrong. The architectural shift in #1246 closes one path (worker-bail-out wedging the dedup); it does nothing about the post-completion-hook path running in a separate process. The bug class is broader than post-completion-hook ↔ check-suite-success — anywhere two processes might independently call claimReviewDispatch for the same (project, PR, SHA) is exposed:

• post-completion-hook (worker) ↔ check-suite-success (router) — confirmed live on PR fix(triggers): update initial PR comment on GitHub agent failure #194.
• review-requested (router) ↔ post-completion-hook (worker) — would also break.
• Multi-replica router deployments — when prod scales horizontally, the design assumption "single router process" is undocumented and fragile.

Why also not the work-item lock?

CLAUDE.md notes a per-(projectId, workItemId, agentType) work-item lock at the BullMQ enqueue layer. It IS held in-memory in the router process (src/router/active-workers.ts). It should have rejected the second enqueue.

It didn't because the post-completion-hook enqueues from the IMPL worker process, bypassing the router's lock state entirely. The router's recentlyDispatched Map AND the router's work-item lock are both router-process-local. A worker-process enqueue is invisible to both. That's a follow-up for a separate PR (the work-item lock has more semantics — counters per agent-type, etc. — and lives in active-workers.ts).

The architectural fix

src/triggers/github/review-dispatch-dedup.ts — rewritten around Redis.

• Drops the in-memory Map<string, number> and cleanupExpiredEntries.
• Lazy IORedis singleton (mirrors src/queue/cancel.ts:28-37); first claim call connects.
• claimReviewDispatch / releaseReviewDispatch are now async.
• Keys namespaced under cascade:review-dedup: so they don't collide with BullMQ or any other Redis user.
• Fail-closed on Redis errors: claim returns false (treats the call as a duplicate) and Sentry-captures under tag review_dedup_redis_down. Better to skip a legit dispatch than duplicate. Mirrors spec-017 fail-closed pipeline-capacity-gate posture.
• 5-min TTL preserved from fix(triggers): defer check-suite-success until aggregate state is complete #1246.
• Test-only __resetForTests() flushes the namespace.

Callers — minimal mechanical await additions:

• check-suite-success.ts:240 — await claimReviewDispatch(...).
• check-suite-success.ts:274 — onBlocked: () => { void releaseReviewDispatch(...) } (callback signature is sync; fire-and-forget the release).
• review-requested.ts:101-102 — both calls become await.
• review-requested.ts:131 — same fire-and-forget shape.
• agent-execution.ts:279 (post-completion-hook) — await.

Tests

• tests/unit/triggers/github/review-dispatch-dedup.test.ts rewritten around a vi.mock('ioredis', ...) factory whose closure captures a single in-memory store. Every new Redis(...) instance shares that backend, so the cross-process invariant is trivially testable: instantiate two IORedis clients and verify the second SET NX EX for the same key returns null. This is the regression pin for ucho/PR fix(triggers): update initial PR comment on GitHub agent failure #194.
• New fails closed when Redis errors, returning false and capturing to Sentry test covers the Sentry-capture path under tag review_dedup_redis_down.
• check-suite-success.test.ts and review-requested.test.ts: replaced the per-test recentlyDispatched.clear() with vi.mock of the dedup module, exposing mockClaimReviewDispatch / mockReleaseReviewDispatch spies. Tests that assert the dedup-skip path now use mockClaimReviewDispatch.mockResolvedValueOnce(true).mockResolvedValueOnce(false) to simulate the SET NX EX race.

Out of scope (intentionally — flagged for follow-up)

• Move work-item lock to Redis. Same class of cross-process gap, larger surface (counters, TTL semantics, cross-cutting with orphan cleanup). Defer; revisit if the same bug class recurs.
• Backfill dedup state on router restart. Not needed — the 5-min TTL means any stale claim self-clears within minutes.
• Cross-region Redis. Single-region deployment today.

Test plan

• npx vitest run --project unit-triggers --project unit-api --project unit-core → 7678/7678 pass
• npm run typecheck clean
• npm run lint clean (13 pre-existing warnings, none on changed files)
• CI green
• Reviewer approval (@nhopeatall)
• After deploy: replay the PR fix(triggers): update initial PR comment on GitHub agent failure #194 scenario by triggering an impl agent via Linear status-change AND letting CI fire. Confirm only ONE review run is dispatched; Loki should show Review already dispatched for this PR+SHA, skipping from whichever path arrives second.

🤖 Generated with Claude Code

[nhopeatall]

Summary

Sound fix for a confirmed cross-process dedup gap. The Redis-backed SET NX EX approach is the right primitive, follows the existing src/queue/cancel.ts lazy-singleton pattern, and the fail-closed error handling aligns with the spec-017 posture established elsewhere. Tests directly pin the production incident scenario.

Should Fix

• tests/unit/triggers/shared/agent-execution.test.ts:82,1061,1163 (not in diff) — this test file still uses mockReturnValue(true) / mockReturnValueOnce(false) for mockClaimReviewDispatch, which is now an async function. It works because await <non-Promise> wraps the value in Promise.resolve(), but it should be mockResolvedValue(true) / mockResolvedValueOnce(false) for consistency with the other two test files updated in this PR (check-suite-success.test.ts and review-requested.test.ts). Worth fixing as a drive-by in this PR to keep the mock patterns uniform.

🕵️ claude-code · claude-opus-4-6 · run details

[codecov]

Codecov Report

❌ Patch coverage is 82.35294% with 12 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/triggers/github/review-dispatch-dedup.ts	84.48%	9 Missing ⚠️
src/triggers/github/review-requested.ts	40.00%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!