fix(fork): make the fallback refresh await its own queued validation - #13960
Conversation
`refreshForkAvailabilityNow`'s fallback-snapshot branch was the only exit that
returned without waiting for the validation it had just queued. The live-index
branch directly below it already waits via
`waitForForkValidationRequestCompletions`; this one did not.
That matters because the request can be drained by someone else.
`applyPendingForkValidations` ends with an unguarded restart:
if activeForkSupportValidationKeys.isEmpty {
restartForkAvailabilityRefreshIfPending()
}
Its sibling inside the probe-owner `defer` is guarded by `!resumedWaiters`
precisely so it does not restart when it has just resumed a waiter. The tail has
no such guard, so it can spawn a detached refresh that races the contention
waiter it just resumed. Whichever continuation the main actor runs first decides
the outcome: when the detached task wins it claims the pending request and starts
the probe, and the original caller resumes, finds an empty queue, and returns
while its own probe is still in flight.
Real callers observe this as "refresh finished" with availability still stale --
`Workspace+ForkAgentConversationAvailability` and `ContentView` both read fork
support straight after awaiting this call, so the Fork Conversation menu item can
render off a snapshot that was never validated. It also surfaces as an
intermittent failure in
`sharedForkProbeFallbackWaitsForActiveSamePanelValidation`, where the second
refresh reports finished and the second fallback is never accepted.
Waiting here fixes the contract regardless of which task drains the queue,
rather than trying to win the race. It is inert on the common path:
`forkValidationRequestIsWaiting` is false once the request is neither pending nor
processing, so the continuation resumes synchronously when this call did its own
work. When another drainer did claim it, the id stays in
`processingForkValidationRequestIDs` and the per-batch `defer` retires it and
resumes completion waiters, releasing this caller exactly when the probe's
validation is recorded.
I considered instead hoisting a `resumedWaiters` flag across batches to guard the
tail restart, mirroring the sibling. Rejected: it can strand pending requests if
the resumed waiter's task is cancelled right after resuming, and it would not fix
the contract for any other drainer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 27 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe fallback-snapshot path in ChangesFork validation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue is established for this change; it is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 23 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (23 passed)
Full details: Description checkExplanation The description gives a detailed summary, explains the bug, documents the intended behavior, and states the available validation. However, it does not follow the required template structure. The Testing, Demo Video, Review Trigger, and Checklist sections are missing. ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
An independent review found the comment's closing claim wrong. It said the live-index branch "already waits this way; this branch was the only exit that did not". That branch waits only when `didReload` is false. When `reload()` did run, it called `applyPendingForkValidations` internally, so the identical steal can happen and that path returns without waiting. The hole is not unique to the fallback branch. Also say plainly that this is a symptom fix. The root cause is the tail restart in `applyPendingForkValidations` missing the `!resumedWaiters` guard its in-loop sibling has, and that is tracked separately rather than being implied as already handled here. No behaviour change; comment only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Independent review: SAFE TO MERGE — one overclaim corrected in
|
teamleaderleo
left a comment
teamleaderleo
left a comment
There was a problem hiding this comment.
Reviewed at 92c4f011ed. This is a code review, not a run. I agree with the independent review above. The change is sound, and it stays with Leo for approval because it changes app runtime behavior.
What I checked by reading SharedLiveAgentIndex.swift at this head:
- The new wait matches the existing
!didReloadwait in the live-index branch. Both use the samewaitForForkValidationRequestCompletionshelper withownsRequest: true. - No lost wakeup.
forkValidationRequestIsWaitingand the waiter append both run inside the synchronouswithCheckedContinuationbody on the main actor. If this call drained its own request, the per-batchdeferinapplyPendingForkValidationshas already retired it (retireProcessingForkValidationRequests), so the wait resumes immediately. - The other drainer's paths all release the waiter. A stolen request stays in
processingForkValidationRequestIDsuntil that drainer'sdeferrunsresumeForkValidationRequestCompletionWaiters. The contention branch restores to pending and then re-drains recursively. ThefallbackSnapshot == nil, index == nilrequeue can't catch a request that carries a fallback snapshot.
What I did not verify: that this fixes the intermittent sharedForkProbeFallbackWaitsForActiveSamePanelValidation failure. There's no deterministic regression test, and one green run proves little. The wait also has no timeout, a gap the non-fallback branch already has. The root cause is tracked in #13995.
— Ophelia g1 🍄
Run: run_cmux_main_red_triage_app_host_census_and_pr_review_20260923_07d8d17b
06c2101 ci: route streamed validation by capability instead of by lane name (manaflow-ai#14002) 1773c54 ci(e2e): start builds from main's DerivedData so test-only changes skip the app compile (manaflow-ai#14016) c890374 ci: pin the nightly runner guards to the whole expression (manaflow-ai#13997) 8abd2e9 ci: flag condition polls bounded by a Task.yield() count (manaflow-ai#14019) e4ca672 ci(ios): record the cmux.app upload once Apple accepts it (manaflow-ai#14014) 260b648 ci: check what the runner variables hold, not just what the workflows say (manaflow-ai#13992) 25ad5af feat(terminal): opt-in macOS text-editing gestures at the shell prompt (manaflow-ai#13921) daf9649 test: drop six focus-history cases superseded by FocusHistoryScopeTests (manaflow-ai#13975) 11202e3 Name the workspace that workspace.reorder could not resolve (manaflow-ai#13961) 2a4f3f6 fix(fork): make the fallback refresh await its own queued validation (manaflow-ai#13960) 4b82298 ci: let test-depot run one app-host test by selector (manaflow-ai#14001) 5d1ecb8 test: give each drained write its own deadline in the short-chunks reader test (manaflow-ai#13999) # Conflicts: # .github/workflows/ci-guards.yml # .github/workflows/ci-health-report.yml # .github/workflows/ios-appstore-upload.yml # .github/workflows/ios-streamed-validate.yml # .github/workflows/iroh-release-gate.yml # .github/workflows/nightly.yml # .github/workflows/test-depot.yml # .github/workflows/test-e2e.yml
1 participant
Fixes the intermittent
sharedForkProbeFallbackWaitsForActiveSamePanelValidationfailure, one of the app-host suite's remaining red tests — and the product
contract bug underneath it.
The bug
refreshForkAvailabilityNow's fallback-snapshot branch was the only exitthat returned without waiting for the validation it had just queued. The
live-index branch directly below it already waits via
waitForForkValidationRequestCompletions.That gap is reachable because another task can drain the request first.
applyPendingForkValidationsends with an unguarded restart, whose siblinginside the probe-owner
deferis guarded by!resumedWaitersfor exactly thisreason:
So it can spawn a detached refresh that races the contention waiter it just
resumed. Whichever continuation the main actor runs first decides the outcome:
when the detached task wins, it claims the pending request and starts the probe,
and the original caller resumes, finds an empty queue, and returns while its
own probe is still in flight.
Why this is a product bug, not a test artifact
Workspace+ForkAgentConversationAvailabilityandContentViewboth read forksupport immediately after awaiting this call. They can observe "refresh
finished" with availability still unvalidated, so the Fork Conversation menu
item renders off a snapshot nobody checked.
Why wait, rather than guard the restart
Waiting makes the contract hold regardless of which task drains the queue,
instead of trying to win a scheduler race. It is inert on the common path:
forkValidationRequestIsWaitingis false once the request is neither pending norprocessing, so the continuation resumes synchronously when this call did its own
work. When another drainer did claim it, the id stays in
processingForkValidationRequestIDsand the per-batchdeferretires it andresumes completion waiters — releasing this caller exactly when the probe's
validation is recorded. It also reuses the existing cancellation-correct helper
(
ownsRequest: truedrops the request on cancel), so no new cancellationsemantics.
The alternative — hoisting a
resumedWaitersflag across batches to guard thetail restart, mirroring the sibling — I rejected: it can strand pending requests
if the resumed waiter's task is cancelled right after resuming, and it does not
fix the contract for any other drainer.
Confidence, stated honestly
The root cause is a hypothesis, though tightly constrained: it is the only
mechanism found that makes the test's assertions at lines 1908/1910/1916/1922
pass while 1917 and 1924 fail together, which is the observed signature.
The failure is intermittent (3 passes, 1 failure across sampled runs), so a
single green run proves little here — this wants repeated runs. I have no macOS
SDK available, so this is
swiftc -parseplus reading, not a build or a run.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes the intermittent
sharedForkProbeFallbackWaitsForActiveSamePanelValidationfailure by makingrefreshForkAvailabilityNowawait its own queued validation before returning, so callers never observe "refresh finished" with stale fork availability.This is a symptom fix: the root cause is the unguarded tail restart in
applyPendingForkValidations, which can spawn a detached refresh that races a contention waiter it just resumed, letting either branch ofrefreshForkAvailabilityNowreturn without waiting.waitForForkValidationRequestCompletionscall before the fallback branch returns.reload()runsapplyPendingForkValidationsinternally, so the fix waits regardless of which task drains the queue.Workspace+ForkAgentConversationAvailabilityandContentViewrendering the Fork Conversation menu item from unvalidated snapshots.Written for commit 92c4f01. Summary will update on new commits.
Summary by CodeRabbit