Repository navigation
Bugfix/28537460 Fix Auto assessment status stuck in-progress - #386
Merged
Merged
Conversation
…ug #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
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
…37460) 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
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
SathishMSFT
requested review from
Koshy John (kjohn-msft),
Mike Richmond (mirichmo) and
Rajasi Rane (rane-rajasi)
as code owners
September 10, 2026 11:00
github-actions
Bot
requested review from
GAURAVRAMRAKHYANI,
Michelle McDaniel (michellemcdaniel),
nikhim-um and
yashnap
September 10, 2026 11:01
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #386 +/- ##
==========================================
+ Coverage 94.98% 95.03% +0.04%
==========================================
Files 113 113
Lines 21955 21994 +39
==========================================
+ Hits 20855 20901 +46
+ Misses 1100 1093 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The generated shim assumes timeout is present with no guard/fallback, and the new test currently hard-codes the absence of such a defensive check, making the runtime behavior and future hardening more fragile.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes systemd service semantics for MsftLinuxPatchAutoAssess.service by switching the unit to Type=simple and bounding auto-assessment runtime so assessments can complete and reliably reach a terminal status without being SIGTERM’d by TimeoutStartSec.
Changes:
- Change generated systemd unit default
Type=fromforkingtosimplefor the auto-assessment service. - Wrap the auto-assessment shim execution in
timeoutwith TERM + KILL grace to prevent indefinite hangs. - Add unit tests to validate the generated unit file and shim script content.
File summaries
| File | Description |
|---|---|
| src/extension/src/ProcessHandler.py | Generates the auto-assess shim to run Core under timeout with TERM/KILL bounds. |
| src/extension/src/Constants.py | Introduces timeout/grace constants used for bounding auto-assessment runtime. |
| src/extension/tests/Test_ProcessHandler.py | Adds a unit test verifying the generated shim includes bounded timeout execution. |
| src/core/src/core_logic/ServiceManager.py | Changes systemd unit generation default from Type=forking to Type=simple. |
| src/core/tests/Test_ServiceManager.py | Adds tests asserting the generated unit uses Type=simple (and not forking/notify). |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
nikhim-um
requested changes
Sep 15, 2026
nikhim-um
self-requested a review
September 17, 2026 06:46
Contributor
|
Have you manually tested the scenario where the auto-assessment remains stuck for more than 50 minutes and verified that the process is successfully terminated? |
nikhim-um
approved these changes
Sep 17, 2026
SathishMSFT
changed the title
Member
Author
Verified by manually setting smaller timeout and adding delay in the package manager commands |
GAURAVRAMRAKHYANI
self-requested a review
September 17, 2026 09:45
GAURAVRAMRAKHYANI
approved these changes
Sep 17, 2026
Rajasi Rane (rane-rajasi)
requested changes
Sep 24, 2026
Rajasi Rane (rane-rajasi)
left a comment
Rajasi Rane (rane-rajasi)
left a comment
Contributor
There was a problem hiding this comment.
Can you attach your test logs in the PR? If not already done, run this on Azure VM/s and fetch logs under /var/log/azure/Microsoft.CPlat.Core.LinuxPatchExtension/
.core.log
.aa.core.log
Attest the time taken for both a regular patch operation and an auto assess operation to complete. (Auto assessments run hourly)
Member
Author
|
2.core.log |
Rajasi Rane (rane-rajasi)
approved these changes
Oct 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue:
MsftLinuxPatchAutoAssess.servicedeclaredType=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 atTimeoutStartSec(default 90s), before Core could write terminal status. Azure Update Manager was left showing a stale "In Progress" assessment.fix:
The fix we are doing here is to declare
MsftLinuxPatchAutoAssess.servicetoType=simple. This makes Auto assess service to run without terminating after 90 seconds.This introduces newer issue. What if the AutoAssess service stalls/ hung indefinitely:
To fix this part, we call the bash script for auto assessment with a time out which sends termination signal at 50 minutes and kills the process at 55 minutes (if it's still running).
With both these fixes, Auto assessment service can run to completion taking > 30 seconds and upto 50 minutes. In our queries, we found that on-demand assessment operation`s p99 was close to 10 minutes. This should be sufficiently enough complete the auto assesses operations and set the status to terminal state.
test: