refactor(block): centralize timeout in client

[julienrbrt]

Overview

Small improvements on timeout in da client and config.

Ref: #2902, #2878

[github-actions]

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

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	⏩ skipped	✅ passed	✅ passed	Dec 2, 2025, 10:20 AM

[claude]

Claude finished @julienrbrt's task —— View job

---

Code Review: Centralize Timeout in DA Client

I've completed a comprehensive review of this refactoring PR that centralizes timeout configuration in the DA client.

Summary

This is a well-executed refactoring that improves timeout handling consistency across the DA client. The changes centralize timeout management by:

1. Applying the defaultTimeout to both Submit and Retrieve operations in the DA client
2. Removing the now-redundant submissionTimeout constant from da_submitter.go
3. Increasing the default timeout from 30s to 60s
4. Cleaning up outdated tests and documentation

Overall Assessment: ✅ Approved - This is a clean refactoring with good adherence to Go best practices. I've identified a few minor points for consideration but nothing blocking.

---

Detailed Findings

✅ Code Quality & Architecture

Strengths:

• Excellent centralization: Moving timeout management into the DA client layer creates a cleaner separation of concerns
• Consistent application: Timeout is now applied to both Submit (line 75) and Retrieve (line 166) operations using context.WithTimeout
• Proper resource cleanup: All timeout contexts are properly cleaned up with defer cancel() calls
• Idiomatic Go: The use of context.WithTimeout follows Go concurrency best practices

Minor Observation:

• In client.go:75-76, the timeout is applied to Submit operations. The DA submitter in da_submitter.go:403 calls this, but the retry loop doesn't account for the cumulative timeout across retries. This is likely intentional (each attempt gets the full timeout), but worth noting that a slow DA could consume maxAttempts * timeout total time.

✅ Error Handling & Resilience

Strengths:

• Proper context hierarchy maintained - child contexts created from parent contexts
• Timeout errors are handled through existing error classification in Submit() (lines 88-111)
• The batched retrieval in Retrieve() (lines 222-241) correctly creates individual timeout contexts per batch while maintaining link to parent context

Enhancement Opportunity:

• Consider whether StatusContextDeadline error handling in Submit() (line 110) should differentiate between client-imposed timeouts vs. caller cancellation. Currently both map to the same status code.

✅ Configuration & Defaults

Strengths:

• The default timeout increase from 30s to 60s (lines 57, 79 in defaults.go) aligns with PR chore: bump default timeout #2902 mentioned in the description
• Fallback logic at client.go:56-58 ensures a sensible default if not configured
• Configuration documentation updated to reflect broader applicability (pkg/config/config.go:170)

Recommendation:

• The 60s timeout is hardcoded in multiple places. Consider adding a constant in client.go:

  const defaultClientTimeout = 60 * time.Second

  This would make future adjustments easier and more visible.

✅ Testing

Strengths:

• Test expectations updated correctly (client_test.go:109)
• Removal of flaky timeout test (da_retriever_test.go) is justified since timing is now controlled by the DA client layer
• Remaining timeout test (TestDARetriever_RetrieveFromDA_TimeoutFast) still validates timeout error handling

Observation:

• The removed test (TestDARetriever_RetrieveFromDA_Timeout) was skipped anyway with timing issues. Good cleanup.
• Consider adding a unit test in client_test.go that explicitly validates the timeout is applied (e.g., mock DA that hangs, verify context deadline exceeded is returned)

✅ Documentation

Strengths:

• Comments updated in config.go to reflect broader scope: "Per-request timeout applied to DA interactions" (line 170)
• CLI flag help text updated accordingly (line 330)

Clarity Enhancement:

• The comment at client.go:75-76 could be more explicit about which operations this timeout applies to. Consider:

  // Submit submits blobs to the DA layer with the specified options.
  // Applies a per-request timeout (defaultTimeout) to the submission operation.

✅ Performance & Resource Efficiency

Strengths:

• Timeout prevents indefinite blocking on slow/unresponsive DA nodes
• Batched retrieval (Retrieve() lines 220-241) balances throughput and timeout granularity - each batch gets its own timeout

Consideration:

• For large batch retrievals with many batches, the cumulative timeout could be significant. The current design (per-batch timeout) is reasonable but means total retrieval time is unbounded. This appears intentional and acceptable.

✅ Security

No Issues Found:

• Timeout handling reduces DoS surface by preventing indefinite waits
• Context cancellation propagation works correctly
• No sensitive data exposure in timeout error messages

---

Code-Specific Comments

block/internal/da/client.go:75-76

submitCtx, cancel := context.WithTimeout(ctx, c.defaultTimeout)
defer cancel()

✅ Good: This ensures Submit operations cannot hang indefinitely. The parent context is still respected (if it's already cancelled/timed out, the child will inherit that).

block/internal/da/client.go:166-167

getIDsCtx, cancel := context.WithTimeout(ctx, c.defaultTimeout)
defer cancel()

✅ Good: Consistent with Submit. Note that the subsequent batched Get operations (lines 225-227) also apply per-batch timeouts, which is correct.

block/internal/submitting/da_submitter.go:22-24

-const (
-	submissionTimeout = 60 * time.Second
-	initialBackoff    = 100 * time.Millisecond
-)
+const initialBackoff = 100 * time.Millisecond

✅ Good: Removes duplication. The timeout is now managed by the DA client layer.

block/internal/syncing/da_retriever_test.go (removed lines 113-141)
✅ Good: Removing flaky timing tests is appropriate. The timeout behavior is better tested at the client layer.

---

Summary of Recommendations

High Priority:

• None - the code is production-ready as-is

Medium Priority:

1. Consider adding a named constant for the default 60s timeout in client.go
2. Add explicit timeout test case in client_test.go to validate context deadline handling

Low Priority:

1. Enhance inline documentation to be more explicit about timeout application
2. Consider documenting the cumulative timeout behavior in retry scenarios

---

Final Verdict

This refactoring successfully centralizes timeout configuration, improving code maintainability and consistency. The implementation follows Go best practices with proper context management and resource cleanup. Testing changes are appropriate given the new architecture.

Recommendation: Approve and Merge ✅

The code is well-structured, safe, and improves the codebase. The minor suggestions above are enhancements for future consideration, not blockers.

---

[gemini-code-assist]

Summary of Changes

Hello @julienrbrt, I'm Gemini Code Assist¹! 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 refactors the Data Availability (DA) client's timeout mechanisms to centralize and standardize how timeouts are applied across various operations. By moving timeout control into the DA client itself and increasing the default duration, the change aims to provide more consistent and robust handling of potentially slow DA layer interactions, while also simplifying configuration and removing redundant timeout definitions.

Highlights

• Centralized Timeout Management: The Data Availability (DA) client now centralizes timeout handling, ensuring consistent application of a default timeout across submission and retrieval operations.
• Increased Default Timeout: The default request timeout for DA client operations has been increased from 30 seconds to 60 seconds, both in the client's internal configuration and the global application configuration.
• Streamlined Timeout Configuration: Redundant timeout constants have been removed, and configuration comments updated to reflect the broader application of the RequestTimeout setting.
• Test Suite Refinement: A previously flaky timeout test in the DA retriever has been removed, as its timing behavior is now implicitly managed by the centralized DA client timeout.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request refactors timeout handling by centralizing it in the da.Client. The default timeout is increased to 60 seconds, and this timeout is now applied to Submit operations in addition to Retrieve operations. The changes are consistent across the configuration, client, and tests. My main feedback is regarding the error handling for the newly introduced timeout in the Submit function. A timeout will currently be treated as a generic error, and this should be corrected. Additionally, a unit test to verify the timeout behavior in Submit is missing.

[gemini-code-assist]

While adding a timeout here is a great improvement, I've noticed two related points:

1. Error Handling: The error handling for context.DeadlineExceeded (which will be returned by c.da.SubmitWithOptions on timeout) seems to be missing. The current error handling in this function does not specifically handle context.DeadlineExceeded. This will cause a timeout to be treated as a generic error (coreda.StatusError) instead of a more specific timeout status like coreda.StatusContextDeadline. Please consider updating the switch statement that handles errors to include a case for context.DeadlineExceeded:

   case errors.Is(err, context.DeadlineExceeded):
       status = coreda.StatusContextDeadline

2. Missing Test: The timeout mechanism for Submit is not tested. While TestClient_Submit has a case for "context deadline error", it only checks the mapping of a pre-canned coreda.ErrContextDeadline and doesn't verify that the context.WithTimeout works as expected. A dedicated test, similar to TestClient_Retrieve_Timeout, should be added to client_test.go to ensure the timeout in Submit is effective.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.52%. Comparing base (4481fea) to head (181e362).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2903      +/-   ##
==========================================
- Coverage   64.67%   64.52%   -0.16%     
==========================================
  Files          81       81              
  Lines        7380     7382       +2     
==========================================
- Hits         4773     4763      -10     
- Misses       2065     2078      +13     
+ Partials      542      541       -1     

Flag	Coverage Δ
combined	64.52% <100.00%> (-0.16%)	⬇️

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.