fix(ingest): bound cursor lock waits so a wedged peer cannot hang ingest

[PeterGuy326]

Refs #139.

#217 already extracted the qoder checkpoint lock into server/internal/ingest. This PR no longer reintroduces server/cmd/mem/qoder_checkpoint*.go.

It ports the remaining review items onto the live package:

• non-blocking LOCK_EX|LOCK_NB / LOCKFILE_FAIL_IMMEDIATELY
• 5s timeout so contention becomes an error (ingest already warns and continues)
• subprocess test that a second acquire fails instead of hanging

Head: 1271102 on current main.

[PeterGuy326]

CI baseline dependency: the current main Web audit gate fails on published js-yaml/Vitest/postcss-selector-parser advisories, independently of this checkpoint diff. Separate maintenance PR #192 refreshes only web/package-lock.json and has local audit/lint/typecheck/build PASS. #191 is now stacked on #192; merge/rebase #192 first, then #191 should be retargeted to main. The #191 application diff remains checkpoint-only.

[PeterGuy326]

Candidate: e44e543d4d8e8f277416d33bbba3f01e1ae14831.

Independent automated preflight (Mendel, not the implementation worker): PASS, bounded source/CI/compile-only scope. This is not a human APPROVE.

Checked retained lock inode, lock-held reread and whole-record maximum selection, private unique staging, checked sync/close, and rename before unlock. Unsupported platforms fail before writing. The Go files equal the final #140 implementation, but no old vote is inherited. Current exact-head CI passes the full race suite; all 20 checks are green. Linux test, Windows and AIX cross-compilation and diff/format checks passed. Reviewer-local runtime execution is NOT VERIFIED; no temporary native macOS test executable was run.

Still stacked on #192. Land the baseline through independent human review, then retarget and recheck the final candidate.

[Bindy-lbb]

Review summary (verified independently)

• Head SHA confirmed at e44e543d (matches). CI confirmed 20/20 green independently.
• Reviewed qoder_checkpoint.go + the new lock files: per-checkpoint OS advisory lock held across reread-compare-write with no TOCTOU window; the unsupported-platform fallback fails closed rather than silently no-op'ing; unique per-attempt temp files via os.CreateTemp fix the previous shared-tmp-path collision. Tests spawn real subprocesses to exercise cross-process contention and lock-holder-crash reclaim — solid, not stubbed.

Minor, currently-unreachable caveat: qoder_checkpoint_lock_aix.go's fcntl lock is per-process+inode (reentrant within the same process, dropped on any fd close from that process). Not exploitable today since the only caller (cmds_ingest.go) is a sequential, single-threaded loop — but worth a tracking note if ingest is ever parallelized within one process.

Same base/retarget blocker as #189/#190 — this PR bases off #192's branch, not main.

[sun-970]

Review

Overall design is sound — lock granularity, coverage, max-cursor semantics, and process-crash recovery paths are all handled correctly. Tests using real subprocesses to verify cross-process semantics are solid. A few points worth noting:

1. package-lock.json change scope

The bumps for vitest 4.1.10→4.1.11, js-yaml 4.3.1→4.3.2, and postcss-selector-parser 6.1.2→6.1.4 are unrelated to the checkpoint fix. The PR description mentions #192 was already merged for Web dependency audit baseline — are these bumps brought in automatically by rebasing on main, or should they be split into a separate PR? If the former, no issue — just confirming.

2. Orphan lock file residue

The comment explains why .lock files are not cleaned up (unlinking a locked file could create a second inode), which is the correct tradeoff. However, if a user manually cleans up checkpoint files in stateDir, the corresponding .lock files will be left behind. Consider adding orphan lock cleanup in loadQoderCheckpoint or stateDir cleanup logic, or mentioning it in the docs. Non-blocking.

3. 100ms blocking assertion may be flaky

In TestSaveQoderCheckpointSerializesAndRecoversAfterOwnerExit, requireNoQoderCheckpointSignal(t, lowReady, 100*time.Millisecond) asserts that the low writer is blocked. Under extreme load in CI, if the high writer unexpectedly releases early, this assertion could be flaky. The actual risk is low since the high writer is controlled via file signaling, but widening the window (e.g. 200–500ms) would add margin.

4. _other.go unlock returns nil

For unsupported platforms, lock already returns an error, so unlock should theoretically never be called. Returning nil is safe, but returning an error (consistent fail-closed semantics with lock) could surface issues faster in unexpected call chains. Style preference, non-blocking.

[waterbro-8]

REQUEST-CHANGES — the lock is correct, the tests don't cover what the lock is for

Two corrections to what I expected going in, stated up front because they change what's left to
fix: locking a separate p + ".lock" sidecar is the right design (a lock file you
rename over would silently split into two inodes, and your comment on
qoder_checkpoint_lock.go:8-12 shows you thought about exactly that), and
TestSaveQoderCheckpointSerializesAndRecoversAfterOwnerExit is a real exclusion test —
delete the lock entirely and it fails. Also cp = current on the losing side keeps the whole
record rather than field-merging, so no torn (LastLine, Size) pair can be committed.

What's left is that the unbounded wait is a hang, not an error, and the property your doc
comment claims about lock extent has zero coverage.

S2-1 — no bound on the wait, on both platforms

• qoder_checkpoint_lock_unix.go:12 → unix.Flock(int(file.Fd()), unix.LOCK_EX) — no
  LOCK_NB, no deadline.
• qoder_checkpoint_lock_windows.go:14-21 → LockFileEx(…, LOCKFILE_EXCLUSIVE_LOCK, 0, 1, 0, …)
  — no LOCKFILE_FAIL_IMMEDIATELY.

Advisory locks are released when the owning process exits, not when it stops. A peer that is
SIGTSTP'd, wedged in a hang, or sitting on a slow/network-backed state dir while holding the
lock blocks the next mem ingest qoder forever, with no output at all.

That contradicts the contract in the comment you just wrote — "Errors are returned (callers may
warn without failing the whole ingest)" (qoder_checkpoint.go:63-64) — because a hang is not
an error and never returns to the caller who would warn. The ingest already tolerates a failed
checkpoint (the caller only prints warn: at cmds_ingest.go:246-251), so degrading the same
way on lock contention costs nothing.

Ask: LOCK_EX|LOCK_NB with a bounded retry and an explicit give-up path (or a deadline
around the blocking wait), so a wedged peer produces a warning instead of a silent freeze.

S2-2 — nothing pins the release to after the commit

The helper's save action calls saveQoderCheckpoint — which locks, merges, writes and
releases — and only then signals readiness: checkpointHelperWriteSignal(ready) sits at
qoder_checkpoint_test.go:75, after the switch at :51-73. So in
TestSaveQoderCheckpointKeepsHighestLastLineAcrossIndependentProcesses (:182) the test observes
highReady and then starts the low writer — high's lock was long gone before low opened the
file. The two critical sections never overlap:

• Delete the entire lock block and that test still passes. It is a max/merge test (a worthwhile
  one — it does pin that Size == 33 and ModTime don't regress, which is not nothing), but it
  is not a concurrency test despite its name and its comment's emphasis on "independent live
  processes (the high writer still waits for release)".
• Conversely, move lock.release() to before saveQoderCheckpointLocked at
  qoder_checkpoint.go:80 and both tests still pass: test 2's high writer uses the
  save-hold-lock action and never releases through release(), and nothing observes the low
  writer's release timing.

So the single property your doc comment claims — "the per-cursor OS-backed lock covers the
read/merge/write sequence" — is the one property with no test behind it, and it's the property
the fix depends on: a lock released before the rename commits protects the read but not the
write, and the lost update is back.

Ask: drive test 1's high writer with the save-hold-lock action you already built (:56), so
the overlap is real, and rename it to match what it proves once it does.

S3 — Size/ModTime are injected, not derived

MEM_QODER_CHECKPOINT_SIZE is read from the environment at qoder_checkpoint_test.go:33 and
the tests hand-pair 33 with LastLine: 12 and 4 with LastLine: 4, while the real file is
one fixed 33-byte write. So the coupling that actually matters in production —
(LastLine, Size) coming from different moments — is invisible to these tests.

That's an observation about the caller, which #191 does not touch and I am not asking you to
touch: cmds_ingest.go:243-244 samples fileState(abs) after the per-line POST loop, so
LastLine reflects lines successfully posted while Size reflects a later file size.
loadQoderCheckpoint's only reset trigger is fi.Size() < cp.Size, so a cursor that recorded a
size larger than the file ever had at that line count raises the bar for truncation detection.
Tracked separately, please — I raise it only so nobody reads this lock as covering that
coupling, and to be explicit that I checked whether a concurrent merge could strand a line
range and concluded it cannot, since the merge copies the whole record.

Small and non-blocking

• _lock_other.go returns an error on unsupported platforms, which surfaces as a warn: and
  leaves ingest working. Good default.
• The lockfile churn is properly isolated in its own
  chore(web): carry audited lockfile baseline from #192 commit, which keeps the Go change
  reviewable.

Provenance

Static read of head f91fb4404 (qoder_checkpoint.go, the three lock files,
qoder_checkpoint_test.go) plus cmds_ingest.go and qoder_checkpoint.go on main 3c13f04.
I ran no Go tests. The two mutation claims in S2-2 are reasoned from the helper's
signal ordering, not demonstrated by deleting code and re-running — if either is wrong, say so
and I'll retract it, since they are the basis for the request. Windows behaviour is unread
experimentally: I read LockFileEx's flags but never ran the bat.

[waterbro-8]

Review: the lock is right, and nothing here proves it does anything

The lock design is sound and I want to say that plainly before the objection.

• qoder_checkpoint_lock.go opens <checkpoint>.lock and deliberately does not unlink it, with the correct reason in the comment: unlinking a locked file lets a second process lock a different inode. That is the mistake most implementations of this pattern make.
• _unix.go / _windows.go / _other.go is the right three-way split, and LockFileEx on a one-byte range does have the same lifecycle guarantee as flock — released on process exit, not on handle leak.
• Wrapping the read/merge/write in saveQoderCheckpoint rather than only the write is correct, because the merge reads loadQoderCheckpoint and that read has to be inside the critical section.
• Replacing the fixed p + ".tmp" with os.CreateTemp fixes a real collision: two writers under the old scheme shared one staging path. The added Sync before rename is also a genuine improvement over the previous version.
• saveQoderCheckpointLocked refusing to let a delayed low cursor overwrite a newer high one is the right merge rule.

The gap: neither test fails if you delete the lock

TestSaveQoderCheckpointKeepsHighestLastLineAcrossIndependentProcesses asserts that after a high writer (12) and a low writer (4), LastLine == 12. But that result comes entirely from the merge in saveQoderCheckpointLocked:

current := loadQoderCheckpoint(stateDir, cp.Abs)
if current.LastLine > cp.LastLine {
	cp = current
}

Two writers running fully concurrently, with no lock at all, would still land on 12. This test measures the merge rule and exercises the lock only incidentally.

TestSaveQoderCheckpointSerializesAndRecoversAfterOwnerExit does touch the lock, and its second half — proving the OS releases the lock when the holder os.Exits without closing — is the most valuable thing in this PR.

So the missing case is mutual exclusion itself: no test asserts that B cannot complete while A holds the lock, and that B's result therefore lands after A's rather than being interleaved with it.

Related, requireNoQoderCheckpointSignal(t, lowReady, 100*time.Millisecond) is a fixed 100 ms race window. On a loaded Windows CI runner that is simultaneously at risk of being too short (B has not reached the lock yet, LockFileEx contention plus scheduler jitter — false pass) and being the only thing standing between the test and a true failure. A timing window is the wrong primitive for asserting ordering.

What I am asking for

Add one case: A takes the lock via save-hold-lock and holds it until the parent test explicitly releases it; B starts a plain save with a LastLine higher than A's. Assert:

1. B's started signal fires (B reached the lock),
2. B's ready signal does not fire while A still holds the lock — and the observation window is bounded by A's release, not by a Sleep,
3. after release, B's ready fires and the stored LastLine is B's higher value.

Point 3 is the one that makes this a lock test. With B's cursor higher than A's, the merge rule cannot produce the final value — only B actually running after A can. That is the property acquireQoderCheckpointLock exists to provide, and right now nothing in the suite would go red if it stopped providing it. A lock whose purpose is untested is, for regression purposes, the same as no lock.

Also

web/package-lock.json is +50/−50 of unrelated vitest / js-yaml / postcss bumps, byte-identical to the copy in #190 (same blob 28a95f0b2f19). Please drop it from both PRs and land the dependency bump separately — otherwise every future rebase of either branch conflicts on a file neither change is about.

[waterbro-8]

Draft + conflicting, and I previously requested changes. Not merging. Rebase onto current main (which now has #194) and address the outstanding review before asking again.

[waterbro-8]

#217 已经把 cursor 锁搬到 `server/internal/ingest`。原先 cmd/mem 的 checkpoint 文件叠回去会变成死代码，所以本 PR 改写成对现包的补丁。

CHANGES_REQUESTED 对应：

• S2-1：非阻塞锁 + 5s 超时，卡住的对端变成 error 而不是无限等。
• S2-2：用独立 helper 进程占锁，断言第二次 `acquireCursorLockWithTimeout` 失败（同进程 flock 测不了 OS 锁）。

请再审。