Skip to content

Read routine transitions and schedules under the owner cap lock - #925

Open
rudycelekli wants to merge 1 commit into
CopilotKit:mainfrom
rudycelekli:fix/routine-cap-current-state-20261031
Open

rudycelekli wants to merge 1 commit into
CopilotKit:mainfrom
rudycelekli:fix/routine-cap-current-state-20261031

Conversation

@rudycelekli

Copy link
Copy Markdown

What this changes

A routine update reads its current state before taking the owner's cap lock. At 20 enabled routines, a queued pause, create, and enable can therefore leave 21 enabled: the last update still thinks the routine was enabled and skips the cap check. Concurrent cron and timezone edits can also save one schedule while calculating the next firing from an older one.

Read the current routine and derive its transition and next firing inside the existing owner transaction lock, alongside the count and write. Ownership and channel checks remain in place; channel resolution uses that same transaction so a one-connection pool can complete the update.

Where it runs

  • New state that outlives a request? None. Routine state remains in shared Postgres.
  • What happens on the second replica? Its queued update reads the committed state after the preceding owner's operation finishes.
  • Anything serialised? The existing per-owner advisory transaction lock now covers the read and derived values as well as the count and write. No second pooled connection is acquired while holding it.
  • Anything fanned out to a browser? No new fan-out.
  • New listener, port, or schedule? None.

Boundary and audit

  • Routine tool calls retain the existing gateway and audit path. Store ownership and shared-channel membership checks remain enforced.
  • The existing cap refusal and response are reused; no new failure category or audit event is introduced.
  • No new client-supplied trust or permissions.

Changelog

  • Added an Unreleased entry explaining the cap and clock corrections.

Proof

Base 743649efc3f21d7bf0badbe6af2bcd5daedd97ce, head 5def6bc7562973f79a4a44c7c5265b947de05e3a.

Two regressions run actual store methods on separate PostgreSQL connections. A real transaction holds the owner's existing advisory lock while the operations queue; the tests observe the sessions' database lock waits before releasing it, without sleeps or mocked locks.

  • Pause → create → enable at the cap: unchanged upstream leaves 21 enabled; afterward the enable gets the existing friendly refusal and the count remains 20.
  • Cron → timezone edits: unchanged upstream saves 0 17 * * * and America/New_York but schedules 09:00 there. Afterward the next firing is 17:00 in the stored zone, independently checked with Intl. The update also resolves an explicit channel through a one-connection pool.
  • All 11 routine test files pass together: 233 tests, 729 assertions, including ownership, channel refusals, schedules, routes, runner, and sweep controls.
  • Format, lint, workspace typecheck, the test-typecheck baseline guard, and build pass on this exact head.

The full suite was not run on this head. A separate routing candidate (d428f669f10e21700b6d5eb1fb14e90c9cc59cfe) on the same base ran full test:ci: 11,017 passed, 210 skipped, two failed (routine-sweep concurrency expectation and recorder held-steps service-worker timing). Both cases passed when isolated on unchanged base and that routing candidate; full-suite order/timing causality remains unresolved. The routine-sweep file passes in this candidate's routine-family run.

Integration tests use a dedicated disposable PostgreSQL 15 database (hosted CI uses 17), with no application data or live provider calls. PR #921 changes a separate sweep test; it does not change this store or these regressions.

AI assistance: implemented and locally validated with OpenAI Codex. Hosted CI and maintainer review remain required.

Implemented and locally validated with OpenAI Codex assistance.

This branch has not been deployed

No deployments
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.

1 participant