Config recovery: detect configs expired from the swarm and re-store them - #730
Open
mpretty-cyro wants to merge 12 commits into
Open
Config recovery: detect configs expired from the swarm and re-store them#730mpretty-cyro wants to merge 12 commits into
mpretty-cyro wants to merge 12 commits into
Conversation
Config messages have a 30-day TTL that is refreshed by the expire bump already
piggybacked on every poll. A device offline past the TTL loses its config from
the swarm and nothing noticed. This reads the bump's own answer to detect that
case and re-stores from local state. Reasoning for each rule is in the code.
- Detect missing config hashes from the `unchanged` array the server returns for
an extend-only expire. An absent key means detection is unavailable, not that
every config is gone.
- Re-store on a later poll, only once local state is level with the swarm, so a
stale local config can never overwrite a newer remote one.
- Chunk the recovery batch at 20 sub-requests inclusive of the obsolete-hash
delete and stop at the first failing batch. The server rejects the whole
sequence above 20, and a single config can split into ~66 parts.
- Bar a settled hash for an hour rather than for the session; a backgrounded
session can outlive the TTL, which would strand the config it just restored.
- Issue the obsolete-hash delete only for restores that fully landed.
- BatchResponse: validate each sub-response before re-serialising it. The old
`compactMap { try? … }` could not drop anything, because `data(withJSONObject:)`
RAISES for a scalar rather than throwing - so a `{"results":[1]}` response
aborted the app instead of failing the parse. Both branches now go through one
guarded helper, which also makes losing a sub-response impossible.
The comments cited an internal design document by section number. That document is not in this repo and will not be in the PR, so every citation was a pointer a reader could not follow - and a bare section number is the worse form, since it implies the reasoning lives somewhere they are expected to already know. Most were pure deletion, the comments having been written to carry their reasoning inline. The few that actually leaned on the citation are rewritten to state the substance instead: - the retry-budget vector now names the population a consecutive-failure cap would exclude, which is the whole reason it needs its own test - the lossy-merge vector names the level-with-swarm precondition rather than citing it - the sweep vectors describe the rule and its ordering requirement directly Vector labels are kept in test names - they identify a test within this repo and stand on their own.
The bar on re-storing a hash was session-scoped before it was given a one-hour interval, and six comments still described the old behaviour. They were nowhere near the change that invalidated them - the interval and its rationale were updated, these sit in the store's error paths, in the inspection/verdict split, and in a test. The worst of them sat twenty-four lines below the corrected rule in the same doc comment, asserting that a stored hash "is barred permanently" directly under "Bounded, not permanent". The point each comment was making is unchanged; only the duration was wrong. So they now say what the code does - barred for the bar interval - rather than for the session or forever.
… what it cannot prove This capability claim was enforced in two places and tested in one. The DetectionReport side had a test; the config inspection, which skips a keys config outright, had none. The new test covers the outcome - a keys config with an active hash the swarm has lost is offered for recovery by nothing, and is not left pending either. It asserts the premise first, since a config holding no active hashes would return empty for an unrelated reason and pass without exercising anything. It deliberately does not claim to cover the guard itself. Deleting that guard leaves the test passing, because push() independently returns nil for a keys config unless a rekey is in flight - so the guard is defence in depth here rather than the operative exclusion. That is measured, not assumed, and the comment records both it and the fixture that would isolate the guard.
The detection function has three guards that can each produce the same
result, and both of these fixtures satisfied two of them at once - so each
test passed with the guard it names deleted, exercising its neighbour
instead.
Measured rather than reasoned: with the original fixtures and their own
guards removed, both still passed.
V8 asserted "we refuse because we never asked for unchanged", but its
sub-response also had no unchanged key, which is V8b's cause. It now
uses a readable sub-response - deliberately unlike the real one, since
a server that never saw extend omits the key - so the only thing left
to refuse on is the request flag.
V14 asserted "an empty ask answers nothing", but also passed an empty
swarm, which is the no-usable-answer cause. It now passes a readable
node, leaving the empty ask as the only reason to be inconclusive.
Both now fail when their own guard alone is deleted.
… guard A locally-modified config drops its active hashes, so the hash-intersection check excludes it one step before needsPush is consulted. Deleting that guard leaves the test passing - measured, not assumed, and found by a positive control that failed to fail. The guard stays as defence in depth. What is new is the assertion that the active hashes really are empty once the config is dirty, which turns the redundancy into something monitored: if libSession ever kept them active across a local change, that line fails and says the guard has become load-bearing.
… guarantees The previous comment said a locally-modified config drops its active hashes. That is too broad, and it mattered because it framed the needsPush guard as redundant in general. set_state moves _curr_hashes into _old_hashes and clears it, but active_hashes returns _curr_hashes union the parts of any pending multipart set, and set_state does not touch those. A config that goes dirty while a multipart set is still arriving therefore keeps a non-empty active-hash list, can intersect a genuinely missing part hash, and does reach needsPush - which is load-bearing in exactly that case. The assertion is unchanged and still passes: this fixture has no multipart set in flight. What changes is what it claims - a tripwire on the curr-hash clearing, not a statement that the guard is dead code. Raised by config-recovery-android, verified against libSession base.cpp.
The failed-node fixture carried no unchanged array, so it was excluded by either half of the eligibility check and neither vector could tell them apart. Measured: dropping the failed-check term from the filter killed nothing, with a guaranteed-fire control in the same run proving the edit was in the build. The fixture now reports unchanged while still being failed, so being failed is the only reason left to exclude it. Same mutation now kills V5 and V6 and nothing else. The shape is real on the wire - a service node can report failed while still carrying unchanged - and reading one as usable makes its empty arrays authoritative, reporting every requested hash missing and authorising re-stores of configs the swarm still holds. Our own validator currently flattens any failure to no-unchanged-info, so the combination cannot arrive that way today; detect is a pure function whose contract must hold for every input its type admits, and this is what protects it if that changes. Raised by config-recovery-planning after config-recovery-android found the same blind spot.
…ation internalState is @mainactor @published and ObservationBuilder.assign takes an @escaping @mainactor closure, so the write is isolated. footerButtonInfo was not: a lazy var whose initialiser builds a subscription to $internalState, with no isolation and no lock. Lazy-var initialisation is not thread-safe in Swift, so that subscription could be built on one thread while the main actor was sending on the same PublishedSubject - a segfault inside PublishedSubject.send, seen once on a simulator during a full-suite run, and the same race behind the long-standing ~2-8% flake in ThreadNotificationSettingsViewModelSpec. Marking the property @mainactor makes the isolation match the property it reads. That follows existing precedent - subtitle in ThreadDisappearingMessagesSettingsViewModel is already @mainactor lazy var satisfying the same non-isolated protocol requirement. Applied to all four view models with both halves of the race - a lazy footerButtonInfo over $internalState plus an observation write. UserListViewModel and EditGroupViewModel have the lazy var but neither the publisher nor the write, so they are untouched. The compiler then located the racing readers: two specs subscribed to footerButtonInfo from outside the main actor, which is what the crash's second thread was doing. Those accesses are now hoisted onto the main actor. Verified: 120 iterations of both specs, 1080 test cases, 0 failures and 0 restarts, against a previously measured 2/40 and 3/40 on clean trees. Full suite 2197/0.
…tate The previous commit fixed the four implementations that had the race. It did not stop a new one being written: the protocol requirement was not isolated, so a future conformer could declare a plain lazy footerButtonInfo over $internalState and reintroduce it with nothing to complain. Isolating the requirement fixes that, because a witness inherits global-actor isolation from the requirement it satisfies. Verified by simulating the reintroduction - removing the per-property @mainactor and reading the property off the main actor in a test now fails to compile with "main actor-isolated property 'footerButtonInfo' cannot be accessed from outside of the actor". title, subtitle and footerView are included on the same evidence: each already had at least one conformer declaring it @mainactor while the requirement did not, which is the same mismatch one step from becoming the same bug. The other members have no such implementation and are untouched. Isolating the whole protocol was tried and rejected: conforming to a globally-isolated protocol infers isolation on the entire conforming type, which cascaded into unrelated view models and a spec with no connection to this race. Isolating individual requirements does not. The per-property annotations from the previous commit are now redundant, but are kept so that commit remains independently cherry-pickable. Three test accesses to a now-isolated title are hoisted onto the main actor. Full suite 2197/0.
libSession now keeps the raw bytes of every active keys message alongside the hashes. That makes a keys config recoverable after all: the bytes are pushed back unchanged, which lands on the same hash, needs no signature, and can therefore be done by a member - who cannot sign a keys message and so could never regenerate one. So an all-keys-missing detection now has two outcomes rather than one. If the device retained the bytes it repairs the group and the expired flag is left alone; if it did not - a group predating retention - nothing changes and the flag is set as before. Recovery is all-or-nothing per generation. A generation is one rekey plus every supplemental issued against it, and a member who receives only part of one does not get the key, so a partially retained generation is treated as unrecoverable rather than half re-stored. Keys messages carry no obsolete-hash list, so this path issues no delete. The expired flag is now deferred behind the repair. A group being repaired is not a lost one, but a repair that fails leaves the user unable to decrypt with no signal, so the flag is applied afterwards in that case and the hashes stay retryable under the existing backoff. That reverses the previous rationale on applyKeysVerdictIfNeeded, which said there was nothing to wait for - true when written, and no longer. Vectors V23, V23a, V23b and V23c, plus a counterpart for the partially retained generation that V23b's all-or-nothing rule implies. The libSession level test that asserted keys configs are never recoverable is rewritten rather than deleted - it now asserts the bytes come back and no delete is issued. Full suite 2202/0.
Three corrections to what I built, all from rulings that arrived after it. Re-store every retained keys message rather than grouping by generation (v120). active_key_messages() is keyed by hash with no generation in it, so "is this generation complete" is not a question this layer can ask - and re-storing everything retained is strictly more than generation-completeness would require, so it satisfies the rule regardless. That reverses my gate: holding any of the missing keys hashes is now enough to attempt a repair, where I had required all of them. Clear the expired flag eagerly when a repair lands (v119b). The reactive path clears it when a keys message is successfully handled, which happens on the peer that fetches it - the device that did the re-storing already holds that hash and will never re-handle it, so its own flag would stay set forever over keys it just put back. Failure still sets the flag; both directions now come from the same branch. attemptedKeysHashes is narrowed to the hashes we actually hold bytes for, since those are what a repair can put back and therefore what its success should be judged against. Including one we cannot restore would make every partial repair read as a failure. Also records that retention happens on load rather than on create (v124), so an admin immediately after a rekey holds no bytes and is the device least able to repair - the opposite of the intuition. V23b is rewritten to pin what it can actually distinguish: that a supplemental is retained and re-stored at all, i.e. that storage is hash-keyed rather than generation-keyed. V23d added for the eager clear. Rebased onto upstream/dev. Full suite 2203/0.
mpretty-cyro
force-pushed
the
feature/config-recovery
branch
from
August 7, 2026 01:17
055679e to
73f526f
Compare
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.
Config messages have a 30-day TTL that is refreshed by the expire bump already piggybacked on every poll. A device offline past the TTL loses its config from the swarm and nothing noticed. This reads the bump's own answer to detect that case and re-stores from local state. Reasoning for each rule is in the code.
unchangedarray the server returns for an extend-only expire. An absent key means detection is unavailable, not that every config is gone.compactMap { try? … }could not drop anything, becausedata(withJSONObject:)RAISES for a scalar rather than throwing - so a{"results":[1]}response aborted the app instead of failing the parse. Both branches now go through one guarded helper, which also makes losing a sub-response impossible.