arbelonson-source commented on issue #810: URL: https://github.com/apache/arrow-rs-object-store/issues/810#issuecomment-5475829111
Traced this — happy to open a PR if you're okay with it. Disclosure up front: I run agent loops (Claude Code) that find and fix issues like this, so this diagnosis and fix are AI-produced, not something I personally read the code for. Asking before opening a PR rather than doing it unprompted. (I checked the ASF's generative-tooling guidance before starting — following its recommended `Generated-by:` commit trailer below.) **Root cause**: `poll_write`, `poll_flush`, `poll_shutdown` (and the plain-async `put()`) all handle the `Prepare`/`Flush` state's inner future the same way — on `Ok`, transition state and continue; on `Err`, propagate via `?` inside `ready!(...)` (or `.await?`) without ever updating `self.state`. Since `?` short-circuits *before* the state assignment runs, `self.state` is left holding the exact same future that just returned `Ready`. Call `write`/`flush`/`shutdown` again afterward — as your repro's `.close()` after a failed `.write()` does — and that same, already-completed future gets polled a second time, which is exactly what panics with `` `async fn` resumed after completion `` instead of erroring. Verified this matches your report precisely: reproducing your two test cases against current `main` panics at `src/buffered.rs:407:71` — the exact file, line, *and column* your original report gave against 0.13.2, confirming this hasn't shifted since you filed it. **Fix**: added a terminal `BufWriterState::Errored` variant. Every place that used to propagate a `Prepare`/`Flush` error via `?` now instead transitions into `Errored` and returns the error explicitly, and every state-matching function (`poll_write`, `poll_flush`, `poll_shutdown`, `put`, `abort`) has a corresponding `Errored` arm. `abort()` treats `Errored` the same as `Buffer`/`Prepare` (returns `Ok(())`), since a failed `Prepare`/`Flush` future never durably created anything remotely (no multipart upload, no partial single-shot object) that needs cleaning up — flagging that specific choice in case you'd rather it behave differently. **Verified**: added 3 regression tests using your own `FailMultipartInitStore` repro pattern (shutdown-after-error, flush-after-error, abort-after-error) — confirmed all three that exercise the bug panic on unmodified `main` with your exact reported message/location, and pass cleanly with the fix. `abort()`'s test passes on `main` too, since that path already handled this gracefully before my change (only needed a new match arm for exhaustiveness once `Errored` exists). Full `cargo test --lib`: 104 passed, 0 failed. `cargo clippy --lib -- -D warnings` and `cargo fmt --check` both clean. (The workspace's `--all-targets` clippy run hits 4 pre-existing, unrelated `unused_macros`/`unused_imports` errors in `src/lib.rs`'s integration-test macros — confirmed identical on unmodified `main`, so scoped my checks to `--lib` to avoid that noise.) Diff, if useful before a PR exists: https://github.com/apache/arrow-rs-object-store/compare/main...arbelonson-source:arrow-rs-object-store:fix/810-bufwriter-panic-after-error -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
