bank: a database per ctest case instead of one shared file, and a rounding that does not charge a whole minor unit for half of one (fixes #682, fixes #678, refs #685)

[Yaraslaut]

Two bank tickets, one commit each.

• bank: the 21 bank_tests cases share one SQLite database and fail under ctest -j, with nothing in the tree saying so #682 — bank's 21 ctest cases shared one SQLite file with no lock. Each case now gets a database of its own.
• parseMinor's '+ 0.5' rounding: the clang-tidy check that was pointing at it no longer does, and the question is still open #678 — parseMinor's + 0.5 rounded a sub-half value up. std::llround instead, with morph#663's guard order preserved.

---

#682 — per-case databases, not a RESOURCE_LOCK

catch_discover_tests() registers one ctest case per TEST_CASE and ctest runs each as its own process. All fourteen bank_tests sources named one fixed path, temp_directory_path() / "morph_bank_tests.db", and ensureDatabase() deleted and re-migrated it on the way in — correct under a serial ctest and nothing else.

Before, on 7d4ca453 (clang-release, -DMORPH_BUILD_BANK_EXAMPLE=ON, clang 22.1.8):

$ ctest -j 12 -L bank
0% tests passed, 21 tests failed out of 21
HY000 (10) - [SQLite]disk I/O error (10)

Three runs of three. Serial was green, which is the only reason CI never saw it.

After, same tree, same command, five runs of five:

$ ctest -j 12 -L bank
100% tests passed out of 21
Total Test time (real) =   1.09 sec

Why not the lock

cmake/morph_add_rung.cmake:47-54 explains the ladder's choice: RESOURCE_LOCK morph_ladder_test_db, deliberately the same name across every rung, because ctest serialises any two cases sharing a lock name even across binaries. That is the right trade for the ladder, where rungs may point at one database through a shared ODBC_CONNECTION_STRING override.

Bank does not have to buy correctness with parallelism. Nothing here reads state another case wrote — every process already began by wiping the schema, and tests isolate themselves by owner principal — so a path per process is behaviour-preserving, which a lock is not free to be:

	lock	per-process path
ctest -j 12 -L bank, 21 cases	5.7s	1.1s
serialises against other suites	no (distinct lock name)	no
new machinery	none	one 90-line header

What the lock would have cost, stated plainly: it pins every developer's bank run to the suite's serial wall clock — 5.7s measured here for 21 cases, and it would have grown with each case added. It is also a remedy that has to be remembered at each new catch_discover_tests() call, which is precisely how #682 happened. The per-process path is remembered by the one header every test already includes.

examples/bank/CMakeLists.txt now records the trade next to the three catch_discover_tests() calls, so the absent RESOURCE_LOCK reads as a decision rather than the oversight it was.

Scope beyond bank_tests

bank_gui_qml_tests had the same defect in miniature — two TEST_CASEs, one morph_bank_gui_qml.db — under a comment asserting that wiping "once per process rather than once per case" made them safe, which is exactly what does not hold when the process is the case. Both GUI suites take a private path too. All three: ctest -j 12 -L bank → 100% of 29.

The mechanism

examples/bank/tests/unique_test_database.hpp claims a directory with std::filesystem::create_directory, whose true return is an exclusive claim against other processes (mkdir/CreateDirectoryW are atomic), retrying on collision — not "generate a name, then check whether it exists", which would race. A function-local static owns it and remove_alls it in its destructor. Best effort on teardown: a process killed outright leaves a stale temp directory, which is not a failed run. Verified: ls -d /tmp/morph_bank_tests-* → 0 after a full suite run.

---

#678 — std::llround

parseMinor computed (major * scale) + 0.5 and truncated, which is not round-to-nearest. For the double immediately below one half, the sum is not representable and rounds up to exactly 1.0.

The witness is typeable, not constructed:

"0.004999999999999999"  ->  scaled = 0.49999999999999994  ->  minor = 1

x                = 0.49999999999999994449
x < 0.5          = true
x + 0.5          = 1
(int64)(x + 0.5) = 1      <- returned
std::llround(x)  = 0      <- correct

The test uses the witness, and it fails first. Built on this branch with the old Format.hpp and the new test case:

test_bank_gui_format.cpp:116: FAILED:
  CHECK( parseMinor("0.004999999999999999") == 0 )
with expansion:
  {?} == 0

test_bank_gui_format.cpp:117: FAILED:
  CHECK( parseMinor("0.0049999999999999994") == 0 )

test cases: 1 | 1 failed
assertions: 3 | 1 passed | 2 failed

and after the one-line change, All tests passed (20 assertions in 5 test cases). 0.005 and 0.004 pass both before and after, so the pre-existing 0.005 case stays but is now labelled as the half-way input that must keep rounding away from zero, rather than as evidence of anything.

Scope, plainly: one minor unit on a pathological input. This is not #663 — that was undefined behaviour on ordinary input — and it is worth fixing because the fix is smaller than the argument, not because it is dangerous. It also removes the construct bugprone-incorrect-roundings was pointing at, rather than moving it somewhere the check no longer matches.

---

The one claim this branch's safety rests on

That #663's guard still runs before the rounding, and still bounds it.

The guard now applies to the unrounded scaled value, which is the stronger of the two checks. Every double strictly below 0x1p63 is at most 2^63 - 1024, so llround of anything that passes the guard lands inside std::int64_t with 1023 to spare. The accept/reject edge does not move either: doubles near 2^63 are spaced 1024 apart, so adding 0.5 never crossed it — the old minor < bound and the new scaled < bound reject exactly the same inputs. nan is still caught by the negated comparison (!(nan < bound) is true), and every #663 case still passes.

If that reasoning is wrong, the failure is a wrong answer or an abort on a large amount, which is #663 again. It is the thing to check hardest in review.

Review notes, done inline

• Would the checks pass if the change did nothing? No, for both, and that is measured rather than asserted: bank: the 21 bank_tests cases share one SQLite database and fail under ctest -j, with nothing in the tree saying so #682's fix is bracketed by a ctest -j 12 that fails 21/21 before and passes 21/21 after; parseMinor's '+ 0.5' rounding: the clang-tidy check that was pointing at it no longer does, and the question is still open #678's test case fails on the unfixed function in the same build tree.
• bank: the 21 bank_tests cases share one SQLite database and fail under ctest -j, with nothing in the tree saying so #682 has no regression guard in CI, and I could not add one — every CI leg runs ctest serially and .github/workflows/ is held by another lane. Filed as No gate catches a ctest case sharing an on-disk database, and no CI leg runs ctest in parallel #685 rather than folded in, together with the missing detector for the pattern and the observation that this is the third instance in two days of a documented remedy not reaching a new consumer (No gate catches a Q_OBJECT header split from its TU; the first signal is a link error in six CI legs #659 was the first).
• A full-file clang-tidy reports readability-identifier-length for bool ok at Format.hpp:106. Pre-existing, on a line this diff does not touch, so CI's clang-tidy-diff will not see it. Not filed: it is the designed scope of a diff-based run, not a defect.
• bank_test_support.hpp's doc comment claimed the file was shared "for the whole binary", which was true and misleading at once — the binary is one case. Corrected in place.

Verification

Measured on this branch, Linux, clang 22.1.8 (CI pins CLANG_VERSION: "22", no skew).

Check	Result
ctest -j 12 -L bank, clang-release, all three suites	100% of 29, 0.98s
ctest -L bank, clang-release	100% of 29, 5.06s
ctest -L bank, clang-ubsan + bank GUI (#683's job)	100% of 29, 14.35s
ctest -j 12 -L bank, clang-ubsan	100% of 29, 1.30s
check_sanitizer_instrumentation.sh build/clang-ubsan ubsan	9 of 9 binaries carry __ubsan_ symbols
check_rung_filters.sh	58 checks passed
check_tidy_suppression_scope.sh	ok
check_qobject_moc_pairing.py + --self-test	ok; 9 fixtures
check_bidi_controls.py	ok, 1250 files
check_nolint_directives.sh	ok, 177 directives
check_ctest_name_collisions.sh build/clang-release	36 names unique
clang-format --dry-run -Werror over the 19 touched files	clean
clang-tidy -p build/clang-release over the changed headers/TUs	no finding on a changed line

Both build directories were configured from empty, because this touches build structure.

Not verified:

• Windows and macOS. create_directory is atomic on both, and nothing here is POSIX-specific, but I ran only Linux. windows-everything configures bank, so CI will say.
• Whether any non-bank suite fails under ctest -j. I ran -j 12 only over -L bank. Recorded as the open half of No gate catches a ctest case sharing an on-disk database, and no CI leg runs ctest in parallel #685.
• -j above 12, and on a runner with fewer cores than this box.
• The serial pre-fix baseline is not cited as a comparison. The 19.45s I first measured was taken immediately after failing parallel runs, which left SQLite journal files the old remove() did not clean up; the honest serial number is the post-fix 5.06s, which is the figure used above and in the CMake comment.

Closes #682, closes #678. Filed: #685.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

Runner verification

The claim you flagged as the one to check hardest — checked numerically, and it holds exactly:

bound (2^63)           = 9223372036854775808
largest double < bound = 9223372036854774784      gap = 1024   (as claimed)
int64 max              = 9223372036854775807
llround(just_below)    = 9223372036854774784      fits, 1023 to spare
!(nan < bound)         = true                     NaN still rejected
UBSan exit=0

Moving the guard onto the unrounded value is the stronger form, and the accept/reject edge genuinely cannot move: doubles up there are 1024 apart, so + 0.5 never crossed it either. Worth verifying rather than accepting, because "the guard still bounds it" is exactly the kind of floating-point argument that reads as obvious and is occasionally wrong.

#682's numbers are the right shape — the before is a failure, not an absence:

before: 0% tests passed, 21 failed of 21   HY000 (10) - [SQLite]disk I/O error (10)   3 of 3 runs
after:  100% passed of 21,  1.09s                                                     5 of 5 runs

Choosing per-case databases over RESOURCE_LOCK is well argued and I agree with it. The lock would have pinned every developer run to the serial 5.06s for these cases forever; the measured alternative is 1.09s at -j 12 with the contention gone rather than hidden. Costing the alternative rather than asserting the choice is what makes that a decision instead of a preference.

#678's witness test fails first, shown against the old header on the same branch — and relabelling the pre-existing 0.005 case as "pins nothing on its own" is the detail I would have wanted and did not ask for.

Two things I am deciding, so they do not sit in a hand-back

The bool ok / readability-identifier-length finding at Format.hpp:106 — do not file it. It is a pre-existing finding on a line this diff does not touch, which is precisely the standing debt #677 already covers tree-wide (4155 findings, of which this is one). A separate ticket would fragment that, and #677 is triage: valid with the measurement attached. Correct call to raise it and not fold it in.

#685 is the important one, and its blocker is mine. You could not add a regression guard because .github/workflows/ belongs to the INSTALL lane — so #682's fix currently has no CI guard, and the structural half you recorded is worse than the missing detector: every CI leg runs ctest serially, so this entire class of cross-case contention is invisible by construction. That is the second time tonight a lane has been correctly stopped by a boundary I drew (the other is #674). I will sequence it rather than have a lane cross it.

The third data point you predicted and found — test_bank_gui_qml_behaviour.cpp asserting the wrong safety property in a comment ("wiped once per process rather than once per case", which is exactly what fails when the process is the case) — is the part that makes #685 worth a gate rather than a note.

Declining to cite the contaminated 19.45s serial measurement was right, and saying why is better than quietly omitting it.

Not verified by me: the ctest runs, the UBSan job reproduction, and the instrumentation sweep — all need builds I did not run. Windows and macOS remain CI's job, as you say.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!