@aweits

I'm still poking at it, but isn't something like this a better approach? This currently passes all tests along with the one you added here...

diff --git a/lib/spack/spack/database.py b/lib/spack/spack/database.py
index ca9ed67fb5b5..45d148a3d17a 100644
--- a/lib/spack/spack/database.py
+++ b/lib/spack/spack/database.py
@@ -1128,6 +1128,8 @@ def _remove(self, spec):
 
         del self._data[key]
         for dep in rec.spec.dependencies(_tracked_deps):
+            if dep._dependents.get(spec.name):
+                del dep._dependents[spec.name]
             self._decrement_ref_count(dep)

@aweits I'm fine with both approaches, so no-problem changing what is in the PR. I did what is here based on the observation that each public method that needs update is wrapped in a transaction so ensuring consistency when the transaction exits would suffice (and self._write is called at the end of a write transaction). Your approach seems more focused and I like it. I'd say let's check that there are no other paths under a write transaction that might need updates and then let's change the code as you suggest?

I'm still concerned that I need this bit, I think I'm still missing something.

if dep._dependents.get(spec.name):

@aweits Pushed the change.

I'm still concerned that I need this bit,

I looked into the failing tests with a debugger and I think it's #11983 biting us again (that issue was never solved).

Given those constraints, what do you think of this instead?

diff --git a/lib/spack/spack/database.py b/lib/spack/spack/database.py
index ca9ed67fb5b5..6804dde906e1 100644
--- a/lib/spack/spack/database.py
+++ b/lib/spack/spack/database.py
@@ -1107,6 +1107,8 @@ def _decrement_ref_count(self, spec):
             del self._data[key]
             for dep in spec.dependencies(_tracked_deps):
                 self._decrement_ref_count(dep)
+        if rec.ref_count == 0:
+            spec._dependents.clear()
 
     def _increment_ref_count(self, spec):
         key = spec.dag_hash()

@aweits Since in the JSON database we encode only ref_count and dependencies I think the two implementations you suggest are equivalent (i.e. they don't introduce any error in the DB but are equivalently wrong for specs in memory due to the bug).

I would stick with the first one because it's the one that in my opinion is closest to what it should be once the bug is fixed. In that case I think we should remove specs from dependents by hash without any condition to protect them. Something logically equivalent to:

for dep in rec.spec.dependencies(_tracked_deps):
    del dep._dependents[spec.dag_hash()]

Does that sound reasonable? If so I'll just add a verbose FIXME comment on top of that change to explain the situation in details.

Concur.

@tgamblin