From 4a6a8fa0f7dc64d5aec8c8e65b42d782412defe3 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sat, 12 Sep 2026 04:56:18 +0700 Subject: [PATCH 1/4] Take a restart by its position, off a list that stopped moving MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two frames offering `retry` put both on the break loop's list and only the inner one within reach: §4's walk takes the first frame offering a name, by definition, so the outer clause was drawn, offered, and unreachable. The old prompt showed `retry` twice and sent the string either way. An index is the only thing that can say which one, which is why SBCL identifies them positionally too. An index is worthless against a stack that moves, though, and this one moves: the break loop is the poll loop, so every restart-case an evaluation enters pushes and pops the same global list between the listing and the choice. So the list is read once on entry and copied — names into the agent's own buffer, frames as the addresses a transfer carries — and every answer comes from that. The name still travels with the index as a receipt, checked against the snapshot and refused if the two have drifted, so a bare integer can be wrong out loud. And the third state. A restart below the thunk a break is inside was accepted, announced, and silently not taken: `flan_reload_call` holds its own transfer channel and drops it on return, so the unwind stops at the thunk. The boundary is now recorded where it is made, at the call — frames a restart-case inside the thunk pushes are above it and still work — and such a restart is listed, marked, and refused with the reason. `break.flan` grew the shadowed pair, and 900 is a value no by-name lookup in that file can produce. --- lib/dev.ml | 81 +++++++++++- lib/wire.ml | 10 ++ runtime/flan_rt.c | 23 ++++ test/programs/break.flan | 13 ++ test/test_agent.ml | 42 +++++- test/test_dev.ml | 67 ++++++++++ vendor/agent/flan_agent.c | 271 +++++++++++++++++++++++++++++++------- 7 files changed, 452 insertions(+), 55 deletions(-) diff --git a/lib/dev.ml b/lib/dev.ml index 0cb1dfc..8a83abd 100644 --- a/lib/dev.ml +++ b/lib/dev.ml @@ -195,18 +195,40 @@ let state t = (* Innermost first, terminated by a line that is a single dot — the agent's framing, not this one's. A refusal comes back as a line starting "err ", and is passed on rather than turned into an empty list: no restarts and cannot - say are different answers. *) + say are different answers. + + Each line is [I ± NAME]: the index it is taken by, whether it can be taken, + and the name. The index is the identity — two frames may offer [retry] and + a name cannot say which — and it is the program's number, not this end's + position in a list, so it is carried rather than recomputed. *) let restarts t = match ask t "restarts" with | text -> let lines = String.split_on_char '\n' text in if List.exists (fun l -> String.length l >= 3 && String.sub l 0 3 = "err") lines then Error (String.trim text) - else + else begin + let parse line = + match String.index_opt line ' ' with + | None -> None + | Some i -> + (match int_of_string_opt (String.sub line 0 i) with + | None -> None + | Some idx -> + let rest = String.sub line (i + 1) (String.length line - i - 1) in + if String.length rest < 2 then None + else + Some + ( idx, + rest.[0] = '+', + String.sub rest 2 (String.length rest - 2) )) + in Ok - (List.filter - (fun l -> l <> "" && l <> ".") - (List.map String.trim lines)) + (List.filter_map parse + (List.filter + (fun l -> l <> "" && l <> ".") + (List.map String.trim lines))) + end | exception Unix.Unix_error (e, _, _) -> Error (Unix.error_message e) let alive t = @@ -465,7 +487,21 @@ let break t = | Unreachable m -> error ("cannot ask the program whether it stopped: " ^ m) | Stopped _ -> (match restarts t with - | Ok names -> ok [ ":restarts " ^ Wire.strings names ] + | Ok rs -> + (* [:restarts] stays a list of names, positional and innermost first, + with duplicates kept — the position *is* the index, which is what + [restart-at] takes. [:unreachable] names the positions that are on + the list and cannot be chosen: a restart below the evaluation the + break is inside has nowhere for a transfer to land. They are shown + rather than filtered, because a client that quietly dropped them + would leave someone asking where their restart went. *) + ok + [ ":restarts " ^ Wire.strings (List.map (fun (_, _, n) -> n) rs); + ":unreachable " + ^ Wire.ints + (List.filter_map + (fun (i, ok, _) -> if ok then None else Some i) + rs) ] | Error m -> error ("the program refused to list its restarts: " ^ m)) (* A choice is validated by the *program*, on its listener thread, against a @@ -477,6 +513,30 @@ let break t = stopped thread next comes round its loop, which is microseconds away and still not now. An editor that read [ok] as "running again" would poll once, find it stopped, and re-open the prompt it had just answered. *) +let choose_at t ~index ~name = + if not (alive t) then error "the program exited; restart flan dev" + else if + match name with + | Some n -> String.exists (fun c -> Char.code c < 32 || Char.code c = 127) n + | None -> false + then error "a restart name cannot contain a control character" + else + let verb = + "restart-at " ^ string_of_int index + ^ match name with Some n -> " " ^ n | None -> "" + in + match ask t verb with + | reply when String.trim reply = "ok" -> + ok + [ ":index " ^ string_of_int index; + ":note " + ^ Wire.quote + "accepted; the program resumes at its next pass of the break loop" + ] + | reply -> error (String.trim reply) + | exception Unix.Unix_error (e, _, _) -> + error ("cannot reach the program: " ^ Unix.error_message e) + let choose t ~name = if not (alive t) then error "the program exited; restart flan dev" else if String.exists (fun c -> Char.code c < 32 || Char.code c = 127) name then @@ -836,6 +896,15 @@ let handle t req = (match Wire.string_field req "name" with | Some name -> choose t ~name | None -> error "restart needs :name") + (* By index, which is the one that can name a shadowed restart. [:name] is + optional and is not the lookup: it is checked against the name the program + has at that index and refused if they have drifted apart, so a client that + listed and then chose cannot take a different restart than the one it + showed. *) + | Some "restart-at" -> + (match Wire.int_field req "index" with + | Some index -> choose_at t ~index ~name:(Wire.string_field req "name") + | None -> error "restart-at needs :index") | Some "abort" -> abort t | Some "disassemble" -> (match Wire.string_field req "name" with diff --git a/lib/wire.ml b/lib/wire.ml index d2d4b23..0a1d0ad 100644 --- a/lib/wire.ml +++ b/lib/wire.ml @@ -89,6 +89,16 @@ let string_field form key = | Some { Form.v = Form.Str s; _ } -> Some s | _ -> None +(* A restart is chosen by index, so the protocol has to carry one. Kept as + narrow as [string_field]: a form that is not an integer is [None] and the + op says what it wanted, rather than this guessing at a string. *) +let int_field form key = + match field form key with + | Some { Form.v = Form.Int i; _ } -> Some (Int64.to_int i) + | _ -> None + +let ints ns = list (List.map string_of_int ns) + let parse src = match Reader.read_all ~file:"" src with | [ f ] -> f diff --git a/runtime/flan_rt.c b/runtime/flan_rt.c index cf932af..6de54a0 100644 --- a/runtime/flan_rt.c +++ b/runtime/flan_rt.c @@ -116,6 +116,29 @@ void *flan_find_restart(uint32_t name_id) { return NULL; } +/* The i'th frame itself, innermost first — the same walk as + * [flan_restart_name] and the other half of it. A break loop that offers + * someone a *list* has to be able to take the entry they picked, and §4's + * by-name lookup cannot express "the second retry": it takes the first frame + * offering the name, by definition, so a shadowed restart is on every list + * and reachable from none of them. Identifying a restart positionally is the + * only thing that fixes that, and it is why SBCL does the same. + * + * The address is the currency: a transfer carries the frame's address, so a + * caller that holds one from before is holding the same frame the walk found, + * whatever the walk finds today. */ +void *flan_restart_frame(int32_t i) { + for (flan_restart *r = restarts; r != NULL; r = r->prev) + if (i-- == 0) return r; + return NULL; +} + +/* Aim the transfer channel at a frame obtained earlier. The same store + * [flan_break_resume] makes and the same one an invoke-restart makes — this + * only spells it without a lookup, for a caller that did its looking up when + * the stack was worth reading. */ +void flan_restart_take(void *frame, void *xfer) { *(void **)xfer = frame; } + /* [T] and string are both ptr+len — see Emit.ll. */ typedef struct { const uint8_t *ptr; int64_t len; } flan_slice; diff --git a/test/programs/break.flan b/test/programs/break.flan index d56ca52..12bc656 100644 --- a/test/programs/break.flan +++ b/test/programs/break.flan @@ -14,8 +14,21 @@ (use-placeholder [] -1) (retry [] 7))) +;;; Two frames offering the same name, which §4 says resolves to the inner one +;;; and only ever the inner one. The outer clause is therefore on every list a +;;; break loop prints and reachable from no name at all, which is why a restart +;;; is taken by *index* now. 900 is the proof: it is the only value in this +;;; file that by-name lookup cannot produce. +(defn shadowed [n i32] i32 + (restart-case + (+ (restart-case (do (error (Missing {:id n})) 0) + (retry [] 5)) + 100) + (retry [] 900))) + (defn main [] i32 (agent/start "/tmp/flan-break.sock") (print-i64 (i64 (fetch 1))) (newline) (print-i64 (i64 (fetch 2))) (newline) + (print-i64 (i64 (shadowed 3))) (newline) 0) diff --git a/test/test_agent.ml b/test/test_agent.ml index 4f16c62..9627b1b 100644 --- a/test/test_agent.ml +++ b/test/test_agent.ml @@ -200,9 +200,9 @@ let () = then fail "the program never reached the break loop: %S" !listed else begin (* Innermost first, and both on offer. *) - if !listed <> "retry\nuse-placeholder\n.\n" then + if !listed <> "0 + retry\n1 + use-placeholder\n.\n" then fail "restarts on offer\n got: %S\n wanted: %S" !listed - "retry\nuse-placeholder\n.\n"; + "0 + retry\n1 + use-placeholder\n.\n"; (* A name nothing offers is refused *here*, before the reply. Answering ok and discovering it on the game thread would report success for something that cannot happen. *) @@ -223,9 +223,41 @@ let () = if not (await printed) then fail "the first restart never produced its value" else if not (await (fun () -> send bsock "restarts" - = "retry\nuse-placeholder\n.\n")) + = "0 + retry\n1 + use-placeholder\n.\n")) then fail "the program never stopped a second time" - else ignore (send bsock "restart use-placeholder") + else begin + ignore (send bsock "restart use-placeholder"); + (* -- Taking a restart by index ------------------------------- *) + + (* The third break is inside a [restart-case] whose name the frame + below it also offers. Both are listed; §4's by-name walk can only + ever reach the first. So this takes the *second*, and 900 is a + value nothing else in the program can produce — the assertion on + the output at the end is what makes this test about shadowing + rather than about a reply. *) + let printed2 () = + let t = In_channel.with_open_bin bout In_channel.input_all in + List.exists (String.equal "-1") (String.split_on_char '\n' t) + in + if not (await printed2) then + fail "the second restart never produced its value" + else if not (await (fun () -> send bsock "restarts" + = "0 + retry\n1 + retry\n.\n")) + then fail "the program never stopped on the shadowed pair" + else begin + (* Out of range is refused before the reply, like a bad name. *) + let oob = send bsock "restart-at 7" in + if not (String.length oob >= 3 && String.sub oob 0 3 = "err") then + fail "an index nothing offers was accepted: %S" oob; + (* The name rides along as a receipt, not as the lookup: an index + whose name has moved is refused rather than silently taken, + which is the same failure by-name lookup had. *) + let drift = send bsock "restart-at 1 use-placeholder" in + if not (String.length drift >= 3 && String.sub drift 0 3 = "err") + then fail "an index whose name had drifted was accepted: %S" drift; + ignore (send bsock "restart-at 1 retry") + end + end end end; let bstatus = ref (Unix.WEXITED 0) in @@ -241,7 +273,7 @@ let () = end else begin let text = In_channel.with_open_bin bout In_channel.input_all in - let want = "7\n-1\n" in + let want = "7\n-1\n900\n" in let got = String.concat "\n" (List.filter diff --git a/test/test_dev.ml b/test/test_dev.ml index 911e835..d87df25 100644 --- a/test/test_dev.ml +++ b/test/test_dev.ml @@ -296,6 +296,73 @@ let () = fail "installing while stopped: %s" (Option.value ~default:"" (Wire.string_field r "message")); + (* -- A break inside a thunk, and the frames it cannot reach ---- *) + + (* Evaluating here runs a thunk through [flan_reload_call], which holds + its own transfer channel and drops it on return. So a restart below + that C frame — the two this program was already stopped on — has + nowhere for a transfer to land: it would unwind to the thunk, stop, + and the program would carry on as though nothing had been chosen. + It used to be accepted and announced and silently not taken, which + is the worst of the three things that could happen. + + Now the list says so and the choice is refused with the reason. *) + let r = + ask "(:op \"eval-expr\" :code \"(i64 (fetch 9))\" :file \"/tmp/buf.flan\")" + in + if status r <> "error" then + fail "an expression that stopped inside a break answered anyway"; + let r = ask "(:op \"break\")" in + if status r <> "ok" then fail "break inside a thunk: %s" (status r); + (* Four now: the thunk's own [fetch] frame over the one below it. *) + let names = + match Wire.field r "restarts" with + | Some { Form.v = Form.List l; _ } -> + List.filter_map + (fun (n : Form.t) -> + match n.Form.v with Form.Str x -> Some x | _ -> None) + l + | _ -> [] + in + if names <> [ "retry"; "use-placeholder"; "retry"; "use-placeholder" ] + then fail "restarts at a break inside a thunk: %s" + (String.concat ", " names); + let unreachable = + match Wire.field r "unreachable" with + | Some { Form.v = Form.List l; _ } -> + List.filter_map + (fun (n : Form.t) -> + match n.Form.v with + | Form.Int i -> Some (Int64.to_int i) + | _ -> None) + l + | _ -> [] + in + if unreachable <> [ 2; 3 ] then + fail "positions below the thunk: %s" + (String.concat ", " (List.map string_of_int unreachable)); + (* Refused, and refused *here* — not accepted and dropped. *) + let r = ask "(:op \"restart-at\" :index 2 :name \"retry\")" in + if status r <> "error" then + fail "a restart below the thunk boundary was accepted"; + (* The ones above it still work, so this refuses a case rather than + disabling the feature. *) + let r = ask "(:op \"restart-at\" :index 0 :name \"retry\")" in + if status r <> "ok" then + fail "a restart inside the thunk was refused: %s" + (Option.value ~default:"" (Wire.string_field r "message")); + (* And the program is back on the *outer* break, which is the one it + was on before any of this — the inner resume must not have been + read as the outer one resuming. *) + if not + (await (fun () -> + let r = ask "(:op \"break\")" in + status r = "ok" + && (match Wire.field r "restarts" with + | Some { Form.v = Form.List l; _ } -> List.length l = 2 + | _ -> false))) + then fail "the outer break did not come back after the inner one"; + (* A name nothing offers is refused against the live stack, on the program's listener thread, before the reply. *) let r = ask "(:op \"restart\" :name \"nonesuch\")" in diff --git a/vendor/agent/flan_agent.c b/vendor/agent/flan_agent.c index ad749d2..71fad9c 100644 --- a/vendor/agent/flan_agent.c +++ b/vendor/agent/flan_agent.c @@ -103,6 +103,27 @@ extern int32_t flan_break_resume(const uint8_t *name, int64_t namelen, void *xfer); extern int32_t flan_restart_count(void); extern const uint8_t *flan_restart_name(int32_t i, int64_t *len); +extern void *flan_restart_frame(int32_t i); +extern void flan_restart_take(void *frame, void *xfer); + +/* -- How far down a transfer can actually land ----------------------- */ + +/* A thunk run from this loop is called through [flan_reload_call], which + * allocates its *own* transfer channel and drops it on return. So a restart + * chosen at a break inside a thunk unwinds as far as the thunk and no + * further: every frame below that C boundary is on the list, is accepted, and + * is silently not taken. Three statements that the program will resume, none + * of them true - which is worse than either resuming or refusing. + * + * The boundary is recorded where it is *made*, at the call, not at the break: + * the frames below it are exactly the ones already on the stack when the + * thunk started, and a restart-case the thunk enters itself is above it and + * reachable. Recording the depth on entering the break loop instead would + * count those too and refuse restarts that work. + * + * Game thread only, saved and restored around the call, so a thunk that + * breaks and whose break runs another thunk nests correctly. */ +static int32_t restart_floor; /* What the listener thread hands the stopped game thread. One slot, because * only one thread is ever stopped. */ @@ -116,9 +137,83 @@ static _Atomic int depth; #define BREAK_MAX 8 /* deep enough to nest, shallow * enough that a loop of breaks * stops rather than grinds */ -static char chosen[128]; +static _Atomic int chosen_index; static _Atomic int chosen_ready; static _Atomic int aborting; + +/* -- The snapshot ---------------------------------------------------- */ + +/* The stopped thread is not holding still. This loop runs [flan_agent_poll], + * which runs arbitrary Flan, and every restart-case that runs pushes and pops + * the one global restart list. Serving names straight off that list hands the + * listener a pointer into a frame that may already have been popped; serving + * *indices* off it is worse still, because an index carries no evidence of + * what it meant - the list and the choice can disagree and nothing can tell. + * + * So the list is read once, on entry, while the thread that owns it is in + * this function and not in Flan, and copied: names into a buffer of our own, + * frames as the addresses a transfer carries. Everything the listener answers + * comes from here and nothing re-reads the live stack. */ +#define SNAP_MAX 64 /* restarts offered at one break */ +#define SNAP_NAMES 4096 /* bytes of names behind them */ + +typedef struct { + int32_t n; + int32_t total; /* before SNAP_MAX truncated it */ + void *frame[SNAP_MAX]; + int32_t off[SNAP_MAX], len[SNAP_MAX]; + int32_t reachable[SNAP_MAX]; + int32_t used; + char names[SNAP_NAMES]; +} snapshot; + +/* One per nested break loop, because an inner break must not answer with the + * outer one's restarts and must not destroy them either - the outer loop is + * still going to need them when the inner one resumes. Same reason [depth] is + * a count and not a flag. */ +static snapshot snaps[BREAK_MAX]; +static _Atomic int snap_depth; /* published last; 0 = none */ + +static snapshot *snap_top(void) { + int d = atomic_load(&snap_depth); + return d <= 0 ? NULL : &snaps[d - 1]; +} + +/* Called on the game thread with the stack held still. 0 if there is no room + * to nest, which the caller reports rather than serving a stale one. */ +static int snap_push(void) { + int d = atomic_load(&snap_depth); + if (d >= BREAK_MAX) return 0; + snapshot *s = &snaps[d]; + int32_t n = flan_restart_count(); + s->total = n; + s->used = 0; + s->n = 0; + for (int32_t i = 0; i < n && s->n < SNAP_MAX; i++) { + int64_t len = 0; + const uint8_t *nm = flan_restart_name(i, &len); + void *fr = flan_restart_frame(i); + if (nm == NULL || fr == NULL) continue; + if (len < 0) len = 0; + if ((int64_t)s->used + len + 1 > SNAP_NAMES) break; + s->frame[s->n] = fr; + s->off[s->n] = s->used; + s->len[s->n] = (int32_t)len; + /* The outermost [restart_floor] frames are below the thunk boundary. */ + s->reachable[s->n] = (i < n - restart_floor); + memcpy(s->names + s->used, nm, (size_t)len); + s->used += (int32_t)len; + s->names[s->used++] = 0; + s->n++; + } + atomic_store(&snap_depth, d + 1); + return 1; +} + +static void snap_pop(void) { + int d = atomic_load(&snap_depth); + if (d > 0) atomic_store(&snap_depth, d - 1); +} /* The condition's class name, so an editor can say what stopped rather than * only that something did. It is all there is to say: the hook is handed the * name and an opaque pointer, and nothing at run time can render a value whose @@ -135,16 +230,30 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition, fflush(stdout); fprintf(stderr, "\nflan: unhandled %.*s — stopped, not dead.\n", (int)namelen, (const char *)name); + /* Taken before anything is printed, and before [depth] says there is a + * break to ask about: the list on the terminal and the list on the socket + * are then the same list, numbered the same way, and the numbers are what a + * choice is made of. */ + if (!snap_push()) { + fflush(stdout); + fprintf(stderr, "flan: %d nested break loops - giving up rather than " + "spinning\n", BREAK_MAX); + fflush(stderr); + exit(134); + } { - int32_t n = flan_restart_count(); - if (n == 0) + snapshot *s = snap_top(); + if (s->n == 0) fprintf(stderr, " no restarts are active; abort, or fix and reload\n"); - for (int32_t i = 0; i < n; i++) { - int64_t len = 0; - const uint8_t *nm = flan_restart_name(i, &len); - if (nm != NULL) - fprintf(stderr, " restart: %.*s\n", (int)len, (const char *)nm); - } + for (int32_t i = 0; i < s->n; i++) + /* Numbered, because that is how one is taken now, and marked when it is + * not takeable - a restart below the thunk boundary is shown rather + * than hidden, since "why can I not have that one" is a fair question + * and silence is how this went wrong the first time. */ + fprintf(stderr, " %2d. restart: %s%s\n", i, s->names + s->off[i], + s->reachable[i] ? "" : " (below this break; cannot be taken)"); + if (s->total > s->n) + fprintf(stderr, " ... and %d more, not listed\n", s->total - s->n); } fflush(stderr); /* What the *outer* loop was reporting, restored on the way out: resuming an @@ -183,13 +292,14 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition, * arriving during the attempt passes its own check, writes a new name * and sets the flag, and the store below then erases it. Copying also * keeps strlen off a buffer the listener may be writing. */ - char take[sizeof chosen]; - memcpy(take, chosen, sizeof take); + int take = atomic_load(&chosen_index); atomic_store(&chosen_ready, 0); - int32_t ok = - flan_break_resume((const uint8_t *)take, (int64_t)strlen(take), xfer); + snapshot *s = snap_top(); + int ok = s != NULL && take >= 0 && take < s->n && s->reachable[take]; if (ok) { - fprintf(stderr, "flan: resuming at restart %s\n", take); + flan_restart_take(s->frame[take], xfer); + fprintf(stderr, "flan: resuming at restart %d. %s\n", take, + s->names + s->off[take]); fflush(stderr); memcpy(condition_name, outer_name, sizeof condition_name); /* Cleared with the resume: an abort that passed its check just as the @@ -198,9 +308,13 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition, * giving nobody the chance to choose. */ atomic_store(&aborting, 0); atomic_fetch_sub(&depth, 1); + snap_pop(); return; } - fprintf(stderr, "flan: no restart named %s is active\n", take); + /* The listener checks all of this before answering ok, so reaching here + * means the two disagreed - worth saying loudly rather than looping on + * in silence, which is the failure this whole change is about. */ + fprintf(stderr, "flan: restart %d is not one this break can take\n", take); fflush(stderr); } nanosleep(&step, NULL); @@ -224,8 +338,19 @@ int32_t flan_agent_poll(void) { job j = queue[t % QUEUE]; atomic_store_explicit(&tail, t + 1, memory_order_relaxed); if (j.install != NULL) { j.install(); n++; } - /* After the install, so a thunk sees the bodies its own module published. */ - if (j.call != NULL) { j.call(); } + /* After the install, so a thunk sees the bodies its own module published. + * + * The floor moves for the duration. [flan_reload_call] holds its own + * transfer channel and drops it on return, so every restart frame that + * was on the stack before this call is unreachable from a break inside + * it. Saved and restored rather than set and cleared: this runs from + * inside break loops, which run from inside thunks. */ + if (j.call != NULL) { + int32_t outer = restart_floor; + restart_floor = flan_restart_count(); + j.call(); + restart_floor = outer; + } if (j.handle != NULL) { dlclose(j.handle); } } } @@ -307,43 +432,101 @@ static void serve(int fd) { reply(fd, "running\n"); return; } + /* One line per restart, innermost first: the index it is taken by, a flag + * for whether it can be taken at all, and the name. The index leads + * because it is the identity - two frames can offer [retry] and only one + * of them is the one meant, which is the whole reason this is not a list + * of names any more. Read from the snapshot, never from the live stack. */ if (strcmp(line, "restarts") == 0) { if (!(atomic_load(&depth) > 0)) { reply(fd, "err not stopped\n"); return; } - int32_t n = flan_restart_count(); - for (int32_t i = 0; i < n; i++) { - int64_t len = 0; - const uint8_t *nm = flan_restart_name(i, &len); - if (nm != NULL) { - send(fd, nm, (size_t)len, MSG_NOSIGNAL); - reply(fd, "\n"); - } + snapshot *s = snap_top(); + if (s == NULL) { reply(fd, "err no restart snapshot\n"); return; } + for (int32_t i = 0; i < s->n; i++) { + char hdr[32]; + int k = snprintf(hdr, sizeof hdr, "%d %c ", i, + s->reachable[i] ? '+' : '-'); + if (k > 0) send(fd, hdr, (size_t)k, MSG_NOSIGNAL); + send(fd, s->names + s->off[i], (size_t)s->len[i], MSG_NOSIGNAL); + reply(fd, "\n"); } reply(fd, ".\n"); return; } - if (strncmp(line, "restart ", 8) == 0) { + /* Take the i'th, optionally checking that the caller and this snapshot + * still agree on what the i'th is called. The name is not the lookup - + * that is the bug - it is a receipt: a client that listed, prompted, and + * chose has the name in hand for nothing, and sending it turns a bare + * integer into something that can be wrong out loud. */ + if (strncmp(line, "restart-at ", 11) == 0) { if (!(atomic_load(&depth) > 0)) { reply(fd, "err not stopped\n"); return; } - size_t k = strlen(line + 8); - if (k == 0 || k >= sizeof chosen) { reply(fd, "err bad restart name\n"); return; } - /* Checked here, against the stack the stopped thread is holding still, - * rather than accepted and found wrong after the reply has gone. "ok" - * has to mean the program will resume. */ - { - int32_t n = flan_restart_count(), found = 0; - for (int32_t i = 0; i < n && !found; i++) { - int64_t len = 0; - const uint8_t *nm = flan_restart_name(i, &len); - found = nm != NULL && (size_t)len == k && memcmp(nm, line + 8, k) == 0; - } - if (!found) { - reply(fd, "err no restart named "); - reply(fd, line + 8); - reply(fd, " is active\n"); + snapshot *s = snap_top(); + if (s == NULL) { reply(fd, "err no restart snapshot\n"); return; } + char *end = NULL; + long idx = strtol(line + 11, &end, 10); + if (end == line + 11) { reply(fd, "err restart-at wants an index\n"); return; } + if (idx < 0 || idx >= s->n) { + reply(fd, "err no restart at that index\n"); + return; + } + while (*end == ' ') end++; + if (*end != '\0') { + size_t k = strlen(end); + if (k != (size_t)s->len[idx] || + memcmp(end, s->names + s->off[idx], k) != 0) { + reply(fd, "err that index is now "); + reply(fd, s->names + s->off[idx]); + reply(fd, ", not what you named; list the restarts again\n"); return; } } - memcpy(chosen, line + 8, k + 1); - /* Published last, so the game thread never reads a half-written name. */ + if (!s->reachable[idx]) { + /* Refused, with the reason, rather than accepted and dropped. The + * transfer would unwind to the thunk this break is inside and stop + * there, and the program would carry on as if nothing had been + * chosen. */ + reply(fd, "err restart "); + reply(fd, s->names + s->off[idx]); + reply(fd, " is below the evaluation this break is inside, so a " + "transfer to it has nowhere to land; choose one offered " + "above it, or abort\n"); + return; + } + atomic_store(&chosen_index, (int)idx); + /* Published last, so the game thread never reads an index that is about + * to change. */ + atomic_store(&chosen_ready, 1); + reply(fd, "ok\n"); + return; + } + /* By name, still, for a person at a raw socket - and now defined as + * exactly [restart-at] on the first index offering the name, which is + * what §4's walk already meant. So the two verbs cannot disagree, and a + * shadowed name is reachable through the other one rather than nowhere. */ + if (strncmp(line, "restart ", 8) == 0) { + if (!(atomic_load(&depth) > 0)) { reply(fd, "err not stopped\n"); return; } + snapshot *s = snap_top(); + if (s == NULL) { reply(fd, "err no restart snapshot\n"); return; } + size_t k = strlen(line + 8); + if (k == 0) { reply(fd, "err bad restart name\n"); return; } + int32_t at = -1; + for (int32_t i = 0; i < s->n && at < 0; i++) + if ((size_t)s->len[i] == k && memcmp(s->names + s->off[i], line + 8, k) == 0) + at = i; + if (at < 0) { + reply(fd, "err no restart named "); + reply(fd, line + 8); + reply(fd, " is active\n"); + return; + } + if (!s->reachable[at]) { + reply(fd, "err restart "); + reply(fd, line + 8); + reply(fd, " is below the evaluation this break is inside, so a " + "transfer to it has nowhere to land; choose one offered " + "above it, or abort\n"); + return; + } + atomic_store(&chosen_index, at); atomic_store(&chosen_ready, 1); reply(fd, "ok\n"); return; From ac7d4a0e95382ce8ade94443292174e7048ba2a6 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sat, 12 Sep 2026 04:56:25 +0700 Subject: [PATCH 2/4] The prompt numbers its choices, because a name could not say which now lists the restarts by position and sends the position, with the name alongside as the receipt the program checks. A restart below the evaluation the break is inside is shown marked rather than hidden: someone who can see a restart in their own source and not on this list has been told nothing, and the refusal carries the reason. --- emacs/flan-dev.el | 65 ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 62 insertions(+), 3 deletions(-) diff --git a/emacs/flan-dev.el b/emacs/flan-dev.el index fc612b6..68db5ab 100644 --- a/emacs/flan-dev.el +++ b/emacs/flan-dev.el @@ -609,12 +609,51 @@ every bit as connected and nothing like running." ;; first. `completing-read' over them is the natural shape: they are a closed ;; set the program computed, so require-match is exactly right. ;; +;; What is chosen is a *position* on that list, not a name. §4 says lookup +;; takes the first frame offering a name, so when two frames offer `retry' the +;; outer one is real, is on this list, and by name is unreachable — the old +;; prompt showed `retry' twice and sent the string either way, and the inner +;; frame took it silently. A position cannot be ambiguous, which is why SBCL +;; identifies restarts positionally too. So the candidates are numbered and +;; the number is what goes on the wire. +;; ;; `abort' is on the same list rather than on a separate key, because it is the ;; same decision: it is what you pick when none of the restarts is the answer. ;; It is last, and it is not the default. +(defun flan-dev--restart-candidates (restarts unreachable) + "Label each of RESTARTS by its position, marking those in UNREACHABLE. +An alist of label to index. The index leads the label because it is the +identity: two entries may read the same and mean different frames." + (let ((i -1)) + (mapcar (lambda (name) + (setq i (1+ i)) + (cons (format "%d. %s%s" i name + (if (memq i unreachable) + " (below this break; cannot be taken)" + "")) + i)) + restarts))) + +(defun flan-dev-restart-at (index name) + "Resume the stopped program at the restart at position INDEX. +NAME is sent with it and is not the lookup: the program checks it against +the name it holds at that position and refuses if the two have drifted +apart, so a prompt cannot take a different restart than the one it showed." + (let ((r (flan-dev--request (list :op "restart-at" :index index :name name)))) + (if (equal (plist-get r :status) "ok") + (progn + ;; Accepted, not resumed — see `flan-dev-restart'. + (setq flan-dev--stopped nil) + (force-mode-line-update t) + (message "flan: %s — %s" name (or (plist-get r :note) "accepted"))) + (user-error "flan: %s" (or (plist-get r :message) "refused"))))) + (defun flan-dev-restart (name) - "Resume the stopped program at the restart called NAME." + "Resume the stopped program at the restart called NAME. +The first frame offering NAME, which is §4's own rule and therefore cannot +reach a shadowed one. `flan-break' chooses by position instead; this is +here for a name known in advance." (interactive (list (completing-read "Restart: " (flan-dev-restarts) nil t))) (let ((r (flan-dev--request (list :op "restart" :name name)))) (if (equal (plist-get r :status) "ok") @@ -651,6 +690,17 @@ nothing left to serve once it has gone." (user-error "flan: %s" (or (plist-get r :message) "refused"))) (plist-get r :restarts))) +(defun flan-dev-unreachable-restarts () + "Positions on the restart list that cannot be chosen. +A restart below the evaluation a break is inside has nowhere for a transfer +to land — the thunk holds its own channel and drops it on return. They are +listed and marked rather than hidden, because someone who can see a restart +in their own source and not on this list has been told nothing." + (let ((r (flan-dev--request '(:op "break")))) + (unless (equal (plist-get r :status) "ok") + (user-error "flan: %s" (or (plist-get r :message) "refused"))) + (append (plist-get r :unreachable) nil))) + ;;;###autoload (defun flan-break () "Show what the stopped program is offering, and choose one. @@ -664,12 +714,21 @@ than being told so." (unless flan-dev--stopped (user-error "flan: the program is running; nothing is stopped")) (let* ((restarts (plist-get r :restarts)) + (unreachable (append (plist-get r :unreachable) nil)) + (table (flan-dev--restart-candidates restarts unreachable)) (choice (completing-read (format "flan: stopped on %s%s — " flan-dev--stopped (if restarts "" " (no restarts are active)")) - (append restarts '("abort")) nil t))) - (if (equal choice "abort") (flan-dev-abort) (flan-dev-restart choice))))) + (append (mapcar #'car table) '("abort")) nil t)) + (index (cdr (assoc choice table)))) + (cond + ((equal choice "abort") (flan-dev-abort)) + ;; `require-match' over a table this built, so a choice outside it is + ;; not something a person can type — but deriving the table wrongly + ;; should say so rather than put nil on the wire as an index. + ((null index) (user-error "flan: %s is not on the list" choice)) + (t (flan-dev-restart-at index (nth index restarts))))))) ;;;###autoload (defun flan-show-output () From 91d1368279ad6bad3074ae1951466d4564b443a5 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sat, 12 Sep 2026 04:58:42 +0700 Subject: [PATCH 3/4] Say in the specs what the break loop actually does now MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit spec-conditions.md §4 gains the rule the shadowing bug was hiding: a handler matches by name, a debugger identifies by position, and the two are not the same question. With it, the snapshot — a position means nothing against a stack that moves — and the fact that a visible restart may still be unreachable, which §6's explicit lowering makes possible. §3 records the open one: a clause should carry a report string. is what invoke-restart needs and not what a person reading a list needs. It wants settling before restarts with parameters, which is where a bare name is least sufficient. conditions.org had the break loop under "Not yet", which it has not been for some time, and now says why find-restart and compute-restarts still are: they are blocked on a Restart type and a list to return one in, not on effort. --- NEXT.md | 80 +++++++++++++++++++++++++++++----------------- conditions.org | 22 ++++++++++++- spec-conditions.md | 29 +++++++++++++++++ 3 files changed, 100 insertions(+), 31 deletions(-) diff --git a/NEXT.md b/NEXT.md index f6ce6b0..9917ea5 100644 --- a/NEXT.md +++ b/NEXT.md @@ -11,10 +11,14 @@ running process. Conditions are two steps in of four. it, so only a transfer gets past; with nothing transferring the program stops and names the condition. `flan_error` is where the break loop goes. -**The break loop is in.** An unhandled `error` now stops the program on the frame that erred, lists the restarts on -offer, and waits; a choice sent to the agent's socket resumes it. See "The break loop" below. What is left of §2 is the -*editor* half — a `break` notification on the daemon protocol and a minibuffer prompt — which is what makes it something -you use rather than something you drive with a socket. +**The break loop is in, editor half included.** An unhandled `error` stops the program on the frame that erred, the +daemon annotates every reply with `:stopped`/`:condition`, and `C-c C-b` in Emacs lists the restarts and resumes into +the choice. See "The break loop" and "The break loop in the editor" below — both describe what is built. + +**A restart is chosen by position now, not by name**, which is the fix for the shadowing bug and the reason `:restarts` +is a positional list with a `:unreachable` set beside it. Restarts below the evaluation a break is inside are listed, +marked, and refused with the reason; the list itself is a snapshot taken when the break was entered, because the +stopped thread's stack does not hold still. `spec-conditions.md` §4 now says all of this. **The old next task, now done, was the dev-build break loop, `spec-conditions.md` §2** — where an unhandled `error` stops and talks to the daemon instead of `rt_die()`, and where **"a crash kills the program"** finally gets fixed. The @@ -477,25 +481,31 @@ vague intention — if it is listed, someone has already established it is real. ### Bugs found and not yet fixed -- **A shadowed restart is offered and cannot be taken.** §4 says lookup takes the *first* frame offering a name, and - `flan_find_restart` does exactly that — so when two frames offer `retry`, the second one is real, is on the list the - break loop prints, and is unreachable. The old `C-c C-b` prompt showed `retry` twice and sent the string either way; - the inner frame took it, silently. `test/programs/restarts.flan`'s own `nested` function is the counterexample, and - it has been there since the transfer landed. The C&R buffer now draws the shadowed row unbracketed and refuses - `RET` on it by name, which stops the lie but does not restore the choice. The fix is to take a restart **by index** - rather than by name — SBCL identifies them positionally for precisely this reason: - - `(:op "restart-at" :index N)` on the daemon, and a matching agent verb. `flan_restart_name(i, &len)` already - walks the stack by index, so resuming by index is that loop plus `flan_break_resume`'s tail. C only, no compiler - work. +- ~~A shadowed restart is offered and cannot be taken.~~ **Fixed.** A restart is taken by *position* now: + `(:op "restart-at" :index N :name NAME)` on the daemon, `restart-at N NAME` on the agent, and a numbered + `completing-read` in `C-c C-b`. `:name` is a receipt, not the lookup — it is checked against the name the snapshot + holds at that index and refused if the two have drifted, so a bare integer can be wrong out loud. `restart ` + survives for a raw socket and is now defined as `restart-at` on the first index offering the name, so the two verbs + cannot disagree. `break.flan` grew the shadowed pair and asserts 900, which is the only value in that file no by-name + lookup can produce. The C&R buffer still marks the shadowed row by name and could now offer it instead — small, and + not done here. + +- ~~A restart chosen at a break inside a thunk is accepted, announced, and silently not taken.~~ **Fixed by refusing + it, with the reason.** Not by the depth NEXT.md proposed: recording the restart-stack depth on *entering the break + loop* counts the frames a `restart-case` inside the thunk pushed before it erred, and those are above the boundary + and work. The boundary is where it is made — `restart_floor` is set to `flan_restart_count()` around `j.call()` in + `flan_agent_poll`, saved and restored so thunks nest — and the outermost `floor` entries of the snapshot are marked + unreachable. They are listed and marked rather than hidden, refused by the listener before the reply, and carried to + the editor as `:unreachable (2 3)`. `test_dev.ml` breaks a stopped program a second time from inside `C-x C-e` and + asserts both halves: index 2 refused, index 0 taken. + +- ~~Restart names are served from a stack that is being mutated.~~ **Fixed, and it was a precondition rather than a + separate bug.** Index-based resume is wrong by construction against a moving stack: unlike a name, an index carries + 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. -- **A restart chosen at a break inside a thunk is accepted, announced, and silently not taken.** Demonstrated. - `flan_reload_call` allocates its own `xfer` and discards it on return, so a transfer aimed at a frame below the - `flan_agent_poll` C frame unwinds only as far as the thunk. Three statements that the program will resume, none - true. Fix: record the restart-stack depth on entering `break_loop` and refuse any frame below it. -- **Restart names are served from a stack that is being mutated.** The stopped thread is not holding still — the break - loop runs `flan_agent_poll`, which runs arbitrary Flan, and every `restart-case` it enters pushes and pops the same - global list. `flan_restart_name` can return a pointer into a popped frame, which `send` then reads out of bounds. - Fix: publish an immutable snapshot when the break loop is entered. - **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. @@ -1211,9 +1221,12 @@ a break loop has to *show* someone their choices and nothing at run time can tur there. Accepting it and discovering on the game thread that no frame offers it would answer `ok` for something that cannot happen. -The socket verbs are `restarts`, `restart ` and `abort`, and all three are refused with the reason when the -program is not stopped — there is no restart stack to walk from a running one. `test/programs/break.flan` errors twice -and the test takes a *different* restart each time, so a loop that always resumed the same way could not pass. +The socket verbs are `restarts`, `restart-at [name]`, `restart ` and `abort`, and all four are refused with +the reason when the program is not stopped — there is no restart stack to walk from a running one. `restarts` answers +` <+|-> ` per line: the index is the identity, and the flag says whether a transfer to that frame has +anywhere to land. `test/programs/break.flan` errors three times — two restarts taken by name, then a shadowed pair +where only an index can reach the outer one — so neither a loop that always resumed the same way nor one that resolved +by name could pass. ### The break loop in the editor — conditions step 3, the other half @@ -1238,13 +1251,20 @@ Three ops, and the *annotation* owns `:stopped`, not the ops — one place in th stopped, so the poll and the prompt cannot disagree. ``` -(:op "break") → (:status "ok" :restarts ("retry" …) :stopped t :condition "Missing") -(:op "restart" :name "retry") → (:status "ok" :restart "retry" :note "accepted; …") -(:op "abort") → (:status "ok" :note "the program is exiting; …") +(:op "break") → (:status "ok" :restarts ("retry" …) :unreachable (2 3) + :stopped t :condition "Missing") +(:op "restart-at" :index 2 :name "retry") → (:status "ok" :index 2 :note "accepted; …") +(:op "restart" :name "retry") → (:status "ok" :restart "retry" :note "accepted; …") +(:op "abort") → (:status "ok" :note "the program is exiting; …") ``` -`break` carries only the restart names, because those cost a second round trip to the program and are wanted only by -someone about to choose one. +`:restarts` is *positional* — innermost first, duplicates kept — because the position is what `restart-at` takes. +`:unreachable` names the positions that are on the list and cannot be chosen. `restart-at`'s `:name` is optional and is +not the lookup: the program checks it against the name it holds at that index and refuses if they have drifted, so a +prompt cannot take a different restart than the one it showed. + +`break` carries only the restart list, because it costs a second round trip to the program and is wanted only by +someone about to choose from it. **`ok` from `restart` means accepted, not resumed.** The choice is validated on the program's listener thread against the stopped stack, then taken when that thread next comes round its loop. A client that read it as "running again" would diff --git a/conditions.org b/conditions.org index 0300423..7d81d3e 100644 --- a/conditions.org +++ b/conditions.org @@ -31,10 +31,30 @@ Why it is shaped this way: [[file:spec-conditions.md][spec-conditions.md]]. Some ~defer~ between the invoke and the target runs, innermost first, before the clause body. ~errdefer~ does not. +* The break loop + +An unhandled ~error~ in a dev build stops on the frame that erred, with nothing +unwound, and waits. ~C-c C-b~ in Emacs lists what is on offer and resumes into +the choice; ~flan:stopped(Missing)~ in the modeline says it happened. + +The list is *numbered*, and the number is what is chosen. Two frames offering +~retry~ both appear and §4's by-name walk can only ever reach the first, so a +name cannot say which one is meant — ~restart-at~ can. + +A restart below the evaluation a break is inside is listed, marked, and +refused: ~C-x C-e~ runs its thunk through a C frame that holds its own transfer +channel, so an unwind aimed past it would stop at the thunk. Choose one offered +above it, or ~abort~. + * Not yet ~handler-case~ · ~find-restart~ · ~compute-restarts~ · restarts with -parameters · the dev-build break loop. Each refused by name with its reason. +parameters. Each refused by name with its reason. + +~find-restart~ and ~compute-restarts~ are blocked on a type rather than on +effort: §4 gives them ~(Option Restart)~ and a list, and there is no ~Restart~ +type and no list to return one in. The break loop reads the same stack through +the agent's socket instead. * Gotchas diff --git a/spec-conditions.md b/spec-conditions.md index 18c3d9b..5aedd48 100644 --- a/spec-conditions.md +++ b/spec-conditions.md @@ -63,6 +63,17 @@ null check. implementation, because restarts are dynamically scoped and named. A statically tracked restart set (Zig's error-set model) remains a nice-to-have. +**Open: a clause should carry a report string.** `use-placeholder` is an +identifier, which is what `invoke-restart` needs and not what a person reading a +break loop's list needs — "carry on with a blank asset" is. SBCL's restart +struct has a `report-function` for exactly this prompt, and an +`interactive-function` for the parameters §3 already has. Nothing here mentions +either, and the break loop today shows names because names are all there are. +The cost is a string constant per clause, a field beside the name in the restart +frame, and one accessor: it is not hard, it is simply not written. It should be +settled before restarts with parameters, which is the feature that makes a bare +name least sufficient. + ## 4. Name shadowing Restart lookup walks the dynamic restart stack from innermost outward and takes @@ -74,6 +85,24 @@ found before an outer one's. `(find-restart 'name)` returns `(Option Restart)` so a handler can test before committing; `(compute-restarts)` lists the visible frames for the debugger. +**A debugger identifies a restart by its position, not by its name.** The rule +above is what a handler wants — an inner `skip-form` should win — and it is +exactly wrong for a human being shown a list: a shadowed frame is on that list +and by name is unreachable, so offering it and resolving by name means taking a +different restart than the one that was pointed at. So the break loop numbers +its list, innermost first, and a choice is a position. `invoke-restart` is +unchanged and stays by name. This is why SBCL's debugger is positional too. + +A position only means something against a stack that is holding still, which +the stopped thread's is not — the break loop runs evaluations, and each one +pushes and pops this list. The list a debugger shows is therefore a **snapshot** +taken when the break was entered, and the positions are positions in it. + +**Not every visible restart is reachable.** Transfer is lowered explicitly (§6), +so it cannot cross a frame that does not carry the channel. An evaluation run +into a stopped program is called through such a frame, and a restart below it +must be refused with the reason rather than accepted and dropped. + ## 5. Cleanup during a transfer Invoking a restart transfers control outward past zero or more frames. From 0c9f043bb1abce470a116835ea2f167dda8987b2 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sat, 12 Sep 2026 05:02:14 +0700 Subject: [PATCH 4/4] Stamp a choice with the break it was chosen from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The snapshot made the listing stand still; it did not make the handoff safe. A choice is validated against the snapshot on top when the request lands and resolved against the snapshot on top when the game thread next looks, and between those the loop runs evaluations — one that errors pushes a break of its own, whose loop reaches the flag first and takes its own index 2 for the one somebody picked off the outer list. That is the failure this change exists to end, arriving by a different door. So each snapshot carries a generation, a choice carries the generation it was validated against, and a loop claims only what is addressed to it. A mismatch is left set rather than dropped: the listener already answered ok, so the break it was meant for must still be able to take it. Depth could not do this — an outer break resuming and a new one starting reuses the number. The snapshot is also popped before the depth comes down now, so the two never describe different breaks. The client's own new path gets tested too: the candidate table is pure, so the shapes a real daemon will not easily produce are checked directly, and the break-and-resume test now goes through restart-at rather than by name. flan-dev-unreachable-restarts was dead on arrival — flan-break reads :unreachable off its own reply — and is gone. --- NEXT.md | 10 ++++++++++ emacs/flan-dev.el | 11 ----------- emacs/test-flan-dev.el | 24 +++++++++++++++++++++++- vendor/agent/flan_agent.c | 35 ++++++++++++++++++++++++++++++++--- 4 files changed, 65 insertions(+), 15 deletions(-) diff --git a/NEXT.md b/NEXT.md index 9917ea5..6b488d6 100644 --- a/NEXT.md +++ b/NEXT.md @@ -506,6 +506,16 @@ vague intention — if it is listed, someone has already established it is real. `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. +- **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, + an evaluation the break loop is running can error and push a break of its own, whose loop would otherwise reach + [chosen_ready] first and take *its* index 2 for the one someone chose from the outer list. Each snapshot now carries + a generation, a choice is stamped with the one it was validated against, and a loop claims only what is addressed to + it — a mismatch is left set rather than discarded, because the listener already answered ok for it. Depth would not + do: an outer break resuming and a new one starting reuses the number. None of this is tested, because arranging the + 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. diff --git a/emacs/flan-dev.el b/emacs/flan-dev.el index 68db5ab..a1e8c03 100644 --- a/emacs/flan-dev.el +++ b/emacs/flan-dev.el @@ -690,17 +690,6 @@ nothing left to serve once it has gone." (user-error "flan: %s" (or (plist-get r :message) "refused"))) (plist-get r :restarts))) -(defun flan-dev-unreachable-restarts () - "Positions on the restart list that cannot be chosen. -A restart below the evaluation a break is inside has nowhere for a transfer -to land — the thunk holds its own channel and drops it on return. They are -listed and marked rather than hidden, because someone who can see a restart -in their own source and not on this list has been told nothing." - (let ((r (flan-dev--request '(:op "break")))) - (unless (equal (plist-get r :status) "ok") - (user-error "flan: %s" (or (plist-get r :message) "refused"))) - (append (plist-get r :unreachable) nil))) - ;;;###autoload (defun flan-break () "Show what the stopped program is offering, and choose one. diff --git a/emacs/test-flan-dev.el b/emacs/test-flan-dev.el index d21a932..de6d605 100644 --- a/emacs/test-flan-dev.el +++ b/emacs/test-flan-dev.el @@ -420,6 +420,26 @@ is written instead — the real `message' call the real command makes." (test-flan--check "the restarts on offer are the ones the frame declared" (equal (flan-dev-restarts) '("use-placeholder"))) + ;; And what it would put in front of someone. The labels carry the position, + ;; because the position is what gets chosen: two frames may offer the same + ;; name and only a number can say which one. A pure function over a reply, + ;; so it is checked against the shapes a real daemon cannot easily be made to + ;; produce as well as against the one it just did. + (test-flan--check "the prompt numbers what it offers" + (equal (flan-dev--restart-candidates '("use-placeholder") nil) + '(("0. use-placeholder" . 0)))) + (test-flan--check "a shadowed name is two distinguishable choices" + (equal (mapcar #'cdr + (flan-dev--restart-candidates + '("retry" "use-placeholder" "retry") nil)) + '(0 1 2))) + (test-flan--check "a restart below the break is shown, and shown as such" + (let ((table (flan-dev--restart-candidates + '("retry" "use-placeholder") '(1)))) + (and (not (string-match-p "cannot be taken" (caar table))) + (string-match-p "cannot be taken" (car (nth 1 table))) + (equal (cdr (nth 1 table)) 1)))) + ;; The payoff. The break loop *is* the poll loop, so an expression sent now ;; runs on the stopped thread and comes back — which is the one moment ;; anybody actually wants C-x C-e to work. @@ -445,7 +465,9 @@ is written instead — the real `message' call the real command makes." ;; Choosing one. "ok" from the daemon means accepted — the stopped thread ;; takes it on its next pass — so the client stops claiming a break and lets ;; the next poll settle it. - (flan-dev-restart "use-placeholder") + ;; By position, which is the path `C-c C-b' takes: the name goes with it as + ;; the receipt the program checks, not as the lookup. + (flan-dev-restart-at 0 "use-placeholder") (let ((deadline (+ (float-time) 20))) (while (and (not (eq (flan-dev-state) 'live)) (< (float-time) deadline)) (flan-dev--poll) diff --git a/vendor/agent/flan_agent.c b/vendor/agent/flan_agent.c index 71fad9c..eed8ad6 100644 --- a/vendor/agent/flan_agent.c +++ b/vendor/agent/flan_agent.c @@ -138,6 +138,19 @@ static _Atomic int depth; * enough that a loop of breaks * stops rather than grinds */ static _Atomic int chosen_index; +/* Which snapshot that index is an index *into*. The listener validates a + * choice against the snapshot on top when the request arrives, and the game + * thread resolves it against the snapshot on top when it next looks - and + * those are two reads of a stack that can move between them. An evaluation + * this loop runs may error, push a break of its own, and reach [chosen_ready] + * first, which would take that break's index 2 for the one someone chose from + * the outer list. Depth is not enough to tell them apart: an outer break + * resuming and a new one starting reuses the number. A generation does not. + * + * A mismatch is *left alone* rather than discarded. The listener already + * answered ok for it, so the break it was meant for must still be able to + * take it; the inner loop simply does not claim what is not addressed to it. */ +static _Atomic int chosen_gen; static _Atomic int chosen_ready; static _Atomic int aborting; @@ -158,6 +171,7 @@ static _Atomic int aborting; #define SNAP_NAMES 4096 /* bytes of names behind them */ typedef struct { + int32_t gen; /* never reused, never 0 */ int32_t n; int32_t total; /* before SNAP_MAX truncated it */ void *frame[SNAP_MAX]; @@ -181,11 +195,14 @@ static snapshot *snap_top(void) { /* Called on the game thread with the stack held still. 0 if there is no room * to nest, which the caller reports rather than serving a stale one. */ +static int32_t snap_gen; /* monotone; 0 is "no snapshot" */ + static int snap_push(void) { int d = atomic_load(&snap_depth); if (d >= BREAK_MAX) return 0; snapshot *s = &snaps[d]; int32_t n = flan_restart_count(); + s->gen = ++snap_gen; s->total = n; s->used = 0; s->n = 0; @@ -234,6 +251,7 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition, * break to ask about: the list on the terminal and the list on the socket * are then the same list, numbered the same way, and the numbers are what a * choice is made of. */ + int32_t my_gen; if (!snap_push()) { fflush(stdout); fprintf(stderr, "flan: %d nested break loops - giving up rather than " @@ -243,6 +261,7 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition, } { snapshot *s = snap_top(); + my_gen = s->gen; if (s->n == 0) fprintf(stderr, " no restarts are active; abort, or fix and reload\n"); for (int32_t i = 0; i < s->n; i++) @@ -292,10 +311,14 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition, * arriving during the attempt passes its own check, writes a new name * and sets the flag, and the store below then erases it. Copying also * keeps strlen off a buffer the listener may be writing. */ + /* Read before the claim: a choice addressed to some other break is + * not this one's to consume. */ + if (atomic_load(&chosen_gen) != my_gen) { nanosleep(&step, NULL); continue; } int take = atomic_load(&chosen_index); atomic_store(&chosen_ready, 0); snapshot *s = snap_top(); - int ok = s != NULL && take >= 0 && take < s->n && s->reachable[take]; + int ok = s != NULL && s->gen == my_gen && take >= 0 && take < s->n + && s->reachable[take]; if (ok) { flan_restart_take(s->frame[take], xfer); fprintf(stderr, "flan: resuming at restart %d. %s\n", take, @@ -307,8 +330,12 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition, * at the *next* unhandled error, minutes later, in unrelated code, * giving nobody the chance to choose. */ atomic_store(&aborting, 0); - atomic_fetch_sub(&depth, 1); + /* Popped *before* the depth comes down. The other order leaves a + * window where [depth] says the outer break is the current one and + * [snap_top] still answers with the inner one's list, so a request + * arriving in it is validated against a list nobody is looking at. */ snap_pop(); + atomic_fetch_sub(&depth, 1); return; } /* The listener checks all of this before answering ok, so reaching here @@ -492,8 +519,9 @@ static void serve(int fd) { return; } atomic_store(&chosen_index, (int)idx); + atomic_store(&chosen_gen, s->gen); /* Published last, so the game thread never reads an index that is about - * to change. */ + * to change, or one whose generation has not arrived yet. */ atomic_store(&chosen_ready, 1); reply(fd, "ok\n"); return; @@ -527,6 +555,7 @@ static void serve(int fd) { return; } atomic_store(&chosen_index, at); + atomic_store(&chosen_gen, s->gen); atomic_store(&chosen_ready, 1); reply(fd, "ok\n"); return;