Skip to content

fix(dashboards): bound TF queries during page render, degrade to client fetch - #475

Merged
tonyalaribe merged 2 commits into
masterfrom
fix/dashboard-render-budget
Aug 2, 2026
Merged

fix(dashboards): bound TF queries during page render, degrade to client fetch#475
tonyalaribe merged 2 commits into
masterfrom
fix/dashboard-render-budget

Conversation

@tonyalaribe

Copy link
Copy Markdown
Contributor

Why

During the 2026-08-02 outage all three monoscope replicas crash-looped (exit 137 OOM + exit 1 healthcheck kills) and app.monoscope.tech served nothing for ~70 minutes.

The trigger was TimeFusion self-starving (its tokio runtime was CPU-saturated, so object-store futures went unpolled and it logged 379s "S3 timeouts" while R2 was actually 3ms away from the host). But monoscope died rather than degraded, and that part is ours:

Dashboard page render ran TF queries inline, unbounded. processConstant, processVariable and processEagerWidget all had to finish before the shell shipped, and there was no timeout anywhere on the TF path — mkHasqlPool sets only a 30s acquisition timeout, and chart queries don't even use that pool (they go through the postgresql-simple timefusionPgPool). Render threads parked forever, heap climbed into the 24GB cap, Swarm killed the containers, repeat.

log_explorer stayed up throughout, because it already splits chrome (apiLogH) from data (logExplorerDataH). Dashboards had no such split.

What

Wrap each render-time query in a 4s budget (withRenderBudget) and fall back to the un-prefilled value — exactly the state the client already knows how to recover from:

widget fallback behaviour
tables renderTableShell always emits hx-trigger="load, …" + spinner when html is absent
stats renderStatContent includes load whenever hasData is False
charts chartWidget shows "Loading chart…" and fetches on intersection when opt.dataset.source is empty

So a slow TF degrades dashboards from server-rendered to client-fetched, instead of from working to down. Healthy TF is unaffected — these queries return in milliseconds and the timeout never fires.

The subtle bit

lazyWidget must clear eager as well as html/dataset. renderStatContent computes hasData = isTrue widget.eager || …, so a widget left flagged-but-empty renders a spinner that nothing ever resolves. WidgetLazySpec pins both directions, including a test that fails if the flag is left set.

WTAnomalies is excluded deliberately: it reads Postgres (not TF) and widget_ renders whenJust w.html toHtmlRaw, i.e. nothing at all without html, so degrading it would blank the card.

Also: HEALTHCHECK /status → /ping

statusH runs select version(), which made every replica's liveness depend on one shared Postgres — a single DB hiccup fails all three probes simultaneously and Swarm kills the entire service. A probe whose failure action is "kill the container" must test liveness only. /ping is a pure handler and still proves the warp accept loop and handler threads are alive, which is what this probe exists to catch. Dependency health already alerts via InfraHealthCheck.

Testing

  • test/unit/Pkg/WidgetLazySpec.hs — 3 new cases, all pass
  • Full unit suite: 239 examples, 0 failures
  • fourmolu clean

Note

The bound is per query and render has three sequential phases, so a total TF stall bounds the shell at ~12s rather than ~4s. That is documented at renderQueryBudgetMicros. Still a bounded page instead of an unbounded one; happy to tighten if you'd prefer.

…nt fetch

Dashboard page render ran TimeFusion queries inline with no timeout anywhere on
the path: processConstant, processVariable and processEagerWidget all had to
finish before the shell shipped, and mkHasqlPool sets only a 30s *acquisition*
timeout (chart queries don't even use that pool — they go through the
postgresql-simple timefusionPgPool).

So when TF wedged on 2026-08-02, every render thread parked on it, the three
replicas grew into their 24GB cap, and Swarm OOM-killed them in a loop while
/status timed out. log_explorer stayed up throughout because it already splits
chrome (apiLogH) from data (logExplorerDataH); dashboards had no such split.

Wrap each render-time query in a 4s budget and fall back to the un-prefilled
value. That is exactly the state the client already recovers from: table shells
always emit hx-trigger="load" and a spinner when html is absent, stat content
includes `load` whenever it has no data, and chartWidget shows "Loading chart…"
and fetches on intersection when dataset.source is empty. A slow TF now degrades
dashboards from server-rendered to client-fetched instead of from working to down.

lazyWidget must clear `eager` as well as html/dataset: renderStatContent reads
the flag itself as "data is present" and drops `load` from the trigger, so a
widget left flagged-but-empty renders a spinner nothing ever resolves.
WidgetLazySpec pins both directions. WTAnomalies is excluded deliberately — it
reads Postgres, not TF, and widget_ renders nothing at all without html.

Also move the container HEALTHCHECK from /status to /ping. statusH runs
`select version()`, which made every replica's liveness depend on one shared
Postgres, so a single DB hiccup failed all three probes at once and Swarm killed
the whole service. A probe whose failure action is "kill the container" must
test liveness only; dependency health already alerts via InfraHealthCheck.
@claude

claude Bot commented Aug 2, 2026

Copy link
Copy Markdown

Review

Solid, well-documented incident fix — the root-cause writeup and the budget/fallback design are easy to follow, and the WidgetLazySpec test correctly pins the "flagged-but-empty" trap the PR calls out. A few notes:

Possible bug: eager WTAnomalies widgets bypass the deliberate exclusion

In processWidget (src/Pages/Dashboards.hs:828-836):

widget' <-
  if widget.eager == Just True
    then withRenderBudget (...) (lazyWidget widget) $ processEagerWidget pid now timeRange allParams widget
    else
      if widget.wType == Widget.WTAnomalies
        then processEagerWidget pid now timeRange allParams widget
        else pure widget

The comment above this states anomalies widgets "keep their (unbounded) eager render" because they read Postgres, not TF, and widget_ renders nothing at all without html. That's only true when eager /= Just True — the eager == Just True branch is checked first, so a WTAnomalies widget that also has eager: true set gets budgeted and, on timeout, lazyWidget'd — which blanks the card rather than degrading gracefully, contradicting the stated intent. None of the bundled static/public/dashboards/*.yaml templates currently set eager: true on an anomalies widget, but eager is a free Maybe Bool on Widget with no type-level coupling to wType, so nothing stops a user-authored/custom dashboard from doing so via the JSON/YAML config. Worth special-casing WTAnomalies ahead of the eager check (or &&-ing it out of the budgeted branch) so the exclusion actually holds for all inputs, not just the ones the built-in templates happen to produce.

Minor: nested if/else could use MultiWayIf

MultiWayIf is already a default extension (package.yaml:97). The nested if in processWidget above would read more clearly (and match the guard-style used elsewhere in this file) as:

widget' <-
  if
    | widget.eager == Just True ->
        withRenderBudget ("widget:" <> maybeToMonoid widget.title) (lazyWidget widget)
          $ processEagerWidget pid now timeRange allParams widget
    | widget.wType == Widget.WTAnomalies -> processEagerWidget pid now timeRange allParams widget
    | otherwise -> pure widget

Minor: render-budget log label collapses for untitled widgets

withRenderBudget ("widget:" <> maybeToMonoid widget.title) ... (Dashboards.hs:830) — every widget without a title logs the identical label "widget:" on timeout, so concurrent budget-blown warnings for different untitled widgets are indistinguishable. widget.id (used elsewhere in this file, e.g. widgetId, as the more reliable identifier) or widget.id <> title would disambiguate better.

Things that check out

  • UnliftIO.timeout inside ATAuthCtx mirrors existing precedent (System/Tracing.hs's forkBackground, Web/MCP.hs), so this isn't a novel risk in the codebase.
  • The "abandoning the query is safe" claim holds: Data.Pool.withResource (used by Charts.queryMetrics) destroys the connection under onException/mask, and LogQueries.executeSecuredQuery's try @Hasql.HasqlException/Charts's catch \(e :: SomePostgreSqlException) are both typed narrowly enough that they won't swallow the timeout's async exception.
  • renderTableShell/renderStatContent/the chart JS fallback are all correctly unconditional on html/dataset presence (not on eager), so lazyWidget degrading tables/traces/stats/charts is consistent with what those renderers actually check.
  • /ping is a genuinely dependency-free pure "pong" handler (Web/Routes.hs:1001-1002), so the healthcheck decoupling from Postgres is real, not just asserted.
  • Sequencing claim (constants → variables → widgets, each internally pooledForConcurrently) matches dashboardTabGetH and friends, so the "~3x budget" bound in the renderQueryBudgetMicros comment is accurate.

Nothing here blocks merging — the main item (eager anomalies) is a narrow edge case with no current template triggering it, but worth a one-line guard given the PR explicitly designed around excluding anomalies from degradation.

Review catch: the budget branch tested `eager` first, so a WTAnomalies widget
that also set `eager: true` got budgeted and lazyWidget'd on timeout — blanking
the card, which is exactly what the exclusion existed to prevent. `eager` is a
free Maybe Bool with no type-level tie to wType, so config can set both.

Reorder as a MultiWayIf with anomalies first, and label the budget warning by
widget id rather than title (untitled widgets all logged the same string).
@claude

claude Bot commented Aug 2, 2026

Copy link
Copy Markdown

Solid fix, well-documented, and the design (bound the render-time prefill, fall back to the client-fetch shell every widget kind already supports) is the right shape for this failure mode — no page can be held hostage by TF again. A few notes, roughly in priority order.

1. monoscope.cabal looks hand-edited, not regenerated via hpack
The new test-suite deps break alphabetical order in the generated file:

, hasql
, lens
, lucid
, hasql-pool

hasql-pool should sort before lens/lucid (it's a prefix-extension of hasql). Every other entry in that build-depends list is alphabetical, so this really looks like the .cabal file was patched by hand to match the package.yaml diff rather than produced by running hpack. Worth re-running hpack (or whatever the repo's generate step is) so the two files stay in sync — otherwise the next hpack run produces unrelated-looking churn on this list.

2. withRenderBudget duplicates forkBackground's timeout pattern
System/Tracing.hs's runWithTimeout (used by forkBackground) and the new withRenderBudget in Dashboards.hs are the same shape: UnliftIO.timeout n action >>= maybe (log a warning) pure. Given the codebase's succinctness leanings, this reads like a good candidate for a single shared helper, e.g. timeoutOr :: (IOE :> es, Log :> es) => Int -> Text -> a -> Eff es a -> Eff es a that does UnliftIO.timeout micros action >>= maybe (Log.logWarn "exceeded budget" label $> fallback) pure. forkBackground's runWithTimeout could then be void . timeoutOr backgroundTaskTimeoutMicros label (), and withRenderBudget becomes a one-line specialization. Not blocking, but it's a good candidate to pull out before a third caller shows up.

3. The core risk here — does timeout actually abort a wedged connection — isn't exercised by a test
WidgetLazySpec is a good pure-logic test (it correctly pins the eager-flag trap called out in the PR description), but it only tests lazyWidget and the renderer's reaction to it — not withRenderBudget/UnliftIO.timeout itself, which is the actual novel code introduced to fix the outage. GHC only delivers async exceptions promptly to a thread blocked in an interruptible operation; a thread stuck in a non-interruptible FFI call won't be preempted until the call returns on its own. This should be fine here since both hasql and postgresql-simple drive postgresql-libpq in its non-blocking/async mode (poll + threadWaitRead, which is interruptible), but since this is precisely the scenario from the 2026-08-02 outage, it's worth an explicit test/chaos check (e.g. hold a connection open and threadDelay past budget) confirming the pool actually reclaims the connection within budget rather than merely discarding a result that arrives later. The "resource-pool destroys a connection whose borrower died" comment is correct for resource-pool-backed pools (used by Charts.queryMetrics/timefusionPgPool) — worth confirming hasql-pool's cleanup-on-exception behaves the same for the executeSecuredQuery path used by constants/variables.

Nits: processWidget's WTAnomalies-before-eager guard ordering (with the comment explaining why) is a nice use of MultiWayIf to keep an invariant inline. lazyWidget's doc correctly flags that eager must be cleared too or renderStatContent renders a spinner nothing resolves — good that this got a regression test. Also: renderQueryBudgetMicros = 4_000_000 across three sequential render phases means worst-case shell latency is closer to ~12s than 4s (already flagged in the PR body) — just noting 12s is the number to assert against if this ever lands in a benchmark.

Overall: good incident response, root-caused correctly (unbounded inline query + a healthcheck coupled to shared Postgres), and the fallback story is sound. Nothing above blocks merge; #1 is the only one I'd actually ask to fix before merging.

@tonyalaribe
tonyalaribe merged commit 3615604 into master Aug 2, 2026
11 checks passed
@tonyalaribe
tonyalaribe deleted the fix/dashboard-render-budget branch August 2, 2026 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant