Skip to content

Commit 38e74be

Browse files
Rama542adecaro
authored andcommitted
fix: address review comments - delegate RunWithErrors and rollback listener changes
- RunWithErrors now delegates to RunWithErrorsContext(context.Background(), ...) instead of duplicating the retry loop, keeping a single source of truth - Revert Listener.OnStatus back to RunWithContext as requested by reviewer; the repo wants to retry on all error types including unknown status - Remove TestOnStatus_UnknownStatusTerminatesWithoutRetry which tested the reverted behaviour Signed-off-by: Rama542 <Rama542@users.noreply.github.com>
1 parent 4669895 commit 38e74be

3 files changed

Lines changed: 3 additions & 51 deletions

File tree

token/services/ttx/finality/listener.go

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -93,19 +93,13 @@ func (t *Listener) OnStatus(ctx context.Context, txID string, status int, messag
9393
start := time.Now()
9494
newCtx, span := t.tracer.Start(ctx, "on_status")
9595
defer span.End()
96-
if err := t.retryRunner.RunWithErrorsContext(newCtx, func() (bool, error) {
96+
if err := t.retryRunner.RunWithContext(newCtx, func() error {
9797
err := t.runOnStatus(newCtx, txID, status, message, tokenRequestHash)
9898
if err != nil {
9999
t.logger.Errorf("finality listener on [%s] failed with error: [%+v], retrying...", txID, err)
100-
// An unrecognized status is a permanent error — retrying will never succeed.
101-
if status != network.Valid && status != network.Invalid {
102-
return true, err
103-
}
104-
105-
return false, err
106100
}
107101

108-
return true, nil
102+
return err
109103
}); err != nil {
110104
t.logger.Errorf("finality listener on [%s] failed with error: [%+v], stop.", txID, err)
111105
}

token/services/ttx/finality/listener_test.go

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -214,30 +214,6 @@ func TestOnStatus_StatusSetToDeletedForInvalidTx(t *testing.T) {
214214
"an Invalid network status should map to Deleted in local storage")
215215
}
216216

217-
// TestOnStatus_UnknownStatusTerminatesWithoutRetry verifies that an unrecognized
218-
// network status causes OnStatus to stop immediately without retrying.
219-
// With RunWithErrorsContext, permanent errors (unknown status) return (true, err)
220-
// so the retry loop exits after a single attempt.
221-
func TestOnStatus_UnknownStatusTerminatesWithoutRetry(t *testing.T) {
222-
var setCalls atomic.Int32
223-
db := &mock.TransactionDB{}
224-
db.SetStatusCalls(func(_ context.Context, _ string, _ storage.TxStatus, _ string) error {
225-
setCalls.Add(1)
226-
227-
return nil
228-
})
229-
l := newTestListener(t, db)
230-
231-
// Status 99 is not network.Valid or network.Invalid — it hits the default branch
232-
// in runOnStatus and returns a permanent error that must not be retried.
233-
start := time.Now()
234-
l.OnStatus(t.Context(), "tx1", 99, "unknown", nil)
235-
elapsed := time.Since(start)
236-
237-
assert.Equal(t, int32(0), setCalls.Load(), "SetStatus should never be called for an unknown status")
238-
assert.Less(t, elapsed, 500*time.Millisecond, "OnStatus should return immediately for permanent errors, not spin through retries")
239-
}
240-
241217
// TestOnError tests the OnError callback
242218
func TestOnError(t *testing.T) {
243219
ctx := t.Context()

token/services/utils/retry.go

Lines changed: 1 addition & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -109,25 +109,7 @@ func (f *retryRunner) RunWithContext(ctx context.Context, runner func() error) e
109109
// If it returns true, then the error or nil will be returned.
110110
// If it returns maxTimes false, then it will always return an error: either a join of all errors it encountered or a ErrMaxRetriesExceeded.
111111
func (f *retryRunner) RunWithErrors(runner func() (bool, error)) error {
112-
errs := make([]error, 0)
113-
var delay time.Duration
114-
for i := 0; f.maxTimes < 0 || i < f.maxTimes; i++ {
115-
terminate, err := runner()
116-
if terminate {
117-
return err
118-
}
119-
if err != nil {
120-
errs = append(errs, err)
121-
}
122-
delay = f.nextDelay(delay)
123-
f.logger.Warnf("Will retry iteration [%d] after a delay of [%v]. %d errors returned so far", i+1, delay, len(errs))
124-
time.Sleep(delay)
125-
}
126-
if len(errs) == 0 {
127-
return ErrMaxRetriesExceeded
128-
}
129-
130-
return errors.Join(errs...)
112+
return f.RunWithErrorsContext(context.Background(), runner)
131113
}
132114

133115
// RunWithErrorsContext retries until runner() returns true, ctx is canceled, or maxTimes

0 commit comments

Comments
 (0)