From 5bfa17783a1f489d30cd6bbd287406d9609f6164 Mon Sep 17 00:00:00 2001 From: Chris Burr Date: Mon, 19 Oct 2026 13:07:46 +0200 Subject: [PATCH 1/2] fix (RMS): stop the ReqProxy from dropping the operations of cached requests Request.__init__ and Operation.__init__ deleted the "Operations"/"Files" key from the dict they were given. Since c467386246 the ReqProxy saves the dict it passed to Request() rather than the original JSON, so every request forwarded through a ReqProxy was stored without any operations. (cherry picked from commit ad9fac60a5ed9178279c339d9fd0704e33a2efc3) --- .../Client/Operation.py | 10 +++--- .../RequestManagementSystem/Client/Request.py | 10 +++--- .../Client/test/Test_Operation.py | 11 +++++++ .../Client/test/Test_Request.py | 14 ++++++++ .../Service/ReqProxyHandler.py | 3 +- .../Service/test/Test_ReqProxyHandler.py | 32 +++++++++++++++++++ 6 files changed, 67 insertions(+), 13 deletions(-) create mode 100644 src/DIRAC/RequestManagementSystem/Service/test/Test_ReqProxyHandler.py diff --git a/src/DIRAC/RequestManagementSystem/Client/Operation.py b/src/DIRAC/RequestManagementSystem/Client/Operation.py index 256599c9809..4a4a9dc046e 100644 --- a/src/DIRAC/RequestManagementSystem/Client/Operation.py +++ b/src/DIRAC/RequestManagementSystem/Client/Operation.py @@ -99,14 +99,12 @@ def __init__(self, fromDict=None): elif not isinstance(fromDict, dict): fromDict = {} - if "Files" in fromDict: - for fileDict in fromDict.get("Files", []): - self.addFile(File(fileDict)) - - del fromDict["Files"] + for fileDict in fromDict.get("Files", []): + self.addFile(File(fileDict)) + # Don't modify fromDict, callers may still be using it for key, value in fromDict.items(): - if value: + if key != "Files" and value: setattr(self, key, value) # # protected methods for parent only diff --git a/src/DIRAC/RequestManagementSystem/Client/Request.py b/src/DIRAC/RequestManagementSystem/Client/Request.py index e7c2377c320..921bdbaaf15 100644 --- a/src/DIRAC/RequestManagementSystem/Client/Request.py +++ b/src/DIRAC/RequestManagementSystem/Client/Request.py @@ -92,14 +92,12 @@ def __init__(self, fromDict=None): elif not isinstance(fromDict, dict): fromDict = {} - if "Operations" in fromDict: - for opDict in fromDict.get("Operations", []): - self += Operation(opDict) - - del fromDict["Operations"] + for opDict in fromDict.get("Operations", []): + self += Operation(opDict) + # Don't modify fromDict, callers may still be using it for key, value in fromDict.items(): - if value: + if key != "Operations" and value: setattr(self, key, value) self._notify() diff --git a/src/DIRAC/RequestManagementSystem/Client/test/Test_Operation.py b/src/DIRAC/RequestManagementSystem/Client/test/Test_Operation.py index c92e40ba4ab..479a8680959 100644 --- a/src/DIRAC/RequestManagementSystem/Client/test/Test_Operation.py +++ b/src/DIRAC/RequestManagementSystem/Client/test/Test_Operation.py @@ -7,6 +7,8 @@ Operation test cases """ +import copy + import pytest from DIRAC.RequestManagementSystem.Client.File import File @@ -132,3 +134,12 @@ def test_List(): # opID set op.OperationID = 1 del op[0] + + +def test_ctorDoesNotModifyInput(): + """c'tor must not modify the dict it is given""" + opDict = {"Type": "ReplicateAndRegister", "TargetSE": "CERN-USER", "Files": [{"LFN": "/a/b/c"}]} + expected = copy.deepcopy(opDict) + operation = Operation(opDict) + assert opDict == expected + assert len(operation) == 1 diff --git a/src/DIRAC/RequestManagementSystem/Client/test/Test_Request.py b/src/DIRAC/RequestManagementSystem/Client/test/Test_Request.py index 86b929fa466..dcb790b4fb4 100644 --- a/src/DIRAC/RequestManagementSystem/Client/test/Test_Request.py +++ b/src/DIRAC/RequestManagementSystem/Client/test/Test_Request.py @@ -8,6 +8,7 @@ test cases for Request class """ +import copy import datetime from DIRAC.RequestManagementSystem.Client.Operation import Operation @@ -134,6 +135,19 @@ def test_CtorSerilization(): req = Request(fromJSON) +def test_CtorDoesNotModifyInput(): + """c'tor must not modify the dict it is given""" + reqDict = { + "RequestName": "test", + "Operations": [{"Type": "ReplicateAndRegister", "TargetSE": "CERN-USER", "Files": [{"LFN": "/a/b/c"}]}], + } + expected = copy.deepcopy(reqDict) + req = Request(reqDict) + assert reqDict == expected + assert len(req) == 1 + assert len(req[0]) == 1 + + def test_Props(): """props""" # # valid values diff --git a/src/DIRAC/RequestManagementSystem/Service/ReqProxyHandler.py b/src/DIRAC/RequestManagementSystem/Service/ReqProxyHandler.py index 595c2eb8a32..1eafe496185 100644 --- a/src/DIRAC/RequestManagementSystem/Service/ReqProxyHandler.py +++ b/src/DIRAC/RequestManagementSystem/Service/ReqProxyHandler.py @@ -178,7 +178,8 @@ def export_putRequest(self, requestJSON): gLogger.info("putRequest: got request", f"{requestName}") # We only need the object to check the authorization - request = Request(requestDict) + # Build it from the JSON rather than requestDict, as requestDict is what gets saved + request = Request(requestJSON) # Check whether the credentials in the Requests are correct and allowed to be set isAuthorized = RequestValidator.setAndCheckRequestOwner(request, self.getRemoteCredentials()) diff --git a/src/DIRAC/RequestManagementSystem/Service/test/Test_ReqProxyHandler.py b/src/DIRAC/RequestManagementSystem/Service/test/Test_ReqProxyHandler.py new file mode 100644 index 00000000000..7c36b6264b2 --- /dev/null +++ b/src/DIRAC/RequestManagementSystem/Service/test/Test_ReqProxyHandler.py @@ -0,0 +1,32 @@ +"""Tests for ReqProxyHandler""" +import json + +from DIRAC.RequestManagementSystem.Client.File import File +from DIRAC.RequestManagementSystem.Client.Operation import Operation +from DIRAC.RequestManagementSystem.Client.Request import Request +from DIRAC.RequestManagementSystem.Service import ReqProxyHandler as ReqProxyModule +from DIRAC.RequestManagementSystem.Service.ReqProxyHandler import ReqProxyHandler + + +def test_putRequestKeepsOperations(tmp_path, monkeypatch): + """The cached request must contain the same operations as the one which was received""" + monkeypatch.setattr(ReqProxyHandler, "cacheDir", classmethod(lambda cls: str(tmp_path))) + monkeypatch.setattr(ReqProxyModule.RequestValidator, "setAndCheckRequestOwner", lambda request, creds: True) + handler = object.__new__(ReqProxyHandler) + handler.getRemoteCredentials = lambda: {} + + request = Request({"RequestName": "test", "OwnerDN": "/DC=ch/CN=user", "OwnerGroup": "group"}) + op = Operation({"Type": "ReplicateAndRegister", "TargetSE": "CERN-USER"}) + op.addFile(File({"LFN": "/a/b/c"})) + request.addOperation(op) + request.addOperation(Operation({"Type": "ForwardDISET", "Arguments": "x"})) + + res = handler.export_putRequest(request.toJSON()["Value"]) + assert res["OK"], res + assert res["Value"] == {"set": False, "saved": True} + + (cachedFile,) = tmp_path.iterdir() + cached = Request(json.loads(cachedFile.read_text())) + assert [o.Type for o in cached] == ["ReplicateAndRegister", "ForwardDISET"] + assert [f.LFN for f in cached[0]] == ["/a/b/c"] + assert (cached.OwnerDN, cached.OwnerGroup) == ("/DC=ch/CN=user", "group") From f83aeffbbe5ceeed90a873d6f1afb92ecbf11691 Mon Sep 17 00:00:00 2001 From: Chris Burr Date: Mon, 19 Oct 2026 13:15:07 +0200 Subject: [PATCH 2/2] fix (RMS): delay requests without any operation instead of crashing on them When a Waiting request has no operation to execute (e.g. it has no operations at all), RequestTask crashed with "'NoneType' object has no attribute 'Type'" and the agent picked the request up again a few minutes later, forever. Set an error on the request and delay it by a day instead, so it stays available for recovery. (cherry picked from commit 42d3a88ae1d135cd97d694383cb301495ba69e22) --- .../private/RequestTask.py | 7 ++++++ .../private/test/Test_RequestTask.py | 22 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/DIRAC/RequestManagementSystem/private/RequestTask.py b/src/DIRAC/RequestManagementSystem/private/RequestTask.py index 97a5447d967..029a156b7e6 100644 --- a/src/DIRAC/RequestManagementSystem/private/RequestTask.py +++ b/src/DIRAC/RequestManagementSystem/private/RequestTask.py @@ -281,6 +281,13 @@ def __call__(self): self.log.error("Cannot get waiting operation", operation["Message"]) return operation operation = operation["Value"] + if operation is None: + # e.g. the request has no operations at all: there is nothing we can do with it, + # so keep it for a human to look at rather than picking it up again immediately + self.log.error("Request has no operation to execute, delaying it", self.request.RequestName) + self.request.Error = "Request has no operation to execute" + self.request.delayNextExecution(24 * 60) + return S_OK(self.request) self.log.info("executing operation", f"{operation.Type}") # # and handler for it diff --git a/src/DIRAC/RequestManagementSystem/private/test/Test_RequestTask.py b/src/DIRAC/RequestManagementSystem/private/test/Test_RequestTask.py index c995104c443..7d9d5e6b182 100644 --- a/src/DIRAC/RequestManagementSystem/private/test/Test_RequestTask.py +++ b/src/DIRAC/RequestManagementSystem/private/test/Test_RequestTask.py @@ -6,6 +6,7 @@ test cases for RequestTask class """ +import datetime import unittest import importlib from unittest.mock import Mock, MagicMock @@ -72,6 +73,27 @@ def testAPI(self): ret = self.task.setupProxy() print(ret) + def testNoOperation(self): + """a request without any operation is delayed instead of crashing""" + rt = importlib.import_module("DIRAC.RequestManagementSystem.private.RequestTask") + rt.Operations = self.mockOps + rt.CS = MagicMock() + + emptyReq = Request({"RequestName": "empty", "OwnerGroup": "lhcb_user", "Owner": "chaen"}) + self.task = RequestTask( + emptyReq.toJSON()["Value"], + self.handlersDict, + "csPath", + "RequestManagement/RequestExecutingAgent", + requestClient=self.mockRC, + ) + self.task.setupProxy = Mock(return_value={"OK": True, "Value": {"Shifter": []}}) + ret = self.task() + self.assertTrue(ret["OK"], ret) + self.assertEqual(ret["Value"].Status, "Waiting") + self.assertEqual(ret["Value"].Error, "Request has no operation to execute") + self.assertGreater(ret["Value"].NotBefore, datetime.datetime.utcnow() + datetime.timedelta(hours=23)) + # # tests execution if __name__ == "__main__":