From e9c0e96a9bb1cec9a34709be42b174d7f29d3705 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sun, 20 Sep 2026 19:44:34 +0700 Subject: [PATCH 1/3] (agent/start) takes no argument, and takes the socket away after itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The path was ceremony. Under [flan dev] the daemon already decides where it wants to talk to the program and writes it into FLAN_AGENT_SOCKET, which the C side has always honoured over whatever the source named — so the argument was a value nothing read. Outside the daemon any path will do as long as the program says which one it picked. So [start] becomes a macro over two functions: no argument picks the daemon's socket if there is one and otherwise /tmp/flan-agent--.sock, announced on stderr because a socket nobody can name is a socket nobody can connect to. The explicit form stays for a program that wants a fixed path. Extra arguments are spliced into [start-at] rather than dropped, so the arity refusal is still the checker's, at the call site. And the socket is removed on the way out. The bind stashes the path it bound and registers an atexit; the two paths that leave by _exit — the break loop's [abort] and the orphan handler — unlink it by hand, as the orphan handler already did for its own copy of the path. Nothing else takes it away: under the daemon it sits in a temp directory that is still never removed (FIX.org), and outside there is no daemon at all. test/programs/dev-loop.flan now names no socket, which puts the whole daemon block in test_dev.ml behind the zero-argument form; agent-auto.flan is the standalone half, reached only through the line the program printed, and it pins the second (agent/start) as a no-op and the socket as gone at exit. --- test/programs/agent-auto.flan | 28 ++++++ test/programs/dev-loop.flan | 12 ++- test/test_agent.ml | 123 ++++++++++++++++++++++++++ vendor/agent/agent.flan | 32 ++++++- vendor/agent/flan_agent.c | 158 ++++++++++++++++++++++++++-------- vendor/agent/flan_agent.web.c | 6 ++ 6 files changed, 315 insertions(+), 44 deletions(-) create mode 100644 test/programs/agent-auto.flan 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/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/test_agent.ml b/test/test_agent.ml index 7fb1fe0..3f4131d 100644 --- a/test/test_agent.ml +++ b/test/test_agent.ml @@ -153,6 +153,122 @@ 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; + List.iter (fun f -> try Sys.remove f with Sys_error _ -> ()) + [ aexe; aso; aout; aerr ]; + (* ── The break loop, spec-conditions.md §2 ──────────────────────── *) (* The claim is that an unhandled [error] stops rather than dying, and can @@ -535,6 +651,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/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..26ba7d8 100644 --- a/vendor/agent/flan_agent.c +++ b/vendor/agent/flan_agent.c @@ -571,6 +571,36 @@ 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 + * + * 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. */ +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 +625,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 +1726,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 +1753,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 +1781,57 @@ 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. */ +static int32_t start_on(const char *path) { + struct sockaddr_un addr; + size_t len; + if (atomic_exchange(&started, 1)) return 1; + len = strlen(path); + if (len == 0 || len >= sizeof addr.sun_path) return -1; + memset(&addr, 0, sizeof addr); + addr.sun_family = AF_UNIX; + memcpy(addr.sun_path, path, 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; + /* Before the listener thread and before either handler, so that nothing + * which unlinks it can run while it is still empty. */ + memcpy(bound_sock, addr.sun_path, len + 1); + atexit(unlink_bound_sock); + if (pthread_create(&listener, NULL, accept_loop, NULL) != 0) return -1; + /* 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; +} + /* [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 +1840,43 @@ 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; } 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; } From 77d6e43b096751b21e4ba5e370e41ebe060bf0fa Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sun, 20 Sep 2026 19:52:34 +0700 Subject: [PATCH 2/3] The agent binds before main, so a dev program need not start it at all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A constructor in the agent package binds FLAN_AGENT_SOCKET when it is set, which is the daemon and nothing else — both shapes set it, before the fork in --two-process and before the exec in the merged build. So a program under [flan dev] that calls (agent/poll) and has no (agent/start) in it takes redefinitions anyway, and one that does call start meets an agent that is already listening and gets a no-op. The window this closes was the complaint in DISCUSS.org: a program that opens a window before starting its agent leaves the daemon waiting on a socket that does not exist yet. Bound here, the socket exists before main whatever the program does afterwards — so test_dev.ml's late-agent row asserts the negation of what it used to. The delivery sent during the sleep no longer carries "the program has not called (agent/start ...) yet", because that is no longer true of it; what is still late, and still asserted, is the poll that installs it. It reaches exactly as far as the linker does. Reach prunes a package nothing calls into, so a program that mentions the agent nowhere does not link this file and has no constructor to run: auto-start is for a program that polls and has dropped its start call, not for one that says nothing about the agent at all. That limit and the release-build residual are in FIX.org, along with the daemon branch that can no longer be reached. agent-nostart.flan is the pin, and its two numbers are the honest ones: 1 before anything could arrive, 1000 after the wait, because a listener bound before main is still not an install. --- FIX.org | 72 ++++++++++++++++++++++++++++++++ test/programs/agent-nostart.flan | 27 ++++++++++++ test/programs/dev-lateagent.flan | 6 +++ test/test_agent.ml | 68 ++++++++++++++++++++++++++++++ test/test_dev.ml | 55 +++++++++++++++++------- vendor/agent/flan_agent.c | 52 +++++++++++++++++++++-- 6 files changed, 260 insertions(+), 20 deletions(-) create mode 100644 test/programs/agent-nostart.flan 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); +} From 38a570bc7a6bea6d382cfb4148232c0e34ccfc39 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sun, 20 Sep 2026 20:10:39 +0700 Subject: [PATCH 3/3] A bind that fails gives the flag back, and three sentences get pinned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bug review found: [start_on] claimed [started] at the top and every failure exit left it claimed. Under [flan dev] the constructor is the first caller and reports to nobody, so a path nothing could bind disarmed the program's own (agent/start ...) as well — it answered 0 with no socket, no listener and no hooks, where before this lane the explicit form answered -1. Success reported for nothing at all is worse than the error it replaced. So every way out that is not a listening socket unwinds: the fd is closed, a file the bind managed to make is unlinked, and [started] goes back to 0 so a later start is a real attempt. Pinned by running the zero-argument fixture with FLAN_AGENT_SOCKET pointing nowhere — constructor fails silently, main's own call then fails loudly, "cannot listen" and exit 1. Two arguments to (agent/start) are refused, which nothing held: the macro's [& args] cannot say "one at most", so what says it is the expansion splicing every argument into a function that declares one. The message names agent/start-at and carries the expanded-from note, and that is what the acceptance row asserts. And the reply a delivery gets when there is no agent in the process, which nothing held either. dev-noagent.flan parks, so it was never this case; dev-noagent-running.flan keeps running, and the answer is a refusal naming the socket that could not be reached — not install_note's "queued", which would promise a poll with nothing to drain. Which leaves that note unreachable in all three shapes rather than merely unpinned, worked through in FIX.org. FIX.org also now says what an exported FLAN_AGENT_SOCKET would do: start_on unlinks before it binds, so an agent-linked program started in that environment takes the path away from whoever bound it first. --- FIX.org | 47 +++++++++++++-- test/programs/agent-start-arity.flan | 14 +++++ test/programs/dev-noagent-running.flan | 26 +++++++++ test/test_acceptance.ml | 8 +++ test/test_agent.ml | 49 +++++++++++++++- test/test_dev.ml | 81 ++++++++++++++++++++++++++ vendor/agent/flan_agent.c | 54 ++++++++++++++--- 7 files changed, 263 insertions(+), 16 deletions(-) create mode 100644 test/programs/agent-start-arity.flan create mode 100644 test/programs/dev-noagent-running.flan diff --git a/FIX.org b/FIX.org index 01739b8..46b2d74 100644 --- a/FIX.org +++ b/FIX.org @@ -2486,6 +2486,20 @@ 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 @@ -2493,12 +2507,33 @@ 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. +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 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-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 e982445..c7c27af 100644 --- a/test/test_acceptance.ml +++ b/test/test_acceptance.ml @@ -3648,6 +3648,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 be64f72..c6d5a66 100644 --- a/test/test_agent.ml +++ b/test/test_agent.ml @@ -266,8 +266,55 @@ let () = 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 ]; + [ aexe; aso; aout; aerr; bad ]; (* ── No (agent/start) at all ────────────────────────────────────── *) diff --git a/test/test_dev.ml b/test/test_dev.ml index ca139be..3a73378 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 diff --git a/vendor/agent/flan_agent.c b/vendor/agent/flan_agent.c index 05914a6..5afe532 100644 --- a/vendor/agent/flan_agent.c +++ b/vendor/agent/flan_agent.c @@ -1809,26 +1809,50 @@ static void watch_the_daemon(void) { * 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. */ + * 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) return -1; + 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); - 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; - /* Before the listener thread and before either handler, so that nothing - * which unlinks it can run while it is still empty. */ + 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) return -1; + 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(); @@ -1838,6 +1862,12 @@ static int32_t start_on(const char *path) { 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. @@ -1922,5 +1952,11 @@ int32_t flan_agent_start_auto(void) { __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); }