[pick2510]

localagents::tests::stop_does_not_wait_for_a_blocked_stdin_writer fails now and then under the full parallel test run. For example, the test / Test (ubuntu-latest) job of #907 failed at localagents.rs:662 on a render-only change. Run on its own, the test always passes.

Why. Dropping a Run stops its agent on a background job whose handle goes into the process-wide stop_jobs() list. finish_stops() takes the whole list (mem::take) and joins what it took. Other tests call it too: the descendants Stop test and every Threads drop. When one of them takes the list just after this test's drop pushed its job, this test's own finish_stops() finds the list empty and returns at once. The try_wait().is_some() check then runs while the stop job is still in its 200 ms cancel wait, or still killing the process.

Fix. Test only. After finish_stops(), the test polls try_wait for up to 5 s, as the neighbouring descendants test already does for its descendant. An agent that is never stopped still fails it. Product code is unchanged.

Not done here: making finish_stops itself wait for jobs another caller took, for example with a pending count and a Condvar. In the app only one Threads calls it, at shutdown, so the race doesn't arise there.

fmt and clippy (-D warnings) pass. cargo test -p rustgis-ui-egui --lib ran 8 times in full: the test passed every time, and the only failures were three connectiontree download tests that also fail on main on this machine.

Summary by CodeRabbit

• Tests
  • Updated process-stop test validation to allow up to five seconds for a child process to exit after stopping completes. This accommodates delayed shutdowns instead of requiring the process to exit immediately.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 96177fad-8017-4cbe-9b58-4281903ed4ed

Commits

Reviewing files that changed from the base of the PR and between 797ccab and b7026de.

Files selected for processing: 1

• crates/ui-egui/src/localagents.rs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

Walkthrough

The blocked-stdin Stop test now polls for up to five seconds after dropping the run and calling finish_stops. It checks every 20 milliseconds and fails if the child process has not exited by the deadline.

Changes: Blocked-stdin Stop test

Layer / File(s)	Summary
Child-process exit polling crates/ui-egui/src/localagents.rs	The test polls for up to five seconds for the child process to exit, instead of requiring immediate exit after finish_stops.

Priority: Low

Estimated code review effort: 1 (Trivial) | ~4 minutes

Change: Other

Suggested reviewers: gpt-cmdr

Merge Risk: Minimal · up to b7026

The test-only change improves reliability without introducing merge-blocking risk.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title names the changed test and its new behavior, which matches the single-file test-only change: the blocked-stdin Stop test now waits for the agent's stop job. It is specific and conveys the pr…
Description check		The description follows the template's intent. The Summary section explains the flaky race, the cause (finish_stops takes the shared stop_jobs list via mem::take), and the fix (a test-only 5 s try_wai…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title names the changed test and its new behavior, which matches the single-file test-only change: the blocked-stdin Stop test now waits for the agent's stop job. It is specific and conveys the primary change. The title is slightly loose on the mechanism (polling try_wait for up to 5 s after finish_stops), but it is related to the main change.

Full details: Description check

Explanation

The description follows the template's intent. The Summary section explains the flaky race, the cause (finish_stops takes the shared stop_jobs list via mem::take), and the fix (a test-only 5 s try_wait poll). The Testing section lists fmt and clippy runs and eight full test runs, with the unrelated connectiontree failures noted. The Related issue section is missing. It has no 'Closes #' or 'Part of #' reference, although the description cites PR #907 as the failing job. The author should add a related issue reference or state that none exists.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{I’m a rabbit; I watch the clock,
Twenty milliseconds, tick by tick.
The child exits before the end,
I thump my paws and hop again.
Stop test waits, then rests its head.}

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Code review

This PR changes only a test in crates/ui-egui/src/localagents.rs. It replaces the immediate try_wait().is_some() assertion after finish_stops() with a poll that waits up to 5 seconds, sleeping 20 ms between checks.

I found no bugs, security issues or performance problems, and no AGENTS.md violations.

• Race fix: another test's finish_stops call can take this test's stop job and still be waiting on it. Polling for a short time fixes that without changing production code.
• Imports: Instant and Duration are already imported in mod tests (line 394), so the new code compiles.
• Lock handling: the mutex guard in the while condition is a temporary and drops before the sleep. The poll therefore never holds the child lock while sleeping, so it can't block a concurrent stopper.
• Timeout failure: the 5 s bound fails with the clear message "agent survived Stop". This keeps the regression meaningful rather than hiding a real hang.
• Never-crash rule: the unwrap() calls are in test code, which the rule allows.
• Web rules: using std::time::Instant is fine here. This is a desktop-only test, and the web_time rule applies to code that builds for wasm.

Nit, confidence low: the fixed 5 s deadline could be flaky on a heavily loaded CI runner. It is already generous, so I would leave it.

Change size

Compared: the whole PR against main (merge base 797ccab to PR head b7026de)

Metric	Value
Files changed	1 (added 0, modified 1, removed 0, renamed 0)
Size before	31.8 KB
Size after	32.1 KB
Total size change	+355 B

File	Before	After	Change
crates/ui-egui/src/localagents.rs	31.8 KB	32.1 KB	+355 B

[github-actions]

📖 Docs preview

Item	Value
Preview	https://preview.rustgis.app/pr-908/
Web app	https://preview.rustgis.app/pr-908/app/
Commit	b7026de

GitHub Pages takes a minute or two to publish a new commit.