Repository navigation
The analysis timeout cancels nothing, and a wedged analysis skips that server forever #2412
Description
Split out of #2393, where the question was whether to expose analysis_timeout_seconds in Lite's Settings window. The answer is "not as it stands", because the knob's name promises something the code does not do — and looking at why turned up a worse problem.
The timeout does not bound the analysis
CollectionBackgroundService:
var analyzeTask = analysisService.AnalyzeAsync(serverId, serverName, hoursBack: 4);
var finished = await Task.WhenAny(analyzeTask, Task.Delay(timeout, stoppingToken));
if (finished != analyzeTask)
{
_logger?.LogWarning("Scheduled analysis for {Server} exceeded {Timeout}s — skipped this cycle", ...);
continue;
}Nothing is cancelled. AnalyzeAsync gets no CancellationToken and keeps running; it persists its findings through SaveFindingsAsync regardless. What the timeout actually bounds is how long the scheduling loop waits before moving to the next server, and the consequence of exceeding it is that this cycle's findings are not routed to the notification channels.
A setting labelled "Analysis timeout" invites exactly the wrong mental model — "give analysis more time to finish" — when raising it only makes the loop wait longer for a result it is going to persist either way. That is why it should not simply be surfaced as-is.
The part that is a real defect
The in-flight guard is deliberately released only on true completion:
if (!_analysisInFlight.TryAdd(serverId, 0)) { continue; }
...
/* Clear the in-flight marker only when the task truly finishes — not
when the timeout below moves us on — so a hung server is not relaunched. */
_ = analyzeTask.ContinueWith(completed => _analysisInFlight.TryRemove(serverId, out _), TaskScheduler.Default);That is correct as far as it goes — it stops a hung server piling up tasks. But combined with the absence of cancellation it means an AnalyzeAsync that never completes (a wedged connection, a query with no timeout) leaves that server's marker set forever. The server is then skipped by continue on every subsequent cycle, silently, and the "exceeded Ns" warning is logged only on the cycle that timed out — not on the thousands of skipped cycles after it.
So the failure mode is: one server's analysis wedges, and that server is never analysed again for the life of the process, with one log line to show for it.
Options
- Pass a
CancellationTokenintoAnalyzeAsyncand cancel it at the timeout. Then the knob means what its name says, the marker clears, and exposing it in Settings becomes reasonable. Needs a look at whetherAnalyzeAsync's SQL reads honour cancellation. - Leave the semantics and fix the observability — re-log (throttled) while a server remains in-flight past some multiple of the timeout, so a permanently wedged server is visible rather than silent.
- Rename the knob to describe what it does (a per-server wait budget) and document it, if 1 is not affordable.
1 is the honest fix. Whether the knob then gets a Settings control is a smaller question that follows from it. Darling hardcodes 120 with a comment saying it is deliberately not a control-plane knob, so parity does not force the issue either way.
Activity
erikdarlingdata commentedon Aug 21, 2026
Fixed on
devby #2419, and the investigation found the defect was worse than this issue described.The timeout could not fire at all. DuckDB.NET has no async execution, so
AnalyzeAsyncran its whole fact-collection phase inline and returned an already-completed task —Task.WhenAnywas racing something that had already won. Only the async tail could ever lose that race, which means a wedged store read took the entire collection loop down rather than skipping one server. SoTask.Runhad to come first before a cancellation token could mean anything.Cancellation is genuinely honoured, and that was measured rather than assumed.
DuckDBCommandoverrides only the synchronousExecuteDbDataReader, butDbCommand.ExecuteDbDataReaderAsyncregisters the token to callCancel(), andDuckDBCommand.Cancel()callsduckdb_interrupt. Against a query with a 15,233 ms uncancelled baseline, a token cancelled at 2 s aborted at 2,004 ms. That is the check I asked for — a token nothing observes would have looked like a fix without being one.What shipped is checkpoints ahead of all five store-touching stages, which is abandonment between reads rather than during one; threading the token into the 138 individual DuckDB call sites is a separate change. The PR says so plainly rather than implying full coverage.
One residue no token can reach, and it is why the observability half is load-bearing rather than a fallback:
DuckDbInitializer.AcquireReadLock()callsReaderWriterLockSlim.EnterReadLock(), which takes no token, on a process-wide lock. A pass waiting there behind a long archival is uninterruptible, so the permanent-skip symptom is closed by the reporting rather than by cancellation.All three review findings were real and worth recording, since the first is a trap specific to this shape: a
when (token.IsCancellationRequested)filter would have relabelled any fault as abandonment — worse here than on Darling, because this token fires on timeout, so it is signalled during normal running. Narrowed toOperationCanceledException.Split out for follow-up: Darling has the identical defect (#2430), and the two smaller residues — threading the token into the DuckDB reads, and
AcquireReadLockhaving no timeout unlike itsAcquireWriteLocksibling — are noted there.Exposing
analysis_timeout_secondsin Settings is now reasonable, since the name is honest for everything the token reaches. Deliberately not done here; it was the thing this issue existed to make possible rather than to do.Review caught a Lite behaviour gap I had described as signature-only
Make the analysis timeout mean something, and stop a wedged server go…
Make Darling's analysis budget mean something, and stop a wedged serv…
Let a read lock be given up on, so the budget reaches the door as wel…