Skip to content

fix(core): isolate scheduled prompt sessions by default - #1651

Open
AaronZ345 wants to merge 2 commits into
chenhg5:mainfrom
AaronZ345:fix/cron-busy-side-session-20260806
Open

fix(core): isolate scheduled prompt sessions by default#1651
AaronZ345 wants to merge 2 commits into
chenhg5:mainfrom
AaronZ345:fix/cron-busy-side-session-20260806

Conversation

@AaronZ345

Copy link
Copy Markdown
Contributor

Summary

  • Default cron and timer prompt jobs to a fresh agent session per run.
  • Keep the old shared-session behavior available only when session_mode = "reuse" or --session-mode reuse is set explicitly.
  • Update CLI help, config comments, docs, changelog, and tests for the new default.

Root cause

Scheduled prompt jobs inherited the active chat session by default. When a cron fired while the user session was already busy, it could fail with a busy-session error or wait behind unrelated interactive work. The scheduled task should be isolated by default instead of competing with normal chat turns.

Test Plan

  • go test -count=1 ./core ./config
  • GOOS=linux GOARCH=amd64 go test -c -tags no_web ./cmd/cc-connect
  • git diff --check

Supersedes #1349. This is the same rebased patch on a fresh PR branch.

Supersedes #1496. This fresh replacement carries the same patch rebased onto the latest main after the original PR exceeded 30 days.

@AaronZ345
AaronZ345 requested a review from chenhg5 as a code owner August 6, 2026 14:44
@AaronZ345
AaronZ345 force-pushed the fix/cron-busy-side-session-20260806 branch 3 times, most recently from bf770ec to 0ef2daa Compare August 13, 2026 14:44
@AaronZ345
AaronZ345 force-pushed the fix/cron-busy-side-session-20260806 branch from 0ef2daa to 1093542 Compare August 14, 2026 14:43

@chenhg5 chenhg5 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conclusion: Approve

Overall assessment:
This PR flips the default session mode for cron/timer prompt jobs from reuse to new_per_run, fixing the long-standing busy-session conflict where scheduled jobs competed with active chat turns. Behavior change is intentional, fully documented, and gated behind a clean opt-out (session_mode = "reuse"). Local go test -race ./... -count=1 -timeout 60s passes for the in-scope packages (core, config, cmd/cc-connect with -tags no_web); the only failing targets are pre-existing env issues (missing web/dist, cursor login requirement) and are unrelated to this patch.

Review scope:

  • Read core/cron.go, core/timer.go, core/interfaces.go, cmd/cc-connect/{cron,timer}.go, config/config.go, config.example.toml, all updated tests, and the touched doc files.
  • Walked through NormalizeCronSessionMode, UsesNewSession, validateCronJob, UpdateJob, and the two scheduler constructors to confirm the new default and the explicit-reuse path are wired end-to-end.
  • Ran go test -race ./core ./config ./cmd/cc-connect (with -tags no_web); all pass. The two unrelated failures (cmd/cc-connect without no_web needs web/dist; agent/acp.TestCursorCLI_ACPHandshake needs agent login) reproduce on main and are not regressions.

✅ What looks good:

  • defaultScheduledSessionMode as a single named constant removes the previous "empty string means reuse" ambiguity. NormalizeCronSessionMode now distinguishes "" (inherit default), "reuse", and "new_per_run" cleanly.
  • validateCronJob and UpdateJob both accept the explicit reuse value with a clear error message — opt-out is well supported.
  • The CHANGELOG entry is concise and tells users exactly what to do if they need the old behavior.
  • The CLI help text, the management API docs (both EN and ZH-CN), and config.example.toml were all updated consistently. The --session-mode and [cron].session_mode defaults are flipped everywhere users could discover them.
  • TestExecuteCronJob_ResolvesCronReplyTarget was correctly retrofitted with SessionMode: "reuse". That single-character edit shows the author understood the default flip and audited the test corpus.

🟠 Should improve:

  • Merge ordering: this PR is the prerequisite for #1650 (hide scheduler background sessions). On its own, #1650 is dormant because defaultSessionMode == "" means useNewSession=false and NewBackgroundSession is never reached. The CHANGELOG doesn't mention this dependency; merging them out of order leaves #1650 silently invisible to default users. Worth a brief coordination note in the PR body or in the team handoff.
  • Migration audit: only one existing test (TestExecuteCronJob_ResolvesCronReplyTarget) was updated for the new default. Any other test that exercises ExecuteCronJob / ExecuteTimerJob with an empty SessionMode would have changed semantics silently. I ran the targeted test set (TestExecuteCronJob_*, TestExecuteTimerJob_*, TestSessionManager_*, TestValidateTimerJob, TestMgmt_CronExecByID) and all pass, but a CI run on the full go test ./... should confirm there are no other implicit-dependency tests in the suite that this author missed.
  • Implicit config-default flip: [cron] section in config.example.toml changes the documented default from reuse to new_per_run. Operators who copy config.example.toml as a starting point will silently get the new behavior even if their previous cc-connect.toml had no [cron] block. This is fine but is the kind of thing that shows up as a "why did my cron behavior change?" support ticket later. Consider adding a one-line "since vX.Y, default is now new_per_run" callout in docs/usage.md (currently the prose was rewritten but doesn't carry a "changed in this release" anchor).

🔵 Optional:

  • The dead-code "" / "reuse" collapse removed in NormalizeCronSessionMode is good — but the comment block above defaultScheduledSessionMode could mention the previous "empty == reuse" semantic so future readers understand why the constant exists.
  • TestCronScheduler_AddJob_NormalizesSessionMode now also asserts that "" round-trips as "". Adding a t.Cleanup-style assertion that an invalid mode (e.g. "foobar") is rejected at validation would lock the current behavior in case someone refactors NormalizeCronSessionMode later.

❓ Questions:

  • Did you audit the Web management API's cron-create/edit forms for any UI elements that hard-code reuse as the default radio button / dropdown selection? If the UI sends back whatever it displays, that would mask the new backend default for users who touch cron jobs through the web console.

Testing / Risk:

  • Verified: go test -race ./core ./config ./cmd/cc-connect with -tags no_web is green. TestCronScheduler_UsesNewSession_GlobalDefault, TestTimerScheduler_UsesNewSession_Defaults, TestValidateTimerJob, and TestExecuteCronJob_ResolvesCronReplyTarget all pass.
  • Unverified risk: any consumer of NormalizeCronSessionMode outside core/ and cmd/cc-connect/ (e.g. web/, tests/blackbox/) that relies on "" collapsing to "reuse" would now see different behavior. Worth a quick grep -rn "NormalizeCronSessionMode" --include="*.go" to confirm scope.

Next step:

  • Land this PR first; then immediately follow with #1650 so the new default is paired with the hide-from-list behavior. Update the management API web form defaults in a follow-up if any UI elements still show reuse as the default selection.

@AaronZ345
AaronZ345 force-pushed the fix/cron-busy-side-session-20260806 branch from a5842a8 to 4b4221e Compare August 16, 2026 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants