Say "is already running" at once instead of after thirty seconds - #172
Closed
myobie wants to merge 2 commits into
Closed
Say "is already running" at once instead of after thirty seconds#172myobie wants to merge 2 commits into
myobie wants to merge 2 commits into
Conversation
A `pty run` that loses a creation race waited out the whole start budget and then reported a generic publication timeout. `wait_for_publication` compares the published metadata against its OWN pid, so for a loser that check is false for the rest of the budget. Its only other way out of the loop is noticing its own daemon die. When that is slow — a loaded machine, a daemon still starting up — the loop spends the entire thirty second default and reports a timeout, when the true answer was on disk in the first iteration. Measured on a Mac by Silber.pty on 2026-09-03: 30.06 s against a 30 s budget, saying "Timed out waiting for daemon publication" instead of "is already running". The loop now checks whether the name is published by a live process that is not us, and stops with the sentence `pty run` already prints when it sees a running session before it spawns. Losing the race later should not produce a different explanation of the same situation. All three conditions matter and each is tested. Published, or a name whose metadata is still being written would be refused. A different pid, or a successful spawn would refuse itself. A live one, or stale metadata from a dead daemon would make the name permanently unusable. The safety property was never in question. The test that found this asserts exactly one winner before it checks the loser message, and that assertion passed every time. What was wrong is what the loser said, and how long it took.
The new check refuses a session name when it is published by a live process that is not us. It asked `pid_alive` / `isProcessAlive`, and a zombie answers `kill(pid, 0)`. So an unreaped daemon would have counted as live and the name would have been refused for as long as the corpse went unreaped. That is the exact failure the liveness condition exists to prevent, arrived at by the check that was supposed to prevent it. An unreaped daemon is the precise case that matters here: dead, not reaped, still in the process list. Both now ask `has_process_exited_for_reap` / `hasProcessExitedForReap`, which reads the process state and counts a zombie as gone. Tested against a real corpse in both languages, with the predicate the production path actually passes. Reverting to the cheap predicate fails both. Node also exports `hasProcessExitedForReap`, which was private. Silber.cos asked what this check does with a zombie daemon on macOS. The answer was worse than the question: it was wrong on both platforms.
Collaborator
Author
|
Folded into the consolidated pull request for this binary. Nathan asked for one per pty rather than three, and the commits are preserved there in order so the three changes stay legible individually. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Read this first, because the title is alarming and the outcome was not
The safety property held. The test that found this,
concurrent_stealers_cannot_both_win, assertswinners == 1before it checksthe loser's message, and that assertion passed every time. Two processes
never both believed they owned the session.
What was wrong is what the loser said, and how long it took to say it.
The defect
pty runloses a creation race. Its daemon will never publish, because thewinner already did. It then waits thirty seconds and prints:
The true answer — this session is already running — was on disk in the first
iteration of the loop.
Why the loop cannot see it.
wait_for_publicationasksis_published_by(name, my_pid), which compares the published metadata againstits own pid. For a loser that is false for the whole budget. The only other
exit is
check_early_exitnoticing its own daemon die — so when that is slow, aloaded machine or a daemon still starting up, the loop has nothing left to look
at and spends
DEFAULT_START_TIMEOUTin full.Silber.ptymeasured it on a Mac on 2026-09-03: 30.06 s against a 30 sbudget.
The fix
The loop now also asks whether the name is published by a live process that is
not us. If it is, this attempt cannot win, so it stops and says so — using the
sentence
pty runalready prints when it sees a running session before itspawns. Losing the race later should not produce a different explanation of the
same situation.
Thirty seconds becomes immediate, and a generic timeout becomes the truth.
Three conditions, and every one of them is tested
The decision is a pure function taking the owner, our pid, a liveness predicate
and a published predicate, so all four ways of answering "no" are tested
rather than raced for — raced for, only one of them would ever be exercised. A
control confirms it bites: drop the liveness and published checks and two tests
fail.
A bug this PR shipped and then removed, worth reading
The liveness condition first asked
pid_alive/isProcessAlive. A zombieanswers
kill(pid, 0)— measured on Linux: stateZ,kill(pid, 0)succeeds. So an unreaped daemon recorded as the owner would have counted as
live, and the session name would have refused every
pty rununtil somethingreaped the corpse.
The check that exists to prevent a permanently unusable name would have
created one. It was wrong on both platforms, not only macOS.
Both now use
has_process_exited_for_reap/hasProcessExitedForReap, whichreads the process state and counts a zombie as gone. Tested against a real
corpse in each language, with the predicate the production path actually passes.
The plain thing worth saying: I had a control, a pure function and four
negative cases, and none of them looked at the predicate being passed in.
Well-tested code with an untested input is the failure mode that survives good
practice.
On these branches the macOS reader still treats an empty
ps -o stat=field as"exited". That errs in the safe direction here — a silent
psmakes theowner look dead, so this falls through to the existing timeout rather than
falsely refusing a name.
What this is not
It is not a timeout change. The budget is untouched. It is not a fix for the
stale-lock steal that lets both processes get this far —
registry/lock.rsdocuments that separately, and this PR does not open it.
Where it came from
Silber.ptyreported it as a suite-load flake on a Mac and said it lookedunrelated to the work being gated. It was unrelated to that work and it was
not a test problem. It is user-visible: starting something that is already
running should say so at once.
Tests
Node: 1622 passed, 2 failed —
fishandzsh, neither installed onthe machine I ran on, failing identically on unmodified
main.The matching Rust change is
compoundingtech/pty-rust#9; same decision, same threeconditions, same message.