Skip to content

[Bug] Prevent concurrent Qoder checkpoint regression and staging collisions #139

Closed
Closed
[Bug] Prevent concurrent Qoder checkpoint regression and staging collisions#139
Labels
bugSomething isn't working

Description

@PeterGuy326
PeterGuy326
opened on Aug 31, 2026

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.

Activity

  1. PeterGuy326
    added
    bugSomething isn't working
    on Aug 31, 2026

  2. PeterGuy326 commented on Aug 31, 2026

    @PeterGuy326
    PeterGuy326
    on Aug 31, 2026
    CollaboratorAuthor
    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.

  3. PeterGuy326 commented on Aug 31, 2026

    @PeterGuy326
    PeterGuy326
    on Aug 31, 2026
    CollaboratorAuthor

    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.

  4. sun-970 commented on Sep 1, 2026

    @sun-970
    sun-970
    on Sep 1, 2026
    Contributor

    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.

  5. waterbro-8 commented on Sep 2, 2026

    @waterbro-8
    waterbro-8
    on Sep 2, 2026
    Collaborator

    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)).

  6. waterbro-8
    added a commit that references this issue on Sep 7, 2026

    Merge branch 'main' into codex/fix-139-cross-process-checkpoint

    de9a15b

  7. sun-970 commented on Sep 8, 2026

    @sun-970
    sun-970
    on Sep 8, 2026
    Contributor

    #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.

  8. PeterGuy326 commented on Sep 10, 2026

    @PeterGuy326
    PeterGuy326
    on Sep 10, 2026
    CollaboratorAuthor

    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.

  9. PeterGuy326 commented on Sep 10, 2026

    @PeterGuy326
    PeterGuy326
    on Sep 10, 2026
    CollaboratorAuthor

    补充当前状态：

  10. waterbro-8
    added a commit that references this issue on Sep 10, 2026

    chore(web): refresh audited development dependencies (#192)

    87db0df

  11. waterbro-8 commented on Sep 17, 2026

    @waterbro-8
    waterbro-8
    on Sep 17, 2026
    Collaborator

    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.

  12. PeterGuy326
    added 2 commits that reference this issue on Sep 18, 2026

    fix(ingest): add OS-backed cursor lock for concurrent writer safety

    b5000f6

    fix(ingest): add OS-backed cursor lock for concurrent writer safety

    65ac6ac
  13. waterbro-8
    added a commit that references this issue on Sep 18, 2026

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

    1271102
  14. Bindy-lbb
    added a commit that references this issue on Sep 19, 2026

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

    fa63a9a

  15. waterbro-8 commented on Sep 20, 2026

    @waterbro-8
    waterbro-8
    on Sep 20, 2026
    Collaborator

    平台组分工补派（冯浩然 / waterbro-8）

  16. waterbro-8 commented on Sep 20, 2026

    @waterbro-8
    waterbro-8
    on Sep 20, 2026
    Collaborator

    平台组群成员派活（冯浩然 / waterbro-8）

  17. waterbro-8 commented on Sep 23, 2026

    @waterbro-8
    waterbro-8
    on Sep 23, 2026
    Collaborator

    关闭验收：并发 checkpoint 路径已由 PR #217 抽入共享 ingest core，并由 PR #191 完成 OS-backed 非阻塞锁、5 秒有界等待及 subprocess 竞争回归测试（merge fa63a9a；final head 1271102）。final head 获 waterbro-8 与 Bindy-lbb 正式 APPROVED，全部 hosted checks 通过，当前 main 继续全绿。共享 staging/游标并发回退问题已解决。

  18. waterbro-8
    closed this as completedon Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions