Fix: hash-based references to upstream specs (#14629)

Spack commands referring to upstream-installed specs by hash have
been broken since 6b619da (merged September 2019), which added a new
Database function specifically for parsing hashes from command-line
specs; this function was inappropriately attempting to acquire locks
on upstream databases.

This PR updates the offending function to avoid locking upstream
databases and also updates associated tests to catch regression
errors: the upstream database created for these tests was not
explicitly set as an upstream (i.e. initialized with upstream=True)
so it was not guarding against inappropriate accesses.

2 files changed, 56 insertions, 36 deletions

diff --git a/lib/spack/spack/database.py b/lib/spack/spack/database.py
index 22fc266b5b..c06d1ae546 100644
--- a/lib/spack/spack/database.py
+++ b/lib/spack/spack/database.py
@@ -1118,7 +1118,27 @@ class Database(object):
                 continue
             # TODO: conditional way to do this instead of catching exceptions
 
-    def get_by_hash_local(self, dag_hash, default=None, installed=any):
+    def _get_by_hash_local(self, dag_hash, default=None, installed=any):
+        # hash is a full hash and is in the data somewhere
+        if dag_hash in self._data:
+            rec = self._data[dag_hash]
+            if rec.install_type_matches(installed):
+                return [rec.spec]
+            else:
+                return default
+
+        # check if hash is a prefix of some installed (or previously
+        # installed) spec.
+        matches = [record.spec for h, record in self._data.items()
+                   if h.startswith(dag_hash) and
+                   record.install_type_matches(installed)]
+        if matches:
+            return matches
+
+        # nothing found
+        return default
+
+    def get_by_hash_local(self, *args, **kwargs):
         """Look up a spec in *this DB* by DAG hash, or by a DAG hash prefix.
 
         Arguments:
@@ -1142,24 +1162,7 @@ class Database(object):
 
         """
         with self.read_transaction():
-            # hash is a full hash and is in the data somewhere
-            if dag_hash in self._data:
-                rec = self._data[dag_hash]
-                if rec.install_type_matches(installed):
-                    return [rec.spec]
-                else:
-                    return default
-
-            # check if hash is a prefix of some installed (or previously
-            # installed) spec.
-            matches = [record.spec for h, record in self._data.items()
-                       if h.startswith(dag_hash) and
-                       record.install_type_matches(installed)]
-            if matches:
-                return matches
-
-            # nothing found
-            return default
+            return self._get_by_hash_local(*args, **kwargs)
 
     def get_by_hash(self, dag_hash, default=None, installed=any):
         """Look up a spec by DAG hash, or by a DAG hash prefix.
@@ -1184,9 +1187,14 @@ class Database(object):
             (list): a list of specs matching the hash or hash prefix
 
         """
-        search_path = [self] + self.upstream_dbs
-        for db in search_path:
-            spec = db.get_by_hash_local(
+
+        spec = self.get_by_hash_local(
+            dag_hash, default=default, installed=installed)
+        if spec is not None:
+            return spec
+
+        for upstream_db in self.upstream_dbs:
+            spec = upstream_db._get_by_hash_local(
                 dag_hash, default=default, installed=installed)
             if spec is not None:
                 return spec
diff --git a/lib/spack/spack/test/database.py b/lib/spack/spack/test/database.py
index 24a219a491..a2b9677ec6 100644
--- a/lib/spack/spack/test/database.py
+++ b/lib/spack/spack/test/database.py
@@ -41,10 +41,11 @@ def test_store(tmpdir):
 @pytest.fixture()
 def upstream_and_downstream_db(tmpdir_factory, gen_mock_layout):
     mock_db_root = str(tmpdir_factory.mktemp('mock_db_root'))
-    upstream_db = spack.database.Database(mock_db_root)
+    upstream_write_db = spack.database.Database(mock_db_root)
+    upstream_db = spack.database.Database(mock_db_root, is_upstream=True)
     # Generate initial DB file to avoid reindex
-    with open(upstream_db._index_path, 'w') as db_file:
-        upstream_db._write_to_file(db_file)
+    with open(upstream_write_db._index_path, 'w') as db_file:
+        upstream_write_db._write_to_file(db_file)
     upstream_layout = gen_mock_layout('/a/')
 
     downstream_db_root = str(
@@ -55,13 +56,14 @@ def upstream_and_downstream_db(tmpdir_factory, gen_mock_layout):
         downstream_db._write_to_file(db_file)
     downstream_layout = gen_mock_layout('/b/')
 
-    yield upstream_db, upstream_layout, downstream_db, downstream_layout
+    yield upstream_write_db, upstream_db, upstream_layout,\
+        downstream_db, downstream_layout
 
 
 @pytest.mark.usefixtures('config')
 def test_installed_upstream(upstream_and_downstream_db):
-    upstream_db, upstream_layout, downstream_db, downstream_layout = (
-        upstream_and_downstream_db)
+    upstream_write_db, upstream_db, upstream_layout,\
+        downstream_db, downstream_layout = (upstream_and_downstream_db)
 
     default = ('build', 'link')
     x = MockPackage('x', [], [])
@@ -75,7 +77,14 @@ def test_installed_upstream(upstream_and_downstream_db):
         spec.concretize()
 
         for dep in spec.traverse(root=False):
-            upstream_db.add(dep, upstream_layout)
+            upstream_write_db.add(dep, upstream_layout)
+        upstream_db._read()
+
+        for dep in spec.traverse(root=False):
+            record = downstream_db.get_by_hash(dep.dag_hash())
+            assert record is not None
+            with pytest.raises(spack.database.ForbiddenLockError):
+                record = upstream_db.get_by_hash(dep.dag_hash())
 
         new_spec = spack.spec.Spec('w')
         new_spec.concretize()
@@ -96,8 +105,8 @@ def test_installed_upstream(upstream_and_downstream_db):
 
 @pytest.mark.usefixtures('config')
 def test_removed_upstream_dep(upstream_and_downstream_db):
-    upstream_db, upstream_layout, downstream_db, downstream_layout = (
-        upstream_and_downstream_db)
+    upstream_write_db, upstream_db, upstream_layout,\
+        downstream_db, downstream_layout = (upstream_and_downstream_db)
 
     default = ('build', 'link')
     z = MockPackage('z', [], [])
@@ -108,13 +117,15 @@ def test_removed_upstream_dep(upstream_and_downstream_db):
         spec = spack.spec.Spec('y')
         spec.concretize()
 
-        upstream_db.add(spec['z'], upstream_layout)
+        upstream_write_db.add(spec['z'], upstream_layout)
+        upstream_db._read()
 
         new_spec = spack.spec.Spec('y')
         new_spec.concretize()
         downstream_db.add(new_spec, downstream_layout)
 
-        upstream_db.remove(new_spec['z'])
+        upstream_write_db.remove(new_spec['z'])
+        upstream_db._read()
 
         new_downstream = spack.database.Database(
             downstream_db.root, upstream_dbs=[upstream_db])
@@ -129,8 +140,8 @@ def test_add_to_upstream_after_downstream(upstream_and_downstream_db):
     DB. When a package is recorded as installed in both, the results should
     refer to the downstream DB.
     """
-    upstream_db, upstream_layout, downstream_db, downstream_layout = (
-        upstream_and_downstream_db)
+    upstream_write_db, upstream_db, upstream_layout,\
+        downstream_db, downstream_layout = (upstream_and_downstream_db)
 
     x = MockPackage('x', [], [])
     mock_repo = MockPackageMultiRepo([x])
@@ -141,7 +152,8 @@ def test_add_to_upstream_after_downstream(upstream_and_downstream_db):
 
         downstream_db.add(spec, downstream_layout)
 
-        upstream_db.add(spec, upstream_layout)
+        upstream_write_db.add(spec, upstream_layout)
+        upstream_db._read()
 
         upstream, record = downstream_db.query_by_spec_hash(spec.dag_hash())
         # Even though the package is recorded as installed in the upstream DB,