diff --git a/emacs/flan-dev.el b/emacs/flan-dev.el index fdb44ca..6f0b0ce 100644 --- a/emacs/flan-dev.el +++ b/emacs/flan-dev.el @@ -405,9 +405,21 @@ Set to nil to leave the program's state to whatever replies happen to say." (error (setq flan-dev--connection nil) (force-mode-line-update t) - (error "flan dev: cannot reconnect to %s: %s" - (abbreviate-file-name flan-dev--socket) - (error-message-string err))))))) + ;; A refusal on a path that exists is a socket file outliving the + ;; process that bound it -- a daemon killed outright rather than one + ;; that ended. `flan dev' removes its own socket on every way out it + ;; controls, so reaching here means it was killed, and the raw + ;; "Connection refused" is the least useful thing that could be said + ;; about that: it reads as a daemon that is there and declining. Two + ;; investigations in this repository have started from that reading + ;; and gone the wrong way. + (if (string-match-p "[Cc]onnection refused" (error-message-string err)) + (error "flan dev: %s is a leftover socket -- whatever bound it is \ +gone; remove it and start `flan dev program.flan' again" + (abbreviate-file-name flan-dev--socket)) + (error "flan dev: cannot reconnect to %s: %s" + (abbreviate-file-name flan-dev--socket) + (error-message-string err)))))))) flan-dev--connection) ;;;###autoload diff --git a/lib/dev.ml b/lib/dev.ml index 9057471..a515446 100644 --- a/lib/dev.ml +++ b/lib/dev.ml @@ -2659,6 +2659,22 @@ static void flan_merged_exit(int32_t status) { for (;;) pause(); } +/* The socket is this process's to remove, and on every way out of it and not + * only the tidy one. A merged daemon that dies through a runtime trap -- a + * bounds failure, an unhandled condition, an abort taken at the break loop -- + * leaves the path on disk with nothing behind it, and the next client's + * connect is then ECONNREFUSED: a socket that plainly exists, refusing. That + * message has already sent two investigations in this repository to the wrong + * place. Gone is the honest state, and the client already has words for it. + * + * [atexit] covers exit(3), which is what flan_rt.c's rt_die uses. _exit and + * abort skip it by design, so the places that take those routes unlink for + * themselves; see die_now in flan_agent.c. */ +static void flan_merged_unlink_sock(void) { + const char *s = getenv("FLAN_DEV_SOCK"); + if (s != NULL && *s != '\0') unlink(s); +} + static char **g_argv; /* Atomic, not a plain int: this is the only happens-before edge between the * two threads, and everything the compiler set up before it — the listening @@ -2698,6 +2714,7 @@ int main(int argc, char **argv) { pthread_t compiler; int rc; g_argv = argv; + atexit(flan_merged_unlink_sock); flan_exit_hook = flan_merged_exit; if (pthread_create(&compiler, NULL, flan_merged_compiler, NULL) != 0) { fprintf(stderr, "flan dev: could not start the compiler thread\n"); @@ -2723,6 +2740,8 @@ int main(int argc, char **argv) { fprintf(stderr, "flan dev: the program returned %d; the session ends with it\n", rc); fflush(NULL); + /* By hand, because _exit does not run the atexit chain registered above. */ + flan_merged_unlink_sock(); _exit(rc); } |c} diff --git a/test/programs/dev-repl.flan b/test/programs/dev-repl.flan index d53e345..e9fa843 100644 --- a/test/programs/dev-repl.flan +++ b/test/programs/dev-repl.flan @@ -18,7 +18,16 @@ (defn main [] i32 (agent/start "/tmp/flan-dev-repl-fallback.sock") - (dotimes [i 4000] + ;; 24000 and not the 4000 the other fixtures use, because this one is the + ;; whole of test_emacs: one daemon carries every check from `flan-connect' to + ;; the break loop, and 4000 ticks of 5ms is a two-minute test with a + ;; twenty-second program in it. Run the suite on a machine with several + ;; compilers on it and the program can reach its last tick while the client + ;; is still working — which fails honestly ("the program exited; restart flan + ;; dev") but fails for a reason that has nothing to do with what was being + ;; checked. dev-robust.flan is 24000 for the same reason and got there first. + ;; The watchdogs, at 600s, are what actually bounds the run. + (dotimes [i 24000] (agent/wait 5) (set ticks (step))) 0) diff --git a/test/test_repl.ml b/test/test_repl.ml index 9486d49..5b0c847 100644 --- a/test/test_repl.ml +++ b/test/test_repl.ml @@ -339,12 +339,51 @@ let () = ignore (request c "(:op \"close\")"); Unix.close c end; - (try Unix.kill pid Sys.sigterm with Unix.Unix_error _ -> ()); - (try ignore (Unix.waitpid [] pid) with Unix.Unix_error _ -> ()); + (* Asked before it is told: a daemon that has already died says so in its + wait status, and that is the difference between "the test's last request + was wrong" and "the session was not there to ask". The kill below then + has nothing to do. See test_emacs.ml, where this was the fact two + diagnoses of the same flake were missing. *) + let died = + match Unix.waitpid [ Unix.WNOHANG ] pid with + | 0, _ -> + (try Unix.kill pid Sys.sigterm with Unix.Unix_error _ -> ()); + (try ignore (Unix.waitpid [] pid) with Unix.Unix_error _ -> ()); + None + | _, st -> Some st + | exception Unix.Unix_error _ -> None + in + (* Read before the removal, because on a failure this is the evidence. *) + let prog_out = + if !failures = 0 then "" + else + match open_in_bin out with + | ic -> + let n = in_channel_length ic in + let want = min n 4000 in + seek_in ic (n - want); + let s = really_input_string ic want in + close_in ic; + s + | exception Sys_error _ -> "" + in List.iter (fun f -> try Sys.remove f with Sys_error _ -> ()) [ sock; out ]; if !failures = 0 then print_endline "repl: all tests passed" else begin Printf.printf "\n%d failure(s)\n" !failures; + (match died with + | Some (Unix.WEXITED n) -> + Printf.printf "the daemon had already exited with status %d\n" n + | Some (Unix.WSIGNALED n) when n = Sys.sigpipe -> + print_endline + "the daemon had already been killed by SIGPIPE — a reply written \ + into a socket whose reader had gone" + | Some (Unix.WSIGNALED n) -> + Printf.printf "the daemon had already been killed by signal %d\n" n + | Some (Unix.WSTOPPED n) -> + Printf.printf "the daemon was stopped on signal %d\n" n + | None -> print_endline "the daemon was still running at the end"); + Printf.printf "\n-- the program's own output, last 4k --\n%s\n" prog_out; exit 1 end | _ -> print_endline "repl: skipped (no clang or llc on PATH)" diff --git a/vendor/agent/flan_agent.c b/vendor/agent/flan_agent.c index 374ed41..b1868b8 100644 --- a/vendor/agent/flan_agent.c +++ b/vendor/agent/flan_agent.c @@ -463,6 +463,17 @@ int32_t flan_agent_poll(void); static _Noreturn void die_now(void) { fflush(stdout); fflush(stderr); + /* The editor's socket, by hand. In a merged `flan dev' this process is the + * listener as well as the program, and _exit skips the atexit handler that + * would otherwise remove it -- deliberately, for the loader-lock reason + * above. A socket file left behind with nothing accepting on it answers the + * next client with ECONNREFUSED, which reads like a daemon that is there and + * refusing rather than one that died. FLAN_DEV_SOCK is unset in an ordinary + * run, and then this does nothing. */ + { + const char *sock = getenv("FLAN_DEV_SOCK"); + if (sock != NULL && *sock != '\0') unlink(sock); + } _exit(134); }