fix(quartz): NIP-46 bunker double-resume + retry id-reuse races

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<id, Channel<Response>(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\`
This commit is contained in:
nrobi144
2026-06-07 14:36:29 +03:00
parent 09a166c7c0
commit 8b9875d9cc
4 changed files with 824 additions and 94 deletions
@@ -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<T>` 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<BunkerResponse>(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<String, Channel<BunkerResponse>>()
suspend fun newResponse(responseEvent: NostrConnectEvent) {
val decryptedJson = signer.decrypt(responseEvent.content, remoteKey)
val bunkerResponse = OptimizedJsonMapper.fromJsonTo<BunkerResponse>(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 <T : SignerResult.RequestAddressed> 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<BunkerResponse>(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<String, Channel<BunkerResponse>>`
- `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<BunkerResponse>(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<IntentResult>(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<SignerResult.RequestAddressed.Result<*>>(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<SignerResult.RequestAddressed.TimedOut>(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<SignerResult.RequestAddressed.Result<*>>(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<T>` 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
@@ -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<String, Continuation<IntentResult>>(2000)
private val pending = LargeCache<String, Channel<IntentResult>>()
// 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<T>].
* @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 <T : IResult> launchWaitAndParse(
requestIntentBuilder: () -> Intent,
parser: (intent: IntentResult) -> SignerResult.RequestAddressed<T>,
): SignerResult.RequestAddressed<T> =
appLauncher?.let { launcher ->
val requestIntent = requestIntentBuilder()
val callId = RandomInstance.randomChars(32)
): SignerResult.RequestAddressed<T> {
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<IntentResult>(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()
}
}
}
@@ -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<NormalizedRelayUrl>,
val maxRetries: Int = 1,
) {
private val awaitingRequests = LargeCache<String, Continuation<BunkerResponse>>()
private val pending = LargeCache<String, Channel<BunkerResponse>>()
suspend fun newResponse(responseEvent: NostrConnectEvent) {
val decryptedJson = signer.decrypt(responseEvent.content, remoteKey)
val bunkerResponse = OptimizedJsonMapper.fromJsonTo<BunkerResponse>(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<T>].
@@ -63,36 +72,38 @@ class RemoteSignerManager(
bunkerRequestBuilder: () -> BunkerRequest,
parser: (response: BunkerResponse) -> SignerResult.RequestAddressed<T>,
): SignerResult.RequestAddressed<T> {
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<BunkerResponse>(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)
@@ -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<BunkerRequest>(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<SignerResult.RequestAddressed.Successful<PingResult>>(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<SignerResult.RequestAddressed.TimedOut<PingResult>>(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<SignerResult.RequestAddressed.Successful<PingResult>>(result)
assertEquals(attempt2Id, success.result.pong)
}
}
private class CapturingNostrClient : INostrClient {
val publishedEvents = mutableListOf<Event>()
override fun connectedRelaysFlow(): StateFlow<Set<NormalizedRelayUrl>> = MutableStateFlow(emptySet())
override fun availableRelaysFlow(): StateFlow<Set<NormalizedRelayUrl>> = 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<NormalizedRelayUrl, List<Filter>>,
listener: SubscriptionListener?,
) {}
override fun count(
subId: String,
filters: Map<NormalizedRelayUrl, List<Filter>>,
) {}
override fun unsubscribe(subId: String) {}
override fun publish(
event: Event,
relayList: Set<NormalizedRelayUrl>,
) {
publishedEvents.add(event)
}
override fun pendingPublishRelaysFor(eventId: String): Set<NormalizedRelayUrl>? = null
override fun addConnectionListener(listener: RelayConnectionListener) {}
override fun removeConnectionListener(listener: RelayConnectionListener) {}
override fun getReqFiltersOrNull(subId: String): Map<NormalizedRelayUrl, List<Filter>>? = null
override fun getCountFiltersOrNull(subId: String): Map<NormalizedRelayUrl, List<Filter>>? = null
override fun activeRequests(url: NormalizedRelayUrl): Map<String, List<Filter>> = emptyMap()
override fun activeCounts(url: NormalizedRelayUrl): Map<String, List<Filter>> = emptyMap()
override fun activeOutboxCache(url: NormalizedRelayUrl): Set<HexKey> = emptySet()
override fun close() {}
}
private class CountingNostrClient(