Skip to content

Conversation

@soluwalana
Copy link
Contributor

@soluwalana soluwalana commented Sep 3, 2025

This MR resolves the following situation:

  • e.record is not Nil
  • res, err := cm.backend.Load(key, v.ID) -> err == ErrNotFound
  • Loop continues without modifying i, addRecord or exportRecord

Also adds a test to the exporter. The added test will hit the infinite loop without the changes in exporter.go.

@soluwalana
Copy link
Contributor Author

soluwalana commented Sep 3, 2025

This should might resolve #6131

@soluwalana
Copy link
Contributor Author

@thaJeztah are these test failures real or flakes?

@thaJeztah
Copy link
Member

Oh; honestly, don't know! I'm not a core maintainer of BuildKit (but have permission in the org, so thought I'd give CI a nudge on your PR to let it run 🤗)

@soluwalana
Copy link
Contributor Author

@thaJeztah Looks like they are flakes, but tests passed now

@soluwalana
Copy link
Contributor Author

soluwalana commented Sep 4, 2025

@tonistiigi, may we have a review on this. We found this issue on our ci build system at nvidia and are running it in a patched version of 0.23.

@soluwalana soluwalana force-pushed the solu/fix-export-infinite-loop branch from dc9b827 to 318b6fc Compare September 4, 2025 18:46
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Looks good, but we don't allow such tests with mocks as they would likely just break in any internal code change. If you can't figure out the conditions for an integration test, then just remove the new test you added.

@soluwalana
Copy link
Contributor Author

@tonistiigi got it, removing the test.

@tonistiigi
Copy link
Member

Squash the commits as well.

@soluwalana
Copy link
Contributor Author

@tonistiigi I uploaded a better test that didn't rely on the mocks. Still correctly tests the ErrNotFound infinite loop. Will Squash the commits now.

- Align record selected for export with the record used for alternatives
- Add integration test for ExportTo ErrNotFound

Signed-off-by: Sam Oluwalana <[email protected]>
@soluwalana soluwalana force-pushed the solu/fix-export-infinite-loop branch from 849bcde to f7639d8 Compare September 4, 2025 20:46
@soluwalana
Copy link
Contributor Author

@tonistiigi / @thaJeztah could one of you restart the tests that couldn't acquire resources?

@tonistiigi tonistiigi closed this Sep 5, 2025
@tonistiigi tonistiigi reopened this Sep 5, 2025
@tonistiigi tonistiigi merged commit 0da2c0d into moby:master Sep 5, 2025
424 of 442 checks passed
@MalteMagnussen
Copy link

When can we expect this to be released as a bug fix patch? :)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants