wangchenxuya opened a new pull request, #1160: URL: https://github.com/apache/incubator-seata-go/pull/1160
- [x] I have registered the PR [changes](https://github.com/apache/incubator-seata-go/tree/master/changes). **What this PR does**: Both TM transports stop their retry loop as soon as `backoff.Ongoing()` turns false, which also happens when the caller's context is already done before the first attempt. No request is sent in that case and the send error stays `nil`, and the two end-phase checks each mishandled it: - `Commit`'s `err != nil || bf.Err() != nil` wrapped a nil error with `errors.Wrap`, which returns `nil`, so the caller was told the global transaction had committed while the TC had never received the request. - `Rollback`'s `err != nil && bf.Err() != nil` skipped the branch entirely and then ran `res.(message.GlobalRollbackResponse)` on a nil response. The second phase runs inside the deferred function of `tm.WithGlobalTx`, whose `recover()` has already returned by that point, so the panic reaches the caller. Both transports now share `endphase.Err`, which classifies the outcome in one fixed order: a response that came back, the caller's context, the last transport error, retries exhausted, then a missing response. A context error is wrapped with `%w` so `errors.Is(err, context.Canceled)` holds. The bare type assertions become comma-ok, and the gRPC path also rejects a response whose nested `AbstractGlobalEndResponse` is missing, which protobuf getters would otherwise turn into global status zero. **Which issue(s) this PR fixes**: Fixes #1153 **Special notes for your reviewer**: **A response outranks a done context, deliberately.** `SendSyncRequest` takes no context, so the retry loop can outlive the deadline while the request itself still reached the TC and came back. Reporting a failure there would hide a second phase that actually completed, and it would have regressed `Rollback`, which handles that case correctly on master today. **Verified against a real TC**, a `seataio/seata-server:1.6.1` container: | scenario | master | this PR | |---|---|---| | commit, canceled context | `err=nil`, and the TC only ever saw the begin | `err=context canceled` | | rollback, canceled context | panic | `err=context canceled` | | commit, live context | `err=nil`, GlobalStatus Committed | unchanged | | rollback, live context | `err=nil`, GlobalStatus Rollbacked | unchanged | **Tests.** `endphase` covers the ordering exhaustively. The conformance tests drive both transports through canceled and expired contexts, participant role, empty xid, retries exhausted, a missing response, a mistyped response, an incomplete gRPC response, and the success path. 18 of the subtests fail on master without the production change. `SendSyncRequest` is patched once for the whole test binary in `TestMain`. Applying and resetting a gomonkey patch per subtest does not reliably restore the original method, which let one case's canned response leak into the next; that shape only passed under `-race`, so the single patch is deliberate. **Left out of scope**, both in the same family and probably worth their own issues: `Commit` returns `nil` after delegating to `Rollback` on `tm.IsTimeout(ctx)`, which is another way to report a rollback as a commit; and `Begin` still uses bare type assertions that panic on an unexpected response type. **Does this PR introduce a user-facing change?**: Yes. A global commit that never reached the TC now returns an error instead of `nil`, and a rollback with a done context returns an error instead of panicking. Callers that treated a nil `Commit` error as proof of a commit will now see `context.Canceled` or `context.DeadlineExceeded`. ```release-note Fix the global transaction second phase reporting success for a commit that never reached the TC, and panicking on rollback, when the caller's context is already done. ``` -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
