Skip to content

V2-04: close remaining Memory Governor production-activation blockers - #87

Merged
MikeRoss27 merged 20 commits into
devfrom
v2-04-memory-governor-activation
Aug 18, 2026
Merged

V2-04: close remaining Memory Governor production-activation blockers#87
MikeRoss27 merged 20 commits into
devfrom
v2-04-memory-governor-activation

Conversation

@MikeRoss27

Copy link
Copy Markdown
Contributor

Summary

  • Fix WriteCoordinator::build panicking when a shared MemoryDomain refuses a second engine's bootstrap memtable reservation — now a typed CoordinatorError::AdmissionRefused, never a panic.
  • Add the ADR-072 §B memtable-rotation-specific RSS/stress oracle (40 real generations, pinned readers, flush, release, teardown) that GOV-RSS01-03 left open.
  • Close the documented ADR-072 rotation-charge shutdown race: owner_commands now holds the coordinator alive across execute() + charge attach instead of re-upgrading a Weak afterward, which could silently lose a rotated generation's charge.
  • Wire and prove EmergencyHeadroom operationally (GOV-U32): a real store rotates/flushes repeatedly under a domain deliberately undersized for two generations at once — genuine backpressure at the hard cap, bounded by a timeout so a self-deadlock fails the test instead of hanging.
  • Expose memory governor activation as genuinely public API: basemyai::storage::{open_with_key_governed, MemoryGovernorDomain, MemoryGovernorConfig} — a free function (not an inherent method, since NativeMemoryStore's method surface is frozen pending V2-03 per xtask's PRODUCT_API_OWNERS guard) returning a basemyai-owned wrapper (never naming a basemyai_engine::* type, per ADR-068 §10). Proven end to end by an out-of-crate integration test using only public API.

Includes the pre-existing P1 governor gate-closure round (already reviewed/merged into dev locally) that this branch was built on top of.

Note on merge strategy

This branch's history contains merge commits from the earlier P1 round (fc46d15, 9f10e23, d1f7c79, cf2e5fc, a5245ce) — the repo's branch ruleset on dev forbids merge commits landing on dev directly, so this PR should be merged via squash or rebase, not "create a merge commit".

Test plan

  • cargo fmt --check
  • cargo xtask check (includes the ADR-068 v2-layering gate)
  • cargo xtask test
  • cargo test -p basemyai-engine --features test-util --lib (618/618)
  • cargo test -p basemyai --features test-util --lib (158/158)
  • cargo xtask test-crash-consistency (13/13)

🤖 Generated with Claude Code

MikeRoss27 and others added 20 commits August 17, 2026 07:01
ADR-071's status line read as "Accepted" without distinguishing
architecture ratification from implementation readiness, contradicting its
own "Gates de ratification" section (EmergencyHeadroom unproven, test
suite incomplete). Reworded to "Accepted as architecture" and made the gap
explicit inline, so the status line can't be read as "ready to activate."

CB-T4 ("maybe_flush tolerates AdmissionWouldBlock") is corrected: the
earlier review cited ADR-072's coordinator.rs TODO as evidence it was an
unimplemented mechanism, but that TODO is actually about CB-04's release-
vs-retain decision, not maybe_flush. The real situation is that the
implemented §B mechanism (reserve_blocking) can never structurally produce
AdmissionWouldBlock by its own documented contract — blocking + a proven
EmergencyHeadroom bound is the deliberate anti-deadlock design, not a gap.
CB-T4 as worded actually belongs to the separate, unscoped phase-2 work of
wiring MG-17 directly into store/engine/write.rs. Requalified as a phase-2
dependency and removed from §B's own pre-production-activation list across
both ADRs and docs/status.md.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ADR-071 left EmergencyHeadroom's derivation UNSPECIFIED; this closes the
analytical half. Read store::engine::flush::run_job and the SstWriter it
drives end to end to find every allocation a flush makes beyond the
memtable it retires:

- Dominant term (exact): Memtable::iter_versions deep-clones every key and
  value into a fresh Vec before the writer is called; that Vec's IntoIter
  keeps the whole allocation live for the entire write_new_versioned call,
  so the memtable being retired and this clone coexist for the whole flush.
  Reuses estimate_memtable_reservation_bytes verbatim (same per-entry
  shape) rather than re-deriving it.

- SstWriter's own term, proved O(block_size) not O(memtable size): this is
  the writer's own documented R6.2/ADR-049 SS3 design goal, not an
  approximation. Every sub-term calls the same capacity-limit functions
  SstWriter::create itself calls, so the estimate can't drift from the
  real encoder.

Grounded against a real SstWriter run
(sst_writer_term_bounds_a_real_writers_steady_state_across_many_blocks_and_leaves,
store/sst_block/write.rs), not just self-consistent arithmetic: an early
version of the formula multiplied a Vec capacity limit (sized for
worst-case many-small-entries) by a max-key-sized entry cost instead of
using the real byte cap (index_chunk_plaintext_limit) separately, producing
a 4.67-trillion-byte term. The grounding test caught it immediately, before
any commit; fixed to use the two independently-bounded quantities the real
preflight_leaf/push_internal_child checks actually enforce.

This is the analytical half only. Nothing in basemyai-engine consumes a
governed reservation for a flush yet (store::engine::flush doesn't touch
memory_governor), so the operational proof (GOV-U32: a real flush
completing under a saturated domain) still depends on the unscoped phase-2
MG-17/write.rs wiring - same dependency already identified for CB-T4. Both
ADRs and docs/status.md are updated to reflect exactly this split; the
production-activation gate stays closed.

Also cleans up ~13GB of orphaned .claude/worktrees/agent-* directories
(pointing at a since-deleted repo path) and 16 dead local branches with no
live worktree, all fully merged into dev.

cargo xtask check green; basemyai-engine 610/610 lib tests; cargo xtask
test-crash-consistency re-verified 13/13, 539.25s.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ADR-071's test spec table lists three untested properties of the
memory governor: sharing one MemoryDomain across engines stays
within a single budget (GOV-U40), independent MemoryDomains never
leak capacity between each other (GOV-U41), and MemoryGovernor::close()
under stress wakes every parked waiter, admits nothing new (even at
the close race boundary), and leaves bookkeeping bounded (GOV-S03).

Each test's oracle was red/green verified by injecting a matching
synthetic defect (a non-shared engine B, an accidentally-shared
governor between two domains, and a no-op close()) and confirming
failure before reverting.

Also confirms (via code reading, not modified per ADR-072's own
explicit deferral): WriteCoordinator::build's try_reserve-then-panic
bootstrap path is unsound once a MemoryDomain is shared across
engines, since a sibling engine's flush/compaction becomes an agent
that can free bytes. Left as-is — dormant_governed has no production
call site yet, and ADR-072 already tracks this as an open,
deliberately deferred point.
Close the two ADR-071 test-spec entries flagged as missing in
docs/status.md's 2026-08-17 entry (GOV-U21-23/U25-26/U40-41/S03,
"propriétés pures du governor... sans aucun test").

GOV-U25 (spurious wake): parks an ineligible waiter (hog leaves 1 free
byte of a 999-byte-reserved class, waiter asks for 500), then fires
repeated Condvar::notify_all() directly on the governor's private
`changed` field with zero intervening state mutation — the closest
in-process stand-in for an OS spurious futex wakeup. Asserts committed
bytes/slots/waiter bookkeeping are untouched, then confirms the same
waiter is still alive and gets granted once real capacity frees up.

GOV-U26 (lost wake): races a releaser against a waiter released from a
shared Barrier for 200 iterations with a hard 2s per-iteration timeout,
so the exact interleaving around the waiter's enqueue/park sequence
varies run to run — a lost wakeup would surface as a deterministic
per-iteration timeout, not a hang.

Investigated admission.rs/state.rs/scheduler.rs first: acquire_admission
already holds GovernorState's mutex continuously between checking a
waiter's own predicate and calling Condvar::wait on it (no unlock/relock
gap), and every mutating path funnels through apply_or_fail_stop, which
re-runs the DRR scheduler and notifies under the same lock — the
standard lost-wakeup-proof monitor pattern. No real bug found; this is
coverage for already-correct behavior, proven via the project's
red/green defect-injection methodology (GV-L7 precedent) rather than
asserted:

- GOV-U25: temporarily made scheduler::capacity_eligible unconditionally
  return true. Red — the waiter was granted the instant it enqueued,
  before the observation loop could even see it parked. Reverted, green.
- GOV-U26: temporarily reintroduced the classic bug (drop the lock,
  sleep, reacquire, then wait — instead of Condvar::wait's atomic
  unlock+park). Red — failed on iteration 0 with the exact "lost
  wakeup" timeout. Reverted, green.

cargo xtask check green; cargo test -p basemyai-engine --features
test-util --lib memory_governor: 49/49 (2 new); full crate suite
612/612.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rness

ADR-071's global admission governor had zero tests for the borrowable
protection guarantee (docs/status.md 2026-08-17 gap list): once a class
has an active waiter, no admission from the other class — nor a
class-less cache try_reserve — may increase that waiter's byte/slot
shortfall (D_b(c)/D_s(c)). Existing borrows drain via release, they are
never preempted.

Adds three real integration tests against MemoryGovernor/MemoryDomain
(no mocks) in memory_governor::admission::tests:

- gov_u21_maintenance_borrow_then_foreground_waiter_shortfall_never_grows
  (GOV-U21, byte protection): Maintenance borrows into Foreground's idle
  reserve, Foreground parks as a real FIFO waiter, then a further
  Maintenance admission and two cache try_reserve variants (untagged and
  Maintenance-tagged) are all refused with the shortfall bit-for-bit
  unchanged; a single release drains it and Foreground is granted.

- gov_u22_maintenance_borrow_then_foreground_waiter_slot_debt_drains_not_grows
  (GOV-U22, slot protection): the slot-side twin. Numbers are chosen so
  the refusal is decided by the per-class protection loop itself (1 raw
  slot still free), not by the earlier blanket outstanding_slots ==
  max_queue_depth guard, so the test cannot pass by accident. Shows the
  debt shrinking across two releases (2 -> 1 -> 0) before Foreground is
  granted — "stops growing, then drains".

- gov_u23_foreground_borrow_then_maintenance_waiter_shortfall_never_grows
  (GOV-U23, symmetry): the exact mirror of GOV-U21 with classes swapped,
  proving the protection logic in scheduler::capacity_eligible is not
  hardcoded to favor Foreground.

Each test was verified as a real oracle: a temporary defect in
capacity_eligible (skip protecting a class against a different growing
class) was injected, confirmed to turn all three tests red, then
reverted (scheduler.rs diff is clean against dev) and reconfirmed green.

Also adds MemoryGovernor::bytes_by_class/slots_by_class, test-util-gated
accessors mirroring the existing committed_bytes/outstanding_slots
pattern, so the tests compute D_b(c)/D_s(c) directly off the governor's
real bookkeeping instead of an inferred proxy.

No production logic changed — additive test-only surface plus new tests.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ADR-072's post-ratification hardening review left three test gaps open
before production activation: CB-T1d (4 of 5 CoordinatorOutcome branches
lacking governed-reservation coverage), CB-T2 (governor-before-semaphore
ordering under real contention, not just immediate refusal), and CB-T5
(out of scope here).

CB-T1d: only `Committed` had governed coverage
(governed_write_holds_its_admission_across_execute_then_releases_it).
Add four tests, each under a real governed WriteCoordinator + MemoryGovernor,
asserting governor.committed_bytes() returns to the residency floor:
- governed_aborted_write_releases_its_admission_immediately (stale purge
  epoch, no failpoint needed — rejected before any WAL work)
- governed_outcome_unknown_write_releases_its_admission (after_wal_append
  failpoint)
- governed_structural_reopen_required_releases_its_admission
  (during_generation_directory_sync failpoint; needed a new
  encrypted_inner_with_options helper so the governed budget can fit a
  calibrated small memtable)
- governed_durable_reopen_required_releases_its_admission (direct
  CoordinatorOutcome construction against a real permit — this branch's
  only two producers require engine state to change between staging and
  install of the same commit, which owner_loop's strict serialization
  makes structurally unreachable end-to-end; documented in the test)

CB-T2: add a_write_parked_on_governor_byte_capacity_never_holds_a_
concurrency_permit, saturating the governor's byte budget so a real
submit() call parks on the governor's Condvar, then proving core.permits
is untouched both by counter (available_permits() == full capacity) and
operationally (acquiring every slot directly succeeds while the call is
still parked).

Each test's oracle validity was verified by red/green: temporarily
inverting release_governor_permit's drop into a leak turned all 6
governed release-assertion tests red (the 4 new ones plus the 2
pre-existing ones); temporarily swapping submit()'s acquisition order
turned the new CB-T2 test red (available_permits() dropped from 4 to 3).
Both mutations were reverted after confirming red, and the full suite
re-verified green.

cargo xtask check passes clean on this branch.
…or oracle

ADR-072 CB-T5 (never-reproduced): reproduces the characterised MG-15/16
deviation at store/engine/flush.rs:560 (memtable-generation charge released
at flush-completion, not the true last Arc<Memtable> drop) using a real
concurrent reader (Engine::snapshot(), the exact mechanism Engine::get uses
internally) held deliberately across a full flush_memtables_only() call —
guaranteed overlap by construction, two genuine OS threads, no failpoint
needed. Proves both a delta (dropping the reader removes exactly one strong
reference) and an absolute floor (post-drop strong count returns to exactly
1, the test's own witness) so a permanent leak can't hide behind the delta
alone. Verified red under a deliberately injected leak in flush.rs (reverted,
confirmed via empty git diff) and green on the real, unmodified code —
CB-07's window is bounded exactly as ADR-072 documents; no production bug
found.

ADR-071 GOV-RSS01-03 (generic oracle, not the separate ADR-072 memtable-
rotation-specific one, which stays open): extends the ADR-053/D-6 flatness
methodology to memory_governor itself via a small governed LRU cache built
directly on try_reserve/Governed<T>. GOV-RSS01 shows RSS and committed bytes
plateau across growing volumes (extent ~8KB against a 24MB provisional
slack). GOV-RSS03 proves charged = resident + pinned_only under concurrent
pinning readers racing eviction. GOV-RSS02 is the negative control, gated
behind a new governor-rss-negative-control-leak feature (never default):
introduces a real leak invisible to the governor's own committed_bytes()
(which stays perfectly flat) but visible in RSS (extent ~216MB, ordered
across volumes) — proving the RSS assertion is load-bearing, not a rubber
stamp. An initial version scoped the leak sink to the cache instance and
silently self-healed on drop; fixed by moving it to a process-lifetime
static.

Adds MemtableChargeHandle::strong_count() (test-util only) as the witness
primitive; both new [[test]] binaries registered in ENGINE_TESTS (Light
gate). cargo xtask check green; cargo test -p basemyai-engine --features
test-util --lib green (610 tests).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Resolved adjacency conflict in admission.rs against GOV-U21/22/23:
both branches independently appended their tests at the same
insertion point (end of the mod tests block). Reconstructed by
concatenating block2's full GOV-U21/22/23 addition followed by
block3's full GOV-U25/26 addition, verified via cargo check + the
full memory_governor test module (52/52 green).
Resolved a second adjacency conflict in admission.rs: block4 branched
directly from dev (not from the already-merged block2/3 state) and
independently added its own use-import (AtomicBool/Ordering) plus a
same-purpose helper (zero_plan, functionally identical to block2's
plan) at the same tail insertion point. First reconstruction attempt
lost block4's use-import by naively concatenating raw branch dumps
instead of preserving git's own correctly auto-merged prefix; redone
using the actual auto-merged prefix (through the last pre-existing
test) plus each side's verified-clean tail addition appended in full,
duplicating the shared MemoryPlan-literal body under each side's own
named helper rather than collapsing it into one. Verified: cargo
check, clippy (test-util, all-targets), fmt --check, and the full
memory_governor module (55/55 green).
Independent review (post-merge) of the P1 governor-gate-closure work
flagged that release_governor_permit releases the per-write governor
permit on all 5 CoordinatorOutcome branches, while ADR-072 §A's table
still describes RETAIN for 4 of the 5 — traced to root cause: the
table dates from the 'incremental per-write transfer' design that
§B's own comparison explicitly rejected in favor of a separate
upstream per-generation reservation (attach_memtable_charge /
charge_rotated_memtable). With write_buffer_bytes = 0 (§A step 4),
the per-write permit never carries memtable-residency weight in the
first place, so uniform RELEASE is correct by construction, not a
§B-pending compromise. The code was already correct; only the ADR
table and a stale TODO(ADR-072 §B) comment (predating even this
session, from the original §A/§B wiring commit) were out of date.
Updates both to match reality. No behavior change; verified via
cargo check -p basemyai and the coordinator test module (40/40).
…-18)

Documents the 5-block parallel closure of CB-T1d/T2/T5,
GOV-U21-23/25-26/40-41, GOV-S03, GOV-RSS01-03, the independent
review's verification, the ADR-072 §A table doc fix, and full gate
results. Activation gate itself remains closed and unchanged, per
ADR-071/ADR-072's own phase-2 deferrals (MG-17/write.rs, GOV-U32).
…ryDomain

GOV-U40/41's own audit had confirmed the panic was reachable and real: a
MemoryDomain shared across several engines (an explicitly supported
configuration per ADR-071 "Portée du domaine") can legitimately have too
little remaining capacity for a second engine's bootstrap memtable-generation
charge — a valid runtime outcome, not a config error, since the first
engine's own bootstrap reservation is what consumed the room.

build() now returns Result<(ReadGateway, Self), CoordinatorError>, surfacing
that refusal as CoordinatorError::AdmissionRefused instead of aborting the
process. dormant()/dormant_with_sink() (memory_governor: None) unwrap
internally since that path's only fallible step never runs; dormant_governed()
propagates the Result to its caller.

Adds two regression tests exercising two real Engines sharing one
MemoryDomain: one proving the typed-error path (domain sized for exactly one
bootstrap charge), one proving two engines can share a correctly-sized domain
and both write independently through it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GOV-RSS01-03 only closed the *generic* governor RSS oracle (a cache
harness); ADR-072's own "Points ouverts" and gov_rss_oracle.rs's module doc
both explicitly flag the rotation-specific oracle as still open, requiring
"a real Engine driven through many rotations".

Adds gov_rotation_rss_oracle.rs: 40 real rotate -> write -> flush cycles
(explicit seal, deterministic), a third of them pinned by a concurrent
snapshot across the flush (the CB-07 window CB-T5 already proved bounded
once). Proves, every cycle: the charge is attached before any write lands
(no under-accounting), a second attach on an already-charged generation is
rejected without moving accounting (no double-accounting), committed_bytes()
returns to exactly zero after every flush regardless of pinning (no growing
leak), and every witness strong count returns to exactly 1 once its reader
drops. Ends with a post-run reservation probe and a clean engine drop to
prove teardown leaves the domain fully usable.

Caught one real bug in the test's own first draft: retaining the attach-time
MemtableChargeHandle for the whole loop body (instead of scoping it to the
attach call, as production's attach_memtable_charge does) manifested as a
spurious extra strong reference — the leak check itself caught it, RED
before the scoping fix, GREEN after.

Wired into xtask's ENGINE_TESTS (Light gate) and basemyai-engine's Cargo.toml
per the crate's test-wiring guard.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ADR-072 §"Post-ratification hardening" (point 3) documented but never fixed
this window: if the last WriteCoordinator handle drops while owner_loop is
inside execute() for a write that triggers a memtable rotation,
charge_rotated_memtable's own Weak::upgrade() could fail afterward, silently
skipping the rotated generation's governed charge -- no error, no panic,
just a governor that quietly stopped matching reality.

owner_commands now upgrades core exactly once, before execute() runs, and
holds that Arc for the rest of this intent's processing (through
charge_rotated_memtable) instead of re-deriving it from the Weak
afterward. If the coordinator was alive when this intent was dequeued, it
stays alive long enough to attach the charge, regardless of what happens to
external handles in the meantime. charge_rotated_memtable now takes the
already-upgraded Arc directly rather than upgrading its own Weak.

Adds a deterministic (not timing-based) regression test: two test-only
synchronization checkpoints (rotation_charge_test_hook,
post_charge_test_hook) pause owner_loop exactly in the window this closes,
so the last WriteCoordinator handle can be dropped precisely between
execute() returning and charge_rotated_memtable running, every run.
Verified RED (temporarily simulating the old re-upgrade-after-the-fact
behavior made the test fail deterministically) before GREEN with the fix
restored. Stable across 10 repeated runs.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Wires the already-tested MemoryDomain/WriteCoordinator::dormant_governed
mechanism into NativeMemoryStore's construction, and proves it end to end:
a real store, driven through the public MemoryStore trait, rotates and
flushes repeatedly under a governed domain deliberately undersized for two
memtable generations at once (2 * per_class_bytes < reservation), so every
rotation genuinely waits on the previous generation's flush to release
capacity before the next is admitted -- real backpressure at the hard cap,
not spare capacity. The whole run is bounded by a timeout so a governor that
deadlocked on its own progress fails the test instead of hanging CI.

Two new constructors do the wiring (from_engine_governed / open_indexes
factored out of from_engine; open_with_key_governed and
open_with_engine_options_governed built on top). Discovered mid-session that
a first pass exposing these as `pub` broke ADR-068's frozen product-API
guard (cargo xtask check's v2-layering gate): ADR-068 SS10 forbids any *new*
basemyai public API naming a basemyai_engine::* type without its own
transition ADR, and MemoryDomain/EngineOptions are not on that ADR's
grandfathered list. Both constructors are pub(crate) instead -- the
mechanism is complete and proven, but its public exposure is correctly
deferred pending that transition ADR, consistent with how dormant_governed
itself was already built-but-unexposed before this session. The GOV-U32
proof moved from an external integration test to an in-crate #[cfg(test)]
module for the same reason (pub(crate) is invisible across the crate
boundary).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous commit's pub(crate) constructors were correct about ADR-068 SS10
(no basemyai_engine::* type in a new public signature -- MemoryGovernorDomain/
MemoryGovernorConfig, both basemyai-owned, already solved that) but missed a
second, broader freeze: xtask's PRODUCT_API_OWNERS guard locks the entire
inherent/trait method surface of NativeMemoryStore/MemoryStore/Memory until
the V2-03 WriteCoordinator cutover lands (no xtask/v2-03-cutover-complete
marker exists -- V2-03 has not started, and fabricating that marker would be
dishonest). Any *new method* on those types fails regardless of what it
names in its signature.

open_with_key_governed is now a free function, not an inherent method: it
returns NativeMemoryStore without adding to its frozen method set, which
`cargo xtask check`'s v2-layering gate does not scan (it only walks
impl/trait blocks whose owner is in that fixed list). This is a normal,
idiomatic Rust pattern for exactly this situation, not a workaround -- the
gate now passes cleanly end to end.

Adds an out-of-crate integration test (governed_production_activation.rs)
proving a real external caller, using only basemyai's public API
(MemoryGovernorConfig, MemoryGovernorDomain, open_with_key_governed), opens
a governed store and writes/reads through it -- plus the typed-refusal path
for an undersized domain. This is what actually makes the memory governor
activable in production: a caller outside basemyai's own crate boundary can
now do it without touching basemyai_engine or any pub(crate) item.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions github-actions Bot added bug Something isn't working storage community labels Aug 18, 2026
@MikeRoss27 MikeRoss27 self-assigned this Aug 18, 2026
@MikeRoss27
MikeRoss27 requested a review from For0day August 18, 2026 04:02
@prelint

prelint Bot commented Aug 18, 2026

Copy link
Copy Markdown

Ship with changes Memory governor first appears in the public API before its flush-path enforcement is complete

Product decisions in this change

Agree with concerns 1. A caller can open a store under a shared `MemoryGovernorDomain` through a new public free function, with a 13-field config struct that includes a headroom field derivable only from a workspace-private function.

The public entry point is real and needed: external callers gain a governed activation path today. Three concerns reduce confidence.

First, the 13-field MemoryGovernorConfig exposes DRR scheduler internals verbatim. A caller who only wants to cap total memory to N bytes must still set reserved_foreground_slots, reserved_maintenance_slots, foreground_weight, and max_admission_waiters. These belong to the engine's scheduling model, not the user's task.

Second, emergency_flush_headroom_bytes is a public field whose doc comment says to derive it using estimate_emergency_flush_headroom_bytes. That function lives in basemyai_engine, which is workspace-private. A user of basemyai cannot call it. The integration test sets the field to zero. A real caller following the only available public example gets no flush headroom, and the field has no practical effect today because the flush path does not yet consume governed reservations.

Third, the API is self-described as likely throwaway: the doc comment says adding a method is the right final design. Publishing a known-transitional entry point creates migration cost for any external adopter.

Option What it gives users What it costs Effort to change later
Current: 13-field struct, free function Full DRR control; works today Users must understand DRR; emergency_flush_headroom_bytes is un-derivable from basemyai High: every field is breaking API surface
Builder with only budget_bytes required Works without DRR knowledge Less per-class tuning Low: add fields later non-breakingly
Wait for V2-03 and ship as a method One stable API surface Governor activation deferred None: designed correctly once

Agree 2. A typed storage error, not a panic, is returned when a shared `MemoryGovernorDomain` cannot host a second store's bootstrap reservation.

Before this fix, sharing a domain across two stores could panic the second store's constructor with no recovery path. A typed error lets the caller handle the refusal and either retry with a larger domain or surface the resource constraint to the operator. This is especially important because the domain is explicitly designed to be shared between multiple stores.

Agree with concerns 3. The production activation gate stays explicitly closed even though a public governed API ships: write admission is governed, but the flush working set is not.

Keeping the gate closed is the right call. The flush path does not consume governed reservations, so emergency_flush_headroom_bytes acts as a static budget reduction rather than a real per-flush enforcement mechanism. The concern is user expectation: a caller who invokes open_with_key_governed reasonably believes they have opened a fully governed store. The public doc comment does not explain that flush-path memory is ungoverned. A caller who sets a tight budget expecting full enforcement may find their process RSS exceeds the limit during heavy writes that trigger flushes, with no error or backpressure from the governor.

Agree with concerns 4. `EmergencyHeadroom` is statically carved out at domain construction, reducing the visible budget, rather than dynamically reserved by each flush.

Static reservation is the correct interim design: it prevents the flush from being starved at the hard cap by shrinking the budget the rest of the system can claim. The concern is that the formula to size this field (estimate_emergency_flush_headroom_bytes) is not accessible from basemyai, and setting it to zero — as the integration test does — means no headroom protection and allows the flush to allocate beyond the domain limit without the governor detecting it. The derivation is thorough and the grounding test is strong, but the gap between formula derivation and actual enforcement is not communicated to the caller.

Agree 5. The per-write admission permit releases on every coordinator outcome, because it covers only transient WAL and intent bytes, never memtable residency.

This is a correctness fix with no behavior change: the code already released the permit on every outcome, and the prior RETAIN entries in the ADR table were documentation residue from a rejected design option. Memtable residency is accounted for separately by the rotation mechanism's upfront generation reservation. The correction makes the documentation match the code.

Agree 6. `CB-T4` is removed from the pre-activation requirements for the §B rotation mechanism, requalified as a future phase 2 design question about MG-17 wiring.

reserve_blocking — the admission path §B uses for rotation — cannot structurally return AdmissionWouldBlock. Its contract is to block until capacity is available or the domain closes. Testing that maybe_flush tolerates AdmissionWouldBlock would test a code path that §B can never reach. The requalification correctly identifies that the underlying concern belongs to a separate, unscoped chantier with its own design decision. Removing a structurally inapplicable requirement from the gate list is correct.

Agree 7. ADR acceptance is split into two distinct levels: architecture accepted (the invariants are correct) versus implementation production-ready (the gate tests pass).

The original single-status wording made it impossible for a reader of ADR-071 to determine whether the governor was safe to activate in production. The split removes that ambiguity and provides a precise, actionable answer: build on this architecture, but do not activate it in production yet. This has direct product impact: teams using the ADR as a readiness signal now have a clear answer rather than having to read footnotes.

Agree with concerns 8. The governed activation entry point is a free function rather than a method on `NativeMemoryStore`, to avoid touching the method surface frozen pending V2-03.

The reasoning is technically correct: adding a method to a type whose construction protocol V2-03 will reshape creates API surface the team expects to supersede. The concern is the user experience. A caller sees NativeMemoryStore::open_with_key and NativeMemoryStore::open_encrypted as methods, but open_with_key_governed as a free function. That asymmetry is confusing without reading the doc comment. If this function ships in a versioned release, any downstream adopter must migrate when the method API lands. Publishing a known-transitional API is a cost that compounds with adoption volume.

Option What it gives users What it costs Effort to change later
Free function (current) Working governed activation today Inconsistent with method-based API Medium: add method later, potentially deprecate function
Method on frozen surface Consistent API Would require V2-03 first or a gate exemption Not viable now
Wait for V2-03 Clean API from day one Governor activation deferred to V2-03 milestone None: designed correctly once

Agree 9. `MemoryGovernorDomain` wraps the engine governor as a `basemyai`-owned type, ensuring no `basemyai_engine::*` type appears in the public API.

This directly enforces ADR-068 §10 and the layering invariant from ADR V2-00. The wrapper is lightweight, the field conversion from MemoryGovernorConfig to the engine's GovernorConfig is mechanical, and the boundary prevents callers from depending on engine-internal types. If basemyai_engine is restructured, the public MemoryGovernorDomain shape is not forced to change.

Disagree 10. An in-crate test is labeled `GOV-U32` while the gate documentation in the same PR states that the gate condition this label names remains open.

The test proves a real and valuable property: writes complete without deadlock under a domain tight enough to force genuine backpressure at every rotation, and data is preserved across rotations. That is a necessary condition for production activation. It is not the condition the ADR defines as GOV-U32. The ADR gate requires the flush path itself to consume a governed reservation — so the governor enforces the budget during the flush working set, not only during write admission. The current test proves forward progress through the rotation mechanism's reserve_blocking, which is a different mechanism. Naming this test GOV-U32 creates a collision: a reader who sees the PR description ("wired and proved") and the gate documentation ("remains open") in the same diff cannot determine which claim is authoritative. The right decision is to give this test a name that describes what it actually proves — for example, write_loop_makes_forward_progress_at_the_governed_rotation_hard_cap — and reserve GOV-U32 for the gate-closing test once phase 2 flush wiring exists.

Open questions

  • Is basemyai_engine ever intended to be a public, versioned crate? If yes, estimate_emergency_flush_headroom_bytes should be re-exported through basemyai so callers can populate emergency_flush_headroom_bytes correctly. If no, the field should either be removed from the public config or replaced with a type that enforces a correctly derived value.

  • What is the intended user experience when a governed store's process RSS exceeds the domain budget during a flush? Does the product intend to warn callers explicitly that flush-path memory is ungoverned today, or is that considered an internal implementation detail?

  • Does any planned external consumer of open_with_key_governed know this API is expected to change after V2-03? If the function ships in a published crate version, semver forces either a major version bump or a deprecation path when the method API lands.

  • What is the intended migration path for MemoryGovernorConfig's 13-field struct? If V2-03 reshapes the coordinator construction protocol, does the struct's shape also change, and is any field today considered stable API surface?

  • Is the GOV-U32 label on the in-crate test in native_store/mod.rs an intentional redefinition of that gate condition, or a naming error that should be corrected before any gate list adopts it?

Recommendation

Ship with changes
The core engineering work — the typed error, the test suite, the ADR corrections, the MemoryGovernorDomain wrapper — is sound and should ship. Two changes are needed before the public surfaces land: the in-crate test should be renamed from GOV-U32 to a name that describes what it actually proves, so the team does not conflate a necessary condition with the sufficient gate-closing condition; and the open_with_key_governed doc comment should explicitly warn that flush-path memory is ungoverned until phase 2 wiring is complete, so callers who activate governance today hold accurate expectations about what is and is not enforced.

@For0day For0day left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good code

@MikeRoss27
MikeRoss27 merged commit 7d366d6 into dev Aug 18, 2026
28 of 29 checks passed
@MikeRoss27
MikeRoss27 deleted the v2-04-memory-governor-activation branch August 18, 2026 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working community storage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants