Implement all and allSettled for parallel task execution#14
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (13)
WalkthroughAdds parallel task orchestration and settled-mode support: implements executeAll/executeAllSettled, threads a new settled() mode through the builder API, introduces comprehensive types for all/settled, updates public exports, and adds extensive tests covering all/settled behavior, timeouts, cancellation, and type inference. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR successfully implements Phase 10 of the hardtry roadmap, adding parallel task execution with Key Changes:
Implementation Quality:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Builder
participant AllExecution
participant Task1
participant Task2
participant ResultProxy
participant Disposer
User->>Builder: try$.all({ task1, task2 })
Builder->>AllExecution: new AllExecution(config, tasks, settled)
AllExecution->>AllExecution: Setup signals & timeout
AllExecution->>AllExecution: Create disposer stack
par Parallel Task Execution
AllExecution->>Task1: #runTask(task1)
Task1->>Task1: Execute function
Task1->>AllExecution: #handleResult(task1, value)
AllExecution->>ResultProxy: Notify resolvers
and
AllExecution->>Task2: #runTask(task2)
Task2->>ResultProxy: await this.$result.task1
ResultProxy->>AllExecution: #waitForResult(task1)
AllExecution-->>Task2: Return task1 result
Task2->>Task2: Execute with dependency
Task2->>AllExecution: #handleResult(task2, value)
end
AllExecution->>Disposer: disposeAsync()
Disposer->>Disposer: Cleanup resources
AllExecution-->>User: Return all results
Last reviewed commit: 63afe82 |
89be563 to
e56039e
Compare
6bbd3a8 to
2873fc1
Compare
e56039e to
43a6547
Compare
2873fc1 to
663957e
Compare
43a6547 to
ce12ab4
Compare
663957e to
4cf130f
Compare
ce12ab4 to
b198238
Compare
4cf130f to
c973b99
Compare
b198238 to
ef4cffc
Compare
c973b99 to
acd7c5b
Compare
ef4cffc to
dc0308f
Compare
3a82f81 to
884bdb2
Compare
5ac8e16 to
af41db4
Compare
884bdb2 to
7d82ad7
Compare
e6f2861 to
ad1f17c
Compare
ad1f17c to
075c5af
Compare
|
@greptileai review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/lib/__tests__/run.test.ts (1)
6-9: Consider importing the existingsleephelper.A
sleeputility already exists insrc/lib/utils.ts(lines 6-14). While having a local copy in tests provides isolation, importing the shared utility would reduce duplication and ensure consistent behavior.♻️ Optional: Import shared sleep utility
import { describe, expect, it } from "bun:test" import type { TryCtx } from "../types/core" import { CancellationError, Panic, TimeoutError, UnhandledException } from "../errors" import { executeRun } from "../run" +import { sleep } from "../utils" -const sleep = (ms: number) => - new Promise<void>((resolve) => { - setTimeout(resolve, ms) - })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/run.test.ts` around lines 6 - 9, Tests define a local sleep function that duplicates the shared helper; remove the local sleep declaration in run.test.ts and import the existing sleep utility instead (import { sleep } from the shared utils module), updating the top-of-file imports so tests use the single exported sleep function to avoid duplication and ensure consistent behavior with the shared sleep implementation.src/lib/builder.ts (1)
198-221: Consider extracting sharedall()logic.The
all()implementation inRunBuilderis identical toWrappedRunBuilder. While the duplication is acceptable for this PR, consider extracting the shared logic to a helper function in a future refactor to reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/builder.ts` around lines 198 - 221, Extract the duplicated all() logic from RunBuilder and WrappedRunBuilder into a single helper (e.g., runAllHelper) that accepts the builder config (this.#config), the tasks parameter, and the optional AllOptions, then inside the helper perform the settled check and call executeAllSettled/executeAll accordingly (preserve the existing conditional return typing and casts), and replace both classes' all() implementations to delegate to this new helper while keeping the same generic signatures and behavior.src/lib/__tests__/all.test.ts (1)
4-8: Consider importingsleepfrom../utilsinstead of duplicating.A
sleeputility already exists insrc/lib/utils.ts. Importing it would reduce duplication, unless test isolation is intentionally preferred here.♻️ Proposed refactor
import { describe, expect, it } from "bun:test" import { executeAll, executeAllSettled } from "../all" +import { sleep } from "../utils" - -function sleep(ms: number): Promise<void> { - return new Promise((resolve) => { - setTimeout(resolve, ms) - }) -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/all.test.ts` around lines 4 - 8, Remove the duplicate local sleep implementation in the test and use the existing exported sleep utility instead: delete the function sleep(...) in the test, add an import for the exported sleep symbol from the project's utils module, and update any test usages to call that imported sleep function so there is a single shared implementation.src/__tests__/index.test.ts (1)
9-13: Consider importingsleepfrom a shared location.This is the second duplicate of the
sleeputility. Consider either importing from../lib/utilsor creating a shared test utilities module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/index.test.ts` around lines 9 - 13, The test defines a duplicate sleep utility function named sleep; instead of keeping this local copy, remove the local function and import the shared sleep implementation (or a shared test helper) from the existing utility module (e.g., ../lib/utils or the shared test utilities module) where the canonical sleep is defined; update the test file to import sleep and use that symbol so there's a single source of truth for the sleep utility across tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/__tests__/all.test.ts`:
- Around line 267-293: The timing assertion is flaky; either raise the tolerance
or compare against a measured sequential baseline: in the "runs truly in
parallel" test (functions a, b, c, executeAll, sleep) replace the fixed
expect(elapsed).toBeLessThan(120) with a more robust check — e.g., run the same
three tasks serially to compute sequentialElapsed (await a(); await b(); await
c()) and then assert that parallel elapsed is meaningfully less than
sequentialElapsed (for example expect(elapsed).toBeLessThan(sequentialElapsed -
20)), or simply increase the threshold to a safer value like 250ms to avoid CI
timing variance.
---
Nitpick comments:
In `@src/__tests__/index.test.ts`:
- Around line 9-13: The test defines a duplicate sleep utility function named
sleep; instead of keeping this local copy, remove the local function and import
the shared sleep implementation (or a shared test helper) from the existing
utility module (e.g., ../lib/utils or the shared test utilities module) where
the canonical sleep is defined; update the test file to import sleep and use
that symbol so there's a single source of truth for the sleep utility across
tests.
In `@src/lib/__tests__/all.test.ts`:
- Around line 4-8: Remove the duplicate local sleep implementation in the test
and use the existing exported sleep utility instead: delete the function
sleep(...) in the test, add an import for the exported sleep symbol from the
project's utils module, and update any test usages to call that imported sleep
function so there is a single shared implementation.
In `@src/lib/__tests__/run.test.ts`:
- Around line 6-9: Tests define a local sleep function that duplicates the
shared helper; remove the local sleep declaration in run.test.ts and import the
existing sleep utility instead (import { sleep } from the shared utils module),
updating the top-of-file imports so tests use the single exported sleep function
to avoid duplication and ensure consistent behavior with the shared sleep
implementation.
In `@src/lib/builder.ts`:
- Around line 198-221: Extract the duplicated all() logic from RunBuilder and
WrappedRunBuilder into a single helper (e.g., runAllHelper) that accepts the
builder config (this.#config), the tasks parameter, and the optional AllOptions,
then inside the helper perform the settled check and call
executeAllSettled/executeAll accordingly (preserve the existing conditional
return typing and casts), and replace both classes' all() implementations to
delegate to this new helper while keeping the same generic signatures and
behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.context/plan.mdsrc/__tests__/index.test.tssrc/__tests__/types.test.tssrc/index.tssrc/lib/__tests__/all.test.tssrc/lib/__tests__/run-sync.test.tssrc/lib/__tests__/run.test.tssrc/lib/all.tssrc/lib/builder.tssrc/lib/types/all.tssrc/lib/types/builder.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 075c5af582
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
075c5af to
448dcd6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/lib/__tests__/all.test.ts (1)
5-9: Consider importing the sharedsleeputility.Same duplication issue as in
run.test.ts. Thesleephelper exists insrc/lib/utils.ts.♻️ Proposed refactor
+import { sleep } from "../utils" + -function sleep(ms: number): Promise<void> { - return new Promise((resolve) => { - setTimeout(resolve, ms) - }) -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/all.test.ts` around lines 5 - 9, Replace the duplicated local sleep implementation in all.test.ts with the shared utility: remove the local function sleep(ms: number) and add an import for the shared sleep exported from the utils module, then use that imported sleep everywhere (matching how run.test.ts was refactored); ensure the imported symbol name is correct and exported signature (Promise<void>) is used so existing awaits still work.src/lib/builder.ts (1)
113-136: Consider extracting duplicatedall()logic into a shared helper.The
all()method implementation is identical in bothWrappedRunBuilderandRunBuilder. A private helper function could reduce duplication.♻️ Suggested extraction pattern
function executeAllWithConfig<T extends TaskRecord, C, Settled extends boolean>( config: BuilderConfig, tasks: T & ThisType<InferredTaskContext<T>>, options?: AllOptions<T, C> ): Promise<Settled extends true ? AllSettledResult<T> : { [K in keyof T]: TaskResult<T[K]> } | C> { if (config.settled) { return executeAllSettled(config, tasks) as any } if (options) { return executeAll(config, tasks, options) as any } return executeAll(config, tasks) as any }Also applies to: 202-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/builder.ts` around lines 113 - 136, Duplicate logic in the all<T...>() method of both RunBuilder and WrappedRunBuilder should be extracted into a single helper: create a private/shared function (e.g., executeAllWithConfig) that accepts the BuilderConfig (this.#config), tasks, and optional AllOptions and returns the same conditional Promise type; inside the helper call executeAllSettled(config, tasks) when config.settled is true, otherwise call executeAll(config, tasks, options) if options is provided or executeAll(config, tasks) when not, then replace both class-level all methods to delegate to this new helper (referencing the existing symbols all, WrappedRunBuilder, RunBuilder, executeAll, executeAllSettled, and BuilderConfig).src/lib/all.ts (2)
182-189: Consider clearing resolver queue after rejection as well.Same cleanup suggestion applies to
#handleError.♻️ Proposed refactor
if (rejected) { for (const [, reject] of rejected) { reject(error) } + this.#resolvers.delete(taskName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/all.ts` around lines 182 - 189, The rejection branch in the function that looks up const rejected = this.#resolvers.get(taskName) currently iterates and calls each reject(error) but leaves the resolver queue intact; after rejecting, remove the entry from this.#resolvers (e.g., this.#resolvers.delete(taskName)) so pending resolvers are cleared to avoid memory leaks and duplicate handling, and apply the same cleanup in the `#handleError` method—after rejecting any stored resolvers there, delete the corresponding key from this.#resolvers or clear that resolver collection.
165-172: Consider clearing resolver queue after fulfillment to aid garbage collection.After resolving all waiters in
#handleResult, the resolver array remains in the map. While this doesn't affect correctness (the execution is short-lived), clearing the entry allows earlier GC of the callback closures.♻️ Proposed refactor
const fulfilled = this.#resolvers.get(taskName) if (fulfilled) { for (const [resolve] of fulfilled) { resolve(value) } + this.#resolvers.delete(taskName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/all.ts` around lines 165 - 172, In `#handleResult`, after iterating and calling each resolve from the array retrieved via this.#resolvers.get(taskName), remove the map entry so the resolver closures can be GC'd — e.g., after the for loop call this.#resolvers.delete(taskName) (or set the array to an empty array) to clear the resolver queue associated with taskName; keep the existing resolve(value) loop unchanged.src/lib/__tests__/run.test.ts (1)
6-9: Consider importing the sharedsleeputility instead of duplicating it.A
sleephelper already exists insrc/lib/utils.ts(lines 6-14). Importing it would reduce duplication and ensure consistent behavior across tests.♻️ Proposed refactor
-const sleep = (ms: number) => - new Promise<void>((resolve) => { - setTimeout(resolve, ms) - }) +import { sleep } from "../utils"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/run.test.ts` around lines 6 - 9, Remove the duplicated local sleep implementation in run.test.ts and import the shared sleep helper from the project's utils (the exported sleep function in utils.ts); update the top-of-file imports to include sleep, delete the local const sleep = ... declaration, and ensure existing tests continue to call sleep as before so they use the centralized utility.src/__tests__/index.test.ts (1)
9-13: Consider importing the sharedsleeputility.Same duplication as other test files. Import from
src/lib/utils.tsfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/index.test.ts` around lines 9 - 13, Replace the local function sleep in this test with the shared exported sleep utility: remove the function sleep(...) declaration and add an import for the shared sleep helper (use the project's utils module export named sleep), then update any calls to use the imported sleep; this keeps tests consistent and avoids duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/__tests__/all.test.ts`:
- Around line 434-456: The "timeout config" test that calls executeAllSettled is
placed inside the executeAll describe block; move the entire it(...) block that
references executeAllSettled (the async test named "rejects with TimeoutError
when tasks exceed configured timeout") out of the executeAll describe and into
the executeAllSettled describe so the test suite matches the function under test
and removes the duplicate "timeout config" describe in executeAll.
---
Nitpick comments:
In `@src/__tests__/index.test.ts`:
- Around line 9-13: Replace the local function sleep in this test with the
shared exported sleep utility: remove the function sleep(...) declaration and
add an import for the shared sleep helper (use the project's utils module export
named sleep), then update any calls to use the imported sleep; this keeps tests
consistent and avoids duplication.
In `@src/lib/__tests__/all.test.ts`:
- Around line 5-9: Replace the duplicated local sleep implementation in
all.test.ts with the shared utility: remove the local function sleep(ms: number)
and add an import for the shared sleep exported from the utils module, then use
that imported sleep everywhere (matching how run.test.ts was refactored); ensure
the imported symbol name is correct and exported signature (Promise<void>) is
used so existing awaits still work.
In `@src/lib/__tests__/run.test.ts`:
- Around line 6-9: Remove the duplicated local sleep implementation in
run.test.ts and import the shared sleep helper from the project's utils (the
exported sleep function in utils.ts); update the top-of-file imports to include
sleep, delete the local const sleep = ... declaration, and ensure existing tests
continue to call sleep as before so they use the centralized utility.
In `@src/lib/all.ts`:
- Around line 182-189: The rejection branch in the function that looks up const
rejected = this.#resolvers.get(taskName) currently iterates and calls each
reject(error) but leaves the resolver queue intact; after rejecting, remove the
entry from this.#resolvers (e.g., this.#resolvers.delete(taskName)) so pending
resolvers are cleared to avoid memory leaks and duplicate handling, and apply
the same cleanup in the `#handleError` method—after rejecting any stored resolvers
there, delete the corresponding key from this.#resolvers or clear that resolver
collection.
- Around line 165-172: In `#handleResult`, after iterating and calling each
resolve from the array retrieved via this.#resolvers.get(taskName), remove the
map entry so the resolver closures can be GC'd — e.g., after the for loop call
this.#resolvers.delete(taskName) (or set the array to an empty array) to clear
the resolver queue associated with taskName; keep the existing resolve(value)
loop unchanged.
In `@src/lib/builder.ts`:
- Around line 113-136: Duplicate logic in the all<T...>() method of both
RunBuilder and WrappedRunBuilder should be extracted into a single helper:
create a private/shared function (e.g., executeAllWithConfig) that accepts the
BuilderConfig (this.#config), tasks, and optional AllOptions and returns the
same conditional Promise type; inside the helper call executeAllSettled(config,
tasks) when config.settled is true, otherwise call executeAll(config, tasks,
options) if options is provided or executeAll(config, tasks) when not, then
replace both class-level all methods to delegate to this new helper (referencing
the existing symbols all, WrappedRunBuilder, RunBuilder, executeAll,
executeAllSettled, and BuilderConfig).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.context/plan.mdsrc/__tests__/ctx-features.test.tssrc/__tests__/index.test.tssrc/__tests__/types.test.tssrc/index.tssrc/lib/__tests__/all.test.tssrc/lib/__tests__/run-sync.test.tssrc/lib/__tests__/run.test.tssrc/lib/all.tssrc/lib/builder.tssrc/lib/types/all.tssrc/lib/types/builder.ts
💤 Files with no reviewable changes (1)
- src/tests/ctx-features.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/index.ts
- src/lib/types/builder.ts
- .context/plan.md
448dcd6 to
63afe82
Compare
|
@coderabbitai review |
|
✅ Actions performedReview triggered.
|

This pull request implements the
allandallSettledfunctionality for parallel task execution with dependency resolution.Key Features Added:
all()runs tasks concurrently and returns when all succeed or first failssettled().all()waits for all tasks to complete regardless of failures, returningSettledResultobjectsthis.$result.taskNameall()supports optionalcatchhandlers with context about failed tasks and partial results$disposerand$signalto tasks for cleanup and cancellationAPI Examples:
The implementation includes comprehensive test coverage and proper TypeScript inference for task return types and dependency relationships.