From 72720533b131d5bd90a7cdd27dce41a7ccaf4b88 Mon Sep 17 00:00:00 2001 From: Ari Sulistiono Date: Wed, 4 Nov 2026 01:46:57 +0700 Subject: [PATCH] fix(firmware): retain exclusive COM ownership until timed-out flasher exits Defer launch and operation timeout results until QProcess confirms termination, including FailedToStart races, rather than treating asynchronous kill() as an exit acknowledgement. Clear worker generation before publishing one terminal result. Exercise a real hung subprocess and clean worker reuse in the native S8 harness. --- .../src/DeterministicSessionHarness.cpp | 84 +++++++++++++++++++ apps/arstack_studio/src/FirmwareWorker.cpp | 52 +++++++----- apps/arstack_studio/src/FirmwareWorker.hpp | 4 + 3 files changed, 121 insertions(+), 19 deletions(-) diff --git a/apps/arstack_studio/src/DeterministicSessionHarness.cpp b/apps/arstack_studio/src/DeterministicSessionHarness.cpp index d9d956417..f039e96b1 100644 --- a/apps/arstack_studio/src/DeterministicSessionHarness.cpp +++ b/apps/arstack_studio/src/DeterministicSessionHarness.cpp @@ -4,13 +4,19 @@ #include "DeviceIoWorker.hpp" #include "FirmwareManager.hpp" +#include "FirmwareWorker.hpp" #include "SclProfileModel.hpp" #include "SmartSessionController.hpp" #include "StudioDeviceController.hpp" #include #include +#include +#include +#include #include +#include +#include #ifndef ARSTACK_STUDIO_VERSION #define ARSTACK_STUDIO_VERSION "0.1.0" @@ -20,6 +26,14 @@ class DeterministicSessionHarness final { public: static int run(int argc, char* argv[]) { QCoreApplication app(argc, argv); + // A real OS child ensures timeout/cancel tests exercise QProcess + // termination rather than a synthetic boolean-only worker model. + if (app.arguments().contains(QStringLiteral("--firmware-worker-hang-child"))) { + QThread::msleep(8000); + return 0; + } + if (app.arguments().contains(QStringLiteral("--firmware-worker-fast-child"))) + return 0; struct CaseResult { const char* name; @@ -29,6 +43,7 @@ class DeterministicSessionHarness final { const CaseResult cases[] = { {"dual-revision catalog selects only qualified silicon", revisionCatalogFailClosed()}, {"firmware timeout retains port until flasher exits", firmwarePortReleaseWaitsForWorker()}, + {"real timed-out firmware subprocess retires before COM release", firmwareWorkerTimeoutAwaitsProcessExit()}, {"current identity -> READY without recovery", currentIdentityNeverRecovers()}, {"legacy identity -> firmware update", legacyIdentityOffersUpdate()}, {"same version missing build ID -> READY with update suggestion", sameVersionMissingBuildOffersUpdate()}, @@ -130,6 +145,75 @@ class DeterministicSessionHarness final { } private: + static bool firmwareWorkerTimeoutAwaitsProcessExit() { + FirmwareWorker worker; + worker.initialize(); + if (worker.process_ == nullptr) return false; + + QEventLoop loop; + QTimer deadline; + deadline.setSingleShot(true); + QObject::connect(&deadline, &QTimer::timeout, &loop, &QEventLoop::quit); + + bool started = false; + bool timedOutAfterExit = false; + bool restartedCleanly = false; + bool unexpectedTerminal = false; + int terminalCount = 0; + QObject::connect(&worker, &FirmwareWorker::operationStarted, &loop, + [&](const quint64 generation) { + if (generation == 41) started = true; + }); + QObject::connect(&worker, &FirmwareWorker::operationTimedOut, &loop, + [&](const quint64 generation) { + ++terminalCount; + timedOutAfterExit = generation == 41 && started && + worker.process_->state() == QProcess::NotRunning && + !worker.operationActive_ && worker.generation_ == 0; + loop.quit(); + }); + QObject::connect(&worker, &FirmwareWorker::operationFinished, &loop, + [&](const quint64 generation, const int code, const bool normal) { + ++terminalCount; + if (generation == 42) { + restartedCleanly = code == 0 && normal && + worker.process_->state() == QProcess::NotRunning && + !worker.operationActive_; + } else { + unexpectedTerminal = true; + } + loop.quit(); + }); + QObject::connect(&worker, &FirmwareWorker::operationLaunchFailed, &loop, + [&](quint64, const QString&) { unexpectedTerminal = true; loop.quit(); }); + QObject::connect(&worker, &FirmwareWorker::operationLaunchTimedOut, &loop, + [&](quint64) { unexpectedTerminal = true; loop.quit(); }); + QObject::connect(&worker, &FirmwareWorker::operationCancelled, &loop, + [&](quint64) { unexpectedTerminal = true; loop.quit(); }); + + const QString executable = QCoreApplication::applicationFilePath(); + const QProcessEnvironment env = QProcessEnvironment::systemEnvironment(); + worker.startOperation(41, executable, {QStringLiteral("--firmware-worker-hang-child")}, + env, 3500, 150); + deadline.start(5500); + loop.exec(); + deadline.stop(); + if (!timedOutAfterExit || unexpectedTerminal || terminalCount != 1) { + worker.shutdown(); + return false; + } + + // The worker must accept a second operation only after the first + // process has exited and its generation has been fully retired. + worker.startOperation(42, executable, {QStringLiteral("--firmware-worker-fast-child")}, + env, 3500, 3500); + deadline.start(5500); + loop.exec(); + deadline.stop(); + worker.shutdown(); + return restartedCleanly && !unexpectedTerminal && terminalCount == 2; + } + static bool revisionCatalogFailClosed() { FirmwareManager firmware; firmware.revisionCatalogAvailable_ = true; diff --git a/apps/arstack_studio/src/FirmwareWorker.cpp b/apps/arstack_studio/src/FirmwareWorker.cpp index c440110fd..e81581dfb 100644 --- a/apps/arstack_studio/src/FirmwareWorker.cpp +++ b/apps/arstack_studio/src/FirmwareWorker.cpp @@ -27,28 +27,27 @@ void FirmwareWorker::initialize() { connect(launchTimer_, &QTimer::timeout, this, [this] { if (!operationActive_ || process_ == nullptr || process_->state() != QProcess::Starting) return; - const quint64 generation = generation_; - operationActive_ = false; - cancelRequested_ = false; + // QProcess::kill() is asynchronous. Retain the operation/COM lease + // until QProcess has confirmed NotRunning via finished/errorOccurred. + killCause_ = KillCause::launchTimeout; if (operationTimer_ != nullptr) operationTimer_->stop(); process_->kill(); - emit operationLaunchTimedOut(generation); - clearOperation(); + if (operationActive_ && process_->state() == QProcess::NotRunning) + finishProcess(-1, false); }); connect(operationTimer_, &QTimer::timeout, this, [this] { if (!operationActive_ || process_ == nullptr) return; - const quint64 generation = generation_; - operationActive_ = false; - cancelRequested_ = false; + killCause_ = KillCause::operationTimeout; if (launchTimer_ != nullptr) launchTimer_->stop(); process_->kill(); - emit operationTimedOut(generation); - clearOperation(); + if (operationActive_ && process_->state() == QProcess::NotRunning) + finishProcess(-1, false); }); connect(process_, &QProcess::started, this, [this] { if (!operationActive_ || shuttingDown_) return; + if (killCause_ != KillCause::none) { process_->kill(); return; } if (launchTimer_ != nullptr) launchTimer_->stop(); if (operationTimer_ != nullptr) operationTimer_->start(); emit operationStarted(generation_); @@ -67,6 +66,13 @@ void FirmwareWorker::initialize() { connect(process_, &QProcess::errorOccurred, this, [this](const QProcess::ProcessError error) { if (!operationActive_ || shuttingDown_ || process_ == nullptr || error != QProcess::FailedToStart) return; + // A timeout during Starting can finish with FailedToStart rather than + // finished(). Treat it as termination acknowledgement, not a new + // operation result while the timed-out process still owns COM. + if (killCause_ != KillCause::none) { + if (process_->state() == QProcess::NotRunning) finishProcess(-1, false); + return; + } const quint64 generation = generation_; const QString message = process_->errorString(); operationActive_ = false; @@ -108,6 +114,7 @@ void FirmwareWorker::startOperation( generation_ = generation; operationActive_ = true; cancelRequested_ = false; + killCause_ = KillCause::none; process_->setProcessEnvironment(environment); process_->setProgram(program); process_->setArguments(arguments); @@ -125,8 +132,8 @@ void FirmwareWorker::cancel(const quint64 generation) { const quint64 cancelledGeneration = generation_; operationActive_ = false; cancelRequested_ = false; - emit operationCancelled(cancelledGeneration); clearOperation(); + emit operationCancelled(cancelledGeneration); return; } process_->kill(); @@ -149,19 +156,26 @@ void FirmwareWorker::finishProcess(const int exitCode, const bool normalExit) { } operationActive_ = false; - if (cancelRequested_) { - cancelRequested_ = false; + const KillCause retiredCause = killCause_; + const bool wasCancelled = cancelRequested_; + cancelRequested_ = false; + clearOperation(); + // Timeout is published only after the OS process has *actually* exited. + // The facade may now drop busy/COM ownership without racing a live flasher. + if (wasCancelled) { emit operationCancelled(completedGeneration); - clearOperation(); - return; + } else if (retiredCause == KillCause::launchTimeout) { + emit operationLaunchTimedOut(completedGeneration); + } else if (retiredCause == KillCause::operationTimeout) { + emit operationTimedOut(completedGeneration); + } else { + emit operationFinished(completedGeneration, exitCode, normalExit); } - - emit operationFinished(completedGeneration, exitCode, normalExit); - clearOperation(); } void FirmwareWorker::clearOperation() { generation_ = 0; + killCause_ = KillCause::none; } void FirmwareWorker::shutdown() { @@ -171,7 +185,7 @@ void FirmwareWorker::shutdown() { if (operationTimer_ != nullptr) operationTimer_->stop(); operationActive_ = false; cancelRequested_ = false; - generation_ = 0; + clearOperation(); if (process_ != nullptr && process_->state() != QProcess::NotRunning) { QObject::disconnect(process_, nullptr, this, nullptr); diff --git a/apps/arstack_studio/src/FirmwareWorker.hpp b/apps/arstack_studio/src/FirmwareWorker.hpp index a96cf0956..838f85f2a 100644 --- a/apps/arstack_studio/src/FirmwareWorker.hpp +++ b/apps/arstack_studio/src/FirmwareWorker.hpp @@ -44,6 +44,9 @@ public slots: void operationCancelled(quint64 generation); private: + friend class DeterministicSessionHarness; + + enum class KillCause { none, launchTimeout, operationTimeout }; void finishProcess(int exitCode, bool normalExit); void clearOperation(); @@ -55,4 +58,5 @@ public slots: bool shuttingDown_{false}; bool operationActive_{false}; bool cancelRequested_{false}; + KillCause killCause_{KillCause::none}; };