Fix AssetContainer behavior with immediate load errors

The AssetContainer was getting "stuck" in the case that it tried to load a missing asset that was already registered with the AssetManager as missing. This fixes the bug, as well as adding a unit test for the specific condition.

Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp

@@ -269,13 +269,13 @@ namespace AZ
             {
                 for (auto& [assetId, dependentAsset] : m_dependencies)
                 {
-                    if (dependentAsset->IsReady())
+                    if (dependentAsset->IsReady() || dependentAsset->IsError())
                     {
                         HandleReadyAsset(dependentAsset);
                     }
                 }
             }
-            if (auto asset = m_rootAsset.GetStrongReference(); asset.IsReady())
+            if (auto asset = m_rootAsset.GetStrongReference(); asset.IsReady() || asset.IsError())
             {
                 HandleReadyAsset(asset);
             }
@@ -496,10 +496,10 @@ namespace AZ
                         m_waitingCount -= 1;
                         disconnectEbus = true;
 
-                        if (m_waitingAssets.empty())
-                        {
-                            allReady = true;
-                        }
+                    }
+                    if (m_waitingAssets.empty())
+                    {
+                        allReady = true;
                     }
                 }
 
@@ -510,8 +510,15 @@ namespace AZ
                 }
             }
 
-            if (allReady && m_initComplete)
+            // If there are no assets left to be loaded, trigger the final AssetContainer notification (ready or canceled).
+            // We guard against prematurely sending it (m_initComplete) because it's possible for assets to get removed from our waiting
+            // list *while* we're still building up the list, so the list would appear to be empty too soon.
+            // We also guard against sending it multiple times (m_finalNotificationSent), because in some error conditions, it may be
+            // possible to try to remove the same asset multiple times, which if it's the last asset, it could trigger multiple
+            // notifications.
+            if (allReady && m_initComplete && !m_finalNotificationSent)
             {
+                m_finalNotificationSent = true;
                 if (m_rootAsset)
                 {
                     AssetManagerBus::Broadcast(&AssetManagerBus::Events::OnAssetContainerReady, this);

Code/Framework/AzCore/AzCore/Asset/AssetContainer.h

@@ -137,6 +137,7 @@ namespace AZ
             AZStd::atomic_int m_invalidDependencies{ 0 };
             AZStd::unordered_set<AZ::Data::AssetId> m_unloadedDependencies;
             AZStd::atomic_bool m_initComplete{ false };
+            AZStd::atomic_bool m_finalNotificationSent{false};
 
             mutable AZStd::recursive_mutex m_preloadMutex;
             // AssetId -> List of assets it is still waiting on 

Code/Framework/AzCore/Tests/Asset/AssetManagerStreamingTests.cpp

@@ -351,6 +351,7 @@ namespace UnitTest
                                                           public AZ::Data::AssetCatalog
     {
         static inline const AZ::Uuid TestAssetId{"{E970B177-5F45-44EB-A2C4-9F29D9A0B2A2}"};
+        static inline const AZ::Uuid MissingAssetId{"{11111111-1111-1111-1111-111111111111}"};
         static inline constexpr AZStd::string_view TestAssetPath = "test";
 
         void SetUp() override
@@ -431,24 +432,40 @@ namespace UnitTest
         // AssetCatalogRequestBus implementation
         
         // Minimalist mocks to provide our desired asset path or asset id
-        AZStd::string GetAssetPathById([[maybe_unused]] const AZ::Data::AssetId& id) override
+        AZStd::string GetAssetPathById(const AZ::Data::AssetId& id) override
         {
-            return TestAssetPath;
+            if (id == TestAssetId)
+            {
+                return TestAssetPath;
+            }
+
+            return "";
         }
+
         AZ::Data::AssetId GetAssetIdByPath(
-            [[maybe_unused]] const char* path, [[maybe_unused]] const AZ::Data::AssetType& typeToRegister,
+            const char* path, [[maybe_unused]] const AZ::Data::AssetType& typeToRegister,
             [[maybe_unused]] bool autoRegisterIfNotFound) override
         {
-            return TestAssetId;
+            if (path == TestAssetPath)
+            {
+                return TestAssetId;
+            }
+
+            return AZ::Data::AssetId();
         }
 
         // Return the mocked-out information for our test asset
-        AZ::Data::AssetInfo GetAssetInfoById([[maybe_unused]] const AZ::Data::AssetId& id) override
+        AZ::Data::AssetInfo GetAssetInfoById(const AZ::Data::AssetId& id) override
         {
             AZ::Data::AssetInfo assetInfo;
-            assetInfo.m_assetId = TestAssetId;
-            assetInfo.m_assetType = AZ::AzTypeInfo<EmptyAsset>::Uuid();
-            assetInfo.m_relativePath = TestAssetPath;
+
+            if (id == TestAssetId)
+            {
+                assetInfo.m_assetId = TestAssetId;
+                assetInfo.m_assetType = AZ::AzTypeInfo<EmptyAsset>::Uuid();
+                assetInfo.m_relativePath = TestAssetPath;
+            }
+
             return assetInfo;
         }
 
@@ -456,15 +473,20 @@ namespace UnitTest
         
         // Set the mocked-out asset load to have a 0-byte length so that the load skips I/O and immediately returns success
         AZ::Data::AssetStreamInfo GetStreamInfoForLoad(
-            [[maybe_unused]] const AZ::Data::AssetId& id, const AZ::Data::AssetType& type) override
+            const AZ::Data::AssetId& id, const AZ::Data::AssetType& type) override
         {
             EXPECT_TRUE(type == AZ::AzTypeInfo<EmptyAsset>::Uuid());
             AZ::Data::AssetStreamInfo info;
+
             info.m_dataOffset = 0;
-            info.m_streamName = TestAssetPath;
             info.m_dataLen = 0;
             info.m_streamFlags = AZ::IO::OpenMode::ModeRead;
 
+            if (id == TestAssetId)
+            {
+                info.m_streamName = TestAssetPath;
+            }
+
             return info;
         }
 
@@ -489,4 +511,27 @@ namespace UnitTest
         EXPECT_TRUE(testAsset.IsReady());
     }
 
+    // This test verifies that even if the asset loading returns immediately with an error, all of the loading code works
+    // successfully.  The test itself loads a missing asset twice - the first time is a non-immediate error, where the error
+    // isn't reported until the DispatchEvents() call.  The second time is an immediate error, because now the asset is already
+    // registered in an Error state.  If the test fails, it will likely get caught in the shutdown of the test class, if any
+    // assets still exist at the point that the asset handler is unregistered.  If they're present, then handling of the immediate
+    // error didn't work, as it left around extra references to the asset that haven't been cleaned up.
+    TEST_F(AssetManagerStreamerImmediateCompletionTests, ImmediateAssetError_WorksSuccessfully)
+    {
+        AZ::Data::AssetLoadParameters loadParams;
+
+        // Attempt to load a missing asset the first time.  It will get an error, but not until the DispatchEvents() call happens.
+        auto testAsset1 = AssetManager::Instance().GetAsset<EmptyAsset>(MissingAssetId, AZ::Data::AssetLoadBehavior::Default, loadParams);
+        AZ::Data::AssetManager::Instance().DispatchEvents();
+        EXPECT_TRUE(testAsset1.IsError());
+
+        // While the reference to the missing asset still exists, try to get it again.  This will cause a more immediate error in
+        // the AssetContainer code, which should still get handled correctly.  In the failure condition, it will instead leave the
+        // AssetContainer in a state where it never sends the final OnAssetContainerReady/Canceled message.
+        auto testAsset2 = AssetManager::Instance().GetAsset<EmptyAsset>(MissingAssetId, AZ::Data::AssetLoadBehavior::Default, loadParams);
+        AZ::Data::AssetManager::Instance().DispatchEvents();
+        EXPECT_TRUE(testAsset2.IsError());
+    }
+
 } // namespace UnitTest