From a9271fd0e646be910f1de3573b6cc812fca6bc73 Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Wed, 1 Jul 2026 14:08:33 -0400 Subject: [PATCH] [subscriptions] Stop spurious "Failed to update subscriptions" on unconfirmed publish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subscribe/unsubscribe intermittently showed an error but "worked after refresh": the kind-30000 list event WAS sent and persisted, but when no relay ACKed it within the 10s window, assertAnyRelayAccepted threw -> toggleChannel threw -> the button handler reverted the optimistic UI and showed "Failed to update your subscriptions." The event was on the relays the whole time (hence the refresh). Fix: in publishSubscriptionList, distinguish an all-timeout (unconfirmed) result from an explicit rejection. The list is an idempotent, replaceable event that was already sent + locally cached, so an all-timeout is treated as a SOFT SUCCESS (record the event id, no throw, reconciles on next load) instead of a hard error. Only an explicit relay rejection (or a pre-publish signer/encryption error, thrown earlier) still fails loudly. New exported classifier subscriptionPublishFailuresAreAllTimeouts(). Tests: tests/subscription-publish-timeout.test.mjs (5 — all-timeout=soft, any-rejection=hard, empty=hard, case-insensitive). Note: assertAnyRelayAccepted is used the same way by userBlocks/app list publishes — they likely have the same false-negative and could get the same treatment later. Co-Authored-By: Claude Opus 4.8 --- js/subscriptions.js | 51 +++++++++++++--- tests/subscription-publish-timeout.test.mjs | 66 +++++++++++++++++++++ 2 files changed, 110 insertions(+), 7 deletions(-) create mode 100644 tests/subscription-publish-timeout.test.mjs diff --git a/js/subscriptions.js b/js/subscriptions.js index 37653fa8..8fa6e846 100644 --- a/js/subscriptions.js +++ b/js/subscriptions.js @@ -175,6 +175,25 @@ const subscribeVideoViewEventsApi = (pointer, options) => * Also handles fetching and rendering subscribed channels' videos * in the same card style as your home page. */ +// A RelayPublishError whose failures are ALL "publish timeout" (no relay ACKed in +// the window) means the event was sent but not confirmed — not that it was +// rejected. For an idempotent, replaceable list this almost always persisted (the +// user sees it after a refresh), so we treat it as a soft success rather than a +// hard error that would revert the optimistic UI. An explicit relay rejection has +// a different reason and is NOT a timeout, so it still fails loudly. +export function subscriptionPublishFailuresAreAllTimeouts(publishError) { + const failures = Array.isArray(publishError?.relayFailures) + ? publishError.relayFailures + : []; + if (!failures.length) { + return false; + } + return failures.every((failure) => { + const reason = typeof failure?.reason === "string" ? failure.reason : ""; + return /timeout/i.test(reason); + }); +} + class SubscriptionsManager { constructor() { this.subscribedPubkeys = new Set(); @@ -1452,7 +1471,8 @@ class SubscriptionsManager { signedEvent ); - let publishSummary; + let publishSummary = null; + let softSuccess = false; try { publishSummary = assertAnyRelayAccepted(publishResults, { context: "subscription list" @@ -1461,17 +1481,28 @@ class SubscriptionsManager { if (publishError?.relayFailures?.length) { publishError.relayFailures.forEach( ({ url, error: relayError, reason }) => { - userLogger.error( - `[SubscriptionsManager] Subscription list rejected by ${url}: ${reason}`, + userLogger.warn( + `[SubscriptionsManager] Subscription list not acked by ${url}: ${reason}`, relayError || reason ); } ); } - throw publishError; + // Only an explicit relay rejection (or a pre-publish signer/encryption + // error, thrown earlier) is a real failure. A result where every relay + // merely timed out means the event was sent to an idempotent, replaceable + // list and almost certainly persisted — surface it as a soft success so the + // optimistic UI is NOT reverted and the user doesn't see a spurious error. + if (!subscriptionPublishFailuresAreAllTimeouts(publishError)) { + throw publishError; + } + userLogger.warn( + "[SubscriptionsManager] Subscription list not acknowledged by any relay within the timeout; treating as optimistic success (reconciles on next load).", + ); + softSuccess = true; } - if (publishSummary.failed.length) { + if (publishSummary?.failed?.length) { publishSummary.failed.forEach(({ url, error: relayError }) => { const reason = relayError instanceof Error @@ -1486,12 +1517,18 @@ class SubscriptionsManager { }); } + // Record the published event id for both a confirmed and an + // optimistic/unconfirmed publish — the event carries this id regardless. this.subsEventId = signedEvent.id; this.subsEventCreatedAt = signedEvent.created_at; this.saveToCache(userPubkey); - const acceptedUrls = publishSummary.accepted.map(({ url }) => url); + const acceptedUrls = publishSummary + ? publishSummary.accepted.map(({ url }) => url) + : []; devLogger.log( - "Subscription list published, event id:", + softSuccess + ? "Subscription list published (optimistic/unconfirmed), event id:" + : "Subscription list published, event id:", signedEvent.id, "accepted relays:", acceptedUrls diff --git a/tests/subscription-publish-timeout.test.mjs b/tests/subscription-publish-timeout.test.mjs new file mode 100644 index 00000000..8ae8b357 --- /dev/null +++ b/tests/subscription-publish-timeout.test.mjs @@ -0,0 +1,66 @@ +// Subscribe/unsubscribe intermittently showed "Failed to update your +// subscriptions" but worked after a refresh — because the kind-30000 list event +// was SENT and persisted, but no relay ACKed it within the 10s window, so +// assertAnyRelayAccepted threw and the optimistic UI was reverted with a scary +// error. Fix: an all-timeout (unconfirmed) publish of the idempotent replaceable +// list is treated as a SOFT SUCCESS (reconciles on next load); only an explicit +// relay rejection (or a pre-publish signer/encryption error) fails loudly. This +// covers the classifier that makes that call. +// +// test_integrity_note: +// change_type: ["new_tests"] +// scenarios: +// - id: SCN-subscription-publish-timeout +// given: "a RelayPublishError from publishing the subscription list" +// when: "its per-relay failures are classified" +// then: "all-timeout => soft success; any explicit rejection => hard failure" +// observable_outcomes: +// - "every-relay-timeout classifies as all-timeouts (soft success)" +// - "an explicit rejection (e.g. 'blocked'/'auth-required') does NOT" +// - "no failures / empty does NOT (nothing to soft-succeed)" +// determinism_controls: +// - "constructs real RelayPublishError objects; no network/clock" +// anti_cheat_rationale: +// prevents: ["hard-coded return value", "over-mocking internal logic"] +// relaxation: +// did_relax_any_assertion: false + +import test from "node:test"; +import { strict as assert } from "node:assert"; +import { RelayPublishError } from "../js/nostrPublish.js"; +import { subscriptionPublishFailuresAreAllTimeouts } from "../js/subscriptions.js"; + +// Build a RelayPublishError the same way assertAnyRelayAccepted does — from the +// per-relay result objects (success:false + an Error whose message is the reason). +const errFrom = (results) => + new RelayPublishError("Failed to publish subscription list to any relay.", results, { + context: "subscription list", + }); +const timeout = (url) => ({ url, success: false, error: new Error("publish timeout") }); +const rejected = (url, reason) => ({ url, success: false, error: new Error(reason) }); + +test("every relay timing out is classified as all-timeouts (soft success)", () => { + const e = errFrom([timeout("wss://a"), timeout("wss://b"), timeout("wss://c")]); + assert.equal(subscriptionPublishFailuresAreAllTimeouts(e), true); +}); + +test("an explicit relay rejection is NOT all-timeouts (hard failure)", () => { + const e = errFrom([timeout("wss://a"), rejected("wss://b", "blocked: spam")]); + assert.equal(subscriptionPublishFailuresAreAllTimeouts(e), false); +}); + +test("only-rejections are NOT all-timeouts", () => { + const e = errFrom([rejected("wss://a", "auth-required"), rejected("wss://b", "restricted")]); + assert.equal(subscriptionPublishFailuresAreAllTimeouts(e), false); +}); + +test("no failures / empty does not soft-succeed", () => { + assert.equal(subscriptionPublishFailuresAreAllTimeouts(errFrom([])), false); + assert.equal(subscriptionPublishFailuresAreAllTimeouts(undefined), false); + assert.equal(subscriptionPublishFailuresAreAllTimeouts({}), false); +}); + +test("case-insensitive: 'Publish Timeout' still counts as a timeout", () => { + const e = errFrom([rejected("wss://a", "Publish Timeout")]); + assert.equal(subscriptionPublishFailuresAreAllTimeouts(e), true); +});