Redesign usage of CDSS with `VaultInternal`

[joshuakarp]

Specification

CDSS

Currently, VaultInternal utilizes the create-destroy pattern. We're considering instead refactoring this into a create-destroy-start-stop pattern. This would allow us to align function usage between VaultManager and VaultInternal as follows:

VaultManager	VaultInternal
createVault	createVaultInternal
openVault	start
closeVault	stop
destroyVault	destroy

The meat of the logic should now be in 2 static methods of VaultInternal, representing "smart asynchronous creators" of VaultInternal. These should be:

VaultInternal.createVaultInternal - creates a new vault from scratch, think `git init`
VaultInternal.cloneVaultInternal - clones a vault, think `git clone`

creating a VaultInternal instance with new or existing state should use VaultInternal.createVaultInternal. First time cloning of a vault should use VaultInternal.cloneVaultInternal. Get all the isogit related side-effects (client-side logic) into these static methods, the VaultManager would only manage any additional metadata.

The start method now has to setup internal state, specifically Git state, it needs to initialize the repository and also perform the initial commit, however it has to be idempotent, so if these actions are already done, it won't bother with it.

The stop cannot stop efs, because the EFS lifecycle is handled outside by VaultManager

The destroy should then destroy the Vault internal state, however additional destruction management would have to occur inside the VaultManager which might mean it has to construct (and thus load into-memory) the instance before it can destroy it.

The cloning and pulling code should work using withF and withG as used by NodeConnectionManager. The cloning and pulling will need to have access to the ACL domain to ensure that permissions are properly set. Because permission checking occurs inside VaultManager and not VaultInternal and occurs before VaultInternal functions are executed, this would imply that VaultManager has access to the ACL and is bound to the ACL. Which would mean the meat of logic discussed above should be in VaultManager.

Metadata

The VaultInternal now needs to maintain its own DB domain to store vault related metadata. 2 new properties are relevant:

1. dirty - this is a Boolean that determines whether the vault is in a dirty state or not
2. remote - if this contains a string nodeId:vaultId, then this vault should be considered a "remote" vault and must be immutable - see Investigate usage of the remote field for vault metadata optimisation #309 and No longer preserve vault ID on cloneVault #253

The internal domain is better than storing a POJO in the VaultManager. That logic should be removed in favor of VaultInternal having it's own internal domain, which is more efficient and more flexible.

Additional metadata along with the dirty Boolean can be added to enable us to fix #252. This will be important to ensure internal consistency of our vaults in the presence of program crashes and also inadvertent OS shutdowns or just program killing.

misc.

The implementations of the functions in VaultManager would internally call the respective function above in VaultInternal. This would also allow us to add any extra functionality that needs to be separate from the VaultInternal functions into the VaultManager. For example, on creation of a vault, there's a few factors that need to be considered:

• Locking: VaultInternal has no notion of the creation/destruction locks used in the ObjectMap structure, so this can be safely handled in VaultManager.createVault/destroyVault.

Conversely, we should remove any instances where we perform similar logic between the two corresponding functions, and ensure it's centralized in a single place.

Locking will be handled via two RWLock locks. One lock for the object map in VaultManager. The other lock for the VaultInternal instance. The VaultManager RWLock will handle the life cycle of the VaultInternal Instance. Write locking will be used for the creation/destroy lifecycle. read locking will be used with the WithVaults when accessing the vaults. The VaultInternal locking will be used for the respective readF/G and writeF/G functions.

Git repository: anything to do with git instantiation for the vault could also be centralised within VaultInternal.createVaultInternal

To ensure the integrity of the vaultName information we need to apply locking when reading and writing to the VaultName->VaultId mapping in the database. This is to ensure that when we check if a vaultName already exists in the mapping we can trust that this information is correct and avoid concurrency issues with vault creation and renaming.

Additional context

• As we discovered in Review and Refactor of git/utils.ts for Serving Vault Git Repositories #298, the usage of isogit is mostly client-side behavior. But server side behaviour is facilitated by VaultManager and functions found in src/git/utils.ts.
• original comment on this Introducing vault sharing and the restrictions of pulling and cloning with vault permissions #266 (comment) based on discussion in our review with @scottmmorris https://vimeo.com/manage/videos/652339635
• discussion of VaultInternal (from commit to end), as well as some discussion of CDSS with VaultInternal https://vimeo.com/manage/videos/652700328

A note on usage of the CDSS pattern:

• create...: performs the dependency side-effects (i.e. creates any encapsulated dependencies before creating itself). That is, we can see this function as outside the instance of the class
• start: performs the side-effects on the created instance (with the created instance coming from the constructor)

tasks

1. Convert VaultInternal to using the CDSS async-init pattern.
   1. create a createVaultInternal constructor for creating a VaultInternal instance on new or existing state.
   2. create a cloneVaultInternal constructor for first time cloning of a vault.
   3. all isogit related side effects and creating logic should be within these creation methods. Ideally VaultManager shouldn't touch isogit at all.
   4. start method handles the creation of any state. This should be idempotent. this also handles any db domain creation.
   5. destroy should destroy all VaultInternal state.
2. Cloning and pulling should use the with functions through the NodeConnectionManager.
3. permissions will need to be checked before cloning and pulling.
4. Move handling of metadata to inside of VaultInternal
   1. Implement a dirty field for tracking if the git repo is in a dirty state.
   2. Implement a remote containing string nodeId:vaultId for tracking remote node and vault. this is also used to check if the vault is immutable.
   3. Any other metadata needs to be moved here as well.
   4. Add ability to VaultManager to access the metadata for checking if a vault exists and checking/updating vaultName.
5. Update VaultManager and VaultInternal to use RWLock locks.
   • Use object map write locking for VaultInternal lifecycle.
   • Use object map readLocking for accessing VaultInternal contents.
   • Use VaultInternal write/read locking for the read/write functions.
   • VaultManager git handling functions should be using both read locks.
   • VaultInternal.pullVault should respect the write lock.
   • add locking for vaultName -> vaultId metadata.
6. Review and update tests for...
   • VaultManager object map life-cycle.
   • VaultManager object map locking.
   • VaultInternal life-cycle.
   • VaultInternal locking.
   • git clone/pull permissions testing.
   • concurrently creating vaults test.
   • test for VaultInternal.pullVault locking

[CMCDragonkai]

Looking into this now.

From what I see, VaultInternal should have at least the static method called createVaultInternal and the destroy method.

However because we have to differentiate destroying the in-memory instance on the on-disk state, then destroy and stop matters as well, this way when we stop the VaultManager, what's most important is that we stop on the VaultInternal. The VaultManager itself can have additional cleanup operations when this occurs.

So having both VaultManager and VaultInternal have CDSS just enables both sides to manage their side-effect/stateful responsibilities.

However there is one issue with the openVault call. One cannot open a vault that isn't already created, because you cannot start the instance. Therefore one might ask what the purpose of openVault might be. I suggest it would be similar to a sort of getter function, and but one that should throw an exception.

This also means the there is a public interface to vaults, and this is returned to the caller of VaultManager.

But as we have evolved the design of the vault manager, we also want vaults to be used within a with context, so that transactions can take place during mutation of the vaults. This is more controlled way of manipulating vaults.

[CMCDragonkai]

I've also moved in the new RWLock into #266. So that can be used for the vault map as well.

[CMCDragonkai]

Currently the fresh parameter is essential to VaultInternal unlike it being an optional property in other domains. It is what determines whether the VaultInternal is constructed with using git.init which would try to initialise the git state, or whether it picks up existing state internally.

The fresh was meant to say to start anew, and wipe out any existing state there.

Where to load existing state or not is meant to be automatic, which is to say that VaultInternal upon starting should be checking the EFS's state for the vault directory and git directory, and then just loading relevant things into memory, and only if these did not exist, would it then do the git.init.

The only issue is that during clone operations, which creates state, this would mean that have to "pre-seed" the git state, prior to running VaultInternal.createVaultInternal so that it is aware the state already exists. I think this is how it is currently working anyway with cloneVault.

Moving to CDSS, would also mean that the deliberation of fresh would go into the start instead of the create.

All of this means that isogit's functionality will be used in VaultManager and VaultInternal, whereas before I thought that git would be isolated to VaultInternal.

It is possible to move git.clone to also VaultInternal, by creating a second creation static method. The creation static methods are functions that outside the class instance, and this would be similar to how C++ an other languages have overloaded/multiple constructors. So cloning would just be a separate constructor and would be an asynchronous method too.

So we may have:

VaultInternal.createVaultInternal()
VaultInternal.cloneVaultInternal()

As 2 ways of constructing a vault internal, one is creating it from scratch, which equates it to git init, and another is cloning from elsewhere which equates it to git clone.

[CMCDragonkai]

This also means EFS state is considered a dependency for VaultInternal, so git.init and git.clone will be placed into createVaultInternal and clonseVaultInternal respectively, and therefore fresh is applied there in particular. Which probably means that fresh parameter won't exist in start at all.

[tegefaulkes]

I have updated the spec here. any discussion of this aspect of the PR is done here.

[tegefaulkes]

After moving the metadata inside of VaultInternal The only metadata the VaultManager would need is a mapping of VaultName -> VaultId. This is used by the methods

• destroyVault removes the mapping.
• listVaults lists all of the vaultName->vaultId mappings.
• renameVault Updates the mapping.
• getVaultId returns the VaultId from the mapping.
• getVaultMeta this would actually touch the VaultInternals metadata But It's only used externally once to get the vault name. So I think it can be safely removed. Or moved into VaultInternal.

So instead of accessing the VaultInternals metadata from the VaultManager, The VaultManager can just maintain a VaultName->VaultId mapping inside it's DB. we can store the VaultName inside of the VaultInternals metadata but I don't think it's really needed.

Alternatively If we feel that the accessing the VaultInternals Metadata directly is needed. We can replicate how the js-encryptedfs works by using the sublevel prefixer method. I will need to explore this some more and test it.

[CMCDragonkai]

Because each VaultInternal has its own DB level. This means it's a prefix.

If you want to know whether a "level" is filled our not. You can use DB.count.

This can return the number of fields in the level. You can use this to know whether a VaultInternal exists or not.

However the VM also needs to be able to look up VaultId by VaultName. Ideally this should be solved by #188 and MatrixAI/js-db#1. But for the mean time you would need a DB level that is VaultName -> VaultId.

[CMCDragonkai]

Make sure you're locking these operations though!