fix(ingest): serialize qoder checkpoint writers

[sun-970]

Tracking

Refs #139

Supersedes #140 (rebased onto latest main to resolve CHANGELOG.md conflict with the security headers entry that landed after the original PR branch was cut).

Consumed requirement revision: #139 R1. This PR must not auto-close the issue; merge and product acceptance remain separate decisions.

Scope

Repairs only the local Qoder checkpoint persistence path on current main. It adds a persistent per-checkpoint OS advisory-lock sidecar, merges competing writes under that lock without allowing LastLine or associated diagnostic state to regress, and replaces the shared *.tmp name with a unique private staging file plus sync and atomic rename.

Traceability

Requirement	Acceptance criterion	Implementation / evidence
REQ-001	AC-001	Lock-held reread preserves the complete higher checkpoint; deterministic independent processes submit 12 then stale 4 and final state remains the full 12 checkpoint.
REQ-002	AC-001, AC-002	Retained sidecar inode uses OS-backed locking (flock / AIX fcntl / Windows LockFileEx); staging uses same-directory CreateTemp; an owner process exits through os.Exit while holding the lock and a waiting writer proceeds after kernel release.
REQ-003	AC-003	JSON schema, caller interface, and normal load behavior are unchanged; only private .lock and transient private temp files are added beside local state.

Validation

Exact candidate head: 53118ab (rebased onto origin/main); base: cc727db0bc72655f299166de1f60756f5c686cc7.

• Focused child-process tests and race tests pass.
• go vet ./..., gofmt, and build pass.
• Original PR fix(ingest): serialize qoder checkpoint writers #140 passed full CI at head b9226a6 and received an independent approval from @waterbro-8 with adversarial discrimination checks.

Risk / rollback

The lock is advisory and applies only to participating mem checkpoint writers on a filesystem with normal OS locking semantics. A live writer can block a later save rather than allowing unsafe lock stealing; existing caller behavior reports a checkpoint-save error and does not advance the cursor. Revert this PR to restore the prior writer. No remote API, transcript format, release artifact, dependency, or repository-setting change is included.

[waterbro-8]

Review of 53118ab. This is an evidence comment, not an approval or a request-changes — merge and product acceptance remain separate decisions per R1.

I checked out the candidate head and its actual parent 4aa06eb and ran them locally (linux/amd64).

Summary: the implementation looks correct and I reproduced both the defect and the fix. Four things stand between this and merge.

1. AC-003 is not yet met: CI has never run at 53118ab

R1's AC-003 requires "exact-head CI pass". At 53118ab there are three check suites, all action_required, and zero check runs — no CI has executed on this head. The only CI evidence in the ledger belongs to #140 at b9226a6 on an older base; the approval I submitted on #140 predates the rebase, and #141 itself has no reviews. This needs a maintainer to approve the workflow runs before anything else is decidable.

Accuracy note on the ledger: base: cc727db… is the base pinned in the R1 spec, not this branch's parent, which is 4aa06eb. main is now 10d4bf7.

2. The lock wait is unbounded, and it is not interruptible

qoder_checkpoint_lock_unix.go:12 calls unix.Flock(fd, LOCK_EX) with no timeout, and the critical section now includes an fsync (tmp.Sync()), so serialized saves across N writers scale with fsync latency.

The larger problem is cancellation: cmd/mem/main.go:25 installs signal.NotifyContext(context.Background(), os.Interrupt), which suppresses the default terminate action, while runIngestQoder calls the API with context.Background() (cmds_ingest.go:210) and never observes that context. So once a save is waiting behind a wedged or stopped peer, Ctrl-C does nothing — I measured a process blocked in the flock path still alive 5s after SIGINT to its process group, requiring SIGKILL. The pre-change writer could not block at all, so this is a new failure mode rather than a pre-existing one.

Suggest LOCK_EX|LOCK_NB in a bounded retry loop (5–10s) that returns an error on exhaustion. That drops straight into the existing "warn and don't advance" contract at cmds_ingest.go:251, and keeps the CLI responsive if the wait observes cmd.Context(). LockFileEx without LOCKFILE_FAIL_IMMEDIATELY blocks the same way on Windows.

3. A crash mid-save now leaves a permanently orphaned staging file

Verified by SIGKILL between CreateTemp and Rename:

• at 53118ab: .bd1b9a31….json.tmp-943010543 stays in the state dir; nothing ever removes .-prefixed temps.
• at 4aa06eb: leaves a single <hash>.json.tmp, which the next save overwrites — self-cleaning, also confirmed.

So the change adds a small permanent garbage accumulation in exactly the crash scenario REQ-002 is about. Strictly, REQ-002's "no permanently orphaned" clause attaches to the lock, not to staging files, and I verified that lock claim holds. Suggest sweeping .<base>.tmp-* immediately after acquiring the lock — the lock is precisely what makes that provably safe, since only the current owner can create those names.

4. Three of the five new platform files are never compiled by any job

• .github/workflows/ci.yml runs the Go build/vet/test jobs on ubuntu-24.04 only — the go test -race at ci.yml:132 is Linux-only.
• .github/workflows/release.yml cross-builds exactly six targets: linux amd64/arm64, darwin amd64/arm64, windows amd64/arm64.

qoder_checkpoint_lock_windows.go therefore ships on 2 of 6 release assets but is never built or tested before release; qoder_checkpoint_lock_other.go is never compiled; and qoder_checkpoint_lock_aix.go is neither compiled in CI nor part of the release matrix — AIX looks like dead code here, carrying FcntlFlock assumptions nothing validates.

I cross-compiled by hand to check whether anything is actually broken today, and nothing is: GOOS=linux,darwin,windows,freebsd,openbsd,netbsd,dragonfly,solaris,illumos,aix,android all build ./cmd/mem/ at 53118ab, identically to 4aa06eb (ios and plan9 already fail at the base for unrelated reasons). go list confirms tag selection: linux/darwin/android/bsd → _unix.go (android resolves through Go's implicit linux tag), windows → _windows.go, aix → _aix.go, js/wasip1/plan9 → _other.go.

Two suggestions: add a compile-only step (GOOS=windows go build ./..., plus aix if you mean to keep it) to the lint job so a build-tag mistake cannot survive to release time; and decide whether _other.go failing closed is really the behaviour you want — if it is ever selected, saveQoderCheckpoint always errors, the cursor never advances, and every run re-ingests the full transcript.

What I verified as correct

Recording this because the ledger's other claims hold up:

• Reproduced the defect at 4aa06eb: 40 rounds × 16 independent processes submitting 1..16 → 0 rounds ended at the correct maximum, the final cursor landed as low as 1, and 94 of 640 saves failed with commit checkpoint: rename …json.tmp: no such file or directory — the shared staging collision. The same stress at 53118ab: 40/40 correct, exactly one .lock, no leftover staging files.
• The merge rule is coherent with the pre-existing truncation detector, which is the subtlest part of this change: because current is loaded inside the lock, qoder_checkpoint.go:53 has already zeroed LastLine for a truncated transcript, so retaining the whole higher record cannot mask a rewrite or reintroduce line skipping. Malformed cursors likewise yield LastLine 0 and never block a valid save, so REQ-003's recovery behaviour is preserved.
• No path collisions: <hash>.json.lock and .hash.json.tmp-* match neither the *.json glob in cmds_ingest_test.go:256 nor the .jsonl transcript walk, and ~/.mem/ingest/qoder is disjoint from ~/.qoder/projects.
• golang.org/x/sys v0.47.0 was already a direct dependency, so not touching go.mod is correct (and satisfies the REQ-003 nonGoal about new dependencies).
• gofmt -l, go vet ./cmd/mem/ and go build ./... are clean; the new tests pass -race -count=3; the full cmd/mem package passes.
• The head is byte-identical to fix(ingest): serialize qoder checkpoint writers #140 under server/cmd/mem, so "the code changes are identical" is accurate.
• CHANGELOG placement is right: a single ### Fixed under [Unreleased], no duplicated heading.

Optional test additions, not blockers: both new tests cover a single interleaving each, so nothing yet asserts REQ-002's unique-staging-name half (that needs two concurrent ascending writers), and the N-writer stress above is cheap to keep. The negative assertion in requireNoQoderCheckpointSignal is well designed though — it can only fail on a genuine lock leak.

All local results are from linux/amd64 at 53118ab and do not substitute for exact-head CI.

[waterbro-8]

Request for cross-platform verification — the Windows path ships untested

Following up on my evidence comment above. Everything I reported there is linux/amd64 only, and the gap it cannot close is platform coverage: ci.yml compiles and tests Go solely on ubuntu-24.04, while release.yml publishes six binaries of which two are Windows. So qoder_checkpoint_lock_windows.go reaches users having never been compiled by any job, let alone run. I have no Windows or macOS hardware here, which is exactly why I am asking rather than claiming.

If you can run this on a Windows or Apple-silicon/Intel macOS machine, please reply with the output.

Checkout (no gh required):

git clone https://github.com/bytefolk/mem.git && cd mem
git fetch origin pull/141/head && git checkout FETCH_HEAD
cd server && go build ./... && go vet ./...

Then the focused tests:

go test -v -count=2 -run QoderCheckpoint ./cmd/mem/

Add -race if it is available on your platform — on Windows the race detector typically requires CGO plus a GCC toolchain, so if -race fails to build, run it without and say so explicitly; that is a toolchain note, not a result about this PR.

What each part of that test run actually proves, so a green run means something specific:

1. LockFileEx genuinely serializes two independent processes on your filesystem — TestSaveQoderCheckpointSerializesAndRecoversAfterOwnerExit asserts the second writer stays blocked while the first is alive.
2. The kernel reclaims an abandoned lock. The doc comment in qoder_checkpoint_lock_windows.go asserts that LockFileEx releases on process exit; the same test kills a holder that never calls release() and requires the waiter to proceed. This assertion is unverified anywhere today.
3. tmp.Chmod(0o600) does not error on NTFS. If it did, every checkpoint save would fail, not just under contention.
4. os.Rename of the private staging file onto an existing checkpoint succeeds while another process holds the .lock sidecar open.
5. Cursor monotonicity: TestSaveQoderCheckpointKeepsHighestLastLineAcrossIndependentProcesses must land on 12, never 4.

On macOS the build risk is lower (_unix.go is shared with Linux and does compile here), but flock on APFS and the child-process timing in both tests have never been exercised on darwin — a -race run there is worth having.

One extra report that would be high-value and is cheap: if you keep $HOME (and therefore ~/.mem) on NFS, SMB, or a FUSE-based home directory, please run the focused tests anyway and paste the output. Locking on such mounts can return ENOLCK/EOPNOTSUPP rather than succeeding, and this design deliberately fails closed — a failed acquisition means the cursor never advances and every subsequent mem ingest qoder re-ingests the whole transcript. I could not test that from here.

Please state your OS version, go version, GOOS/GOARCH, and filesystem (NTFS / APFS / ext4 / NFS …) with the results, and note which of the five points above you covered. By this repo's ladder in docs/maintainers/triage.md, a deterministic run on a non-Linux platform is evidence:e3-reproduced; a non-author checking the acceptance criteria end to end would be e4-e2e. I have left labelling to a maintainer, so please set whichever level the submitted evidence actually supports.

This is in addition to, not instead of, the blocker above: workflow approval for 53118ab still has to happen before exact-head CI exists at all.

[waterbro-8]

Closing under the fork-workflow decision recorded on 2026-09-03: repository
automation is not being enabled for fork pull requests, so a fork head cannot
carry a CI result, and every acceptance gate in this repository is written
against checks that ran. Nothing in this comment is a judgment that the work is
wrong; where it is right, it is re-landed on an organization branch instead.

#139, re-landed on an organization branch as #140.

One commit, and its metadata makes it unmergeable independent of the fork
policy: git log shows Author: PeterGuy326 <47820304+PeterGuy326@users.noreply.github.com> with
Committer: liyuanyang <liyuanyang@users.noreply.github.com>. That is the exact
shape #145 was closed over. A commit authored by an organization member and
committed by a non-member attributes work to the wrong people in the durable
record, and #140 is the same fix carrying one identity throughout.

Note that the cherry-pick 保留原作者署名 clause in the 2026-09-03 decision
does resolve this particular commit cleanly — git cherry-pick preserves the
author and assigns a fresh committer — so if #140's implementation were found
lacking, re-applying this commit would not import a bad attribution. Nothing is
imported here, because #140 already exists.

The commits are not lost. A closed fork PR keeps its head ref:

git fetch https://github.com/bytefolk/mem.git refs/pull/141/head:pr-141

Every file in this branch was therefore available to the re-doing work, whether
or not it was used.

[waterbro-8]

Closing as a content duplicate of #140. This is a mechanical finding, not a judgement on the work.

Evidence:

• All 7 shared code files are byte-identical to fix(ingest): serialize qoder checkpoint writers #140 (git hash-object matches on qoder_checkpoint.go, all five qoder_checkpoint_lock_{,aix,other,windows,unix}.go, and qoder_checkpoint_test.go). Only CHANGELOG.md wording differs.
• The head commit here carries the same author as fix(ingest): serialize qoder checkpoint writers #140 (PeterGuy326 <47820304+PeterGuy326@users.noreply.github.com>) and the identical author timestamp 2026-08-31T19:43:50Z as b9226a67 — i.e. the same commit re-submitted from a fork.
• fix(ingest): serialize qoder checkpoint writers #140 has a complete CI run (15/15 checks success on b9226a67); this head has zero check runs, because a fork PR sits at action_required until someone approves a run.

#139 stays open and is tracked by #140. Nothing in this branch is lost by closing it.