Skip to content

fix(notifications): a notification_deliveries row stuck at pending from a failed enqueue is never rescued #9320

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

evaluateAndEnqueueNotificationDeliveries (src/notifications/service.ts:214-227) commits each
fresh notification_deliveries row (via insertNotificationDeliveryIfAbsent, idempotent on
UNIQUE(dedup_key, channel)src/db/repositories.ts:2237-2276) before it enqueues the
matching notify-deliver job:

export async function evaluateAndEnqueueNotificationDeliveries(
  env: Env,
  events: DetectedNotificationEvent[],
): Promise<NotificationDeliveryRecord[]> {
  const pending: NotificationDeliveryRecord[] = [];
  for (const event of events) {
    pending.push(...(await evaluateNotificationEvent(env, event)));
  }
  await Promise.all(
    pending.map((delivery) =>
      env.JOBS.send({ type: "notify-deliver", requestedBy: "notify-evaluate", deliveryId: delivery.id }),
    ),
  );
  return pending;
}

If env.JOBS.send() rejects for even one delivery in that Promise.all (queue backpressure or a
transient error), the whole call throws. This function is called directly and synchronously from
the AMS notifications ingest route (evaluateNotificationEvent's doc comment: "Used by the AMS
ingest route (#7657)"), so the HTTP request fails and the row(s) already committed are left at
status: "pending". A client retry resubmits the same events, but the idempotency check in
insertNotificationDeliveryIfAbsent now finds those rows already exist (created: false) and
silently omits them from pending on the retry — so no notify-deliver job is ever (re-)sent for
them. deliverNotification (src/notifications/service.ts:277-284) is never invoked for that
row, and buildNotificationFeed (:254-) only surfaces delivered/read rows — so the
notification is permanently invisible to the recipient until the 90-day retention sweep
(src/db/retention.ts:31, notification_deliveries) deletes it. The identical
commit-then-Promise.all-enqueue shape also exists in the queue's own notify-evaluate
notify-deliver handoff (referenced by this same doc comment as the pattern this function
mirrors), so a lost enqueue is not unique to the AMS route.

This is the exact failure class this codebase has already named and fixed once, for a different
subsystem: src/review/pending-closure-watchdog.ts (#9031/#9007) documents "a sequence that
depends on one message surviving" and adds a durable-deadline sweep
(recordPendingClosureFlag + sweepStrandedPendingClosures, re-enqueuing Pass 2 for any flag past
its deadline) precisely because "if the queue lost the job... [the PR sat] going nowhere." No
equivalent rescue sweep exists for notification_deliveries stuck at pending.
deliverNotification is already safely idempotent (if (!delivery || delivery.status !== "pending") return;),
so a rescue sweep that re-enqueues notify-deliver for old pending rows is low-risk by
construction — replaying it for a row that was actually delivered in the meantime is a no-op.

test/unit/notifications-service.test.ts only ever stubs JOBS.send to succeed — no test
exercises a partial/failed enqueue.

Requirements

  • Add a new function (in src/notifications/service.ts or a new src/notifications/*.ts module,
    your choice, following this codebase's convention of one module per subsystem concern) that
    scans for notification_deliveries rows with status: "pending" older than a fixed grace
    period, and re-enqueues a notify-deliver job for each — mirroring
    sweepStrandedPendingClosures's shape (src/review/pending-closure-watchdog.ts:59-): a bounded
    lookback window, a grace period past which a merely-slow-but-in-flight row is not mistaken for a
    lost one, and a minimum re-sweep interval so a row stuck for a reason re-enqueue cannot fix is
    retried periodically rather than every single sweep tick.
  • Add whatever DB query is needed to list notification_deliveries rows by status: "pending"
    and createdAt older than a cutoff (there is currently no such query in
    src/db/repositories.ts — add one following the existing query style in that file, e.g. near
    getNotificationDeliveryById/listNotificationDeliveriesForLogin).
  • Wire the new sweep into the periodic queue tick alongside the other sweep/reconcile calls in
    src/queue/job-dispatch.ts (see the block calling sweepStaleApprovalQueue,
    reconcileMissingPrOutcomes, sweepStrandedPendingClosures,
    reconcileSurfaceWithoutDisposition around src/queue/job-dispatch.ts:299-310) — same
    .catch(() => null) fail-open pattern, same conditional log-on-nonzero-result shape as its
    siblings there.
  • Must not re-enqueue a row that is no longer status: "pending" (i.e. was actually delivered in
    the interim) — the sweep's DB query itself should filter on status: "pending", not rely solely
    on deliverNotification's own idempotency guard to no-op it.
  • Fail open on a DB read error (return an empty/zero result, do not throw), matching
    sweepStrandedPendingClosures's own try { ... } catch { return { scanned: 0, requeued: 0 }; }
    shape.

Deliverables

  • A DB query in src/db/repositories.ts listing notification_deliveries rows with
    status: "pending" and createdAt older than a given cutoff.
  • A sweep function (mirroring sweepStrandedPendingClosures's shape: bounded lookback, grace
    period, minimum re-sweep interval, fail-open on error) that re-enqueues a notify-deliver
    job for each qualifying row.
  • The sweep wired into the periodic queue tick in src/queue/job-dispatch.ts, alongside the
    existing sibling sweeps, with the same fail-open/conditional-log pattern.
  • Regression tests: (1) a pending row older than the grace period is re-enqueued; (2) a
    pending row younger than the grace period is left alone; (3) a row no longer pending
    (already delivered) is not re-enqueued; (4) a DB read failure returns a zero result rather
    than throwing.

All of the above Deliverables are required in the same PR.

Test Coverage Requirements

99%+ Codecov patch coverage, branch-counted, on every new/changed line in
src/notifications/*.ts, src/db/repositories.ts (the new query), and
src/queue/job-dispatch.ts (the new wiring), including a regression test for this bug (a stranded
pending row is rescued by the sweep, not lost until retention deletes it).

Expected Outcome

A notification_deliveries row left at status: "pending" by a failed/partial enqueue (queue
backpressure, transient JOBS.send error) is rescued by a periodic sweep within a bounded grace
period, instead of being invisible to the recipient until the 90-day retention sweep silently
deletes it.

Links & Resources

  • src/notifications/service.ts:214-227 (evaluateAndEnqueueNotificationDeliveries, where the
    gap is), :178-206 (evaluateNotificationEvent, the idempotent-insert step),
    :277-284 (deliverNotification, already idempotent — safe to replay)
  • src/review/pending-closure-watchdog.ts (sweepStrandedPendingClosures,
    PENDING_CLOSURE_GRACE_MS, PENDING_CLOSURE_LOOKBACK_MS,
    PENDING_CLOSURE_REQUEUE_INTERVAL_MS — the shape to mirror)
  • src/queue/job-dispatch.ts:299-310 (the periodic sweep-wiring block to add to)
  • src/db/retention.ts:31 (notification_deliveries, 90-day retention — the silent-loss backstop
    this sweep prevents from being the only thing that ever "resolves" a stuck row)
  • test/unit/notifications-service.test.ts (existing coverage — no partial-enqueue-failure case)

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions