Use wall-time for credential helper invalidation

[illicitonion]

Previously we were using time intervals which excluded time spent with the system sleeping, which is not appropriate for expiring tokens which expire based on wall-time duration.

[illicitonion]

/cc @tjgq

[tjgq]

I'm not sure I understand the problem this PR is trying to solve.

You're proposing to switch from System.nanoTime (the default according to the Caffeine documentaion) to System.currentTimeMillis. My understanding is that the former reads a monotonic clock while the latter reads the system clock. I'd argue that the system clock is not appropriate in this situation, because it would include corrections (e.g., if the system clock is set back by an hour while a cache entry is alive, its lifetime would be extended by an hour).

I'm also skeptical of the claim that System.nanoTime remains stationary while the process is asleep. On my system, this program prints values that are 10 seconds apart:

public class Clock {
  public static void main(String[] args) {
    System.out.println(System.nanoTime() / 1_000_000_000);
    try {
      Thread.sleep(10_000);
    } catch (InterruptedException e) {}
    System.out.println(System.nanoTime() / 1_000_000_000);
  }
}

Is this in relation to a particular implementation of the JVM? Do I misunderstand the Java time APIs or what Caffeine is doing?

[illicitonion]

By "asleep" I mean "if you close your laptop and it suspends". See https://blogs.sap.com/2023/02/10/jfr-timestamps-and-system.nanotime/ for more details - System.nanoTime ends up calling mach_absolute_time which is documented: "this clock does not increment while the system is asleep".

I have observed that if I get a credential helper token while configuring Bazel to use with an 8 hour timeout, close my laptop over night, open it 16 hours later and do a build, Bazel attempts to re-use my stale token. This PR avoids that problem.

[tjgq]

Aha, thanks for the clarification! Unfortunately, I think that means neither System.nanoTime nor System.currentTimeMillis are appropriate for this purpose: the former will misbehave if the machine is suspended, while the latter will misbehave if a (large) correction is made to the system clock. But the former is much more likely than the latter, so I agree that this is moving things in the right direction.

[ben-manes]

I think you should file this as a JDK bug as it is surprising behavior! It sounds like they should have used mach_continuous_time on MacOS.

[illicitonion]

@bazel-io flag

[keertk]

@bazel-io fork 6.3.0

[ben-manes]

Submitted to bugs.java.com as internal id 9075217.

[ben-manes]

Oddly they removed the link to this issue and the target OS, instead stating that it works on Windows. Responded with a clarification on the OS and system methods.

https://bugs.java.com/bugdatabase/view_bug?bug_id=JDK-8307677