feat: Blueapi plan pause - #1589
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1589 +/- ##
==========================================
+ Coverage 96.31% 96.60% +0.28%
==========================================
Files 46 46
Lines 3641 3739 +98
==========================================
+ Hits 3507 3612 +105
+ Misses 134 127 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ead signals Route resume() and cancel_active_task() through the worker thread's signal queue instead of mutating state directly from the caller, so state stays consistent across pause/resume/cancel transitions. Includes tests and a fix for cancel_active_task() when the RunEngine is already in a paused state.
- Resolve a paused task before stopping the worker, instead of leaving the RunEngine paused and the task incomplete forever with no thread left to resolve it. - Apply the latest cancel_active_task() request while paused instead of a stale queued one. - Prevent a race between the caller thread and worker thread when recording a cancelled task's outcome.
racing the worker thread's own finalization, which could report a completed task with no result.
5f85341 to
f6f82cb
Compare
tpoliaw
left a comment
There was a problem hiding this comment.
Taking a while to figure out what is going on with the task worker in general and what needed to change. There are a couple of blocking things (resumed plan results and exceptions) but feel free to ignore/question the rest.
Might need you to go over why the CancelSignal is needed when it wasn't before - I'm not sure I follow the logic.
| if self._current is not None: | ||
| try: | ||
| result = self._ctx.run_engine.resume() | ||
| self._current.set_result(result) |
There was a problem hiding this comment.
resume returns a RunEngineResult not the return value of the plan
| self._current.set_result(result) | |
| self._current.set_result(result.plan_result) |
|
Another thing to look at - running a count plan and then pausing/resuming several times, it occasionally fails with "Received multiple indices in a `stream_datum` document for one event"I'm not sure if it's related to this change or if there is a bug in the run engine/plan that has only just surfaced now pausing is possible. |
a5e8e0f to
70671d9
Compare
fixed a merge conflict error
…ign change due to race conditions.
…tual completion it needed, causing a race where callers woke up before _completed_tasks was updated.
… thread finished writing to _completed_tasks, causingflaky failures , now waits on that instead of returning right after abort()/stop()
… which could miss a rapid pause→resume→re-pause transition and hang until timeout — now waits on a dedicated per-call Event that the worker thread sets once _cycle() has actually finished processing that resume, so there's no window to miss
Bluesky's RunEngine pauses via a RunEngineInterrupted, but blueapi was treating that exception like any other plan failure , pausing a task marked it as failed instead of leaving it resumable. On top of that, resume() and cancel_active_task() mutated RunEngine state directly from the calling thread, racing with the worker thread that owns it, which could leave a paused RunEngine stuck forever, apply a stale cancel, or double-set a task's outcome.