[Bug] Prevent concurrent Qoder checkpoint regression and staging collisions

[PeterGuy326]

Canonical requirement state (current): R1 decision · in progress (implementation candidate: #140). A dedicated minimal PR may reference this issue; merge, release, and acceptance are separate decisions.

Summary

The canonical main Qoder ingest checkpoint writer uses one shared <checkpoint>.tmp name and a final rename with no cross-process serialization. Concurrent ingests of the same transcript can overwrite a newer LastLine with an older value, or race on the same staging path. A regressed cursor causes avoidable re-ingestion and undermines the documented monotonic incremental-ingest behavior.

Source reproduction

On main @ cc727db0bc72655f299166de1f60756f5c686cc7, server/cmd/mem/qoder_checkpoint.go writes every checkpoint through p + ".tmp" and unconditionally renames it over p. Two writers for the same transcript therefore have neither a unique staging name nor a compare/serialization point before commit.

Affected area

• server/cmd/mem/qoder_checkpoint.go
• Qoder incremental ingest checkpoint persistence only.

Scope boundary

This is separate from draft PR #129 and Issue #111. It must ship as a clean current-main bug fix, not by reviving the stale draft. It does not change transcript format, the ingest command contract, remote API behavior, or checkpoint semantics other than guaranteeing non-regression under concurrent writers.

Evidence level

E2 — canonical-source race analysis; implementation must add deterministic E3-style regression evidence without real credentials or user transcript data.

[PeterGuy326]

schemaVersion: requirement-decision.v1
canonicalIssue: https://github.com/fullstack-ai-infra/mem/issues/139
revision: R1
status: ready
priority: P1
productOwner: "@PeterGuy326"
technicalOwner: "@PeterGuy326"
base:
  branch: main
  sha: cc727db0bc72655f299166de1f60756f5c686cc7
requirements:
  - id: REQ-001
    statement: Concurrent writers for the same Qoder transcript must never leave a checkpoint whose LastLine is lower than the greatest successfully submitted LastLine.
  - id: REQ-002
    statement: Checkpoint staging and commit must be safe across independent processes, with no shared temporary-path collision and no permanently orphaned lock after a process exits unexpectedly.
  - id: REQ-003
    statement: Preserve the existing on-disk checkpoint JSON schema, command interface, permissions, and normal missing/malformed-checkpoint recovery behavior.
acceptanceCriteria:
  - id: AC-001
    statement: A deterministic regression reproduces an interleaving that would regress or collide on the old writer and verifies the committed cursor is the maximum after competing writers.
  - id: AC-002
    statement: The focused test exercises independent-process serialization or an equivalent OS-backed primitive; a stale process-held lock becomes acquirable after process exit.
  - id: AC-003
    statement: Focused and full relevant Go tests, race detection where applicable, format/vet/build, and exact-head CI pass without changing remote ingest semantics.
nonGoals:
  - Do not implement #111 cursor package/design work.
  - Do not change transcript parsing, remote API requests, release artifacts, or repository protection.
  - Do not introduce a dependency solely for this lock when the standard library/current module capabilities suffice.
dependencies: []
allowedGitHubWrites:
  - dedicated minimal pull request referencing this issue
  - review and evidence comments
releaseAuthority: none

Product decision: R1 authorizes a dedicated current-main correctness repair only. The PR must use Refs #139; merge, release, and issue acceptance remain separate decisions.

[PeterGuy326]

Status update: implementation candidate #140 now exists at exact head b9226a67921758abe0ad136a9e1848d972a9ee7a; R1 requirements are unchanged. This moves the work item to in-progress only and does not authorize merge, release, or closure.

[sun-970]

Implementation candidate #140 has been rebased onto latest main and re-opened as #141 to resolve a CHANGELOG.md conflict. The code changes are identical; the PR is now MERGEABLE and awaiting review approval per R1.

[waterbro-8]

Not adjudicating — this is decision-support for whoever owns the call, which by this issue's own text is not me (waterbro-8 authored two of #147's three commits, so I am a participant in one of the two candidates below).

There are now two implementations of this fix, and they do not behave the same

	#140 (b9226a67, PeterGuy326)	#147 commit 785de61e (sun-970 fork + liyuanyang)
Shape	minimal: qoder_checkpoint.go + new lock files + its own tests, nothing else	bundled inside the #111 refactor: the same lock, moved into server/internal/ingest
Merge rule	if current.LastLine > cp.LastLine { cp = current }	if current.LastLine > cp.LastLine && current.Size <= cp.Size { return nil }
Cross-process staging	unique CreateTemp per save	unique CreateTemp per save
Deterministic max test	yes — qoder_checkpoint_test.go:182-230, two live processes, high commits then stale low saves, asserts LastLine and Size/ModTime are the high writer's	no — TestConcurrentSaveCursorPublishesWholeCursors still accepts LastLine 1 through 8, i.e. any winner
The other candidate's scenario, run against it	passes	fails: 12 → 4
CI on that head	15 check-runs, CI / PR Policy / Validate Agent memory all success	action_required ×3, statusCheckRollup empty, 0 check-runs — never built
Merge state	CONFLICTING, cause is CHANGELOG.md only (see #140)	MERGEABLE, REVIEW_REQUIRED, zero reviews

The middle-row difference is the whole bug. #147 samples the file size in Run (internal/ingest/ingest.go:476) and then compares it inside the lock (:322), so a slower writer that carries an older, smaller Size gets past the guard and writes its stale LastLine — which is exactly the regression this issue describes. I ran both guards against the same scenario and got 12/33 versus 4/4. A tested patch for #147 is on that PR; #140 needs no such change.

Two constraints this issue already set

1. "It must ship as a clean current-main bug fix, not by reviving the stale draft." fix(ingest): serialize qoder checkpoint writers #140 is that. refactor(ingest): extract shared local ingestion core with concurrent-writer safety #147 is the bundled form, and its own upstream — refactor(ingest): extract the shared local ingestion core #129's CHANGES_REQUESTED from @PeterGuy326 on 2026-08-31 — said "Keep the fix separate from unrelated ingestion refactoring." That requested change has not been satisfied by any PR in this lineage: refactor(ingest): extract the shared local ingestion core #129 is still draft and conflicted, and refactor(ingest): extract shared local ingestion core with concurrent-writer safety #147 carries the fix inside the refactor.
2. "implementation must add deterministic E3-style regression evidence." fix(ingest): serialize qoder checkpoint writers #140's helper-process test meets it. If refactor(ingest): extract shared local ingestion core with concurrent-writer safety #147 is chosen instead, its 1-through-8 concurrent assertion does not — and note that simply tightening that assertion to == 8 is not enough either: I tried it against refactor(ingest): extract shared local ingestion core with concurrent-writer safety #147's untouched head and it passes, because all eight writers share one Size. Catching this needs a differing-Size case.

What I'd ask the owner to decide

Which branch carries the fix. If the answer is #140, then #147's third commit should be dropped and #147 rebases onto it, with internal/ingest taking #140's merge rule as it moves the code. If the answer is #147, #140 closes and the guard patch lands first, plus a differing-Size regression test, plus maintainer-approved workflow runs on that fork head before anyone can call it CI-verified. Either way this issue should not be closed by a PR that leaves the maximum-cursor invariant failing the test the other candidate already passes.

Nothing pushed, no review submitted, no vote, no close. Evidence posted on: #147 (findings, tested patch) and #140 (#140 (comment)).

[sun-970]

#147 Follow-up Plan

Thanks to @waterbro-8 for the detailed analysis. Here is our plan for #147:

1. Separate the fix: Extract the checkpoint concurrency fix from the chore(ingest): extract the shared local ingestion core (walk, cursor, change gate, report) from the qoder connector #111 refactor in refactor(ingest): extract shared local ingestion core with concurrent-writer safety #147 into a standalone, minimal PR — in line with this issue's requirement that it "must ship as a clean current-main bug fix."
2. Align the merge guard: The new PR will adopt fix(ingest): serialize qoder checkpoint writers #140's merge rule (if current.LastLine > cp.LastLine { cp = current }) to guarantee the monotonic increment invariant.
3. Add regression tests: Introduce deterministic test cases covering differing-Size scenarios, specifically the boundary where a stale writer carries a smaller Size.
4. CI validation: Ensure the new PR passes all CI check-runs on its head, including race detection.

---

Issues Identified and Proposed Solutions

Issue 1: Merge guard logic flaw

The guard in #147 is:

if current.LastLine > cp.LastLine && current.Size <= cp.Size { return nil }

When a stale writer carries an older, smaller Size, current.Size <= cp.Size evaluates to false, the guard is bypassed, and the stale LastLine is written — which is exactly the regression this issue describes.

Solution: Remove the Size comparison and rely solely on LastLine monotonicity:

if current.LastLine > cp.LastLine { cp = current }

Issue 2: Regression test is not strict enough

The existing concurrent test in #147 (TestConcurrentSaveCursorPublishesWholeCursors) uses the same Size across all writers, so it cannot detect stale writes in differing-Size scenarios.

Solution: Add a test case that constructs an interleaved two-writer scenario:

• Writer A: commits LastLine=12, Size=large
• Writer B (stale): commits LastLine=4, Size=small
• Assert final checkpoint LastLine == 12

Issue 3: Fix coupled with refactor

#147 bundles the checkpoint fix inside the #111 ingest refactor, violating this issue's scope boundary and causing review/merge decisions to block each other.

Solution: Submit the checkpoint fix as a standalone PR first; the #111 refactor proceeds separately without carrying this bug fix.

---

We will submit the standalone fix PR referencing this issue as soon as possible.

[PeterGuy326]

Clean current-main candidate — evidence/status

A clean replacement PR is now available: #191, head 55b664dbd9753bb8de77861dbb3629c357529595. It contains only the checkpoint implementation, OS lock variants, regression tests, and CHANGELOG; the Web lockfile and unrelated synchronization from #140 are excluded.

Exact-tree local evidence:

• focused independent-process checkpoint tests: PASS
• focused go test -race -count=1 -run 'QoderCheckpoint|Checkpoint' ./cmd/mem/: PASS
• go test -count=1 ./...: PASS
• go test -race -p 1 ./...: PASS
• go vet ./...: PASS
• go build ./...: PASS
• gofmt / git diff --check: PASS

#139 remains OPEN because merge/release/product acceptance are separate R1 decisions. Windows/AIX lock paths are compile/vet verified locally, not runtime-executed here.

[PeterGuy326]

补充当前状态：

• 独立 Web 基线 PR chore(web): refresh audited development dependencies #192 已通过完整 CI（Go、PostgreSQL、Web、CodeQL、依赖审计、npm wrapper 等）。该 PR 仅更新 web/package-lock.json，用于修复当前 main 的 Web 安全审计基线。
• checkpoint 修复 PR fix(ingest): bound cursor lock waits so a wedged peer cannot hang ingest #191 仍保持仅 checkpoint 代码的范围，并堆叠在 chore(web): refresh audited development dependencies #192 基线上；未合并，等待独立评审。
• Issue [Bug] Prevent concurrent Qoder checkpoint regression and staging collisions #139 仍保持 OPEN，未提前关闭。

[waterbro-8]

Still open. Canonical candidate is draft #191 (codex/fix-139-current-main) on current main. Earlier fork #140 was closed.

I am not opening a third checkpoint PR. Tracking #191. No merge or close.