Add query timeout to prevent indefinite hangs on S3 disconnection

[khalid244]

Problem

When S3 becomes unavailable during query execution, queries hang indefinitely (120+ seconds) waiting for DuckDB's internal HTTP timeout. This causes:

• Poor user experience with unresponsive queries
• Connection pool exhaustion under load
• No clear error feedback to clients

Solution

Add query timeout support that uses the existing server.read_timeout configuration value. When the timeout is exceeded, queries return HTTP 504 with a clear "Query timed out" error message.

Changes

internal/database/duckdb.go

• Added QueryContext method that accepts a context for timeout/cancellation support

internal/api/query.go

• Added queryTimeout field to QueryHandler struct
• Updated NewQueryHandler to accept timeout parameter
• Modified query execution to use QueryContext with timeout
• Added timeout detection that returns HTTP 504 Gateway Timeout

cmd/arc/main.go

• Pass cfg.Server.ReadTimeout to query handler

Configuration

Query timeout is controlled by the existing server config:

[server]
read_timeout = "30s"  # Also used as query timeout

Behavior

Scenario	Before	After
S3 unavailable	Hangs 120+ seconds	Returns 504 after read_timeout
Error message	"Query execution failed"	"Query timed out"
HTTP status	500	504

Limitations

The context timeout detects when the deadline has passed but cannot forcibly cancel DuckDB's internal HTTP operations. The query will still wait for DuckDB's internal timeout, but clients receive the correct error type and status code.

Testing

1. Deploy with read_timeout = "30s"
2. Block S3 egress (NetworkPolicy or firewall)
3. Execute query - should return HTTP 504 "Query timed out"

[xe-nvdk]

Thanks for the PR! You've identified a real issue - queries can hang when S3 is unavailable. However, there are several concerns with this implementation:

1. Config Confusion

read_timeout is Fiber's HTTP read timeout - it controls how long the server waits to read the request body from the client. It does NOT control query execution time.

These are different concerns:

• read_timeout = HTTP layer (request reading)
• query_timeout = Application layer (query execution)

A query can run for 2 minutes even with read_timeout=30s because the request was already fully received. Reusing this config is misleading.

Suggestion: Add a dedicated query_timeout config instead of reusing read_timeout.

2. Incomplete Coverage

The timeout is only applied to the non-profiled query path:

if profileMode {
    rows, profile, err = h.db.QueryWithProfile(convertedSQL)  // ❌ No timeout
} else {
    rows, err = h.db.QueryContext(ctx, convertedSQL)  // ✅ Has timeout
}

Profiled queries will still hang indefinitely.

3. Doesn't Actually Cancel DuckDB

As you noted in "Limitations", the context timeout doesn't stop DuckDB's internal HTTP operations. So:

• Client gets 504 after 30s ✅
• But the DuckDB query keeps running in the background until its 120s timeout ❌
• This wastes server resources and could cause connection pool issues

4. We Already Have Query Timeout

DuckDB itself has timeout settings that actually cancel queries:

SET http_timeout=30000;  -- 30 seconds

This is already configurable and actually stops the query.

Recommendation

If you want to improve timeout handling:

1. Add a dedicated [server] query_timeout config (don't reuse read_timeout)
2. Apply timeout to ALL query paths (including profiled queries)
3. Consider using DuckDB's native http_timeout setting which actually cancels operations

I'd suggest closing this PR and opening an issue to discuss the best approach for query timeouts.

[xe-nvdk]

Correction to my previous review:

I was wrong about us already having query timeout - we don't. The ReadTimeout is only for HTTP request reading, not query execution. So this PR is adding something genuinely new and useful.

Updated Review

The core idea is valid - we need query execution timeouts. However, I still have concerns:

1. Config Naming

Reusing read_timeout for query timeout is confusing since they're different concepts. Consider adding a separate query_timeout config, or at minimum document clearly that read_timeout is being reused for both purposes.

2. Incomplete Coverage

Profiled queries still don't get the timeout:

if profileMode {
    rows, profile, err = h.db.QueryWithProfile(convertedSQL)  // ❌ No timeout
} else {
    rows, err = h.db.QueryContext(ctx, convertedSQL)  // ✅ Has timeout
}

3. Context Cancellation Limitation

As you noted, the context doesn't actually cancel DuckDB's HTTP operations - it just detects the timeout. This is a known Go/CGO limitation. Worth documenting but not a blocker.

---

If you address #1 (separate config or clear docs) and #2 (cover profiled queries), I'd be happy to approve.

[xe-nvdk]

Python Implementation Reference

I checked the Python legacy implementation (python-legacy branch) and found that query timeout does exist there:

File: api/duckdb_pool.py

async def execute_async(
    self,
    sql: str,
    priority: QueryPriority = QueryPriority.NORMAL,
    timeout: float = 300.0  # 5 minute default
) -> Dict[str, Any]:

Key differences from your PR:

Aspect	Python	Your PR
Default timeout	300s (5 minutes)	30s (via read_timeout)
Implementation	Connection pool level	Query handler level
Metrics	total_queries_timeout counter	None
Per-query control	Yes (timeout param)	No (global only)

Recommendations based on Python implementation:

1. Use 300s default (or make it configurable) - 30s may be too aggressive for complex queries
2. Add a dedicated config like query_timeout instead of reusing read_timeout
3. Add timeout metrics - Python tracks total_queries_timeout which is useful for monitoring
4. Consider per-query timeout - Python allows different timeouts per query based on priority

This PR is essentially porting a missing feature from Python to Go, which is valuable. The implementation just needs some refinement to match what we had before.

[khalid244]

Thanks for the detailed review! I've addressed all your feedback:

1. Dedicated query.timeout config ✅

• Added new query.timeout config (default: 300 seconds / 5 minutes)
• No longer reusing read_timeout - these are now separate concerns
• Configurable via ARC_QUERY_TIMEOUT environment variable

2. Timeout applies to ALL query paths ✅

• Added QueryWithProfileContext() method in duckdb.go
• Both profiled and non-profiled queries now use the context with timeout:

if profileMode {
    rows, profile, err = h.db.QueryWithProfileContext(ctx, convertedSQL)  // ✅ Now has timeout
} else {
    rows, err = h.db.QueryContext(ctx, convertedSQL)  // ✅ Has timeout
}

3. Timeout metrics ✅

• Added query_timeouts_total counter for monitoring
• Available in JSON API (/api/v1/metrics) and Prometheus (/metrics):

# HELP arc_query_timeouts_total Queries that exceeded timeout
# TYPE arc_query_timeouts_total counter
arc_query_timeouts_total 1

Files changed:

• internal/config/config.go - Added query.timeout config
• internal/database/duckdb.go - Added QueryWithProfileContext()
• internal/api/query.go - Use new config, apply timeout to profiled queries, increment timeout metric
• internal/metrics/metrics.go - Added query_timeouts_total metric
• cmd/arc/main.go - Use cfg.Query.Timeout instead of cfg.Server.ReadTimeout

[xe-nvdk]

Review: Query Timeout - Almost There!

Great job addressing the previous feedback! The implementation is clean and well-documented. Just one more thing needed:

Missing: Timeout Support for Arrow and Estimate Endpoints

The timeout is only applied to executeQuery. The other two query endpoints also need timeout support:

1. Arrow Endpoint (query_arrow.go)

func (h *QueryHandler) executeQueryArrow(c *fiber.Ctx) error {
    // ... needs timeout context like executeQuery
    rows, err := h.db.Query(convertedSQL)  // <- No timeout!
}

2. Estimate Endpoint (query.go:estimateQuery)

func (h *QueryHandler) estimateQuery(c *fiber.Ctx) error {
    // ... needs timeout context
    rows, err := h.db.Query(estimateSQL)  // <- No timeout!
}

Suggested Changes

Apply the same pattern from executeQuery to both endpoints:

ctx := c.UserContext()
var cancel context.CancelFunc
if h.queryTimeout > 0 {
    ctx, cancel = context.WithTimeout(ctx, h.queryTimeout)
    defer cancel()
}

rows, err := h.db.QueryContext(ctx, sql)

// Check for timeout
if h.queryTimeout > 0 && ctx.Err() == context.DeadlineExceeded {
    m.IncQueryTimeouts()
    return c.Status(fiber.StatusGatewayTimeout).JSON(...)
}

Why This Matters

• Arrow queries can also hang indefinitely on S3 issues
• Estimate queries run EXPLAIN ANALYZE which executes the query
• Consistent behavior across all query APIs

Once these are added, this PR is ready to merge!

[khalid244]

Addressed: Timeout Support for Arrow and Estimate Endpoints ✅

Added timeout support to both endpoints as requested:

1. Arrow Endpoint (query_arrow.go)

ctx := c.UserContext()
var cancel context.CancelFunc
if h.queryTimeout > 0 {
    ctx, cancel = context.WithTimeout(ctx, h.queryTimeout)
    defer cancel()
}

rows, err := h.db.QueryContext(ctx, convertedSQL)

if h.queryTimeout > 0 && ctx.Err() == context.DeadlineExceeded {
    m.IncQueryTimeouts()
    return c.Status(fiber.StatusGatewayTimeout).JSON(...)
}

2. Estimate Endpoint (query.go:estimateQuery)

• Same pattern applied to the COUNT(*) query execution

Files changed:

• internal/api/query_arrow.go - Added context/metrics imports, timeout support
• internal/api/query.go - Added timeout support to estimateQuery

All three query APIs now have consistent timeout behavior and increment the query_timeouts_total metric.

[xe-nvdk]

@khalid244 Thanks for the updates! The main query endpoint looks great now with:

• Dedicated query.timeout config (default 300s)
• QueryWithProfileContext for profiled queries
• query_timeouts_total metric
• 504 Gateway Timeout response

However, I noticed query_arrow.go is not in the diff. Your comment mentioned adding timeout support to Arrow and Estimate endpoints, but those changes aren't in the PR yet. Can you verify and push those commits?

Missing from diff:

• internal/api/query_arrow.go - Arrow endpoint timeout
• estimateQuery function timeout in query.go

Once those are added, this PR is ready to merge!

[xe-nvdk]

All feedback addressed. Implementation is complete:

• Dedicated query.timeout config (default 300s)
• QueryWithProfileContext for profiled queries
• Arrow endpoint timeout support
• Estimate endpoint timeout support
• query_timeouts_total metric
• 504 Gateway Timeout responses

Ready to merge.