Skip to content

Fix panic in shouldRetry type assertion and remove deprecated rand.Seed#1438

Open
Nepomuk5665 wants to merge 2 commits intodatabricks:mainfrom
Nepomuk5665:fix-shouldretry-type-assertion
Open

Fix panic in shouldRetry type assertion and remove deprecated rand.Seed#1438
Nepomuk5665 wants to merge 2 commits intodatabricks:mainfrom
Nepomuk5665:fix-shouldretry-type-assertion

Conversation

@Nepomuk5665
Copy link
Copy Markdown

Summary

  • Fix potential panic in shouldRetry function by using comma-ok idiom for type assertion
  • Remove deprecated rand.Seed call that was re-seeding on every backoff attempt
  • Add test coverage for shouldRetry with various error types

Bug Description

Type Assertion Panic

The shouldRetry function in retries/retries.go used a direct type assertion:

e := err.(*Err)  // This panics if err is not *Err type

While this function is internal and typically receives *Err values from Wait and Poll functions, a direct type assertion is unsafe. If any code path passes a non-*Err error, the program will panic with a runtime error.

The fix uses the comma-ok idiom to safely handle any error type:

e, ok := err.(*Err)
if !ok || e == nil {
    return false
}

Deprecated rand.Seed

The backoff function called rand.Seed(time.Now().UnixNano()) on every invocation. This has two issues:

  1. Deprecated since Go 1.20: The global random number generator is now automatically seeded
  2. Wasteful: Re-seeding on every retry attempt is unnecessary and has performance implications

Testing

  • All existing tests pass
  • Added new test TestShouldRetryWithNonErrType that verifies the fix handles various error types without panicking

- Use comma-ok idiom in shouldRetry to handle non-*Err error types safely
- Remove deprecated rand.Seed call that was re-seeding on every backoff
- Add test coverage for shouldRetry with various error types

The shouldRetry function previously used a direct type assertion
that would panic if passed an error not of type *Err. While this
function is internal and typically receives *Err values, the type
assertion should be safe to handle unexpected inputs.

The rand.Seed call has been deprecated since Go 1.20 as the global
random number generator is now automatically seeded. Additionally,
calling Seed on every retry attempt was wasteful.
@Nepomuk5665 Nepomuk5665 force-pushed the fix-shouldretry-type-assertion branch from 63db9d1 to 0cefa26 Compare January 23, 2026 19:28
@github-actions
Copy link
Copy Markdown

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/sdk-go

Inputs:

  • PR number: 1438
  • Commit SHA: 0cefa2609c108943d86c68300e2cce5ca343f817

Checks will be approved automatically on success.

@github-actions
Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. If this PR is still relevant, please leave a comment or push new changes to keep it open. Thank you for your contributions.

@github-actions github-actions bot added the stale The PR has been marked as "stale" and will automatically be closed if no further activity. label Mar 27, 2026
@renaudhartert-db
Copy link
Copy Markdown
Contributor

Hi @Nepomuk5665, thanks for taking the time to contribute.

I had a closer look at the shouldRetry change. I don't think the panic can actually happen — shouldRetry is a private function only called from Wait() and Poll(), and both take callbacks that return *Err. The type system guarantees that only *Err or nil ever reaches shouldRetry, so the direct type assertion is safe by construction. The new !ok branch is effectively a dead code path.

The rand.Seed removal on the other hand is a good cleanup.

Would you be open to scoping this PR down to the rand.Seed removal only, as an Internal Change? That part we can merge quickly.

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

Labels

stale The PR has been marked as "stale" and will automatically be closed if no further activity.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants