Skip to content

feat: adjust REST timeouts#1557

Merged
joanestebanr merged 3 commits intodevelopfrom
feat/adjust_timeout_1540
Mar 27, 2026
Merged

feat: adjust REST timeouts#1557
joanestebanr merged 3 commits intodevelopfrom
feat/adjust_timeout_1540

Conversation

@joanestebanr
Copy link
Copy Markdown
Collaborator

@joanestebanr joanestebanr commented Mar 27, 2026

🔄 Changes Summary

  • Increase defaultDBQueryTimeout from 60s to 5m
  • Increase REST server ReadTimeout and WriteTimeout from 2s to 5m

Motivation

The previous timeout values were too short and caused failures for slow queries and long-running REST requests (e.g. aggchain proof generation can take up to 1h).

✅ Testing

  • Unittest

🐞 Issues

🤖 Generated with Claude Code

@claude
Copy link
Copy Markdown

claude bot commented Mar 27, 2026

Claude finished @joanestebanr's task in 1m 53s —— View job


Code Review Complete ✓


🚀 What's New

  • Increased defaultDBQueryTimeout from 60s to 5m for database operations in bridge and claim sync components
  • Increased REST server ReadTimeout and WriteTimeout from 2s to 5m to accommodate long-running API requests

🐛 Bug Fixes

None - this is a configuration adjustment to address timeout issues with slow operations.

📋 Config Updates

  • defaultDBQueryTimeout: 60s5m (affects BridgeL1Sync.DBQueryTimeout, BridgeL2Sync.DBQueryTimeout, ClaimL1Sync.DBQueryTimeout, ClaimL2Sync.DBQueryTimeout)
  • REST.ReadTimeout: 2s5m
  • REST.WriteTimeout: 2s5m

⚠️ Breaking Changes

None - this is a backward-compatible configuration change that only extends timeout limits.


🔍 Review Feedback

🟡 Suggested: Timeout Value Alignment

Issue: The motivation states that aggchain proof generation can take up to 1 hour, but the REST timeouts are only increased to 5 minutes. This creates a potential mismatch.

Details:

  • Line 43 in config/default.go sets GenerateAggchainProofTimeout = "1h"
  • Lines 150-151 set REST ReadTimeout and WriteTimeout to 5m
  • If a REST endpoint triggers aggchain proof generation (which can take up to 1h), a 5m timeout will still cause failures

Recommendation:
Consider one of these approaches:

  1. Increase REST timeouts to match the 1h aggchain proof timeout: ReadTimeout = "1h" and WriteTimeout = "1h"
  2. If the REST endpoints don't directly wait for aggchain proof generation (async pattern), document this and ensure the REST API returns immediately
  3. Add different timeout configurations for different endpoint types (standard operations vs. long-running operations)

Reference: config/default.go:150-151 and config/default.go:43

🟢 Optional: RPC vs REST Timeout Consistency

Issue: RPC server timeouts remain at 2s while REST timeouts are increased to 5m. This creates an inconsistency that might be intentional but should be documented.

Details:

  • Lines 143-144: RPC ReadTimeout and WriteTimeout remain at 2s
  • Lines 150-151: REST ReadTimeout and WriteTimeout increased to 5m

Recommendation:
If the RPC endpoints have different usage patterns and don't need longer timeouts, consider adding a comment explaining why RPC and REST have different timeout values. This will help future maintainers understand the design decision.

Reference: config/default.go:143-144 vs config/default.go:150-151

✅ Good Changes

  1. Database Query Timeout: The increase from 60s to 5m for defaultDBQueryTimeout is reasonable for slow database queries, especially during syncing operations with large datasets
  2. Cascading Configuration: The use of template variables ensures the timeout is consistently applied across all bridge and claim sync components (BridgeL1Sync, BridgeL2Sync, ClaimL1Sync, ClaimL2Sync)
  3. Backward Compatible: Existing deployments will benefit from longer timeouts without requiring code changes

📝 Additional Notes

  • The E2E test configuration in test/e2e/envs/op-pp/config/001/aggkit-config.toml:473 already uses DBQueryTimeout = "10m", which is longer than the new default of 5m. This suggests that some deployments may need even longer timeouts.
  • Test files use shorter timeouts (5s, 30s) which is appropriate for test environments
  • The config.toml.example file should be updated to reflect the new default value (currently shows 60s on line 86)

🎯 Summary

The changes are generally sound for addressing timeout issues with slow database queries. However, there's a potential disconnect between the 1-hour aggchain proof generation timeout and the 5-minute REST timeout that should be addressed. Consider whether the REST endpoints that trigger long-running operations need their timeouts aligned with the actual operation duration.

Priority: 🟡 Suggested - Consider aligning REST timeouts with the longest possible operation duration (1h for aggchain proof generation)


@joanestebanr joanestebanr self-assigned this Mar 27, 2026
@joanestebanr joanestebanr changed the title fix: adjust REST timeouts feat: adjust REST timeouts Mar 27, 2026
@joanestebanr joanestebanr enabled auto-merge (squash) March 27, 2026 14:10
@sonarqubecloud
Copy link
Copy Markdown

@joanestebanr joanestebanr merged commit 49069fd into develop Mar 27, 2026
24 of 25 checks passed
@joanestebanr joanestebanr deleted the feat/adjust_timeout_1540 branch March 27, 2026 15:06
joanestebanr added a commit that referenced this pull request Mar 27, 2026
## 🔄 Changes Summary

Cherry-pick of #1503 and #1557 into \`release/0.8\`

### #1503 - Treat already-monitored GER tx as expected
- Treats \`ErrAlreadyExists\` from \`ethtxmanager\` as expected behavior
(not an error) in \`submitTransaction\`
- Updates \`zkevm-ethtx-manager\` from v0.2.17 to v0.2.18

### #1557 - Adjust REST timeouts
- Increases \`defaultDBQueryTimeout\` from \`60s\` to \`5m\`
- Increases REST server \`ReadTimeout\` and \`WriteTimeout\` from \`2s\`
to \`5m\`
- Motivation: previous values were too short for slow queries and
long-running REST requests (e.g. aggchain proof generation can take up
to 1h)

## ⚠️ Breaking Changes
None

## 📋 Config Updates
- \`defaultDBQueryTimeout\`: \`60s\` → \`5m\`
- \`REST.ReadTimeout\` / \`REST.WriteTimeout\`: \`2s\` → \`5m\`

## ✅ Testing
- 🤖 **Automatic**: Unittest

## 🐞 Issues
- Related: #1503, #1490, #1557, #1540

## 🔗 Related PRs
- #1503 (cherry-pick against \`develop\`)
- #1557 (cherry-pick against \`develop\`)

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: adjust default timeout for aggkit

2 participants