Skip to content

feat: add opt-in idempotent webhook recovery and restart tests - #886

Draft
Tsubashimo-Nanato wants to merge 2 commits into
Core-Foundry:mainfrom
Tsubashimo-Nanato:test/795-scheduler-restart
Draft

Tsubashimo-Nanato wants to merge 2 commits into
Core-Foundry:mainfrom
Tsubashimo-Nanato:test/795-scheduler-restart

Conversation

@Tsubashimo-Nanato

@Tsubashimo-Nanato Tsubashimo-Nanato commented Sep 29, 2026 •

Copy link
Copy Markdown

Overview

When a scheduled webhook is accepted but the sender exits before recording COMPLETED, lease recovery sends it again. This adds persisted restart tests and an opt-in idempotency contract for cooperating webhook receivers: retries reuse a UUID committed in SQLite before the first send, allowing the receiver to avoid repeating its business effect.

Draft: acceptance scope and upstream prerequisites still need agreement. This does not make arbitrary webhook endpoints, Discord, email or SMS exactly-once. The option is disabled by default and is not automatically enabled by the application.

Related Issue

Closes #795

Linked for Wave tracking; this remains a draft pending agreement on the receiver contract and verification gates. The Wave deadline is September 30, 13:00 UTC (22:00 JST).

Two reviewable scopes are preserved in separate commits:

  • A — 1043d51: seven persisted restart tests, with the post-HTTP-success crash gap explicitly left open.
  • B — 34dd996: A plus the opt-in implementation, receiver contract and crash test below.

Please confirm whether B's conditional guarantee is the intended scope, or whether A should land with the crash gap tracked separately. The earlier scope discussion has the reproduction context.

Changes

  • Add an additive delivery-key table and repository operation using its own committed SQLite transaction. Failed persistence prevents sending; retries and manual dead-letter requeue reuse the key.
  • Pass the key through registered providers in both NotificationScheduler and RetryScheduler. RetryScheduler now honors the shared/injected registry; its existing direct dispatch remains when no provider is registered.
  • Add explicit webhook opt-in and validate the per-job Idempotency-Key; a fixed shared header is rejected in opted-in mode.
  • Add real-SQLite restart coverage, identity/provider tests and a real-HTTP child-process crash test. The fixture is outside production src.
  • Document setup and the receiver contract in docs/WEBHOOK_IDEMPOTENCY.md.

The receiver must atomically commit its business effect and receipt, replay the stored success for the same request, reject conflicting content, and retain keys for every possible retry. All workers must keep the same provider policy. Old unkeyed in-flight jobs must be drained/reconciled before enabling it; mixed destinations need an agreed routing policy. There is no finite retry-lifetime guarantee for safe receipt expiry.

Verification

Node 22.23.3/npm 10.9.9, from listener/:

npm test -- --runInBand --detectOpenHandles --runTestsByPath src/__tests__/scheduler-restart.integration.test.ts src/__tests__/delivery-idempotency.integration.test.ts src/__tests__/scheduler-idempotency-crash.integration.test.ts src/services/retry-scheduler.test.ts
  • 34/34 passed again after moving B onto this PR branch; normal process exit. New tests/fixture pass Prettier.
  • The crash test terminates the sender after real HTTP 200 but before COMPLETED, reopens the receiver database, recovers via RetryScheduler in a new process, and verifies two HTTP attempts, one durable receiver effect. A third sender restart does not resend. This uses controlled lease time and is not production or power-loss testing.
  • Prior full runs on the identical B source under the same local prerequisites: 1,108 passed / 208 failed, versus A 1,093 / 208 and the control 1,086 / 208. All failing assertion identities match; no new failures. Full runs retained open handles after writing results, and their remaining processes were stopped.
  • An additional retry integration check still has the previously recorded failure: expected retry_count 3, actual 2. Typecheck still reports six existing parser errors in index.ts, security-headers.ts and discord-notification.ts; lint invokes the same compiler. No full-suite, build or CI success is claimed.

How to Test

Current main (30b99fe) cannot complete npm ci: its lockfile disagrees with the manifest. Local validation used a lockfile sync to the existing manifest and the three-line request-id.ts repair from #846. Those prerequisite repairs are excluded from this PR. Resolve them before running the focused command; the tests need no live service credentials.

Provider setup and receiver acceptance steps are in the linked contract document. Production activation is intentionally a separate decision.

Checklist

  • Branch is based on current main (30b99fe, checked September 29).
  • Tests added/updated and all pass locally — focused tests pass; full-suite failures disclosed above.
  • cargo fmt --all run — not applicable; no Rust changes.
  • npm run lint passes — existing compiler errors remain.
  • Documentation updated for provider, persistence and receiver behavior.

@Tsubashimo-Nanato Tsubashimo-Nanato changed the title test: cover persisted scheduler restarts feat: add opt-in idempotent webhook recovery and restart tests Sep 29, 2026
@drips-wave

drips-wave Bot commented Sep 29, 2026

Copy link
Copy Markdown

@Tsubashimo-Nanato Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Scheduler Restart Integration Tests

1 participant