fix(client,nip44): subscription routing and provider-independent NIP-44 (2.0.8) - #538
Merged
tcheeric merged 26 commits intoAug 22, 2026
Merged
Conversation
Refactor WebSocket handling and update integration tests
Release v1.2.1
Release: simplify nostr-java from 9 modules to 4, WebSocket improvements, and bug fixes
NostrRelayClient previously logged via clientSession.getUri(), which
can return null after the WebSocket session has been closed. In
staging, this surfaced as 188 occurrences in 2 hours of:
Sending request to relay null: ["CLOSE","sub-..."]
Capture the relay URI at construction time and store it in a
final field. Use that field for all logging instead of querying
the session, so log messages remain meaningful regardless of session
state. The package-private (WebSocketSession, long) constructor used
by tests still attempts to derive the URI from the session.
No behaviour change beyond logging; all 13 nostr-java-client tests
still pass.
Refs: docs/analysis-gateway-core-relay-client-circuitbreaker-2026-05-06.md §4
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Includes the relayUri null-safety fix from 4cdd556.
Addresses Codex review on PR #523 — repository's AGENTS.md guideline requires CHANGELOG.md updates for every version bump.
…05-06 fix(ws): null-safe relay URI logging + 2.0.1 release
… serialise concurrent sends
NostrRelayClient.subscribe() called clientSession.sendMessage(...) without
holding the existing sendLock, while send() held it correctly. Two threads
racing inside subscribe() (or one in subscribe() racing one in send()) could
trigger StandardWebSocketSession's IllegalStateException("The remote endpoint
was in state [TEXT_FULL_WRITING]") (Tomcat) or "Blocking message pending"
(Jetty). The catch (RuntimeException) block at the bottom of subscribe()
rewrapped the exception as IOException("Failed to send subscription
payload", e), which the imani-gateway-core consumer then surfaced as
~70 RelaySubscribeException per 60 minutes once a circuit-breaker tuning
fix unmasked the underlying race.
Fixed by wrapping the underlying WebSocketSession returned by connectSession()
in Spring's ConcurrentWebSocketSessionDecorator(session, sendTimeLimit,
bufferSizeLimit, OverflowStrategy.TERMINATE) at construction time, so
concurrent sendMessage() calls from any send-path are serialised at the
session layer. The decorator is idiomatic Spring and is future-proof against
new send-paths added later (e.g. heartbeat/ping) that would otherwise have
to remember to acquire sendLock.
Sized: 256 KiB buffer (~2.5x the largest expected payload, a chunked
kind-37375 EVENT at ~100 KiB), 10 s send time limit (above any p99.9
healthy-state subscribe REQ, well below the existing awaitTimeoutMs=60 s).
Both tunable via @Value-injected constructor parameters
(nostr.websocket.send-buffer-limit, nostr.websocket.send-time-limit-ms) or
matching env-vars (NOSTR_WEBSOCKET_SEND_BUFFER_LIMIT,
NOSTR_WEBSOCKET_SEND_TIME_LIMIT_MS) under Spring relaxed binding.
OverflowStrategy is explicitly TERMINATE, never DROP — dropping outbound
REQ/EVENT frames silently would make callers believe a subscribe/publish
succeeded when the relay never received it.
Constructor binding:
- The new four-arg public constructor (relayUri, awaitTimeoutMs,
sendBufferLimit, sendTimeLimitMs) is annotated @Autowired so Spring's
prototype-scoped @component selection is deterministic.
- All existing public constructors (one-arg and two-arg) are retained as
delegating overloads (binary-compat) and now also benefit from the
decorator wrap by way of the canonical ctor.
- A new package-private four-arg test ctor + two named static factories
(forTestWithRawSession, forTestWithDecoratedSession) make the
wrap-or-not decision explicit at the call site, so the new
NostrRelayClientConcurrencyTest can deterministically reproduce the
race against a raw session and verify the fix on a decorated one.
- awaitTimeoutMs is now a final field assigned via constructor injection
(previously @Value-annotated field). Constructor-injection ordering
guarantees the value reaches the constructor body before the decorator
is constructed.
New test class NostrRelayClientConcurrencyTest covers:
- (7a) Reproduction: 32 concurrent subscribe() calls against a raw session
reproduce the IllegalStateException race rewrap.
- (7b) Resolution: same harness against a decorated session sees zero
failures and maxInFlight==1; field-injection regression assertions on
the new ctor parameters.
- (7c) Mixed-workload: a 100 KiB send racing with 20 subscribes under the
256 KiB default buffer completes without overflow.
- (7d) Overflow-and-close: a tiny 256-byte buffer is forced to overflow;
SessionLimitExceededException (rewrapped as
IOException("Failed to send subscription payload", …)) propagates to
the caller. Note: the spec's strict "session must be closed after
overflow" assertion is relaxed here because Spring's
ConcurrentWebSocketSessionDecorator does not close the delegate from
limitExceeded() — the close-on-overflow chain is the upstream
imani-wallet-lib's responsibility (§6.7e in the spec).
- Constructor validation tests for non-positive sendBufferLimit /
sendTimeLimitMs / awaitTimeoutMs and null session.
Spec: imani-apps/docs/analysis-gateway-core-relay-subscribe-instability-2026-05-07.md
Phase: 1 of 6
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Includes the concurrent-subscribe-send fix from the previous commit. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
… restore isOpen()==false reconnect contract Spring's ConcurrentWebSocketSessionDecorator under OverflowStrategy.TERMINATE only sets a private limitExceeded flag and throws SessionLimitExceededException from limitExceeded() — it does NOT close the delegate session. As a result, 2.0.2's wrap-only fix left the session open after overflow, breaking the upstream caller's clientSession.isOpen()==false → reconnect contract that the spec's §6.7d ii originally required. NostrRelayClient.subscribe() and NostrRelayClient.send() now detect a SessionLimitExceededException cause in their respective catch blocks and call clientSession.close(CloseStatus.SESSION_NOT_RELIABLE) explicitly before rewrapping the exception. Non-overflow RuntimeExceptions continue to flow through the existing wrap-as-IOException path unchanged — only overflow terminates the session, matching Spring's OverflowStrategy.TERMINATE semantics that the framework only half-implements. The §6.7d concurrency test now strictly asserts clientSession.isOpen()==false after overflow, restoring the spec's original contract. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…026-05-07 fix(ws): wrap clientSession in ConcurrentWebSocketSessionDecorator (2.0.2)
NostrRelayClient called the no-arg clientSession.close() in close() and the send() timeout path. Spring 6.2.x's ConcurrentWebSocketSessionDecorator overrides only close(CloseStatus) (guarded by closeLock); the no-arg close() falls through to delegate.close() with no coordination, racing an in-flight sendMessage flush on the Tomcat delegate (single-in-flight-write → IllegalStateException: Concurrent write operations are not permitted). This was the residual concurrent-write leakage observed during the spec-026 soak, a layer below wallet-lib's adapter sendLock. Routing through close(CloseStatus) alone is insufficient: its closeLock is a separate lock from the flushLock guarding delegate.sendMessage. Introduce a ReentrantReadWriteLock session gate — every write (send/subscribe via sendFrameGated) takes the read side (sends stay concurrent; the decorator still serialises the delegate writes among them), every close (closeGated/ closeQuietly, always close(CloseStatus)) takes the write side and waits for in-flight sends to drain before sending the CLOSE frame. No lock nesting, so no added deadlock risk. Tests: new NostrRelayClientCloseWriteRaceTest (routing through CloseStatus + never no-arg close; timeout-path routing; latch-based serialisation proof). Updated NostrRelayClientTimeoutTest and SpringWebSocketClientTest, which asserted the old no-arg close(), to assert close(any(CloseStatus)) + never().close(). 24/24 nostr-java-client tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Cuts 2.0.4 with the spec-026 US3 close-vs-write fix. Moves [Unreleased] changelog content into the 2.0.4 section. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Address Copilot review on PR #525: - send(): clear pendingRequest on ANY sendFrameGated failure (not only SessionLimitExceededException). A plain IOException previously left the client stuck "in flight", breaking the next send()/@NostrRetryable retry. - closeQuietly(): log the swallowed throwable (+ relayUri) instead of just e.getMessage(), preserving the stack trace. - tests: correct stale sendLock references to the sessionGate RW-lock; narrow the timeout assertThrows from Exception to RelayTimeoutException. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Cuts 2.0.5 with the PR #525 review fixes: send() clears pendingRequest on any send failure (not just overflow) so a failed write can't wedge the in-flight slot, and closeQuietly() logs the throwable + relay URI. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
fix(ws): serialise session close against in-flight writes (spec-026 US3)
… race (spec-026) send() lacked the isOpen() guard subscribe() has. A per-subscription Nostr CLOSE issued while a relay connection is breaking reached Tomcat's sendText, where doClose() fires mid-write and emits a WS CLOSE frame while the text write is still pending on the async channel — throwing "Concurrent write operations are not permitted", surfacing as a transport-error reconnect storm. Fail fast on a closed session instead. +regression test. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
send() isOpen() guard (spec-026) — eliminates the CLOSE-on-dying-connection concurrent-write transport-error storm. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
… (2.0.7, PR #526 review) The 2.0.6 guard checked clientSession.isOpen() at the top of send(), before sendFrameGated() took the sessionGate read lock — so a concurrent close() could close+release the session between the check and sendMessage(). Moved the check inside sendFrameGated() after the read lock, making open-check + write atomic w.r.t. closeGated() (write lock). Bump 2.0.6 -> 2.0.7. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
fix(ws): guard send() with isOpen() — eliminate CLOSE-on-dying-connection race (2.0.6)
…them - Listeners were registered per connection and every inbound payload went to all of them, so a second REQ saw the first one's EVENT frames and, worse, was released by the first one's EOSE. Callers that end a query on EOSE — which is every caller — returned a partial result set that varied from call to call. - subscribe(ReqMessage, ...) now records the subscription id and dispatchMessage delivers EVENT, EOSE and CLOSED only to the listener that opened that subscription. - Everything else is unchanged: NOTICE, OK, AUTH and any payload whose id cannot be read still go to every listener, as do listeners registered through the raw-JSON subscribe overload, where no id is parsed. No caller loses a frame it receives today. - The id is read with a scan rather than a JSON parse: this runs on every frame, and the payload is parsed again by whichever listener consumes it. - Found on a live relay: gift-wrap queries returned 0, 2 or 6 of the 5 events the relay holds, differing call to call, while a direct REQ returned all 5 every time. Co-Authored-By: Claude Opus 5 <[email protected]>
…ider
EncryptedPayloads asked for Cipher.getInstance("ChaCha20") and passed an
IvParameterSpec. Only BouncyCastle's JCE provider accepts that; SunJCE requires a
ChaCha20ParameterSpec and throws InvalidAlgorithmParameterException. The provider
was registered nowhere except as a side effect of Schnorr.generatePrivateKey(), so
NIP-44 encryption worked for a caller that had generated a key in this JVM and
failed for one that loaded a key from storage. On Android it could not work at all:
Security.addProvider for the name "BC" is a no-op there, because the platform ships
its own repackaged BouncyCastle under that name.
Both cipher call sites now use BouncyCastle's lightweight ChaCha7539Engine, which
needs no provider lookup and behaves identically on the JVM and on Android.
Schnorr.generatePrivateKey() drops the Security.addProvider call with it and uses
the lightweight ECKeyPairGenerator, so generating a key no longer mutates
process-wide JCE state. Callers that need the provider registered must register it
themselves; every known consumer already does.
NIP-44 is now checked against the specification's own vectors, and the whole test
class runs with the provider de-registered — which is what makes the fixture a
regression test rather than a restatement. The existing MessageCipherTest passed
throughout, because both of its cases generate a key first.
Closes #537
tcheeric
deleted the
fix/subscription-routing-and-nip44-provider-2026-08-22
branch
August 22, 2026 20:26
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent fixes, released together as nostr-java 2.0.8.
fix(client)— route relay payloads to the subscription that asked for themA relay's
EOSEfor one subscription was ending every in-flight query on thatconnection, so a query could return a random subset of its results and report
itself complete. Payloads are now dispatched by subscription id.
Closes kan card
dkcwoyh42rtl.fix(nip44)— stop depending on a globally registered BouncyCastle providerCipher.getInstance("ChaCha20")resolves to whichever JCE provider happens to beregistered. SunJCE's implementation rejects an
IvParameterSpecand demands aChaCha20ParameterSpec, so NIP-44 only worked in a JVM where BouncyCastle hadalready been registered — which happened as a side effect of
Schnorr.generatePrivateKey(). On Android it could never work:Security.addProviderfor the name "BC" is a no-op there, because the platform ships its own repackaged
BouncyCastle under that name.
Both call sites now use BC's lightweight API (
ChaCha7539Engine), which needs noprovider lookup and behaves identically everywhere.
Schnorr.generatePrivateKey()switches to
ECKeyPairGeneratorand no longer registers a provider — registering oneis a global side effect and the caller's business. Every downstream that needs the
provider already registers it itself.
Closes #537.
Tests
New
EncryptedPayloadsTestruns the officialnip44.vectors.json(35 conversation-key,10 encrypt, 10 decrypt) with the "BC" provider de-registered, so the old behaviour
fails it. Verified red-then-green.
Full suite: 187 tests across the four modules, 0 failures.