Inconsistent handling of dependent databases during close and destroy

[maximerety]

Issue

I noticed some possible inconsistencies while reading the recently added fixes to close dependent databases when closing leveldb databases.

I am absolutely not an expert on PouchDB and its code base, so I may have misunderstood what was done and what was the correct way to handle these cases. Please correct me if I am wrong.

Here are the previous events to understand my concerns:

1. In January 2021, Fix #7331 - close leveldb file handlers #8256 is merged in order to "close leveldb file handlers" directly in the leveldb core adapter. This has been done to fix the issue reported in close does not close dependent dbs fully #7331. Note that no tests were required to merge this work.
2. In April 2022, the version 7.3.0 of PouchDB is released including this fix.
3. In May 2022, a bug report is submitted in the form of a pull request containing a bug repro (fix: Race condition for pouchdb hanging indefinitely when multiple instances are used #8513). It shows that the fix merged in step 1. poses problems for codebases where multiple PouchDB databases are used side-by-side. Soon after, a pull request is submitted (Fix the race condition when closing one of multiple databases. #8515) to fix that issue.
4. In August 2022, having noticed the problem described in step 3. in my own codebase, I started to follow the issue (see Fix the race condition when closing one of multiple databases. #8515 (comment)).
5. In November 2022, I noticed that the proposed fix did not ship in release 7.3.1 of PouchDB and asked for guidance on how to help having a fix released. It was rightly suggested that tests were needed (Fix the race condition when closing one of multiple databases. #8515 (comment)).
6. I started working on tests but after looking at the codebase, I got very confused about how the things were handled internally and decided I needed more time to understand if fixes proposed in 1. and 3. were properly designed.
7. In the meantime, the fix proposed in step 3. was cherry-picked in the repro pull request (fix: Race condition for pouchdb hanging indefinitely when multiple instances are used #8513) and it got merged to master.

At this point, I could patiently wait for release 7.3.2 to ship and consider the issue fixed.

But I'd like to share what I've seen during work done at step 6. so that more knowledgable people can eventually rework the codebase if the fixes listed above actually introduced inconstencies.

So, here we go.

I first took the time to review how things were designed in the code base:

• In pouchdb-core/src/adapter.js, common methods for destroy and close actions are implemented, which delegate some work to the underlying adapter chosen.
• In pouchdb-adapter-leveldb-core/src/index.js, adapter-specific implementations for destroy and close methods are available, and are invoked by the common methods previously linked.

The thing that bothers me is that the common implementation for destroy is responsible for destroying dependent databases, whereas due to fixes introduced in steps 1. and 3. above, the adapter-specific implementation of the close method is reponsible to close dependent databases.

This seems inconstent, and this might introduce various bugs (such as _local/_pouch_dependentDbs referencing databases non longer existing on disk, cf initial use case described in step 1. above (#7331)).

In my opinion, dependent databases should be handled consistently either in the common implementation, or in the adapter-specific implementation, but not with code spread over both sides...

Some reasons to favor the common implementation:

• Genericity of the code: other adapters than leveldb may need the same logic
• Anteriority of the pattern: the handling of dependent databases at this level dates back to 2014 (35cd3abb), while the "recent" leveldb adapter fixes date back to 2021/2022.

I'll submit a pull request soon to demonstrate what it could look like if we handled dependent databases only in the common implementation. Edit Here it is: #8575

[janl]

just leaving a brief note here that this is an amazing report, thank you! 🙏

[rasgo-cc]

Hey @maximerety - I did the #7331 but definitely wasn't able to be as thorough as you. As of now, not sure what would be easier/faster (probably not better?) but 3) (#8513) definitely broke 1) (#7331) as stated now on #8609 . #7331 didn't include a test, which maybe could have prevented this - I'll see if I can create one and make the code work in both cases.