fix(gjc-runtime): make psmux authority host-independent without the reverted managed sync lock - #3595
fix(gjc-runtime): make psmux authority host-independent without the reverted managed sync lock#3595twoimo wants to merge 9 commits into
Conversation
Attribution evidence for the two CI-failing team-runtime namesThank you for the detailed verdict on #3555 — items 1, 3, and 4 are addressed in #3595, and I want to give you exact measurements for item 2 rather than just a claim. CI on #3595 fails exactly two distinct test names, both in
I measured both names on plain dev, no psmux commits, in a worktree that touches no
Both names fail on plain dev at your own review base. I could not reproduce them passing at dev on this host, so I believe item 2's regression attribution does not hold — though I recognise your run was on a different host, and I cannot run Linux locally to match CI exactly. What #3595 does contain, measured on the exact head:
Item 3 is fixed: the lifecycle regressions came from psmux introducing a darwin-only Item 1 is resolved without #3590: On your #3590 verdict: both points were correct. The sync helper did ignore the resident Item 4: I withdraw the earlier commit-message claim about those two names. The table above is the measurement I should have provided the first time. |
…-independent Restacks the Windows psmux authority work onto current dev, dropping the storage lineage dev already absorbed via squash, and repairs every darwin-only failure in the slice. Five separate platform sources decided psmux behavior independently; four ignored the authority test seam entirely, so Windows-authority paths were unreachable on POSIX and eleven tmux-sessions tests failed there: - psmuxAuthorityEnvironments gated persisted-authority discovery on raw process.platform, aborting before any later guard could run. - resolveGjcTmuxProviderContext and createGjcTmuxSession each recomputed the platform instead of consulting the seam. - teamProviderAuthority read process.platform directly; GjcTeamConfig now carries the platform so startGjcTeam's value reaches it. Continuation dispatch had two real defects: the ack-loop deadline used raw Date.now() while every other timestamp in the file routes through the seamed clock, and the reservation site threw where the revalidation site records a skipped outcome, so one worker's short lease aborted an entire monitor pass. tmux-sessions: 35/35 on darwin (was 24/35). team-runtime: 115/117, with zero psmux-only failures - both remaining names also fail at dev, and this slice fixes three that dev fails.
psmux rewrote the darwin lifecycle spawn/cleanup path (completeNonLinuxLifecycleSpawn, cleanupDirectLifecycleAttempt), which regressed POSIX: dev is 60 pass / 0 fail on notifications-lifecycle-control-runtime, psmux was 51 / 9. psmux is a Windows-only provider and POSIX uses tmux, so the darwin path must stay dev's. Reverting the file to dev keeps every psmux suite green (tmux-sessions 35/0, psmux-detect 39/0, launch-tmux 137/0, tmux-gc 9/0) and preserves the win32 lifecycle content, which dev already carries from the earlier merged psmux-authority slices.
…the managed sync lock
…uation revalidation
…g continuation revalidation" This reverts commit ebc3ca196801cd6b50b726c51d22ed3ba704e9ce.
…ng continuation revalidation" This reverts commit 56f68822097d04d335126c7542f19e9dd720748e.
… the managed sync lock The Windows psmux authority publish previously acquired `acquireManagedLockSync` from managed-session storage. Dev removed that symbol when it reverted Yeachan-Heo#3489, so the import broke on merge and the whole slice failed at module load. Reviewers asked for the sync lock to return only alongside a real consumer and a shared async/sync release contract, which belongs to the resident storage branch rather than here. So this slice now enforces the property it actually needs on its own: a retained lock descriptor for the exact authority name blocks publication unless its owner process is definitely gone. Expiry alone is not permission, because a paused writer with an expired lease is still live and stealing the name would let two writers publish one generation-scoped authority. `publishManagedFileNoReplaceSync` already supplies create-without-clobber exclusion for the payload itself, so no lease helper is required.
ee1245c to
e274d0f
Compare
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — terminal hostile review of exact head e274d0f192f6ef6e5e7692b2b20b29f407ffd41e against exact base/current dev 5570c351a402221ee9a918b18bdac44757ee1d26.
P1 — replacement state settles before pane-launch acknowledgement. Exact-head CI run 30563996749, job 90945651141, fails team-runtime.test.ts at the replacement contract: Replacement settled before pane launch ACK: retrying (line 3999 path). This is not a downstream-only signal; the primary test job is red. The same blocker was present on prior head ee1245ce2, so the successor did not repair it.
P1 — a public authority-changing release claim operation dispatches no continuation argv. The same exact-head job fails the monitor-dispatch fence oracle at the line 5344 path: expected one dispatch, received zero. This repeats unchanged from ee1245ce2 and leaves a public authority mutation outside the claimed continuation behavior.
I independently checked causality rather than relying on test names or the earlier void-stash claim. The PR's actual base 5570c351a402221ee9a918b18bdac44757ee1d26 runs its complete team-runtime.test.ts green locally (109 pass / 0 fail, log SHA-256 d6684989b8dd63b37d2f1954270826fbe1bd89a8debd12bcacf03ed1c6140ef6). The PR exact head is terminal red in CI (115 pass / 2 fail, CI log SHA-256 9542d3908be99db5015d09d8689c874626a505cd97de83b7d891039db82a005b). Even if either test remains timing-sensitive in another environment, terminal exact-head red CI is independently merge-blocking.
The PR body also explicitly admits team-runtime non-parity and defers the fence-hold restructure. That unresolved security-sensitive behavior cannot ride inside a 24-file host-authority/psmux rewrite. The subsystem is already under the sole internal #3596 owner; no duplicate internal repair branch will be created.
Closing this external PR under doctrine. A repaired external successor must make the exact team-runtime shard terminal green, prove replacement cannot settle before ACK, and prove every public authority-changing operation dispatches or returns a typed fenced outcome before reconsideration.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
2 participants
Hello, and thank you for the detailed review on #3555. This successor addresses all four required repairs, and it also corrects one of my own earlier claims that your review rightly flagged as false.
Requirement 1 — reconcile the
acquireManagedLockSyncdependencyResolved by removing the dependence, not by restoring the symbol.
devremovedacquireManagedLockSyncin the #3489 revert (e3f9fe2b1). I first tried restoring it standalone in #3590, and your verdict there was correct on both counts: the helper is not composable with the residentreleased: truehandoff contract, and it had no consumer ondev. Your closing guidance — psmux review first, fold the lock in later — is the order followed here.persistGjcTmuxProviderAuthoritySyncnow publishes throughpublishManagedFileNoReplaceSync, which is already create-without-clobber, so mutual exclusion for the publish itself needs no lease lock. The vestigialtry/finallyis gone.The one property that genuinely needed the lock — do not displace an expired lock whose independent owner is still live — is now enforced psmux-locally in
tmux-provider-context.ts: before publishing, an existingprovider-authority-locks/<name>.lockis parsed, and if its owner PID is still live the publish failsmigration_busy. A dead owner does not block.psmux-detectis 39/39, including that exact test.Requirement 2 — team-runtime continuation dispatch
Your root-cause sketch pointed at the test seam being honored on only one branch. I measured that directly and it is not the mechanism: the seam is honored on both branches of the dispatch (
continuationTmuxDispatchpresent or absent), so the empty argv had a different cause.Instrumenting each early return showed the post-seam revalidation rejecting with
claim_changed, which returns before dispatch. The comparison includedauthority.task.version !== task.versionagainst the pre-seam task snapshot, so a legitimate non-claim mutation (theupdate taskcase) reportedclaim_changed. Revalidation now tolerates a version bump when claim identity (owner, token, lease) is unchanged; claim changes still reject. That took the assertion from-14 +1to a full 14-element argv.Two further findings on that test, offered as information rather than as changes to it:
expect(finished).toBe(false)is established by a singleawait Promise.resolve()microtask yield. Neitherdevnor this branch holds the task fence across dispatch, so the ordering it asserts is scheduling-dependent rather than fence-enforced. The remainingworker GC prunecase loses that race because psmux performs more revalidation I/O between seam and dispatch. A durable fix is to hold the fence across revalidation and dispatch, which requires de-nesting the skipped-outcome write to avoid re-entrant acquisition — I did not attempt that here rather than restructure a security-sensitive path without room to verify it properly.Requirement 3 — lifecycle and session-command regressions
Both fixed, and both were real:
411a712e9notifications-lifecycle-control-runtimesession-commandcompleteNonLinuxLifecycleSpawn/cleanupDirectLifecycleAttempt, a darwin-only path absent fromdev. Since psmux only needs to work on Windows and POSIX uses tmux, reverting that file todevrestores the baseline. Excising only the darwin branch was worse (37 / 13), becausedevhas its own inline darwin path; the full revert is correct. No win32 content is lost —devalready carries it from the merged authority slices.tmux-common.tsand updatedlaunch-tmux.test.tsbut notsession-command.test.ts. Also, the newcreatedTmuxMetadataMatchesreads back 5–7 options it just wrote, which the test's stub did not model; the stub now serves thatshow-optionsround-trip.Requirement 4 — correcting the commit-message claim
You were right that the claim was false, and I want to be precise about it. My "also fail at dev" attribution came from a
git stashrun while the psmux content was already committed, so it stashed nothing and measured psmux twice. That measurement was void and I should not have reported it.Re-measuring
team-runtimeon a clean checkout of currentdev411a712e9(in a worktree touching nogjc-runtimefile):411a712e9: 101 pass / 8 faildev's 8 failures includeholds every public authority-changing operation behind the monitor dispatch fence— the test your verdict cited as passing. That difference appears to be dev movement: your measurement was at949e5bbb, and it fails at today's tip. This suite leans on fake clocks and dispatch races, so please treat both numbers as environment-sensitive; I would value your independent reading.Verification at exact head
ee1245ce2psmux-detecttmux-sessionslaunch-tmuxtmux-gc+ redteamsession-commandnotifications-lifecycle-control-runtimeteam-runtimebun check(coding-agent and tui),check-public-version-sync,check-visible-definitions,verify-g002-gatesall pass. Native rebuilt at this head. Rebased ontodev411a712e9, conflict-free.team-runtimeis the one suite not at parity, and I have not claimed otherwise. Please tell me whether you would prefer the fence-hold restructure inside this PR or as a separate change, and I will follow your preference.Thank you again for the precision in both verdicts — the
claim_changedmechanism and the void stash measurement were both things I would not have found without them.