Capture trusted reference object ids in BinaryStorer for target-side validation#293
Open
fh-ms wants to merge 2 commits into
Open
Capture trusted reference object ids in BinaryStorer for target-side validation#293fh-ms wants to merge 2 commits into
fh-ms wants to merge 2 commits into
Conversation
…validation A store's data can reference object ids whose entities are not part of the commit itself: instances skipped by the lazy storer because they are already known to the global object registry, and unloaded Lazy references' cached ids. The storer trusts that their data already exists in the persistence target; if that trust is wrong (GC-reclaimed entity, stale Lazy id after a failed commit, crash/restore artifacts), a dangling reference is committed silently and only surfaces after a later restart as "No entity found for objectId". This change makes those trusted ids available to the persistence target so it can validate their existence before committing the data: - PersistenceObjectIdRequestor.noteTrustedObjectId(long, Object): new default-no-op hook, fired from the global-registry-hit branch of PersistenceObjectManager.Default#ensureObjectId (the branch that skips registerLazyOptional). - PersistenceStoreHandler.noteTrustedReference(long): new default-no-op hook, fired by BinaryHandlerLazyDefault for an unloaded Lazy's cached id (case 3). - BinaryStorer collects the ids in a Set_long (OID range only; TIDs/CIDs filtered), prunes ids that are also stored by the same commit (an id that is both referenced and written is guaranteed, not trusted - e.g. storeAll(parent, child) or the eager storer), and attaches the result to the committed Binary. - Binary.setTrustedObjectIds/trustedObjectIds(): transient store-transport metadata, never part of the persisted data. - BinaryStorer.Creator gains a captureTrustedObjectIds flag; capturing is fully disabled (no allocation, no-op callbacks) when off. All API changes are additive (default methods, delegating constructor and factory overloads); other PersistenceTarget/PersistenceStoreHandler implementations are unaffected, capture defaults to off.
There was a problem hiding this comment.
Pull request overview
Adds an opt-in mechanism for capturing “trusted but not stored” object IDs during binary persistence commits, so persistence targets can validate those references before writing and surface potential dangling references at store time.
Changes:
- Introduces
PersistenceStoreHandler.noteTrustedReference(long)(default no-op) to let handlers report trusted references written directly to the binary stream. - Extends
BinaryStorer(creator + storers) to optionally collect, prune, and attach trusted object IDs to the outgoingBinaryat commit time. - Adds transient transport metadata (
trustedObjectIds) toBinaryand reports unloaded-Lazycached IDs fromBinaryHandlerLazyDefault.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| persistence/persistence/src/main/java/org/eclipse/serializer/persistence/types/PersistenceStoreHandler.java | Adds default hook for reporting trusted references written by handlers. |
| persistence/persistence/src/main/java/org/eclipse/serializer/persistence/types/PersistenceObjectManager.java | Documents trusted-id validation intent at skip-registration call site. |
| persistence/persistence/src/main/java/org/eclipse/serializer/persistence/types/PersistenceObjectIdRequestor.java | Documents optional recording of skipped IDs for later validation. |
| persistence/binary/src/main/java/org/eclipse/serializer/persistence/binary/types/BinaryStorer.java | Implements opt-in capture, pruning, and commit-time attachment; wires creator flag through storer types. |
| persistence/binary/src/main/java/org/eclipse/serializer/persistence/binary/types/Binary.java | Adds transient trustedObjectIds metadata with setter/getter for target-side validation. |
| persistence/binary/src/main/java/org/eclipse/serializer/persistence/binary/org/eclipse/serializer/reference/BinaryHandlerLazyDefault.java | Reports cached IDs for unloaded Lazy references via noteTrustedReference. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…scope - synchYieldTrustedObjectIds built its stored-object-id set with the default (minimal) slot size, causing repeated rehashing on commits with many stored items whenever capture is enabled. The set is now presized to the storer's item count (an upper bound of the item chain length). - Binary.setTrustedObjectIds/trustedObjectIds now document the transport contract explicitly: commit-scope metadata, present exactly once on the representative Binary instance passed to PersistenceTarget.write and covering the entire commit across all channels - consumers partition per channel themselves, per-channel chunk views deliberately do not carry it.
Comment on lines
+785
to
+787
| final Set_long storedObjectIds = Set_long.New( | ||
| XHashing.padHashLength(Math.max((int)this.itemCount, 1)) | ||
| ); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this enables
When the lazy storer serializes an object graph, it writes two kinds of references: ids of objects it stores in the same commit (guaranteed to exist afterwards), and ids it merely trusts - objects skipped because the persistence context already knows them, and cached ids of unloaded
Lazyreferences. If that trust is misplaced (data lost to a crash, a restore from a lagging backup, pre-existing corruption, or a stale cached id), the commit silently persists a reference to nothing. Nothing fails at that moment; the error surfaces at some later load or restart asStorageExceptionConsistency: No entity found for objectId N, with no hint of when the dangling reference was written.This PR makes the storer say which ids those are: it captures every trusted-but-not-stored object id of a commit and attaches the set to the written data. The paired store PR validates them against the entity cache inside the store task - atomically, before anything is written - and, depending on configuration, logs or rejects the store. Silent restart-time corruption becomes an immediate, diagnosable store-time signal.
The recently merged pinning of lazily-skipped instances PREVENTS the main runtime cause on the skip path. This validation is the complementary layer, and for one class of references it is the only mechanism-level protection: type handlers that write a cached object id directly without going through the storer's apply path -
BinaryHandlerLazyDefaultwriting an unloaded Lazy's cached id, and any custom handler with the same pattern - create nothing the pin could hold. For those, write-time validation is the actual closure, not merely detection.Technical details
registerSkippedOptional(the global-registry-hit hook the pinning fix introduced) also records the skipped id when capture is enabled;PersistenceStoreHandler.noteTrustedReference(long), fired fromBinaryHandlerLazyDefaultwhen it writes an unloaded Lazy's cached id.Set_longthat isnullwhen capture is disabled — the feature has zero overhead unless switched on.storer.storeAll(parent, child)and eager storers false-positive-free.trustedObjectIdsfield onBinary(set incommit()right beforetarget.write(...)), so no signature changes anywhere along the write path. Targets that do not know the field simply ignore it.captureTrustedObjectIdsflag on theBinaryStorer.Creatorfactory; all existing signatures delegate tofalse. Standalone serializer/communication usage is untouched.Scope and pairing
reference-validation = off | log | fail(defaultlog), with the full test suite (registry-trusted skips, unloaded-Lazy cached ids, multi-channel partitioning and rollback, post-failure recovery, GigaMap, batching storers).