Conversation
`put --watch` (bytefolk#110) would otherwise re-implement the same cursor store, state-root layout, failure classification and report vocabulary that PR 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
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.
Add per-cursor advisory lock (flock on Unix, fcntl on AIX, LockFileEx on Windows) that covers the read/merge/write sequence in SaveCursor. This prevents concurrent ingest processes from regressing a cursor's LastLine or colliding on a shared staging path. The lock sidecar (<cursor>.json.lock) remains on disk after release; the OS releases the advisory lock when the descriptor or owning process exits. Refs bytefolk#111 Refs bytefolk#139
|
Not a review. No APPROVE, no REQUEST_CHANGES, no acceptance, no vote, and nothing here marks this PR ready. Conflict-of-interest disclosure up front: two of this PR's three commits ( This head has never been built by CIFor Consequence: the What I ran locally, and what it saidGo 1.25.0, linux/amd64. Source fetched from
The two findings below are the ones I'd hold the PR on. 1. The cursor lock does not actually prevent a rewind, because the value it compares is sampled outside the lock
current := LoadCursor(stateDir, cp.Abs)
if current.LastLine > cp.LastLine && current.Size <= cp.Size {
return nil
}The lock does cover the read/merge/write, as the body says. The problem is Reproduced with a scratch test in func TestProbeRewindViaStaleSize(t *testing.T) {
states := t.TempDir()
abs := filepath.Join(t.TempDir(), "a.jsonl")
os.WriteFile(abs, make([]byte, 30000), 0o600) // 30000 bytes, never shrinks
SaveCursor(states, Cursor{Abs: abs, Size: 30000, LastLine: 200}) // run A, finished first
SaveCursor(states, Cursor{Abs: abs, Size: 20000, LastLine: 100}) // run B, stat taken before A's save
// observed: LastLine=100 Size=20000 → 200 -> 100
}The existing suite does not cover this. Dropping the clause to
So this is one clause plus one fixture that needs a real transcript. The alternative, if you'd rather keep 2. Canonicalizing the root re-keys
|
|
Follow-up with a tested patch, not a review. I co-authored Measured: this PR's guard fails #140's own regression scenario
I ran that exact scenario against the guard as submitted here, and against
The cause is what my earlier comment described: Also worth knowing: PatchThree files. It applies to --- a/server/internal/ingest/ingest.go
+++ b/server/internal/ingest/ingest.go
@@ -319,7 +319,7 @@
// read as well as the atomic replacement.
func saveCursorLocked(stateDir, p string, cp Cursor) error {
current := LoadCursor(stateDir, cp.Abs)
- if current.LastLine > cp.LastLine && current.Size <= cp.Size {
+ if current.LastLine > cp.LastLine {
return nil
}
b, err := json.Marshal(cp)
--- a/server/internal/ingest/ingest_test.go
+++ b/server/internal/ingest/ingest_test.go
@@ -285,7 +285,11 @@
func TestSaveCursorKeepsCommittedProgressAndLeavesNoTempFile(t *testing.T) {
states := t.TempDir()
- abs := filepath.Join(t.TempDir(), "a.jsonl")
+ dir := t.TempDir()
+ abs := filepath.Join(dir, "a.jsonl")
+ if err := os.WriteFile(abs, make([]byte, 200), 0o600); err != nil {
+ t.Fatal(err)
+ }
// A run that read the file earlier must not rewind one that finished first.
if err := SaveCursor(states, Cursor{Abs: abs, Size: 200, ModTime: "2026-08-30T06:14:01Z", LastLine: 10}); err != nil {
@@ -299,6 +303,9 @@
}
// A rewrite that shrank the file is the one case allowed to rewind.
+ if err := os.Truncate(abs, 40); err != nil {
+ t.Fatal(err)
+ }
if err := SaveCursor(states, Cursor{Abs: abs, Size: 40, ModTime: "2026-08-30T06:14:03Z", LastLine: 2}); err != nil {
t.Fatal(err)
}
--- /dev/null
+++ b/server/internal/ingest/cursor_stale_size_test.go
@@ -0,0 +1,33 @@
+package ingest
+
+import (
+ "os"
+ "path/filepath"
+ "testing"
+)
+
+// TestSaveCursorRejectsRewindFromStaleSizeSnapshot pins the invariant #139 is
+// about: Run samples FileState before it takes the cursor lock, so a save can
+// arrive carrying a Size older than the committed one. With a real transcript
+// on disk that never shrank, only LastLine may decide the merge.
+func TestSaveCursorRejectsRewindFromStaleSizeSnapshot(t *testing.T) {
+ states := t.TempDir()
+ abs := filepath.Join(t.TempDir(), "a.jsonl")
+ if err := os.WriteFile(abs, make([]byte, 30000), 0o600); err != nil {
+ t.Fatal(err)
+ }
+
+ // A further-along run commits first and saw the whole file.
+ if err := SaveCursor(states, Cursor{Abs: abs, Size: 30000, ModTime: "2026-09-01T00:00:00Z", LastLine: 200}); err != nil {
+ t.Fatal(err)
+ }
+ // A slower run saves second with a stat taken before that commit.
+ if err := SaveCursor(states, Cursor{Abs: abs, Size: 20000, ModTime: "2026-09-01T00:00:01Z", LastLine: 100}); err != nil {
+ t.Fatal(err)
+ }
+
+ got := LoadCursor(states, abs)
+ if got.LastLine != 200 || got.Size != 30000 {
+ t.Fatalf("cursor regressed to %+v, want LastLine 200 / Size 30000", got)
+ }
+}What it does:
An alternative I'd also accept: keep Unchanged by this patch, and still open from my earlier comment: the |
|
Closing under the fork-workflow decision recorded on 2026-09-03: repository #111 and #139. This one needs a specific warning attached to it before it is It is three commits, and only one of them is this contributor's own work:
The first is work that was already merged in this repository under #129, What survives as a fact is the analysis in the PR description, which is sound: The commits are not lost. A closed fork PR keeps its head ref: git fetch https://github.com/bytefolk/mem.git refs/pull/147/head:pr-147Every file in this branch was therefore available to the re-doing work, whether |
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
Tracking
Refs #111
Refs #139
Supersedes #129 (rebased onto latest
mainwith OS-backed cursor lock added).Summary
Extracts the local ingestion mechanics from
server/cmd/meminto a newserver/internal/ingestpackage, so thatput --watch(#110) consumes one core instead of writing a second state layer. Also carries forward the concurrent-writer safeguard from #139.What moved to
server/internal/ingestingest.Walk,ingest.HasJSONLExtension,ingest.CanonicalRootingest.Cursor,CursorPath,LoadCursor,SaveCursor,FileState--dry-run/--limitsemantics, per-file degradationingest.Runscanned/ingested/deduped/failed)ingest.Reportingest.Code,ingest.ClassifyWhat stays in the connector
Cobra flags, the Qoder JSONL parser, the
/v1/memoriespayload shape,Idempotency-Keyderivation, the HTTP upload, and all stdout/stderr text.Concurrent-writer safety (from #139)
SaveCursornow acquires a per-cursor OS-backed advisory lock (flock on Unix, fcntl on AIX, LockFileEx on Windows) that covers the read/merge/write sequence. This prevents concurrent ingest processes from:LastLinewith an older valueThe lock sidecar (
<cursor>.json.lock) remains on disk after release; the OS releases the advisory lock when the descriptor or owning process exits.Behaviour preservation
Idempotency-Keyderivation~/.mem/ingest/qoder)One intentional change: a root given as relative or through a symlink is now canonicalized to its absolute path before keying anything. This prevents cursor collisions between two working directories that each contain
sessions/p.jsonland share one state dir.Requirements trace
server/internal/ingestscanned/ingested/deduped/failed, codesauth/plan_quota/provider_timeout/network/read_denied/upload_rejected/root_missing/state_corruptAcceptance criteria
Validation
go build ./...: passgo test -race ./internal/ingest/...: 14/14 passgo test -race ./cmd/mem/... -run "Ingest|Qoder": all passgofmt,go vet: clean