refactor(ingest): extract the shared local ingestion core - #129
waterbro-8 wants to merge 2 commits into
Conversation
`put --watch` (bytefolk#110) would otherwise re-implement the same cursor store, state-root layout, failure classification and report vocabulary that PR bytefolk#108 already hardened for the qoder connector, and the two copies would drift. Move walk, cursor, change gate, `--dry-run` / `--limit` semantics, per-file degradation and report aggregation into `server/internal/ingest` and leave the connector as a thin call site that supplies the Qoder parser, the memory payload and the HTTP upload. Behaviour is preserved at the bytes level, not just by assertion: memories request bodies, `Idempotency-Key` derivation, stdout summary, stderr conflict warning, exit status and cursor file format and location are identical before and after extraction on a shared fixture tree. The PR bytefolk#108 test suite passes with import-path changes only. Refs bytefolk#111
Draft audit at
|
The connector used --root exactly as spelled, so the walk, the project/session split and the cursor key disagreed whenever the root was relative or reached through a symlink. Two working directories each holding sessions/p.jsonl shared one cursor, and the second run saw an up-to-date checkpoint and posted nothing. Canonicalize the root once and derive every identity from it. Checkpoint saves staged through one shared <cursor>.tmp, so a second run could fail on the name or rewind a cursor a faster run had already committed. Each save now gets its own staging file and never moves a cursor backwards. Failures now reach the classifier intact: the upload adapter keeps the typed API error and the command maps exit codes at its own boundary, and the transport check no longer runs ahead of the local-file checks that a syscall.Errno also satisfies, so an unreadable source reports read_denied or root_missing instead of network. A cycle that aborts while reading records the code it died on.
Audit response at
|
| Check | Result |
|---|---|
gofmt -l server/ |
PASS, no output |
go build ./... |
PASS, exit 0 |
go vet ./... |
PASS, no findings |
go test -race -count=1 ./... |
PASS, whole module, exit 0, 0 failures (uncached) |
go test -race -count=1 ./internal/ingest ./cmd/mem |
PASS — 14 core tests, 11 IngestQoder* CLI tests, all executed |
git diff --check |
PASS |
| AC-001 base↔head byte dump, re-measured at this head | IDENTICAL: 5348 bytes, sha256=e7e042ea081bc6dd92ad27661daf999419e6550ce898f7fa97654140141c5643 for both 3a96ad8 and this head |
The dump compares exit code, stdout, stderr, every raw POST /v1/memories request (path, Idempotency-Key, body bytes), every cursor file name and byte, and a second run of the same command against the same state dir and server. Only the ephemeral stub port is normalized.
Two notes on how that dump was produced, because the first attempt was wrong and the correction is load-bearing: the second run had been executing after the stub server was stopped, so it reported context deadline exceeded for both binaries — a matching pair of failures rather than a measured replay. With the server left alive across both runs, the rerun writes 2 memories and exits 0 on both revisions, and its wire bytes now match as well. The harness also reads --root as an absolute path, so it exercises the shipped path identity, not the relative-root case that P1#1 changed.
Not run, not claimed: Docker/PostgreSQL jobs, any end-to-end run against a real memd, coverage delta.
Hosted checks at this head: none reported. GET /commits/6e864e1/check-runs returns 0, and there is no action_required run queued for issue/111-shared-ingestion-core; the latest runs for this branch are still the three that passed at 3c123cd (PR Policy, Validate Agent memory, CI, all success, 2026-08-31 03:28–03:33Z). So "rerun exact-head checks" is satisfied locally and not yet on the hosted side — the workflows need a maintainer to approve the run for the new head before that row can exist. I have not treated the absence as a pass.
Still open
- Draft stays draft. Independent
CODEOWNERSapproval is required, and P1#2's ruling plus the replay-vs-require-absolute choice in decision 2 are owner calls. - The branch base is
3a96ad8;mainis at731a468, so strictmainis ahead. I did not rebase onto it here: fix(npm): treat Windows cache-lock contention as retryable #134 edits the sameUnreleased/Fixedregion ofCHANGELOG.md, and I would rather surface that resolution than make it silently in someone else's PR. ExpectCHANGELOG.mdas the only conflict. - Transparency about this push: my commit helper defaulted to the base repository, so the first attempt created
issue/111-shared-ingestion-coreinfullstack-ai-infra/meminstead of moving this fork branch. Nothing referenced it (no CI ran for that commit) and I deleted it; the content exists only as6e864e1here. The helper now refuses to create a missing ref and refuses a non-fast-forward move, so a wrong-repo push cannot repeat quietly.
PeterGuy326
left a comment
PeterGuy326
left a comment
There was a problem hiding this comment.
P1 correctness hold on exact head 6e864e1e5d2e2a42bc4018c1239d015e7c76c1ce.
SaveCursor reads the existing cursor and later renames its replacement without a cross-process compare-and-commit. Two writers can both read the old state, the higher cursor can publish first, and the lower cursor can then overwrite it. The current concurrent test accepts final LastLine values 1 through 8, so it permits precisely the regression the cursor must prevent.
Please use a real cross-process serialization or atomic monotonic commit protocol, then add a deterministic interleaving test that proves the final cursor is the maximum value rather than any winner. Keep the fix separate from unrelated ingestion refactoring.
The PR is also Draft, conflicted with main, and its fork head has no current workflow runs. It needs a canonical ready requirement, a clean synchronization, maintainer-approved workflows on the new fork head, and fresh required CI before final review.
Code Review — 问题与建议1.
|
|
Closing this as superseded. It is our PR and our draft, and #147 states "Supersedes #129" in its own body, so leaving this open is the thing that needs explaining — not the closing. Reopen freely; nothing here was pushed or deleted. Where @PeterGuy326's
|
Refs #111 ## Requirement and scope Re-lands #147 onto current `main` as an organization branch. #129/#147 were closed under the 2026-09-03 fork-workflow decision, not as a judgment that the extraction was wrong. Blocker PR #108 is already merged. Preserves qoder behaviour: same memories payload shape, same `Idempotency-Key` derivation for a canonical absolute root, same stdout summary, same cursor file format/location. Adds the OS-backed cursor lock from the #147 follow-up so concurrent writers do not share a `.tmp` name. ## Changes - New `server/internal/ingest` package: walk, per-path cursor (atomic rename, shrink-reset), `--dry-run`/`--limit`, closed failure codes, report aggregation. - `mem ingest qoder` is a thin connector (parser + HTTP upload). - OS advisory lock around cursor load/save (`cursor_lock_*.go`). - No `fsnotify`, no `--watch` (#110 stays a successor). ## Validation ledger | ID | Criterion | Command | Status | | --- | --- | --- | --- | | V1 | Qoder tests with import-path changes | `go test ./cmd/mem -run Ingest` | NOT VERIFIED locally — host Go 1.22, module requires 1.25 | | V2 | Core fixtures: dry-run, shrink-reset, 409 degrade, corrupt cursor | `go test ./internal/ingest` | NOT VERIFIED locally — same toolchain gap | | V3 | `git diff --check` | local | PASS | | V4 | No cobra/stdout in the core package | source review of `server/internal/ingest` | PASS | Independent review still required. No merge or issue close. Original extraction: @waterbro-8. Cursor lock follow-up: @sun-970 / liyuanyang. Canonical-path identity follow-up: 勒布朗-詹姆斯. --------- Co-authored-by: waterbro-8 <waterbro-8@users.noreply.github.com> Co-authored-by: liyuanyang <liyuanyang@users.noreply.github.com> Co-authored-by: 修雨 <47820304+PeterGuy326@users.noreply.github.com>
3 participants
Summary
Extracts the local ingestion mechanics that PR #108 hardened inside the qoder transcript connector into a new internal package,
server/internal/ingest, so thatput --watch(#110) consumes one core instead of writing a second state layer (adjudicated on #110 R2, D-4).Moved out of
server/cmd/mem:ingest.CanonicalRoot,ingest.Walk+ingest.HasJSONLExtensioningest.Cursor,CursorPath,LoadCursor,SaveCursor,FileState--dry-run/--limitsemantics, per-file degradation, cursor persistenceingest.Runscanned/ingested/deduped/unchanged/changed/local_gone/failed), whereunchangedandlocal_goneare reserved namesRundoes not populateingest.Reportingest.Code,ingest.ClassifyLeft in the connector: cobra flags, the Qoder JSONL parser, the
/v1/memoriespayload shape,Idempotency-Keyderivation, the HTTP upload, and all stdout/stderr text.qoder_checkpoint.gois deleted; its cursor is the core's.The extraction itself is behaviour-preserving — no new dependency, no server change, no payload change, no flag change, no cursor format change — with two intentional exceptions the review asked for, both in
mem ingest qoder's identity and reporting rather than in the moved mechanics: a root is now canonicalized before it keys anything, and failure kinds survive to the classifier instead of being flattened. Their compatibility consequences are in decision 2 below and in the changelog.Why this is a draft
#111is stillstatus: needs-design, andAGENTS.mdstep 2 wantsstatus:readybefore a material change lands. The work was driven forward on the technical owner's request so the extraction (and the two decisions it forces) can be reviewed as code rather than as prose. It is not merge-eligible until the issue is promoted and the decisions below are ratified.Proposed owner decisions to ratify
#111explicitly left these open at kickoff; the implementation picks a value for each, and the choice is load-bearing, so it needs a ruling rather than a silent default.server/internal/ingest(the alternative was an internal file underserver/cmd/mem). Chosen because both sinks are CLI-driven and the server has no knowledge of local paths, so the package must be CLI-side but not insidepackage main, which is where the "second copy drifts" problem came from.internal/keeps it out of the public surface.sha1(abs)filename,absrecorded in the body), whereabsis now resolved throughingest.CanonicalRootbefore the walk, the project/session split and theIdempotency-Keysee it. The review established why the caller's spelling cannot be used: two working directories that each containsessions/p.jsonland share one~/.mem/ingest/qodercollide on one cursor, and the second run silently ingests nothing.--rootthat is not already a canonical absolute path. The default~/.qoder/projectsand an absolute--rootkeep the keys they have today, which is what the AC-001 byte dump measures.--rootand refuse a relative one.Spec gap found while implementing
REQ-001 names a "stat-gate-then-hash change decision". No content hashing exists in the shipped connector, and none is invented here: the only change gate in #108's code is size-based — the cursor records the file size at write time, and a file that has since become smaller is treated as rewritten so its cursor resets. A same-size in-place edit is therefore not detected.
ingest.Run's contract comment states this plainly ("The change decision is size-based, not content-hashed … adding a content gate is a decision to make, not an implementation detail of a call site") so #110 does not read the absent gate as a bug in this PR. Wiring a real content gate would change ingestion behaviour, which#111's non-goals forbid ("Do not 'improve' qoder parsing semantics while extracting").Stated as the narrowing the review asked for: #111 is a mechanical extraction of existing behavior, and the shared change-state contract is not delivered here. The stat-then-hash observation gate is a #110 design decision with its own cost (a content hash per file per cycle), not a missing piece of this refactor.
Report.UnchangedandReport.LocalGoneare therefore reserved names thatRunnever populates — labelled "reserved" in the field comments, withChangeddocumented as "at least one unit of the file was accepted this run" rather than "the file changed" — so #110 cannot read the shared vocabulary as an implemented change-state contract.Acceptance-criteria mapping
server/cmd/mem/cmds_ingest_test.godeletes exactly four lines — theloadQoderCheckpoint(...)call sites that becomeingest.LoadCursor(...)— and its other 302 added lines are the import block plus five new test functions. No assertion in a pre-existing test was edited.mem ingest qoderover one shared fixture tree (3 transcripts under 2 projects, fixed mtimes, one file returning HTTP 409 mid-run to exercise degradation) against a stub server, recording every request byte, every cursor byte, stdout, stderr and exit code — twice, the second run against the same state dir to capture incremental replay. Compiled at the base revision and at this head, the harness produced byte-identical dumps: SHA-256e7e042ea081bc6dd92ad27661daf999419e6550ce898f7fa97654140141c5643, 5348 bytes each, over 9 POSTs (7 first run, 2 replay-path rerun) and 3 cursors, with no stray staging file left behind. Request bodies were captured as raw wire bytes, so payload key order is included in the comparison. The harness is scratch-only and is not part of this commit; it is deliberately not added as a permanent test because it compares two revisions, which no single checkout can do. The first version of the harness stopped the stub server before the rerun, so both binaries "matched" on acontext deadline exceededinstead of on real replay behaviour; that is fixed and the numbers above are from the corrected run. The harness passes a canonical absolute--root, which is why the dump is unchanged by the identity fix — the relative-root case is covered by CLI tests instead, and its compatibility consequence is decision 2.server/internal/ingest/ingest_test.go(14 tests) pins: a dry run writes neither a request nor a cursor; a shrunk/rewritten file resets its cursor; a 409 degrades one file while the remaining files still ingest; a corrupt cursor is treated as empty and reported asstate_corruptwithout blocking the run; plus cursor-path stability, the exact on-disk cursor JSON (field names, key order,0600mode) so an old cursor round-trips through the new code,Classifycoverage of every declared code including a real failedopen, andReport.Addaggregation. The cursor-identity and checkpoint-staging tests areTestWalkCanonicalizesRelativeBase,TestSaveCursorDoesNotStageInASharedSlot,TestSaveCursorKeepsCommittedProgressAndLeavesNoTempFileandTestConcurrentSaveCursorPublishesWholeCursors; on the CLI side,TestIngestQoderRelativeRootKeepsSeparateCheckpoints,TestIngestQoderRelativeRootKeepsProjectSplit,TestIngestQoderUploadErrorsStayTyped,TestIngestQoderReadFailuresClassifyandTestIngestQoderMapsExitCodesAtTheBoundary.ingest.Runtakes function seams (ParseFunc,UploadFunc) and returns aReport; nothing in the package imports cobra,io.Writer, orfmt.Println, and diagnostics go throughOptions.Log.go.mod/go.sumare untouched — no new third-party dependency. The package doc records the contract feat(sync): put --watch one-way directory watch (minimal tier, carved from Phase 2 sync drive) #110 consumes, including the two warnings that a call site must not "optimize" away (a dry run must not persist a cursor; degradation must not advance the failed file's cursor).Validation ledger
Exact head:
6e864e1(audit fixes on top of3c123cd), base3a96ad8(origin/mainafter #120; branch rebased onto it before commit).mainhas since moved to731a468; this branch is not rebased onto it — see the open items below.gofmt -l server/go build ./...go vet ./...go test -race -count=1 ./...go test -count=1 -v ./internal/ingest/go test -count=1 -v ./cmd/mem/go test -race -count=1 ./internal/ingest/... ./cmd/mem/git diff --checkHosted checks at
6e864e1: none reported.GET /commits/6e864e1/check-runsreturns 0 entries and noaction_requiredrun is queued for this branch; the last hosted runs for it are the three that passed at3c123cd(PR Policy,Validate Agent memory,CI). A maintainer needs to approve the workflow run for the new head before that evidence exists; it is listed as missing rather than as passing.Not run, and deliberately not claimed: the Docker/PostgreSQL integration jobs; any end-to-end run against a real
memdor a real~/.qoder/projectsstore; coverage delta measurement.Compatibility, operations, and rollback
~/.mem/ingest/qoder/*.jsoncursors are not rewritten by this change and stay readable by the new code; verified by the on-disk-format test and by the AC-001 dump (cursor bytes match, including the degraded file'slast_line: 1). This holds for any root that is already a canonical absolute path, which includes the default~/.qoder/projects. A root spelled relatively or behind a symlink resolves to a different key, so that store re-queues from line 1 and replays once under newIdempotency-Keys — the tradeoff decision 2 asks to ratify.Idempotency-Keyderivation, so re-running after an upgrade neither duplicates nor skips memories. What can differ is the internal failure tally: read and missing-file failures now reportread_denied/root_missinginstead ofnetwork, which is a reporting correction, not an exit-code change.3c123cdand6e864e1); the connector returns to its in-package cursor and walk code with no operator action. If a relative-root store was ingested after the upgrade, its replayed memories are already in the vault and are removed withforget, not by a revert.Automated assistance
Implementation and validation were performed with automated assistance by the submitting account, and every result above is an actual measured output rather than an expectation. The human submitter remains the author and accountable reviewer. Independent
CODEOWNERSapproval is still required (AGENTS.mdstep 6), and no merge, label change, or branch-protection bypass was performed.Refs #111
Refs #110