From 5e2fcec60fe0194f1b158ee51fdcdb455b9e8f79 Mon Sep 17 00:00:00 2001
From: Evgenii Stratonikov <e.stratonikov@yadro.com>
Date: Fri, 26 May 2023 17:00:33 +0300
Subject: [PATCH] [#396] treesvc: properly remember last height on shutdown

Previously `newHeight` was updated in parallel, so that applying
operation at height H did not imply successful TreeApply() for H-1.
And because we have no context in TreeUpdateLastSyncHeight(), invalid
starting height could be written if the context was canceled.

In this commit we return the new height only if all operations were
successfully applied.

Signed-off-by: Evgenii Stratonikov <e.stratonikov@yadro.com>
---
 pkg/services/tree/sync.go | 46 ++++++++++++++-------------------------
 1 file changed, 16 insertions(+), 30 deletions(-)

diff --git a/pkg/services/tree/sync.go b/pkg/services/tree/sync.go
index ed87eac45b..81e4a8e44e 100644
--- a/pkg/services/tree/sync.go
+++ b/pkg/services/tree/sync.go
@@ -206,28 +206,26 @@ func (s *Service) synchronizeSingle(ctx context.Context, cid cid.ID, treeID stri
 	errG, ctx := errgroup.WithContext(ctx)
 	errG.SetLimit(1024)
 
-	var heightMtx sync.Mutex
-
 	for {
-		newHeight := height
 		req := &GetOpLogRequest{
 			Body: &GetOpLogRequest_Body{
 				ContainerId: rawCID,
 				TreeId:      treeID,
-				Height:      newHeight,
+				Height:      height,
 			},
 		}
 		if err := SignMessage(req, s.key); err != nil {
 			_ = errG.Wait()
-			return newHeight, err
+			return height, err
 		}
 
 		c, err := treeClient.GetOpLog(ctx, req)
 		if err != nil {
 			_ = errG.Wait()
-			return newHeight, fmt.Errorf("can't initialize client: %w", err)
+			return height, fmt.Errorf("can't initialize client: %w", err)
 		}
 
+		lastApplied := height
 		res, err := c.Recv()
 		for ; err == nil; res, err = c.Recv() {
 			lm := res.GetBody().GetOperation()
@@ -237,39 +235,27 @@ func (s *Service) synchronizeSingle(ctx context.Context, cid cid.ID, treeID stri
 			}
 			if err := m.Meta.FromBytes(lm.Meta); err != nil {
 				_ = errG.Wait()
-				return newHeight, err
+				return height, err
+			}
+			if lastApplied < m.Meta.Time {
+				lastApplied = m.Meta.Time
 			}
 			errG.Go(func() error {
-				err := s.forest.TreeApply(cid, treeID, m, true)
-				heightMtx.Lock()
-				defer heightMtx.Unlock()
-				if err != nil {
-					if newHeight > height {
-						height = newHeight
-					}
-					return err
-				}
-				if m.Time > newHeight {
-					newHeight = m.Time + 1
-				} else {
-					newHeight++
-				}
-				return nil
+				return s.forest.TreeApply(cid, treeID, m, true)
 			})
 		}
 
+		// First check local errors: if everything is ok, we can update starting height,
+		// because everything was applied.
 		applyErr := errG.Wait()
-		if err == nil {
-			err = applyErr
+		if applyErr != nil {
+			return height, applyErr
 		}
 
-		heightMtx.Lock()
-		if height == newHeight || err != nil && !errors.Is(err, io.EOF) {
-			heightMtx.Unlock()
-			return newHeight, err
+		height = lastApplied
+		if err != nil && !errors.Is(err, io.EOF) {
+			return height, err
 		}
-		height = newHeight
-		heightMtx.Unlock()
 	}
 }