fix(raft): correct ambiguous-write classification, prevent reclaimed-job loss, add operational logging#17
Merged
Merged
Conversation
…job loss, add operational logging Functional correctness: - SubmitAsync: mark a command appended before the hand-off so a SubmitTimeout firing during the commit-wait is surfaced as ambiguous (accurate AmbiguousWrites metric and an actionable operator error) rather than as a safe-to-retry "no leader" outcome; reset the flag only on provably not-appended retryable failures. - RunMaintenance: clear ExpireAt when a fetched job is re-enqueued (stale-lease reclaim and RequeueFetchedOp). A job whose expiry elapsed while it was fetched was protected during the live-lease pass but was then silently evicted on the next maintenance pass with zero executions. Observability: - The leader logs and meters stale-fetch reclaims via a deterministic MaintenanceSummary (the crashed-worker double-run case that previously only a live worker could observe) and warns on dropped orphaned queue entries. - SubmitAsync fails fast and warns when the local state machine has faulted. - Log stranded ambiguous fetches, dispose-time requeue failures, un-loadable job invocation data, snapshot restore, node startup decisions, and a configurable read-staleness threshold for a lagging follower. Configuration robustness: - RaftStorageOptions.Validate() at startup rejects inverted election timeouts, non-positive SnapshotInterval/lease timeouts, RpcPortOffset port overflow, port 0 and empty bracketed IPv6 hosts, and deduplicates Members. Adds 11 tests; full suite (161) and both samples build clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds configuration validation, startup and restore logging, maintenance summary tracking, and fetch/job-data warning logs across the Raft storage path. ChangesRaft storage observability, validation, and maintenance
Sequence Diagram(s)sequenceDiagram
participant Client
participant RaftStorageCluster
participant Leader
Client->>RaftStorageCluster: SubmitAsync(command)
RaftStorageCluster->>Leader: forward after appended=true
Leader-->>RaftStorageCluster: result or exception
RaftStorageCluster-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Hangfire.Raft/State/RaftStore.cs (1)
335-353: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t gate orphan cleanup on same-pass evictions.
EnqueueOpcan append a job id without a_jobsguard, so a missing-job queue entry can already exist before maintenance runs. With theevictedJobs.Count > 0gate, those entries are neither removed nor reflected inMaintenanceSummaryunless another job happens to expire in the same pass, leaving queue counts/metrics stale.Proposed fix
var orphanedQueueEntries = 0; -if (evictedJobs.Count > 0) +if (_queues.Count > 0) { foreach (var queue in _queues.Values) { var node = queue.First;🤖 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 `@src/Hangfire.Raft/State/RaftStore.cs` around lines 335 - 353, The orphan cleanup in RaftStore’s maintenance path is incorrectly gated behind evictedJobs.Count > 0, which leaves queue entries added by EnqueueOp without a matching _jobs record unprocessed when no jobs expire in the same pass. Remove that dependency so the queue scan and orphanedQueueEntries removal always run during maintenance, and ensure MaintenanceSummary is updated from the actual cleanup count regardless of whether any evictions occurred.
🧹 Nitpick comments (1)
tests/Hangfire.Raft.Tests/RaftStoreTests.cs (1)
760-762: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the rest of the new maintenance summary contract.
This only asserts
EvictedJobsandStaleFetchesReclaimed; the PR also addedOrphanedQueueEntriesRemoved,ExpiredLocksReleased, andExpiredCollections. Add assertions or companion tests so the new logging/metrics fields cannot silently regress.🤖 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 `@tests/Hangfire.Raft.Tests/RaftStoreTests.cs` around lines 760 - 762, The MaintenanceSummary coverage in RaftStoreTests is incomplete because it only checks EvictedJobs and StaleFetchesReclaimed; extend the existing maintenance test around Apply and MaintenanceOp to assert the new MaintenanceSummary fields OrphanedQueueEntriesRemoved, ExpiredLocksReleased, and ExpiredCollections as well. If the current scenario cannot exercise all fields, add companion tests in RaftStoreTests that drive MaintenanceSummary through the relevant maintenance paths so these contract values are explicitly validated and can’t regress silently.
🤖 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.
Outside diff comments:
In `@src/Hangfire.Raft/State/RaftStore.cs`:
- Around line 335-353: The orphan cleanup in RaftStore’s maintenance path is
incorrectly gated behind evictedJobs.Count > 0, which leaves queue entries added
by EnqueueOp without a matching _jobs record unprocessed when no jobs expire in
the same pass. Remove that dependency so the queue scan and orphanedQueueEntries
removal always run during maintenance, and ensure MaintenanceSummary is updated
from the actual cleanup count regardless of whether any evictions occurred.
---
Nitpick comments:
In `@tests/Hangfire.Raft.Tests/RaftStoreTests.cs`:
- Around line 760-762: The MaintenanceSummary coverage in RaftStoreTests is
incomplete because it only checks EvictedJobs and StaleFetchesReclaimed; extend
the existing maintenance test around Apply and MaintenanceOp to assert the new
MaintenanceSummary fields OrphanedQueueEntriesRemoved, ExpiredLocksReleased, and
ExpiredCollections as well. If the current scenario cannot exercise all fields,
add companion tests in RaftStoreTests that drive MaintenanceSummary through the
relevant maintenance paths so these contract values are explicitly validated and
can’t regress silently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0aff8172-d0d5-4bd6-a2d7-56b71cebe3c6
📒 Files selected for processing (9)
src/Hangfire.Raft/Cluster/HangfireStateMachine.cssrc/Hangfire.Raft/Cluster/RaftStorageCluster.cssrc/Hangfire.Raft/RaftFetchedJob.cssrc/Hangfire.Raft/RaftJobStorage.cssrc/Hangfire.Raft/RaftStorageConnection.cssrc/Hangfire.Raft/RaftStorageOptions.cssrc/Hangfire.Raft/State/RaftStore.cstests/Hangfire.Raft.Tests/RaftStorageOptionsTests.cstests/Hangfire.Raft.Tests/RaftStoreTests.cs
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
Functional-correctness + observability hardening for the v1 mission-critical storage, from a full multi-perspective review of the library. Library, tests, and both samples build with 0 warnings (
TreatWarningsAsErrors); the full suite (161 tests, incl. 11 new) is green.Correctness fixes
RaftStorageCluster.SubmitAsync): theappendedflag was set after the hand-off await, so aSubmitTimeoutfiring during the commit-wait threwOperationCanceledExceptionfirst and the write was reported as the safe-to-retry "no leader" outcome instead of ambiguous. The flag is now set before the hand-off and reset only on provably-not-appended retryable failures, so theAmbiguousWritesmetric and the operator-facing error are accurate (the metric previously under-counted the most common timeout case).RaftStore.RunMaintenance): a job whoseExpireAtelapsed while it was held under a fetch lease was protected during the live-lease pass, but once the stale lease was reclaimed and re-enqueued its expiry was not cleared, so the next maintenance pass evicted it with zero executions and no log/metric. The reclaim andRequeueFetchedOpnow clearExpireAt(a re-enqueued job is runnable again). Covered by new cross-pass tests; determinism preserved.Observability (warn/error so problems are visible)
MaintenanceSummary(covers the crashed-worker double-run case a live worker can't observe) and warns on dropped orphaned queue entries.SubmitAsyncfails fast and warns when the local state machine has faulted, instead of blocking each write for the fullSubmitTimeout.ReadStalenessWarningThresholdfor a lagging follower.Configuration robustness
RaftStorageOptions.Validate()(called at startup) rejects, with named errors instead of raw framework exceptions: inverted election timeouts, non-positiveSnapshotInterval/lease timeouts,RpcPortOffsetport overflow, port0, empty bracketed IPv6 host, and deduplicatesMembers.Tests
+11tests (3 store, 8 options); full deterministic + e2e cluster + forwarding suites pass locally.🤖 Generated with Claude Code