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); +});