MatrixAI / js-encryptedfs

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

ci: merge staging to master #71

Closed MatrixAI-Bot closed 1 year ago

MatrixAI-Bot commented 1 year ago

This is an automatic PR generated by the pipeline CI/CD. This will be automatically fast-forward merged if successful.

MatrixAI-Bot commented 1 year ago

Pipeline Attempt on 598993155 for 01f44a18c30fe2b15252c25a41929dbc0d6278cc

https://gitlab.com/MatrixAI/open-source/js-encryptedfs/-/pipelines/598993155

ghost commented 1 year ago
👇 Click on the image for a new way to code review - Make big changes easier — review code in small groups of related files - Know where to start — see the whole change at a glance - Take a code tour — explore the change with an interactive tour - Make comments and review — all fully sync’ed with github [Try it now!](https://app.codesee.io/r/reviews?pr=71&src=https%3A%2F%2Fgithub.com%2FMatrixAI%2Fjs-encryptedfs)

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

MatrixAI-Bot commented 1 year ago

Pipeline Attempt on 599061810 for ad2f42e5c707b8faad6ef487f138a3e60d48378f

https://gitlab.com/MatrixAI/open-source/js-encryptedfs/-/pipelines/599061810

CMCDragonkai commented 1 year ago

Ok there's another concurrency non-determinism. This time caused by a slower write while there is a write stream. It's quite confusing. But this is my theory.

We have a write stream that writes 10 chunks of AAAAA. Therefore if it was writing by itself, we would see something like:

AAAAA AAAAA AAAAA AAAAA ... etc

Now we also have an atomic call to a duplicated FD but that which starts at position 0. It writes 25 characters of B.

The test currently expects the 25 characters to be written atomically. Therefore the test thinks the end result is either one of these:

AAAAA AAAAA ... etc
OR
B*25 A*25

But the problem with this expectation is that, the former is true if the B write occurred first, and then A writes started. The latter can occur when A writes complete, then B write starts.

But if B write starts in the middle of A writes. Then we would get a mixed output.

This is because:

  1. Let's suppose that after the first A chunk, the B writes were written. We would have B*25
  2. On the second A chunk, it is still writing from the first chunks ending position.
  3. This results in BBBBB as the first 5 bytes, but subsequent bytes will all be A.

Hence is why we have:

    Expected: "BBBBBBBBBBBBBBBBBBBBBBBBBAAAAAAAAAAAAAAAAAAAAAAAAA"
    Received: "BBBBBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"

Now this could also change if the B writes occurs after the 2nd chunk. In that case we should use B*10 and then all As.

The reason why this is quite non-deterministic is because there are 2 levels of asynchronicity here. The ones involving await, but also lazy drainage events, where just because we do writeStream.write(dataA), there's no guarantee that the writes have written to the disk. Of course in our in-memory FS, we could sequence it properly with a callback to wait for when the chunk is in fact written.

But the end result is that the B writes can occur at any point in time between the atomic A writes. This is the same problem as the previous problem, but it wasn't as clear, because B writes starting from position 0, whereas A writes are in sequence.

CMCDragonkai commented 1 year ago

@tegefaulkes

CMCDragonkai commented 1 year ago

So the solution could be to strictly control the concurrency, but that doesn't allow our test to be robust. A robust test here should instead have a predicate that allows any of the possible permutations of the output. That being of all As, or any number of chunks of B based on the chunk size of A, up to 5 chunks (the length of B divided by the chunk size of A).

CMCDragonkai commented 1 year ago

This has been resolved with a more robust regex check. I suspect more of these bugs are lurking in the tests.

MatrixAI-Bot commented 1 year ago

Pipeline Attempt on 599151439 for a70d4c4876c01dae50ba76259b1099f5204d7edd

https://gitlab.com/MatrixAI/open-source/js-encryptedfs/-/pipelines/599151439

MatrixAI-Bot commented 1 year ago

Pipeline Attempt on 599152359 for 92efef0544e625312a30abc3df0ff7932e649e8a

https://gitlab.com/MatrixAI/open-source/js-encryptedfs/-/pipelines/599152359

MatrixAI-Bot commented 1 year ago

Pipeline Succeeded on 599152359 for 92efef0544e625312a30abc3df0ff7932e649e8a

https://gitlab.com/MatrixAI/open-source/js-encryptedfs/-/pipelines/599152359