Replace Status.waitFor polling logic with file watching events

[CMCDragonkai]

Specification

The current Status.waitFor uses unreliable polling which may miss transition events. Any usage of poll on FS changes can result in this. this creates some flaky tests like status testing where we want to test for when the agent is in STOPPING status.

File watching interface would be more robust and would be a reactive interface, where the OS notifies nodejs about a file change. This should improve our test robustness, and in some cases improve our speeds where poll is being used.

Additional context

• File watching is something we need to implement for EFS to improve our diff understanding - Structured Change Detection for Mutation Events and Schema Compliance js-encryptedfs#55
• File watching interface #171 - Also discussed here originally as an interface to managing secrets

Tasks

1. Research the different OS file watching interfaces
2. Use fs.watch or chokidar https://github.com/paulmillr/chokidar, the fs.watch may be more simpler for this particular usecase, and better than bringing potentially another buggy library, and one that may not work on Android or iOS
3. Replace our testing with poll with fs.watch
4. Update our status tests to be capable of properly testing STOPPING

[CMCDragonkai]

This issue results in intermittent test errors that can be made more reliable similar to #290.

[CMCDragonkai]

This is still causing problems, and while we would like to have synchronised readStatus and writeStatus, bringing in a file watching mechanism would be faster as this can be done without creating our own native dependency #290.

[CMCDragonkai]

Ok while trying out chokidar I discovered some interesting behaviour.

async function main () {
  // Set to persistent: false in js-polykey
  const watcher = chokidar.watch('./tmp/random');
  watcher.once('change', async (path, stats) => {
    console.log(
      path,
      stats,
      'content', (await fs.promises.readFile('./tmp/random', 'utf-8')).trim(),
    );
    await watcher.close();
  });
}

Now if I write to ./tmp/random in quick succession, like echo 'FIRST' > random ; echo 'LAST' > random.

The chokidar colasces this down to 1 change event, and if you try to read the file, you'll only get LAST.

The chokidar also exposes raw events, which shows 2 change events, but it's kind of useless because when you try to read the file, you end up the last value of LAST anyway.

Basically there's no good way to ensure you always get all the status change events in sequence. Basically what I was hoping for was "exactly-once" semantics. But this is not possible with file watching. Even last -F fails to capture all events.

The thing is, I can know that the file was written twice with raw events. But I cannot guarantee that when I try to read the file on those events, that I would have captured the file at that state. The other process's writes might just be too fast.

It seems exactly-once semantics would have to use a different mechanism, something like pipes (but it would need to be non-blocking). A unix domain socket could work, where the process that locks the status file also creates a unix domain socket. Then other clients can connect to the unix domain socket, and listen for events. Then as status events would be written to the socket. But this is alot of infrastructure of this feature which is only used during testing.

Maybe there's a way to design this differently so we don't need exactly-once semantics.

[CMCDragonkai]

Furthermore inside the docs for chokidar there's no guarantee that the event fires only when the write has finished. The option for awaitWriteFinish is based on timing. So even if we try to run a read operation, it could still result in the JSON parse failure if the JSON hasn't finished writing. So read/write synchronisation is still going to be a problem until we can have read/write locks as per #290.

[CMCDragonkai]

I think a solution can be this:

1. Use 2 files status.json and status.lock
2. The status.lock is used as just a lock file to be locked when a Polykey agent wants to assert control over the node path, using fd-lock we can do this just with a flock mutex
3. The status.json is also being locked, ideally with a read write IPC lock, but a flock mutex is fine to synchronise reads and writes - this is what prevents inconsistent writes/reads, the readStatus and writeStatus behaves similar to readToken and writeToken in src/sessions/Session.ts
4. We can still use chokidar now to listen for change events instead of polling, and then trigger a readStatus which locks and therefore synchronises the read.

[CMCDragonkai]

Ok the locking solution appears to be working, and I've had to rewrite the status tests.

@tegefaulkes I believe you wrote this earlier, and it still hasn't been changed over to unit-test style where the unit under test is meant to be under the test functions.

I've already slowly changed over.

Also remember you're not supposed to use full stops after your comments too, and test descriptions should be fully lowercase.

Can you make sure this is the case for all subsequent code you're writing and code that you're refactoring.

[CMCDragonkai]

I've tried using chokidar. It's actually quite flaky. It's not the most robust system. It loses events because I think it's trying to coalesce events together. It's not designed for usecases where file changes may be happening rapidly. For example a bug here: paulmillr/chokidar#1188

The only robust way is to rely on raw events. But if I do this, then perhaps chokidar shouldn't even be used here at all as it just can use whatever NodeJS supplies.

[CMCDragonkai]

I've removed chokidar since it wasn't working. Replaced it with native fs.watch. It requires that the statusPath already exists. So this is just created as an empty file if it doesn't exist in waitFor.

The last test for Status would be when we have quick transitions, and seeing how our waitFor does. I suspect that because exactly-once semantics is not done, then it will miss those transitions, so waitFor may need to be expanded with alternatives, so you for example wait for STOPPING or DEAD. But at the same time we should no longer have JSON parse errors due to the lock synchronisation.

[CMCDragonkai]

Ok I've just tested this, and the likelihood of missing an event is much reduced now. It can still happen, however for our testing, it's going to be very unlikely.

This is because beginStop and finishStart are now ready decorated blocking methods. This means along with the file locking that is occurring, these methods are also blocking the stop calls as well in the async-init.

This gives plenty of time for the waitFor event to trigger and make it attempt a readStatus which then intercepts with a locking acquisition.

It can still happen, but it's much rarer now.

[CMCDragonkai]

Also my tests show that it is still possible to have the skipped event like this:

  test.only('wait for has at-least-once semantics', async () => {
    const status = new Status({
      statusPath: path.join(dataDir, config.defaults.statusBase),
      statusLockPath: path.join(dataDir, config.defaults.statusLockBase),
      fs: fs,
      logger: logger,
    });
    await status.start({ pid: 0 });
    const statusWaitFor = status.waitFor('LIVE');

    const p1 = status.finishStart({
      clientHost: '' as Host,
      clientPort: 0 as Port,
      ingressHost: '127.0.0.1' as Host,
      ingressPort: 0 as Port,
      nodeId: '' as NodeId,
      pid: 0,
    });
    const p2 = status.beginStop({ pid: 1 });

    console.log('PRIOR');
    const r = await statusWaitFor;
    console.log(r);

    await p1;
    await p2;

    await status.stop({ });
  });

If the await is done for finishStart and beginStop, it still works though.

[CMCDragonkai]

Ok I swapped back to using polling. For some reason, the fs.watch just misses events sometimes. This causes a hang up, and is also less flexible since it requires that the statusPath exist already.

The polling although less efficient, is alot more reliable. And with the locking applied, we won't have problems with parse errors anymore, but at-least-once semantics is still an issue. But this would be an issue with fs events anyway.

The implementation for fs.watch is here for posterity:

Details

  public async waitFor(
    status: StatusInfo['status'],
    timeout?: number,
  ): Promise<StatusInfo> {
    // The status path must exist before we can watch
    // Thus we create an empty file if it doesn't exist
    const statusFile = await this.fs.promises.open(
      this.statusPath,
      this.fs.constants.O_CREAT
    );
    await statusFile.close();
    const watcher = this.fs.watch(this.statusPath, { persistent: false })
    const {
      p: transitionP,
      resolveP: resolveTransitionP,
      rejectP: rejectTransitionP
    } = promise<StatusInfo>();
    let timer;
    if (timeout != null) {
      timer = setTimeout(() => {
        rejectTransitionP(new statusErrors.ErrorStatusTimeout());
      }, timeout);
    }
    const handleChange = async () => {
      this.logger.warn('----------CHANGE EVENT---------');
      let statusInfo;
      try {
        statusInfo = await this.readStatus();
      } catch (e) {
        rejectTransitionP(e);
        return;
      }
      // DEAD status is a special case
      // it is acceptable for the status file to not exist
      if (
        status === 'DEAD' &&
        (statusInfo == null || statusInfo.status === 'DEAD')
      ) {
        resolveTransitionP(statusInfo ?? {
          status: 'DEAD',
          data: {},
        });
        return;
      }
      if (statusInfo?.status === status) {
        resolveTransitionP(statusInfo);
        return;
      }
    };
    watcher.on('change', handleChange);
    try {
      return await transitionP;
    } finally {
      clearTimeout(timer);
      watcher.off('change', handleChange);
      watcher.close();
    }
  }

[CMCDragonkai]

So this is technically a wontfix. However we did manage to solve a bunch of other status issues by introducing file locking. Which can still benefit once rw locking is possible at IPC level.