Repository navigation
[scheduler/cuebot] Replace Redis-backed accounting with in-memory store + PG LISTEN/NOTIFY - #2472
Merged
DiegoTavares merged 10 commits intoJul 16, 2026
Merged
Conversation
…re + PG LISTEN/NOTIFY The accounting subsystem coordinated Cuebot and the Rust scheduler through Redis to enable horizontal scaling across N scheduler instances. The scheduler is and will remain single-instance (N=1), so Redis's only unique benefit is unreachable while it manufactured an entire class of accounting-drift bugs (limit-seeding fail-closed, mass dispatch rejection, double-booking, CAS starvation). A single in-process counter is the source of truth that makes that bug class structurally impossible. All shows were already drained to Cuebot-managed, so the cutover is clean (deploy into an idle role, then flip shows back one at a time via `b_scheduler_managed`). **Scheduler** — Redis → in-memory `Store`: - `accounting/store.rs`: one `Mutex`, atomic check-and-increment across the three enforced vertices (subscription burst, folder/job max cores+gpus). Layer/point were incremented but never read, so they're dropped. - Live updates via PG `LISTEN/NOTIFY` (`accounting/listener.rs`): `acct_release` and `acct_limit_change`. - Recompute from `SUM(proc)` is the backstop (absolute overwrite, no CAS), carrying in-flight bookings forward via an epoch double-buffer so it can never erase a not-yet-snapshot-visible booking → never over-books a hard cap. - Blocking seeds gate dispatch: bootstrap and managed-flip both seed caps **and** booked counters before enforcing. - Deleted `redis_client.rs`, `lua.rs`, `acct:seq`/CAS, the `redis` dependency. **Cuebot** — `LettuceAccountingRedisPublisher` → `AccountingNotifier`: - Transactional `pg_notify` on proc release (same txn as `DELETE proc` → delivered iff it commits, a stronger model than the old afterCommit publish) and on the five enforced admin cap changes. - `accounting.redis.*` and the Lettuce dep removed; replaced by a safe `accounting.notify.enabled` kill-switch (off → scheduler degrades to recompute-only, which under-books, never over-books). The old over-booking startup guardrail is gone. **Docs** — `redis-accounting.md` → `scheduler-accounting.md` (full rewrite) plus `scheduler.md`, `deploying-scheduler.md`, stress-testing, and properties. - Caps are hard (license/OOM); every failure mode is safe-direction: a dropped NOTIFY leaves a counter reading high → under-book → healed by the next recompute. - N=1 is now an assumption of the in-memory design; multi-scheduler would need a shared store again (revisit trigger documented). - Rust: 177 lib tests pass (incl. straddle, managed-flip, dropped-NOTIFY invariants); clippy clean; stress suite compiles; bin builds. - Cuebot: `compileJava`/`compileTestJava`/`spotlessJavaCheck` pass (JDK 11). rollback).
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change replaces Redis-based scheduler accounting with an in-memory Rust Store synchronized by transactional PostgreSQL LISTEN/NOTIFY messages from Cuebot. It adds cap and release notifications, booking settlement, recomputation, managed-show seeding, stress validation, configuration updates, and corresponding documentation. ChangesIn-Memory Accounting Migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
DiegoTavares
marked this pull request as ready for review
July 9, 2026 22:24
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
rust/crates/scheduler/src/config/mod.rs (1)
52-70: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConfirm the 8x-tighter recompute cadence is sustainable under production DB load.
recompute_intervaldropped from 120s to 15s. Each recompute cycle runsrecompute_all_from_proc+recompute_subscription_table(PG table rewrites) and theSUM(proc)snapshot/baseline reads across all managed shows. With liveacct_releaseNOTIFY now feeding releases between ticks, the recompute is primarily a backstop, so verify the tighter cadence doesn't add avoidable write amplification on large farms. Consider tracking recompute cycle duration/DB write volume and confirm one cycle comfortably finishes well within 15s at peak scale.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/crates/scheduler/src/config/mod.rs` around lines 52 - 70, Validate the reduced cadence in AccountingConfig::recompute_interval under production-scale load by instrumenting recompute_all_from_proc and recompute_subscription_table with cycle-duration and database write-volume metrics. Run representative peak-scale tests or benchmarks to confirm each cycle completes comfortably within 15 seconds; if not, optimize the recompute work or choose a sustainable interval based on the measurements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/_docs/developer-guide/scheduler-accounting.md`:
- Around line 68-71: Update the scheduler accounting documentation to state that
PostgreSQL accounting tables are derived and display-only only for
scheduler-managed shows; explicitly preserve that Cuebot-managed shows use these
tables for transactional enforcement, consistent with the ownership model
described below.
- Around line 380-396: Update the failure-mode documentation table and the
related kill-switch discussion to remove the claim that missed cap notifications
are always safe. In the `acct_limit_change` row, state that a missed cap
decrease can leave a stale higher limit and admit over-cap bookings until the
limit reseed, and revise the surrounding safety claims to distinguish release
misses from cap-change misses. Apply the same correction to the corresponding
section around the kill-switch analysis.
In `@docs/_docs/getting-started/deploying-scheduler.md`:
- Line 40: Update the scheduler deployment guidance near the PostgreSQL
datastore description to document the N=1 accounting constraint: require exactly
one scheduler process per scheduler-managed show, unless show ownership is
provably partitioned without overlap. Clarify that facility-based distribution
alone is insufficient because scheduler instances do not share accounting state.
In `@rust/crates/scheduler/src/accounting/listener.rs`:
- Around line 22-25: The module documentation overstates the safety of dropped
notifications by including acct_limit_change events. Update the documentation
near the listener behavior description to scope the “nothing can over-book a
hard cap” and gap-healing claim specifically to release notifications, while
explicitly acknowledging that dropped limit-lowering notifications may
temporarily leave a stale higher cap enforced until reseeding.
In `@rust/crates/scheduler/src/dao/job_dao.rs`:
- Around line 91-94: Update the comments describing the query’s over-fetch and
false-exclude behavior to refer to the live proc aggregates or lagged PostgreSQL
columns used by the SQL, rather than the in-memory accounting Store. Preserve
the explanations of wasted rejections and starvation while clearly
distinguishing this query source from the scheduler’s hot-path counters.
In `@rust/crates/scheduler/src/metrics/mod.rs`:
- Around line 71-77: Update the documentation for
ACCOUNTING_LIMIT_EXCEEDED_TOTAL to state that it counts both authoritative
accounting-cap rejections and subscription pre-check skips from
pipeline/matcher.rs::process_layer, rather than only attempts reaching the
accounting check.
In `@rust/crates/scheduler/src/models/core_size.rs`:
- Around line 76-77: Update the documentation comment near the core-size cap
handling to remove the obsolete Redis Lua script reference and describe the
in-memory Store’s cap semantics instead, while preserving the explanation of how
the sentinel value is handled.
---
Nitpick comments:
In `@rust/crates/scheduler/src/config/mod.rs`:
- Around line 52-70: Validate the reduced cadence in
AccountingConfig::recompute_interval under production-scale load by
instrumenting recompute_all_from_proc and recompute_subscription_table with
cycle-duration and database write-volume metrics. Run representative peak-scale
tests or benchmarks to confirm each cycle completes comfortably within 15
seconds; if not, optimize the recompute work or choose a sustainable interval
based on the measurements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b71fab0-37a7-4f52-800d-593d1c9fa9d7
📒 Files selected for processing (48)
cuebot/build.gradlecuebot/src/main/java/com/imageworks/spcue/dao/ShowDao.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/GroupDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/JobDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/ProcDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/SubscriptionDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/service/AccountingNotifier.javacuebot/src/main/java/com/imageworks/spcue/service/AccountingRedisPublisher.javacuebot/src/main/java/com/imageworks/spcue/service/LettuceAccountingRedisPublisher.javacuebot/src/main/resources/conf/spring/applicationContext-accounting.xmlcuebot/src/main/resources/opencue.propertiescuebot/src/test/java/com/imageworks/spcue/test/dao/postgres/ProcDaoTests.javacuebot/src/test/java/com/imageworks/spcue/test/service/AccountingNotifierTests.javacuebot/src/test/java/com/imageworks/spcue/test/service/LettuceAccountingRedisPublisherTests.javadocs/_docs/developer-guide/redis-accounting.mddocs/_docs/developer-guide/scheduler-accounting.mddocs/_docs/developer-guide/scheduler-stress-testing.mddocs/_docs/developer-guide/scheduler.mddocs/_docs/getting-started/deploying-scheduler.mdrust/crates/scheduler/Cargo.tomlrust/crates/scheduler/src/accounting/booking_delta.rsrust/crates/scheduler/src/accounting/bootstrap.rsrust/crates/scheduler/src/accounting/dao.rsrust/crates/scheduler/src/accounting/error.rsrust/crates/scheduler/src/accounting/limit_reseed.rsrust/crates/scheduler/src/accounting/listener.rsrust/crates/scheduler/src/accounting/lua.rsrust/crates/scheduler/src/accounting/managed_shows.rsrust/crates/scheduler/src/accounting/mod.rsrust/crates/scheduler/src/accounting/recompute.rsrust/crates/scheduler/src/accounting/redis_client.rsrust/crates/scheduler/src/accounting/store.rsrust/crates/scheduler/src/cluster.rsrust/crates/scheduler/src/cluster_key.rsrust/crates/scheduler/src/config/mod.rsrust/crates/scheduler/src/dao/job_dao.rsrust/crates/scheduler/src/metrics/mod.rsrust/crates/scheduler/src/models/core_size.rsrust/crates/scheduler/src/pipeline/dispatcher/actor.rsrust/crates/scheduler/src/pipeline/dispatcher/error.rsrust/crates/scheduler/src/pipeline/dispatcher/messages.rsrust/crates/scheduler/src/pipeline/entrypoint.rsrust/crates/scheduler/src/pipeline/matcher.rsrust/crates/scheduler/src/pipeline/placement.rsrust/crates/scheduler/tests/active_scan_superset.rsrust/crates/scheduler/tests/redis_integration.rsrust/crates/scheduler/tests/stress_tests.rsrust/crates/scheduler/tests/util/stress.rs
💤 Files with no reviewable changes (9)
- cuebot/src/test/java/com/imageworks/spcue/test/service/LettuceAccountingRedisPublisherTests.java
- cuebot/src/main/java/com/imageworks/spcue/service/LettuceAccountingRedisPublisher.java
- rust/crates/scheduler/src/pipeline/dispatcher/error.rs
- rust/crates/scheduler/src/accounting/lua.rs
- docs/_docs/developer-guide/redis-accounting.md
- rust/crates/scheduler/tests/redis_integration.rs
- cuebot/src/main/java/com/imageworks/spcue/service/AccountingRedisPublisher.java
- rust/crates/scheduler/src/accounting/redis_client.rs
- cuebot/build.gradle
DiegoTavares
commented
Jul 15, 2026
…tric Address code-review findings on the in-memory accounting model: - scheduler-accounting.md: scope "PG tables are display-only" to scheduler-managed shows (Cuebot-managed shows enforce transactionally); correct failure-mode table + kill-switch to distinguish safe release misses from cap-decrease misses that admit over-cap bookings until reseed. - deploying-scheduler.md: document the N=1 accounting constraint (one scheduler process per managed show; facility distribution alone is insufficient since instances don't share accounting state). - listener.rs: scope the "cannot over-book / self-heals" guarantee to acct_release; note dropped cap-lowering acct_limit_change stays stale-high. - job_dao.rs: comment now references the SQL's live proc aggregates / lagged PG columns instead of the in-memory Store hot-path counters. - metrics/mod.rs: ACCOUNTING_LIMIT_EXCEEDED_TOTAL doc now covers both the authoritative cap rejection and the subscription pre-check skip sites; add RECOMPUTE_CYCLE_DURATION_SECONDS histogram to validate recompute cadence. - core_size.rs: replace obsolete Redis Lua-script reference with the in-memory Store's non-positive-cap semantics. - recompute.rs: record recompute cycle wall-clock duration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@cuebot/src/main/resources/conf/ddl/postgres/migrations/V45__show_scheduler_managed.sql`:
- Around line 7-10: Update the migration comment near the ShowDao/ShowInterface
flag description to remove the obsolete claim that no live consumer exists and
all rows remain false. Describe the current behavior now that AccountingNotifier
and the Rust scheduler accounting consumer use the flag, while preserving the
migration’s functional SQL unchanged.
In `@docs/_docs/developer-guide/scheduler-accounting.md`:
- Around line 33-36: Update the scheduler accounting documentation around the
in-process counter to describe ownership by a single scheduler process, with all
state mutations serialized through the mutex. Remove the claim of exactly one
writer and reader, and preserve the explanation that checking and incrementing
under the same lock prevents races.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87a826b9-d120-4071-83fa-2ff810bd5e60
📒 Files selected for processing (17)
.github/workflows/scheduler-stress-pipeline.ymlcuebot/src/main/java/com/imageworks/spcue/service/AccountingNotifier.javacuebot/src/main/resources/conf/ddl/postgres/migrations/V45__show_scheduler_managed.sqldocs/_docs/developer-guide/scheduler-accounting.mddocs/_docs/developer-guide/scheduler.mddocs/_docs/getting-started/deploying-scheduler.mdrust/crates/scheduler/src/accounting/booking_delta.rsrust/crates/scheduler/src/accounting/recompute.rsrust/crates/scheduler/src/accounting/store.rsrust/crates/scheduler/src/config/mod.rsrust/crates/scheduler/src/metrics/mod.rsrust/crates/scheduler/src/pipeline/dispatcher/actor.rsrust/crates/scheduler/src/pipeline/entrypoint.rsrust/crates/scheduler/src/pipeline/placement.rsrust/crates/scheduler/tests/active_scan_superset.rsrust/crates/scheduler/tests/stress_tests.rsrust/crates/scheduler/tests/util/stress.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- rust/crates/scheduler/tests/active_scan_superset.rs
- docs/_docs/getting-started/deploying-scheduler.md
- rust/crates/scheduler/src/metrics/mod.rs
- rust/crates/scheduler/src/pipeline/entrypoint.rs
- rust/crates/scheduler/src/pipeline/placement.rs
- cuebot/src/main/java/com/imageworks/spcue/service/AccountingNotifier.java
- rust/crates/scheduler/tests/stress_tests.rs
- rust/crates/scheduler/src/pipeline/dispatcher/actor.rs
- rust/crates/scheduler/src/accounting/store.rs
- rust/crates/scheduler/src/accounting/recompute.rs
- docs/_docs/developer-guide/scheduler.md
DiegoTavares
merged commit Jul 16, 2026
9091d36
into
AcademySoftwareFoundation:master
31 of 32 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The accounting subsystem coordinated Cuebot and the Rust scheduler through Redis to enable horizontal scaling across N scheduler instances. The scheduler is and will remain single-instance (N=1), so Redis's only unique benefit is unreachable while it manufactured an entire class of accounting-drift bugs (limit-seeding fail-closed, mass dispatch rejection, double-booking, CAS starvation). A single in-process counter is the source of truth that makes that bug class structurally impossible.
Scheduler — Redis → in-memory
Store:accounting/store.rs: oneMutex, atomic check-and-increment across the three enforced vertices (subscription burst, folder/job max cores+gpus). Layer/point were incremented but never read, so they're dropped.LISTEN/NOTIFY(accounting/listener.rs):acct_releaseandacct_limit_change.SUM(proc)is the backstop (absolute overwrite, no CAS), carrying in-flight bookings forward via an epoch double-buffer so it can never erase a not-yet-snapshot-visible booking → never over-books a hard cap.redis_client.rs,lua.rs,acct:seq/CAS, theredisdependency.Cuebot —
LettuceAccountingRedisPublisher→AccountingNotifier:pg_notifyon proc release (same txn asDELETE proc→ delivered iff it commits, a stronger model than the old afterCommit publish) and on the five enforced admin cap changes.accounting.redis.*and the Lettuce dep removed; replaced by a safeaccounting.notify.enabledkill-switch (off → scheduler degrades to recompute-only, which under-books, never over-books). The old over-booking startup guardrail is gone.Docs —
redis-accounting.md→scheduler-accounting.md(full rewrite) plusscheduler.md,deploying-scheduler.md, stress-testing, and properties.Caps are hard (license/OOM); every failure mode is safe-direction: a dropped NOTIFY leaves a counter reading high → under-book → healed by the next recompute.
N=1 is now an assumption of the in-memory design; multi-scheduler would need a shared store again (revisit trigger documented).
Rust: 177 lib tests pass (incl. straddle, managed-flip, dropped-NOTIFY invariants); clippy clean; stress suite compiles; bin builds.
Cuebot:
compileJava/compileTestJava/spotlessJavaCheckpass (JDK 11).Summary by CodeRabbit
New Features
Bug Fixes
Documentation