🐛 Avoid importing and calling close in __del__ during shutdown

[agoscinski]

Two related fixes to prevent 'Exception ignored in del' errors during Python shutdown:

1. Move 'import warnings' to module level in RabbitmqBroker and StorageBackend to avoid ImportError when sys.meta_path is None during shutdown.

2. Guard __del__ with sys.is_finalizing() to skip the close() call during Python shutdown when asyncio/kiwipy event loop infrastructure is already torn down, preventing AttributeError in pytray's await_ method.

During shutdown:

• sys.meta_path becomes None, making imports unavailable
• Module globals are set to None before garbage collection
• asyncio.run_coroutine_threadsafe() fails with AttributeError

By deferring the import and skipping close during finalization, we avoid both issues since the process is exiting anyway.

Fixes #7309

---

Further changes in the future

When doing the overhaul of the logging system in #7323 I will change the warning to using the logger system so it is properly tracked. ResourceWarnings are only shown when a certain env is on or python is run in developer mode. Both is not really useful for us. The logger system only works however when not finalizing. So we cannot track places where resources were not close during a python shutdown, which is fine.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.92%. Comparing base (62f105f) to head (57a47eb).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7322      +/-   ##
==========================================
+ Coverage   79.89%   79.92%   +0.04%     
==========================================
  Files         568      568              
  Lines       43984    43988       +4     
==========================================
+ Hits        35136    35153      +17     
+ Misses       8848     8835      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[agoscinski]

Regarding codecov I am not sure how much sense it makes to test that a warning intended for developer is emitted.

[mbercx]

If you are OCD about green checkmarks (like me), you can try adding:

Suggested change

	self.close()
	self.close() # pragma: no cover

See https://coverage.readthedocs.io/en/latest/excluding.html

[agoscinski]

I think I want to add a test for this line actually. When I come back on this PR I add a test and merge it.

[agoscinski]

done added tests

[mbercx]

Thanks @agoscinski! I will give this a spin now.

[danielhollas]

The logger system only works however when not finalizing. So we cannot track places where resources were not close during a python shutdown, which is fine.

Hmm, I am pretty sure the python logging module should still work during Python shutdown. (don't ask me how I know, but there are specific regression tests in cpython that ensure that logging calls work in __del__ statements specifically).

[mbercx]

Quick field test: ImportError still there on main, but not on this PR. I don't have time for a code review, but at least can confirm it works on my machine. ^^

[agoscinski]

Hmm, I am pretty sure the python logging module should still work during Python shutdown. (don't ask me how I know, but there are specific regression tests in cpython that ensure that logging calls work in del statements specifically).

It was something that we added to the stdlib logger in our logging system that did not make it work when I tried it out (maybe some font styling?). I can investigate it again when I am doing the logging overhaul, because it would be useful to log also these cases. Interestingly emitting a warning through the stdlib warning module also does not print a warning in this part of the code when it is finalizing, and it might be bug in CPython. It only happened because some other code holded a reference of something related to warnings. I attach the result with agent here (test_warn_bug.zip) but it looked like a rabbit hole I did not want to go into.

[agoscinski]

Its really tricky to test this properly. Using a fixture will not work because pytest still has a reference to the object after deletion. Inheriting from StorageBackend is not trivial because we need to implement its interface which makes the test more complicated than necessary. I don't have a better idea than this.

[agoscinski]

So we could do this instead, but I am not sure if this is better.

   def test_del_closes_backend_when_not_finalizing(aiida_profile, monkeypatch):                 
       """Test `__del__` closes the backend when Python is not finalizing."""                   
       backend = aiida_profile.storage_cls(aiida_profile)                                       

       session_factory = backend._session_factory                                               
       assert session_factory is not None       

       engine = session_factory.bind            
       assert engine is not None                

       original_dispose = engine.dispose        
       dispose = MagicMock(side_effect=original_dispose)                                        
       monkeypatch.setattr(engine, 'dispose', dispose)                                          

       with pytest.warns(ResourceWarning, match='StorageBackend was not closed explicitly'):    
           del backend                          
           gc.collect()                         

       dispose.assert_called_once_with()

We cannot really test through the storage backend interface because we are deleting it. By mocking the underlying resource we basically only test if the close is invoked. So we can simplify the test to just as it is if close was invoked.

[agoscinski]

note that we need a bool in a dict because we need an object that we still can reference after the deletion

[agoscinski]

Okay added tests. I iterated a bit on them. They seem a bit dump but it is just impossible to test if an object is correctly closed if we delete the object without making the test also dependent on the implementation details of close, and I think these added tests should not depend on the implementation detail of close.

[GeigerJ2]

OK, my agent is I am done with the review. Fix looks correct and well-scoped, thanks, @agoscinski. Two suggestions:

1. The existing tests cover the "not finalizing" path (__del__ → close() called), but the opposite path (the actual new behavior: close() skipped when sys.is_finalizing() is True) is untested. Consider adding the two complementary tests shown below. Both monkeypatch sys.is_finalizing to return True and verify that close() is skipped while the ResourceWarning is still emitted.

   In tests/brokers/test_rabbitmq.py:

   def test_del_skips_close_when_finalizing(aiida_profile, monkeypatch):
       """Test ``__del__`` skips close when Python is finalizing."""
       broker = RabbitmqBroker(aiida_profile)
       broker._communicator = MagicMock()
       close = MagicMock()
       monkeypatch.setattr(broker, 'close', close)
       monkeypatch.setattr('sys.is_finalizing', lambda: True)
   
       with pytest.warns(ResourceWarning, match='RabbitmqBroker was not closed explicitly'):
           broker.__del__()
   
       close.assert_not_called()

   In tests/orm/implementation/test_backend.py:

   def test_del_skips_close_when_finalizing(aiida_profile, monkeypatch):
       """Test ``__del__`` skips close when Python is finalizing."""
       backend = aiida_profile.storage_cls(aiida_profile)
       close = MagicMock()
       monkeypatch.setattr(backend, 'close', close)
       monkeypatch.setattr('sys.is_finalizing', lambda: True)
   
       with pytest.warns(ResourceWarning, match='StorageBackend was not closed explicitly'):
           backend.__del__()
   
       close.assert_not_called()

2. side_effect=broker.close / side_effect=backend.close in the existing tests means the mock delegates to the real close(). Since only close.assert_called_once_with() is checked, the side effect adds nothing to what's verified. A plain MagicMock() would be simpler and work just as well.

[agoscinski]

Added feedback from review and merging.