-
Notifications
You must be signed in to change notification settings - Fork 48
feat: Reliable Channel: Status Sync, overflow protection, stop TODOs #2729
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
size-limit report 📦
|
9cc27b1 to
3f40996
Compare
This comment was marked as outdated.
This comment was marked as outdated.
| }); | ||
|
|
||
| it("should start and stop interval correctly", () => { | ||
| // TODO: Skipped because the global state is not being restored and it breaks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why so? test is passing on master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My new tests aren't yet on master and they don't pass with this enabled because setInterval does not work any more once with the code in this test.
Tests on master aren't passing either: #2648
I have spent half a day on this and I can guarantee that the code in this test pollutes the global state. It is not being restored (at least with node 22.17.0, I can try with other node versions in a couple of weeks).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces several improvements to reliable channels including status synchronization, proper cleanup routines, and overflow protection for message history.
- Adds
SyncStatusclass to track and emit events for message synchronization state - Implements proper cleanup in
ReliableChannel.stop()to prevent resource leaks - Delivers irretrievably lost messages to allow clients to resume protocol participation
- Extracts logic into reusable classes (
RandomTimeout,RetryManager.stop()) - Adds overflow protection to
MemLocalHistorywith configurable max length
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk/src/reliable_channel/sync_status.ts | New class to track and emit sync status events for missing/received/lost messages |
| packages/sdk/src/reliable_channel/sync_status.spec.ts | Test coverage for SyncStatus class functionality |
| packages/sdk/src/reliable_channel/random_timeout.ts | Extracted utility class for managing randomized timeouts with multipliers |
| packages/sdk/src/reliable_channel/reliable_channel.ts | Integrated SyncStatus, refactored event listeners into methods, improved stop() cleanup |
| packages/sdk/src/reliable_channel/reliable_channel.spec.ts | Added cleanup in afterEach hooks, moved test suite to avoid skipping, new test for lost message acknowledgment |
| packages/sdk/src/reliable_channel/reliable_channel_sync_status.spec.ts | Integration tests for sync status functionality |
| packages/sdk/src/reliable_channel/retry_manager.ts | Added stop() method to clear pending timeouts |
| packages/sdk/src/reliable_channel/index.ts | Exports new SyncStatus types and classes |
| packages/sds/src/message_channel/message_channel.ts | Delivers messages marked as irretrievably lost to resume log participation |
| packages/sds/src/message_channel/mem_local_history.ts | Added overflow protection with configurable max length |
| packages/sds/src/message_channel/mem_local_history.spec.ts | Test coverage for overflow protection behavior |
| packages/utils/src/common/mock_node.ts | Improved mock implementations for unsubscribe and stop methods |
| packages/sdk/src/waku/wait_for_remote_peer.spec.ts | Added sinon.restore() in afterEach for proper test cleanup |
| packages/sdk/src/query_on_connect/query_on_connect.spec.ts | Added sinon.restore() in afterEach for proper test cleanup |
| packages/sdk/src/light_push/retry_manager.spec.ts | Skipped test that modifies global state |
| packages/sdk/src/light_push/light_push.spec.ts | Added sinon.restore() in afterEach for proper test cleanup |
| packages/core/src/lib/stream_manager/stream_manager.spec.ts | Added sinon.restore() in afterEach for proper test cleanup |
| packages/sdk/src/reliable_channel/reliable_channel_sync.spec.ts | Reduced delay time in test |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/sdk/src/reliable_channel/reliable_channel_sync_status.spec.ts
Outdated
Show resolved
Hide resolved
packages/sdk/src/reliable_channel/reliable_channel_sync_status.spec.ts
Outdated
Show resolved
Hide resolved
packages/sdk/src/reliable_channel/reliable_channel_sync_status.spec.ts
Outdated
Show resolved
Hide resolved
packages/sdk/src/reliable_channel/reliable_channel_sync_status.spec.ts
Outdated
Show resolved
Hide resolved
packages/sdk/src/reliable_channel/reliable_channel_sync_status.spec.ts
Outdated
Show resolved
Hide resolved
weboko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, but looks quite complex
ec6f3d5 to
a6323d8
Compare
This is to re-enable participation in the SDS protocol. Meaning the received message with missing dependencies becomes part of the causal history, re-enabling acknowledgements.
Return a "synced" or "syncing" status on `ReliableChannel.status` that let the developer know whether messages are missing, and if so, how many.
# Conflicts: # packages/sdk/src/reliable_channel/reliable_channel.ts
a6323d8 to
4dde565
Compare
|
wow, the AI reviews are good.. |
I tried my best to isolate the logic, will see if I can simplify further. |
c6add1a to
025dfa2
Compare
|
@danisharora099 would you prefer that I remove the concept of Claude thinks it's too complicated and the consumer should just look at I am thinking it's better this way, so that consumer knows what it means, and can in a second step, decide to also check |
|
Trying to understand why allure fails (no individual test fails). |
danisharora099
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works great in https://opchan.app , LGTM!
I like the "syncing" and "synced"! |
Problem / Description
Various improvements for reliable channels based on developer feedback as well as better code organisation by extracting some classes.
Solution
ReliableChannel.syncStatusthat emit events and information on missing messagesReliableChannel.stopto ensure there are no hanging timeout, intervals, retries or subscriptionssetInterval,clearInterval) being modified and not restored in some tests.ReliableChannelto make it leaner, and test more easily/thoroughlyNotes
For (3) @jm-clius @shash256 the spec is open to interpretation, should we update it?
TODOs:
Need to cross-reference with fix: cleanup routines when stopping node or reliable channel #2717 see if anything else should be done
Resolves
Related to
Checklist