Exempt used-marked Lazy references from evaluator-driven clearing - #292
Merged
Conversation
Lazy.Default.clear(ClearingEvaluator) - the choke point of the LazyReferenceManager's periodic eviction - so far gated only on isStored() and a present subject. Usage marks set via UsageMarkable.markUsedFor were never consulted anywhere: a referent pinned as "in use" (e.g. because it carries changes that have not been persisted yet) could still be evicted, silently dropping that state. The guard now skips used-marked references before even asking the evaluator. Explicit clear()/forceClear() remain unconditional. This closes the eviction half of the GigaMap dirty-segment lost-update scenario: GigaMap pins mutated segments via markUsedFor and releases the pin after commit, but the pin was write-only until now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR prevents evaluator-driven eviction (via LazyReferenceManager / Lazy.Default.clear(ClearingEvaluator)) from clearing Lazy references that are currently usage-marked, ensuring mutated-but-not-yet-persisted referents are not silently dropped.
Changes:
- Update
Lazy.clear(ClearingEvaluator)contract and implementation so used-marked references are exempt from evaluator-driven clearing. - Document the enforced contract in
UsageMarkable.isUsed()Javadoc. - Add an integration regression test covering used-mark eviction guard semantics, per-marker tracking, and the fact that explicit
clear()remains unconditional.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
base/src/main/java/org/eclipse/serializer/reference/Lazy.java |
Implements and documents the “skip used-marked references” behavior for evaluator-driven clearing. |
base/src/main/java/org/eclipse/serializer/reference/UsageMarkable.java |
Documents isUsed() semantics in the context of evaluator-driven clearing. |
integration-tests/src/test/java/test/eclipse/serializer/reference/LazyUsedEvictionGuardTest.java |
Adds regression coverage for the new eviction guard behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Lazy.Default.clear(ClearingEvaluator) - the choke point of the LazyReferenceManager's periodic eviction - so far gated only on isStored() and a present subject. Usage marks set via UsageMarkable.markUsedFor were never consulted anywhere: a referent pinned as "in use" (e.g. because it carries changes that have not been persisted yet) could still be evicted, silently dropping that state. The guard now skips used-marked references before even asking the evaluator. Explicit clear()/forceClear() remain unconditional. This closes the eviction half of the GigaMap dirty-segment lost-update scenario: GigaMap pins mutated segments via markUsedFor and releases the pin after commit, but the pin was write-only until now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
isUsed() went through usageMarks(), whose lazy initialization ALLOCATES the marks collection on first call. With the new eviction guard, evaluator-driven clearing queries isUsed() for every stored+loaded reference in every check cycle - never-marked instances (the vast majority) would each have allocated a HashEnum in the very code path meant to relieve memory pressure. isUsed() now reads the volatile field directly: null means no mark was ever registered and answers false without any allocation; only actually-marked instances synchronize on the marks collection. The mark/query race characteristics are unchanged (the previous version had the same window after its isEmpty check).
…to fix/lazy-used-eviction-guard
Extends the isUsed() allocation fix to the remaining paths: unmarkUsedFor and markUnused short-circuit to 0 when no mark was ever registered, and accessUsageMarks hands the logic the shared immutable empty collection instead of materializing the instance's own marks storage - the contract is preserved (the logic can still notice the no-marks case; its parameter is the read-only XGettingEnum view). Only markUsedFor initializes the collection. Also documents the lock-order constraint on accessUsageMarks: the eviction guard introduces "instance monitor -> marks monitor" (clear(ClearingEvaluator) queries isUsed() under the Lazy monitor), so logic running under the marks monitor must not call back into synchronized methods of the marked instance - that inversion can deadlock. No such callers exist today.
zdenek-jonas
approved these changes
Jul 6, 2026
This was referenced Jul 7, 2026
Merged
fh-ms
added a commit
that referenced
this pull request
Jul 8, 2026
…s success (#296) * Couple healing commits' deferred Lazy $links to the outermost commit's success Fixes the #290/#294 cross-fix interaction of internal#82: the healing storer's compensating commit fired the deferred $link commit listeners of every Lazy it serialized, although the enclosing store's retried write could still fail terminally. A Lazy linked over durable-but- unreachable data is legally clearable (the #292 isUsed() guard covers only used-marked references); once the LazyReferenceManager clears it and the target's GC reclaims the data, a later re-store captures the cached id with a null instance - unhealable, permanent loss. The path arms with reference-validation = heal (store#742), so this must merge before or together with that PR. Healing storers now route registerCommitListener to the ROOT storer whose commit they compensate (transitive healing flattens to the same root): deferred effects fire exactly when the outermost commit succeeds. On success the healed subgraph's Lazies end up properly linked, as if the store had never been rejected; on terminal failure nothing was ever linked, so nothing becomes clearable and no data can be lost. Side effects of the design: - A healing commit never carries local listeners, so the related internal#82 finding - a post-durability listener fault masquerading as a healing failure and aborting a salvageable retry - is structurally unreachable (documented on notifyCommitListeners). - The healing commit's registry merge stays immediate. Its residue after a terminal failure (associations to unreachable healed entities) is benign and self-rescuing: registry knowledge alone makes no Lazy clearable, and a later store trusting such an id either finds the data still present or gets rejected and heals it again from the re-captured instance (documented on writeToTarget). Regression test HealingDeferredLinkTest drives the exact sequence through a scripted PersistenceTarget (reject -> healing write accepted -> retry fails/succeeds), which real storage cannot produce deterministically: the failed-retry case pins the Lazy staying unlinked and unclearable, the success case pins the transferred link firing with the outer commit. * Refactor BinaryStorer to Storer for commitListenerSink to improve type generality
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 fixes
Lazyreferences can be marked as "in use" via theUsageMarkableAPI — the mark's meaning is "the referent carries state that must not be dropped", e.g. modifications that have not been persisted yet. GigaMap uses exactly this: whenever a segment behind aLazyreference is mutated, the segment is pinned viamarkUsedFor, and the pin is released after the changes are committed.The problem: the mark was write-only.
isUsed()had no callers anywhere — no clearing path ever consulted it. TheLazyReferenceManager's periodic clearing decides purely onisStored()plus timeout/memory pressure, so it could evict a loaded-then-mutated-but-not-yet-stored referent. The change flags die with the evicted instance, the next store silently skips the segment, and the mutation is lost — while related non-lazy updates (index entries, size counters) DO get persisted, leaving permanently inconsistent data on disk with no exception anywhere.The fix wires the read side in: a used-marked
Lazyreference is never cleared by evaluator-driven eviction.Technical details
Lazy.Default.clear(ClearingEvaluator)— the single choke point of theLazyReferenceManager's periodic clearing — now skips used-marked references before even consulting the evaluator:isStored() && subject != null && !isUsed() && needsClearing(...).Checker, custom checks) uniformly. Anyone not callingmarkUsedForsees zero behavior change.clear()/forceClear()calls remain unconditional — they are imperative API; callers that combine explicit clearing with usage marks checkisUsed()themselves (see the paired store PR for GigaMap'srelease()).clear(...)holds theLazy.Defaultinstance monitor;isUsed()nests the distinctusageMarksmonitor inside it. That ordering is consistent repo-wide (no reverse nesting exists), so no new deadlock surface.Lazy.clear(ClearingEvaluator)andUsageMarkable.isUsed()documents the now-enforced contract.Test
LazyUsedEvictionGuardTest(public API only): an always-clear evaluator cannot clear a used-marked reference and can again once unmarked; marks are tracked per marker instance (the reference stays pinned while any mark remains); explicitclear()stays unconditional. Fails 2/3 without the guard.Pairing
The store-side counterpart (GigaMap: retain dirty segments in
release(), plus the end-to-end reproducer showing the silent lost update) is a separate PR — no compile or ordering dependency between the two; each closes one of the two eviction paths.