[MirrorSync] Purge events with no updates in last 10s from trie

DriveFs is not guaranteed to send completed events for all
stable_ids that get dispatched, hence we need to regularly
purge the sync status tracker for stale sync statuses.

Bug: b:272157533
Test: Manual
Test: chromeos_unittests SyncStatusTrackerTest.*
Test: chromeos_unittests *DriveFsHostTest*
Change-Id: I1f489f208fbd38c681dd4839a1e0f2a950f332c3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4328372
Reviewed-by: Austin Tankiang <austinct@chromium.org>
Commit-Queue: Marcello Salomao <msalomao@google.com>
Cr-Commit-Position: refs/heads/main@{#1119720}

chromeos/ash/components/drivefs/drivefs_host.cc

@@ -80,7 +80,7 @@ class DriveFsHost::MountState : public DriveFsSession,
           FROM_HERE, kIndividualSyncStatusIntervalMs,
           base::BindRepeating(
               &DriveFsHost::MountState::DispatchBatchIndividualSyncEvents,
-              weak_ptr_factory_.GetWeakPtr()));
+              base::Unretained(this)));
     }
   }
 
@@ -170,6 +170,12 @@ class DriveFsHost::MountState : public DriveFsSession,
     for (auto& observer : host_->observers_) {
       observer.OnIndividualSyncingStatusesDelta(filtered_states);
     }
+
+    // If we still have files in the tracker, keep running, as we might have
+    // stale nodes that will eventually get removed.
+    if (sync_status_tracker_->GetFileCount()) {
+      sync_throttle_timer_->Reset();
+    }
   }
 
   void OnSyncingStatusUpdate(mojom::SyncingStatusPtr status) override {
@@ -383,7 +389,6 @@ class DriveFsHost::MountState : public DriveFsSession,
   // Used to dispatch individual sync status updates in a debounced manner, only
   // sending the sync states that have changed since the last dispatched event.
   std::unique_ptr<base::RetainingOneShotTimer> sync_throttle_timer_;
-  base::WeakPtrFactory<DriveFsHost::MountState> weak_ptr_factory_{this};
 };
 
 DriveFsHost::DriveFsHost(

chromeos/ash/components/drivefs/sync_status_tracker.cc

@@ -16,6 +16,7 @@
 #include "base/files/file_path.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
+#include "base/time/time.h"
 
 namespace drivefs {
 
@@ -61,13 +62,20 @@ SyncState SyncStatusTracker::GetSyncState(const base::FilePath path) const {
               : SyncState::CreateNotFound(std::move(path));
 }
 
-const std::vector<const SyncState> SyncStatusTracker::GetChangesAndClean() {
+std::vector<SyncState> SyncStatusTracker::GetChangesAndClean() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  std::vector<const SyncState> updated_sync_states;
+  std::vector<SyncState> updated_sync_states;
 
   int64_t total_mem_usage_in_bytes = 0;
 
+  for (const auto &[id, node] : id_to_leaf_) {
+    // If the node is stale, set it as "completed" so that it's later removed.
+    if (base::Time::Now() - node->last_update > kMaxStaleTime) {
+      SetNodeState(node, kNotFound, 0, 0);
+    }
+  }
+
   // Traverse trie.
   std::vector<Node*> stack = {root_.get()};
   while (!stack.empty()) {
@@ -94,7 +102,7 @@ const std::vector<const SyncState> SyncStatusTracker::GetChangesAndClean() {
   }
 
   // Reset root node if it's childless.
-  if (root_->children.empty()) {
+  if (root_->IsLeaf()) {
     root_->state.Set(kNotFound, 0, 0);
   }
 
@@ -106,6 +114,8 @@ const std::vector<const SyncState> SyncStatusTracker::GetChangesAndClean() {
 
 SyncStatusTracker::Node* SyncStatusTracker::FindNode(
     const base::FilePath& path) const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   const auto components = path.GetComponents();
   DCHECK(!components.empty() && components.front() == "/");
   const base::span<const base::FilePath::StringType> path_parts(
@@ -155,32 +165,34 @@ void SyncStatusTracker::SetSyncState(const int64_t id,
   // moved/renamed. Mark it as "moved" and remove its status/progress changes
   // from its current ancestors so they are not duplicated with its new
   // ancestors in the trie.
-  if (auto it = id_to_node_.find(id);
-      it != id_to_node_.end() && it->second != node) {
+  if (auto it = id_to_leaf_.find(id);
+      it != id_to_leaf_.end() && it->second != node) {
     SetNodeState(it->second, kMoved, 0, 0);
   }
-  id_to_node_[id] = node;
+  id_to_leaf_[id] = node;
 }
 
 bool SyncStatusTracker::ShouldRemoveNode(const Node* node) const {
   DCHECK(node);
   const auto status = node->state.GetStatus();
-  return node->children.empty() &&
+  return node->IsLeaf() &&
          (status == SyncStatus::kCompleted || status == SyncStatus::kMoved);
 }
 
 void SyncStatusTracker::RemoveNode(const Node* node) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   DCHECK(node);
   Node* parent = node->parent;
   if (!parent) {
     return;
   }
   if (node->id) {
-    id_to_node_.erase(node->id);
+    id_to_leaf_.erase(node->id);
   }
   parent->children.erase(node->path_part);
   Node* grandparent = parent->parent;
-  while (grandparent && parent->children.empty()) {
+  while (grandparent && parent->IsLeaf()) {
     grandparent->children.erase(parent->path_part);
     parent = grandparent;
     grandparent = grandparent->parent;
@@ -201,6 +213,7 @@ void SyncStatusTracker::SetNodeState(Node* const node,
                                      const int64_t transferred = 0,
                                      const int64_t total = 0) {
   const NodeState& delta = node->state.Set(status, transferred, total);
+  node->last_update = base::Time::Now();
 
   // Nothing to do if there were no changes.
   if (delta.IsPristine()) {

chromeos/ash/components/drivefs/sync_status_tracker.h

@@ -17,6 +17,8 @@
 #include "base/containers/flat_map.h"
 #include "base/files/file_path.h"
 #include "base/sequence_checker.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
 
 namespace drivefs {
 
@@ -77,7 +79,7 @@ class COMPONENT_EXPORT(CHROMEOS_ASH_COMPONENTS_DRIVEFS) SyncStatusTracker {
 
     // Completed events might fire more than once for the same id.
     // Only use completed events to update currently tracked nodes.
-    if (id_to_node_.count(id)) {
+    if (id_to_leaf_.count(id)) {
       SetSyncState(id, path, SyncStatus::kCompleted);
     }
   }
@@ -110,9 +112,12 @@ class COMPONENT_EXPORT(CHROMEOS_ASH_COMPONENTS_DRIVEFS) SyncStatusTracker {
   // function. Upon calling it, the trie is reset to a pristine state, meaning
   // it will report 0 accumulated changes until more changes are registered
   // through SetCompleted, SetQueued, SetInProgress, and SetError.
-  const std::vector<const SyncState> GetChangesAndClean();
+  std::vector<SyncState> GetChangesAndClean();
 
-  size_t GetFileCount() const { return id_to_node_.size(); }
+  size_t GetFileCount() const {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    return id_to_leaf_.size();
+  }
 
  private:
   struct NodeState;
@@ -147,8 +152,16 @@ class COMPONENT_EXPORT(CHROMEOS_ASH_COMPONENTS_DRIVEFS) SyncStatusTracker {
 
   SEQUENCE_CHECKER(sequence_checker_);
 
-  std::unique_ptr<Node> root_ = std::make_unique<Node>();
-  base::flat_map<int64_t, Node*> id_to_node_;
+  // DriveFs is not guaranteed to send completed events for all stable_ids that
+  // it dispatches, hence we need to regularly purge stale sync statuses from
+  // the Sync Status Tracker. See b/272157533 for more details. Nodes that
+  // haven't been updated in the last 10 seconds are considered stale.
+  static constexpr auto kMaxStaleTime = base::Seconds(10);
+
+  std::unique_ptr<Node> root_ GUARDED_BY_CONTEXT(sequence_checker_) =
+      std::make_unique<Node>();
+  base::flat_map<int64_t, Node*> id_to_leaf_
+      GUARDED_BY_CONTEXT(sequence_checker_);
 };
 
 struct SyncStatusTracker::NodeState {
@@ -190,6 +203,8 @@ struct SyncStatusTracker::Node {
   Node();
   ~Node();
 
+  bool IsLeaf() const { return children.empty(); }
+
   typedef base::flat_map<base::FilePath::StringType, std::unique_ptr<Node>>
       PathToChildMap;
 
@@ -198,6 +213,7 @@ struct SyncStatusTracker::Node {
   base::FilePath::StringType path_part;
   Node* parent = nullptr;
   NodeState state;
+  base::Time last_update;
 };
 
 }  // namespace drivefs

chromeos/ash/components/drivefs/sync_status_tracker_unittest.cc

@@ -8,6 +8,9 @@
 #include <memory>
 
 #include "base/files/file_path.h"
+#include "base/run_loop.h"
+#include "base/test/task_environment.h"
+#include "base/time/time.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -51,10 +54,9 @@ inline SyncState Error(const base::FilePath path = base::FilePath(),
 }
 
 class SyncStatusTrackerTest : public testing::Test {
- public:
-  SyncStatusTrackerTest() = default;
-  SyncStatusTrackerTest(const SyncStatusTrackerTest&) = delete;
-  SyncStatusTrackerTest& operator=(const SyncStatusTrackerTest&) = delete;
+ protected:
+  base::test::TaskEnvironment task_environment{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
 
   SyncStatusTracker t;
 };
@@ -249,5 +251,27 @@ TEST_F(SyncStatusTrackerTest, OnlyDirtyNodesAreReturned) {
                                             InProgress(b, 20. / 100.)));    //
 }
 
+TEST_F(SyncStatusTrackerTest, StaleNodesGetDeleted) {
+  t.SetQueued(0, abcd, 100);
+
+  ASSERT_EQ(t.GetSyncState(abcd), Queued(abcd));
+
+  t.GetChangesAndClean();
+  ASSERT_EQ(t.GetSyncState(abcd), Queued(abcd));
+
+  // Paths not updated in the last 10s should be purged, so let's test at both
+  // edges (9s and 11s).
+  task_environment.FastForwardBy(base::Seconds(9));
+  auto changes = t.GetChangesAndClean();
+  ASSERT_EQ(t.GetSyncState(abcd), Queued(abcd));
+  ASSERT_TRUE(changes.empty());
+
+  task_environment.FastForwardBy(base::Seconds(2));
+  changes = t.GetChangesAndClean();
+  ASSERT_EQ(t.GetSyncState(abcd), NotFound(abcd));
+  ASSERT_EQ(changes.size(), 5u);
+  ASSERT_EQ(changes.back(), Moved(abcd));
+}
+
 }  // namespace
 }  // namespace drivefs