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

Merge branch 'worktree-agent-a30e436605521558c' into dev-loop

Browse Source 
# Conflicts:
#	NEXT.md
This commit is contained in:
Joseph Ferano 2026-09-13 13:47:12 +07:00
commit 781295e862
4 changed files with 132 additions and 62 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

47
HANDOFF-f1.md
View File

@ -54,22 +54,36 @@ longer sweep was **not measured**.
## What remains
1. **Identify and fix the other "the daemon never listened" flake.** It appeared once in one of two
post-change full `dune test` runs and is *not* test_dev's first block (that block is 0/10 under
the new code, and its own message is distinct). The message text is emitted from
`test/test_emacs.ml:50` and `test/test_repl.ml:71` with exactly that wording; `test/test_dev.ml`'s
other daemons all use qualified wording ("the break daemon…", etc.), so the culprit is almost
certainly `test_emacs` or `test_repl`. Re-run `dune test --root .` in a loop and capture per-binary
output to disambiguate. Likely the same class of bug — a bounded `await` on a socket path under
parallel-dune CPU contention — but that is a hypothesis, not a finding. Pre-existing: it did not
appear in the pre-change full run, but one observation each way proves nothing.
2. **Decide whether `merged_serve`'s 10s warning path deserves a test.** Nothing currently exercises
1. **Decide whether `merged_serve`'s 10s warning path deserves a test.** Nothing currently exercises
lib/dev.ml:2322-2326 (a program that never calls `agent/start`). It was reachable only by accident
here, via the unlink race that is now closed.
3. **Confirm the fix at the rate NEXT.md claims** with a longer sweep (40+ runs) if the 0/10 is
considered too thin, then delete the "One flaky test, measured rather than suspected" section from
`NEXT.md` (around line 138). That section was deliberately **left untouched** — it should be
removed by whoever confirms the fix, not by the change that made it.
## Closed since: the other "the daemon never listened", which was not a race at all
The previous item 1 sent two lanes after a contention race. **There is none**, and the reading it was
built on was wrong twice over:
- The wording is **not** unique to `test_emacs.ml:50` and `test_repl.ml:71`. `test/test_dev.ml:77` —
the *dev* socket check in the same first block this handoff fixed the *agent* socket check in —
emitted exactly `the daemon never listened` as well. Three sites, not two, which is why a sighting
in a full-run log could not be attributed.
- The cause is a **bound that is too short for a build, not a race**. `flan dev` does an llc-and-link
of the whole program *before* it binds its socket, so the await covers a build. Warm and idle that
is ~600ms; under dune's own parallelism it was measured at 6.5s here and at 6.6s and 6.8s by the
lane that fixed it — against a 5000ms await in `test_dev.ml` and 8000ms in the other two. Nothing
about it is occasional; it reproduces whenever the build is cold, and `Build.cachedir` sits under
dune's per-run `TMPDIR`, so the object cache starts empty on **every** `dune test` invocation.
Fixed on another branch: all three awaits now wait a minute, bounded by the existing watchdog rather
than by a guess, and `test_dev.ml` carries a named `listening` helper so the reason is written once
for its thirteen daemons.
Two measurements from this lane that stand whatever else changes: **30 sequential full `dune test`
runs, 0 failures and no `never bound` in any of them** — the `agent.sock` ordering fix above is
confirmed at a sweep wide enough to close item 3 of the old list, so `NEXT.md`'s "One flaky test,
measured rather than suspected" section is deleted. The reproduction that came after it, on the 31st
run, was the *dev* socket at `test_dev.ml:77`, with `built dev-loop.flan in 6525ms` sitting four lines
below the FAIL in the same log — which is what turned the hypothesis into the finding above.
## Tried and rejected
@ -77,7 +91,10 @@ longer sweep was **not measured**.
It looks like the obvious fix and it is wrong: it makes the test observe the file at the *earliest*
possible instant, which makes it *more* likely to win the unlink race against `merged_serve` and
more likely to trigger the silent 10s stall. Do not reintroduce it.
- **Lengthening any timeout.** Not needed; the ordering was missing, not the time.
- **Lengthening any timeout** — *for the `agent.sock` check*. Not needed there; the ordering was
missing, not the time. This does **not** generalise, and reading it as a rule cost two lanes: the
socket-appears checks above are a wait for a *build*, and there lengthening the bound was exactly
the fix. See the section before this one.
## Environment note for whoever picks this up

48
HANDOFF-f3.md
View File

@ -179,25 +179,43 @@ default that needs widening whenever the set of heads grows. Cheaper than the al
---
## What remains, in the order a fresh session should do them
## What remains
Item 5 is complete. These are the two gaps I would close next, both small.
Item 5 is complete, and so is the fixture the previous list asked for: `emacs/test-flan-watch.el` now has a block
"Every head the default regexp claims to cover" pinning `watch-num-i64` and `watch-num-f64` beside the one-segment
heads. It was verified by mutation — restoring the old `?\=` in `flan-watch-ghost-call-regexp` makes exactly those
two checks fail and nothing else — so a regression in that regexp is now caught.
1. **`emacs/test-flan-watch.el` — add a `watch-num-i64` fixture site.** The regexp fix is covered by *nothing*
automated; I verified it by hand in `emacs --batch`. Add a fixture whose buffer text contains
`(watch-num-i64 "cell" x)` and assert `flan-watch--ghost-sites` returns `("cell" . …)`, alongside the existing
`watch-i64` fixtures. No daemon needed — the file is already in the suite via `test-flan-cider.el` and, per
`BUILT.md`, ghost text is a pure function from rows plus buffer text to overlays. **This is the highest-value
remaining item**: it is the one change in this commit with no regression test behind it.
One item is left, and it turned into a finding rather than a test.
2. **`test/test_dev.ml` — assert the `n=0` render.** `watch_render_num` in `runtime/flan_dev.c` has a branch for a
window with no samples that emits `n=0 last=<v>` (the range and the mean have nothing behind them, but `last` is
still the last value the name ever had). No test reaches it: the program in `dev-watch.flan` samples every step,
so a window is never empty in practice. Reaching it needs a reset followed by a read inside one step, or a
program with a watch call that runs only sometimes.
1. **`watch_render_num`'s `n=0` branch is unreachable, so the test that was asked for cannot be written.**
`runtime/flan_dev.c:764` renders `n=0 last=<v>` for a window with no samples. Nothing can present that state to
the reader:
Neither blocks anything. Item 6 (frame rollback as a worked example) is the next real piece of work and `NEXT.md` now
points at it.
- `s->num` is set to 1 in exactly one place, `watch_record` (flan_dev.c:701), and that function always falls
through to `s->n += 1`. The clear (`s->n = 0` on an epoch change) and the increment are both inside one
odd-generation window, so a reader cannot see between them.
- `flan_dev_watch_begin` sets `s->num = 0` and never touches `s->n`, so the scalar path cannot leave a num slot
at zero either.
- `flan_dev_watch_read` never compares `s->epoch` to `watch_epoch`. A reset therefore does not make the slot read
as empty.
The user-visible consequence is worth stating on its own, because it is the thing the branch was presumably
written for: **after `:reset t`, a read taken before the program's next sample reports the *previous* window's
five numbers, not an empty one.** That is why `test_dev.ml`'s reset assertion has to `await` the count dropping
rather than read once — the test is already working around this.
Two ways out, and both are decisions rather than test work, which is why neither was taken here (the lane was
scoped to tests):
- **Delete the branch.** Honest about what the design says: a window begins when the program next runs, and until
then the last window is what there is to show.
- **Make the read epoch-aware** — compare `s->epoch` with `watch_epoch` under the seqlock and render `n=0` when
they differ. This makes a reset visible immediately without the reader writing the table, and would let the
existing `await` in the test become a single read. It is a behaviour change to what an editor sees in the tick
after a reset, so it belongs to whoever owns `runtime/flan_dev.c`, not to a test pass.
Item 6 (frame rollback as a worked example) is the next real piece of work and `NEXT.md` points at it.
---

68
NEXT.md
View File

@ -101,23 +101,34 @@ The fix is ordering, not timing: the test checked for `agent.sock` before comple
the check took it from 2/8 failures to 0/10, and closed a second race that was burning the full 10s timeout.
`lib/dev.ml` was not touched.
1. ~~**A different flake, seen once and unexplained.** `the daemon never listened`~~ **Diagnosed and fixed.** The
hypothesis was right and it is not a race at all: the wait is not for a socket but for an llc-and-link of the
whole program before `flan dev` binds. That is ~600ms idle and was measured twice at **6.6s and 6.8s** with the
rest of the suite running beside it under dune's own parallelism, against a 5s (`test_dev.ml`) and 8s
(`test_emacs.ml`, `test_repl.ml`) `await`. The message reads like a bug in the daemon and is a build slower than
the timeout. All three now wait a minute — the watchdog is what bounds the run, and this only has to be long
enough that a failure means the daemon is not coming. `test_dev.ml` has a named `listening` for it so the reason
is written once for its thirteen daemons.
2. **Decide whether `merged_serve`'s 10s warning path deserves a test.** `lib/dev.ml:2322-2326` is exercised by
nothing now that the unlink race is closed.
3. **Confirm at a longer sweep if 0/10 is too thin, then delete this file's "One flaky test, measured rather than
suspected" section.** It was deliberately left standing — it should go when someone confirms the fix, not in the
change that made it.
**The `the daemon never listened` flake is diagnosed and fixed, and it was never a race.** The wait is not for a
socket but for an llc-and-link of the whole program before `flan dev` binds. That is ~600ms idle and was measured at
**6.6s and 6.8s** with the rest of the suite running beside it under dune's own parallelism, against a 5s
(`test_dev.ml`) and 8s (`test_emacs.ml`, `test_repl.ml`) `await`. The message reads like a bug in the daemon and is
a build slower than the timeout. All three now wait a minute — the watchdog bounds the run, and this only has to be
long enough that a failure means the daemon is not coming. `test_dev.ml` has a named `listening` so the reason is
written once for its thirteen daemons.
Two things already tried and rejected, so they are not re-derived: wrapping the check in the file's `await` helper
makes the test observe the file at the *earliest* instant and so more likely to lose the race, and no timeout needed
lengthening.
Two things found while chasing it that are worth not re-deriving. **The site was `test_dev.ml:77`**, not
`test_emacs.ml`/`test_repl.ml` — the belief that only those two carry that wording was false and sent one lane after
the wrong file. And **every build in the suite is cold**, because `Build.cachedir` (`lib/build.ml:59`) sits under
dune's per-run `TMPDIR`, so no run ever reuses the object cache: ~550ms warm against 2.9-4.0s cold. That is the
root of the timeout problem and a longer `await` only hides it.
**The socket ordering fix is confirmed** at 30 sequential full `dune test` runs, 0 failures, no `never bound` in any
log — so the section this file used to carry about it is gone.
1. **Decide whether `merged_serve`'s 10s warning path deserves a test.** `lib/dev.ml:2322-2326` is exercised by
nothing now that the unlink race is closed.
**The other "never listened" was not a race, and the note that sent people after one was wrong twice.** That wording
came from *three* sites, `test_dev.ml:77` included, not from the two this file named. And it is a bound too short for
a build, not contention: `flan dev` links the whole program before it binds, ~600ms warm and measured at 6.5-6.8s
under dune's own parallelism, against 5000ms in `test_dev.ml` and 8000ms in the other two — `Build.cachedir` lives
under dune's per-run `TMPDIR`, so that build is cold on every invocation. Fixed on another branch: the three awaits
wait a minute now, bounded by the watchdog, with a named `listening` helper in `test_dev.ml`. The one thing not to
re-derive from `HANDOFF-f1.md` is its "lengthening any timeout" bullet — that applies to the `agent.sock` check
only, and reading it as a rule is what kept this open.
### From `HANDOFF-f3.md` — the watch is built; two tests are not
@ -128,11 +139,15 @@ editor's last tick rather than cumulative, because cumulative min/max reach the
play and then never move again. Reset is its own message and never a side effect of reading, so the reader never
writes the table.
1. **`emacs/test-flan-watch.el` — a `watch-num-i64` fixture site.** The regexp fix is the one change in that lane
with no regression test. `flan-watch-ghost-call-regexp` allowed a single hyphenated segment, so `watch-num-i64`
backtracked to failure and a numeric watch showed *no* inline value while looking normal in the buffer.
2. **`test/test_dev.ml` — assert the `n=0` render.** `watch_render_num` in `runtime/flan_dev.c` has a branch for a
slot that was never sampled and nothing reaches it.
The fixture is in: `emacs/test-flan-watch.el` pins both accumulator heads beside the one-segment ones, verified by
putting the old `?` back in `flan-watch-ghost-call-regexp` and watching exactly those two checks fail.
1. **`watch_render_num`'s `n=0` branch is dead, and the choice is whose it is to remove.** No test can reach it:
`s->num` is set only in `watch_record`, which always falls through to `s->n += 1` inside one odd-generation
window, and the reader never compares the slot's epoch to the global one. The live consequence is that a read
after `:reset t` and before the program's next sample reports the *previous* window — which is why the test waits
for the count to drop instead of reading once. Either delete the branch or make the read epoch-aware; both are
`runtime/flan_dev.c` decisions. `HANDOFF-f3.md` has the argument.
### Not started: five more raylib core examples
@ -316,17 +331,6 @@ state holds fixed arrays or `Vec`s.
**raylib 6.0 is not urgent:** all seven struct layouts on the game's path and every enum value it touches are
byte-identical between 5.5 and the vendored 6.0.
### One flaky test, measured rather than suspected — **fixed, pending a longer sweep**
The ordering fix is in (`f80a7a9`): 2/8 failures before, 0/10 after. This section stays until someone confirms it at
a sweep wide enough to be worth the claim, then goes. See `HANDOFF-f1.md`. The original reading follows.
`test_dev.ml`'s first block fails about one run in four with *"the merged program never bound
…/agent.sock"*. **It is not new** — reproduced at `54027ca`, before any of today's work, at the same rate. It is a
startup race in the one-process daemon's socket bind, not a real failure, and it makes "the suite is green" a
statement that needs a second run to make. Worth fixing before it trains someone to re-run on red.
### Left mid-flight when the session ended
Both lanes committed their main work and died on trailing polish; both are merged and the suite is green.

31
emacs/test-flan-watch.el
View File

@ -67,6 +67,37 @@ what bounds its cost; a `with-temp-buffer' would be scanned by nothing."
"and a call written inside a string is not one of them"
(not (member "fake" (mapcar #'car sites)))))))
;; --- Every head the default regexp claims to cover ------------------------
;;
;; `flan-watch-ghost-call-regexp' allows *any number* of hyphenated segments,
;; and the accumulator is why. With one optional segment — the `?\=' this
;; default used to have — `watch-num-i64' matched as far as `watch-num', then
;; required whitespace, found `-i64', and backtracked to failure. The failure
;; is silent and looks like nothing: the row is in the watch buffer as usual
;; and the call site simply never gets an inline value. So the two-segment
;; heads are pinned here beside the one-segment ones, or the next widening of
;; this default has nothing to fail against.
(let ((buf (test-flan-watch--buffer
(concat "(defn step [] i64\n"
" (watch \"bare\" x)\n"
" (watch-i64 \"ticks\" ticks)\n"
" (watch-f64 \"hp\" hp)\n"
" (watch-str \"label\" label)\n"
" (watch-num-i64 \"cell\" (at grid i))\n"
" (watch-num-f64 \"dt\" dt))\n"))))
(with-current-buffer buf
(let ((sites (flan-watch--ghost-sites)))
(test-flan--check
"a two-segment head is a site: the accumulator gets inline text too"
(equal (assoc "cell" sites)
(cons "cell" (save-excursion (goto-char (point-min))
(line-end-position 6)))))
(test-flan--check
"and every head the default documents is found, in buffer order"
(equal (mapcar #'car sites)
'("bare" "ticks" "hp" "label" "cell" "dt"))))))
;; --- Matching a row to a site ---------------------------------------------
;;
;; The runtime holds 31 bytes of a name, so a longer one in the source can