#349: Fix IO leak and retry with empty body in durable_place#386
#349: Fix IO leak and retry with empty body in durable_place#386VasilevNStas wants to merge 1 commit into
Conversation
|
CI failure note: The All 118 other tests pass, and all non-live checks (rubocop, copyrights, markdown-lint, pdd, reuse, xcop, actionlint, typos, yamllint) pass successfully. |
84d7741 to
8e23d05
Compare
GHX5T-SOL
left a comment
There was a problem hiding this comment.
Requesting changes because the current head no longer uploads zip as a file. It reads the durable into a String and passes that as the zip parameter; the PR's hosted Ubuntu rake then fails test_live_durable_lock_unlock at POST https://api.zerocracy.com/durable-place with Flash: A file must be uploaded / Failure: A file must be uploaded. The same-window master rake run passed test_live_durable_lock_unlock and only failed the known test_live_full_cycle stuck-job case, so this is not just the existing live full-cycle noise.
I also ran a no-network WebMock probe locally: the PR sends decoded zip=hello, world! on both attempts, but as a normal form field. The same probe with the old IO-based implementation still returns success under the new test stub, because test_durable_place_sends_body_on_retry only counts two POSTs and never inspects the uploaded zip part/body. Please keep zip as a file upload object while fixing the close/retry behavior, and make the regression assert that the retried request carries the file payload rather than only that a second POST happened.
| 'pname' => pname, | ||
| 'file' => File.basename(file), | ||
| 'zip' => File.open(file, 'rb') | ||
| 'zip' => content |
There was a problem hiding this comment.
This changes zip from a file upload into a plain String form field. The PR's hosted Ubuntu rake fails test_live_durable_lock_unlock with A file must be uploaded, while the same-window master run passed that live test. The fix needs to preserve a file upload here while closing/reopening it safely.
|
Closing in favor of #385 (branch 357), which solves the same file-descriptor lifetime issue with a more idiomatic block-based approach. Both PRs touch the same area in |
|
@GHX5T-SOL Great work on the review! 🎉 You've earned +13 points (+18 base, -5 for limited comments). Your total score is now +1287 - keep up the momentum and remember to check your Zerocracy account for updates! |
Problem
durable_placepassedFile.open(file, rb)IO directly into the POST params without closing it (IO leak). On retry, the IO was at EOF so the retry posted an empty body while the server still returned a DurableId.Solution
This also eliminates the TOCTOU window between
File.exist?/File.sizechecks andFile.openfor reading — the file is opened once, validated, read, and closed atomically.Verification
bundle exec rubocop— 0 offensestest_durable_place_sends_body_on_retryconfirms retry succeedsCloses #349.