Repository navigation
Conversation
4919373 to
6ef493c
Compare
![ultimator-ai[bot]](https://avatars.githubusercontent.com/u/340985799?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultimator's review of 6ef493c
Round 2 at 6ef493c. The branch was rebuilt, so I reviewed the whole diff again. The git engine tests now block on a Condvar that the engine notifies each time it publishes a watch set, which replaces the sleep-polling loop; that part looks correct, because wait_timeout_while checks the predicate before it waits, so a set that is already published returns at once. read_until now waits for output with no deadline at all. That is my one comment. The PR description still describes the 120 s timeout and the output-so-far panic message, so it no longer matches the code. The PR shows no CI results for 6ef493c.
1 issue: 1 nit
The open issues and Fix are in the review of 592560f.
Reviewed in 26s · Session · @ultimator-ai review reviews again · @ultimator-ai fix fixes the open issues
| .expect("output in time") | ||
| .expect("the attachment is open") | ||
| { | ||
| match attachment.next().await.expect("the attachment is open") { |
There was a problem hiding this comment.
⚪ Nit · read_until can now hang the job instead of failing
With the timeout gone, a test whose process never prints the line it waits for blocks until the CI job's own timeout. The run then shows a cancelled job instead of a failure that names the test and the output it got. This also drops the 20 s bound from the ping test, which does not start PowerShell. The previous version (a within parameter, 120 s for PowerShell, and a panic that showed the output so far) fixed the cold start without losing either.
Ultimator · issue #1 · reply @ultimator-ai fix 1 to have it fixed
|
Skipped this pull request: the review of |
A status snapshot can reflect an index written after its pass reconciled the watch set; the set catches up on the pass that index event triggers. Tests that asserted the set right after the snapshot raced that pass. The debug hook now signals each published set, and the tests wait on it.
Windows PowerShell 5.1 can take over 20 s to print its first line when the runner starts it cold under the parallel suite, which failed read_until's 20 s timeout. read_until now waits for the next event; an early exit still fails the test. The residue grace is measured from the first output, so a slow start no longer counts against it.
6ef493c to
592560f
Compare
There was a problem hiding this comment.
Ultimator's review of 592560f
Round 3 at 592560f. The branch was rebuilt again; compared with 6ef493c, the only change is that the git watch-set wait also loses its timeout. #1 is still open: read_until still has no deadline. I've added a nit for the same problem in the git engine tests, which no longer fail after 5 s. The PR description still describes the 120 s timeout and the output-so-far panic, and no CI results show for 592560f.
1 issue: 1 nit
Open issues: check the ones to fix, then Fix.
- ⚪
#1read_until can now hang the job instead of failing ·crates/server/src/yas_process.rs:2707 - ⚪
#2Watch-set waits can hang instead of failing ·crates/git/tests/engine.rs:86-93 - 🔧 Fix the checked issues (all of them if none is checked)
Reviewed in 20s · Session · @ultimator-ai review reviews again · @ultimator-ai fix fixes the open issues
| fn wait_for_watch(&self, dir: &str, armed: bool) { | ||
| let path = self.0.join(dir); | ||
| yas_git::debug_wait_worktree_watches(&self.0.join(".git"), |watches| { | ||
| watches | ||
| .iter() | ||
| .any(|watch| std::fs::canonicalize(watch).is_ok_and(|watch| watch == path)) | ||
| == armed | ||
| }); |
There was a problem hiding this comment.
⚪ Nit · Watch-set waits can hang instead of failing
debug_wait_worktree_watches now uses wait_while with no timeout. If the engine never arms or disarms the directory, these tests block until the CI job's own timeout. That covers a real regression, and also a gitdir key that doesn't match repo.gitdir, such as an uncanonicalized path. Before this change, reattached_status_selection_observes_changes_made_while_pruned failed after 5 s with "ignored directory was not pruned", and the 6ef493c version kept that 5 s bound through wait_timeout_while. This is the same problem as #1, now also in the git engine tests.
Ultimator · issue #2 · reply @ultimator-ai fix 2 to have it fixed
This branch has not been deployed
1 participant
Stacked on #113 (which is on #112). Only the top commit, "Give the Windows residue tests time for a cold PowerShell start", is new here.
leave_residue_reports_the_exit_after_its_grace_and_leaves_the_job_runningandleave_residue_terminate_ends_the_job_after_the_kill_gracefail onmainon the newer windows-2025 runner image (20261007):output in time: Elapsed(()). They fail occasionally on the older image too.These are the only Windows process tests that start
powershell.exe, and the runner launches Windows PowerShell 5.1 cold while the rest of the suite runs in parallel. I timed how long the residue script takes to print itsPIDline in CI: about 7 s on the 20260925 image and about 23 s on 20261007.read_untilallowed 20 s.read_untilnow takes a timeout, asexit_ofalready does. The two residue tests wait up to 120 s for PowerShell's first line, and the ping test keeps 20 s. On timeout the panic message now includes the output received so far.Tag
@indentto continue the conversation here.