Code Review

This PR switches the integration suite from shared-per-file to per-example test resources (aroundAll → around), adds a SpecHook.hs to run the suite with parallel, and shrinks connection pools to fit within Postgres's 100-connection cap. The intent is sound — hermetic per-test DBs enable real parallelism. However, three spec files were converted mechanically but still carry cross-test state dependencies that will silently fail under around.

🐛 Bugs — tests that will fail under around

test/integration/MonitoringSpec.hs — "Widget Alert Query Sync" (≈ line 143)

The test searches for "Alert Cascade Test Dashboard" that was created by the preceding it block ("Widget Alert Cascade Deletion"). With aroundAll both tests shared a DB; with around each gets a fresh DB, so the dashboard won't exist and the test errors with its own message: "Dashboard not found - previous test may have failed". The updated comment ("tests share state via around") makes it worse — around does the opposite. This file needs the same treatment as GitSyncSpec: sequential + aroundAll, or the "Widget Alert Query Sync" test needs to create its own dashboard.

test/integration/Pages/AnomaliesSpec.hs — multiple tests (≈ lines 135, 159, 215, 223)

Several it blocks reference "the previous test" explicitly:

• "should detect new shape anomaly" checks "Verify issues exist in the database (they may be acknowledged from previous test)"
• "should get acknowledged anomalies" asserts "We acknowledged at least one API change issue in the previous test"

These are sequential state-passing tests that relied on aroundAll. With around each starts with an empty DB and the assertions will fail or be vacuously true.

test/integration/Pages/Projects/EveryoneSyncSpec.hs — "resolveTeamEmails..." (≈ line 68)

The second it block explicitly seeds its precondition from the first: "After the previous test, alice is a member AND in notify_emails. Wipe notify_emails". With around, alice was never inserted.

🧹 Stale comments

test/integration/Opentelemetry/GrpcIngestionSpec.hs ≈ line 294

The comment was updated from "aroundAll shares state" to "around shares state" — but around does not share state. The unique-email strategy ([email protected]) was a workaround for aroundAll's shared DB; with around it's unnecessary. The comment should be removed (or inverted: "unique names are no longer needed since each test gets a fresh DB").

Notes

• Pool math in the new withExternalDBSetup comment (3 pg + 3×2 hasql ≈ 9 per test) is correct.
• SpecHook.hs is clean — a minimal hook = parallel with a clear docstring.
• GitSyncSpec.hs opt-out (sequential + aroundAll) is the right pattern; the three files above need the same treatment or test-level self-containment.
• Removing base from unit-tests in the cabal file is fine — it's always in scope via relude/base implicit deps.

🤖 Generated with Claude Code

Code Review

This PR makes a good structural improvement — per-example resource isolation is the right model for a parallel test suite, and the SpecHook.hs + sequential opt-out pattern is clean. However, two spec files were converted to around despite having explicit sequential dependencies between their it blocks, which will cause hard CI failures.

Critical

test/integration/Pages/ErrorPatternsSpec.hs — around breaks ~13 sequentially-dependent tests

Test "2." (line 124) opens with the comment:

-- processOneMinuteErrors in test 1 already created issues synchronously
issueCount <- countIssues tr Issues.RuntimeException
issueCount `shouldSatisfy` (> 0)

On a fresh per-example DB, issueCount is always 0. This cascades: tests 2a, 3, 3a, 4a, 5, 5a–5e, 6, 7, 8, 11, 12, 13 all read state seeded by prior examples and will fail unconditionally. This file needs sequential $ aroundAll withTestResources exactly like MonitoringSpec and AnomaliesSpec.

test/integration/Pages/LogPatternsSpec.hs — same problem

Test "3." (line 128) asserts allPatterns \shouldSatisfy` (not . null)with no local setup — it relies on patterns seeded by test"1.". Test "11."` (line 588) contains the explicit guard:

expectationFailure "no merged log patterns found (test 9 may have failed)"

Both will fail on every run. This file also needs sequential $ aroundAll.

High

src/Pkg/TestUtils.hs — hasql pools never released in withTestResources

withExternalDBSetup correctly uses finally to call destroyAllResources pool on the pg pool (line 315). But the three hasql pools created in withTestResources (lines 699–701) are never explicitly destroyed — no bracket or finally wraps them. Under the old aroundAll this was one leak per file. Switching to around makes it one leak per example, so a few hundred hasql pools (each holding up to 2 open connections) accumulate across the full parallel run. At some concurrency level this will collide with the 100-connection cap and cause spurious too many clients failures late in the suite. Each of the three pools should be destroyed in a finally or via bracket_.

Medium

test/integration/Opentelemetry/GrpcIngestionSpec.hs — test 11.7 passes vacuously

Test "11.7: sort_by=events orders sessions by event count" ingests no sessions of its own. With around and a fresh DB, the result is an empty vector. The assertion counts \shouldBe` sortBy (flip compare) countsbecomes[] == []` — always true, always green, exercises nothing. The comment updated by this PR is on a different test (11.2); the actual 11.7 body has no ingest call. It needs its own setup to be meaningful.

Low

src/Pkg/TestUtils.hs:262 — withLocalSetup pool cap (5) undocumented vs. withExternalDBSetup's 3

withExternalDBSetup (line 305) documents its reasoning: "3 pg + 3×2 hasql ≈ 9 per test". withLocalSetup (line 262) uses 5 with no comment. Both setups now run under the same around + parallel concurrency model, so the local path allows ~67% more connections per example than the external path with no stated justification. Consider aligning to 3 (or at least adding the same budget comment) to keep the ceiling consistent across CI and local runs.

Review: per-test resource isolation + parallel suite

This PR correctly switches 40+ spec files from aroundAll (shared-per-file) to around (per-example) and adds SpecHook.hs to enable suite-wide parallelism. State-coupling specs are correctly identified and wrapped in sequential $ aroundAll. The hasql pool cleanup via finally is a real fix for connection exhaustion under parallel. Overall direction is sound.

1. finally chain silently skips the 2nd/3rd release if the first throws

src/Pkg/TestUtils.hs:794

`finally` (OHasql.release hasqlMain >> OHasql.release hasqlJobs >> OHasql.release hasqlTf)

>> short-circuits on exception: if release hasqlMain throws, hasqlJobs and hasqlTf are never released. Under a parallel suite with many concurrent tests, partial cleanup can push open-connection counts toward the 100-conn cap.

Nested finally guarantees all three:

`finally` (OHasql.release hasqlMain `finally` (OHasql.release hasqlJobs `finally` OHasql.release hasqlTf))

(OHasql.release is unlikely to throw, but the pattern matters for correctness.)

2. withLocalSetup pool is never destroyed, unlike withExternalDBSetup

src/Pkg/TestUtils.hs:264

withExternalDBSetup wraps its pool in:

finally (f pool cstr) $ do
  destroyAllResources pool
  ...

But withLocalSetup just:

pool <- newPool (defaultPoolConfig (connectPostgreSQL cstr) close 60 3)
f pool cstr   -- no finally, no destroyAllResources

Under the old aroundAll this leaked one pool per spec file (minor). Under around it leaks one per example. Connections die when tmp-postgres shuts down so there is no practical connection-cap risk on the local path, but the asymmetry with the external path is surprising and worth fixing with finally (f pool cstr) (destroyAllResources pool).

3. GrpcIngestionSpec numbered tests look sequentially designed — CI confirmation critical

test/integration/Opentelemetry/GrpcIngestionSpec.hs:288

Tests are labelled "Test 1.1", "Test 1.2", ..., "Test 11.1", "Test 11.2". The updated comment says Test 11.2's unique emails are now "belt-and-suspenders" since each example gets a fresh DB — but the numbered scheme across ~40 tests strongly suggests these were written to accumulate state sequentially (API keys created in test 1.1 re-used in later tests, etc.).

If any test body relies on rows seeded by an earlier numbered test, it will silently pass with an empty DB now (vacuously true assertions). Given the PR note — "this change was reconstructed after the working-tree copy was lost; reviewers should confirm the parallel suite is green in CI before merge" — please verify green CI for this spec file in particular before merging.

Minor

• package.yaml:29 — the comment # System.ServerSpec imports Control.Concurrent/Exception directly (not via relude) is added to the whole stanza's dependency list rather than near that file's import; consider dropping or moving it, since the constraint lives in the source file, not here.

• test/integration/SpecHook.hs:8 — docstring says "Files that genuinely share state…opt out locally with sequential" — worth adding "or convert to around for per-test isolation" so future authors know both opt-out strategies.

The aroundAll → around sweep and pool-size reduction are well-executed. The finally chain fix and withLocalSetup symmetry are small wins worth landing before merge.

Review: test(integration): per-test resource isolation + parallel suite

The PR cleanly converts 40+ spec files from shared-per-file (aroundAll) to per-example (around) resource isolation and wires up SpecHook.hs so hspec-discover runs the whole suite under parallel. The GrpcIngestionSpec tests that look sequential (Test 1.1 … 11.10) each create their own keys/data from scratch, so the parallel conversion is correct there. Files with genuine ordering dependencies (MonitoringSpec, LogPatternsSpec, GitSyncSpec) opt out correctly via sequential. The connection-pool shrinkage is well-reasoned.

1. sharedTestLogger bypasses tolerantLogger — potential suite-killing crash

src/Pkg/TestUtils.hs:697

sharedTestLogger = unsafePerformIO $ mkBulkLogger "test-stdout-bulk" (mapM_ (putTextLn . showLogMessage Nothing)) (pure ())

tolerantLogger in System.Logging wraps loggerWriteMessage with void . Safe.tryAny . ... to silence AssertionFailed: attempt to write to a shut down logger. The comment explains this was precisely the crash the PR is fixing. But sharedTestLogger bypasses that protection: if the bulk-logger's background flushing thread crashes for any reason (e.g. an exception in the write callback), every subsequent log call from the parallel suite will throw AssertionFailed and kill workers.

tolerantLogger is not currently exported from System.Logging. The fix is a one-liner export + application:

-- System.Logging: add tolerantLogger to the export list

-- TestUtils.hs:
import System.Logging (tolerantLogger)

sharedTestLogger = tolerantLogger . unsafePerformIO $
  mkBulkLogger "test-stdout-bulk" (mapM_ (putTextLn . showLogMessage Nothing)) (pure ())

All production loggers go through this wrapper; the shared test logger should too.

2. withSharedLogger is a redundant wrapper — remove it

src/Pkg/TestUtils.hs:699–700

withSharedLogger :: (Log.Logger -> m a) -> m a
withSharedLogger act = act sharedTestLogger

This is ($ sharedTestLogger) — pure function application with no setup, teardown, or exception safety. Its only effect is adding indirection and 4 extra lines. Given the project's stated priority of keeping code concise, remove withSharedLogger and inline sharedTestLogger at the 5 call sites. Each becomes:

-- before
withSharedLogger \logger -> ...
-- after
let logger = sharedTestLogger

or simply pass sharedTestLogger directly where it's the only argument.

3. (pure ()) finalizer silently drops terminal log messages

src/Pkg/TestUtils.hs:697

mkBulkLogger takes IO () as a finalizer that runs when the logger is explicitly shut down. With pure (), the bulk worker's in-flight queue is never explicitly flushed before the process exits. The last batch of log messages from tests completing near the end of the parallel run will be lost. Since Control.Exception.Safe is already imported, a minimal fix is:

sharedTestLogger = unsafePerformIO $
  mkBulkLogger "test-stdout-bulk"
    (mapM_ (putTextLn . showLogMessage Nothing))
    (hFlush stdout)  -- flush on explicit close; still won't fire at process exit

A more complete fix requires registering a hFlush stdout handler via System.IO's hSetBuffering or an atexit-equivalent, but for a test logger the hFlush finalizer at least handles orderly shutdown paths.

4. Connection cap is tight if CI parallelism is high

src/Pkg/TestUtils.hs:304 — pool size 3 pg-simple + 3 × 2 hasql = 9 per example.

With hspec parallel, the concurrency bound is numCapabilities (RTS -N). A 16-core CI runner at -N16 would need 144 connections — above Postgres's 100-connection cap. The PR's math works for ≤11 parallel examples (11 × 9 = 99). Worth either capping hspec's thread count explicitly (--jobs 10) in the CI invocation, or documenting the assumption in a comment alongside the pool sizes.

Overall this is a solid structural change. Findings 1 and 2 are the ones I'd act on before merge; 3 and 4 are low-severity.

Code Review

This PR switches ~40 spec files from shared-per-file (aroundAll) to per-example (around) test resources and applies parallel globally via SpecHook.hs, with specs that have genuine cross-example state dependencies opting out via sequential. The resource-pool shrinkage and hasql pool cleanup-on-release are correct approaches to staying under Postgres's 100-connection cap under parallelism.

Bug: wrong comment in GrpcIngestionSpec (misleading, could cause a real regression later)

test/integration/Opentelemetry/GrpcIngestionSpec.hs — diff around Test 11.2:

-        -- Use unique emails to avoid collision with Test 11.1 data (aroundAll shares state)
+        -- Unique emails are belt-and-suspenders; each example gets a fresh DB (around)

The spec was changed to sequential $ aroundAll withTestResources (shared DB across all examples), not around. The old comment was accurate: the unique emails in Test 11.2 are required to avoid polluting Test 11.1's data, not optional belt-and-suspenders. A future maintainer trusting the new comment and removing the unique emails will get silently incorrect test results due to cross-example contamination.

Fix: revert the comment change, or update it to reflect aroundAll semantics.

Nit: withSharedLogger is a trivial one-liner

withSharedLogger :: (Log.Logger -> m a) -> m a
withSharedLogger act = act sharedTestLogger

This is just ($ sharedTestLogger). Call sites could reference sharedTestLogger directly — or if the indirection layer is desired for future swappability, a note in the type signature comment would make the intent clear. No strong objection.

Nit: unsafePerformIO + {-# NOINLINE #-} for the global logger is the idiomatic Haskell pattern here

The sharedTestLogger CAF is correct as written — NOINLINE ensures single evaluation, Log.Logger is documented as thread-safe, and the logger intentionally runs for process lifetime. If the project has extra on its dependency list, System.IO.Unsafe.unsafePerformIO could be replaced with once from extra for a slightly cleaner API, but it's not a correctness issue.

Minor: withLocalSetup cleanup doesn't include the threadDelay that withExternalDBSetup has

withExternalDBSetup wraps its destroyAllResources pool call with a threadDelay 100ms (presumably to let connections drain before the process moves on). The new withLocalSetup cleanup:

finally (f pool cstr) (destroyAllResources pool)

omits this. Under parallel runs this is probably fine (connections are reused, not time-sensitive), but worth a quick check that the delay in withExternalDBSetup wasn't working around a real race.

Overall the approach is sound and the mechanical aroundAll → around conversions look correct. The one actionable item before merge is fixing the GrpcIngestionSpec comment.

Review

This PR converts the integration suite from per-file to per-example resource isolation and adds SpecHook to run the whole suite in parallel. The advisory-lock guard on template-DB construction, the pool-size reductions, and the shared process-lifetime logger are all well-reasoned solutions to the race conditions that per-example around exposes. The audit of which specs genuinely share state and need sequential $ aroundAll is thorough.

Four issues worth addressing before merge:

1. templateConn leaked on migration exception

src/Pkg/TestUtils.hs:392-410

If either runMigration call or the seed execute throws, templateConn is never closed. The outer flip finally (close masterConn) only closes masterConn; templateConn is orphaned and holds a connection to the template DB. That dangling connection blocks the next holder of pg_advisory_lock(873042) from issuing DROP DATABASE WITH (FORCE) (Postgres refuses to drop a DB while connections to it exist), leaving the suite stuck on the next run.

Fix with a bracket:

bracket (connect $ connInfo (toString templateDbName)) close $ \templateConn -> do
  _ <- Migration.runMigration templateConn Migration.defaultOptions MigrationInitialization
  ...

2. runCLILifecycle setEnv/unsetEnv races under parallel

src/Pkg/TestUtils.hs:1366-1368, exposed by CLILifecycleSpec switching to around

setEnv/unsetEnv mutate the process-global environment and are not thread-safe across OS threads. CLILifecycleSpec now runs its it blocks concurrently (via SpecHook's hook = parallel). If thread A's unsetEnv "MONOSCOPE_API_KEY" fires while thread B is still inside its own bracket_, thread B sees the key absent and the CLI bails with a missing-key error -- a flaky failure that is hard to reproduce locally.

Either mark CLILifecycleSpec as sequential (consistent with the other state-sharing specs) or guard runCLILifecycle with an MVar mutex.

3. withSharedLogger is a trivial zero-value wrapper -- remove it

src/Pkg/TestUtils.hs:709-710

withSharedLogger :: (Log.Logger -> m a) -> m a
withSharedLogger act = act sharedTestLogger

This wraps a constant. All six call sites do withSharedLogger \logger -> ..., which is identical to passing sharedTestLogger directly. The CPS shape implies resource acquisition/cleanup that is not there -- it is the old withBulkStdOutLogger form left as a migration artifact. Replacing call sites with the direct reference makes it clear no lifecycle is involved.

4. sequential $ aroundAll withTestResources duplicated 12+ times -- extract a helper

SchemaLearningSpec.hs, ErrorPatternsSpec.hs, MonitorsSpec.hs, DashboardsSpec.hs, ManageMembersSpec.hs, and others

Every state-sharing spec carries the same 3-line comment block plus sequential $ aroundAll withTestResources. A helper exported from TestUtils:

sequentialWithTestResources :: SpecWith TestResources -> Spec
sequentialWithTestResources = sequential . aroundAll withTestResources

collapses each site to spec = sequentialWithTestResources do, centralises the explanation, and removes ~35 lines of copy-paste. Future specs that share state call the helper rather than re-learning the pattern.

Review — test(integration): per-test resource isolation + parallel suite

The PR does solid work: per-example DB isolation via around withTestResources, a shared process-level logger that avoids write-after-shutdown races, advisory locking to serialize template-DB construction, and correctly identified which specs must stay sequential. Three resource-safety issues and a couple of simplification opportunities below.

Bugs

1. masterConn leaks if pg_advisory_lock call throws (src/Pkg/TestUtils.hs ~line 362–372) — new, introduced by this PR

masterConn <- connect $ connInfo "postgres"
let alter clause = ...
_ <- PGS.query_ masterConn "SELECT pg_advisory_lock(873042)::text" :: IO [Only Text]  -- ← outside finally
flip finally (close masterConn) $ do   -- ← finally installed too late
  ...

The finally is installed after the advisory lock call. If pg_advisory_lock throws (network blip, server restart between connect and the query), masterConn is leaked. The fix is to move the lock call inside the finally:

masterConn <- connect $ connInfo "postgres"
flip finally (close masterConn) $ do
  _ <- PGS.query_ masterConn "SELECT pg_advisory_lock(873042)::text" :: IO [Only Text]
  ...

2. templateConn leaks if any migration or seed execute throws (src/Pkg/TestUtils.hs ~line 392–410) — pre-existing, but the PR restructured this block

templateConn <- connect $ connInfo (toString templateDbName)
_ <- Migration.runMigration templateConn ...   -- throws → close never reached
_ <- Migration.runMigration templateConn ...
_ <- execute templateConn [sql|...|] ()
close templateConn                             -- ← never reached on exception

There is no finally or bracket around templateConn. A migration syntax error or disk-full condition leaks the connection and leaves the template DB in a half-built state. Fix:

templateConn <- connect $ connInfo (toString templateDbName)
flip finally (close templateConn) $ do
  _ <- Migration.runMigration templateConn ...
  ...

3. masterConn in withExternalDBSetup leaks if createFromTemplate throws (src/Pkg/TestUtils.hs ~line 290–304) — pre-existing

masterConn <- connect $ connInfo "postgres"
...
createFromTemplate 0     -- throwIO e branch skips close
close masterConn         -- ← unreachable on exception

The otherwise -> throwIO e branch inside createFromTemplate propagates without closing masterConn. Wrap with flip finally (close masterConn).

Simplifications

4. sharedTestLogger expression — prefer fmap over point-free f . unsafePerformIO $ (src/Pkg/TestUtils.hs line 706)

-- current (valid but reads oddly — unsafePerformIO composed point-free):
sharedTestLogger = Logging.tolerantLogger . unsafePerformIO $ mkBulkLogger "test-stdout-bulk" (mapM_ (putTextLn . showLogMessage Nothing)) (pure ())

-- clearer — maps a pure function over the IO result before unsafe-running it:
sharedTestLogger = unsafePerformIO $ Logging.tolerantLogger <$> mkBulkLogger "test-stdout-bulk" (mapM_ (putTextLn . showLogMessage Nothing)) (pure ())

f . unsafePerformIO $ action is valid Haskell (. binds tighter than $) but the fmap form makes the intent (run IO once, wrap result) immediately obvious and avoids the precedence-dependent composition.

5. withSharedLogger is a superfluous CPS wrapper (src/Pkg/TestUtils.hs lines 709–710)

withSharedLogger :: (Log.Logger -> m a) -> m a
withSharedLogger act = act sharedTestLogger

Every call site withSharedLogger \logger -> ... can be replaced with direct use of sharedTestLogger. The wrapper adds nothing over ($ sharedTestLogger) or inline reference. Two call sites (runQueryEffect, runAllBackgroundJobsNoReset) also have trLogger (the same value) in scope via record-wildcard destructuring of TestResources{..} — those can just use trLogger directly, making the logger provenance traceable without touching the global.

Note on parallelism

The PR title ("parallel suite") and description ("The full parallel suite runs in CI") are misleading — SpecHook.hook = sequential forces the entire discovered suite to run sequentially. The comments in SpecHook.hs itself acknowledge this. Consider updating the PR title/description to reflect the actual state: "per-test resource isolation (parallel follow-up TBD)". The per-example isolation is the real value here and it stands on its own.

The advisory lock magic number 873042 is undocumented — a short comment explaining it's arbitrary (or how it was chosen) would help future readers avoid accidentally reusing it.

Code Review

This PR switches the integration suite from per-file shared resources (aroundAll) to per-example resources (around) for hermetic test isolation, adds a pg advisory lock to prevent template-DB races, shrinks pool sizes, and fixes a logger lifecycle crash. The SpecHook.hs runs the whole suite sequentially for now — parallelism is deferred to a follow-up.

Findings

🔴 templateConn leaks on migration failure — src/Pkg/TestUtils.hs (ensureTemplateDatabase)

The PR correctly wraps masterConn in flip finally (close masterConn), but the inner templateConn used for running migrations is only closed by a bare close templateConn — not inside a finally or bracket. If any of runMigration, the seed execute, etc. throw, templateConn is leaked. The fix is consistent with what the PR already did for masterConn:

-- instead of: templateConn <- connect ...; ...; close templateConn
bracket (connect $ connInfo (toString templateDbName)) close $ \templateConn -> do
  _ <- Migration.runMigration templateConn Migration.defaultOptions MigrationInitialization
  _ <- Migration.runMigration templateConn Migration.defaultOptions $ MigrationDirectory migrationsDirr
  _ <- execute templateConn [sql| ... |] ()

This is pre-existing, but the PR touched this code and fixed the outer connection without fixing the inner one.

🟡 withSharedLogger CPS shape implies a lifecycle it doesn't have — src/Pkg/TestUtils.hs

withSharedLogger :: (Log.Logger -> m a) -> m a
withSharedLogger act = act sharedTestLogger

The with* bracket naming convention signals "open → use → close". Here there is no open/close — it's just act sharedTestLogger. Readers expect some resource management. Callers can simply reference sharedTestLogger directly (it's already a module-level binding), or at the very least rename to useSharedLogger / applySharedLogger to avoid the bracket implication.

Additionally, runQueryEffect uses withSharedLogger \logger -> even though TestResources{..} already brings a trLogger into scope (assigned in withTestResources via the same shared logger). Using trLogger there would be consistent with the rest of the record and would survive correctly if a scoped logger is ever injected per-test.

🟡 SpecHook comment is inaccurate and misleading — test/integration/SpecHook.hs

"Each spec file uses around withTestResources (a fresh DB + test clock per example) for per-test isolation; state-sharing files use aroundAll."

The two clauses contradict each other (not "each" if some use aroundAll). More importantly, the ~15 files that kept aroundAll also have their own local sequential wrapper, meaning the suite is doubly-sequential: once from SpecHook, once per file. When parallelism is re-enabled at the hook level, those files will still run sequentially as intended — good. But anyone who reads this comment expecting a single enforcement point will be misled. A cleaner version:

"Non-state-sharing spec files use around withTestResources for per-example isolation. State-sharing files keep aroundAll + a local sequential wrapper; they are safe to parallelize at the suite level when SpecHook is updated."

🟡 Six sequential $ aroundAll specs are missing the standard explanatory comment

Files like MonitoringSpec.hs, ErrorPatternsSpec.hs, LogPatternsSpec.hs, etc. all carry a comment explaining why they use aroundAll + sequential. These six do not:

• test/integration/Pages/ApiSpec.hs
• test/integration/Pages/MonitorsSpec.hs
• test/integration/Pages/Projects/ManageMembersSpec.hs
• test/integration/Pages/Projects/IntegrationsSpec.hs
• test/integration/SchemaLearningPerfSpec.hs
• test/integration/Pages/Endpoints/ApiCatalogSpec.hs

Without the comment it's impossible to distinguish an intentional ordering dependency from an accidental copy-paste of sequential. Anyone trying to migrate these specs to around later has to audit the examples by hand.

🟢 package.yaml dep comment references wrong spec file

- base # System.ServerSpec imports Control.Concurrent/Exception directly (not via relude)

System.ServerSpec lives in the unit-tests suite, not integration-tests. The comment was copied from the wrong stanza and will confuse anyone wondering why the integration suite needs base listed explicitly (it's a boot package available everywhere anyway).

🟢 Pool-size comment rationale is stale

The comments -- Small per-test pool: under \parallel` many test DBs are live at once...appear in bothwithLocalSetupandwithExternalDBSetup. Since SpecHooknow appliessequentialglobally, at most onewithTestResources bracket is live at a time. The math (3 pg + 3×2 hasql ≈ 9 per test`) is irrelevant to the current sequential mode. Update when parallelism is re-enabled, or note that the conservative sizes are retained pending that follow-up.

Overall the approach is solid — per-example isolation is a meaningful improvement and the advisory lock for template-DB races is the right fix. The main actionable items are the templateConn leak (correctness) and the withSharedLogger naming (maintenance).

🤖 Generated with Claude Code

Code Review — PR #449

This PR converts integration specs from shared-per-file (aroundAll) to per-example (around) resources, adds an advisory lock to serialize template-DB construction under concurrency, reduces pool sizes to stay under Postgres's connection cap, and introduces a global SpecHook. The approach is sound; a few issues worth addressing before merge.

1. Hasql pool leak when withTestResources setup throws (src/Pkg/TestUtils.hs)

hasqlMain <- mkHasqlPool 2 cstr
hasqlJobs <- mkHasqlPool 2 cstr
hasqlTf   <- mkHasqlPool 2 tfCstr
sessAndHeader <- testSessionHeader pool hasqlMain   -- ← can throw
...
f TestResources{...}
  `finally` (OHasql.release hasqlMain `finally` ...)

If anything between the mkHasqlPool calls and the f invocation throws (e.g. testSessionHeader, cache creation, lookupEnv), the finally is never established and all three pools leak. Use bracket to bind each pool's lifetime to its acquisition:

bracket (mkHasqlPool 2 cstr)   OHasql.release $ \hasqlMain ->
bracket (mkHasqlPool 2 cstr)   OHasql.release $ \hasqlJobs ->
bracket (mkHasqlPool 2 tfCstr) OHasql.release $ \hasqlTf   -> do
  sessAndHeader <- testSessionHeader pool hasqlMain
  ...
  f TestResources{...}

This is a new gap: the original code didn't release pools at all, but now that it explicitly does, the early-throw window matters.

2. Nested finally chain should be mapM_ (src/Pkg/TestUtils.hs, line ~251)

-- current
`finally` (OHasql.release hasqlMain `finally` (OHasql.release hasqlJobs `finally` OHasql.release hasqlTf))

This is the same as:

`finally` mapM_ OHasql.release [hasqlMain, hasqlJobs, hasqlTf]

The mapM_ form is shorter, doesn't need the "nested so a throw releasing one pool still releases the others" comment (that guarantee holds for mapM_ + finally too via ExitCase), and trivially extends if a 4th pool is added.

3. query_ + SQL ::text cast for pg_advisory_lock — use execute_ (src/Pkg/TestUtils.hs, line ~139)

-- current: awkward cast + type annotation just to satisfy FromRow
_ <- PGS.query_ masterConn "SELECT pg_advisory_lock(873042)::text" :: IO [Only Text]

pg_advisory_lock is called purely for its side effect. execute_ accepts SELECT statements and discards the result without requiring a concrete FromRow:

void $ PGS.execute_ masterConn "SELECT pg_advisory_lock(873042)"

4. Magic number 873042 should be a named constant (src/Pkg/TestUtils.hs, line ~139)

A bare 873042 in SQL is invisible to lock-auditing tooling and collides silently with any other code that independently picks the same integer. A module-level binding makes it greppable and self-documenting:

templateDbBuildLockKey :: Int64
templateDbBuildLockKey = 873042

5. Suite runs fully sequential despite "parallel suite" in the PR title (test/integration/SpecHook.hs)

hook = sequential forces the entire discovered suite to run one example at a time. The PR title and body describe a "parallel suite" and the per-example isolation work done here is a prerequisite for parallelism, but the end result is sequential. The SpecHook.hs comment acknowledges this, but:

• The PR title should be updated to avoid misleading reviewers/git-log readers.
• The aroundAll-under-parallel crash should be captured in an issue with a reference in the comment, otherwise it risks being forgotten once this PR lands.

6. Individual sequential $ wrappers in 13 spec files are correct but only meaningful once hook = parallel is restored

Files like GrpcIngestionSpec, MonitoringSpec, DashboardsSpec, ErrorPatternsSpec, etc., all add sequential $. Under the current hook = sequential these are no-ops, but they are the right pattern to keep — they encode which specs genuinely share state, and they'll do real work once the hook switches to parallel. That context is worth a one-line note in SpecHook.hs so future readers don't strip them out thinking they're dead code.

Minor

• sharedTestLogger is constructed with mkBulkLogger ... (pure ()) as the flush/cleanup action. The {-# NOINLINE #-} pragma is essential for unsafePerformIO correctness (prevents GHC from floating it into a CAF or inlining it multiple times), but the existing comment block doesn't call this out explicitly. A one-liner — -- NOINLINE required: without it GHC may re-evaluate the unsafePerformIO and create multiple loggers — saves a future reader from having to know this rule by heart.

🤖 Generated with Claude Code

Review

This PR switches 30+ integration specs from aroundAll (shared-per-file DB) to around (per-example DB), adds an advisory lock to serialize template-DB construction under parallelism, and introduces a process-lifetime shared logger to avoid write-after-shutdown crashes. The core approach is sound. The suite runs sequentially for now (hook = sequential) with parallel as a follow-up — the SpecHook comment explains the reasoning clearly.

Findings

withSharedLogger has misleading bracket-like naming with no resource management
src/Pkg/TestUtils.hs:711

withSharedLogger :: (Log.Logger -> m a) -> m a
withSharedLogger act = act sharedTestLogger

The with… naming convention in this codebase signals bracket semantics (acquire → use → release), exactly like the withBulkStdOutLogger it replaced. But this function acquires nothing and releases nothing — it's just act sharedTestLogger. A reader diffing withBulkStdOutLogger \logger -> vs withSharedLogger \logger -> will think there's a resource lifecycle when there isn't. Consider either inlining sharedTestLogger at the four call sites (let logger = sharedTestLogger) or renaming it to useSharedLogger / applySharedLogger to drop the with convention.

Advisory lock call typed as IO [Only Text] for a void-returning function
src/Pkg/TestUtils.hs:371

_ <- PGS.query_ masterConn "SELECT pg_advisory_lock(873042)::text" :: IO [Only Text]

pg_advisory_lock returns void; the ::text cast coerces it to '' so query_ has something to deserialise. This works, but the annotation IO [Only Text] implies a meaningful value is returned. A comment clarifying the cast is purely to satisfy query_'s return type would prevent future confusion. Alternatively, many projects use the pattern:

void (PGS.query_ masterConn "SELECT pg_advisory_lock(873042)" :: IO [Only (Maybe ())])

or just add a one-line comment above the existing line.

Magic number 873042 for the advisory lock key
src/Pkg/TestUtils.hs:371

The lock key is a bare integer with no indication of what it represents or how it was chosen. A module-level constant makes it self-documenting and grep-able:

templateBuildLockKey :: Int64
templateBuildLockKey = 873042

PR description contradicts the implementation

The PR body says:

New test/integration/SpecHook.hs: hspec-discover applies hook = parallel to the whole discovered suite.

But the actual SpecHook.hs has hook = sequential. The SpecHook comment explains the rationale well — the description just needs updating to say "sequential" rather than "parallel".

Positives

• The Safe.bracket around templateConn (line 393) is a clean fix — the old code leaked the connection on migration failure.
• createFromTemplate 0 \finally` close masterConn` (line 303) is a genuine correctness improvement over the previous bare sequence.
• Reducing hasql pool sizes (5→2) and pg pool sizes (50/10→3) is the right call given per-example resource creation under a connection cap.
• The {-# NOINLINE #-} pragma on sharedTestLogger is necessary and correctly applied.
• Specs that genuinely share state (GitSyncSpec, ErrorPatternsSpec, SchemaLearningSpec, etc.) are correctly identified with sequential $ aroundAll + explanatory comments.