Skip to content

fix: resolve all waiters for plugin loads #2955

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mikamikasuki wants to merge 1 commit into
base: main
Choose a base branch
from
+68 −21
Open

fix: resolve all waiters for plugin loads #2955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 8 additions & 21 deletions src/lib/acode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ import {
import notificationManager from "lib/notificationManager";
import openFolder, { addedFolder } from "lib/openFolder";
import orientation from "lib/orientation";
import PluginWaiters from "lib/pluginWaiters";
import projects from "lib/projects";
import selectionMenu from "lib/selectionMenu";
import appSettings from "lib/settings";
Expand Down Expand Up @@ -96,7 +97,7 @@ class Acode {
#pluginUnmount = {};
// Registered formatter implementations (populated by plugins)
#formatter = [];
#pluginWatchers = {};
#pluginWatchers = new PluginWaiters();

/**
* Clear a plugin's broken mark (so it can be retried)
Expand Down Expand Up @@ -700,32 +701,18 @@ class Acode {
}

[onPluginLoadCallback](pluginId) {
if (this.#pluginWatchers[pluginId]) {
this.#pluginWatchers[pluginId].resolve();
delete this.#pluginWatchers[pluginId];
}
this.#pluginWatchers.resolve(pluginId);
}

[onPluginsLoadCompleteCallback]() {

@bajrangCoder bajrangCoder Oct 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Timed-out plugins: loadPluginWithTimeout returns false after 15s but keeps loading in the background. loadPlugins then completes and rejects its waiters here. When the plugin finishes later, markPluginLoaded → resolve(pluginId) finds nothing, so dependents never initialize even though the plugin is running.

Maybe skip rejecting plugins that timed out but haven't settled, or defer their rejection until PLUGIN_DISABLE_TIMEOUT.

for (const pluginId in this.#pluginWatchers) {
this.#pluginWatchers[pluginId].reject(
new Error(`Plugin '${pluginId}' failed to load.`),
);
}
this.#pluginWatchers = {};
this.#pluginWatchers.rejectAll(

@bajrangCoder bajrangCoder Oct 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This callback also fires at the end of the theme-only pass (loadPlugins(true) in main.js:766), which runs before the main loadPlugins() pass. Any waiter registered by then (e.g. by a theme plugin waiting on a regular plugin) gets rejected with "failed to load" even though that plugin hasn't been attempted yet.

Could we only reject when !loadOnlyTheme, or pass that flag through to the callback?

(pluginId) => new Error(`Plugin '${pluginId}' failed to load.`),
);
}

waitForPlugin(pluginId) {

@bajrangCoder bajrangCoder Oct 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the initial load has already completed (isInitialPluginLoadComplete()), a call for a plugin that's broken/disabled/not installed registers a waiter that will never settle — no further onPluginsLoadCompleteCallback runs, so the caller hangs silently.

Since this function is being rewritten anyway, it'd be good to reject immediately in that case.

Related: with the new Set, repeated calls for a never-loaded ID now accumulate entries for the rest of the session (previously it was overwritten, so bounded at one per ID). Settling late callers immediately fixes both.

return new Promise((resolve, reject) => {
if (LOADED_PLUGINS.has(pluginId)) {
return resolve(true);
}

this.#pluginWatchers[pluginId] = {
resolve,
reject,
};
});
if (LOADED_PLUGINS.has(pluginId)) return Promise.resolve(true);

@bajrangCoder bajrangCoder Oct 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: the fast path resolves with true, but the waiter path resolves with undefined (waiter.resolve() in PluginWaiters). Code like if (await acode.waitForPlugin("dep")) init(); behaves differently depending on load order. Suggest waiter.resolve(true) and updating the test accordingly.

return this.#pluginWatchers.waitFor(pluginId);
}

get exitAppMessage() {
Expand Down
29 changes: 29 additions & 0 deletions src/lib/pluginWaiters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
export default class PluginWaiters {
#waiters = new Map();

waitFor(pluginId) {
return new Promise((resolve, reject) => {
let waiters = this.#waiters.get(pluginId);
if (!waiters) {
waiters = new Set();
this.#waiters.set(pluginId, waiters);
}
waiters.add({ resolve, reject });

@bajrangCoder bajrangCoder Oct 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggestion: rather than a Set of separate { resolve, reject } objects, store one deferred per plugin ID and hand every caller the same promise:

waitFor(pluginId) {
	let deferred = this.#waiters.get(pluginId);
	if (!deferred) {
		deferred = Promise.withResolvers();
		this.#waiters.set(pluginId, deferred);
	}
	return deferred.promise;
}

resolve/rejectAll then become single calls with no inner loops. (Check Promise.withResolvers support on the minimum WebView; a small manual deferred works too.)

});
}

resolve(pluginId) {
const waiters = this.#waiters.get(pluginId);
if (!waiters) return;

this.#waiters.delete(pluginId);
for (const waiter of waiters) waiter.resolve();
}

rejectAll(getError) {
for (const [pluginId, waiters] of this.#waiters) {
this.#waiters.delete(pluginId);
for (const waiter of waiters) waiter.reject(getError(pluginId));

@bajrangCoder bajrangCoder Oct 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

getError(pluginId) is called per waiter inside the loop, after the entry has already been deleted. If it ever throws, the remaining waiters for this plugin (and all later plugins) are stranded, and the exception propagates into loadPlugins. Building the error once per plugin before the inner loop avoids that and avoids redundant allocations.

}
}
}
31 changes: 31 additions & 0 deletions tests/unit/pluginWaiters.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { expect, it } from "vitest";

@bajrangCoder bajrangCoder Oct 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These tests cover PluginWaiters in isolation, but none of the interesting failure modes are here — they're in how acode.waitForPlugin is wired to loadPlugins (theme-only pass rejection, timed-out plugins that load later, calls after initial load). Would be worth adding at least one test exercising waitForPlugin + the load callbacks.

import PluginWaiters from "../../src/lib/pluginWaiters";

it("resolves every caller waiting for the same plugin", async () => {
const waiters = new PluginWaiters();
const first = waiters.waitFor("example");
const second = waiters.waitFor("example");

waiters.resolve("example");

await expect(Promise.all([first, second])).resolves.toEqual([
undefined,
undefined,
]);
});

it("rejects every pending caller with its plugin-specific error", async () => {
const waiters = new PluginWaiters();
const first = waiters.waitFor("first");
const second = waiters.waitFor("first");
const third = waiters.waitFor("second");
const pending = Promise.allSettled([first, second, third]);

waiters.rejectAll((pluginId) => new Error(`Plugin '${pluginId}' failed to load.`));

@bajrangCoder bajrangCoder Oct 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: this file isn't Biome-formatted (several lines exceed the 80-col width). It slips through because biome.json files.includes only covers src/** and utils/**, so the biome check mentioned in the PR description doesn't actually check it. Running biome format --write on it explicitly would keep it consistent.

expect(await pending).toEqual([
{ status: "rejected", reason: new Error("Plugin 'first' failed to load.") },
{ status: "rejected", reason: new Error("Plugin 'first' failed to load.") },
{ status: "rejected", reason: new Error("Plugin 'second' failed to load.") },
]);
});
Loading