From 8b9875d9cc70ec51109e357c9637530f5a50aadf Mon Sep 17 00:00:00 2001 From: nrobi144 Date: Sun, 7 Jun 2026 14:36:29 +0300 Subject: [PATCH] fix(quartz): NIP-46 bunker double-resume + retry id-reuse races MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two correctness bugs in `RemoteSignerManager` (NIP-46) and its NIP-55 sibling `IntentRequestManager`: 1. **Double-resume crash** — `awaitingRequests.get(id)?.resume(value)` was non-atomic. Multi-relay delivery, bunker echo/retry, and late-after-timeout responses could call `resume` twice for the same continuation, throwing `IllegalStateException: Already resumed` on a `Dispatchers.Default` worker. 2. **Retry id-reuse → wrong data** (NIP-46 only) — `launchWaitAndParse` built the request and event once, then re-used the same `request.id` across retry attempts. A late response from attempt N could resume attempt N+1's continuation with stale data. Replace the cached-`Continuation` map with the in-house Channel-per-request correlation pattern already used in `quartz/.../accessories/NostrClientPublishExt.kt` (`LargeCache(capacity=1)>` + atomic `remove` + `trySend` + `withTimeoutOrNull { receive() }`). Each retry attempt now builds a fresh request with a new id; the builder is still called only once. `finally`-block cleanup removes the cache entry on every path, incidentally fixing a slow leak on the success path. Adds three regression tests: - duplicate responses → no crash + single resume (fails on \`main\` with \`IllegalStateException\`) - late response after timeout → silently discarded - late attempt-1 response does not corrupt attempt-2 result (fails on \`main\`: the two attempts share an id) Design + review notes: \`quartz/plans/2026-06-03-fix-nip46-bunker-double-resume-plan.md\` --- ...-03-fix-nip46-bunker-double-resume-plan.md | 523 ++++++++++++++++++ .../api/foreground/IntentRequestManager.kt | 118 ++-- .../signer/RemoteSignerManager.kt | 67 ++- .../signer/RemoteSignerManagerRetryTest.kt | 210 +++++++ 4 files changed, 824 insertions(+), 94 deletions(-) create mode 100644 quartz/plans/2026-06-03-fix-nip46-bunker-double-resume-plan.md diff --git a/quartz/plans/2026-06-03-fix-nip46-bunker-double-resume-plan.md b/quartz/plans/2026-06-03-fix-nip46-bunker-double-resume-plan.md new file mode 100644 index 0000000000..03351fd0a4 --- /dev/null +++ b/quartz/plans/2026-06-03-fix-nip46-bunker-double-resume-plan.md @@ -0,0 +1,523 @@ +--- +title: "fix(quartz): NIP-46 bunker double-resume + retry id-reuse races" +type: fix +status: completed +date: 2026-06-03 +origin: docs/brainstorms/2026-06-03-nip46-bunker-double-resume-brainstorm.md +--- + +# fix(quartz): NIP-46 bunker double-resume + retry id-reuse races + +## Revision Note (2026-06-03, post-review) + +Three reviews (simplicity / architecture / pattern-recognition) converged on +pivoting the approach. Original plan used atomic-remove + `tryResume` on a +cached `Continuation` map. **Revised approach: Channel-per-request (per +retry attempt) + fresh `request.id` per attempt**, matching the de facto +Quartz convention used in `NostrClientPublishExt.kt` and 4 sibling files +under `quartz/.../accessories/`. + +### Why the pivot + +| Driver | Detail | +|---|---| +| **Architecture review finding** | Quartz already uses Channel-per-request as house style (5+ files). The cached-`Continuation` pattern in `RemoteSignerManager` / `IntentRequestManager` is the outlier — only 2 files. | +| **4th failure mode discovered** | `RemoteSignerManager.kt:74-101` retry loop reuses the same `request.id` across attempts. Late response from attempt 1 can resume attempt 2's continuation with **stale data** (correctness bug, not just crash). Investigation verdict: HIGH probability on flaky relays. Fresh-id-per-retry naturally fixes this. | +| **Simplicity review** | Pivoting eliminates the `@InternalCoroutinesApi` opt-in surface entirely, kills the Plan C / SingleShotContinuation alternative discussions, removes the `TestLogCapture` test-infra invention, and removes the Strategy B stress-loop test. | +| **Pattern review** | "Aligns the existing outlier with the convention" — fewer correlation styles in the codebase, not more. | + +### Scope unchanged from original plan + +- Both managers fixed in the same PR (NIP-46 + NIP-55 sibling). +- Function-level concurrency primitives — no public API change to + `launchWaitAndParse`. 17 call sites of `launchWaitAndParse` across + `NostrSignerRemote.kt`, `ForegroundRequestHandler.kt`, and the retry test + are untouched. +- `tryAndWait` (`ParallelUtils.kt:71-79`) is **retained** — still used by + `collectSuccessfulOperationsReturning` (`ParallelUtils.kt:98`). Only its + use inside the two managers is replaced. + +--- + +## Overview + +Fix two related correctness bugs in the NIP-46 bunker signer +(`RemoteSignerManager`) and its NIP-55 Android sibling +(`IntentRequestManager`): + +1. **Double-resume crash** — `Continuation.resume(...)` called twice for + the same id, throwing `IllegalStateException: Already resumed`. + Triggered by (a) multi-relay delivery, (b) bunker echo/retry, + (c) late response after `tryAndWait` timeout fires. +2. **Retry id-reuse → wrong-data bug** (NIP-46 only) — retry attempts + reuse the same `request.id`, so a late response from attempt N can + resume attempt N+1's continuation with attempt N's data. + +The fix replaces the cached `Continuation` map with a +**Channel-per-request** correlation pattern (mirroring the convention +in `quartz/.../accessories/NostrClientPublishExt.kt`), and regenerates +`request.id` per retry attempt. + +## Problem Statement + +### Bug 1 — Double-resume crash + +```kotlin +// RemoteSignerManager.kt:46-50 +suspend fun newResponse(responseEvent: NostrConnectEvent) { + val decryptedJson = signer.decrypt(responseEvent.content, remoteKey) + val bunkerResponse = OptimizedJsonMapper.fromJsonTo(decryptedJson) + awaitingRequests.get(bunkerResponse.id)?.resume(bunkerResponse) // ← unsafe +} +``` + +Non-atomic `get(id)?.resume(value)` — three races trigger double-resume: + +| Race | Sequence | Result | +|---|---|---| +| **Late response after timeout** | `tryAndWait`'s `withTimeoutOrNull` completes continuation with `null`; bunker's actual response arrives ms later; `newResponse` calls `resume` on already-completed continuation. | `IllegalStateException` at `RemoteSignerManager.kt:49` | +| **Multi-relay delivery (NIP-46 only)** | `NostrSignerRemote.kt:82` fires `scope.launch { manager.newResponse(event) }` per delivered event, no dedupe. Two relays delivering same response → two concurrent `newResponse` calls → both `get` same continuation → both `resume`. | First wins, second throws. | +| **Bunker echo / retry (NIP-46 only)** | Some bunker servers re-publish on relay reconnect. Same as multi-relay but with longer time gap. | Second resume throws. | + +Stack trace: + +``` +Exception in thread "DefaultDispatcher-worker-42" java.lang.IllegalStateException: + Already resumed, but proposed with update BunkerResponse@… + at kotlinx.coroutines.CancellableContinuationImpl.alreadyResumedError(CancellableContinuationImpl.kt:556) + at com.vitorpamplona.quartz.nip46RemoteSigner.signer.RemoteSignerManager.newResponse(RemoteSignerManager.kt:49) + at com.vitorpamplona.quartz.nip46RemoteSigner.signer.NostrSignerRemote$subscription$2$1.invokeSuspend(NostrSignerRemote.kt:83) +``` + +### Bug 2 — Retry id-reuse → wrong data (NIP-46 only) + +```kotlin +// RemoteSignerManager.kt:66-101 (paraphrased) +val request = buildRequest(...) // ← request.id assigned ONCE +val event = signer.encrypt(request, ...) // event id derived once +var attempt = 0 +while (true) { + val result = tryAndWait(timeout) { continuation -> + continuation.invokeOnCancellation { awaitingRequests.remove(request.id) } + awaitingRequests.put(request.id, continuation) // ← SAME id every attempt + client.publish(event, relayList = relayList) + } + when { + result != null -> return parser(result) + attempt >= maxRetries -> return SignerResult.RequestAddressed.TimedOut() + else -> { attempt++; delay(2_000L) } + } +} +``` + +Race sequence (default `timeout = 65_000L`): + +``` +T+0: Attempt 1: put(continuation_1, "req123") +T+65s: Attempt 1: timeout → invokeOnCancellation removes "req123" +T+67s: Attempt 2: put(continuation_2, "req123") ← same id +T+70s: Bunker's late response for ATTEMPT 1 arrives + newResponse() resumes continuation_2 with attempt_1's payload + ⚠ wrong data delivered to caller +``` + +Likelihood: HIGH when relay RTT approaches timeout. Atomic-remove + `tryResume` +**does not fix this** — continuation_2 is genuinely live; `tryResume` +succeeds with stale data. Only fresh-id-per-attempt closes this race. + +`IntentRequestManager.kt:119` already uses a fresh `RandomInstance.randomChars(32)` per +call and has no retry loop — Bug 2 does not apply there. + +## Proposed Solution + +### Mechanism: Channel-per-request + +```kotlin +// after — RemoteSignerManager (paraphrased shape) + +private val pending = ConcurrentHashMap>() + +suspend fun newResponse(responseEvent: NostrConnectEvent) { + val decryptedJson = signer.decrypt(responseEvent.content, remoteKey) + val bunkerResponse = OptimizedJsonMapper.fromJsonTo(decryptedJson) + + // Atomic remove. Multi-relay / bunker-echo duplicates: losers see null. + val channel = pending.remove(bunkerResponse.id) + if (channel == null) { + Log.d("NIP46") { "no channel for bunker response id=${bunkerResponse.id} (duplicate or unknown)" } + return + } + // capacity = 1: first delivery wins. trySend on a closed/full channel + // is a no-op — late response after timeout cannot crash. + channel.trySend(bunkerResponse) +} + +private suspend fun launchWaitAndParse( + request: BunkerRequest, + parser: (BunkerResponse) -> T, +): T { + var attempt = 0 + while (true) { + // Fresh id per attempt: each attempt is a brand-new request to the bunker. + val attemptRequest = request.copy(id = RandomInstance.randomChars(32)) + val event = signer.encrypt(attemptRequest, ...) + val channel = Channel(capacity = 1) + pending[attemptRequest.id] = channel + try { + client.publish(event, relayList = relayList) + val response = withTimeoutOrNull(timeout) { channel.receive() } + when { + response != null -> return parser(response) + attempt >= maxRetries -> return SignerResult.RequestAddressed.TimedOut() + else -> { attempt++; delay(2_000L) } + } + } finally { + pending.remove(attemptRequest.id) // cleanup on both happy + timeout paths + channel.close() + } + } +} +``` + +### Mechanism applied to `IntentRequestManager` + +Same shape, no retry loop, single attempt — `IntentResult` instead of +`BunkerResponse`, `LruCache` becomes `ConcurrentHashMap` (which is also +the structure used by the existing Channel-per-request files), and the +log tag becomes `"NIP55"`. + +### Why this fix + +| Property | Cached `Continuation` (today) | Atomic `remove` + `tryResume` (original plan) | Channel-per-request (this plan) | +|---|---|---|---| +| Double-resume crash | ❌ | ✅ | ✅ | +| Late response after timeout | ❌ | ✅ (`tryResume` returns null) | ✅ (`trySend` on closed channel is a no-op) | +| Multi-relay delivery | ❌ | ✅ (atomic remove) | ✅ (atomic remove) | +| Bunker echo / retry | ❌ | ✅ | ✅ | +| **Retry id-reuse → wrong data** | ❌ | ❌ | ✅ (fresh id per attempt) | +| `@InternalCoroutinesApi` | n/a | required | not required | +| House style match | outlier | outlier (still cached map) | ✅ matches `accessories/` convention | +| Memory leak on success path | leaks | incidentally fixed | fixed (finally block) | + +## Technical Considerations + +### Channel capacity = 1 + +Each request has at most one valid response (NIP-46 `auth_url` flow not +implemented in Amethyst — see "NIP-46 spec" below). `Channel(capacity = 1)`: + +- First `trySend` succeeds → `receive` resumes with the value. +- Concurrent / late `trySend` after the channel has been drained returns + a `ChannelResult.Closed` once the `finally` block calls `close()`. No-op, + no throw. +- `withTimeoutOrNull(timeout) { channel.receive() }` returns `null` on + timeout; the `finally` block cleans up the map entry and closes the + channel. + +### Fresh `request.id` per retry attempt + +NIP-46 has no idempotency contract — each retry is a new request from the +bunker's perspective. Generating a fresh id per attempt is consistent +with the spec and is also what `IntentRequestManager` already does for +single attempts. + +The user-visible cost: a slow bunker that finishes processing attempt 1 +mid-way through attempt 2 will not have its attempt-1 work "rescued" — +the response is discarded and attempt 2's response is the one we return. +This is the **correct** behaviour; the alternative (rescuing attempt 1 +into attempt 2's slot) is the very bug we're fixing. + +### `IntentRequestManager.LruCache` → `ConcurrentHashMap` + +`androidx.collection.LruCache` was sized at 2000 for bounded growth. +With the `finally`-block cleanup the map shrinks on every completed call, +so an unbounded `ConcurrentHashMap` matches the Quartz `accessories/` +convention without leak risk. (If we ever want a safety cap, `Caffeine` +is in the dependency graph already, but YAGNI.) + +### NIP-46 spec: `auth_url` (multi-response per id) + +Spec allows a second response per id when the bunker emits an `auth_url` +challenge first. **Amethyst has zero `auth_url` handling code today**; +every parser maps any non-null `error` to `Rejected`. Channel(capacity=1) +mirrors that current contract: first response is terminal. If `auth_url` +support is added later, the right fix is at the parser / +`launchWaitAndParse` layer (e.g., keep the channel open until the parser +returns `SignerResult.AwaitingAuth`, then `receive` again). Not in scope +here. + +### `tryAndWait` retained for `collectSuccessfulOperationsReturning` + +`tryAndWait` is still used by `ParallelUtils.kt:98` +(`collectSuccessfulOperationsReturning`). We are not deleting it — +only its uses inside the two managers go away. + +### Performance implications + +None measurable. One `Channel(1)` allocation + one entry in +`ConcurrentHashMap` per request, both freed in the `finally` block. +NIP-46 throughput is < 10 req/s in practice; this is noise. + +### Security considerations + +None. Thread-safety + correctness only; no protocol surface change, +no new trust assumptions, no new data exposed. + +## System-Wide Impact + +- **Interaction graph:** Relay → `INostrClient.subscribe` → + `NostrSignerRemote` callback → `scope.launch` → `manager.newResponse` → + channel `trySend` → `launchWaitAndParse`'s `receive` unblocks → + parser returns to Amethyst UI. The fix sits at the correlation layer; + everything downstream is unchanged. Upstream (`withTimeoutOrNull`, the + subscription callback) is unchanged. +- **`launchWaitAndParse` public signature unchanged.** All 17 call sites + (8 in `NostrSignerRemote.kt`, 9 in `ForegroundRequestHandler.kt`, + 4 in tests) are untouched. +- **State lifecycle:** `pending` is the only mutable state. Atomic + `ConcurrentHashMap.put / remove` semantics + `finally`-block cleanup + guarantee no entries leak. +- **Error propagation:** Currently the `IllegalStateException` is thrown + on a `Dispatchers.Default` worker inside `scope.launch { ... }`. After + fix, no exception path remains — duplicates trigger a debug log line. +- **Integration test scenarios** (manual / amy): + 1. **NIP-46 cold-start with multi-relay delivery:** subscribe on N=5 relays → send request → all 5 deliver same response → only one resume, no crash. + 2. **NIP-46 late-response after timeout:** request with `timeout=100ms` against a bunker that responds at 200ms → returns `TimedOut`, debug log fires. + 3. **NIP-46 retry loop with mid-flight stale response:** force `timeout < network RTT` (`timeout=100ms`, RTT=300ms) → attempt 1 times out, attempt 2 in flight, attempt 1's actual response arrives → silently discarded; attempt 2's eventual response is what we return. + 4. **NIP-55 multi-result Intent:** Android signer returns Intent with multiple `results` entries that defensively repeat the same id → no crash. + +## Acceptance Criteria + +### Functional + +- [x] `RemoteSignerManager.newResponse` never throws — late responses, duplicates from multiple relays, and bunker echoes are all silently dropped after a debug log. +- [x] `IntentRequestManager.newResponse` has the equivalent guarantee. +- [x] Each retry attempt in `RemoteSignerManager.launchWaitAndParse` uses a fresh `request.id`. A late response from attempt N cannot resume attempt N+1. +- [x] `pending` map entries are always removed in a `finally` block — no leaks on success, timeout, or thrown exception. +- [x] Caller-visible behaviour of `launchWaitAndParse` is unchanged in the happy path: same return value, same retry semantics, same `SignerResult.RequestAddressed` shapes. + +### Non-functional + +- [x] No `@OptIn(InternalCoroutinesApi::class)` introduced. +- [x] Channel capacity = 1; channel scoped to a single retry attempt (created inside loop, closed in `finally`). + +### Quality gates + +- [x] `./gradlew :quartz:compileKotlinJvm :quartz:compileKotlinAndroid` passes. +- [x] `./gradlew :quartz:jvmTest --tests "*RemoteSignerManager*"` passes, including new race-condition tests. +- [x] `./gradlew spotlessApply` clean before commit. +- [x] Existing `RemoteSignerManagerRetryTest` (5 tests) still passes. +- [x] Three new tests added covering: (a) duplicate response → no crash + one successful resume; (b) late response after timeout → no crash + caller sees `TimedOut`; (c) cross-attempt stale-response → attempt 1's late response does NOT corrupt attempt 2's result. + +## Implementation Plan + +### File-level changes + +``` +quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/ +└── RemoteSignerManager.kt # MODIFY + +quartz/src/androidMain/kotlin/com/vitorpamplona/quartz/nip55AndroidSigner/api/foreground/ +└── IntentRequestManager.kt # MODIFY + +quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/ +└── RemoteSignerManagerRetryTest.kt # MODIFY (add three tests) +``` + +### Step 1 — `RemoteSignerManager.kt` + +Replace `awaitingRequests` cache + `tryAndWait`-based loop with +Channel-per-attempt + fresh id: + +- Cache type: `ConcurrentHashMap>` +- `newResponse`: atomic `remove` + `trySend` on `Channel(1)` (no-op on closed/full). +- `launchWaitAndParse`: inside the retry loop, copy request with fresh + `request.id` via `RandomInstance.randomChars(32)`, create + `Channel(capacity = 1)`, register under the new id, + `client.publish`, then + `withTimeoutOrNull(timeout) { channel.receive() }`. Cleanup in `finally`. + +Imports added: +- `kotlinx.coroutines.channels.Channel` +- `kotlinx.coroutines.withTimeoutOrNull` +- `com.vitorpamplona.quartz.utils.RandomInstance` +- `java.util.concurrent.ConcurrentHashMap` +- `com.vitorpamplona.quartz.utils.Log` + +Imports removed: +- `kotlin.coroutines.Continuation` +- `kotlin.coroutines.resume` +- `com.vitorpamplona.quartz.utils.cache.LargeCache` +- `com.vitorpamplona.quartz.utils.tryAndWait` (no longer used here; keep in `ParallelUtils.kt`) + +### Step 2 — `IntentRequestManager.kt` + +Same shape, no retry loop, single attempt. `LruCache` → `ConcurrentHashMap`, +`Continuation` → `Channel(capacity = 1)`, atomic `remove` + +`trySend`. Log tag `"NIP55"`. The `forEach` over multi-result Intents +now does `pending.remove(id)?.trySend(result)` per entry. + +### Step 3 — Tests (`RemoteSignerManagerRetryTest.kt`) + +Three new tests. All use the existing `runTest`-based scaffolding — +no new test infra (no `TestLogCapture`, no `Dispatchers.Default` stress +loops). Each test must be observable via the public `launchWaitAndParse` +return value, not internal log state. + +```kotlin +@Test +fun `duplicate response events do not crash and resume once`() = runTest { + val client = TestClient() + val manager = RemoteSignerManager(timeout = 5000L, client = client, ...) + val resultDeferred = async { manager.launchWaitAndParse(...) } + runCurrent() + val response = client.captureRequestEvent().toResponse() + // Three deliveries of the same response — only the first should reach the caller. + launch { manager.newResponse(response) } + launch { manager.newResponse(response) } + launch { manager.newResponse(response) } + advanceUntilIdle() + val result = resultDeferred.await() + assertIs>(result) + // Bug exists on main: second/third launch throws IllegalStateException → fails test. +} + +@Test +fun `late response after timeout is silently discarded`() = runTest { + val client = TestClient(neverResponds = true) + val manager = RemoteSignerManager(timeout = 100L, maxRetries = 0, client = client, ...) + val resultDeferred = async { manager.launchWaitAndParse(...) } + val response = client.captureRequestEvent().toResponse() + advanceTimeBy(200L) // timeout fires + val result = resultDeferred.await() + assertIs(result) + // Now the late response arrives — must not crash, must not affect caller. + manager.newResponse(response) + advanceUntilIdle() + // No assertion needed: lack of crash + caller already got TimedOut is the success criterion. +} + +@Test +fun `late response from attempt 1 does not corrupt attempt 2 result`() = runTest { + val client = TestClient() + val manager = RemoteSignerManager(timeout = 100L, maxRetries = 1, client = client, ...) + val resultDeferred = async { manager.launchWaitAndParse(buildRequest("PAYLOAD_A")) } + runCurrent() + val attempt1Event = client.captureRequestEvent() // captures id_1 + advanceTimeBy(150L) // attempt 1 times out + delay(2000) begins + advanceTimeBy(2000L) // retry kicks in + val attempt2Event = client.captureRequestEvent() // captures id_2 — must be different from id_1 + assertNotEquals(attempt1Event.requestId, attempt2Event.requestId) + // Late response for attempt 1 arrives while attempt 2 is in flight + manager.newResponse(attempt1Event.toResponse(payload = "STALE_A")) + // Real response for attempt 2 arrives + manager.newResponse(attempt2Event.toResponse(payload = "FRESH_B")) + advanceUntilIdle() + val result = resultDeferred.await() + assertIs>(result) + assertEquals("FRESH_B", (result as SignerResult.RequestAddressed.Result<*>).value) + // On main: result would be "STALE_A" (Bug 2) — test fails. Also Bug 1 would crash. +} +``` + +### Step 4 — Verify on `main` first + +Before applying Step 1, run the three new tests against `main`. Expected +failures: + +- Test 1: `IllegalStateException: Already resumed` from second/third + `launch { newResponse }`. +- Test 2: `IllegalStateException` from the late `newResponse` call. +- Test 3: either `IllegalStateException` (Bug 1) or `assertEquals` failure + with `actual = "STALE_A"` (Bug 2). Both branches prove the test exercises + the race. + +Apply Step 1, re-run, all three pass. + +### Step 5 — Format + final build + +```bash +./gradlew spotlessApply +./gradlew :quartz:build +``` + +## Success Metrics + +- Zero `IllegalStateException: Already resumed` log entries from + `RemoteSignerManager.newResponse` or `IntentRequestManager.newResponse` + after the fix lands. +- Zero stale-data correctness reports tied to retry id reuse. +- Three new tests in `RemoteSignerManagerRetryTest` pass; same tests fail + on `main`. +- No regressions in the existing 5 retry tests. + +## Dependencies & Risks + +| Item | Risk | Mitigation | +|---|---|---| +| Channel migration changes the correlation primitive | If a future feature needs multi-response per id (e.g., NIP-46 `auth_url`), the channel must be re-`receive`'d after the parser yields `AwaitingAuth` | Not blocking today (no `auth_url` code). Documented as the right architectural seam. | +| Fresh id per retry attempt changes wire behaviour | Bunkers that cache responses by id will not see the second request as a duplicate | Acceptable — NIP-46 has no idempotency contract; this matches `IntentRequestManager`'s existing behaviour. | +| `LruCache` (Android) → `ConcurrentHashMap` change | `LruCache` had a 2000-entry cap; `ConcurrentHashMap` is unbounded | `finally`-block cleanup guarantees entries shrink on every completed call. Mirrors the unbounded `ConcurrentHashMap` already used in `quartz/.../accessories/`. | +| Existing 5 retry tests rely on the old continuation-cache API | Tests may not compile against the new `pending` map | Tests should only interact through the public `launchWaitAndParse` + `newResponse` surface (verified in deepen-plan research). If any directly inspect `awaitingRequests`, update to inspect `pending` or pivot to result-based assertion. | + +## Alternative Approaches Considered + +1. **Atomic `remove` + `tryResume` on cached `CancellableContinuation`** (original plan) + Fixes Bug 1 cleanly but leaves Bug 2 (retry id-reuse) intact. Pulls in + `@InternalCoroutinesApi`. Stays with the outlier pattern. Rejected + after architecture + simplicity reviews. + +2. **try/catch `IllegalStateException` around `resume`** + Anti-pattern; doesn't fix Bug 2. Rejected. + +3. **`SingleShotContinuation` wrapper helper** + Reusable abstraction for a problem already solved by Channel. Doesn't + fix Bug 2. YAGNI; rejected. + +4. **Subscription-level dedupe in `NostrSignerRemote.kt:82`** + Bounded LRU of seen event ids. Suppresses duplicate work upstream. Not + needed because Channel `trySend` on capacity-1 is already O(1) and + guard at the receive side is atomic. Deferred unless field telemetry + shows high duplicate volume. + +5. **Fix only `RemoteSignerManager`, leave `IntentRequestManager`** + Sibling bug is identical; bundling is cheaper than a follow-up. + +6. **Keep `tryAndWait` for the managers** + Rejected because `tryAndWait`'s `CancellableContinuation`-cache + pattern is the source of both bugs. Keeping `tryAndWait` for + `collectSuccessfulOperationsReturning` is fine — different use case + (no shared id, no concurrent multi-source delivery). + +## Sources & References + +### Origin + +- **Brainstorm:** [`docs/brainstorms/2026-06-03-nip46-bunker-double-resume-brainstorm.md`](../../docs/brainstorms/2026-06-03-nip46-bunker-double-resume-brainstorm.md) +- Decisions changed during deepen-plan + review pass: + - Fix mechanism: atomic remove + `tryResume` → Channel-per-request + - Scope: added Bug 2 (retry id-reuse) as in-scope after investigation + showed HIGH probability of reaching it on flaky relays + +### Internal references + +- `quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/RemoteSignerManager.kt:44,46-50,66-101` — the two bug sites +- `quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/NostrSignerRemote.kt:69-86` — subscription handler (line 82 = source of multi-relay concurrent calls) +- `quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/utils/ParallelUtils.kt:71-79,98` — `tryAndWait` (retained for `collectSuccessfulOperationsReturning`) +- `quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientPublishExt.kt` — house-style reference for Channel-per-request +- `quartz/src/androidMain/kotlin/com/vitorpamplona/quartz/nip55AndroidSigner/api/foreground/IntentRequestManager.kt:63,80-97,119,126` — sibling bug + existing fresh-id usage at line 119 +- `quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/RemoteSignerManagerRetryTest.kt` — existing test scaffolding to extend + +### External references + +- [NIP-46 spec](https://github.com/nostr-protocol/nips/blob/master/46.md) — request/response shapes + the `auth_url` challenge flow +- [kotlinx.coroutines `Channel`](https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.channels/-channel/) — capacity, `trySend`, `receive`, `close` semantics +- [`runTest` docs](https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-test/kotlinx.coroutines.test/run-test.html) — virtual time + single-threaded dispatcher + +--- + +## Unanswered Questions + +- `pending` final naming — `pending` vs `awaitingRequests` (retain old name for grep continuity)? — minor; leaning `pending` (matches `accessories/` convention) +- Whether the NIP-55 Intent multi-result branch is ever actually hit in practice — defensive fix either way; if telemetry confirms it's dead code we could collapse to single-result path in follow-up +- Whether to grep existing tests for direct `awaitingRequests` access before Step 1 — yes, do it at the start of /ce:work to flag breakage early diff --git a/quartz/src/androidMain/kotlin/com/vitorpamplona/quartz/nip55AndroidSigner/api/foreground/IntentRequestManager.kt b/quartz/src/androidMain/kotlin/com/vitorpamplona/quartz/nip55AndroidSigner/api/foreground/IntentRequestManager.kt index 8bab2bffaa..8577ba74f5 100644 --- a/quartz/src/androidMain/kotlin/com/vitorpamplona/quartz/nip55AndroidSigner/api/foreground/IntentRequestManager.kt +++ b/quartz/src/androidMain/kotlin/com/vitorpamplona/quartz/nip55AndroidSigner/api/foreground/IntentRequestManager.kt @@ -22,47 +22,37 @@ package com.vitorpamplona.quartz.nip55AndroidSigner.api.foreground import android.content.ActivityNotFoundException import android.content.Intent -import androidx.collection.LruCache import com.vitorpamplona.quartz.nip55AndroidSigner.api.IResult import com.vitorpamplona.quartz.nip55AndroidSigner.api.SignerResult import com.vitorpamplona.quartz.nip55AndroidSigner.api.foreground.intents.results.IntentResult import com.vitorpamplona.quartz.utils.Log import com.vitorpamplona.quartz.utils.RandomInstance -import com.vitorpamplona.quartz.utils.tryAndWait -import kotlin.coroutines.Continuation -import kotlin.coroutines.resume +import com.vitorpamplona.quartz.utils.cache.LargeCache +import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.withTimeoutOrNull /** - * This class manages the lifecycle of foreground signing requests in a NIP-55 compliant Android signer flow. + * Manages the lifecycle of foreground signing requests in a NIP-55 compliant Android signer flow. * - * - It tracks pending signing requests using a unique ID and allows for awaiting their results via coroutines. + * - Tracks pending signing requests by a unique call id via a per-request [Channel]. * - Provides a way to launch foreground Intents (to request user approval) and wait for the response. - * - Handles timeouts on user approval using [tryAndWait]. + * - Handles approval timeouts via [withTimeoutOrNull]. * - Collects results via [newResponse] when the user responds to the foreground request. * - * Main components: + * Key usage flow: request initiated -> store channel by id -> launch intent -> withTimeoutOrNull + * receive -> finally cleanup. User responds -> atomic remove + trySend on the channel. * - * - `awaitingRequests`: LRU cache mapping request IDs to continuations for async response handling. - * - `appLauncher`: Function reference to launch foreground Intents (typically provided by an Activity). - * - `launchAndWait`: Suspend function to send an Intent, wait for an answer, and parse the result. - * - `newResponse`: Handles incoming results from the foreground activity using a unique ID. - * - * Key usage flows: - * - * - Request initiated -> store continuation by ID -> launch intent -> wait - * - User responds -> resume continuation -> remove ID -> return parsed result - * - * The class also cleans up pending requests on timeout or cancellation. + * Duplicate, unknown, or late deliveries (e.g. if the activity surfaces multiple results for the + * same id) atomically read out as null and are dropped after a debug log line — no continuation + * is ever resumed twice. */ class IntentRequestManager( val foregroundApprovalTimeout: Long = 30000, ) { val activityNotFoundIntent = Intent() - // LRU cache to store pending requests and their continuations. - private val awaitingRequests = LruCache>(2000) + private val pending = LargeCache>() - // Function to launch an Intent in the foreground. private var appLauncher: ((Intent) -> Unit)? = null /** Call this function when the launcher becomes available on activity, fragment or compose */ @@ -82,70 +72,66 @@ class IntentRequestManager( if (results != null) { // This happens when the intent responds to many requests at the same time. IntentResult.fromJsonArray(results).forEach { result -> - if (result.id != null) { - awaitingRequests[result.id]?.resume(result) - awaitingRequests.remove(result.id) - } + if (result.id != null) dispatch(result.id, result) } } else { val result = IntentResult.fromIntent(data) - if (result.id != null) { - awaitingRequests[result.id]?.resume(result) - awaitingRequests.remove(result.id) - } + if (result.id != null) dispatch(result.id, result) } } + private fun dispatch( + id: String, + result: IntentResult, + ) { + val channel = pending.remove(id) + if (channel == null) { + Log.d("NIP55") { "no channel for intent result id=$id (duplicate, unknown, or late)" } + return + } + channel.trySend(result) + } + fun hasForegroundActivity() = appLauncher != null /** - * Launches the signer, waits and parses the result + * Launches the signer, waits and parses the result. * - * @param requestIntent The Intent to be launched. + * @param requestIntentBuilder Builder for the Intent to be launched. * @param parser A function that parses the response Intent into a [SignerResult.RequestAddressed]. * @return The result after parsing the Intent using the provided parser. - * - * This function uses the [tryAndWait] utility to implement a timeout on the foreground approval. - * It assigns a unique ID to the request and keeps a continuation to resume once the result is received. - * If the timeout occurs or the continuation is cancelled, the request ID is cleaned up from [awaitingRequests]. - * Flags are added to the Intent to ensure it is brought to the front if already running. */ suspend fun launchWaitAndParse( requestIntentBuilder: () -> Intent, parser: (intent: IntentResult) -> SignerResult.RequestAddressed, - ): SignerResult.RequestAddressed = - appLauncher?.let { launcher -> - val requestIntent = requestIntentBuilder() - val callId = RandomInstance.randomChars(32) + ): SignerResult.RequestAddressed { + val launcher = appLauncher ?: return SignerResult.RequestAddressed.NoActivityToLaunchFrom() - requestIntent.putExtra("id", callId) - requestIntent.addFlags(Intent.FLAG_ACTIVITY_SINGLE_TOP or Intent.FLAG_ACTIVITY_CLEAR_TOP) + val requestIntent = requestIntentBuilder() + val callId = RandomInstance.randomChars(32) + requestIntent.putExtra("id", callId) + requestIntent.addFlags(Intent.FLAG_ACTIVITY_SINGLE_TOP or Intent.FLAG_ACTIVITY_CLEAR_TOP) + + val channel = Channel(capacity = 1) + pending.put(callId, channel) + + return try { try { - val resultIntent = - tryAndWait(foregroundApprovalTimeout) { continuation -> - continuation.invokeOnCancellation { - awaitingRequests.remove(callId) - } - - awaitingRequests.put(callId, continuation) - - try { - launcher.invoke(requestIntent) - } catch (e: Exception) { - Log.e("ExternalSigner", "Error launching intent", e) - awaitingRequests.remove(callId) - throw e - } - } - - when (resultIntent) { - null -> SignerResult.RequestAddressed.TimedOut() - else -> parser(resultIntent) - } + launcher.invoke(requestIntent) } catch (e: ActivityNotFoundException) { Log.e("ExternalSigner", "Error launching intent: Signer not found", e) - SignerResult.RequestAddressed.SignerNotFound() + return SignerResult.RequestAddressed.SignerNotFound() } - } ?: SignerResult.RequestAddressed.NoActivityToLaunchFrom() + + val resultIntent = withTimeoutOrNull(foregroundApprovalTimeout) { channel.receive() } + when (resultIntent) { + null -> SignerResult.RequestAddressed.TimedOut() + else -> parser(resultIntent) + } + } finally { + pending.remove(callId) + channel.close() + } + } } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/RemoteSignerManager.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/RemoteSignerManager.kt index 43c8c0bd5b..432c715e41 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/RemoteSignerManager.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/RemoteSignerManager.kt @@ -27,11 +27,12 @@ import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerInternal import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequest import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerResponse import com.vitorpamplona.quartz.nip46RemoteSigner.NostrConnectEvent +import com.vitorpamplona.quartz.utils.Log +import com.vitorpamplona.quartz.utils.RandomInstance import com.vitorpamplona.quartz.utils.cache.LargeCache -import com.vitorpamplona.quartz.utils.tryAndWait +import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.delay -import kotlin.coroutines.Continuation -import kotlin.coroutines.resume +import kotlinx.coroutines.withTimeoutOrNull class RemoteSignerManager( val timeout: Long = 65_000, @@ -41,19 +42,27 @@ class RemoteSignerManager( val relayList: Set, val maxRetries: Int = 1, ) { - private val awaitingRequests = LargeCache>() + private val pending = LargeCache>() suspend fun newResponse(responseEvent: NostrConnectEvent) { val decryptedJson = signer.decrypt(responseEvent.content, remoteKey) val bunkerResponse = OptimizedJsonMapper.fromJsonTo(decryptedJson) - awaitingRequests.get(bunkerResponse.id)?.resume(bunkerResponse) + + val channel = pending.remove(bunkerResponse.id) + if (channel == null) { + Log.d("NIP46") { "no channel for bunker response id=${bunkerResponse.id} (duplicate, unknown, or late)" } + return + } + channel.trySend(bunkerResponse) } /** * Launches the signer, waits and parses the result. * - * Builds the request once and republishes the same event on retry to ensure - * the bunker's response (keyed by request ID) can always be matched. + * Each retry attempt uses a fresh request id so a late response from a previous + * attempt cannot resume the current attempt with stale data. The bunker request + * builder is still called only once per call; the manager rewrites the id per + * attempt internally. * * @param bunkerRequestBuilder The BunkerRequest to be sent. * @param parser A function that parses the BunkerResponse into a [SignerResult.RequestAddressed]. @@ -63,36 +72,38 @@ class RemoteSignerManager( bunkerRequestBuilder: () -> BunkerRequest, parser: (response: BunkerResponse) -> SignerResult.RequestAddressed, ): SignerResult.RequestAddressed { - val request = bunkerRequestBuilder() - val event = - NostrConnectEvent.create( - message = request, - remoteKey = remoteKey, - signer = signer, - ) + val template = bunkerRequestBuilder() var attempt = 0 while (true) { - val result = - tryAndWait(timeout) { continuation -> - continuation.invokeOnCancellation { - awaitingRequests.remove(request.id) - } + val attemptRequest = + BunkerRequest( + id = RandomInstance.randomChars(32), + method = template.method, + params = template.params, + ) + val event = + NostrConnectEvent.create( + message = attemptRequest, + remoteKey = remoteKey, + signer = signer, + ) - awaitingRequests.put(request.id, continuation) + val channel = Channel(capacity = 1) + pending.put(attemptRequest.id, channel) + val response = + try { client.publish(event, relayList = relayList) + withTimeoutOrNull(timeout) { channel.receive() } + } finally { + pending.remove(attemptRequest.id) + channel.close() } when { - result != null -> { - return parser(result) - } - - attempt >= maxRetries -> { - return SignerResult.RequestAddressed.TimedOut() - } - + response != null -> return parser(response) + attempt >= maxRetries -> return SignerResult.RequestAddressed.TimedOut() else -> { attempt++ delay(2_000L) diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/RemoteSignerManagerRetryTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/RemoteSignerManagerRetryTest.kt index 85a652b874..99d18cb353 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/RemoteSignerManagerRetryTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/RemoteSignerManagerRetryTest.kt @@ -22,6 +22,7 @@ package com.vitorpamplona.quartz.nip46RemoteSigner.signer import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.core.HexKey +import com.vitorpamplona.quartz.nip01Core.core.OptimizedJsonMapper import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair import com.vitorpamplona.quartz.nip01Core.relay.client.EmptyNostrClient import com.vitorpamplona.quartz.nip01Core.relay.client.INostrClient @@ -31,21 +32,46 @@ import com.vitorpamplona.quartz.nip01Core.relay.client.single.IRelayClient import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerInternal +import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequest import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestPing +import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerResponsePong +import com.vitorpamplona.quartz.nip46RemoteSigner.NostrConnectEvent import com.vitorpamplona.quartz.utils.Hex +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.async import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.advanceTimeBy +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertIs +import kotlin.test.assertNotEquals +@OptIn(ExperimentalCoroutinesApi::class) class RemoteSignerManagerRetryTest { private val signer = NostrSignerInternal(KeyPair()) private val remoteKeyPair = KeyPair() private val remoteKey = Hex.encode(remoteKeyPair.pubKey) + private val bunkerSigner = NostrSignerInternal(remoteKeyPair) private val relay = NormalizedRelayUrl("wss://relay.test") + private suspend fun decodeRequestId(event: Event): String { + val plaintext = bunkerSigner.decrypt(event.content, event.pubKey) + val request = OptimizedJsonMapper.fromJsonTo(plaintext) + return request.id + } + + private suspend fun bunkerPongFor(requestId: String): NostrConnectEvent = + NostrConnectEvent.create( + message = BunkerResponsePong(requestId), + remoteKey = signer.pubKey, + signer = bunkerSigner, + ) + @Test fun timeoutReturnsTimedOutAfterMaxRetries() = runTest { @@ -165,6 +191,190 @@ class RemoteSignerManagerRetryTest { assertEquals(1, manager.maxRetries) } + + @Test + fun duplicateResponsesAreSafeAndResumeOnce() = + runTest { + val capturing = CapturingNostrClient() + val manager = + RemoteSignerManager( + timeout = 5_000, + client = capturing, + signer = signer, + remoteKey = remoteKey, + relayList = setOf(relay), + maxRetries = 0, + ) + + val deferred = + async { + manager.launchWaitAndParse( + bunkerRequestBuilder = { BunkerRequestPing() }, + parser = PingResponse::parse, + ) + } + runCurrent() + + val publishedRequestId = decodeRequestId(capturing.publishedEvents.single()) + val response = bunkerPongFor(publishedRequestId) + + // Three deliveries of the same response — only one continuation exists, + // so the second and third would have crashed on the old `get(id)?.resume(...)` + // path. With atomic remove + Channel(1) trySend they are safe no-ops. + launch { manager.newResponse(response) } + launch { manager.newResponse(response) } + launch { manager.newResponse(response) } + advanceUntilIdle() + + val result = deferred.await() + assertIs>(result) + } + + @Test + fun lateResponseAfterTimeoutIsSilentlyDiscarded() = + runTest { + val capturing = CapturingNostrClient() + val manager = + RemoteSignerManager( + timeout = 100, + client = capturing, + signer = signer, + remoteKey = remoteKey, + relayList = setOf(relay), + maxRetries = 0, + ) + + val deferred = + async { + manager.launchWaitAndParse( + bunkerRequestBuilder = { BunkerRequestPing() }, + parser = PingResponse::parse, + ) + } + runCurrent() + + val publishedRequestId = decodeRequestId(capturing.publishedEvents.single()) + + // Let the timeout fire. Caller resolves to TimedOut and the channel is closed. + val result = deferred.await() + assertIs>(result) + + // Now the late response arrives. On the old `get(id)?.resume(...)` path the + // continuation was already completed by the timeout, so this would throw + // IllegalStateException("Already resumed"). With the fix the entry is gone + // from `pending` and trySend on the closed channel is a no-op. + val response = bunkerPongFor(publishedRequestId) + manager.newResponse(response) + advanceUntilIdle() + } + + @Test + fun lateResponseFromAttempt1DoesNotCorruptAttempt2() = + runTest { + val capturing = CapturingNostrClient() + val manager = + RemoteSignerManager( + timeout = 100, + client = capturing, + signer = signer, + remoteKey = remoteKey, + relayList = setOf(relay), + maxRetries = 1, + ) + + val deferred = + async { + manager.launchWaitAndParse( + bunkerRequestBuilder = { BunkerRequestPing() }, + parser = PingResponse::parse, + ) + } + runCurrent() + + // Attempt 1 publishes, then times out at T=100. + val attempt1Id = decodeRequestId(capturing.publishedEvents[0]) + advanceTimeBy(150) + // delay(2_000) between attempts: attempt 2 starts at T=2_100. + // Land mid-window so attempt 2's own 100 ms timeout (T=2_200) hasn't fired yet. + advanceTimeBy(2_000) + runCurrent() + + // Attempt 2 must use a different id — otherwise a late attempt-1 response + // could resume the attempt-2 channel with stale data. + assertEquals(2, capturing.publishedEvents.size) + val attempt2Id = decodeRequestId(capturing.publishedEvents[1]) + assertNotEquals(attempt1Id, attempt2Id) + + // Late delivery of attempt 1's response. With the fix it has no entry in + // `pending` and is silently discarded. + manager.newResponse(bunkerPongFor(attempt1Id)) + // Attempt 2's real response. + manager.newResponse(bunkerPongFor(attempt2Id)) + advanceUntilIdle() + + val result = deferred.await() + val success = assertIs>(result) + assertEquals(attempt2Id, success.result.pong) + } +} + +private class CapturingNostrClient : INostrClient { + val publishedEvents = mutableListOf() + + override fun connectedRelaysFlow(): StateFlow> = MutableStateFlow(emptySet()) + + override fun availableRelaysFlow(): StateFlow> = MutableStateFlow(emptySet()) + + override fun connect() {} + + override fun disconnect() {} + + override fun reconnect( + onlyIfChanged: Boolean, + ignoreRetryDelays: Boolean, + ) {} + + override fun isActive(): Boolean = false + + override fun syncFilters(relay: IRelayClient) {} + + override fun subscribe( + subId: String, + filters: Map>, + listener: SubscriptionListener?, + ) {} + + override fun count( + subId: String, + filters: Map>, + ) {} + + override fun unsubscribe(subId: String) {} + + override fun publish( + event: Event, + relayList: Set, + ) { + publishedEvents.add(event) + } + + override fun pendingPublishRelaysFor(eventId: String): Set? = null + + override fun addConnectionListener(listener: RelayConnectionListener) {} + + override fun removeConnectionListener(listener: RelayConnectionListener) {} + + override fun getReqFiltersOrNull(subId: String): Map>? = null + + override fun getCountFiltersOrNull(subId: String): Map>? = null + + override fun activeRequests(url: NormalizedRelayUrl): Map> = emptyMap() + + override fun activeCounts(url: NormalizedRelayUrl): Map> = emptyMap() + + override fun activeOutboxCache(url: NormalizedRelayUrl): Set = emptySet() + + override fun close() {} } private class CountingNostrClient(