Detect configs expired from the swarm and re-store them - #2151
Open
mpretty-cyro wants to merge 13 commits into
Open
Detect configs expired from the swarm and re-store them#2151mpretty-cyro wants to merge 13 commits into
mpretty-cyro wants to merge 13 commits into
Conversation
…re-store them 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; the server rejects the whole sequence above that, and a single config can split into ~66 parts. - Run the obsolete-hash deletes as a separate pass after the stores, built only from restores that fully landed, so a delete can never go out over a config we did not finish storing. - 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. - Drop the per-store semaphore and let BatchApiExecutor coalesce; the semaphore was bounding batch size by accident, not by design.
… in the repo The comments added by the previous commit cited section numbers from the cross-client design notes they were written against. That document lives in no repo and won't be in the PR, so every citation was a dangling pointer — and a bare "§4.1" is worse than none, because it implies the reasoning lives somewhere the reader is expected to find and can't. The comments were written to carry their reasoning inline, so most citations were pure deletion; the few that leaned on one had the substance written in instead. Vector labels (V13b, V22c) stay in test names: they identify a test within this repo and are shared vocabulary with the other Session clients, so a divergence can be pinned to one rule. Also corrects a comment that still said the chunking fixture "crosses 19" after the sub-request limit moved to 20. Comments only, no behaviour change.
…test checks Test names and assertion messages are the one kind of stale prose nothing contradicts: a wrong comment sits beside code that disagrees with it, but a green test prints its own description as passing on every run. - `V13 ... is put back once` did not advance the clock, so a session-scoped bar and a time-bounded one both satisfied it. The unqualified "once" was a property no assertion there checked; it now says "while the bar holds" and names V13g as the only test that separates the two. - Three vector labels each sat on two different tests (V13b, V16, V22c), in every case because a follow-on test reused the label of the one above it. The continuations are real properties but not vectors, so they are now prose-named. Duplicate labels also defeat a coverage audit by label: an uncovered row can read as covered. Test names and comments only, no assertions changed.
…the guard Both asserted that a response is not read for absence without an extend request, and both used a fixture whose `unchanged` key was absent — which makes the response unreadable on its own grounds. So each passed for a reason unrelated to its name, and would have passed against an implementation that read shorten responses for absence. Verified by mutation rather than by inspection: deleting the extend guard left both green beforehand, and fails both now. The fixtures are readable (`unchanged` present and empty) so the guard is the only thing that can produce Inconclusive. Tests only, no production change.
…factual on purpose Both fixtures send a readable sub-response for a request that a real server would answer without one, so that a single guard is the only thing producing the asserted result. That makes them less accurate than production on purpose, and the previous comments explained the isolation without saying so — leaving the next reader free to restore the realistic shape for accuracy and silently make both tests vacuous again, passing. Where two guards cover two different failures, production usually trips both at once, so isolating one needs a counterfactual, and a counterfactual needs a note or it gets corrected away. Comments only.
… own value detectMissingConfigHashes had three conditions returning one Inconclusive, and AlterTtlApi's decode-failure path returned the same value for a fourth reason. No consumer discriminates on it, so the collapse cost nothing at runtime — it cost every test that touches those guards, because a fixture set up for one condition usually satisfies another too, and the test then passes without reaching the guard it names. Two did exactly that. Inconclusive becomes a sealed interface over ExtendNotRequested, NothingAsked, NoUsableSubResponse and ResponseUnreadable. Behaviour is unchanged and deliberately so: all four still mark nothing missing and authorise no store, and the two sites that discriminate do so on Checked, which a sealed subtype still satisfies. Neither poller names the type. The decode failure is kept separate from NoUsableSubResponse rather than folded in: one means we could not read the answer, the other means the swarm answered and told us nothing. Collapsing them would reintroduce, in the type added to prevent it, the ambiguity this removes. Two assertions that sampled one value for all causes now iterate every cause, so a cause added later cannot quietly acquire the power to flag a group expired or authorise a store.
…mit made false Splitting the inconclusive causes turned this fixture's failure mode from silent to loud: restoring the realistic `unchanged = null` now returns NoUsableSubResponse where the assertion names ExtendNotRequested, so the test fails rather than passing vacuously. The comment still warned about the silent version. The same sentence in AlterTtlApiTest was updated in that commit; this is its twin one file over, which was missed. Comment only.
…d actually is Dirtying a config moves its current hashes into the old set and clears them, so a dirty config usually no longer holds anything the swarm reported missing and the hash-intersection check rejects it before the clean check is consulted — which makes that check look like dead code to anyone auditing it. It is not, and the exception is the reason to keep it: active hashes are the current hashes plus the parts of any pending multipart set, and that second component survives dirtying. A config that went dirty mid-multipart, one of whose part hashes has been lost, reaches the check with a real intersection. Both facts are libsession's rather than ours and cannot be asserted from a JVM unit test, which cannot load the native library — so the reasoning is recorded at the guard with its source location, and the test that reaches the branch now says what it does not establish, since it gets there through a mock that can present a state the real path reaches only one way. Comments only.
…no test reached A sub-response counts as usable only if it is not failed AND carries an unchanged map. Every failed-node fixture in the suite left unchanged absent, so the second condition did all the excluding and the first was never consulted: deleting it passed all 64 tests. The case it alone covers is a snode that reports failure while still carrying unchanged. Read as usable, its empty arrays become authority and every requested hash is reported missing — re-storing configs the swarm still holds, on the word of a node that said it failed. Confirmed rather than assumed: dropping the term alongside a control mutation known to kill two tests left this hole alive while the control fired, proving the build carried both edits. The new test now dies on that same mutation and nothing else does.
…sub-response
A sub-response is unusable if it failed or if it omits unchanged, and this
vector is about exactly that — but the fixture had no failed node at all, so it
exercised the second route twice and was insensitive to the first.
It now mixes all three cases and dies under either term being dropped.
The failed node claims to hold nothing, which is what makes its exclusion
observable: absence is decided by any(), so one snode reporting a hash missing
already settles it and a failed node that HELD the hashes could not change the
verdict — including it would be inert and the test would survive looking
correct. Claiming nothing moves the expected set from {h2} to {h1,h2}.
…s being unrecoverable Four comments and a test name said libsession offers no way to re-serialise a loaded keys config. libsession retains the bytes of active keys messages and exposes them; what is missing is a JNI binding, so the limitation is this platform's and not the format's. The distinction is the whole point of the comments: attributing it to libsession reads as impossible, when it is in fact not yet plumbed — and a member CAN repair a group's keys by pushing retained bytes back, since they land on the same hash without being re-signed. The keys-exclusion test now records that it should invert when the binding lands, rather than be deleted, and no longer asserts the impossibility in its own name.
The keys-exclusion comment justified the expired banner with "only an admin rekey can fix it". That is false once any device holds the bytes: retention happens on LOAD, not on authorship, so an admin immediately after its own rekey holds nothing for the message it just created and is the device least able to repair it. A member holding the bytes can push them back, signature and all. An admin rekey is the remedy only when no device anywhere still holds the bytes, which is what the banner is actually for.
libsession retains the raw bytes of every keys message this device has loaded, and pushing them back lands on the same hash without being re-signed — so a member can repair a group whose keys the swarm has dropped, and an admin immediately after its own rekey holds nothing and is the device least able to. - Keys join the restorable set. Every retained message goes back, not just the reported one: a generation is a rekey plus its supplementals, a member holding part of one cannot derive the key, and the retained map carries no generation field to group by. The superset is bounded and idempotent. - "Expired" now means the keys are gone AND this device cannot put them back. That is one rule, so canRepairKeys is an input to the determinant rather than an override applied to its answer — a lambda, so answering it (which takes the config lock) is deferred until the guards that could make it irrelevant have passed. - The flag is withheld rather than raised and cleared. It drives a visible conversation banner, so correcting it afterwards is a flicker on a group that was never out of reach. - A successful keys re-store announces itself, and ExpiredGroupManager merges that in as another "not expired" event rather than exposing a setter. The reactive clear fires when a keys message is HANDLED, and the device that re-stored it already holds that hash, so it may never handle it again. - One predicate behind both the flag and the re-store. If they disagreed as "flag says repairable, recovery declines", the banner would never appear and nothing would be fixed. Requires the activeKeyMessages() binding on the libsession-util-android branch.
mpretty-cyro
force-pushed
the
feature/config-recovery
branch
from
August 7, 2026 01:20
88ca431 to
2d92d84
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.