Repository navigation
Fix two test_wait flakes: a stop that waited on background commands, and a test lock that held the service loop - #342
Merged
Conversation
…d a test lock that held the service loop A daemon that stopped with a background command still running waited up to 5 s for the publication stream: the command's result task kept a store handle open until the command ended or the stream timed out. The tests' fixture times commands out at 5 s, so the stop took about as long as `Client.close` allows, failed it now and then, and recorded the command as timed out. The registry now ends background commands as the daemon stops, as it ends terminals: each group is killed and its handle records process_lost at once, which the next start would have said. The fatal-storage test holds SQLite's write lock while it sends its admissions. The service looks for ready turns as each turn ends, on its own loop and through the store; a look still to come when the lock was taken held that loop, so the admissions were read only after the failure. The test now waits until the service has looked, by its store counters, before taking the lock. Starving the daemon of CPU reproduced the missing replies in 2 of 8 runs before and none of 12 after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013jkdGEgwvrmgA2w4Bq4gqN
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Merged
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.
Requested by George · project thread
Both flakes come from
tests/test_wait.pyfailing theruntimejob on PRs that never touched the daemon.1. A stop waited on background commands (
test_waiters_share_output_before_and_after_process_completiontimed out inClient.close, runs 40673434375 and 40792167581)When the daemon stopped with a background command still running, it waited up to 5 s for the publication stream, because the command's result task held a store handle until the command ended. The test fixture times commands out at 5 s, so a stop took about 4.9 s, right at
close's 5 s limit, and it recorded the command astimed_outeven though the daemon had killed it. The registry now ends background commands when the daemon stops, the same way it ends terminals. Each one's group is killed and its handle recordsprocess_loststraight away, which is what the next start would have said. Locally the stop went from 4.93 s to 0.02 s. A new test checks the resulting state: after a clean stop the row readsfinished/process_lost, and a restarted daemon reportsprocess_lost. On main the row readtimed_out.2. The storage-failure test's lock held the service loop (
test_a_fatal_storage_failure_still_answers_admissions_queued_before_it, failed on #334 and #333)The test holds SQLite's write lock while it sends its admissions. When a turn ends, the service looks for ready turns (
next_ready, which takesBEGIN IMMEDIATEon the writer) and awaits that look on its own loop. If the look came after the test took the lock, the loop stayed blocked until the lock was released. It then read the admissions only after the failure, so some got no reply. Before taking the lock, the test now waits until the service's store counters show it has finished that look. To check this, I ran the daemon alone on a CPU shared with six busy loops: replies went missing in 2 of 8 runs before the change and in none of 12 after.The other intermittent failures in recent runs fall outside this PR:
subscriptionscall only takes effect on the lookout's next 1 s tick. Sonny: the lookout forwards everything and judges nothing #310 rewrites that file, so the fix comes after it lands.Local runs:
cargo fmtandclippypass;test_waitpasses. In the full runtime suite onlytest_acpfails locally, and that's because this machine has no bubblewrap.Generated by Claude Code