Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix soft delete's "truncate before" checkpoint. #1736

Closed
wants to merge 2 commits into from
Closed

Fix soft delete's "truncate before" checkpoint. #1736

wants to merge 2 commits into from

Conversation

NoelWidmer
Copy link

It seems that the truncate before ($tb) event is incorrectly written into the stream every time a soft delete occurs. The documentation explicitly states the following:

When you delete a stream, its TruncateBefore or $tb is set to the streams current last event number.

Source: https://eventstore.org/docs/server/deleting-streams-and-events/index.html?q=delete%20stream
As you can verify in my PR, this is not the case.

I've already described the issue here:
https://stackoverflow.com/questions/52605285/event-store-cannot-write-to-soft-deleted-streams
and here:
https://groups.google.com/forum/#!topic/event-store/DRtYcK74oRE

To summarize:

  • The current implementation writes long.MaxValue as the $tb checkpoint value instead of the last event number.
  • This prevents appending new events to the stream. The client API calls succeed but the events are not stored.

Please carefully review this PR because it seems strange that no one reported this issue before. I've manually emitted $tb events to my local event store using the client API and was able to get the behavior described in the docs. For this to work I needed to set the checkpoint value to the number of events in the stream. I am not 100% confident that expectedVersion + 1 (my change) is equivalent to the number of events in the stream. Would be great if someone can confirm or suggest a better fix.

@ssttgg
Copy link

ssttgg commented Oct 2, 2018

I'm struggling to replicate this . Are you by any chance appending an event with the same event ID GUID?

Also I imagine the documentation is out of date - to me what ES does to provide soft delete semantics is an internal implementation detail that should not be in the public docs.

@NoelWidmer
Copy link
Author

@ssttgg Are you able to write to a soft deleted stream? If so, what expectedVersion do you provide when sending the request?

I agree, that this should be an implementation detail. But the fact that I was only able to get it to work when writing my own $tb events is a bit concerning. Which by the way was a lot easier than trying to soft delete the stream using the client API.

I don't believe that my application sends events with the same Guid. That would have been a problem before. (I can confirm that tomorrow)

@NoelWidmer
Copy link
Author

@ssttgg Thank you for your input about the duplicate event id. That was indeed what we did. We generated a deterministic guid from the persistence id and the sequence number. That wouldn't be a problem unless you determine the sequence number based on the number of events in the stream. A soft deleted stream did not return any events which caused the new event to have a sequence number of 0 and therefore we generated the same event id as the very first (but deleted) event in the stream.

The solution was to determine the sequence number not on the number of events returned by the client API instead we needed to rely on StreamEventSlice.LastEventNumber.

I'll close this Pull Request since the Event Store is behaving correctly, although the documentation does describe a different behavior.

@NoelWidmer NoelWidmer closed this Oct 3, 2018
@NoelWidmer NoelWidmer deleted the fix-soft-delete-truncate-before-checkpoint branch October 3, 2018 16:03
@ChrisChinchilla
Copy link
Contributor

Noted for docs change 👍

@ChrisChinchilla ChrisChinchilla added the area/documentation Issues relating to project documentation label Oct 4, 2018
@ChrisChinchilla ChrisChinchilla self-assigned this Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Issues relating to project documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants