Skip to content

refactor(core): two mapWithConcurrency implementations, neither exported; the job-index copy silently no-ops on limit <= 0 #421

Description

@edspencer

Summary

packages/core/src/state contains two independent implementations of mapWithConcurrency. Neither is reachable from the package root, so consumers who need bounded I/O fan-out have to write their own — which is how this keeps happening.

state/job-index.ts:166 state/utils/concurrency.ts:17
clamps limit ❌ Math.min(limit, items.length) only ✅ Math.max(1, Math.min(Math.floor(limit), items.length))
early-returns on empty input ❌ ✅
documented one line full JSDoc
tests none state/utils/__tests__/concurrency.test.ts (order, cap, rejection, limit: 0)
used by job-metadata.ts:10, 402 session-discovery.ts:37, 672

Latent bug in the job-index copy

With no lower clamp, limit <= 0 spawns zero workers:

const workers = Array.from({ length: Math.min(limit, items.length) }, worker);
await Promise.all(workers);
return results;                       // new Array(items.length) — all holes

Promise.all([]) resolves immediately and the function returns an array of undefined, having never called fn. In refreshJobIndex that would silently produce an index of empty entries rather than throwing.

Not reachable today — both call sites pass module constants (IO_CONCURRENCY = 32, HYDRATE_CONCURRENCY = 32) — but it is the kind of thing that becomes reachable the moment someone makes concurrency configurable. utils/concurrency.ts already handles it and has the test for it (concurrency.test.ts:64).

Export surface

Neither copy reaches the package root:

  • state/utils/index.ts re-exports only ./atomic.js, ./path-safety.js, ./reads.js — concurrency.js is missing from the barrel.
  • state/index.ts:34 re-exports exactly one job-index symbol, clearJobIndexCache. Not mapWithConcurrency, not refreshJobIndex, not JobIndexRecord/JobIndexEntry/JobIndexRefresh/RefreshJobIndexOptions.
  • packages/core/package.json has no exports field (only main/types/files), so a deep @herdctl/core/dist/state/job-index.js import does resolve — but that reaches into dist and is not a supported entry point.

Proposal

  1. Delete the copy in job-index.ts; import from state/utils/concurrency.js.
  2. Add export * from "./concurrency.js" to state/utils/index.ts so it reaches the package root.
  3. Downstream, Paddock needs exactly this for its own unbounded Promise.all fan-outs (see edspencer/paddock — /chats/usage opens one read stream per chat, 1,515 concurrently on a production-scale instance, measured at 706 MB peak RSS vs 178 MB at concurrency 16 with no latency cost).

Small, low-risk, and unblocks a downstream fix.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions