{{ message }}
Repository navigation
fix: DocStore.lock() is a no-op mutex on Windows - #456
Open
Manish2102 wants to merge 2 commits into
Open
Manish2102 wants to merge 2 commits into
Manish2102 wants to merge 2 commits into
Conversation
lock() only implemented the critical section with fcntl.flock, which does not exist on Windows; the fallback there was just `yield` with no locking at all. This means the check-then-write sequence in submit_document (unique-name assignment before save) has no real mutual exclusion on Windows, so two concurrent submits of the same filename can both save as the same name, silently shadowing one document with another (test_concurrent_same_name_submits_store_unique_names reproduces this on main). Add a real Windows lock using msvcrt.locking on a 1-byte region of the lock file. msvcrt requires an existing byte to lock, so the byte is seeded once (a racing seed is harmless: same byte, and neither side holds the lock yet at that point). Adds tests/test_local_store.py with a direct mutex test (fails against the old no-op fallback, confirmed locally) and a repeat-call sanity test. Confirmed test_concurrent_same_name_submits_store_unique_names now passes on Windows; full suite otherwise unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes a correctness issue in local-mode document submission on Windows by making DocStore.lock() a real cross-process mutex (instead of a no-op fallback), preventing concurrent submit operations from assigning and saving the same “unique” filename.
Changes:
- Implement a Windows locking path for
DocStore.lock()usingmsvcrt.locking()on a 1-byte region of a lock file. - Ensure the store root/lock file path is initialized before locking and unify lock-path usage across platforms.
- Add tests validating that
DocStore.lock()serializes concurrent critical sections and remains safe across repeated calls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| except ImportError: | ||
| yield | ||
| import msvcrt | ||
| fd = os.open(str(lock_path), os.O_CREAT | os.O_RDWR) |
Comment on lines
+12
to
+35
| order = [] | ||
| lock_obj = threading.Lock() | ||
|
|
||
| def worker(name): | ||
| with store.lock(): | ||
| # If DocStore.lock() is not a real mutex, both workers can be | ||
| # inside this block at once and their appends interleave. | ||
| with lock_obj: | ||
| order.append(f"{name}-enter") | ||
| time.sleep(0.2) | ||
| with lock_obj: | ||
| order.append(f"{name}-exit") | ||
|
|
||
| threads = [threading.Thread(target=worker, args=(n,)) for n in ("A", "B")] | ||
| for t in threads: | ||
| t.start() | ||
| for t in threads: | ||
| t.join() | ||
|
|
||
| # Whichever thread goes first, its enter/exit pair must be contiguous — | ||
| # a real mutex never lets the other thread's enter land in between. | ||
| assert order[0][-5:] == "enter" | ||
| assert order[1][-4:] == "exit" | ||
| assert order[0][0] == order[1][0] |
- os.open() for the msvcrt lock path now passes an explicit mode (0o644) and O_BINARY instead of relying on OS/umask defaults, so the lock file's permissions and read/write mode are predictable (Copilot review comment on pageindex/local_store.py:106). - test_lock_serializes_concurrent_critical_sections now forces both threads to contend for the lock at the same instant via a Barrier, and asserts mutual exclusion directly (no thread observes the other "inside" the critical section at the same time) instead of inferring it from post-hoc ordering of sleep-separated events. Verified this still fails deterministically against a genuinely no-op lock and passes against the real fix (Copilot review comment on tests/test_local_store.py:35). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
hw725
added a commit
to hw725/PageIndex
that referenced
this pull request
Oct 17, 2026
포크 main(d0b02ee)이 upstream 보다 147 커밋 뒤처져, upstream/main(f0db746) 에서 브랜치를 새로 따고 커스텀을 새 구조에 맞춰 다시 넣었다. upstream 은 그 사이 OpenAI SDK 분기를 없애고 litellm 단일 경로로 바꿨고, 재시도 소진 시 빈 문자열 대신 LLMRetriesExhausted 를 올린다. 커스텀 네 가지는 upstream 에 여전히 없다. - 빈 본문 → EmptyLLMResponseError, 깨진 JSON·키 누락 → MalformedLLMResponseError 로 올려 같은 재시도 사다리에 태운다. expect_json_keys 는 옵트인(호출부 5곳). 키 검사는 존재만 본다 — {"physical_index": null} 은 응답이다. 예전처럼 null 까지 재시도하면 이제는 사다리 소진 예외로 실행 전체가 죽는다. - 이벤트 루프별 세마포어(PAGEINDEX_MAX_CONCURRENCY=12), timeout(PAGEINDEX_LLM_TIMEOUT=90, backend·호출부가 주면 그쪽이 이김), 지수 백오프(상한 10초). - OPENAI_BASE_URL 이 로컬인데 꺼져 있으면 npx openai-oauth --port <포트> 를 띄운다. - 기본 모델 gpt-5.6-terra(DEFAULT_INDEX_MODEL·DEFAULT_CHAT_MODEL). config.yaml 은 upstream 그대로 둔다 — 거기 모델 키를 적으면 구체 키가 사용자 model= 보다 우선해 《model 은 모든 역할을 정한다》 규칙이 깨진다(upstream 시험 9건이 잡았다). 예전 summary_model=luna 는 terra 로 합쳐졌다. - run_pageindex.py cp949 콘솔 대응, page_index_md 데모의 gpt-4.1 하드코딩 제거. Windows 저장소 잠금(local_store.DocStore.lock): fcntl 이 없으면 잠금이 아무 일도 하지 않아, 같은 이름 PDF 두 개를 동시에 색인하면 둘 다 sample.pdf 로 저장돼 앞 문서를 가렸다 (upstream test_concurrent_same_name_submits_store_unique_names 가 Windows 에서 실패). msvcrt 바이트 잠금으로 직렬화한다. upstream 에 같은 수정 PR 이 이미 열려 있다 (VectifyAI#593·VectifyAI#567·VectifyAI#456) — 그중 하나가 병합되면 다음 동기화 때 이 수정을 빼고 upstream 것을 쓴다. 검증: harness.contract.json C-001～C-003 (영수증 .harness/v2). pytest 799 passed / 0 failed(Windows 가 테스트 준비 자체를 허용하지 않는 4건 제외 — symlink 권한, 이름에 \ · " · CRLF 경로. 순정 upstream 에서도 실패한다). tests/test_fork_robustness.py 20건. scripts/fork_smoke.py — earthmover.pdf standard 모드 실제 LLM. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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.
lock() only implemented the critical section with fcntl.flock, which does not exist on Windows; the fallback there was just
yieldwith no locking at all. This means the check-then-write sequence in submit_document (unique-name assignment before save) has no real mutual exclusion on Windows, so two concurrent submits of the same filename can both save as the same name, silently shadowing one document with another(test_concurrent_same_name_submits_store_unique_names reproduces this on main).
Add a real Windows lock using msvcrt.locking on a 1-byte region of the lock file. msvcrt requires an existing byte to lock, so the byte is seeded once (a racing seed is harmless: same byte, and neither side holds the lock yet at that point).
Adds tests/test_local_store.py with a direct mutex test (fails against the old no-op fallback, confirmed locally) and a repeat-call sanity test. Confirmed test_concurrent_same_name_submits_store_unique_names now passes on Windows; full suite otherwise unaffected.