Copilot commented on code in PR #7845:
URL: https://github.com/apache/texera/pull/7845#discussion_r3836083646
##########
common/auth/src/test/scala/org/apache/texera/auth/UserActivityTrackerSpec.scala:
##########
@@ -373,7 +495,171 @@ class UserActivityTrackerSpec extends AnyFlatSpec with
Matchers {
tracker.cooldownSize shouldBe 1
}
+ it should "let a fatal error from the clock escape markActive" in {
+ val recorder = new Recorder
+ val t0 = Instant.parse("2026-01-01T00:00:00Z")
+ // The catch is `NonFatal`, not `Throwable`: a transient clock/DB hiccup is
+ // swallowed (cases above), but an InterruptedException or an OOM has to
reach
+ // the caller instead of being logged and dropped as if it were transient.
+ //
+ // The first clock read succeeds on purpose. `clock()` is the first
statement in
+ // markActive's try, so after a throw on the very first call every count
is zero no
+ // matter what the rest of the method does; letting one call through first
makes the
+ // trailing assertions observations about state the tracker really built.
+ val reads = new AtomicInteger()
+ val tracker =
+ new UserActivityTracker(
+ Duration.ofMinutes(5),
+ recorder.upsert,
+ sameThread,
+ () =>
+ if (reads.incrementAndGet() == 1) t0
+ else throw new InterruptedException("fatal clock")
+ )
+
+ tracker.markActive(5150)
+ recorder.calls.size shouldBe 1
+ tracker.cooldownSize shouldBe 1
+
+ an[InterruptedException] should be thrownBy tracker.markActive(5150)
+
+ // the escape left the first call's write and its cooldown claim exactly
as they were
+ recorder.calls.size shouldBe 1
+ tracker.cooldownSize shouldBe 1
+ }
+
+ it should "let a fatal error from upsertFn escape markActive" in {
+ val t0 = Instant.parse("2026-01-01T00:00:00Z")
+ val clock = new AtomicReference[Instant](t0)
+ val fatal: (Integer, Instant) => Unit =
+ (_, _) => throw new InterruptedException("fatal upsert")
+ val tracker =
+ new UserActivityTracker(Duration.ofMinutes(5), fatal, sameThread, () =>
clock.get())
+
+ // The same NonFatal-not-Throwable contract at the third catch site, the
wrapper
+ // around upsertFn: on this synchronous executor a fatal from the write
passes
+ // through both catches and out to the caller.
+ an[InterruptedException] should be thrownBy tracker.markActive(4242)
+
+ // the slot was claimed before the write was dispatched
+ tracker.cooldownSize shouldBe 1
+ }
+
+ it should "let a fatal error from the clock escape evictStale" in {
+ val recorder = new Recorder
+ val tracker =
+ new UserActivityTracker(
+ Duration.ofMinutes(5),
+ recorder.upsert,
+ sameThread,
+ () => throw new InterruptedException("fatal clock")
+ )
+
+ an[InterruptedException] should be thrownBy tracker.evictStale()
+ }
+
"UserActivityTracker singleton" should "treat a null uid as a no-op without
touching the DB" in {
noException should be thrownBy UserActivityTracker.markActive(null)
}
+
+ it should "insert a last-active row for a uid that has none" in {
+ // No row at all, which is what makes this the INSERT arm rather than the
DO UPDATE
+ // arm. `readLastActive` alone would not establish that: last_active_time
is
+ // nullable, so a row carrying a NULL also reads back as None.
+ activityRowCount(insertUid) shouldBe 0
+
+ val beforeCall = Instant.now()
+ UserActivityTracker.markActive(insertUid)
+ // Taken here, not after the `eventually` below: the accepted window is
then the
+ // duration of the markActive call itself rather than however long the
poll took,
+ // which is what binds the stored value to the instant handed to the
writer.
+ val afterCall = Instant.now()
+
+ // The singleton dispatches the upsert onto its own writer thread, so the
row
+ // appears asynchronously.
+ val stored = eventually {
+ readLastActive(insertUid).getOrElse(fail(s"no activity row was written
for $insertUid"))
+ }
+
+ // The stamped instant is the claim time, taken synchronously inside
markActive, so
+ // it lands in [beforeCall, afterCall]. The millisecond of slack absorbs
Postgres'
+ // microsecond rounding, nothing more; a timestamp taken at write time
instead
+ // lands after this window, because opening a DSLContext and
round-tripping the
+ // insert is never a sub-millisecond affair.
+ withClue(s"stored=$stored window=[$beforeCall, $afterCall]: ") {
+ stored.isBefore(beforeCall.minusMillis(1)) shouldBe false
+ stored.isAfter(afterCall.plusMillis(1)) shouldBe false
+ }
+ }
+
+ it should "stamp a queued write with its claim time rather than the time it
runs" in {
+ // A tight wall-clock window around markActive is not enough to separate
claim time
+ // from write time: the writer thread normally picks the task up within a
+ // millisecond, so both land inside any window wide enough to be stable.
The
+ // difference only becomes visible when the write is made to WAIT, which
is also the
+ // case that matters -- under a DB stall, tasks sit in the bounded writer
queue, and
+ // what has to be recorded is when the user was seen, not when the row
finally
+ // landed.
+ //
+ // The stall is manufactured with an EXCLUSIVE table lock held on a raw
connection
+ // (outside the pool the writer borrows from): it blocks blockerUid's
insert, and the
+ // single-threaded writer therefore cannot even start delayedUid's task
until the
+ // lock is released.
+ val lockConn = newRawConnection()
+ val beforeCall = Instant.now()
+ val afterCall =
+ try {
+ lockConn.setAutoCommit(false)
+ val stmt = lockConn.createStatement()
+ try stmt.execute("LOCK TABLE texera_db.user_last_active_time IN
EXCLUSIVE MODE")
+ finally stmt.close()
+
+ UserActivityTracker.markActive(blockerUid)
+ // give the writer thread time to start the insert and block on the
lock
+ Thread.sleep(500)
+ UserActivityTracker.markActive(delayedUid)
+ val callReturned = Instant.now()
Review Comment:
This test adds a fixed 2s of `Thread.sleep` (500ms + 1500ms) to the suite.
The initial 500ms sleep appears unnecessary for the asserted property (the
table lock held for 1.5s already guarantees the write cannot complete until
well after `afterCall`). Dropping the 500ms sleep (or replacing it with a
condition-based wait) would reduce runtime and avoid timing-based flakiness.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
