Skip to content

PushCoordinator: remove the silent clock and zone defaults, so omitting them cannot compile #1787

Closed

Description

@ryanbr

#1786 stopped main being red, but it treated the symptom. Every construction in the test sources now passes an explicit clock — nothing stops the next one being written without one.

The hazard

PushCoordinator.kt:14-15:

private val today: () -> LocalDate = { LocalDate.now() },
private val zoneId: ZoneId = ZoneId.systemDefault(),

Both are silent defaults. Omitting them is invisible at the call site, compiles, and usually passes — until the calendar moves. That is exactly how the 1 Sep breakage happened: the journal fixtures were dated 2026-08-18, PushWindow.ending(today(), zoneId) spans today.minusDays(13), and the row sat inside the window until it didn't. CI was green at 23:21Z on 31 Aug and red at 00:13Z on 1 Sep with expected:<[journal]> but was:<[]>, with nothing in between touching push.

zoneId is arguably the worse of the two. LocalDate.now() at least fails everywhere at once; ZoneId.systemDefault() makes the result depend on the machine, so a test can pass in CI and fail on a developer's laptop for a reason neither can see. That is precisely what happened to me here — this box runs UTC+12, so it went red twelve hours before the runners did, which read as a local environment quirk at exactly the moment it was a real bug with a deadline.

The fix

Drop both defaults so omission is a compile error.

It is cheap. There is exactly one production caller, SelfHostedPushWorker.kt:221, which currently relies on both and would pass them explicitly:

val run = PushCoordinator(
    source = dao,
    transport = transport,
    progress = progress,
    sourceId = sourceId,
    today = { LocalDate.now() },
    zoneId = ZoneId.systemDefault(),
    destinationStillCurrent = { settings.enabledEndpoint() == endpoint },
)

That is not more verbose for its own sake: it puts the two facts that decide the push window at the one site that genuinely wants the real clock, instead of leaving them implicit in 24 places that do not.

The tests need no change — PushCoordinatorTest (19) and PushCursorTest (5) already pass pinnedToday and ZoneId.of("UTC") after #1786.

Why this was not folded into #1786

main was red, and a production signature change is the wrong thing to attach to an urgent test-only fix. Doing it separately also means the compile errors, if any turn up outside these two files, land on their own commit where they are readable.

Worth checking while in here

PushCoordinator is currently the only production class in the app with an injectable clock defaulting to now() — I swept for the pattern during #1786 and found no others, and no other test pairs a real-clock read with a fixed 2026-08/2026-09 fixture. If a second one appears later, the same argument applies to it.


Correction to the sweep above

The claim that PushCoordinator is "the only production class in the app with an injectable clock defaulting to now()" is wrong. Two others have the identical shape:

  • OuraDriver.kt:94 — private val nowMsProvider: () -> Long = { System.currentTimeMillis() }
  • LiveSessionRunner.kt:51 — private val nowEpochSec: () -> Long = { System.currentTimeMillis() / 1000L }

My sweep regex matched the parameter name against an alternation of today|now|clock|nowMs|nowSec followed immediately by :, so nowMsProvider: and nowEpochSec: never matched. A name-based grep found what it was told to look for and reported the absence of everything else as a fact — which is the same failure mode as verifying #1786 by grep.

The conclusion still holds: neither is a live time bomb, for reasons that are worth writing down rather than assuming.

LiveSessionRunner has zero constructions in the test sources, so there is nothing to omit the clock.

OuraDriver has 45, and only 2 pass nowMsProvider — but its gate points the other way. plausibleSampleSeconds rejects seconds > nowSeconds + SAMPLE_FUTURE_TOLERANCE_SECONDS, a future bound, so a fixed past-dated fixture gets safer as the clock advances, the opposite drift from PushCoordinator's trailing 14-day window. The two tests that genuinely depend on "now" — sampleConvertingToFutureIsRejected and anchorAdoptionStillUsesFullWindowIndependentOfNow — are exactly the two that pin it. That was done correctly by whoever wrote them.

So this issue's scope is unchanged, but the justification is narrower than stated: PushCoordinator is not the only class of this shape, it is the only one whose window drifts in the direction that turns a passing test into a failing one.

Activity

  1. kvnloo commented on Sep 1, 2026

    @kvnloo

    Landed in #1788 (82f8cb9c, "push: make an omitted clock a compile error, not a green test").

    On current main, PushCoordinator has required today / zoneId (no defaults), and SelfHostedPushWorker passes them explicitly with the #1787 note. Safe to close.

  2. ryanbr commented on Sep 2, 2026

    @ryanbr
    OwnerAuthor

    Landed in #1788 (82f8cb9c8). Verified against main rather than taken on report:
    PushCoordinator.kt:14-15 carries today: () -> LocalDate and zoneId: ZoneId with no
    defaults, and SelfHostedPushWorker.kt:228-231 — still the single production caller — passes
    both explicitly. Omitting either is now a compile error, which was the ask.

    Recording the sweep, so the next person does not redo the wrong one

    This issue's sweep failed twice, and the second failure is the interesting one.

    The original matched parameter names (today|now|clock|nowMs|nowSec followed by :), which
    is why nowMsProvider and nowEpochSec were missed. The correction above found those two — but
    only because they happen to share the () -> Long provider shape. It did not widen the method.

    Sweeping on the default expression instead of the parameter name:

    grep -rnE "^[[:space:]]*(private[[:space:]]+)?(val[[:space:]]+|var[[:space:]]+)?[A-Za-z_][A-Za-z0-9_]*:[[:space:]]*[A-Za-z0-9_<>, ()?.-]*[[:space:]]*=[[:space:]]*\{?[[:space:]]*(LocalDate|LocalDateTime|LocalTime|Instant|ZonedDateTime)\.now\(\)" \
      android/app/src/main/java --include=*.kt
    # plus the same for System.currentTimeMillis / ZoneId.systemDefault / Clock.system
    

    finds 21 production parameter defaults that read the wall clock, not 3. The type annotation
    before the = is what separates a parameter default from an ordinary local val now = ....

    The one candidate worth chasing, and why it is not a second bomb

    A defaulted clock is not itself the hazard. The hazard is the triple: a defaulted clock, and
    a test that omits it, and a fixed-date fixture measured against a trailing window — so
    the fixture drifts out of range as the calendar advances. (A future bound, as correctly noted
    above for OuraDriver, gets safer over time.)

    IntelligenceEngine.analyzeRecent is the only one of the 21 that has all three on paper:
    nowSeconds: Long = System.currentTimeMillis() / 1000L at :325 and :494, maxDays: Int = 21,
    and four test files that call it with fixed dates and reference nowSeconds zero times.

    It is not a bomb. The fixtures are already far outside the window and green:

    test fixture dates
    IntelligenceWatchRecoveryTest 2026-06-10
    IntelligenceDayOwnerLineTest 2026-06-10, 2026-06-12
    IntelligenceDaySourceTokenTest 2026-06-10, 2026-06-12
    RegistryDayOwnerSourceTest 2026-06-15

    Against a 2026-08-12 cutoff on 2026-09-02, all four are ~11 weeks past it and still pass, so
    their assertions do not depend on the window. They crossed that boundary months ago without
    breaking — the opposite of PushCoordinator, whose fixtures sat just inside a 14-day window and
    fell out overnight on 1 Sep.

    So the conclusion here holds, but on narrower ground than the text above claims: not "the only
    class of this shape", and not "the only one whose window drifts" — simply the only one where a
    test was actually positioned to fall off the edge. No second live instance found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions