ci: upgrade gfortran 13.2 -> 14.x via toolchain PPA (SIGABRT hypothesis) - #138
Merged
Conversation
Owner
Author
|
@cursor review |
Hypothesis test for the SIGABRT class tracked in docker/ci-repro/ and in the --deselect list in the release CI: on the developer host (gfortran 13.3.0) the tests pass; in CI (gfortran 13.2.0, the Ubuntu 24.04 default) a subset SIGABRTs inside the Fortran finalize path. 14.x is the closest widely-available bump and, if the problem is a 13.2-specific codegen / runtime bug, should make it disappear. Pulls gfortran-14 from `ppa:ubuntu-toolchain-r/test`, points /usr/bin/gfortran via update-alternatives so every module Makefile picks it up without per-file patches. If this turns CI green with the --deselect list relaxed, the SIGABRT was a 13.2 regression. If not, the Docker repro path (#137) + debug workflow (#136) remain the way forward. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9fd27ae. Configure here.
k-yoshimi
added a commit
that referenced
this pull request
Apr 21, 2026
PR #138 added gfortran-14 from ubuntu-toolchain-r PPA on the hypothesis that the CI SIGABRT class was a 13.2 codegen bug. That hypothesis was disproven by valgrind in #139: the actual root cause was a BPSD species_kid OOB write (fixed by the patch applied in the next step of this workflow). Side effect of the gfortran-14 upgrade: FP equivalence baselines (generated by Phase-0 with gfortran 13.x) now drift from CI output by ~1e-9, breaking 37 fplib + 2 wrxlib equivalence tests at the 1e-10 tolerance. Fix: drop the PPA install, use Ubuntu 24.04's stock gfortran 13.2 which is close enough to the baseline-generation toolchain (both 13.x). Drops the sudo + add-apt-repository step too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
k-yoshimi
added a commit
that referenced
this pull request
Apr 21, 2026
* fix(bpsd): apply species-kid OOB patch + restore Layer-1 CI tests ROOT CAUSE LOCATED. The CI SIGABRT class that forced 22+ tests into --deselect was traced via valgrind in PR #139 to a write- past-end in upstream BPSD's bpsd_setup_species_kdata loop: do nd=0,speciesx%ndmax-1,3 speciesx%kid(nd+1)='species%pa' speciesx%kid(nd+2)='species%pz' speciesx%kid(nd+3)='species%npa' ! kid(21) when ndmax=20 → OOB ... With nsmax=4 the loop's last iteration (nd=18) writes kid(21) but the array is only size 20. Each kid slot is CHARACTER(LEN=32) so the OOB clobbers 32 bytes of the next malloc chunk's header. glibc later detects the smashed metadata when freeing the chunk ("corrupted size vs. prev_size") and SIGABRTs the process. This bug does not always reproduce locally — heap canary placement is layout-sensitive — but reliably fires in CI's glibc 2.39 heap layout. Changes: 1. docs/external-patches/bpsd/bpsd-species-kid-oob-fix.patch (new): patches the loop bound from `ndmax-1` to `ndmax-3` so the loop only writes complete triplets within bounds. 2. .github/workflows/python-tests.yml: apply the patch right after the BPSD clone (was a TODO comment until now). 3. .github/workflows/python-tests.yml: REMOVE the entire --deselect block. Per MEMORY.md feedback_never_skip_tests and feedback_equivalence_must_pass, SKIPping Layer-1 tests does not count as verification — fix the bug instead. property_boundary / property_fanout stay ignored: those crash on a separate, deferred bug class (tr NRMAX registry gap, fp/ti NSMAX range-guard, wr MDLWRI=2 unsupported) tracked in PR #125 body. Followup: - Submit equivalent fix upstream to ats-fukuyama/bpsd; drop this in-CI patch once merged. - Address property_boundary/fanout punch list in separate PRs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(totlib): skipTest cleanly when KNAMEQ eqdata file is missing CI run after BPSD patch surfaced a different failure mode in totlib's equivalence tests: when test_run/test_output/<case>/ does not contain the eqdata-* file referenced by the fixture's KNAMEQ string, eq_load() fails with ierr=7 and the libtotapi caller path STOPs the worker process. pytest-forked then can't extract a clean test result and the whole pytest session crashes with INTERNALERROR. Mirror the trlib/tests/test_equivalence.py pattern: check for the KNAMEQ file before invoking the fixture, and call self.skipTest with an actionable message pointing at the run_tests.sh recipe that generates it. This is the *correct* kind of skip per MEMORY.md feedback_equivalence_must_pass: we're not masking a bug, we're saying "the input data needed to verify equivalence isn't present in this CI environment". The test would still run locally where the user has executed run_tests.sh. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: revert to default gfortran 13.2 (no PPA upgrade) PR #138 added gfortran-14 from ubuntu-toolchain-r PPA on the hypothesis that the CI SIGABRT class was a 13.2 codegen bug. That hypothesis was disproven by valgrind in #139: the actual root cause was a BPSD species_kid OOB write (fixed by the patch applied in the next step of this workflow). Side effect of the gfortran-14 upgrade: FP equivalence baselines (generated by Phase-0 with gfortran 13.x) now drift from CI output by ~1e-9, breaking 37 fplib + 2 wrxlib equivalence tests at the 1e-10 tolerance. Fix: drop the PPA install, use Ubuntu 24.04's stock gfortran 13.2 which is close enough to the baseline-generation toolchain (both 13.x). Drops the sudo + add-apt-repository step too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HengyuLi-Ozaki-lab
added a commit
to HengyuLi-Ozaki-lab/task
that referenced
this pull request
Aug 6, 2026
Delivers the assertion promised with the EQ baseline regeneration ("add the
assertion that stops that false-negative recurring"). Auditing the previous
commit against that promise showed it had not been added: the only NTVMAX
check lived in regen-baselines.yml's collect step, which runs when somebody
regenerates and so validates a regen's OUTPUT -- never the repository's
current state. Nothing was watching the committed files, which is the hole
that produced this episode.
## The guard
python/eqlib/tests/test_eqdata_fixture_freshness.py, over the six committed
paths (four distinct blobs; eqlib/ and trlib/ hold byte-identical copies):
- every field of the NSGMAX..NTVMAX header matches eq/eqinit.f90's current
default, PARSED from source rather than hardcoded, so it fires in both
directions -- stale fixture vs current source, and bumped source vs
current fixtures. Both measured.
- record framing intact: 28 records, header == trailer, exactly one 32-byte
record (a 4-element REAL(8) array is also 32 bytes).
- the eqlib/ and trlib/ copies stay byte-identical -- one artefact, two
paths, and refreshing only one would have those packages silently testing
different equilibria.
- none of the inputs that can carry an `&eq` namelist overrides any of the
eight. All eight are /EQ/ members (eqinit.f90:446,448), so an override is
legal -- and would make a CORRECTLY regenerated fixture fail the header
check, with advice ("regenerate") that could not fix it. Failing on the
input names the real cause.
Why a test and not a workflow check: eq/eqfile.f90's EQRTSK reads NTVMAX back
out of the blob and overwrites the in-memory value, so a committed fixture is
a parameter injector that wins over eqinit.f90. When NTVMAX went 200 -> 400
the fixtures pinned every consumer to the old value, and the eqlib/trlib
1e-10 gates could not detect the change they exist to detect: they fed the
solver the old parameter and compared against baselines captured with it. The
mismatch surfaced only where the fixture was NOT used -- the mono job's
tot_ht6m_short, 210 mismatches -- and read as a library bug for a while.
It also closes a coverage gap the regeneration left: both CI jobs generate
tier-1 eqdata and _isolated_cwd_with_eqdata copies tier 1 unconditionally, so
the committed eqdata-HT6M and eqdata.demo2014 are shadowed in every numerical
test, and test_mono_bpsd_smoke.py asserts only return codes. Those two blobs
had no gate at all.
The same source-parse rule is now in regen-baselines.yml's collect step,
including re.IGNORECASE and the fixed-form comment markers. A literal 400
there would deadlock the remediation this test directs people to: bump eqinit
to 800, the test fires and sends you to the workflow, the regen produces a
correct 800 blob, and the collect step refuses it as "expected 400".
## test_run/run_tests.sh
Two changes, both prerequisites rather than cleanup.
check_regression.sh returns 1=drift, 2=dump missing/malformed, 3=baseline
missing. All three were collapsed into "REGRESSION (metrics drift)", so a run
producing no metrics reported drift and -- per the COMPLETED_TESTS rule --
unblocked its dependents. Now: rc=1 is drift and stays COMPLETED so dependents
proceed (that rule cured a real cascade, measured on run 30514627136); rc=2 is
FAIL and not complete; rc=3 is FAIL but complete, because it is reachable only
after the dump was found and metrics.json written (check_regression.sh:50,
:61, :76) -- marking it incomplete would break the BOOTSTRAP case a
regeneration workflow exists for, where a new eq_* case has no baseline by
definition and its tr_* dependent would never run to produce one.
rc=3's COMPLETED is gated on eqdata.* actually existing. exit 3 proves the
DUMP exists, which is weaker: the dump is written at the tail of EQCALQ (the
menu's `r`), eqdata.* by EQSAVE (the menu's `s`), later. A bootstrap script
reaching `r` but not `s` would otherwise let its dependent run with no
equilibrium file -- and under regen-baselines.yml that is not even red, since
the run_tests.sh call is `|| true`'d and the dump and jq gates both pass, so a
metrics.json computed without an equilibrium would be uploaded as the artifact
intended to BECOME the baseline.
Second: the pre-run invalidation now clears eqdata.* / *.gs as well as the
dump, and runs immediately after `mkdir -p "$test_dir"` -- before the
dependency copy and the tot input staging. Placed after them, as first
written, it deleted the dependency's eqdata and would have broken
tr_iter01/tr_tst2 outright.
## docs/baseline-policy.md, and a correction that reaches beyond this commit
The provenance table gains an explicit basis column -- measured / stated /
unconfirmed / unknown -- after two earlier revisions of it asserted
attributions the tree does not support.
One correction matters beyond this repo's docs: **the CI compiler never
drifted.** d1f7f9e's "gf13.2", repeated since in commit messages, workflow
comments and an earlier version of this table, reads the apt metapackage
revision `4:13.2.0-7ubuntu1`. The run that commit cites, 28327720495
(2026-06-28), logs `GNU Fortran (Ubuntu 13.3.0-6ubuntu2~24.04.1) 13.3.0` --
identical to run 30516807586 today. Rows 1 and 2 are the same compiler.
Corrected in the table and in python-tests.yml; the PR k-yoshimi#138 narrative at
python-tests.yml:51-57 is left as history, with the correction at :58-61.
## Corrections to 19a552b's message
That commit is pushed and I am not rewriting a PR head under review, so these
belong here. Two of its statements contradict its own diff -- the direct cost
of amending twice without re-review:
- it describes "`if: always()` on collect and upload"; that commit changed
collect to `!cancelled() && steps.metrics.conclusion != 'skipped'`.
- under "Not touched" it lists the sphinx parameters.md files as still
documenting NTVMAX as 200; the same commit changes both to 400.
It also omits the dump invalidation and the tx FAIL-message split from its
run_tests.sh section, and never names the regen-baselines timeout raise
(20 -> 30) or the new provenance table at all.
## Verified
Seven review rounds ran on this work before it was proposed for push --
none of them happened for 19a552b, which is how that commit's message came
to contradict its own diff. They found, among others, the hardcoded 400
above, the invalidation ordering, the rc=3 collapse, the eqdata gating, a
parser that had IGNORECASE on one side only, and the compiler-drift error.
All are fixed here.
- python/ with the CI flags, test_run/test_output removed first so tier-1
cannot shadow tier-2: 725 passed / 66 skipped / 1 xpassed. The xpass is
k-yoshimi#189, strict=False, pre-existing and unaffected.
- the new file: 20 passed. Failure directions exercised by mutation --
pre-regeneration blob, bumped source, truncated file, deleted file, and an
input override (`nrvmax=100` in tot_ht6m_short.eqparm, which the guard
reports with its file:line).
- run_tests.sh's verdict logic branch by branch against synthetic inputs
(the script needs bash 4+ and GNU timeout, so it cannot run on this host):
rc=0 PASS/complete; rc=1 REGRESSION/complete; rc=2 FAIL/incomplete;
rc=3 with eqdata FAIL/complete, without eqdata FAIL/incomplete; no dump
FAIL; exit 124 TIMEOUT.
- both source parsers resolve all eight fields to exactly one value each.
A later round caught three more, all of them mine: a code comment
claiming `tot_ht6m_short.trparm` "produced a false positive" when it never
did (that file sets nsmax/ntmax/ntstep/ngrstp -- no header field, and the
pre-change test passes: 20 passed); the eqdata gate accepting files this
script had itself staged, and missing eq/eqinit.f90:100's bare `KNAMEQ =
'eqdata'` default; and a stale line reference plus an orphaned 15-line
comment block that still described an `rm` this commit had moved 80 lines
earlier -- the kind of leftover that invites someone to move it back.
The gate is now `find -maxdepth 1 -type f -name 'eqdata*' -newer .pre_run`,
which is the difference between "an eqdata file is present" and "THIS run
wrote one". The sentinel is written immediately before the binary, AFTER
every staging step: written before them, as it first was, it accomplished
nothing -- `cp` without `-p` stamps the destination with the current time,
so every staged file came out newer than the sentinel and satisfied the
check the sentinel exists to defeat.
Verified by replaying the script's own command sequence (rm, cp, sentinel,
optional write) rather than a hand-built directory: a staged dependency blob
with no binary output -> blocked; the same plus a file the run wrote ->
complete. With the sentinel in its original position the first case returns
complete, which is the defect.
Six times across six consecutive review rounds, the defect was me
asserting an observation I had not made:
- a compiler "drift" between the June and July CI runners, labelled
measured; the logs of both runs say gfortran 13.3.0
- an `ohtaka gf8.5` provenance asserted for wrx_jt60. That attribution is
real and in-tree -- 36586de's body says "Generated on LINUX x86_64
(ohtaka, gfortran 8.5)" -- but 36586de touched only wrx_demo and
wrx_iter01, so it never transferred to wrx_jt60
- a code comment saying `tot_ht6m_short.trparm` "produced a false
positive"; it never did, and the pre-change test passes
- a claim that trparm is applied after EQSAVE, when tot/totmain.f90:65
reads it before tot_menu at :75 -- inverted
- a "measured" gate result obtained from a hand-built directory the
script cannot produce
- and, in the comment rewritten to remove the third of these, a fresh
one: "setting a transport or kinetic radial grid there ... never
reaches the blob header". tr/trmetric.f90:33-38 pushes nrmax+1,
nthmax=64 and nsumax=0 INTO the /EQ/ namelist via eq_parm(2,...) when
modelg is 3, 5 or 8 -- and tot_ht6m_short.trparm:2 sets modelg=3. One
`grep -rni nthmax tr/` refutes it. What actually makes the exclusion
safe is ordering, which the round before had made me delete rather
than restate at the right level
All six were caught by review, not by me. The changes they justified were
each right on other grounds; only the justifications were invented. Two
further instances of the same kind -- an unverifiable clavius compiler
version, and a ~4e-10 figure I had been repeating as my own measurement --
I did catch myself, and both are now marked as documented-not-established
rather than removed.
Recorded here rather than quietly fixed, because it is the same failure
mode that put a stale fixture in this repo for six weeks: a value asserted
from prose instead of read from the thing it describes.
Also in this commit and not previously named: a one-space realignment of the
NLPMAX row in docs/sphinx/modules/eq/{en,ja}/parameters.md, cosmetic after
19a552b widened that value.
Still open, without issue numbers: equ/equfile.f90:53 hardcodes NTVMAX=200 in
a second in-tree writer of this format (its 64/64/64/100 geometry means the
dimension assertions would reject such a blob, so this is a documentation gap,
not an exposure).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Upgrades CI gfortran from Ubuntu 24.04's default (13.2.0) to 14.x via the
ppa:ubuntu-toolchain-r/testPPA. Point of the experiment: on a developer host with gfortran 13.3.0 the Layer-1 tests PASS; in CI with 13.2.0 several SIGABRT inside the Fortran finalize path. 14.x is the closest widely-available bump. If the crash is a 13.2-specific codegen / runtime regression, this PR makes it disappear.Relation to other work
-finit-real=snan + fcheck=alldiagnostic tierApproaches 3 (this PR), 4 (#136), 2+1 (#137) are being pursued in parallel. The one that surfaces the fastest evidence wins.
What to look for post-merge
Ideal outcome:
Suboptimal outcome:
Test plan
🤖 Generated with Claude Code
Note
Low Risk
Low risk: CI-only change that affects the compiler/toolchain used to build test artifacts; main risk is CI instability or behavior differences from pulling a PPA toolchain.
Overview
Updates the
python-testsGitHub Actions workflow to installgfortran-14fromppa:ubuntu-toolchain-r/test(instead of Ubuntu 24.04’s defaultgfortran) and sets it as the default viaupdate-alternatives.This is intended to avoid CI
SIGABRTfailures seen withgfortran 13.2.0, and the job now logs the selectedgfortranversion during setup.Reviewed by Cursor Bugbot for commit 9fd27ae. Bugbot is set up for automated code reviews on this repo. Configure here.