Skip to content

fix(build): stale LEGACY_DELEGATES from undeclared registry input; missing registry now fatal - #616

Merged
sanity merged 1 commit into
mainfrom
fix/stale-registry-build
Aug 11, 2026
Merged

fix(build): stale LEGACY_DELEGATES from undeclared registry input; missing registry now fatal#616
sanity merged 1 commit into
mainfrom
fix/stale-registry-build

Conversation

@sanity

@sanity sanity commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Problem

ui/build.rs bakes LEGACY_DELEGATES — the delegate migration table — from ../legacy_delegates.toml, but deliberately emitted no cargo:rerun-if-changed directives (.rerun_if_changed(false)), on the theory that Cargo's default heuristic "always regenerates". That theory is false: the default heuristic re-runs a build script when files inside the package change, and the registry lives at the workspace root, outside the ui package. So editing the registry does not invalidate the cached build script.

Reproduced in the main (non-worktree) checkout on this repo, before the fix:

$ cargo build -p river-ui          # baseline: Finished in 0.28s
$ echo "# repro: stale-registry probe" >> legacy_delegates.toml
$ cargo build -p river-ui
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.26s
# generated legacy_delegates.rs mtimes UNCHANGED across all build dirs
# (newest stayed 1785892298) — the build script never re-ran

scripts/add-migration.sh edits exactly that file. So the normal migration ritual — record the outgoing delegate's hash, rebuild, publish — can ship a bundle whose baked LEGACY_DELEGATES omits the outgoing delegate. The startup sweep then probes nobody and every returning user loses their rooms, with no error anywhere. This is the freenet/delta#52 failure mode (which stranded Delta users), reproduced in the app with the most users.

A worktree cannot reproduce this: there .git is a file, git-derived paths don't resolve, and Cargo treats the script as always-dirty — everything looks correct from exactly the vantage point the repo's own conventions mandate. That is why it survived this long.

Second route to the same outcome: .allow_missing_registry(true) meant a missing registry file yielded an empty table with at most a cargo:warning (which scrolls past unread in a release build). Same silent total-migration-failure, different trigger.

Approach

Copied the shape Delta landed for this exact problem (delta/ui/build.rs on main), rather than inventing a new one:

  • ui/build.rs now declares every input it reads: build.rs itself, the registry (via the crate's directive, .rerun_if_changed(true)), and the git HEAD/index files that decide GIT_COMMIT_HASH — resolved via git rev-parse --git-path, which works in both a normal checkout and a worktree (a literal ../.git/HEAD doesn't resolve in a worktree and makes the script permanently dirty there).
  • .allow_missing_registry(docs_rs_build()) in both ui/build.rs and common/build.rs: a missing registry is now a hard build failure everywhere except docs.rs (the one environment where ../legacy_delegates.toml legitimately isn't shipped; common's TOML ships inside the river-core package, so its leniency is even narrower in practice).
  • An emptiness gate on the generated output in both scripts: a registry that parses to zero entries fails the build. Both registries are append-only and can never legitimately be empty; the codegen crate accepts an entry-less TOML as valid, so this closes the renamed-section/truncated-file route to an empty table.
  • common/build.rs was already fresh (the crate emits the registry directive by default) — it only gains the missing/empty gates.

Behaviour change: BUILD_TIMESTAMP_ISO freshness

Reviewers should see this rather than discover it. Emitting any rerun-if-changed disables Cargo's default heuristic, so the timestamp is no longer stamped on literally every compile. The new guarantee is: fresh whenever any declared input changes (build.rs, the registry, git HEAD/index — i.e. every commit or checkout), plus on demand via FORCE_REBUILD_TIMESTAMP. Publishes follow a commit, so a published build is always freshly stamped. This is the same trade Delta made.

Testing

Verified in a fresh clone (real .git directory — the layout where the bug lives and where cargo make publish-river runs; a worktree structurally cannot show it):

  1. Registry content change → rebuild + changed const: edit a description in legacy_delegates.toml → build re-runs (4.85s), generated legacy_delegates.rs mtime advances, const content changes (// V1: VERIFY-REBUILD probe in the diff), BUILD_TIMESTAMP_ISO refreshes.
  2. No change → cheap no-op: 0.29s, build-script output mtime and timestamp byte-identical — the fix does not make the script always-dirty (which would re-mask this bug class).
  3. Timestamp rule: unchanged across a no-op build; refreshed by FORCE_REBUILD_TIMESTAMP=1 (21:50:46Z → 21:50:51Z).
  4. Missing registry fails: mv legacy_delegates.toml → exit 101 with reading ../legacy_delegates.toml: No such file or directory; DOCS_RS=1 builds (empty-table fallback, docs.rs only); same hard failure for common/legacy_room_contracts.toml (exit 101).
  5. Worktree layout now caches too: --git-path resolves the per-worktree paths, no-op build is 0.30s with the script not re-running — both layouts behave identically now.

Pin tests (source scans — both build shapes compile identically, so only a pin can hold this):

  • ui/src/util.rs::build_script_declares_registry_and_timestamp_inputs — pins .rerun_if_changed(true), the build.rs self-declaration, FORCE_REBUILD_TIMESTAMP, --git-path (and no literal ../.git/), and the docs.rs-gated leniency. Anchored to non-comment lines only, so a commented-out directive does not satisfy it (Delta's first pin had exactly that hole).
  • common/tests/migration_test.rs::build_script_keeps_registry_fresh_and_missing_registry_fatal — same pins for common/build.rs.

All four meaningful mutations were applied and observed to turn the pins red (.rerun_if_changed(false), commenting out the self-declaration, .allow_missing_registry(true) in each file), then reverted.

Suites: cargo test -p river-ui --bins 895 passed; river-core migration_test 5 passed, room_contract_migration_test 4 passed. fmt/clippy clean.

No WASM changes: no delegate/contract/common source is touched; ui/public/contracts/chat_delegate.wasm is byte-identical (b3 a44c64014d60fd245fe8fb5172f8fd7397039b248b3b6a8e95e89a0bb539fb5e), so no migration entry is needed for this PR.

Scope note: this is a precursor to the freenet-migrate delegate-half adoption; it deliberately does not touch the sweep, the registry contents, or any delegate/contract source.

Risk tier: Full by surface — build/publish configuration on the data-loss-critical migration path.

[AI-assisted - Claude]

…stry fatal

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BEgtjegwuJPWSaAnVJ3z4e
@sanity

sanity commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Fable 5 Full-tier review — no blocking findings. 7 mutations, 7 reds.

Verified empirically in a scratch real clone at this head, plus a git worktree of that clone for the dual-layout checks. That distinction matters here more than anywhere: in a worktree an unresolvable rerun-if-changed makes the script always-dirty, so this entire bug class is invisible from the place we normally stand.

The risk this PR introduces was the main thing to check, and it holds

The old code deliberately emitted no directives so Cargo's default heuristic would keep BUILD_TIMESTAMP_ISO fresh; declaring any rerun-if-changed disables that. If anything consumed per-compile freshness, this trade would be wrong.

It doesn't. Repo-wide grep (plus skills and .claude/ rules) found exactly three consumers, all display-only: a console info! on load, the "Built: …" UI footer, and an operator curl in AGENTS.md:190. No script, CI job or publish task reads either value programmatically — the functional publish gate is the committed monotonic counter in published-contract/contract-version.txt, and river-publish explicitly forbids timestamp-derived versions.

The publish path still stamps correctly, verified rather than reasoned — including a subtlety the PR body didn't spell out: .git/HEAD does not change on a same-branch commit; the index does, and it is declared, which is why commits restamp. Even a message-only --amend re-stamped to the new SHA, and git pull on main rewrites the index, so publish-from-main always builds freshly stamped.

The core fix, in the layout where it actually bit

Editing a description in legacy_delegates.toml in the clone re-ran the script, advanced the generated file's mtime, and the probe string appeared in the baked const. The delta#52 staleness route is closed. Before the fix I measured the same edit producing a 0.26s no-op with all 18 generated mtimes unchanged.

Not always-dirty: a no-op build stays a no-op (~1.8s warm, generated mtime and timestamp byte-identical across runs), so the fix doesn't re-mask the bug class it removes — which is the trap the original comment was trying to avoid.

Both layouts now behave identically. git rev-parse --git-path returns package-root-relative paths in a clone and absolute per-worktree paths in a worktree, both existing. The worktree's permanent always-dirty behaviour is gone (0.29s no-op), while a registry edit there still re-runs the script.

Guards, all made to fail

Mutation Result
ui .rerun_if_changed(true)(false) RED
ui self-declaration println! commented out RED — the Delta pin's hole is closed
ui .allow_missing_registry(…)(true) RED
ui --git-path replaced by literal ../.git/HEAD RED
ui FORCE_REBUILD_TIMESTAMP deleted RED
common .allow_missing_registry(true) RED
common self-declaration commented out RED

Both pins run in CI (build.yml:119 and :189). Separately: empty registry → exit 101 naming "ZERO entries" in both packages; missing registry → exit 101; populated registries build clean with no spurious fire.

DOCS_RS gate — checked for the shape that disarmed a sibling repo today

An env-var-gated bypass on a safety check is exactly how CHECK_MIGRATION_LIB_ONLY disarmed delta's publish gate this morning, so this one got the same scrutiny. It is genuinely scoped: DOCS_RS appears nowhere in .github/, Makefile.toml, scripts/ or the dev environment; it is docs.rs's own canonical variable. Unlike the delta case, the bypass only matters when the registry is missing or empty — with it present the table generates identically either way. Both pins assert the gated form is present and the blanket (true) form is absent, so silently widening it turns CI red.

The reviewer also checked something I hadn't asked: cargo package --list -p river-core includes legacy_room_contracts.toml, so the new hard-fail cannot break cargo install riverctl from crates.io.

WASM untouched: chat_delegate.wasm b3 = a44c64014d60fd245fe8fb5172f8fd7397039b248b3b6a8e95e89a0bb539fb5e, zero .wasm files in the diff. And common/build.rs's "already correct" freshness was confirmed against the crate source (codegen.rs:65-66,264-265) rather than accepted.

NITs — none blocking, all recorded

  1. Block-comment blind spot in the pins. The filter is !line.starts_with("//"), so a directive inside /* … */ would satisfy the pin while being inert. Every realistic disarm is caught and mutation-verified; noted so nobody later "fixes" a pin failure by block-commenting it.
  2. FORCE_REBUILD_TIMESTAMP re-arms on value change, not per-invocation — two consecutive =1 builds, the second is a no-op. Doc nuance.
  3. Watching index makes some pure-git operations restamp (e.g. git checkout -- <file>). Safe direction — over-fresh, never stale — and inherent to the same mechanism that makes commits restamp.

Residual trade, verified acceptable: a dirty-tree rebuild carries the previous stamp while shipping new code, reachable only via a workflow publish-from-main already forbids, and misleading only the display-only curl check.

Bottom line

This fixes a live defect in the app with the most users: River could ship a bundle whose baked LEGACY_DELEGATES omits the outgoing delegate, so the sweep probes nobody and returning users lose their rooms, silently. add-migration.sh edits exactly the file that wasn't being watched.

[AI-assisted - Claude]

@sanity
sanity merged commit f38136f into main Aug 11, 2026
6 checks passed
@sanity
sanity deleted the fix/stale-registry-build branch August 11, 2026 22:14
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