From 685a1a1fe2af9fe8c4b36ce2fdd405d5645e0896 Mon Sep 17 00:00:00 2001 From: charithabandi Date: Fri, 1 Dec 2023 15:23:41 -0600 Subject: [PATCH 1/2] Fixes duplicate idempotent key issue during recovery and non-recovery --- internal/sessions/committable/committable.go | 3 +-- internal/sessions/kv.go | 5 ----- internal/sessions/session.go | 9 +++------ 3 files changed, 4 insertions(+), 13 deletions(-) diff --git a/internal/sessions/committable/committable.go b/internal/sessions/committable/committable.go index 95406f2b3..29ea228a0 100644 --- a/internal/sessions/committable/committable.go +++ b/internal/sessions/committable/committable.go @@ -141,6 +141,7 @@ func (a *SavepointCommittable) Commit(ctx context.Context, idempotencyKey []byte a.writable = false if a.skip { + a.skip = false return a.db.Get(ctx, ApphashKey, true) } @@ -148,8 +149,6 @@ func (a *SavepointCommittable) Commit(ctx context.Context, idempotencyKey []byte return nil, fmt.Errorf("no session exists") } - a.skip = false - var appHash []byte if a.idFn != nil { var err error diff --git a/internal/sessions/kv.go b/internal/sessions/kv.go index 7def94139..269d41723 100644 --- a/internal/sessions/kv.go +++ b/internal/sessions/kv.go @@ -15,11 +15,6 @@ func setCurrentKey(kv KV, idempotencyKey []byte) error { return kv.Set(currentKeyKey, idempotencyKey) } -// deleteCurrentKey deletes the current key in the given connection. -func deleteCurrentKey(kv KV) error { - return kv.Delete(currentKeyKey) -} - // getCurrentKey gets the current key in the given connection. // it can return nil if there is no current key. func getCurrentKey(kv KV) ([]byte, error) { diff --git a/internal/sessions/session.go b/internal/sessions/session.go index f84be4c15..9302c2b9a 100644 --- a/internal/sessions/session.go +++ b/internal/sessions/session.go @@ -79,11 +79,9 @@ func (m *MultiCommitter) Begin(ctx context.Context, idempotencyKey []byte) (err return err } - // if lastKey is not nil, we are recovering from a crash - if lastKey != nil { - if !bytes.Equal(lastKey, idempotencyKey) { - return fmt.Errorf("%w on recovery: expected %s, got %s", ErrIdempotencyKeyMismatch, lastKey, idempotencyKey) - } + // if the last key is the same as the current key, we are recovering + // from a crash, therefore begin recovery mode. + if bytes.Equal(lastKey, idempotencyKey) { for _, committable := range m.committables { err = committable.BeginRecovery(ctx, idempotencyKey) if err != nil { @@ -143,7 +141,6 @@ func (m *MultiCommitter) Commit(ctx context.Context, idempotencyKey []byte) (id id = append(id, newId...) } - err = deleteCurrentKey(m.kv) return id, err } From 6de5922bc8711ca70bdc8816c1e638ca717dca7a Mon Sep 17 00:00:00 2001 From: charithabandi Date: Fri, 1 Dec 2023 15:40:31 -0600 Subject: [PATCH 2/2] Fix unittests --- internal/sessions/session_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/sessions/session_test.go b/internal/sessions/session_test.go index c54eee66f..c4ca2ca15 100644 --- a/internal/sessions/session_test.go +++ b/internal/sessions/session_test.go @@ -45,7 +45,8 @@ func Test_Sessions(t *testing.T) { _, err = mc.Commit(ctx, key2) assert.NoError(t, err) - assert.Equal(t, len(kv.vals), 0) + // IdempotentKey is never deleted, just updated with every block, so it should always be 1 + assert.Equal(t, len(kv.vals), 1) }, }, {