Skip to content

fix(db): add a monotonic tiebreak to listNotificationDeliveriesForRecipient's ORDER BY - #8926

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
kai392:fix/critical-issue-notif-deliveries-tiebreak
Jul 26, 2026
Merged

fix(db): add a monotonic tiebreak to listNotificationDeliveriesForRecipient's ORDER BY#8926
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
kai392:fix/critical-issue-notif-deliveries-tiebreak

Conversation

@kai392

@kai392 kai392 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Summary

listNotificationDeliveriesForRecipient's badge-feed query ordered only by createdAt DESC with no
secondary sort key. evaluateAndEnqueueNotificationDeliveries fans out one event to many recipients,
stamping createdAt via nowIso() per row, so several deliveries in one fan-out can tie on the
millisecond timestamp — leaving display order (and the limit-boundary row) engine-defined.

Adds desc(notificationDeliveries.id) as a deterministic secondary sort, matching the same fix class
as ai_review_cache's tiebreak.

Test plan

  • New: two deliveries with an identical createdAt return a stable, deterministic order (id-desc), using controlled ids so the query provably reorders rather than echoing insertion order
  • Non-vacuity verified: reverting the tiebreak makes the order fall back to insertion order and the test fails
  • npx vitest run test/unit/notifications-service.test.ts — 21/21 pass; tsc --noEmit clean

Closes #8895

@kai392
kai392 requested a review from JSONbored as a code owner July 26, 2026 13:34
@superagent-security

Copy link
Copy Markdown
Contributor

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

…ipient's ORDER BY

listNotificationDeliveriesForRecipient ordered only by `createdAt DESC`. A fan-out (e.g.
issue_watch_match to many miners) stamps createdAt via nowIso() per row, so several deliveries can
tie on the millisecond timestamp, leaving display order and the limit-boundary row engine-defined.
Add `desc(notificationDeliveries.id)` as a deterministic secondary sort, matching the ai_review_cache
tiebreak fix.

Closes JSONbored#8895

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.03%. Comparing base (8a3d2c9) to head (8ca562c).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8926      +/-   ##
==========================================
- Coverage   93.84%   91.03%   -2.82%     
==========================================
  Files         803       97     -706     
  Lines       80084    24366   -55718     
  Branches    24277     4505   -19772     
==========================================
- Hits        75157    22182   -52975     
+ Misses       3562     1954    -1608     
+ Partials     1365      230    -1135     
Flag Coverage Δ
backend 96.74% <ø> (+1.61%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/db/repositories.ts 96.74% <ø> (ø)

... and 706 files with indirect coverage changes

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

loopover-orb Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-26 13:46:55 UTC

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

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This is a narrow, correct fix: adding `desc(notificationDeliveries.id)` as a secondary ORDER BY key deterministically resolves ties on `createdAt` within a fan-out. The test uses controlled ids and a manually-inserted identical timestamp to prove the reorder is real (not insertion-order coincidence), and the PR's own description confirms non-vacuity by reverting the tiebreak and observing the test fail. This closes issue #8895 and is well-scoped to a single query change plus a matching regression test.

Nits — 3 non-blocking
  • src/db/repositories.ts: the inline comment above the orderBy is fairly long (3 lines) for a one-line fix; could be trimmed since the PR description already carries the full rationale.
  • test/unit/notifications-service.test.ts: the new test hardcodes the full column list and raw SQL insert rather than using an existing test helper (e.g. `insertNotificationDeliveryIfAbsent`) — verify there isn't already a helper that allows overriding `createdAt` directly, which would avoid the raw SQL string.
  • Consider whether other list queries ordering solely by `createdAt` (e.g. similar feed queries elsewhere in repositories.ts) have the same tie class and would benefit from the same secondary sort, per the PR's own reference to `ai_review_cache`'s tiebreak pattern.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8895
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: 133 registered-repo PR(s), 66 merged, 6 issue(s).
Contributor context ✅ Confirmed Gittensor contributor kai392; Gittensor profile; 133 PR(s), 6 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: minor
Linked issue satisfaction

Addressed
The diff adds `desc(notificationDeliveries.id)` as a secondary sort key to the orderBy clause exactly as requested, and includes a new test inserting two deliveries with identical createdAt that asserts deterministic id-desc ordering.

Review context
  • Author: kai392
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, Cuda, JavaScript, Kotlin, Perl, TypeScript, Vue
  • Official Gittensor activity: 133 PR(s), 6 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Triage stale or unlinked PRs.
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.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 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 LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit dd9676a into JSONbored:main Jul 26, 2026
8 checks passed
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(db): listNotificationDeliveriesForRecipient's badge-feed query orders by createdAt with no tiebreaker

2 participants