From c767298368a5b8495ca80a201218cbb9462eb39c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 15:14:17 +0000 Subject: [PATCH] =?UTF-8?q?fix(quartz):=20audit=20fixes=20=E2=80=94=20fore?= =?UTF-8?q?ign-OK=20confirmation=20bug,=20normalizer=20hot-path=20allocati?= =?UTF-8?q?on?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit findings across the branch, each verified with a failing test or measurement before the fix: - publishAndCollectResults counted an OK from a relay OUTSIDE relayList (same event id — a probe-wave straggler, or any republish of the same event to a different relay set) toward its confirmation window, ending the wait loop early and misreporting still-pending listed relays as NO_RESPONSE. The OK branch now carries the same relayList guard the onCannotConnect/onDisconnected branches always had. Regression test proves the failure without the guard. readWriteCheck additionally varies the probe event content per wave so wave N's confirmation window can never match wave N-1's event id at all. - RelayUrlNormalizer.fix() called trimEnd('%','2','0') unconditionally, allocating a full string copy for ANY url merely ending in '%', '2' or '0' — which includes every relay port ending in zero (wss://host:3030). Now gated on endsWith("%20"), keeping the hot path allocation-free; semantics unchanged (test pins both the trim and the untouched-port cases). - amy relay probe --file: unreadable file is now a clean bad_args error instead of a stack trace, and skipped onion urls are counted and reported (file_onion_skipped) instead of vanishing from the tally. - probeFlow KDoc now states that a slow collector eats into the current wave's absolute deadline (answers are still recorded; silent relays get less listening time), not just that it delays the next wave. Verified non-issues: androidx.collection LruCache is internally locked (safe for CachedNip11Fetcher/normalizer concurrency); probeWave's per-terminal emission cannot lose or double-emit verdicts (remaining-set guard, data maps read at emission time); existing publish callers all benefit from the OK guard rather than depending on the old behavior. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01A9sSyh1QLJD3PZ18tVPPVK --- .../amethyst/cli/commands/RelayCommands.kt | 12 +++++-- .../accessories/NostrClientPublishExt.kt | 7 +++- .../relay/normalizer/RelayUrlNormalizer.kt | 11 +++---- .../reachability/RelayProber.kt | 14 +++++--- .../nip01Core/relay/RelayUrlFormatterTest.kt | 8 +++++ .../reachability/RelayProberFlowTest.kt | 32 +++++++++++++++++++ 6 files changed, 70 insertions(+), 14 deletions(-) diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/RelayCommands.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/RelayCommands.kt index 8734ba1b02..292a19b6b0 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/RelayCommands.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/RelayCommands.kt @@ -349,21 +349,26 @@ object RelayCommands { var fileRaw = 0 var fileRejected = 0 + var fileOnion = 0 val fileRelays = HashSet() if (fromFile != null) { - File(fromFile).forEachLine { line -> + val candidates = File(fromFile) + if (!candidates.canRead()) return Output.error("bad_args", "cannot read --file $fromFile") + candidates.forEachLine { line -> if (line.isBlank()) return@forEachLine fileRaw++ val normalized = line.normalizeRelayUrlOrNull() if (normalized == null) { fileRejected++ - } else if (!RelayUrlNormalizer.isOnion(normalized.url)) { + } else if (RelayUrlNormalizer.isOnion(normalized.url)) { + fileOnion++ + } else { fileRelays.add(normalized) } } System.err.println( "[relay-probe] $fromFile: $fileRaw urls → ${fileRelays.size} unique clearnet relays " + - "($fileRejected rejected by the normalizer)", + "($fileRejected rejected by the normalizer, $fileOnion onion skipped)", ) } @@ -412,6 +417,7 @@ object RelayCommands { "file_urls" to (if (fromFile != null) fileRaw else null), "file_normalized" to (if (fromFile != null) fileRelays.size else null), "file_rejected" to (if (fromFile != null) fileRejected else null), + "file_onion_skipped" to (if (fromFile != null) fileOnion else null), "reachable" to result.reachable.size, "dead" to result.dead.size, "closed_by_policy" to authWalled, diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientPublishExt.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientPublishExt.kt index 112345d123..c1aa8c14a2 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientPublishExt.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientPublishExt.kt @@ -141,7 +141,12 @@ suspend fun INostrClient.publishAndCollectResults( when (msg) { is OkMessage -> { - if (msg.eventId == event.id) { + // The relayList guard matters, not just the id: the same event may + // have been published to OTHER relays by an earlier call (probe + // waves, republish), and counting their late OKs here would inflate + // receivedResults and end the wait loop before every listed relay + // answered — misreporting the missing ones as NO_RESPONSE. + if (msg.eventId == event.id && relay.url in relayList) { resultChannel.trySend(DetailedResult(relay.url, msg.success, msg.message, mark.elapsedNow().inWholeMilliseconds)) Log.d("publishAndConfirm") { "onSendResponse Received response for ${msg.eventId} from relay ${relay.url} message ${msg.message} success ${msg.success}" } } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/normalizer/RelayUrlNormalizer.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/normalizer/RelayUrlNormalizer.kt index 8949a0b82c..bc5c524482 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/normalizer/RelayUrlNormalizer.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/normalizer/RelayUrlNormalizer.kt @@ -180,12 +180,11 @@ class RelayUrlNormalizer { if (rawUrl.length < 4) return null if (rawUrl.contains("%00")) return null - // Trim trailing %20 (percent-encoded spaces from malformed event data) - val url = - rawUrl.trimEnd('%', '2', '0').let { trimmed -> - // Only accept if we actually removed a trailing %20 pattern - if (trimmed.length < rawUrl.length && rawUrl.endsWith("%20")) trimmed else rawUrl - } + // Trim trailing %20 (percent-encoded spaces from malformed event data). + // The endsWith gate keeps the hot path allocation-free: trimEnd would + // copy the string for ANY url merely ending in '%', '2' or '0' — which + // includes every port ending in zero ("wss://host:3030"). + val url = if (rawUrl.endsWith("%20")) rawUrl.trimEnd('%', '2', '0') else rawUrl if (url.length < 4) return null // Reject URLs with %20 in the middle — these are garbage diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip66RelayMonitor/reachability/RelayProber.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip66RelayMonitor/reachability/RelayProber.kt index 86e57c7e58..5dcf878aad 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip66RelayMonitor/reachability/RelayProber.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip66RelayMonitor/reachability/RelayProber.kt @@ -152,8 +152,11 @@ class RelayProber( * [filters] picks the check, as in [probe]: [LIVENESS_FILTERS] (default) or * [readTestFilter]. * - * Probing starts when the flow is collected and pauses between waves while the - * collector is busy (emission is sequential). Pair each verdict with + * Probing starts when the flow is collected, and emission is sequential — a slow + * collector delays the next wave AND eats into the current wave's [timeoutMs] + * window (the deadline is absolute; answers keep being recorded while the + * collector runs, but silent relays get less listening time). Keep per-verdict + * work light, or buffer, when precise deadlines matter. Pair each verdict with * [toDiscoveryEventTemplate] to turn the stream into signable NIP-66 kind:30166 * records for another process to sign and publish. */ @@ -194,11 +197,14 @@ class RelayProber( ): Map { val out = HashMap() val distinct = relays.toSet() - for (wave in distinct.chunked(waveSize.coerceAtLeast(1))) { + for ((waveIndex, wave) in distinct.chunked(waveSize.coerceAtLeast(1)).withIndex()) { val reads = HashMap() probeWave(wave, timeoutMs, readTestFilter(readLimit)) { reads[it.relay] = it.rttEoseMs } - val event = signer.sign(RelayProbeWriteTest.build()) + // A distinct event id per wave (createdAt has second granularity, so the + // content must vary) keeps a straggler OK from an earlier wave's relays + // from ever matching this wave's confirmation window. + val event = signer.sign(RelayProbeWriteTest.build(content = "NIP-66 write probe $waveIndex")) val writes = client.publishAndCollectResults(event, wave.toSet(), (timeoutMs / 1000).coerceAtLeast(1)) for (relay in wave) { diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/RelayUrlFormatterTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/RelayUrlFormatterTest.kt index fc708eddba..6357c09591 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/RelayUrlFormatterTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/RelayUrlFormatterTest.kt @@ -53,6 +53,14 @@ class RelayUrlFormatterTest { assertNull(RelayUrlNormalizer.normalizeOrNull("wss://relay%20list%20to%20discover%20the%20user's%20content")) } + @Test + fun trailingPercentTwentyIsTrimmedButBareTrailingZeroIsNot() { + assertEquals("wss://nostr.mom/", RelayUrlNormalizer.normalizeOrNull("wss://nostr.mom%20")?.url) + // urls merely ending in '%', '2' or '0' (every port ending in zero) must pass untouched + assertEquals("wss://nostr.mom:3030/", RelayUrlNormalizer.normalizeOrNull("wss://nostr.mom:3030")?.url) + assertEquals("wss://nostr.mom:8020/", RelayUrlNormalizer.normalizeOrNull("wss://nostr.mom:8020")?.url) + } + @Test fun httpWithPathIsNotARelay() { // Mastodon/bridge actor urls from `proxy` tags: web resources, not relays diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip66RelayMonitor/reachability/RelayProberFlowTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip66RelayMonitor/reachability/RelayProberFlowTest.kt index 0195e52b53..153c6e5d3e 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip66RelayMonitor/reachability/RelayProberFlowTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip66RelayMonitor/reachability/RelayProberFlowTest.kt @@ -298,6 +298,38 @@ class RelayProberFlowTest { assertTrue(verdict.rttWriteMs >= 0, "a rejection is still a round trip") } + @Test + fun foreignRelayOkDoesNotEndTheWriteConfirmationEarly() = + runTest { + // A relay OUTSIDE the checked set answering with the same event id (a + // straggler from an earlier wave that got the same probe event) must not + // count toward the confirmation window — before the relayList guard in + // publishAndCollectResults, it ended the wait early and misreported the + // real relay as silent. + val client = ScriptedClient() + val signer = NostrSignerInternal(KeyPair()) + val foreign = RelayUrlNormalizer.normalize("wss://foreign.example.com") + var result: Map? = null + + val check = + launch { + result = RelayProber(client).readWriteCheck(listOf(fast), signer, timeoutMs = 5_000) + } + launch { + delay(50) + client.listener!!.onEose(fast, null) + while (client.published == null) delay(10) + client.answerOk(foreign, true, "") + delay(100) + client.answerOk(fast, true, "") + } + check.join() + + val verdict = result!![fast]!! + assertEquals(true, verdict.writeAccepted, "the listed relay's OK must still be awaited and recorded") + assertNull(result!![foreign], "the foreign relay must not appear in the result") + } + @Test fun silentWriteLeavesTheWriteSideUnobserved() = runTest {