fix(queue): show "Workflow was cancelled" on first Stop click#384
fix(queue): show "Workflow was cancelled" on first Stop click#384t0mdavid-m merged 1 commit intomainfrom
Conversation
After PR #383 the RQ worker actually terminates on Stop, but the UI kept showing "running" and a second Stop click rendered "Errors occurred". Two causes: 1. stop_workflow cleared .job_id on success, so get_workflow_status fell through to the local-mode pid_dir fallback. The killed worker left stale child PID files there, so the fallback flipped running back to True forever. 2. The static log-display branch only knew "WORKFLOW FINISHED" vs error, so a cancelled run was misreported as an error. Fix: stop_workflow now writes a "WORKFLOW CANCELLED" marker via Logger and removes the stale pid_dir; .job_id is kept so the queue status flow stays authoritative and renders the Cancelled pill. StreamlitUI's static display dispatches through a new pure helper classify_log_outcome (finished/cancelled/error). Also fills in the missing canceled branch in _show_queue_status so the queue pill actually renders for canceled jobs.
📝 WalkthroughWalkthroughThis PR centralizes workflow log outcome classification through a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WorkflowManager
participant Logger
participant classify_log_outcome
participant StreamlitUI
User->>WorkflowManager: stop_workflow()
WorkflowManager->>Logger: Write WORKFLOW CANCELLED marker
WorkflowManager->>WorkflowManager: Remove stale pid_dir
WorkflowManager-->>User: Cancellation confirmed
User->>StreamlitUI: Query workflow status
StreamlitUI->>WorkflowManager: get_workflow_status()
WorkflowManager->>Logger: Read log content
Logger-->>WorkflowManager: Log content with markers
WorkflowManager-->>StreamlitUI: Status with log content
StreamlitUI->>classify_log_outcome: classify_log_outcome(log_content)
classify_log_outcome-->>StreamlitUI: "cancelled"
StreamlitUI->>StreamlitUI: Render cancellation warning
StreamlitUI-->>User: Display cancellation message
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/workflow/WorkflowManager.py (1)
201-202: Use the shared cancellation marker constant instead of duplicating the literal.Line 201 and Line 230 hardcode
"WORKFLOW CANCELLED"even though_log_status.pyalready definesCANCELLED_MARKER. Reusing the constant avoids future drift between classifier and logger writes.♻️ Proposed refactor
from .StreamlitUI import StreamlitUI +from ._log_status import CANCELLED_MARKER ... - self.logger.log("WORKFLOW CANCELLED") + self.logger.log(CANCELLED_MARKER) ... - self.logger.log("WORKFLOW CANCELLED") + self.logger.log(CANCELLED_MARKER)Also applies to: 229-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflow/WorkflowManager.py` around lines 201 - 202, Replace the hardcoded "WORKFLOW CANCELLED" string passed to self.logger.log with the shared CANCELLED_MARKER constant from _log_status to avoid duplication; import CANCELLED_MARKER from _log_status at the top of the module and update both places where self.logger.log("WORKFLOW CANCELLED") appears (the one paired with shutil.rmtree(self.executor.pid_dir, ignore_errors=True) and the other occurrence) so they both call self.logger.log(CANCELLED_MARKER) while leaving the pid_dir cleanup logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/workflow/WorkflowManager.py`:
- Around line 201-202: Replace the hardcoded "WORKFLOW CANCELLED" string passed
to self.logger.log with the shared CANCELLED_MARKER constant from _log_status to
avoid duplication; import CANCELLED_MARKER from _log_status at the top of the
module and update both places where self.logger.log("WORKFLOW CANCELLED")
appears (the one paired with shutil.rmtree(self.executor.pid_dir,
ignore_errors=True) and the other occurrence) so they both call
self.logger.log(CANCELLED_MARKER) while leaving the pid_dir cleanup logic
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae8e8f8f-37fc-4393-ab88-8a70b8233ae4
📒 Files selected for processing (5)
src/workflow/StreamlitUI.pysrc/workflow/WorkflowManager.pysrc/workflow/_log_status.pytests/test_log_status.pytests/test_workflow_manager_stop.py
After PR #383 the RQ worker actually terminates on Stop, but the UI kept showing "running" and a second Stop click rendered "Errors occurred". Two causes:
Fix: stop_workflow now writes a "WORKFLOW CANCELLED" marker via Logger and removes the stale pid_dir; .job_id is kept so the queue status flow stays authoritative and renders the Cancelled pill. StreamlitUI's static display dispatches through a new pure helper classify_log_outcome (finished/cancelled/error). Also fills in the missing canceled branch in _show_queue_status so the queue pill actually renders for canceled jobs.
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Tests