From 1fa8bf353a787cfcfd111a659d223a7568600910 Mon Sep 17 00:00:00 2001 From: Archith Date: Fri, 24 Apr 2026 15:44:33 +0200 Subject: [PATCH 1/2] chore(adr): record raft-engine fork for lz4-sys feature-gate (W5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR 0002 §W5 previously assumed upstream raft-engine's lz4-sys C FFI dependency could be mitigated purely at the data path - by disabling raft-engine's own compression and doing it above the engine in mango-raft using lz4_flex. That remains true for the data path, but the build-graph concern - upstream's unconditional lz4-sys dependency pulling the C toolchain into mango's supply chain - was unsolved by the data-path mitigation alone. This commit records the build-time fix: 1. ADR 0002 §W5 expanded to document the active fork (humancto/raft-engine at SHA e1d738d), the upstream PR that tracks its retirement (tikv/raft-engine#397), and the explicit consumer contract (default-features = false, feature list, and the Config::sanitize reject enforcing batch-compression-threshold = 0). 2. New file .planning/fork-raft-engine-lz4-verification.md records the full fork state: SHA pins, PR URL, commit footprint, how mango consumes the fork, rebase policy while the fork lives, retirement plan when #397 merges, and supply-chain audit posture. The fork itself was adversarially reviewed by rust-expert across two rounds (REVISE -> APPROVE_WITH_NITS). Upstream PR #397 is open. No mango Cargo.toml changes here - the workspace dep entry lands with PR-1 (mango-storage skeleton) which pins the fork SHA. --- .planning/adr/0002-storage-engine.md | 16 ++- .../fork-raft-engine-lz4-verification.md | 124 ++++++++++++++++++ 2 files changed, 134 insertions(+), 6 deletions(-) create mode 100644 .planning/fork-raft-engine-lz4-verification.md diff --git a/.planning/adr/0002-storage-engine.md b/.planning/adr/0002-storage-engine.md index 9d48c93..42746ae 100644 --- a/.planning/adr/0002-storage-engine.md +++ b/.planning/adr/0002-storage-engine.md @@ -139,12 +139,16 @@ Every verified wart is engaged explicitly. "This is fine" is not an answer. ### W5 — raft-engine pulls `lz4-sys` C FFI by default (verification B6) -- **How bad:** inconsistent with ROADMAP.md:485 which specifies pure-Rust `lz4_flex` as the workspace default. -- **Mitigation:** **disable raft-engine's compression** in its config and do compression above raft-engine in `mango-raft` using `lz4_flex`. Net effects: - - Zero new C FFI. - - Compression policy moves to the mango layer (where we want it for tuning). - - "Pure Rust" commercial positioning preserved. -- **Escape hatch:** if above-the-engine compression loses meaningfully to raft-engine's per-batch internal compression (measure in Phase 5), revisit with benchmarks. ~1-day swap if we accept the C dep. +- **How bad:** inconsistent with ROADMAP.md:485 which specifies pure-Rust `lz4_flex` as the workspace default. Upstream `tikv/raft-engine` master had `lz4-sys = "=1.9.5"` as an unconditional dependency — consumers could not opt out at build time. +- **Mitigation (build-time, primary):** we run against a patched fork, [`humancto/raft-engine`](https://github.com/humancto/raft-engine) branch `feat/feature-gate-lz4-sys` at SHA `e1d738d9ad1c1fc4f5b21c8c73bf605b5696f535`, which gates `lz4-sys` behind a new `lz4-compression` Cargo feature (default-on upstream; **we consume with `default-features = false` plus the features we need excluding `lz4-compression`**). Under this configuration `cargo tree --edges no-dev | grep lz4` is empty — the C FFI crate is fully absent from mango's build graph. +- **Mitigation (data-path, secondary):** compression policy stays above raft-engine in `mango-raft` using `lz4_flex`. This was the W5 plan before the fork and remains correct regardless of the build-time gate: keeping compression at the mango layer keeps the tuning knobs in our hands. The `Config::sanitize` reject in the patched fork (`batch-compression-threshold = 0` is the only legal value when the feature is off) enforces this at the config boundary — a misconfigured mango build fails loudly at startup, not silently on the first ≥8KB log batch. +- **Upstream PR:** [`tikv/raft-engine#397`](https://github.com/tikv/raft-engine/pull/397) tracks upstream adoption of the feature gate. Fork retires (mango pins shift from `humancto/raft-engine` to `tikv/raft-engine` at the merged SHA) when #397 lands. Tracking file: `.planning/fork-raft-engine-lz4-verification.md`. +- **Net effects:** + - Zero C FFI in mango's build graph. + - Pure-Rust positioning preserved end-to-end (not just at the data path). + - Compression policy tuning owned by mango. + - Small ongoing cost: fork maintenance until the upstream PR merges; integration test must re-run on every fork rebase. +- **Escape hatch:** if above-the-engine compression loses meaningfully to raft-engine's per-batch internal compression (measure in Phase 5), revisit with benchmarks — re-enabling `lz4-compression` in the fork Cargo features would restore the C dep but keep the rest of the fork intact. ~1-day flip. ### W6 — raft-engine has 49 `unsafe` tokens on master (verification B7) diff --git a/.planning/fork-raft-engine-lz4-verification.md b/.planning/fork-raft-engine-lz4-verification.md new file mode 100644 index 0000000..7e924dc --- /dev/null +++ b/.planning/fork-raft-engine-lz4-verification.md @@ -0,0 +1,124 @@ +# Fork tracking: `humancto/raft-engine` (lz4-sys feature gate) + +This file records the state of mango's temporary fork of +[`tikv/raft-engine`](https://github.com/tikv/raft-engine) and the plan +to retire the fork once upstream adopts the change. + +## Why the fork exists + +`tikv/raft-engine` at master SHA `f0fcebe922c384fcb673576f1f5b638203550ee7` +had `lz4-sys = "=1.9.5"` as an unconditional `[dependencies]` entry. +That C FFI crate is incompatible with mango's pure-Rust north-star +(ROADMAP.md "What Rust gives us that Go etcd cannot" — no C in the +default build graph). Upstream had no Cargo feature to opt out. + +See ADR 0002 §W5 (`.planning/adr/0002-storage-engine.md`) for the +full rationale. + +## Current state + +| Field | Value | +| --------------------------- | ---------------------------------------------------------------------- | +| Fork repo | [`humancto/raft-engine`](https://github.com/humancto/raft-engine) | +| Fork branch | `feat/feature-gate-lz4-sys` | +| Fork SHA (pinned by mango) | `e1d738d9ad1c1fc4f5b21c8c73bf605b5696f535` | +| Upstream base SHA | `f0fcebe922c384fcb673576f1f5b638203550ee7` | +| Upstream PR | [`tikv/raft-engine#397`](https://github.com/tikv/raft-engine/pull/397) | +| Upstream PR state | Open (filed 2026-04-24) | +| Review status on fork patch | `rust-expert` APPROVE_WITH_NITS (two rounds) | + +## What the fork changes + +Two commits on top of upstream master: + +1. `4e8a088 feat(features): make lz4-sys optional behind lz4-compression feature` + - `Cargo.toml`: `lz4-sys` → `optional = true`; new feature + `lz4-compression = ["dep:lz4-sys"]`; `default = [..., "lz4-compression"]` + preserves BC. + - `src/util.rs`: `pub mod lz4` split into two `#[cfg]`-gated variants + with identical public surface. +2. `e1d738d fix(lz4-feature): address rust-expert REVISE findings` + - `Config::sanitize` rejects `batch_compression_threshold > 0` when + feature is off. + - Stub `decompress_block` returns `Error::Other` (not `Corruption`). + - `DEFAULT_LZ4_COMPRESSION_LEVEL` hoisted to a parent-scope `pub const`. + - Rustdoc note on `batch_compression_threshold`. + - `CHANGELOG.md` + `README.md` entries. + - New `cargo build --no-default-features --verbose` step in CI. + - New unit test `test_sanitize_rejects_nonzero_threshold_without_lz4_feature`. + +Total footprint: 6 files changed, +93 / -9. + +## How mango consumes the fork + +In `Cargo.toml` at the workspace root: + +```toml +[workspace.dependencies] +raft-engine = { git = "https://github.com/humancto/raft-engine", rev = "e1d738d9ad1c1fc4f5b21c8c73bf605b5696f535", default-features = false, features = ["internals", "scripting"] } +``` + +- `default-features = false` strips `lz4-compression` from the default list. +- `features = ["internals", "scripting"]` keeps the two other default + features `raft-engine` ships with `default = ["internals", "scripting", "lz4-compression"]`. +- `lz4-compression` is deliberately excluded. Mango configs must set + `batch-compression-threshold = 0` (enforced by the fork's + `Config::sanitize`); compression happens above raft-engine in + `mango-raft` via `lz4_flex`. + +Verified absence of lz4-sys: + +``` +$ cargo tree --edges no-dev | grep lz4 +(empty) +``` + +## Retirement plan + +Fork retires when upstream PR #397 merges. + +Retirement steps (in order): + +1. Watch `tikv/raft-engine#397`. When it merges, note the upstream + merge SHA. +2. Run `cargo update -p raft-engine` in a mango branch, repoint the + workspace dep to `tikv/raft-engine` at the merged SHA: + ```toml + raft-engine = { git = "https://github.com/tikv/raft-engine", rev = "", default-features = false, features = ["internals", "scripting"] } + ``` +3. Run the full mango test suite (`cargo nextest run --workspace`). +4. If tests pass, open a mango PR with the repointing, get + `rust-expert` APPROVE, merge. +5. Update this file: mark `Upstream PR state` as merged, blank out + "Fork SHA", move everything below the dividing line to a + "Historical" section. +6. Archive `humancto/raft-engine` repo (GitHub → Settings → Danger + Zone → Archive) so it cannot drift. + +## Rebase policy while the fork lives + +If mango needs a newer upstream SHA before #397 merges: + +1. `cd ~/Desktop/claude-projects/raft-engine` +2. `git fetch upstream && git rebase upstream/master feat/feature-gate-lz4-sys` +3. Resolve conflicts in `src/util.rs` / `src/config.rs` if upstream + touched them. If the upstream churn is material, re-run + `rust-expert` on the rebased diff before pushing. +4. `git push --force-with-lease origin feat/feature-gate-lz4-sys` +5. Update the fork SHA in this file and in mango's workspace + `Cargo.toml`. Both must move together. + +## Supply-chain audit posture + +The fork is public, pinned by SHA (not tag, not branch), and has zero +`unsafe` additions on top of upstream. `cargo vet` handles the fork +the same way it handles any git-pinned crate — the exemption is on +`raft-engine` regardless of origin. When retiring the fork, the +exemption continues to apply against the new upstream SHA without +edits (same crate name, same version `0.4.2`). + +## Last updated + +2026-04-24 (fork created, PR #397 opened, mango still on skeleton +phase — dep not yet wired into a workspace `Cargo.toml`; that lands +with PR-1). From 64fb136e27bcc29cb15add6607701b5b6887588b Mon Sep 17 00:00:00 2001 From: Archith Date: Fri, 24 Apr 2026 15:51:30 +0200 Subject: [PATCH 2/2] docs(fork-tracking): address rust-expert APPROVE_WITH_NITS must-fix items MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rust-expert's review of PR #48 returned APPROVE_WITH_NITS with two must-fix items on .planning/fork-raft-engine-lz4-verification.md. Both are text-only and localized; applying them here to close out the review without another round. 1. Retirement branches for non-merge endings. Original text assumed upstream PR #397 merges. Added a new "What if upstream rejects the approach or stalls" section with two branches: (a) #397 closed without merge, upstream proposes a different shape - swap the tracked PR, follow the same 6-step retirement once the replacement merges. (b) #397 stalls for 12+ months - escalate to ADR 0002 §W4 escape hatch (b) and vendor the pinned fork SHA under crates/vendored-raft-engine/. 2. Supply-chain audit posture corrected. Original paragraph conflated cargo-vet's exemption-identity behavior with SHA pinning and review-by annotations. Rewritten to spell out the actual semantics: - Exemptions key on (crate_name, crate_version); the fork and upstream both ship package.version = "0.4.2", so one exemption line covers both while the fork is active and after retirement. - Patch-version bumps (raft-engine historically bumps on master without publishing) break matching and need a new version line. - SHA swaps are silent to vet - no re-exemption needed on rebase. - review-by is a mango convention, not a vet behavior; dates need manual refresh on rebase and on retirement. Per rust-expert's explicit guidance, no further review round is needed for these text-only edits. --- .../fork-raft-engine-lz4-verification.md | 70 +++++++++++++++++-- 1 file changed, 64 insertions(+), 6 deletions(-) diff --git a/.planning/fork-raft-engine-lz4-verification.md b/.planning/fork-raft-engine-lz4-verification.md index 7e924dc..3ffd62f 100644 --- a/.planning/fork-raft-engine-lz4-verification.md +++ b/.planning/fork-raft-engine-lz4-verification.md @@ -95,6 +95,45 @@ Retirement steps (in order): 6. Archive `humancto/raft-engine` repo (GitHub → Settings → Danger Zone → Archive) so it cannot drift. +## What if upstream rejects the approach or stalls + +The retirement plan above assumes PR #397 merges. Two other +endings need named handling: + +### (a) #397 closed without merge, upstream proposes a different shape + +If TiKV maintainers reject the Cargo-feature approach in favor of +a different upstream shape (e.g. a runtime `Config::enable_lz4` +knob, or a separate `raft-engine-core` crate split): + +1. Engage upstream on the accepted shape; open a fresh fork branch + built from that direction. +2. Update this file: replace `Fork branch`, `Fork SHA`, and + `Upstream PR` rows to point at the new tracking PR. Mark the + old #397 state as "Closed, superseded by #NNN" in a "Historical" + section at the bottom. +3. Retirement now keys on the replacement PR merging, following + the same six steps as the primary retirement plan. + +### (b) #397 stalls for 12+ months with no maintainer engagement + +Matches ADR 0002 §W4's "pre-1.0 / stale on crates.io" escape +hatch. If the fork's only deviation from upstream is still the +feature gate and no mango code depends on any internals that +upstream hasn't addressed: + +1. Escalate to §W4 escape hatch (b): vendor the pinned fork SHA + under `crates/vendored-raft-engine/` with a CODEOWNERS stanza + forcing maintainer review. +2. Mango's workspace dep flips from `git = ...` to `path = "crates/vendored-raft-engine"`. +3. Archive `humancto/raft-engine` — no further rebases. +4. Annotate this file: "Status: vendored. See `crates/vendored-raft-engine/`." + +This is the "upstream is functionally dead for our use case" exit. +Do not do this before the 12-month mark without explicit +discussion — vendoring a 20 kLOC storage engine is not a decision +to take lightly. + ## Rebase policy while the fork lives If mango needs a newer upstream SHA before #397 merges: @@ -110,12 +149,31 @@ If mango needs a newer upstream SHA before #397 merges: ## Supply-chain audit posture -The fork is public, pinned by SHA (not tag, not branch), and has zero -`unsafe` additions on top of upstream. `cargo vet` handles the fork -the same way it handles any git-pinned crate — the exemption is on -`raft-engine` regardless of origin. When retiring the fork, the -exemption continues to apply against the new upstream SHA without -edits (same crate name, same version `0.4.2`). +The fork is public, pinned by SHA (not tag, not branch), and adds +zero `unsafe` tokens on top of upstream. `cargo vet` keys +exemptions on `(crate_name, crate_version)` — both the fork and +upstream ship `package.version = "0.4.2"`, so the same +`[[exemptions.raft-engine]] version = "0.4.2"` entry in +`supply-chain/config.toml` covers both while the fork is active +and after retirement. + +Caveats that change this: + +1. **Patch-version bumps are not auto-covered.** If either the + fork or upstream bumps `package.version` (raft-engine + historically bumps on master without publishing to crates.io + — see ADR 0002 §B3), the exemption stops matching and needs + a new `version = "0.4.3"` line. Watch for this on rebase. +2. **SHA swaps are silent to vet.** `cargo vet` verifies the + resolved git hash against the locked source, but the hash is + not part of exemption identity. Changing the fork SHA without + bumping `package.version` requires no vet edits. +3. **`review-by` is a mango convention, not a vet behavior.** + Mango annotates every exemption with a `review-by: YYYY-MM-DD` + note. Those dates do not auto-regenerate on source change. + When rebasing the fork OR retiring to upstream, manually + refresh the `review-by` date on the `raft-engine` exemption + to reflect that the code under review changed. ## Last updated