diff --git a/core/src/CMakeLists.txt b/core/src/CMakeLists.txt index 8b637a0a..70f5c286 100644 --- a/core/src/CMakeLists.txt +++ b/core/src/CMakeLists.txt @@ -45,6 +45,7 @@ add_executable(plc_main ${CMAKE_SOURCE_DIR}/core/src/plc_app/debug_write_journal.cpp ${CMAKE_SOURCE_DIR}/core/src/plc_app/plc_io_cycle.cpp ${CMAKE_SOURCE_DIR}/core/src/plc_app/plc_state_manager.cpp + ${CMAKE_SOURCE_DIR}/core/src/plc_app/plc_switch.c ${CMAKE_SOURCE_DIR}/core/src/plc_app/plcapp_manager.c ${CMAKE_SOURCE_DIR}/core/src/plc_app/scan_cycle_manager.c ${CMAKE_SOURCE_DIR}/core/src/drivers/plugin_driver.c diff --git a/core/src/drivers/plugin_driver.c b/core/src/drivers/plugin_driver.c index 2aab583c..8cac385a 100644 --- a/core/src/drivers/plugin_driver.c +++ b/core/src/drivers/plugin_driver.c @@ -17,6 +17,7 @@ #include "../plc_app/image_tables.h" #include "../plc_app/journal_buffer.h" #include "../plc_app/plc_state_manager.h" +#include "../plc_app/plc_switch.h" #include "../plc_app/unix_socket.h" #include "../plc_app/utils/log.h" #include "../plc_app/utils/utils.h" @@ -158,21 +159,71 @@ static uint8_t plugin_debug_write(uint8_t arr, uint16_t elem, // plugin's own stop_loop is invoked. Plugins that enter fault-stopped state // are expected to short-circuit their I/O during that window. // -// No pre-check on plc_get_state() here: plc_begin_transition does the -// check atomically (under the same gate that prevents concurrent -// transitions), so doing it again outside would just re-introduce the -// check-then-act race the gate is there to close. +// No pre-check on plc_get_state() here: plc_claim_transition does the check +// atomically under the state lock (the state being TRANSITIONING is itself what +// prevents concurrent transitions), so doing it again outside would just +// re-introduce the check-then-act race that interlock exists to close. static void plugin_request_plc_stop(const char *reason) { - log_error("[PLUGIN] stop requested: %s", reason ? reason : "(no reason given)"); + log_info("[PLUGIN] stop requested: %s", reason ? reason : "(no reason given)"); if (!plc_begin_transition(PLC_STATE_STOPPED)) { // Either the PLC is already stopping/stopped or another stop // is already in flight — either way, nothing to do. + // + // A stop dropped because another transition was in flight is recovered + // by the switch-movement reconciliation once that transition lands (see + // transition_worker in unix_socket.c), so a switch-driven stop cannot be + // lost here. log_warn("[PLUGIN] stop request collapsed (already transitioning or not running)"); } } +// Mirror of plugin_request_plc_stop, routed through the same transition path +// the socket START command uses. Gated on the mode switch: hardware is +// authoritative no matter who asks, so a plugin cannot start a PLC whose switch +// reads STOP (which also keeps a buggy plugin from defeating the interlock). +static void plugin_request_plc_start(const char *reason) +{ + if (!plc_switch_allows_run()) + { + log_warn("[PLUGIN] start request refused — mode switch is in STOP (%s)", + reason ? reason : "(no reason given)"); + return; + } + + log_info("[PLUGIN] start requested: %s", reason ? reason : "(no reason given)"); + if (!plc_begin_transition(PLC_STATE_RUNNING)) + { + log_warn("[PLUGIN] start request collapsed (already transitioning or already running)"); + } +} + +// Thin adapter so plugin_types.h can declare the position as a plain int and +// stay free of the plc_app include tree. +static void plugin_set_switch_position(int position) +{ + plc_set_switch_position(position == PLC_SWITCH_STOP ? PLC_SWITCH_STOP : PLC_SWITCH_RUN); +} + +// Map the runtime's PLCState onto the values FC 0x49 reports on baremetal +// targets (0 = STOPPED, 1 = RUNNING, 2 = ERROR) so vendor code driving a +// status LED can share one mapping across both target types. INIT and EMPTY +// are v4-only and have no physical meaning for an indicator, so they report as +// STOPPED — the PLC is not executing. +static int plugin_get_plc_state(void) +{ + switch (plc_get_state()) + { + case PLC_STATE_RUNNING: + return 1; + case PLC_STATE_ERROR: + return 2; + default: + return 0; + } +} + // Python capsule destructor for runtime args // Breakpoint here to debug capsule issues @@ -687,6 +738,21 @@ int plugin_driver_cleanup_init(plugin_driver_t *driver) return cleaned; } +void plugin_driver_release_gil(void) +{ + if (!Py_IsInitialized()) + { + return; + } + /* Idempotent: a second call with the GIL already released would be calling + * PyEval_SaveThread() without holding it. */ + if (main_tstate != NULL) + { + return; + } + main_tstate = PyEval_SaveThread(); +} + // Call the thread function for each plugin int plugin_driver_start(plugin_driver_t *driver) { @@ -701,11 +767,19 @@ int plugin_driver_start(plugin_driver_t *driver) return 0; } - // Only manage Python GIL if we have Python plugins and Python is initialized + // Only manage Python GIL if we have Python plugins and Python is initialized. + // + // Acquire-then-save leaves this thread without the GIL, which is the point: + // the plugin threads started below need it. The saved state is deliberately + // NOT stored in main_tstate -- this runs on the PLC cycle thread, and + // main_tstate is what plugin_driver_destroy restores before Py_FinalizeEx(), + // which must be the MAIN thread's state. Overwriting it here meant a shutdown + // after a start restored a state belonging to a thread that no longer exists. + // plugin_driver_release_gil() owns that value. if (has_python_plugin && Py_IsInitialized()) { - gstate = PyGILState_Ensure(); - main_tstate = PyEval_SaveThread(); + gstate = PyGILState_Ensure(); + PyEval_SaveThread(); } for (int i = 0; i < driver->plugin_count; i++) @@ -920,10 +994,18 @@ void plugin_driver_destroy(plugin_driver_t *driver) if (python_initialized) { - PyGILState_Release(local_gstate); + /* Py_FinalizeEx() requires the GIL, and getting there with it released is + * what used to segfault the runtime on every graceful shutdown where the + * PLC had never run (Py_FinalizeEx -> PyImport_GetModule with no thread + * state). main_tstate is only non-NULL once the GIL has been saved by the + * main thread, so when it is NULL the right move is to KEEP the state + * PyGILState_Ensure() gave us above rather than dropping it. */ if (main_tstate != NULL) { + PyGILState_Release(local_gstate); PyEval_RestoreThread(main_tstate); + /* Consumed: a second destroy must not restore a stale state. */ + main_tstate = NULL; } Py_FinalizeEx(); } @@ -1025,6 +1107,13 @@ void *generate_structured_args_with_driver(plugin_type_t type, plugin_driver_t * // Plugin-initiated async PLC stop (for unrecoverable hardware faults). args->request_plc_stop = plugin_request_plc_stop; + // Run/stop control: the same transition path the socket START / STOP + // commands drive, plus the mode-switch store the runtime gates starts on + // and the state read a plugin uses to drive a status LED. + args->request_plc_start = plugin_request_plc_start; + args->set_switch_position = plugin_set_switch_position; + args->get_plc_state = plugin_get_plc_state; + // PLC base tick time. Runtime owns base_tick_ns; on first plugin init // (before symbols_init) it carries the 20 ms default, so plugins must // guard against the value being smaller than their needed resolution. diff --git a/core/src/drivers/plugin_driver.h b/core/src/drivers/plugin_driver.h index 62994081..b834ee3f 100644 --- a/core/src/drivers/plugin_driver.h +++ b/core/src/drivers/plugin_driver.h @@ -100,6 +100,19 @@ int plugin_driver_start(plugin_driver_t *driver); int plugin_driver_stop(plugin_driver_t *driver); void plugin_driver_destroy(plugin_driver_t *driver); +/* Release the Python GIL held by the calling thread after plugin loading, and + * remember the thread state so plugin_driver_destroy() can restore it before + * Py_FinalizeEx(). + * + * Call from the MAIN thread, once, after plugins are loaded. The runtime used to + * call PyEval_SaveThread() directly and drop the returned state on the floor, + * which left the driver with nothing to restore: shutdown then finalised the + * interpreter with no GIL held and segfaulted -- on every graceful shutdown of a + * runtime whose PLC had never started, safe mode included. Keeping the + * bookkeeping next to the code that consumes it is what makes that + * unrepresentable. No-op when Python was never initialised. */ +void plugin_driver_release_gil(void); + // Cycle hook functions for native plugins (called during PLC scan cycle) // These iterate through all active native plugins and call their cycle hooks // Plugins opt-in by implementing cycle_start/cycle_end; opt-out by not implementing them diff --git a/core/src/drivers/plugin_types.h b/core/src/drivers/plugin_types.h index 36e3f424..62ca61e6 100644 --- a/core/src/drivers/plugin_types.h +++ b/core/src/drivers/plugin_types.h @@ -100,6 +100,77 @@ typedef uint8_t (*plugin_debug_write_func_t)(uint8_t arr, uint16_t elem, */ typedef void (*plugin_request_plc_stop_func_t)(const char *reason); +/** + * @brief PLC start request from a plugin + * + * Mirror of plugin_request_plc_stop: both are thin wrappers over the same + * transition path the socket START / STOP commands drive, so a plugin-initiated + * run/stop is byte-for-byte the flow the editor already exercises. + * + * Refused while the stored mode-switch position is STOP (see + * plugin_set_switch_position_func_t) -- hardware is authoritative regardless of + * who asks. Non-blocking; `reason` is logged. Safe to call from any plugin + * thread, including while the PLC is stopped: the runtime keeps every plugin + * mapped across a stop, so this is how a hardware STOP -> RUN edge starts the + * PLC again. + */ +typedef void (*plugin_request_plc_start_func_t)(const char *reason); + +/** + * @brief Report the run/stop mode-switch position + * + * THIS is the hardware interface on Linux. The runtime never reads hardware and + * never polls: the package decides how and how often it samples (GPIO + * interrupt, sysfs poll, fieldbus callback, its own thread) and calls this + * whenever the position changes. + * + * Stores only -- it starts nothing and stops nothing. On a switch change the + * plugin calls this and THEN asks for the matching transition: + * + * args->set_switch_position(PLC_SWITCH_STOP); + * args->request_plc_stop("mode switch moved to STOP"); + * + * Always store the position BEFORE requesting the transition. On a falling edge + * that closes the window where a start could slip in; on a rising edge it stops + * the request being refused by the guard still reading the stale STOP. + * + * `position` is a plc_switch_t (0 = STOP, 1 = RUN). Declared as int here so + * this header stays free of the plc_app include tree. Idempotent, and safe to + * call from any plugin thread. + * + * Sample the switch from a thread started in init() and torn down in cleanup(), + * NOT in start_loop() / stop_loop(): the runtime calls stop_loop when the PLC + * stops, which is precisely when the position matters most. Make that start + * idempotent, because init() is re-entered on each PLC start. Reporting the + * initial position from init() also gates the boot auto-start, so a device + * powered up with its switch in STOP never starts and then bounces. + */ +typedef void (*plugin_set_switch_position_func_t)(int position); + +/** + * @brief Read the current PLC state + * + * Returns 0 = STOPPED, 1 = RUNNING, 2 = ERROR (matching the values FC 0x49 + * reports on baremetal targets, so vendor code can share one mapping). + * + * There is no state-change callback: a plugin driving a panel LED polls this. + * A device with no LED never calls it and the runtime is unaware. + * + * Poll it from the plugin's OWN thread -- the same sampler that reads the mode + * switch is the natural home. Do NOT poll it from cycle_end: cycle_end fires + * once per scan, so it stops being called the moment the PLC stops, and an LED + * driven from there would freeze showing RUNNING exactly when it needs to show + * STOPPED. + * + * IMPORTANT (applies to every field of plugin_runtime_args_t): the runtime + * frees the args pointer as soon as init() returns. Copy the struct BY VALUE + * during init -- `memcpy(&my_args, args, sizeof(plugin_runtime_args_t))` -- + * and call through the copy. Caching the pointer is a use-after-free; calling + * a function pointer read back out of the freed struct crashes. See the + * s7comm plugin for the established pattern. + */ +typedef int (*plugin_get_plc_state_func_t)(void); + /** * @brief Runtime buffer access structure for plugins * @@ -182,6 +253,25 @@ typedef struct * Populated when the runtime initializes the plugin; may be 0 if * symbols are not yet resolved (plugin must guard against zero). */ unsigned long long base_tick_ns; + + /* --------------------------------------------------------------------- + * Run/stop control. Appended at the end of the struct so plugin binaries + * compiled against an earlier layout keep their field offsets. + * + * A plugin that ignores all three behaves exactly as before: the switch + * position stays at its RUN default, so every start path is unguarded. + * ------------------------------------------------------------------- */ + + /* Async request to run — see plugin_request_plc_start_func_t. */ + plugin_request_plc_start_func_t request_plc_start; + + /* Report the mode-switch position — see + * plugin_set_switch_position_func_t. The hardware interface on Linux. */ + plugin_set_switch_position_func_t set_switch_position; + + /* Current PLC state, for driving a status LED — see + * plugin_get_plc_state_func_t. */ + plugin_get_plc_state_func_t get_plc_state; } plugin_runtime_args_t; #endif /* PLUGIN_TYPES_H */ diff --git a/core/src/drivers/plugins/python/shared/plugin_runtime_args.py b/core/src/drivers/plugins/python/shared/plugin_runtime_args.py index 05de87a8..3514fcc8 100644 --- a/core/src/drivers/plugins/python/shared/plugin_runtime_args.py +++ b/core/src/drivers/plugins/python/shared/plugin_runtime_args.py @@ -81,8 +81,24 @@ class PluginRuntimeArgs(ctypes.Structure): ("journal_write_int", ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_int)), ("journal_write_dint", ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_uint)), ("journal_write_lint", ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_ulonglong)), + # Async request to stop the whole PLC: void (*)(const char *reason). + # + # This entry was missing while the C struct had the field, so every + # field after it was shifted by one pointer -- `base_tick_ns` was + # actually reading the request_plc_stop pointer. Keep this list in + # lockstep with plugin_types.h; the offsets are load-bearing. + ("request_plc_stop", ctypes.CFUNCTYPE(None, ctypes.c_char_p)), # PLC base tick time in nanoseconds (mirrors C-side base_tick_ns). ("base_tick_ns", ctypes.c_ulonglong), + # --- Run/stop control (appended in C, so appended here too) --------- + # Async request to run: void (*)(const char *reason). Refused while the + # mode switch reads STOP. + ("request_plc_start", ctypes.CFUNCTYPE(None, ctypes.c_char_p)), + # Report the mode-switch position: void (*)(int position), + # 0 = STOP, 1 = RUN. Store the position BEFORE requesting a transition. + ("set_switch_position", ctypes.CFUNCTYPE(None, ctypes.c_int)), + # Current PLC state: int (*)(void), 0 = STOPPED, 1 = RUNNING, 2 = ERROR. + ("get_plc_state", ctypes.CFUNCTYPE(ctypes.c_int)), ] def validate_pointers(self): diff --git a/core/src/plc_app/plc_main.c b/core/src/plc_app/plc_main.c index ed8e3dfc..b8ed246b 100644 --- a/core/src/plc_app/plc_main.c +++ b/core/src/plc_app/plc_main.c @@ -15,6 +15,7 @@ #include "../drivers/plugin_driver.h" #include "image_tables.h" #include "plc_state_manager.h" +#include "plc_switch.h" #include "plcapp_manager.h" #include "unix_socket.h" #include "utils/log.h" @@ -26,7 +27,10 @@ volatile sig_atomic_t keep_running = 1; plugin_driver_t *plugin_driver = NULL; extern bool print_logs; -void handle_sigint(int sig) +/* Graceful shutdown for both signals that mean "stop": SIGINT from an + * interactive Ctrl-C, and SIGTERM from a supervisor. Drops out of the main loop + * so the program is stopped and the plugin driver torn down on the way out. */ +void handle_shutdown_signal(int sig) { (void)sig; keep_running = 0; @@ -82,12 +86,22 @@ int main(int argc, char *argv[]) return -1; } - // Handle SIGINT for graceful shutdown + // Handle SIGINT and SIGTERM for graceful shutdown. + // + // SIGTERM matters as much as SIGINT and was missing: RuntimeManager stops the + // runtime with process.terminate() (SIGTERM) and only escalates to + // process.kill() after a 5 s wait, and systemd's default KillSignal is also + // SIGTERM. With no handler installed the default disposition applied, so every + // supervisor-initiated stop killed the process outright -- the PLC program was + // never unloaded, plugins never stopped, the journal never flushed, and the + // program .so never dlclose'd. The escalation to SIGKILL is the backstop for a + // shutdown that hangs, not the normal path. struct sigaction sa; - sa.sa_handler = handle_sigint; + sa.sa_handler = handle_shutdown_signal; sigemptyset(&sa.sa_mask); sa.sa_flags = 0; sigaction(SIGINT, &sa, NULL); + sigaction(SIGTERM, &sa, NULL); // Install the process-wide SIGUSR1 wake handler exactly once. Task // threads (plc_state_manager.cpp) and EtherCAT bus threads @@ -103,8 +117,9 @@ int main(int argc, char *argv[]) wake_sa.sa_flags = 0; sigaction(SIGUSR1, &wake_sa, NULL); - // Make sure PLC starts in STOP state - plc_set_state(PLC_STATE_STOPPED); + // No need to force STOPPED here: plc_state is statically initialised to it, + // and plc_set_state() is now the body of a claimed transition rather than a + // setter -- calling it with nothing loaded would just log a failed unload. // Initialize watchdog if (watchdog_init() != 0) @@ -113,17 +128,20 @@ int main(int argc, char *argv[]) return -1; } - // Start UNIX socket server - if (setup_unix_socket() != 0) - { - log_error("Failed to set up UNIX socket"); - return -1; - } - // Initialize plugin driver system BEFORE loading the PLC program. // plc_set_state(RUNNING) triggers load_plc_program() which uses the plugin // driver to update config and re-init plugins, and plc_cycle_thread() calls // plugin_driver_start() after image tables are populated. + // + // This block runs BEFORE the command socket exists, deliberately. A START + // accepted while it is still executing runs load_plc_program() -> + // plugin_driver_init() on the transition worker at the same time as this + // thread is inside plugin_driver_load_config()/plugin_driver_init(), and + // rebuilding a plugin slot dlcloses the .so -- so a plugin sleeping in its + // own init() on the other thread returns into an unmapped page. Observed as a + // SIGSEGV in the main thread at an address inside the plugin that had just + // been unloaded. Transition arbitration cannot help here: there is only one + // transition, racing driver setup rather than another transition. plugin_driver = plugin_driver_create(); if (plugin_driver) { @@ -144,20 +162,35 @@ int main(int argc, char *argv[]) // This prevents a deadlock where the main thread holds the GIL forever // while sleeping, blocking other threads (like the unix socket thread) // from using Python when handling commands like START. + // + // Through the driver rather than PyEval_SaveThread() directly: the driver + // has to restore this exact thread state before Py_FinalizeEx() at + // shutdown, and the state was previously discarded here. if (Py_IsInitialized()) { - PyEval_SaveThread(); + plugin_driver_release_gil(); log_info("[PLUGIN]: Released Python GIL"); } } + // Start the command socket only now that the plugin driver is fully built. + // Everything the socket can ask for -- START, STOP, PLUGIN_CMD, STATS -- + // reaches into the driver, so serving commands before this point was serving + // them against a half-configured one. The webserver already tolerates the + // socket appearing a moment later: it connects, retries, and polls. + if (setup_unix_socket() != 0) + { + log_error("Failed to set up UNIX socket"); + return -1; + } + // Start PLC (skip in safe mode to allow program upload without loading the // faulty program that may have caused repeated crashes). // // Use plc_begin_transition() rather than plc_set_state() directly so the - // auto-start goes through the same is_transitioning CAS gate as any + // auto-start is arbitrated by plc_claim_transition() like any // socket-originated START command. This prevents a race where the socket - // listener (already running above) accepts a START before the auto-start + // listener (started just above) accepts a START before the auto-start // finishes, causing two concurrent load_plc_program() calls — and two // dispatcher threads. plc_begin_transition() also makes the start // asynchronous, which is fine: the main thread just sleeps below. @@ -166,6 +199,21 @@ int main(int argc, char *argv[]) log_info("Runtime started in SAFE MODE - PLC program will not be loaded"); log_info("Upload a corrected program to recover"); } + // Same gate as any other start, but note what it can and cannot see. A VPP + // plugin that owns a physical mode switch is initialised as part of loading + // the program — inside the start transition below — so at this point the + // switch has usually NOT been reported yet and the gate reads the default + // (RUN). A device powered up with the switch in STOP therefore does start, + // and is then corrected: the plugin reports STOP during init, that request is + // dropped because a transition is in flight, and the switch-movement + // reconciliation stops the PLC as soon as the start lands. Safe, but the gate + // only bites here for a switch position already known at this point (e.g. one + // reported by a plugin the runtime loaded independently of the program). + else if (!plc_switch_allows_run()) + { + log_info("Hardware mode switch is in STOP - PLC left stopped"); + log_info("Move the switch to RUN to start the PLC"); + } else if (!plc_begin_transition(PLC_STATE_RUNNING)) { log_error("Failed to initiate PLC start"); @@ -177,14 +225,21 @@ int main(int argc, char *argv[]) sleep(1); } - // Cleanup plugin driver system + log_info("Shutting down..."); + + // Stop the program BEFORE destroying the driver, not after. + // + // plc_state_manager_cleanup() tears the program down, and its teardown calls + // plugin_driver_stop(plugin_driver) -- so destroying the driver first left that + // call reading freed memory, and the cycle thread could still be running plugin + // cycle hooks through the same pointer while it wound down. The order here is + // the dependency order: no program, then no driver. + plc_state_manager_cleanup(); + if (plugin_driver) { plugin_driver_destroy(plugin_driver); + plugin_driver = NULL; } - - // Cleanup - log_info("Shutting down..."); - plc_state_manager_cleanup(); return 0; } diff --git a/core/src/plc_app/plc_state_manager.cpp b/core/src/plc_app/plc_state_manager.cpp index a4c976cc..6608d14f 100644 --- a/core/src/plc_app/plc_state_manager.cpp +++ b/core/src/plc_app/plc_state_manager.cpp @@ -194,9 +194,15 @@ static void *plc_task_thread(void *arg) pthread_setname_np(pthread_self(), ctx->name); + /* 99 is reserved for the dispatcher, which has to be strictly above every + * worker for its tick never to be delayed by a busy one. A worker allowed to + * reach 99 would only TIE it, and SCHED_FIFO does not time-slice between equal + * priorities: a task that never blocks (an unbounded loop in IEC code) would + * then keep the dispatcher off that CPU entirely, along with anything else + * trying to bring the PLC down. */ int rt = ctx->priority; if (rt < 1) rt = 1; - if (rt > 99) rt = 99; + if (rt > 98) rt = 98; sched_param sp{}; sp.sched_priority = rt; if (pthread_setschedparam(pthread_self(), SCHED_FIFO, &sp) != 0) @@ -363,6 +369,46 @@ static void *plc_task_thread(void *arg) return nullptr; } +/* Wake every worker, join them, and destroy the task array. + * + * Two callers: the normal end of the dispatcher loop, and the early-out below + * when bring-up finished but this start is no longer the transition in flight. + * The second one exists so that path tears its workers down instead of leaving + * them parked on a semaphore nobody will ever post again. */ +static void reap_task_threads(void) +{ + log_info("Stopping %zu PLC task thread(s)", plc_task_count); + /* Wake every worker: post its release semaphore (breaks sem_wait) and + * SIGUSR1 (breaks a syscall). A worker mid-scan finishes, loops to + * sem_wait, consumes the post, observes state != RUNNING, and exits. */ + for (size_t i = 0; i < plc_task_count; ++i) + { + sem_post(&plc_tasks[i].go); + pthread_kill(plc_tasks[i].thread, SIGUSR1); + } + for (size_t i = 0; i < plc_task_count; ++i) + { + pthread_join(plc_tasks[i].thread, nullptr); + } + + /* Take plc_tasks_lock for the tracker-cleanup + free. A STATS reader + * that started iterating before STOP arrived will block briefly + * waiting for this critical section, then exit because plc_task_count + * is observed as 0. Without the lock, the reader could be midway + * through scan_cycle_tracker_snapshot when we pthread_mutex_destroy + * the tracker's own mutex below — undefined behaviour. */ + pthread_mutex_lock(&plc_tasks_lock); + for (size_t i = 0; i < plc_task_count; ++i) + { + scan_cycle_tracker_cleanup(&plc_tasks[i].tracker); + sem_destroy(&plc_tasks[i].go); + } + std::free(plc_tasks); + plc_tasks = nullptr; + plc_task_count = 0; + pthread_mutex_unlock(&plc_tasks_lock); +} + void *plc_cycle_thread(void *arg) { PluginManager *pm = (PluginManager *)arg; @@ -442,10 +488,16 @@ void *plc_cycle_thread(void *arg) log_info("Starting main loop"); - pthread_mutex_lock(&state_mutex); - plc_state = PLC_STATE_RUNNING; - pthread_mutex_unlock(&state_mutex); - log_info("PLC State: RUNNING"); + /* NOT where RUNNING is published. The state stays TRANSITIONING_TO_RUN until + * the task threads exist and the dispatcher is about to release the first + * scan -- see the publish below the "Spawned N PLC task thread(s)" log. Two + * writes used to happen before this point (here, and in plc_set_state before + * this thread was even created), and both claimed RUNNING while nothing was + * scanning yet. The one here was also a deadlock: a stop landing in the + * window before this thread was first scheduled wrote STOPPED and joined us, + * and this line put RUNNING back -- after which no loop below would ever + * exit, so the join never returned and the runtime refused every command for + * the rest of the process's life. */ clock_gettime(CLOCK_MONOTONIC, &timer_start); @@ -708,6 +760,35 @@ void *plc_cycle_thread(void *arg) } log_info("Spawned %zu PLC task thread(s)", plc_task_count); + /* RUNNING, at last, and this is the earliest point it is true: the workers + * exist and the very next thing that happens is the dispatcher releasing the + * first scan. Nothing observes RUNNING too early as a result -- the workers + * are parked in sem_wait and are only ever posted from the loop below, and + * the loop itself needs RUNNING visible to run at all. + * + * This is also what ends the transition claimed by plc_claim_transition, so + * every path out of this thread from here on must land a final state: the + * crash recovery above publishes ERROR, and the stop path publishes STOPPED + * once teardown joins. + * + * Conditional, because ending a transition is only ours to do while it is + * still the one in flight. Two paths get here otherwise: the watchdog forced + * ERROR because this start exceeded its bound, and plc_state_manager_cleanup + * published TRANSITIONING_TO_STOP on shutdown and is now blocked joining this + * very thread. Publishing RUNNING in either case erases a state someone else + * landed, and in the second it hangs the process -- the dispatcher loop below + * would never see a non-RUNNING state and the join would never return. */ + if (!plc_publish_running_if_claimed()) + { + log_warn("PLC bring-up finished but the start is no longer the transition in " + "flight (state is %d) — not releasing the first scan", + (int)plc_get_state()); + reap_task_threads(); + signal(SIGFPE, SIG_DFL); + signal(SIGSEGV, SIG_DFL); + return NULL; + } + /* --------------------------------------------------------------------- * GCD master-tick dispatcher. * @@ -897,36 +978,7 @@ void *plc_cycle_thread(void *arg) pthread_mutex_unlock(&done_mutex); } - log_info("Stopping %zu PLC task thread(s)", plc_task_count); - /* Wake every worker: post its release semaphore (breaks sem_wait) and - * SIGUSR1 (breaks a syscall). A worker mid-scan finishes, loops to - * sem_wait, consumes the post, observes state != RUNNING, and exits. */ - for (size_t i = 0; i < plc_task_count; ++i) - { - sem_post(&plc_tasks[i].go); - pthread_kill(plc_tasks[i].thread, SIGUSR1); - } - for (size_t i = 0; i < plc_task_count; ++i) - { - pthread_join(plc_tasks[i].thread, nullptr); - } - - /* Take plc_tasks_lock for the tracker-cleanup + free. A STATS reader - * that started iterating before STOP arrived will block briefly - * waiting for this critical section, then exit because plc_task_count - * is observed as 0. Without the lock, the reader could be midway - * through scan_cycle_tracker_snapshot when we pthread_mutex_destroy - * the tracker's own mutex below — undefined behaviour. */ - pthread_mutex_lock(&plc_tasks_lock); - for (size_t i = 0; i < plc_task_count; ++i) - { - scan_cycle_tracker_cleanup(&plc_tasks[i].tracker); - sem_destroy(&plc_tasks[i].go); - } - std::free(plc_tasks); - plc_tasks = nullptr; - plc_task_count = 0; - pthread_mutex_unlock(&plc_tasks_lock); + reap_task_threads(); signal(SIGFPE, SIG_DFL); signal(SIGSEGV, SIG_DFL); @@ -948,13 +1000,15 @@ extern "C" int load_plc_program(PluginManager *pm) if (plugin_manager_load(pm)) { + /* Progress, not a state change. This used to publish PLC_STATE_INIT, + * which is now actively wrong: it overwrites TRANSITIONING_TO_RUN, so the + * runtime would stop reporting a transition in flight and let a stop be + * claimed while the start was still landing -- the same hole the + * TRANSITIONING states exist to close. (It is also what made STATUS + * flicker to INIT mid-start.) INIT survives in the enum as a startup + * value; nothing writes it during a transition. */ log_info("Loading PLC application"); - pthread_mutex_lock(&state_mutex); - plc_state = PLC_STATE_INIT; - pthread_mutex_unlock(&state_mutex); - log_info("PLC State: INIT"); - if (plugin_driver) { if (plugin_driver_update_config(plugin_driver, "./plugins.conf") != 0) @@ -1047,14 +1101,24 @@ extern "C" int unload_plc_program(PluginManager *pm) { if (pm && pm == plc_program) { - PLCState prev_state = plc_get_state(); - - if (prev_state != PLC_STATE_ERROR) + /* The dispatcher and its workers leave their loops on anything that is + * not RUNNING, and the join below depends on that. A claimed stop has + * already published TRANSITIONING_TO_STOP, which is that signal. + * + * Anything that is not ERROR is overwritten, not just RUNNING, because + * this also covers the shutdown path (plc_state_manager_cleanup), which + * tears down without claiming a transition first. On SIGTERM during a + * start the state is TRANSITIONING_TO_RUN, and a guard that only matched + * RUNNING published nothing at all -- so the cycle thread went on to + * publish RUNNING and the join below never returned. Pairs with the + * conditional publish in plc_cycle_thread: this write is what makes that + * one decline. ERROR is left alone -- it must survive teardown. */ + pthread_mutex_lock(&state_mutex); + if (plc_state != PLC_STATE_ERROR) { - pthread_mutex_lock(&state_mutex); - plc_state = PLC_STATE_STOPPED; - pthread_mutex_unlock(&state_mutex); + plc_state = PLC_STATE_TRANSITIONING_TO_STOP; } + pthread_mutex_unlock(&state_mutex); pthread_join(plc_thread, NULL); @@ -1077,7 +1141,10 @@ extern "C" int unload_plc_program(PluginManager *pm) plc_program = NULL; log_info("PLC program unloaded successfully"); - log_info("PLC State: STOPPED"); + + /* The teardown is done, so this is the moment STOPPED becomes true. + * plc_publish_final_state keeps ERROR if a task crashed on the way out. */ + plc_publish_final_state(PLC_STATE_STOPPED); return 0; } else @@ -1095,25 +1162,107 @@ extern "C" PLCState plc_get_state(void) return s; } -extern "C" bool plc_set_state(PLCState new_state) +extern "C" bool plc_state_is_transitioning(void) +{ + const PLCState s = plc_get_state(); + return s == PLC_STATE_TRANSITIONING_TO_RUN || s == PLC_STATE_TRANSITIONING_TO_STOP; +} + +extern "C" bool plc_claim_transition(PLCState target) +{ + if (target != PLC_STATE_RUNNING && target != PLC_STATE_STOPPED) + { + log_error("Refusing to transition to state %d: only RUNNING and STOPPED are" + " requestable targets", (int)target); + return false; + } + + pthread_mutex_lock(&state_mutex); + + /* Drop, don't queue. Requests are dropped while a transition is in flight, + * and the switch's intent is recovered afterwards by reconciliation (see + * plc_switch_take_movement), so nothing has to be remembered here. */ + if (plc_state == PLC_STATE_TRANSITIONING_TO_RUN || plc_state == PLC_STATE_TRANSITIONING_TO_STOP) + { + pthread_mutex_unlock(&state_mutex); + return false; + } + + if (plc_state == target) + { + pthread_mutex_unlock(&state_mutex); + return false; + } + + plc_state = (target == PLC_STATE_RUNNING) ? PLC_STATE_TRANSITIONING_TO_RUN + : PLC_STATE_TRANSITIONING_TO_STOP; + pthread_mutex_unlock(&state_mutex); + + log_info("PLC State: %s", target == PLC_STATE_RUNNING ? "TRANSITIONING_TO_RUN" + : "TRANSITIONING_TO_STOP"); + return true; +} + +extern "C" void plc_publish_final_state(PLCState final_state) +{ + const char *name = "UNKNOWN"; + switch (final_state) + { + case PLC_STATE_RUNNING: name = "RUNNING"; break; + case PLC_STATE_STOPPED: name = "STOPPED"; break; + case PLC_STATE_ERROR: name = "ERROR"; break; + case PLC_STATE_EMPTY: name = "EMPTY"; break; + default: break; + } + + pthread_mutex_lock(&state_mutex); + + /* ERROR outranks a STOPPED landing: a task that crashed mid-teardown recorded + * the fact that matters, and the teardown completing must not erase it. */ + if (plc_state == PLC_STATE_ERROR && final_state == PLC_STATE_STOPPED) + { + pthread_mutex_unlock(&state_mutex); + log_info("Transition finished in ERROR — keeping ERROR rather than STOPPED"); + return; + } + + plc_state = final_state; + pthread_mutex_unlock(&state_mutex); + log_info("PLC State: %s", name); +} + +extern "C" bool plc_publish_running_if_claimed(void) { + /* Land RUNNING only while the start we are completing is still the transition + * in flight. The check and the write share one critical section: reading the + * state and then publishing in two steps would let a stop be claimed in + * between, and RUNNING would go down on top of it. */ pthread_mutex_lock(&state_mutex); - if (plc_state == new_state) + if (plc_state != PLC_STATE_TRANSITIONING_TO_RUN) { pthread_mutex_unlock(&state_mutex); return false; } - plc_state = new_state; + plc_state = PLC_STATE_RUNNING; pthread_mutex_unlock(&state_mutex); + log_info("PLC State: RUNNING"); + return true; +} - // Note: plc_state must flip BEFORE load/unload runs. Task threads - // exit their scan loops via `while (plc_get_state() == RUNNING)`, - // and unload_plc_program() depends on that signal to join them. - // The "STATUS reports STOPPED while teardown is still in flight" - // window this opens is gated externally via is_transitioning in - // unix_socket.c — STATUS returns STATUS:TRANSITIONING for the - // duration of the worker, so external pollers don't see the - // stale STOPPED. +extern "C" bool plc_set_state(PLCState new_state) +{ + // Performs a transition already claimed via plc_claim_transition(), which + // published TRANSITIONING_TO_RUN or TRANSITIONING_TO_STOP. No state is + // written here: writing the target up front is what used to let a stop's + // STOPPED be resurrected by a start still landing. The final state is + // published by whoever knows the transition actually finished -- + // plc_cycle_thread for RUNNING (just before it releases the first scan) and + // unload_plc_program for STOPPED (after the teardown joins) -- with the + // failure paths below publishing ERROR or EMPTY. + // + // The current TRANSITIONING state is itself the signal the task and + // dispatcher loops need: they run while plc_get_state() == RUNNING, so + // TRANSITIONING_TO_STOP breaks them exactly as the old early STOPPED did. if (new_state == PLC_STATE_RUNNING) { @@ -1123,9 +1272,7 @@ extern "C" bool plc_set_state(PLCState new_state) if (libplc_path == NULL) { log_error("Failed to find libplc file"); - pthread_mutex_lock(&state_mutex); - plc_state = PLC_STATE_EMPTY; - pthread_mutex_unlock(&state_mutex); + plc_publish_final_state(PLC_STATE_EMPTY); return false; } @@ -1135,17 +1282,17 @@ extern "C" bool plc_set_state(PLCState new_state) if (plc_program == NULL) { log_error("Failed to create PluginManager"); - pthread_mutex_lock(&state_mutex); - plc_state = PLC_STATE_EMPTY; - pthread_mutex_unlock(&state_mutex); + plc_publish_final_state(PLC_STATE_EMPTY); return false; } } if (load_plc_program(plc_program) < 0) { - pthread_mutex_lock(&state_mutex); - plc_state = PLC_STATE_ERROR; - pthread_mutex_unlock(&state_mutex); + /* load_plc_program publishes ERROR or EMPTY itself on the paths it + * knows about; this covers anything it does not, so a claimed + * transition can never end without a final state. Re-publishing the + * same value is harmless. */ + if (plc_state_is_transitioning()) plc_publish_final_state(PLC_STATE_ERROR); return false; } } @@ -1153,7 +1300,21 @@ extern "C" bool plc_set_state(PLCState new_state) { if (plc_program) { - if (unload_plc_program(plc_program) < 0) return false; + if (unload_plc_program(plc_program) < 0) + { + /* Teardown failed. STOPPED is still the honest landing -- there + * is no program running -- and leaving TRANSITIONING set would + * make the runtime refuse every command from here on. */ + if (plc_state_is_transitioning()) plc_publish_final_state(PLC_STATE_STOPPED); + return false; + } + } + else + { + /* Nothing loaded, so the stop is already true. Still has to be + * published: the transition was claimed, and only a final state + * ends it. */ + plc_publish_final_state(PLC_STATE_STOPPED); } } @@ -1162,6 +1323,38 @@ extern "C" bool plc_set_state(PLCState new_state) extern "C" void plc_state_manager_cleanup(void) { + /* Let an in-flight transition finish before tearing anything down. + * + * Shutdown is the one state change that does not go through + * plc_claim_transition, so it can land on top of a start that is still + * running. Tearing down from there is not safe at any point of it: during + * plugin bring-up load_plc_program has not assigned plc_thread yet, so the + * join below would run on a handle that was never set; a moment later the + * cycle thread is mid-bring-up and would have RUNNING published underneath + * the teardown. Both disappear if the transition is allowed to land first -- + * then this is an ordinary stop of a RUNNING (or ERROR, or EMPTY) runtime. + * + * Bounded by the same constant the transition worker waits on, so a + * transition that will never land cannot hold the process open forever; the + * teardown then proceeds and does what it can. */ + const int poll_ms = 20; + int waited = 0; + while (plc_state_is_transitioning() && waited < PLC_TRANSITION_LANDING_TIMEOUT_MS) + { + struct timespec poll = { 0, (long)poll_ms * 1000000L }; + nanosleep(&poll, nullptr); + waited += poll_ms; + } + if (waited > 0) + { + log_info("Shutdown waited %d ms for the state change in flight to land", waited); + } + if (plc_state_is_transitioning()) + { + log_warn("Shutdown proceeding with a state change still in flight after %d ms", + waited); + } + if (plc_program) unload_plc_program(plc_program); } diff --git a/core/src/plc_app/plc_state_manager.h b/core/src/plc_app/plc_state_manager.h index e59acda5..9cfe24be 100644 --- a/core/src/plc_app/plc_state_manager.h +++ b/core/src/plc_app/plc_state_manager.h @@ -25,13 +25,34 @@ typedef atomic_long plc_atomic_long_t; typedef atomic_uint_least64_t plc_atomic_u64_t; #endif +/** + * Runtime states. + * + * The two TRANSITIONING values are APPENDED, never inserted: the first five are + * wire-visible (FC 0x46 reports them, and plugin_get_plc_state maps them for + * vendor status indicators), so renumbering would quietly change what boards + * report. + * + * RUNNING means running -- it is published at the moment the dispatcher is about + * to release the first scan, not when a start is requested. Everything in + * between is a TRANSITIONING state, and because both compare unequal to + * PLC_STATE_RUNNING, every loop that gates on RUNNING treats them correctly + * without modification: a stop's teardown still gets its exit signal, and a + * half-started PLC cannot scan. + * + * The direction is carried (rather than one flat TRANSITIONING) so that any code + * found to need the target state before it lands can test for the specific + * direction instead of having to reintroduce a premature RUNNING. + */ typedef enum { PLC_STATE_INIT, PLC_STATE_RUNNING, PLC_STATE_STOPPED, PLC_STATE_ERROR, - PLC_STATE_EMPTY + PLC_STATE_EMPTY, + PLC_STATE_TRANSITIONING_TO_RUN, + PLC_STATE_TRANSITIONING_TO_STOP } PLCState; /* ----------------------------------------------------------------------- @@ -109,7 +130,7 @@ extern size_t plc_task_count; * The plc_cycle_thread owns the array — it allocates after walking the * configuration (load) and frees after joining task threads (stop). * Concurrently, the unix-socket thread services STATS by iterating the - * array under format_timing_stats_response. is_transitioning gates new + * array under format_timing_stats_response. The TRANSITIONING state gates new * commands but doesn't bracket an in-flight STATS call: a plugin-initiated * stop can fire mid-iteration, free plc_tasks, and the STATS reader * dereferences freed memory. @@ -130,12 +151,79 @@ void plc_tasks_reader_unlock(void); PLCState plc_get_state(void); /** - * @brief Set the PLC state. In case of a state change, it will load or unload the PLC program as needed. - * @param new_state The new PLC state to set - * @return true if the state was changed, false if it was already in the desired state + * @brief Body of a transition that has ALREADY been claimed. Not a setter. + * + * PRECONDITION: plc_claim_transition(new_state) returned true, so a TRANSITIONING + * state is current. Calling this without a claim tears the program down with no + * TRANSITIONING_TO_STOP for the scan loops to observe and then publishes a final + * state nobody claimed, corrupting the interlock the TRANSITIONING states are. + * + * Writes no state of its own: the landing is published by whoever knows the work + * finished -- plc_cycle_thread for RUNNING, unload_plc_program for STOPPED -- with + * the failure paths here publishing ERROR or EMPTY. + * + * Go through plc_begin_transition() instead. It is the only intended entry point + * for a state change and it does the claiming for you. + * + * @param new_state PLC_STATE_RUNNING or PLC_STATE_STOPPED + * @return true if the transition body completed without error */ bool plc_set_state(PLCState new_state); +/** + * @brief Claim the right to transition, publishing the matching TRANSITIONING state. + * + * The single arbiter for "may this change start". Under the state lock: a request + * arriving while a TRANSITIONING state is current is DROPPED -- you cannot change + * state in the middle of changing state -- and so is a request for the state the + * runtime is already in. Otherwise the direction is published and the caller owns + * the transition until it publishes a final state. + * + * @param target PLC_STATE_RUNNING or PLC_STATE_STOPPED + * @return true when the transition is claimed and the caller must complete it + */ +bool plc_claim_transition(PLCState target); + +/** + * @brief Publish the state a transition landed on: RUNNING, STOPPED, ERROR or EMPTY. + * + * Ends the transition. ERROR is sticky against STOPPED: a task that crashed while + * a stop was tearing down has already recorded the more important fact, and the + * teardown finishing must not paper over it. + */ +void plc_publish_final_state(PLCState final_state); + +/** + * @brief Publish RUNNING, but only if the start being completed is still in flight. + * + * Same landing as plc_publish_final_state(PLC_STATE_RUNNING), refused when the + * current state is not TRANSITIONING_TO_RUN. Ending a transition is only the + * caller's to do while it owns it: a watchdog-forced ERROR or a shutdown that + * published TRANSITIONING_TO_STOP has taken ownership away, and overwriting either + * with RUNNING erases a landing (or, on the shutdown path, hangs the join waiting + * for a state that is never written again). + * + * @return true when RUNNING was published and the caller may start scanning + */ +bool plc_publish_running_if_claimed(void); + +/** @brief True while a transition is in flight (either direction). */ +bool plc_state_is_transitioning(void); + +/* How long a state change may plausibly take before something is wrong. + * + * ONE bound, two consumers, deliberately ordered: transition_worker stops waiting + * to observe the landing at PLC_TRANSITION_LANDING_TIMEOUT_MS, and the watchdog + * forces ERROR strictly later. Two independent numbers is how the watchdog came to + * fire 30 s before the runtime itself had given up -- ending a transition while its + * worker was still executing it. + * + * Generous on purpose: a start brings plugins up (SPI base scans, fieldbus probes, + * certificate generation) and a stop joins task threads. The bound is here to catch + * a transition that will never finish, not to police a slow one. */ +#define PLC_TRANSITION_LANDING_TIMEOUT_MS 90000 +#define PLC_TRANSITION_STUCK_TIMEOUT_MS (PLC_TRANSITION_LANDING_TIMEOUT_MS + 30000) + /** * @brief Cleanup the PLC state manager and unloads the plugin manager. * @return void diff --git a/core/src/plc_app/plc_switch.c b/core/src/plc_app/plc_switch.c new file mode 100644 index 00000000..c744efc0 --- /dev/null +++ b/core/src/plc_app/plc_switch.c @@ -0,0 +1,74 @@ +/** + * @file plc_switch.c + * @brief Storage for the run/stop mode-switch position. + * + * Deliberately tiny: one atomic and three accessors. The runtime never reads + * hardware and never polls -- a VPP plugin pushes the position whenever it + * changes, on whatever schedule that package decides (GPIO interrupt, sysfs + * poll, fieldbus callback, its own thread). The runtime's only use for the + * value is to refuse a start while the switch reads STOP, and to report the + * position to the editor. + * + * The default is RUN so that a runtime with no switch-aware plugin behaves + * exactly as it did before this file existed: the boot auto-start is + * unguarded and every START succeeds. + */ + +#include "plc_switch.h" + +#include + +#include "utils/log.h" + +/* Default RUN: no plugin implementing the interface means no gating. */ +static atomic_int switch_position = PLC_SWITCH_RUN; + +/** + * Set when the switch moves, cleared when someone acts on it. + * + * Exists because state-change requests are dropped while a transition is in + * flight: a flip during a start or stop is refused, and without a record of it + * the switch and the PLC end up disagreeing with nobody retrying. Only the fact + * of movement is kept, never a queue of requests -- `switch_position` above + * already holds where the switch came to rest, which is the only position that + * matters once the dust settles. + * + * Deliberately platform-agnostic: every VPP that owns a switch reports through + * plc_set_switch_position(), so no plugin needs to know reconciliation exists. + */ +static atomic_bool switch_moved = false; + +void plc_set_switch_position(plc_switch_t position) +{ + const int normalized = (position == PLC_SWITCH_STOP) ? PLC_SWITCH_STOP : PLC_SWITCH_RUN; + const int previous = atomic_exchange(&switch_position, normalized); + + /* Log and record edges only. A plugin sampling a GPIO on a fast timer may + * call this on every sample; logging each one would flood the journal, and + * treating each one as movement would make the runtime reconcile forever. */ + if (previous != normalized) + { + atomic_store(&switch_moved, true); + log_info("Mode switch moved to %s", normalized == PLC_SWITCH_RUN ? "RUN" : "STOP"); + } +} + +bool plc_switch_take_movement(void) +{ + return atomic_exchange(&switch_moved, false); +} + +void plc_switch_note_movement(void) +{ + atomic_store(&switch_moved, true); +} + +plc_switch_t plc_get_switch_position(void) +{ + return (plc_switch_t)atomic_load(&switch_position); +} + +bool plc_switch_allows_run(void) +{ + return atomic_load(&switch_position) == PLC_SWITCH_RUN; +} diff --git a/core/src/plc_app/plc_switch.h b/core/src/plc_app/plc_switch.h new file mode 100644 index 00000000..5dcb967b --- /dev/null +++ b/core/src/plc_app/plc_switch.h @@ -0,0 +1,94 @@ +#ifndef PLC_SWITCH_H +#define PLC_SWITCH_H + +#include + +#ifdef __cplusplus +extern "C" { +#endif + +/** + * @brief Physical run/stop mode-switch position. + * + * Every platform has a mode switch. Devices with no physical switch never call + * plc_set_switch_position(), so the stored value stays at its RUN default and + * every gate below is transparent -- identical to the behaviour before this + * interface existed. + */ +typedef enum +{ + PLC_SWITCH_STOP = 0, + PLC_SWITCH_RUN = 1 +} plc_switch_t; + +/** + * @brief Store the mode-switch position. + * + * Stores only -- it starts nothing and stops nothing. A VPP plugin that owns a + * physical switch calls this on each change and then asks for the matching + * transition through request_plc_start / request_plc_stop, so the transition + * flow stays the same one the editor's START / STOP commands drive. + * + * The ordering matters and is part of the plugin contract: store the position + * BEFORE requesting the transition. On a falling edge that closes the window + * where a start could slip in; on a rising edge it stops the request being + * refused by the guard still reading the stale STOP. + * + * A plain atomic store. Safe to call from any thread, including while the PLC + * is stopped (the runtime keeps every plugin mapped across a stop). + * + * @param position PLC_SWITCH_STOP or PLC_SWITCH_RUN + */ +void plc_set_switch_position(plc_switch_t position); + +/** + * @brief Read the stored mode-switch position. Serves the SWITCH socket + * command and the REST status field. + */ +plc_switch_t plc_get_switch_position(void); + +/** + * @brief Whether a start is permitted right now. + * + * Consulted by every start path: the socket START handler, the boot auto-start, + * and the plugin-facing request_plc_start. False while the switch reads STOP -- + * hardware is authoritative, and a start is refused rather than queued so the + * editor can tell the user to flip the switch. + */ +bool plc_switch_allows_run(void); + +/** + * @brief Consume the "switch has moved" record. True if it moved since the last call. + * + * State-change requests are dropped while a transition is in flight, so a flip + * during a start or stop is refused outright. This is how that intent survives: + * the runtime notes only that the switch moved, and the transition-completion + * path compares where it came to rest against the state actually reached, + * correcting a mismatch. Consuming the record is what stops it correcting the + * same movement twice. + * + * Movement-gated on purpose. An editor stop with the switch untouched records no + * movement, so nothing reverses it; comparing position against state + * unconditionally would make Stop impossible whenever the switch sits in RUN. + */ +bool plc_switch_take_movement(void); + +/** + * @brief Re-arm the "switch has moved" record after a correction could not start. + * + * The counterpart to plc_switch_take_movement(): consuming the record commits the + * caller to acting on it, and this hands it back when that action was refused -- + * another transition claimed in the gap, or a spawn that failed. Without it the + * switch's intent is lost silently and the PLC stays in the state the switch + * disagrees with, which is the exact failure the record exists to prevent. + * + * Not for plugins: a plugin reporting a position calls plc_set_switch_position(), + * which records movement on an edge by itself. + */ +void plc_switch_note_movement(void); + +#ifdef __cplusplus +} +#endif + +#endif // PLC_SWITCH_H diff --git a/core/src/plc_app/scan_cycle_manager.c b/core/src/plc_app/scan_cycle_manager.c index 60dcab43..aa6a441e 100644 --- a/core/src/plc_app/scan_cycle_manager.c +++ b/core/src/plc_app/scan_cycle_manager.c @@ -189,7 +189,7 @@ int format_timing_stats_response(char *buffer, size_t buffer_size) if (n < 0) return 0; offset += (size_t)n; - /* Hold plc_tasks_reader_lock for the whole iteration. is_transitioning + /* Hold plc_tasks_reader_lock for the whole iteration. the TRANSITIONING state * gates new commands but does not bracket an in-flight STATS call — * a plugin-initiated STOP can fire while we're mid-loop, the bootstrap * thread joins task threads and frees plc_tasks[], and we'd then read diff --git a/core/src/plc_app/unix_socket.c b/core/src/plc_app/unix_socket.c index 3bfd347d..0244c278 100644 --- a/core/src/plc_app/unix_socket.c +++ b/core/src/plc_app/unix_socket.c @@ -1,10 +1,10 @@ #include #include #include -#include #include #include #include +#include #include #include #include @@ -12,30 +12,34 @@ #include "../drivers/plugin_driver.h" #include "debug_handler.h" #include "plc_state_manager.h" +#include "plc_switch.h" #include "scan_cycle_manager.h" #include "unix_socket.h" #include "utils/log.h" #include "utils/utils.h" extern volatile sig_atomic_t keep_running; -extern PLCState plc_state; static plugin_driver_t *g_plugin_driver = NULL; +/* How long run_transition waits to observe the landing before reconciling with + * the mode switch, and how often it looks. The bound comes from + * plc_state_manager.h so that the watchdog's stuck-transition bound is derived + * from the same number and can only fire strictly later. */ +#define LANDING_WAIT_MS PLC_TRANSITION_LANDING_TIMEOUT_MS +#define LANDING_POLL_MS 20 + void unix_socket_set_plugin_driver(void *driver) { g_plugin_driver = (plugin_driver_t *)driver; } -// Flag to prevent overlapping state transitions (e.g. START while STOP is in progress). -// Set before spawning the transition thread, cleared when the transition completes. -static atomic_int is_transitioning = 0; - -static void *transition_worker(void *arg) +// Body of a claimed transition: perform it, wait for it to land, then reconcile +// with the mode switch. Normally runs on the detached worker spawned below, but +// is called directly when that worker cannot be spawned -- the transition has +// already been claimed by then, and it has to be completed by somebody. +static bool run_transition(PLCState target) { - PLCState target = *(PLCState *)arg; - free(arg); - bool result = plc_set_state(target); if (!result) { @@ -43,67 +47,145 @@ static void *transition_worker(void *arg) target == PLC_STATE_RUNNING ? "RUNNING" : "STOPPED"); } - atomic_store(&is_transitioning, 0); + // Wait for the landing before reconciling. + // + // plc_set_state(RUNNING) returns as soon as load_plc_program() has spawned + // the PLC thread; that thread publishes RUNNING later, once the workers exist + // (measured at ~4 s on an SLM-RP4, most of it plugin bring-up). Reconciling + // straight after plc_set_state() therefore ran while the state was still + // TRANSITIONING_TO_RUN and threw the switch movement away without acting on + // it -- losing precisely the flip-during-a-start this exists to catch. + // + // Polling rather than a condvar handshake: the state IS the interlock, so + // nothing else can begin a transition while we wait, and there is no lock + // held here. The bound only exists so a transition that never lands cannot + // strand this thread -- the watchdog is what turns that into a reported + // fault. + for (int waited_ms = 0; plc_state_is_transitioning() && waited_ms < LANDING_WAIT_MS; + waited_ms += LANDING_POLL_MS) + { + struct timespec poll = { .tv_sec = 0, .tv_nsec = LANDING_POLL_MS * 1000000L }; + nanosleep(&poll, NULL); + } + + // Reconcile with the mode switch. + // + // Requests are DROPPED while a transition is in flight, which on its own + // loses the switch's intent: flip to STOP during a start and the stop + // vanishes, leaving the PLC running with the switch in STOP and nobody + // retrying. Rather than queueing requests, the runtime remembers only + // whether the switch MOVED (plc_switch, so this works for every platform's + // plugin) and compares the position it came to rest at against the state we + // actually landed on. Several flips during one transition collapse to the + // final position, which is the only one that matters. + // + // Gated on movement, deliberately: an editor stop with the switch untouched + // records no movement, so nothing reconciles it away. Comparing position to + // state unconditionally would make Stop impossible whenever the switch sits + // in RUN. It also cannot ping-pong — each pass consumes the movement, and + // only the switch physically moving sets it again. + if (plc_switch_take_movement()) + { + const PLCState landed = plc_get_state(); + const PLCState wanted = plc_switch_allows_run() ? PLC_STATE_RUNNING : PLC_STATE_STOPPED; + + // Only reconcile from a clean landing. ERROR and EMPTY are not states to + // "correct" — restarting a faulted or programless PLC because a switch + // moved would fight the fault rather than report it. + if ((landed == PLC_STATE_RUNNING || landed == PLC_STATE_STOPPED) && landed != wanted) + { + log_warn("Mode switch came to rest in %s but the PLC landed on %s — correcting", + wanted == PLC_STATE_RUNNING ? "RUN" : "STOP", + landed == PLC_STATE_RUNNING ? "RUNNING" : "STOPPED"); + + // The movement record was consumed above, so a refusal here would + // throw the switch's intent away for good: the state is already + // final, which leaves the socket thread free to claim a transition in + // the gap, and the spawn paths below can fail too. Put the record + // back so the next landing reconciles instead. This cannot ping-pong + // -- the retry compares position against state again, and a landing + // that agrees with the switch just consumes the record. + if (!plc_begin_transition(wanted)) + { + plc_switch_note_movement(); + log_warn("Correction to %s did not go through — re-armed for the " + "next landing", + wanted == PLC_STATE_RUNNING ? "RUN" : "STOP"); + } + } + } + + return result; +} + +static void *transition_worker(void *arg) +{ + PLCState target = *(PLCState *)arg; + free(arg); + + run_transition(target); return NULL; } // Start a background thread that performs the (potentially slow) state -// transition. Returns true if the thread was spawned, false otherwise. +// transition. Returns false when the request was refused; otherwise the +// transition is under way (or, if the worker could not be spawned, has already +// been completed on this thread -- see below). // -// This is the single authoritative entry point for all state transitions: -// socket START/STOP commands, plugin-initiated stops, AND the boot auto-start -// in plc_main.c (which calls this instead of plc_set_state() directly so that -// all paths share the same guard). That last point closes the race where the -// socket listener accepts a START while the auto-start is still in flight, -// which previously could spawn two concurrent load_plc_program() calls. +// The single authoritative entry point for every state change: socket +// START/STOP, plugin-initiated requests from a mode switch, and the boot +// auto-start in plc_main.c. All of them come here rather than calling +// plc_set_state() directly, so one arbiter decides what may begin. // -// Two safety guards on the entry path: +// That arbiter is plc_claim_transition(), which under the state lock refuses a +// request while the state is TRANSITIONING_TO_RUN or TRANSITIONING_TO_STOP, and +// refuses a request for the state the runtime is already in. There is no second +// flag to keep in step with plc_state -- the state IS the interlock, which is +// what makes "you cannot change state while changing state" true by +// construction rather than by two variables agreeing. // -// 1. CAS on `is_transitioning` 0→1: collapses concurrent calls. A -// misbehaving plugin spinning on plugin_request_plc_stop, or the boot -// path racing with a socket START, would otherwise pile up detached -// pthread workers. The CAS means only the first call fires the worker; -// everything else is a cheap return. -// -// 2. Re-check current state AFTER the CAS wins: closes the -// check-then-call race where the caller sees RUNNING, calls in, -// and the state flips to STOPPED before we spawn the worker. We'd -// otherwise dispatch a no-op transition, leaving STATUS reporting -// TRANSITIONING for the worker's lifetime for nothing. +// Requests refused here are dropped, not queued. The switch's intent survives +// via the movement reconciliation in transition_worker above. bool plc_begin_transition(PLCState target) { - int expected = 0; - if (!atomic_compare_exchange_strong(&is_transitioning, &expected, 1)) + if (!plc_claim_transition(target)) { - // Another transition is already in flight. Don't pile on. - return false; - } - - if (plc_get_state() == target) - { - // State already at target — release the gate and bail. No - // worker needed; reporting STATUS:TRANSITIONING for a no-op - // would just confuse external pollers. - atomic_store(&is_transitioning, 0); return false; } + // Claimed but the worker cannot be spawned: run the transition on this thread + // rather than publishing a landing. + // + // Publishing STOPPED here used to look like the safe way out, and it is the + // opposite. The claim has already published TRANSITIONING_TO_STOP, which is + // what makes the dispatcher and workers leave their loops -- so on a stop from + // RUNNING the scan really does end, but unload_plc_program never runs: + // journal_cleanup, plugin_driver_stop, plugin_manager_destroy and the dlclose + // are all skipped, plc_program stays non-NULL, plc_thread is never joined, and + // STATUS reports a stop that tore nothing down. The next start then re-enters + // plugin_driver_init on live plugin state and re-runs a program whose statics + // were never reinitialised. (For a start from EMPTY it also reported "no + // program" as "stopped".) + // + // Completing it here blocks this caller for the duration -- the socket is + // single-client, so the editor waits -- which on a thread-or-memory exhaustion + // path is the cheaper of the two costs by a wide margin. PLCState *arg = malloc(sizeof(PLCState)); if (!arg) { - log_error("Failed to allocate transition argument"); - atomic_store(&is_transitioning, 0); - return false; + log_error("Failed to allocate transition argument — completing the " + "transition on the calling thread"); + return run_transition(target); } *arg = target; pthread_t tid; if (pthread_create(&tid, NULL, transition_worker, arg) != 0) { - log_error("Failed to create transition thread: %s", strerror(errno)); + log_error("Failed to create transition thread (%s) — completing the " + "transition on the calling thread", strerror(errno)); free(arg); - atomic_store(&is_transitioning, 0); - return false; + return run_transition(target); } pthread_detach(tid); return true; @@ -133,27 +215,16 @@ static ssize_t read_line(int fd, char *buffer, size_t max_length) static void format_status_response(char *response, size_t response_size) { - // While a transition is in progress, plc_state has already flipped - // to the target value (so the running task threads can exit their - // `while (plc_get_state() == RUNNING)` loops) but the actual - // load/unload work is still happening on the transition worker - // thread. Reporting the bare state here would tell external - // pollers STATUS:STOPPED while the runtime can't yet accept a - // START — they'd race ahead and get COMMAND:BUSY. - // - // Surfacing TRANSITIONING for the duration of the worker keeps - // _wait_for_plc_state(STOPPED) on the webserver side honest: - // STATUS:STOPPED is reported only after the worker has completed - // and is_transitioning has cleared. - if (atomic_load(&is_transitioning)) - { - strncpy(response, "STATUS:TRANSITIONING\n", response_size); - return; - } - PLCState current_state = plc_get_state(); - if (current_state == PLC_STATE_INIT) + // Both directions report as the one TRANSITIONING string that external + // callers already know. The distinction is internal (intent), and the + // webserver's _wait_for_plc_idle plus the editor both key off this wire + // value, so it stays exactly as it was. + if (current_state == PLC_STATE_TRANSITIONING_TO_RUN || + current_state == PLC_STATE_TRANSITIONING_TO_STOP) + strncpy(response, "STATUS:TRANSITIONING\n", response_size); + else if (current_state == PLC_STATE_INIT) strncpy(response, "STATUS:INIT\n", response_size); else if (current_state == PLC_STATE_RUNNING) strncpy(response, "STATUS:RUNNING\n", response_size); @@ -167,11 +238,29 @@ static void format_status_response(char *response, size_t response_size) strncpy(response, "STATUS:UNKNOWN\n", response_size); } +static void format_switch_response(char *response, size_t response_size) +{ + // Report the mode-switch position a VPP plugin last stored. Devices with no + // switch-aware plugin always answer RUN. + if (plc_get_switch_position() == PLC_SWITCH_RUN) + strncpy(response, "SWITCH:RUN\n", response_size); + else + strncpy(response, "SWITCH:STOP\n", response_size); +} + void handle_unix_socket_commands(const char *command, char *response, size_t response_size) { - // While a state transition is in progress, only allow PING and STATUS. - // Everything else gets COMMAND:BUSY so commands cannot overlap. - if (atomic_load(&is_transitioning)) + // While a state transition is in progress, only allow the reads: you cannot + // change state while it is changing, and everything else gets COMMAND:BUSY. + // + // SWITCH belongs here with PING and STATUS. It is a plain atomic load of + // plc_switch with no coupling to plc_state, so there is nothing mid-change for + // it to expose -- and answering BUSY meant the webserver dropped + // switchPosition from every status response for the whole duration of a start + // or stop (parse_switch_position returns None) and GET /switch reported + // "unknown". An editor that decides whether a start is allowed from that field + // lost it precisely while polling through the transition it had just asked for. + if (plc_state_is_transitioning()) { if (strcmp(command, "PING") == 0) { @@ -181,6 +270,10 @@ void handle_unix_socket_commands(const char *command, char *response, size_t res { format_status_response(response, response_size); } + else if (strcmp(command, "SWITCH") == 0) + { + format_switch_response(response, response_size); + } else { strncpy(response, "COMMAND:BUSY\n", response_size); @@ -212,10 +305,22 @@ void handle_unix_socket_commands(const char *command, char *response, size_t res strncpy(response, "STOP:ERROR\n", response_size); } } + else if (strcmp(command, "SWITCH") == 0) + { + format_switch_response(response, response_size); + } else if (strcmp(command, "START") == 0) { PLCState current_state = plc_get_state(); - if (current_state != PLC_STATE_RUNNING) + // Hardware is authoritative: refuse rather than queue, so the editor + // can tell the user to flip the switch instead of leaving a start + // pending. Checked before the transition is ever begun. + if (!plc_switch_allows_run()) + { + strncpy(response, "START:ERROR_SWITCH_STOP\n", response_size); + log_warn("Received START command but the mode switch is in STOP"); + } + else if (current_state != PLC_STATE_RUNNING) { if (plc_begin_transition(PLC_STATE_RUNNING)) strncpy(response, "START:OK\n", response_size); diff --git a/core/src/plc_app/unix_socket.h b/core/src/plc_app/unix_socket.h index 08c98f3a..2c8c0c10 100644 --- a/core/src/plc_app/unix_socket.h +++ b/core/src/plc_app/unix_socket.h @@ -20,7 +20,7 @@ void unix_socket_set_plugin_driver(void *driver); // Spawn a detached worker thread that transitions the PLC to `target`. // Shared with plugin_driver so a plugin's request_plc_stop callback goes // through the same transition-guarded path as an external STOP command -// (same overlap protection via the internal is_transitioning flag). +// (same overlap protection: plc_claim_transition refuses while TRANSITIONING). bool plc_begin_transition(PLCState target); #endif // UNIX_SOCKET_H diff --git a/core/src/plc_app/utils/watchdog.c b/core/src/plc_app/utils/watchdog.c index 3d91670e..11c43bf3 100644 --- a/core/src/plc_app/utils/watchdog.c +++ b/core/src/plc_app/utils/watchdog.c @@ -12,16 +12,56 @@ atomic_long plc_heartbeat; +/* Watchdog loop period. The stuck-transition bound is NOT defined here: it comes + * from plc_state_manager.h, where it is derived from the same constant the + * transition worker waits on, so this can never fire while the runtime still + * considers the transition to be progressing normally. */ +#define WATCHDOG_TICK_S 2 +#define TRANSITION_STUCK_S (PLC_TRANSITION_STUCK_TIMEOUT_MS / 1000) + void *watchdog_thread(void *arg) { (void)arg; long last = atomic_load(&plc_heartbeat); + int transitioning_ticks = 0; while (1) { - sleep(2); // Watch every 2 seconds + sleep(WATCHDOG_TICK_S); PLCState current_state = plc_get_state(); + + // A transition that never publishes a final state would leave the + // runtime in TRANSITIONING forever, refusing every command but PING and + // STATUS — the state is the interlock now, so there is no flag anyone + // could clear to recover. Every path is meant to land a final state; + // this is the backstop for the one that doesn't, turning a silent + // permanent wedge into a reported fault the webserver can act on. + // + // KNOWN LIMITATION: forcing ERROR releases the interlock but does not + // abort the transition, so the worker that failed to land is still + // running -- and a START accepted from ERROR would begin a second one + // over the top of it. Reaching this point at all now takes longer than + // the runtime's own landing bound (see PLC_TRANSITION_STUCK_TIMEOUT_MS), + // which removes the realistic trigger; closing it properly needs the + // transition owner to be able to abort its own work, which is the + // lifecycle-executor refactor and not this function's job. + if (current_state == PLC_STATE_TRANSITIONING_TO_RUN || + current_state == PLC_STATE_TRANSITIONING_TO_STOP) + { + transitioning_ticks++; + if (transitioning_ticks * WATCHDOG_TICK_S > TRANSITION_STUCK_S) + { + log_error("Watchdog: state change stuck in progress for over %d s — " + "forcing ERROR so the runtime accepts commands again", + TRANSITION_STUCK_S); + plc_force_error_state(); + transitioning_ticks = 0; + } + continue; + } + transitioning_ticks = 0; + if (current_state != PLC_STATE_RUNNING) { // Reset tracking when not running so we get a fresh diff --git a/scripts/Makefile.strucpp b/scripts/Makefile.strucpp index c2de33d5..cebd1ccb 100644 --- a/scripts/Makefile.strucpp +++ b/scripts/Makefile.strucpp @@ -56,7 +56,24 @@ CC := $(if $(CCACHE),$(CCACHE) gcc,gcc) # when the per-upload generated.hpp changes, leaving a stale locatedVarsCount # (a constexpr baked in at compile time) in the shim object -> ABI/count # mismatch against the freshly compiled program -> runtime crash. -CXXFLAGS := -std=c++17 -O1 -pipe -fPIC -Wall -DSTRUCPP_THREADED -MMD -MP \ +# -fno-gnu-unique: THIS FLAG IS WHAT MAKES STOP ACTUALLY STOP. The program .so is +# dlopen'd on start and dlclose'd on stop, but glibc refuses to unmap any DSO +# that defines STB_GNU_UNIQUE symbols -- and strucpp's headers emit them for +# template statics (strucpp::GLOBALVAR, GLOBALBOOL), the debug type_ops table, +# and statics inside inline functions. dlclose() therefore returned success, +# logged "PLC program unloaded successfully", and left the mapping resident with +# every static still holding the previous run's values. Because find_libplc_file +# hands dlopen the SAME path on the next start, glibc matched the still-loaded +# object by name and handed it straight back: the program resumed mid-state +# instead of restarting. A CTU that had counted to 100 stayed at 100 across +# stop/start, so the program looked alive in the debugger but did nothing. +# Suppressing the unique attribute costs nothing here -- these symbols are +# private to this one .so (plc_main exports only the ext_strucpp_* hook +# pointers, never the mangled statics), so there is no cross-DSO identity to +# preserve. Verified on an SLM-RP4: with the flag the .so leaves +# /proc//maps on every stop and re-maps fresh on every start; without it, +# it never leaves -- one build's image stays pinned for the life of the process. +CXXFLAGS := -std=c++17 -O1 -pipe -fPIC -Wall -DSTRUCPP_THREADED -fno-gnu-unique -MMD -MP \ -Wno-unknown-pragmas -Wno-deprecated-declarations \ -I $(GENERATED_DIR) -I $(RUNTIME_INC) -I $(PYTHON_INC) diff --git a/tests/lifecycle/README.md b/tests/lifecycle/README.md new file mode 100644 index 00000000..5bff45e9 --- /dev/null +++ b/tests/lifecycle/README.md @@ -0,0 +1,82 @@ +# Lifecycle tests + +End-to-end tests for run/stop state control: what happens when a signal, a +command, or a mode-switch flip lands in the middle of a state change. + +These are not unit tests. Each one boots a real `plc_main` with a real compiled +PLC program, drives it over its command socket, and judges it by what an outside +observer can see — socket replies, the journal, and `/proc//maps`. That +matters because the bugs in this area are not wrong return values; they are a +state that claims a transition finished while the work is still running. Only the +mapping tells you whether a reported stop actually unloaded anything. + +## Running + +Needs Linux (`SCHED_FIFO`, `sem_init`, `/proc`). On macOS, run it in the project's +container image: + +```bash +docker run --rm --cap-add=sys_nice \ + -v "$PWD":/src:ro \ + -v /path/to/editor/payload:/payload:ro \ + -v "$PWD/tests/lifecycle":/fixtures:ro \ + openplc-runtime-runstop:test bash /fixtures/harness.sh +``` + +`--cap-add=sys_nice` is required for the runtime's real-time scheduling. + +`/payload` is a directory of editor-generated Runtime v4 sources — the contents of +`/build/OpenPLC Runtime v4/src`. The harness copies it to +`core/generated` and compiles it with `scripts/compile.sh`, so any program works. +Payloads from editor builds before `defines.h` existed need one added next to the +sources, since `Makefile.strucpp` lists it as an explicit prerequisite: + +```c +#define PROGRAM_MD5 "" +``` + +Run one group at a time with `SUITE_ONLY=crash|shutdown|phantom|switch|misc`, and +get a backtrace out of a crash with `SUITE_GDB=1` (needs `gdb` in the image). + +## What the pieces are for + +| File | Why it exists | +|---|---| +| `suite.py` | The tests. One group per review finding, each with an oracle that fails on the unfixed code. | +| `fakevpp_plugin.c` | Stands in for a board's VPP package: a configurable sleep in `init()`/`stop_loop()` widens a transition enough to aim at, and a file-driven mode switch (`/tmp/fakevpp_switch`, "RUN"/"STOP") drives `set_switch_position` + `request_plc_start`/`request_plc_stop` the way real hardware does. | +| `failinject.c` | `LD_PRELOAD` shim that fails exactly one `pthread_create`, one-shot and self-disarming, so the "transition worker could not be spawned" path is reachable without inducing real thread exhaustion. | +| `logserver.py` | Accepts `/run/runtime/log_runtime.socket`. Without something listening there, `--print-logs` produces nothing on stdout either and every journal assertion silently passes. | +| `harness.sh` | Builds the runtime, the fixtures and the program, writes `plugins.conf`, then runs the suite. | + +## Two traps worth knowing before editing these + +**Keep the Python plugins in `plugins.conf`.** They stay disabled, but *loading* +them is what initialises the interpreter, and `has_python_plugin && +Py_IsInitialized()` is the precondition for the whole class of shutdown bugs +around `Py_FinalizeEx`. A conf containing only the native fixture makes that class +untestable while every test still passes. + +**Never start a thread in a plugin's `init()`.** The contract says so, and the +reason is concrete: `plugin_driver_update_config` rebuilds every slot on each +start, which `dlclose`s the `.so`, so a thread left running from `init()` returns +into an unmapped page. This fixture segfaulted that way before it was written to +use `start_loop`/`stop_loop`. + +## Coverage, and what is still argued rather than observed + +Covered: graceful shutdown with the PLC never started (both signals); shutdown +during a start; a stop whose transition worker cannot be spawned; a mode-switch +flip during a teardown; `SWITCH` answered mid-transition; and 25 rapid start/stop +pairs as a wedge guard. + +Not covered: + +- **The re-arm branch of switch reconciliation.** The tests prove a flip during a + transition is honoured, but the branch that hands the movement record back only + runs when the corrective transition is *refused* by a request that slipped in + first — a race that cannot be forced without a hook in the runtime. +- **The watchdog forcing ERROR on a stuck transition.** Needs a transition that + outlives `PLC_TRANSITION_STUCK_TIMEOUT_MS` (2 minutes), so it belongs in a slow + opt-in run rather than here. +- **A runaway IEC task.** Terminating one needs the forced-abort ladder that does + not exist yet; today a program with an unbounded loop wedges the stop. diff --git a/tests/lifecycle/failinject.c b/tests/lifecycle/failinject.c new file mode 100644 index 00000000..833e62af --- /dev/null +++ b/tests/lifecycle/failinject.c @@ -0,0 +1,48 @@ +/** + * @file failinject.c + * @brief LD_PRELOAD shim that fails exactly one pthread_create, on demand. + * + * Finding 4 lives on the "the transition worker could not be spawned" path, which + * needs thread or memory exhaustion to reach -- not something to induce for real + * inside a container without collateral damage. + * + * Contract: while the file named by FAILINJECT_ARM (default /tmp/failinject_arm) + * exists, the NEXT pthread_create returns EAGAIN, and the shim then unlinks the + * file itself. One-shot and self-disarming, so the failure lands on the call the + * test aimed at and every later thread -- including the ones the recovery path + * needs -- is created normally. + */ + +#define _GNU_SOURCE +#include +#include +#include +#include +#include + +typedef int (*pthread_create_fn)(pthread_t *, const pthread_attr_t *, + void *(*)(void *), void *); + +static const char *arm_path(void) +{ + const char *p = getenv("FAILINJECT_ARM"); + return (p && *p) ? p : "/tmp/failinject_arm"; +} + +int pthread_create(pthread_t *thread, const pthread_attr_t *attr, + void *(*start_routine)(void *), void *arg) +{ + static pthread_create_fn real = NULL; + if (!real) + real = (pthread_create_fn)dlsym(RTLD_NEXT, "pthread_create"); + + const char *p = arm_path(); + if (access(p, F_OK) == 0) + { + /* Disarm first: if unlink fails we must not fail every subsequent call. */ + if (unlink(p) == 0) + return EAGAIN; + } + + return real(thread, attr, start_routine, arg); +} diff --git a/tests/lifecycle/fakevpp_plugin.c b/tests/lifecycle/fakevpp_plugin.c new file mode 100644 index 00000000..953e6b47 --- /dev/null +++ b/tests/lifecycle/fakevpp_plugin.c @@ -0,0 +1,214 @@ +/** + * @file fakevpp_plugin.c + * @brief Test-only native plugin standing in for a board's VPP package. + * + * Exists to make three otherwise-unreachable conditions reproducible: + * + * 1. A SLOW START. init() sleeps FAKEVPP_INIT_MS (default 2500 ms), which is + * what a real board spends on SPI base scans and fieldbus probes. Without + * it, bring-up in a container lands in ~150 ms and there is no window to + * aim a signal or a competing request at. + * + * 2. A MODE SWITCH. A watcher thread polls FAKEVPP_SWITCH_FILE (default + * /tmp/fakevpp_switch) for the text "RUN" or "STOP" and drives the runtime + * exactly as the plugin contract prescribes: store the position first, then + * request the matching transition. Writing that file is the test's way of + * flipping a physical switch. + * + * 3. A RUNAWAY-FREE STOP. stop_loop()/cleanup() return promptly, so a slow + * teardown never confuses a slow start. + * + * Everything is env-driven so one .so covers every scenario, and nothing here + * touches the process image -- it declares no located variables. + */ + +#ifndef _GNU_SOURCE +#define _GNU_SOURCE +#endif + +#include +#include +#include +#include +#include +#include +#include + +/* Keep in step with plugin_types.h. Only the tail matters here, but the whole + * prefix has to be laid out identically for the offsets to line up, so this + * includes the real header rather than re-declaring the struct. */ +#include "plugin_types.h" + +#define SWITCH_RUN 1 +#define SWITCH_STOP 0 + +static plugin_runtime_args_t *g_args = NULL; +static atomic_int g_running = 0; +static pthread_t g_watcher; +static int g_watcher_up = 0; +static int g_last_pos = SWITCH_RUN; + +static void plog(const char *level, const char *msg) +{ + if (g_args && g_args->log_info && strcmp(level, "info") == 0) + g_args->log_info(msg); + else if (g_args && g_args->log_warn && strcmp(level, "warn") == 0) + g_args->log_warn(msg); + else + fprintf(stderr, "[FAKEVPP] %s: %s\n", level, msg); +} + +static long env_long(const char *name, long fallback) +{ + const char *v = getenv(name); + if (!v || !*v) + return fallback; + return atol(v); +} + +static const char *switch_file(void) +{ + const char *v = getenv("FAKEVPP_SWITCH_FILE"); + return (v && *v) ? v : "/tmp/fakevpp_switch"; +} + +/* Read the switch file. Absent or unparseable means RUN, matching the runtime's + * own default for a device with no switch-aware plugin. */ +static int read_switch_file(void) +{ + FILE *f = fopen(switch_file(), "r"); + if (!f) + return SWITCH_RUN; + char buf[16] = {0}; + if (!fgets(buf, sizeof(buf), f)) + { + fclose(f); + return SWITCH_RUN; + } + fclose(f); + return (strncmp(buf, "STOP", 4) == 0) ? SWITCH_STOP : SWITCH_RUN; +} + +static void *watcher_thread(void *arg) +{ + (void)arg; + const long poll_ms = env_long("FAKEVPP_POLL_MS", 5); + struct timespec t = { .tv_sec = poll_ms / 1000, .tv_nsec = (poll_ms % 1000) * 1000000L }; + + while (atomic_load(&g_running)) + { + const int pos = read_switch_file(); + if (pos != g_last_pos) + { + g_last_pos = pos; + /* Contract order: store the position BEFORE requesting, so the start + * gate never reads a stale STOP on a rising edge and no start can slip + * in on a falling one. */ + if (g_args && g_args->set_switch_position) + g_args->set_switch_position(pos); + + if (pos == SWITCH_RUN) + { + plog("info", "fake mode switch moved to RUN"); + if (g_args && g_args->request_plc_start) + g_args->request_plc_start("fake mode switch moved to RUN"); + } + else + { + plog("info", "fake mode switch moved to STOP"); + if (g_args && g_args->request_plc_stop) + g_args->request_plc_stop("fake mode switch moved to STOP"); + } + } + nanosleep(&t, NULL); + } + return NULL; +} + +int init(void *args) +{ + g_args = (plugin_runtime_args_t *)args; + + /* Adopt whatever the switch file already says before anything else, so a test + * can boot a device with the switch in STOP. */ + g_last_pos = read_switch_file(); + if (g_args && g_args->set_switch_position) + g_args->set_switch_position(g_last_pos); + + const long slow_ms = env_long("FAKEVPP_INIT_MS", 2500); + if (slow_ms > 0) + { + char msg[128]; + snprintf(msg, sizeof(msg), "fake VPP bring-up: sleeping %ld ms in init()", slow_ms); + plog("info", msg); + struct timespec t = { .tv_sec = slow_ms / 1000, .tv_nsec = (slow_ms % 1000) * 1000000L }; + nanosleep(&t, NULL); + } + + plog("info", "fake VPP initialised"); + return 0; +} + +/* The watcher lives between start_loop and stop_loop, and NOT a moment longer. + * + * init() is the tempting place for it -- a real mode switch has to be watched + * while the PLC is stopped too -- but the contract forbids threads there for a + * concrete reason: plugin_driver_update_config tears every slot down and + * re-dlopens it on each start, so a thread left running from init() ends up + * executing code that has been unmapped. That is a SIGSEGV, and this fixture + * earned one before being written this way. Everything the tests need still + * works, because the runtime stops plugins only AFTER joining the PLC thread: + * a flip during a stop is still seen. */ +int start_loop(void *args) +{ + (void)args; + if (atomic_exchange(&g_running, 1) == 1) + return 0; + if (pthread_create(&g_watcher, NULL, watcher_thread, NULL) != 0) + { + atomic_store(&g_running, 0); + plog("warn", "failed to start the switch watcher"); + return -1; + } + g_watcher_up = 1; + plog("info", "fake VPP switch watcher running"); + return 0; +} + +int stop_loop(void *args) +{ + (void)args; + + /* Optional slow teardown, BEFORE the watcher is joined, so the switch is + * still being watched while the stop is in flight. That is the only way to + * land a flip inside a stop transition on purpose: a stop is otherwise tens + * of milliseconds and there is nothing to aim at. */ + const long slow_ms = env_long("FAKEVPP_STOP_MS", 0); + if (slow_ms > 0) + { + char msg[128]; + snprintf(msg, sizeof(msg), "fake VPP teardown: sleeping %ld ms in stop_loop()", slow_ms); + plog("info", msg); + struct timespec t = { .tv_sec = slow_ms / 1000, .tv_nsec = (slow_ms % 1000) * 1000000L }; + nanosleep(&t, NULL); + } + + if (atomic_exchange(&g_running, 0) == 1 && g_watcher_up) + { + pthread_join(g_watcher, NULL); + g_watcher_up = 0; + } + return 0; +} + +int cleanup(void *args) +{ + (void)args; + if (atomic_exchange(&g_running, 0) == 1 && g_watcher_up) + { + pthread_join(g_watcher, NULL); + g_watcher_up = 0; + } + g_args = NULL; + return 0; +} diff --git a/tests/lifecycle/harness.sh b/tests/lifecycle/harness.sh new file mode 100644 index 00000000..22ca3e19 --- /dev/null +++ b/tests/lifecycle/harness.sh @@ -0,0 +1,57 @@ +#!/bin/bash +# Builds the runtime, compiles a real PLC program, builds the test fixtures, and +# runs the finding-by-finding suite. Single runtime instance at a time: the +# command socket is single-client, so no webserver competes for it. +set -uo pipefail + +echo "### build runtime" +mkdir -p /work +(cd /src && tar cf - --exclude=build --exclude=venvs --exclude=.git --exclude=core/generated .) | (cd /work && tar xf -) +cp -r /workdir/venvs /work/venvs +cd /work && mkdir -p build && cd build && cmake .. >/tmp/cmake.log 2>&1 && make -j"$(nproc)" >/tmp/make.log 2>&1 \ + || { echo "BUILD FAIL"; tail -25 /tmp/make.log; exit 1; } +cd /work +echo " warnings: $(grep -ci warning /tmp/make.log)" + +echo "### build fixtures" +mkdir -p build/plugins +gcc -shared -fPIC -O1 -Wall -o build/plugins/libfakevpp_plugin.so \ + /fixtures/fakevpp_plugin.c -I core/src/drivers -I "$(python3 -c 'import sysconfig;print(sysconfig.get_paths()["include"])')" \ + -lpthread 2>&1 | head -5 || { echo "FAKEVPP BUILD FAIL"; exit 1; } +gcc -shared -fPIC -O1 -Wall -o /tmp/failinject.so /fixtures/failinject.c -ldl 2>&1 | head -5 +echo '{"name":"fakevpp","protocol":"NONE","config":{}}' > /tmp/fakevpp_config.json +ls -la build/plugins/libfakevpp_plugin.so /tmp/failinject.so | sed 's/^/ /' + +echo "### compile a real PLC program (through the webserver's own compile path)" +mkdir -p /run/runtime core/generated && cp -r /payload/. core/generated/ +bash scripts/compile.sh >/tmp/compile.log 2>&1 +ls build/libplc_*.so >/dev/null 2>&1 || { + # compile.sh leaves the .so as new_libplc.so when invoked outside the webserver + if [ -f build/new_libplc.so ]; then + mv build/new_libplc.so "build/libplc_$(date +%s%N).so" + else + echo "COMPILE FAIL"; tail -12 /tmp/compile.log; exit 1 + fi +} +echo " program: $(ls build/libplc_*.so)" + +echo "### plugins.conf: the shipped set plus the fake VPP" +# The stock Python entries stay, disabled, because loading them is what +# initialises the interpreter -- and has_python_plugin && Py_IsInitialized() is +# the precondition for the Py_FinalizeEx() shutdown crash. A conf with only the +# native fixture in it quietly makes that whole class untestable. +# Native plugin lines whose .so is not built are dropped: they only add warnings. +grep -v 'libs7comm_plugin\|libethercat_plugin' plugins_default.conf > plugins.conf +# name,path,enabled,type,config_json,venv (type 1 = native) +printf 'fakevpp,./build/plugins/libfakevpp_plugin.so,1,1,/tmp/fakevpp_config.json,\n' >> plugins.conf +sed 's/^/ /' plugins.conf + +echo "### start the log-socket server (so --print-logs reaches stdout)" +python3 /fixtures/logserver.py & +LOGSRV=$! +sleep 0.4 + +/work/venvs/runtime/bin/python /fixtures/suite.py +RC=$? +kill $LOGSRV 2>/dev/null +exit $RC diff --git a/tests/lifecycle/logserver.py b/tests/lifecycle/logserver.py new file mode 100644 index 00000000..ad7c110f --- /dev/null +++ b/tests/lifecycle/logserver.py @@ -0,0 +1,39 @@ +"""Minimal stand-in for the webserver's log server. + +plc_main's log_init() connects to /run/runtime/log_runtime.socket; with nothing +listening, --print-logs produces nothing on stdout either, which is how a whole +round of testing ended up with empty journals. +""" + +import os +import socket +import threading + +PATH = "/run/runtime/log_runtime.socket" + +try: + os.unlink(PATH) +except OSError: + pass + +srv = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) +srv.bind(PATH) +srv.listen(16) + + +def drain(conn): + with conn: + while True: + try: + if not conn.recv(8192): + return + except OSError: + return + + +while True: + try: + c, _ = srv.accept() + except OSError: + break + threading.Thread(target=drain, args=(c,), daemon=True).start() diff --git a/tests/lifecycle/suite.py b/tests/lifecycle/suite.py new file mode 100644 index 00000000..aa17f0f6 --- /dev/null +++ b/tests/lifecycle/suite.py @@ -0,0 +1,435 @@ +"""PR #162 review findings: one test per finding, run against a real runtime. + +Each test drives plc_main directly over its command socket, with the fake VPP +plugin providing a slow bring-up (so a transition is wide enough to aim at) and a +file-driven mode switch. Oracles are chosen to be observable from outside the +process -- socket replies, the journal, and /proc//maps -- so the same suite +scores the pristine branch and the fixed one. +""" + +import os +import signal +import socket +import subprocess +import sys +import time + +SOCK = "/run/runtime/plc_runtime.socket" +SWITCH = "/tmp/fakevpp_switch" +ARM = "/tmp/failinject_arm" +results = [] + + +# ---------------------------------------------------------------- plumbing +def cmd(c, timeout=20): + s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + s.settimeout(timeout) + s.connect(SOCK) + s.sendall((c + "\n").encode()) + r = s.recv(4096).decode().strip() + s.close() + return r + + +def status(): + try: + return cmd("STATUS") + except OSError: + return "unreachable" + + +def wait_final(limit=120.0): + t0 = time.time() + while time.time() - t0 < limit: + st = status() + if "TRANSITIONING" not in st and st != "unreachable": + return st, time.time() - t0 + time.sleep(0.02) + return status(), time.time() - t0 + + +def wait_transitioning(limit=20.0): + t0 = time.time() + while time.time() - t0 < limit: + if "TRANSITIONING" in status(): + return True + time.sleep(0.005) + return False + + +def ensure_running(limit=25.0): + """Get the PLC to RUNNING, tolerating the boot-auto-start race. + + plc_main creates the command socket before it claims the boot start, so a + STATUS poll right after the socket appears legitimately answers STOPPED -- + which is not the same as "the start is not coming". + """ + t0 = time.time() + while time.time() - t0 < limit: + st = status() + if st == "STATUS:RUNNING": + return st + if "TRANSITIONING" in st: + wait_final() + continue + if st in ("STATUS:STOPPED", "STATUS:EMPTY", "STATUS:ERROR"): + # Either the boot start has not been claimed yet or it will not come; + # asking explicitly settles it either way. + r = cmd("START") + if "OK" in r: + wait_final() + else: + time.sleep(0.05) + continue + time.sleep(0.02) + return status() + + +def set_switch(pos): + with open(SWITCH, "w") as f: + f.write(pos) + + +class Runtime: + """One plc_main process, its log captured.""" + + def __init__(self, tag, init_ms=2500, preload=None, env=None): + self.tag = tag + for p in (SOCK,): + try: + os.unlink(p) + except OSError: + pass + self.logpath = f"/tmp/rt_{tag}.log" + self.log = open(self.logpath, "w") + e = dict(os.environ, FAKEVPP_INIT_MS=str(init_ms), FAKEVPP_SWITCH_FILE=SWITCH) + if preload: + e["LD_PRELOAD"] = preload + if env: + e.update(env) + argv = ["stdbuf", "-oL", "-eL", "./build/plc_main", "--print-logs"] + if os.environ.get("SUITE_GDB"): + argv = ["gdb", "-q", "-batch", + "-ex", "handle SIGINT nostop noprint pass", + "-ex", "handle SIGTERM nostop noprint pass", + "-ex", "handle SIGUSR1 nostop noprint pass", + "-ex", "run", "-ex", "bt 12", "-ex", "thread apply all bt 4", + "--args", "./build/plc_main", "--print-logs"] + self.p = subprocess.Popen( + argv, + stdout=self.log, + stderr=subprocess.STDOUT, + env=e, + ) + self.ready = False + for _ in range(600): + if self.p.poll() is not None: + break + try: + if cmd("PING", timeout=2) == "PING:OK": + self.ready = True + break + except OSError: + pass + time.sleep(0.01) + if not self.ready: + print(f" runtime {tag} never answered PING") + self.dump() + + def journal(self): + self.log.flush() + try: + return open(self.logpath).read() + except OSError: + return "" + + def dump(self, n=25): + print(f" exit code: {self.p.poll()} journal (socket noise removed):") + lines = [l for l in self.journal().strip().splitlines() + if "Unix socket client" not in l] + for line in lines[-n:]: + print(" ", line) + + def maps(self): + try: + return open(f"/proc/{self.p.pid}/maps").read() + except OSError: + return "" + + def program_mapped(self): + return "libplc_" in self.maps() + + def signal_and_wait(self, sig, limit=10.0): + self.p.send_signal(sig) + t0 = time.time() + while time.time() - t0 < limit: + rc = self.p.poll() + if rc is not None: + return rc, time.time() - t0 + time.sleep(0.02) + return None, time.time() - t0 + + def kill(self): + if self.p.poll() is None: + self.p.kill() + self.p.wait() + self.log.close() + + +def record(finding, name, ok, detail=""): + results.append((finding, name, ok)) + tail = f" -- {detail}" if detail else "" + print(f" [{'PASS' if ok else 'FAIL'}] ({finding}) {name}{tail}") + + +def banner(t): + print(f"\n=== {t}") + + +# ------------------------------------------------- findings 2 + 3: shutdown +def test_shutdown_midstart(sig, signame): + """SIGINT/SIGTERM while TRANSITIONING_TO_RUN must complete, not hang. + + Pre-fix: unload_plc_program's guard only matched RUNNING so it published + nothing, plc_cycle_thread then published RUNNING unconditionally, and the + join in the teardown never returned. + """ + banner(f"findings 2+3: {signame} during a start") + set_switch("RUN") + rt = Runtime(f"mid_{signame}", init_ms=2500) + caught = wait_transitioning() + st = status() + record("2/3", f"{signame}: caught the start in flight", caught, f"state={st}") + rc, took = rt.signal_and_wait(sig, limit=12.0) + j = rt.journal() + if rc is None: + record("2/3", f"{signame}: process exits", False, "HUNG, needed SIGKILL") + elif rc < 0: + record("2/3", f"{signame}: process exits", False, f"died on signal {-rc}") + else: + record("2/3", f"{signame}: process exits", True, f"rc={rc} in {took:.2f}s") + # Either the start was allowed to land and then torn down properly, or the + # cycle thread declined to publish RUNNING over the teardown. What must never + # happen is RUNNING published and no unload. + resurrected = ("PLC State: RUNNING" in j) and ("PLC program unloaded successfully" not in j) + record( + "3", + f"{signame}: RUNNING never left standing over a teardown", + not resurrected, + "declined to publish" if "not releasing the first scan" in j + else ("landed then unloaded" if "unloaded successfully" in j else "no RUNNING published"), + ) + record( + "2", + f"{signame}: shutdown waited for the transition", + "Shutdown waited" in j or "not releasing the first scan" in j, + "waited for the landing" if "Shutdown waited" in j else "cycle thread declined", + ) + rt.kill() + + +# ------------------------------------------------ finding 4: phantom stop +def test_phantom_stop(): + """A STOP whose worker cannot be spawned must not report a stop that never ran. + + The claim has already published TRANSITIONING_TO_STOP, which ends the scan; + pre-fix the failure path then published STOPPED while unload_plc_program never + ran, leaving the .so mapped and the program loaded. /proc//maps is the + oracle: STATUS says STOPPED either way, only the mapping tells the truth. + """ + banner("finding 4: STOP with the transition worker unspawnable") + set_switch("RUN") + rt = Runtime("phantom", init_ms=200, preload="/tmp/failinject.so") + st = ensure_running() + record("4", "reached RUNNING before the test", st == "STATUS:RUNNING", st) + if st != "STATUS:RUNNING": + rt.dump() + record("4", "program mapped while RUNNING", False, "skipped, runtime not running") + rt.kill() + return + record("4", "program mapped while RUNNING", rt.program_mapped()) + + open(ARM, "w").close() # next pthread_create fails, one-shot + reply = cmd("STOP") + st, secs = wait_final() + mapped = rt.program_mapped() + j = rt.journal() + record("4", "STOP still lands a final state", "TRANSITIONING" not in st, f"{reply} -> {st}") + record( + "4", + "the stop was real: program unmapped", + not mapped, + "still mapped -- STATUS lied about the stop" if mapped else f"unmapped, {st}", + ) + fired = ("completing the transition on the calling thread" in j + or "Failed to create transition thread" in j) + record("4", "the spawn failure was actually injected", fired, + "injection fired" if fired else "injection MISSED -- test inconclusive") + record( + "4", + "teardown actually ran", + "PLC program unloaded successfully" in j, + "journal shows the unload" if "unloaded successfully" in j else "no unload in journal", + ) + try: + os.unlink(ARM) + except OSError: + pass + rt.kill() + + +# ------------------------------- finding 5: switch intent must not be lost +def test_switch_intent(): + """A switch flip during a transition must not be lost. + + The sequence that matters, and the only one that records movement at all + (plc_set_switch_position only notes EDGES): + + 1. PLC RUNNING, switch RUN. + 2. Flip to STOP. The plugin stores STOP and requests a stop, which is + accepted -- TRANSITIONING_TO_STOP. + 3. Flip back to RUN while that stop is still tearing down. The plugin + stores RUN (an edge, so movement is recorded) and requests a start, + which is DROPPED because a transition is in flight. + 4. The stop lands on STOPPED. Only the movement record can now honour the + switch, and pre-fix that record was consumed before the corrective + transition was known to have started, with the return value discarded. + + Step 3 needs a stop wide enough to aim at, hence FAKEVPP_STOP_MS: the fake + plugin sleeps in stop_loop() while its switch watcher is still alive. + """ + banner("finding 5: switch flipped back during a stop") + set_switch("RUN") + rt = Runtime("switch", init_ms=300, env={"FAKEVPP_STOP_MS": "1500"}) + st = ensure_running() + record("5", "reached RUNNING before the test", st == "STATUS:RUNNING", st) + if st != "STATUS:RUNNING": + rt.dump() + rt.kill() + return + + disagreements = [] + corrections = 0 + for i in range(4): + set_switch("STOP") # step 2 + if not wait_transitioning(limit=10): + record("5", f"round {i}: stop transition started", False, "no transition seen") + break + time.sleep(0.2) # inside the slow teardown + set_switch("RUN") # step 3: the dropped request + st, _ = wait_final(limit=30) + # Reconciliation runs after the landing and starts its own transition. + time.sleep(0.3) + st, _ = wait_final(limit=30) + sw = cmd("SWITCH") + agree = (sw == "SWITCH:RUN" and st == "STATUS:RUNNING") or ( + sw == "SWITCH:STOP" and st == "STATUS:STOPPED" + ) + print(f" round {i}: switch={sw} state={st} {'agree' if agree else 'DISAGREE'}") + if not agree: + disagreements.append((i, sw, st)) + corrections = rt.journal().count("correcting") + if st != "STATUS:RUNNING": + ensure_running() + + j = rt.journal() + record( + "5", + "switch and PLC agree after every flip", + not disagreements, + f"{len(disagreements)} disagreement(s): {disagreements[:3]}" if disagreements + else "4/4 rounds reconciled", + ) + record( + "5", + "reconciliation is what did it", + corrections > 0, + f"{corrections} correction(s) logged", + ) + rt.kill() + + +# ---------------------------------- shutdown with the PLC never having run +def test_shutdown_never_ran(): + """The graceful-shutdown segfault: Py_FinalizeEx() with no GIL held. + + main_tstate was only ever set by plugin_driver_start(), so a runtime whose PLC + never ran had nothing to restore and finalised the interpreter without the + GIL. Reached by booting with the switch in STOP, which is also the safe-mode + and no-program shape. + """ + banner("graceful shutdown when the PLC never ran") + set_switch("STOP") + for sig, name in ((signal.SIGINT, "SIGINT"), (signal.SIGTERM, "SIGTERM")): + rt = Runtime(f"never_{name}", init_ms=100) + st, _ = wait_final(limit=30) + never_ran = "RUNNING" not in st + record("crash", f"{name}: PLC never ran", never_ran, st) + rc, took = rt.signal_and_wait(sig, limit=10.0) + if rc is None: + record("crash", f"{name}: clean shutdown", False, "HUNG") + elif rc < 0: + record("crash", f"{name}: clean shutdown", False, f"died on signal {-rc}") + else: + record("crash", f"{name}: clean shutdown", True, f"rc={rc} in {took:.2f}s") + rt.kill() + set_switch("RUN") + + +# ----------------------------------------- finding 6 + the interlock guard +def test_switch_readable_and_interlock(): + banner("finding 6 + interlock regression guard") + set_switch("RUN") + rt = Runtime("misc", init_ms=1500) + caught = wait_transitioning() + busy = answered = 0 + while "TRANSITIONING" in status(): + r = cmd("SWITCH") + if r.startswith("SWITCH:"): + answered += 1 + elif "BUSY" in r: + busy += 1 + time.sleep(0.01) + record("6", "SWITCH answered mid-transition", caught and answered > 0 and busy == 0, + f"{answered} answered, {busy} BUSY") + wait_final() + + tally = {} + for _ in range(25): + for c in ("START", "STOP"): + r = cmd(c) + tally[r] = tally.get(r, 0) + 1 + time.sleep(0.15) + st, secs = wait_final() + record("2/3", "no wedge after 25 rapid start/stop pairs", "TRANSITIONING" not in st, + f"settled on {st} in {secs:.2f}s") + record("2/3", "still answering after the stress", cmd("PING") == "PING:OK") + rt.kill() + + +ONLY = os.environ.get("SUITE_ONLY", "") + + +def run(name, fn, *a): + if not ONLY or ONLY == name: + fn(*a) + + +run("crash", test_shutdown_never_ran) +run("shutdown", test_shutdown_midstart, signal.SIGINT, "SIGINT") +run("shutdown", test_shutdown_midstart, signal.SIGTERM, "SIGTERM") +run("phantom", test_phantom_stop) +run("switch", test_switch_intent) +run("misc", test_switch_readable_and_interlock) + +print("\n=== summary") +by_finding = {} +for finding, name, ok in results: + d = by_finding.setdefault(finding, [0, 0]) + d[0 if ok else 1] += 1 +for finding in sorted(by_finding): + p, f = by_finding[finding] + print(f" finding {finding:<6} {p} passed, {f} failed") +failed = sum(1 for _, _, ok in results if not ok) +print(f" TOTAL: {len(results) - failed} passed, {failed} failed") +sys.exit(1 if failed else 0) diff --git a/webserver/app.py b/webserver/app.py index d6a1c0ac..a05505e5 100644 --- a/webserver/app.py +++ b/webserver/app.py @@ -130,6 +130,22 @@ def parse_timing_stats(stats_response: Optional[str]) -> Optional[dict]: return None +def parse_switch_position(switch_response: Optional[str]) -> Optional[str]: + """ + Parse the SWITCH response from the runtime. + Expected format: ``SWITCH:RUN`` / ``SWITCH:STOP``. + Returns ``"run"`` / ``"stop"``, or None when the response is unusable. + """ + if switch_response is None: + return None + value = switch_response.strip() + if value == "SWITCH:RUN": + return "run" + if value == "SWITCH:STOP": + return "stop" + return None + + def handle_status(data: dict) -> dict: response = runtime_manager.status_plc() if response is None: @@ -137,6 +153,14 @@ def handle_status(data: dict) -> dict: result: dict = {"status": response} + # Mode-switch position, so the editor can block a start before sending it + # rather than relying on the runtime's refusal alone. Additive: the existing + # `status` key is untouched, and an older editor simply ignores this field. + # A runtime with no switch-aware plugin always reports "run". + switch_position = parse_switch_position(runtime_manager.switch_plc()) + if switch_position is not None: + result["switchPosition"] = switch_position + # Only fetch timing stats if explicitly requested via include_stats parameter. # This avoids acquiring the stats mutex on every status poll, which could # introduce latency to the critical PLC scan cycle. @@ -186,6 +210,16 @@ def handle_list_serial_ports(data: dict) -> dict: return {"error": str(e), "ports": []} +def handle_switch(data: dict) -> dict: + """ + Report the run/stop mode-switch position on its own, for callers that want + it without a full status poll. Devices with no switch-aware VPP plugin + always answer "run". + """ + position = parse_switch_position(runtime_manager.switch_plc()) + return {"switchPosition": position if position is not None else "unknown"} + + GET_HANDLERS: dict[str, Callable[[dict], dict]] = { "start-plc": handle_start_plc, "stop-plc": handle_stop_plc, @@ -194,6 +228,7 @@ def handle_list_serial_ports(data: dict) -> dict: "status": handle_status, "ping": handle_ping, "serial-ports": handle_list_serial_ports, + "switch": handle_switch, } diff --git a/webserver/runtimemanager.py b/webserver/runtimemanager.py index fc734567..7a188c98 100644 --- a/webserver/runtimemanager.py +++ b/webserver/runtimemanager.py @@ -28,6 +28,12 @@ MAX_RAPID_CRASHES = 3 RAPID_CRASH_WINDOW = 30 # seconds +# How long to let the runtime shut down gracefully after SIGTERM before killing +# it. Has to exceed the worst-case graceful stop: the runtime waits for a state +# change already in flight to land (a boot start with plugin bring-up is ~4 s on +# an SLM-RP4) and then tears the program and plugins down. +RUNTIME_SHUTDOWN_TIMEOUT_S = 15 + class RuntimeManager: def __init__(self, runtime_path, plc_socket, log_socket, print_debug=False): @@ -244,17 +250,32 @@ def stop(self): self.monitor_thread.join(timeout=5) time.sleep(1) if self.process: + # SIGTERM now reaches a handler in the runtime, so terminate() starts a + # real shutdown: it stops the PLC program, waits out any state change + # already in flight (a boot start is ~4 s on an SLM-RP4), stops the + # plugins and unloads the program. Wait long enough for that to finish, + # or the SIGKILL below would preempt the very cleanup the signal asked + # for -- which is what happened for every stop while SIGTERM had no + # handler at all. The kill stays as the backstop for a hung teardown. if HAS_PSUTIL and isinstance(self.process, psutil.Process): self.process.terminate() try: - self.process.wait(timeout=5) + self.process.wait(timeout=RUNTIME_SHUTDOWN_TIMEOUT_S) except (psutil.TimeoutExpired, psutil.Error): + logger.warning( + "PLC runtime did not exit within %d s of SIGTERM; killing it", + RUNTIME_SHUTDOWN_TIMEOUT_S, + ) self.process.kill() elif isinstance(self.process, subprocess.Popen): self.process.terminate() try: - self.process.wait(timeout=5) + self.process.wait(timeout=RUNTIME_SHUTDOWN_TIMEOUT_S) except (subprocess.TimeoutExpired, subprocess.SubprocessError): + logger.warning( + "PLC runtime did not exit within %d s of SIGTERM; killing it", + RUNTIME_SHUTDOWN_TIMEOUT_S, + ) self.process.kill() self.process = None self._safe_stop_log_server() @@ -329,6 +350,22 @@ def status_plc(self): logger.error("Failed to get PLC status (unexpected): %s", e) return "STATUS:ERROR\n" + def switch_plc(self) -> str: + """ + Send SWITCH command to read the run/stop mode-switch position. + + Answers ``SWITCH:RUN`` or ``SWITCH:STOP``. Devices with no switch-aware + VPP plugin always report RUN, so callers need no special case for them. + """ + try: + return self.runtime_socket.send_and_receive("SWITCH\n") + except (OSError, socket.error) as e: + logger.error("Failed to get mode switch position: %s", e) + return "SWITCH:ERROR\n" + except Exception as e: + logger.error("Failed to get mode switch position (unexpected): %s", e) + return "SWITCH:ERROR\n" + def stats_plc(self): """ Send STATS command to get timing statistics