Commit e76ff16
authored
fix(internal): narrow TLS classification to certificate failures only (#206)
## Summary
`FailureClass.hasTlsOrCertificateCause` matched any `SSLException`
**or** `GeneralSecurityException` anywhere in the cause chain. Since all
SDK traffic is HTTPS, every transport error arrives through the TLS
layer — so this swept in transient faults unrelated to certificate
validity and classified them `UNEXPECTED`, pushing data sources into
extended-regime backoff (5 min – 1 hr).
Found during review of #200 by @jsonbailey, who
flagged the breadth but could not run a JVM to confirm what JSSE
actually throws. Confirmed empirically below.
## The problem, measured
Against a real `SSLServerSocket`:
| Scenario | JSSE exception | Old classification |
|---|---|---|
| Peer sends **FIN** mid-handshake | `SSLHandshakeException: Remote host
terminated the handshake`<br>← caused by `EOFException: SSL peer shut
down incorrectly` | **UNEXPECTED** ❌ |
| Peer sends **RST** mid-handshake | `SocketException: Broken pipe` |
NORMAL ✓ |
| Untrusted certificate chain | `SSLHandshakeException` →
`ValidatorException` → `SunCertPathBuilderException` | UNEXPECTED ✓ |
The regime therefore depended on whether an intermediary sent FIN or RST
— an arbitrary implementation detail. Real triggers for
FIN-mid-handshake are all transient: load balancer draining during a
rolling restart, a connection-limit polite close, an idle timeout during
a slow handshake.
**The compounding case is worse than a single stall.** A connection that
flaps faster than the 60 s healthy-operation reset window never
accumulates enough continuous connectivity to reset, so it ratchets 5 m
→ 10 m → 20 m → 40 m → 1 hr and stays there.
## The fix
Match only genuinely long-lived certificate problems:
```java
c instanceof CertificateException // expired, not-yet-valid, hostname mismatch;
// also covers ValidatorException
|| c instanceof CertPathValidatorException // untrusted chain
|| c instanceof CertPathBuilderException
|| c instanceof SSLPeerUnverifiedException // hostname mismatch
```
Verified that a genuinely untrusted chain still classifies `UNEXPECTED`
— JSSE's `ValidatorException` is a `CertificateException` and
`SunCertPathBuilderException` is a `CertPathBuilderException`, so two
links of the real chain match.
## Parity with Go
This aligns Java with the Go server SDK, whose
`classifyTransportFailure` enumerates only certificate errors and treats
everything else as normal:
```go
tls.CertificateVerificationError
x509.UnknownAuthorityError
x509.HostnameError
x509.CertificateInvalidError
// everything else -> FailureClassNormal
```
The previous Java behavior was a divergence from that reference
implementation, not a different reading of the spec.
## Tests
- **Replaces** `sslHandshakeIsUnexpected`, which asserted the over-broad
behavior, with `bareSslHandshakeFailureIsNormal` and
`peerClosedMidHandshakeIsNormal` (the latter reproducing the real
`SSLHandshakeException` → `EOFException` shape).
- **Adds** `certPathValidatorFailureIsUnexpected`,
`certPathBuilderFailureIsUnexpected`,
`certificateNotYetValidIsUnexpected`,
`untrustedChainWrappedInHandshakeExceptionIsUnexpected`,
`sslExceptionFromConnectionResetIsNormal`.
- Full `lib/shared/internal` suite green; `checkstyleMain` clean.
## Test plan for reviewers
- [x] Confirm the four matched types are the right set — in particular
that `CertificateException` is the correct catch-all for validator
failures, and that nothing in Go's four cases lacks a Java counterpart
here.
- [x] Consider whether a bare `SSLHandshakeException` with a
*cipher/protocol* mismatch cause (e.g. `handshake_failure` alert, "No
appropriate protocol") should be `UNEXPECTED`. It is persistent like a
cert problem, but it is not a certificate error and is now classified
`NORMAL`. I left it as `NORMAL` to avoid re-widening, and because a
persistent mismatch keeps retrying at 1–30 s rather than stalling — but
it is a judgment call.
- [x] Sanity-check that no other caller depends on the old broad
behavior.
## Downstream
#200 consumes this classifier. It needs an
internal release (1.11.1) before it can pick this up, in addition to the
`classifyAndLogHttpFailure` rename already noted in review there.
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Overview**
> **Narrows when HTTPS transport failures trigger extended-regime
backoff** by changing `FailureClass.hasTlsOrCertificateCause` to walk
the exception chain for **certificate validation problems only**
(`CertificateException`, `CertPathValidatorException`,
`CertPathBuilderException`, `SSLPeerUnverifiedException`), instead of
any `SSLException` or `GeneralSecurityException`.
>
> Because all SDK traffic is TLS, the old rule treated many
**transient** handshake faults (e.g. peer FIN mid-handshake, bare
`SSLHandshakeException`, connection-reset `SSLException`) as
**UNEXPECTED**, which could push data sources into multi-minute backoff.
Genuine cert issues (expired/not-yet-valid, untrusted chain wrapped in
`SSLHandshakeException`) still classify **UNEXPECTED**.
>
> Tests are updated to match: removed the expectation that every SSL
handshake failure is unexpected, added cases for cert-path errors and
normal transient SSL shapes, and kept wrapped-certificate-cause
coverage.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
fa662f2. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->1 parent 9ddac84 commit e76ff16
2 files changed
Lines changed: 45 additions & 9 deletions
File tree
Lines changed: 8 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | | - | |
| 3 | + | |
4 | 4 | | |
5 | | - | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
6 | 8 | | |
7 | 9 | | |
8 | 10 | | |
| |||
34 | 36 | | |
35 | 37 | | |
36 | 38 | | |
37 | | - | |
38 | | - | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
39 | 43 | | |
40 | 44 | | |
41 | 45 | | |
| |||
Lines changed: 37 additions & 5 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| 5 | + | |
5 | 6 | | |
6 | 7 | | |
7 | 8 | | |
| 9 | + | |
8 | 10 | | |
9 | 11 | | |
10 | 12 | | |
| 13 | + | |
| 14 | + | |
11 | 15 | | |
12 | 16 | | |
| 17 | + | |
13 | 18 | | |
14 | 19 | | |
15 | 20 | | |
| |||
57 | 62 | | |
58 | 63 | | |
59 | 64 | | |
60 | | - | |
61 | | - | |
62 | | - | |
63 | 65 | | |
64 | 66 | | |
65 | 67 | | |
| |||
69 | 71 | | |
70 | 72 | | |
71 | 73 | | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
72 | 104 | | |
73 | 105 | | |
74 | | - | |
75 | | - | |
| 106 | + | |
| 107 | + | |
76 | 108 | | |
77 | 109 | | |
78 | 110 | | |
0 commit comments