Skip to content

Fix request registration before synchronous batch delays #9439

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tim-smart merged 5 commits into from
Oct 19, 2026
Merged

Fix request registration before synchronous batch delays #9439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9a4d0ec
test(Request): reproduce zero-delay batch registration race
tim-smart Oct 16, 2026
4e032e3
Fix request registration before synchronous batch delays
tim-smart Oct 16, 2026
6fbfaf1
test(Request): cover synchronous delay reentrancy and pooled reuse
tim-smart Oct 16, 2026
f7c9dc1
Simplify request batch registration and reword changeset
tim-smart Oct 16, 2026
fbc184d
test(Request): reduce synchronous delay coverage to one regression
tim-smart Oct 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/request-zero-delay.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"effect": patch
---

Fix `Effect.request` failing when a resolver's batch delay completes synchronously, such as `RequestResolver.setDelay(0)` under `TestClock`, by registering the request before the delay starts.
23 changes: 17 additions & 6 deletions packages/effect/src/internal/request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import type * as Request from "../Request.ts"
import { makeEntry } from "../Request.ts"
import type { RequestResolver } from "../RequestResolver.ts"
import { Scheduler } from "../Scheduler.ts"
import { exitDie, isEffect } from "./core.ts"
import { exitDie, isEffect, withFiber } from "./core.ts"
import * as effect from "./effect.ts"

/** @internal */
Expand Down Expand Up @@ -103,7 +103,7 @@ interface Batch {
readonly entries: Set<Request.Entry<any>>
readonly delayEffect: Effect<void>
readonly run: Effect<void, unknown>
fiber?: Fiber<void, unknown> | undefined
fiber?: Fiber<unknown, unknown> | undefined
}

const batchPool: Array<Batch> = []
Expand Down Expand Up @@ -143,6 +143,7 @@ const addEntry = <A extends Request.Any>(
}
const key = resolver.batchKey(entry)
batch = batchMap.get(key)
const isNewBatch = batch === undefined
if (!batch) {
if (batchPool.length > 0) {
batch = batchPool.pop()!
Expand All @@ -157,7 +158,11 @@ const addEntry = <A extends Request.Any>(
entrySet: new Set(),
entries: new Set(),
delayEffect: effect.flatMap(
effect.suspend(() => newBatch.resolver.delay),
withFiber((delayFiber) => {
// Reentrant delay callbacks may fill the batch and interrupt this fiber.
newBatch.fiber = delayFiber
return newBatch.resolver.delay
Comment on lines +161 to +164

@effect-janitor effect-janitor Bot Oct 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add focused regression coverage for the reentrant-delay / pooled-batch behavior introduced here. The checkout adds only the single setDelay(0) test (the targeted file runs 35 tests, not the 38 described in the PR), which does not enqueue a request from the delay, interrupt a delay by filling its batch, or reuse a pooled batch. Consequently the early fiber publication here and the removal of the execution-fiber writeback are untested lifecycle changes. Add the reentrant batchN(2) and pooled-reuse ordering cases described in the PR, asserting completion and that interruption targets the correct delay fiber.

@tim-smart tim-smart Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test count and coverage description were stale; I have corrected the PR body to one added regression and 35 passing Request tests. It now explicitly says that reentrant enqueueing and pooled-reuse ordering are not directly covered by the retained test.

I disagree that restoring both removed cases should block this fix. Tim explicitly requested the bare minimum of focused tests. The retained public-API regression reproduces the reported TestClock/microtask failure without the fix and checks both successful completion and exactly one non-empty batch ([[1]]). Existing batch-limit and cancellation coverage remains unchanged. Independent review of this exact head found no concrete gap requiring another case.

The lifecycle safeguards are intentional: publish the running delay fiber before user callbacks can reenter, retain the post-fork assignment for schedulers that yield before publication, guard against touching an already-executed batch, and avoid writing an execution fiber into a recycled batch. The implementation has not changed during the test reduction, including delay-before-collection ordering.

The coverage limitation you identify is real, but the request does not identify an incorrect outcome in the current implementation or a distinct essential regression that warrants reversing the requested reduction. I recommend retaining the single test. If there is a specific failing public-API scenario at this head, please provide it so we can assess one focused behavioral test rather than automatically restoring both lifecycle/ordering cases. No code or tests changed in this assessment.

}),
(_) => runBatch(newBatch)
) as Effect<void>,
run: effect.onExit(
Expand Down Expand Up @@ -190,15 +195,21 @@ const addEntry = <A extends Request.Any>(
batch = newBatch
}
batchMap.set(key, batch)
batch.fiber = effect.runForkWith(fiber.context)(batch.delayEffect, { scheduler: fiber.cache.scheduler })
}

batch.entrySet.add(entry)
batch.entries.add(entry)
if (isNewBatch) {
const delayFiber = effect.runForkWith(fiber.context)(batch.delayEffect, { scheduler: fiber.cache.scheduler })
// A synchronous delay may have already run and recycled this batch.
if (completed || batchMap.get(key) !== batch) return entry
batch.fiber = delayFiber
}

if (batch.resolver.collectWhile(batch.entries)) return entry

batch.fiber!.interruptUnsafe(fiber.id)
batch.fiber = effect.runForkWith(fiber.context)(runBatch(batch), { scheduler: fiber.cache.scheduler })
effect.runForkWith(fiber.context)(runBatch(batch), { scheduler: fiber.cache.scheduler })
return entry
}

Expand All @@ -216,7 +227,7 @@ const removeEntryUnsafe = <A extends Request.Any>(
if (!batch.entries.delete(entry)) return
batch.entrySet.delete(entry)

let fiber: Fiber<void, unknown> | undefined
let fiber: Fiber<unknown, unknown> | undefined
if (batch.entries.size === 0) {
batchMap.delete(key)
fiber = batch.fiber
Expand Down
17 changes: 17 additions & 0 deletions packages/effect/test/Request.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -794,4 +794,21 @@ describe("Request", { concurrent: false }, () => {
assert.strictEqual(delayServiceValue, "provided")
})
)

it.effect("setDelay(0) registers the request before running the batch", () =>
Effect.gen(function*() {
const batches: Array<Array<number>> = []
const resolver = Resolver.makeGrouped<GetNameById, string>({
key: () => "all",
resolver: (entries) =>
Effect.sync(() => {
batches.push(entries.map(({ request }) => request.id))
for (const entry of entries) entry.completeUnsafe(Exit.succeed(String(entry.request.id)))
})
}).pipe(Resolver.setDelay(0), Resolver.batchN(2))

yield* Effect.promise(() => Promise.resolve())
assert.strictEqual(yield* Effect.request(new GetNameById({ id: 1 }), resolver), "1")
assert.deepStrictEqual(batches, [[1]])
}))
})
Loading