Detect configs expired from the swarm and re-store them

ts/receiver/configMessage.ts

@@ -108,14 +108,21 @@ async function printDumpForDebug(prefix: string, variant: ConfigWrapperObjectTyp
   window.log.info(prefix, StringUtils.toHex(metaGroupDumps));
 }
 
+/**
+ * Note the `mergedCount` in the result. libSession's `merge()` reports which hashes it actually
+ * took in, and a message it could not merge is simply absent from that list — no throw, no error.
+ * So 2-of-3 looks exactly like 3-of-3 to a caller that only checks for an exception, and anything
+ * relying on "we incorporated what we fetched" has to compare the counts instead.
+ */
 async function mergeUserConfigsWithIncomingUpdates(
   incomingConfigs: Array<RetrieveMessageItemWithNamespace>
-): Promise<Map<ConfigWrapperUser, IncomingUserResult>> {
+): Promise<{ results: Map<ConfigWrapperUser, IncomingUserResult>; mergedCount: number }> {
   // first, group by namespaces so we do a single merge call
   // Note: this call throws if given a non user kind as this function should only handle user variants/kinds
   const groupedByNamespaces = byUserNamespace(incomingConfigs);
 
   const groupedResults: Map<ConfigWrapperUser, IncomingUserResult> = new Map();
+  let mergedCount = 0;
 
   const us = UserUtils.getOurPubKeyStrFromCache();
 
@@ -187,6 +194,8 @@ async function mergeUserConfigsWithIncomingUpdates(
           assertUnreachable(variant, `mergeConfigsWithInboxUpdates unhandled case "${variant}"`);
       }
 
+      mergedCount += hashesMerged.length;
+
       const needsDump = await UserGenericWrapperActions.needsDump(variant);
       const needsPush = await UserGenericWrapperActions.needsPush(variant);
       const mergedTimestamps = sameVariant
@@ -211,7 +220,7 @@ async function mergeUserConfigsWithIncomingUpdates(
       groupedResults.set(variant, incomingConfResult);
     }
 
-    return groupedResults;
+    return { results: groupedResults, mergedCount };
   } catch (e) {
     window.log.error('mergeConfigsWithIncomingUpdates failed with', e);
     throw e;
@@ -981,11 +990,16 @@ async function processUserMergingResults(results: Map<ConfigWrapperUser, Incomin
   }
 }
 
+/**
+ * @returns whether every message handed in was actually merged. A partial merge is not an error
+ * here — libSession skips what it can't take and carries on, which is what we want — but the
+ * caller must be able to tell, because "no exception" does not mean "all of it landed".
+ */
 async function handleUserConfigMessagesViaLibSession(
   configMessages: Array<RetrieveMessageItemWithNamespace>
-) {
+): Promise<boolean> {
   if (isEmpty(configMessages)) {
-    return;
+    return true;
   }
 
   window?.log?.debug(
@@ -997,8 +1011,16 @@ async function handleUserConfigMessagesViaLibSession(
     )}`
   );
 
-  const incomingMergeResult = await mergeUserConfigsWithIncomingUpdates(configMessages);
-  await processUserMergingResults(incomingMergeResult);
+  const { results, mergedCount } = await mergeUserConfigsWithIncomingUpdates(configMessages);
+  await processUserMergingResults(results);
+
+  if (mergedCount !== configMessages.length) {
+    window.log.warn(
+      `handleUserConfigMessagesViaLibSession: merged ${mergedCount}/${configMessages.length} config messages`
+    );
+  }
+
+  return mergedCount === configMessages.length;
 }
 
 async function updateOurProfileFromLibSession({

ts/session/apis/snode_api/SnodeRequestTypes.ts

@@ -94,6 +94,19 @@ abstract class ExpireSubRequest extends SnodeAPISubRequest<'expire'> {
   }
 }
 
+/**
+ * The flag the storage server expects on the wire, for a given ShortenOrExtend.
+ * Note: the same value has to be used to build the signature, otherwise the request is signed
+ * for one behaviour and asks the server for another (and the server won't complain).
+ */
+function shortenOrExtendToParams(shortenOrExtend: ShortenOrExtend) {
+  return shortenOrExtend === 'extend'
+    ? { extend: true }
+    : shortenOrExtend === 'shorten'
+      ? { shorten: true }
+      : {};
+}
+
 abstract class StoreSubRequest extends SnodeAPISubRequest<'store'> {
   public readonly getNow: () => number;
 
@@ -775,13 +788,6 @@ export class UpdateExpiryOnNodeUserSubRequest extends ExpireSubRequest {
       );
     }
 
-    const shortenOrExtend =
-      this.shortenOrExtend === 'extend'
-        ? { extend: true }
-        : this.shortenOrExtend === 'shorten'
-          ? { shorten: true }
-          : {};
-
     return {
       method: this.method,
       params: {
@@ -790,7 +796,7 @@ export class UpdateExpiryOnNodeUserSubRequest extends ExpireSubRequest {
         signature: signResult.signature,
         messages: this.messageHashes,
         expiry: this.expiryMs,
-        ...shortenOrExtend,
+        ...shortenOrExtendToParams(this.shortenOrExtend),
       },
     };
   }
@@ -807,6 +813,12 @@ export class UpdateExpiryOnNodeUserSubRequest extends ExpireSubRequest {
 export class UpdateExpiryOnNodeGroupSubRequest extends ExpireSubRequest {
   public readonly messageHashes: Array<string>;
   public readonly expiryMs: number;
+  /**
+   * Same shape as the user request deliberately: the storage server supports shorten on group
+   * expiries too, so narrowing this to 'extend' would state something false about the endpoint.
+   * Note the value is read TWICE — for the signature and for the wire flag — and the two must
+   * agree, which is why both derive from this one field rather than from a literal.
+   */
   public readonly shortenOrExtend: ShortenOrExtend;
   public readonly groupDetailsNeededForSignature: GroupDetailsNeededForSignature;
 
@@ -846,18 +858,11 @@ export class UpdateExpiryOnNodeGroupSubRequest extends ExpireSubRequest {
       );
     }
 
-    const shortenOrExtend =
-      this.shortenOrExtend === 'extend'
-        ? { extends: true }
-        : this.shortenOrExtend === 'shorten'
-          ? { shorten: true }
-          : {};
-
     return {
       method: this.method,
       params: {
         messages: this.messageHashes,
-        ...shortenOrExtend,
+        ...shortenOrExtendToParams(this.shortenOrExtend),
         ...signResult,
 
         // pubkey_ed25519 is forbidden for the group one
@@ -966,15 +971,23 @@ abstract class StoreGroupConfigSubRequest<
   public readonly destination: GroupPubkeyType;
   public readonly ttlMs: number;
   public readonly encryptedData: Uint8Array;
-  // this is mandatory for a group config store, if it is null, we throw
   public readonly secretKey: Uint8Array | null;
+  /**
+   * A member's subaccount auth, used when we have no admin `secretKey`.
+   *
+   * Only config RECOVERY takes this path: a member cannot change group config, so the normal push
+   * always has the admin key. But a member CAN put its own unmodified copy back after it expires
+   * from the swarm, and its subaccount token carries Read+Write for exactly that.
+   */
+  public readonly authData: Uint8Array | null;
 
   constructor(
     args: WithGroupPubkey &
       WithGetNow & {
         namespace: T;
         encryptedData: Uint8Array;
         secretKey: Uint8Array | null;
+        authData?: Uint8Array | null;
         ttlMs: number;
       }
   ) {
@@ -984,15 +997,18 @@ abstract class StoreGroupConfigSubRequest<
     this.ttlMs = args.ttlMs;
     this.encryptedData = args.encryptedData;
     this.secretKey = args.secretKey;
+    this.authData = args.authData ?? null;
 
     if (isEmpty(this.encryptedData)) {
       throw new Error('this.encryptedData cannot be empty');
     }
     if (!PubKey.is03Pubkey(this.destination)) {
       throw new Error('StoreGroupConfigSubRequest: group config namespace required a 03 pubkey');
     }
-    if (isEmpty(this.secretKey)) {
-      throw new Error('StoreGroupConfigSubRequest needs secretKey to be set');
+    // Either credential will do, but not neither: `getSnodeGroupSignature` prefers the admin key
+    // and falls back to the subaccount, and with both empty it cannot sign at all.
+    if (isEmpty(this.secretKey) && isEmpty(this.authData)) {
+      throw new Error('StoreGroupConfigSubRequest needs secretKey or authData to be set');
     }
   }
 
@@ -1006,7 +1022,7 @@ abstract class StoreGroupConfigSubRequest<
     const signDetails = await SnodeGroupSignature.getSnodeGroupSignature({
       method: this.method,
       namespace: this.namespace,
-      group: { authData: null, pubkeyHex: this.destination, secretKey: this.secretKey },
+      group: { authData: this.authData, pubkeyHex: this.destination, secretKey: this.secretKey },
     });
 
     if (!signDetails) {

ts/session/apis/snode_api/configExpiryDetection.ts

@@ -0,0 +1,99 @@
+import { isArray, isEmpty } from 'lodash';
+import { ExpireMessageResultItem, ExpireMessagesResultsContent } from './types';
+
+/**
+ * Deciding whether a config message has expired from the swarm, from the response to the `expire`
+ * request we piggyback on every poll.
+ *
+ * This is a normative rule shared with iOS and Android. The three clients each implement it
+ * separately, so if you change the behaviour here it has to change there too. Every rule below has
+ * a test vector.
+ */
+
+export type ConfigExpiryDetection =
+  /**
+   * The response cannot answer the question. Either we didn't ask for `extend` (so the server
+   * omits `unchanged` entirely and every hash we didn't update *looks* missing), or no
+   * sub-response was usable. Nothing may be marked missing from this.
+   */
+  | { status: 'unavailable' }
+  /**
+   * Every snode either failed or timed out. Distinct from "nothing is missing": we simply have no
+   * evidence either way.
+   */
+  | { status: 'inconclusive' }
+  | { status: 'conclusive'; missingHashes: Array<string> };
+
+/**
+ * A sub-response contributes to the decision only if the snode actually answered.
+ *
+ * `failed: true` may come with `timeout`, `code`, `reason`, `bad_peer_response` or
+ * `query_failure` — none of that matters, `failed` alone is enough to exclude it. Treating a
+ * timeout as "that snode doesn't have the message" would turn every network blip into a re-push
+ * storm, which is the single worst thing this code could do.
+ */
+function isEligible(subResponse: ExpireMessageResultItem | undefined): subResponse is Eligible {
+  if (!subResponse || subResponse.failed || !isArray(subResponse.updated)) {
+    return false;
+  }
+  // The server sets `unchanged` whenever the request set `extend` (or `shorten`), even when it is
+  // empty. So if the key is absent, this response cannot tell presence from absence and has to be
+  // excluded rather than read as "nothing was unchanged".
+  return !!subResponse.unchanged;
+}
+
+type Eligible = ExpireMessageResultItem & { unchanged: Record<string, number> };
+
+function subResponseHolds(subResponse: Eligible, hash: string) {
+  return subResponse.updated.includes(hash) || hash in subResponse.unchanged;
+}
+
+/**
+ * @param requestedHashes the hashes the `expire` sub-request asked about
+ * @param swarm the per-snode `swarm` dict from the recursive `expire` response
+ * @param requestSetExtend whether the request we are reading the response of set `extend: true`.
+ * Note: this must be what *we* sent. The server silently forces extend-only semantics for a group
+ * member's subaccount without telling us, and does *not* return `unchanged` in that case.
+ */
+export function detectMissingConfigHashes({
+  requestedHashes,
+  swarm,
+  requestSetExtend,
+}: {
+  requestedHashes: Array<string>;
+  swarm: ExpireMessagesResultsContent | null | undefined;
+  requestSetExtend: boolean;
+}): ConfigExpiryDetection {
+  if (isEmpty(requestedHashes)) {
+    // We asked about nothing, so we learned nothing. The tempting short-circuit here is
+    // "no hashes requested, therefore none are missing" — but reporting that as *conclusive* makes
+    // detection the authority for a swarm it has no information about, and a conclusive result
+    // outranks the empty-fetch check. That check is precisely the one that should decide when we
+    // hold no hashes, and it could then never be reached.
+    return { status: 'inconclusive' };
+  }
+
+  if (!requestSetExtend) {
+    return { status: 'unavailable' };
+  }
+
+  if (!swarm || isEmpty(swarm)) {
+    return { status: 'inconclusive' };
+  }
+
+  const eligible = Object.values(swarm).filter(isEligible);
+
+  if (!eligible.length) {
+    return { status: 'inconclusive' };
+  }
+
+  // One eligible snode reporting a hash absent is enough (D1). Presence elsewhere does not
+  // override it: re-storing is idempotent, so a false positive costs one redundant request,
+  // whereas waiting for a consensus leans on the swarm replication that is itself the unreliable
+  // part here.
+  const missingHashes = requestedHashes.filter(hash =>
+    eligible.some(subResponse => !subResponseHolds(subResponse, hash))
+  );
+
+  return { status: 'conclusive', missingHashes };
+}