Ensure correct query timeout value is used.

[brandon-pereira]

For example, it defaults to 60 seconds, but a timeout gets created for 60 milliseconds.

Before

After

Original PR:
#1072

Bug Introduced here:
#1132

Closes #1416
Fixes HDX-2931

[changeset-bot]

🦋 Changeset detected

Latest commit: e0f6446

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@hyperdx/app	Patch
@hyperdx/api	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
hyperdx-v2-oss-app	Ready	Preview	Comment	Dec 2, 2025 8:12pm

[github-actions]

E2E Test Results

✅ All tests passed • 46 passed • 3 skipped • 262s

Status	Count
✅ Passed	46
❌ Failed	0
⚠️ Flaky	0
⏭️ Skipped	3

View full report →

[claude]

PR Review

✅ No critical issues found.

The fix correctly addresses the timeout unit mismatch:

• queryTimeout is stored in seconds (e.g., DEFAULT_QUERY_TIMEOUT = 60)
• setTimeout() expects milliseconds
• The fix multiplies by 1000: setTimeout(() => abortController.abort(), queryTimeout * 1000)
• Test updated to reflect that queryTimeout: 30 means 30 seconds (not milliseconds)
• The getClickhouseClient({ queryTimeout }) call properly passes the timeout to set ClickHouse's max_execution_time setting (line 493 in index.ts)

The TODO comment about queryTimeout not being honored is accurate - this PR fixes the client-side abort timeout, but the comment suggests there may be additional investigation needed for server-side timeout enforcement.

[MikeShi42]

Unless I'm misunderstanding something, I actually think the original PR comment was a mistake - I was tracing the code and queryTimeout is indeed respected by the query generation, and we shouldn't need an additional abort controller (though maybe we want one anyways just in case?)

I checked locally and it seems like the setting is being set as well - are you observing the same?

hyperdx/packages/common-utils/src/clickhouse/index.ts

Line 493 in 337be9a

clickhouse_settings.max_execution_time = this.queryTimeout;

[brandon-pereira]

Unless I'm misunderstanding something, I actually think the original PR comment was a mistake - I was tracing the code and queryTimeout is indeed respected by the query generation, and we shouldn't need an additional abort controller (though maybe we want one anyways just in case?)

I checked locally and it seems like the setting is being set as well - are you observing the same?

https://github.com/hyperdxio/hyperdx/blob/337be9a2d1f5870b99
f05fc2086d11eb9d28014e/packages/common-utils/src/clickhouse/index.ts#L493

Actually, I think you're correct! I didn't see that specific comment, just the PR.. I assumed there was another issue. I also verified the header is being sent in the request and see it passing through in the headers.

Let's double-check with @teeohhem before we merge, in case there is another case we should consider.

[teeohhem]

Honestly it's on me for not leaving a comment there. I remember the tests failing and digging through the code to figure out that the abort wasn't ever happening. It could've been a lib version thing or it also could've been ignorance on my part.

If we move forward with this, we'll just want to make sure that the timeout actually aborts when it's supposed to.

[brandon-pereira]

Honestly it's on me for not leaving a comment there. I remember the tests failing and digging through the code to figure out that the abort wasn't ever happening. It could've been a lib version thing or it also could've been ignorance on my part.

If we move forward with this, we'll just want to make sure that the timeout actually aborts when it's supposed to.

I double checked the headers, and I see it being sent.. but the query doesn't seem to honour this setting for some reason. I went down a rabbit hole trying to investigate this, but I think for the sake of time I fixed the issue with the original solution, and I'll open an issue to investigate the core reason why the setting is ignored.

Ex below: header set to 1s, but 30s for a response

[brandon-pereira]

I added this here, doesn't hurt to pass it