Skip to content

fix: close modsdir snapshot file descriptors#38311

Open
buley wants to merge 2 commits intohashicorp:mainfrom
buley:buley/issue-38302-close-modsdir-snapshot-file
Open

fix: close modsdir snapshot file descriptors#38311
buley wants to merge 2 commits intohashicorp:mainfrom
buley:buley/issue-38302-close-modsdir-snapshot-file

Conversation

@buley
Copy link
Copy Markdown

@buley buley commented Mar 24, 2026

Fixes #38302

WriteSnapshotToDir created the manifest file and returned after writing without closing it. This change closes the file on all paths, preserves close errors with errors.Join, adds a regression test that fails if repeated writes leak descriptors, and includes the required changelog entry for the v1.16 release line.

Target Release

1.16.x

Rollback Plan

  • If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

No changes to security controls.

CHANGELOG entry

  • This change is user-facing and I added a changelog entry.
  • This change is not user-facing.

@buley buley requested a review from a team as a code owner March 24, 2026 18:13
@hashicorp-cla-app
Copy link
Copy Markdown

hashicorp-cla-app Bot commented Mar 24, 2026

CLA assistant check
All committers have signed the CLA.

@crw
Copy link
Copy Markdown
Contributor

crw commented Mar 26, 2026

Hi @buley, thanks for this submission. We typically will only review PRs after a submission is solicited and a solution is discussed with the maintainers in an issue, and for that reason I suspect we will close this PR. However, I will bring this to triage to see how we want to proceed. You can read more in https://github.com/hashicorp/terraform/blob/main/.github/CONTRIBUTING.md. Thanks again.

}

after := countOpenFileDescriptors(t)
if leaked := after - before; leaked > 2 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are 2 file descriptors expected to leak?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate your question, @DanielMSchmidt. The 2 isn't an expected leak count: it's a small tolerance margin to keep the test from being flaky due to unrelated runtime activity. The Go runtime, the test harness, and the logging subsystem may transiently open or close a file descriptor between the before and after snapshots, so a strict leaked > 0 check could produce false positives.

The test writes 32 iterations specifically so that a real leak is unambiguous: without the fix all 32 descriptors leak, which is well above the threshold. With the fix, the count stays at or near zero. So the invariant is really "no meaningful growth" rather than "exactly zero," with 2 as a conservative noise floor.

I could tighten this or use a different approach if you'd prefer — for example, I could check that leaked < iterations with a comment explaining the margin, or switch to a percentage-based threshold.

@buley
Copy link
Copy Markdown
Author

buley commented Apr 3, 2026

I hear you @crw and I know how important contribution guidelines are so I want you to know that I attempted to follow them here. I posted about the issue #38302 and (albeit perhaps overoptimistically) took your engagement there as a green flag.

@crw
Copy link
Copy Markdown
Contributor

crw commented Apr 6, 2026

Hi @buley, we have recently updated our Contributing.md guidelines, if you are using an LLM in any manner as part of this contribution, we now require you to disclose this information. Can you confirm this is or is not employing LLM-assisted coding tools? Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: file descriptor leak in modsdir.WriteSnapshotToDir

3 participants