Skip to content

fix(core): yield between plugin setup and full-scope cleanup operations - #55754

Open
mradwankhalil wants to merge 7 commits into
anomalyco:devfrom
mradwankhalil:fix/plugin-lifecycle-event-loop-fairness
Open

mradwankhalil wants to merge 7 commits into
anomalyco:devfrom
mradwankhalil:fix/plugin-lifecycle-event-loop-fairness

Conversation

@mradwankhalil

@mradwankhalil mradwankhalil commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Issue for this PR

Closes #55272

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Port the local lifecycle scheduling prototype onto current dev:

  • 8df12f7ef040618e5f20cedd557b98a92e943280 → 18ae2e4843: native zero-duration scheduler checkpoints between plugin initialization operations, with one-permit admission released before asynchronous setup.
  • 3fe12a353ed6a303fb5361af18a67d6f4bda40f6 → c93a4f2da7: register a checkpoint as each plugin scope's first finalizer, so it runs after that plugin's cleanup on reload and full location disposal, including failed setup. Preserve LIFO ordering, error collection and exactly-once cleanup.

Cannot preempt a single plugin's synchronous work; prevents a sequence of plugins from monopolizing one JavaScript turn. This carried implementation shares the short checkpoint admission permit between setup and close (neither holds it during plugin work); it does not implement the separate-close-permit variant mentioned in the issue's prototype description.

How did you verify your code works?

Fresh isolated clone, Bun 1.4.2 matching packageManager, frozen lockfile:
bun test test/plugin-burst.test.ts test/plugin.test.ts test/plugin-failure.test.ts from packages/core — 35 pass, 0 fail, 125 assertions. Coverage includes overlapping initialization timer progress, cross-location activation without deadlock, full shutdown timer progress, LIFO/exactly-once cleanup and cleanup defects. LSP diagnostics: no errors in all four changed files.

The 7.28-second real-host measurement in the issue is prior recorded evidence, not rerun here. No unrelated TTL, reload-debounce or readiness changes included.

Screenshots / recordings

Not a UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

Plugin.Service.close shuts the parent scope down directly, so the plugin finalizers of a disposed location ran back to back with no scheduling checkpoint: three locations with thirty 80 ms synchronous cleanups silenced the global event stream for 7.28 s. Only the reload path (activate with a shorter list) yielded between plugins.

Register an event-loop checkpoint as the first finalizer of every plugin scope. Scopes close last-in first-out, so it runs after that plugin's cleanup on every close path: reload, failed setup and full location disposal. The reload-only checkpoint goes away. The parent scope still owns ordering, defect collection and exactly-once closure.

The close-path checkpoint carries its own span name (Plugin.scopeClose.eventLoopCheckpoint) so a release build can be told apart from one without this change.
@github-actions github-actions Bot added the needs:compliance This means the issue will auto-close after 2 hours. label Oct 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

The following comment was made by an LLM, it may be inaccurate:

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This fixes the reported starvation. I ran the reproduction from #55272 (30 plugins, each busy-waiting 80 ms) on the base and on this branch. The longest timer gap dropped from about 2.4 s to 160 ms on close and 85 ms on activate. The plugin tests and the packages/core type check pass, and the full core suite shows the same environment-related failures on the base.

Public API: unchanged. PluginScheduling is an internal core module.

A few things worth tightening, see the inline comments: the process-wide admission lock, the extra pause in the supervisor, and the size of the new test file.

Comment thread packages/core/src/plugin/scheduling.ts Outdated
// location at once. Release BEFORE setup: asynchronous plugin work stays
// concurrent and can request another location without a process-wide deadlock.
// Keep the Clock injectable: virtual-clock tests must not wait for wall time.
const turn = Semaphore.makeUnsafe(1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The fix comes from the zero-length sleep itself: the repro passes because each plugin gets its own macrotask turn. This module-level Semaphore.makeUnsafe(1) adds one process-wide lock shared by every location's setup and close. It makes each location's yield wait for all the others' timer turns, and it couples unrelated locations through module state, including across test layers. I don't see a concrete failure it prevents (a burst of locations resuming in one batch still yields each time). Could this be a plain Effect.sleep(0) (one constant for both paths), or could you describe the failure the lock avoids? The repo guidelines ask for that before adding complexity for speculative races.

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.

Addressed in f3e6ab7, included in pushed head f97e165: one shared Effect.sleep(0), no process-wide semaphore. Existing lifecycle call sites and spans are unchanged. Bun 1.4.4 activate/close proofs pass (2/2); disabling each actual yield separately makes its corresponding test fail.

Comment thread packages/core/src/plugin/supervisor.ts Outdated
const plugins = () => [...definitions, ...packages.values()]

for (const operation of operations) {
yield* PluginScheduling.checkpoint

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This adds a macrotask yield before every config plugin operation in resolve, including cheap remove and selector entries, which delays every reload a little. #55272 is about plugin setup and teardown, which plugin.ts now covers, and modules.load is already async. Unless you've measured starvation in this loop, I'd drop this change so the fix stays in the plugin lifecycle.

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.

Removed the resolve-loop checkpoint and its import in f97e165. Cheap remove/select operations no longer pause; plugin setup/teardown yields remain unchanged. Core tsc build and fresh LSP diagnostics for all three edited files pass.

@@ -0,0 +1,163 @@
import { expect } from "bun:test"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This file has 7 timing-based tests with busy loops, setInterval(0) and strict 'a tick happened between every pair' assertions. Several also re-test existing behaviour: cancellation and readiness holds, finalizer-defect ordering, and calling PluginScheduling.checkpoint directly. Could you cut it down to one small activate test and one close test that check a timer fires between CPU-heavy plugins? If the lock goes away (see the comment on scheduling.ts), the cross-location deadlock test isn't needed either.

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.

Reduced to two activate/close tests in pushed head f97e165. Each arms a native timer inside the first CPU-heavy plugin and observes progress strictly between plugin work, retaining registration/LIFO order checks; no elapsed-time threshold or every-pair guarantee. Original baseline 7/7; new tests 2/2 on Bun 1.4.4. Disabling activate and close yields separately produces the respective failure, then both pass after restoration. Direct neighbours remain 35 pass/1 pre-existing failure: "applies organization integration changes to running external plugins" (error: pending), identical before/after. The broader plugin baseline timed out at 120 seconds; full-suite verification remains incomplete, not claimed green.

@github-actions github-actions Bot removed the needs:compliance This means the issue will auto-close after 2 hours. label Oct 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Thanks for updating your PR! It now meets our contributing guidelines. 👍

refresh branch without rewriting history

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new commits only merge dev in. The PR's own changes are unchanged, so the earlier points about the process-wide admission lock in scheduling.ts, the per-operation pause in supervisor.ts and the size of plugin-burst.test.ts are still open.

I re-ran the #55272 reproduction on the merged head and it still passes: the longest timer gap is 161 ms on close and 86 ms on activate. The plugin tests (435 pass) and the packages/core type check pass. Public API: unchanged.

@mradwankhalil

Copy link
Copy Markdown
Contributor Author

Thanks for rechecking the merged head and reporting the timer-gap measurements. This update merged dev; it did not change the scheduling design. Our focused plugin tests passed 35/35 with the repository-pinned Bun 1.4.4, and the core typecheck and build passed. The 435-test result and 161 ms / 86 ms measurements are your verification, not a fresh result from us. The earlier admission-lock, per-operation pause, and test-size review points remain open; the passing reproduction does not by itself resolve those concerns.

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new commits only merge dev in, and the PR's own files are unchanged. The earlier points on the shared admission lock, the supervisor pause and the test file size are still open.

I re-ran the #55272 reproduction on the new head and it still passes: the longest timer gap is 161 ms on close and 86 ms on activate. The plugin tests (439 pass) and typecheck:tsc in packages/core pass. Public API: unchanged.

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All three earlier points are addressed:

  • the shared admission lock is gone, so both checkpoints are a plain Effect.sleep(0);
  • the per-operation yield in supervisor.ts is removed;
  • the burst tests are down to one activate test and one close test.

The new tests fail on the base and pass here. The #55272 reproduction still passes, with a longest timer gap of 160 ms on close and 84 ms on activate (about 2.4 s on the base). The plugin tests (432 pass) and typecheck:tsc in packages/core pass. Public API: unchanged.

@mradwankhalil

Copy link
Copy Markdown
Contributor Author

Thanks for checking the simplified checkpoints and the base-versus-branch regressions. Those are reviewer-reported results, separate from the hosted workflows still awaiting approval. The original inline threads remain open; this acknowledgement does not mark them resolved.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

core: CPU-heavy plugin setup and full-scope teardown starve the shared event loop

1 participant