[Bug] Test bug for Liam

[jan-janssen]

Summary by CodeRabbit

• Bug Fixes

  • Shutdown now respects non-blocking settings, avoiding unnecessary blocking and improving responsiveness during shutdown.

• Tests

  • Updated shutdown tests to reflect non-blocking behavior: tests now assert in-flight tasks may continue after a non-waiting shutdown, improving test reliability.

[coderabbitai]

Warning

Rate limit exceeded

@jan-janssen has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 20 minutes and 29 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 324c646 and 93743d2.

📒 Files selected for processing (1)

• tests/unit/executor/test_api.py

📝 Walkthrough

Walkthrough

TaskSchedulerBase.shutdown now only joins the worker thread and the future queue when called with wait=True. The corresponding test timing was adjusted to avoid asserting immediate cancellation when shutdown(wait=False, cancel_futures=False) is used.

Changes

Cohort / File(s)	Summary
Scheduler shutdown change src/executorlib/task_scheduler/base.py	Join calls for the worker thread and future queue are now conditional on the wait flag; avoids blocking when wait=False.
Shutdown timing test tests/unit/executor/test_api.py	Test adjusted to not expect immediate future cancellation after shutdown(wait=False, cancel_futures=False); timing assertion changed (short sleep used before verifying completion).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• [Fix] Clean up shutdown routine for ClusterExecutors #920: Appears to modify TaskSchedulerBase.shutdown join/wait logic in the same area.
• [Feature] Set wait=False during initialization of Executor  #893: Also addresses shutdown/wait/cancel_futures behavior and related tests.

Suggested reviewers

• liamhuber

Poem

🐰 I hopped the scheduler’s twilight track,
I nudged the joins to step back.
If "wait" says no, I let them roam,
I munch on carrots and call it home. 🥕

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title '[Bug] Test bug for Liam' is vague and non-descriptive. It uses placeholder language ('Test bug for Liam') that doesn't convey what bug was fixed or what code change was made.	Replace with a specific, descriptive title that explains the actual bug fixed, e.g., 'Fix TaskSchedulerBase.shutdown to respect wait flag' or 'Fix test assertion in test_shutdown_wait_false_cancel_futures_false'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch liam

---

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

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

[coderabbitai]

🧹 Nitpick comments (1)

tests/unit/executor/test_api.py (1)

164-166: Replace fixed sleep with bounded synchronization on the future.

Line 164 uses a hardcoded delay (sleep(0.1)), which can make this test flaky under variable runner load. Prefer waiting directly on future_1 with a timeout.

Proposed patch

-        sleep(0.1)
+        done, _ = wait([future_1], timeout=2)
+        self.assertIn(
+            future_1, done, "Future did not settle within timeout after shutdown()."
+        )
         self.assertTrue(future_1.done())
         self.assertTrue(future_1.cancelled())

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/executor/test_api.py` around lines 164 - 166, Replace the
hardcoded sleep by synchronizing on the Future directly: call
concurrent.futures.wait([future_1], timeout=0.1) (or
future_1.result(timeout=0.1) wrapped to catch CancelledError) instead of
sleep(0.1), then keep the assertions self.assertTrue(future_1.done()) and
self.assertTrue(future_1.cancelled()); add the necessary import for
concurrent.futures.wait if not present and reference the test variable future_1
when making the wait call.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/unit/executor/test_api.py`:
- Around line 164-166: Replace the hardcoded sleep by synchronizing on the
Future directly: call concurrent.futures.wait([future_1], timeout=0.1) (or
future_1.result(timeout=0.1) wrapped to catch CancelledError) instead of
sleep(0.1), then keep the assertions self.assertTrue(future_1.done()) and
self.assertTrue(future_1.cancelled()); add the necessary import for
concurrent.futures.wait if not present and reference the test variable future_1
when making the wait call.

---

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dda7e18 and 7fba85b.

📒 Files selected for processing (1)

• tests/unit/executor/test_api.py