diff --git a/FIX.org b/FIX.org index c4a1e18..215528f 100644 --- a/FIX.org +++ b/FIX.org @@ -5629,3 +5629,87 @@ hidden-failure shape a previous lane found. sand.flan also feeds ~@x86~, ~@js~, ~@sanitize~ and ~@valgrind~ through their workspace-file deps, so those go red too. Everything underneath was verified green against a copy of the file with those two lines changed. + +* An accepted re-run is running, 2026-09-21 +Reported as an intermittent failure of test_dev's re-run loops under load: +"after three re-runs the program never printed \"counter 44\"", with the +transcript stopping at counter 43 — three requests, two runs — and, less +often, the second request refused outright with "rerun: the program is already +running". + +Two faces, one cause, and the cause is a state that lagged a decision. +[flan_merged_rerun] took the request under the lock and left [program_state] +as it found it: PROGRAM_PARKED. The flip to PROGRAM_RUNNING happened on the +parked thread, when it got round to waking — and [describe]'s [:parked] reads +that same state. So a caller that asks for a re-run and then waits for the +program to park again was liable to be answered by the park it had just ended. +The wait fell through immediately, the next request went out while the first +run had not started, and one of two things happened: [program_asked] was still +1, so both requests were answered ok and a single run came of them; or the +thread had woken in between, and the second was refused as "already running". +test/test_dev.ml does exactly this shape in four places, three of them in a +loop, which is why it was the tests that found it. + +The window was documented. The comment over [flan_merged_park] said two +re-runs arriving in it are both answered ok for one run, called it a +microsecond wide before the state flip moved in front of the flush and "as +wide as a flush" after, and traded it against a refusal that was simply false. +What it did not account for is the drain that later went in front of the park's +exit: the leaving round polls the agent's ring — a dlopen and a module install, +or a thunk that stops in the break loop — before it breaks. The window was no +longer a flush. It was however long the next thing the program had to do took. + +Fixed by making the state the decision rather than a report of it. The store +is in [flan_merged_rerun], under the lock that accepted the request: accepted +means running, and there is no moment in which a committed re-run reads as +parked. The park's own store on the way back into main stays, now a no-op, as +the one place that knows the thread really is on its way. + +Considered and rejected: teaching the tests to wait on something else — a line +of the new run's output, a counter — rather than on the park. That is the +right fix only if the daemon's contract really is "parked until the thread +wakes", and it is not: the C refuses a second re-run precisely because the +first is committed, so the two answers disagreed about the same fact. The +contract wanted stating, not working around. + +Also considered: keeping the state and refusing a second request while +[program_asked] is set. It closes the collapse and leaves the refusal — the +wait for a park is still answered by the stale one, and the next request is +still refused. Only the state carries both. + +Nothing depends on PARKED lasting until the wakeup. [Program.state] has three +readers: [Dev.liveness], which is every op's guard; [describe]'s [:parked]; +and [flan_merged_wake], which declines a running program because a running +program polls its own ring. The last is the only one whose answer changes in +the window, and the decline is right there too — the round the thread leaves +on drains the ring before main is re-entered, and what misses that drain is +picked up at the new run's first frame boundary, exactly as for any running +program. No test asserts [:parked t] after an accepted re-run; the one that +asserts it against a thunk stopped in the park asks before any re-run. + +There is no path where the request is taken and the run does not happen. The +only way out of the park's loop is the [program_asked] test, and the thread is +either in the wait or in the agent poll that wait runs. What can delay it is a +thunk that stopped in the break loop, which holds the thread inside the poll +until somebody resumes it. The run is still committed — that is why [rerun] +accepts it and says so in a note — and for the duration the state says running +while no frame is executing. Deliberate: [:stopped] is what reports a break +loop, [:parked] reports a park, and this is neither. [Dev.rerun] reads the +liveness and the break for that note *before* it calls [Program.rerun], since +after the call the answer is running by construction. + +The two-process daemon has no such window. Its program is a child process with +no park at all: [flan_program_rerun] finds the weak symbol null and answers +"this session's program is a process of its own", and liveness comes from +[waitpid] — a child that finishes is Gone, not parked. + +Measured rather than argued, on the loop at test/test_dev.ml:5817 driven +standalone against programs/dev-rerun.flan — park, three re-runs each waited +for by [describe], then the transcript. Unloaded: 3 failures in 28 runs, both +faces among them. Under two busy-loop burners, this machine's load average +running 8 to 16: 9 failures in 25 runs. After the fix, same harness and the +same two burners at the same load: 0 in 50. + +Related but separate, and not this: test_reload's "the registry read under a +writer" is a reader racing a writer over the allocation registry, about one run +in five in isolation, and shares nothing with this but the word intermittent. diff --git a/docs/BUILT.md b/docs/BUILT.md index 76055af..419f524 100644 --- a/docs/BUILT.md +++ b/docs/BUILT.md @@ -1430,6 +1430,15 @@ semantics and it is what was asked for. A clean slate is one evaluation away; a **A re-run while the program is running is refused, not queued.** The test and the signal are under one mutex, so the window between them does not exist, and two `main`s writing the same globals at once never starts. +**An accepted re-run is running, from the moment it is accepted.** `flan_merged_rerun` stores `PROGRAM_RUNNING` itself, +under the lock that took the request, rather than leaving the state as it found it for the parked thread to update when +it wakes. That matters to anyone who asks for a re-run and then waits for the program to park again — which is what an +editor does, and what the tests do. While the state lagged the decision, the wait was answered by the park the re-run +had just ended: a second re-run then went out before the first run had started, and the two of them produced one run +between them, or arrived a moment later and were told the program was already running. One state that lagged, two +symptoms. The park's own store of `PROGRAM_RUNNING` on the way back into `main` is now a no-op that documents where the +thread has got to. + `close`ing stdout went with this change, and it had to. That was how the compiler learned the program was done — the pipe read EOF, exactly as the two-process daemon learns it from a dead child — but a pipe delivers EOF *once*, so the signal and the program's output were the same resource: spending it left the second run with nowhere to print. The diff --git a/lib/dev.ml b/lib/dev.ml index b2bad53..035e082 100644 --- a/lib/dev.ml +++ b/lib/dev.ml @@ -3074,11 +3074,27 @@ let abort t = The state is asked twice — here, to say something useful about [Gone], and again inside [Program.rerun], which is the answer that counts. The C does its test and its signal under one lock, so the window between them that this - check cannot see is a window that does not exist there. *) + check cannot see is a window that does not exist there. + + ACCEPTED MEANS RUNNING, and an editor can rely on it. The C stores the new + state as it takes the request rather than when the parked thread gets round + to waking, so a session that asks for a re-run and then waits for the next + park is waiting for the park this run ends with — never for the one it just + left. Before that, the two were indistinguishable for as long as it took the + thread to wake: a wait for "parked" was answered immediately by the old + park, the next request went out while the first run had not started, and the + pair of them produced one run — or, a moment later, a refusal saying the + program was already running. Both faces are gone with the lag. *) let rerun t = match liveness t with | Gone -> error gone | Live | Parked -> + (* Read before the request and not after it. Taking a re-run is what ends + the park — the C stores the new state as it accepts — so by the time + there is an [Ok] to describe, this session's program reads as running + whatever it was doing a moment ago, and the note below would have lost + the one case it exists for. *) + let stopped_in_park = liveness t = Parked && parked_break t in (match Program.rerun () with | Ok () -> (* The park this session was explaining is over, so the park after it @@ -3096,7 +3112,7 @@ let rerun t = here, unlike the running case the C refuses: there is no second main about to start, only one waiting for the thread to be free. *) let note = - if liveness t = Parked && parked_break t then + if stopped_in_park then "accepted; an expression evaluated against the park is stopped in \ the break loop, so main starts once that is resumed or aborted" else @@ -4397,11 +4413,28 @@ static void flan_merged_exit(int32_t status) { * waiting. Nothing is lost in it: [flan_merged_rerun] sets [program_asked] and * signals under the same lock, a signal delivered to nobody is discarded, and * the [while] below reads the flag rather than the wakeup — so a request that - * lands in the window is taken and the wait falls straight through. The one - * visible cost is that two re-runs arriving in that window are both answered - * "ok" for a single run. That race existed before and was a microsecond wide; - * it is now as wide as a flush, which is the right trade against a refusal - * that was simply false. + * lands in the window is taken and the wait falls straight through. + * + * THAT WINDOW USED TO HAVE A COST, and it was paid by whoever asked twice. + * A taken re-run left the state PARKED until this thread got round to waking, + * so the window was one in which the program was going to run and every + * question about it still answered "parked" — as wide as a flush, and wider + * still once the drain below went in front of the exit, because a poll that + * loads a module is in it too. An editor that asks for a re-run and then waits + * for the next park was therefore liable to be answered by the park it had + * just ended: the wait fell through on the *old* park, the next request landed + * in the same window, and the two of them produced one run between them — + * unless the thread woke in between, in which case the second was refused as + * "already running". One cause, two faces, and both are a state that lagged + * the decision. + * + * So the decision is the state now: [flan_merged_rerun] stores + * PROGRAM_RUNNING itself, under the same lock as the test that accepted the + * request. Nothing between the acceptance and the thread's return into main + * can be mistaken for a park, because there is no longer a moment in which an + * accepted re-run reads as parked. The store below is what that leaves: the + * ordinary way out of this loop, and a no-op on the state by the time it + * runs. * * ── THE SECOND THING THE PARK SERVICES ─────────────────────────────── * @@ -4490,6 +4523,10 @@ static void flan_merged_park(void) { if (leaving) break; } program_asked = 0; + /* Already PROGRAM_RUNNING — [flan_merged_rerun] stored it when it took the + * request. Stored again because this is the only place that knows the thread + * is really on its way back into main, and a store that is a no-op is + * cheaper than a reader that has to know which of the two did it. */ program_state = PROGRAM_RUNNING; pthread_mutex_unlock(&program_lock); } @@ -4506,16 +4543,32 @@ static void flan_merged_park(void) { * between the finished run's longjmp and the park is either seen as running * (refused, and the program parks a moment later) or seen as parked (taken). * The park flips the state before it flushes, so the second is now the usual - * answer rather than the lucky one, and what that widens is written out - * there. Queueing it instead would mean a second main starting the - * instant the first finished, which is never what somebody pressing a key - * meant. */ + * answer rather than the lucky one. Queueing it instead would mean a second + * main starting the instant the first finished, which is never what somebody + * pressing a key meant. + * + * TAKING THE REQUEST IS WHAT ENDS THE PARK, so this says so rather than + * leaving it to be said later. The state is stored here, under the lock that + * accepted the request, and not on the parked thread when it happens to wake: + * the park is over the moment a run is committed to, and everything anyone can + * ask between the two is answered by that. Leaving it to the wakeup left a + * window in which an accepted re-run still read as parked — which is the whole + * of the race written out over [flan_merged_park]. + * + * The thread has no say in it and needs none: it is asleep on a condition + * variable inside this park or in the agent poll that park runs, and both + * paths end at the [program_asked] test. Nothing between here and main can + * decline. The one thing that can delay it is a thunk that stopped in the + * break loop, which holds the thread inside the poll until somebody resumes + * it — the run is still committed, still the next thing this thread does, and + * the editor is told so by [rerun]'s own note rather than by the state. */ int flan_merged_rerun(void) { int rc; pthread_mutex_lock(&program_lock); if (program_state != PROGRAM_PARKED) rc = 1; else { program_asked = 1; + program_state = PROGRAM_RUNNING; pthread_cond_signal(&program_wake); rc = 0; } @@ -4533,7 +4586,14 @@ int flan_merged_rerun(void) { * * A running program is refused rather than woken, because there is nothing to * wake: its game thread reaches a frame boundary on its own and polls there. - * The caller treats that as "no wake was needed", not as a failure. */ + * The caller treats that as "no wake was needed", not as a failure. + * + * A program with a re-run already taken reads as running here, and the refusal + * is right for the same reason: the parked thread is on its way out of the + * park, and the round it leaves on drains the ring before it re-enters main. + * A delivery that gets there in time is picked up by that drain and the rest + * at the new run's first frame boundary, which is what a running program does + * with a delivery anyway. */ int flan_merged_wake(void) { int rc; pthread_mutex_lock(&program_lock);