From 342520175d751a46651e7cb3a1c39993ff7aae32 Mon Sep 17 00:00:00 2001
From: Harmen Stoppels <me@harmenstoppels.nl>
Date: Thu, 31 Oct 2024 21:58:42 +0100
Subject: [PATCH] database.py: remove process unsafe update_explicit (#47358)

Fixes an issue reported where `spack env depfile` + `make -j` would
non-deterministically refuse to mark all environment roots explicit.

`update_explicit` had the pattern

```python
rec = self._data[key]
with self.write_transaction():
    rec.explicit = explicit
```

but `write_transaction` may reinitialize `self._data`, meaning that
mutating `rec` won't mutate `self._data`, and the changes won't be
persisted.

Instead, use `mark` which has a correct implementation.

Also avoids the essentially incorrect early return in `update_explicit`
which is a pattern I don't think belongs in database.py: it branches on
possibly stale data to realize there is nothing to change, but in reality
it requires a write transaction to know that for a fact, but that would
defeat the purpose. So, leave this optimization to the call site.
---
 lib/spack/spack/cmd/mark.py  |  5 +++--
 lib/spack/spack/database.py  | 20 +-------------------
 lib/spack/spack/installer.py |  6 +++---
 3 files changed, 7 insertions(+), 24 deletions(-)

diff --git a/lib/spack/spack/cmd/mark.py b/lib/spack/spack/cmd/mark.py
index cf816a21f54..21bba5a5f94 100644
--- a/lib/spack/spack/cmd/mark.py
+++ b/lib/spack/spack/cmd/mark.py
@@ -101,8 +101,9 @@ def do_mark(specs, explicit):
         specs (list): list of specs to be marked
         explicit (bool): whether to mark specs as explicitly installed
     """
-    for spec in specs:
-        spack.store.STORE.db.update_explicit(spec, explicit)
+    with spack.store.STORE.db.write_transaction():
+        for spec in specs:
+            spack.store.STORE.db.mark(spec, "explicit", explicit)
 
 
 def mark_specs(args, specs):
diff --git a/lib/spack/spack/database.py b/lib/spack/spack/database.py
index ff9e5dae155..5d86b4fe9a0 100644
--- a/lib/spack/spack/database.py
+++ b/lib/spack/spack/database.py
@@ -1367,7 +1367,7 @@ def _deprecate(self, spec, deprecator):
         self._data[spec_key] = spec_rec
 
     @_autospec
-    def mark(self, spec, key, value):
+    def mark(self, spec: "spack.spec.Spec", key: str, value: Any) -> None:
         """Mark an arbitrary record on a spec."""
         with self.write_transaction():
             return self._mark(spec, key, value)
@@ -1709,24 +1709,6 @@ def root(key, record):
                 if id(rec.spec) not in needed and rec.installed
             ]
 
-    def update_explicit(self, spec, explicit):
-        """
-        Update the spec's explicit state in the database.
-
-        Args:
-            spec (spack.spec.Spec): the spec whose install record is being updated
-            explicit (bool): ``True`` if the package was requested explicitly
-                by the user, ``False`` if it was pulled in as a dependency of
-                an explicit package.
-        """
-        rec = self.get_record(spec)
-        if explicit != rec.explicit:
-            with self.write_transaction():
-                message = "{s.name}@{s.version} : marking the package {0}"
-                status = "explicit" if explicit else "implicit"
-                tty.debug(message.format(status, s=spec))
-                rec.explicit = explicit
-
 
 class UpstreamDatabaseLockingError(SpackError):
     """Raised when an operation would need to lock an upstream database"""
diff --git a/lib/spack/spack/installer.py b/lib/spack/spack/installer.py
index 54dea973080..43bb066a10f 100644
--- a/lib/spack/spack/installer.py
+++ b/lib/spack/spack/installer.py
@@ -440,7 +440,7 @@ def _process_external_package(pkg: "spack.package_base.PackageBase", explicit: b
         tty.debug(f"{pre} already registered in DB")
         record = spack.store.STORE.db.get_record(spec)
         if explicit and not record.explicit:
-            spack.store.STORE.db.update_explicit(spec, explicit)
+            spack.store.STORE.db.mark(spec, "explicit", True)
 
     except KeyError:
         # If not, register it and generate the module file.
@@ -1363,8 +1363,8 @@ def _prepare_for_install(self, task: BuildTask) -> None:
             self._update_installed(task)
 
             # Only update the explicit entry once for the explicit package
-            if task.explicit:
-                spack.store.STORE.db.update_explicit(task.pkg.spec, True)
+            if task.explicit and not rec.explicit:
+                spack.store.STORE.db.mark(task.pkg.spec, "explicit", True)
 
     def _cleanup_all_tasks(self) -> None:
         """Cleanup all build tasks to include releasing their locks."""