[quality] kvDeliveryStore never forgets a failed delivery, so a pull-request command that fails on the Worker is lost and cannot be redelivered #653

Finding

On the Cloudflare Worker, a pull-request delivery whose run fails is never retried and cannot be redelivered.

receiveWebhook forgets a failed delivery so GitHub's Redeliver button runs it again (src/adapters/app/webhook.ts:92-94: if (run.failures.length > 0) { await forget(receiver, id) }). forget calls store.forget?.(id). memoryDeliveryStore (src/adapters/app/dedupe.ts:30-35) and the redis store (src/adapters/app/redis.ts:32) implement it; kvDeliveryStore (src/server/worker.ts:51-61) implements only seen, so on the Worker forget is a no-op. The failed delivery stays delivery:<id> = '1' in KV for a day, and a redelivery is answered 200 and skipped.

Until #652 this was unreachable: handleDelivery swallowed its run's failures, so the receiver never saw one. #652 makes handleDelivery throw when appRun.failures is non-empty so the queue consumer retries and "the webhook receiver sees the failure" — but for the Durable Object path (every /close, /lgtm, /approve, /hold on a pull request) seeing it changes nothing: no queue retry, and no working redelivery.

Reproduced at #652 head b628fd3 with a patch fail mode added to worker.test.ts's fake GitHub:

it('forgets a pull request delivery whose run fails, so GitHub\'s redelivery runs it again', async () => {
  await failing('patch')

  expect((await deliver('issue_comment', closeComment(), 'd-close-fails')).status).toBe(202)
  await eventually(async () => (await githubSaw()).some(s => s.method === 'PATCH'))
  await settle()

  const deliveries = await mf.getKVNamespace('DELIVERIES', 'prow')
  expect(await deliveries.get('delivery:d-close-fails')).toBeNull()
  expect((await deliver('issue_comment', closeComment(), 'd-close-fails')).status).toBe(202)
})

→ AssertionError: expected '1' to be null.

Recommendation

One PR, two files:

• src/server/worker.ts, kvDeliveryStore: add

      // a claim and a handled id are the same KV entry, so finishing changes nothing and failing drops the claim
      async done() {},
      async forget(id) {
        await kv.delete(`delivery:${id}`)
      },

• __tests__/worker/worker.test.ts: the test above, plus if (failing.includes('patch')) { return fail() } in the fake's PATCH branch.

Verified at b628fd3: with the store change the new test passes and handles a redelivered id once, remembering it in KV still passes (rm -rf dist-worker first — globalSetup.ts only rebuilds the bundle when it is missing). Ideally this lands inside #652, which already changes both files and introduces the throw that makes the path reachable; a follow-up PR works just as well.

Decision taken by hive: forget deletes the key rather than writing a tombstone, matching memoryDeliveryStore's "a claim and a handled id are the same entry" semantics; done is a no-op for the same reason.

Priority

• Impact: medium — a transient GitHub 5xx during a pull-request command drops that command for good on the Worker deployment
• Effort: low

Filed by quality agent (hold-gated mode)

🐝 Hive Agent: quality | Instance: hosted-available-lke648397-260827-5q9t | SHA: 424fb95

— hive: agent=quality backend=copilot model=claude-fable-5.1 copilot=1.0.88