fix(server): one materialized table, written once and readable by every source that shares it

[housejester]

Context

malloydata/malloy#3029 settled a question this repo had been working around. An extension of a
persisted source inheriting #@ persist — and so appearing alongside its base as another name for
one table — is by design, not a bug: #@ persist is an annotation, extend never changes a
source's materialization SQL, and the docs it added state plainly that one table with several sources
is "the normal case, not an edge case". The alternative, malloydata/malloy#3012, would have made an
inherited persist stop producing a build target; it was closed in favour of #3029, which
deliberately leaves Model.getBuildPlan() unchanged and puts the grouping in a new
Runtime.getBuildTargets.

To be clear about which follow-up this is: #3029's list includes "the publisher's builder moves
to getBuildTargets". This is not that. This PR stays on getBuildPlan() and fixes defects the
publisher has today — they do not depend on the migration, and two of them are live on the current
pin. The migration is still open, and is the last item below.

So: a materialized table stops being the thing one source owns and becomes an artifact that
several source names route to. A base and its extension compile to the same SQL, hash to the same
content address, and must share one physical table. That inheritance is load-bearing — without it an
extension of a persisted source could never be served from a table at all — and #@ -persist is the
documented opt-out.

The build and serve paths both assumed one source per table. Two defects came out of that, in
opposite directions.

1. One table was written more than once

executeInstructedBuild iterated per SOURCE, so it wrote a table once per name that reached it —
and once per graph that reached it, since a source declared in one model and consumed in another
appears in both models' graphs. Each source also overwrote the manifest entry in turn, so the entry
was attributed to whichever built LAST.

Each physical table is now written once, and the key is the point:

sourceEntityId says what a table contains. destination-or-connection +
physicalTableName says which table it is. Only the second answers "have I already written
this."

Deduplicating on the address would have been wrong in one direction and deduplicating on nothing was
wrong in the other. The coordinate is the identity a write has.

Two definitions on one physical name is the same guard read backwards: different SQL, one table,
so each build overwrites the other's rows while both addresses resolve to it at serve time — a query
answered from another source's data. It follows the existing collision policy rather than minting a
second one: warn by default, refuse under PERSIST_COLLISION_ENFORCE. It earns its place next to
the load-time gate (Package.persistenceCollisionWarnings) by catching the pair that gate cannot
see — physical names an orchestrating host assigns, which are not in the package until the
instructions arrive.

The refusal fires mid-loop, after the pair's first table is written. That is bounded rather than
merely tolerated: under the same flag a package whose model declares the colliding names cannot be
published at all, so what reaches the in-loop throw is a collision between host-assigned names,
which a host minting generational names does not produce. The code says what would make it want
hoisting to a pre-pass.

2. An extension served LIVE instead of reading its base's table

The consequence of that last-writer attribution, and the half users would notice.

Serve bindings are derived one per manifest ENTRY, keyed on entry.sourceName. An entry names only
the source that built the table, and a base and its extension share one entry — so exactly one of
the two names was bound and the other silently served live, chosen by build order. Nothing in
the model text hinted at which.

deriveServeBindings now binds every source that shares the table, all on one virtualHandle. That
is what the handle is for: it is identity-scoped, and its own contract already describes several
sources deduping onto one virtual table.

Three rules make that safe, and each closes a way for one name to be bound twice — which would put
two source: <name> declarations in a single serve shape, fail to compile, and take the storage tier
out for every model in the package (bindings are pushed package-wide), silently, since base-only
is the tier the ladder returns without a probe:

• Grouped by address, keyed by name. The grouping must use the publisher's own content addresses,
  because that is what decides which sources really share a table. The key must be a name, because
  that is the only identifier a manifest entry carries that means the same thing whoever built it —
  an instructed build stamps the caller's sourceEntityId on its entry, and the publisher treats
  that as opaque by contract.
• A name declared at more than one address aliases nothing. Source names are not unique in a
  package — the wire plan is keyed by sourceID for that reason — so such a name cannot be resolved
  to a table by name at all, and map order was picking one.
• An alias never claims a name some entry OWNS. The owner is the source whose SQL produced that
  table.

The grouping lives in groupAliasesByName, a pure function over plan sources, so it is directly
testable — including that its output does not depend on the order sources arrive in.

The colocated tier was never exposed to any of this: it substitutes through the same-connection
manifest, which is keyed by content address, so every alias already resolved.

What is preserved

• No wire or API change. BuildPlan, ManifestEntry and the instruction shape are identical.
  The alias group is read off the package's own build plan, which already carries a content address
  per source, so nothing new travels or is stored.
• Visibility. Each name still faces the eligibility gate on its own merits, and the physical
  table was already narrowed to the base's public columns at build time (projectToPublicColumns),
  so an alias cannot reach past what the base exposed. shape-bounds-physical-columns, both
  visibility leak guards, host-binding-of-unplanned-source and
  host-binding-honors-row-level-access all stay green.
• #@ -persist. The sharpest thing this could have broken. The alias group is built from
  buildPlan.sources, and an opted-out source is not persistent, so it never appears there — it
  joins no group and gets no binding. Had it, it would share the base's address and begin serving the
  frozen snapshot, inverting the documented opt-out. opt-out-persist-recomputes staying green is
  the proof: it asserts the opted-out source recomputes live.

Metrics

Three counters, and the distinction between the last two is the point — they were one, and only one
of them is worth paging on:

• publisher_materialization_duplicate_target_skipped_total — a source whose table this run already
  wrote. Ordinary for a package that extends a persisted source; a volume signal, not a fault.
• publisher_materialization_shared_address_instructions_total — one content address instructed to
  build more than one table. Wasteful, not wrong: the publisher builds each and records one.
• publisher_materialization_table_collision_total — two definitions materializing into one table.
  Serve-time wrong data. Its rate is also what flipping PERSIST_COLLISION_ENFORCE would start
  refusing, so a rollout can be measured before it is enabled.

Scenarios

extend-routes-to-the-base-table (new). The design intent, asserted: no second table for the
extension, but a routed read for it. It mutates the warehouse, so a routed read is distinguishable
from a live recompute — which is exactly what the existing extend scenarios could not do, and why the
lost routing had gone unnoticed.

extended-source-inherits-persist and its colocated twin — corrected, and no longer known-red.
Both asserted exactly ONE build-plan row: that an extension must not appear as a plan source at all,
pending malloydata/malloy#3012. That PR was closed unmerged, and its replacement,
malloydata/malloy#3029, deliberately leaves Model.getBuildPlan() unchanged and documents one table
with several sources as the ordinary case. So the old expectation was not waiting on an upstream fix
— it asserted the opposite of the design, and satisfying it would break serving, because an
extension's read binding is found by looking its address up in the build plan. They now assert the
invariant that does hold and that a regression would violate: both sources present, sharing one
content address, so one table.

imported-persist-from-flagless-model (new). Found while building the above.
persist-without-flag-served-live shows that a #@ persist in a model missing
##! experimental.persistence is inert. That holds only while nothing else in the package carries
the flag: an entry model declaring nothing but the flag and an import is enough for the walk to
reach the imported definitions and materialize them. The flag is a property of the model being
walked, not of the model that declared the annotation — so whether a #@ persist does anything is
decided by a header in another file, and neither state warns. Pinned rather than changed; it pairs
with the open question in scenario 49's note.

Verification

• 2370 unit tests pass; lint clean; tsc --noEmit clean.
• Hammer suite exits 0 with 76 passing. Known-reds go from three to one; the remainder is
  quoted-persist-name-colocated, an unrelated colocated quoting bug its own note scopes out.
• Every new test was confirmed red against the unfixed sources first. The dedupe tests emit two
  CREATE TABLEs where they should emit one; the collision test resolves successfully;
  extend-routes-to-the-base-table returned live values after a mutation; and each alias rule was
  re-checked with that rule removed.

Follow-ups, deliberately not here

#1001 — serve bindings rebind by source name and are pushed package-wide, so a same-named source
in another model can be served from the materialized table when their public columns overlap.
Pre-existing in mechanism; this PR widens which names are eligible, and its alias rules cannot reach
it because they read buildPlan.sources and a shadowed non-persist source is not in there. The issue
carries the proposed fix and the constraint on it.

Grouping on the host side. The wire build plan reports per-source rows, so a host that mints a
physical name per source has to group by sourceEntityId first or it will ask for two tables for one
address. The publisher cannot resolve that on the host's behalf — declining one of the two
instructions would leave the host holding an anchor for a table nothing ever wrote — so it meters the
condition and leaves the grouping to whoever mints the names.

Runtime.getBuildTargets — the open item on #3029's own list. Available since malloy 0.0.429
(this repo is pinned at 0.0.427), so it is actionable now rather than pending a release. It does the
grouping in the compiler and returns one target carrying its buildId plus every source that maps onto it,
deprecating Model.getBuildPlan(), which this PR still uses. Encouragingly, the two already agree on
identity: target.buildId is computed exactly as computeSourceEntityId is, so migrating preserves
every content address — no table is re-addressed, no ledger key moves. The reason it is not folded in
here is that it changes what the builder consumes, whereas everything above is wrong on the current
pin and worth fixing whether or not the migration happens. Doing it separately also keeps this
diff's blast radius to the build loop and the serve seam.

[housejester]

Filed #1001 for the residual this PR's alias binding surfaced: serve bindings rebind by source NAME and are pushed package-wide, so a same-named source in another model can be served from the materialized table when their public columns overlap.

Pre-existing in mechanism — the seam has always been name-keyed — and out of scope here. What this PR changed is which names are eligible: alias names (an extend of a persisted source) now bind where previously only the builder's name did. The alias rules added here cannot reach it, since they are computed from buildPlan.sources and a shadowed non-persist source is not in there.

#1001 has the proposed fix (match on sourceID, which an imported source preserves — so it survives the import cases path-scoping would break) and the limit that constrains it (sourceID embeds an absolute modelURL, so it is only comparable for self-derived bindings).

[jswir]

Peer review (pre-merge)

Checked against current head 885f7dbc (force-push after the first draft of this review).

Simple frame: Malloy #3029 made “several sources, one physical table” the design (extend inherits #@ persist; #@ -persist opts out). This PR fixes Publisher still assuming one source owns one table — write the table once, bind every name that shares it. It is not the Runtime.getBuildTargets migration.

Important

1. PERSIST_COLLISION_ENFORCE refuses after the first write. materialization_service.ts throws mid-loop. Load is always warn-only (package.ts ~709–712), so an already-loaded colliding package can still rebuild. Reclaim cannot restore prior rows and skips colocated tables. The publish gate refuses before any CTAS — hoist this check to a pre-pass to match.

2. Scenario 72’s ## Note still describes the old bug. 72-extend-routes-to-the-base-table/scenario.md asserts both names stay stale after mutate, but the note says only one name binds and the other serves live. Drop needs-attention if the question is closed; Hammer still lists ## Note as ⚠.

Rebase onto current main — done on this SHA. #1004 / #1023 (failures collection) and writtenTargets already live in the same loop.

Nit

1. Shared-address instructions without sourceID last-write-win on bySourceEntityId, so the first physical table is never built — contradicts “warn and build both.” sourceID is optional in OpenAPI. The unit test stamps sourceID on both instructions. Auto-run (one instruction per address) is fine.
2. The enforce throw’s Fix text says “distinct #@ persist name=” on the host-assigned physicalTableName path.
3. Document the three new counters in RELEASE_NOTES.md (especially table_collision, which this PR says to page on).
4. Scenario 56’s ## Note is rewrite history with no open question — Hammer will keep surfacing it.
5. Directories 71-* / 72-* already exist on main (chained-write-only). Front-matter id is the runner key, so rename on rebase to avoid numbering noise.

Out of scope (agreed)

#1001 — name-keyed, package-wide serve bindings. Pre-existing; this PR widens which names bind. Leave it on that issue.

Checklist for landing

• Rebase onto #1023 (present on 885f7dbc)
• Collision check before the first write
• Rewrite scenario 72 note; drop stale needs-attention
• Optional: no-sourceID shared-address test; Fix text; release notes; scenario 56 note; directory numbers

[jswir]

Local materialization (Hammer) on 885f7dbc, real Publisher + throwaway Postgres + DuckLake (bun hammer/run.ts --rebuild --scenarios …):

Scenario	Result
extend-routes-to-the-base-table	PASS 6/6 — both names stay stale after warehouse mutate (extension reads the base table)
extended-source-inherits-persist	PASS 4/4
extended-source-inherits-persist-colocated	PASS 4/4
imported-persist-from-flagless-model	PASS 7/7

The review comment above is edited in place: rebase item struck; remaining importants are mid-loop ENFORCE and scenario 72’s stale note (Hammer still prints that note under ⚠ needs attention).

[housejester]

Thanks — this was a good review. Rebased onto 8d7f5656 and all items addressed; head is d3541a77.

Important

1. PERSIST_COLLISION_ENFORCE refused after the first write. Fixed, and you were right about why my defence didn't hold. I had argued the mid-loop throw was bounded because the publish gate rejects a model-declared collision under the same flag. But loadPackage says the opposite in as many words — collisions are always warn-only at load, whatever the flag says — so a package published before the flag went on stays loaded and reaches the build with its collision intact. That was the hole.

A pre-pass now claims each physical table before the loop starts, so the refusal lands before any CTAS. The loop keeps only the same-content case (an extra name for one artifact, nothing to build). Both derive the coordinate from one shared helper, so they can't drift on what table an instruction writes.

The enforce test now asserts zero CREATE TABLE statements, and I checked it fails against the old code — Received length: 1, i.e. the first table really was written before refusing.

2. Scenario 72's note contradicted its own assertions. Removed, along with needs-attention. The note still described the pre-fix behaviour (one name binds, the other serves live) while the scenario asserts both stay stale — exactly the doc-vs-reality drift this PR is otherwise about. The body already carries the thesis, so there was nothing left to keep.

Nits

1. No-sourceID shared-address instructions. You're right, and the claim was the wrong part rather than the mechanism. Without a sourceID the address index is the only route back to an instruction and holds one per address, so the last wins and the earlier names are never built — nothing can attribute them, since an address maps to all of them equally. The comment and the counter's help text said "every one is built"; both now describe both shapes, and a test pins the no-sourceID behaviour (its sibling stamps a sourceID and proves the opposite, which is what hid this).
2. Fix text. The enforce message no longer prescribes #@ persist name= on a host-assigned path: it names both remedies and says which belongs to which.
3. Release notes. Added, with table_collision called out as the one to alert on and its rate framed as what enabling enforce would begin refusing.
4. Scenario 56's note. Removed. Trimmed 55's to just the residual that is still open, on the same reasoning — you only asked about 56, but leaving 55's history would earn the same comment next round.
5. Directory numbers. 76- / 77-; 71-/72- are main's chained-write-only pair.

Suite state

3324 unit tests pass, typecheck and lint clean. Hammer: 78 pass, and four non-passes — all of which reproduce identically on plain 8d7f5656 with this branch checked out of the way:

Scenario	On this branch	On origin/main
failed-run-reclaims-its-tables	3/6	3/6
host-binding-honors-row-level-access	2/3	2/3
persisted-sql-select-colocated	1/2	1/2
quoted-persist-name-colocated	known-red	known-red

Flagging rather than fixing, since none is this PR's, but two look worth someone's attention: failed-run-reclaims-its-tables expects an orchestrated build to be refused and now gets MANIFEST_FILE_READY with an unreclaimed table, which reads like fallout from #1023 (a losing source no longer throws, so the reclaim never fires); and host-binding-honors-row-level-access is tagged security. Happy to file both.

One other thing found while checking my own work: packages/server/src/service/model.ts carries three raw NUL bytes at line 1397 (a ${label}\0${exprs.join("\0")}\0… dedupe key), which arrived in 2bd5f46d. Same failure mode you caught in me — the file is binary to grep -r today, though git grep still sees it. Untouched by this branch; the JSON.stringify treatment applies if you want it fixed the same way.

[jswir]

Reviewed the build dedupe and the alias routing closely. Both hold up: single write path with no bypass, no name can be bound twice, #@ -persist sources never enter the alias pool, aliases face the same eligibility gate and get narrowed to their own public fields, and scenario 77 is a real oracle. The findings below are all in the collision guard and its docs, not in the two fixes. None of it blocks merge — the behaviour this PR changes is sound.

Worth fixing before merge, none of it blocking

1. The collision check counts one collision once per graph that reaches it. materialization_service.ts:1899-1941

For sources A and B (different addresses, same target T) both visible from two models' graphs: graph 1 has A claim T and records the collision with B; graph 2 re-confirms A's claim and records B again. One collision, two records — so publisher_materialization_table_collision_total over-counts, the warning prints twice, and the enforce message lists 'A' and 'B' twice via the .join("; ").

This is the same "once per graph that reached it" bug the write dedupe exists to fix, and there's already a test for it on the write side. Every collision test uses a single graph, so nothing catches it. Key the records by target rather than pushing to an array, and add a cross-graph case.

Related, same counter: the pre-pass runs ahead of assertMaterializationEligible (:2021, :2038, :2096), so a pair where one member the gate would refuse still counts and, under the flag, still fails the whole build. Errs safe, but widens the gap between the counter and what the flag would actually refuse — which the release note offers as its main use.

2. physicalTargetKey diagnoses a hazard and leaves it standing in the sibling it names. materialization_service.ts:584-611 vs package.ts:1786

The new key uses JSON.stringify on a tagged array, and the comment says why: a physical name can contain a space, so a plain separator can make two coordinates read as one. It then cross-references Package.persistenceCollisionWarnings, which still builds `${destination} ${physicalName}`.

Destination lake x + table y and destination lake + table x y both key to lake x y — two unrelated targets reported as colliding, and a wrongly-rejected publish under PERSIST_COLLISION_ENFORCE. Quoted names with spaces are supported (60-quoted-persist-name), and I didn't find validation forbidding a space in a destination or connection name. Extract one key-builder both call, or say in the comment why the sibling is safe.

3. PERSIST_COLLISION_ENFORCE now fails builds; three comments still say publish-only. config.ts:787-798, server.ts:228-231

config.ts says "the flag only governs whether publish rejects." server.ts says "whose only other caller is the publish path." The new throw at :1951 fires on ordinary and scheduled builds. That's a much larger blast radius, and these comments are what an operator reads before turning the flag on.

4. The collision throw doesn't set reason, which its own docblock says every throw site sets. errors.ts:183-205, thrown at materialization_service.ts:1951

The comment explains reason exists so callers classify without parsing the message; a classifying caller now gets undefined. The error also documents one source being unprocessable, which isn't what a collision is. Add a "collision" member to EligibilityRefusalReason, or use a distinct class.

Smaller items

5. The description argues for a design the code no longer has. It says the refusal "fires mid-loop, after the pair's first table is written" and defends that as bounded. The code refuses in a pre-pass before any write, which the release notes already state correctly. Worth updating so review attention doesn't go to the superseded argument.

6. One question on publisher_materialization_shared_address_instructions_total. Auto-run can't trigger it — deriveSelfInstructions dedupes by content address and names tables from the annotation — so this only fires on the host path. The metric's help text says both outcomes: with sourceID, every table is built and one recorded; without it, the last instruction wins and the earlier names are never built. The release note and the description say only the first ("Wasteful, not wrong: the content is the same either way"), while the new test pins the second.

So: does the orchestrating host always send sourceID? If yes, soften the release-note wording and this is a pure canary. If no, these are two conditions with different severity sharing one counter — a host holding names for tables that were never built isn't "not wrong."

Nits: the alias lookup indexes a plain {} by source name, so a source named constructor/toString/__proto__ throws (Object.create(null) fixes it, materialization_serve_transform.ts:175); the new function was inserted between the MaterializationService doc comment and the class, orphaning it (:567-612); the order-independence test reverses an input whose groups are singletons, so it can't distinguish anything; duplicate_target_skipped_total is never asserted through the metrics harness and shared_address_instructions_total only as zero; scenario 76 says of itself it's "not the fix's own test."

One structural question, open to the counter-argument: the dedupe and the read-routing fix are independently revertable, neither fixes the other's symptom, and both came out clean. The collision guard is new feature work and carries all four findings above. Splitting at the file boundary would put review weight where the risk is. The case against is that these are one model change seen from two sides, which is fair — but the diff is large enough that it isn't free.