diff --git a/core/src/drivers/plugin_driver.c b/core/src/drivers/plugin_driver.c index c315670..1b511b2 100644 --- a/core/src/drivers/plugin_driver.c +++ b/core/src/drivers/plugin_driver.c @@ -25,6 +25,7 @@ #include "plugin_driver.h" #include "vpp_plugin_seal.h" #include +#include #include #include #include @@ -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) diff --git a/core/src/plc_app/plc_retain.cpp b/core/src/plc_app/plc_retain.cpp index 059b0fa..22273ce 100644 --- a/core/src/plc_app/plc_retain.cpp +++ b/core/src/plc_app/plc_retain.cpp @@ -9,6 +9,7 @@ #include "plc_retain.h" #include +#include #include #include @@ -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 g_buffer; std::atomic g_active{false}; diff --git a/core/src/plc_app/plc_retain_file_store.cpp b/core/src/plc_app/plc_retain_file_store.cpp index 1d0b2a8..2b4863c 100644 --- a/core/src/plc_app/plc_retain_file_store.cpp +++ b/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 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 store(g_store_lock); remove(g_path.c_str()); remove((g_path + ".tmp").c_str()); } diff --git a/tests/pytest/runtimemanager/test_clear_retained.py b/tests/pytest/runtimemanager/test_clear_retained.py new file mode 100644 index 0000000..7485d3c --- /dev/null +++ b/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": "", + }