Skip to content

expr: bound the memory a webhook CHECK may allocate (SQL-431) - #38162

Open
mtabebe wants to merge 1 commit into
MaterializeInc:mainfrom
mtabebe:fix/webhook-check-memory-cap
Open

expr: bound the memory a webhook CHECK may allocate (SQL-431)#38162
mtabebe wants to merge 1 commit into
MaterializeInc:mainfrom
mtabebe:fix/webhook-check-memory-cap

Conversation

@mtabebe

@mtabebe mtabebe commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Problem:
A webhook CHECK is a user defined expression that environmentd evaluates once per in-flight request, and nothing bounds what it could allocate.

The webhook path caps request count (500) and body size (5 MiB), but the evaluation's footprint is per-request and was bounded only by MAX_STRING_FUNC_RESULT_BYTES, a 100 MiB per-call ceiling sized for a cluster.

That ceiling multiplies by request concurrency, so a CHECK doing length(repeat(body, 20)) >= 0 turned 150 concurrent 5 MB posts into 8.7 GiB of RSS, growing with the number of clients.

Solution:

  • Give RowArena an optional budget and build the validation arena with one, defaulting to 20 MiB via a new dyncfg, webhook_validation_memory_budget_bytes.
  • Functions that can predict their result size consult max_string_func_result_bytes, which narrows the constant to the arena's remaining budget, so an over-budget result is never allocated at all.
  • Everything else is caught by a post-call check in the evaluator, which covers functions with no ceiling of their own.
  • Stop copying an owned result into the arena. String and Vec outputs went through push_string/push_bytes, which copied and dropped the original.

An unbudgeted arena, which is every arena in a dataflow, is unaffected

Testing:

  • New integration test webhook_validation_memory_budget: a 2 MiB body that amplifies past the 20 MiB default is refused with 400, a smaller body through the same source succeeds, and raising then lowering the dyncfg flips the result both ways
  • New expr unit tests cover the pre-check and post-call paths
  • New unit tests cover the arena bookkeeping and buffer adoption

@mtabebe
mtabebe force-pushed the fix/webhook-check-memory-cap branch from 81ae96c to de610ce Compare August 11, 2026 16:44
Problem:
A webhook CHECK is a user-authored expression that environmentd evaluates
once per in-flight request, and nothing bounded what it could allocate. The
webhook path caps request count (500) and body size (5 MiB), but the
evaluation's footprint is per-request and was bounded only by
MAX_STRING_FUNC_RESULT_BYTES, a 100 MiB per-call ceiling sized for a
cluster. That ceiling multiplies by request concurrency, so a CHECK doing
length(repeat(body, 20)) >= 0 turned 150 concurrent 5 MB posts into 8.7 GiB
of RSS, growing with the number of clients. Every request returned 200.
Nothing refused the work, so the memory was just held.

Solution:
- Give RowArena an optional budget and build the validation arena with one,
  defaulting to 20 MiB via a new dyncfg, webhook_validation_memory_budget_bytes.
- Enforce in two places, because amplifiers allocate in two shapes. Functions
  that can predict their result size consult max_string_func_result_bytes,
  which narrows the constant to the arena's remaining budget, so an
  over-budget result is never allocated at all. This now covers the string
  amplifiers and array_fill, which sizes its result from a parameter rather
  than its input. Everything else is caught by a post-call check in the
  evaluator, which covers functions with no ceiling of their own.
  string_to_array, for one, builds its array straight into the arena and
  consults none.
- Add EvalError::TempStorageBudgetExceeded for the over-budget case rather
  than reusing LengthTooLarge, whose "requested length too large" text
  misdescribes an over-budget array or arena-built result.
- Report the budget rather than enforce it in the arena itself. Its pushes
  are infallible, and refusing one would hand back a truncated value.
- Stop copying an owned result into the arena. String and Vec<u8> outputs
  went through push_string/push_bytes, which copied and dropped the original,
  so an amplifying call materialized its bytes twice at the peak.
  push_owned_bytes adopts the allocation as a region when the bytes would not
  fit the active region anyway, and copies when they would, so a small value
  still shares a region rather than getting one of its own.
- Register webhook_validation_memory_budget_bytes with parallel-workload's
  FlipFlagsAction and mzcompose's UNINTERESTING_SYSTEM_PARAMETERS, which
  bin/lint-test-flags requires for every new dyncfg.

An unbudgeted arena, which is every arena in a dataflow, is unaffected beyond
one branch on a None.

Testing:
- New integration test webhook_validation_memory_budget in server.rs: a 2 MiB
  body that amplifies past the 20 MiB default is refused with 400, a smaller
  body through the same source succeeds, and raising then lowering the dyncfg
  flips the result both ways.
- New expr unit tests test_repeat_respects_arena_budget,
  test_array_fill_respects_arena_budget, and
  test_arena_built_result_respects_budget cover the pre-check paths (a
  parameter-sized string and array) and the post-call path. miri_test_arena_budget
  and miri_test_arena_adopts_owned_bytes_and_keeps_references in row.rs cover
  the arena bookkeeping and buffer adoption.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mtabebe
mtabebe force-pushed the fix/webhook-check-memory-cap branch from de610ce to 4a5f741 Compare August 11, 2026 16:57
@mtabebe
mtabebe marked this pull request as ready for review August 11, 2026 19:07
@mtabebe
mtabebe requested review from a team as code owners August 11, 2026 19:07
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