fix: recover webhook processing and preserve sync state - #4
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
codra | 3e9509b | Sep 20 2026, 07:37 PM |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e9509b508
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
If Codra has suggestions, it will comment; otherwise it will react with 👍.
| } | ||
|
|
||
| export async function runOpportunisticJobMaintenance(env: AppBindings) { | ||
| await retryPendingWebhookSubmissions(env); |
There was a problem hiding this comment.
New webhook retry step is unguarded and aborts the rest of maintenance
Beyond the stated intent of recovering webhook processing, this new first step of runOpportunisticJobMaintenance has no error isolation, while its siblings are guarded (recoverJobs visibly wraps its body in try/catch). claimWebhookQueueSubmission (line 16) and releaseWebhookQueueSubmission (line 24) are awaited outside any try, so a transient D1 failure rejects retryPendingWebhookSubmissions and cancels the run before recoverJobs and completeTerminalCheckRuns execute, silently dropping that tick's lease recovery and check-run completion. Additionally, if releaseWebhookQueueSubmission throws inside the catch block, the original queue-send error is discarded and the row stays claimed until lease expiry.
| await retryPendingWebhookSubmissions(env); | |
| export async function runOpportunisticJobMaintenance(env: AppBindings) { | |
| try { | |
| await retryPendingWebhookSubmissions(env); | |
| } catch (error) { | |
| logger.error('Webhook submission retry failed', error instanceof Error ? error : new Error(String(error))); | |
| } | |
| await recoverJobs(env); | |
| await completeTerminalCheckRuns(env); | |
| } |
| return c.json({ ok: true, duplicate: true, message: submitted ? 'queued' : 'queue_pending', job }, 202); | ||
| } | ||
|
|
||
| if (!delivery.inserted && delivery.processingStatus !== 'received') { |
There was a problem hiding this comment.
Redelivery of an in-flight 'received' delivery falls through to full reprocessing
The intent is duplicate-delivery convergence, but a redelivery whose row still has status 'received' (first attempt still mid-processing, e.g. slow DB or an early GitHub retry) matches neither this guard nor the 'queue_pending' guard above it, so it falls through and is fully reprocessed: the feedback branch can record the same feedback a second time and the job branch re-runs persistWebhookReviewJob for the same deliveryId. Convergence currently relies on the downstream existingJob check, which does not cover the feedback path. Consider also short-circuiting duplicates still in 'received', or making feedback recording and job persistence idempotent on deliveryId.
|
|
||
| try { | ||
| await c.env.deps.platform.enqueueReviewJob(submission.message); | ||
| await deliveries.markWebhookQueueSubmissionSent(c.env as any, submission.id); |
There was a problem hiding this comment.
Catch block conflates enqueue failure with mark-sent failure, enabling duplicate enqueue
The intent is that only enqueue failures defer work for scheduled retry, but the try covers both enqueueReviewJob and markWebhookQueueSubmissionSent. If the enqueue succeeded but marking the submission sent failed, the catch releases the submission and a later retry re-enqueues a message that was already sent, so the same job can be delivered to the worker twice. Additionally, if releaseWebhookQueueSubmission itself throws, the error propagates out of the catch and the webhook returns 5xx, triggering immediate GitHub redelivery rather than the intended scheduled retry. Consider narrowing the try to the enqueue call (or otherwise distinguishing the two failures) and guarding the release call.
| const migrationsDirectory = path.join(REPO_ROOT, 'packages/db/migrations-d1'); | ||
| for (const migrationFile of readdirSync(migrationsDirectory).filter((file) => file.endsWith('.sql')).sort()) { | ||
| const migrationSql = readFileSync(path.join(migrationsDirectory, migrationFile), 'utf8'); | ||
| const migrationStatements = migrationSql.split(';').map((statement) => statement.trim()).filter(Boolean); |
There was a problem hiding this comment.
Naive semicolon split of migration SQL breaks on semicolons inside statements
The PR's stated goal of applying all D1 migrations in the test environment is only partially achieved: splitting the file on every ; breaks any migration that contains a semicolon inside a string literal, trigger body, or generated column expression, producing malformed statements that either throw or apply corrupted DDL. It works for the current 0001/0002 files, so this is conditional on future migration content, but the loop now silently extends the fragile parsing to every migration added later. Consider a migration runner or at least a comment-only/statement-aware splitter.
Summary
Verification
npm cinpm run lintnpm run typechecknpm test— 60 files, 442 tests passedWRANGLER_LOG_PATH=/tmp/codra-wrangler.log npm run buildgit diff --checkStorage note
Current
mainreplaced PostgreSQL with Cloudflare D1 in PR #1. The integration suite therefore ran against a disposable in-process D1 database via Miniflare, applying all D1 migrations; PostgreSQL is no longer a project backend to exercise.Delivery boundary
Includes the forward D1 migration file, but no production migration, deployment, or merge was performed.