review: bound process concurrency in git capture and bootstrap scans#42
review: bound process concurrency in git capture and bootstrap scans#42kridaydave wants to merge 8 commits into
Conversation
Part of the review split of PR Shreyan1#39 (kridaydave:fix/good-first-issues). This PR handles concern Shreyan1#2 only. The async refactor fanned every unit of work out with Promise.all at once. Measured live on a 400-file commit this peaked at 104 concurrent git child processes (the sequential version on main peaks at 1); a 60-file commit ran all 60 diffs simultaneously. The post-commit hook must stay invisible and fail-open, and on constrained CI or very large commits this risks FD/PID exhaustion. Add mapWithConcurrencyLimit (order-preserving) in src/concurrency.ts and use it instead of the unbounded Promise.all fan-outs: - runGitCapture: limit 8 per commit (also restores the documented "newest-file-last" captured ordering that the fan-out lost). - bootstrap.ts findJsonlFiles / findWireJsonlFiles (recursive) and the three discoverSessions scans: limit 16. Split from Shreyan1#39; the other three concerns (drop Antigravity, the captureDisabled cache fix, config-parse dedup) land in their own PRs.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Shreyan1
left a comment
There was a problem hiding this comment.
The concurrency fix itself is genuinely good, Kriday. Bounded pool of 8, results accumulated after the pool so signalsCaptured/captured stop racing, and ordering is deterministic again. That is exactly the shape I was hoping for, and mapWithConcurrencyLimit is clean.
A few things before it can land (specifics inline):
- The new
concurrency.tshas no test. Its whole reason to exist is two guarantees: never more thanlimitin flight, and output order == input order. Please add a test that proves both. - This branch is not based on #41, so it still lists
antigravityinHOOK_ADAPTERSand fails the adapter-wiring parity gate (3 failures). The #1 removal needs to be the base for this, not a sibling, so rebase this on top of #41. - No version bump (user-facing), and the same CodeQL HIGH from the carried #39 base.
Merged with current main the only failures are those 3 parity ones, so once it is rebased on #41 and the concurrency test is in, this is basically there.
| // on constrained CI. This caps the in-flight work at `limit` while preserving | ||
| // the input order of the results, so callers that depended on ordering (e.g. | ||
| // "newest-file-last" capture logs) keep behaving deterministically. | ||
| export async function mapWithConcurrencyLimit<T, R>( |
There was a problem hiding this comment.
Nice, clean worker pool. Two things: (1) this needs its own test, assert peak in-flight is never > limit (a worker that records a live counter) and that results come back in input order (resolve workers out of order to prove it). (2) minor sharp edge: if any worker rejects, Promise.all rejects and the remaining in-flight work is left dangling. Your current callers all catch inside the worker so it is fine today, but one line in the doc comment ("worker must handle its own errors, a throw abandons the pool") would save the next caller.
|
|
||
| // Adapter ids that have a dedicated hook adapter (used to assert parity in tests). | ||
| export const HOOK_ADAPTERS = ['claude-code', 'gemini', 'codex', 'cline', 'kimi']; | ||
| export const HOOK_ADAPTERS = ['claude-code', 'gemini', 'codex', 'cline', 'kimi', 'antigravity']; |
| // FD/PID exhaustion on constrained CI. The pool caps in-flight work and, | ||
| // because mapWithConcurrencyLimit preserves input order, the `captured` | ||
| // array stays newest-file-last deterministically. | ||
| const capturedForCommit = await mapWithConcurrencyLimit( |
There was a problem hiding this comment.
This is the fix and it is right: bounded to 8, and accumulating into captured after the pool restores the newest-file-last ordering the fan-out lost. One small thing, 8 is a magic number, a named const with a one-line "why 8" would help the next person.
Split out of PR #39 (kridaydave:fix/good-first-issues) to address review concern #2.
The async refactor fanned every unit of work out with
Promise.allat once. Measured live on a 400-file commit this peaked at 104 concurrent git child processes (the sequential version on main peaks at 1); a 60-file commit ran all 60 diffs simultaneously. The post-commit hook must stay invisible and fail-open, and on constrained CI or very large commits this risks FD/PID exhaustion.Add
mapWithConcurrencyLimit(order-preserving) insrc/concurrency.tsand use it instead of the unboundedPromise.allfan-outs:runGitCapture: limit 8 per commit (also restores the documented "newest-file-last" captured ordering that the fan-out lost).bootstrap.tsfindJsonlFiles/findWireJsonlFiles(recursive) and the threediscoverSessionsscans: limit 16.The other three concerns (drop Antigravity #1, the
captureDisabledcache fix #3, config-parse dedup #4) land in their own PRs, also split from #39.