Repository navigation
Restart: abort on a truncated restart file instead of reading garbage - #1948
sbryngelson wants to merge 1 commit into
Conversation
A job killed while writing restart_data/lustre_<step>.dat leaves a short file. MPI reads past its end return short without an error, so the next restart silently read the missing tail as garbage (Inf/NaN pressure). Check the file size against the bytes about to be read and abort with a clear message, for both the shared file and file_per_process. Co-Authored-By: Claude <noreply@anthropic.com>
Lines of Code
|
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open findings
n_MOKis computed fromm_glb_readinstead ofn_glb_read. This can make the computed… · New The expression2*nb*nnodeis evaluated in default integer arithmetic before being added to… · NewierrfromMPI_FILE_GET_SIZEis not checked. If this MPI call fails,file_bytesmay be… · New The phrase'holds <file_bytes> of <expected_bytes> expected bytes'is a bit grammatically… · New
What changed in this PR
Adds a restart-file size sanity check to prevent silent short MPI reads from truncated restart files, aborting with a clear error message instead of proceeding with garbage data.
Changes:
- Compute the number of variables per cell to be read (including optional QBMM data) as an MPI-offset-sized integer.
- Check restart file size via
MPI_FILE_GET_SIZEin bothfile_per_processand shared-file branches before reading. - Introduce
s_check_restart_file_sizehelper to centralize truncation detection and abort messaging.
| File | Description |
|---|---|
| src/simulation/m_start_up.fpp | Adds pre-read restart file size validation and a helper routine to abort on truncated restart files. |
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| & MPI_OFFSET_KIND)*nvars_MOK) | ||
|
|
||
| m_MOK = int(m_glb_read + 1, MPI_OFFSET_KIND) | ||
| n_MOK = int(m_glb_read + 1, MPI_OFFSET_KIND) |
| end if | ||
|
|
||
| nvars_MOK = int(sys_size, MPI_OFFSET_KIND) | ||
| if ((bubbles_euler .or. hypoelasticity) .and. qbmm .and. .not. polytropic) nvars_MOK = nvars_MOK + 2*nb*nnode |
| integer :: ierr | ||
| character(len=64) :: sizes | ||
|
|
||
| call MPI_FILE_GET_SIZE(ifile, file_bytes, ierr) |
| write (sizes, '(I0," of ",I0)') file_bytes, expected_bytes | ||
| call s_mpi_abort('Restart file ' // trim(file_loc) // ' holds ' // trim(sizes) // ' expected bytes. It is ' & |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1948 +/- ##
==========================================
+ Coverage 62.64% 62.65% +0.01%
==========================================
Files 86 86
Lines 22425 22445 +20
Branches 3325 3327 +2
==========================================
+ Hits 14048 14064 +16
- Misses 6119 6122 +3
- Partials 2258 2259 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|



Description
A restart file truncated by a job killed while writing it (
restart_data/lustre_<step>.dat) is read without any check, and the run either starts from garbage (in production: Inf/NaN pressure) or dies without saying why.Root cause.
s_read_parallel_data_files(src/simulation/m_start_up.fpp) opens the restart file and readssys_sizevariables (plus the qbmmpb/mvwhen present) at offsets computed from the global grid. It never compares the file size to what it reads. An MPI read past end-of-file returns short without raising an error, so the missing tail comes back as whatever was in the buffer.Fix. The new helper
s_check_restart_file_sizecomparesMPI_FILE_GET_SIZEagainst the bytes about to be read, and aborts with a message naming the file and both sizes. It is called in both the shared-file and thefile_per_processbranches. It only checks for a file that is too short, so files with extra trailing data (e.g. Lagrangianbeta) still read. Intact files are unaffected.Verification
Production (OLCF Frontier). A job killed during a save left a short
lustre_<step>.dat. The next restart read it as Inf pressure. The workaround was to pick the latest restart whose size equals nx·ny·nz·sys_size·8 bytes.This branch. I ran this on a Frontier CPU compute node (GNU 12.3 + Cray MPICH, Release) with a 2D 50x40 case on 2 ranks. I ran to step 20 with saves every 10 steps, truncated
lustre_10.datfrom 80000 to 40000 bytes, and restarted from step 10.Restart file ./restart_data/lustre_10.dat holds 40000 of 80000 expected bytes. It is truncated, e.g. by a job killed while writing it; restart from an earlier step.masterRestarting from the intact file on this branch runs normally.
simulationcompiles with CCE 19 (CPU) and GNU 12.3. Precheck passes, apart from twotest_thermochemcases that fail on the Frontier login node because they compile with the system/usr/bin/gfortran(addressed by #1943). Existing goldens are unaffected, because they read intact files. No regression test is added: the harness cannot express an expected abort.Contribution Policy
We do not accept pull requests generated primarily by AI without genuine understanding or real-world usage context.
All contributions are expected to demonstrate:
If these expectations are not met, we would prefer to implement the changes ourselves rather than spend time reviewing low-effort submissions.
Acknowledgement
This PR was prepared with the assistance of an AI tool (Claude Code). The problem was hit in production runs on Frontier; the fix was exercised on the small case above.
PR template credit: junegunn