diff --git a/FIX.org b/FIX.org index 27e1313..864bc3d 100644 --- a/FIX.org +++ b/FIX.org @@ -3272,3 +3272,111 @@ a reason that is now stated correctly. deliberately*: the website has its own rewrite lane, and a marketing page is not the place for this lane to be making edits it cannot test. Flagged here so that lane picks it up. + + +* (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 way it would be felt is theft rather than noise, and that is worth +spelling out: =start_on= unlinks the path before binding it, because a stale +socket from a previous run is the ordinary case. So if FLAN_AGENT_SOCKET ever +leaks into a shell's exported environment — a person exporting it by hand to +drive a program with =nc=, a terminal opened from a daemon's child — every +agent-linked program started from that shell takes the path away from whoever +bound it first. The earlier program keeps an fd on a socket with no name and +goes silently unreachable: the daemon that was talking to it now reaches the +newcomer. Before this lane the unlink was reached only by an explicit +=(agent/start ...)=, which is a line somebody wrote; now any agent-linked +program run in that environment does it before main. The gate is the same +variable either way, so the fix, if this is ever felt, is a narrower gate +rather than a narrower unlink. + +** 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. + +It was true of exactly one thing, and the constructor is what removed it: a +merged session whose program *links* the agent, where the ring is reachable +in-process from the first instant while the socket is not bound until +=(agent/start ...)= runs. Bound before main, that window is gone. Going +through the other three shapes leaves nothing: + +- *merged, program links the agent* — the socket is bound before main, so + =agent_bound= is true by the time any editor can ask. This is the window + above, closed. +- *merged, program does not link the agent* — there is no =flan_agent_request= + in the process and no socket either, so the delivery is refused with "cannot + reach the program on ..." and never reaches =install_note= at all. Pinned as + of this lane by =programs/dev-noagent-running.flan= and its row in + test_dev.ml, which also holds that the session survives the refusal. +- *two-process* — =two_process= kills the child and =failwith=s when the + socket never appears, so a program with no agent has no session to be sent + anything. + +So the branch at lib/dev.ml:789 and the =describe= arm at lib/dev.ml:1085 are +unreachable, not merely unexercised. Retiring them is the author's call over a +lane that merged days ago, not this one's — they are left in place, saying a +true thing about a state nothing can now be in. + +Two rows nearby are about different sites and should not be mistaken for +cover: the =dev-noagent.flan= row asserts the *daemon's own stderr warning*, +said by the accept loop once the ten-second deadline is behind it, and the +late-agent row asserts the note's *absence*. + +** 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-auto.flan b/test/programs/agent-auto.flan new file mode 100644 index 0000000..6ab6374 --- /dev/null +++ b/test/programs/agent-auto.flan @@ -0,0 +1,28 @@ +;;;; The zero-argument (agent/start): a program that names no socket at all. +;;;; +;;;; Under [flan dev] it binds where the daemon said, exactly as the explicit +;;;; form did, and nothing here can tell the difference. Run on its own — which +;;;; is what test_agent.ml does with it — it picks a path under /tmp and prints +;;;; it to stderr, and that printed line is the only way anything could connect. +;;;; +;;;; The second start is here to be a no-op. It answers 0 like the first, does +;;;; not bind a second socket, and does not print a second line; a program that +;;;; got its listener from somewhere else and also asks for one by hand is the +;;;; case that has to keep working. +(import agent "vendor:agent") + +(defvar ticks i64) + +(defn tick [] i64 + (set ticks (+ ticks 1)) + ticks) + +(defn main [] i32 + (if (< (agent/start) 0) + (do (println "cannot listen") 1) + (do + (print (agent/start)) (println "") + (print (tick)) (println "") + (while (= (agent/wait 100) 0) 0) + (print (tick)) (println "") + 0))) 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/agent-start-arity.flan b/test/programs/agent-start-arity.flan new file mode 100644 index 0000000..e8f4941 --- /dev/null +++ b/test/programs/agent-start-arity.flan @@ -0,0 +1,14 @@ +;;;; (agent/start) takes no argument or one, and two is neither. +;;;; +;;;; The optional argument is a macro over two functions, and a macro with a +;;;; [& args] tail will take anything — so the refusal has to come from +;;;; somewhere. It comes from the expansion: every argument is spliced into +;;;; [start-at], which declares one, and the checker refuses the call it was +;;;; actually given. Dropping the extra argument instead would compile this +;;;; file and listen on the first path, which is the silent version of the +;;;; same mistake. +(import agent "vendor:agent") + +(defn main [] i32 + (agent/start "/tmp/one.sock" "/tmp/two.sock") + 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/programs/dev-loop.flan b/test/programs/dev-loop.flan index d92cec1..7fbf933 100644 --- a/test/programs/dev-loop.flan +++ b/test/programs/dev-loop.flan @@ -1,6 +1,12 @@ ;;;; What [flan dev] launches: a program with a loop, a function to redefine, -;;;; and a way to stop. The daemon overrides the socket path through the -;;;; environment, so the one written here is only what it falls back to. +;;;; and a way to stop. +;;;; +;;;; It names no socket. Under [flan dev] the daemon has already decided where +;;;; it wants to talk to this program and says so in FLAN_AGENT_SOCKET — which +;;;; overrode whatever the source named anyway — so the argument was only ever +;;;; a value nothing read. This is the shape a program should be written in, +;;;; and the whole daemon block in test_dev.ml runs against it, so a delivery +;;;; landing there is the zero-argument form working end to end. (import agent "vendor:agent") (defvar ticks i64) @@ -20,7 +26,7 @@ ticks) (defn main [] i32 - (agent/start "/tmp/flan-dev-fallback.sock") + (agent/start) (print (step)) (println "") (while (= (agent/wait 100) 0) 0) (print (step)) (println "") diff --git a/test/programs/dev-noagent-running.flan b/test/programs/dev-noagent-running.flan new file mode 100644 index 0000000..ef750c2 --- /dev/null +++ b/test/programs/dev-noagent-running.flan @@ -0,0 +1,26 @@ +;;;; A program that has not been told about the agent and is still running. +;;;; +;;;; dev-noagent.flan is the other half of this pair and stops short of it: its +;;;; main returns, so a moment later it is parked, and a redefinition sent to a +;;;; parked program is answered by the parking path. This one keeps running, +;;;; which is the state nothing had pinned: there is no agent in the process to +;;;; hand a module to and no socket to fall back on, so the daemon refuses the +;;;; delivery and says which socket it could not reach. +;;;; +;;;; That is the honest answer for it. The sentence [install_note] keeps for a +;;;; running program with no socket — queued, installs at its next +;;;; (agent/poll) — was true of a program that *links* the agent and has not +;;;; reached its (agent/start ...) yet, because there the ring is reachable +;;;; in-process while the socket is not yet bound. The package's constructor +;;;; binds before main now, so that window is gone; see FIX.org. +;;;; +;;;; So: no (import agent ...) anywhere, and a loop that outlasts the test. +(defn step [] i64 7) + +(defn main [] i32 + (print (step)) (println "") + ;; Long enough that the session ends before the program does; short enough + ;; that a daemon killed the hard way leaves nothing running for minutes. + (dotimes [i 3000] + (sleep-seconds 0.01)) + 0) diff --git a/test/test_acceptance.ml b/test/test_acceptance.ml index 542b2f9..b4740b7 100644 --- a/test/test_acceptance.ml +++ b/test/test_acceptance.ml @@ -3720,6 +3720,14 @@ level "1" and 3 are written here"; refuses "a macro that does not settle" "programs/macro-spin.flan" "did not settle after"; + (* The optional argument, refused where it runs out. [(agent/start)] is a + macro with a [& args] tail, so nothing in the parameter list can say + "one at most"; what says it is the expansion, which splices every + argument into a function declaring one. The alternative — taking the + first and dropping the rest — would compile and listen on the first + path, and the extra one would have been a sentence nobody read. *) + refuses "two arguments to (agent/start)" "programs/agent-start-arity.flan" + "agent/start-at takes 1 argument, given 2"; outputs "data types" "programs/datas.flan" datas_out; outputs ~opt:"-O0" "data types, -O0" "programs/datas.flan" datas_out; diff --git a/test/test_agent.ml b/test/test_agent.ml index 7fb1fe0..c6d5a66 100644 --- a/test/test_agent.ml +++ b/test/test_agent.ml @@ -153,6 +153,237 @@ let () = "1\n1000\n1007\n" end; + (* ── (agent/start), with nothing named ──────────────────────────── *) + + (* The zero-argument form, run with no daemon anywhere: no + FLAN_AGENT_SOCKET in the environment, so the program has to choose a + path itself and then say which one. The printed line is not a nicety + here — it is the only thing that makes the socket reachable, and + everything below it in this block is reached *through* that line rather + than through a path the test picked in advance. If the sentence ever + changes shape, the connect fails and this says so. + + stderr and stdout are separate files on purpose. The program's numbers + are its output and the agent's line is not; a program whose stdout is + data would be corrupted by a sentence landing in it, and under [flan + dev] stdout is a pipe the editor reads. Asserting the transcript on fd 1 + is exactly the claim that the line did not go there. + + Three things are pinned at once, and they share a process because they + are the same startup: the path chosen and printed, the *second* + [(agent/start)] being a no-op rather than a second listener, and the + socket file being gone once the program exits. *) + let aout = tmp "auto.out" and aerr = tmp "auto.err" in + let at, al = Session.create ~file:"programs/agent-auto.flan" () in + let aexe = tmp "auto" in + ignore + (Build.executable ~opts:dev ~csrcs:al.Load.csrcs ~lflags:al.Load.lflags + at.Session.host ~out:aexe); + let aso = tmp "auto-tick.so" in + let ac = Session.eval at "(defn tick [] i64 1000)" in + ignore (Build.shared ~opts:dev ~ir:ac.Session.ir ~out:aso ()); + (* The variable this suite sets for every other program here, taken back + out: with it set the zero-argument form binds where it says and prints + nothing, which is the daemon's case and not this one. *) + let aenv = + Array.of_list + (List.filter + (fun kv -> + not (String.length kv >= 18 + && String.sub kv 0 18 = "FLAN_AGENT_SOCKET=")) + (Array.to_list (Unix.environment ()))) + in + let ofd name = + Unix.openfile name [ Unix.O_WRONLY; Unix.O_CREAT; Unix.O_TRUNC ] 0o600 + in + let a1 = ofd aout and a2 = ofd aerr in + let apid = Unix.create_process_env aexe [| aexe |] aenv Unix.stdin a1 a2 in + Unix.close a1; + Unix.close a2; + let said () = In_channel.with_open_bin aerr In_channel.input_all in + let prefix = "flan agent: listening on " in + let announced () = + List.filter + (fun l -> + String.length l > String.length prefix + && String.sub l 0 (String.length prefix) = prefix) + (String.split_on_char '\n' (said ())) + in + if not (await (fun () -> announced () <> [])) then begin + fail "(agent/start) never said where it was listening: %S" (said ()); + (try Unix.kill apid Sys.sigkill with Unix.Unix_error _ -> ()) + end + else begin + let line = List.hd (announced ()) in + let apath = + String.sub line (String.length prefix) + (String.length line - String.length prefix) + in + (* The shape is part of the claim: two programs started at once must not + choose the same file, and the pid alone would not separate two runs of + the same program in sequence. *) + let pid_part = Printf.sprintf "/tmp/flan-agent-%d-" apid in + if not (String.length apath > String.length pid_part + && String.sub apath 0 (String.length pid_part) = pid_part) + then fail "the chosen path is not this process's: %S" apath; + if not (Filename.check_suffix apath ".sock") then + fail "the chosen path is not a socket name: %S" apath; + if not (await (fun () -> Sys.file_exists apath)) then + fail "nothing was bound at the path that was printed: %S" apath + else begin + (* Reached only through the printed line. *) + let r = send apath aso in + if r <> "ok\n" then fail "the announced socket refused a module: %S" r; + let astatus = ref (Unix.WEXITED 0) in + let reaped = + await ~ms:5000 (fun () -> + match Unix.waitpid [ Unix.WNOHANG ] apid with + | 0, _ -> false + | _, s -> astatus := s; true) + in + if not reaped then begin + (try Unix.kill apid Sys.sigkill with Unix.Unix_error _ -> ()); + fail "the program never took the delivery it was sent" + end + else begin + (* 0 is the second [(agent/start)] answering, and it is the whole of + the idempotence claim on this side: had it bound again, the + number would be the same but the socket the test is talking to + would be the older of two. The single announcement below is the + other half — one bind, one sentence. *) + let text = In_channel.with_open_bin aout In_channel.input_all in + if !astatus <> Unix.WEXITED 0 || text <> "0\n1\n1000\n" then + fail "(agent/start)\n got: %S\n wanted: %S" text + "0\n1\n1000\n"; + if List.length (announced ()) <> 1 then + fail "a second (agent/start) announced a second socket: %S" + (said ()); + (* And the file is gone. Nothing else removes it — there is no + daemon here and no directory that belongs to one — so this is the + program's own atexit and nothing else. *) + if Sys.file_exists apath then + fail "the socket outlived the program that bound it: %S" apath + end + end + end; + (* ── A path that cannot be bound ────────────────────────────────── *) + + (* The same program again, told to listen somewhere that does not exist. + Two claims, and the second is the one that cost a bug. + + It fails, and says so. [start_on] answers -1, the [if] in the fixture + takes its other arm, and the process ends 1 with "cannot listen" — which + is what the explicit form has always done for a path it could not bind. + + And the *constructor* failing first does not disarm it. That is the + ordering here: FLAN_AGENT_SOCKET is set, so [auto_start] runs before + main, tries this path and fails — before any Flan code runs, with + nothing to report to. If a failed attempt latched [started], the + program's own [(agent/start)] would then answer 0 with no socket, no + listener and no hooks: success reported for nothing at all, which is + strictly worse than the error it replaced. The transcript below is that + not happening. *) + let bad = tmp "bad.out" in + let bfd' = ofd bad in + let benv = + Array.append aenv [| "FLAN_AGENT_SOCKET=/nonexistent-dir/agent.sock" |] + in + let bpid' = Unix.create_process_env aexe [| aexe |] benv Unix.stdin bfd' bfd' in + Unix.close bfd'; + let bstat = ref (Unix.WEXITED 0) in + let reaped = + await ~ms:5000 (fun () -> + match Unix.waitpid [ Unix.WNOHANG ] bpid' with + | 0, _ -> false + | _, s -> bstat := s; true) + in + if not reaped then begin + (try Unix.kill bpid' Sys.sigkill with Unix.Unix_error _ -> ()); + fail "a program told to listen on an impossible path did not finish" + end + else begin + let text = In_channel.with_open_bin bad In_channel.input_all in + if !bstat <> Unix.WEXITED 1 || text <> "cannot listen\n" then + fail "an impossible socket path\n got: %S (%s)\n wanted: \ + %S (exit 1)" text + (match !bstat with + | Unix.WEXITED c -> Printf.sprintf "exit %d" c + | Unix.WSIGNALED c -> Printf.sprintf "signal %d" c + | Unix.WSTOPPED c -> Printf.sprintf "stopped %d" c) + "cannot listen\n" + end; + + List.iter (fun f -> try Sys.remove f with Sys_error _ -> ()) + [ aexe; aso; aout; aerr; bad ]; + + (* ── 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 @@ -535,6 +766,13 @@ let () = | Unix.WEXITED c -> Printf.sprintf "exit %d" c | Unix.WSIGNALED c -> Printf.sprintf "signal %d" c | Unix.WSTOPPED c -> Printf.sprintf "stopped %d" c) + (* The other way out, and the one that skips atexit on purpose: [abort] + leaves by [_exit] so that it cannot hang on the loader lock, and the + socket is therefore unlinked by hand there. A file left here would + answer the next client with ECONNREFUSED — a program that is there + and refusing, rather than one that died. *) + else if Sys.file_exists lsock then + fail "abort left the agent socket behind: %S" lsock end; List.iter (fun f -> try Sys.remove f with Sys_error _ -> ()) diff --git a/test/test_dev.ml b/test/test_dev.ml index 7d086c9..f9fb4c1 100644 --- a/test/test_dev.ml +++ b/test/test_dev.ml @@ -3856,6 +3856,87 @@ let () = List.iter (fun f -> try Sys.remove f with Sys_error _ -> ()) [ nsock; nlog ]; + (* ── ...and what a delivery to one is told ──────────────────────── + + The row above is about the daemon's own stderr. This one is about the + reply an editor gets for a redefinition, and it is here because that + reply was never pinned and this lane changed which of two it is. + + [install_note] has a sentence for a RUNNING program whose socket is not + bound — queued, installs at its next [(agent/poll)], not at all if there + is never one. It was true of exactly one thing: a merged session whose + program links the agent, so the ring is reachable in-process, but has + not got to its [(agent/start ...)] yet. The constructor closed that + window, so nothing reaches the sentence any more; the late-agent row + below asserts its absence, and FIX.org says the branch can be retired. + + A program that does not link the agent at all never reached it either, + and this row is what says so rather than leaving it to be assumed. There + is no agent in this process to call and no socket to fall back to, so + the delivery is REFUSED — which is the honest answer and not the note: + "queued" would have promised a poll that has nothing to drain. + + It needs the program to be running, which is why it is not folded into + the row above: dev-noagent.flan's main returns, so it parks within the + first moment and a delivery to it is answered by the parking path + instead. This fixture loops. *) + let gsock = tmp "noagent-running.sock" and glog = tmp "noagent-running.log" in + (try Sys.remove gsock with Sys_error _ -> ()); + let gfd = + Unix.openfile glog [ Unix.O_WRONLY; Unix.O_CREAT; Unix.O_TRUNC ] 0o600 + in + let gpid = + Unix.create_process flan + [| flan; "dev"; "programs/dev-noagent-running.flan"; "-s"; gsock |] + Unix.stdin gfd gfd + in + Unix.close gfd; + if not (listening ~pid:gpid gsock) then begin + fail "the running agentless daemon %s" !listen_why; + (try Unix.kill gpid Sys.sigkill with Unix.Unix_error _ -> ()) + end + else begin + let gc = connect gsock in + let r = + request gc + "(:op \"eval\" :code \"(defn step [] i64 9)\" :file \ + \"programs/dev-noagent-running.flan\")" + in + let msg = Option.value ~default:"" (Wire.string_field r "message") in + (* Refused, and the reason names the socket it could not reach rather + than the compiler: the module built, and what failed is the hand-off + to a program that has no agent in it. *) + if status r = "ok" then + fail + "a redefinition for a program with no agent in it was answered \ + ok%s — nothing can install it" + (match Wire.string_field r "note" with + | Some n -> Printf.sprintf " (note: %S)" n + | None -> "") + else if not (contains_sub msg "cannot reach the program on ") then + fail "a delivery to a running agentless program was refused with: %S" + msg; + (* And the session is still there afterwards, which is the rest of the + claim: a refusal is a reply, not the end. *) + (match request gc "(:op \"describe\")" with + | r when status r = "ok" -> () + | r -> + fail "the session did not survive an unreachable delivery: %s" + (status r) + | exception e -> + fail "the session ended on an unreachable delivery: %s" + (Printexc.to_string e)); + (try + ignore (Wire.send gc "(:op \"close\")"); + ignore (Wire.recv gc) + with _ -> ()); + (try Unix.close gc with Unix.Unix_error _ -> ()); + (try Unix.kill gpid Sys.sigkill with Unix.Unix_error _ -> ()); + (try ignore (Unix.waitpid [] gpid) with Unix.Unix_error _ -> ()) + end; + List.iter (fun f -> try Sys.remove f with Sys_error _ -> ()) + [ gsock; glog ]; + (* ── A build that fails is a refusal, not the end of the session ── *) (* Evaluating runs a compiler, and a compiler can fail in ways the @@ -5110,13 +5191,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 +5246,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 +5269,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/agent.flan b/vendor/agent/agent.flan index 34559bb..749eab4 100644 --- a/vendor/agent/agent.flan +++ b/vendor/agent/agent.flan @@ -2,7 +2,8 @@ ;;;; design: loading a redefinition and installing it are separate, because ;;;; only the program knows when it is between frames. ;;;; -;;;; (agent/start path) listen on a unix socket; once, at startup +;;;; (agent/start) listen somewhere sensible; once, at startup +;;;; (agent/start path) listen on that unix socket instead ;;;; (agent/poll) install whatever has arrived; returns how many ;;;; (agent/wait ms) the same, but waits for something first ;;;; @@ -10,12 +11,37 @@ ;;;; wait is for a headless test, where waiting is what makes a reload ;;;; deterministic rather than a race against the frame rate. ;;;; +;;;; The path was the ceremony. Under [flan dev] the daemon has already decided +;;;; where it wants to talk to the program and says so in FLAN_AGENT_SOCKET, so +;;;; whatever the source named was being overridden anyway; outside the daemon +;;;; any path will do as long as the program says which one it picked. So the +;;;; argument is now optional, and the zero-argument form is the one to write. +;;;; The explicit form stays for a program that wants to choose — a fixed path +;;;; something else is already configured to connect to. +;;;; ;;;; No aggregate crosses this boundary, so there is no shim: a Flan string is ;;;; already ptr+len and flan_agent_start takes it that way. -(declare start-raw [path string] i32 "flan_agent_start") +(declare start-at-raw [path string] i32 "flan_agent_start") +(declare start-auto-raw [] i32 "flan_agent_start_auto") (declare poll-raw [] i32 "flan_agent_poll") (declare wait-raw [ms i32] i32 "flan_agent_wait") -(defn start [path string] i32 (start-raw path)) +;;; Both answer 0 for "listening" and -1 for "could not". Starting twice is not +;;; an error and not a second listener: the C side answers 0 and leaves the +;;; first socket alone, so a program that gets a listener from somewhere else +;;; and also calls start is a program with one listener. +(defn start-at [path string] i32 (start-at-raw path)) +(defn start-auto [] i32 (start-auto-raw)) + +;;; The optional argument, which Flan has no other spelling for: a function +;;; takes the arity it declares, so the choice between the two above is made +;;; before the checker ever sees a call. Extra arguments are not silently +;;; dropped — they are spliced into [start-at], which then refuses them by its +;;; own arity, at the call site, in the usual words. +(defmacro start [& args] + (if (= (len args) 0) + `(start-auto) + `(start-at ~@args))) + (defn poll [] i32 (poll-raw)) (defn wait [ms i32] i32 (wait-raw ms)) diff --git a/vendor/agent/flan_agent.c b/vendor/agent/flan_agent.c index e1386b7..5afe532 100644 --- a/vendor/agent/flan_agent.c +++ b/vendor/agent/flan_agent.c @@ -571,6 +571,44 @@ static char condition_name[128]; int32_t flan_agent_poll(void); +/* The path this process actually bound, NUL-terminated, or "" if it never + * bound one. Written once in [start_on] between the successful bind and + * anything that could read it — the listener thread, the atexit handler and + * the orphan signal handler are all arranged after it is set. + * + * It is a copy rather than a [getenv] at use time because one of its readers + * is a signal handler: getenv is not async-signal-safe, and [orphan_die] can + * run on any thread at any instruction. It is also the only record of the path + * when the zero-argument form chose it, where there is no environment variable + * to read back. sun_path's size is the bound because that is where the string + * came from. */ +static char bound_sock[sizeof(((struct sockaddr_un *)0)->sun_path)]; + +/* A socket file outlives the process that bound it, and a stale one answers + * the next client with ECONNREFUSED — which reads like a program that is there + * and refusing rather than one that has gone. So the bind registers its own + * removal, and the three ways out of a program each unlink it: + * + * ordinary exit this handler, via atexit + * break-loop abort die_now, by hand, because it takes _exit + * orphaned child orphan_die, by hand, for the same reason + * + * 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); +} + /* Every way out of the break loop that is not a resume. [_exit] and not * [exit], because this runs on the game thread while the listener thread may * be inside [dlopen] holding the loader lock — and [exit] runs the atexit @@ -595,6 +633,10 @@ static _Noreturn void die_now(void) { const char *sock = getenv("FLAN_DEV_SOCK"); if (sock != NULL && *sock != '\0') unlink(sock); } + /* And this program's own agent socket, for the same reason and skipped by + * the same _exit. Empty unless [start_on] bound one, and then this does + * nothing. */ + unlink_bound_sock(); _exit(134); } @@ -1692,12 +1734,6 @@ static void *accept_loop(void *arg) { #define FLAN_ORPHAN_SIG SIGPWR #endif -/* Stashed at arming time rather than read from the environment in the handler: - * getenv is not async-signal-safe, and this path can run on any thread at any - * instruction. sun_path's size is the bound because that is where it came - * from. */ -static char orphan_sock[sizeof(((struct sockaddr_un *)0)->sun_path)]; - #if defined(FLAN_ORPHAN_SIG) /* Three calls, and the restraint is the point. This runs on whichever thread * the kernel picked, which may be the game thread halfway through a printf, @@ -1725,12 +1761,14 @@ static void orphan_die(int sig) { (void)sig; ignored = write(2, said, sizeof said - 1); (void)ignored; - if (orphan_sock[0] != '\0') unlink(orphan_sock); + if (bound_sock[0] != '\0') unlink(bound_sock); _exit(0); } #endif -static void watch_the_daemon(const char *sock) { +/* Called after the bind: the handler it arms unlinks [bound_sock], so it must + * not be able to fire before that is set. */ +static void watch_the_daemon(void) { #if defined(FLAN_ORPHAN_SIG) const char *want_s = getenv("FLAN_DEV_PARENT"); char *end; @@ -1751,16 +1789,87 @@ static void watch_the_daemon(const char *sock) { sa.sa_handler = orphan_die; sigfillset(&sa.sa_mask); if (sigaction(FLAN_ORPHAN_SIG, &sa, NULL) != 0) return; - strncpy(orphan_sock, sock, sizeof orphan_sock - 1); if (prctl(PR_SET_PDEATHSIG, FLAN_ORPHAN_SIG) != 0) return; /* The race, closed. Armed above, so from here on the kernel will tell us; * this asks whether it already should have. */ if ((long)getppid() != want) raise(FLAN_ORPHAN_SIG); -#else - (void)sock; #endif } +/* Bind [path], start the listener, and arrange the two things that depend on + * having bound: the socket's removal and the orphan watch. + * + * Three answers, not two, because the callers below need to tell "bound it + * just now" from "somebody already had": 1 is already started, 0 is bound + * here, -1 could not listen. Only the caller that chose the path itself cares, + * and it cares because it is the one that prints it. + * + * Starting twice is a no-op answering the first socket. That is the whole of + * the idempotence the zero-argument form needs: a program that gets the + * listener for free under [flan dev] and *also* writes [(agent/start ...)] of + * its own must not end up with two listeners, and the second call is the one + * that has to give way — the first is the one whose socket the daemon is + * already talking to. + * + * [started] is what makes that true, and it is claimed at the top because two + * threads may arrive at once. A FAILURE MUST THEREFORE GIVE IT BACK. This is + * not tidiness: the constructor runs before main and does not report anything, + * so a latched flag would mean an unbindable path silently disarms every later + * start — the program's own [(agent/start ...)] would answer 0 with no socket, + * no listener and no hooks, which is worse than the error it replaced. So + * every way out below that is not a listening socket puts it back and answers + * -1, and a second attempt is a real attempt. */ +static int32_t start_on(const char *path) { + struct sockaddr_un addr; + size_t len; + int fd = -1; + /* Whether the bind got as far as making the file, which is what has to be + * taken away again on a later failure. Nothing else knows it is there: it is + * not in [bound_sock] yet, so no handler would remove it. */ + int made = 0; + if (atomic_exchange(&started, 1)) return 1; + len = strlen(path); + if (len == 0 || len >= sizeof addr.sun_path) goto failed; + memset(&addr, 0, sizeof addr); + addr.sun_family = AF_UNIX; + memcpy(addr.sun_path, path, len); + unlink(addr.sun_path); + fd = socket(AF_UNIX, SOCK_STREAM, 0); + if (fd < 0) goto failed; + if (bind(fd, (struct sockaddr *)&addr, sizeof addr) < 0) goto failed; + made = 1; + if (listen(fd, 4) < 0) goto failed; + /* Published together: the fd the accept loop reads and the path the three + * exits unlink. Both before the listener thread and before either handler, + * so that nothing which uses them can run while they are still empty. */ + listen_fd = fd; + memcpy(bound_sock, addr.sun_path, len + 1); + atexit(unlink_bound_sock); + if (pthread_create(&listener, NULL, accept_loop, NULL) != 0) { + /* Published above and now taken back, in the reverse order. The atexit + stays registered — there is no way to withdraw one — and an empty + [bound_sock] is what makes it a no-op. */ + bound_sock[0] = '\0'; + listen_fd = -1; + goto failed; + } + /* After the bind, because a program that is going to fail to listen should + fail on its own terms rather than arrange its death first. */ + watch_the_daemon(); + /* From here an unhandled error stops rather than dying. Installed with the + * socket and not before it: without a listener there is nobody to ask what + * to do, and stopping forever is worse than the abort it replaces. */ + flan_break_hook = break_loop; + flan_trap_hook = trap_stop; + return 0; + +failed: + if (made) unlink(addr.sun_path); + if (fd >= 0) close(fd); + atomic_store(&started, 0); + return -1; +} + /* [path] is a Flan string: ptr and len, not NUL-terminated. * * FLAN_AGENT_SOCKET overrides it. A program's source has to name some path, @@ -1769,32 +1878,85 @@ static void watch_the_daemon(const char *sock) { * and guessing wrong fails silently: everything compiles, the module is built, * and nothing ever receives it. */ int32_t flan_agent_start(const uint8_t *path, int64_t len) { - struct sockaddr_un addr; + char buf[sizeof(((struct sockaddr_un *)0)->sun_path)]; const char *env = getenv("FLAN_AGENT_SOCKET"); - if (atomic_exchange(&started, 1)) return 0; - if (env != NULL && env[0] != '\0') { - path = (const uint8_t *)env; - len = (int64_t)strlen(env); - } - if (len <= 0 || (size_t)len >= sizeof addr.sun_path) return -1; - memset(&addr, 0, sizeof addr); - addr.sun_family = AF_UNIX; - memcpy(addr.sun_path, path, (size_t)len); - unlink(addr.sun_path); - listen_fd = socket(AF_UNIX, SOCK_STREAM, 0); - if (listen_fd < 0) return -1; - if (bind(listen_fd, (struct sockaddr *)&addr, sizeof addr) < 0) return -1; - if (listen(listen_fd, 4) < 0) return -1; - if (pthread_create(&listener, NULL, accept_loop, NULL) != 0) return -1; - /* After the bind, because the path this hands over is the one that was - actually bound — the environment's override included — and because a - program that is going to fail to listen should fail on its own terms - rather than arrange its death first. */ - watch_the_daemon(addr.sun_path); - /* From here an unhandled error stops rather than dying. Installed with the - * socket and not before it: without a listener there is nobody to ask what - * to do, and stopping forever is worse than the abort it replaces. */ - flan_break_hook = break_loop; - flan_trap_hook = trap_stop; - return 0; + if (env != NULL && env[0] != '\0') return start_on(env) < 0 ? -1 : 0; + if (len <= 0 || (size_t)len >= sizeof buf) return -1; + memcpy(buf, path, (size_t)len); + buf[len] = '\0'; + return start_on(buf) < 0 ? -1 : 0; +} + +/* The zero-argument form: (agent/start), with nowhere named. + * + * Under [flan dev] there is a right answer and the daemon has already written + * it down — the same FLAN_AGENT_SOCKET the explicit form honours — so this + * binds there and says nothing. The daemon knows where it put the program's + * socket; a line about it would only go down the pipe the editor reads. + * + * Outside the daemon there is no right answer, so it invents one that no other + * program will collide with and prints it, because a socket nobody can name is + * a socket nobody can connect to. The pid makes it this process's; the clock + * makes a second run of the same program a different path rather than one that + * silently reuses a file it may not own. stderr rather than stdout, so a + * program whose output is data stays data. */ +int32_t flan_agent_start_auto(void) { + char path[sizeof(((struct sockaddr_un *)0)->sun_path)]; + struct timespec ts; + int32_t r; + const char *env = getenv("FLAN_AGENT_SOCKET"); + if (env != NULL && env[0] != '\0') return start_on(env) < 0 ? -1 : 0; + if (clock_gettime(CLOCK_REALTIME, &ts) != 0) ts.tv_nsec = 0; + snprintf(path, sizeof path, "/tmp/flan-agent-%ld-%08lx.sock", + (long)getpid(), (unsigned long)(ts.tv_nsec & 0xffffffffL)); + r = start_on(path); + /* Only when this call is the one that bound: a second [(agent/start)] would + * otherwise print a path it did not bind and nothing is listening on. */ + if (r == 0) { + fprintf(stderr, "flan agent: listening on %s\n", path); + fflush(stderr); + } + 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; + /* The answer is dropped because there is nobody to give it to: this is ELF + * init, before main, before the program has decided anything. What matters + * is that a failure here is not final — [start_on] gives [started] back, so + * a program's own [(agent/start ...)] still tries for itself and still says + * -1 if it cannot listen either. A daemon that named a path nothing can bind + * finds out the way it always did, from the program. */ + (void)start_on(env); } diff --git a/vendor/agent/flan_agent.web.c b/vendor/agent/flan_agent.web.c index 7745284..49332ef 100644 --- a/vendor/agent/flan_agent.web.c +++ b/vendor/agent/flan_agent.web.c @@ -64,6 +64,12 @@ int32_t flan_agent_start(const uint8_t *path, int64_t len) { return -1; } +/* The zero-argument form, which on a native build picks a socket path for + * itself. There is still nothing to bind here, so it answers exactly what the + * explicit form does and prints nothing: a path chosen for a socket that + * cannot exist would be a sentence about nothing. */ +int32_t flan_agent_start_auto(void) { return -1; } + int32_t flan_agent_poll(void) { return 0; } int32_t flan_agent_wait(int32_t ms) { (void)ms; return 0; }