Skip to content

Refactor models store#1721

Merged
dgageot merged 5 commits intodocker:mainfrom
dgageot:refactor-models-store
Feb 13, 2026
Merged

Refactor models store#1721
dgageot merged 5 commits intodocker:mainfrom
dgageot:refactor-models-store

Conversation

@dgageot
Copy link
Member

@dgageot dgageot commented Feb 13, 2026

No description provided.

Signed-off-by: David Gageot <david.gageot@docker.com>
Signed-off-by: David Gageot <david.gageot@docker.com>
Signed-off-by: David Gageot <david.gageot@docker.com>
Signed-off-by: David Gageot <david.gageot@docker.com>
Signed-off-by: David Gageot <david.gageot@docker.com>
@dgageot dgageot requested a review from a team as a code owner February 13, 2026 17:00
Copy link

@docker-agent docker-agent bot left a comment

Choose a reason for hiding this comment

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

Review Summary

Found 2 issues in the refactored models store:

  1. HIGH SEVERITY: sync.OnceValues permanently caches errors, preventing retry/fallback logic from working after initial failures
  2. MEDIUM SEVERITY: Context removal prevents cancellation of network/filesystem operations

The sync.OnceValues bug is critical and will cause permanent Store failures after any transient network issue on first access.

s.client = &http.Client{
Timeout: 30 * time.Second,
}
cacheFile := filepath.Join(cacheDir, CacheFileName)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to cache to a file if the duration is the lifetime of the store?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll measure

@dgageot dgageot merged commit af20cc4 into docker:main Feb 13, 2026
14 checks passed
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.

2 participants