From e691512af3e78a37a1727b6a3f9d11b46b899569 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sat, 12 Sep 2026 10:53:09 +0700 Subject: [PATCH] A render thunk that signals never reaches result_end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The thunk calls flan_dev_result_begin before it evaluates anything, so an expression that signals is stopped inside the seqlock's window — and a restart taken from that break transfers past the thunk, so the matching end never runs. An unpaired begin cost nothing while the counter only moved at the end. It costs everything now: incrementing would leave the count odd for the life of the process, every later read reporting a write in progress, and C-x C-e dead until the program restarts. So begin sets the low bit rather than incrementing, and end clears it by setting rather than adding. The ordinary sequence is unchanged — 2k, 2k+1, 2k+2 — and an abandoned write is over as soon as the next evaluation starts. What that does not fix, because one buffer cannot: an evaluation running while another is stopped mid-render shares the buffer, so the inner value is the one that survives. That was true before the counter was a seqlock and is not a regression. Also noted in NEXT.md: rt_die in flan_rt.c has the same exit-with-the-loader- lock-held shape the break loop just lost. Not fixed with it, because rt_die is the non-dev path too, where there is no listener to deadlock against — whether it should be _exit always or only under --dev is a decision. And the 4K-cap assertions clamp their own String.sub, so a short body prints a failure instead of raising out of the test. --- BUILT.md | 6 +++++- NEXT.md | 6 ++++++ runtime/flan_dev.c | 23 +++++++++++++++++++---- test/test_agent.ml | 14 ++++++++------ 4 files changed, 38 insertions(+), 11 deletions(-) diff --git a/BUILT.md b/BUILT.md index 9cc640a..33036f8 100644 --- a/BUILT.md +++ b/BUILT.md @@ -791,7 +791,11 @@ that loses the race reports the last *complete* generation and no bytes, so a da polling rather than being shown half of one. The count handed out is the number of complete values, so the daemon's "has it moved" still means what it meant. Marking the counter odd needs a release *fence* and not a release store — a release store orders what precedes it, so the writes to the buffer would be free to become visible ahead of it, which -is the original bug with more ceremony. The daemon waits for the count to move rather than assuming the program has +is the original bug with more ceremony. And the odd mark is *set* rather than incremented, because a `begin` with no +`end` is reachable: the thunk calls `begin` before it evaluates anything, so an expression that signals is stopped +inside that window, and a restart taken from the break transfers past the thunk and `end` never runs. Incrementing +would leave the counter odd for the life of the process and every later read reporting "in progress"; setting the bit +means the next evaluation repairs it. The daemon waits for the count to move rather than assuming the program has reached a frame boundary. **This renderer is most of `println`**, which is worth knowing before anyone schedules it. plan.org describes a diff --git a/NEXT.md b/NEXT.md index 7a22e3a..034b5e4 100644 --- a/NEXT.md +++ b/NEXT.md @@ -416,6 +416,12 @@ plan.org's single line on it (831) names a `for` the language does not have and the listener may be holding — a program asked to abort would hang instead of dying; `_exit`, with the streams flushed by hand. The leak was the handle *value* and not the mapping: a module with no installer published nothing, so nothing can point into it, and it is closed. The deadlock is read rather than tested; the exit status is tested. +- **`rt_die` in `flan_rt.c` still calls `exit(134)`**, which is the shape just fixed in the break loop: a trap on the + game thread runs the atexit chain and the ELF destructors, which want the loader lock the agent's listener thread may + be holding inside `dlopen`, so a program that should die could hang. Found while fixing the break loop and not fixed + with it — `rt_die` is the non-dev path too, where there is no listener and nothing to deadlock against, so whether it + should be `_exit` unconditionally or only under `--dev` is a decision rather than a typo. + - **`(A {:x 1})` on a union variant says "unknown struct A"** rather than the union refusal `check_struct` plainly intends — `env` has no table of variant names. A diagnostics bug, not a backend death. diff --git a/runtime/flan_dev.c b/runtime/flan_dev.c index 63c7a5b..8438c1d 100644 --- a/runtime/flan_dev.c +++ b/runtime/flan_dev.c @@ -156,8 +156,21 @@ void flan_dev_result_begin(void) { * and every memcpy in [emit] — would be free to become visible ahead of the * odd count, and a reader could see an even count either side of a copy it * made while the buffer was being overwritten. Which is the bug this - * replaced, with more ceremony. So: mark it relaxed, fence, then write. */ - __atomic_store_n(&generation, generation + 1, __ATOMIC_RELAXED); + * replaced, with more ceremony. So: mark it relaxed, fence, then write. + * + * Setting the low bit rather than incrementing, because a [begin] with no + * [end] is reachable and must not poison the counter for the life of the + * process. A render thunk that signals is stopped inside this window, and a + * restart taken from that break transfers past the thunk — [end] never runs. + * Repairing it here costs nothing in the ordinary case (2k, 2k+1, 2k+2) and + * means an abandoned write is over as soon as the next evaluation starts, + * rather than leaving every later read reporting "in progress" forever. + * + * What it does not fix, because the buffer cannot: an evaluation that runs + * while another is stopped mid-render shares this one buffer, so the inner + * value is the one that survives and the outer thunk, if it is ever resumed, + * appends to it. That was true before the counter was a seqlock. */ + __atomic_store_n(&generation, generation | 1, __ATOMIC_RELAXED); __atomic_thread_fence(__ATOMIC_RELEASE); result_len = 0; result_full = 0; @@ -236,8 +249,10 @@ void flan_dev_result_end(void) { result_len += k; } /* Last, and back to even, so a reader that sees the new generation sees the - * whole value. */ - __atomic_store_n(&generation, generation + 1, __ATOMIC_RELEASE); + * whole value. [| 1] first for the same reason [begin] sets rather than + * increments: this must land on an even count whatever state an abandoned + * write left behind. */ + __atomic_store_n(&generation, (generation | 1) + 1, __ATOMIC_RELEASE); } /* Copy the current value out, with the counter that says which one it is. diff --git a/test/test_agent.ml b/test/test_agent.ml index 86a4108..d5faa57 100644 --- a/test/test_agent.ml +++ b/test/test_agent.ml @@ -398,13 +398,15 @@ let () = if String.length body <> 4096 then fail "the 4K result cap: %d bytes back, header %S" (String.length body) hdr; - if String.length body >= 3 - && String.sub body (String.length body - 3) 3 <> "..." then - fail "a clamped result did not say so: %S" - (String.sub body (String.length body - 8) 8); - if String.length body < 2 || String.sub body 0 2 <> "\"x" then + let tail n = if String.length body < n then body + else String.sub body (String.length body - n) n in + let head n = if String.length body < n then body + else String.sub body 0 n in + if tail 3 <> "..." then + fail "a clamped result did not say so: %S" (tail 8); + if head 2 <> "\"x" then fail "the result is not the value that was rendered: %S" - (String.sub body 0 8) + (head 8) end; (try Sys.remove eso with Sys_error _ -> ()) end;