Fix locking of envScenarioContexts (#9736)

This fixes a race condition in our handling of scenario contexts. See the comments for details. I verified with a bunch of extra logging that this is what is actually failing in our tests. I’ll try to upstream the logging separately since ideally I’d like to have that in CI. I ran all integration tests with --runs_per_test=20 over night and with this change I’m no longer able to get it to flake so dropping the flaky marker. fixes #6910 changelog_begin changelog_end
digital-asset · May 19, 2021 · bb5dd4c · bb5dd4c
1 parent fcbba1c
commit bb5dd4c
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 10 deletions.
diff --git a/compiler/damlc/daml-ide-core/src/Development/IDE/Core/Rules/Daml.hs b/compiler/damlc/daml-ide-core/src/Development/IDE/Core/Rules/Daml.hs
@@ -777,14 +777,22 @@ createScenarioContextRule =
     define $ \CreateScenarioContext file -> do
         ctx <- contextForFile file
         Just scenarioService <- envScenarioService <$> getDamlServiceEnv
-        ctxIdOrErr <- liftIO $ SS.getNewCtx scenarioService ctx
-        ctxId <-
-            liftIO $ either
-                (throwIO . ScenarioBackendException "Failed to create scenario context")
-                pure
-                ctxIdOrErr
         scenarioContextsVar <- envScenarioContexts <$> getDamlServiceEnv
-        liftIO $ modifyMVar_ scenarioContextsVar $ pure . HashMap.insert file ctxId
+        -- We need to keep the lock while creating the context not just while
+        -- updating the variable. That avoids the following race:
+        -- 1. getNewCtx creates a new context A
+        -- 2. Before scenarioContextsVar is updated, gcCtxs kicks in and ends up GCing A.
+        -- 3. Now we update the var and insert A (which has been GCd).
+        -- 4. We return A from the rule and run a scenario on A which
+        --    now fails due to a missing context.
+        ctxId <- liftIO $ modifyMVar scenarioContextsVar $ \prevCtxs -> do
+          ctxIdOrErr <- SS.getNewCtx scenarioService ctx
+          ctxId <-
+              either
+                  (throwIO . ScenarioBackendException "Failed to create scenario context")
+                  pure
+                  ctxIdOrErr
+          pure (HashMap.insert file ctxId prevCtxs, ctxId)
         pure ([], Just ctxId)
 
 -- | This helper should be used instead of GenerateDalf/GenerateRawDalf

diff --git a/compiler/damlc/tests/BUILD.bazel b/compiler/damlc/tests/BUILD.bazel
@@ -207,9 +207,6 @@ da_haskell_library(
             "//compiler/scenario-service/server:scenario_service_jar",
             "@jq_dev_env//:jq",
         ],
-        # NOTE(MH): See https://github.com/digital-asset/daml/issues/6910
-        # for why this is flaky.
-        flaky = True,
         hackage_deps = [
             "base",
         ],