Skip to content

Add stopped record check to callback processing#183

Merged
andrewwormald merged 2 commits intomainfrom
fix/callback-version-check
Mar 10, 2026
Merged

Add stopped record check to callback processing#183
andrewwormald merged 2 commits intomainfrom
fix/callback-version-check

Conversation

@andrewwormald
Copy link
Copy Markdown
Collaborator

@andrewwormald andrewwormald commented Mar 10, 2026

Problem

Callback processing doesn't check RunState.Stopped() before executing the user's callback function, unlike the step consumer which does. Stopped records (paused/cancelled) get their callback function executed unnecessarily.

Why

Callbacks on stopped records can cause unexpected side effects. The step consumer already guards against this — callbacks should too.

Fix

Add RunState.Stopped() check before executing the callback function, matching step consumer behaviour.

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Callbacks now correctly skip processing for paused and cancelled workflows, preventing unintended callback execution and ensuring consistent workflow state management across the system.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b4f8d6ff-27c8-43cf-b060-7d83cfe1f459

📥 Commits

Reviewing files that changed from the base of the PR and between f4d926f and 90fd568.

📒 Files selected for processing (2)
  • callback.go
  • callback_internal_test.go

📝 Walkthrough

Walkthrough

The pull request adds an early exit condition in the processCallback function that skips processing when the underlying RunState is stopped. This change aligns callback handling with step consumer behaviour. Supporting test cases have been added to verify that callbacks are not invoked when the latest matching record is in a stopped state (either paused or cancelled), ensuring the early exit operates as intended.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Suggested reviewers

  • ScaleneZA
  • echarrod

Poem

🐰 A workflow that's sleeping, no more shall proceed,
When stopped is the state—that's the stop we all need!
With tests paused and cancelled, aligned with the plan,
The callback now skips what it simply can't scan! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarises the main change: adding a stopped record check to callback processing, which is clearly reflected in the code modifications.
Description check ✅ Passed The description is directly related to the changeset, explaining the problem, rationale, and solution for the stopped record check implementation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/callback-version-check

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.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai coderabbitai bot requested review from ScaleneZA and echarrod March 10, 2026 12:05
@sonarqubecloud
Copy link
Copy Markdown

@andrewwormald andrewwormald merged commit b024edd into main Mar 10, 2026
9 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