Skip to content

Terminate message queries that exceed a configurable time limit - #5848

Open
ramonsmits wants to merge 5 commits into
masterfrom
ramon/audit-query-timeout
Open

Terminate message queries that exceed a configurable time limit#5848
ramonsmits wants to merge 5 commits into
masterfrom
ramon/audit-query-timeout

Conversation

@ramonsmits

@ramonsmits ramonsmits commented Sep 3, 2026

Copy link
Copy Markdown
Member

Customer debug packages showed audit message queries against MessagesViewIndexWithFullTextSearch running for up to ~65 minutes. RavenDB's own Databases.QueryTimeoutInSec (default 300) does not stop them because the server renews the operation deadline while a query keeps making progress, so large scans/sorts run for hours and their sorted results spill into the server's Temp folder, filling the disk.

Query time limit

The message view and search queries now run under a linked CancellationTokenSource that cancels after a configurable time limit, default 1 minute, maximum 1 hour. Cancelling the client request aborts the call to the database server, which does terminate the query server-side. Invalid values fall back to the default.

One QueryTimeLimit helper in ServiceControl.Infrastructure serves all persisters. Its rule is that whatever exception surfaces after the deadline fired is the timeout: RavenDB and Npgsql raise OperationCanceledException, Microsoft.Data.SqlClient raises SqlException("Operation cancelled by user"). The SQL Server path is covered by a test against the real container that slows the command down server-side.

The limit applies to exactly these queries:

  • Audit RavenAuditDataStore: GetMessages, QueryMessages, QueryMessagesByReceivingEndpoint, QueryMessagesByReceivingEndpointAndKeyword, QueryMessagesByConversationId, read from ServiceControl.Audit/QueryTimeoutInSeconds. QueryAuditCounts (licensing throughput) and QuerySagaHistoryById are not under the limit.
  • Primary IMessagesViewDataStore on RavenDB, SQL Server and PostgreSQL: GetAllMessages, GetAllMessagesForEndpoint, GetAllMessagesByConversation, GetAllMessagesForSearch, SearchEndpointMessages, read from ServiceControl/QueryTimeoutInSeconds. On SQL Server and PostgreSQL these queries also use the limit as their per-command timeout, so Database/CommandTimeout cannot undercut it.

Other paged index queries (GetFailedMessages, groups, event log, ...) are unchanged.

What a caller sees

  • A query that runs out of its time is answered with 504 Gateway Timeout and a application/problem+json body whose detail names the setting, on both hosts.
  • The primary's scatter-gather keeps partial data: a local timeout is absorbed the way a remote failure already was, a remote 504 or error is a missing instance rather than an instance with no data. The response lists every missing instance in the X-Particular-Incomplete-Results header (instanceId:timeout|unavailable|error, exposed through CORS) and carries no ETag. Only when no queried instance answered and one of them timed out does the composite itself answer 504.
  • The in-process audit counts used for licensing throughput fail when any instance is missing, instead of recording a partial sum as the day's throughput.
  • The remote instance HttpClient timeout is ServiceControl/QueryTimeoutInSeconds plus 30 seconds instead of the 100 second default, so raising the limit on both instances does not make the primary give up on the audit instance first.

ServicePulse follow-up: the audit list already treats a non-2xx answer (the new 504) as a failed query; reading X-Particular-Incomplete-Results on a 200 is still to be added.

Docs: Particular/docs.particular.net#8486

Comment thread src/ServiceControl.Audit.Persistence.RavenDB/RavenAuditDataStore.cs Outdated
Comment thread src/ServiceControl.Persistence.RavenDB/ErrorMessagesDataStore.cs Outdated
Comment thread src/ServiceControl.Persistence.EFCore/Implementation/MessagesViewDataStore.cs Outdated
@ramonsmits
ramonsmits marked this pull request as draft September 4, 2026 14:47
@ramonsmits
ramonsmits force-pushed the ramon/audit-query-timeout branch from b7e02a4 to ec9cf12 Compare September 4, 2026 15:24
@ramonsmits
ramonsmits marked this pull request as ready for review September 7, 2026 08:48
ramonsmits added a commit to Particular/ServicePulse that referenced this pull request Sep 7, 2026
ServiceControl (Particular/ServiceControl#5848) now answers a partial
composite with the X-Particular-Incomplete-Results header naming every
instance whose data is missing (instanceId:timeout|unavailable|error),
and a fully timed-out query with a 504. All Messages now reads both: a
partial page keeps its data but shows a 'Partial results' warning naming
each missing instance and its reason, and a 504 gets a precise 'exceeded
the ServiceControl query time limit' message instead of the generic
failure text. The header parser is shared so the conversation and saga
views can adopt it next.
ramonsmits added a commit to Particular/ServicePulse that referenced this pull request Sep 7, 2026
ServiceControl (Particular/ServiceControl#5848) now answers a partial
composite with the X-Particular-Incomplete-Results header naming every
instance whose data is missing (instanceId:timeout|unavailable|error),
and a fully timed-out query with a 504. All Messages now reads both: a
partial page keeps its data but shows a 'Partial results' warning naming
each missing instance and its reason, and a 504 gets a precise 'exceeded
the ServiceControl query time limit' message instead of the generic
failure text. The header parser is shared so the conversation and saga
views can adopt it next.
ramonsmits added a commit to Particular/ServicePulse that referenced this pull request Sep 7, 2026
ServiceControl (Particular/ServiceControl#5848) now answers a partial
composite with the X-Particular-Incomplete-Results header naming every
instance whose data is missing (instanceId:timeout|unavailable|error),
and a fully timed-out query with a 504. All Messages now reads both: a
partial page keeps its data but shows a 'Partial results' warning naming
each missing instance and its reason, and a 504 gets a precise 'exceeded
the ServiceControl query time limit' message instead of the generic
failure text. The header parser is shared so the conversation and saga
views can adopt it next.
ramonsmits and others added 2 commits September 7, 2026 14:40
Customer debug packages showed audit message queries against
MessagesViewIndexWithFullTextSearch running for up to ~65 minutes.
RavenDB's own Databases.QueryTimeoutInSec (default 300) does not stop
them because the server renews the operation deadline while a query
keeps making progress, so large scans/sorts run for hours and their
sorted results spill into the server's Temp folder, filling the disk.

The message view and search queries (all messages, per endpoint,
search, conversation) now run under a linked CancellationTokenSource
that cancels after a configurable time limit, default 1 minute, maximum
1 hour. Cancelling the client request aborts the call to the database
server, which does terminate the query server-side. On expiry a
TimeoutException names the setting to adjust; invalid values fall back
to the default.

One QueryTimeLimit helper in ServiceControl.Infrastructure serves every
persister, so they share one setting with one behavior, a hard
wall-clock deadline per data store call. Its rule is that whatever
exception surfaces after the deadline fired is the timeout: RavenDB and
Npgsql raise OperationCanceledException, Microsoft.Data.SqlClient raises
SqlException("Operation cancelled by user"), which a plain
OperationCanceledException catch never sees. A test against the real
SQL Server and PostgreSQL containers slows the command down server-side
to cover that.

- ServiceControl.Audit/QueryTimeoutInSeconds bounds the audit
  RavenAuditDataStore message view queries. The licensing audit counts
  and the saga history lookup are not under the limit.
- ServiceControl/QueryTimeoutInSeconds bounds the primary instance
  IMessagesViewDataStore queries on RavenDB, SQL Server and PostgreSQL,
  which use the same unbounded sorted index query shape. On SQL Server
  and PostgreSQL these queries also use the limit as their per-command
  timeout, so Database/CommandTimeout cannot undercut it.

Co-authored-by: Mauro Servienti <mauro.servienti@gmail.com>
…data

A data store's TimeoutException now becomes a 504 Gateway Timeout with a
problem body naming the setting, on both the primary and the audit host,
so a caller can tell a timeout from a crash and from an empty result.

The primary's scatter-gather no longer hides what went wrong. A local
timeout is absorbed the way a remote failure already was, a remote 504
or error is a missing instance rather than an instance with no data, and
the response names every missing instance in the
X-Particular-Incomplete-Results header (instanceId:timeout|unavailable|
error) and carries no ETag. Only when no instance that was asked
answered and one of them timed out does the composite itself fail with
the timeout. The signal is a header rather than a body field because the
composite endpoints return a bare array; an envelope would change the
response schema and need a new API for every existing client.

The in-process audit counts used for licensing throughput fail instead
of recording a partial sum as the day's throughput.

The remote instance HttpClient timeout follows
ServiceControl/QueryTimeoutInSeconds plus a 30 second margin instead of
the 100 second HttpClient default, so raising the limit on both
instances does not make the primary give up on the audit instance first.
ramonsmits added a commit to Particular/ServicePulse that referenced this pull request Sep 7, 2026
ServiceControl (Particular/ServiceControl#5848) now answers a partial
composite with the X-Particular-Incomplete-Results header naming every
instance whose data is missing (instanceId:timeout|unavailable|error),
and a fully timed-out query with a 504. All Messages now reads both: a
partial page keeps its data but shows a 'Partial results' warning naming
each missing instance and its reason, and a 504 gets a precise 'exceeded
the ServiceControl query time limit' message instead of the generic
failure text. The header parser is shared so the conversation and saga
views can adopt it next.
Four remotes, one answering 504 at once and three answering with data two
seconds later: the composite waits for the slow ones, keeps their rows and
lists only the timed-out instance.
@ramonsmits
ramonsmits force-pushed the ramon/audit-query-timeout branch from ec9cf12 to e393787 Compare September 7, 2026 13:40
@mauroservienti

Copy link
Copy Markdown
Member

@ramonsmits, I have not finished reviewing this yet. However, we also have this CustomCheck that probably needs to be adapted to incorporate the query timeouts. Also, we have this concept of an instance being temporarily unavailable if it fails to respond to the mentioned custom check. When that property is true, the ScatterGather API skips the unavailable instance until it comes back.

Comment on lines +34 to +48
public static void WithIncompleteResults(this HttpResponse response, IReadOnlyList<IncompleteInstance> incomplete)
{
if (incomplete.Count == 0)
{
return;
}

response.WithHeader(IncompleteResultsHeader, string.Join(", ", incomplete.Select(instance => $"{instance.InstanceId}:{instance.Reason switch
{
QueryFailure.TimedOut => "timeout",
QueryFailure.Unavailable => "unavailable",
QueryFailure.Failed => "error",
_ => "error"
}}")));
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that the TemporaryUnavailable instances, I mentioned, should be counted here

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are. ScatterGatherApi.Execute adds every skipped remote to the results with QueryFailure.Unavailable, AggregateResults turns those into IncompleteInstances, and this header renders them as instanceId:unavailable. IncompleteResultsTests.A_temporarily_unavailable_remote_is_reported_as_missing pins it.

The remote clients waited the limit plus a 30 second margin so a remote's
own 504 could travel back. That made the error instance's setting a promise
only when every audit instance was configured to match: one left on the
default, or hanging, stretched the composite to limit + 30 s.

The remote client now times out at the limit itself. A remote that has not
answered by then is reported as missing whatever its own configuration; its
own limit still ends the query on its side.
…ount

The probe used the remote's HttpClient, whose timeout is now the query time
limit, so a short limit failed the health check of a remote that was fine.
The probe's own 10 second budget never applied: its cancellation was caught
by a no-op branch, so a hanging remote passed the check and was never
disabled, and a shutdown waited out the full budget.

The probe now runs on its own budget regardless of the query time limit; a
remote that does not answer in time is disabled and fails the check, and a
shutdown aborts the probe without a verdict on the remote.
@ramonsmits

Copy link
Copy Markdown
Member Author

Looked at CheckRemotes against the query timeouts. The check probes /api, which runs no query, so a remote whose queries time out never fails the health check, and that is intended: a 504 means the remote is up, so TemporarilyUnavailable stays false and the next query asks it again (A_remote_query_timeout_keeps_the_local_data_and_reports_the_remote_instance asserts that).

There was a real interaction though, caused by this PR: the probe used the remote's named HttpClient, whose timeout is now the query time limit. With a short limit a healthy but slow remote would fail the health check, and the failure message would still claim 10 seconds. While in there I also fixed a pre-existing problem: the probe's own timeout never applied, because its cancellation was swallowed by a no-op branch, so a hanging remote passed the check and was never disabled, and a shutdown waited out the full budget.

79d19e9 gives the probe its own budget independent of the query limit, makes a remote that doesn't answer in time fail the check and become temporarily unavailable, and lets a shutdown abort the probe without a verdict. CheckRemotesTests covers the four cases.

Since your last pass I also dropped the 30 second margin (22e6109): the remote client now times out at this instance's limit, so ServiceControl/QueryTimeoutInSeconds bounds the whole composite. An audit instance that is slow, hung, or configured with a larger limit is reported as missing rather than stretching the response. The docs PR is updated accordingly.

ramonsmits added a commit to Particular/ServicePulse that referenced this pull request Sep 7, 2026
ServiceControl (Particular/ServiceControl#5848) now answers a partial
composite with the X-Particular-Incomplete-Results header naming every
instance whose data is missing (instanceId:timeout|unavailable|error),
and a fully timed-out query with a 504. All Messages now reads both: a
partial page keeps its data but shows a 'Partial results' warning naming
each missing instance and its reason, and a 504 gets a precise 'exceeded
the ServiceControl query time limit' message instead of the generic
failure text. The header parser is shared so the conversation and saga
views can adopt it next.
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.

2 participants