Trace#370
Conversation
Binary Size Analysis
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if traceFile != "" { | ||
| f, err := os.Create(traceFile) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "failed to create trace file: %v\n", err) | ||
| os.Exit(1) | ||
| } | ||
| if err := trace.Start(f); err != nil { | ||
| fmt.Fprintf(os.Stderr, "failed to start trace: %v\n", err) | ||
| os.Exit(1) | ||
| } | ||
| // Use a finalizer or a global cleanup to stop the trace | ||
| // Since Cobra doesn't have a reliable PersistentPostRun for all exit paths (like signals), | ||
| // we'll handle it in specific exit points. | ||
| } |
There was a problem hiding this comment.
trace.Stop()
f is opened in PersistentPreRun but is never stored for later cleanup. trace.Stop() in shutdown.go flushes all trace events to the writer but does not close the underlying file. The handle will eventually be closed by the GC finalizer or OS on process exit, but explicit close is the correct pattern and avoids a dangling descriptor in edge cases (e.g., multiple re-invocations in tests, or OOM scenarios where the GC does not run).
Store the file in a package-level variable and close it after trace.Stop():
var traceFileHandle *os.File
// In PersistentPreRun:
traceFileHandle = f
// In defaultGlobalShutdown (shutdown.go), after trace.Stop():
trace.Stop()
if traceFileHandle != nil {
traceFileHandle.Close()
traceFileHandle = nil
}Prompt To Fix With AI
This is a comment left during a code review.
Path: cmd/root.go
Line: 423-436
Comment:
**Trace file handle not closed after `trace.Stop()`**
`f` is opened in `PersistentPreRun` but is never stored for later cleanup. `trace.Stop()` in `shutdown.go` flushes all trace events to the writer but does not close the underlying file. The handle will eventually be closed by the GC finalizer or OS on process exit, but explicit close is the correct pattern and avoids a dangling descriptor in edge cases (e.g., multiple re-invocations in tests, or OOM scenarios where the GC does not run).
Store the file in a package-level variable and close it after `trace.Stop()`:
```go
var traceFileHandle *os.File
// In PersistentPreRun:
traceFileHandle = f
// In defaultGlobalShutdown (shutdown.go), after trace.Stop():
trace.Stop()
if traceFileHandle != nil {
traceFileHandle.Close()
traceFileHandle = nil
}
```
How can I resolve this? If you propose a fix, please make it concise.
…entation across engine and TUI components
…s for TUI styles and status caches
…set verification test
…nd add pre-warm test coverage
…eConfig field mapping
| testutil.WithFileSize(fileSize), | ||
| testutil.WithRangeSupport(true), | ||
| testutil.WithHandler(func(w http.ResponseWriter, r *http.Request) { | ||
| mu.Lock() | ||
| defer mu.Unlock() | ||
|
|
||
| rng := r.Header.Get("Range") | ||
| if rng == "bytes=0-0" { | ||
| prewarmSeen = true | ||
| } else if rng != "" { | ||
| // Actual download request usually has a real range | ||
| downloadSeen = true | ||
| } | ||
| }), | ||
| ) |
There was a problem hiding this comment.
WithHandler replaces file serving, breaking the download under test
MockServer.handleRequest returns immediately when CustomHandler is set (if m.CustomHandler != nil { m.CustomHandler(w, r); return }), so the custom handler here completely replaces normal file serving. The callback only sets boolean flags and writes no response body, so every request the actual download workers make receives an HTTP 200 with an empty body. Workers will read 0 bytes for each chunk, stall (the Downloaded counter never reaches fileSize), and the 5-second context deadline expires — making downloader.Download return an error and the test fail with t.Fatalf.
The pattern works in switch_429_test.go because that custom handler explicitly writes a 429 status, which is the intended behaviour. Here, the intent is to observe requests while still serving the file normally.
A fix is to add an ObserveHandler option to MockServer that runs the callback before (not instead of) the default file-serving path, or write the full range response inside the custom handler.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/engine/concurrent/prewarm_test.go
Line: 29-43
Comment:
**`WithHandler` replaces file serving, breaking the download under test**
`MockServer.handleRequest` returns immediately when `CustomHandler` is set (`if m.CustomHandler != nil { m.CustomHandler(w, r); return }`), so the custom handler here *completely replaces* normal file serving. The callback only sets boolean flags and writes no response body, so every request the actual download workers make receives an HTTP 200 with an empty body. Workers will read 0 bytes for each chunk, stall (the `Downloaded` counter never reaches `fileSize`), and the 5-second context deadline expires — making `downloader.Download` return an error and the test fail with `t.Fatalf`.
The pattern works in `switch_429_test.go` because that custom handler explicitly writes a 429 status, which is the intended behaviour. Here, the intent is to *observe* requests while still serving the file normally.
A fix is to add an `ObserveHandler` option to `MockServer` that runs the callback before (not instead of) the default file-serving path, or write the full range response inside the custom handler.
How can I resolve this? If you propose a fix, please make it concise.2 participants
Greptile Summary
This PR adds connection pre-warming via hedged dialing (
prewarmConnections), exposes a newDialHedgeCountsetting end-to-end through config, engine types, and the TUI settings view, and refactors TUI initialization frominit()functions to explicitInitializeTUI()/InitializeStyles()/InitializeStatusCache()calls usingsync.Once. A comprehensive test suite accompanies all changes.TestConcurrentDownloader_PrewarmConnectionshas a P1 defect:MockServer.handleRequestreturns immediately whenCustomHandleris set, so the custom handler in this test replaces file-serving entirely. Download workers receive HTTP 200 with no body, stall, and the 5-second context deadline expires — the test will fail.Confidence Score: 4/5
Mostly safe to merge; one P1 test bug should be fixed first to avoid a failing CI gate.
Production code (pre-warming, DialHedgeCount wiring, TUI init refactor) is correct. The P1 finding is confined to a test: TestConcurrentDownloader_PrewarmConnections uses WithHandler in a way that breaks the mock server's file-serving. No runtime or data-integrity risk, but it would block CI and leaves the pre-warming feature without a passing integration test.
internal/engine/concurrent/prewarm_test.go — WithHandler replaces file serving, making the download under test fail.
Important Files Changed
Sequence Diagram
sequenceDiagram participant CLI as cmd/root.go participant Settings as config.Settings participant RC as config.RuntimeConfig participant Conv as types.ConvertRuntimeConfig participant TRC as types.RuntimeConfig participant DL as ConcurrentDownloader participant Prewarm as prewarmConnections CLI->>Settings: getSettings() Settings->>RC: ToRuntimeConfig() [incl. DialHedgeCount] RC->>Conv: ConvertRuntimeConfig(rc) Conv->>TRC: types.RuntimeConfig{DialHedgeCount: ...} CLI->>DL: NewConcurrentDownloader(..., runtime) DL->>DL: Download(ctx, url, mirrors, destPath, fileSize) alt hedgeCount > 0 DL->>Prewarm: prewarmConnections(ctx, client, numRequired, hedgeCount, mirrors) Prewarm-->>DL: N connections warmed in HTTP pool end DL->>DL: workers download chunks using pooled connectionsComments Outside Diff (1)
internal/engine/types/config_convert.go, line 6-21 (link)DialHedgeCountnever propagatedConvertRuntimeConfig(andSettings.ToRuntimeConfiginsettings.go, plus the nil-guard default inNewConcurrentDownloader) all omitDialHedgeCount. BecauseGetDialHedgeCountexplicitly treats zero as "disabled" (not as "use the constant default"), every production-builtRuntimeConfigyieldshedgeCount == 0, and theif hedgeCount > 0guard inDownloadmeansprewarmConnectionsis never called. The constantDialHedgeCount = 4is effectively unreachable via any real code path.Add the field to all three construction sites:
Without this, the entire pre-warming subsystem is dead code for every production download.
Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "test: add exhaustive reflection-based te..." | Re-trigger Greptile