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); }