rpc: implement Drop for KaspaRpcClient to prevent resource leaks

[atharaldsen]

Summary

Implement RAII pattern for KaspaRpcClient by adding Drop trait to prevent resource leaks.

Changes

• Add initiate_cleanup() method to Inner struct
• Implement Drop for Inner that calls cleanup when last reference is dropped
• Add "Resource Management" documentation to KaspaRpcClient

Behavior

When client is dropped while still connected:

1. Logs warning: "Call disconnect() before dropping for clean shutdown"
2. Closes notification channels synchronously
3. Spawns fire-and-forget async cleanup task

Why Drop on Inner?

KaspaRpcClient is Clone via Arc<Inner>, so Drop fires only when the last reference is dropped.

Fixes #683

[biryukovmaxim]

@atharaldsen

I believe it requires writing a test that demonstrates initial problem and fix.

Creation of daemon.
https://github.com/kaspanet/rusty-kaspa/blob/master/testing/integration/src/common/daemon.rs

You need to add wrpc client.

Other references of how to test anything.
https://github.com/kaspanet/rusty-kaspa/blob/master/testing/integration/src/daemon_integration_tests.rs

[aspect]

There are client examples in https://github.com/kaspanet/rusty-kaspa/tree/master/rpc/wrpc/examples

They should probably be moved into integration tests. (albeit integration tests don't do wasm coverage)

[atharaldsen]

thanks @biryukovmaxim and @aspect :-) Added.

[biryukovmaxim]

https://github.com/biryukovmaxim/rusty-kaspa/tree/fix-rpc-client-drop-enhance-testing
@atharaldsen I added couple commits on top. they are not subjects of merging, although they show the issue is not solved. look at the test. drop impl is not called since the Inner has lots of strong references. I dont mind to have drop impl like you added, but the issue is subject of investigation to find what still holds Arc references and prevents execution of drop logic you added, what leads to resource leakage.

@aspect could you please give more detailed instructions, where to start such investigation and what may still hold references? if you have some

[atharaldsen]

Thanks for digging into this @biryukovmaxim - you're right.

I traced the circular reference with Claude and found:

• Inner holds notifier: Arc<Mutex<Option<RpcClientNotifier>>>
• Notifier holds subscribers: Vec<Arc<Subscriber>>
• Subscriber holds subscription_manager: Arc<dyn SubscriptionManager> (which is Arc<Inner>)

Plus the spawned tasks in start_rpc_ctl_service() and Subscriber::spawn_subscription_receiver_task() hold Arc<Inner> for their lifetime.

So even when the user drops KaspaRpcClient, the ref count on Inner never reaches 0.

What would be the preferred fix here?

1. Use Weak<Inner> in Subscriber instead of Arc (breaks the cycle)
2. Close the channels in Drop to force tasks to exit
3. Require explicit stop() call (document the limitation)
4. Something else?

Happy to implement whichever approach you think fits the architecture best.

[biryukovmaxim]

1. Use Weak<Inner> in Subscriber instead of Arc (breaks the cycle)
2. Close the channels in Drop to force tasks to exit
3. Require explicit stop() call (document the limitation)
4. Something else?

1 and 2 are both acceptable to me. I would prefer 2. My intuition tells it will be simpler and will have less changes

@atharaldsen

[atharaldsen]

Addressed the feedback from @biryukovmaxim and @aspect:

Changes:

1. Break reference cycle in stop_notifier(): After notifier.join() completes, we now clear the notifier (= None). This breaks the cycle Inner → Notifier → Subscriber → Arc<Inner> and allows Inner::drop() to fire properly.

2. Added auto_cleanup_on_drop flag: Per aspect´s suggestion, the Drop cleanup is now gated by a boolean flag (defaults to true for safety). This allows applications requiring explicit lifecycle control to disable it.

The cycle-breaking is the key fix — without it, Inner::drop() would never fire because the strong count couldn't reach 0.

Ready for re-review.

[biryukovmaxim]

@atharaldsen I pushed test that shows that inner is still not freed. I believe currently this branch should focus on making the test pass. another thing is that Inner accepts bool flag, however it's part of private API. maybe I missed that but I believe it should client should have constructor-methor or builder/params to disable it. otherwise its pointless since no one can disable cleaning - meaning it always work with true flag