Validate that process is actually a patching operation before terminating by michellemcdaniel · Pull Request #367 · Azure/LinuxPatchExtension

michellemcdaniel · 2026-07-23T20:39:48Z

There are times when the previous patching operation will not be running (reboot, etc), where its PID will be reused before the next patching operation starts. When ProcessHandler checks for running previous patching operations, it will identify this new process which is not the patching extension as a process that needs to be terminated. If this process is a required process for the service, it causes that service to crash.

This change checks the process's cmdline by reading the pid's cmdline file in the proc filesystem. If it contains the patching extension Core filename, then we can be more confident that the process we are about to end is the correct one. If it does not find the core filename in the cmdline, we can guarantee that the process is not a patching operation, and we will not end that process.

Logging:

Processes still running from the previous request: [PIDs=[41245]]
Previous patch operation is still in progress with last status update at 2026-07-23T20:10:34Z. Waiting for a maximum of 0:02:59.461797 seconds for it to complete with intermittent status change checks. Next check will be performed after 60 seconds.
Reading file. [File=CoreState.json]
Previous patch operation is still in progress with last status update at 2026-07-23T20:10:34Z. Waiting for a maximum of 0:01:59.403644 seconds for it to complete with intermittent status change checks. Next check will be performed after 60 seconds.
Reading file. [File=CoreState.json]
Previous patch operation is still in progress with last status update at 2026-07-23T20:10:34Z. Waiting for a maximum of 0:00:59.364601 seconds for it to complete with intermittent status change checks. Next check will be performed after 59.364601 seconds.
Reading file. [File=CoreState.json]
Previous request did not complete in time. Terminating all of it's running processes.
DEBUG: Process is a patching operation. [PID=41245]
Terminating process: [PID=41245]
Deleting file. [File=CoreState.json]
DEBUG: Deleting format-matching items from temp folder. [FormatList=[*]][TempFolderLocation=/var/lib/waagent/Microsoft.CPlat.Core.LinuxPatchExtension-1.6.69/tmp]

…ting There are times when the previous patching operation will not be running (reboot, etc), where its PID will be reused before the next patching operation starts. When ProcessHandler checks for running previous patching operations, it will identify this new process which is not the patching extension as a process that needs to be terminated. If this process is a required process for the service, it causes that service to effectively crash. This change checks the process's cmdline by reading the pid's cmdline file in the proc filesystem. If it contains the patching extension Core filename, then we can be more confident that the process we are about to end is the correct one. If it does not find the core filename in the cmdline, we can guarantee that the process is not a patching operation, and we will not end that process.

codecov · 2026-07-23T20:42:42Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.11%. Comparing base (fc7f336) to head (211de45).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
+ Coverage   94.10%   94.11%   +0.01%     
==========================================
  Files         109      109              
  Lines       20642    20689      +47     
==========================================
+ Hits        19425    19472      +47     
  Misses       1217     1217

Flag	Coverage Δ
python27	`94.11% <100.00%> (+0.01%)`	⬆️
python312	`94.11% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR improves safety when terminating leftover “previous patch” processes by verifying the target PID actually belongs to the LinuxPatchExtension core process (via /proc/<pid>/cmdline) before sending SIGTERM, reducing the risk of killing an unrelated process whose PID was reused after reboot.

Changes:

Added is_process_patching_operation() to validate a PID’s command line contains the core filename before termination.
Updated kill_process() to terminate only when the process is both running and identified as a patching operation.
Updated unit tests to account for the new termination gating logic.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/extension/src/ProcessHandler.py	Adds process cmdline validation and gates termination on “running && patching operation”.
src/extension/tests/Test_ProcessHandler.py	Adjusts `kill_process` test setup for the new patching-operation check.

Comments suppressed due to low confidence (1)

src/extension/tests/Test_ProcessHandler.py:152

test_kill_process only covers the case where is_process_patching_operation returns True; the PR’s key behavior change is that non-patching processes must NOT be terminated. Add a negative test that ensures os.kill is not called when is_process_patching_operation returns False.

    def test_kill_process(self):
        # setting mocks
        is_process_running_backup = ProcessHandler.is_process_running
        ProcessHandler.is_process_running = self.mock_is_process_running_to_return_true
        is_process_patching_operation_backup = ProcessHandler.is_process_patching_operation
        ProcessHandler.is_process_patching_operation = self.mock_is_process_patching_operation_to_return_true
        os_kill_backup = os.kill
        os.kill = self.mock_os_kill_to_raise_exception

        # error in terminating process
        pid = 123
        process_handler = ProcessHandler(self.logger, self.env_layer, self.ext_output_status_handler)
        self.assertRaises(OSError, process_handler.kill_process, pid)

        # reseting mocks
        ProcessHandler.is_process_running = is_process_running_backup
        ProcessHandler.is_process_patching_operation = is_process_patching_operation_backup
        os.kill = os_kill_backup

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/extension/src/ProcessHandler.py:206

/proc/<pid>/cmdline is a null-separated byte stream; opening it in text mode can introduce decoding differences between Python 2/3 and locales. Reading it as bytes and comparing against a UTF-8 encoded core filename keeps the check deterministic.

            with self.env_layer.file_system.open("/proc/{0}/cmdline".format(str(pid)), mode="r") as cmdline_file:
                cmdline = cmdline_file.read()
                if Constants.CORE_CODE_FILE_NAME in cmdline:

src/extension/src/ProcessHandler.py:214

Returning True on exceptions can still terminate an unrelated process in the exact scenarios this PR is trying to avoid (e.g., cmdline read fails due to transient /proc races or permissions). If the cmdline cannot be read, treat it as "not a patching operation" so kill_process will skip termination.

        except Exception as error:
            self.logger.log_debug("Error checking if process is a patching operation. [PID={0}] [Error={1}]".format(str(pid), repr(error)))
            return True  # If we cannot determine, assume it is a patching operation to be safe

yashnap · 2026-07-23T21:24:25Z

This UT class is a good reference for your new UT (mocks) : https://github.com/Azure/LinuxPatchExtension/blob/master/src/core/tests/Test_LifecycleManagerAzure.py

yashnap · 2026-07-23T21:46:31Z

+                    return True
+                else:
+                    self.logger.log_debug("Process is not a patching operation. [PID={0}]".format(str(pid)))
+                    return False


Similar comment to use log_verbose

Please leave as log_debug and include what it actually shows up as instead of our operation. This demonstrates if this actually fixed or changed any outcome at scale.

@kjohn-msft
+1 to log what it actually shows up as.

But I think this should be log_verbose than log_debug. log_debug doesn't show up in telemetry so we will have to depend on getting log files from the machine. Having telemetry will be easier to validate in case we see this again

@kjohn-msft @michellemcdaniel re-opening this comment to check on whether this should be log_debug or log_verbose. log_debug won't show up in telemetry

rane-rajasi

Comments inline

rane-rajasi · 2026-07-24T15:44:26Z

+                    return True
+                else:
+                    self.logger.log_debug("Process is not a patching operation. [PID={0}]".format(str(pid)))
+                    return False


@kjohn-msft
+1 to log what it actually shows up as.

But I think this should be log_verbose than log_debug. log_debug doesn't show up in telemetry so we will have to depend on getting log files from the machine. Having telemetry will be easier to validate in case we see this again

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/extension/src/ProcessHandler.py:210

is_process_patching_operation() decodes /proc/<pid>/cmdline as UTF-8 and logs the entire cmdline when it’s not a patching operation. Two problems: (1) cmdline may contain non-UTF8 bytes; a decode error is caught and currently returns True, which can still terminate an unrelated process and defeats the purpose of this PR. (2) logging full cmdlines can leak sensitive args/paths and is also emitted to telemetry via log_debug.

Consider checking for CORE_CODE_FILE_NAME in the raw bytes (or null-split argv) without decoding, avoid logging the full cmdline, and if cmdline cannot be read, treat it as "not a patching operation" (skip termination).

            with self.env_layer.file_system.open("/proc/{0}/cmdline".format(str(pid)), mode="rb") as cmdline_file:
                cmdline = cmdline_file.read()
                cmdline = cmdline.replace(b'\x00', b' ').decode("utf-8")  # cmdline is null-separated, so replacing nulls with spaces for easier parsing and logging
                
                if Constants.CORE_CODE_FILE_NAME in cmdline:

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

src/extension/src/ProcessHandler.py:210

is_process_patching_operation currently decodes /proc/<pid>/cmdline as UTF-8 (which can raise) and returns True on any exception. Returning True on read/decode errors can still allow terminating unrelated processes (the scenario this PR is trying to avoid). Consider checking for the core filename in the raw cmdline bytes (no decoding needed) and return False when cmdline can't be confidently verified.

            with self.env_layer.file_system.open("/proc/{0}/cmdline".format(str(pid)), mode="rb") as cmdline_file:
                cmdline = cmdline_file.read()
                cmdline = cmdline.replace(b'\x00', b' ').decode("utf-8")  # cmdline is null-separated, so replacing nulls with spaces for easier parsing and logging
                
                if Constants.CORE_CODE_FILE_NAME in cmdline:

src/extension/tests/Test_ProcessHandler.py:50

tempfile.mkdtemp() creates a directory that isn't cleaned up in tearDown, which can leak temp dirs across test runs. Prefer TemporaryDirectory() (or explicit cleanup) so resources are always released.

        self.test_dir = tempfile.mkdtemp()
        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)

    def tearDown(self):
        VirtualTerminal().print_lowlight("\n----------------- tear down test runner -----------------")
        self.process.kill()

src/extension/tests/Test_ProcessHandler.py:149

This test monkey-patches ProcessHandler and os.kill but restores them only at the end of the method. If an assertion fails or an unexpected exception is raised, the patches can leak into other tests. Wrap the patched section in try/finally so restoration is guaranteed.

        # setting mocks
        is_process_running_backup = ProcessHandler.is_process_running
        ProcessHandler.is_process_running = self.mock_is_process_running_to_return_true
        is_process_patching_operation_backup = ProcessHandler.is_process_patching_operation
        ProcessHandler.is_process_patching_operation = self.mock_is_process_patching_operation_to_return_true
        os_kill_backup = os.kill
        os.kill = self.mock_os_kill_to_raise_exception

+        # setting mocks
+        backup_file_system_open = self.env_layer.file_system.open
+        self.env_layer.file_system.open = self.mock_file_system_open_to_return_proc_cmdline
+


Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (5)

src/extension/src/ProcessHandler.py:209

cmdline is decoded with strict UTF-8. If /proc/<pid>/cmdline contains non-UTF8 bytes, this will raise and fall into the exception path, bypassing the normal decision logic. Decode with errors="replace" (or avoid decoding for the core-file check).

            with self.env_layer.file_system.open("/proc/{0}/cmdline".format(str(pid)), mode="rb") as cmdline_file:
                cmdline = cmdline_file.read()
                cmdline = cmdline.replace(b'\x00', b' ').decode("utf-8")  # cmdline is null-separated, so replacing nulls with spaces for easier parsing and logging

src/extension/src/ProcessHandler.py:218

On any exception reading /proc/<pid>/cmdline, this returns True, which can still terminate an unrelated process (e.g., cmdline file missing due to races/permissions). For the PR’s safety goal, failure to verify should default to not treating the PID as a patching operation.

        except Exception as error:
            self.logger.log_debug("Error checking if process is a patching operation. [PID={0}] [Error={1}]".format(str(pid), repr(error)))
            return True  # If we cannot determine, assume it is a patching operation to be safe

src/extension/tests/Test_ProcessHandler.py:151

This test monkey-patches globals/classes without guaranteed restoration if an assertion fails, which can cascade failures into later tests. Also, the updated kill_process gating should be covered for the non-patching path (ensure os.kill is not invoked when is_process_patching_operation is False). Using addCleanup plus a non-patching sub-case makes the test robust and covers the new behavior.

        # setting mocks
        is_process_running_backup = ProcessHandler.is_process_running
        ProcessHandler.is_process_running = self.mock_is_process_running_to_return_true
        is_process_patching_operation_backup = ProcessHandler.is_process_patching_operation
        ProcessHandler.is_process_patching_operation = self.mock_is_process_patching_operation_to_return_true
        os_kill_backup = os.kill
        os.kill = self.mock_os_kill_to_raise_exception

src/extension/tests/Test_ProcessHandler.py:228

This test overrides self.env_layer.file_system.open but only restores it at the end of the test; if an earlier assertion fails, the mock can leak into later tests. Register the restoration with addCleanup immediately after patching.

        # setting mocks
        backup_file_system_open = self.env_layer.file_system.open
        self.env_layer.file_system.open = self.mock_file_system_open_to_return_proc_cmdline

src/extension/tests/Test_ProcessHandler.py:255

If is_process_patching_operation cannot read /proc/<pid>/cmdline, the safest behavior for this PR is to treat it as not a patching operation (to avoid terminating an unrelated reused PID). The test currently asserts True for the exception case, which would preserve the risky behavior.

        # process cmdline exception
        self.env_layer.file_system.open = self.mock_file_system_open_raises_exception

        process_handler = ProcessHandler(self.logger, self.env_layer, self.ext_output_status_handler)
        pid = 1234

        self.assertTrue(process_handler.is_process_patching_operation(pid))

+        self.test_dir = tempfile.mkdtemp()
+        self.proc_cmdline_path = os.path.join(self.test_dir, "proc_cmdline")


Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

src/extension/src/ProcessHandler.py:218

is_process_patching_operation returns True on any exception (including /proc/<pid>/cmdline races/permissions or UTF-8 decode errors), which can still allow terminating an unrelated process—the scenario this PR is trying to prevent. Prefer decoding defensively and returning False when the cmdline cannot be confidently verified.

        except Exception as error:
            self.logger.log_debug("Error checking if process is a patching operation. [PID={0}] [Error={1}]".format(str(pid), repr(error)))
            return True  # If we cannot determine, assume it is a patching operation to be safe

src/extension/tests/Test_ProcessHandler.py:153

These monkey-patches are restored only at the end of the test; if an assertion fails (or an unexpected exception is raised), the patches can leak into later tests and cause cascading failures. Register cleanup (or use try/finally) so patches are always restored.

    def test_kill_process(self):
        # setting mocks
        is_process_running_backup = ProcessHandler.is_process_running
        ProcessHandler.is_process_running = self.mock_is_process_running_to_return_true
        is_process_patching_operation_backup = ProcessHandler.is_process_patching_operation
        ProcessHandler.is_process_patching_operation = self.mock_is_process_patching_operation_to_return_true
        os_kill_backup = os.kill
        os.kill = self.mock_os_kill_to_raise_exception

src/extension/tests/Test_ProcessHandler.py:230

env_layer.file_system.open is monkey-patched here, but restoration relies on the final lines of the test executing. If an assertion fails earlier, the patch can leak. Register a cleanup callback immediately after taking the backup.

        # setting mocks
        backup_file_system_open = self.env_layer.file_system.open
        self.env_layer.file_system.open = self.mock_file_system_open_to_return_proc_cmdline

+    def mock_file_system_open_raises_exception(self, path, mode):
+        raise FileNotFoundError
+


+        process_handler = ProcessHandler(self.logger, self.env_layer, self.ext_output_status_handler)
+        pid = 1234
+
+        self.assertTrue(process_handler.is_process_patching_operation(pid))


rane-rajasi

Comments inline + one earlier comment re-opened

rane-rajasi · 2026-07-27T11:42:24Z

+            f.write(is_not_patching_operation_content)
+
+        process_handler = ProcessHandler(self.logger, self.env_layer, self.ext_output_status_handler)
+        pid = 1234


Do you need to initialize ProcessHandler again or would simply calling is_process_patching_operation() work

pid is also the same as before, could be reused between both cases

rane-rajasi · 2026-07-27T11:43:10Z

+        self.env_layer.file_system.open = self.mock_file_system_open_raises_exception
+
+        process_handler = ProcessHandler(self.logger, self.env_layer, self.ext_output_status_handler)
+        pid = 1234


Same comment as above

rane-rajasi · 2026-07-27T11:47:47Z

+        self.assertTrue(process_handler.is_process_patching_operation(pid))
+
+        # resetting mocks
+        self.env_layer.file_system.open = backup_file_system_open


For one test with multiple scenarios, LPE generally uses a pattern to reduce repetitive lines of code. Refer: test_get_package_manager() in core/tests/Test_EnvLayer.py or test_is_cert_update_supported__with_various_use_cases() in core/src/Test_AptitudePackageManager.py

There's more such examples, sharing these 2.

michellemcdaniel requested review from kjohn-msft, najams and rane-rajasi as code owners July 23, 2026 20:39

Copilot AI review requested due to automatic review settings July 23, 2026 20:39

Copilot started reviewing on behalf of michellemcdaniel July 23, 2026 20:40 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread src/extension/tests/Test_ProcessHandler.py

Comment thread src/extension/src/ProcessHandler.py

fix test

7ca6ffd

Copilot AI review requested due to automatic review settings July 23, 2026 20:56

Copilot started reviewing on behalf of michellemcdaniel July 23, 2026 20:56 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread src/extension/tests/Test_ProcessHandler.py

yashnap reviewed Jul 23, 2026

View reviewed changes

Comment thread src/extension/tests/Test_ProcessHandler.py

yashnap reviewed Jul 23, 2026

View reviewed changes

Comment thread src/extension/src/ProcessHandler.py

yashnap reviewed Jul 23, 2026

View reviewed changes

Comment thread src/extension/src/ProcessHandler.py Outdated

yashnap reviewed Jul 23, 2026

View reviewed changes

rane-rajasi requested changes Jul 24, 2026

View reviewed changes

Address feedback

0761c2f

Copilot AI review requested due to automatic review settings July 24, 2026 16:20

Copilot AI reviewed Jul 24, 2026

Add tests for is_process_patching_operation

8a4474e

Copilot AI review requested due to automatic review settings July 24, 2026 17:32

Copilot started reviewing on behalf of michellemcdaniel July 24, 2026 17:32 View session

Use temp location

2781504

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread src/extension/tests/Test_ProcessHandler.py

Copilot AI review requested due to automatic review settings July 24, 2026 17:36

Copilot started reviewing on behalf of michellemcdaniel July 24, 2026 17:38 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread src/extension/tests/Test_ProcessHandler.py

Comment on lines +222 to +225

# setting mocks

backup_file_system_open = self.env_layer.file_system.open

self.env_layer.file_system.open = self.mock_file_system_open_to_return_proc_cmdline

add final test for except path

defbdf8

Copilot AI review requested due to automatic review settings July 24, 2026 17:59

add comment

a35d0a1

Copilot started reviewing on behalf of michellemcdaniel July 24, 2026 18:00 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread src/extension/tests/Test_ProcessHandler.py

Comment on lines +43 to +44

self.test_dir = tempfile.mkdtemp()

self.proc_cmdline_path = os.path.join(self.test_dir, "proc_cmdline")

cleanup test files

211de45

Copilot AI review requested due to automatic review settings July 24, 2026 18:09

Copilot started reviewing on behalf of michellemcdaniel July 24, 2026 18:10 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

rane-rajasi requested changes Jul 27, 2026

View reviewed changes

		self.test_dir = tempfile.mkdtemp()
		self.proc_cmdline_path = os.path.join(self.test_dir, "proc_cmdline")

		def mock_file_system_open_raises_exception(self, path, mode):
		raise FileNotFoundError

Uh oh!

Conversation

michellemcdaniel commented Jul 23, 2026

Uh oh!

codecov Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

yashnap commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rane-rajasi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

rane-rajasi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

codecov Bot commented Jul 23, 2026 •

edited

Loading

yashnap commented Jul 23, 2026 •

edited

Loading