`LocalFileIdentifiableStore`: Fix concurrency issues by hpoeche · Pull Request #591 · eclipse-basyx/basyx-python-sdk

hpoeche · 2026-07-14T12:04:34Z

Previously the LocalFileIdentifiableStore had no access control for concurrent access through the same or multiple instances on the same directory.
This caused serious problems especially if this store is used as storage backend for a multi-worker WSGI server.

Issues with multiple instances

Multiple instances of the LocalFileIdentifiableStore were allowed to operate on the same directory. The main issue lies in the self._object_cache of each instance which does not get updated or invalidated if the other instance is modifying the underlying files through a write operation. In order for the caches to be coherent with the persistent content a inter-process cache synchronization would be necessary. This would introduce to much overhead so we decided to lock the access to a directory to a single instance of LocalFileIdentifiableStore . This also helps to simplify resolving the following issues.

Issues with concurrent calls to a single instance

Race condition on writes
Concurrent calls to writing methods add() and commit() currently race in the _write_atomic() method on the os.replace(...) call. Whenever two threads invoke the method with an identifiable that has the same ID but different content both calls succeed. However, the persisted content depends on which thread gets scheduled to execute the os.replace(...) last.
TOCTOU for writes
This occurs multiple times across the writing functions. For example in the add() function the time of check (TOC) for existence may be different that the time of use (TOU) where the new file is created. A concurrently executed add() might be scheduled such that at TOC both threads find no file for the ID so they both create a new file at TOU. Here again the last scheduled thread silently overwrites the content from the first.

Changes

In order to fix the multi-instance issue, these changes introduce a directory lock mechanism, that only allows one instance of the class to hold a lock on a .lock file in the directory. A context manager is used to secure operations on the directory and prevent concurrent release of the lock.
Caveat: For now this only work on POSIX machines, as it relies on flock functionality which is not provided by Windows.

To fix concurrent writes from the same store instance, all write accesses to a file need to be mutual exclusive.
In this implementation I chose to use a global _writing_lock to lock write operations to all files of the store, as locking individual files has no performance benefit, due to serialization of multiple threads through Pythons GIL.

This implementation prevents the following concurrency issues:

Two concurrent add(): Instead of silent overwriting, first call will second fails with KeyError.
commit() + discard(): Instead of commit() creating file after discard() deleted it, the file is now certainly discarded. Depending on the scheduled order commit() may raise a KeyError.

Fixes #554

Previously the LocalFileIdentifiableStore allowed multiple instances (potentially across multiple processes) to acces the same directory. This can lead to undetected invalid cache entries. Additionally it would increase the overhead for adding thread-safety to the R/W operations as synchronization across multiple instances would be necessary. In order to overcome these limitations, these changes introduce a directory lock mechanism, that only allows one instance of the class to hold a lock on a `.lock` file in the directory. A contex manager is used to secure operations on the directory and prevent concurrent release of the lock. For now this only work on POSIX machines, as it relies on `flock` functionatily which is not provided by Windows.

Until now the `LocalFileIdentifiableStore` had a TOCTOU race condition in the `add()` and `commit()` methods. After a file was checked for existance a concurrent running `add()` or `discard()` could still create or remove the file before the original invocation accesses it. To fix this, all write accesses to a file need to be mutual exclusive. In this implementation I chose to use a global `_writing_lock` to lock write operations to all files of the store, as locking individual files has no performance benefit, due to serialization of multiple threads through Pythons GIL. This implementation prevents the following concurrency issues: 1. Two concurrent `add()`: Instead of silent overwriting, first call will succeed, second failes with `KeyError`. 2. `commit()` + `discard()`: Instead of `commit()` creating file after `discard()` deleted it, the file is now certainly discarded.

s-heppner · 2026-07-27T13:36:06Z

@hpoeche could you update this branch with the changes from develop?

hpoeche · 2026-07-28T07:59:25Z

I resolved all merge conflicts and made sure ruff check passes on the files modified by me.

s-heppner

The DescriptorStore in server/app/backend/local_file.py needs the same mechanism. I'm fine with moving this into a separate issue.

The cache is now updated and read under the same `self._write_lock` as the files to prevent inconsitencies between actual file existance and cache entries.

Previously, systems not providing the fcntl library could run the LocalFileIdentifiableStore, only a warning would be logged but no concurrency control was available during runtime. With these changes, the standard behavior is to fail on `DirectoryLock` init with `RuntimeError` if fcntl is not available. Users can opt-out from this check via constructor parameters. Corresponding tests were added.

s-heppner

I'll mention it now here, so that we hopefully don't forget:
Failing by default on systems without fcntl is a breaking change for Windows users, so we need to mention it in the release notes.

s-heppner · 2026-10-05T11:54:07Z

+        with self._dir_lock.ensure_locked():
+            for name in os.listdir(self.directory_path):
+                if name.lower().endswith(".json"):
+                    yield self.get_identifiable_by_hash(name[:-5])


The ensure_locked() stays locked as long as the iterator is alive.
If someone stops iterating early, _active_accesses never drops back to 0, and close() / release() blocks until the iterator is garbage collected. If that happens in the same thread, it never returns:

it = iter(store) next(it) store.close() # hangs

I think this might be solved by moving the yield outside of the lock, but I'm not sure if that can cause other problems again:

with self._dir_lock.ensure_locked(): names = [name for name in os.listdir(self.directory_path) if name.lower().endswith(".json")] for name in names: yield self.get_identifiable_by_hash(name[:-5])

s-heppner · 2026-10-05T11:56:26Z

+        with self._dir_lock.ensure_locked():
+            try:
+                with open("{}/{}.json".format(self.directory_path, hash_), "r") as file:
+                    data = json.load(file, cls=json_deserialization.AASFromJsonDecoder)
+                    obj = data["data"]
+            except FileNotFoundError as e:
+                raise KeyError(


This is the same problem as my comment on add() / discard(), but when reading.
The file is read without _write_lock, and the cache is only updated afterwards. So this can happen:

Thread A reads the file and has obj in hand.

Thread B runs discard(): deletes the file and pops the cache entry.

Thread A takes _write_lock and puts obj back into the cache.

After that, get_item() returns an object whose file no longer exists. Can you move the file read under _write_lock too?

s-heppner · 2026-10-05T12:00:51Z

+            self._dir_lock_file = open(self.lock_path, "a")  # use "a" to not swap the inode as in "w"
+
+            if _fcntl is None:
+                if self._strict_locking:
+                    raise RuntimeError("fcntl unavailable, directory locking not possible."
+                                        "Set strict_locking=False to continue unsafe operation on this system.")
+
+                logger.warning("fcntl unavailable; directory locking is disabled (Windows?)")
+                self._is_locked_flag = True
+                return
+
+            try:
+                _fcntl.flock(self._dir_lock_file.fileno(), _fcntl.LOCK_EX | _fcntl.LOCK_NB)
+            except BlockingIOError:
+                # dir_lock already taken by other process
+                self._dir_lock_file.close()
+                self._dir_lock_file = None
+                raise LockedDirectoryError(f"Directory {self.directory_path} "
+                                           "is already locked by another DirectoryLock")


If the program encounters any other error than BlockingIOError, (for example a filesystem specific error), _dir_lock_file stays open. Can you add a second try that closes the file on any failure?

s-heppner · 2026-10-05T12:01:18Z

+                    raise RuntimeError("fcntl unavailable, directory locking not possible."
+                                        "Set strict_locking=False to continue unsafe operation on this system.")


Missing space:

Suggested change

raise RuntimeError("fcntl unavailable, directory locking not possible."

"Set strict_locking=False to continue unsafe operation on this system.")

raise RuntimeError("fcntl unavailable, directory locking not possible. "

"Set strict_locking=False to continue unsafe operation on this system.")

s-heppner · 2026-10-05T12:03:36Z

        return hashlib.sha256(identifier.encode("utf-8")).hexdigest()


 class LocalFileObjectStore(LocalFileIdentifiableStore):


One could argue that this class needs the same disable_strict_locking option as LocalFileIdentifiableStore.
On the other hand, Windows users using the old class need to touch it anyways, might be a good time to migrate to the new class. But that means, they'd need to know...

How would you solve this?

s-heppner · 2026-10-05T12:06:18Z

+        except LockedDirectoryError as ex:
+            raise RuntimeError(f"Directory {self.directory_path} is already in use"
+                               f" by another LocalFileIdentifiableStore instance.") from ex


We catch a more specific LockedDirectoryError (which is a subclass of RuntimeError) and raise a more generic RuntimeError. I say we raise the new error as LockedDirectoryError:

Suggested change

except LockedDirectoryError as ex:

raise RuntimeError(f"Directory {self.directory_path} is already in use"

f" by another LocalFileIdentifiableStore instance.") from ex

except LockedDirectoryError as ex:

raise LockedDirectoryError(f"Directory {self.directory_path} is already in use"

f" by another LocalFileIdentifiableStore instance.") from ex

s-heppner · 2026-10-05T12:06:47Z

 logger = logging.getLogger(__name__)

+class LockedDirectoryError(RuntimeError):
+    pass


Suggested change

logger = logging.getLogger(__name__)

class LockedDirectoryError(RuntimeError):

pass

logger = logging.getLogger(__name__)

class LockedDirectoryError(RuntimeError):

pass

hpoeche added 3 commits June 29, 2026 12:03

fix small bugs

f61b589

Merge branch 'develop' into fix/554

bceccfc

Fix: regorganize imports

d6cbb41

s-heppner requested changes Aug 11, 2026

View reviewed changes

hpoeche added 4 commits August 25, 2026 19:02

fix: same lock for cache and file update

1e739f8

The cache is now updated and read under the same `self._write_lock` as the files to prevent inconsitencies between actual file existance and cache entries.

fix: tighten OSError to BlockingIOError

242e44f

fix: module-level fcntl import

af71bee

hpoeche requested a review from s-heppner August 25, 2026 18:32

zrgt linked an issue Sep 3, 2026 that may be closed by this pull request

LocalFileIdentifiableStore: add per-object write locking to prevent silent data loss under concurrent writes #554

Open

s-heppner requested changes Oct 5, 2026

View reviewed changes

		raise RuntimeError("fcntl unavailable, directory locking not possible."
		"Set strict_locking=False to continue unsafe operation on this system.")

		return hashlib.sha256(identifier.encode("utf-8")).hexdigest()


		class LocalFileObjectStore(LocalFileIdentifiableStore):

Conversation

hpoeche commented Jul 14, 2026

Issues with multiple instances

Issues with concurrent calls to a single instance

Changes

Uh oh!

s-heppner commented Jul 27, 2026

Uh oh!

hpoeche commented Jul 28, 2026

Uh oh!

s-heppner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

s-heppner left a comment

Choose a reason for hiding this comment

Uh oh!

s-heppner Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

s-heppner Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

s-heppner Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

s-heppner Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

s-heppner Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

s-heppner Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

s-heppner Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants