fix: separate storage location accounting for internal cache holds - #20150
Conversation
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 7 of 7 changed files.
Validation: focused git diff --check passed. Builds and tests were not run.
This is an automated review by Codex GPT-5.6-Luna(max)
| currHoldBytes.updateAndGet(v -> Math.max(0L, v - holdDelta)); | ||
| } | ||
| // Same correction for internal holds | ||
| final long activeInternalHolds = weak.internalHolds.get(); |
There was a problem hiding this comment.
[P2] Internal hold bytes race with reservation resize
adjustReservation changes the entry size before reading internalHolds, while ReservationHold.close() updates internalHolds and subtracts entry.getSize() without the location lock. If a hold closes after resizeReservation but before this read, release subtracts the new size from the old contribution and this correction is skipped, leaving internalHoldBytes nonzero even though internalHoldCount is zero. Synchronize resize and hold accounting or use a consistent per-hold size snapshot, and add a concurrent resize/release test.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.
Reviewed 7 of 7 changed files.
Validation: focused git diff --no-ext-diff --check passed. Builds and tests were not run.
This is an automated review by Codex GPT-5.6-Luna(max)
cryptoe
left a comment
cryptoe
left a comment
There was a problem hiding this comment.
Changes look good to me.
3 participants
Description
Partially loaded segments use the same type of 'holds' a query places on cache entries for 'structural' reasons such as preventing metadata entry or parent bundle entries from being evicted, partial load rules also place holds to pin all entries that the rule requires be loaded, etc. This PR distinguishes these internal usages by adding methods to
StorageLocationwhich internal callers can use to influence how metrics are reported so that these internal usages do not count as cache hits and the like.