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;