Skip to content

Commit 42305e6

Browse files
fix(effect): scopedTempDirectory keeps the force:true finalizer of the former try/finally rm
rc.112's makeTempDirectoryScoped finalizes with rm({ recursive: true }) and orDie, so an operation that removed its own staging directory would reject an already-successful listMcp/invokeMcp/... call or the Codex validator's AB6033 result with ENOENT at scope close. Both sites now use scopedTempDirectory (makeTempDirectory + rm({ recursive, force })), with a regression test.
1 parent cd89db1 commit 42305e6

5 files changed

Lines changed: 58 additions & 20 deletions

File tree

‎docs/effect-conventions.md‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -237,8 +237,14 @@ contract).
237237
modules. `fromFileUrl` fails with `BadArgument`; `Effect.orDie` it when the
238238
URL is built from `import.meta.url`.
239239
- Temporary directories whose lifetime ends with the enclosing operation:
240-
`makeTempDirectoryScoped` inside `Effect.scoped`, replacing `mkdtemp` +
241-
`try`/`finally` `rm`. **Not** when ownership of the directory is
240+
a scoped temp directory inside `Effect.scoped`, replacing `mkdtemp` +
241+
`try`/`finally` `rm`. In `agent-bundle` use `scopedTempDirectory` from
242+
`src/effect/platform.ts`, not `fs.makeTempDirectoryScoped` directly: the
243+
rc.112 finalizer removes without `force`, so an operation that deletes its
244+
own staging directory would die with ENOENT at scope close, where the old
245+
`rm(dir, { recursive: true, force: true })` succeeded. Tests may use
246+
`makeTempDirectoryScoped` (nothing removes the fixture underneath them).
247+
**Not** when ownership of the directory is
242248
transferred to a longer-lived object (the MCP session plugin-data dir in
243249
`dev/mcp-session/mcp-session-service.ts`): a scoped temp is removed when
244250
the scope closes, which is too early there.
@@ -301,16 +307,16 @@ provides `NodeServices.layer` once. Measured on rc.112 (bundled by Rslib,
301307
the bundle.
302308

303309
`packages/agent-bundle/src/effect/platform.ts` owns the framework's platform
304-
layer: `platformLayer` (= `NodeServices.layer`), `unwrapPlatformError`, and
305-
`runWithPlatform`, which provides the layer and unwraps `PlatformError`
310+
layer: `platformLayer` (= `NodeServices.layer`), `scopedTempDirectory`,
311+
`unwrapPlatformError`, and `runWithPlatform`, which provides the layer and unwraps `PlatformError`
306312
before handing off to `boundary.ts`'s `runPromise`. It is the only module
307313
that imports `effect/PlatformError`: `boundary.ts` is bundled into every
308314
emitted hook wrapper, and the error class would drag `Data.TaggedError` into
309315
each one (measured: +12 kB per hook). Phase-1 callers are the throwaway
310316
artifact in `api.ts` (`listMcp` / `invokeMcp` / `runMcp` / `listHooks` /
311317
`simulateHook` without `artifact`) and the Codex validator's
312-
schema-generation directory, both `makeTempDirectoryScoped` inside
313-
`Effect.scoped`. Emitted artifacts, hook wrappers, and compiler hot paths
318+
schema-generation directory, both `scopedTempDirectory` (the `force: true`
319+
finalizer) inside `Effect.scoped`. Emitted artifacts, hook wrappers, and compiler hot paths
314320
never import this module; the dev server picks it up in phase 2 through
315321
`makeScopedEffectRuntime(platformLayer)`.
316322

‎packages/agent-bundle/src/api.ts‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { execFile as executeFile } from 'node:child_process';
22
import { join, resolve } from 'node:path';
33
import { promisify } from 'node:util';
44

5-
import { Effect, FileSystem } from 'effect';
5+
import { Effect } from 'effect';
66

77
import { capabilityIsSupported, unavailableCapability } from './adapters/capability-state.ts';
88
import { createDefaultRegistry, TargetRegistry } from './adapters/registry.ts';
@@ -166,7 +166,7 @@ import {
166166
// Imported after the service modules on purpose: the position of
167167
// `effect/lift.ts` in the module graph fixes its position in the emitted
168168
// hook bundles, and this order keeps those bundles byte-identical.
169-
import { runWithPlatform } from './effect/platform.ts';
169+
import { runWithPlatform, scopedTempDirectory } from './effect/platform.ts';
170170
import { liftPromise } from './effect/lift.ts';
171171

172172
export {
@@ -579,8 +579,7 @@ const temporaryArtifact = async <Result>(
579579
// to the project (same filesystem as a real `artifact/`), removed when the
580580
// build or the operation settles, success or failure.
581581
return runWithPlatform(Effect.scoped(Effect.gen(function* () {
582-
const fs = yield* FileSystem.FileSystem;
583-
const artifact = yield* fs.makeTempDirectoryScoped({
582+
const artifact = yield* scopedTempDirectory({
584583
directory: resolve(options.root),
585584
prefix: '.agent-bundle-artifact-',
586585
});

‎packages/agent-bundle/src/effect/platform.ts‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import * as NodeServices from '@effect/platform-node/NodeServices';
2-
import { Effect, type Layer } from 'effect';
2+
import { Effect, FileSystem, type Layer, type Scope } from 'effect';
33
import { PlatformError } from 'effect/PlatformError';
44

55
import { runPromise, type RunPromiseOptions } from './boundary.ts';
@@ -36,6 +36,27 @@ export const unwrapPlatformError = <E>(error: E): Exclude<E, PlatformError> | Er
3636
? (error.cause instanceof Error ? error.cause : error)
3737
: (error as Exclude<E, PlatformError>);
3838

39+
/**
40+
* A temporary directory that lives exactly as long as the enclosing scope,
41+
* with the `rm(dir, { recursive: true, force: true })` finalizer the
42+
* `try`/`finally` sites had before they moved onto Effect. rc.112's own
43+
* `makeTempDirectoryScoped` finalizes without `force`, so an operation that
44+
* removes (or renames away) its own staging directory would die with ENOENT
45+
* at scope close and reject a call that had already succeeded. A finalizer
46+
* failure other than "already gone" is still a defect, as the `finally`
47+
* throw was.
48+
*/
49+
export const scopedTempDirectory = (
50+
options?: { readonly directory?: string; readonly prefix?: string },
51+
): Effect.Effect<string, PlatformError, FileSystem.FileSystem | Scope.Scope> =>
52+
Effect.gen(function* () {
53+
const fs = yield* FileSystem.FileSystem;
54+
return yield* Effect.acquireRelease(
55+
fs.makeTempDirectory(options),
56+
(directory) => Effect.orDie(fs.remove(directory, { force: true, recursive: true })),
57+
);
58+
});
59+
3960
/**
4061
* Run a platform-dependent Effect program at a Promise edge. Same failure
4162
* contract as `runPromise`, with `PlatformError` unwrapped to its Node cause.

‎packages/agent-bundle/src/host-contracts/codex-plugin-validation.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ import {
2626
// Imported last on purpose: see the matching note in `src/api.ts` — the
2727
// position of `effect/lift.ts` in the module graph keeps the emitted hook
2828
// bundles byte-identical.
29-
import { runWithPlatform } from '../effect/platform.ts';
29+
import { runWithPlatform, scopedTempDirectory } from '../effect/platform.ts';
3030
import { liftPromise } from '../effect/lift.ts';
3131

3232
const maximumOutputBytes = 1024 * 1024;
@@ -276,8 +276,7 @@ const schemaGenerationDiagnostics = (
276276
readonly version: string | undefined;
277277
}>,
278278
): Effect.Effect<readonly Diagnostic[], PlatformError, FileSystem.FileSystem> => Effect.scoped(Effect.gen(function* () {
279-
const fs = yield* FileSystem.FileSystem;
280-
const outputDirectory = yield* fs.makeTempDirectoryScoped({ prefix: 'agent-bundle-codex-schema-' });
279+
const outputDirectory = yield* scopedTempDirectory({ prefix: 'agent-bundle-codex-schema-' });
281280
const started = yield* liftPromise(() => options.run(Object.freeze({
282281
args: Object.freeze(['app-server', 'generate-json-schema', '--out', outputDirectory]),
283282
cwd: options.cwd,

‎packages/agent-bundle/tests/effect-platform.test.ts‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { describe, expect, it } from '@rstest/core';
77

88
import { DiagnosticError } from '../src/core/diagnostics.ts';
99
import { liftPromise } from '../src/effect/lift.ts';
10-
import { platformLayer, runWithPlatform, unwrapPlatformError } from '../src/effect/platform.ts';
10+
import { platformLayer, runWithPlatform, scopedTempDirectory, unwrapPlatformError } from '../src/effect/platform.ts';
1111
import * as devApi from '../src/dev/index.ts';
1212
import * as rootApi from '../src/index.ts';
1313

@@ -64,7 +64,7 @@ describe('effect platform layer (agent-bundle)', () => {
6464
try {
6565
const directory = await runWithPlatform(Effect.scoped(Effect.gen(function* () {
6666
const fs = yield* FileSystem.FileSystem;
67-
const created = yield* fs.makeTempDirectoryScoped({ directory: parent, prefix: '.staging-' });
67+
const created = yield* scopedTempDirectory({ directory: parent, prefix: '.staging-' });
6868
expect(created.startsWith(join(parent, '.staging-'))).toBe(true);
6969
yield* fs.writeFileString(join(created, 'manifest.json'), '{}');
7070
yield* Effect.promise(() => access(created));
@@ -82,8 +82,7 @@ describe('effect platform layer (agent-bundle)', () => {
8282
const failure = new DiagnosticError([{ code: 'AB7200', message: 'rebuild failed', severity: 'error' }]);
8383
try {
8484
await expect(runWithPlatform(Effect.scoped(Effect.gen(function* () {
85-
const fs = yield* FileSystem.FileSystem;
86-
directory = yield* fs.makeTempDirectoryScoped({ directory: parent, prefix: '.staging-' });
85+
directory = yield* scopedTempDirectory({ directory: parent, prefix: '.staging-' });
8786
yield* liftPromise(() => Promise.reject(failure));
8887
})))).rejects.toBe(failure);
8988
expect(directory).toBeDefined();
@@ -93,11 +92,25 @@ describe('effect platform layer (agent-bundle)', () => {
9392
}
9493
});
9594

95+
it('keeps the result when the operation already removed its scoped temp directory', async () => {
96+
const parent = await mkdtemp(join(tmpdir(), 'agent-bundle-platform-'));
97+
try {
98+
const result = await runWithPlatform(Effect.scoped(Effect.gen(function* () {
99+
const fs = yield* FileSystem.FileSystem;
100+
const created = yield* scopedTempDirectory({ directory: parent, prefix: '.staging-' });
101+
yield* fs.remove(created, { recursive: true });
102+
return 'settled';
103+
})));
104+
expect(result).toBe('settled');
105+
} finally {
106+
await rm(parent, { force: true, recursive: true });
107+
}
108+
});
109+
96110
it('throws the Node error when the temp directory cannot be created', async () => {
97111
const missingParent = join(tmpdir(), 'agent-bundle-platform-missing', String(process.pid));
98112
await expect(runWithPlatform(Effect.scoped(Effect.gen(function* () {
99-
const fs = yield* FileSystem.FileSystem;
100-
return yield* fs.makeTempDirectoryScoped({ directory: missingParent, prefix: '.staging-' });
113+
return yield* scopedTempDirectory({ directory: missingParent, prefix: '.staging-' });
101114
})))).rejects.toMatchObject({ code: 'ENOENT', syscall: 'mkdtemp' });
102115
});
103116
});

0 commit comments

Comments
 (0)