From f775a2111109b8e8130c48d54e4786dd7415e940 Mon Sep 17 00:00:00 2001 From: Sathish Mathimaran Date: Wed, 9 Sep 2026 11:14:34 +0530 Subject: [PATCH 01/10] Fix stale auto-assessment status caused by systemd startup timeout (Bug #28537460) MsftLinuxPatchAutoAssess.service declared Type=forking, but the shell wrapper it executes runs Core in the foreground and never daemonizes. systemd therefore treated the entire assessment as service startup and SIGTERM'd the cgroup at TimeoutStartSec (default 90s), before Core could write terminal status. Azure Update Manager was left showing a stale "In Progress" assessment. Measured impact before the fix: 17.36% of auto-assessment runs were killed mid-flight; 20.5% of ~266k Azure Linux VMs had at least one stuck run in 24h. Over a month, 19,179 VMs across 8,275 subscriptions were persistently stuck, a median of 26 days out of 31. Killed runs were hard-capped at 89.94s with zero runs observed between 92s and 600s - a guillotine at the deadline, not a tail. Changes 1. ServiceManager generates Type=simple, which matches how the wrapper actually behaves. This also removes a circular wait: under forking, systemctl start blocked for the whole startup window, so a ConfigurePatching call issued during an in-flight assessment stalled until TimeoutStartSec while the assessment itself waited on that same Core operation. Raising the timeout made that stall worse, not better; Type=simple removes it entirely. 2. The generated wrapper bounds each run with GNU timeout (50m budget, 3m kill grace). Type=simple alone would trade a common failure for a rarer permanent one: a hung run keeps the unit active forever, and systemd will not start a second instance of an active unit, so every later hourly timer fire becomes a silent no-op. Under forking this self-healed by accident because the unit went failed and the timer restarted it. The bound lives in the wrapper rather than the unit because it must hold on every systemd version in the fleet. Roughly 6.9% of the periodic-assessment Linux fleet - 19,050 machines - runs pre-229 systemd (EL7 219, SLES 12 228, Amazon Linux 2 219), where unit-level runtime limits are silently ignored rather than rejected. coreutils timeout is present on all of them. timeout is called unconditionally, with no "command -v" guard. It is already an unguarded hard dependency in EnvHealthManager.check_sudo_status and Bootstrapper.check_sudo_status ("timeout 10 sudo id"), and that sudo check runs inside ActionHandler.setup with raise_if_not_sudo=True before this wrapper is generated - so a machine without timeout fails setup outright and a fallback branch would be unreachable while being the only unbounded path in the design. If timeout ever were missing, exec fails with 127 and the unit goes failed, which the timer retries: loud and bounded rather than hanging. Budget + grace (3180s) is sized to complete before the next hourly fire so that fire always finds an inactive unit. The invariant is asserted in tests. Deliberately not included No SuccessExitStatus. When the wrapper bounds a runaway, timeout exits 124 and the unit enters failed. That is left visible on purpose: Core is SIGTERM'd without a signal handler and cannot report the timeout itself, so the failed unit is the only fleet-visible evidence the bound fired. A run exceeding 50 minutes against a p99 of ~35 seconds is genuinely abnormal. The timer restarts a failed unit regardless, so masking it buys nothing. The rationale is recorded on create_service_unit_file. Verification Live on Azure VMs across Ubuntu 22.04 (systemd 249), RHEL 8.9 (239) and SLES 12 SP5 (228), with the extension generating the unit and wrapper itself: normal runs return systemctl start in 0s and complete in 20-35s; a hung run is bounded and exits 124; no orphan processes; the next timer fire recovers. An InstallPatches issued during an in-flight assessment succeeded in 285s with 0s ConfigurePatching stall, where forking deadlocked for 600s and returned CompletedWithWarnings. Unit tests: ServiceManager, both LifecycleManagers, ConfigurePatchingProcessor and ProcessHandler. Tests pin Type=simple on the production creation path, the wrapper shape, the absence of any conditional fallback, that the budget constants are ints, and that budget + grace stays inside the timer interval. test_auto_assess_sh_actually_terminates_a_hung_run executes the generated script against a stub Core that sleeps 120s and asserts it is killed at the budget with exit 124; it is POSIX-only and skips on Windows, where CI runs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 91444677-8618-4dc0-8cf1-f415f7cb7185 --- src/core/src/core_logic/ServiceManager.py | 8 +- src/core/tests/Test_ServiceManager.py | 21 +++++ src/extension/src/Constants.py | 16 ++++ src/extension/src/ProcessHandler.py | 3 +- src/extension/tests/Test_ProcessHandler.py | 101 +++++++++++++++++++++ 5 files changed, 146 insertions(+), 3 deletions(-) diff --git a/src/core/src/core_logic/ServiceManager.py b/src/core/src/core_logic/ServiceManager.py index 9c8dde78..fd7993b8 100644 --- a/src/core/src/core_logic/ServiceManager.py +++ b/src/core/src/core_logic/ServiceManager.py @@ -89,8 +89,12 @@ def is_service_enabled(self): # endregion # region - Service Unit Management - def create_service_unit_file(self, exec_start, desc, after="network.target", service_type="forking", wanted_by="multi-user.target"): - """ Note: Service type defaults to forking because of sh to py process fork """ + def create_service_unit_file(self, exec_start, desc, after="network.target", service_type="simple", wanted_by="multi-user.target"): + """ Note: Service type is simple because the shell wrapper runs Core in the foreground and never daemonizes. + Deliberately no SuccessExitStatus: when the wrapper bounds a runaway, timeout exits 124 and the unit + enters failed. That is left visible on purpose - Core is SIGTERM'd without a handler and cannot report + the timeout itself, so the failed unit is the only fleet-visible evidence the bound fired. The timer + restarts a failed unit regardless, so nothing is gained by masking it. """ service_unit_content_template = "\n[Unit]" + \ "\nDescription={0}" + \ "\nAfter={1}\n" + \ diff --git a/src/core/tests/Test_ServiceManager.py b/src/core/tests/Test_ServiceManager.py index 0eb406ff..258adc4d 100644 --- a/src/core/tests/Test_ServiceManager.py +++ b/src/core/tests/Test_ServiceManager.py @@ -28,6 +28,8 @@ def setUp(self): self.service_manager = ServiceManager(self.runtime.env_layer, self.runtime.execution_config, self.runtime.composite_logger, self.runtime.telemetry_writer,ServiceInfo("AutoAssessment", "Auto assessment service", "path")) self.service_manager.service_name = "test_service" self.mock_systemd_service_unit_path = "/etc/systemd/system/{0}.service" + self.written_service_unit_path = None + self.written_service_unit_content = None def tearDown(self): self.runtime.stop() @@ -38,6 +40,8 @@ def mock_run_command_to_set_service_file_permission(self, cmd, no_output=False, return 0, "permissions set" def mock_write_with_retry_valid(self, file_path_or_handle, data, mode='a+'): + self.written_service_unit_path = file_path_or_handle + self.written_service_unit_content = data return def mock_invoke_systemctl(self, command, description): @@ -63,6 +67,23 @@ def test_create_service_unit_file(self): self.service_manager.env_layer.file_system.write_with_retry = self.mock_write_with_retry_valid self.service_manager.create_service_unit_file(exec_start="/bin/bash " + self.service_manager.service_exec_path, desc="Microsoft Azure Linux Patch Extension - Auto Assessment") + self.assertEqual("/etc/systemd/system/test_service.service", self.written_service_unit_path) + self.assertIn("\nType=simple\n", self.written_service_unit_content) + self.assertNotIn("Type=forking", self.written_service_unit_content) + + def test_create_and_set_service_idem_generates_simple_unit(self): + """ Guards the production creation path, not just the helper, so a future default flip is caught """ + self.service_manager.env_layer.run_command_output = self.mock_run_command_to_set_service_file_permission + self.service_manager.env_layer.file_system.write_with_retry = self.mock_write_with_retry_valid + self.service_manager.invoke_systemctl = self.mock_invoke_systemctl + self.service_manager.systemctl_daemon_reload = lambda: None + + self.service_manager.create_and_set_service_idem() + + self.assertIn("\nType=simple\n", self.written_service_unit_content) + self.assertNotIn("Type=forking", self.written_service_unit_content) + self.assertNotIn("Type=notify", self.written_service_unit_content) + def test_start_service(self): # Set method calls self.service_manager.invoke_systemctl_called = False diff --git a/src/extension/src/Constants.py b/src/extension/src/Constants.py index de5bda73..b28cc922 100644 --- a/src/extension/src/Constants.py +++ b/src/extension/src/Constants.py @@ -59,6 +59,22 @@ def __iter__(self): ENABLE_MAX_RUNTIME = 3 DISABLE_MAX_RUNTIME = 13 + # Auto-assessment runaway protection (Bug 28537460). + # Under Type=simple a hung assessment keeps the unit active forever and every + # later timer fire becomes a no-op, so the run must be externally bounded. + # The bound lives in the generated shell wrapper (GNU timeout) rather than in the + # systemd unit, because it has to hold on every systemd version in the fleet - + # including pre-229 builds such as EL7 (219) and SLES 12 (228), where unit-level + # runtime limits are silently ignored. coreutils timeout is present on all of them, + # and the extension already depends on it unguarded in check_sudo_status. + # Values are integer seconds (coerced with str() at the single emit site in ProcessHandler). + # Invariant enforced by Test_ProcessHandler: budget + grace must complete before the next + # hourly timer fire so that fire always finds an inactive unit: 3000 + 180 = 3180s (53m) vs 60m. + # Raising the budget past that invariant silently reintroduces Bug 28537460. + AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS = 3000 # 50m + AUTO_ASSESSMENT_KILL_GRACE_IN_SECS = 180 # 3m -> hard kill by 53m + AUTO_ASSESSMENT_TIMER_INTERVAL_IN_SECS = 3600 # hourly; the ceiling the two above must fit inside + # Telemetry Settings # Note: these limits are based on number of characters as confirmed with agent team TELEMETRY_MSG_SIZE_LIMIT_IN_CHARS = 3072 diff --git a/src/extension/src/ProcessHandler.py b/src/extension/src/ProcessHandler.py index a0bc9ee0..2795f4fd 100644 --- a/src/extension/src/ProcessHandler.py +++ b/src/extension/src/ProcessHandler.py @@ -119,10 +119,11 @@ def stage_auto_assess_sh_safely(self, core_process_command): .format(cmd_core_py_path, exec_dir, core_py_path, auto_assess_sh_path, core_process_command)) # generating exec script + auto_assess_core_command = core_process_command + " -" + Constants.AUTO_ASSESS_ONLY + " True" auto_assess_sh_data = "#!/usr/bin/env bash" +\ "\n# Copyright 2021 Microsoft Corporation." + \ "\ncd \"$(dirname \"$0\")\"" + \ - "\n" + core_process_command + " -" + Constants.AUTO_ASSESS_ONLY + " True" + "\nexec timeout -s TERM -k " + str(Constants.AUTO_ASSESSMENT_KILL_GRACE_IN_SECS) + " " + str(Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS) + " " + auto_assess_core_command # stage exec script if os.path.exists(auto_assess_sh_path): diff --git a/src/extension/tests/Test_ProcessHandler.py b/src/extension/tests/Test_ProcessHandler.py index 98b7fb0a..eea99ffb 100644 --- a/src/extension/tests/Test_ProcessHandler.py +++ b/src/extension/tests/Test_ProcessHandler.py @@ -19,6 +19,7 @@ import subprocess import sys import tempfile +import time import unittest from extension.src.Constants import Constants from extension.src.file_handlers.ExtOutputStatusHandler import ExtOutputStatusHandler @@ -45,6 +46,8 @@ def setUp(self): self.proc_cmdline_path = os.path.join(self.test_dir, "proc_cmdline") self.ext_output_status_handler = ExtOutputStatusHandler(self.logger, self.utility, self.json_file_handler, dir_path) self.process = subprocess.Popen(["echo", "Hello World!"], shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + self.written_auto_assess_sh_path = None + self.written_auto_assess_sh_content = None def tearDown(self): VirtualTerminal().print_lowlight("\n----------------- tear down test runner -----------------") @@ -81,6 +84,10 @@ def mock_file_system_open_raises_exception(self, path, mode): def mock_run_command_to_set_auto_assess_shell_file_permission(self, cmd, no_output=False, chk_err=False): return 0, "permissions set" + def mock_write_with_retry_valid(self, file_path_or_handle, data, mode='a+'): + self.written_auto_assess_sh_path = file_path_or_handle + self.written_auto_assess_sh_content = data + def mock_subprocess_popen_process_not_running_after_launch(self, command, shell, stdout, stderr): self.process.pid = 1 self.process.poll = self.mock_process_poll_return_Not_None @@ -223,6 +230,100 @@ def test_start_daemon(self): process_handler.env_layer.run_command_output = run_command_output_backup ExtEnvHandler.get_temp_folder = ext_env_handler_get_temp_folder_backup + def test_auto_assess_sh_is_bounded_by_timeout(self): + process_handler = ProcessHandler(self.logger, self.env_layer, self.ext_output_status_handler) + write_backup = process_handler.env_layer.file_system.write_with_retry + run_backup = process_handler.env_layer.run_command_output + process_handler.env_layer.file_system.write_with_retry = self.mock_write_with_retry_valid + process_handler.env_layer.run_command_output = self.mock_run_command_to_set_auto_assess_shell_file_permission + + process_handler.stage_auto_assess_sh_safely("/usr/bin/python3 /tmp/MsftLinuxPatchCore.py -sequenceNumber 1") + + data = self.written_auto_assess_sh_content + self.assertIn(Constants.CORE_AUTO_ASSESS_SH_FILE_NAME, self.written_auto_assess_sh_path) + self.assertIn("exec timeout -s TERM -k " + str(Constants.AUTO_ASSESSMENT_KILL_GRACE_IN_SECS) + + " " + str(Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS), data) + self.assertIn("-" + Constants.AUTO_ASSESS_ONLY + " True", data) + # timeout is a hard dependency the extension already relies on unguarded in + # check_sudo_status, which runs during setup before this script is generated. There must + # be no conditional fallback here: the only alternative branch would be an unbounded run, + # which is the exact failure this wrapper exists to prevent. + self.assertNotIn("command -v timeout", data) + self.assertNotIn("else", data) + + # the process must be killed within the allocated time budget + # before the next timer interval fires. + self.assertLess(Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS + + Constants.AUTO_ASSESSMENT_KILL_GRACE_IN_SECS, + Constants.AUTO_ASSESSMENT_TIMER_INTERVAL_IN_SECS) + + process_handler.env_layer.file_system.write_with_retry = write_backup + process_handler.env_layer.run_command_output = run_backup + + def test_auto_assess_sh_actually_terminates_a_hung_run(self): + """ Bug 28537460: behavioural proof that the generated wrapper really does kill a hung + assessment, rather than merely containing the right text. Executes the exact bytes + the extension writes, against a stub Core that never exits. + + POSIX-only. The wrapper is bash + GNU coreutils timeout, and Windows has neither + (its timeout.exe is an unrelated command that pauses, and WSL bash cannot resolve + Windows paths). CI currently runs windows-latest for both the 3.12 and 2.7 jobs, + so this SKIPS there - the authoritative cross-distro evidence remains the live-VM + verification on Ubuntu 22.04, RHEL 8.9 and SLES 12 SP5. """ + if os.name != 'posix': + self.skipTest("wrapper is bash + GNU timeout; not runnable on Windows") + try: + probe = subprocess.Popen(["timeout", "--version"], stdout=subprocess.PIPE, stderr=subprocess.PIPE) + probe_out = probe.communicate()[0] + if probe.returncode != 0 or "coreutils".encode() not in probe_out: + self.skipTest("GNU coreutils timeout not available") + except OSError: + self.skipTest("GNU coreutils timeout not available") + + budget_backup = Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS + grace_backup = Constants.AUTO_ASSESSMENT_KILL_GRACE_IN_SECS + write_backup = self.env_layer.file_system.write_with_retry + run_backup = self.env_layer.run_command_output + # a real 50m budget cannot be waited out, so shrink it; the wrapper shape is unchanged + Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS = 2 + Constants.AUTO_ASSESSMENT_KILL_GRACE_IN_SECS = 1 + stub_dir = tempfile.mkdtemp() + try: + stub_core_path = os.path.join(stub_dir, Constants.CORE_CODE_FILE_NAME) + stub_core = open(stub_core_path, "w") + stub_core.write("import time\ntime.sleep(120)\n") # never exits within the budget + stub_core.close() + + process_handler = ProcessHandler(self.logger, self.env_layer, self.ext_output_status_handler) + process_handler.env_layer.file_system.write_with_retry = self.mock_write_with_retry_valid + process_handler.env_layer.run_command_output = self.mock_run_command_to_set_auto_assess_shell_file_permission + process_handler.stage_auto_assess_sh_safely(sys.executable + " " + stub_core_path + " -sequenceNumber 1") + + # write out the exact generated bytes and execute them + sh_path = os.path.join(stub_dir, Constants.CORE_AUTO_ASSESS_SH_FILE_NAME) + sh_file = open(sh_path, "w") + sh_file.write(self.written_auto_assess_sh_content) + sh_file.close() + os.chmod(sh_path, 0o755) + + started_at = time.time() + proc = subprocess.Popen(["/bin/bash", sh_path], stdout=subprocess.PIPE, stderr=subprocess.PIPE) + proc.communicate() + elapsed = time.time() - started_at + + # 124 is GNU timeout's budget-expired code, so the kill came from the wrapper and + # not from the stub exiting on its own + self.assertEqual(124, proc.returncode) + self.assertGreaterEqual(elapsed, Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS) + self.assertLess(elapsed, Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS + + Constants.AUTO_ASSESSMENT_KILL_GRACE_IN_SECS + 30) + finally: + Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS = budget_backup + Constants.AUTO_ASSESSMENT_KILL_GRACE_IN_SECS = grace_backup + self.env_layer.file_system.write_with_retry = write_backup + self.env_layer.run_command_output = run_backup + shutil.rmtree(stub_dir, ignore_errors=True) + def test_is_process_patching_operation(self): # setting mocks backup_file_system_open = self.env_layer.file_system.open From fe7ab0c1ce67e1370c985a6313b98b71474c5800 Mon Sep 17 00:00:00 2001 From: Sathish Mathimaran Date: Wed, 9 Sep 2026 13:26:05 +0530 Subject: [PATCH 02/10] Clean up constant --- src/extension/src/Constants.py | 1 - src/extension/tests/Test_ProcessHandler.py | 6 ------ 2 files changed, 7 deletions(-) diff --git a/src/extension/src/Constants.py b/src/extension/src/Constants.py index b28cc922..91d8b020 100644 --- a/src/extension/src/Constants.py +++ b/src/extension/src/Constants.py @@ -73,7 +73,6 @@ def __iter__(self): # Raising the budget past that invariant silently reintroduces Bug 28537460. AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS = 3000 # 50m AUTO_ASSESSMENT_KILL_GRACE_IN_SECS = 180 # 3m -> hard kill by 53m - AUTO_ASSESSMENT_TIMER_INTERVAL_IN_SECS = 3600 # hourly; the ceiling the two above must fit inside # Telemetry Settings # Note: these limits are based on number of characters as confirmed with agent team diff --git a/src/extension/tests/Test_ProcessHandler.py b/src/extension/tests/Test_ProcessHandler.py index eea99ffb..8fa8e33b 100644 --- a/src/extension/tests/Test_ProcessHandler.py +++ b/src/extension/tests/Test_ProcessHandler.py @@ -251,12 +251,6 @@ def test_auto_assess_sh_is_bounded_by_timeout(self): self.assertNotIn("command -v timeout", data) self.assertNotIn("else", data) - # the process must be killed within the allocated time budget - # before the next timer interval fires. - self.assertLess(Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS - + Constants.AUTO_ASSESSMENT_KILL_GRACE_IN_SECS, - Constants.AUTO_ASSESSMENT_TIMER_INTERVAL_IN_SECS) - process_handler.env_layer.file_system.write_with_retry = write_backup process_handler.env_layer.run_command_output = run_backup From 4ab349aaaa6c74ba38858eb239b9172bc1046910 Mon Sep 17 00:00:00 2001 From: Sathish Mathimaran Date: Wed, 9 Sep 2026 13:27:17 +0530 Subject: [PATCH 03/10] clean up unused test --- src/extension/tests/Test_ProcessHandler.py | 64 ---------------------- 1 file changed, 64 deletions(-) diff --git a/src/extension/tests/Test_ProcessHandler.py b/src/extension/tests/Test_ProcessHandler.py index 8fa8e33b..7ca4b557 100644 --- a/src/extension/tests/Test_ProcessHandler.py +++ b/src/extension/tests/Test_ProcessHandler.py @@ -254,70 +254,6 @@ def test_auto_assess_sh_is_bounded_by_timeout(self): process_handler.env_layer.file_system.write_with_retry = write_backup process_handler.env_layer.run_command_output = run_backup - def test_auto_assess_sh_actually_terminates_a_hung_run(self): - """ Bug 28537460: behavioural proof that the generated wrapper really does kill a hung - assessment, rather than merely containing the right text. Executes the exact bytes - the extension writes, against a stub Core that never exits. - - POSIX-only. The wrapper is bash + GNU coreutils timeout, and Windows has neither - (its timeout.exe is an unrelated command that pauses, and WSL bash cannot resolve - Windows paths). CI currently runs windows-latest for both the 3.12 and 2.7 jobs, - so this SKIPS there - the authoritative cross-distro evidence remains the live-VM - verification on Ubuntu 22.04, RHEL 8.9 and SLES 12 SP5. """ - if os.name != 'posix': - self.skipTest("wrapper is bash + GNU timeout; not runnable on Windows") - try: - probe = subprocess.Popen(["timeout", "--version"], stdout=subprocess.PIPE, stderr=subprocess.PIPE) - probe_out = probe.communicate()[0] - if probe.returncode != 0 or "coreutils".encode() not in probe_out: - self.skipTest("GNU coreutils timeout not available") - except OSError: - self.skipTest("GNU coreutils timeout not available") - - budget_backup = Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS - grace_backup = Constants.AUTO_ASSESSMENT_KILL_GRACE_IN_SECS - write_backup = self.env_layer.file_system.write_with_retry - run_backup = self.env_layer.run_command_output - # a real 50m budget cannot be waited out, so shrink it; the wrapper shape is unchanged - Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS = 2 - Constants.AUTO_ASSESSMENT_KILL_GRACE_IN_SECS = 1 - stub_dir = tempfile.mkdtemp() - try: - stub_core_path = os.path.join(stub_dir, Constants.CORE_CODE_FILE_NAME) - stub_core = open(stub_core_path, "w") - stub_core.write("import time\ntime.sleep(120)\n") # never exits within the budget - stub_core.close() - - process_handler = ProcessHandler(self.logger, self.env_layer, self.ext_output_status_handler) - process_handler.env_layer.file_system.write_with_retry = self.mock_write_with_retry_valid - process_handler.env_layer.run_command_output = self.mock_run_command_to_set_auto_assess_shell_file_permission - process_handler.stage_auto_assess_sh_safely(sys.executable + " " + stub_core_path + " -sequenceNumber 1") - - # write out the exact generated bytes and execute them - sh_path = os.path.join(stub_dir, Constants.CORE_AUTO_ASSESS_SH_FILE_NAME) - sh_file = open(sh_path, "w") - sh_file.write(self.written_auto_assess_sh_content) - sh_file.close() - os.chmod(sh_path, 0o755) - - started_at = time.time() - proc = subprocess.Popen(["/bin/bash", sh_path], stdout=subprocess.PIPE, stderr=subprocess.PIPE) - proc.communicate() - elapsed = time.time() - started_at - - # 124 is GNU timeout's budget-expired code, so the kill came from the wrapper and - # not from the stub exiting on its own - self.assertEqual(124, proc.returncode) - self.assertGreaterEqual(elapsed, Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS) - self.assertLess(elapsed, Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS - + Constants.AUTO_ASSESSMENT_KILL_GRACE_IN_SECS + 30) - finally: - Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS = budget_backup - Constants.AUTO_ASSESSMENT_KILL_GRACE_IN_SECS = grace_backup - self.env_layer.file_system.write_with_retry = write_backup - self.env_layer.run_command_output = run_backup - shutil.rmtree(stub_dir, ignore_errors=True) - def test_is_process_patching_operation(self): # setting mocks backup_file_system_open = self.env_layer.file_system.open From a83608c38d0ed509101ad0db6caa02318d9cb48a Mon Sep 17 00:00:00 2001 From: Sathish Mathimaran Date: Wed, 9 Sep 2026 14:34:31 +0530 Subject: [PATCH 04/10] fix comments and unit tests --- src/core/src/core_logic/ServiceManager.py | 6 +----- src/extension/tests/Test_ProcessHandler.py | 9 ++++----- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/core/src/core_logic/ServiceManager.py b/src/core/src/core_logic/ServiceManager.py index fd7993b8..c79b4ce0 100644 --- a/src/core/src/core_logic/ServiceManager.py +++ b/src/core/src/core_logic/ServiceManager.py @@ -90,11 +90,7 @@ def is_service_enabled(self): # region - Service Unit Management def create_service_unit_file(self, exec_start, desc, after="network.target", service_type="simple", wanted_by="multi-user.target"): - """ Note: Service type is simple because the shell wrapper runs Core in the foreground and never daemonizes. - Deliberately no SuccessExitStatus: when the wrapper bounds a runaway, timeout exits 124 and the unit - enters failed. That is left visible on purpose - Core is SIGTERM'd without a handler and cannot report - the timeout itself, so the failed unit is the only fleet-visible evidence the bound fired. The timer - restarts a failed unit regardless, so nothing is gained by masking it. """ + """ Note: Service type is simple because the shell wrapper runs Core in the foreground.""" service_unit_content_template = "\n[Unit]" + \ "\nDescription={0}" + \ "\nAfter={1}\n" + \ diff --git a/src/extension/tests/Test_ProcessHandler.py b/src/extension/tests/Test_ProcessHandler.py index 7ca4b557..af74c515 100644 --- a/src/extension/tests/Test_ProcessHandler.py +++ b/src/extension/tests/Test_ProcessHandler.py @@ -239,17 +239,16 @@ def test_auto_assess_sh_is_bounded_by_timeout(self): process_handler.stage_auto_assess_sh_safely("/usr/bin/python3 /tmp/MsftLinuxPatchCore.py -sequenceNumber 1") - data = self.written_auto_assess_sh_content self.assertIn(Constants.CORE_AUTO_ASSESS_SH_FILE_NAME, self.written_auto_assess_sh_path) self.assertIn("exec timeout -s TERM -k " + str(Constants.AUTO_ASSESSMENT_KILL_GRACE_IN_SECS) - + " " + str(Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS), data) - self.assertIn("-" + Constants.AUTO_ASSESS_ONLY + " True", data) + + " " + str(Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS), self.written_auto_assess_sh_content) + self.assertIn("-" + Constants.AUTO_ASSESS_ONLY + " True", self.written_auto_assess_sh_content) # timeout is a hard dependency the extension already relies on unguarded in # check_sudo_status, which runs during setup before this script is generated. There must # be no conditional fallback here: the only alternative branch would be an unbounded run, # which is the exact failure this wrapper exists to prevent. - self.assertNotIn("command -v timeout", data) - self.assertNotIn("else", data) + self.assertNotIn("command -v timeout", self.written_auto_assess_sh_content) + self.assertNotIn("else", self.written_auto_assess_sh_content) process_handler.env_layer.file_system.write_with_retry = write_backup process_handler.env_layer.run_command_output = run_backup From 7a32d5dad006411227407aa22e1bf138938fbc29 Mon Sep 17 00:00:00 2001 From: Sathish Mathimaran Date: Wed, 9 Sep 2026 14:39:02 +0530 Subject: [PATCH 05/10] fix comments --- src/extension/src/Constants.py | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/extension/src/Constants.py b/src/extension/src/Constants.py index 91d8b020..381d8fda 100644 --- a/src/extension/src/Constants.py +++ b/src/extension/src/Constants.py @@ -59,20 +59,11 @@ def __iter__(self): ENABLE_MAX_RUNTIME = 3 DISABLE_MAX_RUNTIME = 13 - # Auto-assessment runaway protection (Bug 28537460). - # Under Type=simple a hung assessment keeps the unit active forever and every + # Bug 28537460:Under Type=simple a hung assessment keeps the unit active forever and every # later timer fire becomes a no-op, so the run must be externally bounded. - # The bound lives in the generated shell wrapper (GNU timeout) rather than in the - # systemd unit, because it has to hold on every systemd version in the fleet - - # including pre-229 builds such as EL7 (219) and SLES 12 (228), where unit-level - # runtime limits are silently ignored. coreutils timeout is present on all of them, - # and the extension already depends on it unguarded in check_sudo_status. - # Values are integer seconds (coerced with str() at the single emit site in ProcessHandler). - # Invariant enforced by Test_ProcessHandler: budget + grace must complete before the next - # hourly timer fire so that fire always finds an inactive unit: 3000 + 180 = 3180s (53m) vs 60m. - # Raising the budget past that invariant silently reintroduces Bug 28537460. + # This is the bound for the auto-assessment process. AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS = 3000 # 50m - AUTO_ASSESSMENT_KILL_GRACE_IN_SECS = 180 # 3m -> hard kill by 53m + AUTO_ASSESSMENT_KILL_GRACE_IN_SECS = 300 # 5m. kill process after 55 minutes. # Telemetry Settings # Note: these limits are based on number of characters as confirmed with agent team From 44d5c74ff075bf1bf20346fe5cda5f34fca8245a Mon Sep 17 00:00:00 2001 From: Sathish Mathimaran Date: Thu, 10 Sep 2026 12:09:25 +0530 Subject: [PATCH 06/10] Harden the auto-assess wrapper test; drop a dead import (Bug #28537460) Independent review flagged two issues in Test_ProcessHandler. assertNotIn("else", ...) was a load-bearing check written as a four-letter substring match. It is brittle - an "#else" in a comment would break it - and more importantly it had a real hole: a case/esac fallback contains neither "command -v timeout" nor "else", so it passed the old assertions while still introducing an unbounded execution path. Verified: a case/esac variant passes the old check and fails the new one. Assert the structure instead. The wrapper must be exactly one exec, and that exec must be the bounded one on the final line. Any conditional fallback adds a second exec or displaces the last line, so this catches the whole class rather than two spellings of it. Also removes an unused "import time", left behind when a live-timing test was deleted. Tests only - no behavioural change, so the existing live verification across Azure VMs and Arc-enabled servers (Ubuntu 22.04, RHEL 8.10, SLES 12 SP5) still applies. Full extension suite: 121 tests OK. Core ServiceManager, both LifecycleManagers and ConfigurePatchingProcessor: 36 passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 91444677-8618-4dc0-8cf1-f415f7cb7185 --- src/extension/tests/Test_ProcessHandler.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/extension/tests/Test_ProcessHandler.py b/src/extension/tests/Test_ProcessHandler.py index af74c515..085647e4 100644 --- a/src/extension/tests/Test_ProcessHandler.py +++ b/src/extension/tests/Test_ProcessHandler.py @@ -19,7 +19,6 @@ import subprocess import sys import tempfile -import time import unittest from extension.src.Constants import Constants from extension.src.file_handlers.ExtOutputStatusHandler import ExtOutputStatusHandler @@ -246,9 +245,12 @@ def test_auto_assess_sh_is_bounded_by_timeout(self): # timeout is a hard dependency the extension already relies on unguarded in # check_sudo_status, which runs during setup before this script is generated. There must # be no conditional fallback here: the only alternative branch would be an unbounded run, - # which is the exact failure this wrapper exists to prevent. + # which is the exact failure this wrapper exists to prevent. Assert that structurally - + # exactly one exec, and it is the bounded one on the last line. self.assertNotIn("command -v timeout", self.written_auto_assess_sh_content) - self.assertNotIn("else", self.written_auto_assess_sh_content) + self.assertEqual(1, self.written_auto_assess_sh_content.count("exec ")) + script_lines = [line for line in self.written_auto_assess_sh_content.split("\n") if line.strip()] + self.assertTrue(script_lines[-1].startswith("exec timeout -s TERM -k ")) process_handler.env_layer.file_system.write_with_retry = write_backup process_handler.env_layer.run_command_output = run_backup From 608da12d168274ab9ce932fe5b52249db19f1b46 Mon Sep 17 00:00:00 2001 From: Sathish Mathimaran Date: Thu, 10 Sep 2026 14:44:41 +0530 Subject: [PATCH 07/10] Record the timer-interval ceiling on the auto-assess budget (Bug #28537460) The existing comment explained why the run must be bounded but not what the bound has to stay under, or where that number comes from. A future editor raising the budget would read "must be externally bounded", conclude the run is still bounded, and not realise that exceeding the hourly timer interval silently restores the original bug - a hung run keeps the unit active, systemd refuses a second instance, and every later fire is a no-op. The interval lives in a different package (core AUTO_ASSESSMENT_CRON_INTERVAL, passed explicitly by TimerManager.create_and_set_timer_idem), so nothing near these constants hints at it. Name it here instead of restating what the constant names already say. Comment only - no code, test, or behaviour change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 91444677-8618-4dc0-8cf1-f415f7cb7185 --- src/extension/src/Constants.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/extension/src/Constants.py b/src/extension/src/Constants.py index 381d8fda..67b78e87 100644 --- a/src/extension/src/Constants.py +++ b/src/extension/src/Constants.py @@ -61,7 +61,8 @@ def __iter__(self): # Bug 28537460:Under Type=simple a hung assessment keeps the unit active forever and every # later timer fire becomes a no-op, so the run must be externally bounded. - # This is the bound for the auto-assessment process. + # Budget + grace must stay under the hourly auto-assessment timer interval + # (core AUTO_ASSESSMENT_CRON_INTERVAL = PT1H) so every fire finds an inactive unit. AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS = 3000 # 50m AUTO_ASSESSMENT_KILL_GRACE_IN_SECS = 300 # 5m. kill process after 55 minutes. From e57d631cc6285c9a83bf68fdde24691cac693529 Mon Sep 17 00:00:00 2001 From: SathishMSFT <68918805+SathishMSFT@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:53:32 +0530 Subject: [PATCH 08/10] Fix comments formatting in Constants.py Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/extension/src/Constants.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/extension/src/Constants.py b/src/extension/src/Constants.py index 67b78e87..008001d8 100644 --- a/src/extension/src/Constants.py +++ b/src/extension/src/Constants.py @@ -59,12 +59,12 @@ def __iter__(self): ENABLE_MAX_RUNTIME = 3 DISABLE_MAX_RUNTIME = 13 - # Bug 28537460:Under Type=simple a hung assessment keeps the unit active forever and every + # Bug 28537460: Under Type=simple a hung assessment keeps the unit active forever and every # later timer fire becomes a no-op, so the run must be externally bounded. # Budget + grace must stay under the hourly auto-assessment timer interval # (core AUTO_ASSESSMENT_CRON_INTERVAL = PT1H) so every fire finds an inactive unit. AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS = 3000 # 50m - AUTO_ASSESSMENT_KILL_GRACE_IN_SECS = 300 # 5m. kill process after 55 minutes. + AUTO_ASSESSMENT_KILL_GRACE_IN_SECS = 300 # 5m. Kill process after 55 minutes. # Telemetry Settings # Note: these limits are based on number of characters as confirmed with agent team From 53ce1a6b29188b95867da65038cd56f2c04ae8f1 Mon Sep 17 00:00:00 2001 From: Sathish Mathimaran Date: Thu, 10 Sep 2026 16:01:47 +0530 Subject: [PATCH 09/10] clean up --- src/extension/src/Constants.py | 4 ++-- src/extension/tests/Test_ProcessHandler.py | 5 ----- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/extension/src/Constants.py b/src/extension/src/Constants.py index 008001d8..535f2452 100644 --- a/src/extension/src/Constants.py +++ b/src/extension/src/Constants.py @@ -61,8 +61,8 @@ def __iter__(self): # Bug 28537460: Under Type=simple a hung assessment keeps the unit active forever and every # later timer fire becomes a no-op, so the run must be externally bounded. - # Budget + grace must stay under the hourly auto-assessment timer interval - # (core AUTO_ASSESSMENT_CRON_INTERVAL = PT1H) so every fire finds an inactive unit. + # Maximum Runtime + grace time must stay under the hourly auto-assessment timer interval + # (core AUTO_ASSESSMENT_CRON_INTERVAL = PT1H) so process is killed within 1 hour if still running. AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS = 3000 # 50m AUTO_ASSESSMENT_KILL_GRACE_IN_SECS = 300 # 5m. Kill process after 55 minutes. diff --git a/src/extension/tests/Test_ProcessHandler.py b/src/extension/tests/Test_ProcessHandler.py index 085647e4..07223603 100644 --- a/src/extension/tests/Test_ProcessHandler.py +++ b/src/extension/tests/Test_ProcessHandler.py @@ -242,11 +242,6 @@ def test_auto_assess_sh_is_bounded_by_timeout(self): self.assertIn("exec timeout -s TERM -k " + str(Constants.AUTO_ASSESSMENT_KILL_GRACE_IN_SECS) + " " + str(Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS), self.written_auto_assess_sh_content) self.assertIn("-" + Constants.AUTO_ASSESS_ONLY + " True", self.written_auto_assess_sh_content) - # timeout is a hard dependency the extension already relies on unguarded in - # check_sudo_status, which runs during setup before this script is generated. There must - # be no conditional fallback here: the only alternative branch would be an unbounded run, - # which is the exact failure this wrapper exists to prevent. Assert that structurally - - # exactly one exec, and it is the bounded one on the last line. self.assertNotIn("command -v timeout", self.written_auto_assess_sh_content) self.assertEqual(1, self.written_auto_assess_sh_content.count("exec ")) script_lines = [line for line in self.written_auto_assess_sh_content.split("\n") if line.strip()] From e807d553def07b8db55012df62cb143c8fb4e267 Mon Sep 17 00:00:00 2001 From: SathishMSFT <68918805+SathishMSFT@users.noreply.github.com> Date: Fri, 11 Sep 2026 09:10:34 +0530 Subject: [PATCH 10/10] Remove assertion for 'command -v timeout' Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/extension/tests/Test_ProcessHandler.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/extension/tests/Test_ProcessHandler.py b/src/extension/tests/Test_ProcessHandler.py index 07223603..732ac4f6 100644 --- a/src/extension/tests/Test_ProcessHandler.py +++ b/src/extension/tests/Test_ProcessHandler.py @@ -242,7 +242,6 @@ def test_auto_assess_sh_is_bounded_by_timeout(self): self.assertIn("exec timeout -s TERM -k " + str(Constants.AUTO_ASSESSMENT_KILL_GRACE_IN_SECS) + " " + str(Constants.AUTO_ASSESSMENT_MAX_RUNTIME_IN_SECS), self.written_auto_assess_sh_content) self.assertIn("-" + Constants.AUTO_ASSESS_ONLY + " True", self.written_auto_assess_sh_content) - self.assertNotIn("command -v timeout", self.written_auto_assess_sh_content) self.assertEqual(1, self.written_auto_assess_sh_content.count("exec ")) script_lines = [line for line in self.written_auto_assess_sh_content.split("\n") if line.strip()] self.assertTrue(script_lines[-1].startswith("exec timeout -s TERM -k "))