fix: make breadmill default features compile #13
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/road-15-breadmill-default-build"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fixes ROAD-15 and SEARCH-14.
The pinned bread-onnx v0.7.2 provider dispatcher references ORT's MIGraphX, CUDA, OpenVINO, and Vitis APIs unconditionally. breadmill enables those Rust API features in its baseline ORT dependency so the default consumer build compiles without GPU SDKs or provider libraries at build time.
R1: ORT rc.13 makes its download-binaries/copy-dylibs behavior inert with load-dynamic. The build now fetches the official ONNX Runtime 1.28.0 CPU release tarball from its pinned GitHub release URL, verifies the published SHA-256, and embeds the runtime plus provider support library. At startup, breadmill extracts them into its cache unless ORT_DYLIB_PATH explicitly selects a provider-enabled runtime. The CPU runtime is thus available on a fresh install without a system onnxruntime package. License and third-party notices are included in bakery, RC, stable, and GitHub release artifacts.
R2: the daemon now explicitly initializes ORT via its fallible init_from API before constructing a session. Missing or corrupt libraries become an observable model-unavailable state in status and do not panic the loader thread. Regression smoke covers a real pinned model/tokenizer with a fresh HOME/XDG: bundled CPU load, missing and corrupt runtime errors, and CPU fallback when --cuda is selected with a supplied CPU runtime.
ECO-78 is not resolved by this PR. breadmill keeps the ORT provider API features enabled as a consumer-side compatibility workaround for pinned bread-onnx v0.7.2; shared provider feature-gating remains a separate bread-ecosystem prerequisite.
CI retains fail-fast checks and adds the runtime provisioning regression. Locked default, full, and release builds plus existing breadmill tests are covered.
Closes ROAD-15
Closes SEARCH-14
Independent ROAD-15 review: CHANGES REQUESTED
Reviewed head:
295838df42Base:
18cd5295a8Verified in a fresh isolated checkout at ~/.worktrees/breadsearch/fix/review-road15-295838d, without .cargo patches (home Cargo config only git-fetch-with-cli), with ROCM_PATH and ORT_DYLIB_PATH removed for builds:
Blocking regression SEARCH-14:
[ ] R1, breadmill/Cargo.toml:37-46, README.md:20, bakery.toml:4, release/RC artifact loops: preserve the default CPU installation/runtime contract. In actual pinned ort rc.13, load-dynamic enables ort-sys/disable-linking and build/main.rs:25-33 exits before download-binaries/copy-dylibs. Fresh target contains zero libonnxruntime files and no runtime rpath. The ordinary CPU binary now requires an externally supplied runtime despite the explicit no-system-runtime promise and absent package dependency. Provide a real compatible runtime/install solution or revise the dependency/feature design, with a pristine default CPU model-load regression test not satisfied by the review host's installed runtime. Existing full builds already used dlopen, but this PR newly extends that requirement to the ordinary build.
[ ] R2, breadmill/src/main.rs:172-185 and runtime initialization: handle missing/incompatible runtime as a controlled load failure, not an uncaught loader-thread panic. Reproduced with isolated HOME/XDG dirs, roots=[], real model/tokenizer and ORT_DYLIB_PATH pointing to an absent library: ort/src/lib.rs:234 panics, socket stays alive but model never becomes ready. Add executable behavior coverage for absent runtime and missing provider (distinct failure modes). Correct the now-false bundled-runtime/rpath comments alongside the chosen fix.
ROAD-15 still has an explicit blocked-by ECO-78 dependency, which remains backlog with no comments. Reconcile whether the consumer-side solution replaces that prerequisite; it cannot silently count as integrated shared-crate gating. No change to this dependency was made by review.
Build criterion PASS; feature gate and missing-provider fallback checks PASS when CPU runtime supplied; no-runtime default install/runtime preservation FAIL; workflow additions have correct fail-fast/release flags but PR check remains dev-patched. Evidence retained under review-evidence/{default-build.log,full-build.log,release-build.log,tests.log,runtime/*/{log,cpu-runtime.log}}. Preserve PR13/branch, repair same PR, fresh exact-head CI/review. No merge or done. GPU acceleration itself is outside this build repair's scope and unverified. First runtime attempt hit Unix socket path-length limit; rerun used short /tmp/opencode runtime dirs and all owned processes were terminated. Primary and other worktrees untouched. No tags or deployment. Update card to todo for rework.
Rework update: independent review confirmed the original provider-feature compile fix also enabled
ort/load-dynamicin breadmill's baseline ORT declaration. With the locked ORT rc.13 graph, that activates ort-sys/disable-linking, skips download-binaries/copy-dylibs, and removes the documented bundled CPU runtime. The reviewer reproduced a missing-runtime panic in the model-loader thread. Dropping load-dynamic while forcing all four provider features does not work either: no bundled ORT distribution matches that feature set, so link fails. I restored the original dependency declaration in the working tree; it remains an explicit prerequisite blocker (ECO-78 / bread-ecosystem#77) for correctly gated provider dispatch. I will amend this same PR with only a verified supported fix, or report the blocker if the prerequisite cannot be accepted without a separate upstream change. No merge or tag.Independent ROAD-15 review: CHANGES REQUESTED (second review, no repair landed)
Reviewed head:
295838df42Base:
18cd5295a8(equals live main tip, no drift)Head is unchanged since the previous review: still a single commit, no repair amended. Comment 5037's working-tree restore was never pushed. ECO-78 remains backlog with no comments and bread-onnx is still pinned to tag v0.7.2 (30517f16), so the prerequisite is not integrated and is not counted as such.
Verified in a fresh isolated worktree at ~/.worktrees/breadsearch/fix/muse-review-pr13-295838d (detached at
295838d), env with ORT_DYLIB_PATH and ROCM_PATH removed for builds:295838d) status success. Reviews API returns []. No branch protection observable. PR open and mergeable. Note check.yml resolves dev dependencies to main and builds without --locked, so its success does not prove pinned-graph behavior; the locked builds above are the evidence for that.Blocking, reproduced again at this exact head:
[ ] R1, breadmill/Cargo.toml:43-48, README.md:20, bakery.toml:4, release/RC artifact loops: the default build still provisions no CPU runtime. Fresh debug and release targets contain zero libonnxruntime files; neither binary has an ORT NEEDED entry or RPATH/RUNPATH (readelf). In pinned ort/ort-sys rc.13, load-dynamic implies ort-sys/disable-linking and the build script returns early, so download-binaries and copy-dylibs are inert despite being listed. The README bundled-runtime promise and the bare-binary release artifacts are unchanged, so a fresh install's ordinary CPU path has no runtime. Either provision a real compatible runtime/install path or revise the dependency and feature design, with a pristine default CPU model-load regression test that passes without the review host's libraries.
[ ] R2, breadmill/src/main.rs:170-180 and runtime init: absent runtime is still an uncaught loader-thread panic, not a controlled error. Reproduced with isolated HOME/XDG dirs, real model/tokenizer, and ORT_DYLIB_PATH pointing at an absent file: thread panics at ort 2.0.0-rc.13 src/lib.rs:234 (expect "Failed to load ONNX Runtime dylib"), the daemon stays alive on its socket, and status reports model ready: false permanently. The match on OrtEmbedder::load cannot catch a panic from inside ort setup. Handle load failure as a controlled error and correct the now-false bundled-runtime/rpath comments in breadmill/Cargo.toml:33-36.
Evidence retained under review-evidence/{default-build.log,full-build.log,release-build.log,tests.log,cpu-runtime.log,cuda-fallback.log,openvino-fallback.log,no-runtime.log} in the worktree above. No merge performed (verdict is not pass). Primary checkout untouched. No tags or deployment. Scratch runtime dirs removed; all owned breadmill processes terminated (the installed breadmill serve daemon was never touched).
R1/R2 repair pushed at
354343c58e.R1: breadmill now provisions the official ONNX Runtime 1.28.0 CPU runtime from the pinned GitHub release archive, verifies the published SHA-256 a3e1b79d7bb1bf09696ce675f49e4064e6c81f6202b8225624fff0e93f8d6407, and embeds libonnxruntime.so.1.28.0 plus libonnxruntime_providers_shared.so in the binary. First model load installs them into the user cache. ORT_DYLIB_PATH remains the explicit override for a provider-enabled runtime. Bakery, dev, RC, stable, and GitHub stable artifacts include ONNX Runtime license and third-party notices. The README now describes the actual runtime choice.
R2: breadmill explicitly calls ort::init_from before session construction. Missing/corrupt runtimes become model_error in status with model_ready=false; loader thread stays alive. Regression smoke uses a fresh HOME/XDG and real pinned model/tokenizer (model SHA-256 147d5aa88c2101237358e17796cf3a227cead1ec304ec34b465bb08e9d952965; tokenizer SHA-256 d241a60d5e8f04cc1b2b3e9ef7a4921b27bf526d9f6050ab90f9267a1f9e5c66).
Old-fails/new-passes proof, same isolated missing-library condition: reviewed head
295838dlogs thread panic at ort rc.13 src/lib.rs:234 and status remains model_ready=false without a model_error. New head returns status model_ready=false with ONNX Runtime library not found at .../missing.so, logs model unavailable, and has no panic. Smoke also passes bundled CPU load, corrupt-runtime controlled error, and CPU fallback with --cuda against a supplied CPU ORT.Checks passed: locked default debug build, locked full debug build, locked default release build, locked release workspace build and tests (4 breadmill tests plus workspace tests), locked breadmill/shared test (4 breadmill tests), clippy --locked --workspace --all-targets --features full -- -D warnings, runtime smoke. cargo fmt --check reports broad existing formatting differences across unrelated files, so no repo-wide formatting churn was applied. ORT bundles only for x86_64 Linux; build.rs fails clearly on other target triples.
ECO-78 remains separate and unresolved: pinned bread-onnx v0.7.2 references provider APIs unconditionally. This PR carries the needed API features as a documented consumer workaround, not as a shared-crate fix. No prerequisite PR/tag was fabricated. Not merged or marked done.
Independent exact-head review: CHANGES REQUESTED
Head
354343c58e; base main18cd5295a8. All 15 files and complete diff reviewed, PR/file pagination exhausted. ECO-78 explicitly unresolved and out of scope; provider API features are a consumer workaround.PASS evidence in ~/.worktrees/breadsearch/fix/astra-review-pr13-354343c/review-evidence:
Required repairs, filed SEARCH-18:
[ ] R1 breadmill/src/embed.rs:97-105: make cache owned/private with safe modes, reject unsafe symlink/nonregular entries and bound reads. Actual isolated binary with umask 000 installs runtime directory 0777 and libraries 0666. Precreated FIFO at runtime/libonnxruntime.so.1.28.0 blocks fs::read: observed status model_ready=false/model_error=null after 4 seconds; source has no timeout. Oversized files also read unbounded. Add controlled-error regression coverage.
[ ] R2 breadmill/src/embed.rs:117-123: replace exists/unlink/rename with safe serialized atomic publication. Concurrent installers can race exists/remove (ENOENT); another loader can hit missing file between unlink and rename. This is a code-proven race, not a claimed timing reproduction. Test concurrent first install and repair.
[ ] R3 README.md:164-168 and breadmill/Cargo.toml:24-26 still promise system dynamic-linker discovery without ORT_DYLIB_PATH, but new code always chooses bundled CPU when unset. Correct and document x86_64 Linux limit.
Packaging: official LICENSE/ThirdPartyNotices present in build output; dev/RC/stable copy both and GitHub stable upload lists both. Runtime embedded, executable-only CPU provisioning smoke passes. Bakery end-to-end notice installation not independently exercised; license_file names only LICENSE, so verify ThirdPartyNotices delivery. No tagged release/deployment claim.
Unsafe-cache evidence retained /tmp/opencode/pr13-fmd0vs8q; owned daemons terminated, primary/live daemon untouched. ROAD-15 build passes, but combined PR runtime safety/docs fail. SEARCH-14 ordinary provisioning/error paths pass, bounded safe concurrent cache fails. Preserve PR13/branch, repair and request fresh review. No merge or done. Cards return to todo for rework under review policy. Head/base freshly rechecked unchanged and mergeable; green CI does not waive these defects.
Review 5386 repairs pushed at
71648b4903.R1 cache safety: runtime cache directory is created/secured as 0700; runtime libraries are installed as 0644 via unique same-directory temporary files, synced, and atomically renamed under a persistent advisory flock. Cache entries are inspected without following symlinks; runtime and provider entries must be regular files. Reads use O_NOFOLLOW/O_NONBLOCK, verify descriptor type and bound content read by expected size. A FIFO cache runtime now returns model_error promptly instead of blocking.
R2 concurrency: publication is serialized with flock. Eight simultaneous installer threads leave exact embedded runtime/provider bytes and safe modes. The smoke also launches two independent breadmill processes against one fresh shared cache with distinct sockets and umask 000; both reach model loaded.
R3 docs: Cargo feature comments and README GPU notes now say runtime selection is ORT_DYLIB_PATH or the bundled CPU cache only; system dynamic-linker discovery is not promised.
Old-fails/new-passes evidence on fixture-only/temp XDG state: old
354343cunder umask 000 installed cache mode 0755 and runtime mode 0666; new smoke asserts 0700 and 0644. Old FIFO-cache daemon returned model_ready=false, model_error=null after 2 seconds and remained blocked; new daemon promptly reports model_errorONNX Runtime cache entry is not a regular file. Old isolated two-installer unit test failed withcannot create ONNX Runtime staging directory: File exists; new eight-thread test passes and verifies complete byte-exact files. Primary checkout and live services untouched.Checks passed on this head: locked default build, locked full-feature build, locked release build, locked workspace tests with full features (6 breadmill tests including FIFO and concurrency, 1 GUI test), clippy --locked --workspace --all-targets --features full -- -D warnings, shell syntax/diff checks, and real cached-model runtime smoke for bundled CPU, missing/corrupt overrides, supplied CPU override plus --cuda, FIFO, umask modes, and two-process concurrent startup. No model download was needed.
Bakery notices: release/dev/RC workflows now stage
onnxruntime-notices.tar.gz; bakery.toml declares it as data_archive. Verified in a disposable artifact fixture that the shared index generator records the archive plus matching SHA-256; the archive contains ThirdPartyNotices.txt. Bakery's Rust installer implements verified license copy and verified data archive extraction, and its existing archive tests pass locally. Actual end-to-endbakery installinto a user prefix was not run, because even with --bin-dir its data install targets the resolved user share directory and service hooks can have effects. Therefore production Bakery notice installation remains unverified.ECO-78 remains out of scope. No merge, tag, or done transition.
Independent exact-head re-review of PR13: PASS
Reviewed head
71648b4903(single-parent chain295838d->354343c->71648b4), base main18cd5295a8. All 15 changed files and the complete diff re-read; PR and file pagination exhausted (page 2 empty). Prior changes_requested 5386 read first and every repair verified against this exact head. ECO-78 remains backlog with no comments and stays out of scope.Evidence retained in /tmp/opencode/pr13-71648b4/evidence (README-evidence.md plus logs and raw harness output). Isolated worktrees: ~/.worktrees/breadsearch/review-pr13-71648b4 (
71648b4), review-pr13-old-354343c (354343c), review-old-repro (354343cwith a review-only, uncommitted test module). Owned daemons terminated; primary checkout and the live service untouched.R1 non-regular cache entries, old-fails/new-passes:
354343c, precreated FIFO at cache/breadsearch/runtime/libonnxruntime.so.1.28.0 (umask 000): status model_ready=false with model_error null and still blocked after 8s, log stops at "loading model..." (old-runtime.log).71648b4, same fixture: matched model_error "ONNX Runtime cache entry is not a regular file: " in 0.1s, no panic, daemon keeps serving (new-runtime.log).R2 umask independence, old-fails/new-passes:
354343c, umask 000, fresh cache: directory 0777, both libraries 0666 (old-runtime.log).71648b4, umask 000, fresh cache: directory 0700, both libraries 0644, lock 0600 (new-runtime.log); also 0700/0644 under umask 077, and pre-existing 0777/0666 entries left by the old head were repaired to 0700/0644 on the next start (new-runtime4.log).R3 atomic serialized publication:
354343cworktree, uncommitted): 7 of 8 threads fail with "cannot create ONNX Runtime staging directory: File exists (os error 17)" (old-concurrency-repro.log).R3 docs: README.md:20,49-63,164-167 and breadmill/Cargo.toml:19-26 now describe only ORT_DYLIB_PATH or the bundled CPU cache, and state explicitly that no system runtime is discovered through the dynamic linker. Repo-wide search finds no remaining implicit-discovery claim.
Earlier passes re-confirmed at this head: independently downloaded official onnxruntime-linux-x64-1.28.0.tgz SHA-256 a3e1b79d7bb1bf09696ce675f49e4064e6c81f6202b8225624fff0e93f8d6407 equals build.rs ORT_SHA256, and both installed cache libraries are byte-identical to the extracted official files (1461ef7c..., 086ec1d5...); a corrupted tarball fails the build script with exit 101 checksum mismatch before extraction, and a good tarball rebuilds; ORT_DYLIB_PATH pointing at an absent or corrupt library yields a controlled model_error ("library unavailable" / "dlopen failed") with no panic and no hidden fallback to the bundled runtime.
Locked builds and checks on this host without .cargo patches: cargo build --locked -p breadmill PASS; --features breadmill/full PASS; cargo build --release --locked -p breadmill PASS (the new release-step build); cargo test --locked -p breadmill -p breadsearch-shared PASS, 6 tests including both new runtime-cache tests; cargo clippy --locked -p breadmill -p breadsearch-shared --all-targets -- -D warnings PASS. Cargo.lock unchanged by the build, both worktrees clean.
Real-model runtime smoke (cached nomic model and tokenizer, fresh HOME/XDG, roots=[]): implementer runtime_smoke.sh with the built binary exits 0 (bundled CPU ready, missing and corrupt overrides controlled, supplied CPU ORT with --cuda ready, FIFO rejected, private modes, two-process concurrent install). Debug and release binaries both load the bundled runtime on a fresh cache in under a second and both reject a FIFO cache entry promptly.
Exact head CI: /commits/71648b49.../status state success, single status "check / check (push)", run 71, successful in 13m34s. No outstanding reviews or inline review comments (reviews [] and review_comments 0). PR open and mergeable, base
18cd5295unchanged. check.yml intentionally resolves dev dependencies to main and is not --locked; the locked builds above are the pinned-graph evidence.Non-blocking observations, recorded rather than waived: the x86_64-Linux-only limit is enforced by build.rs:25-29 with a clear panic but is not stated in README; main.rs:181 has an odd extra indentation and the repo has broad pre-existing rustfmt drift, which CI does not gate on; temp files left by a hard kill before rename are not garbage-collected (0600 inside a 0700 directory, replaced on the next successful install).
Verdict: pass, all requested repairs verified at this exact head. Merging this head to main is authorized by the coordinator; cards ROAD-15 and SEARCH-14 stay in review for coordinator verification and are not marked done.
Integration result: merged to main.
Merge commit
8d620245a3on main, parents18cd5295a8(old main) and71648b4903(the exact reviewed head). The merge tree is identical to the reviewed head tree. Fresh pre-merge reads: head and base unchanged, required check / check (push) success on the exact head, reviews empty, mergeable true. The merge API has no expected-head parameter, so the residual race was bounded by re-reading the branch tip (git ls-remote) immediately before merging and verifying the resulting parents afterwards.Head branch fix/road-15-breadmill-default-build is preserved (not deleted); no open PR targets it. Review evidence remains in /tmp/opencode/pr13-71648b4/evidence.
ROAD-15 and SEARCH-14 move to review for coordinator verification of the integrated behavior; neither is marked done. Merging main also triggers the dev-track release build, which is the coordinator's to confirm.