From 2a08a140abe75d357b73ea4f2977c9ee56400353 Mon Sep 17 00:00:00 2001 From: foxnne Date: Sun, 18 Oct 2026 15:51:18 -0500 Subject: [PATCH 1/2] sdk: a plugin's data folder entry goes with the plugin Host.retirePlugin is the runtime unload path's one call: unregister, the plugin's deinit, then EditorAPI.pluginRetired, where the app stops handing out the plugin's data folder. A plugin can write its state out in deinit, and every app built on App gets this, not only one that runs releaseRetiredFolders. Entries are keyed by the exact id; only the folder name is lowercased. A registration that ran out of memory sets one flag, and the entry is made on the plugin's next ask, replacing the table of eight missed ids. Fingerprint recorded. Part of #530 Co-Authored-By: Claude Opus 5.5 --- app/App.zig | 123 ++++++++++++++++-------------------------- docs/PLUGINS.md | 13 +++-- sdk/src/EditorAPI.zig | 7 ++- sdk/src/Host.zig | 25 ++++++--- sdk/src/version.zig | 2 +- src/editor/Editor.zig | 9 ++-- tests/integration.zig | 37 ++++++++----- 7 files changed, 103 insertions(+), 113 deletions(-) diff --git a/app/App.zig b/app/App.zig index 15cd17ef5..3b89d7996 100644 --- a/app/App.zig +++ b/app/App.zig @@ -75,15 +75,16 @@ config_folder: []const u8, palette_folder: []const u8, -/// Each registered plugin's data folder (`pluginDataFolder`), by lowercased id, filled on the -/// main thread as plugins register (`pluginRegistered`). Key and path owned, and kept after the -/// plugin goes, so a path handed out stays good for the app's life. Guarded by -/// `plugin_data_lock`: a plugin may ask from a worker thread (an indexer, an LSP client). +/// Each registered plugin's data folder (`pluginDataFolder`), by id, filled on the main thread as +/// plugins register (`pluginRegistered`). Key and path owned, and kept after the plugin is +/// retired, so a path handed out stays good for the app's life. Guarded by `plugin_data_lock`: a +/// plugin may ask from a worker thread (an indexer, an LSP client). plugin_data_folders: std.StringHashMapUnmanaged(PluginDataFolder) = .empty, plugin_data_lock: std.Io.Mutex = .init, -/// Registered plugins whose entry could not be made when they registered (out of memory), by the -/// plugin's own id (its memory, good while it is registered): made on their next ask instead. -plugin_data_missed: [8]?[]const u8 = @splat(null), +/// A registration ran out of memory making its plugin's entry (`pluginRegistered`): until one is +/// made, an ask for an id with no entry makes it then. Without this an ask for an unknown id is +/// null and makes nothing. +plugin_data_short: bool = false, /// Plugin registry + service locator exposed to plugins host: Host, @@ -768,26 +769,14 @@ pub fn retireFolder(app: *App) void { pub fn releaseRetiredFolders(app: *App) void { for (app.folder_retired.items) |folder| app.gpa.free(folder); app.folder_retired.clearRetainingCapacity(); - // Plugins unregistered last frame have run their `deinit` by now: their data folders stop - // being handed out (`pluginUnregistered`). - app.lockPluginData(); - defer app.unlockPluginData(); - var it = app.plugin_data_folders.valueIterator(); - while (it.next()) |f| if (f.leaving) { - f.leaving = false; - f.registered = false; - }; } pub const PluginDataFolder = struct { /// Null for an id that names no folder (`isValidPluginId`: `fizzy.store` has a dot in it). path: ?[]const u8, - /// Its plugin is registered now. A path stays in the table after its plugin goes, since a - /// caller may hold it, but is not handed out again until the plugin is back. + /// Its plugin is registered now. A path stays in the table after its plugin is retired, since + /// a caller may hold it, but is not handed out again until the plugin is back. registered: bool = true, - /// Its plugin was unregistered this frame. Still handed out, since its `deinit` (which runs - /// after) may write there; `releaseRetiredFolders`, at the next frame's top, makes it gone. - leaving: bool = false, /// When the folder was last found or made, on `std.Io.Clock.awake`: an ask within /// `plugin_data_recheck_ns` of it trusts it is there. made_at: ?i96 = null, @@ -804,13 +793,6 @@ pub const plugin_data_recheck_ns: i96 = std.time.ns_per_s; /// How long a plugin data folder that could not be made is left before it is tried again. pub const plugin_data_retry_ns: i96 = 5 * std.time.ns_per_s; -/// The key a plugin's data folder is kept under: its id in lowercase, in `buf`. Null for an id -/// longer than any valid one, which then has no folder and no entry. -fn pluginDataKey(buf: *[64]u8, id: []const u8) ?[]const u8 { - if (id.len > buf.len) return null; - return std.ascii.lowerString(buf, id); -} - /// `plugin_data_lock`, which the web, single-threaded and with `std.Io`'s mutex unavailable to /// it, goes without. fn lockPluginData(app: *App) void { @@ -825,51 +807,40 @@ fn unlockPluginData(app: *App) void { /// A plugin registered (`Host.registerPlugin`, through `EditorAPI.pluginRegistered`), on the main /// thread: its data folder's entry is made ready here, so `pluginDataFolder`, asked from any -/// thread, never walks the host's plugin list. Out of memory, it is remembered and made on the -/// plugin's next ask. +/// thread, never walks the host's plugin list. Out of memory, the entry is made on its first ask. pub fn pluginRegistered(app: *App, id: []const u8) void { - var buf: [64]u8 = undefined; - const name = pluginDataKey(&buf, id) orelse return; app.lockPluginData(); defer app.unlockPluginData(); - if (app.plugin_data_folders.getPtr(name)) |f| { + if (app.plugin_data_folders.getPtr(id)) |f| { f.registered = true; - f.leaving = false; return; } - if (app.addPluginDataEntry(name, id) != null) return; - std.log.warn("plugin '{s}' data folder: out of memory; tried again on its first ask", .{id}); - for (&app.plugin_data_missed) |*slot| if (slot.* == null) { - slot.* = id; - return; - }; + if (app.addPluginDataEntry(id) == null) { + std.log.warn("plugin '{s}' data folder: out of memory; tried again on its first ask", .{id}); + app.plugin_data_short = true; + } } -/// A plugin was unregistered (`Host.unregisterPlugin`, through `EditorAPI.pluginUnregistered`), -/// before its `deinit` runs: its folder is still handed out for that, and stops being at the next -/// frame's top (`releaseRetiredFolders`). The entry stays, since a worker may still hold the path. -pub fn pluginUnregistered(app: *App, id: []const u8) void { - var buf: [64]u8 = undefined; - const name = pluginDataKey(&buf, id) orelse return; +/// A plugin was retired (`Host.retirePlugin`, through `EditorAPI.pluginRetired`): its `deinit` +/// has run, so its folder is not handed out until it is back. The entry stays, since a worker +/// may still hold the path. +pub fn pluginRetired(app: *App, id: []const u8) void { app.lockPluginData(); defer app.unlockPluginData(); - for (&app.plugin_data_missed) |*slot| if (slot.*) |missed| { - if (std.mem.eql(u8, missed, id)) slot.* = null; - }; - if (app.plugin_data_folders.getPtr(name)) |f| f.leaving = true; + if (app.plugin_data_folders.getPtr(id)) |f| f.registered = false; } -/// The entry for plugin `id` under `name` (`pluginDataKey`), made now. Null out of memory. Under -/// `plugin_data_lock`. -fn addPluginDataEntry(app: *App, name: []const u8, id: []const u8) ?*PluginDataFolder { - const key = app.gpa.dupe(u8, name) catch return null; - const path: ?[]const u8 = if (isValidPluginId(id)) - std.fs.path.join(app.gpa, &.{ app.config_folder, "plugin-data", name }) catch { +/// The entry for plugin `id`, made now: its folder `/plugin-data/`, or no +/// path for an id that names no folder. Null out of memory. Under `plugin_data_lock`. +fn addPluginDataEntry(app: *App, id: []const u8) ?*PluginDataFolder { + const key = app.gpa.dupe(u8, id) catch return null; + const path: ?[]const u8 = if (isValidPluginId(id)) path: { + var buf: [64]u8 = undefined; // `isValidPluginId`: at most 64 bytes + break :path std.fs.path.join(app.gpa, &.{ app.config_folder, "plugin-data", std.ascii.lowerString(&buf, id) }) catch { app.gpa.free(key); return null; - } - else - null; + }; + } else null; const entry = app.plugin_data_folders.getOrPut(app.gpa, key) catch { app.gpa.free(key); if (path) |p| app.gpa.free(p); @@ -879,9 +850,10 @@ fn addPluginDataEntry(app: *App, name: []const u8, id: []const u8) ?*PluginDataF return entry.value_ptr; } -/// `/plugin-data/` for the registered plugin `id` (`Host.pluginDataFolder`), the id -/// in lowercase, which ids are unique in (`Host.registerPlugin`), so the folder is the same on a -/// case-sensitive filesystem as on macOS and Windows. The path is borrowed for the app's life. +/// `/plugin-data/` for plugin `id` (`Host.pluginDataFolder`, which passes the +/// caller's own plugin's id, exactly). The folder name is the id in lowercase: `Host.registerPlugin` +/// refuses an id that differs only in case from a registered one, so the folder is this plugin's +/// alone on every filesystem. The path is borrowed for the app's life. /// /// Made on the first ask, and made sure of again on an ask a second or more after the last: a /// folder deleted while the app runs comes back then, and a caller whose write into a kept path @@ -890,25 +862,20 @@ fn addPluginDataEntry(app: *App, name: []const u8, id: []const u8) ?*PluginDataF /// /// Separate from `plugins/`, which an update replaces and an uninstall deletes. On the web /// nothing is made, as there are no directories: the path is the prefix `core.fs` files under in -/// the page's storage, as `palette_folder` is. Null for an id no registered plugin has, for one -/// that is not a plain identifier (said once), or when the folder can't be made: said once for -/// each run of failures, and tried again on an ask `plugin_data_retry_ns` after the last. +/// the page's storage, as `palette_folder` is. Null for a retired plugin, for an id that is not a +/// plain identifier (said once), or when the folder can't be made: said once for each run of +/// failures, and tried again on an ask `plugin_data_retry_ns` after the last. pub fn pluginDataFolder(app: *App, id: []const u8) ?[]const u8 { - var buf: [64]u8 = undefined; - const name = pluginDataKey(&buf, id) orelse return null; + if (!isValidPluginId(id)) return null; const is_wasm = comptime builtin.target.cpu.arch == .wasm32; const path = path: { app.lockPluginData(); defer app.unlockPluginData(); - const f = app.plugin_data_folders.getPtr(name) orelse missed: { - // Registered while memory ran out (`pluginRegistered`): made now. - for (&app.plugin_data_missed) |*slot| if (slot.*) |missed| { - if (!std.ascii.eqlIgnoreCase(missed, id)) continue; - const made = app.addPluginDataEntry(name, missed) orelse return null; - slot.* = null; - break :missed made; - }; - return null; + const f = app.plugin_data_folders.getPtr(id) orelse made: { + // No entry: unknown, unless a registration ran out of memory making it + // (`pluginRegistered`), in which case it is made now. + if (!app.plugin_data_short) return null; + break :made app.addPluginDataEntry(id) orelse return null; }; if (!f.registered) return null; const path = f.path orelse { @@ -927,12 +894,12 @@ pub fn pluginDataFolder(app: *App, id: []const u8) ?[]const u8 { const now = std.Io.Clock.awake.now(dvui.io).nanoseconds; app.lockPluginData(); defer app.unlockPluginData(); - const f = app.plugin_data_folders.getPtr(name) orelse return null; + const f = app.plugin_data_folders.getPtr(id) orelse return null; if (made) |_| { f.made_at = now; f.failed_at = null; f.said = false; - // Its plugin may have gone while the folder was made. + // Its plugin may have been retired while the folder was made. return if (f.registered) path else null; } else |err| { // Said once for a run of failures: an ask every frame would say it every frame. diff --git a/docs/PLUGINS.md b/docs/PLUGINS.md index c6f79fc63..f4f4b0c28 100644 --- a/docs/PLUGINS.md +++ b/docs/PLUGINS.md @@ -884,16 +884,19 @@ during `register` and read back via `@ptrCast` in your vtable functions. There i What a plugin keeps **between runs** that is not a setting — its recents, an index, a cache — goes in **`host.pluginDataFolder(&plugin)`**: `/plugin-data/`, the id in lowercase. Plugin ids are unique ignoring case (`host.registerPlugin` refuses `Graphl` beside `graphl`, and -a loader asks `host.pluginByIdIgnoringCase` before it opens a build), so the folder is the same -on every filesystem. It follows the -app's config folder, so a `FIZZY_PROFILE` sandbox gets its own, and it is not `plugins/`, the -folder the plugin is installed in, which an update replaces and an uninstall deletes. It is -always local, whether or not the open folder is on a mount (§3.12). +a loader asks `host.pluginByIdIgnoringCase` before it opens a build), so the folder is the +plugin's alone on every filesystem. It follows the app's config folder, so a `FIZZY_PROFILE` +sandbox gets its own, and it is not `plugins/`, the folder the plugin is installed in, which +an update replaces and an uninstall deletes. It is always local, whether or not the open folder +is on a mount (§3.12). - The path is borrowed for the life of the app. The folder is made on the first call and made sure of again on a call a second or more after the last, so a plugin whose write into a kept path fails with `FileNotFound` calls again (a second later, if it called just now). Safe from any thread (an indexer, an LSP client): the app sets each plugin's entry up as it registers. +- It is yours from `register` through your `deinit`: an app unloads a plugin with + `host.retirePlugin` (unregister, your `deinit`, then the app is told), so writing your state + out in `deinit` works, and from then on the path is not handed out until you are back. - Null when no app is installed (a unit test's bare `Host`), when the plugin is not registered, when its id is not a plain identifier (letters, digits, `-`, `_`), or when the folder could not be made: logged once, and tried again on a call a few seconds later. diff --git a/sdk/src/EditorAPI.zig b/sdk/src/EditorAPI.zig index 4a2db8364..3142140b1 100644 --- a/sdk/src/EditorAPI.zig +++ b/sdk/src/EditorAPI.zig @@ -159,10 +159,9 @@ pub const VTable = struct { /// plugin is made ready here, so asks from other threads never walk the host's plugin list. /// Null where the app keeps nothing per plugin. pluginRegistered: ?*const fn (ctx: *anyopaque, id: []const u8) void = null, - /// A plugin was unregistered (`Host.unregisterPlugin`), before its `deinit` runs: what the app - /// keeps for it must still serve that `deinit`, and goes once it has run. Null where the app - /// keeps nothing per plugin. - pluginUnregistered: ?*const fn (ctx: *anyopaque, id: []const u8) void = null, + /// A plugin was taken out for good (`Host.retirePlugin`): unregistered, and its `deinit` has + /// run, so what the app kept for it goes now. Null where the app keeps nothing per plugin. + pluginRetired: ?*const fn (ctx: *anyopaque, id: []const u8) void = null, /// Mark fizzy's settings dirty so the debounced autosave persists them. markSettingsDirty: *const fn (ctx: *anyopaque) void, /// A credential kept for a plugin, by key (`.`), outside `settings.zon` — diff --git a/sdk/src/Host.zig b/sdk/src/Host.zig index 09accc4b4..0166449bb 100644 --- a/sdk/src/Host.zig +++ b/sdk/src/Host.zig @@ -448,9 +448,10 @@ pub fn paletteFolder(self: *Host) ?[]const u8 { /// comes back then: a plugin whose write into a kept path fails with `FileNotFound` calls again /// (a second later, if it called just now). Safe to call from any thread. /// -/// Null when no app is installed, when `plugin` is not registered, when its id is not a plain -/// identifier (letters, digits, `-`, `_`; logged once), or when the folder could not be made -/// (logged once, and tried again on a call a few seconds later). +/// Null when no app is installed, when `plugin` is not registered (or has been retired, +/// `retirePlugin`), when its id is not a plain identifier (letters, digits, `-`, `_`; logged +/// once), or when the folder could not be made (logged once, and tried again on a call a few +/// seconds later). /// /// On the web, as with `paletteFolder`, the path names no directory: it is the prefix under /// which `core.fs.read`, `readZ`, `write` and `remove` keep files in the page's storage. Those @@ -976,6 +977,17 @@ pub fn registerPlugin(self: *Host, plugin: *Plugin) !void { if (self.fizzy_api) |a| if (a.vtable.pluginRegistered) |registered| registered(a.ctx, plugin.id); } +/// Take `plugin` out for good: `unregisterPlugin`, then its own `deinit`, then the app is told +/// (`EditorAPI.pluginRetired`) so what it kept for the plugin (its data folder's entry) goes now, +/// after a `deinit` that may still have used it. The runtime unload path's one call: unload, +/// disable, reload and uninstall all come through it, in any app. Before `dlclose`, as +/// `unregisterPlugin`. +pub fn retirePlugin(self: *Host, plugin: *Plugin) void { + self.unregisterPlugin(plugin); + plugin.deinit(); + if (self.fizzy_api) |a| if (a.vtable.pluginRetired) |retired| retired(a.ctx, plugin.id); +} + /// Remove every contribution, service, and registry entry owned by `plugin`, then drop /// the plugin itself. The inverse of `registerPlugin` + the `register*` calls a plugin /// makes in its `register`. Used by the runtime unload path (the store's "disable" / @@ -986,7 +998,6 @@ pub fn registerPlugin(self: *Host, plugin: *Plugin) !void { /// this *before* `dlclose`, so that the active-selection ids (which may point into that /// image) are compared and reset while the memory is still mapped. pub fn unregisterPlugin(self: *Host, plugin: *Plugin) void { - if (self.fizzy_api) |a| if (a.vtable.pluginUnregistered) |gone| gone(a.ctx, plugin.id); for (self.surfaces.items) |s| if (s.owner == plugin) self.dropSurfaceTitle(s.id); removeOwned(Surface, &self.surfaces, plugin); removeOwned(MenuContribution, &self.menus, plugin); @@ -2906,7 +2917,7 @@ test "pluginForExtension honors a user assignment and ignores a stale one" { // Only the one vtable member `pluginForExtension` reaches; the rest stay unreachable here. var api_vt: EditorAPI.VTable = undefined; api_vt.pluginRegistered = null; - api_vt.pluginUnregistered = null; + api_vt.pluginRetired = null; api_vt.extensionOwnerOverride = Fake.extensionOwnerOverride; var ctx: u8 = 0; host.installFizzyApi(.{ .ctx = &ctx, .vtable = &api_vt }); @@ -3298,7 +3309,7 @@ test "a claim on a document holds an override of its owner's command too" { S.owner = &pixi; var api_vt: EditorAPI.VTable = undefined; api_vt.pluginRegistered = null; - api_vt.pluginUnregistered = null; + api_vt.pluginRetired = null; api_vt.activeDoc = S.active; api_vt.refresh = S.refresh; var ctx: u8 = 0; @@ -3607,7 +3618,7 @@ test "commands with arguments: callCommand, runCommand's fallback, and listing" // ...and something required has the app ask for it. var api_vt: EditorAPI.VTable = undefined; api_vt.pluginRegistered = null; - api_vt.pluginUnregistered = null; + api_vt.pluginRetired = null; api_vt.askCommandArguments = S.ask; var ctx: u8 = 0; host.installFizzyApi(.{ .ctx = &ctx, .vtable = &api_vt }); diff --git a/sdk/src/version.zig b/sdk/src/version.zig index 222378de1..47baec4da 100644 --- a/sdk/src/version.zig +++ b/sdk/src/version.zig @@ -75,7 +75,7 @@ pub const sdk_version = @import("sdk_version").sdk_version; /// why it is a single target/mode-invariant literal rather than a per-target table. Update this /// value from the `@compileError` it triggers whenever it changes; leave `sdk_version` to the /// next SDK release. -pub const recorded_sdk_shape_fingerprint: u64 = 0xba4664870675e13d; +pub const recorded_sdk_shape_fingerprint: u64 = 0x23ccb903d7a4fb41; comptime { if (dylib.sdk_shape_fingerprint != recorded_sdk_shape_fingerprint) { diff --git a/src/editor/Editor.zig b/src/editor/Editor.zig index c94fba659..54b39a5bb 100644 --- a/src/editor/Editor.zig +++ b/src/editor/Editor.zig @@ -1923,9 +1923,8 @@ pub fn unloadPlugin(editor: *Editor, id: []const u8, opts: UnloadOptions) Unload // Remove all contributions + services + active-id references (before dlclose), then // run the plugin's own teardown. - editor.app.host.unregisterPlugin(plugin); + editor.app.host.retirePlugin(plugin); fizzy.backend.rebuildDynamicNativeMenus(); - plugin.deinit(); // Drop the unloaded plugin's keybinds by rebuilding from the survivors. rebuildKeybinds(editor); @@ -2410,7 +2409,7 @@ const fizzy_api_vtable: sdk.EditorAPI.VTable = .{ .paletteFolder = fizzyPaletteFolder, .pluginDataFolder = fizzyPluginDataFolder, .pluginRegistered = fizzyPluginRegistered, - .pluginUnregistered = fizzyPluginUnregistered, + .pluginRetired = fizzyPluginRetired, .getSecret = fizzyGetSecret, .setSecret = fizzySetSecret, .markSettingsDirty = fizzyMarkSettingsDirty, @@ -2583,8 +2582,8 @@ fn fizzyPluginDataFolder(ctx: *anyopaque, id: []const u8) ?[]const u8 { fn fizzyPluginRegistered(ctx: *anyopaque, id: []const u8) void { fizzyCtx(ctx).app.pluginRegistered(id); } -fn fizzyPluginUnregistered(ctx: *anyopaque, id: []const u8) void { - fizzyCtx(ctx).app.pluginUnregistered(id); +fn fizzyPluginRetired(ctx: *anyopaque, id: []const u8) void { + fizzyCtx(ctx).app.pluginRetired(id); } fn fizzyGetSecret(ctx: *anyopaque, key: []const u8) ?[]const u8 { return fizzyCtx(ctx).app.secrets.get(key); diff --git a/tests/integration.zig b/tests/integration.zig index dc36c1b40..8c6204eef 100644 --- a/tests/integration.zig +++ b/tests/integration.zig @@ -7600,30 +7600,41 @@ test "headless: the whole editor comes up, opens a file, plays the tour and goes try std.testing.expectEqual(data.ptr, (host.pluginDataFolder(text_plugin) orelse return error.TestUnexpectedResult).ptr); const other = host.pluginDataFolder(markdown_plugin) orelse return error.TestUnexpectedResult; try std.testing.expect(!std.mem.eql(u8, data, other)); - // An id no registered plugin has gets no folder, and no entry; ids are one ignoring case. + // An id no registered plugin has gets no folder, and no entry, exactly as asked: `Text` + // is not `text`. try std.testing.expect(editor.app.pluginDataFolder("../escape") == null); try std.testing.expect(editor.app.plugin_data_folders.get("../escape") == null); - try std.testing.expectEqual(data.ptr, (editor.app.pluginDataFolder("Text") orelse return error.TestUnexpectedResult).ptr); + try std.testing.expect(editor.app.pluginDataFolder("Text") == null); + try std.testing.expect(editor.app.plugin_data_folders.get("Text") == null); const level = std.testing.log_level; std.testing.log_level = .err; // the warnings the cases below ask for defer std.testing.log_level = level; const vt = sdk.Plugin.VTable{}; - // A plugin whose id has capitals gets it in lowercase, the same folder on every - // filesystem; another in a different case of it is not registered at all. + // A plugin whose id has capitals gets it in lowercase; another in a different case of it + // is not registered at all. Retired (`Host.retirePlugin`), its folder is still its own + // in its `deinit`, and not handed out after. { - var cased = sdk.Plugin{ .state = undefined, .vtable = &vt, .id = "CaseTest", .display_name = "Case Test" }; - try host.registerPlugin(&cased); + const Retiring = struct { + var plugin: sdk.Plugin = undefined; + var host_seen: ?*sdk.Host = null; + var in_deinit: ?bool = null; + fn deinit(_: *anyopaque) void { + in_deinit = host_seen.?.pluginDataFolder(&plugin) != null; + } + }; + const retiring_vt = sdk.Plugin.VTable{ .deinit = Retiring.deinit }; + var state: u8 = 0; + Retiring.plugin = .{ .state = &state, .vtable = &retiring_vt, .id = "CaseTest", .display_name = "Case Test" }; + Retiring.host_seen = host; + try host.registerPlugin(&Retiring.plugin); const want_cased = try std.fs.path.join(gpa, &.{ root, "plugin-data", "casetest" }); defer gpa.free(want_cased); - try std.testing.expectEqualStrings(want_cased, host.pluginDataFolder(&cased) orelse return error.TestUnexpectedResult); + try std.testing.expectEqualStrings(want_cased, host.pluginDataFolder(&Retiring.plugin) orelse return error.TestUnexpectedResult); var lower = sdk.Plugin{ .state = undefined, .vtable = &vt, .id = "casetest", .display_name = "Case Test" }; try std.testing.expectError(error.DuplicatePluginId, host.registerPlugin(&lower)); - // Unregistered, its path is still handed out for its `deinit`, which runs after; from - // the next frame's top it is not. - host.unregisterPlugin(&cased); - try std.testing.expect(host.pluginDataFolder(&cased) != null); - editor.app.releaseRetiredFolders(); - try std.testing.expect(host.pluginDataFolder(&cased) == null); + host.retirePlugin(&Retiring.plugin); + try std.testing.expectEqual(@as(?bool, true), Retiring.in_deinit); + try std.testing.expect(host.pluginDataFolder(&Retiring.plugin) == null); } // An id that is not a plain identifier names no folder. { From 556338324a153842d777caf8a8f0e028990cd406 Mon Sep 17 00:00:00 2001 From: foxnne Date: Sun, 18 Oct 2026 16:20:14 -0500 Subject: [PATCH 2/2] sdk: a retired plugin's folder entry is keyed, kept and released safely Review of the data folder's retirement. retirePlugin copies the id before the plugin's deinit and takes a before_deinit step, which fizzy uses to rebuild its native menus before deinit as before; unregisterPlugin alone still releases the folder, so no caller leaves a live one. Entries are keyed by the lowercased id, one per folder; ids that are not plain identifiers get none and are said once; only registrations that ran out of memory get an entry made on their next ask. Part of #530 Co-Authored-By: Claude Opus 5.5 --- app/App.zig | 153 ++++++++++++++++++++++++++---------------- docs/PLUGINS.md | 5 +- sdk/src/Host.zig | 38 +++++++++-- src/editor/Editor.zig | 4 +- tests/integration.zig | 34 +++++++--- 5 files changed, 157 insertions(+), 77 deletions(-) diff --git a/app/App.zig b/app/App.zig index 3b89d7996..1bb787308 100644 --- a/app/App.zig +++ b/app/App.zig @@ -75,16 +75,20 @@ config_folder: []const u8, palette_folder: []const u8, -/// Each registered plugin's data folder (`pluginDataFolder`), by id, filled on the main thread as -/// plugins register (`pluginRegistered`). Key and path owned, and kept after the plugin is +/// Each registered plugin's data folder (`pluginDataFolder`), by lowercased id, one entry per +/// folder: `Host.registerPlugin` refuses ids differing only in case, so a retired `Graphl` and a +/// later `graphl` are one folder with one entry. Filled on the main thread as plugins register +/// (`pluginRegistered`). Key and path owned, and kept after the plugin is /// retired, so a path handed out stays good for the app's life. Guarded by `plugin_data_lock`: a /// plugin may ask from a worker thread (an indexer, an LSP client). plugin_data_folders: std.StringHashMapUnmanaged(PluginDataFolder) = .empty, plugin_data_lock: std.Io.Mutex = .init, -/// A registration ran out of memory making its plugin's entry (`pluginRegistered`): until one is -/// made, an ask for an id with no entry makes it then. Without this an ask for an unknown id is -/// null and makes nothing. -plugin_data_short: bool = false, +/// Registered plugins whose entry could not be made when they registered (out of memory), by +/// lowercased id, owned: only these get one made on their next ask. An unknown id never does. +plugin_data_pending: std.ArrayListUnmanaged([]u8) = .empty, +/// Ids that asked for a data folder they cannot have (not a plain identifier), owned: each is +/// said once. +plugin_data_refused: std.ArrayListUnmanaged([]u8) = .empty, /// Plugin registry + service locator exposed to plugins host: Host, @@ -772,8 +776,7 @@ pub fn releaseRetiredFolders(app: *App) void { } pub const PluginDataFolder = struct { - /// Null for an id that names no folder (`isValidPluginId`: `fizzy.store` has a dot in it). - path: ?[]const u8, + path: []const u8, /// Its plugin is registered now. A path stays in the table after its plugin is retired, since /// a caller may hold it, but is not handed out again until the plugin is back. registered: bool = true, @@ -783,8 +786,7 @@ pub const PluginDataFolder = struct { /// When making it last failed (a read-only or full disk): asks within `plugin_data_retry_ns` /// of it get null without trying. failed_at: ?i96 = null, - /// Something was said about this folder (an id that names none, a run of failures), so it - /// is not said again until things change. + /// A run of failures was said, so it is not said again until the folder is made. said: bool = false, }; @@ -805,55 +807,75 @@ fn unlockPluginData(app: *App) void { app.plugin_data_lock.unlock(dvui.io); } +/// The key a plugin's entry is kept under: its id in lowercase, in `buf`. Null for an id that is +/// not a plain identifier (`isValidPluginId`: `fizzy.store` has a dot in it), which has no folder. +fn pluginDataKey(buf: *[64]u8, id: []const u8) ?[]const u8 { + if (!isValidPluginId(id)) return null; + return std.ascii.lowerString(buf, id); +} + /// A plugin registered (`Host.registerPlugin`, through `EditorAPI.pluginRegistered`), on the main /// thread: its data folder's entry is made ready here, so `pluginDataFolder`, asked from any -/// thread, never walks the host's plugin list. Out of memory, the entry is made on its first ask. +/// thread, never walks the host's plugin list. Out of memory, the id is kept to make it on its +/// first ask. pub fn pluginRegistered(app: *App, id: []const u8) void { + var buf: [64]u8 = undefined; + const key = pluginDataKey(&buf, id) orelse return; app.lockPluginData(); defer app.unlockPluginData(); - if (app.plugin_data_folders.getPtr(id)) |f| { + if (app.plugin_data_folders.getPtr(key)) |f| { f.registered = true; return; } - if (app.addPluginDataEntry(id) == null) { - std.log.warn("plugin '{s}' data folder: out of memory; tried again on its first ask", .{id}); - app.plugin_data_short = true; - } + if (app.addPluginDataEntry(key) != null) return; + std.log.warn("plugin '{s}' data folder: out of memory; tried again on its first ask", .{id}); + const kept = app.gpa.dupe(u8, key) catch return; + app.plugin_data_pending.append(app.gpa, kept) catch app.gpa.free(kept); } -/// A plugin was retired (`Host.retirePlugin`, through `EditorAPI.pluginRetired`): its `deinit` -/// has run, so its folder is not handed out until it is back. The entry stays, since a worker -/// may still hold the path. +/// A plugin was retired (`Host.retirePlugin`, after its `deinit`, or `Host.unregisterPlugin`, +/// through `EditorAPI.pluginRetired`): its folder is not handed out until it is back. The entry +/// stays, since a worker may still hold the path. pub fn pluginRetired(app: *App, id: []const u8) void { + var buf: [64]u8 = undefined; + const key = pluginDataKey(&buf, id) orelse return; app.lockPluginData(); defer app.unlockPluginData(); - if (app.plugin_data_folders.getPtr(id)) |f| f.registered = false; + if (app.plugin_data_folders.getPtr(key)) |f| f.registered = false; + _ = app.takePending(key); } -/// The entry for plugin `id`, made now: its folder `/plugin-data/`, or no -/// path for an id that names no folder. Null out of memory. Under `plugin_data_lock`. -fn addPluginDataEntry(app: *App, id: []const u8) ?*PluginDataFolder { - const key = app.gpa.dupe(u8, id) catch return null; - const path: ?[]const u8 = if (isValidPluginId(id)) path: { - var buf: [64]u8 = undefined; // `isValidPluginId`: at most 64 bytes - break :path std.fs.path.join(app.gpa, &.{ app.config_folder, "plugin-data", std.ascii.lowerString(&buf, id) }) catch { - app.gpa.free(key); - return null; - }; - } else null; - const entry = app.plugin_data_folders.getOrPut(app.gpa, key) catch { - app.gpa.free(key); - if (path) |p| app.gpa.free(p); +/// Whether `key` is waiting for its entry (`plugin_data_pending`), which it no longer is. Under +/// `plugin_data_lock`. +fn takePending(app: *App, key: []const u8) bool { + for (app.plugin_data_pending.items, 0..) |p, i| if (std.mem.eql(u8, p, key)) { + app.gpa.free(app.plugin_data_pending.swapRemove(i)); + return true; + }; + return false; +} + +/// The entry for `key` (`pluginDataKey`), made now: its folder `/plugin-data/`. Null +/// out of memory. Under `plugin_data_lock`. +fn addPluginDataEntry(app: *App, key: []const u8) ?*PluginDataFolder { + const owned = app.gpa.dupe(u8, key) catch return null; + const path = std.fs.path.join(app.gpa, &.{ app.config_folder, "plugin-data", key }) catch { + app.gpa.free(owned); + return null; + }; + const entry = app.plugin_data_folders.getOrPut(app.gpa, owned) catch { + app.gpa.free(owned); + app.gpa.free(path); return null; }; entry.value_ptr.* = .{ .path = path }; return entry.value_ptr; } -/// `/plugin-data/` for plugin `id` (`Host.pluginDataFolder`, which passes the -/// caller's own plugin's id, exactly). The folder name is the id in lowercase: `Host.registerPlugin` -/// refuses an id that differs only in case from a registered one, so the folder is this plugin's -/// alone on every filesystem. The path is borrowed for the app's life. +/// `/plugin-data/` for the registered plugin `id` (`Host.pluginDataFolder`, +/// which passes the caller's own plugin's id). `Host.registerPlugin` refuses an id that differs +/// only in case from a registered one, so the folder is this plugin's alone on every filesystem. +/// The path is borrowed for the app's life. /// /// Made on the first ask, and made sure of again on an ask a second or more after the last: a /// folder deleted while the app runs comes back then, and a caller whose write into a kept path @@ -862,39 +884,43 @@ fn addPluginDataEntry(app: *App, id: []const u8) ?*PluginDataFolder { /// /// Separate from `plugins/`, which an update replaces and an uninstall deletes. On the web /// nothing is made, as there are no directories: the path is the prefix `core.fs` files under in -/// the page's storage, as `palette_folder` is. Null for a retired plugin, for an id that is not a -/// plain identifier (said once), or when the folder can't be made: said once for each run of -/// failures, and tried again on an ask `plugin_data_retry_ns` after the last. +/// the page's storage, as `palette_folder` is. Null for an id no registered plugin has, a retired +/// plugin's, an id that is not a plain identifier (said once), or when the folder can't be made: +/// said once for each run of failures, and tried again on an ask `plugin_data_retry_ns` after the +/// last. pub fn pluginDataFolder(app: *App, id: []const u8) ?[]const u8 { - if (!isValidPluginId(id)) return null; const is_wasm = comptime builtin.target.cpu.arch == .wasm32; + var buf: [64]u8 = undefined; const path = path: { app.lockPluginData(); defer app.unlockPluginData(); - const f = app.plugin_data_folders.getPtr(id) orelse made: { - // No entry: unknown, unless a registration ran out of memory making it - // (`pluginRegistered`), in which case it is made now. - if (!app.plugin_data_short) return null; - break :made app.addPluginDataEntry(id) orelse return null; - }; - if (!f.registered) return null; - const path = f.path orelse { - if (!f.said) std.log.warn("plugin '{s}' has no data folder: its id is not a plain identifier (letters, digits, '-', '_')", .{id}); - f.said = true; + const key = pluginDataKey(&buf, id) orelse { + app.sayNoFolder(id); return null; }; - if (is_wasm) return path; + const f = app.plugin_data_folders.getPtr(key) orelse made: { + // Registered while memory ran out (`pluginRegistered`): made now. Anything else + // asking is no registered plugin. + if (!app.takePending(key)) return null; + break :made app.addPluginDataEntry(key) orelse { + const kept = app.gpa.dupe(u8, key) catch return null; + app.plugin_data_pending.append(app.gpa, kept) catch app.gpa.free(kept); + return null; + }; + }; + if (!f.registered) return null; + if (is_wasm) return f.path; const now = std.Io.Clock.awake.now(dvui.io).nanoseconds; if (f.failed_at) |at| if (now - at < plugin_data_retry_ns) return null; - if (f.made_at) |at| if (now - at < plugin_data_recheck_ns) return path; - break :path path; + if (f.made_at) |at| if (now - at < plugin_data_recheck_ns) return f.path; + break :path f.path; }; const made = std.Io.Dir.cwd().createDirPath(dvui.io, path); const now = std.Io.Clock.awake.now(dvui.io).nanoseconds; app.lockPluginData(); defer app.unlockPluginData(); - const f = app.plugin_data_folders.getPtr(id) orelse return null; + const f = app.plugin_data_folders.getPtr(pluginDataKey(&buf, id).?) orelse return null; if (made) |_| { f.made_at = now; f.failed_at = null; @@ -911,6 +937,15 @@ pub fn pluginDataFolder(app: *App, id: []const u8) ?[]const u8 { } } +/// Say once, per id, that `id` has no data folder: it is not a plain identifier. Under +/// `plugin_data_lock`. +fn sayNoFolder(app: *App, id: []const u8) void { + for (app.plugin_data_refused.items) |r| if (std.mem.eql(u8, r, id)) return; + std.log.warn("plugin '{s}' has no data folder: its id is not a plain identifier (letters, digits, '-', '_')", .{id}); + const kept = app.gpa.dupe(u8, id) catch return; + app.plugin_data_refused.append(app.gpa, kept) catch app.gpa.free(kept); +} + /// What the app itself owns and frees: the plugin data folders and the per-plugin id lists. /// Fizzy's `Editor.deinit` calls it; an app running `App` without the editor calls it at its own /// teardown. @@ -918,9 +953,13 @@ pub fn deinit(app: *App) void { var it = app.plugin_data_folders.iterator(); while (it.next()) |e| { app.gpa.free(e.key_ptr.*); - if (e.value_ptr.path) |p| app.gpa.free(p); + app.gpa.free(e.value_ptr.path); } app.plugin_data_folders.deinit(app.gpa); + for (app.plugin_data_pending.items) |p| app.gpa.free(p); + app.plugin_data_pending.deinit(app.gpa); + for (app.plugin_data_refused.items) |r| app.gpa.free(r); + app.plugin_data_refused.deinit(app.gpa); for (app.disabled_plugin_ids.items) |id| app.gpa.free(id); app.disabled_plugin_ids.deinit(app.gpa); for (app.undecided_plugin_ids.items) |id| app.gpa.free(id); diff --git a/docs/PLUGINS.md b/docs/PLUGINS.md index f4f4b0c28..c572dc6f9 100644 --- a/docs/PLUGINS.md +++ b/docs/PLUGINS.md @@ -895,8 +895,9 @@ is on a mount (§3.12). path fails with `FileNotFound` calls again (a second later, if it called just now). Safe from any thread (an indexer, an LSP client): the app sets each plugin's entry up as it registers. - It is yours from `register` through your `deinit`: an app unloads a plugin with - `host.retirePlugin` (unregister, your `deinit`, then the app is told), so writing your state - out in `deinit` works, and from then on the path is not handed out until you are back. + `host.retirePlugin` (your registrations go, then your `deinit`, then the app is told), so + writing your state out in `deinit` works, and from then on the path is not handed out until + you are back. `host.unregisterPlugin` alone releases it at once. - Null when no app is installed (a unit test's bare `Host`), when the plugin is not registered, when its id is not a plain identifier (letters, digits, `-`, `_`), or when the folder could not be made: logged once, and tried again on a call a few seconds later. diff --git a/sdk/src/Host.zig b/sdk/src/Host.zig index 0166449bb..fdac6d480 100644 --- a/sdk/src/Host.zig +++ b/sdk/src/Host.zig @@ -977,15 +977,28 @@ pub fn registerPlugin(self: *Host, plugin: *Plugin) !void { if (self.fizzy_api) |a| if (a.vtable.pluginRegistered) |registered| registered(a.ctx, plugin.id); } -/// Take `plugin` out for good: `unregisterPlugin`, then its own `deinit`, then the app is told +/// Take `plugin` out for good: its registrations go (`unregisterPlugin`'s work), then +/// `before_deinit` runs if given (fizzy rebuilds its native menus, which may still name the +/// plugin's strings), then the plugin's own `deinit`, then the app is told /// (`EditorAPI.pluginRetired`) so what it kept for the plugin (its data folder's entry) goes now, -/// after a `deinit` that may still have used it. The runtime unload path's one call: unload, -/// disable, reload and uninstall all come through it, in any app. Before `dlclose`, as -/// `unregisterPlugin`. -pub fn retirePlugin(self: *Host, plugin: *Plugin) void { - self.unregisterPlugin(plugin); +/// after a `deinit` that may still have used it. The unload path's one call: unload, disable, +/// reload and uninstall all come through it, in any app. Before `dlclose`, as `unregisterPlugin`. +pub fn retirePlugin(self: *Host, plugin: *Plugin, before_deinit: ?*const fn () void) void { + // The id is the plugin's own memory: copied before its `deinit`, which may free it. One longer + // than any valid id has nothing kept for it to tell about. + var buf: [64]u8 = undefined; + const told = plugin.id.len <= buf.len; + const n = if (told) plugin.id.len else 0; + @memcpy(buf[0..n], plugin.id[0..n]); + self.dropRegistrations(plugin); + if (before_deinit) |f| f(); plugin.deinit(); - if (self.fizzy_api) |a| if (a.vtable.pluginRetired) |retired| retired(a.ctx, plugin.id); + if (told) self.tellRetired(buf[0..n]); +} + +/// The app is told `id` is gone (`EditorAPI.pluginRetired`). +fn tellRetired(self: *Host, id: []const u8) void { + if (self.fizzy_api) |a| if (a.vtable.pluginRetired) |retired| retired(a.ctx, id); } /// Remove every contribution, service, and registry entry owned by `plugin`, then drop @@ -997,7 +1010,18 @@ pub fn retirePlugin(self: *Host, plugin: *Plugin) void { /// `*Plugin` itself live in the plugin image's static memory. The caller must invoke /// this *before* `dlclose`, so that the active-selection ids (which may point into that /// image) are compared and reset while the memory is still mapped. +/// +/// What the app kept for `plugin` goes too (`EditorAPI.pluginRetired`), so no caller leaves a +/// live data folder behind. To unload a plugin that still writes out its state in `deinit`, call +/// `retirePlugin`, which tells the app only after that `deinit`. pub fn unregisterPlugin(self: *Host, plugin: *Plugin) void { + self.dropRegistrations(plugin); + self.tellRetired(plugin.id); +} + +/// `unregisterPlugin`'s work without telling the app: `retirePlugin` tells it after the plugin's +/// `deinit`. +fn dropRegistrations(self: *Host, plugin: *Plugin) void { for (self.surfaces.items) |s| if (s.owner == plugin) self.dropSurfaceTitle(s.id); removeOwned(Surface, &self.surfaces, plugin); removeOwned(MenuContribution, &self.menus, plugin); diff --git a/src/editor/Editor.zig b/src/editor/Editor.zig index 54b39a5bb..d0376b15a 100644 --- a/src/editor/Editor.zig +++ b/src/editor/Editor.zig @@ -1923,8 +1923,8 @@ pub fn unloadPlugin(editor: *Editor, id: []const u8, opts: UnloadOptions) Unload // Remove all contributions + services + active-id references (before dlclose), then // run the plugin's own teardown. - editor.app.host.retirePlugin(plugin); - fizzy.backend.rebuildDynamicNativeMenus(); + // Native menus rebuilt before the plugin's `deinit`, as they may still name its strings. + editor.app.host.retirePlugin(plugin, fizzy.backend.rebuildDynamicNativeMenus); // Drop the unloaded plugin's keybinds by rebuilding from the survivors. rebuildKeybinds(editor); diff --git a/tests/integration.zig b/tests/integration.zig index 8c6204eef..1536e79d3 100644 --- a/tests/integration.zig +++ b/tests/integration.zig @@ -7600,15 +7600,16 @@ test "headless: the whole editor comes up, opens a file, plays the tour and goes try std.testing.expectEqual(data.ptr, (host.pluginDataFolder(text_plugin) orelse return error.TestUnexpectedResult).ptr); const other = host.pluginDataFolder(markdown_plugin) orelse return error.TestUnexpectedResult; try std.testing.expect(!std.mem.eql(u8, data, other)); - // An id no registered plugin has gets no folder, and no entry, exactly as asked: `Text` - // is not `text`. - try std.testing.expect(editor.app.pluginDataFolder("../escape") == null); - try std.testing.expect(editor.app.plugin_data_folders.get("../escape") == null); - try std.testing.expect(editor.app.pluginDataFolder("Text") == null); - try std.testing.expect(editor.app.plugin_data_folders.get("Text") == null); const level = std.testing.log_level; std.testing.log_level = .err; // the warnings the cases below ask for defer std.testing.log_level = level; + // An id that is not a plain identifier gets no folder and no entry, and one no plugin + // registered gets none either; `Text` is `text`'s folder, one entry per folder. + try std.testing.expect(editor.app.pluginDataFolder("../escape") == null); + try std.testing.expect(editor.app.plugin_data_folders.get("../escape") == null); + try std.testing.expect(editor.app.pluginDataFolder("nobody") == null); + try std.testing.expect(editor.app.plugin_data_folders.get("nobody") == null); + try std.testing.expectEqual(data.ptr, (editor.app.pluginDataFolder("Text") orelse return error.TestUnexpectedResult).ptr); const vt = sdk.Plugin.VTable{}; // A plugin whose id has capitals gets it in lowercase; another in a different case of it // is not registered at all. Retired (`Host.retirePlugin`), its folder is still its own @@ -7632,15 +7633,30 @@ test "headless: the whole editor comes up, opens a file, plays the tour and goes try std.testing.expectEqualStrings(want_cased, host.pluginDataFolder(&Retiring.plugin) orelse return error.TestUnexpectedResult); var lower = sdk.Plugin{ .state = undefined, .vtable = &vt, .id = "casetest", .display_name = "Case Test" }; try std.testing.expectError(error.DuplicatePluginId, host.registerPlugin(&lower)); - host.retirePlugin(&Retiring.plugin); + const kept = host.pluginDataFolder(&Retiring.plugin) orelse return error.TestUnexpectedResult; + host.retirePlugin(&Retiring.plugin, null); try std.testing.expectEqual(@as(?bool, true), Retiring.in_deinit); try std.testing.expect(host.pluginDataFolder(&Retiring.plugin) == null); + // The same id in another case, registered once the first is gone, is the same folder + // and the same entry: one per folder. + try host.registerPlugin(&lower); + defer host.retirePlugin(&lower, null); + try std.testing.expectEqual(kept.ptr, (host.pluginDataFolder(&lower) orelse return error.TestUnexpectedResult).ptr); + } + // Unregistered without being retired (an app's own unload, a test's), its folder is + // released all the same: no caller leaves a live one behind. + { + var released = sdk.Plugin{ .state = undefined, .vtable = &vt, .id = "released", .display_name = "Released" }; + try host.registerPlugin(&released); + _ = host.pluginDataFolder(&released) orelse return error.TestUnexpectedResult; + host.unregisterPlugin(&released); + try std.testing.expect(host.pluginDataFolder(&released) == null); } // An id that is not a plain identifier names no folder. { var dotted = sdk.Plugin{ .state = undefined, .vtable = &vt, .id = "dotted.id", .display_name = "Dotted" }; try host.registerPlugin(&dotted); - defer host.unregisterPlugin(&dotted); + defer host.retirePlugin(&dotted, null); try std.testing.expect(host.pluginDataFolder(&dotted) == null); } // Deleted since, it is made again by an ask once the second it is trusted for is over. @@ -7655,7 +7671,7 @@ test "headless: the whole editor comes up, opens a file, plays the tour and goes { var blocked = sdk.Plugin{ .state = undefined, .vtable = &vt, .id = "blocked", .display_name = "Blocked" }; try host.registerPlugin(&blocked); - defer host.unregisterPlugin(&blocked); + defer host.retirePlugin(&blocked, null); try tmp.dir.writeFile(dvui.io, .{ .sub_path = "plugin-data/blocked", .data = "" }); try std.testing.expect(host.pluginDataFolder(&blocked) == null); try tmp.dir.deleteFile(dvui.io, "plugin-data/blocked");