From 0c9f043bb1abce470a116835ea2f167dda8987b2 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sat, 12 Sep 2026 05:02:14 +0700 Subject: [PATCH] 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;