Repository navigation
fix(daemon): wait for a busy store off the runtime threads - #111
Merged
Merged
Conversation
A runtime thread that waits on the store's mutex can leave tokio's timer and I/O driver unpolled: a worker that wakes from parking on the driver and then blocks hands it to nobody, and the idle workers sleep on their condvars until something wakes them (the eager handoff that fixes this is behind tokio_unstable). With a slow request holding the store on the blocking pool, the courier waking on its timer was enough to stall every timer and socket in the daemon: GET /health, which never touches the store, waited about 950 ms of a 1 s hold. Daemon::store() now tries the lock first and, when it is taken on a multi-thread runtime, waits inside block_in_place, which moves the thread's other work to a new one. Blocking-pool threads, plain threads and current-thread test runtimes keep waiting as before. Spec 11.1 says so. tests/store_wait.rs holds the store for 1 s: /health fails without the fix and passes with it; live text already went out on its 40 ms timer, because the pump's own wake parks a worker that picks the driver up again.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #108. There, a
tokio::time::sleep(200 ms)only woke after a plain thread let go of the store, about 1 s later. This PR checks whether that happens in the real daemon and fixes it.Cause
Tokio's timer and I/O driver is polled only by a worker parked on it. When that worker wakes, for example the courier's poll timer firing, and runs a task that blocks on the store's
std::sync::Mutex, the driver goes to nobody. The other workers stay asleep on their condvars until something outside wakes one. Tokio's fix for this ("eager driver handoff",worker.rs) is behindtokio_unstable.Does the real daemon have it?
Yes, for timers and sockets in general. Not for live text.
tests/store_wait.rsholds the store for 1 s on a plain thread, standing in for a slow request on the blocking pool. Meanwhile the courier wakes on its timer and waits for the store:mainGET /health(never touches the store)chat.deltaon the 40 ms timer)Live text survives because waking the pump sends a worker back to park, and that worker picks up the driver. In the daemon, bot stdout comes from the pipe reader threads, which do the same. What stalls is everything that waits on a timer or a socket while nothing else wakes a worker:
/health, app requests (including the aside reads of 11.1), bots' MCP calls, routine, approval and settle timers. The stall lasts as long as the store is held, so it is short in practice. A slow delete or a slow disk can make it long.Fix
Daemon::store()tries the lock first. If the lock is taken and the caller is on a multi-thread runtime, it waits insidetokio::task::block_in_place, which moves the thread's other work, and so the driver, to a new thread. Blocking-pool threads, plain threads and the current-thread runtime of most tests wait as before. The uncontended path is onetry_lock.It does not cover a slow SQLite statement running directly on a runtime thread, but those are single small statements in the courier, supervisor and tools. Spec 11.1 now describes this.
Checks
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings: passcargo test --workspace: passcargo test -p botloftd --test store_wait: 5 runs in a row pass. Onmain, the/healthtest fails every time.No real-Claude check needed: the change is in how the daemon waits for its own store.