Skip to content

Bound the viewer/mcp roles' on-disk temp-file spill (#4605) - #4610

Merged
erikdarlingdata merged 2 commits into
devfrom
fix/4605-read-temp-limit
Sep 28, 2026
Merged

erikdarlingdata merged 2 commits into
devfrom
fix/4605-read-temp-limit

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

Part of #4605 (part 1 of 2; the read-routing half is separate).

Summary

The viewer and mcp store roles already carry a statement_timeout backstop on composed reads (Custom Views v2, #1563). This adds the on-disk-spill half next to it: a role-level temp_file_limit on the same two roles, so a runaway dashboard panel or MCP read that would spill gigabytes of temporary files to the store's own volume is cancelled by PostgreSQL itself (SQLSTATE 53400) instead of running to the statement_timeout while writing that spill and starving the collector's writes.

Branch: fix/4605-read-temp-limit, from origin/dev at fast-forward merge sha e7d90a274 (the last step before this push).

What changed

1. The setting.

  • ComposeLimits.TempFileLimit = "1GB" (Compose/ComposeSpec.cs), next to ComposeLimits.StatementTimeout.

  • The value is final, not a placeholder: 1 GB per process. The largest legitimate viewer/mcp spill per call measured in the field (pg_stat_statements, summed across the leader and its parallel workers, so the per-process ceiling this constant sets is at or below the call's real total) was 322 MB, from the rollup coverage probe — 1 GB gives that better than 3× headroom. The runaway panel Custom Views panels over a recent window scan raw hypertables with no bound; a Query Store panel spilled ~23 GB and tore collector writes #4605 describes spilled 1,830 MB per call. No max_parallel_workers_per_gather cap accompanies this; that lever was considered and left out on purpose.

  • tools/provision-roles.sql (the bring-your-own-store script) carries the matching ALTER ROLE ... SET temp_file_limit = '1GB' pair for viewer/mcp, with a comment naming the setting as superuser-only (PGC_SUSET), exactly like the existing log_min_duration_statement/log_parameter_max_length lines it sits beside. No new fallback branch was added for a non-superuser owner: the existing code has no precedent for degrading a superuser-only role setting on that path (the statement_timeout/slow-statement lines already assume the same superuser posture managed provisioning always runs under), so this mirrors that as-is, and adds no new failure handling.

  • Its own renderer. The temp_file_limit pair is rendered by DarlingManagedRoles.BuildComposeTempFileLimitSql, separate from the timeout. Provisioning emits it whether or not the compose timeout can be read, and the reload re-assert emits it beside the timeout, so the spill guard is never dropped with an unreadable timeout. ControlPlaneReloadDurabilityTests pins that only the timeout-derived lines are left out, and TheProvisioningBatch_EmitsTempFileLimit_EvenWhenTheComposeTimeoutIsUnreadable pins that the limit stays.

2. The classifier. ReadOutcome (ReadLatencyAccumulator.cs) gains a Limit member. ReadOutcomeClassifier.Classify now maps SQLSTATE 53400 (configuration_limit_exceeded) to ReadOutcome.Limit, checked before the existing 57014 branch, with 57014 unchanged. The web composer's own outcome mapping (DarlingWebEndpoints.RecordComposeLatency) is updated the same way for the AuthorSqlState path. DarlingReadLatencyReader's doc comment (the histogram's "every outcome" note) is updated to name it.

3. The composer's caller-facing message. DarlingWebEndpoints.IsComposeRunAuthorActionable now treats 53400 as author-actionable (the panel author fixes it the same way they fix a statement_timeout cancel: narrow the panel). FromPostgresException gives it its own text instead of the generic "Query failed: {MessageText}": TempFileLimitExceededMessage = "This panel needed more temporary disk space than a dashboard read may use. Narrow the time window, choose an hourly or daily grain, or add a filter." — named because the store's own wording ("temporary file size exceeds temp_file_limit") names an internal setting the author has no way to change.

Pins

  • Source/SQL: TheProvisioningBatch_EmbedsTheSharedRenderer_Verbatim now also asserts the temp_file_limit line appears exactly twice; TheRenderer_SetsTempFileLimitOnBothComposeRoles_AndNotAdmin pins the exact SQL on both roles and its absence on admin.
  • Classifier: Classify_53400ConfigurationLimitExceeded_IsLimit, Classify_RequestTokenCancelled_IsCancelled_EvenWithA53400Exception (token-check priority over the new branch).
  • Composer: IsComposeRunAuthorActionable_ConfigurationLimitExceeded53400_IsTrue, FromPostgresException_ConfigurationLimitExceeded53400_CarriesTheSpillMessage_NoFault.
  • LIVE, on a local TimescaleDB rig (removed at the end): ComposeTempFileLimitLiveTests provisions a throwaway role with the renderer's own SQL output (BuildComposeStatementTimeoutSql, role names substituted), lowers temp_file_limit to 1MB for the test only, connects as that role, confirms SHOW temp_file_limit returns the lowered value on a fresh session, runs a query that spills past it, and asserts the caught PostgresException carries SQLSTATE 53400 and classifies as ReadOutcome.Limit. GREEN on this branch (1/1). RED on origin/dev (e7d90a274): copying the same test file into a detached worktree of that commit fails to COMPILE (CS0117: 'ReadOutcome' does not contain a definition for 'Limit') — the type doesn't exist yet on dev.
  • Manual live confirmation (same rig, before writing the xUnit pin): as viewer with temp_file_limit lowered to 1MB, SELECT string_agg(md5(random()::text), '') FROM generate_series(1, 500000) g raises ERROR: 53400: temporary file size exceeds "temp_file_limit" (1024kB).
  • Census family: CommentFilterAdoptionTests, DocCommentHygieneTests, McpPayloadContractCensusTests, McpToolsListBudgetTests, StartupCommandTimeoutTests, StorageCommandTimeoutTests all pass. (DocCommentHygieneTests initially failed on a stale <see cref="!:DarlingManagedRoles.BuildComposeStatementTimeoutSql"/> outside-repo-style cref left over from an edit; fixed to a plain resolvable cref, matching an existing working reference to the same type two lines above it in the same file.)
  • Full run of the touched classes (267 tests total, including the above): 0 failed, 0 skipped (aside from one pre-existing unrelated live test needing its own DARLING_TEST_PG shape, unaffected by this change).
  • Build: Darling.Tests and Lite.Tests both build clean, 0 warnings, 0 errors, -p:EnableWindowsTargeting=true. Both target net10.0-windows and cannot RUN here; Darling.Tests.dll ran in-process per the standard macOS method above. No Lite.Tests changes in this PR.
  • Merged origin/dev at e7d90a274 as the last step before this push (fast-forward, no conflicts); rebuilt and reran the full touched-class set afterward — still green.

Notes

  • No migration: temp_file_limit has no probeable schema footprint, exactly like statement_timeout's existing non-migration reasoning in the same file.
  • Nothing in this PR changes max_parallel_workers_per_gather; it is deliberately out of scope, not left out by oversight.

CHANGELOG

SECTION: Changed
ENTRY: - Darling Viewer and MCP store reads can no longer spill unbounded temporary files ([#4610]) - The viewer and mcp store roles now carry a temp_file_limit, so a Custom Views panel that would have written gigabytes of temporary files fails at once with a message naming the limit, instead of running to the 60-second statement timeout and slowing the collector's writes.
REF: [#4610]: #4610

Adds a role-level temp_file_limit to the viewer and mcp store roles, next to
their existing statement_timeout backstop, so a runaway dashboard or MCP read
that would spill gigabytes of on-disk temp files is cancelled by the store
itself instead of running to the statement_timeout while starving the
collector's writes. The ALTER ROLE pair is rendered by the same single SQL
builder that already emits statement_timeout and its slow-statement line
(DarlingManagedRoles.BuildComposeStatementTimeoutSql), so provisioning and the
control-plane reload can never disagree about the ceiling. tools/provision-
roles.sql carries the matching statements for a bring-your-own store, guarded
the same way its existing superuser-only settings are.

Adds a distinct ReadOutcome.Limit for SQLSTATE 53400
(configuration_limit_exceeded), which is what PostgreSQL raises when a role's
temp_file_limit is exceeded, so ReadOutcomeClassifier no longer folds it into
the generic Error bucket. Gives the composed-panel runner a caller-facing
message for it, distinct from the generic 'Query failed' text, naming the
actions a panel author can take (narrow the window, change grain, add a
filter) rather than the store's own internal-setting wording.
…compose timeout (#4610)

The temp_file_limit backstop was rendered inside BuildComposeStatementTimeoutSql,
so an unreadable compose timeout dropped the spill guard along with the
timeout-derived lines. temp_file_limit is a constant, not derived from the
timeout, so it now has its own renderer (BuildComposeTempFileLimitSql),
emitted unconditionally in provisioning and alongside the timeout lines on
a control-plane reload.
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