Repository navigation
Improve logs cache size diagnostics - #59336
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
✅ Ponytail Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ponytail pass focused only on removable complexity.
- One trim opportunity found in the new top-file accounting path; existing behavior can stay while simplifying implementation churn during traversal.
net: -8 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #59336 · codex · gpt53codex · 3.91 AIC · ⌖ 4.2 AIC · ⊞ 13.9K
Comment /ponytail to run again
| } | ||
| return walkErr | ||
| } | ||
| if info == nil || !info.Mode().IsRegular() { |
There was a problem hiding this comment.
pkg/cli/logs_storage_limit.go:L173: shrink: full-sort on every visited file inside Walk. Keep a fixed-size top-K insert (or append+single sort at end) and sort once for output.
There was a problem hiding this comment.
Fixed in the latest commit: computeLogsCacheStats now appends file candidates during the walk and sorts once after the walk completes, instead of re-sorting on every visited file.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
REQUEST_CHANGES
The new cache-file diagnostics introduce a startup-time performance regression in the exact path that runs before downloads begin.
Blocking theme
The implementation sorts the candidate slice on every file visited while walking the logs cache. That turns a simple inspection step into work that grows roughly quadratically with cache size, so the larger the cache gets, the slower gh aw logs --max-storage becomes before it can do anything useful. This should be reworked to collect first and sort once, or to keep a bounded top-N structure.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 16.1 AIC · ⌖ 7.37 AIC · ⊞ 23.5K
Comment /review to run again
| return folders, nil | ||
| } | ||
|
|
||
| func largestLogsFiles(path string, limit int) ([]logsFileSize, int, error) { |
There was a problem hiding this comment.
This adds an avoidable startup-time regression: largestLogsFiles sorts the growing slice on every visited file, so cache inspection degrades toward quadratic work as the logs cache grows.
💡 Why this should block
initialize() now calls this before any download starts, which means every gh aw logs --max-storage run pays the extra cost even when pruning never happens. On a cache with many files, repeatedly sorting inside filepath.Walk is much more expensive than collecting candidates once and sorting after the walk, or maintaining a bounded heap of the top N files.
A safer shape is:
err := filepath.Walk(path, func(filePath string, info os.FileInfo, walkErr error) error {
// collect file metadata only
})
slices.SortFunc(files, compareLogsFileSizeDesc)
if len(files) > limit {
files = files[:limit]
}That keeps the diagnostics deterministic without turning a lightweight startup measurement into an O(n2 log n) pass.
There was a problem hiding this comment.
Fixed: computeLogsCacheStats collects file candidates during the walk and sorts once after the walk, avoiding the O(n² log n) behavior.
There was a problem hiding this comment.
🟡 Changes recommended
The implementation adds an avoidable full cache traversal, and path tie-breaking lacks coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds detailed logs-cache diagnostics to help identify oversized files.
Changes:
- Reports the 20 largest cache files with deterministic ordering and truncation details.
- Adds focused tests and a patch changeset.
File summaries
| File | Description |
|---|---|
pkg/cli/logs_storage_limit.go |
Collects and reports largest cache files. |
pkg/cli/logs_storage_limit_test.go |
Tests limits, paths, and missing directories. |
.changeset/patch-logs-cache-file-diagnostics.md |
Documents the patch release. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| return fmt.Errorf("failed to measure logs cache folders: %w", err) | ||
| } | ||
| l.reportStartingUsage(size, folders) | ||
| files, fileCount, err := largestLogsFiles(l.outputDir, maxReportedLogsCacheFiles) |
There was a problem hiding this comment.
Fixed: introduced computeLogsCacheStats, a single-pass walk that computes total size, per-folder sizes, and the bounded top-file list together, replacing the separate logsDirectorySize/logsFolderSizes/largestLogsFiles traversals.
| if a.size == b.size { | ||
| return cmp.Compare(a.path, b.path) |
There was a problem hiding this comment.
Added TestLargestLogsFilesTieBreaksByPath with equally sized files in reverse-lexical creation order to lock in the ascending path tie-breaker.
There was a problem hiding this comment.
Reviewed the cache diagnostics enhancement in logs_storage_limit.go. Logic is correct and well-tested (sorting, truncation, missing-dir cases covered). One non-blocking performance note left inline: the top-N tracking re-sorts the full slice on every walked file, which is O(n2 log n) for large caches.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 20.4 AIC · ⌖ 13.3 AIC · ⊞ 8.3K
Comments that could not be inline-anchored
pkg/cli/logs_storage_limit.go:68
Performance: this re-sorts the entire files slice on every single file encountered during the walk (slices.SortFunc inside the filepath.Walk callback), giving roughly O(n2 log n) behavior for a directory with n files. For a logs cache with many thousands of files this can be noticeably slow. Since only the top limit entries are ever kept, consider only re-sorting/truncating when len(files) > limit (e.g. compare against the current minimum before appending), or maintain a bounded min-h…
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd to the new largestLogsFiles/reportStartingUsage logic. Two non-blocking suggestions posted inline.
📋 Key Themes & Highlights
Key Themes
- Performance:
largestLogsFilessorts the whole accumulator on every file visited during the walk — O(n2 log n) for large caches, which is exactly the scenario--max-storagediagnostics target. - Test coverage: new tests cover
largestLogsFilessorting/limiting/missing-directory well, but the truncation-message branching inreportStartingUsageisn't directly exercised.
Positive Highlights
- ✅ Deterministic ordering (size desc, then path) is well-tested and matches the PR's stated goal.
- ✅ Missing-directory handling (
os.IsNotExist) mirrors the existinglogsFolderSizes/logsDirectorySizepattern for consistency. - ✅ Changeset included per release process.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 35 AIC · ⌖ 13.7 AIC · ⊞ 10.3K
Comment /matt to run again
| } | ||
| fileCount++ | ||
| files = append(files, logsFileSize{path: relativePath, size: info.Size()}) | ||
| slices.SortFunc(files, func(a, b logsFileSize) int { |
There was a problem hiding this comment.
[/diagnosing-bugs] largestLogsFiles re-sorts the entire accumulator (slices.SortFunc) on every file visited during the walk, giving O(n2 log n) behavior. For a logs cache large enough to need --max-storage diagnostics (the exact scenario this PR targets), this could be thousands of files and noticeably slow down every logs invocation.
💡 Suggested approach
Since only the top limit (20) entries are kept, a cheaper pattern is to only re-sort/trim when the running list actually exceeds limit, or better, maintain a bounded min-heap keyed on size and do a single sort at the end:
var files []logsFileSize
...
files = append(files, logsFileSize{path: relativePath, size: info.Size()})
if len(files) > limit*4 { // keep it from growing unbounded before trimming
slices.SortFunc(files, cmpFn)
files = files[:limit]
}
...
// after Walk completes:
slices.SortFunc(files, cmpFn)
if len(files) > limit {
files = files[:limit]
}This keeps correctness while avoiding a full sort per file for large caches.
@copilot please address this.
There was a problem hiding this comment.
Fixed: computeLogsCacheStats collects candidates during the walk and sorts once at the end.
| ))) | ||
| logsOrchestratorLog.Printf("Logs cache folder size: folder=%q size=%d", folder.name, folder.size) | ||
| } | ||
| if fileCount > len(files) { |
There was a problem hiding this comment.
[/tdd] The new truncation-message branch in reportStartingUsage ("showing %d largest of %d" vs "Logs cache files: %d") isn't directly exercised by a test — the added tests only cover largestLogsFiles, not the reporting output/formatting logic that decides which message to print.
💡 Suggested test
Add a test that calls reportStartingUsage (or an integration-style test through initialize) with fileCount > len(files) and fileCount == len(files), capturing stderr to assert the correct message variant is produced. This locks in the "indicate when truncated" behavior called out in the PR description.
@copilot please address this.
There was a problem hiding this comment.
Added TestReportStartingUsageIndicatesTruncation and TestReportStartingUsageNoTruncation, which call reportStartingUsage directly and assert on the captured stderr output for both message variants.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot please address the outstanding review feedback on cache traversal/performance, refresh the branch if needed, then run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed: |
|
🎉 This pull request is included in a new release. Release: |
gh aw logs --max-storageonly reported aggregate cache-folder sizes, making oversized entries difficult to identify.Cache diagnostics
Coverage
Release
Run: https://github.com/github/gh-aw/actions/runs/34185177055
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
hub.lumenfield.workTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.