feat(sdk)!: granular, self-classifying per-surface errors (removes S2Error)

[devin-ai-integration]

Motivation

Downstream consumers (notably feldera/feldera#5728) had to string-match SDK error messages and manually unwrap deep variant chains to decide whether an operation was retryable, whether an append could have taken effect, or whether authentication failed. This replaces the broad S2Error umbrella with granular, self-classifying errors for each SDK surface.

Summary

• Add a canonical s2_sdk::error module that exports all SDK error types plus StatusCode and the typed ErrorCode.
• Return narrow errors by surface: unary metadata operations use RequestError, reads use ReadError, appends use AppendError, sessions use ReadSessionError / AppendSessionError, and producers use ProducerError.
• Remove shared SessionError; read and append sessions now expose only the lifecycle and protocol states they can actually produce.
• Add is_retryable() and has_no_side_effects() across the hierarchy, with conservative mutation-safety semantics and shared server classification helpers.
• Add request_error() and server_error() accessors to wrapper errors, so callers do not need nested pattern matches.
• Preserve unknown server error codes as strings while exposing recognized codes through ErrorResponse::known_code().
• Split response decoding, session protocol, compression, and unclassified failures into distinct ClientError variants.
• Mark public error enums and append condition failures #[non_exhaustive].
• Migrate the CLI and workspace callers to typed codes and accessors.

Breaking changes

• S2Error and SessionError are removed.
• Session errors are no longer re-exported from append_session / read_session; import errors from s2_sdk::error.
• ClientError::Others is renamed to ClientError::Other, with new structured variants for decode, protocol, and compression failures.
• Producer validation is represented directly as ProducerError::Validation.

Display strings are preserved where practical.

Simulator / downstream verification

s2-verification#39 migrates the linearizability harness to the granular API. In particular, it delegates append outcome classification to AppendError::has_no_side_effects() instead of matching server-code strings. This PR pins that verified commit in both the simulator workspace and CI checker build.

Testing

• just fmt
• just clippy
• just test (718 passed)
• just sim-clippy
• RUSTFLAGS="--cfg tokio_unstable" cargo test --manifest-path sim/Cargo.toml --all-targets
• cargo doc -p s2-sdk --all-features --no-deps
• companion repository: cargo fmt --all -- --check, cargo clippy --all-targets --all-features -- -D warnings, and cargo test --all-features --workspace

Part of a set of SDK improvements from reviewing feldera/feldera#5728 (see #654, #655, #658, #659).

Link to Devin session: https://app.devin.ai/sessions/6a1c8bd8dcd144128571f1cab5af0dcb
Requested by: @shikhar

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment, CI, and merge conflict monitoring

[greptile-apps]

Greptile Summary

This PR makes S2Error structured and self-classifying by promoting the internal ClientError to the public API, adding a new SessionError variant for streaming/producer lifecycle conditions, and enriching ErrorResponse with an HTTP status field plus is_retryable()/has_no_side_effects() methods — eliminating the need for downstream callers to string-match on error codes or messages.

• S2Error::Client(String) → S2Error::Client(ClientError) with Display text preserved; ClientError is now pub, Clone, #[non_exhaustive], and re-exported from s2_sdk::types.
• New S2Error::Session(SessionError) variant replaces the old S2Error::Client(other.to_string()) catch-all for heartbeat timeouts, ack timeouts, server disconnects, and producer/session lifecycle errors.
• S2Error and ErrorResponse now expose is_retryable() / has_no_side_effects(), mirroring the existing internal ApiError classification logic.

Confidence Score: 4/5

The breaking-change surface (renamed enum variant, new Session variant, status field on ErrorResponse) is clean and the Display text of all converted errors matches the old strings exactly. All existing internal retry decisions are unaffected because ApiError::is_retryable() is unchanged.

The conversion from internal structured errors to public SessionError variants is mechanically correct and test-covered. Two issues give pause: ErrorResponse::is_retryable() / has_no_side_effects() duplicate the identical logic from ApiError for the Server arm, creating two independently maintained copies that must stay in sync; and SessionError::HeartbeatTimeout.has_no_side_effects() returns false even though heartbeat timeouts are produced exclusively by read sessions which can never have side effects, giving callers inaccurate information.

sdk/src/types.rs — the ErrorResponse retry classification methods and the SessionError::has_no_side_effects implementation deserve a second look before the crate is published.

Important Files Changed

Filename	Overview
sdk/src/types.rs	Core of the PR: adds SessionError, makes S2Error::Client(ClientError) structured, adds ErrorResponse::status + is_retryable()/has_no_side_effects(); retry logic is duplicated from ApiError and HeartbeatTimeout.has_no_side_effects() is conservatively wrong.
sdk/src/api.rs	Adds Clone + #[non_exhaustive] + doc-comments to the existing ClientError enum and re-exports it; no logic changes, existing is_retryable/has_no_side_effects untouched.
sdk/src/session/append.rs	Replaces the catch-all with an explicit match mapping each AppendSessionError variant to the corresponding SessionError; Display strings and retryability semantics are preserved.
sdk/src/session/read.rs	Maps ReadSessionError::HeartbeatTimeout to S2Error::Session(SessionError::HeartbeatTimeout) instead of the old S2Error::Client(string); Display is preserved, retryability is preserved.
sdk/src/producer.rs	Routes all three ProducerError variants to typed S2Error::Session(SessionError::Producer*) instead of S2Error::Client(string); fully exhaustive, Display strings match.
sdk/tests/stream_ops.rs	Updates producer-drop test assertions from string-matching S2Error::Client(msg) to S2Error::Session(SessionError::ProducerDropped); straightforward adaptation.
sdk/tests/metrics_ops.rs	Updates the synthetic S2Error::Client(string) construction to S2Error::Client(ClientError::Others(string)); test intent is unchanged.

Prompt To Fix All With AI

Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
sdk/src/types.rs:3764-3784
**Duplicated server-error retry logic**

`ErrorResponse::is_retryable()` and `ErrorResponse::has_no_side_effects()` reproduce the exact same status-code checks that already exist inside `ApiError::is_retryable()` / `ApiError::has_no_side_effects()` for the `Server` arm. These two implementations are now independent: if the set of retryable status codes changes in `ApiError` (e.g., a new status is added or `transaction_conflict` is removed), `ErrorResponse` will silently diverge and callers using the public API will see different retryability decisions than the internal retry layer.

### Issue 2 of 3
sdk/src/types.rs:3727-3739
**`HeartbeatTimeout` conservatively mis-classifies side effects**

`HeartbeatTimeout` originates exclusively from `ReadSessionError` (read sessions never mutate state), so `has_no_side_effects()` should return `true` for it. As written it returns `false`, which tells callers that a read-session heartbeat timeout may have caused a state change — they may then add unnecessary idempotency guards before retrying. The old code never exposed this question publicly (reads had no `has_no_side_effects()`), so this is a net-new inaccuracy in the public surface.

### Issue 3 of 3
sdk/src/types.rs:3688-3725
**`SessionError` conflates two distinct abstraction layers**

The enum bundles session-level concerns (`HeartbeatTimeout`, `AckTimeout`, `ServerDisconnected`, `StreamClosedEarly`, `SessionClosed/Closing/Dropped`) with producer-level concerns (`ProducerClosed/Closing/Dropped`). A caller matching on a read-session result must either handle producer variants they can never receive or rely on the `#[non_exhaustive]` wildcard. Splitting into `SessionError` and `ProducerError` (or at least grouping with doc-comments) would make the match surface cleaner and the enum self-documenting about which variants can appear in which call paths.

_{Reviews (1): Last reviewed commit: "feat(sdk)!: structured, self-classifying..." | Re-trigger Greptile}

[shikhar]

weird inside a ReadSessionError

[devin-ai-integration]

Agreed — Request is redundant there since ReadError already wraps it. Flattening to ReadSessionError = Read(ReadError) | Session(SessionError).