MatrixAI / js-encryptedfs

Encrypted Filesystem for TypeScript/JavaScript Applications
https://polykey.com
Apache License 2.0
10 stars 3 forks source link

Added fresh parameter and readiness tests for EncryptedFS #58

Closed CMCDragonkai closed 2 years ago

CMCDragonkai commented 2 years ago

Description

The EFS didn't have a fresh parameter. Adding this in to match https://github.com/MatrixAI/js-db/pull/6.

Also noticed that EFS doesn't have readiness testing. I'm adding this in to tests/EncryptedFS.test.ts.

Related to https://github.com/MatrixAI/js-polykey/pull/266#issuecomment-1010603563

Tasks

  1. [x] Add fresh to EncryptedFS and maybe INodeManager
  2. [x] Readiness testing
  3. [x] Added promise rejection to subleveldown() and listening for open errors when subleveldown is called

Final checklist

CMCDragonkai commented 2 years ago

As discovered here, EFS is not passing a persistence test: https://github.com/MatrixAI/js-polykey/pull/266#issuecomment-1010639788.

This may have something to do with INodeManager not having reset up everything because it is CD, and its setup is not redone during EncryptedFS.start.

CMCDragonkai commented 2 years ago

A variant of this is:

    const efs1 = await EncryptedFS.createEncryptedFS({
      dbPath: dataDir,
      dbKey,
      logger,
    });
    await efs1.writeFile('testfile', 'hello world');
    await efs1.stop();
    await efs1.start();
    try {
      await efs1.readFile('testfile', { encoding: 'utf-8' });
    } catch (e) {
      console.log('OH NO', e);
    }
    await efs1.stop();

If the readFile is done on a separate file, we get ENOENT which is a correct error. But if we try to read testfile itself, we end up with the Inner database is not open internally.

That's super weird.

However it seems strange we cannot get the underlying exception of Inner database is not open.

CMCDragonkai commented 2 years ago

Had to create an upstream issue about this error: https://github.com/Level/subleveldown/issues/109

CMCDragonkai commented 2 years ago

Managed to replicate the error.

import DB from './src/DB';

async function main () {
  const db = await DB.createDB({
    dbPath: './tmp/db',
  });
  const mgrDomain = ['INodeManager'];
  const dataDomain = [mgrDomain[0], 'data'];
  const mgrDb = await db.level(mgrDomain[0]);
  const dataDb = await db.level(dataDomain[1], mgrDb);
  await db.put(['INodeManager', 'inodes', '123'], 'a', 'b');
  await db.stop();
  await db.start();
  await db.level('123', dataDb);
  console.log(await db.get(['INodeManager', 'inodes', '123'], 'a'));
  await db.stop();
}

main();

Upon calling await db.level('123', dataDb); we get this error:

/home/cmcdragonkai/Projects/js-db/node_modules/subleveldown/leveldown.js:142
    if (self.leveldown.status !== 'open') return cb(new Error('Inner database is not open'))
                                                 ^
OpenError: Inner database is not open
    at /home/cmcdragonkai/Projects/js-db/node_modules/subleveldown/node_modules/levelup/lib/levelup.js:119:23
    at /home/cmcdragonkai/Projects/js-db/node_modules/deferred-leveldown/node_modules/abstract-leveldown/abstract-leveldown.js:38:14
    at /home/cmcdragonkai/Projects/js-db/node_modules/deferred-leveldown/deferred-leveldown.js:31:21
    at /home/cmcdragonkai/Projects/js-db/node_modules/encoding-down/node_modules/abstract-leveldown/abstract-leveldown.js:38:14
    at /home/cmcdragonkai/Projects/js-db/node_modules/subleveldown/node_modules/abstract-leveldown/abstract-leveldown.js:38:14
    at onopen (/home/cmcdragonkai/Projects/js-db/node_modules/subleveldown/leveldown.js:142:50)
    at processTicksAndRejections (internal/process/task_queues.js:77:11)
CMCDragonkai commented 2 years ago

I believe this would known as an uncaught rejection. Because this is not actually received by the catch handler of the level call.

Furthermore I realised that I don't have the types for levelup, level, subleveldown and abstract-leveldown. So I have installed them now:

npm install --save-dev @types/subleveldown
npm install --save-dev @types/abstract-leveldown
npm install --save-dev @types/levelup
npm install --save-dev @types/level

Now at least I can see what kind of event types we can have. Maybe help get access to the open error.

CMCDragonkai commented 2 years ago

Through some sleuthing I found a way to actually acquire the error. It's quite sneaky.

So basically subleveldown ends up "constructing" the AbstractLevelDOWN. This is done in: https://github.com/Level/subleveldown/blob/f132be81a01fd1260f3bc4e8801185707154a4a7/leveldown.js#L141

During this construction, it will call the abstract level down constructor. Which is actually not in abstract-leveldown, nor in leveldown, but in levelup.

This ends up calling this.open(). https://github.com/Level/levelup/blob/46d6e183d1d93404417c107f68698654ba6aca52/lib/levelup.js#L55-L57

Notice that the callback passed into that is either the callback passed by the user at the beginning of constructing a levelup instance. Or it is given a default value of a callback that just does this.emit('error', err);.

This is the callback that is actually being called with the error object. And this would normally be the callback passed in by us when we create the leveldb object. In our start method we have level() and we do have a callback there.

So it turns out that subleveldown actually calls the open call for the inner db.

Now because the error is emitted on the constructed sublevel instance, that instance is what we need to watch for errors, not the root leveldb instance.

        dbLevelNew.on('error', (e) => {
          console.log('GOT AN ERRROR!!!!!', e);
        });
CMCDragonkai commented 2 years ago

Upstream should indicate that error is a legitimate event for the leveldb instances.

CMCDragonkai commented 2 years ago

The fix for the open problem is that the DB levels must be recreated upon start.

This means INodeManager needs to be CDSS. Not CD pattern.

This is good opportunity to just integrate async-init into this as well.

CMCDragonkai commented 2 years ago

The js-db has received a fix for being able to handle and report the error as part of promise rejection: https://github.com/MatrixAI/js-db/pull/9

For this PR, I need to rework the INodeManager and EncryptedFS and then integrate async-init as well.

Will need to update to 1.1.5 of @matrixai/db.

CMCDragonkai commented 2 years ago

Based on the idea of recreating the sublevels, my thinking is that just doing:

      // INodeManager needs to be recreated
      // It contains sublevels that needs to be re-established
      this.iNodeMgr = await INodeManager.createINodeManager({
        db: this.db,
        logger: this.logger.getChild(INodeManager.name),
        fresh,
      });

In the EncryptedFS.start should be enough to fix the problem.

However it doesn't seem to work. I'm still getting the Inner database is not open error.

CMCDragonkai commented 2 years ago

If I recreate the sublevels right before I call await this.db.level, it ends up working again. But this should already work because the db domains were already recreated after restarting.

Furthermore I noticed that fileGetBlocks is called twice.


Ok I just realised why recreating INodeManager is not enough.

The problem is that this instance is also injected into FileDescriptorManager, and that is still using the old instance.

It appears that therefore, we need to represent INodeManager as a singleton. Therefore it should be StartStop pattern, and then we can just propagate the start stop instead.

CMCDragonkai commented 2 years ago

The INodeManager is now CDSS.

One major change is that during creation/start, it doesn't gc dangling inodes. This was too difficult to do when CDSS and ready decorators are applied, because it could not call those destroy methods if the instance isn't yet started. However, instead the stop method can explicitly destroy dangling inodes.

However 2 other tests failed:

    ✕ cwd still works if the current directory is deleted (74 ms)
    ✕ deleted current directory can still use . and .. for traversal (135 ms)
CMCDragonkai commented 2 years ago

Appears that the source the error comes from await efs.stop();.

It thinks that it's no longer running... need to investigate why.

CMCDragonkai commented 2 years ago

Ok it turns out the error is the same problem with trying to do GC operations in start as well.

Basically the current async-init will say that it's not ready if we are in the middle of stopping because it checking the lock now.

Therefore we do have to do some code duplication and create a separate gcAll() that does the garbage collection fully. And if we do this, we can then enable it for start too for INodeManager.

CMCDragonkai commented 2 years ago

Tests are passing now. Now finalising the linting.

CMCDragonkai commented 2 years ago

Ok it's all done. Time to merge.