Review invalidation is skipped when a stale change request is served from the storage cache

Symptom

A review that should have been invalidated silently stays valid.

When an approver posts a review on a change request, the previous review concerning the same approver must be marked as outdated. Occasionally it is not: both reviews remain valid. Because a valid approval is what unlocks publication, a change request can then be reported as ready for publication while one of its approvals should have been superseded, and the approver who reviewed last has no indication that their new review did not replace the old one.

This was caught by the functional test DelegateApproversIT#delegateApprovalAndReview failing on main build 834 at DelegateApproversIT.java:218: Buz approves on behalf of Bar, then Bar requests changes for himself, and Buz's delegated approval is expected to become outdated. The test screenshot attached to that build shows both reviews rendered as valid on a fully reloaded page, so this is a real behavioural failure and not a Selenium timing artifact. Builds 820 to 833 were green on this test and the only commit new in 834 (CRAPP-437) touches DefaultFileChangeStorageManager#mergeCreation only, so this is an intermittent defect rather than a regression.

Root cause

DefaultChangeRequestManager#addReview looks the previous review up in memory:

optionalLatestReview = changeRequest.getLatestReviewFromOrOnBehalfOf(reviewer);
this.reviewStorageManager.save(review);
if (optionalLatestReview.isPresent()) {   // silently does nothing when empty
    previousReview.setValid(false);
    ...
}

ChangeRequest#getLatestReviewFromOrOnBehalfOf only filters the in-memory review list; it never re-reads storage. That list comes from DefaultChangeRequestStorageManager#load, which is backed by ChangeRequestStorageCacheManager and, at the end of every load, unconditionally re-populates the cache:

this.reviewStorageManager.load(changeRequest);
result = Optional.of(changeRequest);
this.changeRequestStorageCacheManager.cacheChangeRequest(changeRequest);

Nothing coordinates that cache fill with the invalidations performed by ReviewXObjectUpdatedListener, ChangeRequestXObjectUpdatedListener, FileChangeXObjectUpdatedListener and DefaultChangeRequestStorageManager#save. Any load whose document read starts before a save but whose cacheChangeRequest call lands after the matching invalidation therefore leaves a stale ChangeRequest in the cache, and that instance is served to every later request until the next invalidation.

Concurrent loads of the same change request are normal: sub-requests rendering parts of the change request page, the change request handlers invoked with async=1, the notification pipeline resolving titles through ChangeRequestTitleCacheManager#loadTitle, and the invalidation listeners themselves, which call load before invalidating.

That produces the observed run:

• Buz posts his approval. The document is saved and the cache is invalidated.
• A concurrent reader that had read the document before that save calls cacheChangeRequest after the invalidation. The cache now holds a change request with no review.
• Bar opens the change request page. The stale instance is served. Nothing on that page view depends on the review list, so the staleness is invisible.
• Bar posts his review. loadChangeRequest returns the same stale instance, getLatestReviewFromOrOnBehalfOf returns empty, and the invalidation block is never entered. Buz's approval keeps valid=1 in the database.
• At the end of that request DefaultChangeRequestStorageManager#save invalidates the cache, so the following page render loads fresh from the document and shows two reviews, the older one still valid.

The stale instance is already gone by the time the page is inspected, which is why this cannot be reproduced afterwards and why it only appears on a loaded CI agent.

Ruled out, with evidence:

• Wrong branch in addReview treating Bar as reviewing on behalf of someone else: the preceding assertNull(reviewElement.getOriginalApprover()) passed, so the equality branch was taken.
• The save being skipped by the isMetaDataDirty() guard in DefaultReviewStorageManager#save: its "Trying to save a review without performing any change" error is logged at ERROR and never appears in the build console.
• A stale DOM read by the test: Bar's own review is displayed with a later timestamp, so the page was reloaded.

Why this matters beyond this test

The cache hands out ChangeRequest aggregates carrying status, reviews, file changes, authors, modified documents, title, description and dates. Every decision in the application is taken against that in-memory aggregate, and several are then written back wholesale, so a stale entry corrupts persisted state rather than only the display:

• Review invalidation on a new review - DefaultChangeRequestManager#addReview. The defect reported here.
• Merge eligibility and status - MergeApprovalStrategy#canBeMerged reads changeRequest.getReviews() in AbstractAllApproversMergeApprovalStrategy, FixedNumberApprovalsMergeApprovalStrategy and OnlyApprovedMergeApprovalStrategy. computeReadyForMergingStatus then persists the computed status. A stale review list can move a change request to "ready for publication" while an approval is actually missing, or leave it in "ready for review" after the last approval.
• Review invalidation on a new change - DefaultChangeRequestManager#invalidateReviews, called from FileChangeUpdatedListener, iterates the in-memory review list, so a short list leaves approvals valid across a new change.
• Review invalidation on approver removal - ApproversUpdatedListener#invalidateOutdatedReviews builds its map from changeRequest.getReviews(), so a removed approver's approval can survive.
• Whole-aggregate write-back - DefaultChangeRequestStorageManager#prepareChangeRequestDocument overwrites title, content, status, the changed-documents list and the authors list from the in-memory model. Saving a stale aggregate reverts a concurrently changed title, description or status, and can drop a modified document or an author, making a file change that still exists as an xobject invisible.
• Split - DefaultChangeRequestStorageManager#split clones changeRequest.getReviews() into the new change requests and then deletes the original, so a stale list loses reviews irrecoverably.
• Merge - merge iterates changeRequest.getFileChanges(), so a stale aggregate can publish an older version of a document, or skip one, while the change request is marked as merged.

Most of these leave the system in a self-consistent but wrong state, which is considerably harder to notice than a failing test.

Proposed fix

Two defects live in the same place and only the first is the race:

1. Close the read-then-cache race. Guard the insert in DefaultChangeRequestStorageManager#load with a per-identifier generation counter bumped by ChangeRequestStorageCacheManager#invalidate: capture the generation before reading the document and store the built instance only if the generation is unchanged. The remote invalidation path used by ChangeRequestEventsConverterHelper must bump it too, otherwise clustering regresses the way CRAPP-364 did.
2. Stop sharing a mutable aggregate. getChangeRequest returns the live instance and request threads mutate it, ChangeRequest#addReview writing into a plain LinkedList. Two concurrent requests on the same change request therefore mutate one unsynchronised list. This is independent of staleness and is not addressed by the generation counter; caching a defensive copy, or returning one from load, fixes it.

Independently of both, addReview should not depend on the cached model for correctness: looking the previous review up from storage makes the invalidation robust whatever the cache holds. A warning logged when an approver posts a review and no previous review is found would also have made this diagnosable from the build console alone, which it currently is not.

Note that the LRU size of the cache is not involved: eviction causes a correct reload.