refactor(durability): rely on quorum replication, not per-node fsync#16
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR updates WAL durability wording and options documentation, removes synchronous flush handling from ChangesBackground WAL flushing and durability wording
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)README.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) 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 `@README.md`:
- Around line 81-90: The README durability note still implies per-write
flushing, which conflicts with the updated quorum-commit behavior. Update the
later operational note in the documentation to match the same ack semantics
described by the durability section: the write should be described as returning
after replication to a majority and local apply, with flushes happening
asynchronously or on graceful shutdown, not as a requirement for each
acknowledged write. Use the surrounding durability/operational wording in README
to keep the latency and durability story consistent.
In `@src/Hangfire.Raft/Cluster/RaftStorageCluster.cs`:
- Around line 465-470: The shutdown flush in RaftStorageCluster.DisposeAsync is
unbounded because _wal.FlushAsync(CancellationToken.None) can hang forever on a
stalled filesystem. Update the DisposeAsync cleanup path around _wal to use a
bounded/cancellable flush with a short timeout or shutdown token, and keep it
best-effort by logging and swallowing timeout failures rather than blocking
process exit.
🪄 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 Plus
Run ID: ec310b5b-211c-407a-9828-6ead67db2d4c
📒 Files selected for processing (6)
README.mdsamples/Hangfire.Raft.K8sSample/Program.cssamples/Hangfire.Raft.Sample/Program.cssrc/Hangfire.Raft/Cluster/RaftStorageCluster.cssrc/Hangfire.Raft/RaftStorageOptions.cstests/Hangfire.Raft.Tests/RaftJobStorageTests.cs
ca73566 to
c21afab
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/Hangfire.Raft/Cluster/RaftStorageCluster.cs`:
- Around line 461-466: The Raft storage shutdown path currently only disposes
`_wal`, so `FlushInterval = Timeout.InfiniteTimeSpan` has no real way to persist
buffered entries through the storage API. Update `RaftStorageCluster` to expose
and use an explicit flush path for `WriteAheadLog` (or revise
`RaftStorageOptions`/its docs to remove the “persisted only when explicitly
flushed” promise), and make sure the behavior is consistent with the
`FlushAsync` limitation noted in the teardown logic.
🪄 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 Plus
Run ID: 97a01fdc-5e10-48c2-be06-04b40a929d40
📒 Files selected for processing (6)
README.mdsamples/Hangfire.Raft.K8sSample/Program.cssamples/Hangfire.Raft.Sample/Program.cssrc/Hangfire.Raft/Cluster/RaftStorageCluster.cssrc/Hangfire.Raft/RaftStorageOptions.cstests/Hangfire.Raft.Tests/RaftJobStorageTests.cs
✅ Files skipped from review due to trivial changes (3)
- samples/Hangfire.Raft.K8sSample/Program.cs
- samples/Hangfire.Raft.Sample/Program.cs
- README.md
Per the dotNext maintainer (dotnet/dotNext#285), durability is a cluster property: a write is durable once committed to a quorum, and a crashed node recovers its unflushed tail from the leader on restart. The per-write synchronous flush only hardened the handling node's disk, not the quorum, at the cost of throughput, so remove it. - Remove the per-write synchronous flush (DurablyFlushAsync and its gate). SubmitAsync returns once the entry is committed and applied; the WAL's eager background flusher (FlushInterval.Zero) persists it to local disk shortly after, without blocking the writer. - Make FlushInterval a public tuning knob. Zero (default) flushes eagerly per commit in the background; a positive value batches fsyncs for throughput at the cost of a larger crash window, which a multi-node cluster re-replicates from the quorum on restart. - Document single node as development-only (no fault tolerance) across the README, samples, and options; production needs an odd node count. - Drop the single-node crash-durability test (it asserted a guarantee the quorum model no longer makes). Restart durability stays covered by the WAL replay and multi-node tests.
c21afab to
5c2a8cc
Compare
1 participant
Summary
Aligns the storage durability model with the dotNext maintainer's guidance in dotnet/dotNext#285: durability is a cluster property (a write is durable once committed to a quorum), not a per-node fsync.
DurablyFlushAsyncand its gate).SubmitAsyncreturns once the entry is committed and applied. The per-write flush only hardened the handling node's disk, not the quorum, at the cost of throughput and CI fsync contention.FlushIntervalbecomes a public tuning knob (default 100 ms) that batches the background flush; it trades fsync frequency against restart catch-up, not durability.Tests
Repurposed the single-node crash-durability test into a graceful-shutdown flush test. Multi-node durability remains covered by the three-node and quorum-loss tests. Local build/test was not possible in the dev environment (SDK 10.0.300 installed vs 10.0.301 pinned), so this relies on CI.