Skip to content

test(materialization): GC + scheduler state-machine transition coverage - #902

Merged
girishjeswani merged 5 commits into
malloydata:mainfrom
girishjeswani:test/materialization-gc-coverage
Jul 22, 2026
Merged

girishjeswani merged 5 commits into
malloydata:mainfrom
girishjeswani:test/materialization-gc-coverage

Conversation

@girishjeswani

Copy link
Copy Markdown
Collaborator

Summary

Closes #895 — the GC + state-machine-transition coverage split out of #888. All additive tests plus one documenting comment; no behavior change.

Garbage collection

  • Drop metering: going through deleteMaterialization({ dropTables }) meters recordDropTables("success") on a successful physical-table drop and ("failure") when the drop throws — and the record is still deleted (a best-effort drop failure never surfaces to the caller). Asserted via the OTel in-memory harness.
  • Staging cleanup edges: the original build error propagates when the staging-cleanup drop itself fails (leak logged), and a cancelled mid-run build drops its staging table like a failure — no _staging orphan. (Build-SQL failure was already covered.)
  • Cascade deletes are records-only: deleteByEnvironmentId / deleteByPackage issue a single DELETE and no DDL. A materialized table intentionally outlives its record on env/package delete — physical GC is the caller's responsibility (same contract as deleteMaterialization's default). Locked in with SQL-shape tests + a contract comment on both repo methods, so a future refactor can't silently add a DROP there. This is the "explicit decision, not silent" the feat(materialization): standalone scheduler, environment-scoped listing, and schedule management UI #888 review asked to keep visible; the optional standalone cascade-drop is tracked in Optional: standalone cascade-drop of materialized tables on env/package delete #901.

State-machine transitions (integration, real store)

Complements the per-tick unit guards by driving each transition end to end through REST-created packages the scheduler sweeps:

  • a cron edit between ticks re-arms from now (no stale fire from the old cadence)
  • a package unload/reload prunes arming state and re-anchors without a spurious fire
  • a fired occurrence advances to the next occurrence (fires once per occurrence, not every tick; the advance is outcome-independent, so the FAILED-fire isolation stays unit-covered — a genuine failed build isn't reliably reproducible without a brittle fixture)
  • maxFiresPerTick caps a tick and the capped package fires on the next tick

Each transition test uses its own environment and tears it down in afterEach, since the scheduler sweeps every loaded environment.

Testing

  • Server unit: 1165 pass / 0 fail (incl. drop-metering, staging-edge, records-only repo tests).
  • Materialization integration: 38 pass / 0 fail (incl. the 4 new transition tests).
  • typecheck + eslint clean on changed files.

Notes

  • All commits are DCO signed-off.
  • No production behavior change — coverage + one documenting comment only.

…cords-only cascade deletes

Hardens garbage-collection coverage for the materialization service (fast-follow
to malloydata#888, split out per review as malloydata#895):

- deleteMaterialization telemetry: a successful physical-table drop meters
  outcome=success; a failed drop meters outcome=failure and still deletes the
  record, so a best-effort drop failure never surfaces to the caller.
- buildOneSource staging cleanup: the original build error propagates when the
  staging-cleanup drop itself fails (leak logged), and a cancelled mid-run build
  drops its staging table like a failure — no _staging orphan.
- Cascade deletes are records-only: deleteByEnvironmentId / deleteByPackage issue
  a single DELETE and no DDL, documenting that physical-table GC is the caller's
  responsibility so a materialized table intentionally outlives its record on an
  env/package delete. Contract comment added on both repo methods.

Covers items 1-3 of malloydata#895; the state-machine transition tests (item 4) follow
once malloydata#888's scheduler lands on main.

Signed-off-by: Girish Jeswani <[email protected]>
…ne transitions

Drives the scheduler's transitions end to end against the real store (malloydata#895,
item 4), complementing the per-tick unit guards:

- a cron edit between ticks re-arms from now (no stale fire from the old cadence)
- a package unload/reload prunes arming state and re-anchors without a spurious fire
- a fired occurrence advances to the next occurrence (fires once per occurrence,
  not every tick; the advance is outcome-independent, so the FAILED-fire
  isolation stays unit-covered)
- maxFiresPerTick caps a tick and the capped package fires on the next tick

Each test uses its own environment and tears it down in afterEach, since the
scheduler sweeps every loaded environment.

Signed-off-by: Girish Jeswani <[email protected]>
The integration version drove the prune + re-anchor transition via a package
delete + same-name re-add, which races the package-install staging rename on
Windows (ENOENT on the staging->final rename; POSIX handles it) — unrelated to
the scheduler logic under test. The transition is pure in-memory arming state,
so it's now a deterministic unit test in materialization_scheduler.spec.ts
(mutating the fake loaded set). The other three transitions stay as integration
tests.

Signed-off-by: Girish Jeswani <[email protected]>
@mlennie

mlennie commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

@girishjeswani Reviewed thoroughly, including running everything locally: full root gate on the PR head (unit 1166 pass / 0 fail, integration 225 / 0) and again on a local merge with current main (1205 / 0 unit, 225 / 0 integration, no conflicts and no interaction with #904's quoting change, which is confined to manifest-bind sites the failure-path tests never reach). Behavior-change claim verified: the only non-test hunk is the comment block in MaterializationRepository.ts, and the three diff deletions are all in-place expansions, nothing weakened. DCO present on all 3 commits. The transition tests are carefully built: hand-driven ticks with no started scheduler, UTC-pinned cron, fire counts read back through metadata.trigger, and waitAllTerminal before the no-refire assertion closes the coalesce loophole. Nice work. LGTM once the description is refreshed, plus one test I would reframe or drop.

Description is stale after 842ee44. The body still lists unload/reload under "State-machine transitions (integration, real store)" and implies 4 integration transition tests; the final diff has 3, with unload/reload moved to a deterministic unit test (a good trade for the Windows staging-rename race, and the moved test genuinely covers prune, fresh re-anchor, and re-armed liveness). Also "contract comment on both repo methods" is one block above deleteByEnvironmentId; fine as is, just worth matching the words to the diff since this PR is the durable record of what was covered.

The cancelled-mid-run staging test does not exercise cancellation (materialization_service.spec.ts, "drops staging when the build is cancelled mid-run"). Production cancellation is cooperative: the only abort check is if (signal.aborted) throw new Error("Build cancelled") inside executeInstructedBuild's per-source loop, before buildOneSource is entered, and neither the signal nor any abort ever reaches buildOneSource/runSQL. So a real cancel can never reject the CREATE inside buildOneSource's try block, which is the exact scenario the test stages; it can only fire between sources, at which point no staging table exists. The test passes, but it is the existing failure-cleanup test with a renamed error string, and it cannot fail under any cancellation-specific regression. I would either drop it or reword the test name/comment (and the PR bullet) to "a build-SQL failure mid-CREATE takes the staging-cleanup path", which is what it actually locks in. No staging orphan is reachable from cooperative cancel, so the coverage story is actually simpler than the body claims.

Optional hardening, take or leave:

  • The records-only SQL-shape tests assert on runCalls only. DDL routed through db.all would evade them, and DuckDBConnection exposes both. One extra assertion that the all captures contain no DROP (or stay empty) closes the loop the comment says the tests exist to lock.
  • afterEach env teardown never checks res.ok, so a 4xx delete is silently absorbed, and the spec's own header explains why a leaked env matters: tick sweeps every loaded environment and maxFiresPerTick = 1 is a process-wide budget, so a leaked schedulable env (this suite's or a sibling suite's, since environmentStore is a shared module singleton across the integration run) can steal the cap test's single fire. A res.ok check plus a defensive sweep of leftover sched-tx-* envs in beforeAll would make the cap test unstealable.
  • The new contract comment documents the operational rationale well; one more line noting the data-retention consequence (a materialized table outliving its record means the data outlives an environment delete, which is exactly what Optional: standalone cascade-drop of materialized tables on env/package delete #901 tracks) would make the security tradeoff as visible as the operational one.

@sagarswamirao

Copy link
Copy Markdown
Collaborator

Nice, well-scoped coverage. One small note on the records-only cascade test + contract comment: they pin "no DDL in deleteByEnvironmentId / deleteByPackage" as the behavior, which is right for an orchestrated deployment where the control plane owns physical GC. The one thing I'd guard against is the test making the standalone gap (#901: no caller drops the tables, so they accumulate in the warehouse) look like a settled contract rather than an open follow-up.

Could the test/comment reference #901, so a future best-effort drop-before-teardown (the fix #901 already sketches) reads as completing the design rather than violating a green "no DDL here" test? Purely about keeping the follow-up visible at the code site; the behavior in this PR is correct as-is.

…de contract

The records-only cascade-delete comment and its guard test pin "no DDL in
deleteByEnvironmentId / deleteByPackage". Note the standalone GC gap (malloydata#901)
at both code sites so the behavior reads as an intentional-for-now contract
with an open follow-up, not a settled one: malloydata#901's best-effort
drop-before-teardown lives above these methods, so adding it completes the
design and leaves this test green. Comment-only; no behavior change.

Signed-off-by: Girish Jeswani <[email protected]>
@girishjeswani
girishjeswani enabled auto-merge (squash) July 22, 2026 19:44
@girishjeswani
girishjeswani merged commit 628c710 into malloydata:main Jul 22, 2026
10 checks passed
@girishjeswani
girishjeswani deleted the test/materialization-gc-coverage branch July 22, 2026 19:56
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.

Materialization scheduler: GC + integration state-machine transition coverage (fast-follow to #888)

3 participants