Fix race condition in discussion file operations (#250)#260
Open
Fix race condition in discussion file operations (#250)#260
Conversation
…oss (#250) Add an in-memory per-document mutex (promise chain) that serializes all read-modify-write operations on discussion JSON files. All four mutating endpoints (POST /discussions, POST /respond, POST /discussions/resolve, POST /submit-reply) are now wrapped in withDiscussionLock(doc, fn) so concurrent requests on the same document are queued instead of racing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jessie-coco
approved these changes
Mar 9, 2026
Contributor
jessie-coco
left a comment
There was a problem hiding this comment.
Codex Review R1: CLEAN — 0 P1 + 0 P2 + 1 P3
PR #260 final status:
- 1 review round, R1 CLEAN
- 1 file changed (
server/index.js), +129 / -83
Analysis
Lock mechanism (withDiscussionLock): Promise-chain mutex pattern is correct for single-process Node.js. finally(() => resolve()) guarantees lock release even on exceptions. Synchronous file I/O inside the callback means the lock correctly serializes the full read-modify-write cycle.
All 4 wrapped endpoints (POST /discussions, POST /respond, POST /discussions/resolve, POST /submit-reply):
- Early-return pattern correctly changed from
return res.status(404)tores.status(404); return;inside lock callbacks — no code path reaches a doubleres.json()call .catch()handlers all guard withif (!res.headersSent)— correctGET /discussionsandGET /pendingintentionally not locked (read-only) — correct
Checked dimensions:
- Correctness: Lock serialization logic is sound; promise chain properly queues concurrent operations
- Security: No new attack surface; auth middleware (
v2Auth) still applied before lock - Edge cases: Error in locked operation → 500 response + lock released via
finally— correct - Integration: No callers broken; lock is transparent to the HTTP contract
- Dead code: None found
P3 (informational, non-blocking)
- P3:
discussionLocksMap grows unbounded — one entry per uniquedocId, never pruned. In practice this is negligible (one resolved Promise reference per document), but for long-running servers with many documents, a periodic cleanup or LRU eviction could be added later.
LGTM — approving.
Contributor
|
@boot-coco Codex review R1: CLEAN — approved. See review comments for full details. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
withDiscussionLock) that serializes all read-modify-write operations on discussion JSON filesPOST /discussions,POST /respond,POST /discussions/resolve,POST /submit-replyApproach
Simple in-memory mutex per document ID using a promise chain — no new dependencies needed. This is safe for single-process Node.js (which ClawMark runs as). The read-only
GET /discussionsendpoint is intentionally not locked to avoid unnecessary serialization.Closes #250
Test plan
🤖 Generated with Claude Code