fix(scheduler): stop canonical tasks after delete#24
Conversation
Use the live scheduled task in Del instead of going through Lookup, and skip timer rearming once task cancellation has been signaled. Also add a regression test that holds a running task, deletes it, and verifies no later invocation slips through.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughScheduler.Del now removes tasks from the scheduler map immediately under a single lock and explicitly cancels/stops timers; execTask prevents timer.Reset when the task context is canceled or the timer is nil. A new test verifies tasks do not run after deletion during execution. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Harness
participant Sched as Scheduler
participant Task as Task Goroutine
participant Timer as Timer
Test->>Sched: schedule task (short interval)
Sched->>Timer: create/start timer
Timer->>Task: trigger first run
Task->>Task: begin work (blocks)
Test->>Sched: call Del(taskName)
activate Sched
Sched->>Sched: acquire lock
Sched->>Sched: remove task from map
Sched->>Task: if t.cancel != nil call t.cancel()
Sched->>Timer: if t.timer != nil call t.timer.Stop()
deactivate Sched
Task->>Task: execTask checks t.ctx.Err() or t.timer == nil
alt cancelled or timer nil
Task->>Task: early return, do not Reset
else
Task->>Timer: Reset interval
end
Test->>Test: verify no post-delete execution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Align the test workflow with the working Codecov pattern from testlazy. The old goveralls step was dead weight and tripped over modern Go tool installation behavior, which is a rough hobby for a CI job.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24 +/- ##
==========================================
+ Coverage 98.21% 98.26% +0.04%
==========================================
Files 1 1
Lines 112 115 +3
==========================================
+ Hits 110 113 +3
Misses 1 1
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests.yml:
- Around line 18-35: Replace mutable action tags (actions/checkout@v6,
actions/setup-go@v6, codecov/codecov-action@v5) with their corresponding pinned
commit SHAs; update the workflow to reference the full commit SHA for each
action and keep the human-friendly tag (e.g., `@v6` or `@v5`) as a trailing comment
for readability, ensuring the checkout, setup-go, and codecov steps use the
SHA-pinned refs so CI uses immutable versions.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4e0e1f8-b74f-4153-bee2-4acf51576f91
📒 Files selected for processing (1)
.github/workflows/tests.yml
There was a problem hiding this comment.
Pull request overview
This PR fixes scheduler task deletion semantics so deleting a task stops the canonical scheduled instance (not a cloned copy) and prevents recurring timers from being re-armed after cancellation, with a regression test to cover a key delete-while-running interleaving.
Changes:
- Update
Scheduler.Delto remove tasks directly from the scheduler map and cancel/stop the canonical task instance. - Guard recurring reschedule logic to avoid
timer.Resetafter task cancellation. - Add a regression test for deleting a recurring task while its first invocation is still running, and modernize the CI workflow steps.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tasks.go | Deletes canonical tasks from the scheduler map and avoids resetting timers once a task is canceled. |
| tasks_test.go | Adds a regression test for deleting a task during its in-flight execution. |
| .github/workflows/tests.yml | Updates Go setup/checkout steps and refines Codecov upload configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| id, err := scheduler.Add(&Task{ | ||
| Interval: time.Duration(100 * time.Millisecond), | ||
| TaskFunc: func() error { | ||
| currentRun := atomic.AddInt32(&runCount, 1) | ||
|
|
||
| if currentRun == 1 { | ||
| started() | ||
| <-releaseCtx.Done() | ||
| finished() | ||
| return nil | ||
| } | ||
|
|
||
| if atomic.LoadUint32(&deleted) == 1 { | ||
| postDelete() | ||
| } | ||
|
|
||
| t.Errorf("task executed after delete, run=%d", currentRun) | ||
| return nil | ||
| }, |
There was a problem hiding this comment.
This test calls t.Errorf from inside the scheduled task function, which runs in a scheduler goroutine. If the task runs after the subtest has completed, this can trigger "testing.T called after test finished" and make the test flaky. Consider reporting failures via a channel/atomic flag that the test goroutine asserts on instead of calling t.Errorf inside TaskFunc.
There was a problem hiding this comment.
@copilot you are right, it should instead return an error and let the error function cancel an errors context which is checked in the main test. Can you fix it?
There was a problem hiding this comment.
Fixed in commit 4c1b4b7. The TaskFunc now returns an error when it runs after deletion, the ErrFunc cancels an errCtx, and the main test goroutine asserts that errCtx is not cancelled within the timeout window. t.Errorf and the old postDeleteCtx/postDelete variables are removed.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Resource not accessible by integration - https://docs.github.com/rest/git/trees#create-a-tree |
Agent-Logs-Url: https://github.com/madflojo/tasks/sessions/762c29c6-b762-441c-9560-cae6427c5379 Co-authored-by: madflojo <1731256+madflojo@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary
Fix task deletion so the scheduler stops the live stored task rather than a cloned copy, and avoid rearming recurring timers after cancellation.
Changes
Rationale
The previous delete path went through
Lookup, which returns a clone. That made task lifecycle control harder to reason about and leftexecTaskfree to rearm canceled tasks. This change makes cancellation authoritative and adds coverage for the most suspicious interleaving.Risk & Impact
Summary by CodeRabbit
Bug Fixes
Performance
Tests
Chores