From 84e7ead465345ff687e875859f6ee7f20d6f3f65 Mon Sep 17 00:00:00 2001 From: YueGuobin Date: Tue, 15 Sep 2026 01:14:44 +0800 Subject: [PATCH] fix: make node working directory deletion robust The rmtree error handler in BaseNode.delete() was chmod'ing the failed path to S_IWRITE (0o200). On POSIX this strips the search permission from directories, turning a transient deletion failure into a directory that can no longer be traversed or deleted. The node deletion itself silently "succeeds" (rmtree gives up once its error handler returns) and a later project deletion then fails with EACCES. The handler also never retried the failed operation, so it did not help on Windows either (the platform it was written for). The transient failure exists in practice: a concurrent MD5 checksum computation caching its result in the node directory (e.g. a properties request racing the deletion) can recreate a file after rmtree has listed the directory, making the final rmdir fail with ENOTEMPTY. - add the missing user permissions instead of replacing the whole mode, and retry the failed unlink/rmdir - retry the whole deletion a few times to absorb files recreated while the directory is being deleted - raise a ComputeError when the directory cannot be fully deleted instead of failing silently --- gns3server/compute/base_node.py | 33 ++++++++- tests/compute/docker/test_docker_vm.py | 15 ++++- tests/compute/test_base_node.py | 92 ++++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 4 deletions(-) diff --git a/gns3server/compute/base_node.py b/gns3server/compute/base_node.py index 65b816e5c..30fa856c5 100644 --- a/gns3server/compute/base_node.py +++ b/gns3server/compute/base_node.py @@ -340,14 +340,43 @@ class BaseNode: """ def set_rw(operation, name, exc): - os.chmod(name, stat.S_IWRITE) + # Add the missing user permissions instead of replacing the whole mode: chmod'ing + # a directory to S_IWRITE removes the search permission on POSIX systems, making + # the directory impossible to traverse or to delete. Also note that S_IWRITE + # clears the read-only attribute on Windows. + try: + if os.path.isdir(name) and not os.path.islink(name): + os.chmod(name, os.stat(name).st_mode | stat.S_IRWXU) + elif not os.path.islink(name): + os.chmod(name, os.stat(name).st_mode | stat.S_IRUSR | stat.S_IWUSR) + # retry the failed operation now that the permissions are fixed + # (retrying os.scandir is not possible, the directory is handled by the + # retry loop below) + if operation in (os.unlink, os.rmdir): + operation(name) + except OSError: + pass directory = self.project.node_working_directory(self) - if os.path.exists(directory): + # Retry the deletion: a concurrent task (e.g. a MD5 checksum computation caching + # its result in the node directory) can recreate a file while the directory is + # being deleted, and shutil.rmtree silently gives up when its error handler returns + for attempt in range(3): + if not os.path.exists(directory): + return try: await wait_run_in_executor(shutil.rmtree, directory, onerror=set_rw) except OSError as e: raise ComputeError(f"Could not delete the node working directory: {e}") + if not os.path.exists(directory): + return + if attempt == 2: + raise ComputeError( + f"Could not delete the node working directory '{directory}': a file may have been " + "recreated in it or could not be removed" + ) + log.warning(f"Could not completely delete the node working directory '{directory}', retrying") + await asyncio.sleep(0.1) def start(self): """ diff --git a/tests/compute/docker/test_docker_vm.py b/tests/compute/docker/test_docker_vm.py index 51359b024..5ad4eb4f8 100644 --- a/tests/compute/docker/test_docker_vm.py +++ b/tests/compute/docker/test_docker_vm.py @@ -21,6 +21,7 @@ import pytest import pytest_asyncio import uuid import os +import shutil from types import SimpleNamespace from unittest.mock import patch @@ -2111,12 +2112,22 @@ async def test_close_reclaims_node_directory(vm, port_manager): @pytest.mark.asyncio async def test_delete_retries_after_reclaim(vm): + real_rmtree = shutil.rmtree + calls = 0 + + def rmtree_first_fails_then_deletes(directory, onerror=None, **kwargs): + nonlocal calls + calls += 1 + if calls == 1: + raise OSError("permission denied") + real_rmtree(directory, onerror=onerror) + with patch.object(vm, "close", new_callable=AsyncioMagicMock): with patch.object(vm, "_reclaim_directory_ownership", new_callable=AsyncioMagicMock, return_value=True) as mock_reclaim: # First rmtree hits the root-owned leftovers, the retry (after - # the reclaim) succeeds. + # the reclaim) succeeds and really deletes the directory. with patch("gns3server.compute.base_node.shutil.rmtree", - side_effect=[OSError("permission denied"), None]) as mock_rmtree: + side_effect=rmtree_first_fails_then_deletes) as mock_rmtree: await vm.delete() assert mock_rmtree.call_count == 2 mock_reclaim.assert_called_once_with(vm.working_dir) diff --git a/tests/compute/test_base_node.py b/tests/compute/test_base_node.py index 7eca945e1..6420d905b 100644 --- a/tests/compute/test_base_node.py +++ b/tests/compute/test_base_node.py @@ -16,6 +16,7 @@ # along with this program. If not, see . import os +import shutil from collections import OrderedDict import pytest @@ -24,6 +25,7 @@ import pytest_asyncio from tests.utils import asyncio_patch, AsyncioMagicMock from unittest.mock import patch, MagicMock +from gns3server.compute.compute_error import ComputeError from gns3server.compute.vpcs.vpcs_vm import VPCSVM from gns3server.compute.docker.docker_vm import DockerVM from gns3server.compute.error import NodeError @@ -507,3 +509,93 @@ async def test_console_websocket_client_disconnect_while_node_output_streams( "has disconnected from compute console WebSocket while node output" in r.message for r in caplog.records ) + + +@pytest.mark.asyncio +async def test_delete_node_working_directory(node): + + working_dir = node.working_dir + with open(os.path.join(working_dir, "test.txt"), "w") as f: + f.write("TEST") + await node.delete() + assert not os.path.exists(working_dir) + + +@pytest.mark.asyncio +async def test_delete_directory_without_user_permissions(node): + # regression test: a failed deletion must not chmod the directory to S_IWRITE (0o200), + # which removes the search permission and makes the directory undeletable + working_dir = node.working_dir + with open(os.path.join(working_dir, "test.txt"), "w") as f: + f.write("TEST") + os.chmod(working_dir, 0o200) + try: + await node.delete() + assert not os.path.exists(working_dir) + finally: + # restore the permissions so a failed test does not leave an undeletable + # directory behind on the shared project path + if os.path.exists(working_dir): + os.chmod(working_dir, 0o700) + + +@pytest.mark.asyncio +async def test_delete_directory_with_readonly_entries(node): + + working_dir = node.working_dir + with open(os.path.join(working_dir, "test.txt"), "w") as f: + f.write("TEST") + os.chmod(os.path.join(working_dir, "test.txt"), 0o000) + os.chmod(working_dir, 0o500) # remove the write permission + try: + await node.delete() + assert not os.path.exists(working_dir) + finally: + if os.path.exists(working_dir): + os.chmod(working_dir, 0o700) + + +@pytest.mark.asyncio +async def test_delete_directory_with_file_recreated_during_deletion(node, monkeypatch): + # regression test: a concurrent MD5 checksum computation can cache its result in the + # node directory while it is being deleted, recreating a file after shutil.rmtree + # has listed the directory (rmtree then silently gives up on the final rmdir) + working_dir = node.working_dir + with open(os.path.join(working_dir, "hda_disk_image.md5sum"), "w") as f: + f.write("0" * 32) + real_rmtree = shutil.rmtree + calls = 0 + + def rmtree_recreating_a_file(directory, onerror=None, **kwargs): + nonlocal calls + calls += 1 + if calls == 1: + # delete the entries but leave one behind, as if the final rmdir + # had failed with ENOTEMPTY after the error handler returned + for entry in os.listdir(directory): + os.remove(os.path.join(directory, entry)) + with open(os.path.join(directory, "hda_disk_image.md5sum"), "w") as f: + f.write("0" * 32) + return + real_rmtree(directory, onerror=onerror, **kwargs) + + monkeypatch.setattr("gns3server.compute.base_node.shutil.rmtree", rmtree_recreating_a_file) + await node.delete() + assert calls == 2 + assert not os.path.exists(working_dir) + + +@pytest.mark.asyncio +async def test_delete_directory_failure_raises(node, monkeypatch): + + working_dir = node.working_dir + with open(os.path.join(working_dir, "test.txt"), "w") as f: + f.write("TEST") + + def rmtree_not_deleting(directory, onerror=None, **kwargs): + pass # simulate a persistent deletion failure + + monkeypatch.setattr("gns3server.compute.base_node.shutil.rmtree", rmtree_not_deleting) + with pytest.raises(ComputeError): + await node.delete() + assert os.path.exists(working_dir)