Fix cancellation of running updates and script-handler CancelUpdate (#776, #777)

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
 

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);

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)

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
 //

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))

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<std::string> 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<std::string> 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<int32_t>(json_object_get_number(actionResultObject, "resultCode"));
+            if (IsAducResultCodeFailure(rc))
+            {
+                cancelResult.ResultCode = ADUC_Result_Cancel_UnableToCancel;
+                cancelResult.ExtendedResultCode =
+                    static_cast<int32_t>(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;
 }
 

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();