fix(server): make scheduled-task edits atomic#11592

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This PR introduces a new atomic scheduled-task update API and changes task persistence, scheduling, dispatch, recovery, project scoping, webhook handling, MCP authorization, and both web and mobile save flows. Its broad concurrency-sensitive production impact exceeds the scope of an automatically approvable change.

Needs human approval because:

• Per-PR cost limit exceeded (workspace setting). Approvability relies on correctness review in order to determine eligibility

^{Review your spending limits in Billing settings, or comment @macroscope-app review this PR to bypass the limit and review now. You can add or adjust custom eligibility rules. Learn more.}

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

Macroscope skipped reviewing this pull request. Per-PR cost limit exceeded (workspace setting).

Reviews on this PR have cost $49.69 so far. This review would add an estimated $3.56, bringing the total to $53.25 — above your per-PR limit of $50.00.

^{Turn off this reminder going forward}

Effect Service Conventions found 1 issue in apps/server/src/scheduledTasks/ScheduledTaskService.ts: an Effect subpath module was imported as a named symbol instead of a namespace. See the inline review comment for the expected fix.

^{Posted via Macroscope — Effect Service Conventions}

Effect Service Conventions found 1 issue in apps/server/src/mcp/OrchestratorMcpService.ts: a statically known ScheduledTaskError is handled with Effect.mapError instead of Effect.catchTags. See the inline review comment for the suggested fix.

^{Posted via Macroscope — Effect Service Conventions}

Effect Service Conventions found 1 issue in apps/server/src/mcp/OrchestratorMcpService.ts: a statically known ScheduledTaskError is handled with Effect.mapError instead of Effect.catchTags. See the inline review comment for the suggested fix.

^{Posted via Macroscope — Effect Service Conventions}

All clear

^{Posted via Macroscope — Effect Service Conventions}

Please hold this PR for the newly reproduced integration finding shared with #11592/#11635. Individually green checks missed conflicting Settings behavior when a stale scheduled-task editor saves a project move: one helper emits threadId: null and the other omits the binding field, preserving a concurrent binding.

The existing #11592 owner is reconciling explicit project-move intent, authorization, and stale-editor concurrency with focused regression tests; #11635 will consume the same semantics afterward. The fixes will update these existing PRs. The frozen main-integration rehearsal otherwise passed 1,530 tests with zero failures; this behavior disagreement still needs an explicit resolution before we request these two merges.

Reconciled in 6db4c8b45b — the canonical semantics ships in this PR: scheduledTasks.logic.ts::buildScheduledTaskUpdateInput emits threadId: null only when the patch itself re-homes the task away from the live binding's project (nextProjectId !== undefined && nextProjectId !== task.projectId && task.threadId !== null).

Rules:

• Real project move on a bound task → nextProjectId + threadId: null (the server's merged-pair check rejects an invalid pair otherwise).
• Stale move landing where the task already lives → no threadId in the patch → a binding committed mid-session by another client is preserved (valid in the destination).
• The dialog has no thread picker, so threadId is never emitted from draft state — a stale editor can neither re-attach nor erase a concurrent binding.

Matrix coverage (deterministic tests): bound/unbound moves, concurrent move+rebind preservation, third-project detach, untouched-binding saves, reverse move-back, retry-after-committed-move — scheduledTasks.logic.test.ts. #11635 consumes the same semantics on rebase.

@juliusmarminge The integration hold is cleared: #11592 and #11635 now agree on live-aware thread detachment when moving a scheduled task. The stale-editor/concurrent-binding regression matrix passes in the composed candidate. Please review and merge the coordinated pair; land #11592 first, then preserve the documented resolution when rebasing #11635.

Fresh check, 19 September 09:11 Sydney: this PR is mergeable and every current check is green or skipped at e1e86cb79aed228049d7e435616730eec551acb5. The frozen post-fix current-main rehearsal passed 1,490 tests with zero failures and zero merge conflicts, plus Cursor recovery 7/7. That rehearsal is scoped to its recorded inputs; later rebases have their own current-head CI. Latest-head independent agent review remains pending capacity; no new clean review is claimed.

Both P2 stale-save findings addressed in bd091a7a56 (15 files, same atomic-update commit):

1. Mobile dirty patches — SettingsScheduledTasksRouteScreen edit path now builds a ScheduledTaskUpdateInput via buildScheduledTaskUpdateInput(draft, liveTask) (in scheduledTaskDraft.ts) instead of a full-row upsert. The opening snapshot stays the baseline; projectId scopes to the live row; a delete racing the save is a typed not-found. runtimeMode added to ScheduledTaskUpdateInput + the server's partial-write set so mobile's editable runtime mode rides the same path. Creates still upsert.
2. Web workspace per-control overlay — scheduledTasks.logic.ts now compares each workspace control against its opening baseline and overlays only dirty controls onto the live strategy: a base-ref-only edit keeps a concurrent startFromOrigin change (and a concurrent kind switch drops the stale control edit rather than resurrecting the old kind).

Regressions: mobile stale rename vs concurrent prompt+enabled (patch carries title only), runtimeMode patch, live-aware detach on re-home, base-ref edit after concurrent origin-toggle, kind-switch omit, server runtimeMode write — 105/105 focused tests pass; web/mobile/server/contracts typechecks clean.

Effect Service Conventions found 1 issue in apps/server/src/scheduledTasks/ScheduledTaskService.ts: new transaction wrappers manually discriminate ScheduledTaskError inside mapError instead of handling the known tag with Effect.catchTags. See the inline review comment.

Suggested fix: replace each manual isScheduledTaskError branch with Effect.catchTags for ScheduledTaskError, then map only the remaining lower-level failure.

^{Posted via Macroscope — Effect Service Conventions}

All clear

^{Posted via Macroscope — UI Consistency}

All clear

^{Posted via Macroscope — UI Consistency}

All clear

^{Posted via Macroscope — UI Consistency}

All clear

^{Posted via Macroscope — Effect Service Conventions}

Review in Change Stack →

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

_{Comment @coderabbitai help to get the list of available commands.}

@juliusmarminge, here is where each point from your 18 September review stands. Current head: ee31791478, rebased onto main fed41fa88b.

1. P2, mobile stale full-row upsert. Fixed. When editing an existing task, the mobile editor now sends a dirty-field ScheduledTaskUpdateInput from buildScheduledTaskUpdateInput(draft, liveTask) and calls updateScheduledTask. Only creating a task still uses upsert. runtimeMode is part of the update input. Regressions in apps/mobile/src/features/settings/scheduledTaskDraft.test.ts:
   • "sends only the fields a stale editor changed, preserving concurrent edits": a stale rename after another client changes the prompt and pauses the task sends { title } only.
   • "patches the editable runtime mode".
2. P2, web workspace controls. Fixed, and the inline thread is resolved. Edits that keep the same strategy kind send a sparse workspaceStrategyPatch with only the changed controls. The server merges it into the live strategy inside the update transaction. Your repro is covered: a base-ref-only edit after a concurrent startFromOrigin change keeps that change.
3. CAS and re-read branches. Removed in ee31791478, as you suggested. update and setEnabled no longer guard on enabled/schedule_json, and they no longer re-read to tell a delete apart from a contested edit. Same-connection transactions are serialized, and a cross-connection commit raises SQLITE_BUSY_SNAPSHOT, which retryContended retries on a fresh snapshot. The file-database tests with a second physical connection still pass without that code: disjoint fields, schedule vs. committed disable, and delete inside the read window.
4. Size.
   • The pause, postpone and delete dispatch-gap cases are now one it.effect.each.
   • ScheduledTasksSettings.tsx imports the single scheduleFromDraft from scheduledTasks.logic.ts. The duplicate is gone, and invalid intervals are still rejected at submit.
   • I left the project-move delete and runNow cases separate. They run on different layers (plain vs. gated dispatch), so parameterizing them would mostly add setup plumbing.
   • Net change in this pass: -69 lines.
5. Unused ScheduledTaskError import. It is no longer present. Targeted lint on the changed scheduled-task, MCP and settings paths reports no new warnings.

I also fixed CodeRabbit's open finding: upsert now validates a thread binding only when the write changes it.

Verification on ee31791478:

• ScheduledTaskService.test.ts + OrchestratorMcpToolkit.integration.test.ts: 31 passed.
• Web scheduledTasks.logic.test.ts: 28 passed.
• Mobile scheduledTaskDraft.test.ts: 40 passed.
• Server and web typechecks clean.

An independent GPT-6.1-Sol review (high reasoning) of this change and the re-stacked #11635 approved it with no actionable findings.

Could you take another look?

Review requested

_{Logged so this PR shows when a maintainer was asked to review it.}

@juliusmarminge Rebased onto current main (9bd1d8009a) as 697ecb3790. The six earlier commits are squashed into one, so the conflicts with webhook-triggered scheduled tasks are resolved once.

Both points from your 18 September review still hold after the rebase:

1. Mobile stale full-row upsert. Fixed. Edits send a dirty-field ScheduledTaskUpdateInput. The regression "sends only the fields a stale editor changed, preserving concurrent edits" still passes.
2. Web workspace controls. Fixed. Edits send a sparse workspaceStrategyPatch, and your base-ref repro is still covered.

The rebase also needed the atomic update to support webhook triggers:

• update follows the same webhook rules as upsert. Switching to a webhook creates its URL token. A signature without a new secret keeps the stored secret. A secretRef is spent once, before the write transaction. Leaving the webhook trigger clears the token and secret.
• The web and mobile patch builders compare webhook triggers correctly. A save that does not touch the trigger leaves it out of the patch, so it cannot overwrite a concurrent signature change. Typing a new secret counts as a change.
• A scoped delete also deletes the task's webhook delivery log, in the same transaction. A delivery that was waiting while its task was paused, replaced or switched to another trigger is still skipped, now inside the mark-running transaction.

New regressions: server "update gives a webhook trigger its URL token and signing secret", plus web and mobile "unchanged webhook trigger" patch tests.

Locally: server src/scheduledTasks + src/mcp (290 tests), web logic (30), mobile draft (44) and contracts all pass. Server, web, mobile, contracts and client-runtime typecheck clean, apart from the known main breakage in externalLauncher.test.ts (fixed by #16436).

Review requested

_{Logged so this PR shows when a maintainer was asked to review it.}

The diff adds the Project field's "detaches the thread binding" hint and a mobile "Task unavailable" alert, despite the description saying nothing changes visually. Please add before/after screenshots of these states under the verification rule. The focused concurrency tests support the backend change; this request is for the added UI.

Re the screenshot request (https://github.com/pingdotgg/t3code/pull/11592#issuecomment-6115290747): the description no longer says nothing changes visually.

Web Project hint, same flow on a real dev server (task bound to a thread in project A, edit, switch Project to B):

The native mobile "Task unavailable" before/after will be added from the iOS simulator in a follow-up comment.

Also in this push: merged main, c7177bef30 keeps the SecretRequestError as the cause of a failed secretRef save/edit and adds the scheduledTasks.update permission behavior test.

@coderabbitai Re the merge-risk sentence "saving a task that is still bound to an archived thread fails when the save repeats the existing binding": that refers to e704c3b213, an earlier head of this branch. On the current head 25385e33e6, upsert and update only check a thread binding when the save changes it (bindingChanged in ScheduledTaskService.ts), so resending the stored binding of a since-archived thread saves normally. Two regression tests cover this: "update keeps an unchanged binding to a since-archived thread editable" and "legacy upsert keeps an unchanged binding to a since-archived thread editable". The MCP update path only sends a binding when bindToCurrentThread is set, and that thread is the caller's own running thread.

@saphid Thanks for the clarification. I checked 25385e33e6 and withdraw that merge-risk sentence. It does not apply to the current head.

• Both upsert and update validate a non-null thread binding only when the thread/project pair changes.
• Both regression tests cover saving an unchanged binding after the thread is archived. They also check that a changed binding still fails validation.
• The MCP update path omits threadId when bindToCurrentThread is absent. Explicit true binds to the caller’s thread; explicit false clears the binding.

I inspected the code and tests. I did not run the tests.

You are interacting with an AI system.

@juliusmarminge I re-checked each point of your 18 September review against the current head 25385e33e6, which merges cleanly with main 614b432a79. Nothing new needed fixing. Here is where each point is handled:

1. P2, mobile overwrites concurrent edits. Fixed in 59ed76f300. An edit now sends a dirty-field ScheduledTaskUpdateInput from buildScheduledTaskUpdateInput through updateScheduledTask. That input includes runtimeMode. Only create still uses upsert. Regressions in apps/mobile/src/features/settings/scheduledTaskDraft.test.ts:

   • "sends only the fields a stale editor changed, preserving concurrent edits": your sequence. A stale rename after another client rewrites the prompt and pauses the task sends { title } only.
   • "patches the editable runtime mode".

   On the server, the server test "update keeps disjoint concurrent edits and untouched fields" shows that a patch changes only the columns it names.

2. P2, web workspace edits overwrite untouched controls. Fixed in 59ed76f300. A same-kind edit sends a sparse workspaceStrategyPatch with only the changed controls. The server merges that patch into the live strategy inside the update transaction (ScheduledTaskService.ts#L1486). Your repro, a base-ref edit after a concurrent startFromOrigin: false, is covered by these tests:

   • web: "sends only the workspace controls the user changed as a sparse patch"
   • mobile: the same case
   • server: "concurrent workspaceStrategyPatch edits merge into the live strategy"
   • server: "…drops keys foreign to a concurrently switched kind"

   The inline thread is resolved.

Nonblocking notes:

• CAS and re-read branches. Removed. update and setEnabled now do a scoped read and a plain UPDATE … WHERE task_id AND project_id inside one retryContended(sql.withTransaction(…)). A SQLITE_BUSY_SNAPSHOT is retried there.
• Repeated dispatch cases. Parameterized with it.effect.each (test#L1327).
• Unused ScheduledTaskError import. Gone. Targeted lint on the 25 changed files reports no new warnings. The four warnings left in ws.ts and rpc.ts are already on main.
• scheduleFromDraft. This PR adds no copy. Web and mobile each keep the one they already had on main, because their draft types differ.

Validation on 25385e33e6:

• Focused tests pass: server 134 (scheduledTasks + MCP), web 30, mobile 44.
• Typecheck passes for server, web, mobile, contracts and client-runtime.

Taking over this one as maintainer for batch scheduler-tasks-05. I will rebuild the atomic edit path against current main, preserve the stale-editor and authorization semantics, and split the server API, client editors, and scheduler transaction changes into focused review layers. The original author will be credited; no merge is planned.

Thanks @saphid — the atomic patch design correctly identified why concurrent edits lose fields and stale saves recreate deleted tasks. I rebuilt this against the current scheduler and client architecture and published it as three focused layers:

• #21791: atomic scoped service updates, protected RPC/capability, and live MCP authorization.
• #21792: dirty-field web/mobile edits, sparse workspace changes, legacy fallback, and live-aware binding detachment.
• #21793: transactional reservation, completion, missed-run rescheduling, and recovery.

I compared the original pinned diff against the complete replacement. Mobile runtime-mode edits, the previously requested sparse base-ref/origin behavior, changed-binding validation, webhook credential preservation and secret-error causes, due times and run history are all retained. Current SQLite write transactions replace the extra retry/CAS machinery. Incidental Project hint/mobile alert additions and redundant test matrices were omitted; missing tasks disable Save instead. The existing legacy-interval notice is preserved and now verified for title-only saves.

The original added 66 cases and 2,071 test lines; the replacement adds 19 focused cases in five existing files (652 added/4 removed lines), plus adaptations to existing MCP fixtures. Final focused suites pass 123 cases. Two independent full-diff review rounds (Opus 5.5 and GPT-6 Sol) found and fixed resulting-thread mode, live manual-trigger, and sub-minute fallback issues; the second round found no code blockers. Current-head web proof covers two-client concurrent saves, deleting an open editor, and legacy interval normalization, with images/recording in the client PR. Native platforms and actual old-server fallback runtime remain unverified.

Closing in favor of the linked stack, with credit retained in every layer. Nothing has been merged. Fresh CI and human review remain required.

Maintainer takeover: GPT-6.1 Sol (high), Codex harness through T3 Code; tests GPT-6 Astra, independent reviews Claude Opus 5.5/GPT-6 Sol.