Skip to content

queue(job-dispatch): bound and isolate the notify-evaluate deliver fan-out, whose retry is a no-op #10022

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

The notify-evaluate job bounds its evaluate fan-out and then immediately does the exact thing that bound was
written to prevent. src/queue/job-dispatch.ts:88-93:

// A batched notify-evaluate job (#selfhost-maintenance-self-pin) can carry many events from one webhook (a
// popular newly-opened issue can have dozens of watchers) -- an unbounded Promise.all over all of them would
// let a single job spend as many concurrent DB/eval calls as it likes, bypassing the queue's own
// backgroundConcurrency cap (which defaults to 1) entirely from inside one job's execution.
const NOTIFY_EVALUATE_EVENT_CONCURRENCY = 5;

src/queue/job-dispatch.ts:386-397:

      const deliveries = (
        await mapWithConcurrency(events, NOTIFY_EVALUATE_EVENT_CONCURRENCY, (event) => evaluateNotificationEvent(env, event))
      ).flat();
      await Promise.all(
        deliveries.map((delivery) =>
          env.JOBS.send({
            type: "notify-deliver",
            requestedBy: "notify-evaluate",
            deliveryId: delivery.id,
          }),
        ),
      );

Three defects in those twelve lines:

  1. The send fan-out is unbounded over a set that is strictly larger than events
    evaluateNotificationEvent returns one delivery per event per resolved channel
    (src/notifications/service.ts:191-209), so deliveries.length >= events.length. The 5-way cap applies to
    the cheap half and not to the half that actually enqueues.

  2. A failed send is completely silent. No console.error, no delivery id, no event name. Compare the
    sibling fan-out 240 lines above in the same file, src/queue/job-dispatch.ts:159-175, which logs
    backfill_registered_repos_fanout_send_failed per failure and then throws a message naming every failed
    repo; and src/queue/signal-snapshot.ts:70-92 (queue: generateSignalSnapshots has no per-repo failure isolation, unlike its job-dispatch.ts sibling (#8355) #9293), which does the same per repo.

  3. The retry recovers nothing. Promise.all rejects on the first send failure and the job is retried, but
    evaluateNotificationEvent returns only rows it just createdsrc/notifications/service.ts:207:

        if (created && delivery.status === "pending") pending.push(delivery);

    On the retry every row already exists, so insertNotificationDeliveryIfAbsent reports created: false,
    pending is empty, and zero notify-deliver jobs are enqueued. The retry does no useful work, burns
    the attempt budget, and dead-letters the job (src/queue/dlq.ts:22-46) — while the delivery whose send
    actually failed stays at status: "pending" and is invisible to the recipient
    (src/notifications/service.ts:261 skips anything not delivered/read). The only thing that ever
    recovers it is sweepStrandedNotificationDeliveries, whose own doc names this exact shape and which
    deliberately waits STRANDED_NOTIFICATION_GRACE_MS = 10 minutes
    (src/notifications/stranded-delivery-sweep.ts:26) before stepping in.

Requirements

  • The notify-deliver enqueue at src/queue/job-dispatch.ts:389-397 must run through
    mapWithConcurrency with a named exported constant NOTIFY_DELIVER_SEND_CONCURRENCY defined in
    src/queue/job-dispatch.ts, so one job can never issue more concurrent env.JOBS.send calls than that.
  • Every delivery's send must be attempted exactly once regardless of an earlier one's outcome: the mapper must
    catch its own error and return a { deliveryId, ok } record rather than rejecting, mirroring
    src/queue/job-dispatch.ts:142-158's Promise.allSettled posture.
  • Each failed send must emit one structured console.error line with
    event: "notify_deliver_fanout_send_failed", the deliveryId, and the stringified reason — mirroring
    src/queue/job-dispatch.ts:170.
  • After every send has been attempted, if any failed the handler must throw an Error whose message states
    <failed>/<total> and lists the failed delivery ids, mirroring src/queue/job-dispatch.ts:173-175, so the
    invocation is still observably failed.
  • The evaluate half (mapWithConcurrency(events, NOTIFY_EVALUATE_EVENT_CONCURRENCY, ...)) and
    NOTIFY_EVALUATE_EVENT_CONCURRENCY itself must NOT change.
  • The legacy event/events payload normalisation at src/queue/job-dispatch.ts:384-385 must NOT change.
  • evaluateNotificationEvent and src/notifications/service.ts must NOT change — the created-only return is
    the correct idempotency contract for that function; this issue is about the caller.
  • The identical fan-out inside evaluateAndEnqueueNotificationDeliveries
    (src/notifications/service.ts:225-232) is out of scope and must NOT change in this PR.

⚠️ Required pattern: src/queue/job-dispatch.ts:142-175 — the backfill-registered-repos fan-out in the
same file: attempt every send, collect the failures, log each one structurally, then throw one aggregate
error naming them. What does NOT satisfy this issue: (a) wrapping the whole Promise.all in a bare
try {} catch {} so the job stops failing — that removes the only signal instead of adding one;
(b) swapping Promise.all for Promise.allSettled and discarding the rejected results, so a lost delivery
is silent and the job reports success; (c) re-enqueuing the whole notify-evaluate job on failure, which
re-runs an evaluate that provably returns nothing; (d) a test-only PR.

Deliverables

  • src/queue/job-dispatch.ts exports NOTIFY_DELIVER_SEND_CONCURRENCY and the notify-evaluate case
    enqueues through mapWithConcurrency at that concurrency, with a per-delivery catch.
  • A failed send emits console.error with event: "notify_deliver_fanout_send_failed" and the
    deliveryId.
  • After all sends are attempted, a non-empty failure set throws an Error naming the count and the failed
    delivery ids.
  • A test in test/unit/notifications-events.test.ts asserting that when env.JOBS.send rejects for the
    2nd of 3 deliveries, all three sends were attempted, the handler throws once, and the thrown message
    contains the 2nd delivery's id.
  • A test in the same file asserting that with 12 deliveries and a send that records its in-flight count,
    the observed maximum concurrency never exceeds NOTIFY_DELIVER_SEND_CONCURRENCY.
  • A regression test at test/unit/notifications-events.test.ts named for this bug asserting that when
    every send succeeds, the handler does not throw and exactly one notify-deliver message is sent per
    delivery — byte-identical to today's happy path.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example adding
the concurrency bound while leaving the bare Promise.all rejection, so a single send failure is still
unlogged and unnamed — does not resolve this issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's coverage.include
covers src/**/*.ts, so src/queue/job-dispatch.ts is measured and gated. The change introduces two branches:
the per-delivery ok/failed arm inside the mapper, and the failures.length > 0 throw arm. Both arms of
both branches need a test — an all-succeed run and a partial-failure run — and the empty-deliveries case
(no events resolved to a delivery) must also be exercised so the zero-iteration path on the new
mapWithConcurrency call is covered.

Expected Outcome

After this ships, a notify-evaluate job with many deliveries issues a bounded number of concurrent queue
sends instead of one per delivery; a send that fails names the delivery it lost in the logs instead of
vanishing; and the job's failure message tells an operator exactly which deliveries need rescuing rather than
leaving them to be discovered ten minutes later by the stranded-delivery sweep.

Links & Resources

  • src/queue/job-dispatch.ts:88-93 — the concurrency rationale the send fan-out ignores
  • src/queue/job-dispatch.ts:379-399 — the notify-evaluate case
  • src/queue/job-dispatch.ts:142-175 — the sibling fan-out that does this correctly (fix(queue): backfill-registered-repos cron fan-out has no per-repo isolation, risking duplicate dispatch on partial failure #8355)
  • src/notifications/service.ts:191-209evaluateNotificationEvent's created-only return
  • src/notifications/service.ts:257-276buildNotificationFeed skips pending rows
  • src/notifications/stranded-delivery-sweep.ts:1-40 — the 10-minute-grace rescue this currently depends on
  • src/queue/map-with-concurrency.ts:1-19 — the bounded worker pool to reuse

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