From 4aa2c49a1ab793836ef28430b7025a10b5c3c65d Mon Sep 17 00:00:00 2001 From: Nacl-HCl Date: Thu, 5 Nov 2026 08:30:39 +0000 Subject: [PATCH] server: Write registry files atomically and ignore non-JSON entries LocalFileDescriptorStore wrote descriptors in place (open with "w", then serialize), so a crash or a full disk left an empty or truncated JSON file behind. __len__() and __iter__() also looked at every entry in the storage directory, so foreign entries (lost+found, .DS_Store) broke GET /shell-descriptors with a 500. The store now writes through a temp file plus os.replace(), like the SDK's LocalFileIdentifiableStore does since #553, and len() and iteration only consider .json files, like the SDK store since #507. Fixes #666 --- server/app/backend/local_file.py | 47 ++++++++++++++++++-------- server/test/backend/test_local_file.py | 45 +++++++++++++++++++++++- 2 files changed, 76 insertions(+), 16 deletions(-) diff --git a/server/app/backend/local_file.py b/server/app/backend/local_file.py index e9988d68d..2c834d9cb 100644 --- a/server/app/backend/local_file.py +++ b/server/app/backend/local_file.py @@ -9,6 +9,7 @@ import json import logging import os +import tempfile import threading import weakref from typing import Dict, Iterator, Type, Union @@ -99,6 +100,30 @@ def get_item(self, identifier: model.Identifier) -> _DESCRIPTOR_TYPE: except KeyError as e: raise KeyError("No Identifiable with id {} found in local file database".format(identifier)) from e + def _write_atomic(self, x: _DESCRIPTOR_TYPE) -> None: + """ + Serialize x to a temp file in the store directory, then atomically replace the final file. + + Using os.replace() (rename(2) on POSIX) ensures readers always see a complete file — never + a partially-written one from a crash or concurrent access mid-write. + """ + final_path = "{}/{}.json".format(self.directory_path, self._transform_id(x.id)) + tmp_fd, tmp_path = tempfile.mkstemp(dir=self.directory_path, suffix=".tmp") + try: + with os.fdopen(tmp_fd, "w") as tmp_file: + # Usually, we don't need to serialize the modelType, since during HTTP requests, we know exactly if + # this is an AASDescriptor or SubmodelDescriptor. However, here we cannot distinguish them, so to + # deserialize them successfully, we hack the `modelType` into the JSON. + serialized = json.loads(json.dumps(x, cls=jsonization.ServerAASToJsonEncoder)) + serialized["modelType"] = DESCRIPTOR_TYPE_TO_STRING[type(x)] + json.dump(serialized, tmp_file, indent=4) + os.replace(tmp_path, final_path) + # Catch all `Exception`s, as well as `KeyboardInterrupt` and `SystemExit` too, so the temp + # file is never left behind even if the process is being torn down: + except BaseException: + os.unlink(tmp_path) + raise + def add(self, x: _DESCRIPTOR_TYPE) -> None: """ Add a Descriptor object to the store @@ -108,15 +133,9 @@ def add(self, x: _DESCRIPTOR_TYPE) -> None: logger.debug("Adding object %s to Local File Store ...", repr(x)) if os.path.exists("{}/{}.json".format(self.directory_path, self._transform_id(x.id))): raise KeyError("Descriptor with id {} already exists in local file database".format(x.id)) - with open("{}/{}.json".format(self.directory_path, self._transform_id(x.id)), "w") as file: - # Usually, we don't need to serialize the modelType, since during HTTP requests, we know exactly if this - # is an AASDescriptor or SubmodelDescriptor. However, here we cannot distinguish them, so to deserialize - # them successfully, we hack the `modelType` into the JSON. - serialized = json.loads(json.dumps(x, cls=jsonization.ServerAASToJsonEncoder)) - serialized["modelType"] = DESCRIPTOR_TYPE_TO_STRING[type(x)] - json.dump(serialized, file, indent=4) - with self._object_cache_lock: - self._object_cache[x.id] = x + self._write_atomic(x) + with self._object_cache_lock: + self._object_cache[x.id] = x def commit(self, x: _DESCRIPTOR_TYPE) -> None: """ @@ -127,10 +146,7 @@ def commit(self, x: _DESCRIPTOR_TYPE) -> None: """ if not os.path.exists("{}/{}.json".format(self.directory_path, self._transform_id(x.id))): raise KeyError("No AAS Descriptor object with id {} exists in local file database".format(x.id)) - with open("{}/{}.json".format(self.directory_path, self._transform_id(x.id)), "w") as file: - serialized = json.loads(json.dumps(x, cls=jsonization.ServerAASToJsonEncoder)) - serialized["modelType"] = DESCRIPTOR_TYPE_TO_STRING[type(x)] - json.dump(serialized, file, indent=4) + self._write_atomic(x) def discard(self, x: _DESCRIPTOR_TYPE) -> None: """ @@ -172,7 +188,7 @@ def __len__(self) -> int: :return: The number of objects (determined from the number of documents) """ logger.debug("Fetching number of documents from database ...") - return len(os.listdir(self.directory_path)) + return sum(1 for f in os.listdir(self.directory_path) if f.lower().endswith(".json")) def __iter__(self) -> Iterator[_DESCRIPTOR_TYPE]: """ @@ -183,7 +199,8 @@ def __iter__(self) -> Iterator[_DESCRIPTOR_TYPE]: """ logger.debug("Iterating over objects in database ...") for name in os.listdir(self.directory_path): - yield self.get_descriptor_by_hash(name.rstrip(".json")) + if name.lower().endswith(".json"): + yield self.get_descriptor_by_hash(name[:-5]) @staticmethod def _transform_id(identifier: model.Identifier) -> str: diff --git a/server/test/backend/test_local_file.py b/server/test/backend/test_local_file.py index 93788f113..3f4fceeca 100644 --- a/server/test/backend/test_local_file.py +++ b/server/test/backend/test_local_file.py @@ -6,7 +6,7 @@ # SPDX-License-Identifier: MIT import os.path import shutil -from unittest import TestCase +from unittest import TestCase, mock from app import model from app.backend import local_file @@ -71,6 +71,49 @@ def test_iterating(self) -> None: self.assertIn(self.aasd1, retrieved_descriptor_store) self.assertIn(self.aasd2, retrieved_descriptor_store) + def test_len_and_iter_ignore_non_json_entries(self) -> None: + self.descriptor_store.add(self.sd1) + self.descriptor_store.add(self.aasd1) + + # Foreign entries in the storage directory (e.g. lost+found at the root of an ext4 volume, .DS_Store) + # must be ignored by len() and __iter__() + os.mkdir(os.path.join(store_path, "lost+found")) + with open(os.path.join(store_path, ".DS_Store"), "w") as file: + file.write("not json") + self.assertEqual(2, len(self.descriptor_store)) + + retrieved_descriptor_store = provider.DictDescriptorStore() + for item in self.descriptor_store: + retrieved_descriptor_store.add(item) + self.assertEqual(2, len(retrieved_descriptor_store)) + self.assertIn(self.sd1, retrieved_descriptor_store) + self.assertIn(self.aasd1, retrieved_descriptor_store) + + def test_failed_commit_keeps_previous_file(self) -> None: + self.descriptor_store.add(self.sd1) + stored_files = [f for f in os.listdir(store_path) if f.endswith(".json")] + self.assertEqual(1, len(stored_files)) + file_path = os.path.join(store_path, stored_files[0]) + with open(file_path, "r") as file: + content_before = file.read() + + # Simulate a failure during serialization (e.g. crash or full disk): the previously stored file must + # stay intact instead of being truncated + with mock.patch("app.backend.local_file.json.dump", side_effect=RuntimeError("simulated failure")): + with self.assertRaises(RuntimeError): + self.descriptor_store.commit(self.sd1) + with open(file_path, "r") as file: + self.assertEqual(content_before, file.read()) + # No temp file must be left behind + self.assertEqual([], [f for f in os.listdir(store_path) if f.endswith(".tmp")]) + + def test_failed_add_leaves_no_file(self) -> None: + with mock.patch("app.backend.local_file.json.dump", side_effect=RuntimeError("simulated failure")): + with self.assertRaises(RuntimeError): + self.descriptor_store.add(self.sd1) + self.assertNotIn(self.sd1, self.descriptor_store) + self.assertEqual([], os.listdir(store_path)) + def test_key_errors(self) -> None: self.descriptor_store.add(self.aasd1) with self.assertRaises(KeyError) as cm: