MatrixAI / js-encryptedfs

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

Update project structure to align with node v18 and new async-locks #80

Closed CMCDragonkai closed 1 year ago

CMCDragonkai commented 1 year ago

Description

This is following up with the updates with respect to node v18 and macos updates and npm root over npm bin.

Issues Fixed

Tasks

Final checklist

CMCDragonkai commented 1 year ago

We have a lot of test failures mostly related to concurrency, probably due to the new async-locks. Need to see if it due to similar problems as js-db.

ghost commented 1 year ago
👇 Click on the image for a new way to code review #### [![Review these changes using an interactive CodeSee Map](https://s3.us-east-2.amazonaws.com/maps.codesee.io/images/github/MatrixAI/js-encryptedfs/80/8c571312/b0ad9466b199abac9eba18dff8dff4a809cdae95.svg)](https://app.codesee.io/r/reviews?pr=80&src=https%3A%2F%2Fgithub.com%2FMatrixAI%2Fjs-encryptedfs) #### Legend CodeSee Map legend
CMCDragonkai commented 1 year ago

Ok one of the big things is because we no longer support ToString in async-locks, this propagated to js-db which then propagates to js-encryptedfs.

This is actually a breaking change unfortunately. But we released it as 5.2.0. I think I just need to update all the usages of inode index and convert them to strings using toString().

CMCDragonkai commented 1 year ago

Converting all the inode indexes to strings solved a bunch of concurrency issues.

However there's another problem now... it seems to do with some encoding. Could it be the node v18 issue?

CMCDragonkai commented 1 year ago

There seems to be some problem with the database persistence. I wrote a file called testfile, and I can read it again.

However if I stop EFS then start it again, I should be able to read the same thing again.

However I seem to get ErrorDBParseValue: Unexpected token e in JSON at position 0 which indicates it couldn't deserialise from JSON.

This is pretty strange, since the new DB shouldn't have this problem... but we have to see.

CMCDragonkai commented 1 year ago

This error is definitely due to the upgrade in @matrixai/db. Changing to node v18 did not cause any problems, neither did any other of the dependencies.

CMCDragonkai commented 1 year ago

Oh this error was due to 5.1.0 not 5.2.0. https://github.com/MatrixAI/js-db/commit/a7053cb41097a8bea87749b1b10678780ebe1755

We must repass back the same crypto object in when restarting the db.

CMCDragonkai commented 1 year ago

This is problem for EFS because when we do createEncryptedFS, we don't actually end up with the same crypto object.

When we stop the EFS, we stop the DB, the DB must then must be started again with the same crypto object.

We will need to keep track of the crypto object between restarts of the EFS.

CMCDragonkai commented 1 year ago

Another thing, is that the DB gets started/stopped even if the DB object was passed into the EFS from the outside, in that sense the EFS takes over the lifecycle of a passed in DB.

This is not quite correct. We need to only manage the lifecycle of the DB if the DB was created by EFS.

If the EFS did not create the DB, the DB's lifecycle should be independent.

@tegefaulkes

CMCDragonkai commented 1 year ago

Ok changing that management of the DB instance now means that in PK's side, when giving a DB to EFS, we must then consider that the DB's own lifecycle must be managed outside of the EFS. This is a breaking change @tegefaulkes.

One thing I note is the complication of having to start the DB just to destroy things. This actually doesn't occur here in EFS because if it was our own encapsulated instance, the DB can just be destroyed after stopping and would clear all relevant state. Otherwise we just clear the inode manager state.

However in PK, we end up restarting the DB only to delete things. I wonder if some of that can be simplified if you can just destroy the DB and have expect all the state to be cleared. The only thing would be domains that leave state outside the DB... and may require the DB to do something in order to destroy. Unless we encapsulate all state within our local Polykey node path and just destroy that and it should be fine... but that does break the composition abstraction.

It just appears that the idea that stopping then destroying shouldn't actually stop the DB since destroying domains actually depend on the DB instance running. Still not sure how to elegantly express this.

CMCDragonkai commented 1 year ago

Ok that fixed everything but now broke some of the chroot tests.

    ✕ should be able to access inodes inside chroot (145 ms)
    ✕ should not be able to access inodes outside chroot (176 ms)
    ✕ should not be able to access inodes outside chroot using symlink (174 ms)
    ✕ prevents users from changing current directory above the chroot (150 ms)
    ✓ can sustain a current directory inside a chroot (162 ms)
    ✕ can chroot, and then chroot again (125 ms)
    ✕ chroot returns a running efs instance (97 ms)
    ✕ chroot start & stop does not affect other efs instances (162 ms)
    ✕ root efs instance stops all chrooted instances (166 ms)
    ✓ destroying chroot is a noop (129 ms)

I think that may have relied on the DB lifecycle.

CMCDragonkai commented 1 year ago

The solution involved a weird trick from https://stackoverflow.com/a/64638986/582917.

The constructor parameter type didn't work. But the type assertion did:

      const efsChrooted = new (this.constructor as new (...params: ConstructorParameters<typeof EncryptedFS>) => this)({
        db: this.db,
        iNodeMgr: this.iNodeMgr,
        fdMgr: this.fdMgr,
        rootIno: target,
        blockSize: this.blockSize,
        umask: this.umask,
        logger: this.logger,
        chroots: this.chroots,
        chrootParent: this,
      });

This is partly due to the experimental decorator situation in swc. Remember we had to use new this in our static creator, well our chroot method also constructs itself, and instead of using new EncryptedFS it had to refer to its own decorated self being new this.constructor().

In this case it also required a type assertion on the actual constructor function type.

I remember TSC didn't have this problem, but our tests now use SWC so this becomes an issue.

CMCDragonkai commented 1 year ago

Tasks 2. and 3. are going to staging.