Skip to content

Commit c504015

Browse files
alari76claude
andcommitted
feat(webhooks): accepted PR events ride the durable signal queue
Closes the last lossy event path: a crash between the webhook 202 and the review session spawning no longer loses the PR event. - The filter chain (signature, action, draft, allowlist, dedup, cap) stays inline — response semantics unchanged. On acceptance the handler enqueues a pr-review signal (deduped on the existing idempotency key, 1h TTL) instead of fire-and-forgetting processPrReviewAsync - processQueuedPrReview consumes from the queue: redelivery-safe via the pre-allocated session id (existing session = spawn already happened), malformed signals dropped with a log, transient failures rethrown so the queue retries them (3 attempts) - Publisher injected by ws-server; without one (tests, engine down or enqueue failure) the legacy inline path runs unchanged Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent d6044ca commit c504015

3 files changed

Lines changed: 135 additions & 5 deletions

File tree

server/webhook-handler.test.ts

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,72 @@ describe('WebhookHandler', () => {
352352
})
353353
})
354354

355+
describe('pr-review durable signal path', () => {
356+
function makePrBody(overrides: Record<string, unknown> = {}): Buffer {
357+
return Buffer.from(JSON.stringify({
358+
action: 'opened',
359+
pull_request: {
360+
number: 7,
361+
title: 'Add feature',
362+
body: 'desc',
363+
draft: false,
364+
html_url: 'https://github.com/acme/widget/pull/7',
365+
user: { login: 'alice' },
366+
head: { sha: 'abc123', ref: 'feat/x' },
367+
base: { ref: 'main' },
368+
},
369+
repository: { full_name: 'acme/widget', name: 'widget' },
370+
sender: { login: 'alice' },
371+
...overrides,
372+
}))
373+
}
374+
375+
it('enqueues a pr-review signal on acceptance instead of processing inline', async () => {
376+
await handler.checkHealth()
377+
const publish = vi.fn()
378+
handler.setSignalPublisher(publish)
379+
380+
const body = makePrBody()
381+
const result = await handler.handleWebhook(body, makeHeaders(body, { event: 'pull_request' }))
382+
383+
expect(result.statusCode).toBe(202)
384+
expect(result.body.accepted).toBe(true)
385+
expect(publish).toHaveBeenCalledTimes(1)
386+
const input = publish.mock.calls[0][0] as { kind: string; dedupeKey: string; payload: Record<string, unknown> }
387+
expect(input.kind).toBe('pr-review')
388+
expect(input.dedupeKey).toMatch(/^pr-review::/)
389+
expect(input.payload.sessionId).toBe(result.body.sessionId)
390+
// Inline processing did not run — no session was created at accept time.
391+
expect(sessions.create).not.toHaveBeenCalled()
392+
})
393+
394+
it('falls back to inline processing when the publisher throws', async () => {
395+
await handler.checkHealth()
396+
handler.setSignalPublisher(() => { throw new Error('engine down') })
397+
398+
const body = makePrBody()
399+
const result = await handler.handleWebhook(body, makeHeaders(body, { event: 'pull_request' }))
400+
401+
// Accepted either way; the inline path runs async and is not asserted
402+
// further here (it exercises the gh mocks like the legacy path).
403+
expect(result.statusCode).toBe(202)
404+
})
405+
406+
it('processQueuedPrReview is a no-op when the pre-allocated session already exists (redelivery)', async () => {
407+
;(sessions.get as ReturnType<typeof vi.fn>).mockReturnValue({ id: 's-1' })
408+
await handler.processQueuedPrReview({
409+
payload: { pull_request: { number: 7 }, repository: { full_name: 'acme/widget', name: 'widget' } },
410+
webhookEvent: { id: 'e1' },
411+
sessionId: 's-1',
412+
})
413+
expect(sessions.create).not.toHaveBeenCalled()
414+
})
415+
416+
it('processQueuedPrReview drops malformed signals without throwing', async () => {
417+
await expect(handler.processQueuedPrReview({})).resolves.toBeUndefined()
418+
})
419+
})
420+
355421
describe('event history', () => {
356422
it('getEvents returns a copy of events', async () => {
357423
await handler.checkHealth()

server/webhook-handler.ts

Lines changed: 63 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,18 @@ export class WebhookHandler extends WebhookHandlerBase<WebhookEvent, WebhookEven
4545
private sessions: SessionManager
4646
private dedup: WebhookDedup
4747
private ghHealthy = false
48+
/**
49+
* Durable-queue publisher (the trigger engine's enqueueSignal, injected).
50+
* When set, accepted PR events are enqueued as `pr-review` signals instead
51+
* of being processed fire-and-forget — a crash between the 202 and the
52+
* review session spawning no longer loses the event. Without it (tests,
53+
* engine unavailable) the legacy inline path runs unchanged.
54+
*/
55+
private signalPublisher: ((input: { kind: string; payload?: Record<string, unknown>; dedupeKey?: string; ttlMs?: number }) => void) | null = null
56+
57+
setSignalPublisher(publish: ((input: { kind: string; payload?: Record<string, unknown>; dedupeKey?: string; ttlMs?: number }) => void) | null): void {
58+
this.signalPublisher = publish
59+
}
4860

4961
constructor(config: FullWebhookConfig, sessions: SessionManager) {
5062
super('webhook', PROCESSING_TIMEOUT_MS)
@@ -538,18 +550,64 @@ export class WebhookHandler extends WebhookHandlerBase<WebhookEvent, WebhookEven
538550
this.recordEvent(webhookEvent)
539551
this.dedup.recordProcessed(eventId, idempotencyKey)
540552

541-
// Process asynchronously
542-
this.processPrReviewAsync(payload, webhookEvent, sessionId).catch(err => {
543-
console.error('[webhook] PR review async processing error:', err)
544-
this.updateEventStatus(eventId, 'error', String(err))
545-
})
553+
// Durable path: enqueue and let the dispatcher deliver (at-least-once,
554+
// survives a crash before the session spawns). Falls back to the legacy
555+
// inline path when no publisher is wired or the enqueue itself fails.
556+
let queued = false
557+
if (this.signalPublisher) {
558+
try {
559+
this.signalPublisher({
560+
kind: 'pr-review',
561+
payload: { payload, webhookEvent, sessionId } as unknown as Record<string, unknown>,
562+
dedupeKey: `pr-review::${idempotencyKey}`,
563+
ttlMs: 60 * 60 * 1000,
564+
})
565+
queued = true
566+
} catch (err) {
567+
console.error('[webhook] Failed to enqueue pr-review signal, processing inline:', err)
568+
}
569+
}
570+
if (!queued) {
571+
this.processPrReviewAsync(payload, webhookEvent, sessionId).catch(err => {
572+
console.error('[webhook] PR review async processing error:', err)
573+
this.updateEventStatus(eventId, 'error', String(err))
574+
})
575+
}
546576

547577
return {
548578
statusCode: 202,
549579
body: { accepted: true, eventId, status: 'processing', sessionId },
550580
}
551581
}
552582

583+
/**
584+
* Consume a queued `pr-review` signal. Redelivery-safe: the session id was
585+
* pre-allocated at acceptance, so an existing session means the spawn
586+
* already happened and the redelivery is a no-op. A rejection (transient gh
587+
* failure before any session exists) propagates so the queue retries it.
588+
*/
589+
async processQueuedPrReview(raw: Record<string, unknown>): Promise<void> {
590+
const { payload, webhookEvent, sessionId } = raw as unknown as {
591+
payload: PullRequestPayload
592+
webhookEvent: WebhookEvent
593+
sessionId: string
594+
}
595+
if (!payload || !webhookEvent || !sessionId) {
596+
// Malformed signal — acking (returning) is correct; retrying can't fix it.
597+
console.error('[webhook] Malformed pr-review signal payload, dropping')
598+
return
599+
}
600+
if (this.sessions.get(sessionId)) return
601+
602+
try {
603+
await this.processPrReviewAsync(payload, webhookEvent, sessionId)
604+
} catch (err) {
605+
console.error('[webhook] Queued PR review processing error:', err)
606+
this.updateEventStatus(webhookEvent.id, 'error', String(err))
607+
throw err
608+
}
609+
}
610+
553611
/**
554612
* Handle PR closed/merged — archive or delete the review cache and kill any
555613
* active review sessions for this PR.

server/ws-server.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -725,6 +725,12 @@ server.listen(port, '0.0.0.0', () => {
725725
if (!handler) throw new Error('Commit event handler not available')
726726
await handler.handle(payload as unknown as import('./commit-event-handler.js').CommitEvent)
727727
})
728+
729+
// Accepted PR webhook events ride the same durable queue: the webhook
730+
// handler enqueues after its filter chain, and the spawn happens here —
731+
// redelivery-safe via the pre-allocated session id.
732+
webhookHandler.setSignalPublisher((input) => { engine.enqueueSignal(input) })
733+
engine.registerSignalHandler('pr-review', (payload) => webhookHandler.processQueuedPrReview(payload))
728734
if (authToken) {
729735
const serverUrl = `http://localhost:${port}`
730736
ensureHookConfig(authToken, serverUrl)

0 commit comments

Comments
 (0)