From 946aab1f32142a9f086bfdd30f20037faf2f34ab Mon Sep 17 00:00:00 2001 From: HugoFara Date: Wed, 12 Aug 2026 22:11:23 +0200 Subject: [PATCH] fix(core): stop quit[] from ending the host process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit quit is in the script command table, so it is reachable from the Python binding and from the HTTP server, and it called exit(0). A host got no exception and no traceback, its finally blocks and atexit handlers never ran, buffered output was lost, and the status was 0 — so a batch job recorded success having stopped early. quit now releases the session and records the request; quitRequested() exposes it and clearQuitRequest() forgets it. The shell in app/forefire is what leaves: both the interactive loop and the HTTP listen loop check it, so a served quit[] still stops the server. executeLoop stops reading a script after one, rather than the process disappearing mid-file. currentSession.params is no longer deleted. It is the SimulationParameters singleton, owned by GetInstance() and shared with every other holder, so deleting it left GetInstance() handing out a dangling pointer. That went unnoticed only because exit() followed on the next line; without the exit it would be a use-after-free on the next parameter read. The safe topology error path called quit() too, so an internal failure terminated the host on its own, with status 0. It reports and returns error instead. The message moves to stderr and is no longer gated on commandOutputs: a returned error nobody is obliged to check should not also be silent. tests/unit/test_quit_command.cpp covers it, and the CTest entries now require doctest's closing 'Status: SUCCESS!' line. Without that a suite whose process exits mid-run reports as a pass, because ctest sees 0 and does not look for the summary that never printed — the first version of this test passed happily with exit(0) still in place. Verified with the reproduction from #160: before quit[] / AFTER quit[] / FINALLY ran / ATEXIT ran, exit 0 against the previous output, which was 'before quit[]' and a dead interpreter. Unit suite 5/5; runff KML and NetCDF match. Closes #160 --- app/forefire/ForeFire.cpp | 14 ++++-- src/Command.cpp | 52 +++++++++++++++++----- src/Command.h | 21 ++++++++- tests/unit/CMakeLists.txt | 12 ++++- tests/unit/test_quit_command.cpp | 75 ++++++++++++++++++++++++++++++++ 5 files changed, 157 insertions(+), 17 deletions(-) create mode 100644 tests/unit/test_quit_command.cpp diff --git a/app/forefire/ForeFire.cpp b/app/forefire/ForeFire.cpp index d9c72bf..5a36a03 100644 --- a/app/forefire/ForeFire.cpp +++ b/app/forefire/ForeFire.cpp @@ -85,12 +85,14 @@ int ForeFire::startShell(int argc, char* argv[]) { string listenCommand = "listenHTTP[]"; executor.ExecuteCommand(listenCommand); - // Keep the main thread alive indefinitely + // Keep the main thread alive while the server runs. `quit[]` used to + // end the process from inside the library; now it asks, and this is + // the loop that answers, so a served quit[] still stops the server. cout << "(Press Ctrl+C to exit)" << endl; - while (true) { - sleep(3600); + while (!Command::quitRequested()) { + sleep(1); } - + return 0; } else { @@ -160,6 +162,10 @@ void ForeFire::FFShell(ifstream* inputStream) { if (line.empty()) continue; executor.ExecuteCommand(line); + // `quit[]` no longer ends the process from inside the library, so + // the shell is what leaves on request. + if (Command::quitRequested()) + break; } } } diff --git a/src/Command.cpp b/src/Command.cpp index d250916..49d1092 100644 --- a/src/Command.cpp +++ b/src/Command.cpp @@ -29,6 +29,7 @@ namespace libforefire double Command::endTime = 0; bool Command::firstCommand = true; + bool Command::quitAsked = false; size_t Command::refTabs = 0; FFPoint *Command::lastReadLoc = 0; @@ -1116,13 +1117,14 @@ namespace libforefire } catch (...) { - if (getDomain()->commandOutputs) - { - cout << getDomain()->getDomainID() << ": " - << "**** ERROR IN SAFE TOPOLOGY MODE, QUITING ****" << endl; - } - // TODO supersafe mode ? - quit(arg, numTabs); + // Reports an error rather than ending the process. This + // used to call quit(), so an internal failure here took + // the host down with exit status 0 — a coupled run or a + // batch job recorded success while having stopped early. + // The caller now sees the failure and chooses. + cerr << getDomain()->getDomainID() << ": " + << "**** ERROR IN SAFE TOPOLOGY MODE ****" << endl; + return error; } } } @@ -2423,6 +2425,11 @@ namespace libforefire while (std::getline(*inputStream, line)) { + // A `quit[]` earlier in the script stops the file here rather than + // ending the process, so the caller still gets to return normally. + if (quitAsked) + break; + if (!inTripleQuote) { /* Skip comments and empty lines when *not* inside a multiline literal */ @@ -2959,15 +2966,40 @@ namespace libforefire int Command::quit(const string &arg, size_t &numTabs) { - + // No exit() here. `quit` is in the command table, so it is reachable + // from the Python binding and from the HTTP server, and a library + // that ends its host's process skips every destructor, `finally` and + // atexit handler on the way out — and used to do it with status 0, so + // a batch run reported success having done nothing. The session is + // released and the request is recorded; whoever is driving the shell + // decides what to do about it. delete currentSession.fd; + currentSession.fd = 0; delete currentSession.outStrRep; + currentSession.outStrRep = 0; delete currentSession.sim; - delete currentSession.params; - exit(0); + currentSession.sim = 0; + + // currentSession.params is deliberately not deleted: it is the + // SimulationParameters singleton, owned by GetInstance() and shared + // with every other holder. Deleting it left GetInstance() handing out + // a dangling pointer, which only went unnoticed because exit() + // followed on the next line. + + quitAsked = true; return normal; } + bool Command::quitRequested() + { + return quitAsked; + } + + void Command::clearQuitRequest() + { + quitAsked = false; + } + void Command::setOstringstream(ostringstream *oss) { currentSession.outStream = oss; diff --git a/src/Command.h b/src/Command.h index bd0f8b3..3c9af95 100644 --- a/src/Command.h +++ b/src/Command.h @@ -121,6 +121,8 @@ class Command { }; static bool firstCommand; + /*! \brief set by quit(), read by quitRequested() */ + static bool quitAsked; static size_t refTabs; static FFPoint* lastReadLoc; @@ -176,7 +178,14 @@ class Command { static int systemExec(const string&, size_t&); /*! \brief command to run a system trough pipe */ static int clear(const string&, size_t&); - /*! \brief command to quit the ForeFire shell */ + /*! \brief releases the session and asks the host to stop + * + * Does NOT terminate the process. This is library code, and the host + * process is not ours to end: a `quit[]` arriving over the Python binding + * or the HTTP server used to take the interpreter down with it, skipping + * every destructor and `finally`, and exiting 0 so a batch job reported + * success. Callers driving a shell should check quitRequested() and stop. + */ static int quit(const string&, size_t&); /*! \brief command to quit the ForeFire shell */ static int listenHTTP(const string&, size_t &) ; @@ -232,6 +241,16 @@ class Command { public: + /*! \brief whether a `quit[]` has asked the host to stop + * + * Set by quit() and cleared by clearQuitRequest(). A shell reading + * commands should stop its loop when this is true; a library embedder is + * free to ignore it and carry on, which is the difference between asking + * and terminating. */ + static bool quitRequested(); + /*! \brief forgets a pending quit request */ + static void clearQuitRequest(); + // Reference time static double refTime; diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 18ecabd..c734d50 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -12,7 +12,8 @@ add_executable(forefire_unit_tests test_flux_models.cpp test_http_server.cpp test_model_registry.cpp - test_propagation_models.cpp) + test_propagation_models.cpp + test_quit_command.cpp) target_link_libraries(forefire_unit_tests PRIVATE forefireL) target_include_directories(forefire_unit_tests PRIVATE @@ -25,8 +26,15 @@ endif() # doctest groups its cases into suites; registering one CTest entry per suite # keeps `ctest` output useful without needing doctest's CMake integration. -foreach(_suite "model registry" "propagation models" "flux models" "http server") +foreach(_suite "model registry" "propagation models" "flux models" "http server" + "quit command") string(REPLACE " " "_" _suite_id "${_suite}") add_test(NAME "unit.${_suite_id}" COMMAND forefire_unit_tests --test-suite=${_suite} --no-skipped-summary) + # Requiring doctest's closing line, not just a zero exit status. A test + # that ends the process mid-run — which is exactly what quit[] used to do, + # with exit(0) — otherwise reports as a pass, because ctest sees 0 and the + # summary that never printed is not something it looks for. + set_tests_properties("unit.${_suite_id}" PROPERTIES + PASS_REGULAR_EXPRESSION "\\[doctest\\] Status: SUCCESS!") endforeach() diff --git a/tests/unit/test_quit_command.cpp b/tests/unit/test_quit_command.cpp new file mode 100644 index 0000000..47216a3 --- /dev/null +++ b/tests/unit/test_quit_command.cpp @@ -0,0 +1,75 @@ +/** + * @file test_quit_command.cpp + * @brief `quit[]` must not end the process it is running inside. + * @copyright Copyright (C) 2025 ForeFire, Fire Team, SPE, CNRS/Universita di Corsica. + * @license This program is free software; See LICENSE file for details. (See LICENSE file). + * + * `quit` sits in the script command table, so it is reachable from the Python + * binding and from the HTTP server, and it used to call exit(0). A host got no + * traceback, no exception, no `finally`, no destructors, and a status of 0 — + * a batch job recorded success having stopped early. See #160. + * + * The test for that is the whole file running to completion: if `quit[]` still + * called exit(0), the cases below this one would not report, and ctest would + * see a suite that passed nothing rather than a suite that failed. The + * `--test-suite` entry in CMakeLists is what turns that into a visible result. + */ + +#include "doctest/doctest.h" + +#include "Command.h" + +#include + +using libforefire::Command; + +TEST_SUITE("quit command") { + +TEST_CASE("quit[] returns instead of ending the process") { + Command executor; + Command::clearQuitRequest(); + + std::string command = "quit[]"; + executor.ExecuteCommand(command); + + // Reaching this line at all is the assertion that matters. + CHECK(Command::quitRequested()); +} + +TEST_CASE("the quit request can be cleared and the session reused") { + // A host that decides to ignore the request has to be able to carry on, + // which is the difference between asking and terminating. + Command executor; + Command::clearQuitRequest(); + REQUIRE_FALSE(Command::quitRequested()); + + std::string quitCommand = "quit[]"; + executor.ExecuteCommand(quitCommand); + REQUIRE(Command::quitRequested()); + + Command::clearQuitRequest(); + CHECK_FALSE(Command::quitRequested()); + + // The interpreter still works after a quit: setParameter and getParameter + // round-trip, which they could not do if quit had freed the parameters + // singleton as it used to. + std::string setCommand = "setParameter[ff_test.after.quit=42]"; + executor.ExecuteCommand(setCommand); + CHECK(libforefire::SimulationParameters::GetInstance() + ->getParameter("ff_test.after.quit") == "42"); +} + +TEST_CASE("a second quit is harmless") { + // quit() deletes the session objects and nulls them; running it twice must + // not double free what the first call released. + Command executor; + Command::clearQuitRequest(); + + std::string command = "quit[]"; + executor.ExecuteCommand(command); + executor.ExecuteCommand(command); + + CHECK(Command::quitRequested()); +} + +} /* TEST_SUITE */