cost-analysis-export: Fix resource leaks and apply timeout#5771
cost-analysis-export: Fix resource leaks and apply timeout#5771SebTardif wants to merge 2 commits into
Conversation
Apply the configured EXPORT_TIMEOUT via context.WithTimeout so the entire pipeline has a deadline. Replace http.DefaultClient with a client that has a 2-minute per-request timeout. Close and remove the temp file returned by JoinData in joinAndExportData. Introduced in Azure#5393 (2025-10-24). Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes resource management issues in the cost-analysis-export example by applying the configured timeout and properly cleaning up temp files.
Changes:
- Wrap context with
context.WithTimeoutusingcfg.TimeoutinConfigureAndProcess. - Replace
http.DefaultClientwith a client that has a 2-minute per-request timeout. - Close and remove the temp file returned by
JoinData()injoinAndExportData.
| // Execute request with timeout | ||
| httpClient := &http.Client{Timeout: 2 * time.Minute} | ||
| resp, err := httpClient.Do(req) |
There was a problem hiding this comment.
Good catch. Removed the separate http.Client{Timeout: 2m} and reverted to http.DefaultClient. The request already uses NewRequestWithContext(ctx) which carries cfg.Timeout, so the context deadline handles cancellation without a second undocumented knob.
| _ = os.Remove(file.Name()) | ||
| }() | ||
| slog.Info("data joined successfully", "file", file.Name()) | ||
|
|
There was a problem hiding this comment.
Not an issue here. JoinData() already seeks back to position 0 before returning (line 812: tmpFile.Seek(0, 0)), so the file handle is at the start when UploadFile receives it. The Azure SDK's UploadFile also takes an *os.File and handles positioning internally.
The request already uses NewRequestWithContext(ctx) which carries
cfg.Timeout. A separate http.Client{Timeout: 2m} creates a second
undocumented timeout knob. Use DefaultClient and let the context
deadline handle cancellation.
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
This PR fixes two resource management issues in the cost-analysis-export example:
1. Apply the configured timeout (
EXPORT_TIMEOUT)The
Config.Timeoutfield (env varEXPORT_TIMEOUT, default 10 minutes) is parsed and validated but never used.context.Background()propagates through every downstream call with no deadline, so the entire pipeline (HTTP requests, Azure Blob operations, SQLite queries) can hang indefinitely.This change wraps the context with
context.WithTimeout(ctx, cfg.Timeout)inConfigureAndProcess, and replaceshttp.DefaultClientwith a client that has a 2-minute per-request timeout as a safety net.2. Close temp file and remove on disk in
joinAndExportDataJoinData()creates a temp file and returns it with an open handle. The callerjoinAndExportDatapasses it toUploadFilebut never callsClose()oros.Remove(), leaking a file descriptor and temp file on every run.Introduced in #5393 (2025-10-24).