fix: heal dangling XCTestDevices redirect and prove lock-owner death before reclaim - #1672
Conversation
…cile fs.existsSync follows symlinks, so a redirect symlink whose target set was deleted read as absent: reconcile skipped the unlink and the backup rename failed with ENOTDIR, wedging every XCTest command on the machine until manual cleanup. Detect symlinks with lstat (throwIfNoEntry) in both the reconcile check and unlinkIfSymlink so the stale link is removed and the backup restored.
A zombified lock owner passes kill(pid, 0) and still reports its original lstart, so a killed-but-unreaped daemon held the XCTest device-set lock forever. Conversely a ps read lost to CPU contention condemned a live owner and let waiters steal a held lock (the flake pinOwnProcessStartTime papers over in tests). Unify both liveness surfaces on classifyOwnerLiveness: condemn zombies via ps state, treat failed ps reads as no-proof rather than death, and reclaim null-start-time owners whose pid provably started after the lock was acquired (ps etime bound). Lock timeout errors now report ownerLiveness.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Reviewed exact head The dangling- P1 — the null-start PID-reuse fallback can still steal a live lock after a clock step. Please either keep null-start identities fail-closed, or persist and compare a same-domain monotonic/boot-relative acquisition marker, with a regression that simulates a clock step while the original owner is still live. All completed checks are green; iOS Smoke is still in progress. |
Review P1 on #1672: the started-after-acquisition bound compared a wall-clock birth estimate (Date.now() - ps etime) against the persisted wall-clock acquiredAtMs, so a clock step larger than the fixed 30s slack could condemn a live null-start owner and let a waiter steal the held lock — the failure class this PR eliminates. There is no same-clock-domain proof of birth order to be had here (macOS ps etime is itself wall-derived), so drop the bound: an alive pid with no recorded start time is never reclaimed. Regression covers the steal at both the classifier and the lock level.
|
Addressed the P1 in 9af92d5 — you're right, and the steal reproduces: with the etime bound in place, a null-start owner whose Went with the fail-closed option rather than a monotonic marker: on macOS Regressions: classifier-level ( The residual held-forever hole (null start time + pid reuse) is accepted; its precondition is |
|
Re-reviewed |
thymikee
added
the
ready-for-human
|
1 participant
Two reproducible machine-wedging failures in the shared XCTest device-set machinery, hit while live-validating on a heavily loaded shared dev machine. Both left every subsequent XCTest command on the machine failing until manual cleanup.
1. Dangling
XCTestDevicessymlink defeats reconcile (ENOTDIR wedge)reconcileXcodebuildSimulatorSetRedirectdetected symlinks viafs.existsSync(...) && fs.lstatSync(...).isSymbolicLink().existsSyncfollows symlinks, so a redirect whose target set was deleted (e.g. a caller-cleaned scratch--ios-simulator-device-set) read as absent: reconcile skipped the unlink andrenameSync(backup, XCTestDevices)failed withENOTDIRagainst the still-present dangling entry — on every future self-heal attempt too.unlinkIfSymlinkhad the sameexistsSyncguard, so fixing only the detection wouldn't have healed anything.Both sites now detect symlinks with
lstatSync(..., { throwIfNoEntry: false }). Regression test verified red (ENOTDIR path) on the old code.2. Lock-owner liveness: zombies read as live, live owners read as dead
The
xctest-device-set.lockstale-owner reclaim had two inverse holes, confirmed empirically:kill(pid, 0)and still reports its originallstart, so both existing guards read it as live. Verified with a real zombie: stateZN, original start time intact.psread lost to CPU contention (1s timeout) returned null, mismatched the recorded start time, and condemned a genuinely-live owner — waiters then reclaimed a held lock mid-redirect. The test suite already papered over exactly this inpinOwnProcessStartTime(its docstring describes the flake verbatim); that was a production defect surfacing as test flake, and a plausible origin of the dangling-symlink state above.startTime: null(ps lost at acquire) plus PID reuse read as live indefinitely.Fix unifies both liveness surfaces (
isLiveProcessLockOwner, #1647'sclassifyOwnerLiveness) on one proof-oriented classifier:ps -o state=) — they can never release anything;psread is not proof of death and never condemns an alive pid;ps -o etime=bound, 30s slack) — a process born after the lock was acquired cannot be the acquirer.Runner leases and device claims inherit the zombie/no-steal semantics through the shared function. Lock timeout errors now include
ownerLivenessso the next field occurrence immediately separates genuine contention from a classifier miss.Testing
Follow-up candidate:
pinOwnProcessStartTimeinhost-process-mock.tsnow guards a flake that can no longer occur and is likely removable.