fix(import): serialize comment marker-dedupe across concurrent imports#4
Conversation
The native comment sync's read-markers-then-append sequence was not atomic across two concurrent 'pm github import' processes on the same workspace: both could observe a GitHub comment id as absent and append it twice (pm-github-503u, CodeRabbit PR #33). Add acquireImportLock(): a cross-process per-item lockfile in the workspace locks/ dir (fs.openSync 'wx', payload mirroring the pm CLI's lock convention so pm gc can sweep it). Stale locks (older than the 5 min TTL, or dead owner PID) are broken with a stderr warning; live contention waits with jittered backoff up to 30s, then the item's comment sync is skipped with a warning (never run unlocked); lock mechanism failures degrade to the pre-lock behavior with a warning. Per-item scope keeps unrelated issues in concurrent imports syncing in parallel. Tests: 10 new cases incl. a two-process parallel-import race test asserting zero duplicate markers. Full suite 122/122 green.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
WalkthroughThe change adds cross-process per-item lockfiles around GitHub comment marker synchronization, handles stale locks and contention, and adds unit, in-process, and cross-process concurrency tests. Task records and the changelog document the completed fix. ChangesComment synchronization locking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ImportProcess
participant syncGithubCommentsToAnnotations
participant acquireImportLock
participant PmComments
ImportProcess->>syncGithubCommentsToAnnotations: import GitHub comments
syncGithubCommentsToAnnotations->>acquireImportLock: acquire per-item lock
acquireImportLock-->>syncGithubCommentsToAnnotations: acquired or contended
syncGithubCommentsToAnnotations->>PmComments: read markers and append missing comments
syncGithubCommentsToAnnotations-->>ImportProcess: return added and skipped counts
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces cross-process serialization for the comment marker read-and-append critical section during GitHub imports, resolving a race condition where concurrent imports could duplicate comments. It implements a robust lockfile mechanism with stale-lock breaking, jittered backoff, and comprehensive tests. Feedback on the implementation highlights a potential issue where a failure during lockfile writing or closing could leave an orphaned lockfile on disk, and suggests cleaning up the file in the catch block to prevent blocking other processes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (fd !== undefined) { | ||
| try { | ||
| fs.closeSync(fd); | ||
| } catch { | ||
| // Ignore close errors on the failure path. | ||
| } | ||
| } |
There was a problem hiding this comment.
If fs.writeFileSync or fs.closeSync throws an error after the lockfile has been successfully created via fs.openSync(lockPath, "wx"), the empty or partially written lockfile will be left on disk. Other concurrent processes will see this file, fail to parse it, and will be blocked from acquiring the lock until the 5-minute TTL expires.
To prevent this, we should clean up and delete the lockfile in the catch block if we were the ones who successfully opened/created it (i.e., when fd is defined).
if (fd !== undefined) {
try {
fs.closeSync(fd);
} catch {
// Ignore close errors on the failure path.
}
try {
fs.unlinkSync(lockPath);
} catch {
// Ignore unlink errors on the failure path.
}
}There was a problem hiding this comment.
Valid catch — fixed in df153ba: on the failure path after a successful O_EXCL create (fd defined), the partial lockfile is now unlinked (best-effort) so contenders are not blocked until the TTL. Covered indirectly by the existing unparseable-payload tests; the cleanup keeps that path rare.
Greptile SummaryThis PR serializes native GitHub comment marker dedupe during concurrent imports. The main changes are:
Confidence Score: 5/5Safe to merge with low risk. The locking path addresses the concurrent duplicate-marker race and includes safeguards for live owners, stale breakers, and token-checked release. Tests cover the main lock edge cases and the cross-process import scenario. No files require special attention.
 T-Rex LogsWhat T-Rex did
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant A as Import process A
participant L as Workspace lockfile
participant P as pm comments SDK
participant B as Import process B
A->>L: acquireImportLock for item
L-->>A: acquired with token
B->>L: acquireImportLock for item
L-->>B: lock exists, wait while owner live
A->>P: read existing markers
A->>P: append missing marked comments
A->>L: release when token matches
B->>L: retry acquire
L-->>B: acquired
B->>P: read markers including A appends
B->>L: release when token matches
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant A as Import process A
participant L as Workspace lockfile
participant P as pm comments SDK
participant B as Import process B
A->>L: acquireImportLock for item
L-->>A: acquired with token
B->>L: acquireImportLock for item
L-->>B: lock exists, wait while owner live
A->>P: read existing markers
A->>P: append missing marked comments
A->>L: release when token matches
B->>L: retry acquire
L-->>B: acquired
B->>P: read markers including A appends
B->>L: release when token matches
Reviews (5): Last reviewed commit: "fix(lock): breaker election makes stale-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…; clean up partial lockfiles Review feedback from gemini-code-assist and greptile on PR #4: - gemini (index.ts catch path): a write/close failure after O_EXCL create left an empty/partial lockfile blocking contenders until TTL — now unlinked on the failure path. - greptile P1: TTL-only staleness could break a live slow holder mid-append, and release() blindly unlinked whatever was at lockPath (possibly a successor's lock), reopening the duplicate-marker race. Staleness is now liveness-first (dead owner PID, or own-PID leftovers not held in-process; age applies only to unparseable payloads), and release() is token-checked via a per-acquisition UUID in the payload. - In-process concurrent holders share our PID: a module-level held-locks set distinguishes legitimate in-process contention from recycled-PID leftovers. Tests: 124/124 (new: live-owner-never-age-broken, token-checked release, recycled-own-pid; updated payload/header expectations).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@index.ts`:
- Around line 824-847: Update importLockStaleReason so a lock with a
verified-live owner is not marked stale solely because its age exceeds ttlMs;
either renew the lease while held or evaluate owner liveness before applying the
TTL expiration. Preserve stale detection for dead owners and locks without a
verifiable live owner, and adjust the TTL test accordingly.
- Around line 920-938: Update the stale-lock handling around
importLockStaleReason and unlinkSync to use an ownership-safe atomic takeover
protocol: revalidate that the lock is still the same file inspected before
removing it, and perform replacement atomically so a contender cannot delete a
newer lock or acquire ownership concurrently. Preserve the existing retry
behavior when the lock changes or disappears.
- Around line 884-918: Update the lock acquisition try/catch around fs.openSync
in the lock helper to track whether the lock file was successfully created, and
unlink lockPath on any subsequent failure before returning the degraded status.
Keep descriptor cleanup intact and ensure cleanup is best-effort without masking
the original error; do not remove locks for failures that occur before file
creation.
In `@test/import-lock.test.ts`:
- Around line 304-340: Add a synchronization barrier to the two-child flow in
the test “two concurrent processes importing the same comments never duplicate
(pm-github-503u)” so both child processes report readiness before either begins
the import, then have the parent release them together. Update the child
invocation/CHILD_SCRIPT coordination while preserving the existing result
parsing and assertions, ensuring the test reliably creates lock contention
rather than merely launching processes near each other.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 639af799-4dfd-4d3f-999f-2899825b495f
📒 Files selected for processing (6)
.agents/pm/history/pm-github-503u.jsonl.agents/pm/tasks/pm-github-503u.toonCHANGELOG.mdindex.tstest/helpers/comment-sync-child.mjstest/import-lock.test.ts
|
@greptileai @coderabbitai full review Pushed df153ba addressing the round-1 findings: liveness-first staleness (a live owner's lock is never age-broken), token-checked release (per-acquisition UUID), partial-lockfile cleanup on the create-failure path, and an in-process held-locks set so same-PID contention is not mistaken for a recycled PID. 124/124 tests green incl. three new regression tests. Please re-review. (Sourcery: acknowledged your weekly diff limit notice — nothing actionable this round.) |
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 51 minutes. |
…n race test CodeRabbit round-2 feedback on PR #4: - Stale locks are now removed under an O_EXCL sidecar election (<lock>.break): only the single winner may unlink, and it re-verifies staleness under the mutex, so two concurrent breakers can never double-break or unlink a fresh replacement lock (the double-acquire scenario). Crashed breakers age out via a 10s sidecar TTL. - Cross-process race test now uses a start-barrier file so both children enter the sync concurrently by construction. Tests: 126/126 (new: election-in-progress blocks second breaker; aged-out sidecar cleared then stale lock broken).
|
@greptileai @coderabbitai full review Round 2 pushed (7046446): stale-break is now atomic via an O_EXCL breaker-election sidecar with staleness re-verified under the mutex (kills the double-break/unlink-replacement race), and the cross-process race test uses a start barrier. 126/126 tests green. Please re-review. |
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 44 minutes. |
1 participant
Problem
The native comment sync in
pm github import --comments-mode annotationsperformed a read-markers-then-append sequence that is not atomic across two concurrent 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 the check-then-act spans multiple mutations. Previously documented as a known limitation onsyncGithubCommentsToAnnotations(CodeRabbit, PR #33).Solution
A cross-process mutex serializes the marker-dedupe critical section, scoped per item so unrelated items in concurrent imports still sync in parallel:
acquireImportLock()— atomicfs.openSync(path, "wx")(O_EXCL) lockfile under the workspace runtime dir, JSON payload (pid, timestamp) shaped like the CLI's own locks sopm gccan sweep strays.syncGithubCommentsToAnnotationsdocstring updated: the limitation is lifted.Tests
test/import-lock.test.ts, including a two-process parallel-import race test (viatest/helpers/comment-sync-child.mjs) asserting zero duplicate markers.changelog:checkpass.pm item
🤖 Generated with pi (K3) under Claude orchestration
Summary by cubic
Stops duplicate comments during concurrent imports by serializing the comment marker dedupe in
pm github import --comments-mode annotations. Adds a per-item lockfile so the read-and-append step is atomic across processes; satisfiespm-github-503u.locks/viafs.openSync("wx"); payload mirrors thepmCLI lock format and includes a per-acquisition token. Staleness is liveness-first: break only on dead or recycled owner PIDs; TTL (5 minutes) applies only to unparseable payloads; never age-break a live owner.<lock>.breaksidecar election; the single winner re-verifies then unlinks, and crashed breaker sidecars age out (10s) and are cleaned.syncGithubCommentsToAnnotationsnow uses this lock to prevent duplicates. Tests cover live-owner-never-age-broken, token-checked release, recycled-PID handling, partial-lockfile cleanup, breaker election (in-progress and aged-out), and a two-process race test with a start barrier asserting zero duplicate markers.Written for commit 7046446. Summary will update on new commits.