feat(trogon-nats): extract release logic from CRON#115
Merged
Conversation
PR SummaryMedium Risk Overview Extends the JetStream abstraction layer to support KeyValue operations (create/get bucket, status, create-with-ttl, update, delete-with-expected-revision) and adds corresponding mocks for testing. Bumps the docker-compose NATS image to Reviewed by Cursor Bugbot for commit 148458b. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
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:
WalkthroughAdds a KV-backed lease and distributed leadership system to the trogon-nats crate, including validated lease identifiers, TTL/renew timing types, async lease traits and a NATS JetStream KV implementation, plus a LeaderElection state machine. Also enables the Changes
Sequence Diagram(s)sequenceDiagram
actor App as Application
participant LE as LeaderElection
participant NKL as NatsKvLease
participant KV as NATS_KV
App->>LE: new(lock, node_id, timing)
App->>LE: ensure_leader()
alt Not leader
LE->>NKL: try_acquire(value)
NKL->>KV: Put/create key (expect create)
KV-->>NKL: revision
NKL-->>LE: Ok(revision)
LE->>LE: set is_leader, store revision
LE-->>App: Ok(true)
else Leader & renew needed
LE->>NKL: renew(value, revision)
NKL->>KV: Update with expected revision
KV-->>NKL: new_revision or WrongLastRevision
NKL-->>LE: Ok(new_revision) / Err
LE->>LE: update revision / clear state on error
LE-->>App: Ok(true) / Err(EnsureLeaderError::Renew)
end
App->>LE: release()
alt leader with revision
LE->>NKL: release(revision)
NKL->>KV: Delete with expected revision
KV-->>NKL: Ok() or WrongLastRevision
NKL-->>LE: Result
LE->>LE: clear leader state
LE-->>App: Ok(())
else not leader or no revision
LE->>LE: clear leader state
LE-->>App: Ok(())
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Code Coverage SummaryDetailsDiff against mainResults for commit: 148458b Minimum allowed coverage is ♻️ This comment has been updated with latest results |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rsworkspace/crates/trogon-nats/src/lease.rs`:
- Around line 172-190: The Lease trait currently groups three operations
(try_acquire, renew, release) making downstream bounds too broad; split it into
three single-operation traits—e.g., TryAcquireLease with fn try_acquire(&self,
value: Bytes) -> impl Future<Output = Result<TryAcquireOutcome, Self::Error>> +
Send, RenewLease with fn renew(&self, value: Bytes, revision: u64) -> impl
Future<Output = Result<u64, Self::Error>> + Send, and ReleaseLease with fn
release(&self, revision: u64) -> impl Future<Output = Result<(), Self::Error>> +
Send—each trait should keep the same associated type constraint (type Error:
std::error::Error + Send + Sync) and Send+Sync+Clone+'static bounds as needed;
then replace uses of the composite Lease bound with the minimal combination of
these new traits where only specific operations are required.
- Around line 18-25: Update the LeaseBucket::new and LeaseKey::new constructors
to validate names at construction time: reject empty values and enforce NATS KV
character and length rules (bucket: only ASCII alphanumeric, dash and
underscore; key: must match ^[-/_=\.a-zA-Z0-9]+\z and reasonable nonzero
length). Return a descriptive LeaseConfigError (add variants such as
InvalidBucketName and InvalidKeyName if needed) when validation fails instead of
allowing invalid instances; implement the checks with a simple char-class/regex
test in LeaseBucket::new and LeaseKey::new so invalid identifiers are
unrepresentable.
- Around line 193-255: The code currently wraps SDK errors in the custom
LeaseError and mutates SDK semantics; remove the LeaseError wrapper and change
NatsKvLease to expose the SDK error types directly via the trait associated
types (set type Error to the kv SDK error type(s) used and add type RevisionOut
to match SDK return values), then update function signatures/return types to
return the SDK errors (remove map_err in renew() that maps to LeaseError and any
other map_errs that wrap to LeaseError), and stop converting
DeleteErrorKind::WrongLastRevision into Ok(()) inside map_release_result() —
instead return the SDK DeleteError as-is so higher layers can handle semantics;
locate and modify the LeaseError enum and its helper methods (provision_source,
acquire_source, renew_source, release_source), the renew() implementation that
calls map_err, and the map_release_result() helper to remove wrapping and pass
through SDK errors.
🪄 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: CHILL
Plan: Pro
Run ID: 556c11bd-b914-4e26-a250-82a27d8715df
📒 Files selected for processing (3)
rsworkspace/crates/trogon-nats/Cargo.tomlrsworkspace/crates/trogon-nats/src/lease.rsrsworkspace/crates/trogon-nats/src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rsworkspace/crates/trogon-nats/src/lease.rs`:
- Around line 428-444: In release(), treat kv::DeleteErrorKind::NotFound as an
idempotent success similar to WrongLastRevision: after calling
self.lock.release(revision).await and before returning Err(source), add a branch
that checks if source.kind() == kv::DeleteErrorKind::NotFound and return Ok(())
for that case; keep the existing Ok(()) and WrongLastRevision branches and leave
other errors propagated as Err(source).
🪄 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: CHILL
Plan: Pro
Run ID: 9d20e10e-64c3-476a-8f15-43dbeba9e1f6
📒 Files selected for processing (2)
rsworkspace/crates/trogon-nats/src/lease.rsrsworkspace/crates/trogon-nats/src/lib.rs
yordis
added
the
rust:coverage-baseline-reset
yordis
force-pushed
the
yordis/trogon-nats-releasing
branch
from
5c0ba0a to
8f8924c
Compare
yordis
removed
the
rust:coverage-baseline-reset
There was a problem hiding this comment.
♻️ Duplicate comments (1)
rsworkspace/crates/trogon-nats/src/lease/mod.rs (1)
450-455:⚠️ Potential issue | 🟠 MajorTreat
NotFoundas an idempotent release outcome.
release()clears local leadership state before matching the SDK result. If the lease entry has already expired or been removed, surfacingDeleteErrorKind::NotFoundhere turns a completed cleanup into a false failure. Handle it the same way asWrongLastRevision.💡 Proposed fix
match result { Ok(()) => Ok(()), Err(source) if source.kind() == kv::DeleteErrorKind::WrongLastRevision => Ok(()), + Err(source) if source.kind() == kv::DeleteErrorKind::NotFound => Ok(()), Err(source) => Err(source), }In async-nats 0.47.0 JetStream KV, what `DeleteErrorKind` does `Store::delete_expect_revision` return when the key is already missing or has expired? Is `DeleteErrorKind::NotFound` the expected case, and should callers treat it as an idempotent success during cleanup?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-nats/src/lease/mod.rs` around lines 450 - 455, The release path currently treats kv::DeleteErrorKind::WrongLastRevision as an idempotent success but not kv::DeleteErrorKind::NotFound; update the match in the release() flow (the block after calling self.lock.release(revision).await and self.clear_state()) to treat Err(source) where source.kind() == kv::DeleteErrorKind::NotFound the same as WrongLastRevision (return Ok(())), so that already-missing or expired keys don't surface as errors; keep the existing Err(source) => Err(source) fallback for other kinds.
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-nats/src/lease/mod.rs (1)
100-106: Keep lease timing accessors typed.These getters unwrap
LeaseTtlandLeaseRenewIntervalback to rawDuration, which weakens the value-object boundary you just introduced. Prefer returning the domain types here and converting toDurationonly at the JetStream boundary.As per coding guidelines, "Prefer domain-specific value objects over primitives (e.g., `AcpPrefix` not `String`)."♻️ Proposed refactor
impl LeaseTiming { - pub fn ttl(self) -> Duration { - self.ttl.as_duration() + pub fn ttl(self) -> LeaseTtl { + self.ttl } - pub fn renew_interval(self) -> Duration { - self.renew_interval.as_duration() + pub fn renew_interval(self) -> LeaseRenewInterval { + self.renew_interval } } ... - pub fn ttl(&self) -> Duration { - self.timing.ttl() + pub fn ttl(&self) -> LeaseTtl { + self.timing.ttl() } - pub fn renew_interval(&self) -> Duration { - self.timing.renew_interval() + pub fn renew_interval(&self) -> LeaseRenewInterval { + self.timing.renew_interval() } }Also applies to: 146-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-nats/src/lease/mod.rs` around lines 100 - 106, The ttl() and renew_interval() accessors currently unwrap LeaseTtl and LeaseRenewInterval to raw Duration, breaking the value-object boundary; change their signatures to return the domain types (e.g., pub fn ttl(&self) -> LeaseTtl and pub fn renew_interval(&self) -> LeaseRenewInterval) instead of Duration (and take &self rather than consuming self), and update call sites (notably the JetStream boundary code) to call .as_duration() where a raw Duration is actually required; apply the same change to the other similar accessors noted in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@rsworkspace/crates/trogon-nats/src/lease/mod.rs`:
- Around line 450-455: The release path currently treats
kv::DeleteErrorKind::WrongLastRevision as an idempotent success but not
kv::DeleteErrorKind::NotFound; update the match in the release() flow (the block
after calling self.lock.release(revision).await and self.clear_state()) to treat
Err(source) where source.kind() == kv::DeleteErrorKind::NotFound the same as
WrongLastRevision (return Ok(())), so that already-missing or expired keys don't
surface as errors; keep the existing Err(source) => Err(source) fallback for
other kinds.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-nats/src/lease/mod.rs`:
- Around line 100-106: The ttl() and renew_interval() accessors currently unwrap
LeaseTtl and LeaseRenewInterval to raw Duration, breaking the value-object
boundary; change their signatures to return the domain types (e.g., pub fn
ttl(&self) -> LeaseTtl and pub fn renew_interval(&self) -> LeaseRenewInterval)
instead of Duration (and take &self rather than consuming self), and update call
sites (notably the JetStream boundary code) to call .as_duration() where a raw
Duration is actually required; apply the same change to the other similar
accessors noted in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c3e0a92-8518-4695-9a8e-743f422c7630
📒 Files selected for processing (5)
rsworkspace/crates/trogon-nats/Cargo.tomlrsworkspace/crates/trogon-nats/src/lease/mod.rsrsworkspace/crates/trogon-nats/src/lease/renew_interval.rsrsworkspace/crates/trogon-nats/src/lease/ttl.rsrsworkspace/crates/trogon-nats/src/lib.rs
✅ Files skipped from review due to trivial changes (2)
- rsworkspace/crates/trogon-nats/Cargo.toml
- rsworkspace/crates/trogon-nats/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rsworkspace/crates/trogon-nats/src/lease/renew_interval.rs
- rsworkspace/crates/trogon-nats/src/lease/ttl.rs
yordis
added
rust:coverage-baseline-reset
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
yordis
force-pushed
the
yordis/trogon-nats-releasing
branch
13 times, most recently
from
15f4032 to
79173c2
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 79173c2. Configure here.
yordis
force-pushed
the
yordis/trogon-nats-releasing
branch
2 times, most recently
from
8386cd8 to
ed897f1
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
yordis/trogon-nats-releasing
branch
from
ed897f1 to
148458b
Compare
yordis
added
the
rust:coverage-baseline-reset
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
1 participant
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.
Summary
rsworkspace/crates/trogon-natslease/release implementation fromorigin/CRONleasemodule and re-exports inlib.rsasync-natskvfeature required by lease release flowTesting
mise exec -- env CARGO_TARGET_DIR=/tmp/madison-cargo-target cargo test -p trogon-nats(run fromrsworkspace/)