fix(retain): review fixes — buffer boundary, clear/save race, diagnostics [NODE-94]

core/src/drivers/plugin_driver.c

@@ -25,6 +25,7 @@
 #include "plugin_driver.h"
 #include "vpp_plugin_seal.h"
 #include <dlfcn.h>
+#include <pthread.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -1500,11 +1501,31 @@ static bool plugin_provides_retain_store(const plugin_instance_t *p)
     // storage plugin, and retain went on claiming to work.
     if (!p->config.enabled) return false;
 
+    if (!p->native_plugin) return false;
+
     // BOTH halves required. A store that can save and not load is worse than
     // none: it would accept values every scan and silently never give them
     // back, which looks like working retention right up until the reboot that
     // matters.
-    return p->native_plugin && p->native_plugin->retain_save && p->native_plugin->retain_load;
+    //
+    // Half a store is a MISTAKE, not a configuration, so say so. Exporting one
+    // hook and not the other is a typo or an unfinished driver, and treating it
+    // as "not a store" without a word looks identical to a plugin that never
+    // meant to provide one — the vendor sees retain quietly not happening and
+    // has nothing to go on. The two-stores case below already logs; this is the
+    // same courtesy for the more likely error.
+    const bool has_save = p->native_plugin->retain_save != NULL;
+    const bool has_load = p->native_plugin->retain_load != NULL;
+    if (has_save != has_load)
+    {
+        log_warn("Retain: plugin '%s' exports %s but not %s, so it cannot be a retain store — "
+                 "a store must implement both",
+                 p->config.name,
+                 has_save ? "retain_save" : "retain_load",
+                 has_save ? "retain_load" : "retain_save");
+        return false;
+    }
+    return has_save && has_load;
 }
 
 plugin_instance_t *plugin_driver_find_retain_store(plugin_driver_t *driver)
@@ -1532,17 +1553,40 @@ plugin_instance_t *plugin_driver_find_retain_store(plugin_driver_t *driver)
     return chosen;
 }
 
+/*
+ * Serialises the plugin's retain callbacks.
+ *
+ * `retain_save` runs on the PLC cycle thread, every scan. `retain_clear` runs
+ * on the control-socket thread whenever a program is uploaded — and the upload
+ * path clears unconditionally, without waiting for the PLC to stop. Without
+ * this lock the two can enter the SAME plugin's C entry points at once, and
+ * nothing in the plugin contract says a vendor's `retain_save` has to be
+ * reentrant with its own `retain_clear`. For the expected backends — a file, an
+ * FRAM page, an NVS partition — that overlap is how a store ends up torn.
+ *
+ * Held only across the plugin call, which the contract already requires to
+ * return promptly, so a per-scan acquire on an uncontended mutex is the whole
+ * cost.
+ */
+static pthread_mutex_t retain_call_lock = PTHREAD_MUTEX_INITIALIZER;
+
 int plugin_driver_retain_save(plugin_instance_t *store, const uint8_t *blob, uint16_t len)
 {
     if (!plugin_provides_retain_store(store)) return -1;
-    return store->native_plugin->retain_save(blob, len);
+    pthread_mutex_lock(&retain_call_lock);
+    const int rc = store->native_plugin->retain_save(blob, len);
+    pthread_mutex_unlock(&retain_call_lock);
+    return rc;
 }
 
 int plugin_driver_retain_load(plugin_instance_t *store, uint8_t *out, uint16_t cap, uint16_t *out_len)
 {
     if (out_len) *out_len = 0;
     if (!plugin_provides_retain_store(store)) return -1;
-    return store->native_plugin->retain_load(out, cap, out_len);
+    pthread_mutex_lock(&retain_call_lock);
+    const int rc = store->native_plugin->retain_load(out, cap, out_len);
+    pthread_mutex_unlock(&retain_call_lock);
+    return rc;
 }
 
 int plugin_driver_retain_clear(plugin_instance_t *store)
@@ -1551,7 +1595,11 @@ int plugin_driver_retain_clear(plugin_instance_t *store)
     // reporting that as failure would make the editor's post-upload clear look
     // broken on every such device.
     if (!store || !store->native_plugin || !store->native_plugin->retain_clear) return 0;
-    return store->native_plugin->retain_clear();
+    // Same lock as save/load: this is the call that races the scan thread.
+    pthread_mutex_lock(&retain_call_lock);
+    const int rc = store->native_plugin->retain_clear();
+    pthread_mutex_unlock(&retain_call_lock);
+    return rc;
 }
 
 void plugin_driver_cycle_start(plugin_driver_t *driver)

core/src/plc_app/plc_retain.cpp

@@ -9,6 +9,7 @@
 #include "plc_retain.h"
 
 #include <atomic>
+#include <cstdint>
 #include <cstring>
 #include <vector>
 
@@ -34,7 +35,17 @@ namespace {
  * something to discover on a running machine. A program needing more is
  * refused at init with a message naming both numbers.
  */
-constexpr size_t RETAIN_BUFFER_MAX = 64 * 1024;
+/*
+ * UINT16_MAX, not 64 KB.
+ *
+ * The plugin retain API takes `uint16_t len` / `cap`, so 65535 is the largest
+ * blob it can describe. A cap of 65536 admitted exactly one size the API cannot
+ * express: `needed == 65536` passed the check below, `g_active` went true, and
+ * then every `(uint16_t)` cast of the length wrapped to 0 — the plugin was
+ * handed cap/len 0 and retain silently neither saved nor restored, at the one
+ * size the error message above claims is supported.
+ */
+constexpr size_t RETAIN_BUFFER_MAX = UINT16_MAX;
 
 std::vector<uint8_t> g_buffer;
 std::atomic<bool>    g_active{false};

core/src/plc_app/plc_retain_file_store.cpp

@@ -97,8 +97,23 @@ void read_config(const char *config_path)
  * and fall back to initial values anyway, but losing the previous values as
  * well would be gratuitous.
  */
+/*
+ * Guards the STORE, not the staging buffer.
+ *
+ * `g_lock` covers `g_pending` and is deliberately released before the write, so
+ * the scan thread never waits on a disk I/O. That leaves the file itself
+ * unguarded, and two threads reach it: the flusher, and `clear()` on the
+ * control-socket thread when a program is uploaded. Without this, a clear could
+ * `remove()` the file while an in-flight commit was between its write and its
+ * rename — the rename then republished the blob a moment after it was supposed
+ * to be gone, and the next start restored values from the PREVIOUS program.
+ */
+std::mutex g_store_lock;
+
 void commit(const uint8_t *buf, uint16_t len)
 {
+    std::lock_guard<std::mutex> store(g_store_lock);
+
     const std::string tmp = g_path + ".tmp";
 
     FILE *f = fopen(tmp.c_str(), "wb");
@@ -257,9 +272,14 @@ int plc_retain_file_store_clear(void)
         g_dirty = false;
     }
     /* Not gated on `enabled`: a clear has to remove what a PREVIOUS
-     * configuration stored, which is the whole point of clearing on upload. */
+     * configuration stored, which is the whole point of clearing on upload.
+     *
+     * Under `g_store_lock`, so a commit that is mid write-and-rename finishes
+     * first and this removes the file it published — rather than the rename
+     * landing after the remove and resurrecting the blob. */
     if (!g_path.empty())
     {
+        std::lock_guard<std::mutex> store(g_store_lock);
         remove(g_path.c_str());
         remove((g_path + ".tmp").c_str());
     }

tests/pytest/runtimemanager/test_clear_retained.py

@@ -0,0 +1,120 @@
+"""Behavioural tests for `RuntimeManager.clear_retained()` and `retain_status()`.
+
+`clear_retained` runs on every program upload, and it is the one thing standing
+between a new program and the previous program's retained values. Two properties
+matter and neither is obvious from reading it:
+
+  * it must never break an upload — the upload is what the user asked for, and a
+    runtime that cannot be reached has nothing stored that this program will
+    read anyway;
+  * it must not report success when it did not happen, because "cleared" is what
+    the caller writes to the log.
+"""
+
+import socket
+import sys
+import types
+from pathlib import Path
+
+import pytest
+
+# `webserver.runtimemanager` pulls in the whole webserver package at import
+# time; the socket is the only collaborator these tests care about.
+sys.path.insert(0, str(Path(__file__).resolve().parents[3]))
+
+from webserver.runtimemanager import RuntimeManager  # noqa: E402
+
+
+class FakeSocket:
+    """Stands in for the runtime control socket."""
+
+    def __init__(self, reply=None, raises=None):
+        self.reply = reply
+        self.raises = raises
+        self.sent = []
+
+    def send_and_receive(self, message):
+        self.sent.append(message)
+        if self.raises is not None:
+            raise self.raises
+        return self.reply
+
+
+def manager(sock):
+    """A RuntimeManager with only its socket wired — no threads, no daemon."""
+    mgr = RuntimeManager.__new__(RuntimeManager)
+    mgr.runtime_socket = sock
+    return mgr
+
+
+# --- clear_retained -------------------------------------------------------
+
+
+def test_sends_the_clear_command():
+    sock = FakeSocket(reply="RETAIN:OK\n")
+    assert manager(sock).clear_retained() == "RETAIN:OK\n"
+    assert sock.sent == ["RETAIN:CLEAR\n"]
+
+
+@pytest.mark.parametrize(
+    "failure",
+    [
+        OSError("no such socket"),
+        socket.error("connection reset"),
+        RuntimeError("something unexpected"),
+    ],
+)
+def test_a_failure_never_propagates_into_the_upload(failure):
+    # An upload must not fail because retained values could not be discarded.
+    sock = FakeSocket(raises=failure)
+    assert manager(sock).clear_retained() == "RETAIN:ERROR\n"
+
+
+def test_a_failure_is_reported_as_error_not_as_ok():
+    # The distinction the caller logs. Swallowing the exception AND answering OK
+    # would say "cleared" about a device that still holds the old values.
+    sock = FakeSocket(raises=OSError("down"))
+    assert "ERROR" in manager(sock).clear_retained()
+
+
+# --- retain_status --------------------------------------------------------
+
+
+def test_reports_the_live_backend():
+    sock = FakeSocket(reply="RETAIN:STATUS active plugin synergy\n")
+    assert manager(sock).retain_status() == {
+        "active": True,
+        "backend": "plugin",
+        "detail": "synergy",
+    }
+
+
+def test_reports_the_file_backend_with_its_path():
+    sock = FakeSocket(reply="RETAIN:STATUS active file /var/lib/openplc-runtime/retain.bin\n")
+    status = manager(sock).retain_status()
+    assert status["backend"] == "file"
+    assert status["detail"] == "/var/lib/openplc-runtime/retain.bin"
+
+
+def test_reports_inactive_without_a_detail():
+    sock = FakeSocket(reply="RETAIN:STATUS inactive none\n")
+    assert manager(sock).retain_status() == {"active": False, "backend": "none", "detail": ""}
+
+
+@pytest.mark.parametrize(
+    "reply",
+    ["", "RETAIN:OK\n", "garbage\n", "RETAIN:STATUS active\n", None],
+)
+def test_an_unreadable_reply_is_unknown_rather_than_a_guess(reply):
+    # A runtime too old to answer, or a truncated reply, must not be reported as
+    # "no retention configured" — the Persistent Storage screen would then tell
+    # the operator something it does not know.
+    assert manager(FakeSocket(reply=reply)).retain_status()["backend"] == "unknown"
+
+
+def test_an_unreachable_runtime_is_unknown():
+    assert manager(FakeSocket(raises=OSError("down"))).retain_status() == {
+        "active": False,
+        "backend": "unknown",
+        "detail": "",
+    }