Skip to content

fix(queue): bound reconcileLiveDuplicateSiblings' live per-sibling fan-out#7610

Closed
glorydavid03023 wants to merge 1 commit into
JSONbored:mainfrom
glorydavid03023:fix/duplicate-siblings-bounded-fanout
Closed

fix(queue): bound reconcileLiveDuplicateSiblings' live per-sibling fan-out#7610
glorydavid03023 wants to merge 1 commit into
JSONbored:mainfrom
glorydavid03023:fix/duplicate-siblings-bounded-fanout

Conversation

@glorydavid03023

Copy link
Copy Markdown
Contributor

Closes #5835

Summary

reconcileLiveDuplicateSiblings (src/queue/duplicate-detection.ts) live-reconciles a PR's duplicate-cluster siblings before the winner is elected. When a PR has overlapping open siblings sharing a linked issue, it fired one live GitHub REST call per sibling through an unbounded Promise.all(overlapping.map(...)) — every open sibling got a simultaneous fetchLivePullRequestState in a single pass.

A popular linked issue can accumulate dozens of duplicate PRs, so one webhook delivery could burst that many concurrent REST calls against the one ~5000/hr bucket every managed repo shares through a single installation — exactly the "many live per-item GitHub reads from one delivery" pressure the module's own doc comments already worry about.

The fix

The fan-out now runs through mapWithConcurrency at DUPLICATE_SIBLING_LIVE_CHECK_CONCURRENCY = 10. That's the same bound src/queue/processors.ts — the file duplicate-detection.ts was extracted from — already applies to the two comparable per-item live-check fan-outs:

  • CONTRIBUTOR_CAP_LIVE_CHECK_CONCURRENCY = 10
  • GLOBAL_OPEN_ITEM_LIVE_CHECK_CONCURRENCY = 10

So all three bounded fan-outs now use one consistent number rather than each picking their own. Every sibling is still verified — the cap bounds concurrency, not coverage — so the reconcile's fail-open semantics (a sibling is dropped only on a positive "not open" confirmation) are entirely unchanged.

Why mapWithConcurrency moved to a new module

The naive fix — import mapWithConcurrency from processors.ts — would create a circular import. duplicate-detection.ts deliberately does not import processors.ts, and its own header comment records why: it inlines an admission-key wrapper rather than importing it back, precisely because "importing it back would have made this file and processors.ts circularly dependent on each other."

So the helper moves to a neutral src/queue/concurrency.ts that both files depend on — sharing it without reintroducing the cycle, and without growing a third private copy of the same worker-pool loop. processors.ts re-exports it, so its existing public surface is byte-identical. This is the same shim shape processors.ts already uses for transient-locks.ts and signal-snapshot.ts.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves — see the closing reference at the top of this body.

Validation

  • git diff --check
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires >=99% coverage of the lines AND branches changed
  • npm run engine-parity:drift-check
  • npm run docs:drift-check
  • npm run selfhost:validate-observability
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

Patch coverage: 100% (11/11 executable changed lines, zero partial branches), measured by intersecting the lcov report against this diff's added lines.

No behavior change: the 890 tests in the existing queue suites (queue, queue-2queue-5, queue-lifecycle-guards, reconcile-live-duplicate-siblings) pass unchanged.

The regression test (reconcile-live-duplicate-siblings.test.ts) drives 40 overlapping siblings and asserts peak simultaneous fetches is exactly 10 (it was 40 before), that all 40 are still verified, and that the result is identical to the unbounded version. New queue-concurrency.test.ts gives the moved helper direct branch coverage: the concurrency cap is honoured and saturated, input order is preserved when mappers finish out of order, empty input resolves without calling the mapper, a zero/negative concurrency is clamped to a single worker (so a zero-worker pool can't hang), and a mapper rejection propagates.

If any required check was skipped, explain why:

  • None skipped.

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.
  • API/OpenAPI/MCP behavior is updated and tested where needed.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks.
  • Visible UI changes include a UI Evidence section. Not applicable — backend only.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

UI Evidence

Not applicable — this is a backend queue/rate-limit change. No UI, frontend, docs, or extension surface is touched.

Notes

  • The bound (10) is not a new magic number — it is deliberately the same constant the two sibling fan-outs in processors.ts already use, so a future change to the installation REST budget can be reasoned about in one place.

…n-out (JSONbored#5835)

reconcileLiveDuplicateSiblings fired one live fetchLivePullRequestState per overlapping duplicate
sibling through an unbounded `Promise.all(overlapping.map(...))`. A popular linked issue can hold
dozens of duplicate PRs, so a single webhook delivery could burst that many simultaneous GitHub REST
calls against the ONE ~5000/hr bucket every managed repo shares through one installation -- exactly
the "many live per-item GitHub reads from one delivery" shape processors.ts already bounds twice
(CONTRIBUTOR_CAP_LIVE_CHECK_CONCURRENCY and GLOBAL_OPEN_ITEM_LIVE_CHECK_CONCURRENCY, both 10).

The fan-out now runs through mapWithConcurrency at DUPLICATE_SIBLING_LIVE_CHECK_CONCURRENCY = 10,
matching those two so the three bounded fan-outs stay consistent. Every sibling is still verified --
the cap bounds concurrency, not coverage -- so the reconcile's fail-open semantics are unchanged.

mapWithConcurrency moves from processors.ts to a new src/queue/concurrency.ts rather than being
imported across: duplicate-detection.ts deliberately does not import processors.ts, and its own header
records why (it inlines an admission-key wrapper for the same reason -- importing back would make the
two circularly dependent). A neutral module both can depend on shares the helper without reintroducing
that cycle or growing a third private copy of the same worker-pool loop. processors.ts re-exports it,
so its existing surface is unchanged -- the same shim shape it already uses for transient-locks.ts and
signal-snapshot.ts.

Tested: a regression test drives 40 overlapping siblings and asserts peak in-flight fetches is exactly
10 (it was 40) while all 40 are still verified and the result is unchanged; plus direct branch coverage
for the moved helper, including order preservation when mappers finish out of order, the empty-input
case, and the clamp that keeps a zero/negative concurrency from hanging on a zero-worker pool.
@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 21, 2026
@loopover-orb

loopover-orb Bot commented Jul 21, 2026

Copy link
Copy Markdown

Caution

🛑 LoopOver review result - reject/close recommended

Review updated: 2026-07-21 01:32:58 UTC

5 files · 1 AI reviewer · 1 blocker · CI pending · dirty

🛑 Suggested Action - Reject/Close

Review summary
This PR extracts `mapWithConcurrency` from processors.ts into a neutral src/queue/concurrency.ts module and uses it to bound the previously-unbounded `Promise.all(overlapping.map(...))` fan-out in `reconcileLiveDuplicateSiblings`, capping it at 10 concurrent live GitHub fetches, matching the two existing caps in processors.ts. The move avoids a circular import between duplicate-detection.ts and processors.ts (documented reason for the original inlining pattern), and processors.ts re-exports the helper so its public surface is unchanged. The fix is correct, well-tested (concurrency-bounding, ordering, empty-input, clamping, and rejection-propagation tests for the helper, plus a regression test asserting peak in-flight ≤10 while still verifying all 40 siblings), and closes the linked issue #5835.

Nits — 3 non-blocking
  • The regression test in test/unit/reconcile-live-duplicate-siblings.test.ts asserts `peakInFlight` equals exactly 10 via a shared `setTimeout(0)` delay for all 40 fetches, which is a slightly timing-dependent way to prove saturation but is low-risk given it mirrors the same pattern already used in queue-concurrency.test.ts.
  • processors.ts re-exports `mapWithConcurrency` purely for backward compatibility with existing callers in that file — worth confirming no callers there could instead import directly from ./concurrency.ts going forward to avoid the indirection, though this is a style preference not a defect.
  • Consider having processors.ts's internal callers of mapWithConcurrency import directly from ./concurrency to eventually drop the re-export shim, though this is optional cleanup, not required for this PR.

Why this is blocked

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.

Decision drivers

  • ❌ Code review — 1 blocker (1 reviewer)
  • ❌ Gate result — Blocking (Repo-configured hard blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #5835
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: 207 registered-repo PR(s), 121 merged, 15 issue(s).
Contributor context ✅ Confirmed Gittensor contributor glorydavid03023; Gittensor profile; 207 PR(s), 15 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff replaces the unbounded Promise.all fan-out in reconcileLiveDuplicateSiblings with mapWithConcurrency capped at 10 (moved to a neutral concurrency.ts module to avoid the documented circular-import constraint), matching the requested order of magnitude and preserving fail-open semantics and signature/flag gating.

Review context
  • Author: glorydavid03023
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, JavaScript, TypeScript, Rust, C++, Kotlin, MDX, Ruby
  • Official Gittensor activity: 207 PR(s), 15 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
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 <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> 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 commented Jul 21, 2026

Copy link
Copy Markdown

LoopOver is closing this pull request on the maintainer's behalf (conflicts with the base branch — resolve and open a fresh PR; No linked issue detected). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

@loopover-orb loopover-orb Bot closed this Jul 21, 2026
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(queue): reconcileLiveDuplicateSiblings fans out unbounded GitHub fetches per duplicate-cluster sibling

1 participant