Repository navigation
perf: bound file IO concurrency - #1948
Conversation
📝 WalkthroughWalkthroughChangesBounded file I/O
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/attachments.ts`:
- Around line 1068-1075: Update the attachment helper maybe so it checks the
optional AbortSignal with throwIfAborted() before evaluating or invoking the
producer f, then passes the signal to raceAbort as before. Preserve existing
result and timeout handling, and add or update tests verifying a pre-aborted
signal rejects without invoking f.
In `@src/utils/boundedAsync.ts`:
- Around line 66-80: Update the worker loop in boundedAsync so the first mapper
rejection records shared failure state before rethrowing, and make workers stop
claiming queued items once that state is set. Preserve abort handling and
successful scheduling behavior, then add focused regression coverage proving
queued mapper work does not start after the first rejection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce310d81-9c06-4b08-a3c2-6be1668345d3
📒 Files selected for processing (6)
src/services/teamMemorySync/index.test.tssrc/services/teamMemorySync/index.tssrc/utils/attachments.performance.test.tssrc/utils/attachments.tssrc/utils/boundedAsync.test.tssrc/utils/boundedAsync.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
Files:
src/services/teamMemorySync/index.test.tssrc/utils/boundedAsync.tssrc/utils/attachments.performance.test.tssrc/utils/boundedAsync.test.tssrc/services/teamMemorySync/index.tssrc/utils/attachments.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.src/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
src/services/teamMemorySync/index.test.tssrc/utils/boundedAsync.tssrc/utils/attachments.performance.test.tssrc/utils/boundedAsync.test.tssrc/services/teamMemorySync/index.tssrc/utils/attachments.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/teamMemorySync/index.test.tssrc/utils/boundedAsync.tssrc/utils/attachments.performance.test.tssrc/utils/boundedAsync.test.tssrc/services/teamMemorySync/index.tssrc/utils/attachments.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/services/teamMemorySync/index.test.tssrc/utils/attachments.performance.test.tssrc/utils/boundedAsync.test.ts
🔇 Additional comments (6)
src/utils/boundedAsync.ts (1)
1-65: LGTM!Also applies to: 81-85
src/utils/boundedAsync.test.ts (1)
1-138: LGTM!src/utils/attachments.ts (1)
122-122: LGTM!Also applies to: 768-769, 1103-1107, 1995-2136, 2291-2375, 3243-3246
src/utils/attachments.performance.test.ts (1)
1-90: LGTM!src/services/teamMemorySync/index.ts (1)
29-126: LGTM!Also applies to: 606-755, 772-846, 1349-1354
src/services/teamMemorySync/index.test.ts (1)
1-174: LGTM!
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Findings
F1 — Major: 1s abort timeout now enforced on attachment producers (real behavior change)
- Evidence (base):
getAttachmentsalready didconst timeoutId = setTimeout(ac => ac.abort(), 1000, abortController)
(now ~attachments.ts:795). In the base commit2448ea9…the oldmaybe(label, f)took no signal
(base line 1043) andprocessAtMentionedFiles/getChangedFilesused barePromise.allwith no
throwIfAborted. So the 1s timer was effectively inert for those producers. - Evidence (this PR):
maybeTimedAttachment(attachments.ts:804) calls
maybe(label, f, context.abortController.signal)→maybe(:1068) now does
signal?.throwIfAborted()(:1075) thenawait raceAbort(f(), signal, …); and
processAtMentionedFilesWithDependencies(:2073,:2087) +getChangedFiles(:2312,:2329)
calltoolUseContext.abortController.signal.throwIfAborted()per item. So the 1s timeout is now live
for:at_mentioned_files,mcp_resources,skill_discovery,changed_files,nested_memory,
dynamic_skill,skill_listing. - Effect: under slow FS / large diffs / large repos any of these exceeding 1s now hard-resolves to
[](the per-producermaybeswallows the abort and returns[], and the abort is not logged —
attachments.ts:1104guards logging withif (!isAbortError(e))). User-visible content (the entire
changed-files attachment, or all@-mentioned files) can be silently dropped where it was previously
always delivered. - Note:
maybeAttachment(no signal,:800) is correctly used for non-IO/CPU-bound producers, so
only the timing-sensitive IO producers are affected. - Recommendation: decide intent. If intended, document it, add a log/telemetry line when an
attachment is dropped by the timeout, and consider exempting local-fs producers or returning partial
results; if not intended, raise/remove the 1s ceiling for these producers. Either way, fix the PR
description.
F2 — Major: Missing focused regression coverage for the changed attachment paths
getChangedFileswas converted tomapWithConcurrency+ per-itemthrowIfAborted
(attachments.ts:2290-2372) but has no tests (the newattachments.performance.test.tsonly
exercisesprocessAtMentionedFilesandmaybe).mapWithConcurrencyabort mid-flight (abort fires while mappers run) is never tested end-to-end;
only pre-abort and the standaloneraceAbortpath are covered.processAtMentionedFilesWithDependenciesis only exercised via the perf test with a never-aborted
controller, so itsthrowIfAborted(:2073,:2087) andif (isAbortError(err)) throw err
(:2130) wiring is unverified.- Per AGENTS.md ("Add or update tests when behavior changes") and the test-path review rubric ("Block
when risky runtime changes lack focused regression coverage"), these gaps should be closed. The
helper and team-memory already have good coverage.
F3 — Low: Abort-reason typing robustness hole in throwIfAborted call sites
raceAbort/mayberoute theraceAbortrejection throughabortReason, but the per-item
throwIfAborted()calls (attachments.ts:2073, 2087, 2312, 2329) and the worker loop
(boundedAsync.ts:72) rethrowsignal.reasonverbatim.isAbortError(src/utils/errors.ts:28) is true only forAbortError/APIUserAbortError/anError
whose.name === 'AbortError'. A non-Erroror non-AbortErrorreason (e.g.
controller.abort('cancelled')orcontroller.abort(new TypeError())) is therefore not recognized.- Consequence: in
processAtMentionedFilesWithDependencies(catch:2129-2130) andgetChangedFiles
(catch ~:2359) the guardif (isAbortError(err)) throw errwould NOT rethrow such a reason; it
falls through toreturn null, so the worker'sfailedflag stays false and remaining files keep
being processed — cancellation is silently ignored.maybe(:1104) would also mis-log a
cancellation as a real error. - In practice the PR's own wiring (
setTimeout(ac => ac.abort(), 1000, abortController)) yields a
DOMExceptionwithname === 'AbortError', so the common path is correct — this is a narrow
robustness gap, not a current bug. Recommend routing everythrowIfAborted()throughabortReason
(or anisAbortError-safe wrapper) and adding a test with a non-Errorreason.
F4 — Low: Concurrent abort can mask a genuine mapper error
- In
raceAbort(boundedAsync.ts:24-45), if theabortevent fires and rejects before the inner
promise rejects, the genuine mapper error is swallowed and surfaces as an abort. Generally acceptable
(abort precedence), but consider giving inner rejection precedence when both occur in the same tick.
Nits / Minor
processAtMentionedFilesreturnsresults.filter(Boolean)(:2137) whilegetChangedFilesuses the
saferresults.filter(result => result != null)(:2366).Attachmentobjects are always truthy so
no bug, but align for consistency.- Perf-contract
waitForpredicates assert exactactiveReads === CONCURRENCY
(teamMemorySync/index.test.ts:77,attachments.performance.test.ts:596,boundedAsync.test.ts);
airtight today (gated mappers) but>=is more robust to refactors. boundedAsync.test.tsregisters a globalprocess.on('unhandledRejection')(~:134); low risk under
Bun per-file isolation +finallycleanup, but prefer a scoped captured-rejection assertion.- Team-memory logs: oversized-file log now uses
relPathinstead ofentry.name(more informative) and
the secret-skip log changed em-dash—to hyphen-. Cosmetic only.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/attachments.ts (1)
804-817: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThread the abort signal into
maybeAttachment.maybe()only enforces the 1s timeout when a signal is passed;maybeAttachmentcurrently callsmaybe(label, f)withoutabortController.signal, so a slow attachment producer can stallgetAttachments().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/attachments.ts` around lines 804 - 817, Update the maybeAttachment helper to pass abortController.signal into maybe(label, f), ensuring attachment producers are subject to the existing timeout and cancellation behavior while preserving the current attachment collection flow.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/utils/attachments.ts`:
- Around line 804-817: Update the maybeAttachment helper to pass
abortController.signal into maybe(label, f), ensuring attachment producers are
subject to the existing timeout and cancellation behavior while preserving the
current attachment collection flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 543f139f-ffcc-4219-9d74-5daddb427cc9
📒 Files selected for processing (5)
src/services/teamMemorySync/index.test.tssrc/utils/attachments.performance.test.tssrc/utils/attachments.tssrc/utils/boundedAsync.test.tssrc/utils/boundedAsync.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
Files:
src/services/teamMemorySync/index.test.tssrc/utils/attachments.performance.test.tssrc/utils/boundedAsync.tssrc/utils/boundedAsync.test.tssrc/utils/attachments.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.src/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
src/services/teamMemorySync/index.test.tssrc/utils/attachments.performance.test.tssrc/utils/boundedAsync.tssrc/utils/boundedAsync.test.tssrc/utils/attachments.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/teamMemorySync/index.test.tssrc/utils/attachments.performance.test.tssrc/utils/boundedAsync.tssrc/utils/boundedAsync.test.tssrc/utils/attachments.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/services/teamMemorySync/index.test.tssrc/utils/attachments.performance.test.tssrc/utils/boundedAsync.test.ts
🔇 Additional comments (7)
src/utils/boundedAsync.ts (2)
14-21: LGTM!Also applies to: 70-70, 81-81
59-103: LGTM!Confirmed the previously-flagged scheduling bug (queued items starting after a mapper rejection) is fixed via the
failedflag — workers stop claiming new indices once set.src/utils/boundedAsync.test.ts (1)
84-128: LGTM!Good coverage: non-Error abort reason normalization, and mid-flight abort correctly stopping queued (not yet started) work.
src/utils/attachments.ts (2)
1068-1112: LGTM!
maybe()'s own abort handling (check-then-race, abort errors excluded fromlogError/logAntError) is correctly implemented. Its correctness is undermined only by callers not forwardingsignal(see comment on themaybeAttachmentwrapper).
1996-2134: LGTM!Dependency-injected
AtMentionedFileDeps/ChangedFileDepsplusmapWithConcurrency(..., ATTACHMENT_FILE_IO_CONCURRENCY, ...)integration preserves ordering, per-file error containment (returnsnullrather than propagating), and permission/path checks. Test exposure via__testis appropriately scoped.Also applies to: 2281-2403, 3268-3272
src/utils/attachments.performance.test.ts (1)
61-220: LGTM!Bounded-concurrency assertions (
>=polling instead of brittle===) and the "run in-flight work to completion on abort" contract are well covered.src/services/teamMemorySync/index.test.ts (1)
40-174: LGTM!Deterministic truncation test after secret filtering is a good regression guard for the ordering guarantee
mapWithConcurrencyrelies on, and the path-traversal skip test matches the stated preservation requirement.
* perf: bound file IO concurrency * fix: honor abort and failure gates in bounded IO * fix: preserve attachment timeout behavior --------- Co-authored-by: Gautam Manchandani <gautammanch@Gautams-MacBook-Air.local>
Summary
Impact
Summary by CodeRabbit