Open
Conversation
Collaborator
|
Wouldn't we need 2 timeouts then? Because the app logic might need to also have a time limit for signing, if the signer is unresponsive. |
Collaborator
Author
|
Related to #236 |
Contributor
For not implement solution 2 tbd: e2e timeout and what to make the default value ajusted by the dev (what to communicate) In cause of signer required after network query its included in the network timeout |
nogringo
force-pushed
the
fix/issue-398-timeout-during-signing
branch
from
7cc7fdb to
f1817bf
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #399 +/- ##
==========================================
+ Coverage 73.36% 73.37% +0.01%
==========================================
Files 195 195
Lines 8969 9007 +38
==========================================
+ Hits 6580 6609 +29
- Misses 2389 2398 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1-leo
changed the title
nogringo
force-pushed
the
fix/issue-398-timeout-during-signing
branch
from
ea76bc4 to
5ae8db7
Compare
📝 WalkthroughWalkthroughThe changes implement a deferred timeout initialization mechanism for broadcast events and pause/resume functionality for request timeouts. A new public Changes
Sequence DiagramsequenceDiagram
participant Client
participant Engine as JIT/RelaySet Engine
participant BroadcastState
participant RelayManager
participant Signer as MockSlowSigner
participant RequestState
Client->>Engine: broadcast event / query
Engine->>Signer: sign event (async with delay)
Note over RelayManager: During slow signing...
Engine->>RelayManager: pause timeouts for in-flight requests
RelayManager->>RequestState: pauseTimeout()
RequestState->>RequestState: store remaining duration
Signer->>Signer: apply 12s delay
Signer-->>Engine: return signed event
Engine->>RelayManager: resume timeouts
RelayManager->>RequestState: resumeTimeout()
RequestState->>RequestState: restart with remaining duration
Engine->>BroadcastState: startTimeout()
BroadcastState->>BroadcastState: initialize timeout timer
Engine->>Engine: dispatch to relays
Engine-->>Client: broadcast/query result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ndk/lib/domain_layer/usecases/jit_engine/jit_engine.dart (1)
168-169:⚠️ Potential issue | 🟡 MinorDead code:
doneStreamvariable is unused.The
doneStreamvariable is declared but never used since lines 228-229 inline the same stream expression directly.🧹 Remove unused variable
- final doneStream = broadcastState.stateUpdates - .map((state) => state.broadcasts.values.toList()); - Future<void> asyncStuff() async {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/lib/domain_layer/usecases/jit_engine/jit_engine.dart` around lines 168 - 169, The local variable doneStream is dead code—it's declared as `doneStream = broadcastState.stateUpdates.map((state) => state.broadcasts.values.toList())` but never used; remove the `doneStream` declaration and any associated unused imports so the code uses the inlined stream expression directly (as already done later) and keep only the live reference to `broadcastState.stateUpdates.map(...)`.
🧹 Nitpick comments (2)
packages/ndk/lib/domain_layer/entities/request_state.dart (1)
91-117: Well-structured pause/resume implementation.The timeout pause/resume logic correctly handles the common cases. The guards ensure safe operation when methods are called multiple times or out of order.
One minor observation: after the timeout fires naturally,
_timeoutisn't nulled, so a subsequentpauseTimeout()call would attempt to calculate remaining time. In practice this is unlikely sinceclose()ends the request lifecycle, but for defensive clarity:🛡️ Optional: Null the timer reference after firing
void _startTimeout(Duration duration) { _timeoutStartedAt = DateTime.now(); _timeout = Timer(duration, () { + _timeout = null; onTimeout?.call(this); close(); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/lib/domain_layer/entities/request_state.dart` around lines 91 - 117, The Timer callback in _startTimeout should clear the internal timer reference so subsequent calls like pauseTimeout() won't act on a fired timer; modify the closure inside _startTimeout (the Timer created and assigned to _timeout) to set _timeout = null (and optionally _timeoutStartedAt = null) after calling onTimeout?.call(this) and close(), ensuring the _timeout field reflects that the timer has completed.packages/ndk/lib/domain_layer/usecases/relay_manager.dart (1)
597-633: Timeout coordination during eager AUTH looks correct but has subtle asymmetry.The pause/resume pattern correctly prevents timeout during signing. However, with multiple accounts, the first account to complete signing resumes all timeouts while other accounts may still be signing. This differs from
_handleClosedAuthRequiredwhich waits for all signings.For eager AUTH this is likely acceptable since requests aren't blocked on authentication, but consider aligning the patterns for consistency:
♻️ Optional: Wait for all signings before resuming (consistent with lazy AUTH)
void _authenticateAccounts( RelayConnectivity relayConnectivity, String challenge, Set<Account> accounts, ) { // Pause timeout for all requests on this relay during AUTH signing final requestsOnRelay = globalState.inFlightRequests.values .where((state) => state.requests.keys.contains(relayConnectivity.url)) .toList(); for (final state in requestsOnRelay) { state.pauseTimeout(); } int authCount = 0; + int pendingSignCount = accounts.where((a) => a.signer.canSign()).length; + for (final account in accounts) { if (account.signer.canSign()) { final auth = AuthEvent(pubKey: account.pubkey, tags: [ ["relay", relayConnectivity.url], ["challenge", challenge] ]); account.signer.sign(auth).then((signedAuth) { - // Resume timeout for requests after signing completes - for (final state in requestsOnRelay) { - state.resumeTimeout(); - } + pendingSignCount--; + if (pendingSignCount == 0) { + // Resume timeout for requests after all signings complete + for (final state in requestsOnRelay) { + state.resumeTimeout(); + } + } send(relayConnectivity, ClientMsg(ClientMsgType.kAuth, event: signedAuth));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/lib/domain_layer/usecases/relay_manager.dart` around lines 597 - 633, The current eager AUTH flow pauses timeouts for requestsOnRelay but resumes them as soon as any account.signer.sign completes, which can resume timeouts while other accounts are still signing; change the logic in the relay auth block so you collect the Futures returned by account.signer.sign for each signing-capable account (or otherwise count completions) and only call state.resumeTimeout() for all states after all signings complete (e.g., await Future.wait on the list of sign futures), while preserving the existing authCount==0 fast-path and the log messages; look for references to requestsOnRelay, account.signer.sign, authCount, and resumeTimeout to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/ndk/lib/domain_layer/usecases/jit_engine/jit_engine.dart`:
- Around line 168-169: The local variable doneStream is dead code—it's declared
as `doneStream = broadcastState.stateUpdates.map((state) =>
state.broadcasts.values.toList())` but never used; remove the `doneStream`
declaration and any associated unused imports so the code uses the inlined
stream expression directly (as already done later) and keep only the live
reference to `broadcastState.stateUpdates.map(...)`.
---
Nitpick comments:
In `@packages/ndk/lib/domain_layer/entities/request_state.dart`:
- Around line 91-117: The Timer callback in _startTimeout should clear the
internal timer reference so subsequent calls like pauseTimeout() won't act on a
fired timer; modify the closure inside _startTimeout (the Timer created and
assigned to _timeout) to set _timeout = null (and optionally _timeoutStartedAt =
null) after calling onTimeout?.call(this) and close(), ensuring the _timeout
field reflects that the timer has completed.
In `@packages/ndk/lib/domain_layer/usecases/relay_manager.dart`:
- Around line 597-633: The current eager AUTH flow pauses timeouts for
requestsOnRelay but resumes them as soon as any account.signer.sign completes,
which can resume timeouts while other accounts are still signing; change the
logic in the relay auth block so you collect the Futures returned by
account.signer.sign for each signing-capable account (or otherwise count
completions) and only call state.resumeTimeout() for all states after all
signings complete (e.g., await Future.wait on the list of sign futures), while
preserving the existing authCount==0 fast-path and the log messages; look for
references to requestsOnRelay, account.signer.sign, authCount, and resumeTimeout
to update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a128813c-6fe0-494d-8dfe-325142e2126a
📒 Files selected for processing (9)
packages/ndk/lib/domain_layer/entities/broadcast_state.dartpackages/ndk/lib/domain_layer/entities/request_state.dartpackages/ndk/lib/domain_layer/usecases/jit_engine/jit_engine.dartpackages/ndk/lib/domain_layer/usecases/relay_manager.dartpackages/ndk/lib/domain_layer/usecases/relay_sets_engine.dartpackages/ndk/test/mocks/mock_relay.dartpackages/ndk/test/mocks/mock_slow_signer.dartpackages/ndk/test/usecases/broadcast_test.dartpackages/ndk/test/usecases/slow_signer_timeout_test.dart
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
3 participants
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.
#398
Summary by CodeRabbit
New Features
Tests