impl: KVS Instance state registry [cpp] by karolnowak · Pull Request #538 · eclipse-score/persistency

karolnowak · 2026-10-20T09:38:40Z

Problem

The existing [cpp] impl allows to load the same instance from disk many times.
After loading, each KVS operates on its own non-shared entries.
This can affect data integrity when two KVSs, which refer to the same instance, save their entries on disk.

KvsBuilder builder(instance_id=1);
builder.build(); -> KVS with its own entries.*
builder.build(); -> another KVS with its own entries.*
builder.build(); -> yet another KVS with its own entries.*

*Even though all KVSs refer to the same instance.

Solution

A registry to share the same state of a given KVS Instance.

Note

At the moment, the registry implements the Singleton pattern and is hidden from the lib user.
- I don't think it is the best approach, however, the idea was to be aligned with the Rust impl.
I believe that the registry should be rewritten so that:
- It is available for the lib user.
- The lib user can create its own instance of the registry - No issue with the order of destruction.
- The lib user can specify how many kvs instances the registry can hold.

KvsRegistry registry(instance_count, ...);
registry.getOrOpen(params); -> `Kvs`

github-actions · 2026-10-20T09:42:30Z

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run --lockfile_mode=error //:license-check

Status: ⚠️ Needs Review

Click to expand output

[License Check Output]
Starting local Bazel server (8.7.0) and connecting to it...
INFO: Invocation ID: 11a2c17d-de65-4c9a-8131-412db5465dc6
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Loading: 
Loading: 2 packages loaded
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Analyzing: target //:license-check (3 packages loaded, 0 targets configured)
Analyzing: target //:license-check (3 packages loaded, 0 targets configured)

Analyzing: target //:license-check (87 packages loaded, 14 targets configured)

Analyzing: target //:license-check (99 packages loaded, 14 targets configured)

Analyzing: target //:license-check (100 packages loaded, 14 targets configured)

Analyzing: target //:license-check (100 packages loaded, 14 targets configured)

Analyzing: target //:license-check (150 packages loaded, 3520 targets configured)

Analyzing: target //:license-check (200 packages loaded, 6543 targets configured)

Analyzing: target //:license-check (207 packages loaded, 11305 targets configured)

Analyzing: target //:license-check (207 packages loaded, 11661 targets configured)

Analyzing: target //:license-check (207 packages loaded, 11661 targets configured)

Analyzing: target //:license-check (208 packages loaded, 11785 targets configured)

Analyzing: target //:license-check (211 packages loaded, 13670 targets configured)

INFO: Analyzed target //:license-check (213 packages loaded, 17903 targets configured).
[10 / 16] [Prepa] Expanding template external/score_tooling+/dash/tool/formatters/dash_format_converter [for tool]
INFO: From Generating Dash formatted dependency file ...:
INFO: Successfully converted 50 packages from Cargo.lock to bazel-out/k8-fastbuild/bin/tools/formatted.txt
[14 / 16] JavaToolchainCompileBootClasspath external/rules_java+/toolchains/platformclasspath.jar; 0s disk-cache, processwrapper-sandbox
INFO: Found 1 target...
Target //tools:license.check.license_check up-to-date:
  bazel-bin/tools/license.check.license_check
  bazel-bin/tools/license.check.license_check.jar
INFO: Elapsed time: 43.444s, Critical Path: 2.42s
INFO: 16 processes: 12 internal, 3 processwrapper-sandbox, 1 worker.
INFO: Build completed successfully, 16 total actions
INFO: Running command line: bazel-bin/tools/license.check.license_check tools/formatted.txt <args omitted>
[main] INFO Querying Eclipse Foundation for license data for 42 items.
[main] INFO Found 32 items.
[main] INFO Querying ClearlyDefined for license data for 10 items.
[main] INFO Found 10 items.
[main] INFO License information could not be automatically verified for the following content:
[main] INFO 
[main] INFO crate/cratesio/-/lazy_static/1.5.1
[main] INFO 
[main] INFO This content is either not correctly mapped by the system, or requires review.

github-actions · 2026-10-20T09:53:02Z

Documentation preview for this pull request is available at:
pr-538: https://eclipse-score.github.io/persistency/pr-538/

arkjedrz

Review is probably not exhaustive, but discussions might be needed at this points.

Ahmed-Elsaka-JC · 2026-11-02T10:30:44Z

+        }
+
+        kvs.state_ = entry;
+        return std::move(kvs);


here you are providing new instance for same storage :

each instance will has it's own mutex but both writing to same storage file .
what if both instances triggered Flush() ???
i would recommend to make builder returns shared pointer to kvs , and we can have multiple pointers to same kvs instance.

here you are providing new instance for same storage :

No, no new instance is created here. The "handle" to the existing instance is provided.

Considering your case, two Kvs objects will be created which will share the same instance, that said, there will be only one mutex as two objects will be using the mutex the given instance provides.

Kvs_1 Kvs_2 | | | | \ / \ / INSTANCE_1 | the mutex of the instance

then

Kvs_1.Flush() will use `INSTANCE_1.mutex` Kvs_2.Flush() will use `INSTANCE_1.mutex`

i would recommend to make builder returns shared pointer to kvs , and we can have multiple pointers to same kvs instance.

When you look at kvs.h file, you will find std::shared_ptr<KvsInstanceState> state_;. It is kind of what you are expecting. The builder return a new Kvs object that has the handle to Pimpl (or internal members that are not exposed).

Nevertheless, I agree with you regarding this that the builder should return the shared pointer to kvs - I came up with the same idea at first, however, I wanted to be aligned with the Rust impl. I would do it as a follow up task, I believe we should first refine the public interface so that we know what we want to provide: Kvs object or std::shared_ptr.

arkjedrz

Other reported issues:

score/kvs/docs/safety_analysis/dfa.rst:57, score/kvs/docs/safety_analysis/dfa.rst:104, coding-standards.yaml:125

The DFA row says "Function arguments only; no global data (comp_req__kvs__multi_instance)". The commit adds a process-wide mutable registry, so that justification is false, and the "Data per instance" row at line 104 needs the registry added.
The new kvs-instance-state-registry deviation covers only the static line. The registry's public mutable mutex and array members are not addressed.

+    }
+}
+
+bool KvsInstanceState::operator==(const KvsInstanceState& other) const noexcept


+           (max_storage_bytes == other.max_storage_bytes);
+}
+
+bool KvsInstanceState::operator!=(const KvsInstanceState& other) const noexcept


+/// Handles opened for the same filename prefix share this state, including the
+/// snapshot and storage limits. The storage limit applies to this store's files,
+/// not to storage used by other KVS instances.
+struct KvsInstanceState final


A registry to share the same state of a given KVS Instance. It avoids loading a given KVS Instance from disk again which could otherwise lead to the use of different versions of the same instance. Signed-off-by: Karol Nowak <Karol.Novak.Ext@qorix.ai>

karolnowak requested review from PandaeDo, antonkri, umaucher and vinodreddy-g as code owners October 20, 2026 09:38

karolnowak deployed to workflow-approval October 20, 2026 09:38 — with GitHub Actions Active

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from 3168f21 to f6be05e Compare October 20, 2026 09:43

karolnowak deployed to workflow-approval October 20, 2026 09:44 — with GitHub Actions Active

vinodreddy-g requested a review from arkjedrz October 20, 2026 11:28

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from f6be05e to cc7e450 Compare October 20, 2026 13:22

karolnowak deployed to workflow-approval October 20, 2026 13:22 — with GitHub Actions Active

Ahmed-Elsaka-JC suggested changes Oct 20, 2026

View reviewed changes

Comment thread score/kvs/internal/kvs_instance_state_registry.hpp Resolved

arkjedrz requested changes Oct 23, 2026

View reviewed changes

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from cc7e450 to e734e41 Compare October 26, 2026 10:45

karolnowak had a problem deploying to workflow-approval October 26, 2026 10:45 — with GitHub Actions Error

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from e734e41 to 614a402 Compare October 26, 2026 13:31

karolnowak had a problem deploying to workflow-approval October 26, 2026 13:31 — with GitHub Actions Error

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from 614a402 to 62b64f8 Compare October 26, 2026 14:18

karolnowak deployed to workflow-approval October 26, 2026 14:18 — with GitHub Actions Active

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from 62b64f8 to e30a0d8 Compare October 26, 2026 15:57

karolnowak had a problem deploying to workflow-approval October 26, 2026 15:57 — with GitHub Actions Error

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from e30a0d8 to 4ee69cc Compare October 26, 2026 16:13

karolnowak had a problem deploying to workflow-approval October 29, 2026 15:33 — with GitHub Actions Error

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from 8cb4da0 to c86f424 Compare October 29, 2026 15:40

karolnowak had a problem deploying to workflow-approval October 29, 2026 15:40 — with GitHub Actions Error

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from c86f424 to 8c87d3a Compare October 29, 2026 15:52

karolnowak had a problem deploying to workflow-approval October 29, 2026 15:52 — with GitHub Actions Error

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from 8c87d3a to f2984c1 Compare October 29, 2026 16:19

karolnowak had a problem deploying to workflow-approval October 29, 2026 16:19 — with GitHub Actions Error

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from f2984c1 to 6071260 Compare October 29, 2026 16:35

karolnowak had a problem deploying to workflow-approval October 29, 2026 16:35 — with GitHub Actions Error

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from 6071260 to 46aae14 Compare October 29, 2026 17:11

karolnowak had a problem deploying to workflow-approval October 29, 2026 17:12 — with GitHub Actions Error

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from 46aae14 to af49c4e Compare October 30, 2026 08:18

karolnowak had a problem deploying to workflow-approval October 30, 2026 08:18 — with GitHub Actions Error

karolnowak requested a deployment to workflow-approval October 30, 2026 08:18 — with GitHub Actions Waiting

arkjedrz mentioned this pull request Oct 30, 2026

test: inject filesystem, parser and writer into Kvs::Open #621

Draft

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from af49c4e to a72c746 Compare October 30, 2026 13:57

karolnowak had a problem deploying to workflow-approval October 30, 2026 13:57 — with GitHub Actions Error

Ahmed-Elsaka-JC reviewed Nov 2, 2026

View reviewed changes

github-advanced-security Bot found potential problems Nov 3, 2026

View reviewed changes

Comment thread score/kvs/internal/kvs_instance_state.cpp Fixed

Comment thread score/kvs/internal/kvs_instance_state.h Fixed

arkjedrz requested changes Nov 3, 2026

View reviewed changes

github-advanced-security Bot found potential problems Nov 3, 2026

View reviewed changes

Comment thread score/kvs/internal/kvs_instance_state.cpp Fixed

Comment thread score/kvs/internal/kvs_instance_state.h Fixed

github-advanced-security Bot found potential problems Nov 3, 2026

View reviewed changes

Comment thread score/kvs/internal/kvs_instance_state.h

/// Handles opened for the same filename prefix share this state, including the

/// snapshot and storage limits. The storage limit applies to this store's files,

/// not to storage used by other KVS instances.

struct KvsInstanceState final

Conversation

karolnowak commented Oct 20, 2026

Problem

Solution

Note

Uh oh!

github-actions Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

License Check Results

Uh oh!

github-actions Bot commented Oct 20, 2026

Uh oh!

Uh oh!

arkjedrz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ahmed-Elsaka-JC Nov 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karolnowak Nov 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arkjedrz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This branch is waiting to be deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Oct 20, 2026 •

edited

Loading

Ahmed-Elsaka-JC Nov 2, 2026 •

edited

Loading

karolnowak Nov 2, 2026 •

edited

Loading

arkjedrz left a comment •

edited

Loading