fix(types): add timeout per request in retrieve helper

[julienrbrt]

Overview

Downloading many blobs would timeout after 10s. We should timeout per request and for all requests.

[github-actions]

The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	⏩ skipped	✅ passed	✅ passed	Oct 2, 2025, 1:01 PM

[claude]

Code Review: Timeout Per Request in Retrieve Helper

Summary

This PR addresses an important timeout issue where downloading many blobs would timeout after 10 seconds total. The fix properly applies timeout per individual request rather than for all requests combined.

---

✅ Strengths

1. Correct Problem Identification: The fix properly addresses the root cause - the timeout was applied once at the RetrieveFromDA level, causing all subsequent operations to share the same deadline.

2. Clean API Design: Adding requestTimeout as a parameter to RetrieveWithHelpers makes the timeout behavior explicit and configurable.

3. Proper Context Management: The implementation correctly creates new timeout contexts for each operation:

   • GetIDs gets its own timeout context
   • Each batch in the Get loop gets its own timeout context with proper cleanup via defer cancel()

4. Good Test Coverage: The new timeout tests properly verify:

   • Timeout during GetIDs operation
   • Timeout during Get operation
   • Error messages contain expected context

5. Documentation: Added helpful comment explaining the requestTimeout parameter.

---

🔍 Code Quality Observations

1. Context Cancellation Pattern (types/da.go:184-186)

getBlobsCtx, cancel := context.WithTimeout(ctx, requestTimeout)
batchBlobs, err := da.Get(getBlobsCtx, idsResult.IDs[i:end], namespace)
cancel()

✅ Good: Using immediate cancel() after the call instead of defer cancel() is correct here since it's in a loop - this releases resources faster.

⚠️ Minor Issue: In the GetIDs section (line 127-128), you use defer cancel() which is fine, but for consistency with the batch loop pattern, you could use immediate cancellation.

2. Timeout Value (da_retriever.go:23)

const defaultDATimeout = 10 * time.Second

💭 Consider: 10 seconds per request seems reasonable, but:

• For GetIDs: Single operation, 10s is generous
• For Get: Batch of 100 blobs, 10s might be tight depending on blob sizes and network conditions

Suggestion: Consider making this configurable via config or adding a comment explaining the rationale for 10s.

3. Test Timeout Values (da_test.go:265, 291)

result := types.RetrieveWithHelpers(context.Background(), mockDA, logger, dataLayerHeight, encodedNamespace.Bytes(), 1*time.Millisecond)

✅ Good: Using 1ms ensures tests run quickly and reliably trigger timeouts.

---

🐛 Potential Issues

No Critical Bugs Found

The implementation correctly handles:

• Context cancellation and cleanup
• Error propagation
• Timeout behavior per operation

---

🚀 Performance Considerations

1. Batch Processing: The code processes up to 100 blobs per batch, which is reasonable. Each batch gets its own 10s timeout.

2. Total Time Calculation:

   • Previously: Max 10s total regardless of blob count
   • Now: 10s * (1 + ceil(blob_count/100)) for GetIDs + all Get batches
   • For 500 blobs: ~60s max (1 GetIDs + 5 Get batches)

3. Potential Improvement: For large blob counts, consider:

   • Adaptive timeouts based on batch size
   • Parallel batch fetching (if DA layer supports it)
   • Configurable batch sizes

---

🔒 Security Considerations

✅ No security concerns identified

The changes:

• Don't expose sensitive data
• Properly handle context cancellation (prevents resource leaks)
• Maintain error handling integrity

---

📝 Additional Suggestions

1. Logging Enhancement: Consider adding a log entry showing how many batches will be processed:

logger.Debug().Int("total_ids", len(idsResult.IDs)).Int("batches", (len(idsResult.IDs)+batchSize-1)/batchSize).Msg("Starting blob retrieval")

2. Metrics: Consider adding metrics for:

   • Timeout occurrences per operation type
   • Actual retrieval duration per batch
   • Batch sizes processed

3. Configuration: Make timeout configurable:

type DARetrieverConfig struct {
    GetIDsTimeout time.Duration
    GetBlobsTimeout time.Duration
    BatchSize int
}

4. Error Context: When timeout occurs in a batch, the error message shows batch range but not which specific IDs failed. This is probably fine for debugging.

---

✅ Recommendations

Approve with minor suggestions

The PR successfully fixes the timeout issue and is well-tested. The implementation is clean and follows Go best practices.

Before merging:

• ✅ Ensure all tests pass (make test)
• ✅ Run integration tests (make test-integration) to verify behavior with real DA layers
• ✅ Consider adding documentation about timeout behavior in relevant docs

Nice to have (for future PRs):

• Make timeout configurable via config
• Add metrics for timeout tracking
• Consider adaptive timeouts for large blob batches

---

📚 References

• Follows patterns from CLAUDE.md: proper error handling, context usage
• Aligns with Go conventions: defer for cleanup, clear variable names
• Test patterns match existing codebase standards

Great work fixing this issue! 🎉

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.36%. Comparing base (1917bd7) to head (69a71a6).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2726   +/-   ##
=======================================
  Coverage   66.35%   66.36%           
=======================================
  Files          77       77           
  Lines        7901     7902    +1     
=======================================
+ Hits         5243     5244    +1     
  Misses       2173     2173           
  Partials      485      485           

Flag	Coverage Δ
combined	66.36% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[claude]

Code Review: PR #2726 - Add timeout per request in retrieve helper

Summary

This PR fixes a timeout issue where downloading many blobs would timeout after 10 seconds total. The fix applies a 10-second timeout per request (GetIDs and Get operations) instead of a single timeout for all requests combined.

✅ Strengths

1. Correct Problem Identification: The PR correctly identifies that a single 10s timeout for the entire retrieval operation (which may involve multiple GetIDs and Get calls) is insufficient when retrieving many blobs.

2. Good Test Coverage: The new timeout tests properly verify timeout behavior for both GetIDs and Get operations using context cancellation.

3. Backward Compatible: The timeout value remains 10 seconds, just applied differently - per request rather than globally.

4. Clean Implementation: The timeout handling is properly scoped with deferred cancel() calls to prevent context leaks.

---

🔍 Issues and Concerns

1. Critical: Unused Constant in da_retriever.go

Severity: High

The old constant dAFetcherTimeout is still defined but no longer used after removing the timeout from RetrieveFromDA:

// Line 22 in da_retriever.go (current file)
const dAFetcherTimeout = 10 * time.Second  // ❌ UNUSED after changes

Meanwhile, the PR adds defaultDATimeout in types/da.go. This creates confusion and dead code.

Recommendation:

• Remove dAFetcherTimeout from block/internal/syncing/da_retriever.go:22
• Consider making defaultDATimeout in types/da.go a public constant if it should be reusable, or keep it package-private if it's specific to this helper

2. Design Issue: Timeout Removed from Top-Level Operation

Severity: Medium

The original code had a timeout for the entire RetrieveFromDA operation:

// BEFORE (lines 67-68 in old code)
ctx, cancel := context.WithTimeout(ctx, dAFetcherTimeout)
defer cancel()

This has been completely removed. Now there's only per-request timeouts in RetrieveWithHelpers, but no overall timeout for the entire retrieval operation.

Problem: If you're retrieving from two namespaces (header + data), and each namespace has multiple batches, the total time is unbounded. For example:

• GetIDs for namespace 1: 10s
• Get blobs for namespace 1 (3 batches): 30s
• GetIDs for namespace 2: 10s
• Get blobs for namespace 2 (3 batches): 30s
• Total: 80 seconds (vs the intended 10s max)

Recommendation: Consider adding back an overall timeout for RetrieveFromDA, perhaps with a larger value (e.g., 60s) to allow for multiple requests while still preventing indefinite hangs.

3. Potential Resource Leak in Get Loop

Severity: Low-Medium

In types/da.go:184-185, the cancel function is called inside the loop:

for i := 0; i < len(idsResult.IDs); i += batchSize {
    getBlobsCtx, cancel := context.WithTimeout(ctx, requestTimeout)
    batchBlobs, err := da.Get(getBlobsCtx, idsResult.IDs[i:end], namespace)
    cancel()  // ✅ Good - called immediately
    // ...
}

This is correct, but it's slightly unconventional. The more idiomatic Go pattern would be:

for i := 0; i < len(idsResult.IDs); i += batchSize {
    func() {
        getBlobsCtx, cancel := context.WithTimeout(ctx, requestTimeout)
        defer cancel()
        batchBlobs, err := da.Get(getBlobsCtx, idsResult.IDs[i:end], namespace)
        // ...
    }()
}

However, your current approach is valid and actually clearer in this case. No change needed, but be aware some linters might flag it.

4. Missing Error Context in Tests

Severity: Low

The new timeout tests check for "context deadline exceeded" in error messages:

assert.Contains(t, result.Message, "context deadline exceeded")

However, the actual error from context.DeadlineExceeded is "context deadline exceeded", but when wrapped in RetrieveWithHelpers, it becomes:

• "failed to get IDs: context deadline exceeded"
• "failed to get blobs for batch 0-1: context deadline exceeded"

The tests should verify the full message format for clarity. Consider:

assert.Contains(t, result.Message, "failed to get IDs")
assert.Contains(t, result.Message, "context deadline exceeded")

5. Documentation Could Be Clearer

Severity: Low

The comment for requestTimeout parameter states:

// requestTimeout defines the timeout for the each retrieval request.

Minor grammar issue: "the each" → "each". Better version:

// requestTimeout defines the timeout for each individual retrieval request (GetIDs and Get operations).

---

🎯 Performance Considerations

✅ Positive: The per-request timeout approach is better for scenarios with many blobs, as it allows each operation to have adequate time.

⚠️ Consider: With the current implementation, retrieving from 2 namespaces with 100 IDs each (2 batches per namespace) would take:

• 2 × GetIDs @ 10s = 20s
• 4 × Get @ 10s = 40s
• Total potential: 60s

This is a 6x increase from the previous 10s limit. This might be intentional, but should be documented or made configurable.

---

🔒 Security Considerations

✅ No security concerns identified. The timeout changes don't introduce vulnerabilities.

✅ Proper context handling prevents goroutine leaks.

---

📋 Recommendations Summary

Must Fix:

1. Remove unused dAFetcherTimeout constant from da_retriever.go:22

Should Consider:
2. Add back an overall timeout to RetrieveFromDA (perhaps 60s or configurable)
3. Update test assertions to check full error message structure
4. Fix grammar in requestTimeout comment

Optional:
5. Document the timeout behavior change and potential total duration in PR description or code comments

---

✅ Final Verdict

The core fix is sound and addresses a real issue. With the suggested fixes (especially removing the unused constant), this PR will be in good shape. The removal of the overall operation timeout should be explicitly discussed with the team to ensure it aligns with the intended behavior.

Great work on the comprehensive test coverage! 🎉