JosephFerano/flan
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Stamp a choice with the break it was chosen from

Browse Source 
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.
This commit is contained in:
Joseph Ferano 2026-09-12 05:02:14 +07:00
parent 91d1368279
commit 0c9f043bb1
4 changed files with 65 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
NEXT.md
View File

@ -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.

11
emacs/flan-dev.el
View File

@ -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.

24
emacs/test-flan-dev.el
View File

@ -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)

35
vendor/agent/flan_agent.c vendored
View File

@ -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;