Akka.Persistence: SnapshotMetadata always uses DateTime.MinValue, which creates "duplicate key" exceptions when multiple snapshots are taken at same seqNr

[Aaronontheweb]

Version Information
Version of Akka.NET? v1.5.27.1 and all prior versions
Which Akka.NET Modules? Akka.Persistence has the bug, but it's only the SQL-based implementations that have this problem

Describe the bug

A bug report from an end-user contained the following exception when using Akka.Persistence.Sql:

      [WARNING][07/27/2024 07:11:47.184Z][Thread 0042][akka://Journey/user/journey] Failed to SaveSnapshot given metadata [SnapshotMetadata<pid: JourneyAggregateActorRouter, seqNr: 0, timestamp: 0001/01/01>] due to: [Akka.Pattern.OpenCircuitException: Circuit Breaker is open; calls are failing fast: Circuit Breaker is open; calls are failing fast]
fail: Akka.Actor.ActorSystem[0]
      [ERROR][07/27/2024 07:11:47.185Z][Thread 0037][akka://Journey/user/journey/10935440] Persistence failure when replaying events for persistenceId [10935440]. Last known sequence number [0]

The real kicker here is the timestamp value - 0001/01/01. This is DateTime.MinValue, which is used as the default timestamp on all SnapshotMetadata instances used by Akka.Persistence's actor base classes:

akka.net/src/core/Akka.Persistence/Eventsourced.cs

Lines 219 to 222 in acc8bd3

	public void SaveSnapshot(object snapshot)
	{
	SnapshotStore.Tell(new SaveSnapshot(new SnapshotMetadata(SnapshotterId, SnapshotSequenceNr), snapshot));
	}

And that CTOR always uses DateTime.MinValue for the snapshot timestamp:

akka.net/src/core/Akka.Persistence/SnapshotProtocol.cs

Lines 62 to 72 in acc8bd3

	public static DateTime TimestampNotSpecified = DateTime.MinValue;
	/// <summary>
	/// Initializes a new instance of the <see cref="SnapshotMetadata"/> class.
	/// </summary>
	/// <param name="persistenceId">The id of the persistent actor fro which the snapshot was taken.</param>
	/// <param name="sequenceNr">The sequence number at which the snapshot was taken.</param>
	public SnapshotMetadata(string persistenceId, long sequenceNr)
	: this(persistenceId, sequenceNr, TimestampNotSpecified)
	{
	}

Expected behavior

It is perfectly valid and legal, according to the Akka.Persistence specification, to have multiple snapshots taken at the same PersistentId and same seqNr so long as they are separated by Timestamp. The combination of these three values is what is supposed to form the "uniqueness constraint" for a snapshot, hence why Timestamps are included in things like SnapshotSelection queries.

However, there is a secondary bug in the case of SQL-related plugins: the primary keys for Snapshots themselves are also wrong:

USE [Akka]
GO

SET ANSI_PADDING ON
GO

/****** Object:  Index [PK_SnapshotStore]    Script Date: 8/6/2024 10:01:04 AM ******/
ALTER TABLE [dbo].[SnapshotStore] ADD  CONSTRAINT [PK_SnapshotStore] PRIMARY KEY CLUSTERED 
(
	[PersistenceId] ASC,
	[SequenceNr] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, SORT_IN_TEMPDB = OFF, IGNORE_DUP_KEY = OFF, ONLINE = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF) ON [PRIMARY]
GO

I'll file a second issue for that.

Actual behavior

Saving two snapshots at the same seqNr fails.

Environment

Running on SQL Server, but I'm sure it's the same issue for the other SQL platforms we support as well.

Additional context

This bug has been around for ~10 years, so I'm sure it doesn't happen very often.

If you're impacted by this bug, I recommend the following workaround:

• Each time you want to save a snapshot, call Persist on a useless data type you don't care about (i.e. an integer)
• Add a Recover<int>(i => { }); to just discard those values during recovery
• Inside the Persist callback function, THEN call SaveSnapshot - this will force the seqNr to implement even though you're not actually using it.

[Aaronontheweb]

Opened a separate issue for the indexing / transactional problems in Akka.Persistence.Sql: akkadotnet/Akka.Persistence.Sql#432