feat: add webhook jobs work-queue and delivery history tables with store methods and tests - #5248
Conversation
|
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds persistent webhook delivery jobs, endpoint outcome counters, and webhook delivery history with migrations, store APIs, SQLite/Postgres/ClickHouse support, hybrid delegation, and lifecycle and parity tests. ChangesWebhook delivery persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "feat: webhook delivery queue and history..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/configstore/store.go`:
- Around line 692-694: Update the webhook job lease API around ClaimWebhookJob,
RescheduleWebhookJob, and DeleteWebhookJob so each successful claim issues a
unique lease generation/token, and both mutation methods require and validate
that token along with the job ownership context. Ensure stale tokens cannot
reschedule or delete a job after expiry and reclaim, including when runnerID is
empty, and add a regression test covering stale-owner mutations after reclaim
with race-safe state handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8fa39ee6-6320-4535-a81f-872ea364db75
📒 Files selected for processing (16)
framework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/rdb_test.goframework/configstore/store.goframework/configstore/tables/webhooks.goframework/logstore/clickhousemigrate.goframework/logstore/clickhousestore.goframework/logstore/clickhousestore_test.goframework/logstore/hybrid.goframework/logstore/logstoreparity_test.goframework/logstore/migrations.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goframework/logstore/webhookdelivery_test.gotransports/bifrost-http/lib/config_test.go
b098b0b to
5354465
Compare
e623f4b to
44ef0a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/configstore/rdb_test.go`:
- Around line 2678-2698: Update TestWebhookJobClaimRace to run both
ClaimWebhookJob calls concurrently, using a synchronization barrier so node-a
and node-b begin claiming at the same time. Collect both results and assert that
exactly one claimer returns true, while preserving error checks and the final
stored-claim assertions.
In `@framework/configstore/rdb.go`:
- Around line 7607-7632: Update CreateWebhookJob to validate that a new job has
the initial queue state: reject nonzero attempt counts and any existing claim or
lease fields before persisting it. Update ClaimWebhookJob so successful claims
always assign a lease strictly in the future, preserving race-safe ownership and
preventing immediately expired claims.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b5a2233f-717a-40e1-9ebd-cb051592c264
📒 Files selected for processing (16)
framework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/rdb_test.goframework/configstore/store.goframework/configstore/tables/webhooks.goframework/logstore/clickhousemigrate.goframework/logstore/clickhousestore.goframework/logstore/clickhousestore_test.goframework/logstore/hybrid.goframework/logstore/logstoreparity_test.goframework/logstore/migrations.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goframework/logstore/webhookdelivery_test.gotransports/bifrost-http/lib/config_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- framework/logstore/hybrid.go
- framework/logstore/clickhousemigrate.go
- framework/configstore/tables/webhooks.go
- framework/configstore/migrations.go
- framework/logstore/clickhousestore.go
- framework/configstore/store.go
- transports/bifrost-http/lib/config_test.go
- framework/logstore/tables.go
- framework/logstore/logstoreparity_test.go
- framework/logstore/webhookdelivery_test.go
- framework/logstore/rdb.go
- framework/logstore/migrations.go
5354465 to
fbcfb94
Compare
44ef0a1 to
f759018
Compare
f759018 to
ffbfc01
Compare
Merge activity
|
The base branch was changed.
ffbfc01 to
53be07c
Compare
…ore methods and tests (#5248) ## Summary Adds the persistence layer for webhook delivery: a `webhook_jobs` work-queue table for in-flight deliveries and a `webhook_deliveries` history table for delivery attempt records, along with the store methods needed to drive the delivery worker loop. ## Changes - **`webhook_jobs` table** (`configstore`): New `TableWebhookJob` model and `add_webhook_jobs_table` migration. Rows exist only while a delivery is pending or retrying and are deleted on terminal outcome. The row ID doubles as the stable `webhook-id` wire header across attempts. - **Webhook job store methods** (`configstore`): `CreateWebhookJob`, `ListDueWebhookJobs`, `ClaimWebhookJob`, `RescheduleWebhookJob`, and `DeleteWebhookJob`. Claiming uses a conditional UPDATE so at most one concurrent worker wins per job; expired leases make rows reclaimable for crash recovery. - **Endpoint delivery counters** (`configstore`): `RecordWebhookEndpointSuccess` and `RecordWebhookEndpointFailure` update `consecutive_failures` and the last-success/failure timestamps via atomic column expressions, bypassing save hooks so config fields and `updated_at` are never touched. - **`webhook_deliveries` table** (`logstore`): New `WebhookDelivery` model, `webhook_deliveries_init` migration, and ClickHouse table creation. Rows are insert-only delivery attempt metadata (no payloads). Expiry is handled by `DeleteExpiredWebhookDeliveries` with batched deletes. - **`async_jobs` schema** (`logstore`): `async_jobs_add_webhook_endpoint_id_column` migration adds `webhook_endpoint_id` to reference the endpoint to notify on job completion. - **`LogStore` and `ConfigStore` interfaces** extended with all new methods; `HybridLogStore` delegates to its inner store. - **`MockConfigStore`** in the HTTP transport test suite updated to satisfy the expanded `ConfigStore` interface. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/configstore/... ./framework/logstore/... ``` Key test coverage added: - `TestWebhookEndpointRecordFailureAndSuccess` — counter increment/reset and not-found errors - `TestWebhookEndpointCounterUpdatesLeaveConfigUntouched` — verifies `updated_at` and config fields are not modified - `TestWebhookJobCreateValidation`, `TestWebhookJobCreateDefaultsAndDuplicate` — input validation and duplicate detection - `TestWebhookJobClaimRace`, `TestWebhookJobClaimLeaseExpiryReclaim`, `TestWebhookJobClaimNotDueRejected` — claim fencing and lease expiry recovery - `TestWebhookJobListDue` — ordering, future-job exclusion, live-lease exclusion, expired-lease inclusion - `TestWebhookJobReschedule`, `TestWebhookJobDelete` — ownership fencing on reschedule and delete - `TestWebhookJobClaimCycleWithEmptyRunnerID` — single-node mode with empty runner ID - `TestWebhookDeliveryCreateAndFind`, `TestWebhookDeliverySearchPagination`, `TestWebhookDeliveryDeleteExpired` — delivery history CRUD - `TestLogStoreParity` extended with a `WebhookDeliveries` phase covering all three backends ## Breaking changes - [ ] Yes - [x] No ## Security considerations Webhook secrets are not stored in or exposed by any of the new tables. Delivery counter updates bypass GORM save hooks to prevent accidental secret re-encryption or field clobbering during high-frequency operational writes. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…ore methods and tests (#5248) ## Summary Adds the persistence layer for webhook delivery: a `webhook_jobs` work-queue table for in-flight deliveries and a `webhook_deliveries` history table for delivery attempt records, along with the store methods needed to drive the delivery worker loop. ## Changes - **`webhook_jobs` table** (`configstore`): New `TableWebhookJob` model and `add_webhook_jobs_table` migration. Rows exist only while a delivery is pending or retrying and are deleted on terminal outcome. The row ID doubles as the stable `webhook-id` wire header across attempts. - **Webhook job store methods** (`configstore`): `CreateWebhookJob`, `ListDueWebhookJobs`, `ClaimWebhookJob`, `RescheduleWebhookJob`, and `DeleteWebhookJob`. Claiming uses a conditional UPDATE so at most one concurrent worker wins per job; expired leases make rows reclaimable for crash recovery. - **Endpoint delivery counters** (`configstore`): `RecordWebhookEndpointSuccess` and `RecordWebhookEndpointFailure` update `consecutive_failures` and the last-success/failure timestamps via atomic column expressions, bypassing save hooks so config fields and `updated_at` are never touched. - **`webhook_deliveries` table** (`logstore`): New `WebhookDelivery` model, `webhook_deliveries_init` migration, and ClickHouse table creation. Rows are insert-only delivery attempt metadata (no payloads). Expiry is handled by `DeleteExpiredWebhookDeliveries` with batched deletes. - **`async_jobs` schema** (`logstore`): `async_jobs_add_webhook_endpoint_id_column` migration adds `webhook_endpoint_id` to reference the endpoint to notify on job completion. - **`LogStore` and `ConfigStore` interfaces** extended with all new methods; `HybridLogStore` delegates to its inner store. - **`MockConfigStore`** in the HTTP transport test suite updated to satisfy the expanded `ConfigStore` interface. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/configstore/... ./framework/logstore/... ``` Key test coverage added: - `TestWebhookEndpointRecordFailureAndSuccess` — counter increment/reset and not-found errors - `TestWebhookEndpointCounterUpdatesLeaveConfigUntouched` — verifies `updated_at` and config fields are not modified - `TestWebhookJobCreateValidation`, `TestWebhookJobCreateDefaultsAndDuplicate` — input validation and duplicate detection - `TestWebhookJobClaimRace`, `TestWebhookJobClaimLeaseExpiryReclaim`, `TestWebhookJobClaimNotDueRejected` — claim fencing and lease expiry recovery - `TestWebhookJobListDue` — ordering, future-job exclusion, live-lease exclusion, expired-lease inclusion - `TestWebhookJobReschedule`, `TestWebhookJobDelete` — ownership fencing on reschedule and delete - `TestWebhookJobClaimCycleWithEmptyRunnerID` — single-node mode with empty runner ID - `TestWebhookDeliveryCreateAndFind`, `TestWebhookDeliverySearchPagination`, `TestWebhookDeliveryDeleteExpired` — delivery history CRUD - `TestLogStoreParity` extended with a `WebhookDeliveries` phase covering all three backends ## Breaking changes - [ ] Yes - [x] No ## Security considerations Webhook secrets are not stored in or exposed by any of the new tables. Delivery counter updates bypass GORM save hooks to prevent accidental secret re-encryption or field clobbering during high-frequency operational writes. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Adds the persistence layer for webhook delivery: a
webhook_jobswork-queue table for in-flight deliveries and awebhook_deliverieshistory table for delivery attempt records, along with the store methods needed to drive the delivery worker loop.Changes
webhook_jobstable (configstore): NewTableWebhookJobmodel andadd_webhook_jobs_tablemigration. Rows exist only while a delivery is pending or retrying and are deleted on terminal outcome. The row ID doubles as the stablewebhook-idwire header across attempts.configstore):CreateWebhookJob,ListDueWebhookJobs,ClaimWebhookJob,RescheduleWebhookJob, andDeleteWebhookJob. Claiming uses a conditional UPDATE so at most one concurrent worker wins per job; expired leases make rows reclaimable for crash recovery.configstore):RecordWebhookEndpointSuccessandRecordWebhookEndpointFailureupdateconsecutive_failuresand the last-success/failure timestamps via atomic column expressions, bypassing save hooks so config fields andupdated_atare never touched.webhook_deliveriestable (logstore): NewWebhookDeliverymodel,webhook_deliveries_initmigration, and ClickHouse table creation. Rows are insert-only delivery attempt metadata (no payloads). Expiry is handled byDeleteExpiredWebhookDeliverieswith batched deletes.async_jobsschema (logstore):async_jobs_add_webhook_endpoint_id_columnmigration addswebhook_endpoint_idto reference the endpoint to notify on job completion.LogStoreandConfigStoreinterfaces extended with all new methods;HybridLogStoredelegates to its inner store.MockConfigStorein the HTTP transport test suite updated to satisfy the expandedConfigStoreinterface.Type of change
Affected areas
How to test
go test ./framework/configstore/... ./framework/logstore/...Key test coverage added:
TestWebhookEndpointRecordFailureAndSuccess— counter increment/reset and not-found errorsTestWebhookEndpointCounterUpdatesLeaveConfigUntouched— verifiesupdated_atand config fields are not modifiedTestWebhookJobCreateValidation,TestWebhookJobCreateDefaultsAndDuplicate— input validation and duplicate detectionTestWebhookJobClaimRace,TestWebhookJobClaimLeaseExpiryReclaim,TestWebhookJobClaimNotDueRejected— claim fencing and lease expiry recoveryTestWebhookJobListDue— ordering, future-job exclusion, live-lease exclusion, expired-lease inclusionTestWebhookJobReschedule,TestWebhookJobDelete— ownership fencing on reschedule and deleteTestWebhookJobClaimCycleWithEmptyRunnerID— single-node mode with empty runner IDTestWebhookDeliveryCreateAndFind,TestWebhookDeliverySearchPagination,TestWebhookDeliveryDeleteExpired— delivery history CRUDTestLogStoreParityextended with aWebhookDeliveriesphase covering all three backendsBreaking changes
Security considerations
Webhook secrets are not stored in or exposed by any of the new tables. Delivery counter updates bypass GORM save hooks to prevent accidental secret re-encryption or field clobbering during high-frequency operational writes.
Checklist
docs/contributing/README.mdand followed the guidelines