Config recovery: detect configs expired from the swarm and re-store them - #730
Open
mpretty-cyro wants to merge 26 commits into
Open
Config recovery: detect configs expired from the swarm and re-store them#730mpretty-cyro wants to merge 26 commits into
mpretty-cyro wants to merge 26 commits into
Conversation
mpretty-cyro
force-pushed
the
feature/config-recovery
branch
from
August 7, 2026 01:17
055679e to
73f526f
Compare
mpretty-cyro
marked this pull request as draft
August 16, 2026 23:26
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.
Found by cold review (Q3). There are two places a poll can conclude the local state is level with the swarm - one for a poll that returned no config messages, one for a poll whose config messages all merged - and only the first was gated on every requested config namespace having answered. So a poll where one config namespace's retrieve failed while another returned a mergeable config message took the second marker and marked the swarm level, which is the state 4.1 exists to exclude. Reachable on group polls, where configGroupKeys handles synchronously and therefore merges inside the poll; the user namespaces never reach it because none of them do. Both markers now read one shared value rather than repeating the condition, since the two sites being far apart in the function is how they came to differ in the first place. The reviewer also noted the existing V22b could not reach the second site: its fixture returns an empty retrieve for every namespace it does not fail, so the poll returns before any merge runs. That is correct. The new test supplies one failed config namespace and one that returns a config message, asserts that handleConfigMessages was actually called before asserting the outcome, and fails on the unfixed code with "expected to be false, got true". Two comment corrections in the same area (Q8): the chunking comments claimed the delete rides in the final batch and counts against the chunk budget. It does not - stores are chunked and the delete is its own request. The behaviour is deliberate, because a full batch plus a delete would be one sub-request over the server's limit, so a later reader tidying the delete into the batch to match the comment would reintroduce the overflow the chunking prevents. And the forced empty batch (Q11) is now an explicit guard. Striding to max(count, 1) produced one empty sequence when there was nothing to send; striding to count instead would run no batches at all, leaving failedVariants empty so every config reads as landed and every hash is banked as stored with nothing sent. The guard says that directly instead of relying on an empty request to fail everything back into retryable. Full suite 2204/0.
The rebase onto current dev conflicted in this fixture, where dev had added its own helpers (configMessage, userSessionId, snode) alongside mine. Both sides were purely additive so I resolved it as a union, and the union ate configMessage's closing brace: dev's hunk ended on the `)!` and mine began on the next declaration's doc comment, so the `}` between them belonged to neither side. Caught by the build, not by reading the resolution - which is the argument for compiling a mechanical merge before trusting it rather than after noticing something odd. Kept as its own commit rather than amended into the rebase so the repair is visible in review; the alternative hides a resolution error inside a commit whose message says nothing about it. Full suite 2269/0.
Retention captures a keys message's bytes when it is loaded, so a group that loaded its keys before retention existed holds the key and the hash with nothing behind them. Re-loading the same message fixes that: insert_key takes its early return - we already have this key - which is a no-op for key state, and still try_emplaces the bytes and flags a dump. The merge that does nothing is exactly the merge that backfills, so this needs no libSession change. The trigger is bytes-absent, not keys-related: we hold an active keys hash the bytes accessor returns nothing for. That is true exactly of pre-retention groups and false forever after, so it needs no migration flag or version check - the condition clears itself. Run proactively from the poll, deliberately not from the detection path. Detection fires when the swarm has lost a hash; this fires when we lack bytes for a hash the swarm still has. By the time detection fires the message is gone and the window has closed. The fetch re-reads the keys namespace without a lastHash, since passing ours would return only what has arrived since - precisely the set we already have bytes for. The merge goes through a database write because retention lives in the config dump: capturing the bytes in memory without persisting would be lost on the next launch, which would look like it worked. The attempt is barred for the existing one-hour interval rather than a new one, and claimed before the fetch rather than after, so a group whose keys are genuinely gone is barred by having tried. The bar is kept separate from the re-store bar - reusing that one would block the V23 repair a successful backfill just enabled. Vectors V24, V24a, V24b, V24c. V24b is mutation-verified: making the trigger keys-related rather than bytes-absent fails it alone. Also corrects three comments carrying the claim that groups predating retention cannot be recovered. That was true only of groups whose keys message has also expired from the swarm; for the rest this is the fix. Full suite 2273/0.
…a seam Last resort for a group whose keys messages are gone from the swarm and whose bytes no device can supply: rekey so members get usable keys again. Built to be removable, because it is the one write in this feature that every member on every version sees and Morgan wants the option of withdrawing it. So it is a file of its own with one call site, and removing it is deleting that file and that block. Nothing else references it - the only mentions of ConfigForceRekey anywhere are its own file, its one call site and its own tests. The precondition lives at the call site rather than inside, so a reader deciding whether to keep B2 can see when it fires without opening it. It needs two facts that do not imply each other: a backfill has already run for this group and found nothing, so the bytes are not obtainable by re-reading; and this poll's detection says the swarm has lost the keys. Keys-missing alone is also true of a group nobody has looked at yet, and rekeying that group throws away keys a backfill would have restored. The record of a failed backfill is in-memory and session-scoped, per the ruling. A persisted "tried and failed" is a sticky negative that would let an irreversible write fire on evidence gathered weeks ago; session scope fails closed, delaying B2 by one poll after a restart rather than enabling it. It is kept distinct from B1's bar - that governs how often B1 retries, this governs whether B2 may fire, and one store must not carry both meanings. The storm guard lives with B2 so deleting B2 deletes it. Several admins reach the precondition in the same window, since they poll the same swarm and see the same missing keys. Vectors V25, V25a, V25c. V25b - the seam - is demonstrated rather than asserted: with B2's entry point stubbed to a no-op the whole V24 series still passes, which is the property Morgan asked for and is not expressible as a unit test. Full suite 2276/0, measured against unlanded libSession work.
Four corrections to the keys backfill and force-rekey work. The backfill's session-scoped attempt record now means "attempted this session and the bytes are still absent", not "the swarm was empty" - a fetch that returns messages which still don't restore the bytes is just as much a failed attempt, and previously left the door open to refetch the same useless messages for the rest of the session. The force rekey now requires levelness established by the poll that immediately precedes it. localStateIsLevelWithSwarm answers a different question - "level at some point this session" - and stays true afterwards, so on its own it is fail-open: a device whose last complete poll was yesterday still reads true today. performPoll therefore reports whether the current cycle set the marker, and the rekey branch reads that instead. That distinction is also why the file comment on ConfigForceRekey was wrong. A rekey does not exclude members by design; Keys::rekey encrypts the new key for every member in the Members config it is handed. The risk is the view it is handed - a member added while we were away and not yet merged locally is silently excluded - which is a stale-config risk, not a rekey one, and it is exactly what the freshness precondition addresses. V24a and V24c each asserted only half of what they claimed, so both now assert the block and its lapse. V25d asserts what is reachable from here - that the sticky predicate stays true a day later, the fact making the call-site rule necessary - and names the rest as a gap: the precondition reads a local inside one poll, so no test driving rekeyIfPossible can exercise it, and stubbing one would assert nothing.
The freshness precondition was enforced entirely at the call site, which left half the rule there: the signal is a local living for one poll() invocation, rekeyIfPossible never saw it, and so no test driving rekeyIfPossible could exercise the refusal. My first attempt at covering it stubbed a false signal and then did not call the entry point, which asserts nothing while reading as coverage. rekeyIfPossible now takes the signal as a parameter and refuses when it is false. The caller still computes it, so the seam is unchanged and the parameter is deleted along with the file; but the refusal is now B2's own behaviour, and V25d asserts it directly - that nothing is attempted against any config, and that the refusal is not sticky. Mutation-verified: removing the guard fails V25d and nothing else.
The record was named keysBackfillFoundNothing but is set on two paths: an empty fetch, and a fetch that returned messages which still did not restore the bytes. The second call site had to carry a comment denying the identifier next to it - "the record means we looked this session and the bytes are still not here, not the swarm was empty" - which is the clearest evidence available that the name was wrong. It was also asymmetric within one type: the reader was already named keysBackfillHasFailed, so tracing from the getter gave the right idea and tracing from the setter gave the wrong one. Renamed the field and setter to keysBackfillFailed / markKeysBackfillFailed to match the reader. The comment at the second call site no longer has to contradict the name, and the field's own comment now states the predicate: set once a backfill has run and the bytes are still absent, by either route. No behaviour change.
The comments were written in the vocabulary of the work rather than of the code. "B1"/"B2", section numbers and vector numbers are all useful to someone who has the planning documents open and meaningless to everyone else, and a comment citing a document nobody can open leaves the local obligation unrecorded - the reader sees an explanation where there isn't one. Where a citation was carrying the reasoning, the reasoning is now stated. "Reuses §5.5's one-hour interval" becomes why an hour: the cost being bounded is an extra namespace read per poll for a group likely to keep failing, and the repair is not time-critical because the keys have already been missing long enough to expire. Folded the force rekey into ConfigRecovery as an extension, renamed to match the repo's X+Y.swift convention for extension files. It keeps the property that mattered - no dependency from the backfill to the rekey, so the rekey deletes without touching the backfill - which was always carried by the structure rather than by being a separate type. Dropped the prose describing that removability. It documented a decision that has not been made, so if the rekey is kept it becomes false with nothing to catch it, and the structure already says it. Also dropped a guard comment restating the doc comment three lines above it, and corrected a MARK naming a function that had been renamed. No behaviour change: 2277 passed / 0 failed.
The cooldown was an hour, matching the re-store bar. That bar can err short because a redundant re-store sends byte-identical data and costs one request. This one cannot: a redundant rekey makes every member on every version process a new generation, and content encrypted under superseded keys can become unreadable to anyone who never held them. Erring long costs nothing by comparison. A group that reaches this path has had no retrievable keys message for at least the message TTL, so it has already been broken for far longer than the wait. V25c only asserted the block, so it would have passed a guard that blocked forever - and the longer the interval, the longer that bug survives in the field. It now advances the clock past the interval and asserts the second rekey happens. Mutation-verified: making the guard permanent fails V25c and nothing else. Behaviour change is the interval only: 2277 passed / 0 failed.
The rekey needed "level as of this poll" where the store only offered "level at some point this session". That was bridged by a Bool threaded out of performPoll, set at some of the sites that write the sticky marker and not others - two representations of one fact, free to drift, and the third writer was already outside the poller entirely. The mark is now a stamp: a map from swarm to the poll token the mark was made in. Both questions read that one field. Ever level is a present key; level as of this poll is equality with that poll's token. The Bool is gone. Tokens are per swarm, so another swarm's poll cannot invalidate this one's mark, and minted at the start of the poll - a token taken at the end would name the poll that just finished, so a mark made during it would always match and the check could never refuse. ConfigMessageReceiveJob is not a poll and stamps a sentinel that no live token can equal. It merges messages handed to it and cannot know whether the swarm was fully answered, so its mark must count for the sticky question and never for the poll-scoped one. Two things this surfaced that were not in the plan: The lossy-merge withdrawal on this client is a second set rather than a deletion, so a swarm with an incomplete merge still holds a stamp. The poll-scoped reading has to apply that disqualification too, or the state the withdrawal exists to exclude would satisfy the stricter question. Matching the stamp alone accepts a stale token, because the mark that old poll left is still there - it answers "was the mark made in the poll you name" rather than "are we level now". The named poll must also be the current one. Found by V25e. V25e covers the assigned property and asserts the sticky reading stays true at the stale step, so it cannot pass by both readings going false together. V25f pins the sentinel, which mutation showed was otherwise unprotected: stamping a live token there was caught only by an unrelated spec. 2279 passed / 0 failed.
V25c asserted that the guard blocks and that it lapses after a day, but nothing in it distinguished a day from the hour this used to use - a silent regression to the old value would have passed unchanged. Adds the step Android had and both other clients lacked: two hours in, assert still exactly one rekey. That fails on any interval shorter than two hours, so the value is pinned rather than merely its existence. Mutation-verified: reverting the interval to an hour fails V25c and nothing else.
Seven comments described this branch's own history rather than the code as it stands - "this used to be applied immediately", "used to be avoided by striding to max(count, 1)", "this test used to assert the opposite". A reader arriving later has no access to the version being contrasted, so the sentence spends its length on something they cannot see and leaves the current rule implied rather than stated. Each is restated as present fact, keeping the substance. The reason a keys config is not flagged on sight is that it has a recovery path, not that it once didn't. The reason the empty-batch guard is explicit is that the alternative relies on a failure path rather than saying what it means, not that the alternative was there first. Three were in production and four in tests, and one was written after the earlier comment sweep - a single cleanup does not hold while the vocabulary that produces these is still in use. No behaviour change: 2279 passed / 0 failed.
The previous pass caught every "used to be" and "now uses" and left two comments that say the same thing without a verb: "a day rather than an hour" is a bare comparative, and "this was previously a lazy var" hangs the narration on an adverb. A sweep for narrating verbs finds one grammatical form of the defect, and the defect is mentioning a state the code is not in - which a comparative, an adverb or a noun phrase carries just as well. Four fixed. The cooldown comment now says why the two intervals err in opposite directions rather than what this one is not; the eager registration says what would go wrong if it were lazy rather than that it once was; the fixture note describes what a fixture must produce rather than what an older one could not; and the uncatchable-abort note says "without the guard" rather than "on the previous implementation". Swept over 25 files, 0 unreadable, with the pattern recorded in the report so the gap is visible without re-running it: three hits remain and all describe present state - a hypothetical conformer, a message already handled at runtime, and "the former of these two". No behaviour change: 2279 passed / 0 failed.
The comments carried the emphasis of the messages they were written alongside. Measured against the base: 1 emoji in the whole repo and 18 bold spans, where this branch had added 18 and roughly 249. Emphasis is not foreign here - the base has "this phase runs on **every** build" - but at that density it stops working. A bus message competes for attention against other messages; a comment already has the reader's whole attention, so a marker on every third sentence only makes the sentences compete with each other. Emoji removed outright. Bold kept for seven clauses across the branch, each one where missing it produces a silent fail-open: the token being per-swarm and minted at the start, the sentinel never equalling a live token, the named poll having to be current, an empty batch not meaning everything landed, the receive job not being a poll, the backfill not being gated on detection, and the exclusion risk being the members view. No content removed - every one of these was a real why or complexity comment with a marker glued to the front. No behaviour change: 2279 passed / 0 failed @ libSession ab75f54b.
mpretty-cyro
force-pushed
the
feature/config-recovery
branch
from
September 8, 2026 06:30
73f526f to
44436bb
Compare
mpretty-cyro
marked this pull request as ready for review
September 8, 2026 06:32
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.
Detect and recover configs that have expired from the swarm
A config message that falls out of every swarm node is unrecoverable today: the device
still holds the dump, still believes it is in sync, and pushes nothing — so the config is
gone for every other device and nothing says so.
This detects that from the
expireresponses we already send on every poll, re-stores thebytes we still hold, and — for group keys, which cannot be rebuilt from a dump the way other
configs can — repairs from what is still retrievable or, as a last resort, rekeys.
What it does
Detection. The expiry bump each poll already carries reports which hashes it did not
recognise. A hash the swarm does not know, for a config we consider active, is a config
that has expired off the swarm.
unchangedbeing absent from a response is nowdistinguished from it being empty — conflating them was the trap this feature exists to
avoid.
Recovery. For configs whose bytes we still hold, the dump is re-stored in one batch per
swarm, with the obsolete hash swept in the same sequence. Foreground only, once per hash
per session, without dirtying the config or touching its seqno.
Group keys are the hard case: a keys message cannot be reconstructed from a dump, so the
re-store above has nothing to send. Three separate things handle them, and only the last is a
fallback:
about is missing. Where libSession has retained the key messages, those are re-stored from
that retention on the ordinary recovery path.
keys hash, we re-read the keys namespace from scratch — at most once an hour per group —
regardless of what detection said this poll. It is deliberately not gated on detection:
detection reports that the swarm has lost a hash, whereas this fires when we lack bytes
for a hash the swarm still
has. Those are opposite conditions, and the second stops being repairable at the moment the
first becomes true. Gating the backfill on detection would look right and repair almost
nothing.
has actually run for that group and the bytes are still absent — and the group is
detected expired, and this poll established levelness with the swarm. Those are separate
facts and none implies the others: an expired verdict alone would fire on a group nobody
has looked at yet.
The rekey is the single write here that every member on every version sees, so it is kept
separable: it lives in its own file with one call site and its own tests, and nothing else
depends on it. If we decide against it, the file and the call site go and nothing else
changes.
Guards
The re-store is gated on the device having polled and merged this session, the config
being clean, the hash still being active, and the group not being kicked or destroyed. (The
backfill is not one of these — see above; its own limit is bytes-absent plus the one-hour
per-group bar.)
The rekey additionally requires levelness with the swarm as of the poll that triggers
it — not merely at some point this session.
The reason is worth stating directly, because the instinct is to worry about the wrong thing.
A rekey encrypts the new key to this device's view of the members. Issued from a stale view
it silently excludes anyone we have not merged yet — and this path fires precisely on devices
whose config state is known to be degraded, so a stale view is the expected case rather than
the unlucky one. Nothing here can lose a config; the worst case is a member quietly losing
access to a group.
What guards it: the level mark is stamped with the poll that made it, and the rekey requires
that stamp to be the currently-running poll's. A mark from an earlier poll, or one made
outside a poll at all, does not qualify. The residual is that the marking site is still
trusted — if a poll marks levelness it should not have, the rekey believes it.
Two things reviewers should know before reading the diff
one fails silently.
97aafbbd— retains the active keys message bytes so they can be re-stored. This is whatboth keys-repair paths read; without it there is nothing to re-store and they are simply
inert.
a18b0f08— flags a dump when a known key arrives under a new hash. This is the easy oneto overlook: with retention present but this absent, the backfill fetches, merges, and
produces no dump to persist, so it reports success and repairs nothing.
Both are in libsession-util#123, open and not yet merged. Until it merges, detection, re-store and
the expired-group verdict all work, but keys repair does not exist for users.
The test numbers below were measured against libSession
ab75f54b, which is the head oflibsession-util#123 and contains both. So the numbers describe this change against libSession as
currently proposed — if that PR moves during review, they need re-running.
A config expiring off this device is not the same as it being gone. Every other member's
poll renews the message's TTL, so a device that has lost its own copy can often still read one
back from the swarm. That is why the backfill exists at all, and why the force rekey is a last
resort rather than the fix.
Also included
Two commits unrelated to config recovery, kept separate: a data race between
footerButtonInfoand theinternalStateobservation, and the isolation of theSessionTableViewModelrequirements that read main-actor state. Both surfaced whilerunning this suite.
Testing
2279 passed / 0 failed @ libSession
ab75f54b, across the four CI suites(
SessionTests,SessionUtilitiesKitTests,SessionNetworkingKitTests,SessionMessagingKitTests).The guards and the rekey refusals are mutation-verified.
Three limits:
back" wants an Appium pass or a manual check.
ConfigRecoverycache API directly are coupled to that API ratherthan asserting the requirement independently of it, so a behaviour-preserving refactor
would break them.
caller computes, so the refusal is tested directly. What is not covered is the marking site
itself: a poll that marks levelness it should not have would be believed.