[joelmoss]

Fixes #224. The two flakes it names had the same cause, and so does a third (reports_the_foreground_process, the other known wr-agent flake), which failed in the reproduction loop. In each, a test read state that a concurrently forked process could still hold, or that a just-forked process didn't have yet. Only test code changes.

Causes (measured, not assumed)

Test	Cause	Evidence
vcs::tests::without_the_barrier_the_lock_dies_with_the_parents_descriptor (and serve::tests::instance_lock_is_exclusive)	A process forked by any concurrent test in the binary holds a copy of the flock's open file description until it execs, CLOEXEC descriptors included. So one check made right after the parent drops the lock can see it still held.	Reproduced locally: (false, true) twice in about 10 full lib runs, the same values as CI run 35935900854. A standalone probe saw the lock still held after release 4 times in 3000 with a concurrent spawner, and 0 in 3000 without.
pty::tests::reports_the_foreground_process	The pty's foreground group exists as soon as the child is session leader, which is before it execs, and a process between fork and exec has no readable arguments.	Failed once in the same loop.
AgentFileIntegrationTests.testTheViewersReadCapIsTheAgentsReadCeiling (any AgentHarness user)	UnixListener::bind creates the socket path at bind() and only then calls listen(). The harness connected as soon as the path existed, and a connect in that window is refused, which read as .connectionLost.	A probe that connects the instant the path appears was refused 3 times in 300.

Fixes

• flock checks: poll briefly. lock_is_free(within:) polls for 200 ms while the child runs (the child sleeps 1 s and the lock is dropped at 400 ms), so a child that genuinely holds the lock still reads as held; after the child exits it polls for 1 s. The instance-lock re-acquire retries for 1 s.
• pty test: poll for the foreground group and its name together, for up to 5 s of the child's 10 s. Dropping the pty hangs the child up, so the test still finishes in about 0.02 s.
• AgentHarness.start: waits until a connect() succeeds, not until the path exists.

Production

Not affected, which is why only tests change. Both production snapshot locks (SnapshotLock::acquire, JJProcessBarrier.acquire) already retry for up to 30 s, and JJProcessBarrier.release uses LOCK_UN, which releases every holder of the description. The app already retries connect after spawning an agent (LocalAgentVCS).

Validation

• 40 consecutive cargo test -p wr-agent --lib runs pass. The unfixed code failed 3 times in a shorter loop.
• Full make app-test: 2932 tests pass.
• cargo fmt --check and cargo clippy --all-targets -D warnings are clean, and so is make app-lint.
• An independent review found no way for the polling to hide a broken barrier. Its one actionable note, that the pty test's 1 s budget now had to cover two conditions, is fixed (5 s, with a longer child).
• Not done: a statistical rerun of the Swift harness flake. It hit about once in 8+ full suites, and the fix holds by construction: a successful connect means the agent is listening.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Tests
  • Improved process-startup checks to wait for a successful connection and report clearer failure details.
  • Updated process and lock tests to allow for timing variation, with bounded retries and checks for expected process and lock states.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 388ff255-63f1-4741-97b2-380c2fe5889b

📥 Commits

Reviewing files that changed from the base of the PR and between 495c397 and 81f60a3.

📒 Files selected for processing (4)

• macapp/WorkroomAppTests/Session/AgentHarness.swift
• vcs/crates/wr-agent/src/pty.rs
• vcs/crates/wr-agent/src/serve.rs
• vcs/crates/wr-agent/src/vcs.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.

---

📝 Walkthrough

Walkthrough

Agent startup now waits for a successful Unix-socket connection. Several wr-agent tests now poll for foreground-process and lock conditions over bounded time windows.

Changes

Agent and test synchronization

Layer / File(s)	Summary
Agent socket readiness macapp/WorkroomAppTests/Session/AgentHarness.swift	AgentHarness.start retries socket connections until one succeeds, the agent exits, or five seconds pass. On failure, it terminates the agent and includes connection and process diagnostics in the error.
Foreground process test readiness vcs/crates/wr-agent/src/pty.rs	The test polls for a foreground process group and a readable executable name, then asserts that the process group is positive.
Lock test polling vcs/crates/wr-agent/src/serve.rs, vcs/crates/wr-agent/src/vcs.rs	The lock tests retry acquisition and availability checks over bounded polling windows instead of relying on immediate checks.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 81f60

The test synchronization changes are mergeable after normal checks. The existing child-startup timing race is separate from these changes.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: fixing three flaky fork-window tests.
Linked Issues check	✅ Passed	The changes implement the coding objectives in #224. vcs::tests::without_the_barrier_the_lock_dies_with_the_parents_descriptor now polls for lock release after the child exits, while its 200 ms chec…
Out of Scope Changes check	✅ Passed	The diff changes only four test-related files. The lock, PTY, and socket changes address fork/exec or listener-readiness races identified by #224. The additional instance_lock_is_exclusive and PTY f…
Docstring Coverage	✅ Passed	Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 4 files.

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge; no outstanding finding or new actionable issue was identified.

Summary

The PR changes test-only startup and process checks to tolerate brief fork and listener-readiness windows.

• Polls lock availability and foreground-process naming within bounded intervals.
• Waits for the agent socket to accept connections, rather than merely exist.
• Explicitly kills and reaps the PTY test child.

_{Reviews (2) · Last reviewed commit: "test(wr-agent): kill and reap the pty te..."}

[joelmoss]

@coderabbitai pause

🤖 Left by Claude Code

[coderabbitai]

@joelmoss, I will pause automatic reviews for this PR.

✅ Action performed

Reviews paused.