Code Review: PR #329 — Unified time testing with app_now()

The core idea is sound — a GUC-synced app_now() SQL function plus an IORef-backed Haskell clock is a clean approach. Several issues to address before merging.

Bug: monitors.check_triggered_query_monitors was already dropped in migration 0038

static/migrations/0094_app_now_function.sql lines 106–116

Migration 0038_cleanup_dead_background_jobs.sql explicitly drops this procedure. The CREATE OR REPLACE PROCEDURE in 0094 silently re-creates an orphaned, unreachable procedure (no add_job() scheduling it). On a fresh DB (0001 → 0094), 0038 drops it, then 0094 re-creates it as dead code. Remove it from this migration.

Bug: app_now() marked STABLE but reads a session-mutable GUC

static/migrations/0094_app_now_function.sql line 16

$$ LANGUAGE plpgsql STABLE;

STABLE lets PostgreSQL cache the result within a single statement. But app_now() reads current_setting('app.current_time', true), which can change mid-test (after advanceTime + syncConnectionTime). The planner may reuse the pre-advance value. Change to VOLATILE (the default) to prevent intra-statement caching.

Correctness: is_local=true in syncConnectionTime is transaction-scoped, not connection-scoped

src/Pkg/TestClock.hs lines 68–75

With is_local=true, the GUC resets at end of transaction. In autocommit mode (the default for postgresql-simple), each statement is its own transaction — meaning the GUC resets after every syncConnectionTime call itself, before your actual query runs. Consider is_local=false so the setting persists for the full connection session and is cleared on the next pool checkout, or document the requirement that all interactions be wrapped in an explicit transaction.

Incomplete feature: TestClock is never wired into TestUtils/TestResources

The PR description mentions replacing runFrozenTime with runMutableTime/runWithTimeSyncedPool — but TestUtils.hs still uses runFrozenTime throughout (lines 542, 832, 846, 1008, 1015, 1022, etc.) and System/Types.hs line 163 is unchanged. Pkg.TestClock is exported but entirely unused. The GUC sync will never be called in tests. This ships infrastructure without the integration.

Code quality: Redundant import Data.IORef

src/Pkg/TestClock.hs line 20

import Data.IORef (IORef, modifyIORef', newIORef, readIORef, writeIORef)

Relude already re-exports all of these. Remove the explicit import.

Code quality: Leak the IORef abstraction vs. keeping it internal

src/Pkg/TestClock.hs line 36

newtype TestClock = TestClock {unTestClock :: IORef UTCTime}

unTestClock is never used outside the module — all internal functions pattern-match. Either use positional style (newtype TestClock = TestClock (IORef UTCTime)) or don't export the accessor. As-is it leaks the IORef implementation detail.

Minor: Hardcoded +00 timezone suffix in syncConnectionTime

src/Pkg/TestClock.hs line 74

let timeStr = formatTime defaultTimeLocale "%F %T%Q+00" t

Using %z or leveraging iso8601DateFormat (Just "%H:%M:%S%Q") from Data.Time.Format.ISO8601 would be more idiomatic and self-documenting (PostgreSQL accepts either).

Minor: PR description references wrong migration number

The description says 0033_app_now_function.sql but the actual file is 0094_app_now_function.sql.

Priority order:

1. Remove re-created check_triggered_query_monitors (dead code, contradicts 0038)
2. app_now() → VOLATILE (correctness: planner caching)
3. Reconsider is_local=true (GUC resets per-statement in autocommit mode)
4. Wire TestClock into TestResources and replace runFrozenTime calls (feature is incomplete)
5. Drop redundant import Data.IORef
6. Hide or remove unTestClock

Code Review: PR 329 - Unified time testing with app_now()

OVERVIEW

This PR adds the foundational layer for controllable time in tests: a PostgreSQL app_now() function driven by a GUC, and a TestClock Haskell module. The approach is sound.

However, the diff is materially incomplete vs. the PR description. The description lists 15+ changed files (TestUtils wiring, TestResources.testClock, runWithTimeSyncedPool, query-function Time constraints, test updates) but the actual diff contains only 3 files. TestUtils.hs still calls runFrozenTime on lines 542/832/846 and has no TestClock integration. Until those follow-up changes land, the new module is dead code.

CORRECTNESS: set_config transaction scope

syncConnectionTime calls set_config with third argument true, making the GUC transaction-local. In autocommit mode (the default for postgresql-simple connections not inside an explicit BEGIN), each statement is its own transaction, so the GUC resets immediately after set_config returns, before the next query can read it. The docstring says resets when connection returns to the pool, which is misleading.

For the pattern to work, syncConnectionTime must be called within the same explicit transaction as the queries that need to see the test time. If the intended use is per-connection-checkout, consider false (session-scoped) instead, or document the transaction requirement explicitly. This needs resolving before wiring into the pool wrapper.

CODE SUCCINCTNESS (eta-reductions available)

Current:
advanceTime (TestClock ref) dt = modifyIORef' ref (addUTCTime dt)
getTestTime (TestClock ref) = readIORef ref
newTestClock t = TestClock <$> newIORef t

More concise:
advanceTime (TestClock ref) = modifyIORef' ref . addUTCTime
getTestTime = readIORef . unTestClock
newTestClock = fmap TestClock . newIORef

In runMutableTime, CurrentTime -> liftIO (readIORef (unTestClock clock)) removes the extra hop through getTestTime.

app_now() VOLATILITY

Declared STABLE but reads current_setting (a session variable). VOLATILE is more accurate since PostgreSQL may cache STABLE results across repeated calls in a single query. Recommend VOLATILE unless there is a measured performance reason to keep STABLE.

SMALLER NITS

• syncConnectionTime uses PGS.execute for SELECT set_config(). PGS.execute_ fits better semantically (no result); functionally harmless.
• GHC.Clock.getMonotonicTime is an internal GHC module; prefer System.Clock or effectful-provided equivalent if available.
• TestClock (..) export exposes unTestClock, letting callers bypass the API. Fine for test code, just be intentional.
• PR body says 0033_app_now_function.sql; actual file is 0094.

SUMMARY

The migration and TestClock module are clean. Two things to resolve: (1) land the TestUtils/query-function integration the PR description promises, or explicitly scope the PR down to foundation-only; and (2) clarify set_config transaction-scope semantics before the pool-wrapper is wired in.

Code Review

Good overall design — the IORef-backed mutable clock threaded through both the Haskell Time effect and PostgreSQL's app_now() GUC is the right approach for unified time testing. The API simplifications (toServantResponse tr instead of three args, testServant = toServantResponse) are clean wins. A few issues worth addressing before merge:

Bug / correctness

syncConnectionTime gap: The SQL migration replaces NOW() with app_now() in set_updated_at, new_anomaly_proc, etc., but syncConnectionTime is never called on pool checkouts (the runWithTimeSyncedPool wrapper is explicitly deferred). Until that follow-up lands, app_now() falls back to real NOW() on every connection, so updated_at stamps written by triggers during tests will use wall-clock time, not test-clock time. Any test that asserts on trigger-stamped timestamps will be comparing against the wrong clock. Worth a comment in the migration and/or a TODO in TestClock.hs so it's discoverable.

Stale module doc

The TestClock.hs module Haddock says:

Pass a test connection through runWithTimeSyncedPool to wrap connection pools…

runWithTimeSyncedPool is neither implemented nor exported in this PR. Remove the reference or replace it with a note that pool-level sync is a follow-up.

Minor style

runTestBg spurious lambda (TestUtils.hs):

-- current
runTestBg t TestResources{..} = \action -> do
  setTestTime trTestClock t
  ...

-- cleaner — move the arg into the binding
runTestBg t TestResources{..} action = do
  setTestTime trTestClock t
  ...

Magic number seconds — Data.Time already exports nominalDay :: NominalDiffTime. The three helpers could use it to avoid the 60/3600/86400 literals:

advanceMinutes tr n = advanceTestTime tr (fromIntegral n * nominalDay / 1440)
advanceHours   tr n = advanceTestTime tr (fromIntegral n * nominalDay / 24)
advanceDays    tr n = advanceTestTime tr (fromIntegral n * nominalDay)

PGS.execute for a SELECT (syncConnectionTime): execute is conventionally for DML and returns Int64 affected rows. The call works in practice but is unconventional. Using void $ PGS.query @[PGS.Only Text] conn "SELECT set_config(...)" ... would match intent more clearly, or wrap the call as PGS.execute_ conn (fromString ("SET app.current_time = '" <> timeStr <> "'")) (though that loses parameterisation — stick with query then).

SQL

app_now() is declared STABLE. This is safe for the trigger use case (consistent time within one INSERT), but be aware it prevents PostgreSQL from re-evaluating it per-row inside a single query scan. For a clock function that's fine — NOW() behaves the same way — just documenting the intent would help future readers.

The check_tests_to_trigger loop references next_run_at after the inner loop; if the collection's schedule yields zero iterations next_run_at is uninitialized and the trailing UPDATE guard IF next_run_at < collection_record.last_run silently no-ops. This appears to be pre-existing behaviour preserved from migration 0001, but worth a note.

Summary

Foundation is solid. The two blockers are: (1) clean up the stale runWithTimeSyncedPool reference in the module doc, and (2) add a visible note somewhere that DB trigger timestamps will still use wall clock until pool sync lands. The rest are polish.

Code Review: Unified Time Testing with app_now()

Good overall design — replacing the frozen-clock approach with a mutable, advanceable TestClock is the right abstraction. The Haskell side is clean. However, there is one critical functional gap and a few code-quality items worth addressing.

Critical: DB sync is not wired up

syncConnectionTime is defined, exported, and even imported in TestUtils.hs, but it is never called anywhere. The PR description mentions runWithTimeSyncedPool, which would have hooked it into pool checkout — but that function does not appear in the diff at all.

Consequence: app_now() in PostgreSQL always falls back to NOW() during tests. Every trigger (set_updated_at, new_anomaly_proc) and procedure (check_triggered_query_monitors, check_tests_to_trigger) ignores the test clock entirely. The SQL migration in 0094 is effectively a no-op for tests as written.

Fix needed: Wrap the test pool with a connection-checkout callback that calls syncConnectionTime, or call it explicitly before each query that touches a trigger/procedure. The import of syncConnectionTime in TestUtils.hs is currently unused dead code.

Time format string is fragile; iso8601Show already exists in the codebase

TestClock.hs:78:

let timeStr = formatTime defaultTimeLocale "%F %T%Q+00" t

The +00 suffix is hand-rolled. The codebase already uses iso8601Show from Data.Time.Format.ISO8601 (e.g. Models.Apis.LogQueries), which produces a well-formed RFC 3339 string that PostgreSQL parses unambiguously:

import Data.Time.Format.ISO8601 (iso8601Show)
void $ PGS.execute conn "SELECT set_config('app.current_time', ?, true)" (PGS.Only (iso8601Show t))

Prefer the package function over the hand-coded format string.

Magic numbers in advance helpers; use Data.Time.Clock.nominalDay

TestUtils.hs:671-681:

advanceMinutes tr n = advanceTestTime tr (fromIntegral n * 60)
advanceHours   tr n = advanceTestTime tr (fromIntegral n * 3600)
advanceDays    tr n = advanceTestTime tr (fromIntegral n * 86400)

Data.Time.Clock exports nominalDay :: NominalDiffTime (= 86400s). Derive from it to remove the magic numbers:

import Data.Time.Clock (nominalDay)

advanceDays    tr n = advanceTestTime tr (fromIntegral n * nominalDay)
advanceHours   tr n = advanceTestTime tr (fromIntegral n * nominalDay / 24)
advanceMinutes tr n = advanceTestTime tr (fromIntegral n * nominalDay / 1440)

logNotifications runs two effect stacks per notification

TestUtils.hs:233-252: the interpreter chain (Logging.runLog ... & Effectful.runEff) is rebuilt and executed twice per notification — once for logInfo, once for logTrace. Sequence both log actions and run them in a single stack:

let runLog = Logging.runLog ("background-job:" <> show appCtx.config.environment) logger appCtx.config.logLevel
forM_ notifications \notification -> do
  let notifInfo = ...
  (Log.logInfo "Notification" notifInfo >> Log.logTrace "Notification payload" notification)
    & runLog & Effectful.runEff

runTestBg silently resets the shared clock on every call

TestUtils.hs:224-228:

runTestBg t TestResources{..} action = do
  setTestTime trTestClock t   -- resets shared clock

A test that calls advanceMinutes tr 60 and then runTestBg frozenTime tr will have its advancement silently undone. This is noted in the Haddock but is easy to miss. Consider renaming to runTestBgAt to signal the reset semantics.

Minor

• GHC.Clock is imported directly in TestClock.hs for getMonotonicTime. Worth confirming effectful-time does not already re-export it to avoid an extra raw base/ghc-prim dependency in test infra.
• Several Haddock comments in TestClock.hs restate the type signature verbatim ("Read the current time from the test clock" over getTestTime :: TestClock -> IO UTCTime). These can be trimmed.