Skip to content

fix(queue): mark superseded webhook_events rows instead of leaving them stuck at 'queued' forever - #3843

Merged
JSONbored merged 1 commit into
mainfrom
fix/webhook-superseded-status
Jul 6, 2026
Merged

fix(queue): mark superseded webhook_events rows instead of leaving them stuck at 'queued' forever#3843
JSONbored merged 1 commit into
mainfrom
fix/webhook-superseded-status

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • pg-queue.ts/sqlite-queue.ts's general job_key coalesce path (used for e.g. a pull_request "opened"/"synchronize" pr-refresh delivery, keyed github-webhook:pr-refresh:{repo}#{pr}@{headSha}) finds an existing pending row sharing the same key and overwrites its payload with the new message. The overwritten row's original deliveryId — and the webhook_events row that delivery already wrote as status='queued' before it ever reached this coalesce — is discarded with nothing ever marking it processed, error, or otherwise explained. An operator grepping webhook_events for stuck queued rows finds real entries with no way to tell "silently coalesced away, handled by a later delivery" from "actually lost."
  • Before overwriting, both queue backends now read the superseded row's OLD payload, and if it was itself a github-webhook delivery (skipped for every other job type, e.g. rag-index-repo/refresh-registry, which never have a webhook_events row), issue an UPDATE webhook_events SET status='superseded' WHERE delivery_id=... AND status='queued'. Best-effort and fail-safe: a write hiccup here is logged and swallowed, never allowed to abort the coalesce/enqueue itself (the same class of "one failure shouldn't break everything else" bug this whole audit exists to close).
  • Extends the webhook_events.status type in src/db/repositories.ts (recordWebhookEvent) to include "superseded" for documentation completeness, even though the two queue backends write this status directly via raw SQL (they have no access to the D1/Drizzle layer recordWebhookEvent uses — they operate on the same physical Postgres/SQLite database through their own raw connection).
  • Also applies the identical fix to sqlite-queue.ts (self-host's other queue backend), which mirrors pg-queue.ts's coalescing logic exactly and has the identical bug — the issue's own deliverables named only pg-queue.ts, but fixing one backend and leaving the other with the same defect would be incomplete.

Closes #3814.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes. (Touches both queue backends because they share the identical bug, not unrelated scope.)
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate. (src/selfhost/pg-queue.ts is codecov-exempt per codecov.yml, integration/smoke-tested instead; src/selfhost/sqlite-queue.ts is fully covered — 100% on every new line and branch.)
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — no auth/session/CORS surface touched.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — no API/OpenAPI/MCP surface changed.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A — backend-only change, no UI touched.)
  • Visible UI changes include a UI Evidence section below. (N/A — no visible UI change.)
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

…em stuck at 'queued' forever (#3814)

Both self-host queue backends coalesce a new message into an existing
pending row sharing the same job_key by overwriting its payload. The
overwritten row's original deliveryId -- and the webhook_events row that
delivery already wrote as 'queued' before it ever reached this coalesce
-- was discarded with nothing ever marking it processed, error, or
otherwise explained. An operator grepping webhook_events for stuck
'queued' rows found real entries with no way to tell "silently coalesced
away" from "actually lost."

Before overwriting, both pg-queue.ts and sqlite-queue.ts (which mirror
each other's coalescing logic exactly and share the identical bug) now
read the superseded row's old payload and, when it was itself a
github-webhook delivery, mark its webhook_events row 'superseded'.
Best-effort and fail-safe: a write hiccup here is logged and swallowed,
never allowed to abort the coalesce/enqueue itself.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 6, 2026
@loopover-orb

loopover-orb Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-06 20:53:51 UTC

5 files · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review: This PR changes guardrail-protected path(s): src/selfhost/pg-queue.ts (matched src/selfhost/**), src/selfhost/sqlite-queue.ts (matched src/selfhost/**).

Review summary
This PR adds a best-effort `markSupersededWebhookEvent` step to both `pg-queue.ts` and `sqlite-queue.ts`'s job_key coalesce path: before overwriting an existing pending row's payload, it parses the old payload, and if it was a `github-webhook` delivery, marks the corresponding `webhook_events` row 'superseded' instead of leaving it stuck at 'queued' forever. The SQL parameter ordering is correct in both backends, the guard (`status='queued'` in the WHERE clause) prevents clobbering an already-terminal status, and failures are caught/logged without aborting the coalesce, matching the stated fail-safe design. Tests cover the main regression, the same-deliveryId no-op case, a corrupted-payload path (sqlite), and a write-failure fail-safe path (sqlite only, not pg).

Nits — 7 non-blocking
  • Cannot verify from this diff whether `webhook_events.status` has a CHECK constraint in schema.ts restricting allowed values — if it does, this PR needs a migration to add 'superseded' to that constraint (src/db/repositories.ts:4957 only updates the TS type, not the DB schema).
  • test/unit/selfhost-pg-queue.test.ts has a regression test and a non-webhook-type test but, unlike selfhost-sqlite-queue.test.ts, no test exercising the fail-safe/error-swallowing path when the `UPDATE webhook_events` query itself throws.
  • The `incomingMessage.type !== "github-webhook"` branch is marked `/* v8 ignore next */` as unreachable in sqlite-queue.ts but the equivalent line in pg-queue.ts (src/selfhost/pg-queue.ts) has no such annotation despite the same reasoning applying, which is an inconsistency in documented intent.
  • The three near-identical multi-line `#audit-webhook-supersede-trace` comment blocks repeated across both queue files are verbose; consider trimming to the non-obvious parts (fail-safe rationale) and dropping the restated file-local mechanics.
  • Confirm (e.g. by grepping schema.ts/migrations) that webhook_events.status has no CHECK constraint needing a migration alongside the TS type widening in src/db/repositories.ts.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #3814
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 51 registered-repo PR(s), 43 merged, 372 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 51 PR(s), 372 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 51 PR(s), 372 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.42%. Comparing base (7f84fa7) to head (eee1965).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3843   +/-   ##
=======================================
  Coverage   93.41%   93.42%           
=======================================
  Files         326      326           
  Lines       32887    32896    +9     
  Branches    12044    12046    +2     
=======================================
+ Hits        30723    30732    +9     
  Misses       1530     1530           
  Partials      634      634           
Files with missing lines Coverage Δ
src/db/repositories.ts 96.54% <ø> (ø)
src/selfhost/sqlite-queue.ts 99.60% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 6, 2026
@JSONbored
JSONbored merged commit 11dd9ea into main Jul 6, 2026
11 checks passed
@JSONbored
JSONbored deleted the fix/webhook-superseded-status branch July 6, 2026 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Development

Successfully merging this pull request may close these issues.

fix(queue): mark superseded webhook_events rows instead of leaving them stuck at 'queued' forever

1 participant