diff --git a/FIX.org b/FIX.org index 9cdda86..01739b8 100644 --- a/FIX.org +++ b/FIX.org @@ -2442,3 +2442,75 @@ failures were that row and the sixth was ~dev-trap-free-all~, so what is racy is ~trap_park~ itself and every row that calls it — which is exactly what the mechanism described there predicts. Per the sweep policy the ~@x86~ and ~@sanitize~ sweeps were not run here. + +* (agent/start) lost its argument, 2026-09-20 +Four notes from the lane that made the socket path optional and bound it +before main. Three of them are about ground this lane deliberately did not +take; the fourth is a line the author can delete at leisure. + +** sand.flan can drop its socket path +=(agent/start "/tmp/flan-sand.sock")= at sand.flan:125 still works and always +will — the explicit form is not going anywhere. But the path was only ever a +value nothing read under =flan dev=, because the daemon overrides it through +FLAN_AGENT_SOCKET, and the zero-argument =(agent/start)= now does the right +thing in both places: the daemon's socket when there is one, and an announced +=/tmp/flan-agent--.sock= when there is not. Changing that line is +a one-word edit whenever the author feels like it; this lane does not touch +sand.flan. + +With the constructor below, sand.flan could delete the call outright — it +calls =(agent/poll)= in its frame loop, which is the condition. That is a +bigger claim than a shortened line and is worth making deliberately. + +** Auto-start reaches as far as the linker does, and no further +=vendor/agent/flan_agent.c='s =auto_start= constructor binds FLAN_AGENT_SOCKET +before main, so a program under =flan dev= needs no =(agent/start)= at all. +What it cannot do is reach a program that never mentions the agent: =Reach= +prunes a package nothing calls into, so an executable with no =(import agent +...)= — or one that imports it and calls nothing — does not link the file the +constructor is in. There is nothing to run. + +So the true scope is: *a program that calls =(agent/poll)= or =(agent/wait)= +and has dropped its start call*. That is the ceremony the feature was asked to +remove, and it is removed. Full invisibility — a dev build that links the +agent because it is a dev build, whether or not the source says so — needs the +package force-linked from Load/Build, which are files this lane did not own +and a decision about what =--dev= means rather than about the agent. + +The constructor is also not =--dev=-only, because nothing in +=vendor/agent/flan_agent.c= can tell the two builds apart: the dev runtime is +linked either way and there is no weak symbol to ask. A *release* binary that +links the agent and is run with FLAN_AGENT_SOCKET set in its environment +therefore binds a listener it would not have bound before. Only =flan dev= +sets that variable and it never runs release builds, so this is a sentence +about the shape of the gate rather than an observed problem — but it is the +one behavioural difference outside the dev loop and it should be said. + +** The daemon's "has not called (agent/start ...)" note is now unreachable +=install_note= (lib/dev.ml:789) and the =describe= branch at lib/dev.ml:1085 +say, of a RUNNING program whose socket is not bound, that a redefinition +installs at its next =(agent/poll)= and not at all if there is none. For a +program that links the agent that cannot happen any more: the constructor +binds before main, so by the time any editor can ask, =agent_bound= is true. + +The sentence is still reached — and still right — for a program that does not +link the agent at all, which is =programs/dev-noagent.flan= and the row in +test_dev.ml that drives it. So nothing here is dead; what is dead is the +*late* case, which is what test_dev.ml's late-agent row used to assert and now +asserts the negation of. Retiring the branch is the author's call over a lane +that merged days ago, not this one's. + +** The agent socket under the daemon is still the temp directory's problem +=start_on= now stashes the path it bound and unlinks it three ways: an atexit +for an ordinary exit, and by hand in =die_now= and =orphan_die=, which both +leave by =_exit= and skip the atexit chain deliberately. That covers a program +run on its own, a program aborted out of the break loop, and an orphan whose +daemon died. + +It does not cover an ordinary =flan dev= session ending, and cannot: the +two-process daemon kills its child with SIGTERM and the merged session leaves +by =Unix._exit 0=, neither of which runs an atexit. The socket sits in +=/tmp/flan-dev-/= and goes when that directory goes — which is the item +above, "The daemon leaves its temp directory behind", still open. No separate +fix is wanted here; the session-end cleanup that item asks for takes the +socket with it. diff --git a/test/programs/agent-nostart.flan b/test/programs/agent-nostart.flan new file mode 100644 index 0000000..f1a5543 --- /dev/null +++ b/test/programs/agent-nostart.flan @@ -0,0 +1,27 @@ +;;;; A program with no (agent/start) in it at all, which still takes a +;;;; redefinition. +;;;; +;;;; The listener comes from the package's constructor, which binds +;;;; FLAN_AGENT_SOCKET when the daemon has set it — so under [flan dev] there +;;;; is nothing to call and nothing to forget. Installing is still the +;;;; program's own decision and still happens where it says: [wait] is the +;;;; headless spelling of the poll at the top of a frame, and the second number +;;;; below cannot appear before it. +;;;; +;;;; It does import the agent and it does call into it. That is not an accident +;;;; of how the fixture was written: [Reach] prunes a package nothing calls, so +;;;; an executable that mentions the agent nowhere does not link the +;;;; constructor either. See flan_agent.c's [auto_start]. +(import agent "vendor:agent") + +(defvar ticks i64) + +(defn tick [] i64 + (set ticks (+ ticks 1)) + ticks) + +(defn main [] i32 + (print (tick)) (println "") + (while (= (agent/wait 100) 0) 0) + (print (tick)) (println "") + 0) diff --git a/test/programs/dev-lateagent.flan b/test/programs/dev-lateagent.flan index dd9eea4..a483245 100644 --- a/test/programs/dev-lateagent.flan +++ b/test/programs/dev-lateagent.flan @@ -12,6 +12,12 @@ ;;;; Two claims are checked against it in test_dev.ml: the session answers ;;;; while the sleep is still running, and a redefinition sent during the ;;;; sleep is installed once the program is up. +;;;; +;;;; The socket is no longer late, only the call is. The package's constructor +;;;; binds it before main when a daemon has said where, so the start below +;;;; arrives at an agent that is already listening and is a no-op answering the +;;;; same socket — which is what makes this fixture the double-start case under +;;;; a real daemon as well. (import agent "vendor:agent") (defvar frames i64) diff --git a/test/test_agent.ml b/test/test_agent.ml index 3f4131d..be64f72 100644 --- a/test/test_agent.ml +++ b/test/test_agent.ml @@ -269,6 +269,74 @@ let () = List.iter (fun f -> try Sys.remove f with Sys_error _ -> ()) [ aexe; aso; aout; aerr ]; + (* ── No (agent/start) at all ────────────────────────────────────── *) + + (* The ceremony gone rather than shortened: this program says nothing about + the agent except that it installs what has arrived. The listener is the + package's constructor, and the condition it runs on is + FLAN_AGENT_SOCKET — which is exactly what the daemon sets, in both + shapes, before the program starts. Setting it here is therefore not a + test fixture standing in for the daemon; it is the same variable set the + same way, which is why this does not need a daemon to be the claim. + + The two numbers are the split the agent exists for, and they are what + keeps this honest: a listener bound before main is still not an install. + 1 is the body the program was built with, printed before anything could + have arrived; 1000 is a body that did not exist then, and it appears + only after the [wait] — the program's own frame boundary — which is the + half no constructor can do for it. + + [Reach] is the limit, and the fixture's header says so: the package is + linked because the program calls into it. A program that called nothing + would not have this constructor in it to run. *) + let nsock = tmp "nostart.sock" and nout = tmp "nostart.out" in + (try Sys.remove nsock with Sys_error _ -> ()); + let nt, nl = Session.create ~file:"programs/agent-nostart.flan" () in + let nexe = tmp "nostart" in + ignore + (Build.executable ~opts:dev ~csrcs:nl.Load.csrcs ~lflags:nl.Load.lflags + nt.Session.host ~out:nexe); + let nso = tmp "nostart-tick.so" in + let nc = Session.eval nt "(defn tick [] i64 1000)" in + ignore (Build.shared ~opts:dev ~ir:nc.Session.ir ~out:nso ()); + let nenv = + Array.append (Unix.environment ()) [| "FLAN_AGENT_SOCKET=" ^ nsock |] + in + let nfd = + Unix.openfile nout [ Unix.O_WRONLY; Unix.O_CREAT; Unix.O_TRUNC ] 0o600 + in + let npid = Unix.create_process_env nexe [| nexe |] nenv Unix.stdin nfd nfd in + Unix.close nfd; + if not (await (fun () -> Sys.file_exists nsock)) then begin + fail "a program with no (agent/start) never bound %s" nsock; + (try Unix.kill npid Sys.sigkill with Unix.Unix_error _ -> ()) + end + else begin + let r = send nsock nso in + if r <> "ok\n" then + fail "the constructor's socket refused a module: %S" r; + let nstatus = ref (Unix.WEXITED 0) in + let reaped = + await ~ms:5000 (fun () -> + match Unix.waitpid [ Unix.WNOHANG ] npid with + | 0, _ -> false + | _, s -> nstatus := s; true) + in + if not reaped then begin + (try Unix.kill npid Sys.sigkill with Unix.Unix_error _ -> ()); + fail "the redefinition never reached a program that never started an \ + agent" + end + else begin + let text = In_channel.with_open_bin nout In_channel.input_all in + if !nstatus <> Unix.WEXITED 0 || text <> "1\n1000\n" then + fail "no (agent/start)\n got: %S\n wanted: %S" text + "1\n1000\n" + end + end; + List.iter (fun f -> try Sys.remove f with Sys_error _ -> ()) + [ nexe; nso; nsock; nout ]; + (* ── The break loop, spec-conditions.md §2 ──────────────────────── *) (* The claim is that an unhandled [error] stops rather than dying, and can diff --git a/test/test_dev.ml b/test/test_dev.ml index 0f578f3..ca139be 100644 --- a/test/test_dev.ml +++ b/test/test_dev.ml @@ -5110,13 +5110,28 @@ let () = (* ── A program that starts its agent late ────────────────────────── The shape a real one has: sand.flan opens a window and starts the - agent afterwards, so the socket appears seconds into the run. The - merged session used to wait up to ten seconds for it *before* running - its accept loop, which charged those seconds to the first thing the - editor asked. Two claims, and the second is what stops the first from - being bought with a lie: the session answers during the delay, and a - redefinition sent during it is really installed once the program is - up. *) + agent afterwards, so the program's own [(agent/start ...)] is seconds + into the run. The merged session used to wait up to ten seconds for the + socket *before* running its accept loop, which charged those seconds to + the first thing the editor asked. Two claims, and the second is what + stops the first from being bought with a lie: the session answers during + the delay, and a redefinition sent during it is really installed once + the program is polling. + + The socket itself is no longer late. The agent package's constructor + binds FLAN_AGENT_SOCKET before main (vendor/agent/flan_agent.c, + [auto_start]), so under a daemon it is there from the first instant + whatever the program does afterwards — which is what this fixture's + sleep was a stand-in for, and the window DISCUSS.org complained about, + closed. What is still late is the *poll*, and that is the half that + matters: a module queued now installs when the program reaches its loop + and not before. + + So this is also where the double start is pinned against a real daemon. + The fixture keeps its explicit [(agent/start ...)] three seconds in, + arriving at an agent that is already listening, and the delivery below + is installed on the socket the constructor bound — one listener, not + two, or the module would be queued on a ring nothing drains. *) let lsock = tmp "lateagent.sock" and lout = tmp "lateagent.out" in (try Sys.remove lsock with Sys_error _ -> ()); let lfd = @@ -5150,12 +5165,20 @@ let () = fail "the first editor request waited %.1fs on a program whose agent \ starts late; the accept loop is gated on the agent again" ldt; - (* And the delivery, also inside the delay. It is taken — in one process - the agent's ring is reachable whether or not the program has bound a - socket — and the note says what "queued" means for a program that has - not got to its poll yet. Asserted because the alternative was the - reply this whole area exists to prevent: an "ok" that reads as - installed. *) + (* And the delivery, also inside the delay. It is taken, and it carries + no note about [(agent/start ...)]: that sentence is for a program + whose socket is not bound, and the constructor bound this one before + main. [install_note] answering nothing here is therefore the evidence + that the socket is up — the assertion is on the absence because the + absence is the claim. + + It used to be the presence. The program's own start call is still + three seconds away, so this is the same moment it always was; what + changed is that the moment is no longer one in which the program + cannot be reached. lib/dev.ml's branch still says the true thing for + a program that does not link the agent at all (the agentless row + above), and FIX.org records that a dev program which links it can no + longer get there. *) let r = request lc "(:op \"eval\" :code \"(defn step [] i64 9)\" :file \ @@ -5165,10 +5188,10 @@ let () = fail "a redefinition sent before (agent/start ...): %s" (said r) else begin let note = Option.value ~default:"" (Wire.string_field r "note") in - if not (contains_sub note "(agent/start ...)") then + if contains_sub note "(agent/start ...)" then fail - "a redefinition delivered before the program's agent was up said \ - nothing about it: %S" note + "the agent socket was not bound before main, so a delivery was \ + told to wait for a call the program had not made: %S" note end; (* The claim the note makes, checked against the program rather than against the reply: once the sleep is over and the program is polling, diff --git a/vendor/agent/flan_agent.c b/vendor/agent/flan_agent.c index 26ba7d8..05914a6 100644 --- a/vendor/agent/flan_agent.c +++ b/vendor/agent/flan_agent.c @@ -593,10 +593,18 @@ static char bound_sock[sizeof(((struct sockaddr_un *)0)->sun_path)]; * break-loop abort die_now, by hand, because it takes _exit * orphaned child orphan_die, by hand, for the same reason * - * Under [flan dev] the socket also sits inside the daemon's temp directory, - * but that directory is not removed today (FIX.org, "The daemon leaves its - * temp directory behind"), so this is the only thing that takes the socket - * away. Outside the daemon the path is under /tmp and nothing else would. */ + * Outside a daemon that is the whole story, and the path is under /tmp where + * nothing else would ever reclaim it. + * + * Under [flan dev] none of the three is how a session usually ends, and this + * is worth being exact about rather than claiming cover it does not give: the + * two-process daemon kills its child with SIGTERM, whose default disposition + * runs no atexit, and the merged session leaves by [Unix._exit 0]. So the + * socket there is left in the daemon's temp directory — which is itself never + * removed today. FIX.org, "The daemon leaves its temp directory behind": the + * session-end cleanup that item asks for takes the socket with it, and until + * it lands the socket outlives the session. Nothing below can fix that from + * here; a program that is killed does not get to tidy up. */ static void unlink_bound_sock(void) { if (bound_sock[0] != '\0') unlink(bound_sock); } @@ -1880,3 +1888,39 @@ int32_t flan_agent_start_auto(void) { } return r < 0 ? -1 : 0; } + +/* And the call itself, gone. A program under [flan dev] that imports this + * package gets the listener before main, without asking. + * + * FLAN_AGENT_SOCKET is the whole condition, and it is the right one: the + * daemon sets it in both shapes — before the fork in --two-process, before the + * exec in the merged build — and nothing else on a machine sets it. So an + * ordinary run of an ordinary program falls straight through here and this + * costs it one getenv. (Not FLAN_DEV_PARENT, which is deliberately unset in + * the merged build; gating on it would quietly skip half the daemon.) + * + * WHAT THIS DOES NOT REACH, because it is a fact about linking rather than a + * choice made here: [Reach] prunes a package nothing calls into, and an + * executable that never mentions the agent does not link this file at all — so + * there is no constructor in it to run. Auto-start covers a program that calls + * [(agent/poll)] and has dropped its [(agent/start)], which is the ceremony + * this was asked to remove. A program that wants a dev loop while calling + * nothing at all would need the package force-linked into every --dev build, + * which is a decision for Load and Build and not for this file. + * + * The window it closes is real and was costing seconds: a program that opened + * a window before its [(agent/start)] left the daemon waiting on a socket that + * did not exist yet (DISCUSS.org). Bound here, it exists before main. + * + * A pthread from a constructor is fine — it is this executable's own init, not + * a dlopen, so nothing holds the loader lock against it — and the thread it + * starts does nothing until something connects. The narrow hazard is a request + * arriving before [flan_rt_init]: the verbs read runtime state, and a person + * with nc and very quick fingers could ask before there is any. The daemon + * cannot, in either shape — it has an editor to hear from first, and a module + * to compile after that. */ +__attribute__((constructor)) static void auto_start(void) { + const char *env = getenv("FLAN_AGENT_SOCKET"); + if (env == NULL || env[0] == '\0') return; + (void)start_on(env); +}