From 5aca896553a8d5c27445e70deed53662429b2b02 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Tue, 1 Sep 2026 18:08:50 +0000 Subject: [PATCH] fix(effect): abort-recheck in boundary bridges + alias-aware lint Defer the aborted check until the effect starts and recheck after addEventListener so a signal that aborts between composition and run interrupts instead of hanging. Track aliased Effect namespaces in the boundary lint rule so Fx.runPromise cannot slip through. --- packages/agent-bundle/src/effect/boundary.ts | 41 +++++++----- .../tests/effect-boundary.test.ts | 13 ++++ packages/rsc-runtime/src/effect/boundary.ts | 12 ++-- .../tests/effect-boundary-lint.test.ts | 67 ++++++++++++++++++- .../rsc-runtime/tests/effect-boundary.test.ts | 6 +- scripts/eslint-plugin-effect-boundary.ts | 44 +++++++++--- 6 files changed, 149 insertions(+), 34 deletions(-) diff --git a/packages/agent-bundle/src/effect/boundary.ts b/packages/agent-bundle/src/effect/boundary.ts index 4fef0c922..99bfb8b38 100644 --- a/packages/agent-bundle/src/effect/boundary.ts +++ b/packages/agent-bundle/src/effect/boundary.ts @@ -132,28 +132,39 @@ export const makeScopedEffectRuntime = ( }; /** - * AbortSignal → Effect interruption, for programs that still run inside - * Effect and receive a host signal. The Promise edge also accepts `signal` - * directly via {@link runPromise}. + * Host AbortSignal → Effect interruption. Re-checks `signal.aborted` when + * the effect starts (not only when this helper is constructed) so a signal + * that aborts between construction and run still interrupts. The listener + * is registered first; aborted signals do not replay `abort`, so the + * callback rechecks immediately after `addEventListener`. */ -export const interruptWhenAborted = ( - effect: Effect.Effect, - signal: AbortSignal, -): Effect.Effect => { - if (signal.aborted) return interruptAs(); - return Effect.raceFirst( - effect, - Effect.callback((resume) => { - const onAbort = () => { +export const abortToInterrupt = (signal: AbortSignal): Effect.Effect => + Effect.suspend(() => { + if (signal.aborted) return interruptAs(); + return Effect.callback((resume) => { + let settled = false; + const onAbort = (): void => { + if (settled) return; + settled = true; resume(Effect.interrupt); }; signal.addEventListener('abort', onAbort, { once: true }); + if (signal.aborted) onAbort(); return Effect.sync(() => { signal.removeEventListener('abort', onAbort); }); - }), - ); -}; + }); + }); + +/** + * AbortSignal → Effect interruption, for programs that still run inside + * Effect and receive a host signal. The Promise edge also accepts `signal` + * directly via {@link runPromise}. + */ +export const interruptWhenAborted = ( + effect: Effect.Effect, + signal: AbortSignal, +): Effect.Effect => Effect.raceFirst(effect, abortToInterrupt(signal)); /** * Effect interruption → AbortSignal, for Promise/fetch APIs that take a diff --git a/packages/agent-bundle/tests/effect-boundary.test.ts b/packages/agent-bundle/tests/effect-boundary.test.ts index feead003f..dcb3e26eb 100644 --- a/packages/agent-bundle/tests/effect-boundary.test.ts +++ b/packages/agent-bundle/tests/effect-boundary.test.ts @@ -8,6 +8,7 @@ import { DevLockError } from '../src/dev/dev-lock.ts'; import { ProjectEventHubError } from '../src/dev/events.ts'; import { abortError, + abortToInterrupt, interruptWhenAborted, isAbortError, isTypedDevError, @@ -76,4 +77,16 @@ describe('effect boundary (agent-bundle dev seam)', () => { expect(toDevError('plain')).toEqual(new Error('plain')); expect(abortError().name).toBe('AbortError'); }); + + it('interrupts when the host signal aborts between construction and run', async () => { + const controller = new AbortController(); + const program = interruptWhenAborted(Effect.never, controller.signal); + controller.abort(); + const pending = runPromise(program); + const hung = new Promise((_, reject) => { + setTimeout(() => reject(new Error('interruptWhenAborted hung after abort-before-start')), 250); + }); + await expect(Promise.race([pending, hung])).rejects.toSatisfy(isAbortError); + await expect(runPromise(abortToInterrupt(controller.signal))).rejects.toSatisfy(isAbortError); + }); }); diff --git a/packages/rsc-runtime/src/effect/boundary.ts b/packages/rsc-runtime/src/effect/boundary.ts index 9fd4bbb10..b09fe566d 100644 --- a/packages/rsc-runtime/src/effect/boundary.ts +++ b/packages/rsc-runtime/src/effect/boundary.ts @@ -132,20 +132,22 @@ export const makeScopedEffectRuntime = ( /** * Host AbortSignal → Effect interruption. Re-checks `signal.aborted` when * the effect starts (not only when this helper is constructed) so a signal - * that aborts between construction and run still interrupts. + * that aborts between construction and run still interrupts. The listener + * is registered first; aborted signals do not replay `abort`, so the + * callback rechecks immediately after `addEventListener`. */ export const abortToInterrupt = (signal: AbortSignal): Effect.Effect => Effect.suspend(() => { if (signal.aborted) return interruptAs(); return Effect.callback((resume) => { - if (signal.aborted) { - resume(Effect.interrupt); - return undefined; - } + let settled = false; const onAbort = (): void => { + if (settled) return; + settled = true; resume(Effect.interrupt); }; signal.addEventListener('abort', onAbort, { once: true }); + if (signal.aborted) onAbort(); return Effect.sync(() => { signal.removeEventListener('abort', onAbort); }); diff --git a/packages/rsc-runtime/tests/effect-boundary-lint.test.ts b/packages/rsc-runtime/tests/effect-boundary-lint.test.ts index b35e87619..f297fa024 100644 --- a/packages/rsc-runtime/tests/effect-boundary-lint.test.ts +++ b/packages/rsc-runtime/tests/effect-boundary-lint.test.ts @@ -4,14 +4,32 @@ import { effectBoundaryPlugin, isEffectBoundaryFile } from '../../../scripts/esl const rule = effectBoundaryPlugin.rules['no-ad-hoc-run']; -const apply = (filename: string, visit: (listeners: ReturnType) => void) => { +type ImportNode = { + readonly imported?: { readonly name?: string }; + readonly local?: { readonly name?: string }; + readonly parent?: { readonly source?: { readonly value?: unknown } }; +}; + +type MemberNode = { + readonly computed?: boolean; + readonly object?: { readonly name?: string; readonly type?: string }; + readonly property?: { readonly name?: string; readonly type?: string }; +}; + +type LintListeners = ReturnType & { + ImportNamespaceSpecifier?(node: ImportNode): void; + ImportSpecifier?(node: ImportNode): void; + MemberExpression?(node: MemberNode): void; +}; + +const apply = (filename: string, visit: (listeners: LintListeners) => void) => { const reports: Array<{ readonly messageId: string; readonly data: { readonly name: string } }> = []; const listeners = rule.create({ filename, report(descriptor) { reports.push({ data: descriptor.data, messageId: descriptor.messageId }); }, - }); + }) as LintListeners; visit(listeners); return reports; }; @@ -75,4 +93,49 @@ describe('effect-boundary lint', () => { }); expect(reports).toEqual([]); }); + + it('rejects aliased Effect namespaces (named and star imports)', () => { + const named = apply('packages/rsc-runtime/src/dispatcher.ts', (listeners) => { + listeners.ImportSpecifier?.({ + imported: { name: 'Effect' }, + local: { name: 'Fx' }, + parent: { source: { value: 'effect' } }, + }); + listeners.MemberExpression?.({ + computed: false, + object: { name: 'Fx', type: 'Identifier' }, + property: { name: 'runPromise', type: 'Identifier' }, + }); + }); + expect(named).toEqual([{ data: { name: 'Fx.runPromise' }, messageId: 'forbiddenCall' }]); + + const star = apply('packages/agent-bundle/src/dev/coordinator.ts', (listeners) => { + listeners.ImportNamespaceSpecifier?.({ + local: { name: 'E' }, + parent: { source: { value: 'effect' } }, + }); + listeners.MemberExpression?.({ + computed: false, + object: { name: 'E', type: 'Identifier' }, + property: { name: 'runSync', type: 'Identifier' }, + }); + }); + expect(star).toEqual([{ data: { name: 'E.runSync' }, messageId: 'forbiddenCall' }]); + }); + + it('does not flag aliased Effect used for non-runners', () => { + const reports = apply('packages/rsc-runtime/src/reconciler.ts', (listeners) => { + listeners.ImportSpecifier?.({ + imported: { name: 'Effect' }, + local: { name: 'E' }, + parent: { source: { value: 'effect' } }, + }); + listeners.MemberExpression?.({ + computed: false, + object: { name: 'E', type: 'Identifier' }, + property: { name: 'succeed', type: 'Identifier' }, + }); + }); + expect(reports).toEqual([]); + }); }); diff --git a/packages/rsc-runtime/tests/effect-boundary.test.ts b/packages/rsc-runtime/tests/effect-boundary.test.ts index 0b1d95ff0..310ba9206 100644 --- a/packages/rsc-runtime/tests/effect-boundary.test.ts +++ b/packages/rsc-runtime/tests/effect-boundary.test.ts @@ -81,7 +81,11 @@ describe('effect boundary', () => { const controller = new AbortController(); const program = interruptWhenAborted(Effect.never, controller.signal); controller.abort(); - await expect(runPromise(program)).rejects.toSatisfy(isAbortError); + const pending = runPromise(program); + const hung = new Promise((_, reject) => { + setTimeout(() => reject(new Error('interruptWhenAborted hung after abort-before-start')), 250); + }); + await expect(Promise.race([pending, hung])).rejects.toSatisfy(isAbortError); await expect(runPromise(abortToInterrupt(controller.signal))).rejects.toSatisfy(isAbortError); }); diff --git a/scripts/eslint-plugin-effect-boundary.ts b/scripts/eslint-plugin-effect-boundary.ts index 840c8064e..3688b6841 100644 --- a/scripts/eslint-plugin-effect-boundary.ts +++ b/scripts/eslint-plugin-effect-boundary.ts @@ -19,6 +19,8 @@ const RUN_NAMES = new Set([ ]); const EFFECT_MODULES = new Set(['effect']); +const EFFECT_NAMESPACES = new Set(['Effect', 'Runtime']); +const EFFECT_NAMESPACE_MODULES = new Set(['effect', 'effect/Effect', 'effect/Runtime']); const posixPath = (filename: string): string => filename.replaceAll('\\', '/'); @@ -33,9 +35,15 @@ const importedName = (node: { return imported.name; }; +const localName = (node: { readonly local?: { readonly name?: string } }): string | undefined => + node.local?.name; + const moduleName = (node: { readonly source?: { readonly value?: unknown } }): string | undefined => typeof node.source?.value === 'string' ? node.source.value : undefined; +const isEffectModule = (source: string | undefined): boolean => + source !== undefined && (EFFECT_MODULES.has(source) || source.startsWith('effect/')); + export const effectBoundaryPlugin = { meta: { name: 'effect-boundary', @@ -60,7 +68,31 @@ export const effectBoundaryPlugin = { report(descriptor: { readonly node: unknown; readonly messageId: string; readonly data: { readonly name: string } }): void; }) { if (isEffectBoundaryFile(context.filename)) return {}; + const runnerNamespaces = new Set(EFFECT_NAMESPACES); + const rememberNamespace = (name: string | undefined): void => { + if (name !== undefined) runnerNamespaces.add(name); + }; return { + ImportSpecifier(node: { + readonly imported?: { readonly name?: string }; + readonly local?: { readonly name?: string }; + readonly parent?: { readonly source?: { readonly value?: unknown } }; + }) { + const name = importedName(node); + const source = moduleName(node.parent ?? {}); + if (!isEffectModule(source) || name === undefined) return; + if (EFFECT_NAMESPACES.has(name)) rememberNamespace(localName(node) ?? name); + if (!RUN_NAMES.has(name)) return; + context.report({ data: { name }, messageId: 'forbiddenImport', node }); + }, + ImportNamespaceSpecifier(node: { + readonly local?: { readonly name?: string }; + readonly parent?: { readonly source?: { readonly value?: unknown } }; + }) { + const source = moduleName(node.parent ?? {}); + if (source === undefined || !EFFECT_NAMESPACE_MODULES.has(source)) return; + rememberNamespace(localName(node)); + }, MemberExpression(node: { readonly computed?: boolean; readonly object?: { readonly name?: string; readonly type?: string }; @@ -70,19 +102,9 @@ export const effectBoundaryPlugin = { const name = node.property?.name; if (name === undefined || !RUN_NAMES.has(name)) return; const objectName = node.object?.name; - if (objectName !== 'Effect' && objectName !== 'Runtime') return; + if (objectName === undefined || !runnerNamespaces.has(objectName)) return; context.report({ data: { name: `${objectName}.${name}` }, messageId: 'forbiddenCall', node }); }, - ImportSpecifier(node: { - readonly imported?: { readonly name?: string }; - readonly parent?: { readonly source?: { readonly value?: unknown } }; - }) { - const name = importedName(node); - if (name === undefined || !RUN_NAMES.has(name)) return; - const source = moduleName(node.parent ?? {}); - if (source === undefined || (!EFFECT_MODULES.has(source) && !source.startsWith('effect/'))) return; - context.report({ data: { name }, messageId: 'forbiddenImport', node }); - }, }; }, },