Skip to content

fix(storage): guard prepare timestamp serialization#420

Merged
yordis merged 6 commits into
masterfrom
yordis/fix-prepare-timestamp-utc-guard
Jul 4, 2026
Merged

fix(storage): guard prepare timestamp serialization#420
yordis merged 6 commits into
masterfrom
yordis/fix-prepare-timestamp-utc-guard

Conversation

@yordis

@yordis yordis commented Jul 4, 2026

Copy link
Copy Markdown
Member
  • Prevents prepare records from accepting timestamps that cannot round-trip with UTC semantics.
  • Keeps in-memory and persisted prepare-record construction aligned around one timestamp contract.
  • Makes the storage invariant testable instead of relying on debug-only assertion behavior.

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@cursor

cursor Bot commented Jul 4, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes validation on the prepare-record write path in persistent storage; callers passing local or unspecified DateTime will now fail at construction instead of silently mis-serializing.

Overview
Prepare log records now require UTC timestamps at construction time: PrepareLogRecord throws if timeStamp.Kind is not DateTimeKind.Utc, matching on-disk serialization that always reads ticks as UTC.

In-memory writes use DateTime.UtcNow instead of DateTime.Now when building prepare records in SingleEventInMemoryStream.

Tests and fixtures are updated to pass explicit UTC DateTime values in transaction-log and chaser tests; scavenge Rec rejects non-UTC optional timestamps; scavenge metadata helpers use a dedicated UTC record timestamp; and a new xUnit test asserts non-UTC input is rejected.

Reviewed by Cursor Bugbot for commit ce6899e. Bugbot is set up for automated code reviews on this repo. Configure here.

@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@yordis, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 42 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8c3d92ea-d66a-484e-8e49-1eaa16def500

📥 Commits

Reviewing files that changed from the base of the PR and between 93b637f and ce6899e.

📒 Files selected for processing (7)
  • src/EventStore.Core.Tests/Services/Storage/Chaser/when_chaser_reads_commit_event.cs
  • src/EventStore.Core.Tests/TransactionLog/when_appending_past_end_of_a_tfchunk.cs
  • src/EventStore.Core.Tests/TransactionLog/when_appending_to_a_tfchunk_and_flushing.cs
  • src/EventStore.Core.Tests/TransactionLog/when_appending_to_a_tfchunk_without_flush.cs
  • src/EventStore.Core.Tests/TransactionLog/when_reading_from_a_cached_tfchunk.cs
  • src/EventStore.Core.Tests/TransactionLog/when_uncaching_a_tfchunk.cs
  • src/EventStore.Core.Tests/TransactionLog/when_writing_multiple_records_to_a_tfchunk.cs

Walkthrough

PrepareLogRecord now rejects non-UTC timestamps at construction, and the in-memory storage plus transaction-log and scavenge tests were updated to use UTC timestamps consistently.

Changes

Timestamp validation and UTC propagation

Layer / File(s) Summary
Constructor validation
src/EventStore.Core/TransactionLog/LogRecords/PrepareLogRecord.cs, src/EventStore.Core.XUnit.Tests/TransactionLog/LogRecords/PrepareLogRecordViewTests.cs
PrepareLogRecord now throws for non-UTC timestamps, and a test asserts the constructor rejects DateTime.Now.
UTC callers and fixtures
src/EventStore.Core/Services/Storage/InMemory/SingleEventInMemoryStream.cs, src/EventStore.Core.XUnit.Tests/Scavenge/Infrastructure/StreamMetadatas.cs, src/EventStore.Core.Tests/TransactionLog/Scavenging/Helpers/TFChunkDbCreationHelper.cs, src/EventStore.Core.Tests/Services/Storage/Chaser/*, src/EventStore.Core.Tests/TransactionLog/*
The in-memory stream uses DateTime.UtcNow, and the surrounding tests and fixtures now construct or expect UTC timestamps consistently.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Poem

A bunny hops by moonlit code,
with UTC stamped on every node.
No local time can blur the way,
the records hop in bright array. 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: enforcing UTC-safe prepare timestamp handling in storage.
Description check ✅ Passed The description is directly related to the UTC timestamp validation and aligned fixture updates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch yordis/fix-prepare-timestamp-utc-guard

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

yordis added 2 commits July 4, 2026 00:10
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/EventStore.Core.XUnit.Tests/Scavenge/Infrastructure/StreamMetadatas.cs (1)

29-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicate "now" constants risk drift.

EffectiveNow and EffectiveNowRecordTimestamp encode the same instant but with different DateTimeKind. Consider deriving one from the other (e.g., EffectiveNow = DateTime.SpecifyKind(EffectiveNowRecordTimestamp, DateTimeKind.Unspecified)) so a future edit to the literal date doesn't desynchronize the two.

♻️ Proposed consolidation
-	public static DateTime EffectiveNow { get; } = new DateTime(2022, 1, 5, 00, 00, 00);
-	private static DateTime EffectiveNowRecordTimestamp { get; } = new DateTime(2022, 1, 5, 00, 00, 00, DateTimeKind.Utc);
+	private static DateTime EffectiveNowRecordTimestamp { get; } = new DateTime(2022, 1, 5, 00, 00, 00, DateTimeKind.Utc);
+	public static DateTime EffectiveNow { get; } = DateTime.SpecifyKind(EffectiveNowRecordTimestamp, DateTimeKind.Unspecified);
🤖 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/EventStore.Core.XUnit.Tests/Scavenge/Infrastructure/StreamMetadatas.cs`
around lines 29 - 39, Duplicate "now" constants should be consolidated to avoid
drift. In StreamMetadatas, keep a single source of truth for the shared instant
by deriving EffectiveNow from EffectiveNowRecordTimestamp (or vice versa) using
the desired DateTimeKind, and update any dependent helpers like ScavengePointRec
to use that shared value consistently. This ensures future edits to the literal
date can’t desynchronize the two properties.
🤖 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.

Nitpick comments:
In `@src/EventStore.Core.XUnit.Tests/Scavenge/Infrastructure/StreamMetadatas.cs`:
- Around line 29-39: Duplicate "now" constants should be consolidated to avoid
drift. In StreamMetadatas, keep a single source of truth for the shared instant
by deriving EffectiveNow from EffectiveNowRecordTimestamp (or vice versa) using
the desired DateTimeKind, and update any dependent helpers like ScavengePointRec
to use that shared value consistently. This ensures future edits to the literal
date can’t desynchronize the two properties.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 84b9b015-97d6-467c-987c-e33e26d48bfb

📥 Commits

Reviewing files that changed from the base of the PR and between 2d1ebf8 and 0a295dc.

📒 Files selected for processing (5)
  • src/EventStore.Core.Tests/TransactionLog/Scavenging/Helpers/TFChunkDbCreationHelper.cs
  • src/EventStore.Core.XUnit.Tests/Scavenge/Infrastructure/StreamMetadatas.cs
  • src/EventStore.Core.XUnit.Tests/TransactionLog/LogRecords/PrepareLogRecordViewTests.cs
  • src/EventStore.Core/Services/Storage/InMemory/SingleEventInMemoryStream.cs
  • src/EventStore.Core/TransactionLog/LogRecords/PrepareLogRecord.cs

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ 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 93b637f. Configure here.

Comment thread src/EventStore.Core.Tests/TransactionLog/when_writing_commit_record_to_file.cs Outdated
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@yordis yordis merged commit c948398 into master Jul 4, 2026
21 checks passed
@yordis yordis deleted the yordis/fix-prepare-timestamp-utc-guard branch July 4, 2026 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant