Repository navigation
tool_operate: clear the retry flag when the output file fails - #23001
MegaManSec wants to merge 1 commit into
Conversation
|
Analysis of PR #23001 at e5bf1eb3: Test 2270 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 8 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (3, to be specific) so it may be that this failure is rather a systemic issue with this job and not with this specific PR. Generated by Testclutch |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The reproduced failure path lacks an automated regression test.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Fixes retry error handling to prevent reuse and double-closing of a failed output stream.
Changes:
- Clears the retry flag when output-file preparation fails.
- Resets output stream state after closing.
| File | Description |
|---|---|
src/tool_operate.c |
Corrects retry and output-stream cleanup state. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
retrycheck() left *retryp set when the flush/truncate/seek of the partial output file failed, so post_per_transfer() closed the output file and the caller retried the same transfer anyway. The next response was written through the closed FILE*, which was then closed a second time. Clear the flag on those errors and reset the stream after closing it. Add test 1716, with a debug-build hook to make the truncate fail.
e5bf1eb to
ac242f6
Compare
retrycheck()sets*retryp = TRUEbefore flushing/truncating/seeking the partial output file, and returnsCURLE_WRITE_ERRORwithout clearing it if any of those steps fails.post_per_transfer()only skips the close path when!result && *retryp, so it closes the output file, while the caller still seesretry == TRUEand reuses the same transfer.outs->streamis never cleared, sotool_write_cb()writes the next response through the closedFILE*andpost_close_output()closes it again.Clear the flag on those four error paths and reset
outs->stream/fopenedafter closing.Test 1716 covers it. The failure needs a local I/O error, so it uses a DEBUGBUILD-only hook on the truncate, in the style of the existing
CURL_DBG_NO_USE_SSL_ON_FIRST. Without the fix the tool retries and the server sees a second request; with it the transfer ends with (23) after one request. Verified separately with a shim forcingftruncate()to fail with ENOSPC:fclose()is called twice on the sameFILE*, the second time withfileno()returning -1/EBADF. Same in-Z.