Add plugin hot reload#4578
Open
Garulf wants to merge 5 commits into
Open
Conversation
Reload plugins in place after install/update/uninstall instead of requiring an app restart, plus a manual reload API and Sys command. - Make PluginAssemblyLoader a collectible AssemblyLoadContext and track it per plugin so dotnet plugin assemblies can be unloaded; verify collection via WeakReference with a bounded GC loop (best effort: a pinned context is logged and reclaimed on restart) - Add PluginManager.ReloadPluginAsync/ReloadAllPluginsAsync/ UnloadPluginAsync/LoadAndInitializePluginAsync with per-plugin locking; failures fall back to the existing modified+restart flow - Convert capability registries to removable dictionaries, extract single-plugin init and inverse-registration helpers, track and detach ResultsUpdated handlers, support DialogJump removal - Wire hot reload into PluginInstaller and the PluginsManager plugin behind a new HotReloadAfterChanging setting (default on), keeping AutoRestartAfterChanging as the fallback; uninstall now deletes the plugin directory immediately when the unload is verified - Add IPublicAPI.ReloadPluginAsync/ReloadAllPluginsAsync and a Sys plugin "Reload All Plugins" command; merge newly added default Sys commands into persisted settings - Run the taskkill PreBuild step only on Windows so the solution can be cross-compiled - Add PluginHotReloadTest covering register/unregister symmetry, load context collectibility, and single-directory metadata parsing
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 23 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- Fix PluginHotReloadTest fake metadata: set ExecuteFileName before PluginDirectory so the setter's Path.Combine does not throw - DialogJump: clear the cached active dialog window when its plugin is removed, and use TryGetValue for lookups that can race a hot reload - ReloadPluginAsync: keep the plugin directory discoverable after a failed reload so a retry can still load it - InstallPlugin: skip hot reload for packages whose plugin.json ID differs from the requested plugin ID - LoadAndInitializePluginAsync: bail out before creating a load context when the plugin ID is already running - Update-all flows: mark the batch as not fully hot reloaded on cancellation, failed update, or exception so the restart fallback still applies - Sys reload command: only show the success message when the reload ran to completion
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughPlugin hot reload is added across plugin loading, unloading, public APIs, Plugin Store operations, settings, UI cleanup, system commands, and tests. Collectible assembly contexts and lifecycle cleanup support in-place plugin replacement without restarting. ChangesPlugin hot reload
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PluginsManager
participant PublicAPIInstance
participant PluginManager
User->>PluginsManager: install or update plugin
PluginsManager->>PublicAPIInstance: ReloadPluginAsync(id)
PublicAPIInstance->>PluginManager: reload plugin
PluginManager-->>PublicAPIInstance: reload success
PublicAPIInstance-->>PluginsManager: return status
PluginsManager-->>User: show hot-reload or restart message
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs (1)
559-569: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winCheck
TryGetValuereturn value to avoid accessing unloaded plugins.If a plugin is removed concurrently via hot-reload,
TryGetValuewill returnfalseand leaveexistingDialogWindowasnull. The code then incorrectly falls into theelseblock and invokesdialog.Plugin.CheckDialogWindow(hwnd)on the unloaded/disposed plugin, which can throw an exception and disrupt theWINEVENTPROChook.
Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs#L559-L569: Check the return value andcontinueif the plugin was removed.Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs#L981-L991: Check the return value andcontinueif the plugin was removed here as well.🐛 Proposed fix
- // The dialog can be removed concurrently by a plugin hot reload - _dialogJumpDialogs.TryGetValue(dialog, out var existingDialogWindow); + // The dialog can be removed concurrently by a plugin hot reload + if (!_dialogJumpDialogs.TryGetValue(dialog, out var existingDialogWindow)) + { + continue; + } + if (existingDialogWindow != null && existingDialogWindow.Handle == hwnd) { // If the dialog window is already in the list, no need to check again dialogWindow = existingDialogWindow; } else { dialogWindow = dialog.Plugin.CheckDialogWindow(hwnd); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs` around lines 559 - 569, The dialog lookup paths must handle concurrent plugin removal before accessing the plugin. In both DialogJump.cs sites (anchor lines 559-569 and sibling lines 981-991), capture the boolean result from _dialogJumpDialogs.TryGetValue and continue the enclosing iteration when it returns false; only use the existing-dialog or dialog.Plugin.CheckDialogWindow(hwnd) logic after a successful lookup.Flow.Launcher.Core/Plugin/PluginInstaller.cs (1)
349-391: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid data race by aggregating task results instead of mutating shared variables.
Mutating the shared local variables
anyPluginSuccessandallPluginsHotReloadedconcurrently from insideTask.WhenAllcreates a data race. The C# memory model does not guarantee memory visibility across threads without synchronization, which could lead to missed updates or stale reads.
Flow.Launcher.Core/Plugin/PluginInstaller.cs#L349-L391: Return a tuple(bool success, bool hotReloaded)from theasync plugin =>lambda and aggregate the results with.Any()and.All()after theWhenAllcompletes.Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs#L486-L533: Apply the same tuple-aggregation pattern to this concurrent update loop.🛡️ Proposed fix for `PluginInstaller.cs`
- var anyPluginSuccess = false; - var allPluginsHotReloaded = true; - await Task.WhenAll(resultsForUpdate.Select(async plugin => + var updateTasks = resultsForUpdate.Select(async plugin => { + bool success = false; + bool hotReloaded = true; var downloadToFilePath = Path.Combine(Path.GetTempPath(), $"{plugin.Name}-{plugin.NewVersion}.zip"); try { using var cts = new CancellationTokenSource(); await DownloadFileAsync( $"{Localize.DownloadingPlugin()} {plugin.PluginNewUserPlugin.Name}", plugin.PluginNewUserPlugin.UrlDownload, downloadToFilePath, cts); // check if user cancelled download before installing plugin if (cts.IsCancellationRequested) { - allPluginsHotReloaded = false; - return; + hotReloaded = false; + return (success, hotReloaded); } if (!await PublicApi.Instance.UpdatePluginAsync(plugin.PluginExistingMetadata, plugin.PluginNewUserPlugin, downloadToFilePath)) { - allPluginsHotReloaded = false; - return; + hotReloaded = false; + return (success, hotReloaded); } - anyPluginSuccess = true; + success = true; if (!Settings.HotReloadAfterChanging || !await PluginManager.ReloadPluginAsync(plugin.ID)) { - allPluginsHotReloaded = false; + hotReloaded = false; } } catch (Exception e) { - allPluginsHotReloaded = false; + hotReloaded = false; PublicApi.Instance.LogException(ClassName, "Failed to update plugin", e); PublicApi.Instance.ShowMsgError(Localize.ErrorUpdatingPlugin()); } - })); + return (success, hotReloaded); + }); + + var results = await Task.WhenAll(updateTasks); + var anyPluginSuccess = results.Any(r => r.success); + var allPluginsHotReloaded = results.All(r => r.hotReloaded); if (!anyPluginSuccess) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher.Core/Plugin/PluginInstaller.cs` around lines 349 - 391, Replace the shared anyPluginSuccess and allPluginsHotReloaded mutations in PluginInstaller.cs lines 349-391 with tuple results returned by each async plugin lambda, then aggregate the completed WhenAll results using Any() and All(). Apply the same tuple-aggregation pattern to the concurrent update loop in PluginsManager.cs lines 486-533, preserving each operation’s existing success and hot-reload outcomes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Flow.Launcher.Core/Plugin/PluginManager.cs`:
- Around line 503-515: Extend the initialization transaction in InitAsync to
cover RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation,
InitializeDialogJumpPlugin, and AddPluginToLists, catching failures and
returning false instead of propagating exceptions. On any post-init failure,
roll back every registration or list insertion performed for the plugin so
reload can restart cleanly.
- Around line 143-146: Use a single per-plugin lifecycle gate from _reloadLocks
for ReloadPluginAsync, UninstallPluginAsync, and the update flow. Add
non-locking internal implementations for these operations, and have each public
entry point acquire the plugin-specific semaphore before invoking its internal
method, ensuring concurrent reload, update, and uninstall calls cannot mutate
shared plugin state or files simultaneously.
- Around line 195-203: Update Flow.Launcher.Core/Plugin/PluginManager.cs lines
195-203 in ReloadAllPluginsAsync to collect failed and skipped plugin IDs
instead of discarding ReloadPluginAsync results, then return an aggregate
success or structured reload summary. Update
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs lines 618-622 to expose the
matching Task<bool> or structured summary contract so callers can report partial
failures.
- Around line 226-252: Update the plugin removal flow around
RemovePluginFromLists and DisposePluginAsync to quiesce the plugin before
disposal: use a per-plugin execution gate to cancel and await all active query
and event invocations holding pair, then proceed with DisposePluginAsync,
pair.Plugin = null, and unloading. Ensure new invocations are blocked during
shutdown and no active call can access the instance after disposal begins.
- Around line 1268-1273: Update the plugin installation flow around the
plugin.json deserialization to require exact, case-sensitive equality between
the manifest ID and the requested plugin ID, using the existing plugin ID
validation path. Reject and abort the installation before any files are copied
when they differ; do not merely skip the _pendingInstallPaths update or use
StringComparison.OrdinalIgnoreCase.
- Around line 468-500: In the initialization failure catch block of the plugin
registration flow, unregister the action keywords previously added by
RegisterPluginActionKeywords(pair) before returning false. Reuse the existing
keyword-unregistration mechanism and ensure cleanup occurs for every InitAsync
failure while preserving the disabled-plugin bookkeeping.
In `@Flow.Launcher.Core/Plugin/PluginsLoader.cs`:
- Around line 96-138: Declare the PluginAssemblyLoader used in the loading block
outside the try so it remains available to failure handling. Initialize it
before LoadAssemblyAndDependencies, and ensure every non-debug exception path
unloads it when loading or plugin creation fails, while preserving the existing
logging and successful PluginManager.TrackAssemblyLoader behavior.
---
Outside diff comments:
In `@Flow.Launcher.Core/Plugin/PluginInstaller.cs`:
- Around line 349-391: Replace the shared anyPluginSuccess and
allPluginsHotReloaded mutations in PluginInstaller.cs lines 349-391 with tuple
results returned by each async plugin lambda, then aggregate the completed
WhenAll results using Any() and All(). Apply the same tuple-aggregation pattern
to the concurrent update loop in PluginsManager.cs lines 486-533, preserving
each operation’s existing success and hot-reload outcomes.
In `@Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs`:
- Around line 559-569: The dialog lookup paths must handle concurrent plugin
removal before accessing the plugin. In both DialogJump.cs sites (anchor lines
559-569 and sibling lines 981-991), capture the boolean result from
_dialogJumpDialogs.TryGetValue and continue the enclosing iteration when it
returns false; only use the existing-dialog or
dialog.Plugin.CheckDialogWindow(hwnd) logic after a successful lookup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d24710f-2402-4291-bbef-f067426d3610
📒 Files selected for processing (23)
Flow.Launcher.Core/Plugin/IResultUpdateRegister.csFlow.Launcher.Core/Plugin/PluginAssemblyLoader.csFlow.Launcher.Core/Plugin/PluginConfig.csFlow.Launcher.Core/Plugin/PluginInstaller.csFlow.Launcher.Core/Plugin/PluginManager.csFlow.Launcher.Core/Plugin/PluginsLoader.csFlow.Launcher.Infrastructure/DialogJump/DialogJump.csFlow.Launcher.Infrastructure/UserSettings/Settings.csFlow.Launcher.Plugin/Interfaces/IPublicAPI.csFlow.Launcher.Test/PluginHotReloadTest.csFlow.Launcher/Flow.Launcher.csprojFlow.Launcher/Languages/en.xamlFlow.Launcher/PublicAPIInstance.csFlow.Launcher/SettingPages/Views/SettingsPaneGeneral.xamlFlow.Launcher/ViewModel/MainViewModel.csPlugins/Flow.Launcher.Plugin.PluginsManager/Languages/en.xamlPlugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.csPlugins/Flow.Launcher.Plugin.PluginsManager/Settings.csPlugins/Flow.Launcher.Plugin.PluginsManager/ViewModels/SettingsViewModel.csPlugins/Flow.Launcher.Plugin.PluginsManager/Views/PluginsManagerSettings.xamlPlugins/Flow.Launcher.Plugin.Sys/Languages/en.xamlPlugins/Flow.Launcher.Plugin.Sys/Main.csPlugins/Flow.Launcher.Plugin.Sys/Settings.cs
Comment on lines
+195
to
+203
| public static async Task ReloadAllPluginsAsync() | ||
| { | ||
| foreach (var pair in GetAllLoadedPlugins()) | ||
| { | ||
| var id = pair.Metadata.ID; | ||
| if (PluginModified(id)) continue; | ||
|
|
||
| await ReloadPluginAsync(id); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Report partial failures from “Reload All.”
The implementation discards each reload result, while the public contract exposes no aggregate outcome.
Flow.Launcher.Core/Plugin/PluginManager.cs#L195-L203: aggregate failed and skipped plugin IDs instead of ignoring the returned boolean.Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs#L618-L622: returnTask<bool>or a structured reload summary so callers can report the correct status.
📍 Affects 2 files
Flow.Launcher.Core/Plugin/PluginManager.cs#L195-L203(this comment)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs#L618-L622
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Flow.Launcher.Core/Plugin/PluginManager.cs` around lines 195 - 203, Update
Flow.Launcher.Core/Plugin/PluginManager.cs lines 195-203 in
ReloadAllPluginsAsync to collect failed and skipped plugin IDs instead of
discarding ReloadPluginAsync results, then return an aggregate success or
structured reload summary. Update Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs
lines 618-622 to expose the matching Task<bool> or structured summary contract
so callers can report partial failures.
Comment on lines
+226
to
+252
| // Removing the plugin from the initialized list first makes in-flight queries treat it as | ||
| // still initializing, so they show a placeholder result instead of touching a dead instance | ||
| RemovePluginFromLists(id); | ||
| UnregisterPluginActionKeywords(id); | ||
| _register?.UnregisterResultsUpdatedEvent(pair); | ||
| DialogJump.RemoveDialogJumpPlugin(pair); | ||
| _allLoadedPlugins.TryRemove(id, out _); | ||
|
|
||
| // For JsonRPC V2 plugins this kills the child process, releasing its file locks | ||
| await DisposePluginAsync(pair); | ||
|
|
||
| if (!_assemblyLoaders.TryRemove(id, out var loader)) return true; | ||
|
|
||
| // Persist and evict the Type-keyed storages that would otherwise pin the collectible load context | ||
| PublicApi.Instance.SavePluginSettings(); | ||
| PublicApi.Instance.SavePluginCaches(); | ||
| if (PublicApi.Instance is IRemovable removable) | ||
| { | ||
| removable.RemovePluginSettings(metadata.AssemblyName); | ||
| removable.RemovePluginCaches(metadata.PluginCacheDirectoryPath); | ||
| } | ||
|
|
||
| pair.Plugin = null; | ||
|
|
||
| var weakReference = PluginAssemblyLoader.UnloadAndGetWeakReference(loader); | ||
| loader = null; | ||
| var unloaded = await PluginAssemblyLoader.WaitForUnloadAsync(weakReference); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Quiesce active plugin calls before disposal.
Removing the pair from registries only prevents new lookups. Queries already holding pair can still execute against the disposed instance or race with pair.Plugin = null, causing failures during reload.
Cancel and await active query/event invocations through a per-plugin execution gate before disposal and unloading.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Flow.Launcher.Core/Plugin/PluginManager.cs` around lines 226 - 252, Update
the plugin removal flow around RemovePluginFromLists and DisposePluginAsync to
quiesce the plugin before disposal: use a per-plugin execution gate to cancel
and await all active query and event invocations holding pair, then proceed with
DisposePluginAsync, pair.Plugin = null, and unloading. Ensure new invocations
are blocked during shutdown and no active call can access the instance after
disposal begins.
Comment on lines
+96
to
+138
| try | ||
| { | ||
| var assemblyLoader = new PluginAssemblyLoader(metadata.ExecuteFilePath); | ||
| assembly = assemblyLoader.LoadAssemblyAndDependencies(); | ||
|
|
||
| var type = assemblyLoader.FromAssemblyGetTypeOfInterface(assembly, | ||
| typeof(IAsyncPlugin)); | ||
|
|
||
| plugin = Activator.CreateInstance(type) as IAsyncPlugin; | ||
|
|
||
| metadata.AssemblyName = assembly.GetName().Name; | ||
|
|
||
| // Keep the load context around so the assembly can be unloaded when the plugin is | ||
| // reloaded or uninstalled | ||
| PluginManager.TrackAssemblyLoader(metadata.ID, assemblyLoader); | ||
| } | ||
| #if DEBUG | ||
| catch (Exception) | ||
| { | ||
| throw; | ||
| } | ||
| #else | ||
| catch (Exception e) when (assembly == null) | ||
| { | ||
| PublicApi.Instance.LogException(ClassName, $"Couldn't load assembly for the plugin: {metadata.Name}", e); | ||
| } | ||
| catch (InvalidOperationException e) | ||
| { | ||
| PublicApi.Instance.LogException(ClassName, $"Can't find the required IPlugin interface for the plugin: <{metadata.Name}>", e); | ||
| } | ||
| catch (ReflectionTypeLoadException e) | ||
| { | ||
| PublicApi.Instance.LogException(ClassName, $"The GetTypes method was unable to load assembly types for the plugin: <{metadata.Name}>", e); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| PublicApi.Instance.LogException(ClassName, $"The following plugin has errored and can not be loaded: <{metadata.Name}>", e); | ||
| } | ||
| #endif | ||
|
|
||
| if (plugin == null) return; | ||
|
|
||
| pair = new PluginPair { Plugin = plugin, Metadata = metadata }; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline Flow.Launcher.Core/Plugin/PluginsLoader.cs \
--match DotNetPlugin --view expanded
rg -n -C4 \
'new PluginAssemblyLoader|TrackAssemblyLoader|UnloadAndGetWeakReference|\.Unload\(\)' \
Flow.Launcher.Core Flow.Launcher.TestRepository: Flow-Launcher/Flow.Launcher
Length of output: 2079
🌐 Web query:
.NET collectible AssemblyLoadContext unload occurs on GC file lock official docs
💡 Result:
In.NET, the unloading of a collectible AssemblyLoadContext (ALC) is a cooperative process, not an immediate or forced one [1]. When you call AssemblyLoadContext.Unload, it merely initiates the request to unload; the actual release of resources—including file locks—occurs only after the Garbage Collector (GC) successfully collects the ALC and all its associated objects [1][2]. Key points regarding the interaction between unloading, the GC, and file locks: 1. Cooperative Unloading: The ALC cannot be unloaded as long as there are any active references to it, or to any types, instances, or assemblies loaded within that context [1][3]. This includes objects on the execution stack, static variables, or strong references held elsewhere in the application [1][4]. 2. File Locks and GC: When an assembly is loaded from a file path (e.g., via LoadFromAssemblyPath), the runtime maps the file into memory, which often results in an OS-level file lock [5]. This lock is not released until the ALC is fully unloaded. Because the unload process depends on the GC to reclaim all references, the file lock will persist until the GC has successfully performed enough collection cycles to fully clean up the context [6][2][7]. 3. Forcing Collection: Because the process is cooperative, there is no API to "force" an immediate unload [4]. To facilitate the release of file locks, you must ensure that all references to objects within the ALC are cleared [1][4]. Developers often use a pattern of calling GC.Collect followed by GC.WaitForPendingFinalizers in a loop to encourage the runtime to complete the collection process [2][7]. 4. Recommended Workarounds: If file locking is a blocking issue (e.g., you need to delete or move the assembly file), a common practice is to load the assembly into memory first using a Stream (via LoadFromStream) rather than directly from a file path [7]. This avoids the OS-level file lock entirely, as the runtime does not maintain a handle to the original file on disk [7]. In summary, the file lock is not a bug but a result of the cooperative unload mechanism. To ensure the lock is released, you must eliminate all strong references to the ALC's contents and allow the GC to finalize the collection [1][3]. If immediate file access is required, loading from a stream is the preferred architectural solution [7].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/standard/assembly/unloadability
- 2: https://github.com/dotnet/coreclr/blob/a7cbc5c8d1bd48cafec48ac50900ff9e96c1485c/Documentation/project-docs/unloadability-howto.md
- 3: AssemblyLoadContext.Unload silently fails to unload Assemblies, leaking filehandles dotnet/runtime#44679
- 4: Collectible ALC never collected after Unload() — need guidance on diagnosing root cause dotnet/runtime#125381
- 5: [API Proposal]: Support non-locking assembly loading with preserved Assembly.Location dotnet/runtime#127097
- 6: AssemblyLoadContext Unload does not release assembly lock on Windows dotnet/runtime#13370
- 7: Loaded assemblies from an AssemblyLoadContext not unloaded on AssemblyLoadContext.Unload() call dotnet/runtime#119648
Unload the plugin loader on failure
Declare assemblyLoader outside the try so the failure path can call Unload(). Without that, a failed load can keep the collectible context and plugin file locked until GC runs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Flow.Launcher.Core/Plugin/PluginsLoader.cs` around lines 96 - 138, Declare
the PluginAssemblyLoader used in the loading block outside the try so it remains
available to failure handling. Initialize it before LoadAssemblyAndDependencies,
and ensure every non-debug exception path unloads it when loading or plugin
creation fails, while preserving the existing logging and successful
PluginManager.TrackAssemblyLoader behavior.
- DialogJump: skip dialogs removed mid-enumeration instead of probing the unloaded plugin, and always clear the cached active dialog window when its plugin is removed (it may not be attributable by value) - Route uninstall through the same per-plugin lifecycle lock as reload so concurrent reload/uninstall cannot interleave - InitializePluginAsync: unregister action keywords when init fails and roll back partial registrations when post-init wiring throws - InstallPlugin: reject packages whose plugin.json ID does not match the requested plugin before copying any files - PluginsLoader: track the assembly load context only after the plugin instance is accepted - ReloadAllPluginsAsync now returns whether every plugin reloaded; the Sys command reports failure instead of an unconditional success toast
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Plugins/Flow.Launcher.Plugin.Sys/Languages/en.xaml (1)
55-55: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid promising that reload always completes without a restart.
Reload is best-effort; the Sys command can report incomplete reloads and recommend restarting. Update this description to reflect that limitation.
Proposed wording
- <system:String x:Key="flowlauncher_plugin_sys_reload_all_plugins">Fully reloads all plugins without restarting Flow Launcher</system:String> + <system:String x:Key="flowlauncher_plugin_sys_reload_all_plugins">Attempts to reload all plugins without restarting Flow Launcher; a restart may be required</system:String>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Plugins/Flow.Launcher.Plugin.Sys/Languages/en.xaml` at line 55, Update the flowlauncher_plugin_sys_reload_all_plugins resource description to describe plugin reloading as best-effort, without promising completion, and mention that restarting Flow Launcher may be recommended when reloads are incomplete.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Plugins/Flow.Launcher.Plugin.Sys/Languages/en.xaml`:
- Line 55: Update the flowlauncher_plugin_sys_reload_all_plugins resource
description to describe plugin reloading as best-effort, without promising
completion, and mention that restarting Flow Launcher may be recommended when
reloads are incomplete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d74d4eb-96ad-49be-bd01-63d812388f81
📒 Files selected for processing (8)
Flow.Launcher.Core/Plugin/PluginManager.csFlow.Launcher.Core/Plugin/PluginsLoader.csFlow.Launcher.Infrastructure/DialogJump/DialogJump.csFlow.Launcher.Plugin/Interfaces/IPublicAPI.csFlow.Launcher/Languages/en.xamlFlow.Launcher/PublicAPIInstance.csPlugins/Flow.Launcher.Plugin.Sys/Languages/en.xamlPlugins/Flow.Launcher.Plugin.Sys/Main.cs
🚧 Files skipped from review as they are similar to previous changes (5)
- Flow.Launcher/PublicAPIInstance.cs
- Flow.Launcher.Core/Plugin/PluginsLoader.cs
- Plugins/Flow.Launcher.Plugin.Sys/Main.cs
- Flow.Launcher/Languages/en.xaml
- Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs
Contributor
There was a problem hiding this comment.
2 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs">
<violation number="1" location="Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs:179">
P2: Hot-reloading a plugin can abandon its active dialog wrapper without disposal when the wrapper exists only in `_dialogWindow`. Capture and clear the cached instance under `_dialogWindowLock`, then dispose it (without double-disposing when it is the dictionary value).</violation>
<violation number="2" location="Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs:560">
P1: A reload can still invoke and recache a plugin after it has been removed because `TryGetValue` is only a point-in-time check. Coordinate this callback with removal/plugin lifetime (or revalidate under a shared lock) before calling `CheckDialogWindow`, otherwise old contexts can remain pinned or a disposed plugin can be called.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| IDialogJumpDialogWindow dialogWindow; | ||
| var existingDialogWindow = _dialogJumpDialogs[dialog]; | ||
| // Skip dialogs removed concurrently by a plugin hot reload | ||
| if (!_dialogJumpDialogs.TryGetValue(dialog, out var existingDialogWindow)) continue; |
Contributor
There was a problem hiding this comment.
P1: A reload can still invoke and recache a plugin after it has been removed because TryGetValue is only a point-in-time check. Coordinate this callback with removal/plugin lifetime (or revalidate under a shared lock) before calling CheckDialogWindow, otherwise old contexts can remain pinned or a disposed plugin can be called.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs, line 560:
<comment>A reload can still invoke and recache a plugin after it has been removed because `TryGetValue` is only a point-in-time check. Coordinate this callback with removal/plugin lifetime (or revalidate under a shared lock) before calling `CheckDialogWindow`, otherwise old contexts can remain pinned or a disposed plugin can be called.</comment>
<file context>
@@ -556,8 +556,8 @@ uint dwmsEventTime
- // The dialog can be removed concurrently by a plugin hot reload
- _dialogJumpDialogs.TryGetValue(dialog, out var existingDialogWindow);
+ // Skip dialogs removed concurrently by a plugin hot reload
+ if (!_dialogJumpDialogs.TryGetValue(dialog, out var existingDialogWindow)) continue;
if (existingDialogWindow != null && existingDialogWindow.Handle == hwnd)
{
</file context>
| // foreground event repopulates it from the remaining dialogs. | ||
| lock (_dialogWindowLock) | ||
| { | ||
| _dialogWindow = null; |
Contributor
There was a problem hiding this comment.
P2: Hot-reloading a plugin can abandon its active dialog wrapper without disposal when the wrapper exists only in _dialogWindow. Capture and clear the cached instance under _dialogWindowLock, then dispose it (without double-disposing when it is the dictionary value).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs, line 179:
<comment>Hot-reloading a plugin can abandon its active dialog wrapper without disposal when the wrapper exists only in `_dialogWindow`. Capture and clear the cached instance under `_dialogWindowLock`, then dispose it (without double-disposing when it is the dictionary value).</comment>
<file context>
@@ -169,14 +169,14 @@ public static void RemoveDialogJumpPlugin(PluginPair pair)
- {
- _dialogWindow = null;
- }
+ _dialogWindow = null;
}
</file context>
- InstallPlugin: adopt the package's plugin.json ID when the requested UserPlugin has no ID (install-from-URL builds one with an empty ID), instead of rejecting the install as an ID mismatch - ReloadPluginAsync: do not clear the modified flag when a concurrent install recorded a newer pending version during the reload - Sys reload command: handle faulted reloads in the continuation (log the exception, show the failure message) instead of skipping it - Skip the delete marker when the plugin directory no longer exists - Keep the assembly-unload GC wait loop off the UI thread context - Drop unreachable duplicate-load cleanup now guarded by the lifecycle lock, and the unused fromPendingInstall local
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- Sys reload failure toast: pass a short title and the message as the subtitle, matching ShowMsgError's signature - InstallPlugin: record the pending install path before the modified flag so a concurrent reload that observes the flag also sees the pending path and never clears it prematurely - Soften the Reload All Plugins description: a restart may still be needed if a reload fails
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher.Core/Plugin/PluginManager.cs">
<violation number="1" location="Flow.Launcher.Core/Plugin/PluginManager.cs:1311">
P2: A concurrent manual/reload-all request can reload this new directory between these two publications, then this install adds a stale modified flag after reload succeeds. Serialize installation publication with the per-plugin reload lock (or make reload recheck/clear after publication) so successful hot reloads do not incorrectly require restart.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // Remember where this version was installed so a hot reload can load it without a | ||
| // restart. Recorded before the modified flag so that a concurrent reload observing | ||
| // the flag always sees the pending path too and never clears the flag prematurely. | ||
| _pendingInstallPaths[plugin.ID] = newPluginPath; |
Contributor
There was a problem hiding this comment.
P2: A concurrent manual/reload-all request can reload this new directory between these two publications, then this install adds a stale modified flag after reload succeeds. Serialize installation publication with the per-plugin reload lock (or make reload recheck/clear after publication) so successful hot reloads do not incorrectly require restart.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Flow.Launcher.Core/Plugin/PluginManager.cs, line 1311:
<comment>A concurrent manual/reload-all request can reload this new directory between these two publications, then this install adds a stale modified flag after reload succeeds. Serialize installation publication with the per-plugin reload lock (or make reload recheck/clear after publication) so successful hot reloads do not incorrectly require restart.</comment>
<file context>
@@ -1305,14 +1305,16 @@ internal static bool InstallPlugin(UserPlugin plugin, string zipFilePath, bool c
+ // Remember where this version was installed so a hot reload can load it without a
+ // restart. Recorded before the modified flag so that a concurrent reload observing
+ // the flag always sees the pending path too and never clears the flag prematurely.
+ _pendingInstallPaths[plugin.ID] = newPluginPath;
+
if (checkModified)
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Reload plugins in place after install/update/uninstall — and on demand — instead of requiring a full app restart.
AssemblyLoadContext:PluginAssemblyLoaderis now created withisCollectible: trueand tracked per plugin, so .NET plugin assemblies can be unloaded. Unload is verified with aWeakReference+ bounded GC loop and is best-effort: if something still pins the old context (cached result delegates, event handlers), it's logged and reclaimed on restart while the new version still loads from its own directory.PluginManager:ReloadPluginAsync(id)/ReloadAllPluginsAsync()(public),UnloadPluginAsync/LoadAndInitializePluginAsync(internal), with per-plugin locking. Any failure falls back to the existingModifiedPlugins+ restart-required flow.ConcurrentBagto removableConcurrentDictionarys; single-plugin init extracted fromInitializePluginsAsync; inverse-registration helpers;ResultsUpdatedhandlers are now tracked so they can be detached;DialogJump.RemoveDialogJumpPlugin.HotReloadAfterChangingsetting (default on,AutoRestartAfterChangingremains the fallback) in both the app settings and the PluginsManager plugin. Install/update/uninstall inPluginInstallerand the PluginsManager plugin now hot reload first. Uninstall deletes the plugin directory immediately when the unload is verified (marker-file fallback otherwise).IPublicAPI.ReloadPluginAsync(string)andIPublicAPI.ReloadAllPluginsAsync()(additive). New Sys plugin command Reload All Plugins; newly added default Sys commands are now merged into persisted settings so they appear for existing users.taskkillPreBuild step now only runs on Windows, so the solution cross-compiles.Tests
PluginHotReloadTestcovers register/unregister symmetry of all plugin registries, load-context collectibility after unload, and single-directoryplugin.jsonparsing. Verifieddotnet build Flow.Launcher.slnclean; test execution needs the Windows CI (WindowsDesktop runtime).Known limitations