Repository navigation
Conversation
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is sound. On the base, the reproduction from #57089 fails: when a plugin's cleanup throws, activate() fails and the builtin's command is gone afterwards. With this PR the swap succeeds and the builtin's new revision is registered. Your new test fails without the fix and passes with it. The plugin test suites (422 tests) and the packages/core typecheck pass.
The new handling copies what the file already does when cleaning up disabled plugins (catchCause + log), so it fits. Interruption isn't a concern here, because Scope.close runs finalizers in a way that can't be interrupted.
Public API: unchanged.
No blocking concerns.
Jun516
left a comment
Jun516
left a comment
There was a problem hiding this comment.
One edge case in the new catch: Effect.catchCause also turns an interruption from a cleanup finalizer into successful activation. Could this use the existing Effect.catchCauseIf((cause) => !Cause.hasInterrupts(cause), ...) pattern used later in this file, and add an interruption regression?
I checked with the real PluginTestLayer and Bun 1.4.2 using this test body:
Effect.gen(function* () {
const plugins = yield* Plugin.Service
yield* plugins.activate([
{ id: "interrupted", revision: "1", effect: () => Effect.addFinalizer(() => Effect.interrupt) },
])
const activation = yield* plugins.activate([]).pipe(Effect.forkChild({ startImmediately: true }))
const exit = yield* Fiber.await(activation)
expect(Exit.isFailure(exit)).toBe(true)
if (Exit.isFailure(exit)) expect(Cause.hasInterrupts(exit.cause)).toBe(true)
})From packages/core: bun run script/test.ts test/plugin-failure.test.ts --test-name-pattern "preserves interruption during plugin cleanup".
The check passes on the pre-fix source, fails with the exact plugin.ts blob from this PR's 10e87b9 (Expected: true; Received: false), and passes with catchCauseIf. This is a local focused check, not a full-suite result. AI assistance was used for the test and analysis.
`Plugin.activate` closes the previous generation before loading the new one, and
the close loop had no per-plugin isolation. A plugin whose finalizer throws
aborted the whole activation: the old builtins had already been closed, the new
generation never loaded, and their transforms were never re-registered. Builtin
state then fell back to defaults, so a user plugin's broken cleanup silently
re-enabled `compaction.auto` despite `compaction: { auto: false }` (anomalyco#57089).
Each old slot now closes independently, logging `failed to close plugin` with the
id and cause. One plugin's teardown failure is reported instead of aborting the
swap, so the new generation still loads and builtins keep their registrations.
Adds a case with a finalizer that throws only for the revision being replaced, so
the surviving generation tears down normally and the assertion covers the swap
alone. It fails without the change: the error escapes `Plugin.activate` and the
builtin command is gone.
10e87b9 to
536dc02
Compare
This branch has not been deployed
2 participants
Issue for this PR
Closes #57089
Type of change
What does this PR do?
Plugin.activatecloses the previous generation before loading the new one, and the close loop had no per-plugin isolation. A plugin whose finalizer throws aborted the whole activation. By then the old builtins had already been closed — they are last in activation order, so the reverse close loop reaches them first — and the new generation never loaded, so their transforms were never re-registered:Each old slot now closes independently:
A teardown failure is logged with the plugin id and cause instead of aborting the swap, so the new generation still loads and builtin registrations survive.
load()already isolates setup failures; this closes the corresponding gap on the teardown path.This is the minimal option from the report. Two larger options remain open and are not attempted here: staging the new generation before closing the old (plugins hold process-wide resources), and separating builtins from user-plugin generations so a user change cannot unload them. Either would make the swap transactional rather than best-effort.
How did you verify your code works?
One case in
packages/core/test/plugin-failure.test.ts:brokenmirrors the user plugin andbuiltinis last in activation order, likeinternal.post. Generation 2 replaces both, and the case asserts the swap succeeds and the builtin transform is re-registered with the new revision.The throwing finalizer is registered only for the revision being replaced. That is deliberate: it keeps the surviving generation healthy so teardown at the end of the test does not add an unrelated failure. (With a finalizer that always throws, the fixed behaviour loads the broken plugin successfully and the case then fails at teardown instead, which would be testing the wrong thing.)
Against the current code the case fails: the
ReferenceErrorescapesPlugin.activateand the builtin command is gone.Screenshots / recordings
Not applicable; no UI change.
Checklist
Added by the opencode bot (AI-generated).
Related: #49982, #52618