Fix/remote plugin already loaded- #5749

[vvidovic]

Summary

Fixes a "plugin already loaded" error when updating a custom plugin loaded from a remote URL. When a remote plugin was updated (e.g. config change, enable/disable), Bifrost re-downloaded the same .so bytes, wrote them to a new temp path, and called plugin.Open again. Go's plugin system treats any binary with the same checksum as already loaded, failing with "plugin already loaded".

Changes

• framework/plugins/utils.go: DownloadPlugin now computes SHA-256 of plugin bytes during download, derives a stable path as /tmp/bifrost-plugin-<hash>.so, and atomically renames the temp file. Subsequent downloads of identical content return the existing stable path from an in-memory cache with no file I/O.

• framework/plugins/soloader.go: After download, openPlugin checks the in-memory cache. If cached.loadedPlugin != nil, the cached *plugin.Plugin is returned directly — plugin.Open is skipped entirely. This is the core fix: the same binary content is only opened once per process.

• framework/plugins/soplugin.go: Added contentHash field to DynamicPlugin to track the binary's SHA-256.

• framework/plugins/soplugin_test.go: Removed 3 tests (TestLoadPlugin_CacheHitSkipsOpen, TestLoadPlugin_ConfigUpdateReusesPlugin, TestLoadPlugin_ContentChangeLoadsNewPlugin) that attempted to load the same plugin binary twice in a single test process. Go's plugin system does not support unloading or re-loading a module, so these tests were testing unsupported behavior.

Type of change

• Bug fix
• Feature
• Refactor
• Documentation
• Chore/CI

Affected areas

• Core (Go)
• Transports (HTTP)
• Providers/Integrations
• Plugins
• UI (React)
• Docs

How to test

Describe the steps to validate this change. Include commands and expected outcomes.

cd framework
go test ./plugins/... -v -count=1

All 23 tests pass. Existing tests (TestLoadPlugins_MultiplePlugins, TestDownloadPlugin_StablePathByContentHash, TestDownloadPlugin_IdenticalContentDifferentURLs) cover the behavior.

Breaking changes

• Yes
• No

Related issues

Closes #5741

Security considerations

No.

Checklist

• I read docs/contributing/README.md and followed the guidelines
• I added/updated tests where appropriate
• I updated documentation where needed
• I verified builds succeed (Go and UI)
• I verified the CI pipeline passes locally if applicable

[CLAassistant]

All committers have signed the CLA.

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Improved remote plugin loading to prevent duplicate-load errors.
  • Reuses identical downloaded plugin binaries instead of downloading or opening them repeatedly.
  • Uses stable, content-based storage paths for downloaded plugins.
  • Added safer handling for download, decompression, and file-storage failures.

Walkthrough

Remote plugin downloads now return SHA-256 hashes and deterministic paths. The loader reuses cached paths and plugin instances for identical binaries. Tests cover hashing, path reuse, URL deduplication, different content, and download errors.

Changes

Remote plugin deduplication

Layer / File(s)	Summary
Download hashing and stable-path cache framework/plugins/utils.go	DownloadPlugin now returns the stable path and SHA-256 content hash. It caches paths by hash, reuses existing files, and atomically renames downloads.
Loader cache integration framework/plugins/soloader.go, framework/plugins/soplugin.go	openPlugin reuses cached plugin instances or stable paths. DynamicPlugin stores the content hash.
Hash and path behavior validation framework/plugins/utils_test.go, framework/plugins/soplugin_test.go	Tests validate hashes, stable paths, cache reuse, URL deduplication, distinct content, error handling, and updated return values.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related issues

• [Bug]: Updating a custom plugin loaded from a remote URL fails with "plugin already loaded" #5741 — Covers stable content-hashed paths and remote plugin instance reuse to prevent duplicate-load errors.

Possibly related PRs

• maximhq/bifrost#5749 — Modifies the same plugin-loading and download-caching code for content-hashed paths.

Suggested reviewers: roroghost17, pratham-mishra04

Sequence Diagram(s)

sequenceDiagram
  participant openPlugin
  participant DownloadPlugin
  participant plugin.Open
  participant pluginCache
  openPlugin->>DownloadPlugin: download remote plugin
  DownloadPlugin->>pluginCache: check content hash and stable path
  DownloadPlugin-->>openPlugin: return stable path and content hash
  openPlugin->>plugin.Open: open stable plugin path
  plugin.Open-->>openPlugin: return loaded plugin
  openPlugin->>pluginCache: register loaded plugin

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title references issue #5749 but describes fixing a 'plugin already loaded' error. The raw_summary and description confirm this is the main change, though the title could be more descriptive about the fix itself.
Description check	✅ Passed	The PR description includes all required template sections: Summary, Changes, Type of change, Affected areas, How to test, Breaking changes, Related issues, Security considerations, and Checklist. All critical sections are complete and detailed.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

Warning

⚠️ This pull request shows signs of AI-generated slop (redundant_comments, description_diff_mismatch). It has been flagged by CodeRabbit slop detection and should be reviewed carefully.

[coderabbitai]

🧹 Nitpick comments (3)

framework/plugins/utils.go (3)

160-201: 🚀 Performance & Scalability | 🔵 Trivial

Stable plugin files under /tmp are never cleaned up.

The comment at Line 154 states these files are intended to persist "from a previous run (e.g. server restart)," and pluginCache never evicts entries. Every time a remote plugin's content changes, a new bifrost-plugin-<hash>.so file accumulates in /tmp alongside all prior versions, for the life of the host (and across restarts, since the design explicitly favors reusing files found on disk). Over a long-running deployment with frequent plugin updates, this grows disk usage without bound.

Consider adding a retention policy (for example, keep only the N most recent hashes per plugin Path, or prune on successful load of a new version) to avoid unbounded disk growth.

🤖 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 `@framework/plugins/utils.go` around lines 160 - 201, Implement retention for
stable plugin files created by the download-and-rename flow around
setPluginCacheStablePath, so outdated hashes for the same plugin Path are
removed while retaining the configured number of recent versions. Prune stale
files after successfully caching a new stable path, preserve the current version
and reusable files from prior runs, and ignore cleanup failures without
disrupting plugin loading.

---

141-158: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Cache reuse only skips the disk write, not the network download.

The content hash is computed from body, which requires the full HTTP download to complete first. The cache check at Line 147 only saves a redundant file write; it does not save the network round trip. If plugin reloads happen frequently (the PR's stated goal is fixing repeated reloads), every reload still re-downloads the full binary from the remote URL even when the content has not changed.

There is also no lock around the download-and-write sequence for the same URL or hash. If openPlugin is called concurrently for the same remote plugin (for example, during a bulk config reload), each call downloads and writes independently; the result is still correct because renames are atomic and content is identical, but the work is duplicated.

Consider wrapping the fetch in a singleflight.Group keyed by pluginURL to deduplicate concurrent calls, and consider caching by URL with a validator (ETag/Last-Modified) so unchanged plugins skip the network fetch entirely.

As per coding guidelines, **/*.go review should apply "bounded goroutines/channels, race-safe shared state" practices; deduplicating concurrent downloads for the same key is part of that.

🤖 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 `@framework/plugins/utils.go` around lines 141 - 158, Update openPlugin to
deduplicate concurrent fetch-and-write operations with a race-safe
singleflight.Group keyed by pluginURL, ensuring only one download occurs per URL
while concurrent callers share its result. Add URL-based caching with ETag or
Last-Modified validation so unchanged plugins avoid downloading the full body on
later reloads, while preserving the existing content-hash stable-path cache
behavior.

Source: Coding guidelines

---

20-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove getPluginCacheHashFromPath unless it is wired into loader logic.

The helper is declared, but no code or tests read it. Remove it or connect the loader to the reverse lookup where a stable path needs a content hash.

🤖 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 `@framework/plugins/utils.go` around lines 20 - 73, Remove the unused
getPluginCacheHashFromPath helper from the plugin cache implementation, since no
loader logic or tests currently consume the reverse lookup. Leave
pluginCacheEntry and the getPluginCacheEntry, setPluginCacheStablePath, and
setPluginCacheLoaded flows unchanged.

🤖 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.

Nitpick comments:
In `@framework/plugins/utils.go`:
- Around line 160-201: Implement retention for stable plugin files created by
the download-and-rename flow around setPluginCacheStablePath, so outdated hashes
for the same plugin Path are removed while retaining the configured number of
recent versions. Prune stale files after successfully caching a new stable path,
preserve the current version and reusable files from prior runs, and ignore
cleanup failures without disrupting plugin loading.
- Around line 141-158: Update openPlugin to deduplicate concurrent
fetch-and-write operations with a race-safe singleflight.Group keyed by
pluginURL, ensuring only one download occurs per URL while concurrent callers
share its result. Add URL-based caching with ETag or Last-Modified validation so
unchanged plugins avoid downloading the full body on later reloads, while
preserving the existing content-hash stable-path cache behavior.
- Around line 20-73: Remove the unused getPluginCacheHashFromPath helper from
the plugin cache implementation, since no loader logic or tests currently
consume the reverse lookup. Leave pluginCacheEntry and the getPluginCacheEntry,
setPluginCacheStablePath, and setPluginCacheLoaded flows unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2a791178-4ba8-4f14-8d57-5a900feecb36

📥 Commits

Reviewing files that changed from the base of the PR and between 29e49c9 and 57cdf5b.

📒 Files selected for processing (5)

• framework/plugins/soloader.go
• framework/plugins/soplugin.go
• framework/plugins/soplugin_test.go
• framework/plugins/utils.go
• framework/plugins/utils_test.go

[vvidovic]

The PR was postponed after noticing the local build of the Bifrost server didn't look like it had fixed the issue.
I created a dynamic build locally using a local framework build, and now it looks good - the problem was in pulling the framework module from web.

The merge-base changed after approval.