Skip to content

queue(map-with-concurrency): dedupe src/github/backfill.ts's private copy against the canonical src/queue/map-with-concurrency.ts #10289

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

src/queue/map-with-concurrency.ts exports the canonical bounded-concurrency fan-out helper used
throughout ORB's queue subsystem:

export async function mapWithConcurrency<T, R>(
  items: T[],
  concurrency: number,
  mapper: (item: T) => Promise<R>,
): Promise<R[]> {
  const results: R[] = new Array(items.length);
  let nextIndex = 0;
  const workerCount = Math.max(1, Math.min(concurrency, items.length || 1));
  await Promise.all(
    Array.from({ length: workerCount }, async () => {
      while (nextIndex < items.length) {
        const index = nextIndex;
        nextIndex += 1;
        results[index] = await mapper(items[index] as T);
      }
    }),
  );
  return results;
}

It is imported and used by src/signals/focus-manifest-loader.ts, src/queue/duplicate-detection.ts,
src/queue/processors.ts, and src/queue/patchless-secret-scan.ts.

src/github/backfill.ts independently defines its own private, unexported, byte-for-byte
functionally identical copy
of the exact same helper, at the bottom of the file:

async function mapWithConcurrency<T, R>(items: T[], concurrency: number, mapper: (item: T, index: number) => Promise<R>): Promise<R[]> {
  const results: R[] = new Array(items.length);
  let nextIndex = 0;
  const workerCount = Math.max(1, Math.min(concurrency, items.length || 1));
  await Promise.all(
    Array.from({ length: workerCount }, async () => {
      while (nextIndex < items.length) {
        const index = nextIndex;
        nextIndex += 1;
        results[index] = await mapper(items[index] as T, index);
      }
    }),
  );
  return results;
}

The only textual difference is that backfill.ts's private copy also threads an index argument
into mapper. Every one of backfill.ts's own 9 call sites of this local function (search the file
for mapWithConcurrency() ignores that second argument — none of them declare a mapper with an
index parameter — so the two implementations are behaviorally identical for every real caller in
this codebase today.

This is exactly the class of defect this repo has already paid for once: src/review/content-lane/security-scan.ts's
own header comment documents #4608, where a hand-duplicated copy of the secret-detection primitives
"already caused two independent, currently-live drifts" before being consolidated into a single shared
module (src/review/secret-patterns.ts). mapWithConcurrency is now in that same position — two
independent copies of the same fan-out logic that can silently drift apart (a fix or a subtle
concurrency change applied to one and forgotten in the other) with nothing enforcing they stay in sync.

Requirements

  • src/github/backfill.ts must import mapWithConcurrency from ../queue/map-with-concurrency
    instead of defining its own private copy.
  • The private mapWithConcurrency function currently defined at the bottom of src/github/backfill.ts
    must be deleted entirely.
  • The canonical mapWithConcurrency in src/queue/map-with-concurrency.ts must be extended so its
    mapper parameter type accepts an optional second index: number argument (i.e.
    mapper: (item: T, index: number) => Promise<R>), and the implementation must pass index as the
    second argument on every call to mapper — this is what lets backfill.ts's 9 call sites switch to
    the shared import with zero call-site changes, since none of them currently uses the index
    argument but the type must remain a strict superset of both today's shapes (a caller that declares a
    single-argument mapper, e.g. async (item) => ..., must continue to type-check and behave exactly as
    it does today — TypeScript allows a callback with fewer declared parameters than the function type
    provides, so this is not a breaking change for any existing caller).
  • Every existing call site of mapWithConcurrency in src/github/backfill.ts (there are multiple —
    grep the file for mapWithConcurrency() must continue to compile and behave identically after the
    switch to the shared import, with no call-site logic changes beyond the import itself.
  • Do not change mapWithConcurrency's exported name, its location (src/queue/map-with-concurrency.ts
    stays the single source of truth), or its behavior for any existing caller in
    src/signals/focus-manifest-loader.ts, src/queue/duplicate-detection.ts, src/queue/processors.ts,
    or src/queue/patchless-secret-scan.ts.

Deliverables

  • src/queue/map-with-concurrency.ts's exported mapWithConcurrency accepts and passes an
    index: number second argument to mapper, verified by a new test asserting the indexes a
    mapper receives, across multiple items and a concurrency setting greater than 1, are exactly
    0, 1, 2, ... in item order (not worker-arrival order).
  • src/github/backfill.ts no longer defines its own mapWithConcurrency function anywhere in the
    file (verified by grep -c "^async function mapWithConcurrency" src/github/backfill.ts returning
    0) and instead imports it from ../queue/map-with-concurrency.
  • Every pre-existing call site of mapWithConcurrency inside src/github/backfill.ts is unchanged
    in behavior — the existing test/unit/** backfill test suite (search for the tests exercising
    src/github/backfill.ts's repo/PR/issue/label fan-out paths) passes unmodified against the new
    shared-import version with no test assertions weakened or removed.

Both Deliverables above are required in the same PR — this is not a narrowly-scoped issue with a
follow-up; there is no legitimate reason to land the type change without also deleting the duplicate,
or vice versa.

Test Coverage Requirements

This repo's Codecov patch gate requires 99%+ patch coverage on every changed line and branch under
src/**. The new index-threading behavior in src/queue/map-with-concurrency.ts needs a dedicated
new test (see Deliverable 1) — do not rely on backfill.ts's existing tests alone to cover it, since
none of them currently exercise the index argument. The deletion of the private duplicate in
src/github/backfill.ts does not by itself need a new test (its removal is verified by the existing
backfill test suite continuing to pass), but do not skip re-running that suite locally before opening
the PR — a signature mismatch here would silently miscompile call sites that pass a two-argument
mapper.

Expected Outcome

There is exactly one implementation of the bounded-concurrency fan-out helper in this codebase,
src/queue/map-with-concurrency.ts, and src/github/backfill.ts consumes it like every other caller.
A future change to the fan-out logic (e.g. a bug fix, an instrumentation hook, a different worker
scheduling strategy) only ever needs to be made in one place instead of two.

Links & Resources

  • src/queue/map-with-concurrency.ts — the canonical implementation to extend.
  • src/github/backfill.ts — delete the private duplicate near the bottom of the file (search for
    async function mapWithConcurrency), and update the import + call sites.
  • src/review/content-lane/security-scan.ts's header comment (the #4608 note) — the precedent for
    why this codebase treats hand-duplicated shared logic as a real defect worth consolidating, not just
    a style nit.

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