agent: per-destination scheduler workers + transfer timeouts (#160)#181
Merged
mbertschler merged 3 commits intoJul 24, 2026
Conversation
The scheduler was a single serial worker with no per-run time bound: a hung rclone froze every destination, every volume, the peer syncs, and the index cadences until an operator killed the process by hand (F25). Split sync execution off the tick goroutine into a per-destination dispatcher. The concurrency unit is the destination: at most one sync per destination is in flight (two volumes targeting the same endpoint serialise on it), different destinations run concurrently, and an overall semaphore (default 4) bounds total parallelism. A slow or wedged destination is now confined to its own worker and can never delay the tick loop, index runs, peer syncs, or LAN pairs. The per-pair `skipped reason="in-flight sync run"` discipline and the pre-sync index ordering are preserved. Bound every automatic transfer two ways. rclone carries explicit --contimeout/--timeout on each invocation. A squirrel-side no-progress guard (stallGuard) watches rclone's per-second stats and, if transferred+checked+bytes stops advancing for StallTimeout (default 10m), kills the child and fails the run with a diagnosable "stalled" error that composes with #157's stderr capture. Checks count as progress so a long verification pass is never mistaken for a stall; a slow-but-moving transfer of a large volume is never killed. The guard is off for the foreground CLI (a human can interrupt) and set by the agent. In-flight visibility is already per-pair via the runs table (the TUI's active-runs block); the timeouts make those rows truthful by failing a stuck run instead of leaving it "running" forever. No schema change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Skm2Qm6wWUB7dD4CpPTz7
There was a problem hiding this comment.
Pull request overview
This PR hardens the agent’s scheduled sync pipeline against “dark” or wedged destinations by moving sync execution off the scheduler tick loop and adding explicit transfer time bounds to rclone runs, so one stuck endpoint can’t freeze unrelated syncs/index cadences.
Changes:
- Introduces a per-destination sync dispatcher (with a global parallelism cap) so different destinations can sync concurrently without blocking the scheduler tick loop.
- Adds explicit rclone connect/IO timeouts and a scheduler-enabled no-progress stall guard for streaming transfers, producing diagnosable failures instead of indefinite hangs.
- Updates/extends tests to cover stall-guard behavior and dispatcher concurrency/limits, and adapts scheduler tests to drain async workers.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sync/rclone.go | Adds explicit rclone timeouts and a stall/no-progress guard integrated with JSON stats parsing. |
| sync/rclone_test.go | Adds unit tests for stall guard behavior and updates parseJSONLog call sites. |
| cmd/squirrel/agent.go | Enables the stall guard for the agent scheduler’s rclone instance (kept off for foreground CLI). |
| agent/scheduler.go | Switches scheduler sync execution to an async dispatcher and drains it on shutdown. |
| agent/scheduler_test.go | Updates scheduler tests to wait for async dispatched syncs to complete. |
| agent/dispatcher.go | New per-destination dispatcher implementation with an overall concurrency limit. |
| agent/dispatcher_test.go | New tests validating per-destination concurrency, serialization behavior, and global parallelism cap. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review on #181. Fix 1 (starvation): the dispatcher skipped a sync when its destination was already busy. Because the tick iterates volumes by name, two volumes due to the same destination on one tick meant the later one was skipped every tick forever — never a run row, never consuming its cadence, just skip-log spam. In the reference setup this is the common case (photos and docs both push to cloudbox / s3archive / kopia-mirror), and it defeated the per-destination QUEUES that #160 option 4b specified. Replace skip-on-busy with a real per-destination FIFO queue: an idle destination starts its worker at once, a busy one takes the pair onto its queue (deduped so re-evaluation can't queue it twice) and runs it when the current transfer finishes. At most one sync per destination, overall parallelism still bounded, pre-sync-index ordering and per-pair semantics preserved. The genuine same-pair in-flight skip stays (the DB HasRunningRun check in the scheduler). Fix 2/3 (flaky tests): the stallGuard tests used tight real-time bounds. Loosen them — the fire test waits for the guard rather than racing a deadline; the reset test drives advance() off a ticker at 20x the bound's resolution across a span twice the stall window, so scheduler jitter can't fake a stall. Tests: TestSyncDispatcherSerializesSameDestination (two volumes, one destination: both run, FIFO, max one concurrent) and TestSchedulerTwoVolumesSameDestinationBothRun (two volumes due to the same destination on one tick both produce a sync run — neither starved). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Skm2Qm6wWUB7dD4CpPTz7
Async dispatch (this PR) freed the tick loop to keep firing every 30s while a dispatched sync runs in the background. But maybeRunSync ran the pre-sync index before the per-pair in-flight check, and syncDue stays true for the whole duration of an in-flight sync — the finished-run watermark only advances when the sync completes. So a slow multi-hour push made the scheduler re-index the source on every tick, burning I/O and flooding the never-pruned runs audit trail with a kind='index' row per tick. The old serial scheduler never hit this: the tick blocked inside the sync, so it could not re-evaluate until the sync finished. Filter each volume's due destinations down to the pairs that are not already syncing before running the pre-sync index, and skip the index entirely when none remain (there is no new push for it to precede). The in-flight signal reads from two sources that compose to close every window: the dispatcher's in-memory queue (set the instant a pair is enqueued, before its run row is visible) and the runs table (authoritative once the row exists, and the signal that also catches a stale row or a concurrent CLI `squirrel sync`). The ordering invariant holds — a volume syncing to one in-flight dest and one free dest still indexes once and dispatches the free one — and the per-pair scheduler.skipped log is unchanged. runSync becomes syncInFlight (a bool predicate, no dispatch); maybeRunSync is decomposed into dueSyncDests/dispatchableSyncDests to stay small. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Skm2Qm6wWUB7dD4CpPTz7
mbertschler
merged commit Jul 24, 2026
1fd5cd3
into
claude/issue-157-run-rows-standing-states
3 checks passed
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.
Summary
The agent scheduler was a single serial worker with no per-run time bound. When an S3 endpoint stopped accepting writes, the in-flight rclone hung and everything froze — every destination, every volume, the peer syncs, and the index cadences — for 12.6 min, then indefinitely on a full outage until an operator killed rclone by hand (friction log F25, S1, confirmed live twice). In the reference household a dark cloud destination silently stalled local NAS→HTPC replication too — a direct violation of ux-principle 4 (silent degradation) and the reference-setup guarantee that a dark offsite must not stall LAN pairs.
This goes straight to Martin's option 4b: per-destination worker queues plus transfer timeouts, in one change.
Changes
Per-destination workers (
agent/dispatcher.go). Sync execution moves off the scheduler's tick goroutine into asyncDispatcher. The concurrency unit is the destination: at most one sync per destination is in flight (two volumes targeting the same endpoint serialise on it — the store's per-pair gate does not, so this is the new guarantee), different destinations run concurrently, and an overall semaphore bounds total parallelism (default 4). A slow or wedged destination is confined to its own worker and can never delay the tick loop, index runs, peer syncs, or LAN pairs. The per-pairskipped reason="in-flight sync run"discipline and the pre-sync-index-before-sync ordering are preserved (the tick still runs a volume's index to completion before handing off its due syncs).Transfer timeouts (
sync/rclone.go). Two independent bounds:--contimeout/--timeouton every streaming rclone invocation (in one tunable place).stallGuard) driven off rclone's per-second--stats: iftransferred+checked+bytesstops advancing forStallTimeout(default 10m), it kills the child and fails the run with a diagnosablerclone stalled: no progress for …error that folds in the captured stderr tail (composes with runs: every failure and refusal becomes a run row or a standing state #157). Checks count as progress, so a long verification pass or a slow-but-moving transfer of a large volume is never killed. The guard is off for the foreground CLI (a human can interrupt) and set by the agent scheduler only.In-flight visibility. Already per-pair via the runs table (the TUI's active-runs block shows volume/destination/elapsed); the timeouts make those rows truthful by failing a stuck run instead of leaving it
runningforever after #157's orphan reaping. This also feeds #159's status grid. No new code needed here.Testing
go vet ./...clean.go test ./...passes.go test -race ./agent/... ./sync/...passes.golangci-lint run(v2.12.2, rebuilt with the module's go1.26.1 toolchain) — 0 issues.stallGuardfire/reset (sync); dispatcher concurrency, per-destination serialisation with per-pair skip log, overall parallelism bound, and nil-runner skip (agent). Existing scheduler tests updated to drain the async workers after a tick.No schema change (respected the #157=v25 / #158=v26 boundary).
Notes for the maintainer
defaultMaxParallelSyncs) and the no-progress bound 10m (sync.DefaultStallTimeout). 4 covers the reference NAS's four sync destinations without a LAN pair ever queueing behind a slow offsite; 10m is longer than the worst legitimate progress gap (hashing one very large file for a--checksumcompare) while bounding a wedge far below "indefinite". Both are package constants — no config/schema surface added.Stacked on #157 (PR #174). Base retargets to
mainwhen #157 merges.Closes #160
🤖 Generated with Claude Code
https://claude.ai/code/session_014Skm2Qm6wWUB7dD4CpPTz7
Generated by Claude Code