Skip to content

Exit STM workers quietly on lost master connection (flaky output/Partac.v on Windows CI) #22286

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JasonGross wants to merge 1 commit into
base: master
Choose a base branch
from
+18 −0
Open

Exit STM workers quietly on lost master connection (flaky output/Partac.v on Windows CI) #22286

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions stm/asyncTaskQueue.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,20 @@ module Make(T : Task) () = struct
let slave_handshake () =
Pool.worker_handshake (Option.get !slave_ic) (Option.get !slave_oc)

(* The master going away while we read from or write to it is not
always observed as [End_of_file]: reads can fail with a connection
reset (in particular on Windows, where the master closing the
socket resets it) and writes with a broken pipe, both showing up
as [Sys_error] with the corresponding strerror text. *)
let connection_lost msg =
let contains what = CString.string_contains ~where:msg ~what in
contains "Connection reset by peer" ||
contains "Broken pipe" ||
(* WSAECONNRESET *)
contains "forcibly closed by the remote host" ||
(* WSAECONNABORTED *)
contains "aborted by the software in your host machine"

let pp_pid pp = Pp.(str (Spawned.process_id () ^ " ") ++ pp)

let debug_with_pid = Feedback.(function
Expand Down Expand Up @@ -309,10 +323,14 @@ module Make(T : Task) () = struct
marshal_response (Option.get !slave_oc) response;
CEphemeron.clean ()
with
| MarshalError s when connection_lost s ->
stm_prerr_endline "connection lost"; flush_all (); exit 2

@gares gares Jul 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here as well, I don't see the advantage of printing "connection lost" over "marshal error" and returning the same error code. please remove this hunk.

@JasonGross JasonGross Jul 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the alternative is adapting the test-suite runner to filter out these sorts of messages so that the test involving par is less flaky on Windows? Or adding support for expect tests with multiple alternatives and adding all the various alternatives we might see? I'll have Fable prepare two alternate PRs for each of these strategies, we can see which one is better

@SkySkimmer SkySkimmer Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how printing "connection lost" is less flaky than printing "marshall error whatever"

@SkySkimmer SkySkimmer Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid spamming PRs, you can just link to your branch instead

@JasonGross JasonGross Jul 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how printing "connection lost" is less flaky than printing "marshall error whatever"

My understanding of the flakiness is that the behavior of the test is always the same, but the error message varies depending on thread interleaving. (Does this seem correct?)

Please avoid spamming PRs, you can just link to your branch instead

Apologies, and noted for the future.

@SkySkimmer SkySkimmer Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that sometimes we get the message and sometimes we don't

@gares gares Jul 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if there is a test depending on the string, we can change it depending on the error code. That is fine to me.

| MarshalError s ->
stm_pr_err Pp.(prlist str ["Fatal marshal error: "; s]); flush_all (); exit 2
| End_of_file ->
stm_prerr_endline "connection lost"; flush_all (); exit 2
| Sys_error s when connection_lost s ->

@gares gares Jul 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grepping the error message is below my standards, also because these things are translated using LOCALE.

Just catch any Sys_error.

stm_prerr_endline "connection lost"; flush_all (); exit 2
| e ->
stm_pr_err Pp.(seq [str "Slave: critical exception: "; print e]);
flush_all (); exit 1
Expand Down
Loading