fix(code-index): keep the reader charge on both publication tails - #1873
Merged
Merged
Conversation
Both text-artifact publication tails released the charge they already held and asked the process authority for the reader again. `reserve` re-samples measured RSS and refuses every admission above the 8 MiB pressure floor while the over-budget latch is set, and the reader budget is 256 MiB. Releasing a ledger charge does not lower measured RSS, so the release cannot clear that latch: an overlapping graph replay sitting on the watermark turns a finalized artifact into a generation that can never seat its owners. `ResidentMemoryReservationV1::transfer_component` renames a held charge and shrinks it under one lock, which asks for no admission. Both tails hand their charge to the reader that way. The clone successor now holds at least the reader budget so its tail has something to hand over; its working set is unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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
1 participant
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.
Ports the reservation shrink from #1840 onto master, on both publication tails, and leaves the rest of that PR out.
Both publication tails in
serving.rsreleased the build's resident-memory charge and then asked the ledger for a fresh reader charge.ProcessResidentMemoryV1::reserverefuses any request above the 8 MiB pressure floor while the over-budget latch is set, and releasing a ledger charge does not lower measured RSS, so the release could not clear the latch the new reservation needed. A finalized artifact then had owners that could never be installed. The ledger now hastransfer_component, which re-keys retained bytes from the build component to the reader component without a fresh admission, and both tails use it.begin_clone_successorcharges the larger of the successor and reader budgets so the second tail has enough to hand over; its working set is unchanged.transfer_keeps_retained_bytes_when_a_new_admission_cannotasserts both sides against one ledger state: a fresh reserve of the same bytes is refused withReservationCeilingand the transfer succeeds. The seated-owners assertion in the code-index serving tests now requires the reader charge to equal the reader budget exactly, which catches a shrink applied to one tail or one that keeps the build key.No in-process test reproduces the refusal at the call sites, because
reserve_resident_memory_up_tore-samples real process RSS on every reservation and overwrites synthetic pressure; the mechanism is argued from the code and pinned at the ledger.Left out from #1840: the dedicated worker runtime. It is compiled under
cfg(not(test)), so no test can fail without it; its own commit message says it hit the 360 s deadline when many registries shared few threads, which is the daemon's shape; and the worker already offloads heavy work throughspawn_blocking, so starving it needs some other component to block the shared runtime, which moving one victim does not address.🤖 Generated with Claude Code