Keep tapd running when authmailbox subscription fails #1915
Conversation
Refactor to make it explicit that every error from the main event loop is currently treated as critical and results in a TAPD shutdown. This change does not alter the existing behavior but makes the criticality of error handling more obvious, paving the way to relax this in the future.
Improve code clarity.
Introduce `AddrImportErrEvent` to notify subscribers of address import errors without shutting down the main event loop. Also, ensure `AssetReceiveEvent` and `AddrImportErrEvent` implement the `Event` interface.
Introduce `AddrImportCompleteEvent` to notify subscribers when an address is successfully imported. Ensure the event implements the `Event` interface and integrate it with the main event loop.
Modify SubscribeReceiveEvents such that it silently ignores the newly added `tapgarden.Custodian` event types.
Summary of ChangesHello @ffranr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the resilience of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements to make tapd more resilient, particularly when dealing with authmailbox subscription failures. The main changes include making the NewAddr RPC synchronous by waiting for an address import confirmation, preventing crashes on unknown events, and making connection retries configurable. The changes are well-structured and address the reported issue effectively. The new integration test is a great addition to verify the fix. My feedback is minor and focuses on improving code clarity and maintainability.
Pull Request Test Coverage Report for Build 20236451398Details
💛 - Coveralls |
- Change BackoffCfg.NumTries to uint32 with clearer docs; zero now falls back to DefaultProofTransferNumTries. - Apply the default when wiring courier configs and reflect the semantics in sample-tapd.conf - Update tests to use uint32 counters and add a custodian helper to wait for address import events. This change will be useful later for authmailbox connection attempts.
Introduce `MaxConnectAttempts` to allow configurable retry limits for mailbox connection attempts. Replace hardcoded retry limits with this new parameter and update related configurations and tests accordingly.
- Register a custodian subscriber before creating the address to avoid racing the import event. - Block until the custodian reports success/error (with timeout fallback) and surface failures. - Clean up the subscription after handling the import outcome.
Add an integration test that exercises V2 address creation with an unreachable mailbox courier and a skipped upfront connection check. The test verifies that the resulting custodian subscription failure is handled without causing tapd to crash or shut down.
ffranr
force-pushed
the
wip/tapgarden-custodian/fix-errchan-handling
branch
from
ad457a6 to
2ff1a5e
Compare
| "nonexistent.invalid:65500", | ||
| ) | ||
|
|
||
| t.Logf("Trying to create an address with bad mailbox %s", badCourier) |
There was a problem hiding this comment.
Please no logging in the itests 🙏 . The test purpose and its assertions are the only thing we need.
There was a problem hiding this comment.
Why no logging? Sometime I find log messages helpful when debugging a failing itest.
|
|
||
| _, err = tapd.GetInfo(infoCtx, &taprpc.GetInfoRequest{}) | ||
| require.NoError(t.t, err) | ||
| t.Logf("Tapd is still up and running") |
There was a problem hiding this comment.
Same remark, this is what the test does so logging is useless if the test passes.
Fix for #1909
NewAddrRPC subscribe and wait for the outcome with a timeout, and ignore unsupported events in the RPC stream so import errors don’t crash the daemon.