Skip to content

Conversation

@juslesan
Copy link
Contributor

@juslesan juslesan commented Oct 24, 2025

Summary

Moved exit handlers from the trackerless-network to the node package. This ensures that the operator nodes will still gracefully leave the network if the process is exited. Also removed the on unload event listener in NetworkStack. Users of the SDK are being made aware of the change by updating the SDK's README to include a section about properly cleaning up the Network node when the StreamrClient is no longer needed.

Other Changes

  • The exit case is no longer handled as we cannot do asynchronous operations there.

Note

Move shutdown handling to the node CLI, remove global exit/unload handlers from trackerless-network, and document proper StreamrClient cleanup.

  • Node CLI (packages/node/bin/streamr-node.ts)
    • Add graceful shutdown handlers for SIGINT, SIGTERM, SIGUSR1, SIGUSR2.
    • On uncaughtException/unhandledRejection, log fatal and stop broker before exit.
  • Trackerless Network (packages/trackerless-network/src/NetworkStack.ts)
    • Remove global process/window exit handlers and instance tracking; simplify stop().
  • Docs
    • Add cleanup guidance to docs/docs/usage/sdk/how-to-use.md and packages/sdk/README.md to call StreamrClient.destroy() when done.

Written by Cursor Bugbot for commit f2d88f0. This will update automatically on new commits. Configure here.

@juslesan juslesan requested review from ptesavol and teogeb October 24, 2025 12:16
@github-actions github-actions bot added network Related to Network Package node labels Oct 24, 2025
Copy link
Contributor

@teogeb teogeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be worth the effort to update the sdk's README to make sure that users are aware that they need to call StreamrClient#destroy always when shutting down the process.

Yes, let's add that

For browser nodes the graceful leaving on unload event is still kept in the trackerless-network. Alternatively the on unload event handler could be removed as well. If this is done the users must be made aware that they need to call NetworkNode#stop manually when a browser window is closed.

The StreamrClient#destroy() does that, right? So when we have the documentation, we can remove this functionality, too.

@github-actions github-actions bot added the sdk label Nov 19, 2025
@juslesan
Copy link
Contributor Author

So if I understood correctly do you @teogeb want to remove the clean up from the on unload events as well?

@teogeb
Copy link
Contributor

teogeb commented Nov 20, 2025

So if I understood correctly do you @teogeb want to remove the clean up from the on unload events as well?

Yes

@juslesan juslesan requested a review from teogeb November 21, 2025 09:31
```

### Clean up
After the StreamrClient is no longer used or the process is shutting down it is very important to call `destroy` on the StreamrClient. This ensures that the network node of the client will be shutdown gracefully.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that would make sense. How do I update this page?

Copy link
Contributor

@teogeb teogeb Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is this file in this repo: docs/docs/usage/sdk/how-to-use.md

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the final PR Bugbot will review for you during this billing cycle

Your free Bugbot reviews will reset on November 28

Details

Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

const shutdown = async (exitCode: number) => {
await broker.stop()
process.exit(exitCode)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Missing guard allows concurrent broker shutdown attempts

The shutdown function lacks a guard against concurrent invocations. If multiple signals arrive rapidly (e.g., quick successive Ctrl+C presses), the function can be triggered multiple times before process.exit() is called. This leads to concurrent broker.stop() calls, which can cause stopServer and plugin stop() methods to be invoked concurrently. The old NetworkStack.stop() had a stopped flag guard that prevented this, but the new broker-level stop() has no such protection.

Additional Locations (1)

Fix in Cursor Fix in Web

@juslesan juslesan requested a review from teogeb November 27, 2025 16:05
process.on('unhandledRejection', (err) => {
logger.fatal('Encountered unhandledRejection', { err })
shutdown(1)
})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Unhandled rejection in error handlers may cause recursion

The shutdown async function is called without awaiting and without a .catch() handler in both the uncaughtException and unhandledRejection listeners. If broker.stop() throws during shutdown, the resulting promise rejection goes unhandled. This is particularly problematic in the unhandledRejection handler: if shutdown(1) fails, it creates another unhandled rejection, which triggers the same handler again, leading to infinite recursion until the process crashes or exceeds resource limits.

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

network Related to Network Package node sdk

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants