Skip to content

fix: free tau_Re_vf in all viscous runs, not only cyl_coord#1398

Open
sbryngelson wants to merge 1 commit intoMFlowCode:masterfrom
sbryngelson:fix/tau-re-vf-memory-leak
Open

fix: free tau_Re_vf in all viscous runs, not only cyl_coord#1398
sbryngelson wants to merge 1 commit intoMFlowCode:masterfrom
sbryngelson:fix/tau-re-vf-memory-leak

Conversation

@sbryngelson
Copy link
Copy Markdown
Member

Summary

Fixes #1366.

In s_finalize_rhs_module, tau_Re_vf is allocated inside if (viscous) but was only deallocated inside if (cyl_coord). Any Cartesian viscous run (viscous = .true., cyl_coord = .false.) leaked all tau_Re_vf host and GPU allocations.

The fix removes the erroneous if (cyl_coord) guard — the deallocation is already inside the if (viscous) block, so no additional condition is needed.

Change

src/simulation/m_rhs.fpp: drop the if (cyl_coord) then ... end if wrapper around the three tau_re_vf @:DEALLOCATE calls.

Test plan

  • Existing viscous regression tests pass
  • Existing cylindrical-coordinate viscous tests pass (deallocation still fires via the outer if (viscous) guard)

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

Claude Code Review

Head SHA: dcb8a72

Files changed:

  • 1
  • src/simulation/m_rhs.fpp

Findings

Memory management: @:DEALLOCATE guard removed without confirming @:ALLOCATE is unconditional

src/simulation/m_rhs.fpp — deallocation hunk

The if (cyl_coord) guard is removed from three @:DEALLOCATE calls for tau_re_vf:

-                if (cyl_coord) then
-                    do i = 1, num_dims
-                        @:DEALLOCATE(tau_re_vf(eqn_idx%cont%end + i)%sf)
-                    end do
-                    @:DEALLOCATE(tau_re_vf(eqn_idx%E)%sf)
-                    @:DEALLOCATE(tau_re_vf)
-                end if
+                do i = 1, num_dims
+                    @:DEALLOCATE(tau_re_vf(eqn_idx%cont%end + i)%sf)
+                end do
+                @:DEALLOCATE(tau_re_vf(eqn_idx%E)%sf)
+                @:DEALLOCATE(tau_re_vf)

Per CLAUDE.md: "Conditional allocation MUST have conditional deallocation."

The matching @:ALLOCATE calls for tau_re_vf and its .sf components are not shown in this diff. If those allocations are still guarded by if (cyl_coord), then attempting to deallocate these arrays when cyl_coord = .false. will cause a Fortran runtime error (deallocation of an unallocated variable). The change is safe only if the allocation side is also unconditional. Please verify that the @:ALLOCATE calls for tau_re_vf and tau_re_vf(...)%sf are unconditional (or have been made unconditional as part of this fix).

@sbryngelson
Copy link
Copy Markdown
Member Author

The fix is correct. The allocation of tau_Re_vf at line 333 of m_rhs.fpp is guarded only by if (viscous) — there is no cyl_coord guard anywhere near the allocation:

if (viscous) then
    @:ALLOCATE(tau_Re_vf(1:sys_size))
    do i = 1, num_dims
        @:ALLOCATE(tau_Re_vf(eqn_idx%cont%end + i)%sf(...))
    end do
    @:ALLOCATE(tau_Re_vf(eqn_idx%E)%sf(...))
    ...
end if

The old code had the deallocation incorrectly nested inside if (cyl_coord), which had no corresponding guard on the allocation — that asymmetry was the original bug causing a memory leak in Cartesian (cyl_coord = .false.) viscous runs. The fix removes the spurious cyl_coord guard from the deallocation so both allocation and deallocation are symmetric: they both execute whenever viscous = .true., regardless of cyl_coord.

@sbryngelson sbryngelson marked this pull request as ready for review May 6, 2026 14:06
@qodo-code-review
Copy link
Copy Markdown
Contributor

ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 6, 2026

Warning

Rate limit exceeded

@sbryngelson has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 59 minutes and 48 seconds before requesting another review.

To continue reviewing without waiting, purchase usage credits in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7e3d0271-b83a-4878-ad67-c4bb98b2a5ae

📥 Commits

Reviewing files that changed from the base of the PR and between d513442 and dcb8a72.

📒 Files selected for processing (1)
  • src/simulation/m_rhs.fpp

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sbryngelson
Copy link
Copy Markdown
Member Author

Automated Code Review

Summary: No issues found. ✅

Verified:

  • All three tau_Re_vf allocation levels are now symmetrically paired with their deallocations after removing the if (cyl_coord) guard
  • GPU deallocation is fully covered — @:DEALLOCATE macro always emits GPU_EXIT_DATA(delete=...) before the Fortran deallocate
  • No double-free risk: tau_Re_vf is allocated in exactly one place (inside if (viscous)) and deallocated in one matching block
  • The igr path is consistent: both allocation (if (.not. igr)) and deallocation (if (.not. igr)) carry the same guard — igr=true runs unaffected

Minimal, correct fix. Safe to merge.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.77%. Comparing base (d513442) to head (dcb8a72).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1398      +/-   ##
==========================================
+ Coverage   64.75%   64.77%   +0.01%     
==========================================
  Files          71       71              
  Lines       18721    18717       -4     
  Branches     1551     1550       -1     
==========================================
  Hits        12123    12123              
+ Misses       5640     5637       -3     
+ Partials      958      957       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

bug: tau_Re_vf memory leak in Cartesian viscous runs (dealloc only under cyl_coord)

1 participant