Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions .planning/adr/0002-storage-engine.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
182 changes: 182 additions & 0 deletions .planning/fork-raft-engine-lz4-verification.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
# 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 = "<merged-sha>", 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.

## 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:

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 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

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).
Loading