Inject run config store into WorkloadFromContainerInfo#4342
Merged
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4342 +/- ##
==========================================
- Coverage 68.45% 68.42% -0.04%
==========================================
Files 479 479
Lines 48642 48652 +10
==========================================
- Hits 33300 33288 -12
- Misses 12373 12385 +12
- Partials 2969 2979 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
yrobla
requested changes
Mar 24, 2026
Contributor
yrobla
left a comment
There was a problem hiding this comment.
The DI approach is architecturally correct and cleanly fixes the flaky test root cause. The DoAndReturn changes are exactly right — sharing a single pre-consumed reader across .AnyTimes() calls was the actual bug.
Two things need addressing before merge (marked blocker); two more are nits that can be tackled in a follow-up.
Blockers
loadRunConfigFields:reader.Close()error is silently discarded — diverges from established project patternloadRunConfigFields: TOCTOU betweenstore.Existsandstore.GetReader— a concurrent deletion between the two calls returns an error that was previously handled gracefully as "not found"
Nits (follow-up PR OK)
3. store.Exists and store.GetReader errors are returned without workload name context, making debugging harder
4. TestNewStatusManagerFromRuntime hits the real XDG filesystem via NewRunConfigStore, inconsistent with the mock-based pattern used everywhere else in the suite
WorkloadFromContainerInfo called loadRunConfigFields which internally created a real state.LocalStore hitting the XDG filesystem on every call. This caused flaky tests when parallel tests created or truncated runconfig files, leading to intermittent EOF errors from json.Decode. Add a state.Store parameter to WorkloadFromContainerInfo and loadRunConfigFields so callers inject the store, matching the existing dependency injection pattern in fileStatusManager. Add a runConfigStore field to runtimeStatusManager for parity. Fix file_status_test.go mock readers to use DoAndReturn for fresh readers on each call, preventing EOF when the store is read more than once per test. Fixes #4341 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Check reader.Close error return and break long function signature line to satisfy errcheck and lll linters. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…elds The Exists+GetReader sequence had a race where a concurrent workload deletion between the two calls would return an error instead of the intended empty config. Remove the Exists call entirely and handle not-found from GetReader directly via httperr status code. Also log reader.Close() errors with slog.Warn to match the established project pattern, and wrap errors with workload name for debuggability. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ff81f7d to
f4fc388
Compare
Move store creation out of NewStatusManagerFromRuntime into NewStatusManagerWithEnv so the constructor is pure DI. Tests inject a mock store instead of hitting the real XDG filesystem. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
e7ec9e1 to
4a578a3
Compare
yrobla
approved these changes
Mar 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
WorkloadFromContainerInfocalledloadRunConfigFieldswhich internally created a realstate.LocalStorehitting the XDG filesystem on every call. In CI, parallel tests could create or truncate runconfig files, causing intermittentEOFerrors fromjson.Decode— makingTestRuntimeStatusManager_GetWorkloadflaky.state.Storeparameter toWorkloadFromContainerInfoandloadRunConfigFieldsso callers inject the store, matching the existing DI pattern infileStatusManager. Add arunConfigStorefield toruntimeStatusManagerfor parity.file_status_test.gomock readers to useDoAndReturnfor fresh readers on each call, preventing EOF when the store is read more than once per test.Fixes #4341
Type of change
Test plan
task test)Changes
pkg/workloads/types/types.gostate.Storeparam toWorkloadFromContainerInfoandloadRunConfigFields; rewriteloadRunConfigFieldsto use injected store directlypkg/workloads/types/workload_test.gostoreto updatedWorkloadFromContainerInfopkg/workloads/statuses/status.gorunConfigStorefield toruntimeStatusManager; updateNewStatusManagerFromRuntimeto return error; pass store at 2 call sitespkg/workloads/statuses/file_status.gof.runConfigStoreat 6 call sites; name receiver onmergeHealthyWorkloadDatapkg/workloads/statuses/status_test.goruntimeStatusManager; set upExistsexpectationspkg/workloads/statuses/file_status_test.goReturn(mockReader, nil).AnyTimes()toDoAndReturn(...)for fresh readersDoes this introduce a user-facing change?
No
Special notes for reviewers
The
loadRunConfigFieldsrewrite switches fromstate.LoadRunConfig(which creates a new store internally) to directstore.Exists+store.GetReadercalls. The semantics are identical: returns empty config when not found, propagates errors otherwise — but no longer creates a store on every invocation.Generated with Claude Code