feat(retain): retain-variable persistence for runtime v4 [NODE-94]

[thiagoralves]

The runtime's half of RETAIN support. Lands with STruC++ #222, openplc-editor #1034 and openplc-web #691.

The split

The runtime marshals; a plugin stores. Marshalling itself lives inside the loaded .so — STruC++'s iec_retain.hpp, reached through the strucpp_retain_* exports — because that is where the debug tables live, and because one copy of a wire format is better than two.

void plc_retain_init(void);   /* decide once whether retain can run, and log what it found */
void plc_retain_load(void);   /* restore, before the first task is released */
void plc_retain_save(void);   /* every scan cycle, from the dispatcher's quiescent window */
void plc_retain_clear(void);  /* cold reset: on upload, or the RETAIN:CLEAR socket command */

Why there is no default backend

Retention hardware is a property of the device, not of the runtime: an SLM-RP4 has a data partition, another box has FRAM or battery-backed SRAM, a third has nothing. A file-backed default here would look like support on every device and be wrong on most of them. With no plugin the calls are no-ops and retain degrades to NON_RETAIN — what the runtime did before this existed.

Plugins opt in by exporting retain_save / retain_load / retain_clear, dlsym'd optionally in the same pattern get_stats already uses. Both save and load are required: a store that accepts values every scan and never gives them back looks like working retention right up until the reboot that matters. Two stores would both appear to work and disagree on the next boot, so first wins and the rest are named in the log.

Where the calls sit

plc_retain_save() runs in the dispatcher's quiescent window — g_tasks_running == 0 under image_lock(), the same guarantee image_tables_copy_config_globals_out() relies on. Reading the leaves anywhere else would race the task threads.

Restores go through runtime_external_write(..., DBGW_OP_WRITE, ...), not straight to the IECVar. A retained variable may also be located (VAR RETAIN x AT %MW10), and poking such a leaf's storage directly is undone by the next copy-in from the process image — the value would appear to restore and then silently revert on the first scan. DBGW_OP_WRITE and never a force: restoring must not pin a value, and an operator's force has to stay authoritative.

A bug this branch also fixes

Loading a plugin resolves its symbols; only starting it is gated on enabled. The store selection checked neither, so a disabled plugin was still chosen — retain logged itself active, took the blob every cycle, and kept none of it. Found on hardware by the most ordinary route: an upload rewrote plugins.conf, disabled the storage plugin, and retain went on claiming to work.

Verification

On an SLM-RP4, the blob on disk decoded to exactly the designed header — magic=0x4F52 format=1 layout=0x618b1d38 payload_len=40 crc=… — with the layout hash matching the editor's debug-map.json. The flush mtime advanced every 5s while retain_save ran at 50 Hz, roughly 250 API calls per write, which is the asynchronous contract working as intended.

Across a STOP/START that unloads and reloads the .so, a retained TON came back already elapsed while an identical un-retained one restarted its ten-second wait.

🤖 Generated with Claude Code

https://claude.ai/code/session_012FNp61926UEggtmsQPj3A3

[dcoutinho1328]

Reviewed the retain-variable persistence feature (runtime side of NODE-94). The design is solid and well-reasoned (marshal-in-runtime/store-in-plugin split, first-store-wins, DBGW_OP_WRITE instead of a force for restores), but I found two correctness/safety issues worth fixing before merge, plus a testing gap and a minor diagnostics suggestion. Details inline.

[dcoutinho1328]

❌ Correctness — integer truncation at the retain buffer boundary

RETAIN_BUFFER_MAX is 64 * 1024 = 65536, but the plugin retain API (plugin_retain_save_func_t/plugin_retain_load_func_t in plugin_driver.h) takes uint16_t len/cap (max 65535). The needed > RETAIN_BUFFER_MAX check a few lines below admits needed == 65536, so a program whose retain blob is exactly 65536 bytes gets g_buffer.assign(65536, 0) and g_active = true. Then in plc_retain_load() and plc_retain_save(), the (uint16_t) casts of g_buffer.size() / the packed length wrap 65536 to 0 — the plugin's retain_load/retain_save gets called with cap/len == 0. Retain silently fails to restore or persist for exactly the one blob size the runtime's own error message a few lines above ("handles at most %zu") claims is supported.

Fix: cap RETAIN_BUFFER_MAX at UINT16_MAX (65535) instead of 64 * 1024, or widen the plugin retain API to uint32_t/size_t.

[thiagoralves]

Fixed in #178 — the analysis is exactly right.

RETAIN_BUFFER_MAX is now UINT16_MAX. Your trace was correct end to end: needed > 65536 admitted 65536, g_active went true, and then all four (uint16_t) casts — two in plc_retain_load, two on the save path — wrapped to 0, so the plugin got cap/len 0 and retain silently neither saved nor restored. At exactly the one size the error message above it advertises as supported.

[dcoutinho1328]

❌ Thread safety — retain_save/retain_clear can run concurrently on the same plugin instance with no synchronization

This runs on the Unix Socket Thread, invoking plc_retain_clear() while plc_retain_save() runs every scan cycle on the PLC Cycle Thread whenever the PLC is RUNNING (plc_state_manager.cpp). webserver/app.py's handle_upload_file() calls runtime_manager.clear_retained() unconditionally on every upload — it never checks whether the PLC is currently RUNNING. So uploading a new program while the current one is still running fires RETAIN:CLEAR on the socket thread at the same moment the scan thread may be inside plugin_driver_retain_save() (plugin_driver.c) for the same plugin instance. Neither plugin_driver_retain_save/_load/_clear nor anything upstream takes a lock around the call into the plugin's C function pointers, and neither the PR description nor the core/src/drivers/README.md contract documents that a plugin's retain_save/retain_load/retain_clear must tolerate concurrent invocation from two different threads.

For a plugin backed by a file or a not-fully-atomic write (the expected case — FRAM/SRAM/data-partition backends), this can corrupt or lose the stored blob.

Fix: either serialize retain calls (a mutex around the plugin's retain entry points), gate clear_retained()/upload on the PLC being STOPPED, or explicitly document in the plugin contract that these callbacks may be invoked concurrently and must self-synchronize.

[thiagoralves]

Fixed in #178, and there was a sharper version of this inside the built-in store.

You are right that nothing serialised the plugin entry points. The driver now takes one mutex across retain_save / retain_load / retain_clear, held only for the plugin call — which the contract already requires to return promptly, so an uncontended acquire per scan is the whole cost. That is the option you listed first, chosen over documenting "must self-synchronise" because it puts the burden in one place instead of on every vendor.

The built-in file store had the same race in a more concrete form: g_lock covers the staging buffer and is released before the write (deliberately, so the scan thread never waits on disk I/O), which left the file unguarded. A clear could remove() it while a commit sat between its write and its rename, and the rename then republished the blob a moment after it was meant to be gone — the next start would restore the previous program's values, which is precisely what clear-on-upload exists to prevent. A separate g_store_lock now serialises the file operations.

Exercised on an SLM-RP4: an upload against a running PLC now leaves the store absent and the next start logs nothing stored yet.

I did not gate clear_retained() on the PLC being STOPPED. Making the clear correct under concurrency seemed better than making the upload depend on PLC state, but say the word if you would rather have the gate as well.

[dcoutinho1328]

❌ Testing — no coverage for the new retain-store selection or clear-on-upload behavior

This PR adds four new public functions here (plugin_driver_find_retain_store, plugin_driver_retain_save/load/clear) plus a whole new subsystem (plc_retain.cpp) and a new RuntimeManager.clear_retained() method, but no test file touches any of it. tests/test_plugin_driver.c already exists and tests this exact file (plugin lifecycle, config, init failure paths) — the "first store wins, rest are logged and ignored" selection logic and the "disabled plugin is not a store" bug fix bundled in this same PR (both genuinely subtle, regression-prone logic) have zero coverage there. review-guidelines.md also calls out pytest coverage for the Python side (clear_retained() / the upload-triggers-clear behavior in handle_upload_file), which is likewise untested.

Suggestion: at minimum, add cases to tests/test_plugin_driver.c for (a) first-plugin-wins when two plugins export both hooks, (b) a disabled plugin is never chosen, (c) a plugin exporting only one of retain_save/retain_load is not chosen; and a pytest case for clear_retained()'s error-swallowing behavior.

[thiagoralves]

Partly addressed in #178, and I want to be straight about the part I declined.

Added: 14 pytest cases for clear_retained() and retain_status() — that a clear never propagates a failure into the upload, that a failure reports ERROR rather than OK (the caller logs "cleared", so answering OK about a device that still holds the old values is the failure that matters), and that an unreadable or truncated status reply comes back unknown rather than being guessed as "no retention configured".

Not added: the cases in tests/test_plugin_driver.c. That harness has no build target in CMakeLists, no CI wiring, and unity.h is not vendored anywhere in the repo — it cannot compile or run today. Cases added there would read as coverage and be none, which I think is worse than an honest gap.

So the C-side selection logic you name — first-store-wins, disabled-is-not-a-store, half-a-store-is-not-a-store — is currently proven only on hardware. That is a real hole and your instinct about it is right; the fix is giving that harness a build, which is its own piece of work. Happy to take it if you want it in this cycle.

[dcoutinho1328]

⚠️ A plugin exporting only one of retain_save/retain_load is rejected silently

This correctly requires both hooks, but a plugin author who implements only one (typo, WIP, or misunderstanding) gets no diagnostic — it's silently treated as "not a store," identical to a plugin with neither. The "two full stores" case a few lines below (plugin_driver_find_retain_store) does log a warning naming the plugin. Given the whole point of this PR's disabled-plugin fix was surfacing exactly this kind of silent misconfiguration, a one-line log_warn here ("plugin '%s' exports retain_save but not retain_load, ignoring it as a retain store") would be consistent and cheap.

[thiagoralves]

Fixed in #178.

plugin_provides_retain_store now logs when exactly one of the two hooks is present, naming the plugin and which half is missing. Your framing is the reason it went in: the whole point of the disabled-plugin fix was surfacing silent misconfiguration, and half a store is a typo or an unfinished driver rather than a configuration — treating it identically to "no store at all" leaves the vendor nothing to go on.

[dcoutinho1328]

PR #174 Review

Summary

Runtime-side implementation of retain-variable persistence for STruC++ programs (NODE-94): the runtime marshals retained variables into an opaque blob (via strucpp_retain_* exports from the loaded .so) and hands it to a native plugin that owns the storage medium. Adds plc_retain.{cpp,h}, a RETAIN:CLEAR socket command, an optional retain_save/retain_load/retain_clear plugin contract, and a webserver hook that clears stored retain values on program upload. Also fixes a real bug where a disabled plugin could still be selected as the retain store.

Module: PLC Runtime Core / Plugin System (C/C++)

Checklist Results

• ✅ Real-time safety — plc_retain_save() runs from the documented quiescent window (g_tasks_running == 0, under image_lock), consistent with image_tables_copy_config_globals_out()'s existing guarantee; no allocation or blocking call added to the steady-state scan path itself.
• ✅ Plugin interface contract — new hooks are optional and dlsym'd the same way get_stats already is; absence degrades gracefully to NON_RETAIN.
• ❌ Correctness (boundary bug) — RETAIN_BUFFER_MAX = 64 * 1024 (65536) is one past what fits in the plugin API's uint16_t len/cap. A program with an exactly-65536-byte retain blob gets cap/len silently truncated to 0 on both save and load. See inline comment on plc_retain.cpp:36.
• ❌ Thread safety — plc_retain_clear() (Unix Socket Thread, triggered unconditionally by upload) and plc_retain_save() (PLC Cycle Thread, every scan while RUNNING) can invoke the same plugin instance's C callbacks concurrently with no synchronization and no documented constraint on the plugin contract. See inline comment on unix_socket.c:323.
• ❌ Test coverage — zero tests for the four new plugin_driver_*_retain_* functions or plc_retain.cpp, despite tests/test_plugin_driver.c already existing for this exact file. See inline comment on plugin_driver.c:1510.
• ⚠️ Diagnostics — a plugin exporting only one of retain_save/retain_load is silently ignored with no log, unlike the "two full stores" case a few lines away. See inline comment on plugin_driver.c:1488.

Code Improvement Findings

No duplication or extraction issues found; the retain marshalling split (runtime owns the buffer/call sites, .so owns the wire format) is consistent with how image_tables.cpp already handles other optional strucpp_* exports (required=false resolution, graceful degradation).

Module: REST API Server (Python)

Checklist Results

• ✅ Error handling — clear_retained() catches (OSError, socket.error) then a broad Exception fallback, both logged with context; matches the project's "never fail an upload over this" intent.
• ❌ Test coverage — no pytest coverage added for RuntimeManager.clear_retained() or the upload-triggers-clear behavior in handle_upload_file. Grouped with the C-side testing finding above (plugin_driver.c:1510).

Cross-Module Observations

The thread-safety finding spans three files: webserver/app.py (handle_upload_file never checks PLC run state before triggering clear), unix_socket.c (dispatches RETAIN:CLEAR on the socket thread), and plc_state_manager.cpp/plc_retain.cpp (the scan thread's plc_retain_save()). Both sides of the race are new code introduced by this PR, so it's in scope here rather than pre-existing debt.

Verdict

• Request Changes
• Two blocking correctness/safety issues (retain-buffer truncation at the 64KB boundary; unsynchronized concurrent plugin callback invocation between the socket thread and the scan thread) plus a missing-tests finding that review-guidelines.md calls out explicitly for the Python side and that the existing tests/test_plugin_driver.c convention calls out for the C side.

[thiagoralves]

Review response — fixes in #178

@dcoutinho1328 all four findings were valid; three are fixed and one is partly fixed with the gap named. Each inline thread has its own reply.

Fixed

plc_retain.cpp:37 — the buffer cap admitted a size the API cannot express. Now UINT16_MAX. Your trace held end to end: needed > 65536 admitted 65536, g_active went true, and all four (uint16_t) casts wrapped to 0 — so the plugin got cap/len 0 and retain silently neither saved nor restored, at exactly the size the error message above it advertises.

unix_socket.c:323 — the clear/save race. Fixed, and there was a sharper version of it inside the built-in store that the comment did not reach:

• Plugin path: the driver now takes one mutex across retain_save / retain_load / retain_clear. That is your first option, chosen over documenting "must self-synchronise" because it puts the burden in one place rather than on every vendor. Held only across the plugin call, which the contract already requires to return promptly.
• Built-in file store: g_lock covers the staging buffer and is released before the write — deliberately, so the scan thread never waits on disk I/O — which left the file unguarded. A clear could remove() it while a commit sat between its write and its rename, and the rename then republished the blob a moment after it was meant to be gone. The next start would restore the previous program's values, which is precisely what clear-on-upload exists to prevent. A separate g_store_lock now serialises the file operations.

I did not gate clear_retained() on the PLC being STOPPED. Making the clear correct under concurrency seemed better than making an upload depend on PLC state — but that was a judgement call and I am happy to add the gate as well if you would rather have both.

plugin_driver.c:1488 — half a store rejected silently. Now logs, naming the plugin and the missing half. Your framing is why it went in: the whole point of the disabled-plugin fix was surfacing silent misconfiguration, and half a store is a typo or an unfinished driver rather than a configuration.

Partly fixed — and the part I declined

plugin_driver.c:1510 — no coverage for the new retain paths.

Added: 14 pytest cases for clear_retained() and retain_status() — that a clear never propagates a failure into the upload, that a failure reports ERROR rather than OK (the caller logs "cleared", so answering OK about a device that still holds the old values is the failure that matters), and that an unreadable or truncated status reply comes back unknown rather than being guessed as "no retention configured".

Not added: the cases in tests/test_plugin_driver.c. That harness has no build target in CMakeLists, no CI wiring, and unity.h is not vendored anywhere in the repo — it cannot compile or run today. Cases added there would read as coverage and be none, which I think is worse than an honest gap.

So the C-side selection logic you named — first-store-wins, disabled-is-not-a-store, half-a-store-is-not-a-store — is currently proven only on hardware. That is a real hole and you are right to push on it; the fix is giving that harness a build, which is its own piece of work. I will take it if you want it in this cycle.

Verification

Rebuilt on an SLM-RP4 via install.sh, clean. Retain still restores across a program reload:

before reload   dwell(RETAIN): Q=True  ET=10s  STATE=2  |  loose: Q=True  ET=10s
after reload    dwell(RETAIN): Q=True  ET=10s  STATE=2  |  loose: Q=False ET=3s200ms

And the race the lock was added for: an upload against a running PLC now leaves the store absent, with the next start logging nothing stored yet — the clear is honoured and no in-flight flush resurrects it.