scheduler: don't resurrect a task deleted mid-run (#276) - #277
Open
CakeCrusher wants to merge 2 commits into
Open
CakeCrusher wants to merge 2 commits into
CakeCrusher wants to merge 2 commits into
Conversation
ankrgyl
reviewed
Sep 8, 2026
CakeCrusher
marked this pull request as draft
September 8, 2026 18:40
CakeCrusher
force-pushed
the
fix/scheduler-no-resurrect-deleted-task
branch
from
September 8, 2026 18:40
20d83d5 to
269e976
Compare
A fire runs a model turn, so a task can be deleted while a run holds a copy of its record. run_task then wrote that stale copy back, and because put_task created the file if it was missing, the delete was undone and the task kept firing. delete_scheduled_task and cancel_scheduled_task are model-facing tools, so a delete naturally arrives during a run. create_task is the only creator, so put_task becomes replace-only: it is a no-op once the task is gone. The existence check and the write happen under an exclusive lock on the tasks directory, taken by delete_task as well, so a delete cannot land between them. The lock is held around the file operations only, never across a run, and the OS releases it if a holder dies. An in-flight fire still finishes; the task is simply never scheduled again. No call sites change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QhK4EoQJsbjEaqGR3VJ7g5
CakeCrusher
force-pushed
the
fix/scheduler-no-resurrect-deleted-task
branch
from
September 8, 2026 18:52
269e976 to
30ad950
Compare
CakeCrusher
marked this pull request as ready for review
September 8, 2026 18:52
ankrgyl
reviewed
Sep 10, 2026
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| enum Write { | ||
| Create, | ||
| Replace, |
Collaborator
There was a problem hiding this comment.
small nit is that Replace to me sounds like "if it exists, overwrite it". I spent like 10 minutes trying to understand why it wasn't doing that before reading your comment. I'd prefer names like CreateIfNotExists (although I think Create is actually Replace?) and ReplaceIfExists
Author
There was a problem hiding this comment.
Makes sense, cleaned up the ambiguioty, also renamed to CreateOrReplace instead which is what that path actually does.
CakeCrusher
force-pushed
the
fix/scheduler-no-resurrect-deleted-task
branch
3 times, most recently
from
September 10, 2026 23:26
1a40106 to
f818265
Compare
`Create` did not enforce non-existence and `Replace` did not overwrite an absent record, so both names described contracts the code does not have. Say which case each one handles when the file is missing: the write `create_task` uses is an upsert, and the write `put_task` uses is a no-op once the task is gone. Naming only; behavior is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QhK4EoQJsbjEaqGR3VJ7g5
CakeCrusher
force-pushed
the
fix/scheduler-no-resurrect-deleted-task
branch
from
September 10, 2026 23:32
f818265 to
3b338e9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #276.
A fire runs a model turn, so a task can be deleted while a run holds a copy of its record.
run_taskthen wrote that stale copy back, and becauseput_taskcreated the file when it was missing, the delete was undone and the task kept firing.delete_scheduled_taskandcancel_scheduled_taskare model-facing tools, so a delete naturally arrives during a run — that is how this was hit.create_taskis the only creator, soput_taskbecomes replace-only: a no-op once the task is gone. The existence check and the write happen under an exclusive lock on the tasks directory, taken bydelete_taskas well, so a delete cannot land between them. The lock is held around the file operations only, never across a run, and the OS releases it if a holder dies (std::fs::File::lock, stable since 1.89 — no new dependency).Deletion is therefore terminal: an in-flight fire still finishes, and the task is simply never scheduled again.
The whole change is in
scheduler_store.rs. No call sites change —run_taskis untouched.Includes a test that reproduces the original failure: a run holds a record, a delete lands mid-run, and the stale write-back must not recreate the task. It fails if
put_taskis allowed to create.Related, if useful
The doc comment on
claim_due_tasksnotes its read-lease-write is unconditional pending "the conditional puts in upstream PR #113". Replace-onlyput_taskalso covers that call site.Worth flagging: #113 as it stands keeps the unconditional
put_taskat the end ofrun_task, so this bug survives it — and #113 removesclaim_due_tasksentirely. Happy to rebase onto #113 or #232 in whatever order suits.cargo test -p executor(115 passed),cargo clippy --all-targets -D warnings,cargo fmt, andcargo check --workspace --all-targetsall clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01QhK4EoQJsbjEaqGR3VJ7g5