fix: resource lifecycle and context propagation (lint-monster)#41589
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses resource-lifecycle and context-propagation issues in gh-aw by refactoring several loops to avoid deferring cleanup across iterations, and by ensuring cancellation can flow into external git command execution.
Changes:
- Refactors GitHub API calls in loops to scope
cancel()correctly (function literals / helper extraction). - Introduces
peelTagObjecthelper to avoiddeferusage inside the annotated-tag peel loop. - Switches
git archiveexecution toexec.CommandContext(ctx, ...)to honor context cancellation.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_actions.go | Wraps per-iteration context.WithTimeout + cmd.Output() in a function literal so cancel() runs per iteration. |
| pkg/workflow/action_resolver.go | Extracts annotated-tag peel logic into peelTagObject to keep defer out of the peel loop while preserving timeout behavior. |
| pkg/parser/remote_fetch.go | Uses exec.CommandContext(ctx, ...) for git archive so cancellations/timeouts can terminate the command. |
| pkg/cli/token_usage.go | Extracts per-file JSONL processing to a helper so file.Close() is deferred immediately after open and close errors can be propagated. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/parser/remote_fetch.go:633
exec.CommandContext(ctx, ...)now honors cancellation, but on anycmd.Output()error the code unconditionally falls back todownloadFileViaGitClone(...), which does not accept a context and will keep running even ifctxwas canceled/deadline-exceeded. This undermines the intended cancellation propagation and can make a canceled operation do more work than before.
cmd := exec.CommandContext(ctx, "git", "archive", "--remote="+repoURL, ref, path)
archiveOutput, err := cmd.Output()
if err != nil {
// If git archive fails, try with git clone + git show as a fallback
return downloadFileViaGitClone(owner, repo, path, ref, host)
}
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Low
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. PR #41589 ('fix: resource lifecycle and context propagation (lint-monster)') changed 4 production Go files only: pkg/cli/token_usage.go, pkg/parser/remote_fetch.go, pkg/workflow/action_resolver.go, pkg/workflow/safe_outputs_actions.go. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (99 additions, threshold is >100). |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose, /tdd, and /zoom-out — overall the changes are correct and well-explained; leaving as COMMENT (no blockers).
📋 Key Themes & Highlights
Key Themes
- Missing regression tests: None of the 4 lint fixes are accompanied by tests. The
processOneUsageJSONLFilehelper in particular has complex error-path logic (named returns, deferred close, scanner error priority) that would benefit from table-driven coverage. - Inconsistent refactoring pattern:
action_resolver.goextracts a namedpeelTagObjecthelper (testable, readable ✅);safe_outputs_actions.gouses an inline anonymous function literal (valid, but harder to test and inconsistent with the rest of the PR). - Pre-existing
context.Background()insafe_outputs_actions.go: surfaced clearly by the refactor but not introduced here — worth a follow-up.
Positive Highlights
- ✅ All four root causes are correctly diagnosed and fixed —
exec.CommandContext, helper extraction, and named-return close-error propagation are all idiomatic Go. - ✅
peelTagObjectis a clean extraction: independent timeout per peel, well-commented, error messages retain full context. - ✅
processOneUsageJSONLFilecorrectly preserves the original behavior where scanner errors take priority over close errors. - ✅ PR description is thorough and includes a code snippet explaining the named-return pattern — good for reviewers.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 76.9 AIC · ⌖ 9.98 AIC · ⊞ 6.5K
| } | ||
| // processOneUsageJSONLFile reads a single usage JSONL file and returns the total AIC | ||
| // accumulated from its records. The file is deferred-closed immediately after open. | ||
| func processOneUsageJSONLFile(filePath string) (total float64, found bool, err error) { |
There was a problem hiding this comment.
[/tdd] processOneUsageJSONLFile is unexported with no dedicated test — the named-return + defer close-error wiring is subtle enough that a few table-driven tests would give confidence.
💡 Suggested test cases
func TestProcessOneUsageJSONLFile(t *testing.T) {
t.Run("returns error on missing file", func(t *testing.T) {
_, _, err := processOneUsageJSONLFile("/nonexistent.jsonl")
require.Error(t, err)
})
t.Run("accumulates AIC from valid lines", func(t *testing.T) {
f := writeTempJSONL(t, `{"aic":1.5}`)
total, found, err := processOneUsageJSONLFile(f)
require.NoError(t, err)
assert.True(t, found)
assert.InDelta(t, 1.5, total, 0.001)
})
}Covering open-error, partial-parse, and the happy path would also verify that defer file.Close() named-return wiring works correctly under each exit.
| // querying the GitHub API. It is called iteratively for chained tag objects. | ||
| // The timeout context is created and immediately deferred within this function so | ||
| // that cancel is always called when the function returns (not deferred inside a loop). | ||
| func peelTagObject(ctx context.Context, baseRepo, repo, version, sha string) (newSHA, objType string, err error) { |
There was a problem hiding this comment.
[/diagnose] No regression test covers the annotated-tag peel loop, so the peelTagObject extraction has no safety net. A lint-compliance refactor is a low-risk change, but a test verifying that chained tag peeling still returns the correct commit SHA would lock in the behavior and make future refactors safer.
💡 What to test
A table-driven test against a mock ExecGHContext that returns successive sha/tag TSV rows until sha/commit would cover:
- single annotated tag (depth 1)
- doubly-chained tag (depth 2)
- max-depth exceeded → expected error
- API failure mid-peel → expected wrapped error
This also pins the 30-second-per-peel timeout logic, which is easy to break silently.
| // Wrap the context creation and command execution in a function literal so that | ||
| // defer cancel() runs when the literal returns (not when the outer function returns), | ||
| // avoiding a defer-inside-loop resource-lifecycle issue. | ||
| output, err := func() ([]byte, error) { |
There was a problem hiding this comment.
[/zoom-out] This fix uses an inline anonymous function literal, while action_resolver.go in this same PR extracts a named peelTagObject helper for the same lint issue. A named helper like fetchActionFileContent(apiPath string) ([]byte, error) would be more consistent, and more importantly, directly unit-testable without going through fetchRemoteActionYAML.
💡 Suggested refactor
func fetchActionFileContent(apiPath string) ([]byte, error) {
ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second)
defer cancel()
cmd := ExecGHContext(ctx, "api", apiPath, "--jq", ".content")
return cmd.Output()
}Then at the call site: output, err := fetchActionFileContent(apiPath)
A single pattern across the codebase also makes future lint rules easier to verify mechanically.
| if err != nil { | ||
| return 0, false, fmt.Errorf("failed to open usage JSONL file %s: %w", filePath, err) | ||
| } | ||
| defer func() { |
There was a problem hiding this comment.
[/diagnose] The defer close-error logic silently drops a file.Close() error when scanner.Err() has already set err. This matches the original code's behavior (which also dropped closeErr on scanner error), but it's worth a brief comment so future readers understand the intentional priority.
💡 One-liner clarification
defer func() {
// Close error is only propagated when no other error is already set;
// scanner errors (set explicitly below) take priority.
if closeErr := file.Close(); closeErr != nil && err == nil {
err = fmt.Errorf("failed to close usage JSONL file %s: %w", filePath, closeErr)
}
}()The current code is correct — this is purely a documentation suggestion.
| // defer cancel() runs when the literal returns (not when the outer function returns), | ||
| // avoiding a defer-inside-loop resource-lifecycle issue. | ||
| output, err := func() ([]byte, error) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) |
There was a problem hiding this comment.
[/diagnose] context.Background() is used here (preserved from before this PR), so the 20-second timeout does not respect upstream cancellation from the caller. If fetchRemoteActionYAML is ever called from a context that can be cancelled (e.g., a request with a shorter deadline), the fetch will still run for the full 20 seconds. Consider threading the parent context through if cancellation propagation becomes desirable.
Not introduced by this PR — just surfaced by the refactor making it more visible.
There was a problem hiding this comment.
The defer-in-loop and context-propagation fixes are structurally correct across three of the four changed files. One medium issue blocks the remote_fetch.go fix from actually working as advertised.
### Blocking
remote_fetch.go — exec.CommandContext(ctx, ...) now kills git archive on cancellation, but the error path immediately calls downloadFileViaGitClone(...) with no context. A cancelled context silently becomes an unbounded shallow-clone + temp-dir operation, which is the opposite of what the PR intends.
### Non-blocking
action_resolver.go — peelTagObject uses named returns without any defer that relies on them, which is misleading (named returns + defer is a recognized idiom for close-error propagation; using named returns without that pattern adds noise).
🔎 Code quality review by PR Code Quality Reviewer · 102.5 AIC · ⌖ 6.79 AIC · ⊞ 5.2K
Comments that could not be inline-anchored
pkg/parser/remote_fetch.go:632
Context cancellation is not guarded before the fallback: if exec.CommandContext returned because ctx was already done, this call launches an unbounded downloadFileViaGitClone (temp dir + shallow git clone), defeating the context propagation this PR adds.
<details>
<summary>💡 Suggested fix</summary>
Check ctx.Err() before falling through to the clone path:
if err != nil {
if ctx.Err() != nil {
return nil, fmt.Errorf("git archive cancelled for %s/%s: %w", repo,…
</details>
<details><summary>pkg/workflow/action_resolver.go:243</summary>
**Named returns serve no structural purpose here**: `peelTagObject` has only `defer peelCancel()`, which doesn't interact with return values. The named returns `(newSHA, objType string, err error)` don't enable any defer-close propagation pattern (unlike `processOneUsageJSONLFile` where they're essential). Readers familiar with the named-return+defer idiom may waste time looking for a defer that captures `err`.
<details>
<summary>💡 Suggested fix</summary>
Use unnamed returns and a short vari…
</details>|
Please refresh the branch and rerun checks after the review fixes land.
|
|
@copilot review all comments and address the unresolved review feedback.
|
Four targeted fixes for resource lifecycle and context propagation issues flagged by the custom lint scanner.
Changes
pkg/workflow/action_resolver.go— Extract annotated-tag peel body from theforloop into a newpeelTagObjecthelper sodefer peelCancel()is scoped to the helper, not deferred inside a loop.pkg/workflow/safe_outputs_actions.go— Wrapcontext.WithTimeout+cmd.Output()in an inline function literal sodefer cancel()fires on literal return rather than outer function return, eliminating the defer-in-loop issue.pkg/parser/remote_fetch.go— Replaceexec.Command("git", "archive", ...)withexec.CommandContext(ctx, ...)to propagate cancellation;ctxwas already in scope.pkg/cli/token_usage.go— Extract the per-file body ofsumAICFromUsageJSONLFilesinto aprocessOneUsageJSONLFilehelper thatdefersfile.Close()immediately after open. Named returns preserve close-error propagation: