Plugin version

[svaroqui]

Contributor Agreement

By submitting this pull request, I agree to the terms outlined in the Contributor Agreement.

[claude]

PR Review: Plugin Version (#1431)

Summary

This PR adds three distinct features: Ed25519 plugin signing & deployment, plugin wire protocol versioning, and a PFS join-weight time-machine API + React scrubber UI. The functionality is useful, but there are several blocking issues that should be resolved before merge.

---

Critical Issues

1. plugin-signing-public-key CLI flag is never registered

config/config.go adds PluginSigningPublicKey but server/server.go has no corresponding flags.StringVar for "plugin-signing-public-key". The signing gate (cluster.Conf.PluginSigningPublicKey != "") is only reachable via hand-edited TOML — not via CLI flag or environment variable. Additionally, make plugins calls plugin-keygen and plugin-sign subcommands that are not implemented, so any developer trying to build locally with signing will hit a crash.

2. Doc comment for CheckPullPlugins is merged with loadPluginSigningKey

In cluster/cluster_chk.go, the comment block beginning // CheckPullPlugins copies plugin binaries… is immediately followed (without a blank line) by // loadPluginSigningKey reads… and then func (cluster *Cluster) loadPluginSigningKey(). CheckPullPlugins itself has no doc comment. go doc and IDEs will incorrectly attribute both doc blocks to loadPluginSigningKey.

---

Major Issues

3. GetServerFromName matches on server.Id, not hostname — new endpoint will return 404

server/api_database.go line ~5495: mycluster.GetServerFromName(vars["serverName"]) compares server.Id == name. The React client populates serverName from s.name or selectedCluster.master?.name, which is typically a hostname/IP string. Other handlers work around this by also trying GetServerFromURL. This endpoint does not, so it will silently return "server not found" for any normal request.

4. SHA-256 re-hash on every Evaluate() call — performance regression

cluster/logplugin/external.go lines ~193–204: sha256File(p.binPath) reads and hashes the full plugin binary on every single monitoring-loop evaluation. For a 10 MB binary called once per monitoring-ticker tick (potentially every second), this adds O(binary_size) I/O per tick. Consider checking os.Stat mtime+inode and only re-hashing when those change.

5. extractTablesFromDigest contains dead code and incorrect string search

server/api_database.go lines ~5689–5740:

• kwPattern is declared and suppressed with _ = kwPattern — dead code should be deleted, not silenced
• The manual strings.Index scan for "from " misses FROM\t, FROM\n, and incorrectly matches "transform ", "platform " etc. (no word-boundary check), producing false-positive table names

6. copyPluginAtomic has a double-close on error paths

cluster/cluster_chk.go lines ~1381–1408: tmp.Close() is called explicitly at line ~1399, and the deferred closure also calls tmp.Close() unconditionally. Closing a file twice violates the contract and can cause silent issues on some platforms.

---

Minor Issues

7. Version() is not part of the LogPlugin interface

The new Version() method on ExternalLogPlugin is unreachable through the interface. Consider adding Version() string to the interface or using an optional VersionedPlugin interface, otherwise callers need to type-assert to the concrete type.

8. HTTP status codes are wrong in the new endpoint

server/api_database.go line ~5487, ~5497: "server not found" and "cluster not found" should return 404 Not Found, not 500 Internal Server Error.

9. computeSnapshotWeights diverges from live LoadPFSJoinLinks logic

The comment acknowledges the re-implementation but the two algorithms use different weight derivations: explain Ref column pairing (live) vs. ExecCount × RowsScanned heuristic (historical). This makes the heat maps incomparable between live and historical views, which could mislead users. Should be documented prominently in the UI or the approaches unified.

10. React TimeMachineBar leaks event listeners on unmount

SchemaGraph.jsx: window.addEventListener('mousemove'/'mouseup') are added on drag start. If the component unmounts while the user is dragging, the listeners leak. Add a useEffect cleanup to remove them.

---

Test Coverage

No tests added for:

• CheckPullPlugins, verifyPluginSignature, loadPluginSigningKey
• handlerMuxPFSJoinWeightsHistory
• computeSnapshotWeights, extractTablesFromDigest
• Existing logplugin/external_test.go not updated for SHA-256 hashing or version probing

---

Verdict: Request Changes

Issues #3 (broken API endpoint due to GetServerFromName mismatch) and #1 (missing CLI commands breaking make plugins) are blockers. Issue #4 (per-call SHA-256 hashing) is a meaningful performance regression. Please address these before re-requesting review.

[tanji]

Merged with #1438