Skip to content

fix(fork): an access-time update no longer discards a fresh fork validation - #14799

Merged
teamleaderleo merged 1 commit into
mainfrom
fix/fork-probe-watch-flake
Sep 26, 2026
Merged

teamleaderleo merged 1 commit into
mainfrom
fix/fork-probe-watch-flake

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 26, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

The sharedForkProbe* executable-watch tests in WorkspaceForkConversationContextMenuTests fail intermittently: on #14785, and on main in run 36197594615. The failures:

  • ...InvalidatesWhenExecutableChanges
  • ...InvalidatesWhenPathDirectorySymlinkRetargets
  • ...SharesExecutableWatchAcrossPanelsWithSameExecutable
  • ...InvalidatesWhenEarlierPathExecutableAppearsForPiAndOmp

In each one, prepareForkAvailabilityProbe returns false right after refreshForkAvailabilityNow.

Root cause

applyPendingForkValidations does two things around the probe:

  1. Before running the fork capability probe, it installs a vnode watch on the executable and its directories.
  2. It stores the validation only if the watch generation is unchanged afterwards. The watch also drops the validation on any later event.

The probe's first run of a freshly written or updated executable updates the file's access time, and Darwin reports that as NOTE_ATTRIB. I reproduced it with a standalone kqueue script. Over 5 fresh files each, the first exec fired NOTE_ATTRIB 5/5 times. When the file had already been executed once before the watch was installed, it fired 0/5. The probe was invalidating its own result. Whether the test saw it depended on when the main-actor invalidation ran relative to the check, which is why it flakes. Real users hit the same thing once after installing or updating an agent binary: fork availability is dropped and has to be re-probed.

The file-descriptor headroom theory is ruled out. When the watch budget is exhausted, the validation is still stored with refreshBeforeReuse, and prepareForkAvailabilityProbe returns true (see sharedForkProbeValidationRefreshesBeforeReuseWhenWatchPathBudgetIsExceeded). The app also raises RLIMIT_NOFILE to 65536 at startup.

Change

An attrib-only event now invalidates only when the inode's ctime changed. A real chmod, chown or xattr change moves ctime; an access-time-only update does not. Writes, deletes, renames and link changes invalidate exactly as before.

New regression test: sharedForkProbeValidationSurvivesTheProbeRunningAFreshExecutable. It probes a just-written pi, waits 300 ms for the watch event, then asserts the validation is still there.

Test plan

  • CI app-host changed suites: WorkspaceForkConversationContextMenuTests green

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes a flaky fork-availability validation that was being discarded by the probe that created it. The first run of a freshly written or updated executable updates its access time, which Darwin reports as NOTE_ATTRIB; the executable watch treated that as a real change and invalidated the validation it had just produced. Real changes (chmod, chown, xattr) still work as before.

  • An attrib-only event now invalidates only when the inode's ctime changed; an access-time-only update leaves ctime untouched and no longer drops the validation.
  • Adds sharedForkProbeValidationSurvivesTheProbeRunningAFreshExecutable to cover the regression.

Written for commit c74ec13. Summary will update on new commits.

Review in cubic

…dation

SharedLiveAgentIndex installs a vnode watch on the agent executable before
running the fork capability probe, and keeps the validation only while that
watch's generation survives. The probe's first run of a freshly written or
updated executable updates its access time, and Darwin reports that as
NOTE_ATTRIB. The watch treated it like a real change and invalidated the
validation it had just produced, so prepareForkAvailabilityProbe returned
false depending on whether the main-actor invalidation landed before or after
the generation check.

That is the flake behind WorkspaceForkConversationContextMenuTests'
sharedForkProbe* executable-watch tests (all probe a just-written pi script).
It is not file-descriptor headroom: without a watch the validation is kept
with refreshBeforeReuse and prepareForkAvailabilityProbe still returns true.

An attrib-only event now invalidates only when the inode's ctime moved
(chmod, chown, xattr); an atime-only update leaves ctime unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 26, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 6 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 8b6445b4-5548-43ab-bba1-b7441da59c3c

📥 Commits

Reviewing files that changed from the base of the PR and between 6f3af0a and c74ec13.

📒 Files selected for processing (2)
  • Sources/SharedLiveAgentIndex.swift
  • cmuxTests/WorkspaceForkConversationContextMenuTests.swift

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.

❤️ Share

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

@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@teamleaderleo
teamleaderleo merged commit 9d459e3 into main Sep 26, 2026
98 of 103 checks passed
@teamleaderleo
teamleaderleo deleted the fix/fork-probe-watch-flake branch September 26, 2026 05:14
@github-actions

Copy link
Copy Markdown
Contributor

Merge receipt for c74ec13b2d: every check was green at merge (16 verified; 14 skipped by policy). Full suite runs on main after merge.

rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 26, 2026
cc90659 test: a window with no restorable workspaces is dropped from the snapshot (manaflow-ai#14801)
9b10f7c Merge pull request manaflow-ai#14756 from manaflow-ai/12956-ssh-auth-followup-main
fb665a0 test(ime): install option-as-alt right before the dead-key dispatch (manaflow-ai#14800)
9d459e3 fix(fork): an access-time update no longer discards a fresh fork validation (manaflow-ai#14799)
39e2c3a fix(ssh): share one route check across concurrent cmux ssh opens
c344ce9 test: cover concurrent cmux ssh opens sharing one route check
9ff9017 fix(ssh): report OpenSSH failures from cmux ssh instead of a Cloud VM error
311797d test: cover cmux ssh failing fast on refused and unreachable hosts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant