refactor: extract global coordination into invariant-coordinator crate#12
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR introduces two new Rust crates to the workspace: ChangesInvariant Coordinator - Global Lease Coordination
Invariant Scheduler - Local Worker Scheduling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@crates/invariant-coordinator/src/domain/lease.rs`:
- Around line 184-185: The renew method currently assigns self.renewed_at before
computing self.expires_at, causing self.renewed_at to be mutated even if
checked_add_duration overflows; to fix, first compute the new expiration with
now.checked_add_duration(duration.as_duration())? and only if that returns Some
(or Ok) assign both self.renewed_at = now and self.expires_at = new_expires,
otherwise return the error without mutating fields; update the renew
implementation to perform the checked_add_duration call and validation prior to
any mutation of renewed_at/expires_at.
In `@crates/invariant-scheduler/PLAN.md`:
- Line 5: Replace the local filesystem reference
`~/.claude/.../memory/invariant-scheduler-local-design.md` in PLAN.md with a
repository-accessible link or embedded decision info: either point to a
checked-in ADR/decision file (e.g., docs/decisions/invariant-scheduler-*.md)
using a relative path, or list the included decision IDs (1–11) and their
summaries inline so CI/contributors can resolve them without relying on a
home-directory file; update the Locked decisions line accordingly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65588f38-2a8d-4cde-aa94-4f6be84f2b5d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.tomlcrates/invariant-coordinator/Cargo.tomlcrates/invariant-coordinator/src/domain/error.rscrates/invariant-coordinator/src/domain/job.rscrates/invariant-coordinator/src/domain/lease.rscrates/invariant-coordinator/src/domain/mod.rscrates/invariant-coordinator/src/domain/time.rscrates/invariant-coordinator/src/domain/worker.rscrates/invariant-coordinator/src/lib.rscrates/invariant-scheduler/Cargo.tomlcrates/invariant-scheduler/PLAN.mdcrates/invariant-scheduler/src/domain/error.rscrates/invariant-scheduler/src/domain/mod.rscrates/invariant-scheduler/src/domain/time.rscrates/invariant-scheduler/src/lib.rs
| self.renewed_at = now; | ||
| self.expires_at = now.checked_add_duration(duration.as_duration())?; |
There was a problem hiding this comment.
Make renew state updates atomic on overflow failure.
renewed_at is assigned before checked_add_duration. If checked_add_duration overflows, renew returns Err with renewed_at already mutated, leaving inconsistent state after a failed renewal.
Suggested fix
pub fn renew(
&mut self,
credential: &LeaseCredential,
now: SchedulerTime,
duration: LeaseDuration,
) -> Result<(), DomainError> {
self.require_current(credential, now)?;
- self.renewed_at = now;
- self.expires_at = now.checked_add_duration(duration.as_duration())?;
+ let new_expires_at = now.checked_add_duration(duration.as_duration())?;
+ self.renewed_at = now;
+ self.expires_at = new_expires_at;
Ok(())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.renewed_at = now; | |
| self.expires_at = now.checked_add_duration(duration.as_duration())?; | |
| pub fn renew( | |
| &mut self, | |
| credential: &LeaseCredential, | |
| now: SchedulerTime, | |
| duration: LeaseDuration, | |
| ) -> Result<(), DomainError> { | |
| self.require_current(credential, now)?; | |
| let new_expires_at = now.checked_add_duration(duration.as_duration())?; | |
| self.renewed_at = now; | |
| self.expires_at = new_expires_at; | |
| Ok(()) | |
| } |
🤖 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 `@crates/invariant-coordinator/src/domain/lease.rs` around lines 184 - 185, The
renew method currently assigns self.renewed_at before computing self.expires_at,
causing self.renewed_at to be mutated even if checked_add_duration overflows; to
fix, first compute the new expiration with
now.checked_add_duration(duration.as_duration())? and only if that returns Some
(or Ok) assign both self.renewed_at = now and self.expires_at = new_expires,
otherwise return the error without mutating fields; update the renew
implementation to perform the checked_add_duration call and validation prior to
any mutation of renewed_at/expires_at.
|
|
||
| Status: ready to implement · Author hands-on (you implement; this is the spec to work from) | ||
| Research backing: `research/local-layer.md` | ||
| Locked decisions: `~/.claude/.../memory/invariant-scheduler-local-design.md` (decisions 1–11) |
There was a problem hiding this comment.
Replace local filesystem reference with a repo-accessible source.
Line 5 points to ~/.claude/..., which other contributors/CI cannot resolve. Please link to a checked-in ADR/decision doc (or embed the decision IDs here) so the plan remains reproducible.
🤖 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 `@crates/invariant-scheduler/PLAN.md` at line 5, Replace the local filesystem
reference `~/.claude/.../memory/invariant-scheduler-local-design.md` in PLAN.md
with a repository-accessible link or embedded decision info: either point to a
checked-in ADR/decision file (e.g., docs/decisions/invariant-scheduler-*.md)
using a relative path, or list the included decision IDs (1–11) and their
summaries inline so CI/contributors can resolve them without relying on a
home-directory file; update the Locked decisions line accordingly.
Split the local node scheduling layer from the global durable-ownership layer along a hard crate boundary (bounded-context wall). - Add `invariant-coordinator` (WIP): owns leases, fencing epochs, the lease-laden Job aggregate, and WorkerId — the global "who may run this" layer. Parked here until the global layer is fully designed. - Make `invariant-scheduler` lease-free: domain/ is now SchedulerTime + a DomainError trimmed to local variants. No upward dependency on the coordinator crate (work flows down, not up). - No shared-kernel crate: each crate owns its value types and translates at the boundary. All 27 coordinator tests pass unchanged; scheduler tests green.
ab77dd1 to
50afe5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/invariant-coordinator/src/domain/lease.rs (1)
183-186:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCompute the new expiry before mutating
renewed_at.
renewassignsself.renewed_at = nowbefore callingchecked_add_duration. On overflow the method returnsErrwithrenewed_atalready advanced whileexpires_atis unchanged, leaving the lease in an inconsistent state after a failed renewal.Suggested fix
self.require_current(credential, now)?; - self.renewed_at = now; - self.expires_at = now.checked_add_duration(duration.as_duration())?; + let new_expires_at = now.checked_add_duration(duration.as_duration())?; + self.renewed_at = now; + self.expires_at = new_expires_at; Ok(())🤖 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 `@crates/invariant-coordinator/src/domain/lease.rs` around lines 183 - 186, The renew implementation mutates self.renewed_at before computing expires_at, causing an inconsistent state if checked_add_duration overflows; change renew (the method containing self.require_current, self.renewed_at, self.expires_at) to first compute new_expires = now.checked_add_duration(duration.as_duration())? and only after that succeeds assign self.renewed_at = now and self.expires_at = new_expires, preserving atomicity on error.
🤖 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 `@crates/invariant-coordinator/src/domain/job.rs`:
- Around line 134-138: The code in
crates/invariant-coordinator/src/domain/job.rs uses a let-chain (`if let
Some(current) = &self.current_lease && !current.is_expired_at(now)`) which
requires Rust 1.88+, so make the MSRV explicit: add rust-version = "1.88" either
to the root workspace Cargo.toml under a [workspace.package] table (to apply to
all members) or add rust-version = "1.88" to each package Cargo.toml (including
the invariant-coordinator crate) so older toolchains won't fail to compile.
---
Duplicate comments:
In `@crates/invariant-coordinator/src/domain/lease.rs`:
- Around line 183-186: The renew implementation mutates self.renewed_at before
computing expires_at, causing an inconsistent state if checked_add_duration
overflows; change renew (the method containing self.require_current,
self.renewed_at, self.expires_at) to first compute new_expires =
now.checked_add_duration(duration.as_duration())? and only after that succeeds
assign self.renewed_at = now and self.expires_at = new_expires, preserving
atomicity on error.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15368ea0-c831-4c65-b242-6cdbc8c6d30b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.tomlcrates/invariant-coordinator/Cargo.tomlcrates/invariant-coordinator/src/domain/error.rscrates/invariant-coordinator/src/domain/job.rscrates/invariant-coordinator/src/domain/lease.rscrates/invariant-coordinator/src/domain/mod.rscrates/invariant-coordinator/src/domain/time.rscrates/invariant-coordinator/src/domain/worker.rscrates/invariant-coordinator/src/lib.rscrates/invariant-scheduler/Cargo.tomlcrates/invariant-scheduler/src/domain/error.rscrates/invariant-scheduler/src/domain/mod.rscrates/invariant-scheduler/src/domain/time.rscrates/invariant-scheduler/src/lib.rs
| if let Some(current) = &self.current_lease | ||
| && !current.is_expired_at(now) | ||
| { | ||
| return Err(DomainError::JobAlreadyLeased); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect coordinator crate manifest and workspace for edition / MSRV
fd -t f 'Cargo.toml' | while read -r f; do
echo "==== $f ===="
rg -n 'edition|rust-version|\[workspace\.package\]|\[package\]' "$f"
doneRepository: ybird-labs/invariant
Length of output: 473
🏁 Script executed:
#!/bin/bash
# Find any rust-version/MSRV pins across the workspace Cargo.toml files
fd -t f 'Cargo.toml' | while read -r f; do
echo "==== $f ===="
rg -n 'rust-version' "$f" || true
doneRepository: ybird-labs/invariant
Length of output: 318
🌐 Web query:
Rust let chains stabilized version if let ... && condition
💡 Result:
As of May 30, 2026, "let chains" are a stabilized feature in the Rust language, introduced in Rust 1.88 [1][2]. This feature allows the use of let expressions alongside boolean conditions within if and while statements, connected by the && operator [2][3]. Key details regarding this feature include: - Edition Requirement: Let chains are stabilized specifically for the 2024 edition and later editions [1][4]. They are not available in older editions (2015, 2018, 2021) [1][5]. - Functionality: You can intermix let patterns (which may be refutable) with standard boolean expressions [2][3]. The expression evaluates from left to right; if a let pattern match fails or a boolean expression evaluates to false, the subsequent operands are not evaluated (short-circuiting) [5][6]. - Syntax Constraints: If any condition operand is a let pattern, the || operator cannot be used directly in the chain due to ambiguity [5][6]. If a logical OR is required, parentheses must be used (e.g., if let Some(x) = foo && (cond_a || cond_b) { ... }) [5][6]. - Usage: This feature significantly reduces code nesting by allowing flat chains of conditions rather than deeply nested if let blocks [3]. It is also supported in match guards [4][3].
Citations:
- 1: Stabilize let chains in the 2024 edition rust-lang/rust#132833
- 2: Tracking issue for release notes of #132833: Stabilize let chains in the 2024 edition rust-lang/rust#139951
- 3: https://dev.to/kent-tokyo/rust-in-2025-2026-from-most-loved-language-to-core-infrastructure-4l5k
- 4: Tracking issue for eRFC 2497, "if- and while-let-chains, take 2" rust-lang/rust#53667
- 5: https://doc.rust-lang.org/stable/reference/expressions/if-expr.html
- 6: https://github.com/rust-lang/reference/blob/master/src/expressions/if-expr.md
Confirm MSRV for let chains (if let ... && ...)
The code at crates/invariant-coordinator/src/domain/job.rs uses if let Some(current) = &self.current_lease && !current.is_expired_at(now), which requires Rust 1.88+ (let-chains; works with edition = "2024"). All workspace crates use edition = "2024", but no rust-version/MSRV is pinned in the workspace manifests, so older toolchains could fail to compile. Consider adding rust-version = "1.88" to make the MSRV explicit.
🤖 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 `@crates/invariant-coordinator/src/domain/job.rs` around lines 134 - 138, The
code in crates/invariant-coordinator/src/domain/job.rs uses a let-chain (`if let
Some(current) = &self.current_lease && !current.is_expired_at(now)`) which
requires Rust 1.88+, so make the MSRV explicit: add rust-version = "1.88" either
to the root workspace Cargo.toml under a [workspace.package] table (to apply to
all members) or add rust-version = "1.88" to each package Cargo.toml (including
the invariant-coordinator crate) so older toolchains won't fail to compile.
1 participant
What
Slice 0 of the local-scheduler build: split the local node scheduling layer from the global durable-ownership layer along a hard crate boundary.
invariant-coordinatorcrate (WIP) — owns leases, fencing epochs, the lease-ladenJobaggregate, andWorkerId. This is the global "who may run this job" layer, parked here until it's fully designed.invariant-scheduleris now lease-free —domain/is justSchedulerTime+ aDomainErrortrimmed to local variants. It does not depend on the coordinator crate (work flows down, not up).PLAN.md— the slice-by-slice implementation plan for the local layer (Slices 1–5 follow).Why
The old
Jobjammed global lease/fencing concerns together with local scheduling. The crate boundary is the compiler-enforced wall that keeps the two bounded contexts from leaking into each other.Verification
cargo build✅cargo test✅ — 27 coordinator tests pass unchanged, scheduler tests greencargo clippy --all-targets -- -D warnings✅cargo fmt --all --check✅Scope
Mechanical split only. The lease-free local
Job,JobOrder,ReadyQueue,ReadyClock, slot/admission boundaries, and the run loop are future slices perPLAN.md.