Repository navigation
fix(csharp): prevent Thrift buffer corruption on cancellation - #706
vuanhphung wants to merge 2 commits into
Conversation
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Medium
Looks good — a well-targeted fix with strong regression coverage. Making the per-poll timeoutCts independent of the stop token correctly prevents Stop()/Dispose() from aborting an in-flight status poll mid-serialization (the root cause of the lost CloseOperation), while the ThrowIfCancellationRequested() pre-check keeps prompt exit when stop precedes serialization, and the 30s timeout still bounds a hung poll. One medium note: disposal now blocks up to the full request timeout when the server is unresponsive (acknowledged in the PR) — raised inline for a human to weigh a bounded drain.
vuanhphung
added
integration-test
|
🚀 Integration tests triggered! View workflow run |
1 similar comment
|
🚀 Integration tests triggered! View workflow run |
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
github-actions
Bot
removed
the
integration-test
|
🔒 Integration test approval reset New commits were pushed to this PR. The A maintainer must re-review the changes and re-add the label to trigger tests again. Why is this necessary?
Latest commit: b6d58d1 |
vuanhphung
changed the title
vuanhphung
added
integration-test
|
🚀 Integration tests triggered! View workflow run |
1 similar comment
|
🚀 Integration tests triggered! View workflow run |
This branch has not been deployed
1 participant
Cancellation during Thrift serialization can leave partial bytes that corrupt the next cleanup RPC, consistent with CLOUDFETCH-014's intermittent failure. The race is reproduced locally; the original CI transport trace was unavailable.
Finish in-memory request writes without cancellation so HTTP flush can reset the buffer. Semaphore waits, flushes, and response reads remain cancellable. Restore linked heartbeat cancellation to avoid waiting for the request timeout on stop, and suppress shutdown-related poll errors.
Validation: 17 regression cases fail before the fix; 116 related unit tests pass. All supported driver targets and net472 tests compile. Static checks pass except Apache RAT, blocked by its artifact download. Live workspace tests and Windows execution were not run.