fix(import): serialize comment marker-dedupe across concurrent imports

.agents/pm/history/pm-github-503u.jsonl

@@ -1 +1,6 @@
 {"ts":"2026-07-17T00:17:34.148Z","author":"claude-fleet","op":"create","patch":[{"op":"add","path":"/metadata/id","value":"pm-github-503u"},{"op":"add","path":"/metadata/title","value":"Serialize marker-dedupe read-and-append across concurrent imports"},{"op":"add","path":"/metadata/description","value":"CodeRabbit (PR #33, comment 3599527301) correctly notes the native comment sync's read-markers-then-append sequence is not atomic across two concurrent 'pm github import' processes targeting the same workspace: both can observe a GitHub comment id as absent and append it twice. Individual writes go through the pm CLI (which locks per mutation), but our check-then-act spans multiple mutations. Current disposition: documented limitation (see syncGithubCommentsToAnnotations docstring) — concurrent same-repo imports are an operator edge case and worst case yields a duplicate comment, not data loss. Proper fix: cross-process lock (e.g. lockfile in workspace runtime dir) or an SDK-level conditional append (append-if-marker-absent) — the latter is worth proposing upstream."},{"op":"add","path":"/metadata/type","value":"Task"},{"op":"add","path":"/metadata/status","value":"open"},{"op":"add","path":"/metadata/priority","value":2},{"op":"add","path":"/metadata/tags","value":[]},{"op":"add","path":"/metadata/created_at","value":"2026-07-17T00:17:34.148Z"},{"op":"add","path":"/metadata/updated_at","value":"2026-07-17T00:17:34.148Z"},{"op":"add","path":"/metadata/author","value":"claude-fleet"},{"op":"add","path":"/metadata/acceptance_criteria","value":"Concurrent 'pm github import --comments-mode annotations' runs against the same workspace never duplicate a comment (verified by a parallel-import test), or the limitation is lifted via an upstream SDK conditional-append primitive"}],"before_hash":"3cc22dff72be7b14824654a7a64ea62b04799939b2fee54c1b5f52ca60bf6df0","after_hash":"23571755a1196598f1bd247d25fd76b4fc2f8c0a1867418368ba7a36fcc42891","message":""}
+{"ts":"2026-07-18T09:54:19.859Z","author":"pi-agent","op":"claim","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-07-18T09:54:19.859Z"},{"op":"add","path":"/metadata/assignee","value":"pi-agent"}],"before_hash":"23571755a1196598f1bd247d25fd76b4fc2f8c0a1867418368ba7a36fcc42891","after_hash":"399675b8b100dec1a3fca002f3de0968ea3444b2d92cc2b9ce1ff39209a18ba1"}
+{"ts":"2026-07-18T09:54:35.851Z","author":"pi-agent","op":"update","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-07-18T09:54:35.851Z"},{"op":"replace","path":"/metadata/status","value":"in_progress"}],"before_hash":"399675b8b100dec1a3fca002f3de0968ea3444b2d92cc2b9ce1ff39209a18ba1","after_hash":"9e170f0cba9f28d818380738d5794ca1af1a747f9fe0c829cea8928f16373a04"}
+{"ts":"2026-07-18T09:54:36.132Z","author":"pi-agent","op":"note_add","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-07-18T09:54:36.132Z"},{"op":"add","path":"/metadata/notes","value":[{"created_at":"2026-07-18T09:54:36.132Z","author":"pi-agent","text":"Started work in worktree .wt-pm-github-503u on branch feat/serialize-comment-import-lock. Plan: cross-process lockfile around comment marker read->append critical section, stale-lock TTL break, parallel-import test + lock unit tests."}]}],"before_hash":"9e170f0cba9f28d818380738d5794ca1af1a747f9fe0c829cea8928f16373a04","after_hash":"85e9f5570fa8e6972c3702c9f0fbad70eb13d49c72c5a415006e22fca60c7149"}
+{"ts":"2026-07-18T10:20:49.442Z","author":"pi-agent","op":"note_add","patch":[{"op":"add","path":"/metadata/notes/1","value":{"created_at":"2026-07-18T10:20:49.441Z","author":"pi-agent","text":"Implemented cross-process serialization of the comment marker read->append critical section: acquireImportLock() creates <pm-data-dir>/locks/pm-github.comment-sync.<itemId>.lock via fs.openSync(wx) (atomic O_EXCL), payload mirrors pm CLI lock convention ({id,pid,owner,created_at,ttl_seconds}) so pm gc can sweep it. Stale locks (older than 5min TTL, or dead owner PID) broken with stderr warning; unparseable payloads fall back to mtime so a live writer mid-write is never broken. Live contention waits with jittered backoff up to 30s; on timeout the item's comment sync is skipped with a warning (never run unlocked), on lock-mechanism failure it degrades to unlocked with a warning. Lock scope is per item, not per import run: unrelated issues in concurrent imports still sync in parallel. Tests: 10 new in test/import-lock.test.ts incl. cross-process acceptance test spawning two real concurrent child syncs against one workspace -> 20 unique comments, zero duplicates, loser skips all. Full suite 122/122 green x3 runs, typecheck clean."}},{"op":"replace","path":"/metadata/updated_at","value":"2026-07-18T10:20:49.442Z"}],"before_hash":"85e9f5570fa8e6972c3702c9f0fbad70eb13d49c72c5a415006e22fca60c7149","after_hash":"e8707e73f81fe4d12e12f2c5ba4b47d9074e43c86d278856b2e6c7936a2ba6bc"}
+{"ts":"2026-07-18T10:21:18.914Z","author":"pi-agent","op":"close","patch":[{"op":"remove","path":"/metadata/assignee"},{"op":"replace","path":"/metadata/updated_at","value":"2026-07-18T10:21:18.914Z"},{"op":"replace","path":"/metadata/status","value":"closed"},{"op":"add","path":"/metadata/closed_at","value":"2026-07-18T10:21:18.913Z"},{"op":"add","path":"/metadata/close_reason","value":"Fixed: cross-process per-item lockfile (acquireImportLock in index.ts) serializes the comment marker read->append section across concurrent pm github import processes. Stale locks broken by 5min TTL or dead-PID check with stderr warning; live contention waits up to 30s then skips the item's sync with a warning (never appends unlocked). Acceptance verified by parallel-import test: two concurrent OS processes syncing 20 identical comments into one workspace item produce exactly 20 stored comments, zero duplicate markers; lock unit tests cover acquire/contend/stale-break. Full suite 122/122 green."}],"before_hash":"e8707e73f81fe4d12e12f2c5ba4b47d9074e43c86d278856b2e6c7936a2ba6bc","after_hash":"9cc3905782b940ea4d58223ba582530eea2fb57495475eb94fac36791693b09d"}

.agents/pm/tasks/pm-github-503u.toon

@@ -2,11 +2,16 @@ id: pm-github-503u
 title: Serialize marker-dedupe read-and-append across concurrent imports
 description: "CodeRabbit (PR #33, comment 3599527301) correctly notes the native comment sync's read-markers-then-append sequence is not atomic across two concurrent 'pm github import' processes targeting the same workspace: both can observe a GitHub comment id as absent and append it twice. Individual writes go through the pm CLI (which locks per mutation), but our check-then-act spans multiple mutations. Current disposition: documented limitation (see syncGithubCommentsToAnnotations docstring) — concurrent same-repo imports are an operator edge case and worst case yields a duplicate comment, not data loss. Proper fix: cross-process lock (e.g. lockfile in workspace runtime dir) or an SDK-level conditional append (append-if-marker-absent) — the latter is worth proposing upstream."
 type: Task
-status: open
+status: closed
 priority: 2
 tags: []
 created_at: "2026-07-17T00:17:34.148Z"
-updated_at: "2026-07-17T00:17:34.148Z"
+updated_at: "2026-07-18T10:21:18.914Z"
+closed_at: "2026-07-18T10:21:18.913Z"
 author: claude-fleet
 acceptance_criteria: "Concurrent 'pm github import --comments-mode annotations' runs against the same workspace never duplicate a comment (verified by a parallel-import test), or the limitation is lifted via an upstream SDK conditional-append primitive"
+notes[2]{created_at,author,text}:
+  "2026-07-18T09:54:36.132Z",pi-agent,"Started work in worktree .wt-pm-github-503u on branch feat/serialize-comment-import-lock. Plan: cross-process lockfile around comment marker read->append critical section, stale-lock TTL break, parallel-import test + lock unit tests."
+  "2026-07-18T10:20:49.441Z",pi-agent,"Implemented cross-process serialization of the comment marker read->append critical section: acquireImportLock() creates <pm-data-dir>/locks/pm-github.comment-sync.<itemId>.lock via fs.openSync(wx) (atomic O_EXCL), payload mirrors pm CLI lock convention ({id,pid,owner,created_at,ttl_seconds}) so pm gc can sweep it. Stale locks (older than 5min TTL, or dead owner PID) broken with stderr warning; unparseable payloads fall back to mtime so a live writer mid-write is never broken. Live contention waits with jittered backoff up to 30s; on timeout the item's comment sync is skipped with a warning (never run unlocked), on lock-mechanism failure it degrades to unlocked with a warning. Lock scope is per item, not per import run: unrelated issues in concurrent imports still sync in parallel. Tests: 10 new in test/import-lock.test.ts incl. cross-process acceptance test spawning two real concurrent child syncs against one workspace -> 20 unique comments, zero duplicates, loser skips all. Full suite 122/122 green x3 runs, typecheck clean."
+close_reason: "Fixed: cross-process per-item lockfile (acquireImportLock in index.ts) serializes the comment marker read->append section across concurrent pm github import processes. Stale locks broken by 5min TTL or dead-PID check with stderr warning; live contention waits up to 30s then skips the item's sync with a warning (never appends unlocked). Acceptance verified by parallel-import test: two concurrent OS processes syncing 20 identical comments into one workspace item produce exactly 20 stored comments, zero duplicate markers; lock unit tests cover acquire/contend/stale-break. Full suite 122/122 green."
 body: ""

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+### Other
+
+- Serialize marker-dedupe read-and-append across concurrent imports ([pm-github-503u](https://github.com/unbraind/pm-github/blob/main/.agents/pm/tasks/pm-github-503u.toon))
+
 ## 2026.7.17 - 2026-07-17
 
 ### Added