test(journaling): cover legacy recovery contracts - #10959
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A newly introduced helper writes an incorrect legacy record length for stream ids requiring multi-byte varint encoding, which can produce malformed test data if reused beyond the current call site.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
test/Orleans.Journaling.Tests/StateManagerTests.cs — CreateUnsupportedLegacyCommandVersionRecord hard-codes the legacy body length to 2, but the body… |
What changed in this PR
Adds new deterministic tests for Orleans journaling legacy-binary replay and state-manager recovery/work-loop sequencing, supporting the broader coverage push in #10863 by asserting specific ordering, failure, and retry contracts.
Changes:
- Expanded
StateManagerTestswith new contracts around snapshot/replace retry behavior, append/delete fencing, post-delete recovery, and unsupported legacy record retry-after-repair. - Added binary journal format replay tests to validate legacy (V0) framing parsing, mixed V0/V1 physical order, and “do not consume input on failure” behavior for malformed/unsupported records.
| File | Description |
|---|---|
| test/Orleans.Journaling.Tests/StateManagerTests.cs | Adds additional state-manager recovery/work-loop sequencing tests and enhances in-test storage fakes for deterministic verification. |
| test/Orleans.Journaling.Tests/OrleansBinaryJournalBufferWriterTests.cs | Adds new replay tests covering concatenated legacy records, mixed legacy/current records, and malformed/unsupported legacy record handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@copilot review |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
A couple of test/helper updates introduce avoidable overhead and a potential assertion gap that could let a fencing regression slip through undetected.
Review tier: Lite
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
test/Orleans.Journaling.Tests/StateManagerTests.cs — CreateUnsupportedLegacyCommandVersionRecord hard-codes the legacy body length to 2, but the body… View resolved comment |
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
test/Orleans.Journaling.Tests/StateManagerTests.cs:1460
- This test only checks
deleteEntered.IsCompletedimmediately after queuing the delete. IfDeleteStateAsyncwere able to enter storage slightly later (while the append is still blocked), the final assertions could still pass and the fencing regression would be missed. Consider yielding a small scheduling window and re-checking thatDeleteEnteredis still not completed before releasing the append.
test/Orleans.Journaling.Tests/StateManagerTests.cs:2121 CapturingStorage.ReplaceAsyncnow materializesvalue.ToArray()unconditionally (viaattemptedBytes), adding an extra allocation on every successful replace. SinceFailedReplaceAttemptsis only used when throwing, deferToArray()into the failure branch so the success path only materializes once (as it did before).
|
@copilot review |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes are test-focused, align with the stated coverage goals, and no functional/runtime code paths are modified (only a minor optional test-double thread-safety improvement was noted).
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
test/Orleans.Journaling.Tests/StateManagerTests.cs — OperationLog/Appends/Replaces/FailedReplaceAttempts and _segments are all List<T> and… |
|
@copilot review |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
Changes are isolated to tests and test doubles, with deterministic assertions and verified legacy framing assumptions aligning with the repository’s varint encoding implementation.
Review tier: Lite
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
test/Orleans.Journaling.Tests/StateManagerTests.cs — OperationLog/Appends/Replaces/FailedReplaceAttempts and _segments are all List<T> and… View resolved comment |
Part of #10863.
Adds deterministic contracts for legacy binary journal replay and state-manager recovery sequencing. The tests verify physical command order, malformed and unsupported record behavior, unchanged input/state after failures, append/delete fencing, post-delete snapshot recovery, and retry behavior after snapshot and recovery failures.
The canonical
Orleans.Journaling.Testscoverage moved from 3,522/4,311 lines (81.67%) to 3,531/4,311 (81.88%) and from 662/930 branches (71.18%) to 665/930 (71.51%).OrleansBinaryV0JournalReader.ReadEntryHeadernow reaches 31/31 lines and 6/6 branches (100%/100%), reducing its CRAP score from 6.88 to 6.00.Microsoft Reviewers: Open in CodeFlow