feat(tx_cache): surface auth token retrieval errors #116
Conversation
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
ENG-1798: Add BuilderTxCacheError enum to properly surface auth token retrieval failures instead of silently using an empty string. Changes: - Add BuilderTxCacheError with TokenRetrieval variant wrapping RecvError - Add TxCache variant wrapping existing TxCacheError - Replace unwrap_or_else with ? operator to propagate errors - Update local Result type alias to use BuilderTxCacheError This is a breaking change for consumers that expect the old Result type, but surfaces the actual error cause for better debugging.
Evalir
requested review from
Evalir and
prestwich
and removed request for
prestwich
Evalir
approved these changes
Feb 6, 2026
Fraser999
requested changes
Feb 7, 2026
Contributor
Fraser999
left a comment
Fraser999
left a comment
There was a problem hiding this comment.
The BuilderTxCacheError::TxCache(TxCacheError::Reqwest(err)) issue is a blocker I think.
Comment on lines
+18
to
+21
| assert!(matches!( | ||
| bundles, | ||
| BuilderTxCacheError::TxCache(TxCacheError::NotOurSlot) | ||
| )); |
Contributor
There was a problem hiding this comment.
I don't think this matches the new behaviour, but the test is ignored, so not picked up in CI.
prestwich
reviewed
Feb 7, 2026
src/perms/tx_cache.rs
Outdated
| #[derive(Debug, Error)] | ||
| pub enum BuilderTxCacheError { | ||
| /// Failed to retrieve the authentication token. | ||
| #[error("failed to retrieve auth token: {0}")] |
Member
There was a problem hiding this comment.
this isn't a "failed to retrieve" right? this error is permanent and persistent, and implies something has gone wrong with the background auth task resulting in the sender being dropped?
- Use TxCacheError::from(err) to properly map reqwest errors to appropriate variants (NotFound, NotOurSlot, or Reqwest) - Use core::result::Result for consistency - Improve TokenRetrieval error message to clarify it indicates the background auth task stopped
Contributor
Author
|
Addressed review feedback:
|
Fraser999
approved these changes
Feb 8, 2026
Contributor
Fraser999
left a comment
Fraser999
left a comment
There was a problem hiding this comment.
It'd be good to have the test issue resolved, but that's probably not a blocker, so approving to unblock the PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Summary
Surfaces auth token retrieval errors instead of silently using an empty string.
Changes
BuilderTxCacheErrorenum with:TokenRetrievalvariant wrappingwatch::error::RecvErrorTxCachevariant wrapping existingTxCacheErrorunwrap_or_elsewith?operator to propagate errorsResulttype alias to useBuilderTxCacheErrorBreaking Change
This changes the return type from
signet_tx_cache::error::Resultto a new localResulttype. Consumers will need to handle the newBuilderTxCacheErrortype.Why
Per the ticket: "Right now, an empty string is used if the token retrieval fails, placing the responsibility of returning an appropriate error on the service being called. We should wrap the original error type, and add a variant that points out that there was a problem retrieving the token instead, surfacing the inner Recv error."
Closes ENG-1798