Fix matrix job cascading for new compiler releases

.gitignore

@@ -19,6 +19,7 @@ result*
 
 TODO.md
 .spec-results
+/generated-docs
 
 # Keep it secret, keep it safe.
 .env

app-e2e/src/Test/E2E/Endpoint/Startup.purs

@@ -14,6 +14,7 @@ import Registry.Test.Assert as Assert
 import Registry.Test.Utils as Utils
 import Test.E2E.Support.Client as Client
 import Test.E2E.Support.Env (E2ESpec)
+import Test.E2E.Support.Env as Env
 import Test.Spec as Spec
 
 spec :: E2ESpec
@@ -47,3 +48,26 @@ spec = do
           <> show (Array.length matrixJobs)
           <> " matrix jobs for packages: "
           <> String.joinWith ", " (map PackageName.print matrixPackages)
+
+    Spec.it "cascades matrix jobs to dependant packages after dependency-free jobs complete" do
+      -- Wait for ALL pending jobs: the dependency-free matrix jobs and any
+      -- cascade jobs they trigger.
+      Env.waitForAllPendingJobs
+
+      -- Check that effect got a matrix job with the new compiler (0.15.11).
+      -- effect depends on prelude, so after prelude's 0.15.11 matrix job
+      -- succeeds, the cascade should enqueue a job for effect.
+      allJobs <- Client.getJobsWith Client.IncludeCompleted
+      let
+        effectName = Utils.unsafePackageName "effect"
+        newCompiler = Utils.unsafeVersion "0.15.11"
+        effectCascadeJobs = Array.filter
+          ( case _ of
+              MatrixJob { packageName, compilerVersion } ->
+                packageName == effectName && compilerVersion == newCompiler
+              _ -> false
+          )
+          allJobs
+
+      when (Array.null effectCascadeJobs) do
+        Assert.fail "Expected cascade matrix job for effect with compiler 0.15.11, but found none."

app-e2e/src/Test/E2E/Support/Fixtures.purs

@@ -43,9 +43,9 @@ import Registry.Version (Version)
 
 type PackageFixture = { name :: PackageName, version :: Version }
 
--- | effect@4.0.0 fixture package
+-- | effect@4.0.1 fixture package (4.0.0 is pre-populated in startup fixtures for cascade testing)
 effect :: PackageFixture
-effect = { name: Utils.unsafePackageName "effect", version: Utils.unsafeVersion "4.0.0" }
+effect = { name: Utils.unsafePackageName "effect", version: Utils.unsafeVersion "4.0.1" }
 
 -- | console@6.1.0 fixture package
 console :: PackageFixture
@@ -59,8 +59,8 @@ prelude = { name: Utils.unsafePackageName "prelude", version: Utils.unsafeVersio
 slug :: PackageFixture
 slug = { name: Utils.unsafePackageName "slug", version: Utils.unsafeVersion "3.0.0" }
 
--- | Standard publish data for effect@4.0.0, used by E2E tests.
--- | This matches the fixtures in app/fixtures/github-packages/effect-4.0.0
+-- | Standard publish data for effect@4.0.1, used by E2E tests.
+-- | This matches the fixtures in app/fixtures/github-packages/effect-4.0.1
 effectPublishData :: Operation.PublishData
 effectPublishData =
   { name: effect.name
@@ -69,7 +69,7 @@ effectPublishData =
       , repo: "purescript-effect"
       , subdir: Nothing
       }
-  , ref: "v4.0.0"
+  , ref: "v4.0.1"
   , compiler: Just $ Utils.unsafeVersion "0.15.10"
   , resolutions: Nothing
   , version: effect.version

app/fixtures/github-packages/console-6.1.0/bower.json

@@ -16,7 +16,7 @@
     "package.json"
   ],
   "dependencies": {
-    "purescript-effect": "^4.0.0",
+    "purescript-effect": "^4.0.1",
     "purescript-prelude": "^6.0.0"
   }
 }

app/fixtures/registry-index/ef/fe/effect

@@ -0,0 +1 @@
+{"name":"effect","version":"4.0.0","license":"BSD-3-Clause","location":{"githubOwner":"purescript","githubRepo":"purescript-effect"},"ref":"v4.0.0","description":"Native side effects","dependencies":{"prelude":">=6.0.0 <7.0.0"}}

app/fixtures/registry/metadata/effect.json

@@ -0,0 +1,18 @@
+{
+  "location": {
+    "githubOwner": "purescript",
+    "githubRepo": "purescript-effect"
+  },
+  "published": {
+    "4.0.0": {
+      "bytes": 5904,
+      "compilers": [
+        "0.15.10"
+      ],
+      "hash": "sha256-6a6UH+Q1C86LCmHWiIq/Xh/2+vHRS69ZeEsQAXrfFQs=",
+      "publishedTime": "2022-08-18T20:04:00.000Z",
+      "ref": "v4.0.0"
+    }
+  },
+  "unpublished": {}
+}

app/src/App/Server/MatrixBuilder.purs

@@ -211,15 +211,22 @@ solveDependantsForCompiler { compilerIndex, name, version, compiler } = do
   manifestIndex <- Registry.readAllManifests
   let dependentManifests = ManifestIndex.dependants manifestIndex name version
   newJobs <- for dependentManifests \(Manifest manifest) -> do
-    -- we first verify if we have already attempted this package with this compiler,
-    -- either in the form of having it in the metadata already, or as a failed compilation
-    -- (i.e. if we find compilers in the metadata for this version we only check this one
-    -- if it's newer, because all the previous ones have been tried)
+    -- We skip if this compiler is already in the package's metadata compilers
+    -- list (meaning it was already successfully tested). Failed compilations
+    -- are not recorded in metadata, but the DB deduplication in insertMatrixJob
+    -- prevents re-enqueuing jobs that already exist.
     shouldAttemptToCompile <- Registry.readMetadata manifest.name >>= case _ of
-      Nothing -> pure false
-      Just metadata -> pure $ case Map.lookup version (un Metadata metadata).published of
-        Nothing -> false
-        Just { compilers } -> any (_ > compiler) compilers
+      Nothing -> do
+        Log.debug $ "Skipping " <> PackageName.print manifest.name <> "@" <> Version.print manifest.version <> ": no metadata found"
+        pure false
+      Just metadata -> do
+        let
+          result = case Map.lookup manifest.version (un Metadata metadata).published of
+            Nothing -> false
+            Just { compilers } -> all (_ /= compiler) compilers
+        unless result do
+          Log.debug $ "Skipping " <> PackageName.print manifest.name <> "@" <> Version.print manifest.version <> ": compiler " <> Version.print compiler <> " already tested or version not published"
+        pure result
     case shouldAttemptToCompile of
       false -> pure Nothing
       true -> do

app/test/App/API.purs

@@ -174,10 +174,12 @@ spec = do
         copyFixture "registry"
         copyFixture "registry-storage"
         copyFixture "github-packages"
-        -- We remove effect-4.0.0.tar.gz since the unit test publishes it from
-        -- scratch and will fail if it's already in storage. We have it in
-        -- storage for the separate integration tests.
+        -- We remove effect fixtures since the unit test publishes effect from
+        -- scratch and will fail if it's already registered. We have these in
+        -- fixtures for the separate integration tests.
         FS.Extra.remove $ Path.concat [ testFixtures, "registry-storage", "effect-4.0.0.tar.gz" ]
+        FS.Extra.remove $ Path.concat [ testFixtures, "registry", "metadata", "effect.json" ]
+        FS.Extra.remove $ Path.concat [ testFixtures, "registry-index", "ef" ]
 
       let
         readFixtures = do

app/test/App/Server/MatrixBuilder.purs

@@ -0,0 +1,109 @@
+module Test.Registry.App.Server.MatrixBuilder (spec) where
+
+import Registry.App.Prelude
+
+import Data.Map as Map
+import Data.Set as Set
+import Effect.Ref as Ref
+import Registry.App.Server.MatrixBuilder as MatrixBuilder
+import Registry.ManifestIndex as ManifestIndex
+import Registry.PackageName as PackageName
+import Registry.Solver as Solver
+import Registry.Test.Assert.Run (runRegistryMock)
+import Registry.Test.Utils as Utils
+import Test.Spec as Spec
+import Test.Spec.Assertions as Assert
+
+spec :: Spec.Spec Unit
+spec = do
+  Spec.describe "solveDependantsForCompiler" do
+    Spec.it "cascades to dependant for a new compiler" do
+      let { solverData, index, metadata } = setup [ "0.15.10" ]
+      result <- runSolver solverData index metadata
+      let names = Set.map _.name result
+      unless (Set.member effectName names) do
+        Assert.fail $ "Expected cascade to effect, but got: "
+          <> show (Set.map PackageName.print names)
+
+    Spec.it "skips dependant already tested with this compiler" do
+      let { solverData, index, metadata } = setup [ "0.15.10", "0.15.11" ]
+      result <- runSolver solverData index metadata
+      unless (Set.isEmpty result) do
+        Assert.fail "Expected empty result set when compiler already tested"
+
+  where
+  preludeName = Utils.unsafePackageName "prelude"
+  effectName = Utils.unsafePackageName "effect"
+
+  compiler_0_15_10 = Utils.unsafeVersion "0.15.10"
+  compiler_0_15_11 = Utils.unsafeVersion "0.15.11"
+  allCompilers = Utils.unsafeNonEmptyArray [ compiler_0_15_10, compiler_0_15_11 ]
+
+  preludeVersion = Utils.unsafeVersion "6.0.0"
+
+  preludeManifest = Utils.unsafeManifest "prelude" "6.0.0" []
+  effectManifest = Utils.unsafeManifest "effect" "4.0.0"
+    [ Tuple "prelude" ">=6.0.0 <7.0.0" ]
+
+  dummySha = Utils.unsafeSha256 "sha256-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa="
+  dummyTime = Utils.unsafeDateTime "2022-08-18T20:04:00.000Z"
+
+  mkPublishedMeta version compilers =
+    Map.singleton version
+      { bytes: 1000.0
+      , compilers: Utils.unsafeNonEmptyArray (map Utils.unsafeVersion compilers)
+      , hash: dummySha
+      , publishedTime: dummyTime
+      , ref: Nothing
+      }
+
+  -- | Set up a test scenario where prelude@6.0.0 (no deps) has completed its
+  -- | matrix job for 0.15.11, and effect@4.0.0 (depends on prelude) has the
+  -- | given compilers in its metadata.
+  setup effectCompilers = do
+    let
+      index = Utils.fromRight "Failed to build ManifestIndex" do
+        ManifestIndex.insert ManifestIndex.ConsiderRanges preludeManifest ManifestIndex.empty
+          >>= ManifestIndex.insert ManifestIndex.ConsiderRanges effectManifest
+
+      -- prelude must include 0.15.11 in its compilers because this test
+      -- simulates the state AFTER prelude's own matrix job has completed.
+      -- In the real flow: runMatrixJob writes the new compiler into the
+      -- parent's metadata (MatrixBuilder.purs:82-91), then the executor
+      -- rebuilds the CompilerIndex from current metadata before calling
+      -- solveDependantsForCompiler. Without 0.15.11 here, the solver
+      -- would compute purs >=0.15.10 <0.15.11 for prelude, excluding
+      -- the target compiler from effect's build plan.
+      preludeMetadata = Metadata
+        { location: Git { url: "https://github.com/purescript/purescript-prelude.git", subdir: Nothing }
+        , owners: Nothing
+        , published: mkPublishedMeta preludeVersion [ "0.15.10", "0.15.11" ]
+        , unpublished: Map.empty
+        }
+
+      effectMetadata = Metadata
+        { location: Git { url: "https://github.com/purescript/purescript-effect.git", subdir: Nothing }
+        , owners: Nothing
+        , published: mkPublishedMeta (Utils.unsafeVersion "4.0.0") effectCompilers
+        , unpublished: Map.empty
+        }
+
+      metadata = Map.fromFoldable [ Tuple preludeName preludeMetadata, Tuple effectName effectMetadata ]
+
+      compilerIndex = Solver.buildCompilerIndex allCompilers index metadata
+
+      solverData =
+        { compilerIndex
+        , compiler: compiler_0_15_11
+        , name: preludeName
+        , version: preludeVersion
+        , dependencies: Map.empty
+        }
+
+    { solverData, index, metadata }
+
+  runSolver solverData index metadata = liftAff do
+    indexRef <- liftEffect $ Ref.new index
+    metadataRef <- liftEffect $ Ref.new metadata
+    runRegistryMock metadataRef indexRef
+      $ MatrixBuilder.solveDependantsForCompiler solverData

app/test/Main.purs

@@ -16,6 +16,7 @@ import Test.Registry.App.Legacy.LenientVersion as Test.Legacy.LenientVersion
 import Test.Registry.App.Legacy.Manifest as Test.Legacy.Manifest
 import Test.Registry.App.Legacy.PackageSet as Test.Legacy.PackageSet
 import Test.Registry.App.Manifest.SpagoYaml as Test.Manifest.SpagoYaml
+import Test.Registry.App.Server.MatrixBuilder as Test.Server.MatrixBuilder
 import Test.Spec as Spec
 import Test.Spec.Reporter.Console (consoleReporter)
 import Test.Spec.Runner.Node (runSpecAndExitProcess')
@@ -49,6 +50,9 @@ main = runSpecAndExitProcess' config [ consoleReporter ] do
 
   Spec.describe "Registry.App.Manifest" do
     Spec.describe "SpagoYaml" Test.Manifest.SpagoYaml.spec
+
+  Spec.describe "Registry.App.Server" do
+    Spec.describe "MatrixBuilder" Test.Server.MatrixBuilder.spec
   where
   config =
     { defaultConfig: Cfg.defaultConfig { timeout = Just $ Milliseconds 300_000.0 }

app/test/Test/Assert/Run.purs

@@ -3,6 +3,7 @@
 module Registry.Test.Assert.Run
   ( TEST_EFFECTS
   , runBaseEffects
+  , runRegistryMock
   , runTestEffects
   , shouldContain
   , shouldNotContain
@@ -139,6 +140,12 @@ runBaseEffects = do
     >>> Except.catch (\err -> Run.liftAff (Aff.throwError (Aff.error err)))
     >>> Run.runBaseAff'
 
+-- | For testing Run functions that only need the REGISTRY effect.
+runRegistryMock :: forall a. Ref (Map PackageName Metadata) -> Ref ManifestIndex -> Run (EXCEPT String + LOG + REGISTRY + AFF + EFFECT + ()) a -> Aff a
+runRegistryMock metadataRef indexRef =
+  Registry.interpret (handleRegistryMock { metadataRef, indexRef })
+    >>> runBaseEffects
+
 runGitHubCacheMemory :: forall r a. CacheRef -> Run (GITHUB_CACHE + LOG + EFFECT + r) a -> Run (LOG + EFFECT + r) a
 runGitHubCacheMemory = Cache.interpret GitHub._githubCache <<< Cache.handleMemory
 

lib/src/ManifestIndex.purs

@@ -133,10 +133,9 @@ insert consider manifest@(Manifest { name, version, dependencies }) (ManifestInd
     Left unsatisfied
 
 -- | Delete a package version from the manifest index, failing if it produces an
--- | invalid manifest index. Since we only verify unsatisfied dependencies wrt
--- | package names (and not package versions), it is always acceptable to delete
--- | a package version so long as it has at least 2 versions. However, removing
--- | a package altogether incurs a full validation check.
+-- | invalid manifest index. When considering ranges, we must validate that
+-- | removing the version doesn't leave any dependent's ranges unsatisfied,
+-- | even if other versions of the same package remain.
 delete :: IncludeRanges -> PackageName -> Version -> ManifestIndex -> Either (Map PackageName (Map Version (Map PackageName Range))) ManifestIndex
 delete consider name version (ManifestIndex index) = do
   case Map.lookup name index of
@@ -146,8 +145,14 @@ delete consider name version (ManifestIndex index) = do
         Tuple _ versions <- Map.toUnfoldableUnordered (Map.delete name index)
         Tuple _ manifest <- Map.toUnfoldableUnordered versions
         [ manifest ]
-    Just _ -> do
-      pure (ManifestIndex (Map.update (Just <<< Map.delete version) name index))
+    Just _ -> case consider of
+      IgnoreRanges ->
+        pure (ManifestIndex (Map.update (Just <<< Map.delete version) name index))
+      ConsiderRanges ->
+        fromSet consider $ Set.fromFoldable do
+          Tuple _ versions <- Map.toUnfoldableUnordered (Map.update (Just <<< Map.delete version) name index)
+          Tuple _ manifest <- Map.toUnfoldableUnordered versions
+          [ manifest ]
 
 -- | Convert a set of manifests into a `ManifestIndex`. Reports all failures
 -- | encountered rather than short-circuiting.

lib/test/Registry/ManifestIndex.purs

@@ -139,6 +139,40 @@ spec = do
 
     testIndex ManifestIndex.ConsiderRanges { satisfied, unsatisfied: [] }
 
+  Spec.it "Rejects deletion when remaining versions don't satisfy dependents' ranges" do
+    let
+      prelude = unsafeManifest "prelude" "1.0.0" []
+      effect_1 = unsafeManifest "effect" "1.0.0" [ Tuple "prelude" ">=1.0.0 <2.0.0" ]
+      effect_2 = unsafeManifest "effect" "2.0.0" [ Tuple "prelude" ">=1.0.0 <2.0.0" ]
+      -- console depends on effect >=2.0.0, so removing effect@2.0.0 should fail
+      console = unsafeManifest "console" "1.0.0" [ Tuple "effect" ">=2.0.0 <3.0.0" ]
+      index = ManifestIndex.insert ManifestIndex.ConsiderRanges prelude ManifestIndex.empty
+        >>= ManifestIndex.insert ManifestIndex.ConsiderRanges effect_1
+        >>= ManifestIndex.insert ManifestIndex.ConsiderRanges effect_2
+        >>= ManifestIndex.insert ManifestIndex.ConsiderRanges console
+    case index of
+      Left errors -> Assert.fail $ formatInsertErrors errors
+      Right idx -> case ManifestIndex.delete ManifestIndex.ConsiderRanges (Utils.unsafePackageName "effect") (Utils.unsafeVersion "2.0.0") idx of
+        Left _ -> pure unit
+        Right _ -> Assert.fail "Expected deletion to fail when remaining versions don't satisfy dependent's range"
+
+  Spec.it "Allows deletion when remaining versions satisfy dependents' ranges" do
+    let
+      prelude = unsafeManifest "prelude" "1.0.0" []
+      effect_1 = unsafeManifest "effect" "1.0.0" [ Tuple "prelude" ">=1.0.0 <2.0.0" ]
+      effect_2 = unsafeManifest "effect" "2.0.0" [ Tuple "prelude" ">=1.0.0 <2.0.0" ]
+      -- console depends on effect >=1.0.0, so removing effect@2.0.0 is fine (1.0.0 satisfies)
+      console = unsafeManifest "console" "1.0.0" [ Tuple "effect" ">=1.0.0 <3.0.0" ]
+      index = ManifestIndex.insert ManifestIndex.ConsiderRanges prelude ManifestIndex.empty
+        >>= ManifestIndex.insert ManifestIndex.ConsiderRanges effect_1
+        >>= ManifestIndex.insert ManifestIndex.ConsiderRanges effect_2
+        >>= ManifestIndex.insert ManifestIndex.ConsiderRanges console
+    case index of
+      Left errors -> Assert.fail $ formatInsertErrors errors
+      Right idx -> case ManifestIndex.delete ManifestIndex.ConsiderRanges (Utils.unsafePackageName "effect") (Utils.unsafeVersion "2.0.0") idx of
+        Left errors -> Assert.fail $ "Expected deletion to succeed but got errors:\n" <> formatDeleteErrors errors
+        Right _ -> pure unit
+
   Spec.it "Does not parse unacceptable cyclical index" do
     let
       unsatisfied :: Array Manifest
@@ -213,6 +247,12 @@ testSorted input = do
       , JSON.printIndented $ CJ.encode (CJ.array manifestCodec') sorted
       ]
 
+formatDeleteErrors :: Map PackageName (Map Version.Version (Map PackageName Range)) -> String
+formatDeleteErrors errors = String.joinWith "\n"
+  [ "Failed to delete. Unsatisfied:"
+  , JSON.printIndented $ CJ.encode (Internal.Codec.packageMap (Internal.Codec.versionMap (Internal.Codec.packageMap Range.codec))) errors
+  ]
+
 formatInsertErrors :: Map PackageName Range -> String
 formatInsertErrors errors = String.joinWith "\n"
   [ "Failed to insert. Failed to satisfy:"