diff --git a/js/app.js b/js/app.js index 592c0fcb..fe8aa063 100644 --- a/js/app.js +++ b/js/app.js @@ -92,6 +92,7 @@ import { assertAnyRelayAcceptedOrUnconfirmed, describePublishOutcome, readRelayPublishSummary, + anyRelayAcceptedInSummaries, } from "./nostrPublish.js"; import { getActiveSigner, @@ -5073,7 +5074,7 @@ class Application { this.showStatus("Deleting. Please wait.", { showSpinner: true }); try { - await this.nostrService.handleFullDeleteVideo({ + const deleteDetail = await this.nostrService.handleFullDeleteVideo({ videoRootId: rootId, video: targetVideo, pubkey: this.pubkey, @@ -5110,7 +5111,21 @@ class Application { } await this.loadVideos(); - this.showSuccess("All versions deleted successfully!"); + // #49 conservative delete semantics: only claim "deleted" when at least + // one relay ACKed something. An all-timeout run (tombstones + kind-5 sent + // but unconfirmed) must not read as a confirmed delete — the video may + // linger on relays until they catch up. + const deleteSummaries = [ + ...(deleteDetail?.result?.reverts || []), + ...(deleteDetail?.result?.deletes || []), + ].map((entry) => entry?.summary); + if (anyRelayAcceptedInSummaries(deleteSummaries)) { + this.showSuccess("All versions deleted successfully!"); + } else { + this.showSuccess( + "Delete request sent, but no relay has confirmed it yet — the video may reappear until relays catch up. Check again in a bit.", + ); + } if (storageLockedRemnant) { this.showStatus( "Heads up: the hosted file is still in your storage bucket because storage is locked. Unlock storage and delete again, or remove it from your bucket manually.", diff --git a/js/nostr/client.js b/js/nostr/client.js index 41f72c4b..e48f4b92 100644 --- a/js/nostr/client.js +++ b/js/nostr/client.js @@ -111,7 +111,7 @@ import "./maxListenerDiagnostics.js"; import { publishEventToRelay, publishEventToRelays, - assertAnyRelayAccepted, + assertAnyRelayAcceptedOrUnconfirmed, attachRelayPublishSummary, } from "../nostrPublish.js"; import { @@ -2960,13 +2960,17 @@ export class NostrClient { await this.ensureActiveSignerForPubkey(userPubkeyLower); try { - const { signedEvent } = await this.signAndPublishEvent(context.event, { + const { signedEvent, summary } = await this.signAndPublishEvent(context.event, { context: "edited video note", logName: "Edited video note", devLogLabel: "edited video note", resolveActiveSigner: (p) => this.signerManager.resolveActiveSigner(p), }); + // Same tally as publishVideo so the edit UI can tell an unconfirmed + // (all-timeout, #49) update apart from a confirmed one. + attachRelayPublishSummary(signedEvent, summary); + await handlePublishNip94(this, signedEvent, context); await handlePublishNip71(this, signedEvent, context); @@ -3216,7 +3220,10 @@ export class NostrClient { let publishSummary; try { - publishSummary = assertAnyRelayAccepted(publishResults, { + // #49: an all-timeout tombstone publish is an unconfirmed soft success + // (summary.unconfirmed=true) so a slow-ack revert/delete doesn't abort + // midway; explicit relay rejections still throw below. + publishSummary = assertAnyRelayAcceptedOrUnconfirmed(publishResults, { context: "video revert", }); } catch (publishError) { @@ -3237,6 +3244,12 @@ export class NostrClient { devLogger.log(`Revert event published to ${url}`) ); + if (publishSummary.unconfirmed) { + userLogger.warn( + "[nostr] Video revert was sent but no relay confirmed it within the timeout — proceeding as unconfirmed.", + ); + } + if (publishSummary.failed.length) { publishSummary.failed.forEach(({ url, error: relayError }) => { const reason = diff --git a/js/nostr/publishHelpers.js b/js/nostr/publishHelpers.js index 2b2e05f4..7c62b6e2 100644 --- a/js/nostr/publishHelpers.js +++ b/js/nostr/publishHelpers.js @@ -1,7 +1,7 @@ import { ENSURE_PRESENCE_REBROADCAST_COOLDOWN_SECONDS } from "../config.js"; import { publishEventToRelays, - assertAnyRelayAccepted, + assertAnyRelayAcceptedOrUnconfirmed, summarizePublishResults, } from "../nostrPublish.js"; import { @@ -469,7 +469,10 @@ export async function signAndPublishEvent({ let publishSummary; try { - publishSummary = assertAnyRelayAccepted(publishResults, { context }); + // #49: an all-timeout publish (sent, nothing rejected) is treated as an + // unconfirmed soft success — summary.unconfirmed=true — instead of a false + // "failed to publish". Explicit relay rejections still throw below. + publishSummary = assertAnyRelayAcceptedOrUnconfirmed(publishResults, { context }); } catch (publishError) { if (publishError?.relayFailures?.length) { const logLevel = rejectionLogLevel === "warn" ? "warn" : "error"; @@ -485,6 +488,12 @@ export async function signAndPublishEvent({ devLogger.log(`${logName} published to ${url}`); }); + if (publishSummary.unconfirmed) { + userLogger.warn( + `[nostr] ${logName} was sent but no relay confirmed it within the timeout — treating as an unconfirmed publish.`, + ); + } + if (publishSummary.failed.length) { publishSummary.failed.forEach(({ url, error: relayError }) => { const reason = @@ -1331,7 +1340,11 @@ export async function rebroadcastEvent({ client, eventId, options = {} }) { const publishResults = await publishEventToRelays(client?.pool, relays, rawEvent); try { - const summary = assertAnyRelayAccepted(publishResults, { context: "rebroadcast" }); + // #49: rebroadcast re-publishes an existing event, so an all-timeout is a + // safe unconfirmed success; only explicit rejections fall to the catch. + const summary = assertAnyRelayAcceptedOrUnconfirmed(publishResults, { + context: "rebroadcast", + }); return { ok: true, rebroadcast: true, diff --git a/js/nostrPublish.js b/js/nostrPublish.js index a9a7a751..c146f464 100644 --- a/js/nostrPublish.js +++ b/js/nostrPublish.js @@ -278,9 +278,16 @@ export function attachRelayPublishSummary(event, summary) { } const accepted = Array.isArray(summary?.accepted) ? summary.accepted.length : 0; const failed = Array.isArray(summary?.failed) ? summary.failed.length : 0; + // #49: carry the unconfirmed flag (all relays timed out, nothing rejected) + // so the outcome describer can say "sent, unconfirmed" instead of lying in + // either direction ("published to N relays" / "failed"). + const tally = + summary?.unconfirmed === true + ? { accepted, total: accepted + failed, unconfirmed: true } + : { accepted, total: accepted + failed }; try { Object.defineProperty(event, RELAY_PUBLISH_SUMMARY_KEY, { - value: { accepted, total: accepted + failed }, + value: tally, enumerable: false, configurable: true, writable: true, @@ -308,7 +315,11 @@ export function readRelayPublishSummary(event) { * @param {{ accepted?: number, total?: number }} tally * @returns {{ tone: "success"|"warning"|"error", message: string }} */ -export function describePublishOutcome({ accepted = 0, total = 0 } = {}) { +export function describePublishOutcome({ + accepted = 0, + total = 0, + unconfirmed = false, +} = {}) { const a = Number.isFinite(accepted) ? Math.max(0, Math.floor(accepted)) : 0; const t = Number.isFinite(total) ? Math.max(0, Math.floor(total)) : 0; @@ -317,6 +328,16 @@ export function describePublishOutcome({ accepted = 0, total = 0 } = {}) { return { tone: "success", message: "Video shared successfully!" }; } if (a <= 0) { + // #49: every relay timed out without rejecting — the event was sent and + // almost always persisted (it shows up after a refresh). Say exactly that: + // neither a hard failure nor a fake "published to N relays". + if (unconfirmed === true) { + return { + tone: "warning", + message: + "Video sent, but no relay has confirmed receiving it yet. It should appear shortly — refresh in a moment to double-check.", + }; + } return { tone: "error", message: @@ -335,6 +356,24 @@ export function describePublishOutcome({ accepted = 0, total = 0 } = {}) { }; } +// #49 delete semantics: a delete flow publishes several events (tombstone +// reverts + kind-5 chunks) and the UI must NOT claim "deleted" unless at least +// one relay actually ACKed something — a false "deleted" is worse than a false +// "failed". An empty list (nothing needed publishing) counts as confirmed. +export function anyRelayAcceptedInSummaries(summaries = []) { + const list = Array.isArray(summaries) ? summaries.filter(Boolean) : []; + if (!list.length) { + return true; + } + return list.some((summary) => { + const accepted = summary?.accepted; + if (Array.isArray(accepted)) { + return accepted.length > 0; + } + return Number.isFinite(accepted) && accepted > 0; + }); +} + // A RelayPublishError whose per-relay failures are ALL "publish timeout" means the // event was sent but not ACKed in the window — NOT that any relay rejected it. For // an idempotent, replaceable event this almost always persisted (users see it diff --git a/js/ui/editModalController.js b/js/ui/editModalController.js index 815891b2..2a74c06c 100644 --- a/js/ui/editModalController.js +++ b/js/ui/editModalController.js @@ -1,5 +1,6 @@ import { devLogger } from "../utils/logger.js"; import r2Service from "../services/r2Service.js"; +import { readRelayPublishSummary } from "../nostrPublish.js"; export default class EditModalController { constructor({ services, state, ui, callbacks, helpers }) { @@ -86,7 +87,7 @@ export default class EditModalController { } try { - await this.services.nostrService.handleEditVideoSubmit({ + const editedEvent = await this.services.nostrService.handleEditVideoSubmit({ originalEvent, updatedData, pubkey, @@ -139,7 +140,16 @@ export default class EditModalController { videosMap.clear(); } - this.ui.showSuccess("Video updated successfully!"); + // #49: an all-timeout publish reaches here flagged unconfirmed — say so + // honestly instead of claiming a confirmed update. + const tally = readRelayPublishSummary(editedEvent); + if (tally?.unconfirmed === true && !(tally.accepted > 0)) { + this.ui.showSuccess( + "Video update sent, but no relay has confirmed it yet — it may take a moment to propagate.", + ); + } else { + this.ui.showSuccess("Video updated successfully!"); + } if (editModal?.setSubmitState) { editModal.setSubmitState({ pending: false }); diff --git a/js/ui/revertModalController.js b/js/ui/revertModalController.js index 8815ff6d..deb76d68 100644 --- a/js/ui/revertModalController.js +++ b/js/ui/revertModalController.js @@ -91,8 +91,12 @@ export default class RevertModalController { this.revertModal.setBusy(true, "Reverting…"); try { + // #49: revertVideo no longer hard-fails when every relay merely times + // out (summary.unconfirmed) — track whether ANY relay actually ACKed so + // the success message stays honest. + let anyAccepted = false; for (const entry of entries) { - await this.services.nostrClient.revertVideo( + const revertResult = await this.services.nostrClient.revertVideo( { id: entry.id, pubkey: entry.pubkey, @@ -100,12 +104,22 @@ export default class RevertModalController { }, pubkey, ); + const accepted = revertResult?.summary?.accepted; + if (Array.isArray(accepted) && accepted.length > 0) { + anyAccepted = true; + } } await this.callbacks.loadVideos(); const timestampLabel = this.helpers.formatAbsoluteTimestamp(target.created_at); - this.ui.showSuccess(`Reverted to revision from ${timestampLabel}.`); + if (anyAccepted) { + this.ui.showSuccess(`Reverted to revision from ${timestampLabel}.`); + } else { + this.ui.showSuccess( + `Revert to the revision from ${timestampLabel} was sent, but no relay has confirmed it yet — check again in a moment.`, + ); + } this.revertModal.close(); this.callbacks.forceRefreshAllProfiles(); } catch (err) { diff --git a/tests/relay-ack-unconfirmed-content.test.mjs b/tests/relay-ack-unconfirmed-content.test.mjs new file mode 100644 index 00000000..3284e16d --- /dev/null +++ b/tests/relay-ack-unconfirmed-content.test.mjs @@ -0,0 +1,168 @@ +// #49: relay-ack false-negatives on the FLAGSHIP content flows. When no relay +// ACKs within the publish window but nothing rejected (all-timeout), the event +// was sent and almost always persisted — it must surface as an UNCONFIRMED soft +// success, not "Failed to share video". Equally important: the outcome messaging +// must stay honest — unconfirmed is never reported as "published to N relays", +// and a delete is only "deleted" when at least one relay ACKed something. +// +// test_integrity_note: +// change_type: ["new_tests"] +// scenarios: +// - id: SCN-content-publish-all-timeout +// given: "a relay pool where every relay times out (reason 'publish timeout') vs one that rejects" +// when: "signAndPublishEvent (video publish/edit path) runs" +// then: "all-timeout resolves with summary.unconfirmed=true; an explicit rejection still throws RelayPublishError" +// - id: SCN-honest-outcome-messaging +// given: "relay tallies including the new unconfirmed flag" +// when: "describePublishOutcome / attachRelayPublishSummary / anyRelayAcceptedInSummaries run" +// then: "unconfirmed -> warning ('sent, not confirmed'), never a relay-count success and never a hard error; deletes only 'confirmed' with >=1 ACK" +// observable_outcomes: +// - "signAndPublishEvent resolves (no throw) on all-timeout with unconfirmed summary" +// - "signAndPublishEvent still throws on explicit rejection" +// - "unconfirmed tally round-trips through attach/readRelayPublishSummary" +// - "describePublishOutcome(unconfirmed) is a warning with honest wording" +// determinism_controls: +// - "fake pool fires ok/failed handlers synchronously; no real sockets or timers race the 10s window" +// anti_cheat_rationale: +// prevents: ["hard-coded return value", "over-mocking internal logic", "snapshot rubber-stamping"] +// relaxation: +// did_relax_any_assertion: false + +import test from "node:test"; +import { strict as assert } from "node:assert"; +import { + RelayPublishError, + describePublishOutcome, + attachRelayPublishSummary, + readRelayPublishSummary, + anyRelayAcceptedInSummaries, +} from "../js/nostrPublish.js"; +import { signAndPublishEvent } from "../js/nostr/publishHelpers.js"; + +const PUB = "a".repeat(64); + +// Fake nostr-tools pool: publish([url]) returns a handle whose "failed" (or +// "ok") handler fires immediately with the scripted outcome for that relay. +function makePool(outcomes) { + let call = 0; + return { + publish(urls, _event) { + const outcome = outcomes[call % outcomes.length]; + call += 1; + return { + on(eventName, handler) { + if (outcome === "ok" && eventName === "ok") { + handler(); + } else if (outcome !== "ok" && eventName === "failed") { + handler(new Error(outcome)); + } + return this; + }, + }; + }, + }; +} + +const signer = { + pubkey: PUB, + signEvent: async (event) => ({ ...event, id: "signed-1", sig: "sig" }), +}; + +function publishWith(pool) { + return signAndPublishEvent({ + client: { + pubkey: PUB, + pool, + relays: ["wss://a.example", "wss://b.example"], + writeRelays: ["wss://a.example", "wss://b.example"], + }, + event: { kind: 30078, pubkey: PUB, created_at: 100, tags: [], content: "{}" }, + resolveActiveSigner: () => signer, + shouldRequestExtensionPermissions: () => false, + options: { context: "video note", logName: "Video note" }, + }); +} + +test("all-timeout video publish resolves as an unconfirmed soft success", async () => { + const result = await publishWith(makePool(["publish timeout", "publish timeout"])); + assert.equal(result.signedEvent.id, "signed-1"); + assert.equal(result.summary.unconfirmed, true, "flagged unconfirmed"); + assert.equal(result.summary.accepted.length, 0, "no fake acceptances"); + assert.equal(result.summary.failed.length, 2, "both timeouts recorded"); +}); + +test("an explicit relay rejection still hard-fails the publish", async () => { + await assert.rejects( + publishWith(makePool(["publish timeout", "blocked: spam"])), + RelayPublishError, + ); +}); + +test("a confirmed ack stays a confirmed (not unconfirmed) success", async () => { + const result = await publishWith(makePool(["ok", "publish timeout"])); + assert.equal(result.summary.unconfirmed, false); + assert.equal(result.summary.accepted.length, 1); +}); + +test("the attached tally carries the unconfirmed flag to the UI", () => { + const event = { id: "e1", kind: 30078 }; + attachRelayPublishSummary(event, { + accepted: [], + failed: [{ url: "wss://a" }, { url: "wss://b" }], + unconfirmed: true, + }); + assert.deepEqual(readRelayPublishSummary(event), { + accepted: 0, + total: 2, + unconfirmed: true, + }); + + // Confirmed publishes keep the exact legacy tally shape (no stray flag). + const confirmed = { id: "e2" }; + attachRelayPublishSummary(confirmed, { + accepted: [{ url: "wss://a" }], + failed: [], + }); + assert.deepEqual(readRelayPublishSummary(confirmed), { accepted: 1, total: 1 }); +}); + +test("describePublishOutcome: unconfirmed is an honest warning, not success or error", () => { + const out = describePublishOutcome({ accepted: 0, total: 3, unconfirmed: true }); + assert.equal(out.tone, "warning"); + assert.match(out.message, /no relay has confirmed/i); + assert.doesNotMatch(out.message, /\d+ relays?/, "must not fake a relay count"); + assert.doesNotMatch(out.message, /could not be published/i); + + // Without the flag, zero acceptance is still the hard error. + assert.equal(describePublishOutcome({ accepted: 0, total: 3 }).tone, "error"); + // A real acceptance is unaffected by a stray flag. + assert.equal( + describePublishOutcome({ accepted: 2, total: 3, unconfirmed: false }).tone, + "warning", + ); + assert.equal(describePublishOutcome({ accepted: 3, total: 3 }).tone, "success"); +}); + +test("delete confirmation: only >=1 relay ACK counts as a confirmed delete", () => { + const unconfirmedSummary = { accepted: [], failed: [{ url: "wss://a" }], unconfirmed: true }; + const confirmedSummary = { accepted: [{ url: "wss://b" }], failed: [] }; + + assert.equal( + anyRelayAcceptedInSummaries([unconfirmedSummary, unconfirmedSummary]), + false, + "all-timeout delete must NOT be reported as deleted", + ); + assert.equal( + anyRelayAcceptedInSummaries([unconfirmedSummary, confirmedSummary]), + true, + ); + assert.equal( + anyRelayAcceptedInSummaries([]), + true, + "nothing needed publishing counts as confirmed", + ); + assert.equal(anyRelayAcceptedInSummaries([null, undefined]), true); + // Numeric tally shape (from readRelayPublishSummary) also works. + assert.equal(anyRelayAcceptedInSummaries([{ accepted: 0, total: 2 }]), false); + assert.equal(anyRelayAcceptedInSummaries([{ accepted: 1, total: 2 }]), true); +});