fix(scheduler): address review findings
- JobScheduler: a stored job whose zone lookup throws no longer fails StartAsync; timer re-armed under the lock; a delivery cut off by shutdown isn't recorded (one-offs fire again after restart); CancelJobAsync in one step - Schedules.ParseAt: ISO 8601 with a date only (no machine-clock date for a bare 09:00) - core: a FollowUp prompt that loses the race to a starting run joins it as a follow-up, not steering - Telegram: TimeZoneFor tolerates non-numeric targets; RunPromptAsync waits only when queued; one agent dir - ScriptRunner: bad workspace paths are Failed, not exceptions; zone id helpers share TimeZoneIds.ToIana's rule - tests: no wall-clock sleeps in scheduler tests; transcript-entry, disabled-scheduler and failing-prompt coverage - docs: spec notes for the above; README row for the scheduler
This commit is contained in:
@@ -61,7 +61,10 @@ same chat.
|
||||
- **Scheduled agent prompts are queued as follow-ups, never as steering.** When the chat is idle a prompt job starts a
|
||||
run. When a run is active (a user request, or another job), the prompt goes in with `StreamingBehavior.FollowUp`: the
|
||||
current work finishes first, then the job's prompt is handled in the same run. A clock-driven prompt shouldn't
|
||||
redirect what the user is doing mid-turn. User messages keep steering as they do today.
|
||||
redirect what the user is doing mid-turn. User messages keep steering as they do today. This also holds when a run
|
||||
starts between `PromptAsync`'s idle check and its run gate: the core's `StartRunAsync` gets the caller's
|
||||
`StreamingBehavior`, and a `FollowUp` prompt joins the active run as a follow-up (with a `QueueUpdate`), not as
|
||||
steering.
|
||||
- **Cron parsing uses Cronos** (`Cronos` on NuGet, Hangfire, MIT, no dependencies). It parses standard 5-field
|
||||
expressions and macros (`@daily`, `@hourly`, …). `GetNextOccurrence(DateTimeOffset, TimeZoneInfo)` works in a given
|
||||
zone and has defined DST semantics: a time skipped when clocks go forward fires at the transition, and an ambiguous
|
||||
@@ -72,7 +75,10 @@ same chat.
|
||||
sets a 1-minute floor for watchers.
|
||||
- **Each chat has its own time zone.** It applies to cron, to displayed times, to the session's system prompt, to the
|
||||
local-time header on each prompt, and to the `TZ` of the chat's `bash`, watcher and `claude -p` processes (§8). An `at` without an offset is read in the chat's zone, because
|
||||
models often leave the offset out. An `at` with an offset is used as given. The zone is a chat setting, not a
|
||||
models often leave the offset out. An `at` with an offset is used as given. The date is required and only ISO 8601 is
|
||||
accepted (`yyyy-MM-dd`, or `yyyy-MM-dd[T| ]HH:mm[:ss[.fff]]` with an optional `Z`/offset): a bare `09:00` or a culture
|
||||
format such as `09/10/2026` is rejected with a readable error, because it would be resolved against the machine's
|
||||
clock rather than the chat's. The zone is a chat setting, not a
|
||||
scheduler setting, because the system prompt and `bash` need it with the scheduler disabled too:
|
||||
- **Stored by the host**, in a small per-chat settings store outside every workspace:
|
||||
`<AgentDir>/telegram/chat-settings.json` (§7). Not in `jobs.json` (the scheduler can be off) and not in the session
|
||||
@@ -93,7 +99,9 @@ same chat.
|
||||
fires immediately. If it's more than 60 s late, direct text gets `(due <time>, delivered late)` and an agent prompt
|
||||
gets `This was due at <time>; the bot was offline.` A cron job's next occurrence is computed strictly after "now".
|
||||
Occurrences missed while the bot was down are dropped, which is logged at Information. A one-off is removed only
|
||||
**after** its delivery completes, so a crash mid-fire re-fires it after a restart (at-least-once).
|
||||
**after** its delivery completes, so a crash mid-fire re-fires it after a restart (at-least-once). The same holds for
|
||||
a graceful stop: a delivery cut off by shutdown isn't recorded, so the one-off stays in `jobs.json` and fires late on
|
||||
the next start.
|
||||
- **Persistence is one central file:** `<AgentDir>/scheduler/jobs.json` (by default `~/.volcanic/agent/scheduler/`).
|
||||
The workspace is deliberately not used: the agent's `bash` can write anywhere there, and a central file is loaded
|
||||
without scanning every workspace. The format is `{ "version": 1, "jobs": [...] }` in `VolcanicJson` camelCase.
|
||||
@@ -153,7 +161,9 @@ watchers. There are at most `MaxJobsPerTarget` (50) jobs per target. Every error
|
||||
|
||||
`LoadAsync()` returns the jobs. A missing file means an empty list. A corrupt file is renamed to
|
||||
`jobs.json.corrupt-<timestamp>`, logged as an error, and loading starts empty: a bad file must not keep the host down.
|
||||
Invalid jobs are dropped with a warning. `SaveAsync(jobs)` writes `jobs.json.<pid>.tmp` and then does
|
||||
Invalid jobs are dropped with a warning. A stored cron job whose next occurrence can't be computed on start (for
|
||||
example a target the host can't map to a zone) is logged and left unarmed; it never fails `StartAsync`. The Telegram
|
||||
delivery's `TimeZoneFor` returns the default zone for a target that isn't a chat id. `SaveAsync(jobs)` writes `jobs.json.<pid>.tmp` and then does
|
||||
`File.Move(overwrite: true)`. Writes go through one `SemaphoreSlim`.
|
||||
|
||||
### 3. `ScriptRunner` (`IScriptRunner`, faked in tests)
|
||||
@@ -189,6 +199,7 @@ public sealed class JobScheduler(SchedulerOptions options, IScriptRunner runner,
|
||||
IReadOnlyList<JobView> List(string target); // JobView = job + NextRunAt (null = never)
|
||||
Task<JobView> CreateAsync(string target, NewJob job, CancellationToken ct); // throws JobValidationException
|
||||
Task<bool> CancelAsync(string target, string id, CancellationToken ct); // false: unknown, or another target's
|
||||
Task<ScheduledJob?> CancelJobAsync(string target, string id, CancellationToken ct); // the removed job, in one step (tool, /cancel)
|
||||
TimeZoneInfo TimeZoneFor(string target); // delivery.TimeZoneFor
|
||||
Task<ZoneChange> TimeZoneChangedAsync(string target, CancellationToken ct); // recompute cron, re-arm; counts for the reply
|
||||
}
|
||||
@@ -386,7 +397,11 @@ and Claude Code's `TZ`:
|
||||
the allow-list, like its pinned `CLAUDE_CODE_*` flags. Anything Claude Code itself adds about the date then agrees
|
||||
with our prompt. It isn't set on Windows (the OS ignores `TZ`, as in the process-environment-hook spec §3), nor for
|
||||
`claude auth login`/`auth status` and the doctor's probes, which have no session. `TZ` stays off the inherited
|
||||
allow-list: the host's own `TZ` never leaks through. This goes through the provider, not `IProcessSpawnPolicy`
|
||||
allow-list: the host's own `TZ` never leaks through. When the session has no configured zone, `claude -p` gets the
|
||||
IANA id of `TimeZoneInfo.Local` (it can't inherit `TZ`) while `bash` inherits the host's `TZ` (its `TimeZoneId` is
|
||||
null); both end up in the host's zone. All three id conversions follow one rule, `TimeZoneIds.ToIana` (mirrored by
|
||||
`ClaudeArgs.IanaZoneId` and the scheduler's `ZoneNames.Iana`, since extensions can't reference the core): null unless
|
||||
the id resolves to a system zone, so an unnamed `Local` sets no `TZ`. This goes through the provider, not `IProcessSpawnPolicy`
|
||||
(process-environment-hook spec §7). `docs/claude-code-provider.md` (its environment section) is updated with the
|
||||
implementation.
|
||||
- Headless sample: no change; it gets `TimeZoneInfo.Local` as before, now shown in the prompt.
|
||||
|
||||
Reference in New Issue
Block a user