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

[thiagoralves]

Addresses the review on #174. Targets the feature branch so #174 picks it up.

The buffer cap admitted a size the API cannot express

RETAIN_BUFFER_MAX was 64 * 1024 = 65536 while the plugin retain API takes uint16_t len / cap, whose maximum is 65535. So needed == 65536 passed the check, 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 exactly the one size the error message above it claims is supported. Capped at UINT16_MAX.

Clear could be undone by an in-flight flush

plc_retain_save() runs on the PLC cycle thread every scan; RETAIN:CLEAR runs on the control-socket thread, and the upload path fires it unconditionally without waiting for the PLC to stop. Two distinct problems:

The 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. That 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 restored the previous program's values — the exact thing clear-on-upload exists to prevent. A separate g_store_lock now serialises the file operations, still without holding anything across the scan path.

The plugin store. No lock at all, so a vendor's retain_save and retain_clear could be entered concurrently. Nothing in the plugin contract says they must be reentrant with each other, and for the expected backends — a file, an FRAM page, an NVS partition — that overlap is how a store ends up torn. The driver now serialises its three retain entry points; held only across the plugin call, which the contract already requires to return promptly.

Half a store is a mistake, not a configuration

A plugin exporting retain_save but not retain_load (or the reverse) was silently treated as "not a store" — indistinguishable from a plugin that never meant to provide one. The two-stores case already logs; this is the same courtesy for the more likely error, which is a typo or an unfinished driver.

Tests, and one I deliberately did not write

14 new pytest cases for clear_retained() and retain_status(): that a clear never propagates a failure into the upload, that a failure is reported as ERROR rather than OK (the caller logs "cleared"), and that an unreadable or truncated status reply comes back unknown rather than being guessed as "no retention configured" — the Persistent Storage screen would otherwise state something it does not know.

The review also asked for cases in tests/test_plugin_driver.c. I did not add them, and the reason is worth recording: 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 look like coverage and be none. The C-side selection logic (first-store-wins, disabled-is-not-a-store, half-a-store-is-not-a-store) is currently proven only on hardware; giving that harness a build is its own piece of work, and worth doing.

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 — the clear is honoured, no in-flight flush resurrects it, and the next start logs nothing stored yet.

🤖 Generated with Claude Code

https://claude.ai/code/session_012FNp61926UEggtmsQPj3A3