feat: propagate task cancellation to in-progress activities#419
feat: propagate task cancellation to in-progress activities#419GabriellePoncey wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a task terminal-state watcher subsystem: ChangesTask Terminal-State Watcher
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 36 complexity · 0 duplication
Metric Results Complexity 36 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/internal/workflows/activities/apply_event.go (1)
71-73: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog watcher stream failures before disabling interruption support.
Setup failures are warned on Line 64, but runtime watcher failures silently stop cancellation propagation. Capture the error value here so operators can diagnose why task cancellation no longer interrupts this activity.
Proposed adjustment
- case <-watcher.Error(): + case err := <-watcher.Error(): // Watch stream died; stop monitoring without cancelling // the activity — we don't know the task's current state. + activity.Logger(ctx).Warn("task watcher stopped; activity won't be interrupted on task cancellation", "error", err)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/internal/workflows/activities/apply_event.go` around lines 71 - 73, The watcher error path in apply_event.go currently stops interruption support without recording why, so update the watch loop around watcher.Error() in applyEventActivity to capture the received error and log it before returning or breaking. Reuse the same logging pattern used for watcher setup failures, and include the actual error value plus enough context to identify the activity/task so operators can diagnose runtime watcher stream failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/internal/task/watcher.go`:
- Around line 142-158: The shared watcher error handling in propagateErrors
leaves a dead sharedWatcher in registry.entries after watchOp.Error() fires, so
later acquire/newSubscription calls can attach to a stale watcher and miss
Done/Error. Update sharedWatcher/registry teardown so stream errors are
persisted on sharedWatcher and the registry entry is removed or marked failed
immediately after the error is observed, and make newSubscription populate errCh
from that stored error for late subscribers; also ensure propagateErrors
continues to use the sharedWatcher state consistently for both current and
future subscribers.
- Around line 105-114: The shared watcher completion state is being overwritten
on subsequent terminal events, so once finishAll marks sharedWatcher as terminal
it should not update terminalErr again. Add an early return at the start of
sharedWatcher.finishAll when sw.terminal is already true, and keep the existing
subscriber fan-out only for the first terminal transition so new subscribers
don’t see a later status replace the original canceling state.
---
Nitpick comments:
In `@server/internal/workflows/activities/apply_event.go`:
- Around line 71-73: The watcher error path in apply_event.go currently stops
interruption support without recording why, so update the watch loop around
watcher.Error() in applyEventActivity to capture the received error and log it
before returning or breaking. Reuse the same logging pattern used for watcher
setup failures, and include the actual error value plus enough context to
identify the activity/task so operators can diagnose runtime watcher stream
failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d012b0d-53bb-4144-acce-773016814c0c
📒 Files selected for processing (4)
server/internal/task/service.goserver/internal/task/task_store.goserver/internal/task/watcher.goserver/internal/workflows/activities/apply_event.go
GabriellePoncey
requested review from
moizpgedge and
tsivaprasad
and removed request for
tsivaprasad
|
Hi @GabriellePoncey can you give a look at coderabbit issues |
When a task is cancelled, ApplyEvent now detects it via an etcd watch and cancels its own context, interrupting long-running operations rather than running to completion. PLAT-583
- Make terminal completion sticky: `finishAll` now returns early if `sw.terminal` is already set, preventing a later status event (e.g. `StatusFailed` after `StatusCanceling`) from overwriting `terminalErr` and delivering the wrong error to future subscribers. - Fix dead sharedWatcher left in registry after stream error: `propagateErrors` now persists the stream error in `sw.streamErr` and calls `shutdown()` after fanning out, removing the stale entry from `registry.entries`. Late subscribers created in the window between the fanout and registry removal receive the error immediately via `newSubscription`. PLAT-583
GabriellePoncey
force-pushed
the
feat/PLAT-583/cancel-lifecycle-operations
branch
from
c2f7cba to
eb0399a
Compare
moizpgedge
left a comment
moizpgedge
left a comment
There was a problem hiding this comment.
Approving. Tested cancel end to end and it works, the in progress activity stops right after cancel instead of running to the end.
2 participants
When a task is cancelled, ApplyEvent now detects it via an etcd watch and cancels its own context, interrupting long-running operations rather than running to completion. Operations that respect context cancellation (eg Patroni HTTP calls) are interrupted promptly while others complete their current step before the activity exits.
Changes
task.Watcher: subscribes to a task key in etcd and closes aDonechannel when the task reaches a terminal state or is deleted;Errreturns the reason (ErrTaskCanceled,ErrTaskFailed, or nil).task.Service.NewWatcherandtask.TaskStore.Watchto createwatchers from the service layer.
watcherRegistrytotask.Service: concurrentApplyEventcallswatching the same task share one etcd stream rather than each opening
their own; events are fanned out to all subscribers.
ApplyEvent: a goroutine cancels the activitycontext when
watcher.Done()fires. Watcher setup failure isnon-fatal — the activity continues without interruption support.
StatusCancelingthe same asStatusCanceledso activities areinterrupted during the canceling phase, not only after it completes.
PLAT-583