Skip to content

libstore: keep reading the daemon's messages while blocked sending to it - #76

Merged
joshheinrichs-shopify merged 1 commit into
mainfrom
remote-store-drain-while-sending
Oct 8, 2026
Merged

joshheinrichs-shopify merged 1 commit into
mainfrom
remote-store-drain-while-sending

Conversation

@joshheinrichs-shopify

Copy link
Copy Markdown
Contributor

nix copy into the daemon streams the paths over the daemon socket in frames, and only reads the daemon's stderr messages between frames (FramedSink::checkError). Meanwhile the daemon's addToStore can start an auto-GC and wait for it, and that GC logs every path it deletes to the client through the same socket, synchronously (TunnelLogger::enqueueMsg). Once both socket buffers are full the client is stuck in write(), the GC is stuck in write(), and the daemon is waiting for the GC: a deadlock that also holds the GC lock and a path lock, so every later addToStore on that host queues behind it. About 80 log lines are enough on macOS (8 KiB socket buffers); Linux needs more (about 208 KiB), but a GC freeing hundreds of GB has them.

Fix it on the client side: route withFramedSink's writes through a sink that, whenever a write would block, polls the connection for input as well and processes the daemon's messages before retrying. writeFullWhileDraining does the non-blocking write and poll. It sets O_NONBLOCK only while writing, and clears it around the drain callback, because the socket's read side shares the flag and processStderr relies on blocking reads. (MSG_DONTWAIT would avoid the toggling, but macOS ignores it on AF_UNIX stream sockets.)

Why poll() rather than a thread: a reader thread is how the client did this until upstream 2.25, and how runProgram2 still feeds a child's stdin. It was removed here in 39daa4a because nix flake show created 46k of them (9% of its runtime), and that removal is what made this a deadlock. Bringing it back only for addMultipleToStore would be cheap, but single-path addToStore goes through the same sink and deadlocks the same way as soon as a path outgrows the socket buffer. Polling both directions is the codebase's other idiom for this (retryOnBlock, MonitorFdHup, the build Worker), and it covers every caller at no measurable cost: copying 1 GiB into the daemon on macOS takes the same time and CPU as before, and the per-write cost is three fcntl() calls.

Why not the daemon: there this is a class of bug, not a spot. TunnelLogger writes to the client synchronously from whichever thread logs, so the auto-GC thread blocks on the client while addToStore waits for the GC; a client-requested GC (CollectGarbage) logs every deleted path the same way while holding the global GC lock; path locks are held while reading upload data from the client; and no client I/O or lock wait has a timeout. Fixing that properly is a daemon-wide change that belongs upstream. The client fix is self-contained, works against every daemon version (the builders that hit this run an upstream daemon; only their nix copy comes from this repo), and restores the invariant the client had before 2.25: it never stops reading while the daemon may be writing.

The functional test reproduces the hang: a daemon told it is nearly out of space, 2000 garbage paths with long names, and a 20 MB path copied in from a binary cache. Without the fix the client blocks forever (and ignores SIGTERM, as seen on the builders).

`nix copy` into the daemon streams the paths over the daemon socket in
frames, and only reads the daemon's stderr messages between frames
(`FramedSink::checkError`). Meanwhile the daemon's `addToStore` can
start an auto-GC and wait for it, and that GC logs every path it
deletes to the client through the same socket, synchronously
(`TunnelLogger::enqueueMsg`). Once both socket buffers are full the
client is stuck in write(), the GC is stuck in write(), and the daemon
is waiting for the GC: a deadlock that also holds the GC lock and a
path lock, so every later addToStore on that host queues behind it.
About 80 log lines are enough on macOS (8 KiB socket buffers); Linux
needs more (about 208 KiB), but a GC freeing hundreds of GB has them.

Fix it on the client side: route `withFramedSink`'s writes through a
sink that, whenever a write would block, polls the connection for
input as well and processes the daemon's messages before retrying.
`writeFullWhileDraining` does the non-blocking write and poll. It sets
O_NONBLOCK only while writing, and clears it around the drain callback,
because the socket's read side shares the flag and `processStderr`
relies on blocking reads. (MSG_DONTWAIT would avoid the toggling, but
macOS ignores it on AF_UNIX stream sockets.)

Why poll() rather than a thread: a reader thread is how the client did
this until upstream 2.25, and how `runProgram2` still feeds a child's
stdin. It was removed here in 39daa4a because `nix flake show`
created 46k of them (9% of its runtime), and that removal is what made
this a deadlock. Bringing it back only for `addMultipleToStore` would
be cheap, but single-path `addToStore` goes through the same sink and
deadlocks the same way as soon as a path outgrows the socket buffer.
Polling both directions is the codebase's other idiom for this
(`retryOnBlock`, `MonitorFdHup`, the build `Worker`), and it covers
every caller at no measurable cost: copying 1 GiB into the daemon on
macOS takes the same time and CPU as before, and the per-write cost is
three fcntl() calls.

Why not the daemon: there this is a class of bug, not a spot.
`TunnelLogger` writes to the client synchronously from whichever thread
logs, so the auto-GC thread blocks on the client while `addToStore`
waits for the GC; a client-requested GC (`CollectGarbage`) logs every
deleted path the same way while holding the global GC lock; path locks
are held while reading upload data from the client; and no client I/O
or lock wait has a timeout. Fixing that properly is a daemon-wide
change that belongs upstream. The client fix is self-contained, works
against every daemon version (the builders that hit this run an
upstream daemon; only their `nix copy` comes from this repo), and
restores the invariant the client had before 2.25: it never stops
reading while the daemon may be writing.

The functional test reproduces the hang: a daemon told it is nearly
out of space, 2000 garbage paths with long names, and a 20 MB path
copied in from a binary cache. Without the fix the client blocks
forever (and ignores SIGTERM, as seen on the builders).

@jacobmichels jacobmichels left a comment

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.

🚀

@joshheinrichs-shopify
joshheinrichs-shopify merged commit a53a872 into main Oct 8, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants