compute: assert the read hold and compaction invariants at their emission points - #38622
Draft
antiguru wants to merge 1 commit into
Draft
compute: assert the read hold and compaction invariants at their emission points#38622antiguru wants to merge 1 commit into
antiguru wants to merge 1 commit into
Conversation
…sion points Read holds and compaction have passed the point where a reviewer can convince themselves that a change to them is safe by reading it, and the failure mode is silently retiring data a reader still needs. This adds assertions over the state the controller already keeps, so that the next defect of that shape reports itself instead of surfacing as unbounded memory growth. The load-bearing part is which quantity the progress check names. An invariant phrased over a collection's own upper is vacuous, because that upper is derived from the first replica to report an advance: one replica sealing carries the whole collection with it, and every invariant statable about the recorded value still holds. The check therefore reconstructs an agreed upper, the meet of the write frontiers of the hosting replicas that have not retired, from the per-replica collection state, and requires the read policy applied to it to be less_equal the implied capability. That is a genuine invariant because read policies are monotone away from the empty upper and the agreed upper is always less_equal the recorded upper, so only a lattice-inverting step can violate it. A collection whose dataflow legitimately terminates passes through the same instantaneous state as a stuck one, with one replica retired while another is not, so a violation is only reported once it has persisted across ten maintenance ticks. One shape of the violation is reachable today, a log collection an introspection-disabled replica has sealed while its peers still maintain it, and it is reported as a warning until that defect is fixed. Every other shape is a soft panic. Alongside it, two cheaper checks. Capability counts never go negative, and a collection's since stays less_equal every read hold the instance can attribute: the input holds of dependent collections, the holds of index-targeted peeks, and the storage input holds of installed replica dataflows, the last against the storage controller's own since. Holds are attributed by provenance rather than by ID, because a materialized view occupies the same GlobalId twice, once as the compute sink that writes it and once as the persist shard it writes to, and the two have unrelated sinces. Note that `since <= upper` is deliberately not asserted, as the compute protocol explicitly permits read frontiers beyond a collection's upper. The capability check also runs at the AllowCompaction emission point, where it only touches the collection being compacted. The hold coverage and progress checks walk every collection and replica, so they run on the maintenance tick only. The checks are gated behind enable_compute_read_hold_invariant_checks, which defaults off in production and on in the test configuration. A unit test pins the empty-upper branch of the lag read policies, which maps the top of the frontier lattice to its bottom. That lattice inversion is what turns a sealed collection into one whose since can never advance, so changing those two lines should be a deliberate act. Finally, a failed read hold downgrade is now logged rather than discarded. A capability change that never arrives leaves a since permanently wrong with no signal. Releases are still silent, because a hold outliving its issuer is routine both at instance drop and at process shutdown. Closes: CPU-235 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Read holds and compaction have passed the point where a reviewer can convince themselves that a change to them is safe by reading it, and the failure mode is silently retiring data a reader still needs. CPU-234 is a live instance: an introspection-disabled replica seals its cluster's log collections, which pins their
sinceat[0]forever, which broadcastsAllowCompaction(id, [0])to every replica of the cluster, so the healthy replicas' log arrangements never compact. This adds assertions over the state the controller already keeps, so that the next defect of that shape reports itself.The load-bearing part is which quantity the progress check names. An invariant phrased over a collection's own upper is vacuous, because that upper is derived from the first replica to report an advance: one replica sealing carries the whole collection with it, and every invariant statable about the recorded value still holds. The check therefore reconstructs an agreed upper, the meet of the write frontiers of the hosting replicas that have not retired, from the per-replica collection state, and requires the read policy applied to it to be
less_equalthe implied capability. That is a genuine invariant rather than an accident: read policies are monotone away from the empty upper, and the agreed upper is alwaysless_equalthe recorded upper, so only a lattice-inverting step can violate it.A collection whose dataflow legitimately terminates passes through the same instantaneous state as a stuck one, with one replica retired while another is not, so controller state cannot separate the two at an instant and a violation is only reported once it has persisted across ten maintenance ticks. One shape of the violation is reachable today, a log collection an introspection-disabled replica has sealed while its peers still maintain it, and that one is reported as a warning until CPU-234 is fixed. Every other shape is a soft panic, so a regression in ordinary index compaction fails CI now.
Alongside it, two cheaper checks. Capability counts never go negative, and a collection's since stays
less_equalevery read hold the instance can attribute: the input holds of dependent collections, the holds of index-targeted peeks, and the storage input holds of installed replica dataflows, the last against the storage controller's own since. Holds are attributed by provenance rather than by ID, because a materialized view occupies the sameGlobalIdtwice, once as the compute sink that writes it and once as the persist shard it writes to, and the two have unrelated sinces. Attributing by ID produced 1345 false positives in two minutes on a stock local environment. Note thatsince <= upperis deliberately not asserted, asComputeCommand::AllowCompactionexplicitly permits read frontiers beyond a collection's upper andReadPolicy::ValidFromuses that permission.The capability check also runs at the
AllowCompactionemission point, where it only touches the collection being compacted. The hold coverage and progress checks walk every collection and replica of the instance, so they run on the maintenance tick only, and everything is gated behindenable_compute_read_hold_invariant_checks, which defaults off in production and on in the test configuration.Two smaller pieces. A unit test pins the empty-upper branch of the lag read policies, which maps the top of the frontier lattice to its bottom; that inversion is what turns a sealed collection into one whose since can never advance, so changing those two lines should be a deliberate act. And a failed read hold downgrade is now logged rather than discarded, since a capability change that never arrives leaves a since permanently wrong with no signal. Releases stay silent, because a hold outliving its issuer is routine both at instance drop and at process shutdown.
Worth a reviewer's attention: the tick counter is state added for the benefit of a check, and the warning branch for the known log-collection seal is a deliberate hole that should close with CPU-234.
Closes: CPU-235
Filed by Claude Code.