fix(seal): a failed cache write is not a refresh - #367
Merged
Merged
Conversation
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.
Closes #362.
recordincrementedRefreshStatsbefore attempting the write and then dropped the error on the floor. The counters therefore described what the pass intended rather than what the cache holds — and it is the cache a verdict is answered from. A pass whose every write failed still published a full, healthy pair of gauges while the cache silently stopped ageing.The half that #365 made urgent
The issue's title has two clauses and the second is the sharper one: "leaves the stale copy the fail-closed rule exists to drop".
A territory that verified last week and fails to verify today gets an
Unavailableentry. If that write fails, the territory keeps its previousVerifiedrow. Before #365 that was a latent problem — the cache was only read at boot. #365 made the refresh republish into the running inspector by re-reading the cache, so the staleVerifiedrow now flows straight into served verdicts, with nothing inuncheckednaming it. The verdict answersnotListedfor a provider on the strength of a list the node knows it could not refresh.So the fix I merged earlier today widened this hole, and closing it is part of this change rather than a follow-up.
What changed
recordcounts after the write. A failedputincrements a newunwrittenand returns;verifiedandunavailableare only reached when the row landed. Those two are documented as describing the cache —unavailableis "the width of everynotListedthis node gives until the next pass" — and that is now true rather than nearly true.unwritten_territoriesnames them, unbounded rather than capped. The ordinary cause is the database being briefly unavailable, which hits every territory in the pass at once, so a pass that could write nothing must be able to say so about all of them.The publish step admits them. Each unwritten territory is removed from
listsand pushed ontouncheckedwith a reason. Dropped fromlistsas well as added touncheckeddeliberately: leaving it in both would let the stale copy answer the question while the verdict merely expressed doubt about it. Narrow and honest beats wide and wrong — the same trade the module header already makes.A pass that could not write comes back in 15 minutes rather than 24 hours. #345's failure-taxonomy bullet distinguishes causes by how retryable they are: a timeout is worth retrying, a signature that does not verify is not, Germany's parser ceiling (#320) never will be until the parser changes. A failed write is a fourth kind and the most retryable of them — it is this node's own database. Fifteen minutes rather than seconds because a pass is thirty-odd external fetches, and hammering the Union because Postgres blinked turns one outage into two; the fetches are the expensive half and they are not what failed.
Tests
Two, against a
RefusesToWritestore double, both confirmed to bite by revertingrecordto its previous shape:a_write_that_fails_is_not_counted_as_a_refresh— a list that verifies perfectly, so the only thing that can go wrong is the write.verifiedmust stay 0.an_unavailable_entry_that_cannot_be_written_is_unwritten_not_unavailable— the fail-closed case: the row that would have replaced aVerifiedcopy cannot be written, and counting it asunavailablewould claim the cache had been narrowed when it had not.Against the old code both fail with
left: 1, right: 0on exactly the count that is supposed to describe the cache's width.Incidental
RefreshStatsloses itsCopyderive, because it now carries aVec<String>. Nothing depended on copying it.Not in this branch
The retry interval is a constant, not configurable. Same reasoning
RENEWAL_LEADrecords: a threshold becomes a policy the moment something acts on it, and there is no evidence yet about what interval an operator would want.just checkgreen.