Capture trusted reference object ids in BinaryStorer for target-side validation - #293
Conversation
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.
…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.
…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.
itemCount includes pin entries, which occupy hash slots but are never part of the item chain - on commits with many pinned references and few stored entities the previous presize over-allocated. The exact chain length is itemCount - pinCount (skip items are chained but rare; their exclusion from the set costs no rebuild).
81e1a63 to
6b9a583
Compare
zdenek-jonas
left a comment
There was a problem hiding this comment.
Reviewed: correct, additive, and off by default (captureTrustedObjectIds defaults to false; existing signatures delegate to false, standalone serializer/communication untouched). Verified the two capture points (registerSkippedOptional registry hits; BinaryHandlerLazyDefault in the referent == null && referenceOid != 0 unloaded-Lazy branch), the commit-time prune of also-stored ids, the OID-range filter, and thread-safety (trustedObjectIds add/clear/iterate all under objectRegistryMonitor; synchComplete() does not clear the item chain, so synchYieldTrustedObjectIds() prunes against an intact chain). No correctness issues found.
Two notes, not blockers:
- This PR is inert on its own — nothing reads
Binary.trustedObjectIds()without the paired store PR. Recommend merging together with (or immediately before) the store PR rather than standalone. - No serializer-side test: the capture/prune/filter logic is currently exercised only transitively by the store PR's suite. A serializer-level test (fake
PersistenceTargetassertingtrustedObjectIds()) is a sensible follow-up so serializer CI guards it independently. Agreed to add tests as a follow-up.
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).