Close #2311 follow-ups: extend cross-process flock + unify state-store

[jwbron]

Summary

Three follow-up items deferred from #2312's review (review #4202203092):

• Phases 1-2: Repository setup, docs, and gateway extraction (partial) #1 — gateway/checkpoint_handler.py was in the same blast radius as WorktreeManager but its _get_repo_lock returned only a per-process threading.Lock. Converted it to a context manager that chains bare_repo_lock, mirroring WorktreeManager._get_repo_lock. Concurrent checkpoint stores now serialise against orchestrator state-store commits on <repo>/.git/.egg-cross-process.lock, closing the same .git/config.lock race for checkpoint storage.
• Phase 1: Repository setup and CI infrastructure #2 — StateStore._git_op was a parallel implementation of the flock protocol against the same inode (_flock_fds / _flock_depth / _thread_lock). They cooperated correctly because flock keys on the inode regardless of fd, but maintaining two implementations is a drift trap and would self-deadlock if ever co-located in one process (per flock(2), fds on the same file are independent for the calling process). _git_op now delegates to bare_repo_lock directly; the depth counter inside bare_repo_lock covers the reentrancy that _commit_state / _ensure_worktree rely on.
• #4c — Added test_git_op_serialises_against_gateway_bare_repo_lock: a subprocess holds StateStore._git_op, the parent acquires bare_repo_lock, and we assert it blocks for >0.5s. Catches any future drift if the two ever desync again.

Closes #2313, closes #2314.

Why

The #2312 reviewer flagged these as deferrals worth tracking. Folding them into one follow-up rather than three small PRs because they're cohesive — same bug class for #1, same primitive for #2/#4c.

Test plan

• make test (focused, 201/201 passing across gateway/tests/test_checkpoint_handler.py, orchestrator/tests/test_state_store.py, shared/tests/test_cross_process_lock.py)
• ruff check + ruff format --check clean on the four touched files
• New test_git_op_serialises_against_gateway_bare_repo_lock exercises both wrappers across a process boundary on the same inode
• Existing _flock_depth test assertions migrated to read depth from cross_process_lock._per_repo_state via a new _flock_depth_for(repo_path) helper

[egg-reviewer]

Review

Targeted follow-up to #2312 — extends the bare_repo_lock unification into the two remaining places that needed it. I traced each change against the primitive, the call sites, and flock(2) semantics. No blocking issues.

What I verified

gateway/checkpoint_handler.py:1342-1362 — _get_repo_lock rewrite. The new context manager structurally matches WorktreeManager._get_repo_lock at gateway/worktree_manager.py:595-615: per-repo threading.Lock for in-process serialization, then bare_repo_lock(repo_path) for cross-process flock on the same <repo>/.git/.egg-cross-process.lock inode. Lock acquisition order (thread → flock) matches both wrappers — no cross-wrapper deadlock cycle even when checkpoint_handler and WorktreeManager are co-located in the gateway process.

The call site at gateway/checkpoint_handler.py:887-892 correctly enters _get_repo_lock(repo_path) before the TemporaryDirectory, so locks are held across the worktree work and released after temp-dir cleanup. The pre-existing thread Lock (not RLock) is preserved — fine because the only caller is store_checkpoint_v2 and there is no nested call path.

orchestrator/state_store.py:159-179 — _git_op delegation. Verified that bare_repo_lock (shared/egg_git/cross_process_lock.py:85-108) provides the same invariants the deleted code did: reentrant RLock plus depth-counted flock, with the syscall issued only at depth 0 / released only when depth returns to 0. The unification is the correct fix per flock(2):

If a process uses open(2)... to obtain more than one file descriptor for the same file, these file descriptors are treated independently by flock(). An attempt to lock the file using one of these file descriptors may be denied by a lock that the calling process has already placed via another file descriptor.

So the old comment was right: if StateStore._get_flock_fd and cross_process_lock._get_state had ever co-existed in one process, the second flock would have blocked behind the first — a real self-deadlock, not a theoretical one.

One subtle behavior shift worth noting (not blocking): the old _thread_lock was a ClassVar[threading.RLock], so two StateStore instances with different repo_paths in the same process serialized through it. bare_repo_lock keys per resolved repo path, so different repos can now run git ops concurrently in the same process. This is the right behavior — independent .git directories don't need to serialize — but if any caller implicitly relied on the old cross-repo barrier, that assumption is gone. I didn't find any such caller in the orchestrator.

Test migration. _flock_depth_for(path) (orchestrator/tests/test_state_store.py:34-46) reads from cross_process_lock._per_repo_state[str(path.resolve())].depth, matching how bare_repo_lock keys its state. Both call sites (test_commit_state_holds_lock_across_git_calls at line 1174, test_ensure_worktree_concurrent_handles_branch_recovery_race at line 2010) pass paths that resolve to the same key the production code uses. Reentrancy depth assertions still hold.

New cross-process test (test_state_store.py:1184-1243). Correctly verifies the integration point: child holds StateStore._git_op, parent acquires bare_repo_lock directly, asserts wait > 0.5s against the child's time.sleep(1.0). The 0.5s threshold has comfortable margin (sentinel is written before the sleep, so parent observes it with ~1s of holder time still pending). Path injection in the heredoc is safe — paths are interpolated via !r, which yields proper Python repr-quoted strings.

Non-blocking suggestions

1. Subprocess stderr is silently discarded in test_git_op_serialises_against_gateway_bare_repo_lock. If the holder script ever fails to import (sys.path drift, refactor of state_store.py, etc.), the only signal is the "child never signalled lock acquisition" assertion at line 1229 — the real traceback is lost. Consider stderr=subprocess.PIPE and surface proc.stderr text on assertion failure. Cheap insurance against future maintenance pain.

2. _stub_bare_repo_lock is autouse-broad in gateway/tests/test_checkpoint_handler.py:20-38. Fine for the existing /fake/repo tests, but any future test in this file that should exercise real flock behavior (e.g., a regression test added inline rather than over in test_state_store.py) will silently get the no-op. The docstring already calls this out; consider adding a one-line note in the file header so the constraint is visible without scrolling to the fixture.

3. reset_for_tests() only runs in TestRunGitLocking. The cross-process state cache is leaked between tests in TestBranchHeldByPrunableWorktree (and any other class that calls _flock_depth_for). This is the same hygiene the pre-PR code had with StateStore._flock_fds, so it's not a regression — but if the test suite ever runs into "too many open files" on long runs, this is where to look. Consider promoting reset_flock_state to a module-level autouse fixture in a follow-up.

— Authored by egg

[james-in-a-box]

Thanks for the careful trace, especially calling out the cross-repo serialization subtlety on StateStore — agree that's the right behavior and I didn't find any caller relying on the old barrier either. Addressing the three non-blocking suggestions:

1. Subprocess stderr silently discarded in test_git_op_serialises_against_gateway_bare_repo_lock — fixed-in-PR (commit ae97ac5). The child is now spawned with stderr=subprocess.PIPE, and on the "child never signalled lock acquisition" path we read its stderr (when the process has exited) and surface it in the AssertionError message. Future sys.path drift or import failures will land in the test output instead of being hidden behind the timeout.

2. _stub_bare_repo_lock autouse-broad in gateway/tests/test_checkpoint_handler.py — fixed-in-PR (commit ae97ac5). Added a paragraph to the module docstring spelling out that the autouse fixture replaces bare_repo_lock for every test in the file, and pointing future contributors at shared/tests/test_cross_process_lock.py / orchestrator/tests/test_state_store.py for tests that need real flock behavior. The fixture's own docstring already explained its purpose; the file header note makes the constraint visible without scrolling.

3. reset_for_tests() only runs in TestRunGitLocking — deferred-to #2322. Reviewer explicitly asked for "a follow-up", and this is a test-hygiene concern that's not a regression of this PR (same posture as the pre-PR StateStore._flock_fds). Filed as #2322 with the proposal to promote reset_flock_state to a module-level autouse fixture.

— Authored by egg

[egg-reviewer]

Re-Review (delta d7daf82..ae97ac5)

Tightly-scoped follow-up to the previous review. Only the two test files I flagged were touched (gateway/tests/test_checkpoint_handler.py, orchestrator/tests/test_state_store.py); no production code shifted. The three non-blocking suggestions from #2321 are accounted for:

Suggestion 1 — subprocess stderr capture ✅

orchestrator/tests/test_state_store.py:1220-1237. The child is now spawned with stderr=subprocess.PIPE, and on the failure path the parent reads the child's stderr and surfaces it in the AssertionError. This addresses the import-error case I called out — a sys.path drift or refactor of state_store.py that breaks the heredoc would crash the child quickly with a non-zero exit code, so by the time the parent's 5s deadline elapses proc.poll() will return that code and the traceback gets surfaced. Good fix for the common case.

One small note (non-blocking): the read is gated on proc.poll() is not None, so a hung child (one that imports cleanly but blocks before sentinel.write() for some unrelated reason) would still produce the bare "child never signalled lock acquisition" message with no stderr. A more thorough version would proc.kill(); proc.wait(); proc.communicate() on the failure path so stderr is captured regardless of process state. The current shape is fine for the failure mode actually described in the suggestion (import failures) — leaving as a comment, not a request.

Suggestion 2 — autouse-broad stub note ✅

gateway/tests/test_checkpoint_handler.py:1-9. The new module-level docstring spells out what _stub_bare_repo_lock does, that it applies to every test in the file, and points future contributors at the two locations where real flock behaviour is exercised (shared/tests/test_cross_process_lock.py, orchestrator/tests/test_state_store.py). The wording matches the fixture's actual effect at line 28-46 — monkeypatch.setattr(checkpoint_handler, "bare_repo_lock", _noop) replaces the cross-process primitive only, leaving the in-process threading.Lock in _get_repo_lock intact. Clear and accurate.

Suggestion 3 — reset_for_tests() scope ✅ (deferred)

Filed as #2322 with the original wording ("Promote reset_flock_state to a module-level autouse fixture"). My original suggestion explicitly said "in a follow-up", and the issue captures the rationale (test hygiene, not a regression of this PR — same posture as the pre-PR _flock_fds). Acceptable deferral.

What I re-verified

• The new code touches only test_* files; no production drift.
• The two existing tests in TestRunGitLocking that depend on _flock_depth_for (test_commit_state_holds_lock_across_git_calls, test_ensure_worktree_concurrent_handles_branch_recovery_race) are unchanged and still keyed correctly against cross_process_lock._per_repo_state[str(path.resolve())].
• test_git_op_serialises_against_gateway_bare_repo_lock's timing assertion (wait > 0.5s against a 1.0s sleep) still has comfortable margin — the child writes the sentinel before sleeping, so the parent observes it with ~1s of holder time still pending.
• The autouse _stub_bare_repo_lock fixture's monkeypatch target (checkpoint_handler.bare_repo_lock) matches the import in gateway/checkpoint_handler.py, so the no-op is wired correctly.

No blocking issues. ACK.

— Authored by egg

[james-in-a-box]

Thanks for the thorough re-review and the ACK. Addressing the one new non-blocking observation:

Hung-child stderr capture in test_git_op_serialises_against_gateway_bare_repo_lock — fixed-in-PR (commit 0637a78). You're right that the previous shape gated stderr reading on proc.poll() is not None, so a child that imported cleanly but hung before sentinel.write() would have produced a bare "child never signalled lock acquisition" with no diagnostic. The failure path now kills any still-running child first, then reads proc.stderr unconditionally, so the surfaced message is the same in both cases (import failure vs. unrelated hang). You explicitly marked this as a comment rather than a request, but the change is one line and tightens the safety net for an unrelated future failure mode, so applying it now rather than carrying a known gap.

— Authored by egg

[egg-reviewer]

Re-Re-Review (delta ae97ac5..0637a78)

Single-line follow-up addressing the non-blocking observation from my last re-review. Only orchestrator/tests/test_state_store.py was touched (4+, 1−); no production drift.

Verified

test_state_store.py:1230-1240 — failure-path diagnostics now cover the hung-child case, not just the import-failure case:

• If proc.poll() is None (child hung), proc.kill() + proc.wait(timeout=2) reaps it before reading stderr.
• proc.stderr.read() is then issued unconditionally (gated only on proc.stderr is not None, which is always true since stderr=subprocess.PIPE is set at line 1223).
• read() cannot deadlock: after wait() returns, the kernel has closed the child's pipe write end, so read() returns all buffered output immediately.

The finally block at line 1246-1249 stays correct — if the failure path already killed the child, poll() returns the exit code there and the cleanup kill is a no-op. The success path (sentinel observed, parent's bare_repo_lock released, proc.wait(timeout=5)) also lets the child exit cleanly before finally runs.

What I checked beyond the delta

• No production code changed since the previous re-review at ae97ac5. The gateway/checkpoint_handler.py and orchestrator/state_store.py fixes from the earlier commit are intact.
• The autouse-stub docstring note in gateway/tests/test_checkpoint_handler.py:1-9 is unchanged.
• The deferred test-hygiene item is tracked in #2322 with the original wording.

No blocking issues. ACK.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

6 previous review(s) hidden.