From 8ce05087c89afc9e109036eda729d7a631dc9d9c Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sat, 12 Sep 2026 10:39:28 +0700 Subject: [PATCH 1/5] flan_dev_result_get was not the seqlock its comment claimed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It read the generation, then a non-atomic length, then returned the buffer itself — and the agent sent those bytes down a socket some time later, while the game thread was free to be a hundred bytes into the next value. A seqlock cannot validate a read that finishes after it returns, so the pointer was the bug and not the ordering. Made into a real one rather than documented down to what it guaranteed, because what it guaranteed was nothing: the generation told the daemon a new value had arrived and said nothing about whether the bytes it then read were that value. Writing the honest comment would have left the daemon's only way of reading a result unsound with a note beside it. The counter is odd for exactly as long as a value is being written. flan_dev_result_read copies into the caller's buffer and checks the counter either side of the copy, retrying if it moved; a reader that loses the race reports the last complete generation and no bytes, so a daemon polling for a new value keeps polling rather than being shown half of one. The count handed out is the number of complete values, so "has it moved" still means what lib/dev.ml takes it to mean. The agent's buffer is RESULT_MAX, so the copy is never truncated. The race itself has no regression test. Arranging it means landing a socket read inside a render thunk from outside the process, which is the same hook the snapshot generation wants. What is tested is that eval still reads back the value it rendered, through test_dev's existing cases. --- runtime/flan_dev.c | 63 +++++++++++++++++++++++++++++++++++---- vendor/agent/flan_agent.c | 11 +++++-- 2 files changed, 66 insertions(+), 8 deletions(-) diff --git a/runtime/flan_dev.c b/runtime/flan_dev.c index ef3427d..946e14d 100644 --- a/runtime/flan_dev.c +++ b/runtime/flan_dev.c @@ -123,7 +123,23 @@ void *flan_dev_global(const char *name, uint64_t size, const void *init) { * * [generation] is what makes the read safe without a handshake. The thunk runs * on the game thread at a frame boundary, whenever that happens to be; the - * daemon waits for the counter to move rather than guessing it has. */ + * daemon waits for the counter to move rather than guessing it has. + * + * It is a *seqlock*, and it has to be a real one, because the reader is the + * agent's listener thread and the writer is the game thread and neither waits + * for the other. The counter is odd for exactly as long as a value is being + * written, so a reader that sees an odd count, or a different count either + * side of its copy, has read a value that was being overwritten underneath it + * and reads again. A count of 2k means k complete values; the count the + * outside world is given is that k, so that the daemon's "has it moved" keeps + * meaning "is there a new value". + * + * The copy is what makes it safe, and the API is shaped around that: a reader + * gets *bytes of its own*, not a pointer into [result]. The pointer version of + * this was the bug — it read the generation, then a length, then handed back + * the buffer itself, and the caller sent it down a socket some time later + * while the game thread was free to be a hundred bytes into the next value. + * A seqlock cannot validate a read that happens after it returns. */ #define RESULT_MAX 4096 static char result[RESULT_MAX]; @@ -132,6 +148,9 @@ static int result_full; static uint64_t generation; void flan_dev_result_begin(void) { + /* Odd first, and only then the reset: the counter has to say "in progress" + * before the buffer stops being the value it used to be. */ + __atomic_store_n(&generation, generation + 1, __ATOMIC_RELEASE); result_len = 0; result_full = 0; } @@ -208,12 +227,44 @@ void flan_dev_result_end(void) { memcpy(result + result_len, ell, k); result_len += k; } - /* Last, so a reader that sees the new generation sees the whole value. */ + /* 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); } -const char *flan_dev_result_get(uint64_t *gen, uint64_t *len) { - *gen = __atomic_load_n(&generation, __ATOMIC_ACQUIRE); - *len = (uint64_t)result_len; - return result; +/* Copy the current value out, with the counter that says which one it is. + * + * Returns 1 having copied a value that was complete for the whole of the copy, + * 0 if the game thread was in the middle of writing one — in which case [gen] + * is the last *complete* value's number and [len] is 0, so a caller polling + * for a new one keeps polling instead of being handed half of it. Spinning + * here is bounded: the writer is a render thunk between frames, not a loop, + * and the reader is the listener thread, which has nothing better to do. + * + * [cap] is the caller's buffer. A value longer than it is truncated, which is + * the only failure this can have and is a clamp rather than an overrun; the + * agent sizes its buffer at RESULT_MAX so it does not arise. */ +int flan_dev_result_read(char *dst, uint64_t cap, uint64_t *gen, + uint64_t *len) { + for (int attempt = 0; attempt < 64; attempt++) { + uint64_t g1 = __atomic_load_n(&generation, __ATOMIC_ACQUIRE); + if (g1 & 1) continue; /* a write is in progress */ + size_t n = __atomic_load_n(&result_len, __ATOMIC_RELAXED); + if (n > RESULT_MAX) n = RESULT_MAX; /* a torn read cannot overrun */ + if ((uint64_t)n > cap) n = (size_t)cap; + memcpy(dst, result, n); + /* The copy must be ordered before the second read of the counter, or the + * check is of a copy the compiler was free to make afterwards. */ + __atomic_thread_fence(__ATOMIC_ACQUIRE); + if (__atomic_load_n(&generation, __ATOMIC_ACQUIRE) == g1) { + *gen = g1 / 2; + *len = (uint64_t)n; + return 1; + } + } + /* Integer division is the same answer either side of a write in progress: + * during value k the counter is 2k-1 and k-1 are complete. */ + *gen = __atomic_load_n(&generation, __ATOMIC_ACQUIRE) / 2; + *len = 0; + return 0; } diff --git a/vendor/agent/flan_agent.c b/vendor/agent/flan_agent.c index eed8ad6..d8cdef8 100644 --- a/vendor/agent/flan_agent.c +++ b/vendor/agent/flan_agent.c @@ -47,7 +47,13 @@ typedef void (*install_fn)(void); * is consistent. */ typedef void (*call_fn)(void); -const char *flan_dev_result_get(uint64_t *gen, uint64_t *len); +/* The value of the last evaluated expression, copied out under the seqlock in + * runtime/flan_dev.c rather than borrowed. The buffer below is RESULT_MAX, so + * the copy is never truncated; a read that loses the race reports the last + * complete generation and no bytes, which leaves the daemon polling rather + * than showing it half a value. */ +#define RESULT_MAX 4096 +int flan_dev_result_read(char *dst, uint64_t cap, uint64_t *gen, uint64_t *len); /* A ring the listener writes and the game thread reads. One producer, one * consumer, so two atomics and no lock — the game thread must never block on @@ -568,7 +574,8 @@ static void serve(int fd) { } if (strcmp(line, "result") == 0) { uint64_t gen = 0, len = 0; - const char *v = flan_dev_result_get(&gen, &len); + char v[RESULT_MAX]; + flan_dev_result_read(v, sizeof v, &gen, &len); char hdr[64]; int k = snprintf(hdr, sizeof hdr, "%llu %llu\n", (unsigned long long)gen, (unsigned long long)len); From b54f24873e70a064f475c1c1b953921d9c0a10da Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sat, 12 Sep 2026 10:39:46 +0700 Subject: [PATCH 2/5] The job ring never looked at tail, and the comment described a drop it never did MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit publish() wrote queue[head % QUEUE] without consulting tail, so the 65th module queued between two agent/poll calls landed on the slot the game thread was reading — twenty-four bytes of function pointers copied field by field with no atomic near them, so the consumer could take half of one job and half of another and call it. The comment claimed the overflow dropped the oldest request; nothing did that. A full ring is refused now, at the sender, before the dlopen. Dropping loses a reload the sender was told was ok, which is the same lie more quietly; blocking stalls the accept loop, which serves connections inline, so a program that had stopped polling would also stop answering status and abort — the dev loop would have no way to reach a program that had stopped listening to it. The check is separate from the store because there is one producer: room, once seen, cannot be taken away. Two smaller defects in the same file: A module with no flan_reload_install was refused and its handle dropped on the floor. Not an exception to "nothing is ever dlclosed" — that rule is about a module something points into, and this one installed nothing, so no cell names it. What leaked was the handle value rather than the mapping: dlopen refcounts by path, so re-sending the same bad file raised a count nothing could lower. exit(134) from the break loop runs the atexit chain and the ELF destructors, which want the loader lock the listener thread may be holding inside dlopen. A program asked to abort would hang instead of dying. _exit, with the streams flushed by hand at each call site. The deadlock itself is read rather than tested; what the tests pin is that the exit status is still 134. programs/agent-queue.flan blocks on stdin so the window is held open by the test rather than by a timer: it takes 64 modules, refuses the 65th with a reason, and installs 64 when it finally polls. noinstall.c's destructor prints while the program is still running, which is the only way to see the close — at exit the loader runs every destructor whether anything was closed or not. Both halves fail on the old code. --- test/dune | 3 + test/noinstall.c | 16 ++++++ test/programs/agent-queue.flan | 36 ++++++++++++ test/test_agent.ml | 102 ++++++++++++++++++++++++++++++++- vendor/agent/flan_agent.c | 99 ++++++++++++++++++++++++++++---- 5 files changed, 244 insertions(+), 12 deletions(-) create mode 100644 test/noinstall.c create mode 100644 test/programs/agent-queue.flan diff --git a/test/dune b/test/dune index 4efb846..96d60fa 100644 --- a/test/dune +++ b/test/dune @@ -25,6 +25,9 @@ (glob_files programs/*.flan) ; The reload primitive's host: a C main that dlopens what Build.shared made. (file reload_host.c) + ; A shared object that is not a redefinition module, for the agent's refusal + ; path. Its destructor is what proves the handle was closed rather than lost. + (file noinstall.c) ; test_dev runs the compiler itself: flan dev launches and owns a program. (file %{workspace_root}/bin/main.exe) ; The Emacs client, which test_emacs drives against a real daemon. diff --git a/test/noinstall.c b/test/noinstall.c new file mode 100644 index 0000000..34c6623 --- /dev/null +++ b/test/noinstall.c @@ -0,0 +1,16 @@ +/* A shared object that is not a redefinition module: it loads, and it has no + * flan_reload_install for the agent to find. + * + * The destructor is the observation. The agent used to drop the handle on the + * floor when it refused a module like this one — the mapping stayed, the + * reference count went up, and the one handle that could have brought it down + * was gone. Now the module is closed, because nothing was installed from it + * and so nothing can point into it, and this line appears while the program is + * still running. Waiting for the program to exit would prove nothing: the + * loader runs every destructor at exit whether anything was closed or not. */ +#include + +__attribute__((destructor)) static void unloaded(void) { + printf("unloaded\n"); + fflush(stdout); +} diff --git a/test/programs/agent-queue.flan b/test/programs/agent-queue.flan new file mode 100644 index 0000000..23e25e9 --- /dev/null +++ b/test/programs/agent-queue.flan @@ -0,0 +1,36 @@ +;;;; The job ring between the listener thread and the game thread, and what it +;;;; does when it is full. +;;;; +;;;; The window this needs is "the listener has queued modules the game thread +;;;; has not looked at yet", and that window has to be held open by something +;;;; other than a timer — how long sixty-five connections take on a loaded +;;;; machine is exactly the kind of race a test must not be. So the program +;;;; blocks on stdin: the test fills the ring, checks what the agent said, and +;;;; only then writes the byte that lets the program poll. +(import agent "vendor:agent") + +;;; libc's, declared straight: no aggregate crosses the boundary, so there is +;;; nothing for a shim to do. +(declare stdin-byte [] i32 "getchar") + +(defvar ticks i64) + +(defn tick [] i64 + (set ticks (+ ticks 1)) + ticks) + +(defn main [args [string]] i32 + (if (< (len args) 2) + (do (println "usage: agent-queue ") 2) + (do + (if (< (agent/start (at args 1)) 0) + (do (println "cannot listen") 1) + (do + (println "ready") + (stdin-byte) + ;; How many the ring actually held. Every module queued is installed + ;; here, so this number is the count of slots that survived — which + ;; is the whole claim: a ring that overwrote the slot it was reading + ;; would answer with something else. + (print (agent/poll)) (println "") + 0))))) diff --git a/test/test_agent.ml b/test/test_agent.ml index 557c33b..1e9f4e4 100644 --- a/test/test_agent.ml +++ b/test/test_agent.ml @@ -285,8 +285,108 @@ let () = fail "break loop transcript\n got: %S\n wanted: %S" got want end; + (* ── The job ring, and what a full one does ─────────────────────── *) + + (* Two claims, one program. A module the agent refuses because it carries + no installer is *closed* rather than leaked; and a ring with no room + refuses the module instead of overwriting the slot the game thread is + reading. + + The program blocks on stdin until the test has finished filling the + ring, so neither claim is a race against how fast sixty-five + connections are served. *) + let qsock = tmp "queue.sock" and qout = tmp "queue.out" in + (try Sys.remove qsock with Sys_error _ -> ()); + let qt, ql = Session.create ~file:"programs/agent-queue.flan" () in + let qexe = tmp "queue" in + ignore + (Build.executable ~opts:dev ~csrcs:ql.Load.csrcs ~lflags:ql.Load.lflags + qt.Session.host ~out:qexe); + (* One module, sent many times. dlopen keys on the path, so this is the + same relocation over and over — what is being counted is publishes, and + building sixty-five of them would measure llc instead. *) + let qso = tmp "queue-tick.so" in + let qc = Session.eval qt "(defn tick [] i64 (set ticks (+ ticks 1)) ticks)" in + ignore (Build.shared ~opts:dev ~ir:qc.Session.ir ~out:qso ()); + let noinstall = tmp "noinstall.so" in + let cc = + Printf.sprintf "clang -shared -fPIC -o %s noinstall.c 2>/dev/null" + (Filename.quote noinstall) + in + if Sys.command cc <> 0 then fail "could not build noinstall.so" + else begin + let rfd, wfd = Unix.pipe () in + let qfd = + Unix.openfile qout [ Unix.O_WRONLY; Unix.O_CREAT; Unix.O_TRUNC ] 0o600 + in + let qpid = Unix.create_process qexe [| qexe; qsock |] rfd qfd qfd in + Unix.close qfd; + Unix.close rfd; + let qtext () = In_channel.with_open_bin qout In_channel.input_all in + let has needle = + let t = qtext () in + List.exists (String.equal needle) (String.split_on_char '\n' t) + in + if not (await (fun () -> Sys.file_exists qsock && has "ready")) then begin + fail "the queue program never bound its socket"; + (try Unix.kill qpid Sys.sigkill with Unix.Unix_error _ -> ()) + end + else begin + (* A module with no installer. The refusal was always there; what is + new is that the handle is closed, and the destructor saying so + *while the program is still running* is the only way to see it — + at exit the loader would run it either way. *) + let r = send qsock noinstall in + if r <> "err no flan_reload_install\n" then + fail "a module with no installer: %S" r; + if not (await (fun () -> has "unloaded")) then + fail "the refused module was not closed: %S" (qtext ()); + + (* QUEUE slots, then one more. The one more is refused, at the sender, + with a reason — the old code took it, wrote it over slot 0, and + said ok. *) + let queue_size = 64 in + let bad = ref "" in + for _ = 1 to queue_size do + let r = send qsock qso in + if r <> "ok\n" && !bad = "" then bad := r + done; + if !bad <> "" then fail "a module that fitted was refused: %S" !bad; + let full = send qsock qso in + if full <> "err reload queue full; the program is not calling agent/poll\n" + then fail "a full ring did not refuse: %S" full; + + (* Let it poll. Every slot the ring kept is installed here, so the + number is how many survived — 64, not 65 and not some torn count. *) + ignore (Unix.write wfd (Bytes.of_string "\n") 0 1); + Unix.close wfd; + let qstatus = ref (Unix.WEXITED 0) in + let reaped = + await ~ms:5000 (fun () -> + match Unix.waitpid [ Unix.WNOHANG ] qpid with + | 0, _ -> false + | _, s -> qstatus := s; true) + in + if not reaped then begin + (try Unix.kill qpid Sys.sigkill with Unix.Unix_error _ -> ()); + fail "the queue program never finished" + end + else begin + let got = + String.concat "\n" + (List.filter (fun l -> l <> "") + (String.split_on_char '\n' (qtext ()))) + in + if !qstatus <> Unix.WEXITED 0 || got <> "ready\nunloaded\n64" then + fail "job ring\n got: %S\n wanted: %S" got + "ready\nunloaded\n64" + end + end + end; + List.iter (fun f -> try Sys.remove f with Sys_error _ -> ()) - [ exe; so1; so2; sock; out; bsock; bout; bexe ]; + [ exe; so1; so2; sock; out; bsock; bout; bexe; qexe; qso; qsock; qout; + noinstall ]; if !failures = 0 then print_endline "agent: all tests passed" else begin Printf.printf "\n%d failure(s)\n" !failures; diff --git a/vendor/agent/flan_agent.c b/vendor/agent/flan_agent.c index d8cdef8..60f49f2 100644 --- a/vendor/agent/flan_agent.c +++ b/vendor/agent/flan_agent.c @@ -17,8 +17,11 @@ * that is when the swap becomes visible. Nothing else in the program needs to * know the agent exists. * - * Nothing is ever dlclosed: a cell holds an address inside a module's text, - * and unloading it would leave every call site pointing at unmapped memory. + * Nothing that published anything is ever dlclosed: a cell holds an address + * inside a module's text, and unloading it would leave every call site + * pointing at unmapped memory. The two modules that are closed are the ones + * nothing can point into — a transient thunk, which installs no bodies, and a + * module refused before it was queued. * * This is not a protocol. One line per request, the path to load, and a one * line answer. The daemon and its nREPL are a separate program that will speak @@ -57,8 +60,27 @@ int flan_dev_result_read(char *dst, uint64_t cap, uint64_t *gen, uint64_t *len); /* A ring the listener writes and the game thread reads. One producer, one * consumer, so two atomics and no lock — the game thread must never block on - * the loader. Overflow drops the oldest request rather than stalling; a dev - * loop that queues 64 reloads between two frames has a bigger problem. */ + * the loader. + * + * A full ring is *refused*, at the sender, with an error. The previous comment + * here claimed it dropped the oldest request, and nothing did that: [publish] + * never read [tail], so the 65th module overwrote the slot the game thread was + * reading — twenty-four bytes of function pointers, copied field by field with + * no atomic anywhere near them, so the consumer could take half of one job and + * half of another and call it. Silent corruption of the one thing in this file + * that gets *called*. + * + * Of the three honest answers, refusing is the only one that reaches the + * person who asked. Dropping loses a reload the sender was told was ok — the + * same lie in quieter clothes. Blocking stalls the accept loop, which serves + * connections inline, so a program that has stopped polling would also stop + * answering [status] and [abort]: the dev loop would have no way to say + * anything to a program that had stopped listening to it. A refusal is a line + * the daemon can show, and the fix is to call agent/poll. + * + * The check is safe to make separately from the store because there is exactly + * one producer — this thread — so room, once seen, cannot be taken away by + * anyone: the consumer only ever makes more of it. */ #define QUEUE 64 /* [handle] is set only for a module that declared itself transient — one that * ran a thunk and left nothing behind. Everything else is kept mapped forever: @@ -74,12 +96,26 @@ static int listen_fd = -1; static pthread_t listener; static atomic_int started; -static void publish(job j) { +/* Room for one more. Unsigned subtraction, so the answer survives [head] and + * [tail] wrapping; only their difference means anything. */ +static int queue_room(void) { unsigned h = atomic_load_explicit(&head, memory_order_relaxed); + unsigned t = atomic_load_explicit(&tail, memory_order_acquire); + return (h - t) < QUEUE; +} + +/* 0 if the ring is full, having published nothing. Checked here as well as at + * the caller, because a producer that forgot would otherwise reintroduce + * exactly the overwrite this replaced. */ +static int publish(job j) { + unsigned h = atomic_load_explicit(&head, memory_order_relaxed); + unsigned t = atomic_load_explicit(&tail, memory_order_acquire); + if (h - t >= QUEUE) return 0; queue[h % QUEUE] = j; /* Release: the store to the slot must be visible before the index that * advertises it. */ atomic_store_explicit(&head, h + 1, memory_order_release); + return 1; } /* Returns how many modules were installed. Call it between frames. */ @@ -246,6 +282,22 @@ static char condition_name[128]; int32_t flan_agent_poll(void); +/* Every way out of the break loop that is not a resume. [_exit] and not + * [exit], because this runs on the game thread while the listener thread may + * be inside [dlopen] holding the loader lock — and [exit] runs the atexit + * chain and the ELF destructors, which want that same lock. A program asked to + * abort would hang instead of dying, which is the failure mode the break loop + * exists to replace. Nothing here needs an orderly teardown: the streams are + * flushed by hand above every call. + * + * 134 is kept because that is what a trap exits with; see rt_die in + * flan_rt.c. */ +static _Noreturn void die_now(void) { + fflush(stdout); + fflush(stderr); + _exit(134); +} + static void break_loop(const uint8_t *name, int64_t namelen, void *condition, void *xfer) { struct timespec step = { 0, 2000000 }; /* 2ms */ @@ -263,7 +315,7 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition, fprintf(stderr, "flan: %d nested break loops - giving up rather than " "spinning\n", BREAK_MAX); fflush(stderr); - exit(134); + die_now(); } { snapshot *s = snap_top(); @@ -302,14 +354,14 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition, "flan: %d nested break loops — giving up rather than spinning\n", BREAK_MAX); fflush(stderr); - exit(134); + die_now(); } for (;;) { flan_agent_poll(); if (atomic_load(&aborting)) { fflush(stdout); fprintf(stderr, "flan: aborted at the break loop\n"); - exit(134); + die_now(); } if (atomic_load(&chosen_ready)) { /* Claimed into a local and the flag cleared *before* the attempt. The @@ -585,6 +637,15 @@ static void serve(int fd) { } return; } + /* Before the dlopen, not after it: a module there is no room to queue is + * one there is no point relocating, and refusing here means no handle is + * taken for it at all. Only this thread produces, so room seen now is room + * still there at [publish] below. */ + if (!queue_room()) { + reply(fd, "err reload queue full; the program is not calling " + "agent/poll\n"); + return; + } void *h = dlopen(line, RTLD_NOW | RTLD_LOCAL); if (h == NULL) { reply(fd, "err "); @@ -593,7 +654,19 @@ static void serve(int fd) { return; } install_fn f = (install_fn)(uintptr_t)dlsym(h, "flan_reload_install"); - if (f == NULL) { reply(fd, "err no flan_reload_install\n"); return; } + if (f == NULL) { + /* Closed, and this is not an exception to "nothing is ever dlclosed". + * That rule is about a module something *points into* — a cell holding + * an address in its text. This one published nothing: no installer ran, + * so no cell names it, and it is unreachable the moment this function + * returns. What leaked before was the handle value rather than the + * mapping — dlopen refcounts by path, so re-sending the same bad file + * bumped a count nothing could ever bring down, and the one reference + * that could was dropped on the floor here. */ + dlclose(h); + reply(fd, "err no flan_reload_install\n"); + return; + } /* Optional: only an expression evaluation has one. */ call_fn c = (call_fn)(uintptr_t)dlsym(h, "flan_reload_call"); /* And only one that leaves nothing behind may be unloaded. */ @@ -605,8 +678,12 @@ static void serve(int fd) { * sender as a reset, not as an answer. */ reply(fd, "ok\n"); /* "queued", not "installed": the store happens on the game thread, at a - * time this thread does not get to choose. */ - publish((job){ f, c, transient == NULL ? NULL : h }); + * time this thread does not get to choose. The room was checked before the + * dlopen and only this thread consumes it, so this cannot fail; it is + * asserted rather than assumed because a silent [publish] that did nothing + * is the failure being fixed. */ + if (!publish((job){ f, c, transient == NULL ? NULL : h })) + fprintf(stderr, "flan: reload queue full after it was checked\n"); return; } } From e22a8dba8292cff0798ca9937dce5aa23552af46 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sat, 12 Sep 2026 10:47:41 +0700 Subject: [PATCH 3/5] Two of the four buffers with no evidence now have some MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 4K result cap and condition_name[128] are on the agent's socket path, which is why the sanitizer corpus cannot reach them: a program in the sweep has no socket and nobody on the other end of it. test_agent has both. A 5000-byte string literal evaluated into the running program comes back as exactly 4096 bytes ending in the ellipsis result_end puts there to say it clamped — and it comes back through the seqlock's copy, so the cap and the new reader are pinned by the same case. The header also shows the generation as 1, which is the count of complete values rather than the raw counter. A condition class of 198 characters comes back from `status` as 127 and a terminator. Aborting out of that break is what pins the exit status at 134 now that the loop leaves with _exit rather than exit. SNAP_MAX, SNAP_NAMES and the dev registry's overflow guard are still read rather than tested. Sixty-five nested restart-cases and four thousand interned names are a lot of program to write for a clamp each, and neither is on a path this session changed. flan_dev_result_cap() exists so the size is asked for rather than written down in two files: "the copy is never truncated" is only true while the agent's buffer and the runtime's bound agree, and the agent checks that where the copy happens. The pipe the queue program blocks on is close-on-exec, or the child inherits the write end and its own stdin never reaches end of file — it sat in its last read waiting for a byte only it could send. --- runtime/flan_dev.c | 18 ++++- test/programs/agent-longname.flan | 15 ++++ test/programs/agent-queue.flan | 7 ++ test/test_agent.ml | 116 ++++++++++++++++++++++++++++-- vendor/agent/flan_agent.c | 12 +++- 5 files changed, 161 insertions(+), 7 deletions(-) create mode 100644 test/programs/agent-longname.flan diff --git a/runtime/flan_dev.c b/runtime/flan_dev.c index 946e14d..63c7a5b 100644 --- a/runtime/flan_dev.c +++ b/runtime/flan_dev.c @@ -149,8 +149,16 @@ static uint64_t generation; void flan_dev_result_begin(void) { /* Odd first, and only then the reset: the counter has to say "in progress" - * before the buffer stops being the value it used to be. */ - __atomic_store_n(&generation, generation + 1, __ATOMIC_RELEASE); + * before the buffer stops being the value it used to be. + * + * The fence is the half of that a release *store* cannot do. A release store + * orders what comes before it, not what comes after, so the writes below — + * 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); + __atomic_thread_fence(__ATOMIC_RELEASE); result_len = 0; result_full = 0; } @@ -244,6 +252,12 @@ void flan_dev_result_end(void) { * [cap] is the caller's buffer. A value longer than it is truncated, which is * the only failure this can have and is a clamp rather than an overrun; the * agent sizes its buffer at RESULT_MAX so it does not arise. */ +/* What a caller's buffer has to be for the copy never to be truncated. The + * bound is declared in one place and asked for rather than written down twice: + * the agent's buffer and this one agreeing is the whole of "never truncated", + * and two literals in two files is how that stops being true. */ +uint64_t flan_dev_result_cap(void) { return RESULT_MAX; } + int flan_dev_result_read(char *dst, uint64_t cap, uint64_t *gen, uint64_t *len) { for (int attempt = 0; attempt < 64; attempt++) { diff --git a/test/programs/agent-longname.flan b/test/programs/agent-longname.flan new file mode 100644 index 0000000..c1a7d2f --- /dev/null +++ b/test/programs/agent-longname.flan @@ -0,0 +1,15 @@ +;;;; The agent's condition_name[128], which had no coverage at all: it is on +;;;; the socket path, so nothing in the sanitizer corpus can reach it. +;;;; +;;;; The condition class here is 200 characters, so the copy into that buffer +;;;; has to clamp, and [status] answers with the 127 that fit. The name is ugly +;;;; on purpose — its length is the whole of the test. +(import agent "vendor:agent") + +(defstruct MissingYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYY [id i32]) + +(defn main [] i32 + ;; The path is overridden by FLAN_AGENT_SOCKET; a program has to name one. + (agent/start "/tmp/flan-longname.sock") + (error (MissingYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYY {:id 1})) + 0) diff --git a/test/programs/agent-queue.flan b/test/programs/agent-queue.flan index 23e25e9..6d98ec6 100644 --- a/test/programs/agent-queue.flan +++ b/test/programs/agent-queue.flan @@ -33,4 +33,11 @@ ;; is the whole claim: a ring that overwrote the slot it was reading ;; would answer with something else. (print (agent/poll)) (println "") + ;; Round two: one evaluated expression, whose rendering is longer + ;; than the 4K the dev runtime will hold. The program has to still be + ;; here afterwards for the value to be read back off the socket, so + ;; it waits a third time and leaves on end of file. + (stdin-byte) + (print (agent/poll)) (println "") + (stdin-byte) 0))))) diff --git a/test/test_agent.ml b/test/test_agent.ml index 1e9f4e4..86a4108 100644 --- a/test/test_agent.ml +++ b/test/test_agent.ml @@ -315,7 +315,11 @@ let () = in if Sys.command cc <> 0 then fail "could not build noinstall.so" else begin - let rfd, wfd = Unix.pipe () in + (* Close-on-exec, or the child inherits the write end and its own stdin + never reaches end of file: the program would sit in its last read + waiting for a byte only it could send. The read end is dup'd onto fd 0 + by [create_process], which clears the flag on the copy. *) + let rfd, wfd = Unix.pipe ~cloexec:true () in let qfd = Unix.openfile qout [ Unix.O_WRONLY; Unix.O_CREAT; Unix.O_TRUNC ] 0o600 in @@ -359,6 +363,51 @@ let () = (* Let it poll. Every slot the ring kept is installed here, so the number is how many survived — 64, not 65 and not some torn count. *) ignore (Unix.write wfd (Bytes.of_string "\n") 0 1); + if not (await (fun () -> has "64")) then + fail "the ring never drained: %S" (qtext ()) + else begin + (* ── The 4K result cap ─────────────────────────────────────── + + NEXT.md names this buffer twice as having no coverage at all, + because it is on the agent's path and so needs a socket. There is + a socket here. The value is a 5000-byte string literal, which is + longer than anything [emit] will keep, so what comes back is the + clamp and the ellipsis [result_end] puts there to say it clamped + — and it comes back through the seqlock's copy rather than off a + borrowed pointer. *) + let long = String.make 5000 'x' in + let ec = Session.eval_expr qt ("\"" ^ long ^ "\"") in + let eso = tmp "queue-eval.so" in + ignore (Build.shared ~opts:dev ~ir:ec.Session.ir ~out:eso ()); + let r = send qsock eso in + if r <> "ok\n" then fail "the eval module was not queued: %S" r; + ignore (Unix.write wfd (Bytes.of_string "\n") 0 1); + if not (await (fun () -> has "1")) then + fail "the eval thunk never ran: %S" (qtext ()) + else begin + let reply = send qsock "result" in + match String.index_opt reply '\n' with + | None -> fail "no result header: %S" reply + | Some i -> + let hdr = String.sub reply 0 i in + let body = + String.sub reply (i + 1) (String.length reply - i - 1) + in + (* Exactly the cap, ellipsis included: [result_end] makes room + for it rather than assuming there is any. *) + 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 + fail "the result is not the value that was rendered: %S" + (String.sub body 0 8) + end; + (try Sys.remove eso with Sys_error _ -> ()) + end; Unix.close wfd; let qstatus = ref (Unix.WEXITED 0) in let reaped = @@ -377,16 +426,75 @@ let () = (List.filter (fun l -> l <> "") (String.split_on_char '\n' (qtext ()))) in - if !qstatus <> Unix.WEXITED 0 || got <> "ready\nunloaded\n64" then + if !qstatus <> Unix.WEXITED 0 || got <> "ready\nunloaded\n64\n1" then fail "job ring\n got: %S\n wanted: %S" got - "ready\nunloaded\n64" + "ready\nunloaded\n64\n1" end end end; + (* ── condition_name[128] ────────────────────────────────────────── *) + + (* The other named buffer with no coverage at all, and it needs a socket + for the same reason: nothing but [status] ever reads it. The condition + class is 198 characters, so what comes back is the 127 that fit and a + terminator — a clamp, not an overrun, and now measured rather than + read. Aborting out of it also pins that the break loop's way out is + still exit status 134 now that it takes it with _exit. *) + let lsock = tmp "long.sock" and lout = tmp "long.out" in + (try Sys.remove lsock with Sys_error _ -> ()); + let lt, ll = Session.create ~file:"programs/agent-longname.flan" () in + let lexe = tmp "long" in + ignore + (Build.executable ~opts:dev ~csrcs:ll.Load.csrcs ~lflags:ll.Load.lflags + lt.Session.host ~out:lexe); + let lfd = Unix.openfile lout [ Unix.O_WRONLY; Unix.O_CREAT; Unix.O_TRUNC ] 0o600 in + let lenv = + Array.append (Unix.environment ()) [| "FLAN_AGENT_SOCKET=" ^ lsock |] + in + let lpid = Unix.create_process_env lexe [| lexe |] lenv Unix.stdin lfd lfd in + Unix.close lfd; + let stopped = ref "" in + if not + (await (fun () -> + Sys.file_exists lsock + && (stopped := send lsock "status"; + String.length !stopped > 8 + && String.sub !stopped 0 8 = "stopped "))) + then begin + fail "the long-named condition never stopped: %S" !stopped; + (try Unix.kill lpid Sys.sigkill with Unix.Unix_error _ -> ()) + end + else begin + let got = String.trim (String.sub !stopped 8 (String.length !stopped - 8)) in + if String.length got <> 127 then + fail "condition_name clamps at 127: got %d characters" + (String.length got); + if String.length got >= 7 && String.sub got 0 7 <> "Missing" then + fail "the clamped name is not the condition's: %S" got; + ignore (send lsock "abort"); + let lstatus = ref (Unix.WEXITED 0) in + let reaped = + await ~ms:5000 (fun () -> + match Unix.waitpid [ Unix.WNOHANG ] lpid with + | 0, _ -> false + | _, s -> lstatus := s; true) + in + if not reaped then begin + (try Unix.kill lpid Sys.sigkill with Unix.Unix_error _ -> ()); + fail "abort did not end the program" + end + else if !lstatus <> Unix.WEXITED 134 then + fail "abort left status %s, wanted exit 134" + (match !lstatus with + | Unix.WEXITED c -> Printf.sprintf "exit %d" c + | Unix.WSIGNALED c -> Printf.sprintf "signal %d" c + | Unix.WSTOPPED c -> Printf.sprintf "stopped %d" c) + end; + List.iter (fun f -> try Sys.remove f with Sys_error _ -> ()) [ exe; so1; so2; sock; out; bsock; bout; bexe; qexe; qso; qsock; qout; - noinstall ]; + noinstall; lexe; lsock; lout ]; if !failures = 0 then print_endline "agent: all tests passed" else begin Printf.printf "\n%d failure(s)\n" !failures; diff --git a/vendor/agent/flan_agent.c b/vendor/agent/flan_agent.c index 60f49f2..fedb300 100644 --- a/vendor/agent/flan_agent.c +++ b/vendor/agent/flan_agent.c @@ -56,7 +56,9 @@ typedef void (*call_fn)(void); * complete generation and no bytes, which leaves the daemon polling rather * than showing it half a value. */ #define RESULT_MAX 4096 -int flan_dev_result_read(char *dst, uint64_t cap, uint64_t *gen, uint64_t *len); +int flan_dev_result_read(char *dst, uint64_t cap, uint64_t *gen, + uint64_t *len); +uint64_t flan_dev_result_cap(void); /* A ring the listener writes and the game thread reads. One producer, one * consumer, so two atomics and no lock — the game thread must never block on @@ -627,6 +629,14 @@ static void serve(int fd) { if (strcmp(line, "result") == 0) { uint64_t gen = 0, len = 0; char v[RESULT_MAX]; + /* The one thing that could make the copy truncate, checked where it + * would happen rather than trusted to two files holding the same + * number. */ + if (flan_dev_result_cap() > sizeof v) { + reply(fd, "err the agent's result buffer is smaller than the " + "runtime's\n"); + return; + } flan_dev_result_read(v, sizeof v, &gen, &len); char hdr[64]; int k = snprintf(hdr, sizeof hdr, "%llu %llu\n", (unsigned long long)gen, From 66dcf30c8af98cfb7545eb8f86dd94da7d0c6e29 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sat, 12 Sep 2026 10:49:05 +0700 Subject: [PATCH 4/5] Strike four fixed defects, and say why the dlclose rule has two exceptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NEXT.md: the ring, the seqlock, the break loop's exit and the leaked handle are struck with what each was fixed to rather than only that it was. The snapshot generation stays open — it wants a hook a test can drive, which is a design decision and not a fix. The four-buffer paragraph is now two and two. BUILT.md carries the reasoning that outlives the change. "Nothing is ever dlclosed" is restated as "nothing that published anything is ever dlclosed", because that is what the rule was always about — being pointed into — and the two modules that are closed are the ones nothing can point into. Stating it the weaker way is what made a dropped handle look like obedience. The agent section gains why a full ring refuses rather than drops or blocks, and why the break loop leaves with _exit. The renderer section gains why the result counter had to become a real seqlock and why marking it odd needs a release fence rather than a release store — a release store orders what precedes it, so the buffer writes could still be hoisted over it, which is the original bug with more ceremony. One correction: the release-build story named flan_dev_result_get as the symbol that came up undefined. That symbol no longer exists. dune test green; dune build @sanitize clean. --- BUILT.md | 38 +++++++++++++++++++++++++++++++++----- NEXT.md | 43 +++++++++++++++++++++++++++++-------------- 2 files changed, 62 insertions(+), 19 deletions(-) diff --git a/BUILT.md b/BUILT.md index 1fe5847..9cc640a 100644 --- a/BUILT.md +++ b/BUILT.md @@ -382,7 +382,7 @@ slice parameter. **`flan_dev.c` is compiled into every build, not only a dev one.** Nothing in a release build calls into it — the compiler only emits a registry lookup for a name the host was not built with, which cannot arise without cells — but the agent package's C refers to it, and a package's C sources are collected whatever `main` does. Leaving it out of release -builds made `flan build sand.flan` fail at the link with an undefined `flan_dev_result_get`, which reads as a compiler +builds made `flan build sand.flan` fail at the link with an undefined `flan_dev_result_read`, which reads as a compiler bug rather than as a missing flag. The table is BSS, so the cost is address space and not binary size; `-rdynamic` and the cells are still what `--dev` means. `test_agent.ml` links the agent program both ways for this reason. @@ -467,8 +467,13 @@ still null. Not race-testable, so it is asserted on the emitted `flan_reload_ins - **`flan_dev_global` refuses a size change.** The running process has already laid that memory out; handing back the old allocation for a differently shaped type means the new body reads fields at the wrong offsets and nothing says so. This is the layout-drift rule's first enforcement point. Retyping a var needs a restart. -- **Nothing is ever `dlclose`d.** A cell holds an address inside a module's text; unloading it leaves every call site -pointing at unmapped memory. That is a constraint on the agent too. +- **Nothing that published anything is ever `dlclose`d.** A cell holds an address inside a module's text; unloading it +leaves every call site pointing at unmapped memory. That is a constraint on the agent too. The rule is about being +*pointed into*, which is why it has exactly two exceptions and they are not exceptions to the reasoning: a transient +thunk, which takes no registry slot and has returned; and a module the agent refuses before queueing it — no installer, +or no room in the ring — which published nothing and which nothing can name. What was leaked in the second case was the +handle *value* rather than the mapping: `dlopen` refcounts by path, so re-sending the same bad file raised a count +nothing could lower, and the one reference that could was dropped on the floor. - **The registry never moves.** A module holds a cell's address for as long as it is loaded, so the table is fixed capacity with a loud failure rather than growable. @@ -497,6 +502,19 @@ redefined function is on the stack, so it happens on the game thread, at the top The two are connected by a single-producer/single-consumer ring and two atomics; the game thread never blocks on the loader. +**A full ring is refused, at the sender, before the `dlopen`.** Of the three honest answers this is the only one that +reaches the person who asked: dropping loses a reload the sender was told was `ok`, which is the same lie more quietly, +and blocking stalls the accept loop — it serves connections inline, so a program that had stopped polling would also +stop answering `status` and `abort`, leaving the dev loop with no way to reach a program that had stopped listening to +it. The check is separate from the store because there is exactly one producer: room, once seen, cannot be taken away, +since the consumer only ever makes more of it. Sixty-four is a lot of reloads between two frames and the refusal says +what to do about it — call `agent/poll`. + +**The way out of the break loop is `_exit`, not `exit`.** `exit` runs the atexit chain and the ELF destructors, which +want the loader lock the listener thread may be holding inside `dlopen`; a program asked to abort would hang instead of +dying, which is the failure the break loop exists to replace. The streams are flushed by hand at each call site, and 134 +stays because that is what a trap exits with. + `wait` exists for tests. A test that races the frame rate fails on a loaded machine, so `test/programs/agent.flan` waits for the reload instead of sleeping past it. It takes **two** reloads, which is the daemon's actual loop: the first introduces a global the process was never built with, the second only reads it, and the second can only answer 1007 if @@ -763,8 +781,18 @@ decision's bill, and it is why the printer set is small rather than universal. It does not go through stdout. Stdout belongs to the program, it is in the hot path for anything that prints, and a dev-only feature must not put a branch in it — so `flan_rt.c` is untouched and the value is read back over the agent's -socket. The read is safe without a handshake because `flan_dev_result` bumps a generation counter last; the daemon waits -for it to move rather than assuming the program has reached a frame boundary. +socket. The read is safe without a handshake because the counter is a **seqlock**, and it had to be made into a real +one: the first version bumped the generation last and handed back the buffer itself, which says a new value has arrived +and says nothing about whether the bytes the agent then wrote to a socket were that value — the game thread is free to +be a hundred bytes into the next one by then. A seqlock cannot validate a read that finishes after it returns, so the +bare pointer was the bug rather than the ordering. `flan_dev_result_read` copies into the caller's buffer and checks +the counter either side of the copy; the counter is odd for exactly as long as a value is being written, and a reader +that loses the race reports the last *complete* generation and no bytes, so a daemon polling for a new value keeps +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 +reached a frame boundary. **This renderer is most of `println`**, which is worth knowing before anyone schedules it. plan.org describes a compiler-provided, type-directed intrinsic that selects or emits a structural printer per concrete instantiation, prints diff --git a/NEXT.md b/NEXT.md index 4f2b851..7a22e3a 100644 --- a/NEXT.md +++ b/NEXT.md @@ -82,11 +82,13 @@ it *would* have written. which is a compiler feature of the same shape the bounds checks already have, or they belong to the checker. Not decided. `test_sanitize` pins the current answer with a control that must *not* report, so a future clang changing this is a test failure rather than a discovery. -2. **Four named buffers got no evidence at all.** The 4K result cap, the dev registry overflow guard, - `SNAP_MAX`/`SNAP_NAMES` and `condition_name[128]` are on the daemon and agent paths, which need a socket and are not - in the corpus. Their guards were read and are correct; that is reading, not testing. `escaped[ESCAPE_MAX]` is the one - that *is* covered, because `println.flan` drives a 1100-character string through it on purpose — 1019 bytes out - against a worst case of 1021 into 1024. `scratch[SCRATCH]` never sees more than 20 characters of 64. +2. **Two of the four named buffers now have evidence; two still do not.** The 4K result cap and `condition_name[128]` + are driven over the agent's socket from `test_agent.ml` — a 5000-byte value comes back as 4096 ending in the + ellipsis, a 198-character condition class comes back from `status` as 127. The **dev registry overflow guard** and + `SNAP_MAX`/`SNAP_NAMES` are still read rather than tested: four thousand interned names and sixty-five nested + `restart-case`s are a lot of program for a clamp each. `escaped[ESCAPE_MAX]` is covered because `println.flan` + drives a 1100-character string through it on purpose — 1019 bytes out against a worst case of 1021 into 1024. + `scratch[SCRATCH]` never sees more than 20 characters of 64. 3. **Valgrind over the headless corpus, not done.** ASan does not see uninitialised reads, which is where `zeroed` and struct padding live. MSan is out: it needs every dependency instrumented and raylib settles that. @@ -383,7 +385,7 @@ plan.org's single line on it (831) names a `for` the language does not have and no evidence of what it meant. The agent copies the list on entering `break_loop` — names into its own buffer, frames as the addresses a transfer carries — one snapshot per nested break, and every verb answers from it. Caps are `SNAP_MAX` 64 restarts and `SNAP_NAMES` 4096 bytes; past either, the listing says how many it did not show. Neither - cap has a test, same blind spot as the 4K result cap below. + cap has a test; the 4K result cap that shared that blind spot now does. - **A snapshot generation has no test, and the window is a race.** A choice is validated against the snapshot on top when the request arrives and resolved against the snapshot on top when the game thread next looks. Between those, @@ -395,13 +397,25 @@ plan.org's single line on it (831) names a `for` the language does not have and window means landing a request inside a two-millisecond poll from outside the process. It wants a hook the test can drive, not a sleep. -- **The job ring has no fullness check**, and the comment describing its overflow is wrong. `publish` never consults - `tail`; past `QUEUE` entries it overwrites the slot the consumer is reading, and `job` is 24 non-atomic bytes. - Reachable from a program that goes a long time between `agent/poll` calls. -- **`flan_dev_result_get` is not the seqlock its comment claims** — it reads the generation first, then a non-atomic - length, then returns a bare pointer the caller sends later. -- Smaller: `exit(134)` from the break loop with the listener inside `dlopen`; a `dlopen` handle leaked when a module - has no installer. +- ~~The job ring has no fullness check.~~ **Fixed by refusing, at the sender.** Dropping loses a reload the sender was + told was ok; blocking stalls the accept loop, which serves connections inline, so a program that had stopped polling + would also stop answering `status` and `abort`. The refusal happens before the `dlopen`, so a module there is no room + for is never relocated and no handle is taken for it. `programs/agent-queue.flan` blocks on stdin so the window is + held open by the test rather than by a timer: 64 queued, the 65th refused with a reason, 64 installed when it finally + polls. + +- ~~`flan_dev_result_get` is not the seqlock its comment claims.~~ **Fixed by making it one**, rather than by writing + the honest comment — what it guaranteed was nothing, and the daemon has no other way to read a result. The counter is + odd while a value is being written, `flan_dev_result_read` copies into the caller's buffer and checks the counter + either side of the copy, and a reader that loses the race reports the last complete generation and no bytes. The + count handed out is the number of complete values, so `lib/dev.ml`'s "has it moved" still means what it meant. The + race itself has no test, for the same reason the snapshot generation above has none. + +- ~~Smaller: `exit(134)` from the break loop with the listener inside `dlopen`; a `dlopen` handle leaked when a module + has no installer.~~ **Both fixed.** `exit` runs the atexit chain and the ELF destructors, which want the loader lock + 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. - **`(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. @@ -414,7 +428,8 @@ Sixty mutations, nineteen left the whole suite green. The severe cluster is clos function a valid program calls, so the build fails to link. - `flan_dev_global`'s size-change guard — the layout-drift check, with no test that retypes a global across a reload. - A local shadowing an imported name is qualified anyway. -- The 4K result cap and the registry overflow guard have **no coverage at all**, rather than a missing assertion. +- The registry overflow guard has **no coverage at all**, rather than a missing assertion. The 4K result cap that used + to sit beside it here is driven over the agent's socket now. - The reader accepts an unknown string escape; `+5` stops being a number. - And a warning: a reader mutation makes the suite **hang** rather than fail. A green run is not the only outcome to plan for in CI. From e691512af3e78a37a1727b6a3f9d11b46b899569 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sat, 12 Sep 2026 10:53:09 +0700 Subject: [PATCH 5/5] 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;