A bind that fails gives the flag back, and three sentences get pinned
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.
This commit is contained in:
parent
77d6e43b09
commit
38a570bc7a
47
FIX.org
47
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
|
||||
|
||||
14
test/programs/agent-start-arity.flan
Normal file
14
test/programs/agent-start-arity.flan
Normal file
@ -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)
|
||||
26
test/programs/dev-noagent-running.flan
Normal file
26
test/programs/dev-noagent-running.flan
Normal file
@ -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)
|
||||
@ -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;
|
||||
|
||||
@ -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 ────────────────────────────────────── *)
|
||||
|
||||
|
||||
@ -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
|
||||
|
||||
54
vendor/agent/flan_agent.c
vendored
54
vendor/agent/flan_agent.c
vendored
@ -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);
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user