Skip to content

test: give equation()'s 70000-node stack-safety test a wider TIMEOUT under sanitizers - #590

Merged
Yaraslaut merged 1 commit into
masterfrom
fix-589-tsan-timeout
Sep 19, 2026
Merged

test: give equation()'s 70000-node stack-safety test a wider TIMEOUT under sanitizers#590
Yaraslaut merged 1 commit into
masterfrom
fix-589-tsan-timeout

Conversation

@Yaraslaut

@Yaraslaut Yaraslaut commented Sep 19, 2026

Copy link
Copy Markdown
Member

Closes #589.

equation()'s 70,000-node provenance-chain test (tests/test_quantity.cpp:736, added by #581/morph574) exists only to prove the iterative-traversal rewrite doesn't overflow the stack — it has no performance budget of its own. But #582 already measured its O(n²) string-building cost at 32.3s for this size under ASan+UBSan, and TSan's heavier per-access instrumentation pushes it past ctest's blanket 120s TIMEOUT:

1076/1739 Test #1076: equation() walks a 70000-node provenance chain without
overflowing the stack ...... ***Timeout 120.07 sec

Reproduced on master's own CI, independent of any PR, at commit 3e8f59f2: https://github.com/LASTRADA-Software/morph/actions/runs/35431457068/job/105872791321 — and independently on two unrelated draft PRs (#585, #586) via the pull_request merge-ref, which is how it was first noticed.

The fix

A [slow] Catch2 tag on just this test case, with tests/CMakeLists.txt's catch_discover_tests() split in two by TEST_SPEC — the tagged test gets TIMEOUT 600, everything else keeps the existing TIMEOUT 120.

set_tests_properties() can't target one Catch2 test case directly here: DISCOVERY_MODE PRE_TEST defers discovery to ctest invocation time, so nothing named that test yet at configure time (CMake Error ... Can not find test to add properties to, confirmed locally before landing on the tag-split approach).

This does not touch equation()'s design or its documented output contract — that's #582's separate, larger question (a possible depth cap). This is only about the test's own timeout, since the test was never meant to measure performance.

Verified

  • ctest --show-only=json-v1 (local Debug config, no sanitizers): the tagged test alone reports TIMEOUT: 600.0; its neighbor (A 100000-node provenance chain is destroyed without overflowing the stack) keeps TIMEOUT: 120.0; total discovered test count unchanged at 1513.
  • The test itself still passes when run directly.
  • clang-format --dry-run --Werror on the one changed .cpp file: clean. (tests/CMakeLists.txt isn't clang-format's domain — CI's clang-format job only checks *.hpp/*.cpp, confirmed by reading .github/workflows/ci.yml.)
  • Not run: the actual sanitizer legs (would need the CI environment to reproduce the >120s timing this fixes). The fix is a pure test-infrastructure change with no code-path impact, so CI on this PR itself is the real verification.

🤖 Generated with Claude Code

@Yaraslaut @claude
test: give equation()'s 70000-node stack-safety test a wider TIMEOUT …
34b38fd 
…under sanitizers (fixes #589)

It exceeds ctest's blanket 120s cap under TSan (observed >120.07s on
master's own CI), whose per-access instrumentation pushes #582's
already-measured O(n^2) string-building cost over the line. The test
has no performance budget of its own -- it exists only to prove the
morph#574 iterative rewrite doesn't overflow the stack -- so this
widens its timeout alone via a `[slow]` tag rather than raising the
cap for every test, which would defeat the "hang fails fast" property
the 120s default exists for.

DISCOVERY_MODE PRE_TEST defers test discovery to ctest invocation
time, so set_tests_properties() can't target one Catch2 test case
directly at configure time; splitting the discovery call by tag is
the mechanism that's actually available.

Verified: `ctest --show-only=json-v1` shows the tagged test alone at
TIMEOUT 600 while its neighbor keeps TIMEOUT 120; the full 1513-test
count is unchanged; the test itself still passes.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@Yaraslaut
Yaraslaut merged commit 491c081 into master Sep 19, 2026
48 checks passed
@Yaraslaut
Yaraslaut deleted the fix-589-tsan-timeout branch September 19, 2026 13:14
This was referenced Sep 20, 2026
Merged
Closed
Yaraslaut added a commit that referenced this pull request Sep 20, 2026
@Yaraslaut @claude
util: bound what equation() renders, and make rendering it in full ch…
5b2e909 
…eap (fixes #582, fixes #602)

A derivation has no bound -- a data-driven `total = total + row` loop
records one step per iteration -- and equation() rendered every one of
them. 100,000 steps produced a 500,001-character first line in 58.8 s
(clang 22.1.8, -O1 under ASan+UBSan); a caller printing it emitted a
half-megabyte line. #574 called that out ("an explanation 200,000 steps
deep is not an explanation") and deferred it, because capping changes a
documented output contract rather than fixing a defect.

So this changes the contract, deliberately and in the spec first.
equation() takes `maxSteps`, defaulted to kDefaultEquationSteps (100).
Past it a sub-derivation is elided: it renders as `eK` in the formula,
as its value in the substitution, and takes a legend line of its own,
`e1 = 99900 (elided at the 100-step limit)` -- self-describing, and it
names the limit, so a caller meeting an `eK` can tell it from an
ordinary value without reading the spec. The result line is untouched:
eliding changes the account of how a value was reached, never the
value. Same input, same build: 502 characters in 0.057 s.

The limit is a parameter rather than a constant because how much of a
derivation is worth reading belongs to the layer doing the reading, not
to the value type -- an audit log and a tooltip do not want the same
answer. Its default sits at the human end of the range rather than at
the affordable end, because the two errors are not symmetric: a default
too low costs one argument, a default too high costs everyone a line
nobody can read and never knew was unbounded. kEquationStepsUnlimited
restores the old rendering; 0 gives the formatted value alone.

The second half of #582 is the quadratic cost, and it wanted its own
answer. `combine` now appends to its left operand instead of
concatenating both sides into a fresh string, so the left-leaning chain
an accumulate loop records is linear in its depth: rendering 70,000
steps in full went from 27.7 s to 0.11 s under ASan+UBSan, and from
past ctest's 120 s timeout to 1.3 s at -O0 under TSan. That is what
lets the #574 depth regression test keep its evidence -- it now asks
for kEquationStepsUnlimited, because under the default limit the
renderer stops 100 steps in and would pass against the recursive code
it exists to catch. It also retires #590's `[slow]` tag and 600 s
timeout exception: every test is back under one 120 s cap. A
right-leaning chain and a chain of unary negations still copy the big
operand per level and are still quadratic; the step limit bounds those,
and the spec says so.

#602, found on the way and fixed here because the limit depends on it:
assignLabels was the one walk without a visited set, so a node
reachable by k displayed paths was walked k times. On a DAG that is
exponential -- 31 nodes built by repeated `q = q + q` have 2^30 paths
and took 10.3 s to render 33 short lines. Without the set the step
budget would have been spent on repeat visits and the limit would fire
on derivations far smaller than 100 distinct steps.

Measured, clang 22.1.8, -O1 under ASan+UBSan, `total = total + one`
n times then equation(); median of three runs:

    n        before            after (default)   after (unlimited)
    40,000    7.544 s/200,001c  0.020 s/502c      0.074 s/200,001c
    70,000   27.720 s/350,001c  0.024 s/502c      0.112 s/350,001c
    100,000  58.837 s/500,001c  0.057 s/502c      0.157 s/500,001c

The new tests were checked against a mutant whose budget never
exhausts: four assertions fail, all of them the size of the output.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GS5K2vqZtC4xbRiGJHT7jH
Yaraslaut added a commit that referenced this pull request Sep 20, 2026
@Yaraslaut @claude
util: bound what equation() renders, and make rendering it in full ch…
26bfdb8 
…eap (fixes #582, fixes #602) (#603)

A derivation has no bound -- a data-driven `total = total + row` loop
records one step per iteration -- and equation() rendered every one of
them. 100,000 steps produced a 500,001-character first line in 58.8 s
(clang 22.1.8, -O1 under ASan+UBSan); a caller printing it emitted a
half-megabyte line. #574 called that out ("an explanation 200,000 steps
deep is not an explanation") and deferred it, because capping changes a
documented output contract rather than fixing a defect.

So this changes the contract, deliberately and in the spec first.
equation() takes `maxSteps`, defaulted to kDefaultEquationSteps (100).
Past it a sub-derivation is elided: it renders as `eK` in the formula,
as its value in the substitution, and takes a legend line of its own,
`e1 = 99900 (elided at the 100-step limit)` -- self-describing, and it
names the limit, so a caller meeting an `eK` can tell it from an
ordinary value without reading the spec. The result line is untouched:
eliding changes the account of how a value was reached, never the
value. Same input, same build: 502 characters in 0.057 s.

The limit is a parameter rather than a constant because how much of a
derivation is worth reading belongs to the layer doing the reading, not
to the value type -- an audit log and a tooltip do not want the same
answer. Its default sits at the human end of the range rather than at
the affordable end, because the two errors are not symmetric: a default
too low costs one argument, a default too high costs everyone a line
nobody can read and never knew was unbounded. kEquationStepsUnlimited
restores the old rendering; 0 gives the formatted value alone.

The second half of #582 is the quadratic cost, and it wanted its own
answer. `combine` now appends to its left operand instead of
concatenating both sides into a fresh string, so the left-leaning chain
an accumulate loop records is linear in its depth: rendering 70,000
steps in full went from 27.7 s to 0.11 s under ASan+UBSan, and from
past ctest's 120 s timeout to 1.3 s at -O0 under TSan. That is what
lets the #574 depth regression test keep its evidence -- it now asks
for kEquationStepsUnlimited, because under the default limit the
renderer stops 100 steps in and would pass against the recursive code
it exists to catch. It also retires #590's `[slow]` tag and 600 s
timeout exception: every test is back under one 120 s cap. A
right-leaning chain and a chain of unary negations still copy the big
operand per level and are still quadratic; the step limit bounds those,
and the spec says so.

#602, found on the way and fixed here because the limit depends on it:
assignLabels was the one walk without a visited set, so a node
reachable by k displayed paths was walked k times. On a DAG that is
exponential -- 31 nodes built by repeated `q = q + q` have 2^30 paths
and took 10.3 s to render 33 short lines. Without the set the step
budget would have been spent on repeat visits and the limit would fire
on derivations far smaller than 100 distinct steps.

Measured, clang 22.1.8, -O1 under ASan+UBSan, `total = total + one`
n times then equation(); median of three runs:

    n        before            after (default)   after (unlimited)
    40,000    7.544 s/200,001c  0.020 s/502c      0.074 s/200,001c
    70,000   27.720 s/350,001c  0.024 s/502c      0.112 s/350,001c
    100,000  58.837 s/500,001c  0.057 s/502c      0.157 s/500,001c

The new tests were checked against a mutant whose budget never
exhausts: four assertions fail, all of them the size of the output.


Claude-Session: https://claude.ai/code/session_01GS5K2vqZtC4xbRiGJHT7jH

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Yaraslaut Yaraslaut mentioned this pull request Sep 23, 2026
Merged
Yaraslaut added a commit that referenced this pull request Sep 23, 2026
@Yaraslaut @claude
tests: give the strand-race case a budget sized from what it actually…
53960ec 
… costs, instead of the blanket 120 s it was never measured against (fixes #760)

`catch_discover_tests(morph_tests ... PROPERTIES TIMEOUT 120)` applied one
ceiling to all 2966 cases in the binary. The longest, most load-sensitive case
in it was governed by the same number as the cheapest, and that number was
chosen against "every test is sub-second in practice" -- true of the other
2965.

The case is not slow because it computes anything. It serialises 3200 tasks per
iteration through one strand, and every handoff is a thread wakeup that has to
wait its turn on the run queue, so its cost is set by how contended the host is.
Measured on this tree, 12 cores, clang 22.1.8 Release, load generated with plain
spin loops, whole-case wall clock, one run each:

    run queue  1 (idle)  ->    0.136 s
    run queue 14         ->   23.8 s
    run queue 27         ->  170.5 s
    run queue 38         ->  396.4 s     (load average 31-38 at measurement)

Steeply superlinear -- 12 spin loops to 24 costs 7.2x, 24 to 36 another 2.3x --
and **every one of those runs passed**: 40 assertions, `inFlight 1,
maxInFlight 1` throughout. The invariant the case exists for never wavered at
any load. The 120 s ceiling was killing a passing test, which is a false red
and, on shared hardware, eventually a false red on somebody else's PR.

The same measurement with a 120 s cap, for the control:

    load36 run 1: rc=124 wall=120.014 s
        last watchdog: iteration 5, phase 'draining (~StrandExecutor)', 10 s
        into the iteration, completed 1975/3200, inFlight 1, maxInFlight 1
    load36 run 2: rc=124 wall=120.018 s   (reached iteration 6)
    load36 run 3: rc=124 wall=120.021 s   (reached iteration 6)

morph#760 reported four iterations in at 120 s under load average 29-48; six
here at 26-38. Same shape.

## What was not done

`kIterations` is untouched at 20. It is this case's detection power for a rare
interleaving, not a duration knob, and cutting it to fit a ceiling would make
the case cheaper and worse at the only thing it is for. The budget moved
instead. Both constants now say so in the source.

## The budget

`[slow]` is excluded from the blanket and registered again at `TIMEOUT 900` --
the mechanism #589/#590 already established here, because `DISCOVERY_MODE
PRE_TEST` defers discovery to ctest invocation time and `set_tests_properties()`
has nothing to name at configure time.

900 s is ~2.3x the measured 396 s. Extrapolating the curve to a run queue of
~50 -- the top of morph#760's reported load average -- gives roughly 730 s, so
it is ~1.2x that too. It is also the number this same file already uses for its
other load-sensitive case, `forms_schema_generation_is_not_route_count_
sensitive`, chosen there for the same reason.

It is a ceiling, not immunity: past roughly 4.5x oversubscription this will
exceed 900 s as well. What it still catches is a *deadlock*, which is unbounded
rather than merely large -- in 15 minutes instead of 2, for one test out of
3044.

## Verified

The tag is load-bearing, not decorative. `ctest --show-only=json-v1`:

    with    [slow]: 3044 tests, {120.0: 2966, 900.0: 2, 300.0: 2, 60.0: 2},
                    strand case TIMEOUT 900.0
    without [slow]: 3044 tests, {120.0: 2967, 900.0: 1, 300.0: 2, 60.0: 2},
                    strand case TIMEOUT 120.0

Same total either way and zero duplicate names, so splitting the discovery call
in two neither drops a test nor registers one twice. Its neighbour in the same
file, `StrandExecutor keeps one strand per key when a post races the drain`,
stays at 120.

Catch2 tags are never translated into ctest labels anywhere in this repository
(no `catch_discover_tests` call passes `ADD_TAGS_AS_LABELS`; ci.yml says so at
the TSan leg), so `[slow]` cannot change what any `-L`/`-LE` filter selects.

## The budget proved where it has to hold

Full `ctest -j 12` with 24 spin loops competing for the same 12 cores, in four
foreground ranges, load average 17 -> 37 across them:

    -I 1,1000     CTEST_EXIT=0   60 s   100% tests passed out of 1000
    -I 1001,1800  CTEST_EXIT=0  274 s   100% tests passed out of 800
    -I 1801,2600  CTEST_EXIT=0  233 s   100% tests passed out of 800
    -I 2601,3044  CTEST_EXIT=0   30 s   100% tests passed out of 444

and in the range that holds it:

    799/800 Test #1651: StrandExecutor never runs two tasks for one key
    concurrently under contention ... Passed  188.43 sec

188.43 s is a hard timeout at 120 and passes comfortably at 900 -- the defect
and the fix in one line, under the load the issue asks the case to survive.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tests: equation()'s 70000-node stack-safety regression test exceeds ctest's 120s TIMEOUT under TSan, failing master's CI

1 participant

@Yaraslaut