[codex] Fix benchmark harness and runtime trace regressions#212
[codex] Fix benchmark harness and runtime trace regressions#212briancavalier wants to merge 5 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb14bbe74e
ℹ️ 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".
| */ | ||
| export const runTask = <const R>(f: Fx<Async | HandlerCapture<string> | Interrupt, R>, options: RunForkOptions = {}): Task<R, never> => { | ||
| return runFork(f.pipe(provideAll({})), { | ||
| export const runTask = <const R>(f: Fx<Async | HandlerCapture<string> | Interrupt | Get<Record<PropertyKey, unknown>>, R>, options: RunForkOptions = {}): Task<R, never> => { |
There was a problem hiding this comment.
Widening runTask/runPromise/run to accept Get<Record<PropertyKey, unknown>> removes a useful type-safety boundary: programs that require context (for example fx(function* ({ userId }: { userId: string }) { ... })) now type-check at the runtime boundary even when no provider is installed, and they will be resumed with {}. That turns what used to be a compile-time error into runtime undefined values (or crashes) in ordinary user code, which is a behavioral regression for environment-dependent Fx programs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a06ec82848
ℹ️ 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".
| } else if (isEffect(ir.value) && ir.value._fxEffectId === EnvEffectId) { | ||
| ir = resumeWithRuntimeContext(i, runtimeContext, {}) |
There was a problem hiding this comment.
This resumes each unhandled Get with a fresh {} on every request, which changes runtime behavior from the previous provideAll({}) boundary: multiple get() calls in one execution no longer observe the same context object. Programs that cache on context identity or intentionally mutate optional context between get() calls will now break (or diverge) even though they were valid before this refactor. Creating one per-run empty env object and reusing it for all Get resumptions preserves prior semantics and avoids extra allocations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50cc82516c
ℹ️ 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".
| type MissingRuntimeEnvArgs<E> = | ||
| [RuntimeEnv<E>] extends [never] | ||
| ? [] | ||
| : {} extends RuntimeEnv<E> |
There was a problem hiding this comment.
MissingRuntimeEnvArgs treats environment requirements as optional when RuntimeEnv<E> is a union containing any optional branch, because {} extends (A | B) succeeds if it matches either side. That means composed programs like requiredFx.pipe(andThen(optionalFx)) (or any union of Get<{required: ...}> with Get<{optional?: ...}>) can now pass run/runTask/runPromise without a provider and receive {} at runtime, reintroducing the runtime-undefined failure mode this type gate is meant to prevent.
Useful? React with 👍 / 👎.
| runtimeContext?: RuntimeContext | ||
| ): Promise<A> => { | ||
| let interrupting: Promise<void> | undefined | ||
| const defaultEnv = {} |
There was a problem hiding this comment.
Allocating defaultEnv inside each runForkLoop gives every forked task its own {} instead of sharing the parent run’s default environment object. Before this refactor, runTask wrapped the whole run with provideAll({}), so parent and children observed the same default env; now a child get() mutation is invisible to the parent (and siblings), which is a behavioral change for programs relying on env identity/coordination across forks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ca204e362
ℹ️ 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".
| const defaultEnv = parentRuntimeContext?.defaultEnv ?? {} | ||
| const runtimeContext = withDefaultEnv(parentRuntimeContext, defaultEnv) |
There was a problem hiding this comment.
Compute defaultEnv from a fresh object at the runtime boundary instead of inheriting currentRuntimeContext().defaultEnv; as written, calling runTask/runPromise from inside an active Fx execution (for example in an assertPromise callback) causes the nested run to reuse the parent run’s mutable env object. That lets separate entrypoints read and overwrite each other’s get() state, which regresses the previous per-entrypoint isolation and can produce action-at-a-distance bugs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3eccfb8fed
ℹ️ 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".
| type RequiredRuntimeEnv<E> = | ||
| Extract<E, Get<Record<PropertyKey, unknown>>> extends infer G | ||
| ? G extends Get<infer A> | ||
| ? {} extends A ? never : A |
There was a problem hiding this comment.
RequiredRuntimeEnv currently uses {} extends A on the whole inferred context type, so a single Get with a union like Get<{name: string} | {name?: string}> is treated as optional and run/runTask/runPromise accept it without provideAll. That reintroduces the runtime-boundary unsoundness this guard is meant to prevent: the program can be resumed with {} and observe undefined for required fields. Fresh evidence: a minimal repro with fx(function* ({name}: {name: string} | {name?: string}) { ... }) compiles through runTask(...) unless manually blocked.
Useful? React with 👍 / 👎.
1 participant
Summary
benchmark:runtime-loopsharness after scope/token and task lifecycle API changesprovideAll({})overhead fromrun,runTask, andrunForkbenchmarks/results/2026-05-26-benchmark-harness-and-trace-regressions.mdRoot Cause
The broken
runtime-loopsbenchmark was using stale APIs:scope(name)now creates a scope token and must be paired withwithScope(...)task.interrupt()instead of the old disposal helper pathThe trace regressions were not mainly in trace-policy logic.
runandrunTaskalways wrapped execution inprovideAll({}), which adds a general handler frame around every runtime execution. Afterfefff46added cleanup-drain semantics for interrupted finalizers, that default environment wrapper became visible in hot paths. This change keeps the cleanup semantics and handles the default empty environment directly at the runtime boundary.Impact
benchmark:runtime-loopsruns again and preserves existing case names for historical comparisons.pure runtime baselinereturns from about1178 ns/opto about235 ns/opin the investigated worktree.successful assertPromise labels/offrecover most of the regression signal from the weekly benchmark run.handled fail labels/offremains slower than the older May 7 artifact; the investigation note isolates that to synchronouscontrol(Fail, ...)cleanup-drain overhead and leaves it as follow-up work.Validation
corepack pnpm benchmark:tracecorepack pnpm benchmark:runtime-contextcorepack pnpm benchmark:runtime-loopscorepack pnpm buildcorepack pnpm lintcorepack pnpm testcorepack pnpm typecheck