fix(serve): start scheduler at startup instead of first ACP connection - #10766
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65cc58c869
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }) | ||
| .collect::<Result<Vec<_>>>()?; | ||
| let secret_key = env_secret.unwrap_or_else(generate_serve_secret_key); | ||
| if let Err(error) = server.start_scheduler().await { |
There was a problem hiding this comment.
Because this eagerly initializes the server's scheduler from the current schedule.json, a goose serve process that starts with no schedules will keep an empty in-memory job set. If a user then runs goose schedule add in another process before the first ACP client connects, create_agent will reuse this already-initialized scheduler instead of loading the newly persisted job, so the schedule will not be registered for automatic execution until some later operation happens to call list_scheduled_jobs() and sync from storage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The stale window existed before this PR too for anything added after the first client connected, but eager init did make it wider. Will investigate...
There was a problem hiding this comment.
OK, should be resolved by create_agent: scheduler.list_scheduled_jobs().await - runs sync_fron_storage() which should register anything new.
9eeea72 to
97c6c52
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97c6c524cd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }) | ||
| .collect::<Result<Vec<_>>>()?; | ||
| let secret_key = env_secret.unwrap_or_else(generate_serve_secret_key); | ||
| if let Err(error) = server.start_scheduler().await { |
There was a problem hiding this comment.
With this eager initialization, a goose serve process that starts before any jobs are created has a live scheduler whose job set is populated only from the schedule.json contents seen at construction. The new storage sync runs only from create_agent, so if a user runs goose schedule add ... in another process and no ACP client connects—the headless mode this change targets—the persisted job is never registered and will not fire until some later client/list path happens to sync storage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't think this is a regression. From what I can tell, before the change a serve that never got a client had no scheduler at all, so external jobs never fired. Now, jobs present at startup fire, and any client that connects picks up later additions. To solve this there would need to be a periodic re-sync which I think is out of scope for this change.
A headless `goose serve` builds its scheduler lazily, on the first create_agent call, so nothing runs scheduled jobs until a client connects. Nobody notices in Desktop because opening the app connects, but a server left running on its own just never fires anything. The lazy OnceCell looks like migration residue rather than a decision: the old REST goosed started its scheduler at boot, and stdio `goose acp` still creates its agent immediately at startup. Start it in handle_serve_command and warn instead of failing if it does not come up, since create_agent will retry on the first connection anyway. With aaif-goose#10781 this is a no-op unless --enable-scheduler is passed.
Starting the scheduler at startup means it loads whatever was on disk at that moment. A job added later by another process, `goose schedule add` or the Desktop, is not in the running server's registry until something happens to re-read storage. list_scheduled_jobs syncs from storage and the sync is diff based, so calling it on connect is cheap and idempotent.
97c6c52 to
97d0115
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97d0115771
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }) | ||
| .collect::<Result<Vec<_>>>()?; | ||
| let secret_key = env_secret.unwrap_or_else(generate_serve_secret_key); | ||
| if let Err(error) = server.start_scheduler().await { |
There was a problem hiding this comment.
When goose serve --enable-scheduler starts with jobs already in schedule.json, this eagerly registers them in the in-process scheduler, but there is no headless refresh after that; the only new storage refresh is create_agent calling list_scheduled_jobs(), which requires an ACP client. If another process later runs goose schedule remove (or a Desktop process pauses/updates the same persisted job) while no client connects, the serve process keeps firing the stale in-memory cron entry. Unlike the prior add-after-start case, this job was loaded by the new eager startup path, so the PR can keep executing work the user has since removed or disabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The firing task checks the in-memory map only (current_jobs_arc.lock(), !j.paused) and never re-reads schedule.json. So a live scheduler already goes stale on external removals today. I could fix this but it expands the scope and I'd like to get this merged, the more I add, the more scrutiny we will need.
|
Re-based for #10781 |
|
cc @lifeizhou-ap who I think may have done something else related to the same objective, but I cannot find the commit at the moment. I want to clarify before we go ahead with this change. |
Sounds good, thanks for looking it over. |
lifeizhou-ap
left a comment
lifeizhou-ap
left a comment
There was a problem hiding this comment.
The use case is really a valid use case. Thanks for the fix
* origin/main: (42 commits) chore (codex-acp): migrate to @agentclientprotocol/codex-acp (#10923) style: add dark mode inline code styling for message bubbles (#10861) docs: clarify macOS sandbox feature was removed in post-v1.25.0 cleanup (#10900) fall back to static model list only for non-models payloads (#10189) Remove CLI project support (#10838) fix(anthropic): add claude-sonnet-5 and claude-fable-5 to known models list (#10865) fix: stdio extensions silently skipped when name missing or env: used in config (#10773) feat(desktop): show session metadata on sidebar chat hover (#10663) fix(serve): start scheduler at startup instead of first ACP connection (#10766) fix: respect disabled builtin extensions (developer) at session start (#10223) fix: parse PyPI requirements for OSV checks (#10510) fix(telegram): contain voice file extensions (#10456) Revert "feat(security): chunk command-classifier input with overlapping windows" (#10416) (#10870) docs: update Discord invite (#10863) fix(security): preserve denied tool request precedence (#10612) fix(hints): contain subdirectory hint discovery (#10545) chore(deps): bump pem from 3.0.6 to 4.0.0 (#10853) chore(deps): bump base64 from 0.22.1 to 0.23.0 (#10851) chore(deps): bump jsonwebtoken from 10.4.0 to 11.0.0 (#10850) chore(deps): bump astral-sh/setup-uv from 8.3.2 to 9.0.0 (#10847) ...
* origin/main: (58 commits) Handle MCP tool list change notifications fix: restore final_output_tool when agent is recreated after LRU eviction (#10793) feat(ui): bring back make sidebar resizable with drag handle and persisted width (#10795) docs: document auto-injected GOOSE_SHELL flags (#10759) feat: surface output-token-limit info (#10831) fix(desktop): avoid O(n²) cloning during session load (#10665) feat: add interactive menu for single select elicitations (#10327) fix(acp): prefix child stderr log target so it passes the default goose=info filter (#10901) chore (codex-acp): migrate to @agentclientprotocol/codex-acp (#10923) style: add dark mode inline code styling for message bubbles (#10861) docs: clarify macOS sandbox feature was removed in post-v1.25.0 cleanup (#10900) fall back to static model list only for non-models payloads (#10189) Remove CLI project support (#10838) fix(anthropic): add claude-sonnet-5 and claude-fable-5 to known models list (#10865) fix: stdio extensions silently skipped when name missing or env: used in config (#10773) feat(desktop): show session metadata on sidebar chat hover (#10663) fix(serve): start scheduler at startup instead of first ACP connection (#10766) fix: respect disabled builtin extensions (developer) at session start (#10223) fix: parse PyPI requirements for OSV checks (#10510) fix(telegram): contain voice file extensions (#10456) ... # Conflicts: # Cargo.lock # crates/goose/src/agents/extension_manager.rs # crates/goose/tests/mcp_replays/npx-y@modelcontextprotocol_server-everything@2026.1.14
4 participants
Fixes #10765
Initializes the scheduler in
handle_serve_commandright after arg/auth validation instead of waiting for the first client. If init fails it warns and falls back to the current lazy path (the OnceCell stays empty, so the nextcreate_agentretries like before).Tested with an every-minute schedule under an isolated
GOOSE_PATH_ROOTand nothing ever connecting: 0 fires before, fires every minute after. Also added a small test for init/idempotence.Shouldn't fight with #10738:
start_schedulerjust drivesscheduler(), which returns None when the kill switch is set, so eager init becomes a no-op for harnesses that opt out. And this doesn't change the multi-process story, every Desktop window's goosed already starts its scheduler the moment the window connects.