Fix issues related to session restore and creation.
- Timeout tasks should not be enqueued for
  NO_REQUEST_WITH_TIMEOUT when there is no outstanding
  request.
- REQUEST_WITH_CONTENT and NO_REQUEST_WITH_CONTENT should
  always populate sessions immediately.
- Restored sessions should be populated immediately.
- No-Cards errors should be suppressed when HEAD has
  content.

PiperOrigin-RevId: 243144503
Change-Id: I374061d1658f88ce33d73a02931a752618cedd37
diff --git a/src/main/java/com/google/android/libraries/feed/feedmodelprovider/FeedModelProvider.java b/src/main/java/com/google/android/libraries/feed/feedmodelprovider/FeedModelProvider.java
index d6d7e6d..c863535 100644
--- a/src/main/java/com/google/android/libraries/feed/feedmodelprovider/FeedModelProvider.java
+++ b/src/main/java/com/google/android/libraries/feed/feedmodelprovider/FeedModelProvider.java
@@ -1198,6 +1198,7 @@
         return;
       }
 
+      // TODO: this logic assumes that parents are passed before children.
       newParents.add(childKey);
       FeatureChangeImpl change = getChange(parentKey);
       if (change != null) {
diff --git a/src/main/java/com/google/android/libraries/feed/feedsessionmanager/FeedSessionManager.java b/src/main/java/com/google/android/libraries/feed/feedsessionmanager/FeedSessionManager.java
index 9093b50..5857405 100644
--- a/src/main/java/com/google/android/libraries/feed/feedsessionmanager/FeedSessionManager.java
+++ b/src/main/java/com/google/android/libraries/feed/feedsessionmanager/FeedSessionManager.java
@@ -14,11 +14,6 @@
 
 package com.google.android.libraries.feed.feedsessionmanager;
 
-import static com.google.android.libraries.feed.host.scheduler.SchedulerApi.RequestBehavior.NO_REQUEST_WITH_TIMEOUT;
-import static com.google.android.libraries.feed.host.scheduler.SchedulerApi.RequestBehavior.REQUEST_WITH_CONTENT;
-import static com.google.android.libraries.feed.host.scheduler.SchedulerApi.RequestBehavior.REQUEST_WITH_TIMEOUT;
-import static com.google.android.libraries.feed.host.scheduler.SchedulerApi.RequestBehavior.REQUEST_WITH_WAIT;
-
 import android.support.annotation.VisibleForTesting;
 import com.google.android.libraries.feed.api.common.MutationContext;
 import com.google.android.libraries.feed.api.common.PayloadWithId;
@@ -50,7 +45,6 @@
 import com.google.android.libraries.feed.feedsessionmanager.internal.ContentCache;
 import com.google.android.libraries.feed.feedsessionmanager.internal.HeadAsStructure;
 import com.google.android.libraries.feed.feedsessionmanager.internal.HeadAsStructure.TreeNode;
-import com.google.android.libraries.feed.feedsessionmanager.internal.HeadSessionImpl;
 import com.google.android.libraries.feed.feedsessionmanager.internal.InitializableSession;
 import com.google.android.libraries.feed.feedsessionmanager.internal.Session;
 import com.google.android.libraries.feed.feedsessionmanager.internal.SessionCache;
@@ -127,7 +121,7 @@
 
   // This captures the NO_CARDS_ERROR when a request fails. The request fails in one task and this
   // is sent to the ModelProvider in the populateSessionTask.
-  /*@Nullable*/ @VisibleForTesting ModelError noCardsError;
+  /*@Nullable*/ private ModelError noCardsError;
 
   private final SessionFactory sessionFactory;
   private final SessionManagerMutation sessionManagerMutation;
@@ -273,68 +267,65 @@
   // SchedulerApi to determine how the session is created. It creates a new task to populate the
   // new session.
   private void populateSession(InitializableSession session) {
-    HeadSessionImpl headSession = sessionCache.getHead();
+
+    // Create SessionManagerState and call SchedulerApi to determine what the session-creation
+    // experience should be.
     SessionManagerState sessionManagerState =
         new SessionManagerState(
-            !headSession.isHeadEmpty(),
+            !sessionCache.getHead().isHeadEmpty(),
             sessionCache.getHeadLastAddedTimeMillis(),
             taskQueue.isMakingRequest());
-    @RequestBehavior int behavior = schedulerApi.shouldSessionRequestData(sessionManagerState);
-
-    Runnable timeoutTask = null;
-    if (behavior == REQUEST_WITH_TIMEOUT || behavior == NO_REQUEST_WITH_TIMEOUT) {
-      timeoutTask = () -> populateSessionTask(session, true);
-    }
-    boolean legacyHeadContent =
-        behavior == REQUEST_WITH_CONTENT || behavior == REQUEST_WITH_TIMEOUT;
-    boolean makeRequest =
-        (behavior == REQUEST_WITH_TIMEOUT
-                || behavior == REQUEST_WITH_WAIT
-                || behavior == REQUEST_WITH_CONTENT)
-            && !taskQueue.isMakingRequest();
     Logger.i(
         TAG,
-        "request behavior: %s, making request %s, timeout task %s",
-        behavior,
-        makeRequest,
-        timeoutTask != null);
+        "shouldSessionRequestData; hasContent(%b), contentCreationTime(%d), outstandingRequest(%b)",
+        sessionManagerState.hasContent,
+        sessionManagerState.contentCreationDateTimeMs,
+        sessionManagerState.hasOutstandingRequest);
+    @RequestBehavior int behavior = schedulerApi.shouldSessionRequestData(sessionManagerState);
 
-    // if we are making a request, there are two orders, request -> populate for all cases except
-    // for REQUEST_WITH_CONTENT which is populate -> request
-    if (makeRequest && behavior != REQUEST_WITH_CONTENT) {
-      // determine if we make the request before or after populating a session
-      triggerRefresh(null, RequestReason.OPEN_WITHOUT_CONTENT, UiContext.getDefaultInstance());
+    // Based on sessionManagerState and behavior, determine if FeedSessionManager should start a
+    // request, append an ongoing request to this session, or include a timeout.
+    boolean shouldAppendOutstandingRequest = shouldAppendToSession(sessionManagerState, behavior);
+    boolean shouldStartRequest = shouldStartRequest(sessionManagerState, behavior);
+    Runnable timeoutTask =
+        shouldPopulateWithTimeout(sessionManagerState, behavior)
+            ? () -> populateSessionTask(session, shouldAppendOutstandingRequest)
+            : null;
+    Logger.i(
+        TAG,
+        "shouldSessionRequestDataResult: %s, shouldMakeRequest(%b), withTimeout(%b),"
+            + " withAppend(%b)",
+        requestBehaviorToString(behavior),
+        shouldStartRequest,
+        timeoutTask != null,
+        shouldAppendOutstandingRequest);
+
+    // If we are making a request, there are two orders, request -> populate for all cases except
+    // for REQUEST_WITH_CONTENT which is populate -> request.
+    if (shouldStartRequest && behavior != RequestBehavior.REQUEST_WITH_CONTENT) {
+      triggerRefresh(
+          /* sessionId= */ null,
+          RequestReason.OPEN_WITHOUT_CONTENT,
+          UiContext.getDefaultInstance());
     }
-
-    int taskType = TaskType.USER_FACING;
-    if (behavior == RequestBehavior.NO_REQUEST_WITH_CONTENT
-        && taskQueue.isMakingRequest()
-        && !taskQueue.isDelayed()) {
-      // don't wait for the request to finish, go ahead and show the content
-      taskType = TaskType.IMMEDIATE;
-    } else if (behavior == RequestBehavior.REQUEST_WITH_CONTENT) {
-      // Make sure the populate session runs immediately (before refresh)
-      taskType = TaskType.IMMEDIATE;
-    }
-
-    // Task which creates and populates the newly created session.  This must be done
-    // on the Session Manager thread so it atomic with the mutations.
     taskQueue.execute(
         Task.POPULATE_NEW_SESSION,
-        taskType,
-        () -> populateSessionTask(session, legacyHeadContent),
+        requestBehaviorToTaskType(behavior),
+        () -> populateSessionTask(session, shouldAppendOutstandingRequest),
         timeoutTask,
         sessionPopulationTimeoutMs);
-    if (makeRequest && behavior == REQUEST_WITH_CONTENT) {
-      triggerRefresh(null, RequestReason.OPEN_WITH_CONTENT, UiContext.getDefaultInstance());
+    if (shouldStartRequest && behavior == RequestBehavior.REQUEST_WITH_CONTENT) {
+      triggerRefresh(
+          /* sessionId= */ null, RequestReason.OPEN_WITH_CONTENT, UiContext.getDefaultInstance());
     }
   }
 
-  private void populateSessionTask(InitializableSession session, boolean legacyHeadContent) {
+  private void populateSessionTask(
+      InitializableSession session, boolean shouldAppendOutstandingRequest) {
     threadUtils.checkNotMainThread();
     ElapsedTimeTracker timeTracker = timingUtils.getElapsedTimeTracker(TAG);
 
-    if (noCardsError != null) {
+    if (noCardsError != null && sessionCache.getHead().isHeadEmpty()) {
       ModelProvider modelProvider = session.getModelProvider();
       if (modelProvider == null) {
         Logger.e(TAG, "populateSessionTask - noCardsError, modelProvider not found");
@@ -384,7 +375,7 @@
     cachedBindings = contentCache.size() > 0;
     long creationTimeMillis = clock.currentTimeMillis();
     session.populateModelProvider(
-        streamStructuresResult.getValue(), cachedBindings, legacyHeadContent);
+        streamStructuresResult.getValue(), cachedBindings, shouldAppendOutstandingRequest);
     sessionCache.putAttached(sessionId, creationTimeMillis, session);
     synchronized (lock) {
       sessionsUnderConstruction.remove(session);
@@ -445,7 +436,7 @@
     // on the Session Manager thread so it atomic with the mutations.
     taskQueue.execute(
         Task.GET_EXISTING_SESSION,
-        TaskType.USER_FACING,
+        TaskType.IMMEDIATE,
         () -> {
           threadUtils.checkNotMainThread();
           if (!sessionCache.hasSession(sessionId)) {
@@ -891,6 +882,73 @@
     }
   }
 
+  private static boolean shouldAppendToSession(
+      SessionManagerState sessionManagerState, @RequestBehavior int requestBehavior) {
+    switch (requestBehavior) {
+      case RequestBehavior.REQUEST_WITH_CONTENT: // Fall-through
+      case RequestBehavior.REQUEST_WITH_TIMEOUT:
+        return sessionManagerState.hasContent;
+      case RequestBehavior.NO_REQUEST_WITH_CONTENT: // Fall-through
+      case RequestBehavior.NO_REQUEST_WITH_TIMEOUT:
+        return sessionManagerState.hasContent && sessionManagerState.hasOutstandingRequest;
+      default:
+        return false;
+    }
+  }
+
+  private static boolean shouldStartRequest(
+      SessionManagerState sessionManagerState, @RequestBehavior int requestBehavior) {
+    return (requestBehavior == RequestBehavior.REQUEST_WITH_TIMEOUT
+            || requestBehavior == RequestBehavior.REQUEST_WITH_WAIT
+            || requestBehavior == RequestBehavior.REQUEST_WITH_CONTENT)
+        && !sessionManagerState.hasOutstandingRequest;
+  }
+
+  private static boolean shouldPopulateWithTimeout(
+      SessionManagerState sessionManagerState, @RequestBehavior int requestBehavior) {
+    return requestBehavior == RequestBehavior.REQUEST_WITH_TIMEOUT
+        || (requestBehavior == RequestBehavior.NO_REQUEST_WITH_TIMEOUT
+            && sessionManagerState.hasOutstandingRequest);
+  }
+
+  private static String requestBehaviorToString(@RequestBehavior int requestBehavior) {
+    switch (requestBehavior) {
+      case RequestBehavior.NO_REQUEST_WITH_CONTENT:
+        return "NO_REQUEST_WITH_CONTENT";
+      case RequestBehavior.NO_REQUEST_WITH_TIMEOUT:
+        return "NO_REQUEST_WITH_TIMEOUT";
+      case RequestBehavior.NO_REQUEST_WITH_WAIT:
+        return "NO_REQUEST_WITH_WAIT";
+      case RequestBehavior.REQUEST_WITH_CONTENT:
+        return "REQUEST_WITH_CONTENT";
+      case RequestBehavior.REQUEST_WITH_TIMEOUT:
+        return "REQUEST_WITH_TIMEOUT";
+      case RequestBehavior.REQUEST_WITH_WAIT:
+        return "REQUEST_WITH_WAIT";
+      default:
+        return "UNKNOWN";
+    }
+  }
+
+  private static int requestBehaviorToTaskType(@RequestBehavior int requestBehavior) {
+    switch (requestBehavior) {
+      case RequestBehavior.REQUEST_WITH_WAIT: // Fall-through
+      case RequestBehavior.NO_REQUEST_WITH_WAIT:
+        // Wait for the request to complete and then show content.
+        return TaskType.USER_FACING;
+      case RequestBehavior.REQUEST_WITH_CONTENT: // Fall-through
+      case RequestBehavior.NO_REQUEST_WITH_CONTENT:
+        // Show content immediately and append when the request completes.
+        return TaskType.IMMEDIATE;
+      case RequestBehavior.REQUEST_WITH_TIMEOUT: // Fall-through
+      case RequestBehavior.NO_REQUEST_WITH_TIMEOUT:
+        // Wait for the request to complete but show current content if the timeout elapses.
+        return TaskType.USER_FACING;
+      default:
+        return TaskType.USER_FACING;
+    }
+  }
+
   // Interner that caches the inner PietSharedStateItem from a StreamSharedState, which may
   // sometimes be identical, only the inner content_id differing (see [INTERNAL LINK]).
   @VisibleForTesting
diff --git a/src/main/java/com/google/android/libraries/feed/feedsessionmanager/internal/TimeoutSessionImpl.java b/src/main/java/com/google/android/libraries/feed/feedsessionmanager/internal/TimeoutSessionImpl.java
index 9bf0e0e..8ef5c6c 100644
--- a/src/main/java/com/google/android/libraries/feed/feedsessionmanager/internal/TimeoutSessionImpl.java
+++ b/src/main/java/com/google/android/libraries/feed/feedsessionmanager/internal/TimeoutSessionImpl.java
@@ -51,7 +51,7 @@
       List<StreamStructure> head,
       boolean cachedBindings,
       boolean legacyHeadContent) {
-    Logger.i(TAG, "TimeoutSession.populateModelProvider, legacyHeadContent %s", legacyHeadContent);
+    Logger.i(TAG, "TimeoutSession.populateModelProvider, shouldAppend %s", legacyHeadContent);
     super.populateModelProvider(head, cachedBindings, legacyHeadContent);
   }
 
@@ -63,10 +63,10 @@
     String localSessionId = Validators.checkNotNull(sessionId);
     Logger.i(
         TAG,
-        "TimeoutSession.updateSession, sessionId %s, clearHead %s, legacyHeadContent %s",
-        localSessionId,
+        "updateSession; clearHead(%b), shouldAppend(%b), sessionId(%s)",
         clearHead,
-        legacyHeadContent);
+        legacyHeadContent,
+        localSessionId);
     if (clearHead) {
       if (!legacyHeadContent) {
         if (shouldInvalidateModelProvider(mutationContext, localSessionId)) {
@@ -83,7 +83,7 @@
           }
         } else {
           // Without having legacy HEAD content, don't update the session,
-          Logger.i(TAG, "Session %s not updated due to clearHead", localSessionId);
+          Logger.i(TAG, "Not configured to append: %s", localSessionId);
         }
         return;
       }
diff --git a/src/test/java/com/google/android/libraries/feed/feedsessionmanager/FeedSessionManagerTest.java b/src/test/java/com/google/android/libraries/feed/feedsessionmanager/FeedSessionManagerTest.java
index 4185917..99d5bc9 100644
--- a/src/test/java/com/google/android/libraries/feed/feedsessionmanager/FeedSessionManagerTest.java
+++ b/src/test/java/com/google/android/libraries/feed/feedsessionmanager/FeedSessionManagerTest.java
@@ -307,7 +307,7 @@
   }
 
   @Test
-  public void testNoRequestWithContent_populateIsUserFacing() {
+  public void testNoRequestWithContent_populateIsImmediate() {
     when(schedulerApi.shouldSessionRequestData(any(SessionManagerState.class)))
         .thenReturn(RequestBehavior.NO_REQUEST_WITH_CONTENT);
 
@@ -315,12 +315,12 @@
     populateSession(sessionManager, 3, 1, true, null);
     fakeTaskQueue.resetCounts();
 
-    // Population will happen in a user-facing task and no request is sent.
+    // Population will happen in an immediate task and no request is sent.
     ModelProvider modelProvider = getModelProvider(sessionManager);
     assertThat(modelProvider).isNotNull();
-    assertThat(fakeTaskQueue.getImmediateTaskCount()).isEqualTo(0);
+    assertThat(fakeTaskQueue.getImmediateTaskCount()).isEqualTo(1);
     assertThat(fakeTaskQueue.getBackgroundTaskCount()).isEqualTo(0);
-    assertThat(fakeTaskQueue.getUserFacingTaskCount()).isEqualTo(1);
+    assertThat(fakeTaskQueue.getUserFacingTaskCount()).isEqualTo(0);
     assertThat(fakeTaskQueue.isMakingRequest()).isFalse();
   }
 
@@ -361,16 +361,55 @@
   }
 
   @Test
+  public void testGetExistingSession_populateIsImmediate() {
+    FeedSessionManager sessionManager = getInitializedSessionManager();
+    populateSession(
+        sessionManager,
+        /* featureCnt= */ 2,
+        /* idStart= */ 1,
+        /* reset= */ true,
+        /* sharedStateId= */ null);
+    ModelProvider modelProvider = getModelProvider(sessionManager);
+    String sessionId = modelProvider.getSessionId();
+    modelProvider.detachModelProvider();
+    fakeTaskQueue.resetCounts();
+
+    // Population will happen in an immediate task.
+    modelProvider = getModelProvider(sessionManager, sessionId);
+    assertThat(modelProvider).isNotNull();
+    assertThat(fakeTaskQueue.getImmediateTaskCount()).isEqualTo(1);
+    assertThat(fakeTaskQueue.getBackgroundTaskCount()).isEqualTo(0);
+    assertThat(fakeTaskQueue.getUserFacingTaskCount()).isEqualTo(0);
+  }
+
+  @Test
   public void testNoCardsError() {
     FeedSessionManager sessionManager = getInitializedSessionManager();
-    sessionManager.noCardsError = new ModelError(ErrorType.NO_CARDS_ERROR, null);
-    int featureCnt = 2;
-    populateSession(sessionManager, featureCnt, 1, true, null);
+    sessionManager.getUpdateConsumer(EMPTY_MUTATION).accept(Result.failure());
 
-    // We can't verify that raiseError was called on ModelProvider since all of that happens in
-    // the createNew() call due to the single threaded nature.  We may want to have a test method
-    // on the factory to install the observer when creating the ModelProvider.
     ModelProvider modelProvider = getModelProvider(sessionManager);
+    assertThat(modelProvider.getRootFeature()).isNull();
+
+    // Verify the failed session is correct
+    SessionCache sessionCache = sessionManager.getSessionCacheForTest();
+    assertThat(sessionCache.getAttachedSessions()).hasSize(1);
+    Session session = sessionCache.getAttached(modelProvider.getSessionId());
+    assertThat(session).isNotNull();
+  }
+
+  @Test
+  public void testNoCardsError_populatedHeadSuppressesError() {
+    FeedSessionManager sessionManager = getInitializedSessionManager();
+    populateSession(
+        sessionManager,
+        /* featureCnt= */ 2,
+        /* idStart= */ 1,
+        /* reset= */ true,
+        /* sharedStateId= */ null);
+    sessionManager.getUpdateConsumer(EMPTY_MUTATION).accept(Result.failure());
+
+    ModelProvider modelProvider = getModelProvider(sessionManager);
+    assertThat(modelProvider.getRootFeature()).isNotNull();
 
     // Verify the failed session is correct
     SessionCache sessionCache = sessionManager.getSessionCacheForTest();
@@ -780,14 +819,15 @@
     }
     Consumer<Result<List<StreamDataOperation>>> updateConsumer =
         sessionManager.getUpdateConsumer(EMPTY_MUTATION);
-    List<StreamDataOperation> operations = new ArrayList<>();
-    operations.addAll(internalProtocolBuilder.build());
-    Result<List<StreamDataOperation>> result = Result.success(operations);
-    updateConsumer.accept(result);
+    updateConsumer.accept(Result.success(internalProtocolBuilder.build()));
     return operationCount;
   }
 
   private ModelProvider getModelProvider(FeedSessionManager sessionManager) {
+    return getModelProvider(sessionManager, /* sessionId= */ null);
+  }
+
+  private ModelProvider getModelProvider(FeedSessionManager sessionManager, String sessionId) {
     ModelProviderFactory modelProviderFactory =
         new FeedModelProviderFactory(
             sessionManager,
@@ -796,7 +836,11 @@
             fakeTaskQueue,
             fakeMainThreadRunner,
             configuration);
-    return modelProviderFactory.createNew(null);
+    if (sessionId == null) {
+      return modelProviderFactory.createNew(/* viewDepthProvider= */ null);
+    } else {
+      return modelProviderFactory.create(sessionId);
+    }
   }
 
   private FeedSessionManager getInitializedSessionManager() {