From 8b487448641d284a59e2aea49898d2b4222501f6 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Fri, 18 Sep 2026 12:45:46 +0700 Subject: [PATCH] A render job carries the stop it was built under, and the agent will not run it without one --- docs/BUGS-2026-09-18.md | 21 ++++++- lib/dev.ml | 123 ++++++++++++++++++++++++++++++++------ test/test_agent.ml | 54 +++++++++++++++++ vendor/agent/flan_agent.c | 118 +++++++++++++++++++++++++++++++++++- 4 files changed, 294 insertions(+), 22 deletions(-) diff --git a/docs/BUGS-2026-09-18.md b/docs/BUGS-2026-09-18.md index e3780f5..54a9f5e 100644 --- a/docs/BUGS-2026-09-18.md +++ b/docs/BUGS-2026-09-18.md @@ -104,9 +104,24 @@ territory; fix or record, the lane's call. narrowed buffers. - **`flan-connect` + `flan-dev-quit` kill two sessions** (`flan-dev.el:721`): quit sends `close` down the current connection and kills the daemon it started for another program. -- **`reg at` TOCTOU** (`dev.ml:1488`): the stopped-only gate is checked three round trips - before the render thunk runs; a `restart` in between lets the thunk chase freed memory - with the gate's blessing. +- ~~**`reg at` TOCTOU**~~ — FIXED. The gate was checked three round trips before the + render thunk ran, and nothing held the break across the ~300ms build, so a `restart` + in between let the thunk chase freed memory with the gate's blessing. The sound fix is + agent-side: the render job now carries the condition it was built under. `inspect` by + address delivers its module as `stopped-only `, the job header keeps the flag, + and `flan_agent_poll` drops such a job — counted, handle closed, nothing installed and + nothing called — when `depth` is 0 at the moment it is claimed. That read is sound + rather than narrower: only the game thread polls and only the game thread raises + `depth`, so `depth > 0` seen inside a poll means *this* thread is parked in the break + loop and cannot be running a frame. The daemon reads the `refusals` count either side + of the delivery and surfaces the agent's own sentence — "the program resumed while this + inspection was being built — stop it again and re-ask" — instead of the timeout's + wrong-cause "is it calling (agent/poll)?". `reg at`'s depth gate stays as the front + door. `locals`, `globals` and `inspect` by slot are deliberately *not* stopped-only: + they hold no address, re-deriving the frame through `snap_top` or binding a global by + name at thunk-run time, so they carry no blessing that can expire. Pinned in + `test_agent.ml`, where a stopped-only job delivered to a program that is definitively + running is dropped while the eval module beside it installs — one install, not two. - ~~**defenum values never range-checked to i32**~~ — FIXED. Every resolved member value, explicit or autoincremented, is range-checked against i32 in the parser before the collision scan, so the scan compares the numbers the program will actually have. Out of range is refused diff --git a/lib/dev.ml b/lib/dev.ml index 0d3ec30..5825528 100644 --- a/lib/dev.ml +++ b/lib/dev.ml @@ -158,6 +158,39 @@ let request t line = in the new. *) let deliver t path = String.trim (request t path) +(* The same, for a module that may only run from a break. + + The word goes in front of the path rather than into the module, which is + what keeps this change out of both emitters and out of the [.so]'s ABI. The + agent's own note says why it belongs on the request: what is stopped-only is + the *question*, not the code. *) +let deliver_stopped_only t path = deliver t ("stopped-only " ^ path) + +(* How many stopped-only modules the program has thrown away for reaching the + game thread while it was running, and the sentence the agent says about it. + + Both come from the agent because the sentence lives there, once. Reading it + here and keeping a second copy of the words would be the same refusal in two + places, drifting apart the first time either is reworded — and this one is + the sentence somebody reads in the minibuffer when their inspection comes + back empty, so the wording is the whole of its value. + + [None] where the program cannot be reached or answers something else: a + count that could not be read is not a count that did not move, and the + caller treats it as "no evidence" rather than as zero. *) +let refusals t : (int * string) option = + match request t "refusals" with + | exception Unix.Unix_error _ -> None + | text -> + (match String.index_opt text '\n' with + | None -> None + | Some i -> + (match int_of_string_opt (String.trim (String.sub text 0 i)) with + | None -> None + | Some n -> + Some (n, String.trim (String.sub text (i + 1) + (String.length text - i - 1))))) + (* Read back the value of the last expression evaluated, with the counter that says whether it is a new one. The thunk runs on the game thread whenever the program next reaches a frame boundary, which is not a moment the compiler @@ -1135,15 +1168,58 @@ let backtrace_op t = nobody sees until the one path that lost it is the one being used. [tag] only names the [.so] on disk, which is what someone reads when they - go looking at [t.dir] to find out which verb produced what. *) -let run_render_thunk t ~tag ~(c : Session.change) : (string, string) result = + go looking at [t.dir] to find out which verb produced what. + + ── [stopped_only], and why only one of the three wants it ──────────── + + Everything between the gate and the thunk takes time the gate does not + cover. The verb checks that the program is stopped, the agent checks it + again, and then a module is *built* — a third of a second of llc and a + linker — and delivered, and waited on for up to five seconds. A [restart] + arriving anywhere in there resumes the game thread, and the thunk runs at + the next frame boundary instead of from the break. The wait below does not + even notice: it keeps waiting while liveness is [Live], and a resumed + program is the liveliest thing there is. + + What that costs depends entirely on what the thunk holds. + + [inspect] by *address* holds a number. [Dev.render_addr] bakes the address + into the module as an integer literal, because the registry's blessing — + "something live is there, and it is a [Foo]" — was given at build time by a + table that the running program is exactly the thing that changes. Run that + thunk after a resume and it dereferences an address whose blessing expired, + possibly into storage the program has since freed. So it is delivered + stopped-only, and the agent drops it rather than running it. + + [locals], [inspect] by *slot* and [globals] hold no address at all, and that + is the line. A local is reached through [flan/dev-slot], which is + [flan_agent_frame_slot], which asks [snap_top] for the frame *when the thunk + runs* — and [snap_top] is empty once the break that pushed it has been + resumed past. A global is reached by name: [Emit.redefinition] leaves it + [external], the dynamic linker binds it to the program's own storage, and + that storage has existed since the process started. Neither carries a + permission that can go stale between the asking and the running, because + neither was given one. Tagging them stopped-only would refuse work that is + sound, which is the other way to lose an answer. *) +let run_render_thunk ?(stopped_only = false) t ~tag ~(c : Session.change) + : (string, string) result = let before = match result t with Some (g, _) -> g | None -> 0L in + (* Read *before* the build, not before the wait: the resume this is watching + for can land while llc is still running, and the job it kills is this one. + [None] when the program cannot say, in which case nothing below compares + against it — a missing count is no evidence either way. *) + let refused_before = if stopped_only then refusals t else None in + let resumed () = + match (refused_before, if stopped_only then refusals t else None) with + | Some (before, _), Some (now, why) when now > before -> Some why + | _ -> None + in t.n <- t.n + 1; let out = Filename.concat t.dir (Printf.sprintf "%s%d.so" tag t.n) in match build_module c ~debug:t.session.Session.debug ~out with | exception Failure m -> Error m | _ -> - (match deliver t out with + (match (if stopped_only then deliver_stopped_only t out else deliver t out) with | exception Unix.Unix_error (e, _, _) -> Error ("cannot reach the program: " ^ Unix.error_message e) | "ok" -> @@ -1158,21 +1234,31 @@ let run_render_thunk t ~tag ~(c : Session.change) : (string, string) result = than anywhere. *) drain t; match result t with - | Some (g, v) when Int64.compare g before > 0 -> Some v - | _ when ms <= 0 -> None + | Some (g, v) when Int64.compare g before > 0 -> Ok v + (* Asked every tick and not once at the end, because the five seconds + are the point: a dropped job produces no value, so without this the + answer would be the timeout's sentence — "is it calling + (agent/poll)?" — which names the wrong cause and sends the reader + to look at a loop that was polling perfectly well. *) | _ -> - ignore (Unix.select [] [] [] 0.005); - (* [Live] for the reason [eval_expr]'s own wait gives: a thunk - needs a frame boundary, and neither a gone program nor a parked - one is going to reach one. *) - if liveness t = Live then wait (ms - 5) else None + match resumed () with + | Some why -> Error why + | None -> + let gave_up = + Error + "the program did not reach a frame boundary; is it calling \ + (agent/poll)?" + in + if ms <= 0 then gave_up + else begin + ignore (Unix.select [] [] [] 0.005); + (* [Live] for the reason [eval_expr]'s own wait gives: a thunk + needs a frame boundary, and neither a gone program nor a + parked one is going to reach one. *) + if liveness t = Live then wait (ms - 5) else gave_up + end in - (match wait 5000 with - | Some v -> Ok v - | None -> - Error - "the program did not reach a frame boundary; is it calling \ - (agent/poll)?") + wait 5000 | reply -> Error ("the program refused the module: " ^ reply)) (* The frame checks, which every verb that reads a *frame* has to make and @@ -1687,7 +1773,10 @@ let inspect_addr t ~addr ~want_type = | Error m -> error m | exception Failure m -> error m | Ok c -> - (match run_render_thunk t ~tag:"a" ~c with + (* Stopped-only, and the only one of the three that is. The + gate above was read three round trips and a build ago; + this is what holds it. See [run_render_thunk]. *) + (match run_render_thunk ~stopped_only:true t ~tag:"a" ~c with | Error m -> error m | Ok v -> ok diff --git a/test/test_agent.ml b/test/test_agent.ml index 1066a25..464b494 100644 --- a/test/test_agent.ml +++ b/test/test_agent.ml @@ -383,6 +383,32 @@ let () = 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 ()); + + (* ── A stopped-only job that reaches a running program ──────── + + The [reg at] TOCTOU, pinned at the one place it can be pinned + deterministically. In the daemon it is a race: [inspect] by + address checks that the program is stopped, spends a third of a + second building a thunk with that address baked in, and a + [restart] landing in the window resumes the game thread before + the thunk runs. Reproducing *that* means winning a race against + llc. What the fix actually turns on does not need the race at + all — a job tagged stopped-only, arriving at a frame boundary + with no break in force, must be dropped — and this program is + never stopped, so "no break in force" is not something to arrange. + + It is also the A/B, in one program at one moment, which is why it + sits beside the eval module rather than in a block of its own. + Two jobs are queued back to back: the same [tick] module the ring + above installed sixty-four times, now sent stopped-only, and the + eval module sent plainly. The program polls once and prints how + many it installed. One. Without the flag it is two, and the two + deliveries differ in nothing but the word in front of the path. *) + let refusals_before = send qsock "refusals" in + let s = send qsock ("stopped-only " ^ qso) in + if s <> "ok\n" then + fail "a stopped-only module was not queued: %S" s; + 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); @@ -412,6 +438,34 @@ let () = fail "the result is not the value that was rendered: %S" (head 8) end; + (* And the program says so, because a job that vanishes quietly is + the same lie the ring's dropped-oldest was. The count is what the + daemon compares either side of a delivery; the sentence is what it + puts in front of the person, and it lives here so that there is + one copy of it. *) + let refusals_after = send qsock "refusals" in + let count r = + match String.index_opt r '\n' with + | None -> None + | Some i -> int_of_string_opt (String.sub r 0 i) + in + (match (count refusals_before, count refusals_after) with + | Some b, Some a when a = b + 1 -> () + | _ -> + fail "a dropped stopped-only job was not counted: %S then %S" + refusals_before refusals_after); + let wanted = + "the program resumed while this inspection was being built — \ + stop it again and re-ask" + in + (match String.index_opt refusals_after '\n' with + | Some i + when String.trim + (String.sub refusals_after (i + 1) + (String.length refusals_after - i - 1)) + = wanted -> () + | _ -> + fail "the refusal does not say why: %S" refusals_after); (try Sys.remove eso with Sys_error _ -> ()) end; Unix.close wfd; diff --git a/vendor/agent/flan_agent.c b/vendor/agent/flan_agent.c index 4e61733..f9e0584 100644 --- a/vendor/agent/flan_agent.c +++ b/vendor/agent/flan_agent.c @@ -141,7 +141,66 @@ int flan_dev_reg_overflowed(void); * ran a thunk and left nothing behind. Everything else is kept mapped forever: * a cell holds an address inside a module's text, and unloading it would leave * every call site pointing at unmapped memory. */ -typedef struct { install_fn install; call_fn call; void *handle; } job; +/* [stopped_only] is the second half of a promise the *daemon* made and cannot + * keep on its own. A module built to render what is at a raw address has that + * address baked into it as an integer literal: the registry blessed the + * address as live at one moment, and the thunk dereferences it at another, + * three round trips and a ~300ms build later. Nothing in between held the + * break. A [restart] arriving mid-build resumes the game thread, and this ring + * would then install and run the thunk at the very next frame boundary — + * mid-frame, against an address the program may have freed since. + * + * So the job carries the condition it was built under, and [flan_agent_poll] + * re-asks it at the moment of truth instead of trusting the answer the sender + * got. The gate lives *here* and nowhere else on purpose: a second check on + * the listener thread, at delivery, would only shrink the window — [depth] can + * flip between that check and this one — and two places emitting the same + * refusal is how two places stop agreeing, which is the note the struct + * comment further down already makes about layouts. + * + * What makes the check in [flan_agent_poll] sound rather than narrower: only + * the game thread polls (agent.flan's [poll-raw] at a frame boundary, and the + * break loop below, which is the same thread parked), and only the game thread + * raises [depth]. So [depth > 0] read from inside a poll means *this* thread + * is inside the break loop right now — it cannot be running a frame at the + * same time — and the stop the daemon asked under is still the stop in force. + * A false negative is possible in one direction only and is the harmless one: + * a break entered after the read refuses a job that would have been legal, and + * refusing is right anyway, because that is a different stop from the one the + * registry answered under. + * + * [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: + * a cell holds an address inside a module's text, and unloading it would leave + * every call site pointing at unmapped memory. */ +typedef struct { + install_fn install; + call_fn call; + void *handle; + int stopped_only; +} job; + +/* Said once, in one place, and shipped to the daemon over [refusals] rather + * than written down again at the other end. A refusal is a sentence naming + * what actually happened, and the thing that actually happened is not "the + * gate failed" — it is that the program the person was inspecting started + * running again while the inspector was being compiled. */ +static const char *RESUMED = + "the program resumed while this inspection was being built — stop it again " + "and re-ask"; + +/* How many stopped-only jobs have been dropped, ever. A count and not a flag: + * the daemon reads it before it delivers and again while it waits, and what it + * wants to know is whether one happened *in between*, which a flag somebody + * else could have cleared cannot say. + * + * It is the whole of the channel because the sentence is fixed. One residual, + * left alone deliberately: two inspections in flight at once would let either + * one's refusal surface to the other's waiter. The result buffer they both + * write into has exactly that property already — flan_dev.c says so where the + * seqlock is — and fixing it here without fixing it there would be half a + * repair wearing the whole one's clothes. */ +static atomic_ullong refused_while_running; static job queue[QUEUE]; static atomic_uint head; /* written by the listener */ @@ -601,6 +660,24 @@ int32_t flan_agent_poll(void) { if (t == h) return n; job j = queue[t % QUEUE]; atomic_store_explicit(&tail, t + 1, memory_order_relaxed); + /* The gate the [job] comment argues for, asked at the only moment whose + * answer is worth anything. Claimed first and then dropped, rather than + * left in the ring for a later stop: the address it was built around was + * blessed by a stop that has already ended, and a job that waits for the + * *next* break would run against a blessing older still. + * + * The handle is closed for the reason the listener closes one it refuses + * for having no installer: a transient module nothing ever points into is + * unreachable the moment this returns, and dlopen refcounts by path, so + * dropping the handle on the floor here would leave a count that re-asking + * the same inspection bumps and nothing brings down. [n] is not bumped — + * nothing was installed, and [n] is what a caller polls to find out that + * something was. */ + if (j.stopped_only && atomic_load(&depth) <= 0) { + atomic_fetch_add(&refused_while_running, 1); + if (j.handle != NULL) { dlclose(j.handle); } + continue; + } if (j.install != NULL) { j.install(); n++; } /* After the install, so a thunk sees the bodies its own module published. * @@ -1028,6 +1105,24 @@ static void handle_line(char *line, sink *o) { free(nb); free(vb); return; } + /* How many stopped-only jobs the game thread has dropped for arriving at a + * frame boundary instead of at a break, and the sentence to say about it. + * Answered while running and while stopped alike — it is a count of things + * that have already happened, not a claim about the program's state now. + * + * The count leads and the text follows, because the daemon needs the count + * to decide and the text only to speak. It reads this before it delivers a + * stopped-only module and again while it waits for the value, and a number + * that moved in between is the answer. */ + if (strcmp(line, "refusals") == 0) { + char hdr[32]; + int k = snprintf(hdr, sizeof hdr, "%llu\n", + (unsigned long long)atomic_load(&refused_while_running)); + if (k > 0) emit(o, hdr, (size_t)k); + reply(o, RESUMED); + reply(o, "\n"); + return; + } if (strcmp(line, "result") == 0) { uint64_t gen = 0, len = 0; uint64_t cap = flan_dev_result_cap(); @@ -1212,6 +1307,23 @@ static void handle_line(char *line, sink *o) { free(counts); free(bytes); free(typelens); free(types); return; } + /* A module that may only run from a break, and the word for it goes in front + * of the path rather than inside the module. The alternative was a fourth + * exported symbol beside [flan_reload_transient], which would have put the + * decision in both emitters and in the .so's ABI — for a fact that is not + * about the module's *contents* at all. What is stopped-only is the + * *question*, not the code: the same rendering machinery, rooted at a frame + * slot instead of a raw address, is perfectly safe to run while the program + * runs. So it travels with the request. See [job]. + * + * Everything after the prefix is the ordinary path, gate included, so a + * stopped-only module with no installer is still refused for having no + * installer. */ + int stopped_only = 0; + if (strncmp(line, "stopped-only ", 13) == 0) { + stopped_only = 1; + line += 13; + } /* 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 one producer runs at a time, so room seen now is @@ -1261,7 +1373,9 @@ static void handle_line(char *line, sink *o) { * 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 })) + if (!publish((job){ .install = f, .call = c, + .handle = transient == NULL ? NULL : h, + .stopped_only = stopped_only })) fprintf(stderr, "flan: reload queue full after it was checked\n"); return; }