fix(memory-ingest): refuse to advance state when gbrain imports fewer pages than staged - #2486
Open
Charles-Grant wants to merge 1 commit into
Open
Conversation
… pages than staged `written` was counted from the staged set, never reconciled against gbrain's own `imported`/`skipped` totals. readNewFailures() only surfaces PER-FILE failures, so when `gbrain import` collects zero files it writes nothing to sync-failures.jsonl, failedSources comes back empty, and every prepared file is state-recorded as ingested. The pass reports "N written" while the brain gained nothing, and because state now says "done", no later run retries. Silent, permanent data loss. This reproduces on any machine that has run `gstack-artifacts-init`: it writes `.gitignore = "*"` into $GSTACK_HOME, `gbrain import` honours .gitignore, and makeStagingDir() stages under $GSTACK_HOME — so collect_files returns 0 for every batch. Observed on a real install: 1013 sessions marked ingested in state, 391 pages actually in the brain. Reconcile imported+skipped against prepared-minus-failed. On a shortfall, error out and return written:0 without touching state, so the next run re-prepares and gbrain's content_hash dedup short-circuits the re-import. Mirrors the existing "exited 0 but emitted no parseable --json payload" branch directly above. Regression test asserts the state file stays empty when gbrain reports collecting nothing — it fails without this change.
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
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
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.
Summary
gstack-memory-ingestcan reportN writtenwhile writing nothing to the brain, and mark every one of those sessions as ingested in state — so no later run ever retries them. On the install where I found this, state claimed 1013 sessions ingested while the brain held 391 pages. Transcripts from ~2 days were gone with no error anywhere.This PR makes that failure loud instead of silent. It does not change the happy path.
The bug
writtenis counted from the staged set, never reconciled against gbrain's own totals:failedSourcescomes fromreadNewFailures(), which reads per-file failures out ofsync-failures.jsonl. Whengbrain importcollects zero files, there are no per-file failures to report — gbrain never saw them — so that set is empty, every prepared file is state-recorded, and the pass reports success.importJson.importedholds the truth. It's used in exactly one place: theconsole.errorsummary a few lines below. It is never compared to anything.Why it triggers in practice
gbrain importhonours.gitignore.bin/gstack-artifacts-initwrites.gitignore = "*"into$GSTACK_HOME:makeStagingDir()stages into$GSTACK_HOME/.staging-ingest-<pid>-<ts>/. So on any machine that has runartifacts-init, every staged file is invisible tocollect_files, forever.Only affects the local-gbrain path. In remote-http MCP mode the code takes
makePersistentTranscriptDir()and never callsgbrain import, so those users don't hit it.Clean-room reproduction
No reference to my setup — reproducible on a fresh checkout in about two minutes:
Same bytes, same filenames, same directory shape. The only variable is whether the path sits inside an
artifacts-inithome.The fix
Reconcile
imported + skippedagainstprepared - failed(skippedis a content-hash no-op, which is a successful landing). On a shortfall: error, returnwritten: 0, and leave state untouched so the next run re-prepares and gbrain'scontent_hashdedup short-circuits the re-import.This mirrors the existing
"exited 0 but emitted no parseable --json payload. Refusing to advance state."branch directly above — same intent, one failure mode further along.The error names the likely cause and the staging path, so the next person sees it in one line rather than bisecting a 70KB file.
Test
test/gstack-memory-ingest.test.tsgains acollectNothingoption on the existinginstallFakeGbrainshim: gbrain exits 0, reportsimported=0/total_files=0, and writes nosync-failures.jsonlentry — the exact silent-loss shape.The test asserts the state file stays empty. It fails without the source change (verified by reverting the fix and re-running).
Notes for the maintainer
/ship. Happy to add both at whatever version you want this to land at, just say the word.$GSTACK_HOME— ephemeral scratch in a directory that syncs to a GitHub artifacts repo is uncomfortable regardless, since transcripts can carry PII. I left that alone:checkOwnedStagingDir()hard-requires "direct child of$GSTACK_HOME", so moving it means changing that contract and its tests. Reconciling the counts makes the failure visible either way, and seemed like the right smallest first step. Glad to follow up if you'd prefer the relocation.bun test) is unaffected and passes. Push the branch to the base repo if you want a full green run.