diff --git a/CHANGELOG.md b/CHANGELOG.md index 011c7132b..e17b0f812 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,19 @@ -## Release 1.3.1 +## Release 1.4.0 + +### Cancellation Fixes (behavior change) + +* Fix cancellation of a running update being ignored ([#777](https://github.com/Azure/iot-hub-device-update/issues/777)) — the cloud delivers a cancel with the internal workflow id `"nodeployment"` both during the pre-deployment delay period *and* when an operator cancels an update that is already running. The agent previously treated *every* `"nodeployment"` cancel as a no-op, so an in-progress deployment could never be cancelled and a step handler polling `workflow_is_cancel_requested()` never observed the request. The `"nodeployment"` cancel is now ignored only when no operation is in progress; while an operation is in progress it is processed as a real cancellation. +* Fix `CancelUpdate()` in script-handler scripts never being invoked ([#776](https://github.com/Azure/iot-hub-device-update/issues/776)) — on cancellation the steps handler now dispatches `Cancel()` to the step that is actively running, and the script handler now runs the script's `cancel` action (`--action-cancel`), which invokes the author's `CancelUpdate()` function. This allows a long-running, blocking `download`/`install` script to be interrupted (previously only an internal cancel flag was set, which a blocked script could never observe). + +#### Migration guidance for script-handler authors + +If you deploy updates that use the **script handler** (`microsoft/script:1`, including the SWUpdate sample scripts derived from `example-installscript.sh`), be aware that your script's `CancelUpdate()` function is now actually called when a deployment is cancelled. Previously it was dead code. Review your script and ensure that: + +* `CancelUpdate()` is implemented and returns a success result (`resultCode` `0`/`Cancel_Success`) when there is nothing to cancel. The sample `example-installscript.sh` already does this. +* `CancelUpdate()` is **idempotent** and safe to run concurrently with an in-progress `InstallUpdate()`/`DownloadUpdate()` — it runs on a separate thread while the in-progress action may still be executing. It should signal the in-progress action to stop (e.g. by writing a stop file or terminating a child process), then return. +* `CancelUpdate()` does **not** perform destructive work assuming the install completed; it may be invoked at any point during the operation. + +No changes are required if you do not author script-handler `CancelUpdate()` logic; the default sample template is already compatible. ### Other Bug Fixes diff --git a/src/adu_workflow/inc/aduc/agent_workflow.h b/src/adu_workflow/inc/aduc/agent_workflow.h index 5c329c6be..17cc14f0f 100644 --- a/src/adu_workflow/inc/aduc/agent_workflow.h +++ b/src/adu_workflow/inc/aduc/agent_workflow.h @@ -24,6 +24,14 @@ void ADUC_Workflow_HandlePropertyUpdate( void ADUC_Workflow_HandleUpdateAction(ADUC_WorkflowData* workflowData); +/** + * @brief Thread-safe check for whether an operation is currently in progress on the workflow. + * + * @param workflowData The workflow data. May be NULL. + * @return true if an operation is in progress; false otherwise. + */ +bool ADUC_Workflow_IsOperationInProgress(const ADUC_WorkflowData* workflowData); + void ADUC_Workflow_TransitionWorkflow(ADUC_WorkflowData* workflowData); void ADUC_Workflow_HandleStartupWorkflowData(ADUC_WorkflowData* currentWorkflowData); diff --git a/src/adu_workflow/src/agent_workflow.c b/src/adu_workflow/src/agent_workflow.c index 9acd9d441..524747b74 100644 --- a/src/adu_workflow/src/agent_workflow.c +++ b/src/adu_workflow/src/agent_workflow.c @@ -68,6 +68,33 @@ static inline void s_workflow_unlock(void) pthread_mutex_unlock(&s_workflow_mutex); } +/** + * @brief Thread-safe check for whether an operation is currently in progress on the workflow. + * + * Reads the workflow's OperationInProgress flag while holding the workflow lock so that callers + * outside of agent_workflow.c (e.g. the orchestrator property-update callback) do not race with + * the worker thread that mutates this flag. + * + * @param workflowData The workflow data. May be NULL. + * @return true if an operation is in progress; false otherwise (including when workflowData or its + * handle is NULL). + */ +bool ADUC_Workflow_IsOperationInProgress(const ADUC_WorkflowData* workflowData) +{ + bool inProgress = false; + + if (workflowData == NULL) + { + return false; + } + + s_workflow_lock(); + inProgress = workflow_get_operation_in_progress(workflowData->WorkflowHandle); + s_workflow_unlock(); + + return inProgress; +} + static const char* ADUC_Workflow_CancellationTypeToString(ADUC_WorkflowCancellationType cancellationType) { switch (cancellationType) diff --git a/src/adu_workflow/tests/agent_workflow_ut.cpp b/src/adu_workflow/tests/agent_workflow_ut.cpp index 454a91277..d5554bd94 100644 --- a/src/adu_workflow/tests/agent_workflow_ut.cpp +++ b/src/adu_workflow/tests/agent_workflow_ut.cpp @@ -1186,6 +1186,34 @@ TEST_CASE("ADUC_Workflow_MethodCall_Cancel") workflow_free(workflowData.WorkflowHandle); } +TEST_CASE("ADUC_Workflow_IsOperationInProgress") +{ + SECTION("Returns false for NULL workflow data") + { + CHECK_FALSE(ADUC_Workflow_IsOperationInProgress(nullptr)); + } + + SECTION("Reflects the workflow's operation-in-progress flag") + { + ADUC_WorkflowHandle handle = CreateTestWorkflowHandle(sample_process_deployment_json); + REQUIRE(handle != nullptr); + + ADUC_WorkflowData workflowData; + memset(&workflowData, 0, sizeof(workflowData)); + workflowData.WorkflowHandle = handle; + + CHECK_FALSE(ADUC_Workflow_IsOperationInProgress(&workflowData)); + + workflow_set_operation_in_progress(handle, true); + CHECK(ADUC_Workflow_IsOperationInProgress(&workflowData)); + + workflow_set_operation_in_progress(handle, false); + CHECK_FALSE(ADUC_Workflow_IsOperationInProgress(&workflowData)); + + workflow_free(workflowData.WorkflowHandle); + } +} + // // Unit Tests for ADUC_Workflow_SetInstalledUpdateIdAndGoToIdle // diff --git a/src/agent/adu_core_interface/src/adu_core_interface.c b/src/agent/adu_core_interface/src/adu_core_interface.c index 1ae0f1e49..25b6a3dc6 100644 --- a/src/agent/adu_core_interface/src/adu_core_interface.c +++ b/src/agent/adu_core_interface/src/adu_core_interface.c @@ -421,12 +421,25 @@ void OrchestratorUpdateCallback( // that a new deployment has been deployed recently and should be flowing // down the pipe soon. // - // Instead of processing a cancel, just ignore this and wait for normal - // cancellation to occur once a "process deployment" action flows down. + // However, the cloud sends the *same* "nodeployment" cancel when an operator + // cancels a deployment that is already running on the device. In that case we + // must process the cancel so the in-progress workflow is actually cancelled + // (otherwise the running step handler never observes the cancellation request). + // See issue #777. + // + // So: only ignore the "nodeployment" cancel when there is no operation in + // progress (the genuine delay-period NOOP). When an operation is in progress, + // fall through and let ADUC_Workflow_HandlePropertyUpdate route the cancel to + // the in-progress workflow. if (updateAction == ADUCITF_UpdateAction_Cancel && (0 == strcmp(workflowId, "nodeployment"))) { - Log_Info("Received deployment delay period NOOP cancel. Will wait for update deployment to be pushed..."); - goto done; + if (!ADUC_Workflow_IsOperationInProgress(workflowData)) + { + Log_Info("Received deployment delay period NOOP cancel. Will wait for update deployment to be pushed..."); + goto done; + } + + Log_Info("Received 'nodeployment' cancel while an operation is in progress - processing cancel request."); } if (updateAction == ADUCITF_UpdateAction_ProcessDeployment && !IsNullOrEmpty(workflowId)) diff --git a/src/extensions/step_handlers/script_handler/src/script_handler.cpp b/src/extensions/step_handlers/script_handler/src/script_handler.cpp index 5da86eb10..1ea4fea6b 100644 --- a/src/extensions/step_handlers/script_handler/src/script_handler.cpp +++ b/src/extensions/step_handlers/script_handler/src/script_handler.cpp @@ -782,9 +782,136 @@ ADUC_Result ScriptHandlerImpl::Apply(const tagADUC_WorkflowData* workflowData) return result; } +/** + * @brief Actively invokes the script's 'cancel' action so the author's CancelUpdate() function runs. + * + * This is invoked while the workflow's install/apply script may still be running on a different + * (worker) thread. To avoid a data race on the workflow handle, this function reads only immutable + * workflow data (update manifest properties and the work folder) to build the adu-shell command, and + * it intentionally does NOT write any result, result-details, or state back to the shared workflow + * handle (unlike ScriptHandler_PerformAction). The 'cancel' action writes to its own dedicated result + * file (action_cancel_aduc_result.json), which never collides with the install/apply result files. + * + * @param workflowData The workflow data for the step whose script should be cancelled. + * @return ADUC_Result ADUC_Result_Cancel_Success if the script's cancel action ran and reported + * success (or did not report a failure); ADUC_Result_Cancel_UnableToCancel otherwise. + */ +static ADUC_Result ScriptHandler_RunCancelAction(const tagADUC_WorkflowData* workflowData) +{ + const std::string action = "cancel"; + + if (workflowData == nullptr || workflowData->WorkflowHandle == nullptr) + { + Log_Error("Cancel: workflow data or handle is null."); + return ADUC_Result{ ADUC_Result_Cancel_UnableToCancel, ADUC_ERC_SCRIPT_HANDLER_INSTALL_ERROR_NULL_WORKFLOW }; + } + + const ADUC_ConfigInfo* config = ADUC_ConfigInfo_GetInstance(); + if (config == nullptr) + { + Log_Error("Cancel: failed to get config info instance."); + return ADUC_Result{ ADUC_Result_Cancel_UnableToCancel, + ADUC_ERC_SCRIPT_HANDLER_INSTALL_FAILED_TO_GET_CONFIG_INSTANCE }; + } + + char* workFolder = ADUC_WorkflowData_GetWorkFolder(workflowData); + + // Run the work inside a lambda so the config instance and work folder are released exactly once + // on every path. + ADUC_Result result = [&]() -> ADUC_Result { + const char* apiVer = workflow_peek_update_manifest_handler_properties_string( + workflowData->WorkflowHandle, HANDLER_PROPERTIES_API_VERSION); + + if (workFolder == nullptr) + { + Log_Error("Cancel: failed to get work folder."); + return ADUC_Result{ ADUC_Result_Cancel_UnableToCancel, 0 }; + } + + const std::string scriptWorkfolder = workFolder; + const std::string scriptResultFile = scriptWorkfolder + "/action_" + action + "_aduc_result.json"; + std::string scriptFilePath; + std::vector args; + + ADUC_Result prepareResult = ScriptHandlerImpl::PrepareScriptArguments( + workflowData, scriptResultFile, scriptWorkfolder, scriptFilePath, args); + if (IsAducResultCodeFailure(prepareResult.ResultCode)) + { + Log_Warn("Cancel: unable to prepare script arguments (erc 0x%08X).", prepareResult.ExtendedResultCode); + return ADUC_Result{ ADUC_Result_Cancel_UnableToCancel, prepareResult.ExtendedResultCode }; + } + + std::vector aduShellArgs = { adushconst::config_folder_opt, config->configFolder, + adushconst::update_type_opt, adushconst::update_type_microsoft_script, + adushconst::update_action_opt, adushconst::update_action_execute, + adushconst::target_data_opt, scriptFilePath }; + + if (apiVer == nullptr || strcmp(apiVer, "1.0") == 0) + { + std::string backcompatAction = "--action-" + action; + aduShellArgs.emplace_back(adushconst::target_options_opt); + aduShellArgs.emplace_back(backcompatAction.c_str()); + } + else if (strcmp(apiVer, "1.1") == 0) + { + aduShellArgs.emplace_back(adushconst::target_options_opt); + aduShellArgs.emplace_back(HANDLER_ARG_ACTION); + aduShellArgs.emplace_back(adushconst::target_options_opt); + aduShellArgs.emplace_back(action.c_str()); + } + + for (const auto& a : args) + { + aduShellArgs.emplace_back(adushconst::target_options_opt); + aduShellArgs.emplace_back(a); + } + + std::string scriptOutput; + int exitCode = ADUC_LaunchChildProcess(config->aduShellFilePath, aduShellArgs, scriptOutput); + if (!scriptOutput.empty()) + { + Log_Info("%s\n", scriptOutput.c_str()); + } + + if (exitCode != 0) + { + Log_Warn("Cancel: script 'cancel' action exited with code %d.", exitCode); + return ADUC_Result{ ADUC_Result_Cancel_UnableToCancel, + ADUC_ERC_SCRIPT_HANDLER_CHILD_PROCESS_FAILURE_EXITCODE(exitCode) }; + } + + // Best-effort: read the cancel action's own result file (does not touch the shared workflow handle). + ADUC_Result cancelResult = { ADUC_Result_Cancel_Success, 0 }; + JSON_Value* actionResultValue = json_parse_file(scriptResultFile.c_str()); + if (actionResultValue != nullptr) + { + JSON_Object* actionResultObject = json_object(actionResultValue); + int32_t rc = static_cast(json_object_get_number(actionResultObject, "resultCode")); + if (IsAducResultCodeFailure(rc)) + { + cancelResult.ResultCode = ADUC_Result_Cancel_UnableToCancel; + cancelResult.ExtendedResultCode = + static_cast(json_object_get_number(actionResultObject, "extendedResultCode")); + } + json_value_free(actionResultValue); + } + + return cancelResult; + }(); + + workflow_free_string(workFolder); + ADUC_ConfigInfo_ReleaseInstance(config); + return result; +} + /** * @brief Performs 'Cancel' task. - * @return ADUC_Result The result (always success) + * + * Marks the workflow (and its children) as cancel-requested, and actively invokes the script's + * 'cancel' action so the script author's CancelUpdate() function runs and can interrupt an + * in-progress install/download (issue #776). + * + * @return ADUC_Result ADUC_Result_Cancel_Success when the cancellation request was recorded. */ ADUC_Result ScriptHandlerImpl::Cancel(const tagADUC_WorkflowData* workflowData) { @@ -809,6 +936,17 @@ ADUC_Result ScriptHandlerImpl::Cancel(const tagADUC_WorkflowData* workflowData) result.ResultCode = ADUC_Result_Cancel_UnableToCancel; } + // Actively run the script's 'cancel' action so the author's CancelUpdate() runs. This is what + // allows a long-running, blocking install/download script to be interrupted on cancellation. + ADUC_Result cancelActionResult = ScriptHandler_RunCancelAction(workflowData); + if (IsAducResultCodeFailure(cancelActionResult.ResultCode)) + { + Log_Warn( + "Script 'cancel' action did not complete successfully (rc %d, erc 0x%08X).", + cancelActionResult.ResultCode, + cancelActionResult.ExtendedResultCode); + } + return result; } diff --git a/src/extensions/step_handlers/script_handler/tests/script_handler_ut.cpp b/src/extensions/step_handlers/script_handler/tests/script_handler_ut.cpp index 34ed9be0f..c5a985533 100644 --- a/src/extensions/step_handlers/script_handler/tests/script_handler_ut.cpp +++ b/src/extensions/step_handlers/script_handler/tests/script_handler_ut.cpp @@ -861,6 +861,10 @@ TEST_CASE("Script Handler wrapper methods backup restore cancel", "[script_handl ADUC_Result cancelResult = handler->Cancel(&stepWorkflow); CHECK(cancelResult.ResultCode == ADUC_Result_Cancel_Success); + // Cancel must record the cancellation request on the step's workflow handle so that any + // cancellation checks (and the actively-dispatched script 'cancel' action) observe it. See #776. + CHECK(workflow_is_cancel_requested(stepHandle)); + workflow_free(rootHandle); ADUC_ConfigInfo_ReleaseInstance(config); ExtensionManager::Uninit(); diff --git a/src/extensions/update_manifest_handlers/steps_handler/src/steps_handler.cpp b/src/extensions/update_manifest_handlers/steps_handler/src/steps_handler.cpp index ae198dadc..01e87a488 100644 --- a/src/extensions/update_manifest_handlers/steps_handler/src/steps_handler.cpp +++ b/src/extensions/update_manifest_handlers/steps_handler/src/steps_handler.cpp @@ -22,6 +22,7 @@ #include // mallocAndStrcpy #include // STRING_* #include +#include // pthread_mutex_* #include #include @@ -46,6 +47,68 @@ static bool IsStepsHandlerExtraDebugLogsEnabled() return (!IsNullOrEmpty(getenv("DU_AGENT_ENABLE_STEPS_HANDLER_EXTRA_DEBUG_LOGS"))); } +// +// Active child-step tracking for cooperative cancellation. +// +// The steps handler runs each child step's Install/Apply on a worker thread, where the child +// handler may block for a long time (e.g. a script handler waiting on a child process). A +// cancellation request, however, arrives on a different (main) thread via StepsHandler_Cancel. +// +// To allow a cancel to actively interrupt the in-progress child step (e.g. run a script's +// CancelUpdate()), the worker records the currently-executing leaf child handler and its workflow +// handle here while it processes a long-running step phase, and StepsHandler_Cancel dispatches +// Cancel() to it. Handler instances are long-lived singletons owned by the ExtensionManager and the +// workflow handle remains valid for the duration of the workflow, so caching these here is safe. +// +static pthread_mutex_t s_activeChildStepMutex = PTHREAD_MUTEX_INITIALIZER; +static ContentHandler* s_activeChildStepHandler = nullptr; +static ADUC_WorkflowHandle s_activeChildStepWorkflowHandle = nullptr; + +/** + * @brief Record (or clear) the currently-executing leaf child step so a concurrent cancel can reach it. + * + * @param handler The child step content handler (or nullptr to clear). + * @param handle The child step workflow handle (or nullptr to clear). + */ +static void StepsHandler_SetActiveChildStep(ContentHandler* handler, ADUC_WorkflowHandle handle) +{ + pthread_mutex_lock(&s_activeChildStepMutex); + s_activeChildStepHandler = handler; + s_activeChildStepWorkflowHandle = handle; + pthread_mutex_unlock(&s_activeChildStepMutex); +} + +/** + * @brief Dispatch Cancel() to the currently-executing leaf child step, if any. + * + * Reads the active child under the lock and invokes its Cancel() so it can actively interrupt the + * in-progress operation (e.g. run a script's CancelUpdate()). + */ +static void StepsHandler_CancelActiveChildStep() +{ + pthread_mutex_lock(&s_activeChildStepMutex); + ContentHandler* handler = s_activeChildStepHandler; + ADUC_WorkflowHandle handle = s_activeChildStepWorkflowHandle; + pthread_mutex_unlock(&s_activeChildStepMutex); + + if (handler == nullptr || handle == nullptr) + { + return; + } + + Log_Info("Dispatching cancel to active child step handler."); + ADUC_WorkflowData childWorkflow = {}; + childWorkflow.WorkflowHandle = handle; + try + { + handler->Cancel(&childWorkflow); + } + catch (...) + { + Log_Warn("Active child step handler threw during Cancel()."); + } +} + /** * @brief Destructor for the Steps Handler Impl class. */ @@ -873,6 +936,15 @@ static ADUC_Result StepsHandler_Install(const tagADUC_WorkflowData* workflowData // for (size_t i = 0; i < stepsCount; i++) { + // Stop starting new step work once a cancellation has been requested. + if (workflow_is_cancel_requested(handle)) + { + Log_Info("Install loop: cancellation requested - not starting step #%lu.", i); + result.ResultCode = ADUC_Result_Failure_Cancelled; + result.ExtendedResultCode = 0; + goto done; + } + if (IsStepsHandlerExtraDebugLogsEnabled()) { Log_Debug( @@ -973,6 +1045,9 @@ static ADUC_Result StepsHandler_Install(const tagADUC_WorkflowData* workflowData // // Perform 'install' action. // + // Record the active child step so a concurrent cancel (on another thread) can actively + // interrupt this potentially long-running, blocking call (e.g. run a script's CancelUpdate()). + StepsHandler_SetActiveChildStep(contentHandler, stepHandle); try { result = contentHandler->Install(&stepWorkflow); @@ -1038,6 +1113,7 @@ static ADUC_Result StepsHandler_Install(const tagADUC_WorkflowData* workflowData // // Perform 'apply' action. // + StepsHandler_SetActiveChildStep(contentHandler, stepHandle); try { result = contentHandler->Apply(&stepWorkflow); @@ -1136,6 +1212,10 @@ static ADUC_Result StepsHandler_Install(const tagADUC_WorkflowData* workflowData done: + // No child step is actively executing anymore; clear the active child so a late cancel does + // not dispatch to a step that has already finished. + StepsHandler_SetActiveChildStep(nullptr, nullptr); + // NOTE: Do not free child workflow here, so that it can be reused in the next phase. // Only free child handle when the workflow is done. // @@ -1255,6 +1335,13 @@ static ADUC_Result StepsHandler_Cancel(const tagADUC_WorkflowData* workflowData) result.ResultCode = ADUC_Result_Cancel_UnableToCancel; } + // Setting the cancel-requested flag (above) is sufficient for step handlers that poll + // workflow_is_cancel_requested() between operations. However, a step handler that is currently + // blocked inside a long-running operation (e.g. a script handler waiting on a child process) + // cannot observe the flag until it returns. Dispatch Cancel() to the active child step so it can + // actively interrupt that operation (e.g. run the script's CancelUpdate()). See issue #776. + StepsHandler_CancelActiveChildStep(); + return result; }