Repository navigation
Trust a cached Lean build only when the run that wrote it finished (fixes #119) - #151
jiro-by-nori[bot] wants to merge 1 commit into
Conversation
…ssue #119) CI caches `ruby-lean/.lake` and `books/.lake`, and the proofs job saves them under `if: always()`, so a run that the concurrency group cancels part-way through `make books` still uploads a tree. Lake trusts the build trace it restores over the contents of the file, so a `.olean` the killed compiler left half written is taken as current and the next run dies ~410 modules into the books with `invalid header` — blaming a file the change did not touch, and re-uploading the damage on every failed rerun. Each tree now carries a `COMPLETE` mark. Every target that writes a tree removes it first and writes it back after the build, unless the build was killed: a killed build exits with 128 or more (130 for SIGINT, 143 for SIGTERM), where a failed build of these packages exits with 1. An unmarked tree is discarded instead of trusted, so one run's modules are rebuilt rather than a truncated file being restored by every run the key matches. A build that ends on its own marks the tree even when it reports an error, because it finished writing every `.olean` it started: the failure mode is a killed writer, not a failing one, and a proof that stops on a Lean error leaves a tree worth caching. Only CI discards, because only CI restores a tree from outside the checkout; a local build that was interrupted stays incremental. `lake-cache-check` is a prerequisite of every target that builds, so the mark is checked before the first command that reads a tree — including `genbootedheap`, which builds the model from an order-only prerequisite before any recipe runs. The workflow is unchanged, so a cancelled run still uploads its tree; it can no longer be restored as if it were complete, and the `-partial-` entries already in the store are discarded the same way. Suppressing the upload too needs the `workflows` permission, which is a change to `.github/workflows/ci.yml` and a separate pull request. Validated on this machine: * The failure, reproduced twice: truncating `books/.../Checker/Static/NativeInstanceNames.olean` and `ruby-lean/.../RubyCore/Syntax.olean` and forcing a rebuild gave the issue's error — `failed to read file '…Syntax.olean', invalid header` — the second from `genbootedheap`, before any recipe ran. * `CI=1 make books` with an unmarked tree discarded both trees and rebuilt them (108, 207 and 1405 jobs), exit 0; with a marked tree it discarded nothing and replayed in 55s. * A build that fails on a Lean error (`theorem scratch_broken : (1 : Nat) = 2`) left the tree marked, and the next CI run did not discard it. * A run killed with SIGTERM (make exit 143) left the tree unmarked. * `make lean`, `make books` (1405 jobs), `make metatheory`, `make gen-check`, `make ci-sync`, `make docs`, `scripts/check-not-weakened.py --base origin/main`. Not run: `make lint` (RuboCop needs the pinned gems, which need a C toolchain this machine does not have; the change is a Makefile and a Markdown section), `make soundness`, `make comparator`, `make conformance` (their prerequisites include `make deps`).
There was a problem hiding this comment.
Not approving: this pull request touches a human-gated path — Makefile — so it needs @sam-xif, per the approver policy. Everything I could check without that decision is in order, and the review below is context for the human, not a verdict on the gate.
Reviewed head 59d865a880368ba79b58021ab170144eeced7da0. 2 files, 72 changed lines.
Checks observed at this head: all eight green — CI, checks not weakened, lint, docs, front-end (the desugarer against CRuby), conformance (the model against CRuby), proofs (build, every book, axioms, soundness, comparator). No unresolved review threads.
What it does (issue #119): a COMPLETE mark in each Lake tree. Every target that writes ruby-lean/.lake or books/.lake deletes the mark first and rewrites it after the build unless the build was killed (status >= 128), and lake-cache-check — a prerequisite of lean-exes and soundness, and invoked by lake_build — discards an unmarked tree in CI before the first command that reads it.
Points I would want a human to weigh, since I cannot decide them for you:
- The exit-code heuristic.
< 128means "finished";>= 128means "killed", on the reading that signal death is 130/143 while a failed build of these packages exits 1. A genuine failure that reports 128+ would cost one run's rebuild, not a wrong answer — the failure direction is safe. A killed build that exits below 128 would re-trust a half-written tree, which is the bug being fixed;trap/timeoutpaths that re-exit are the case to think about. - A tree is marked after a failed build. That is deliberate and documented ("it finished writing every
.oleanit started"), but it means the mark asserts "not killed", not "green" — worth confirming that is the property you want the cache to carry. - CI needed no workflow change.
.github/workflows/ci.yml:88-94already saves both trees withif: always()under a-partial-<run>-<attempt>key, so an unmarked tree can be saved and restored; the Makefile is what refuses to trust it.ci-sync(scripts/check-ci-matches-make.sh) passes in CI, so the target list and the workflow still agree. - Local behaviour is unchanged by design —
lake_checkis gated on$CI, so an interrupted local build stays incremental;CI=1 make booksis the way to exercise the discard path.
I did not run anything: this runner has no ruby, lean or make, so the Makefile behaviour is taken from CI at this head, not re-executed by me. The CI=1 make books discard path in particular is not covered by a test in the diff — it is exercised only by a cancelled run.
@sam-xif this is the human gate: Makefile is on the approver's human-gated path list, so I will not approve it. If you are happy with the heuristic and the mark semantics, approve and merge it yourself.
Fixes #119, the half of it that this repository can fix without the
workflowspermission.
The defect
The
proofsjob savesruby-lean/.lakeandbooks/.lakeunderif: always().A run cancelled by the
ci-<ref>concurrency group part-way throughmake bookstherefore uploads a tree holding a
.oleanits compiler was killed in the middleof writing, and Lake trusts the build trace it restores over the contents of the
file. The next run reports
~410 modules into the books, 10 minutes in, blaming a file the change did not
touch — and every failed rerun re-uploads the truncated tree, so the obvious
response reproduces it.
The change
Each build tree now carries a
COMPLETEmark, and only a marked tree istrusted:
lake_buildremoves the mark, runs the build, and writes the mark back unlessthe build was killed. A killed build exits with 128 or more (130 SIGINT, 143
SIGTERM); a failed build of these packages exits with 1. So the mark means
"the run that wrote this tree finished writing every
.oleanit started" —which is also why a build that stops on a Lean error keeps the mark: a broken
proof must not throw away a good tree for every other pull request.
lake_checkdiscards an unmarked tree instead of restoring it — the samestate as a cache miss, so Lake rebuilds what the sources ask for.
lake-cache-checkruns both checks as a prerequisite of every target thatbuilds, so the mark is decided before the first command that reads a tree.
That includes
genbootedheap: it builds the model from a prerequisite oflean-exes, before any recipe runs, and it hits the truncated file first.Only CI discards (
CI=1 make booksexercises it locally). A local tree is onlywritten by the developer's own runs, so an interrupted build stays incremental.
What this gives up: a cancelled run still uploads its tree, so one rebuild is
paid by whichever run restores it next; no run can restore a truncated file as
if it were complete, and the
-partial-entries already in the store arediscarded the same way. Stopping the upload as well is a two-line change to
.github/workflows/ci.yml— save onsuccess()and bump the key — and needs asession with the
workflowspermission (!admin). Filed separately; this pullrequest does not touch the workflow, so it does not need that permission.
Files
MakefileCOMPLETEmark:lake_build,lake_check,lake-cache-check, and the targets that write a tree (lean-exes,lean,books,metatheory,soundness,comparator)docs/contributing.mdMakefileanddocs/are owner-review paths (.github/CODEOWNERS); this pullrequest touches nothing else.
Test plan and what was run
Run on this machine with Lean
v4.32.2, CRuby is not involved (no Ruby sourcechanges).
Truncated
books/.lake/build/lib/lean/Books/TypeSoundness/Checker/Static/NativeInstanceNames.oleanto 32 bytes and forced its importer to rebuild:
error: Books/TypeSoundness/Conformance/Names/NativePrefix.lean:1:0: failed to read file '…NativeInstanceNames.olean', invalid header.Same with
ruby-lean/.lake/build/lib/lean/RubyCore/Syntax.olean, which failedin the
genbootedheapprerequisite — before any recipe, which is whylake-cache-checkis a prerequisite and not a recipe line.CI=1 make bookswith markers removed:discarding ruby-lean/.lake: the run that wrote it did not finish, then afull rebuild (108, 207 and 1405 jobs), exit 0. This is the first run after
this lands, so the first CI run pays one cold books build.
CI=1 make bookswith markers present discardednothing and replayed in 55s.
theorem scratch_broken : (1 : Nat) = 2appended to
Books/Metatheory/Examples/Demo.lean,CI=1 make booksfailedwith the Lean error and left
books/.lake/COMPLETEin place.CI=1 make -f Makefile -f extra.mk slow-test(
$(call lake_build,books,sleep 20)), SIGTERM at 3s: make exited 143 andbooks/.lake/COMPLETEwas absent.make lean,make books(1405 jobs),make metatheory(every audited theorem on propext, Classical.choice andQuot.sound only; the booted-heap probes pass),
make gen-check(all fourgenerated files fresh),
make ci-sync,make docs(mkdocs--strict), andscripts/check-not-weakened.py --base origin/main→CHECKS NOT WEAKENED.Not run here, and why:
make lint's RuboCop step (the pinned gems need a Ctoolchain this machine does not have; the change is a Makefile and a Markdown
section, and CI lints both), and
make soundness,make comparator,make conformance(their prerequisites includemake deps, i.e. the samegems). CI runs all of them.
Authored by Jiro, an autonomous agent: the code and this description were
written by an agent, not by a person.