Skip to content

fix(queue): show "Workflow was cancelled" on first Stop click#384

Merged
t0mdavid-m merged 1 commit intomainfrom
claude/fix-vendor-queue-error-EwPar
Apr 28, 2026
Merged

fix(queue): show "Workflow was cancelled" on first Stop click#384
t0mdavid-m merged 1 commit intomainfrom
claude/fix-vendor-queue-error-EwPar

Conversation

@t0mdavid-m
Copy link
Copy Markdown
Member

@t0mdavid-m t0mdavid-m commented Apr 28, 2026

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.

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Fixed workflow status reporting that incorrectly showed "running" after cancellation
    • Improved workflow cancellation logging and cleanup to prevent state inconsistencies
  • New Features

    • Enhanced UI feedback with specific cancellation warnings instead of generic error messages
    • Improved outcome classification for finished, cancelled, and error states
  • Tests

    • Added comprehensive test coverage for workflow cancellation behavior and status reporting

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.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 28, 2026

📝 Walkthrough

Walkthrough

This PR centralizes workflow log outcome classification through a new classify_log_outcome function that standardizes detection of terminal states (finished, cancelled, error). It updates the UI to use this classifier and enhances cancellation logic to write outcome markers and clean up stale PID directories, preventing incorrect status reporting.

Changes

Cohort / File(s) Summary
Log outcome classification
src/workflow/_log_status.py
New module introducing LogOutcome type alias and classify_log_outcome() function that checks for cancellation markers first, then finished markers, defaulting to error. Includes marker string constants.
Workflow manager cancellation
src/workflow/WorkflowManager.py
Updated stop_workflow() to write "WORKFLOW CANCELLED" log marker and remove stale pid_dir on successful cancellation. Added cancellation marker in _stop_local_workflow() when processes are terminated.
Streamlit UI integration
src/workflow/StreamlitUI.py
Replaced hardcoded "WORKFLOW FINISHED" string matching with classify_log_outcome() result. UI now displays success/cancellation/error messages based on classifier output.
Test coverage
tests/test_log_status.py, tests/test_workflow_manager_stop.py
New test suites verifying classify_log_outcome() behavior across finished/cancelled/error cases and confirming stop_workflow() correctly marks cancellations, cleans up PID directories, and maintains idempotency.

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
Loading

Possibly related PRs

  • OpenMS/streamlit-template#320: Refactors log reading and separates parsing from display logic for final status detection, complementing the centralized classify_log_outcome approach introduced here.

Poem

🐰 Logs now speak in crystal tones,
Cancelled, finished, errors known—
No more stale PID bones,
Classification brings us home! 🎯

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 64.71% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly summarizes the main change: showing a 'Workflow was cancelled' message when the Stop button is clicked in queue mode, which directly addresses the root cause of incorrect UI state after workflow cancellation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/fix-vendor-queue-error-EwPar

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

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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.py already defines CANCELLED_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

📥 Commits

Reviewing files that changed from the base of the PR and between ce4092f and 465eaa2.

📒 Files selected for processing (5)
  • src/workflow/StreamlitUI.py
  • src/workflow/WorkflowManager.py
  • src/workflow/_log_status.py
  • tests/test_log_status.py
  • tests/test_workflow_manager_stop.py

@t0mdavid-m t0mdavid-m merged commit f7574df into main Apr 28, 2026
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants