fix(session): route session beads to no_history storage — both halves of vp-ia76 - #124
Merged
Conversation
bourgois
added a commit
that referenced
this pull request
Aug 6, 2026
…, shared teardown Four findings from the PR #124 review, all verified before fixing: * THE ONCE-GUARD COULD BE BURNED BY A FALSE ALARM. warnSessionStorageUnsupported was once-per-process, and the first (benign) alarm for beadPolicyGraphStore consumed it — so a later, genuinely incapable chain of a DIFFERENT store type wrote sessions to the committed table with no warning at all: a silent failure inside the warning that exists to prevent silent failure. Now once per store TYPE (bounded noise, never mutes a new offender). * THE FALSE ALARM ITSELF. beadPolicyStore applies the session storage policy in its own Create (policyForCreate -> createWithStoragePolicy) but deliberately does not forward CreateWithStorage, so the front door's capability probe misread it as incapable. It now declares AppliesBeadStoragePolicy(), and the front door recognizes the structural marker and creates quietly through it — verified equivalent on the live fleet (27 sessions -> wisps, 0 -> issues, all via this wrapper). * STALE DOC: createSessionBead's comment still described CachingStore's OLD silently-degrading fallback, which commit 3 of this PR replaced with class-stamping. Reworded to past tense. * DUPLICATE TEARDOWN: cleanupManagedDoltForTest re-implemented a narrower version of the existing cleanupManagedDoltTestCity (path_helpers_test.go), which additionally stops the controller, waits for it, shuts the beads provider, and sweeps stragglers. The five init tests now use the existing helper; mine is deleted. The original leak reproduction stays green. Also assessed, no change: the new error returns on CachingStore's incapable fallback (both-flags-preset, unknown class) fire only on contradictory input the capable path already rejected — refusing loudly beats persisting garbage. Mutations, each confirmed applied: dropping the self-applying branch -> 1 failure; reverting to a process-wide guard -> 1 failure. Suites: session, beads, and the original leak single-test all green.
…front door vp-ia76 — phase 1 of vp-9u1. The stop-the-bleed change, REBASED onto fork/main: the first attempt was built on a STALE origin/main ref (242 commits divergent, from a remote that no longer exists) — which is what made the pre-push gate's path tests fail, since their fix already existed upstream. Verified by applying this same patch to fork/main and watching those tests pass unchanged. CreateSessionInfo is the SINGLE front door for session-bead creation, and it called s.store.Create(), which carries no storage class. gascity's own policy (beadPolicySession -> beadStorageNoHistory) was therefore dropped, and every session bead landed in the Dolt-COMMITTED issues table instead of the dolt_ignore'd wisps table, one DOLT_COMMIT each. Measured: 262 sessions/24h. That commit volume grows hq, drives compaction, and rebuilds the push backlog faster than the 15s listener window can ship it (the ratchet measured 2026-08-05/06). no_history, NOT ephemeral: ephemeral sets ephemeral=1, which bead_policy_store declares incompatible for sessions and matchesTier silently DROPS. The 204 session rows already in hq.wisps are no_history=1, ephemeral=0 — matched here. THE FALLBACK IS LOUD. CachingStore.CreateWithStorage silently degrades to plain Create when the backing store lacks StorageCreateStore — and NativeDoltStore lacks it, so this fix ALONE is inert on a native-store chain: the warning is what makes that visible instead of silent. The COMPANION fix that makes the native store honor the class exists as branch vp-ia76-session-nohistory (c2382c780, built 2026-07-31, never pushed); both are needed for the commit volume to actually drop. This commit deliberately does not absorb it — it is another author's work and should land as itself. Verified red-first on this base: restoring the plain Create -> 6 failures. internal/session ok, internal/beads ok, go vet clean.
A session bead created through the front door is supposed to land in the
dolt_ignore'd wisps table, where it costs no DOLT_COMMIT on create or on any
later update. On the live hq city it lands in the committed issues table
instead: 727 of 730 session beads created in 24h, and 21,911 of 22,427 rows in
hq.issues (97.7%) are session beads.
The policy layer was not the problem. beadPolicyStore.Create already resolves a
session bead to the no_history class and calls CreateWithStorage. The class was
discarded one layer lower:
caching_store_writes.go storageBacking, ok := c.backing.(StorageCreateStore)
if !ok { return c.Create(b) }
*NativeDoltStore -- the store the live city runs on, as its "gc: update bead
<id>" commit messages show -- implements Create but not CreateWithStorage, so
the assertion failed and the class was dropped with no error and no warning.
*BdStore does implement it, which is why the few scopes still on the bd
subprocess store produce correct wisp rows while everything else does not.
That is ADR-0043 Cause 1: an unsupported capability coerced into the quiet
default, sharing a code path with the definite negative.
- NativeDoltStore gains CreateWithStorage. The tier rides on the issue's own
Ephemeral/NoHistory fields, which is what the upstream beads layer routes
on and what makes it skip DOLT_COMMIT.
- The caching store's incapable-backing fallback now stamps the class onto
the bead instead of discarding it, so the policy still lands on any backend
that lacks the method.
- beadWithStorageClass is the shared field-carrying form of a storage class.
Guards were watched red before the fix, against the real call rather than a
mock (ADR-0043 Rule 4). The capable-backend control stayed green throughout, so
the pair localizes the defect instead of only reporting it.
native_dolt_storage_class_live_test.go drives a real NativeDoltStore against a
real Dolt sql-server and measures commits: plain create -> issues, 1 commit;
no-history create -> wisps, 0 commits; 10 updates -> 10 commits vs 0.
Three findings from the PR #124 review, all verified before fixing: * THE ONCE-GUARD COULD BE BURNED BY A FALSE ALARM. warnSessionStorageUnsupported was once-per-process, and the first (benign) alarm for beadPolicyGraphStore consumed it — so a later, genuinely incapable chain of a DIFFERENT store type wrote sessions to the committed table with no warning at all: a silent failure inside the warning that exists to prevent silent failure. Now once per store TYPE (bounded noise, never mutes a new offender). * THE FALSE ALARM ITSELF. beadPolicyStore applies the session storage policy in its own Create (policyForCreate -> createWithStoragePolicy) but deliberately does not forward CreateWithStorage, so the front door's capability probe misread it as incapable. It now declares AppliesBeadStoragePolicy(), and the front door recognizes the structural marker and creates quietly through it — verified equivalent on the live fleet (27 sessions -> wisps, 0 -> issues, all via this wrapper). * STALE DOC: createSessionBead's comment still described CachingStore's OLD silently-degrading fallback, which commit 2 of this PR replaced with class-stamping. Reworded to past tense. Also assessed, no change: the new error returns on CachingStore's incapable fallback (both-flags-preset, unknown class) fire only on contradictory input the capable path already rejected — refusing loudly beats persisting garbage. A fourth finding in the original round-2 commit (deduplicating the init-test teardown onto cleanupManagedDoltTestCity) is dropped: PR #125 fixed the same init-test leak on main with the hermetic GC_BEADS/GC_DOLT idiom, and cmd/gc/init_from_hosted_dolt_test.go now matches main exactly. Mutations, each confirmed applied: dropping the self-applying branch -> 1 failure; reverting to a process-wide guard -> 1 failure. Suites: session, beads, and the original leak single-test all green.
…orage-class test, stamping fallback
Eight findings from the merge-set review. Every guard was mutation-verified
against the file it protects; the mutation was confirmed present before running.
* THE MARKER CONTRACT HAD NO GUARD THAT COULD GO RED (finding 1). Renaming
beadPolicyStore.AppliesBeadStoragePolicy, or internal/session's consumer,
left the whole suite green: the front door just stopped recognizing the
wrapper and fell through to the warn-and-stamp path, which still creates the
bead with the same class by coincidence of the default. Two halves now fail:
the interface is exported as session.StoragePolicySelfApplying and pinned in
cmd/gc with `var _ session.StoragePolicySelfApplying = (*beadPolicyStore)(nil)`
(a rename on either side is a build failure), and
TestPolicyStoreCompositionCreatesSessionsSilently drives a create through the
REAL wrapStoreWithBeadPolicies + wrapWithCachingStore composition and asserts
stderr silence. That assertion resets the process-wide warn ledger first
(session.ResetStorageWarningsForTest) — without the reset a sibling test burns
the ledger for the same store type and the silence assertion passes for the
wrong reason, which is the defect this whole finding is about.
* NativeDoltStore.CreateWithStorage HAD NO ALWAYS-ON BEHAVIORAL TEST (finding 2).
Its only coverage was the opt-in live probe. TestNativeDoltStoreCreateWithStorageStampsClass
runs the real method body against newNativeDoltStoreForTest(newNativeDoltMemStorage())
and asserts the created AND persisted bead carries the class for all four
classes, plus that an unknown class is refused without persisting anything.
* THE WARN-AND-DROP FALLBACK NOW STAMPS (finding 11). createSessionBead's last
resort warned and then created with no class at all — the same field routing
the caching store's own fallback relies on costs nothing, so it sets
NoHistory before Create. The warning stays.
* THE WARNING WAS DOLT-SPECIFIC (finding 4). It asserted "committed issues
table (one DOLT_COMMIT each)", which is false for the FileStore and MemStore
chains that reach the same path. It now names the store type and the policy
that was lost.
* TEST HYGIENE (finding 5). The stderr swaps in create_storage_test.go restored
os.Stderr sequentially, so a t.Fatalf inside a capture stranded the whole test
binary on an orphaned pipe; captureStderr now restores via defer. And
TestWarnIsPerStoreTypeNotPerProcess printed its first, real warning to the
real stderr before redirecting — it captures from the first call.
* COMMENT ALTITUDE (findings 13/14). The 35-line essay was attached to the
marker type, leaving createSessionBead undocumented; it still said "once per
process" (now per-type) and narrated PR history ("Before this PR...", "Review
finding (PR #124)..."). Rewritten to state the current contract at the symbol
that owns it.
* CAPABILITY IDIOM (finding 12). `_ StorageCreateStore = (*NativeDoltStore)(nil)`
joins the existing compile-time var block; the runtime
TestNativeDoltStoreDeclaresStorageCreateCapability re-check is deleted.
* LIVE PROBE (finding 8). Tagged `//go:build integration` per AGENTS.md, matching
bdstore_conditional_integration_test.go. GC_NATIVE_WISP_PROBE_DB reaches the
probe queries as an SQL identifier, where a placeholder cannot bind, so it is
validated against a strict `^[A-Za-z0-9_]+$` whitelist first. Its stale claim
that the native store lacks CreateWithStorage is gone.
Two PLAUSIBLE findings assessed:
* THE FRONT DOOR'S HARDCODED CLASS (9a) — FIXED CHEAPLY. The
StoragePolicySelfApplying probe now runs BEFORE the StorageCreateStore probe,
so a store that resolves the CONFIGURED class wins over the front door's
hardcoded StorageNoHistory. No store implements both today, so behavior is
unchanged; the ordering makes the marker authoritative and bounds the
divergence, which is documented at the call site.
* THE STAMPING FALLBACK'S BLAST RADIUS (9b) — DOCUMENTED, NOT CHANGED. It
activates classes for all six policy names on previously-incapable backings,
not just sessions. session/wait/nudge/order_tracking gain no_history, which
does not change query visibility (matchesTier only filters on Ephemeral).
wisp gains EPHEMERAL under bd-105 ready semantics, which IS GC/TTL-eligible
and IS dropped by a default-tier read — safe through the policy layer (it
expands TierIssues to TierBoth), visible to a raw un-wrapped read. Capable
backings (BdStore) already behaved this way, so this converges incapable ones
rather than inventing new behavior. Written into the caching-store comment.
Mutations, each confirmed present in the file before running:
rename beadPolicyStore.AppliesBeadStoragePolicy -> build failure
rename the StoragePolicySelfApplying method -> build failure
drop the marker branch in createSessionBead -> 3 failures
NativeDoltStore.CreateWithStorage: Create(staged)->Create(b) -> 1 failure (3 subtests)
drop the NoHistory stamp in the last resort -> 1 failure
restore the Dolt-specific warning text -> 1 failure
put StorageCreateStore probe back ahead of the marker -> 1 failure
rename NativeDoltStore.CreateWithStorage -> build failure
regress the per-type ledger to per-process -> 1 failure
unanchor the probe-name regexp -> 1 failure
CachingStore fallback: Create(staged)->Create(b) -> 1 failure
Not mutation-verified, triaged as no-op-under-green: captureStderr's deferred
restore has no observable effect while every assertion passes — it only changes
what happens after a failure — so there is no guard to write for it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
The root-cause fix for the fleet's Dolt commit churn, in two complementary halves that are each inert without the other:
The front door requests the class (
internal/session/create.go):CreateSessionInfo— the single entry point for session-bead creation — calleds.store.Create(), which carries no storage class, so gascity's ownbeadPolicySession → beadStorageNoHistorypolicy was silently dropped. Now routes throughCreateWithStorage(…, StorageNoHistory), with a loud once-per-process stderr warning when the chain cannot honour it (naming the offending store type), because the silent-fallback path is exactly how this defect existed undetected.The native store honours it (
internal/beads/, authored by voxist.executor 2026-07-31):NativeDoltStore— the store the live city runs — implementedCreatebut notCreateWithStorage, soCachingStoresilently discarded the class. It now implements the method (tier rides on the issue's ownEphemeral/NoHistoryfields), and the caching store's incapable-backing fallback stamps the class onto the bead instead of dropping it.Also included: the five city-creating init tests now stop the managed dolt server they start (production
stopManagedDoltProcess, waits for exit) — this leak deterministically failed the wholecmd/gcjob and blocked three consecutive pre-push runs.Why it matters
Measured on the live fleet: 262 sessions/24h each cost one
DOLT_COMMITon create plus one per update, growing hq 3.5G→6.7G in a week, driving compaction, and rebuilding the push backlog faster than the 15s listener window could ship it (the backlog ratchet: va accumulated 1,412 unpushed commits in under a day).Evidence
issues, 1 commit; no-history create →wisps, 0 commits; 10 updates → 10 commits vs 0.Create→ 6 failures;ephemeralinstead ofno_history→ 5 (they are not interchangeable:ephemeral=1is policy-incompatible for sessions andmatchesTiersilently drops such rows); silencing the fallback warning → 4.e9db058d2+ these two fixes only). Acceptance after restart: 27 sessions →wisps, 0 →issues.Notes for review
no_history, NOTephemeral— see the mutation above and the inline docs.beadPolicyGraphStore, which lacksCreateWithStoragebut applies the policy internally — a false alarm in that one wiring (verified: zero sessions reachedissuesthrough it). Will be refined separately.fork/main; an earlier build sat on a staleorigin/mainref (242 commits divergent, deleted remote) — that base is abandoned.