From e570235077162de3b72060121686f39dc6cb4df3 Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 08:53:29 -0700 Subject: [PATCH 01/32] docs: design spec for adbd-aware error model redesign Reserve exceptions for transport/connection/auth failures (typed, rooted at IOException) and model expected operation outcomes (install/uninstall/push/pull/root) as result types. Direct-to-adbd only; host-server smart-socket errors are out of scope. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../2026-06-05-dadb-error-model-design.md | 188 ++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 docs/superpowers/specs/2026-06-05-dadb-error-model-design.md diff --git a/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md b/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md new file mode 100644 index 0000000..c6ae063 --- /dev/null +++ b/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md @@ -0,0 +1,188 @@ +# dadb Error Model Redesign — Design Spec + +- **Date:** 2026-06-05 +- **Status:** Proposed (awaiting review) +- **Scope:** Direct-to-adbd error handling only (concern #1). Host-server / smart-socket separation (concern #2) is a **separate** effort and explicitly out of scope here. + +--- + +## 1. Problem + +`dadb` funnels nearly every failure through bare `java.io.IOException`. The only library-specific exception type, `AdbStreamClosed`, is `internal` and uncatchable by consumers. As a result, a caller cannot programmatically distinguish: + +- **"the connection to adbd is dead — reconnect"** (socket reset, handshake failed, auth rejected), from +- **"the connection is fine, but the operation I asked for failed"** (an apk was rejected, a remote file was missing, a command exited non-zero). + +The only way to tell them apart today is string-matching exception messages. Concretely, these throw `IOException` for *operation* failures while the connection is perfectly alive (`Dadb.kt`): install rejected (`:93`, `:150`, `:196`), pm session failures (`:121`, `:123`, `:159`, `:162`), finalize failures (`:145`, `:193`), uninstall non-zero — exit code discarded into a string (`:205`), root/unroot refused (`:227`, `:236`); plus sync `FAIL` for a missing/forbidden remote file (`AdbSync.kt:88`). + +There is also an **inverse** bug: `AdbStreamImpl.nextMessage` (`AdbStream.kt:117-124`) swallows a real socket fault mid-read and converts it into a clean EOF, so a dropped connection during `shell`/`pull` can masquerade as success — the opposite failure, same root cause (an undifferentiated `IOException` channel). + +## 2. Goal & principle + +Make the error model **adbd-aware**: every outcome maps to a distinct, documented place the ADB wire protocol (`packages/modules/adb/protocol.txt`) can produce it. One governing principle: + +> **An exception means "I could not get an outcome — the transport/connection/auth is in trouble." A returned result means "I got an outcome — here is whether the operation succeeded."** + +This mirrors what the most active implementations do: Google's `adblib` (distinct `AdbProtocolErrorException` vs `AdbFailResponseException`; exit codes as values), `adam` (`ShellCommandResult.exitCode`), and Rust `adb_client` (non-zero exit is `Ok(...)`). It also follows JVM convention: root the hierarchy at `IOException` (as `java.net` does with `SocketException`), and treat an expected "no" as data, not a throw (OkHttp's "a response is not an exception"; Kotlin's sealed-result guidance). + +## 3. Scope + +**In scope** — failure modes reachable by talking straight to adbd over the socket: + +| adbd failure mode (`protocol.txt` unless noted) | Maps to | +|---|---| +| TCP connect refused/timeout; `A_CNXN` handshake never completes / version mismatch / unparseable banner | `AdbConnectException` (throw) | +| `A_AUTH` rejected — keys refused or pubkey prompt denied; device stays UNAUTHORIZED | `AdbAuthException` (throw) | +| `A_OPEN` answered with `A_CLSE` — adbd refused to open the stream (bad/forbidden service, device offline). Connection still alive. | `AdbStreamOpenException` (throw) | +| Unexpected socket fault (EOF/RST) **mid-stream** | `AdbConnectionClosedException` (throw) | +| Malformed/unexpected apacket — checksum/magic/length desync | `AdbProtocolException` (throw) | +| `sync:` returns `FAIL` + msg — ENOENT, EACCES (`SYNC.TXT`) | `SyncResult.Failure` (**result**) | +| `shell,v2:` `kIdExit` exit code (`shell_service.cpp`) | `exitCode` in `AdbShellResponse` (**result**, already exists) | + +**Out of scope (deliberately):** +- The host-server **smart-socket** protocol (`SERVICES.TXT`, 4-hex-length `OKAY`/`FAIL`) and its failures (`device offline`, `not found`, `more than one device`). That is the adb-server path (`dadb.adbserver.*`) and belongs to concern #2. adbd never speaks the smart-socket protocol; it has no place in this model. +- Restructuring `discover()`/`list()`/`AdbServer` (concern #2). +- Concurrency/resource fixes found during review (separate work). + +## 4. Exception hierarchy + +All types extend a sealed `AdbException`, which extends `IOException` so **every existing `catch (IOException)` keeps catching them** and existing `@Throws(IOException::class)` clauses remain valid. + +```kotlin +package dadb + +import java.io.IOException + +/** Root of all dadb transport/connection failures. Extends IOException for backward-compat. */ +sealed class AdbException(message: String, cause: Throwable? = null) : IOException(message, cause) + +/** Could not establish the connection: TCP connect/timeout, or the A_CNXN handshake failed. + * Nothing ran — safe to retry by reconnecting. */ +class AdbConnectException(message: String, cause: Throwable? = null) : AdbException(message, cause) + +/** A_AUTH was rejected (keys refused / on-device prompt denied; device UNAUTHORIZED). + * Reconnecting with the same key will not help — fix or accept the key. */ +class AdbAuthException(message: String, cause: Throwable? = null) : AdbException(message, cause) + +/** adbd refused to OPEN the stream (A_OPEN answered with A_CLSE): unknown/forbidden service + * string, or device offline. The underlying connection is still usable. */ +class AdbStreamOpenException(val destination: String, message: String, cause: Throwable? = null) + : AdbException(message, cause) + +/** An established connection/stream died unexpectedly (socket EOF/RST mid-operation). + * Reconnect — but the operation MAY have partially executed; re-check state before retrying + * anything non-idempotent. */ +class AdbConnectionClosedException(message: String, cause: Throwable? = null) + : AdbException(message, cause) + +/** The peer sent a malformed or unexpected apacket (checksum/magic/length desync). There is no + * reliable way to re-sync — the connection must be closed and re-established. */ +class AdbProtocolException(message: String, cause: Throwable? = null) : AdbException(message, cause) +``` + +**On retryability:** we deliberately do **not** add a `retryable: Boolean` flag. A flag would lie in the one case that matters most — `AdbConnectionClosedException` — where adbd's protocol gives no way to know whether a mid-stream `pm install` already committed (gRPC's "only retry if the server never saw the request" problem). Instead, retry semantics are encoded in the **type** (the JDBC `SQLTransientException` / adblib approach) and documented per type: `AdbConnectException` = nothing ran, safe to retry; `AdbConnectionClosedException` = re-check state first. + +### Mapping to current code + +- `AdbConnection.connect` (`AdbConnection.kt`): `IOException("Connection failed", :118)` and the feature-parse `IOException` (`:134`, an unparseable handshake banner) → `AdbConnectException` (both are connection-establishment failures); auth `checkNotNull`/`check` (`:105-106`) → `AdbAuthException`. Raw socket faults from `AdbReader`/`AdbWriter` during the handshake → wrapped as `AdbConnectException(cause = e)`. `AdbProtocolException` is reserved for an unexpected/malformed apacket on an **already-established** connection (desync), not for handshake-time failures. +- `AdbConnection.open` (`AdbConnection.kt:43-55`): when `take(localId, CMD_OKAY)` sees a peer `CLSE` (currently surfaces as the internal `AdbStreamClosed`), throw `AdbStreamOpenException(destination)`. A raw socket fault here → `AdbConnectionClosedException`. +- `AdbStreamImpl.nextMessage` (`AdbStream.kt:117-124`) — the behavioral fix, see §6. +- `AdbStreamClosed` (internal) is retained only as an **internal** signal that the peer closed a stream; it is translated to the appropriate public type at the boundary (open vs mid-stream). It is never thrown to consumers. + +## 5. Result types + +Expected, in-band adbd outcomes become return values. Per-operation sealed types keep each call site's `when` exhaustive and readable. + +```kotlin +sealed interface InstallResult { + object Success : InstallResult + /** `reason` is the raw pm/`cmd package` response (e.g. "Failure [INSTALL_FAILED_...]"). */ + data class Failure(val reason: String) : InstallResult +} + +sealed interface SyncResult { // push() and pull() + object Success : SyncResult + /** `reason` is the adbd sync FAIL message (e.g. "No such file or directory"). */ + data class Failure(val reason: String) : SyncResult +} + +sealed interface CommandResult { // uninstall(), root(), unroot() + object Success : CommandResult + data class Failure(val reason: String) : CommandResult +} +``` + +`shell()`/`openShell()` are unchanged — `AdbShellResponse(output, errorOutput, exitCode)` already carries the exit code as a value and never throws on non-zero. It is the template the rest of the API now follows. + +**Division of labor — results vs throws within an operation.** Low-level stream codecs keep throwing internally; the high-level operation catches the *operation-failure* signal and returns a `Failure`, while letting *transport* exceptions propagate: + +- `AdbSyncStream.send`/`recv` (`AdbSync.kt`) throw a new internal `AdbSyncFailException(message)` for a sync `FAIL`/unexpected-id (replacing the bare `IOException` at `:70`, `:88`, `:90`). `Dadb.push`/`pull` catch it → `SyncResult.Failure(message)`; any `AdbConnectionClosedException`/`AdbProtocolException` propagates untouched. +- `Dadb.install`/`installMultiple` return `InstallResult` by inspecting the `cmd`/`pm` response instead of `throw IOException("Install failed: …")`. +- `Dadb.uninstall`/`root`/`unroot` return `CommandResult` by inspecting the exit code / restart response instead of throwing. + +So every high-level op may still **throw** an `AdbException` (transport/auth died → no outcome), and otherwise **returns** a result (an outcome was obtained, success or failure). + +## 6. Behavioral fix: stop masquerading transport death as EOF + +`AdbStreamImpl.nextMessage` currently swallows *all* `IOException`s into a clean EOF. The fix distinguishes the two cases the protocol actually produces: + +```kotlin +private fun nextMessage(command: Int): AdbMessage? { + return try { + messageQueue.take(localId, command) + } catch (e: AdbStreamClosed) { + // Peer sent A_CLSE: normal end-of-stream (e.g. shell/exec output finished). EOF is correct. + close() + null + } catch (e: IOException) { + // Real socket fault (EOF/RST) mid-stream: the transport died, not a clean close. + close() + throw AdbConnectionClosedException("Connection lost while reading stream $localId", e) + } +} +``` + +A peer `A_CLSE` remains a legitimate EOF (it is how adbd signals a service's output is done); only a genuine socket fault now surfaces as `AdbConnectionClosedException`. This is the one behavioral break in the spec. + +## 7. Per-operation migration table + +| Operation | Today | After | +|---|---|---| +| `open(dest): AdbStream` | throws `IOException` (incl. internal `AdbStreamClosed`) | throws `AdbStreamOpenException` / `AdbConnectException` / `AdbConnectionClosedException` / `AdbAuthException` (all `: IOException`) | +| `shell(cmd): AdbShellResponse` | exit code in result; transport → `IOException` | unchanged; transport → typed `AdbException` | +| `install(...)`: `Unit` | throws `IOException` on rejection | **returns `InstallResult`**; transport → `AdbException` | +| `installMultiple(...)`: `Unit` | throws `IOException` on rejection | **returns `InstallResult`**; transport → `AdbException` | +| `uninstall(pkg)`: `Unit` | throws `IOException` if exit≠0 | **returns `CommandResult`**; transport → `AdbException` | +| `push(...)`: `Unit` | throws `IOException` on sync FAIL | **returns `SyncResult`**; transport → `AdbException` | +| `pull(...)`: `Unit` | throws `IOException` on sync FAIL/missing file | **returns `SyncResult`**; transport → `AdbException` | +| `root()` / `unroot()`: `Unit` | throws `IOException` if refused | **returns `CommandResult`**; transport → `AdbException` | +| `AdbSyncStream.send`/`recv` | throws bare `IOException` | throws internal `AdbSyncFailException` (op) / `AdbException` (transport) | + +`AdbStreamClosed` becomes a non-public translation detail. No public method loses its `@Throws(IOException::class)` clause (every new exception is an `IOException`). + +## 8. Backward compatibility & migration + +- **Source-compatible for `catch`:** all new exceptions extend `IOException`; existing `catch (IOException)` blocks keep working and now receive more specific subtypes. +- **Breaking — return types:** `install`, `installMultiple`, `uninstall`, `push`, `pull`, `root`, `unroot` change from `Unit` to result types. Every caller (notably Maestro) must update call sites to inspect the result. This is the deliberate "clean break" chosen for this redesign. +- **Breaking — behavior:** the `nextMessage` fix (§6) means a dropped connection mid-stream now throws instead of looking like clean EOF. +- **Versioning:** ship as a **major** version bump with release notes enumerating the result-type changes and the EOF-behavior change, plus a short migration guide (`when (result) { … }` snippets). +- **No change** to the wire-protocol classes' on-the-wire behavior (`AdbReader`/`AdbWriter`/`AdbMessageQueue`/`AdbKeyPair`); only how their failures are typed/surfaced. + +## 9. Testing strategy + +- **Unit (no emulator):** drive `AdbConnection.connect` and `open` against in-memory okio `Buffer`s (as `AdbStreamTest` already does) to assert each apacket failure maps to the right type — `A_CNXN` failure → `AdbConnectException`, `A_AUTH` rejection → `AdbAuthException`, `A_OPEN`→`A_CLSE` → `AdbStreamOpenException`, mid-stream socket fault → `AdbConnectionClosedException`, malformed apacket → `AdbProtocolException`. Cover the `nextMessage` split: peer `A_CLSE` → EOF (`read() == -1`), socket fault → throw. +- **Sync codec:** feed a `FAIL`+message packet → assert `AdbSyncStream.recv` throws `AdbSyncFailException` and `Dadb.pull` returns `SyncResult.Failure(message)`. +- **Emulator (existing `DadbTest` style):** install a bad/duplicate apk → `InstallResult.Failure`; uninstall an absent package → `CommandResult.Failure`; pull a missing path → `SyncResult.Failure`; happy paths → `Success`. + +## 10. Future / not now + +- Concern #2: split the direct-adbd module from the adb-server module; make binary-spawning opt-in; give server-backed connections a distinct type. The smart-socket error types (`AdbHostResponseException` carrying `service`/`failMessage`) belong to that effort, not this one. +- Optional `shellOrThrow`-style convenience wrappers, if callers ask for them after the result migration. + +## 11. References + +- AOSP `packages/modules/adb`: `protocol.txt` (A_CNXN/A_AUTH/A_OPEN/A_OKAY/A_WRTE/A_CLSE), `SERVICES.TXT` (smart-socket — out of scope), `SYNC.TXT` (sync FAIL), `shell_service.cpp` (`kIdExit`). +- Google `adblib`: `AdbProtocolErrorException` vs sealed `AdbFailResponseException`; shell-v2 exit code as a flow value. +- `adam` (Malinskiy): `ShellCommandResult.exitCode`; the cautionary conflation of transport-dead and FAIL into one `RequestRejectedException`. +- Rust `adb_client`: `RustADBError` — non-zero exit is `Ok`, FAIL string carried in `ADBRequestFailed`. +- Conventions: Effective Java items 70–73 (checked-for-recoverable, exception translation); JDBC `SQLTransientException`/`SQLNonTransientException`; OkHttp "a response is not an exception"; Kotlin sealed-result guidance (Elizarov). From 41a72059d3a6137c950269d17b209d24b6c669c7 Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 11:55:11 -0700 Subject: [PATCH 02/32] docs: justify error contract on merits, drop compat framing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Major version bump — backward compatibility is not a design constraint. Reframe the IOException base as a semantic choice (recoverable socket faults, consistent with okio surface) rather than a compat hack; tighten to @Throws(AdbException::class) with a no-bare-IOException-leaks guarantee; carry exitCode in CommandResult.Failure instead of discarding it; add Open Questions for result granularity / uninstall shape. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../2026-06-05-dadb-error-model-design.md | 45 +++++++++++++------ 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md b/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md index c6ae063..b2b2931 100644 --- a/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md +++ b/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md @@ -46,14 +46,17 @@ This mirrors what the most active implementations do: Google's `adblib` (distinc ## 4. Exception hierarchy -All types extend a sealed `AdbException`, which extends `IOException` so **every existing `catch (IOException)` keeps catching them** and existing `@Throws(IOException::class)` clauses remain valid. +All types extend a sealed `AdbException`, which extends `IOException`. This is a merit choice, not a compatibility one: these failures genuinely *are* socket/transport I/O faults and they are recoverable (reconnect) — the textbook case for a checked `IOException` (Effective Java item 70), mirroring `java.net`'s `SocketException : IOException`. It is also consistent with our own surface: `AdbStream` exposes okio `Source`/`Sink`, whose reads/writes already throw `IOException`, so a consumer streaming from adbd is already handling `IOException`. + +**Contract guarantee:** no bare `IOException` ever leaks from a public method — every transport fault is wrapped as one of the subtypes below. Public methods that can fail at the transport are therefore annotated `@Throws(AdbException::class)` (a strictly more precise Java signal than the current `@Throws(IOException::class)`). ```kotlin package dadb import java.io.IOException -/** Root of all dadb transport/connection failures. Extends IOException for backward-compat. */ +/** Root of all dadb transport/connection failures. Extends IOException because these are + * genuine, recoverable socket I/O faults, consistent with the okio stream surface. */ sealed class AdbException(message: String, cause: Throwable? = null) : IOException(message, cause) /** Could not establish the connection: TCP connect/timeout, or the A_CNXN handshake failed. @@ -108,10 +111,14 @@ sealed interface SyncResult { // push() and pull() sealed interface CommandResult { // uninstall(), root(), unroot() object Success : CommandResult - data class Failure(val reason: String) : CommandResult + /** `exitCode` is populated for shell-backed ops (uninstall); null for daemon ops (root/unroot) + * that have no exit code. Carrying it avoids the current `Dadb.kt:205` sin of discarding it. */ + data class Failure(val reason: String, val exitCode: Int? = null) : CommandResult } ``` +Failure payloads carry the richest honest detail the operation produced rather than a flattened string. (If, in review, `uninstall` warrants exposing full stdout/stderr, it can return `AdbShellResponse` directly instead — see Open Questions.) + `shell()`/`openShell()` are unchanged — `AdbShellResponse(output, errorOutput, exitCode)` already carries the exit code as a value and never throws on non-zero. It is the template the rest of the API now follows. **Division of labor — results vs throws within an operation.** Low-level stream codecs keep throwing internally; the high-level operation catches the *operation-failure* signal and returns a `Failure`, while letting *transport* exceptions propagate: @@ -142,13 +149,13 @@ private fun nextMessage(command: Int): AdbMessage? { } ``` -A peer `A_CLSE` remains a legitimate EOF (it is how adbd signals a service's output is done); only a genuine socket fault now surfaces as `AdbConnectionClosedException`. This is the one behavioral break in the spec. +A peer `A_CLSE` remains a legitimate EOF (it is how adbd signals a service's output is done); only a genuine socket fault now surfaces as `AdbConnectionClosedException`. The previous blanket swallow was a latent bug — silently presenting a dropped connection as a clean end-of-stream — so this is simply the correct behavior, not a reluctant break. ## 7. Per-operation migration table | Operation | Today | After | |---|---|---| -| `open(dest): AdbStream` | throws `IOException` (incl. internal `AdbStreamClosed`) | throws `AdbStreamOpenException` / `AdbConnectException` / `AdbConnectionClosedException` / `AdbAuthException` (all `: IOException`) | +| `open(dest): AdbStream` | throws `IOException` (incl. internal `AdbStreamClosed`) | throws `AdbStreamOpenException` / `AdbConnectException` / `AdbConnectionClosedException` / `AdbAuthException` (all `AdbException`) | | `shell(cmd): AdbShellResponse` | exit code in result; transport → `IOException` | unchanged; transport → typed `AdbException` | | `install(...)`: `Unit` | throws `IOException` on rejection | **returns `InstallResult`**; transport → `AdbException` | | `installMultiple(...)`: `Unit` | throws `IOException` on rejection | **returns `InstallResult`**; transport → `AdbException` | @@ -158,15 +165,21 @@ A peer `A_CLSE` remains a legitimate EOF (it is how adbd signals a service's out | `root()` / `unroot()`: `Unit` | throws `IOException` if refused | **returns `CommandResult`**; transport → `AdbException` | | `AdbSyncStream.send`/`recv` | throws bare `IOException` | throws internal `AdbSyncFailException` (op) / `AdbException` (transport) | -`AdbStreamClosed` becomes a non-public translation detail. No public method loses its `@Throws(IOException::class)` clause (every new exception is an `IOException`). +`AdbStreamClosed` becomes a non-public translation detail. Public methods that can fail at the transport are annotated `@Throws(AdbException::class)` (precise Java checked signal); methods that return result types only throw `AdbException` when the transport dies before an outcome can be obtained. + +## 8. Migration (major version) + +This ships as a **major** version bump. Backward compatibility is *not* a design constraint — the contract above is chosen on its merits, and consumers opt into the upgrade when ready. This section exists only to make that opt-in straightforward, not to soften the contract. -## 8. Backward compatibility & migration +Breaking changes consumers will see: -- **Source-compatible for `catch`:** all new exceptions extend `IOException`; existing `catch (IOException)` blocks keep working and now receive more specific subtypes. -- **Breaking — return types:** `install`, `installMultiple`, `uninstall`, `push`, `pull`, `root`, `unroot` change from `Unit` to result types. Every caller (notably Maestro) must update call sites to inspect the result. This is the deliberate "clean break" chosen for this redesign. -- **Breaking — behavior:** the `nextMessage` fix (§6) means a dropped connection mid-stream now throws instead of looking like clean EOF. -- **Versioning:** ship as a **major** version bump with release notes enumerating the result-type changes and the EOF-behavior change, plus a short migration guide (`when (result) { … }` snippets). -- **No change** to the wire-protocol classes' on-the-wire behavior (`AdbReader`/`AdbWriter`/`AdbMessageQueue`/`AdbKeyPair`); only how their failures are typed/surfaced. +- **Return types:** `install`, `installMultiple`, `uninstall`, `push`, `pull`, `root`, `unroot` change from `Unit` to result types; call sites must inspect the result (`when (result) { is …Success -> …; is …Failure -> … }`). +- **Exception types:** transport/auth failures are now `AdbException` subtypes instead of bare `IOException`/`IllegalStateException`. Code that switched on exception *messages* should switch on *types* instead. +- **Behavior:** a connection dropped mid-stream now throws `AdbConnectionClosedException` instead of presenting as a clean EOF (§6). + +Release notes will enumerate these with before/after snippets. (Incidental: because `AdbException : IOException`, a coarse `catch (IOException)` still compiles and catches — but that is a consequence of the correct base type, not a goal, and is not how callers should distinguish failures going forward.) + +No on-the-wire behavior of the protocol classes (`AdbReader`/`AdbWriter`/`AdbMessageQueue`/`AdbKeyPair`) changes — only how their failures are typed and surfaced. ## 9. Testing strategy @@ -179,7 +192,13 @@ A peer `A_CLSE` remains a legitimate EOF (it is how adbd signals a service's out - Concern #2: split the direct-adbd module from the adb-server module; make binary-spawning opt-in; give server-backed connections a distinct type. The smart-socket error types (`AdbHostResponseException` carrying `service`/`failMessage`) belong to that effort, not this one. - Optional `shellOrThrow`-style convenience wrappers, if callers ask for them after the result migration. -## 11. References +## 11. Open questions + +- **Result granularity:** per-operation sealed types (`InstallResult`/`SyncResult`/`CommandResult`) vs. a single shared generic result. Per-op chosen for readable exhaustive `when`; revisit if the sprawl isn't worth it. +- **`uninstall` shape:** dedicated `CommandResult` (uniform with `root`/`unroot`) vs. returning `AdbShellResponse` directly (most honest — uninstall *is* a shell command, exposes full stdout/stderr/exitCode). Leaning `CommandResult` for API uniformity; flag for decision. +- **Structured install reasons:** keep `Failure(reason: String)` (the raw pm response) vs. additionally parsing `INSTALL_FAILED_*` codes into an enum. Leaning raw string — parsing is fragile and version-sensitive. + +## 12. References - AOSP `packages/modules/adb`: `protocol.txt` (A_CNXN/A_AUTH/A_OPEN/A_OKAY/A_WRTE/A_CLSE), `SERVICES.TXT` (smart-socket — out of scope), `SYNC.TXT` (sync FAIL), `shell_service.cpp` (`kIdExit`). - Google `adblib`: `AdbProtocolErrorException` vs sealed `AdbFailResponseException`; shell-v2 exit code as a flow value. From c8778444521b524414fd65af850b43a99e80c83c Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 12:05:11 -0700 Subject: [PATCH 03/32] docs: resolve result-type questions from adb source, drop open questions Per the canonical adb client (client/adb_install.cpp does strncmp("Success") and treats failures as opaque text; INSTALL_FAILED_* live in frameworks/base, not adbd): - install reasons stay a raw string (mirror adb, don't import a framework enum) - uninstall returns AdbShellResponse directly (it is a shell command; don't obfuscate adbd's stdout/stderr/exitCode) - root/unroot get a RootResult (adbd service text line, no exit code) - three per-shape result types, not a generic Result Removes the Open Questions section; decisions now folded into the spec. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../2026-06-05-dadb-error-model-design.md | 54 +++++++++++-------- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md b/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md index b2b2931..f1303b8 100644 --- a/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md +++ b/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md @@ -96,28 +96,42 @@ class AdbProtocolException(message: String, cause: Throwable? = null) : AdbExcep Expected, in-band adbd outcomes become return values. Per-operation sealed types keep each call site's `when` exhaustive and readable. +The return type of each operation mirrors **what adbd actually does** for it, so the contract never obfuscates the underlying interaction: + +| Operation | adbd interaction | Return type | +|---|---|---| +| `install` / `installMultiple` | `exec:cmd` / shell; verdict is a `Success` / `Failure[…]` **string** (the `cmd` path has no exit code) | `InstallResult` | +| `push` / `pull` | `sync:` sub-protocol; `FAIL` + message | `SyncResult` | +| `uninstall` | `shell("cmd package uninstall …")` — a **shell command** | `AdbShellResponse` (reused) | +| `root` / `unroot` | `root:` / `unroot:` adbd service; a **text line, no exit code** | `RootResult` | + ```kotlin -sealed interface InstallResult { +sealed interface InstallResult { // install(), installMultiple() object Success : InstallResult - /** `reason` is the raw pm/`cmd package` response (e.g. "Failure [INSTALL_FAILED_...]"). */ + /** Raw pm/`cmd package` verdict, surfaced opaquely — exactly as the canonical adb client does: + * client/adb_install.cpp checks `strncmp("Success", buf, 7)` and prints any other response + * verbatim. The `INSTALL_FAILED_*` codes are a frameworks/base (PackageManager) concept, NOT + * part of the adbd contract, so dadb does not parse them into an enum. */ data class Failure(val reason: String) : InstallResult } -sealed interface SyncResult { // push() and pull() +sealed interface SyncResult { // push(), pull() object Success : SyncResult - /** `reason` is the adbd sync FAIL message (e.g. "No such file or directory"). */ + /** The adbd sync FAIL message, e.g. "No such file or directory" (SYNC.TXT). */ data class Failure(val reason: String) : SyncResult } -sealed interface CommandResult { // uninstall(), root(), unroot() - object Success : CommandResult - /** `exitCode` is populated for shell-backed ops (uninstall); null for daemon ops (root/unroot) - * that have no exit code. Carrying it avoids the current `Dadb.kt:205` sin of discarding it. */ - data class Failure(val reason: String, val exitCode: Int? = null) : CommandResult +sealed interface RootResult { // root(), unroot() + object Success : RootResult + /** The adbd root:/unroot: service response line, e.g. + * "adbd cannot run as root in production builds". This service has no exit code. */ + data class Failure(val reason: String) : RootResult } ``` -Failure payloads carry the richest honest detail the operation produced rather than a flattened string. (If, in review, `uninstall` warrants exposing full stdout/stderr, it can return `AdbShellResponse` directly instead — see Open Questions.) +**`uninstall` returns `AdbShellResponse` directly** rather than a domain wrapper. It *is* `cmd package uninstall` over the shell service, so the honest contract exposes adbd's actual `output`/`errorOutput`/`exitCode`; the caller checks `exitCode == 0`. A `CommandResult.Failure(reason)` wrapper would obfuscate that — discarding stderr and the real process exit code (repeating the `Dadb.kt:205` sin). The one accepted tradeoff: the public type acknowledges uninstall is implemented as a shell command, which is exactly what adbd does. + +**Granularity:** seven result-returning methods collapse to three sealed types (`InstallResult`, `SyncResult`, `RootResult`) plus the reused `AdbShellResponse`. This is deliberately per-*shape*, not a single generic `Result`: the shapes differ because the adbd interactions differ, and a generic wrapper would erase exactly those distinctions. `shell()`/`openShell()` are unchanged — `AdbShellResponse(output, errorOutput, exitCode)` already carries the exit code as a value and never throws on non-zero. It is the template the rest of the API now follows. @@ -125,7 +139,8 @@ Failure payloads carry the richest honest detail the operation produced rather t - `AdbSyncStream.send`/`recv` (`AdbSync.kt`) throw a new internal `AdbSyncFailException(message)` for a sync `FAIL`/unexpected-id (replacing the bare `IOException` at `:70`, `:88`, `:90`). `Dadb.push`/`pull` catch it → `SyncResult.Failure(message)`; any `AdbConnectionClosedException`/`AdbProtocolException` propagates untouched. - `Dadb.install`/`installMultiple` return `InstallResult` by inspecting the `cmd`/`pm` response instead of `throw IOException("Install failed: …")`. -- `Dadb.uninstall`/`root`/`unroot` return `CommandResult` by inspecting the exit code / restart response instead of throwing. +- `Dadb.uninstall` returns the shell command's `AdbShellResponse` unchanged (no throw on non-zero exit). +- `Dadb.root`/`unroot` return `RootResult` by classifying the adbd service's response line instead of throwing. So every high-level op may still **throw** an `AdbException` (transport/auth died → no outcome), and otherwise **returns** a result (an outcome was obtained, success or failure). @@ -159,10 +174,10 @@ A peer `A_CLSE` remains a legitimate EOF (it is how adbd signals a service's out | `shell(cmd): AdbShellResponse` | exit code in result; transport → `IOException` | unchanged; transport → typed `AdbException` | | `install(...)`: `Unit` | throws `IOException` on rejection | **returns `InstallResult`**; transport → `AdbException` | | `installMultiple(...)`: `Unit` | throws `IOException` on rejection | **returns `InstallResult`**; transport → `AdbException` | -| `uninstall(pkg)`: `Unit` | throws `IOException` if exit≠0 | **returns `CommandResult`**; transport → `AdbException` | +| `uninstall(pkg)`: `Unit` | throws `IOException` if exit≠0 | **returns `AdbShellResponse`** (exit code in the value); transport → `AdbException` | | `push(...)`: `Unit` | throws `IOException` on sync FAIL | **returns `SyncResult`**; transport → `AdbException` | | `pull(...)`: `Unit` | throws `IOException` on sync FAIL/missing file | **returns `SyncResult`**; transport → `AdbException` | -| `root()` / `unroot()`: `Unit` | throws `IOException` if refused | **returns `CommandResult`**; transport → `AdbException` | +| `root()` / `unroot()`: `Unit` | throws `IOException` if refused | **returns `RootResult`**; transport → `AdbException` | | `AdbSyncStream.send`/`recv` | throws bare `IOException` | throws internal `AdbSyncFailException` (op) / `AdbException` (transport) | `AdbStreamClosed` becomes a non-public translation detail. Public methods that can fail at the transport are annotated `@Throws(AdbException::class)` (precise Java checked signal); methods that return result types only throw `AdbException` when the transport dies before an outcome can be obtained. @@ -185,22 +200,17 @@ No on-the-wire behavior of the protocol classes (`AdbReader`/`AdbWriter`/`AdbMes - **Unit (no emulator):** drive `AdbConnection.connect` and `open` against in-memory okio `Buffer`s (as `AdbStreamTest` already does) to assert each apacket failure maps to the right type — `A_CNXN` failure → `AdbConnectException`, `A_AUTH` rejection → `AdbAuthException`, `A_OPEN`→`A_CLSE` → `AdbStreamOpenException`, mid-stream socket fault → `AdbConnectionClosedException`, malformed apacket → `AdbProtocolException`. Cover the `nextMessage` split: peer `A_CLSE` → EOF (`read() == -1`), socket fault → throw. - **Sync codec:** feed a `FAIL`+message packet → assert `AdbSyncStream.recv` throws `AdbSyncFailException` and `Dadb.pull` returns `SyncResult.Failure(message)`. -- **Emulator (existing `DadbTest` style):** install a bad/duplicate apk → `InstallResult.Failure`; uninstall an absent package → `CommandResult.Failure`; pull a missing path → `SyncResult.Failure`; happy paths → `Success`. +- **Emulator (existing `DadbTest` style):** install a bad/duplicate apk → `InstallResult.Failure`; uninstall an absent package → `AdbShellResponse` with non-zero `exitCode`; pull a missing path → `SyncResult.Failure`; `unroot` on a production-style build → `RootResult.Failure`; happy paths → `Success`. ## 10. Future / not now - Concern #2: split the direct-adbd module from the adb-server module; make binary-spawning opt-in; give server-backed connections a distinct type. The smart-socket error types (`AdbHostResponseException` carrying `service`/`failMessage`) belong to that effort, not this one. - Optional `shellOrThrow`-style convenience wrappers, if callers ask for them after the result migration. -## 11. Open questions - -- **Result granularity:** per-operation sealed types (`InstallResult`/`SyncResult`/`CommandResult`) vs. a single shared generic result. Per-op chosen for readable exhaustive `when`; revisit if the sprawl isn't worth it. -- **`uninstall` shape:** dedicated `CommandResult` (uniform with `root`/`unroot`) vs. returning `AdbShellResponse` directly (most honest — uninstall *is* a shell command, exposes full stdout/stderr/exitCode). Leaning `CommandResult` for API uniformity; flag for decision. -- **Structured install reasons:** keep `Failure(reason: String)` (the raw pm response) vs. additionally parsing `INSTALL_FAILED_*` codes into an enum. Leaning raw string — parsing is fragile and version-sensitive. - -## 12. References +## 11. References -- AOSP `packages/modules/adb`: `protocol.txt` (A_CNXN/A_AUTH/A_OPEN/A_OKAY/A_WRTE/A_CLSE), `SERVICES.TXT` (smart-socket — out of scope), `SYNC.TXT` (sync FAIL), `shell_service.cpp` (`kIdExit`). +- AOSP `packages/modules/adb`: `protocol.txt` (A_CNXN/A_AUTH/A_OPEN/A_OKAY/A_WRTE/A_CLSE), `SERVICES.TXT` (smart-socket — out of scope), `SYNC.TXT` (sync FAIL), `shell_service.cpp` (`kIdExit`), `client/adb_install.cpp` (`strncmp("Success", buf, 7)` — failure text is opaque, not structured). +- `frameworks/base` `core/java/android/content/pm/PackageManager.java` — the `INSTALL_FAILED_*` constants. Noted only to document that they live in the framework, *not* the adb/adbd contract, which is why dadb does not parse them. - Google `adblib`: `AdbProtocolErrorException` vs sealed `AdbFailResponseException`; shell-v2 exit code as a flow value. - `adam` (Malinskiy): `ShellCommandResult.exitCode`; the cautionary conflation of transport-dead and FAIL into one `RequestRejectedException`. - Rust `adb_client`: `RustADBError` — non-zero exit is `Ok`, FAIL string carried in `ADBRequestFailed`. From 6527832d5c3ce780d755d53f0a9df538f9cb1b23 Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 12:21:42 -0700 Subject: [PATCH 04/32] =?UTF-8?q?docs:=20adopt=20Philosophy=20B=20?= =?UTF-8?q?=E2=80=94=20operation-specific=20result=20types=20throughout?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit install/uninstall (and all named high-level ops) are API peers: each returns its own domain ...Result. uninstall gets UninstallResult, whose Failure carries adbd's real exitCode + output so the wrapper hides nothing. Adds the two-axis consistency rule (transport exceptions universal; outcome results operation-specific, peers sharing a type). shell/openShell stay the generic primitive (AdbShellResponse). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../2026-06-05-dadb-error-model-design.md | 45 ++++++++++++------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md b/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md index f1303b8..a0a39a1 100644 --- a/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md +++ b/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md @@ -94,16 +94,24 @@ class AdbProtocolException(message: String, cause: Throwable? = null) : AdbExcep ## 5. Result types -Expected, in-band adbd outcomes become return values. Per-operation sealed types keep each call site's `when` exhaustive and readable. +Expected, in-band adbd outcomes become return values, never exceptions. Each **named high-level operation returns its own domain result type**, so peer operations are peers in the API — `install` and `uninstall` both return an `…Result`, not one a result and the other a raw shell response. Each `Failure` carries the richest honest detail the underlying adbd interaction produced, so the wrapper never *hides* what adbd did even though it presents a uniform surface. -The return type of each operation mirrors **what adbd actually does** for it, so the contract never obfuscates the underlying interaction: +### Two-axis consistency rule -| Operation | adbd interaction | Return type | -|---|---|---| -| `install` / `installMultiple` | `exec:cmd` / shell; verdict is a `Success` / `Failure[…]` **string** (the `cmd` path has no exit code) | `InstallResult` | -| `push` / `pull` | `sync:` sub-protocol; `FAIL` + message | `SyncResult` | -| `uninstall` | `shell("cmd package uninstall …")` — a **shell command** | `AdbShellResponse` (reused) | -| `root` / `unroot` | `root:` / `unroot:` adbd service; a **text line, no exit code** | `RootResult` | +The `Dadb` methods sit at different distances from adbd — `open()` is 1:1 with an `A_OPEN`; `shell`/`push`/`pull`/`uninstall` are single-service; `install`/`installMultiple`/`root`/`unroot` are composites that orchestrate several adbd ops. Consistency comes from applying one rule across all of them, on two independent axes: + +- **Transport/connection failures are universal.** Every operation can throw the single `AdbException` hierarchy (§4), because every operation ultimately goes through `open()` and the stream. These are *never* operation-specific. +- **Outcome results are operation-specific.** Each named operation gets its own `…Result { Success | Failure(…) }`; peers share one type. A composite folds any constituent failure into *its own* verdict rather than leaking a lower-level result (e.g. an `install` whose `pm`-path push sync-FAILs returns `InstallResult.Failure`, not a `SyncResult`). + +This is the deliberate choice of API-surface uniformity ("Philosophy B") over mechanism-mirroring: `uninstall` gets a domain type even though it is one shell command, accepting a small amount of wrapping in exchange for `install`/`uninstall` being true peers. The non-obfuscation constraint is preserved by carrying adbd's real detail in the `Failure` payload. + +| Operation(s) | Result type | +|---|---| +| `install`, `installMultiple` | `InstallResult` | +| `uninstall` | `UninstallResult` | +| `push`, `pull` | `SyncResult` | +| `root`, `unroot` | `RootResult` | +| `shell`, `openShell` | `AdbShellResponse` / `AdbShellStream` (generic primitive — see below) | ```kotlin sealed interface InstallResult { // install(), installMultiple() @@ -115,6 +123,15 @@ sealed interface InstallResult { // install(), installMultiple() data class Failure(val reason: String) : InstallResult } +sealed interface UninstallResult { // uninstall() + object Success : UninstallResult + /** `uninstall` is `cmd package uninstall` over the shell service. Failure preserves adbd's + * actual process `exitCode` and combined output (`reason`) so the domain wrapper hides + * nothing — the whole point of choosing a wrapper here is peer-symmetry with install, not + * concealment. */ + data class Failure(val reason: String, val exitCode: Int) : UninstallResult +} + sealed interface SyncResult { // push(), pull() object Success : SyncResult /** The adbd sync FAIL message, e.g. "No such file or directory" (SYNC.TXT). */ @@ -129,17 +146,15 @@ sealed interface RootResult { // root(), unroot() } ``` -**`uninstall` returns `AdbShellResponse` directly** rather than a domain wrapper. It *is* `cmd package uninstall` over the shell service, so the honest contract exposes adbd's actual `output`/`errorOutput`/`exitCode`; the caller checks `exitCode == 0`. A `CommandResult.Failure(reason)` wrapper would obfuscate that — discarding stderr and the real process exit code (repeating the `Dadb.kt:205` sin). The one accepted tradeoff: the public type acknowledges uninstall is implemented as a shell command, which is exactly what adbd does. - -**Granularity:** seven result-returning methods collapse to three sealed types (`InstallResult`, `SyncResult`, `RootResult`) plus the reused `AdbShellResponse`. This is deliberately per-*shape*, not a single generic `Result`: the shapes differ because the adbd interactions differ, and a generic wrapper would erase exactly those distinctions. +**Granularity:** the named operations collapse to four domain types (`InstallResult`, `UninstallResult`, `SyncResult`, `RootResult`), peers sharing one type. Not a single generic `Result` — the `Failure` payloads genuinely differ (uninstall carries an exit code; the others do not), and a generic wrapper would erase those distinctions. -`shell()`/`openShell()` are unchanged — `AdbShellResponse(output, errorOutput, exitCode)` already carries the exit code as a value and never throws on non-zero. It is the template the rest of the API now follows. +**`shell`/`openShell` stay `AdbShellResponse`/`AdbShellStream`.** These are the *generic shell primitive* callers build their own commands on, not a named package-lifecycle operation, so they expose the shell service's outcome directly (`exitCode` as a value, never thrown). `shell` is the tool; `install`/`uninstall`/etc. are the abstractions built with it. **Division of labor — results vs throws within an operation.** Low-level stream codecs keep throwing internally; the high-level operation catches the *operation-failure* signal and returns a `Failure`, while letting *transport* exceptions propagate: - `AdbSyncStream.send`/`recv` (`AdbSync.kt`) throw a new internal `AdbSyncFailException(message)` for a sync `FAIL`/unexpected-id (replacing the bare `IOException` at `:70`, `:88`, `:90`). `Dadb.push`/`pull` catch it → `SyncResult.Failure(message)`; any `AdbConnectionClosedException`/`AdbProtocolException` propagates untouched. - `Dadb.install`/`installMultiple` return `InstallResult` by inspecting the `cmd`/`pm` response instead of `throw IOException("Install failed: …")`. -- `Dadb.uninstall` returns the shell command's `AdbShellResponse` unchanged (no throw on non-zero exit). +- `Dadb.uninstall` runs the shell command and maps the `AdbShellResponse` into `UninstallResult` — `exitCode == 0` → `Success`, else `Failure(reason = response.allOutput, exitCode = response.exitCode)`. - `Dadb.root`/`unroot` return `RootResult` by classifying the adbd service's response line instead of throwing. So every high-level op may still **throw** an `AdbException` (transport/auth died → no outcome), and otherwise **returns** a result (an outcome was obtained, success or failure). @@ -174,7 +189,7 @@ A peer `A_CLSE` remains a legitimate EOF (it is how adbd signals a service's out | `shell(cmd): AdbShellResponse` | exit code in result; transport → `IOException` | unchanged; transport → typed `AdbException` | | `install(...)`: `Unit` | throws `IOException` on rejection | **returns `InstallResult`**; transport → `AdbException` | | `installMultiple(...)`: `Unit` | throws `IOException` on rejection | **returns `InstallResult`**; transport → `AdbException` | -| `uninstall(pkg)`: `Unit` | throws `IOException` if exit≠0 | **returns `AdbShellResponse`** (exit code in the value); transport → `AdbException` | +| `uninstall(pkg)`: `Unit` | throws `IOException` if exit≠0 | **returns `UninstallResult`** (`Failure` carries `reason` + `exitCode`); transport → `AdbException` | | `push(...)`: `Unit` | throws `IOException` on sync FAIL | **returns `SyncResult`**; transport → `AdbException` | | `pull(...)`: `Unit` | throws `IOException` on sync FAIL/missing file | **returns `SyncResult`**; transport → `AdbException` | | `root()` / `unroot()`: `Unit` | throws `IOException` if refused | **returns `RootResult`**; transport → `AdbException` | @@ -200,7 +215,7 @@ No on-the-wire behavior of the protocol classes (`AdbReader`/`AdbWriter`/`AdbMes - **Unit (no emulator):** drive `AdbConnection.connect` and `open` against in-memory okio `Buffer`s (as `AdbStreamTest` already does) to assert each apacket failure maps to the right type — `A_CNXN` failure → `AdbConnectException`, `A_AUTH` rejection → `AdbAuthException`, `A_OPEN`→`A_CLSE` → `AdbStreamOpenException`, mid-stream socket fault → `AdbConnectionClosedException`, malformed apacket → `AdbProtocolException`. Cover the `nextMessage` split: peer `A_CLSE` → EOF (`read() == -1`), socket fault → throw. - **Sync codec:** feed a `FAIL`+message packet → assert `AdbSyncStream.recv` throws `AdbSyncFailException` and `Dadb.pull` returns `SyncResult.Failure(message)`. -- **Emulator (existing `DadbTest` style):** install a bad/duplicate apk → `InstallResult.Failure`; uninstall an absent package → `AdbShellResponse` with non-zero `exitCode`; pull a missing path → `SyncResult.Failure`; `unroot` on a production-style build → `RootResult.Failure`; happy paths → `Success`. +- **Emulator (existing `DadbTest` style):** install a bad/duplicate apk → `InstallResult.Failure`; uninstall an absent package → `UninstallResult.Failure` (non-zero `exitCode` preserved); pull a missing path → `SyncResult.Failure`; `unroot` on a production-style build → `RootResult.Failure`; happy paths → `Success`. ## 10. Future / not now From 65251a68bd6c5306f62208e8d72d2688097ebdc9 Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 12:33:41 -0700 Subject: [PATCH 05/32] docs: implementation plan for adbd-aware error model 13 TDD tasks: AdbException hierarchy, result types, nextMessage socket-fault fix, handshake/open exception mapping, AdbSyncFailException, and Dadb push/pull/install/uninstall/root result migration. Unit-tested via in-memory okio buffers and a FakeDadb fixture; emulator suite covers A_OPEN refusal and the install paths. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../plans/2026-06-05-dadb-error-model.md | 1239 +++++++++++++++++ 1 file changed, 1239 insertions(+) create mode 100644 docs/superpowers/plans/2026-06-05-dadb-error-model.md diff --git a/docs/superpowers/plans/2026-06-05-dadb-error-model.md b/docs/superpowers/plans/2026-06-05-dadb-error-model.md new file mode 100644 index 0000000..eacb7d4 --- /dev/null +++ b/docs/superpowers/plans/2026-06-05-dadb-error-model.md @@ -0,0 +1,1239 @@ +# dadb Error Model Redesign — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Replace dadb's undifferentiated `IOException` error model with an adbd-aware contract — typed transport exceptions (`AdbException` hierarchy) for connection failures, and operation-specific result types (`InstallResult`/`UninstallResult`/`SyncResult`/`RootResult`) for in-band outcomes. + +**Architecture:** A sealed `AdbException : IOException` hierarchy carries every transport/auth/protocol failure (one type per documented adbd `protocol.txt` failure point). Expected operation outcomes become return values; the named high-level `Dadb` methods return per-operation sealed result types. Internal stream codecs throw a private `AdbSyncFailException` for sync `FAIL`, which the high-level methods catch and fold into a result. See the design spec: `docs/superpowers/specs/2026-06-05-dadb-error-model-design.md`. + +**Tech Stack:** Kotlin/JVM, okio 2.10.0, JUnit Platform (`kotlin.test`), Google Truth. Unit tests drive the wire protocol with in-memory okio `Buffer`s (the existing `AdbStreamTest` pattern); a `FakeDadb`/`FakeAdbStream` fixture exercises the high-level `Dadb` default methods without an emulator. + +**Scope:** Direct-to-adbd error handling only. Out of scope (do NOT touch): the `dadb.adbserver.*` host-server path, `discover`/`list` restructuring, the swapped-timeout bug at `Dadb.kt:271`, and the concurrency/resource fixes noted in review. + +**Conventions for every task:** new files get the standard Apache-2.0 license header (copy it verbatim from any existing file in `dadb/src/main/kotlin/dadb/`). Run the build from the repo root with `./gradlew :dadb:test`. Compile-only checks use `./gradlew :dadb:compileKotlin :dadb:compileTestKotlin`. + +--- + +## File Structure + +**New main files:** +- `dadb/src/main/kotlin/dadb/AdbException.kt` — sealed `AdbException` + the five transport subtypes. +- `dadb/src/main/kotlin/dadb/InstallResult.kt`, `UninstallResult.kt`, `SyncResult.kt`, `RootResult.kt` — operation result types. + +**New test files:** +- `dadb/src/test/kotlin/dadb/FakeDadb.kt` — `FakeAdbStream`, `FakeDadb`, and the `shellV2Buffer`/`syncFailBuffer` byte-builders. +- `dadb/src/test/kotlin/dadb/AdbExceptionTest.kt`, `ResultTypesTest.kt`, `AdbConnectionHandshakeTest.kt`, `DadbResultTest.kt` — unit tests. + +**Modified main files:** +- `dadb/src/main/kotlin/dadb/AdbStream.kt` — `nextMessage` distinguishes clean close from socket fault. +- `dadb/src/main/kotlin/dadb/AdbConnection.kt` — handshake + `open` map failures to typed exceptions; one `private`→`internal` test seam. +- `dadb/src/main/kotlin/dadb/AdbSync.kt` — internal `AdbSyncFailException`; `send`/`recv` throw it on `FAIL`. +- `dadb/src/main/kotlin/dadb/Dadb.kt` — high-level methods return result types; `@Throws(AdbException::class)`. + +**Modified test files:** +- `dadb/src/test/kotlin/dadb/AdbStreamTest.kt` — add the `nextMessage` behavior tests. +- `dadb/src/test/kotlin/dadb/DadbTest.kt` — update emulator tests to the new return types; add `AdbStreamOpenException` test. + +--- + +## Task 1: `AdbException` hierarchy + +**Files:** +- Create: `dadb/src/main/kotlin/dadb/AdbException.kt` +- Test: `dadb/src/test/kotlin/dadb/AdbExceptionTest.kt` + +- [ ] **Step 1: Write the failing test** + +Create `dadb/src/test/kotlin/dadb/AdbExceptionTest.kt`: + +```kotlin +package dadb + +import com.google.common.truth.Truth.assertThat +import java.io.IOException +import kotlin.test.Test + +internal class AdbExceptionTest { + + @Test + fun allTypesAreIOExceptions() { + val exceptions: List = listOf( + AdbConnectException("x"), + AdbAuthException("x"), + AdbStreamOpenException("shell:", "x"), + AdbConnectionClosedException("x"), + AdbProtocolException("x"), + ) + exceptions.forEach { assertThat(it).isInstanceOf(IOException::class.java) } + } + + @Test + fun streamOpenExceptionCarriesDestination() { + val e = AdbStreamOpenException("exec:cmd package install", "refused") + assertThat(e.destination).isEqualTo("exec:cmd package install") + } + + @Test + fun preservesCause() { + val cause = IOException("socket reset") + assertThat(AdbConnectException("x", cause).cause).isSameInstanceAs(cause) + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `./gradlew :dadb:compileTestKotlin` +Expected: FAIL — unresolved references `AdbConnectException`, `AdbException`, etc. + +- [ ] **Step 3: Write the implementation** + +Create `dadb/src/main/kotlin/dadb/AdbException.kt` (prefix with the Apache-2.0 license header copied from an existing file): + +```kotlin +package dadb + +import java.io.IOException + +/** + * Root of all dadb transport/connection failures. Extends [IOException] because these are genuine, + * recoverable socket I/O faults, consistent with the okio stream surface dadb exposes. Every + * transport fault thrown from a public method is one of these subtypes — no bare IOException leaks. + */ +sealed class AdbException(message: String, cause: Throwable? = null) : IOException(message, cause) + +/** Could not establish the connection: TCP connect/timeout, or the A_CNXN handshake failed. + * Nothing ran — safe to retry by reconnecting. */ +class AdbConnectException(message: String, cause: Throwable? = null) : AdbException(message, cause) + +/** A_AUTH was rejected (no key, or device left UNAUTHORIZED). Reconnecting with the same key will + * not help — fix or accept the key. */ +class AdbAuthException(message: String, cause: Throwable? = null) : AdbException(message, cause) + +/** adbd refused to OPEN the stream (A_OPEN answered with A_CLSE): unknown/forbidden service string, + * or device offline. The underlying connection is still usable. */ +class AdbStreamOpenException( + val destination: String, + message: String, + cause: Throwable? = null +) : AdbException(message, cause) + +/** An established connection/stream died unexpectedly (socket EOF/RST mid-operation). Reconnect — + * but the operation MAY have partially executed; re-check state before retrying non-idempotent work. */ +class AdbConnectionClosedException(message: String, cause: Throwable? = null) : AdbException(message, cause) + +/** The peer sent a malformed or unexpected apacket (desync). No reliable way to re-sync — the + * connection must be closed and re-established. */ +class AdbProtocolException(message: String, cause: Throwable? = null) : AdbException(message, cause) +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `./gradlew :dadb:test --tests dadb.AdbExceptionTest` +Expected: PASS (3 tests) + +- [ ] **Step 5: Commit** + +```bash +git add dadb/src/main/kotlin/dadb/AdbException.kt dadb/src/test/kotlin/dadb/AdbExceptionTest.kt +git commit -m "feat: add AdbException transport exception hierarchy" +``` + +--- + +## Task 2: Operation result types + +**Files:** +- Create: `dadb/src/main/kotlin/dadb/InstallResult.kt`, `UninstallResult.kt`, `SyncResult.kt`, `RootResult.kt` +- Test: `dadb/src/test/kotlin/dadb/ResultTypesTest.kt` + +- [ ] **Step 1: Write the failing test** + +Create `dadb/src/test/kotlin/dadb/ResultTypesTest.kt`: + +```kotlin +package dadb + +import com.google.common.truth.Truth.assertThat +import kotlin.test.Test + +internal class ResultTypesTest { + + @Test + fun installResultVariants() { + val ok: InstallResult = InstallResult.Success + val fail: InstallResult = InstallResult.Failure("Failure [INSTALL_FAILED_INVALID_APK]") + assertThat(ok).isInstanceOf(InstallResult.Success::class.java) + assertThat((fail as InstallResult.Failure).reason).contains("INSTALL_FAILED") + } + + @Test + fun uninstallFailureCarriesExitCodeAndReason() { + val fail = UninstallResult.Failure(reason = "Failure [DELETE_FAILED_INTERNAL_ERROR]", exitCode = 1) + assertThat(fail.exitCode).isEqualTo(1) + assertThat(fail.reason).contains("DELETE_FAILED") + } + + @Test + fun syncAndRootVariants() { + assertThat(SyncResult.Failure("No such file or directory").reason).contains("No such file") + assertThat(RootResult.Failure("adbd cannot run as root in production builds").reason) + .contains("production builds") + val syncOk: SyncResult = SyncResult.Success + val rootOk: RootResult = RootResult.Success + assertThat(syncOk).isInstanceOf(SyncResult.Success::class.java) + assertThat(rootOk).isInstanceOf(RootResult.Success::class.java) + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `./gradlew :dadb:compileTestKotlin` +Expected: FAIL — unresolved `InstallResult`, `UninstallResult`, `SyncResult`, `RootResult`. + +- [ ] **Step 3: Write the implementations** + +Create `dadb/src/main/kotlin/dadb/InstallResult.kt` (with license header): + +```kotlin +package dadb + +/** Outcome of [Dadb.install] / [Dadb.installMultiple]. */ +sealed interface InstallResult { + object Success : InstallResult + /** Raw pm/`cmd package` verdict, surfaced opaquely — exactly as the canonical adb client does + * (client/adb_install.cpp checks `strncmp("Success", buf, 7)` and prints the rest verbatim). + * `INSTALL_FAILED_*` codes are a frameworks/base concept, not the adbd contract, so they are + * not parsed into an enum. */ + data class Failure(val reason: String) : InstallResult +} +``` + +Create `dadb/src/main/kotlin/dadb/UninstallResult.kt` (with license header): + +```kotlin +package dadb + +/** Outcome of [Dadb.uninstall]. `uninstall` is `cmd package uninstall` over the shell service, so + * [Failure] preserves adbd's actual process exit code and combined output — the wrapper hides nothing. */ +sealed interface UninstallResult { + object Success : UninstallResult + data class Failure(val reason: String, val exitCode: Int) : UninstallResult +} +``` + +Create `dadb/src/main/kotlin/dadb/SyncResult.kt` (with license header): + +```kotlin +package dadb + +/** Outcome of [Dadb.push] / [Dadb.pull]. [Failure.reason] is the adbd sync FAIL message. */ +sealed interface SyncResult { + object Success : SyncResult + data class Failure(val reason: String) : SyncResult +} +``` + +Create `dadb/src/main/kotlin/dadb/RootResult.kt` (with license header): + +```kotlin +package dadb + +/** Outcome of [Dadb.root] / [Dadb.unroot]. [Failure.reason] is the adbd root:/unroot: response line. */ +sealed interface RootResult { + object Success : RootResult + data class Failure(val reason: String) : RootResult +} +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `./gradlew :dadb:test --tests dadb.ResultTypesTest` +Expected: PASS (3 tests) + +- [ ] **Step 5: Commit** + +```bash +git add dadb/src/main/kotlin/dadb/InstallResult.kt dadb/src/main/kotlin/dadb/UninstallResult.kt dadb/src/main/kotlin/dadb/SyncResult.kt dadb/src/main/kotlin/dadb/RootResult.kt dadb/src/test/kotlin/dadb/ResultTypesTest.kt +git commit -m "feat: add operation result types" +``` + +--- + +## Task 3: Test fixtures (`FakeDadb`, byte-builders) + +This is test scaffolding (no red/green of its own); later tasks depend on it. It must compile against the `Dadb` interface and existing wire constants. + +**Files:** +- Create: `dadb/src/test/kotlin/dadb/FakeDadb.kt` + +- [ ] **Step 1: Write the fixture** + +Create `dadb/src/test/kotlin/dadb/FakeDadb.kt`: + +```kotlin +package dadb + +import okio.Buffer +import java.nio.charset.StandardCharsets + +/** An [AdbStream] backed by in-memory buffers. [source] is pre-loaded with device bytes; + * [sink] captures whatever the code under test writes. */ +internal class FakeAdbStream( + override val source: Buffer, + override val sink: Buffer = Buffer() +) : AdbStream { + override fun close() {} +} + +/** A [Dadb] whose `open(destination)` returns a caller-supplied [AdbStream]. Records every + * destination opened, in order, in [opened]. */ +internal class FakeDadb( + private val features: Set = setOf("shell_v2", "cmd"), + private val streamFor: (destination: String) -> AdbStream +) : Dadb { + val opened = mutableListOf() + + override fun open(destination: String): AdbStream { + opened += destination + return streamFor(destination) + } + + override fun supportsFeature(feature: String) = feature in features + + override fun close() {} +} + +/** Builds a shell,v2 byte stream: optional stdout/stderr packets followed by an exit packet. */ +internal fun shellV2Buffer(stdout: String = "", stderr: String = "", exitCode: Int): Buffer { + val buffer = Buffer() + if (stdout.isNotEmpty()) { + val bytes = stdout.toByteArray() + buffer.writeByte(ID_STDOUT); buffer.writeIntLe(bytes.size); buffer.write(bytes) + } + if (stderr.isNotEmpty()) { + val bytes = stderr.toByteArray() + buffer.writeByte(ID_STDERR); buffer.writeIntLe(bytes.size); buffer.write(bytes) + } + buffer.writeByte(ID_EXIT); buffer.writeIntLe(1); buffer.writeByte(exitCode) + return buffer +} + +/** Builds a single sync `FAIL` packet (4-char id + LE length + message). */ +internal fun syncFailBuffer(message: String): Buffer { + val buffer = Buffer() + buffer.writeString("FAIL", StandardCharsets.UTF_8) + buffer.writeIntLe(message.toByteArray().size) + buffer.writeString(message, StandardCharsets.UTF_8) + return buffer +} + +/** Builds a single sync `OKAY` packet (success terminator for SEND). */ +internal fun syncOkayBuffer(): Buffer { + val buffer = Buffer() + buffer.writeString("OKAY", StandardCharsets.UTF_8) + buffer.writeIntLe(0) + return buffer +} +``` + +- [ ] **Step 2: Verify it compiles** + +Run: `./gradlew :dadb:compileTestKotlin` +Expected: PASS (compiles; `FakeAdbStream` satisfies `AdbStream`, constants `ID_STDOUT`/`ID_STDERR`/`ID_EXIT` resolve from `AdbShell.kt`). + +- [ ] **Step 3: Commit** + +```bash +git add dadb/src/test/kotlin/dadb/FakeDadb.kt +git commit -m "test: add FakeDadb fixture and wire byte-builders" +``` + +--- + +## Task 4: `AdbStream.nextMessage` — clean close vs socket fault + +The core behavioral fix: a peer `A_CLSE` is a legitimate EOF; a real socket fault must surface as `AdbConnectionClosedException` instead of masquerading as a clean end-of-stream. + +**Files:** +- Modify: `dadb/src/main/kotlin/dadb/AdbStream.kt:117-124` +- Test: `dadb/src/test/kotlin/dadb/AdbStreamTest.kt` (add tests) + +- [ ] **Step 1: Write the failing tests** + +Append these two methods inside the `AdbStreamTest` class in `dadb/src/test/kotlin/dadb/AdbStreamTest.kt` (and add the imports `import kotlin.test.assertFailsWith` and `import java.io.IOException` at the top): + +```kotlin + @Test + fun peerCloseIsCleanEof() { + // Reader yields a CLSE for localId=1 and nothing else. + val buffer = Buffer() + AdbWriter(buffer).write(Constants.CMD_CLSE, 2, 1, null, 0, 0) + val messageQueue = AdbMessageQueue(AdbReader(buffer)) + messageQueue.startListening(1) + val stream = AdbStreamImpl(messageQueue, AdbWriter(Buffer()), 1024, 1, 2) + + // A clean peer close reads as EOF (empty), NOT an exception. + Truth.assertThat(stream.source.readByteArray()).isEqualTo(ByteArray(0)) + } + + @Test + fun socketFaultThrowsConnectionClosed() { + // Empty reader: the next read hits EOF on the socket itself (not a protocol CLSE). + val messageQueue = AdbMessageQueue(AdbReader(Buffer())) + messageQueue.startListening(1) + val stream = AdbStreamImpl(messageQueue, AdbWriter(Buffer()), 1024, 1, 2) + + assertFailsWith { stream.source.readByteArray() } + } +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `./gradlew :dadb:test --tests dadb.AdbStreamTest` +Expected: `peerCloseIsCleanEof` PASSES already (today's swallow returns EOF), `socketFaultThrowsConnectionClosed` FAILS (today's swallow also returns EOF, so `readByteArray()` returns empty instead of throwing). + +- [ ] **Step 3: Apply the fix** + +In `dadb/src/main/kotlin/dadb/AdbStream.kt`, replace the `nextMessage` method (lines 117-124): + +```kotlin + private fun nextMessage(command: Int): AdbMessage? { + return try { + messageQueue.take(localId, command) + } catch (e: IOException) { + close() + return null + } + } +``` + +with: + +```kotlin + private fun nextMessage(command: Int): AdbMessage? { + return try { + messageQueue.take(localId, command) + } catch (e: AdbStreamClosed) { + // Peer sent A_CLSE: normal end-of-stream (e.g. shell/exec output finished). EOF is correct. + close() + null + } catch (e: IOException) { + // Real socket fault (EOF/RST) mid-stream: the transport died, not a clean close. + close() + throw AdbConnectionClosedException("Connection lost while reading stream $localId", e) + } + } +``` + +(`AdbStream.kt` already imports `java.io.IOException`; `AdbStreamClosed` and `AdbConnectionClosedException` are in the same package — no new imports needed.) + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `./gradlew :dadb:test --tests dadb.AdbStreamTest` +Expected: PASS (3 tests, including the original `testLargeRemoteWrite`) + +- [ ] **Step 5: Commit** + +```bash +git add dadb/src/main/kotlin/dadb/AdbStream.kt dadb/src/test/kotlin/dadb/AdbStreamTest.kt +git commit -m "fix: surface mid-stream socket faults as AdbConnectionClosedException" +``` + +--- + +## Task 5: `AdbConnection` handshake — typed failures + +Map handshake failures to typed exceptions and guarantee no bare `IOException` leaks from the handshake. Requires making the `(Source, Sink)` connect overload `internal` as a test seam. + +**Files:** +- Modify: `dadb/src/main/kotlin/dadb/AdbConnection.kt:86-137` +- Test: `dadb/src/test/kotlin/dadb/AdbConnectionHandshakeTest.kt` + +- [ ] **Step 1: Write the failing tests** + +Create `dadb/src/test/kotlin/dadb/AdbConnectionHandshakeTest.kt`: + +```kotlin +package dadb + +import okio.Buffer +import kotlin.test.Test +import kotlin.test.assertFailsWith + +internal class AdbConnectionHandshakeTest { + + @Test + fun nonCnxnResponseThrowsConnectException() { + // Device replies with an unexpected OKAY instead of CNXN. + val device = Buffer() + AdbWriter(device).writeOkay(0, 0) + assertFailsWith { + AdbConnection.connect(device, Buffer(), null) + } + } + + @Test + fun authRequiredWithoutKeyPairThrowsAuthException() { + // Device demands auth; we have no key pair. + val device = Buffer() + AdbWriter(device).writeAuth(Constants.AUTH_TYPE_TOKEN, ByteArray(20)) + assertFailsWith { + AdbConnection.connect(device, Buffer(), null) + } + } + + @Test + fun truncatedHandshakeThrowsConnectException() { + // Empty device stream: the first readMessage hits EOF. + assertFailsWith { + AdbConnection.connect(Buffer(), Buffer(), null) + } + } +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `./gradlew :dadb:compileTestKotlin` +Expected: FAIL — `connect(Source, Sink, ...)` is `private` (unresolved/inaccessible). + +- [ ] **Step 3: Make the test seam internal and map failures** + +In `dadb/src/main/kotlin/dadb/AdbConnection.kt`: + +(a) Change the visibility of the `(Source, Sink)` overload (line 86) from `private` to `internal`, and wrap its body so any non-`AdbException` `IOException` from the handshake becomes `AdbConnectException`: + +```kotlin + internal fun connect(source: Source, sink: Sink, keyPair: AdbKeyPair? = null, closeable: Closeable? = null): AdbConnection { + val adbReader = AdbReader(source) + val adbWriter = AdbWriter(sink) + + try { + return connect(adbReader, adbWriter, keyPair, closeable) + } catch (e: AdbException) { + adbReader.close() + adbWriter.close() + throw e + } catch (e: IOException) { + adbReader.close() + adbWriter.close() + throw AdbConnectException("Connection handshake failed", e) + } catch (t: Throwable) { + adbReader.close() + adbWriter.close() + throw t + } + } +``` + +(b) Replace the handshake body in the `(AdbReader, AdbWriter)` overload (lines 99-125) with typed mappings: + +```kotlin + private fun connect(adbReader: AdbReader, adbWriter: AdbWriter, keyPair: AdbKeyPair?, closeable: Closeable?): AdbConnection { + adbWriter.writeConnect() + + var message = adbReader.readMessage() + + if (message.command == Constants.CMD_AUTH) { + if (keyPair == null) throw AdbAuthException("Authentication required but no key pair was provided") + if (message.arg0 != Constants.AUTH_TYPE_TOKEN) throw AdbProtocolException("Unsupported auth type: $message") + + val signature = keyPair.signPayload(message) + adbWriter.writeAuth(Constants.AUTH_TYPE_SIGNATURE, signature) + + message = adbReader.readMessage() + if (message.command == Constants.CMD_AUTH) { + adbWriter.writeAuth(Constants.AUTH_TYPE_RSA_PUBLIC, keyPair.publicKeyBytes) + message = adbReader.readMessage() + } + } + + if (message.command != Constants.CMD_CNXN) { + // A trailing AUTH means the device rejected our key / stayed unauthorized. + if (message.command == Constants.CMD_AUTH) throw AdbAuthException("Device rejected authentication (unauthorized)") + throw AdbConnectException("Connection failed: $message") + } + + val connectionString = parseConnectionString(String(message.payload)) + val version = message.arg0 + val maxPayloadSize = message.arg1 + + return AdbConnection(adbReader, adbWriter, closeable, connectionString.features, version, maxPayloadSize) + } +``` + +(c) In `parseConnectionString` (line 134), change the feature-parse throw to a connect failure: + +```kotlin + if ("features" !in keyValues) throw AdbConnectException("Failed to parse features from connection string: $connectionString") +``` + +(`AdbConnection.kt` already imports `java.io.IOException`; the new exception types share the package.) + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `./gradlew :dadb:test --tests dadb.AdbConnectionHandshakeTest` +Expected: PASS (3 tests) + +- [ ] **Step 5: Commit** + +```bash +git add dadb/src/main/kotlin/dadb/AdbConnection.kt dadb/src/test/kotlin/dadb/AdbConnectionHandshakeTest.kt +git commit -m "feat: map adbd handshake failures to typed AdbException subtypes" +``` + +--- + +## Task 6: `AdbConnection.open` — stream-open refusal + +When adbd answers `A_OPEN` with `A_CLSE`, surface `AdbStreamOpenException` (the connection is still alive) instead of the internal `AdbStreamClosed`. The `localId` is random, so this mapping is verified by an emulator test (opening a bogus service) plus the implementation review. + +**Files:** +- Modify: `dadb/src/main/kotlin/dadb/AdbConnection.kt:42-55` +- Test: `dadb/src/test/kotlin/dadb/DadbTest.kt` (add one emulator test) + +- [ ] **Step 1: Write the failing emulator test** + +Add this method inside the `DadbTest` class in `dadb/src/test/kotlin/dadb/DadbTest.kt` (it requires a running emulator on `localhost:5555`, like the other `localEmulator { }` tests; add `import kotlin.test.assertFailsWith` if not present): + +```kotlin + @Test + fun open_invalidService_throwsStreamOpenException() { + localEmulator { dadb -> + assertFailsWith { + dadb.open("definitely-not-a-real-service:") + } + } + } +``` + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `./gradlew :dadb:test --tests dadb.DadbTestImpl` +Expected: FAIL — today this throws the internal `AdbStreamClosed` (an `IOException`), not `AdbStreamOpenException`. (If no emulator is available the test errors on connect; in that case verify the mapping by code review and proceed.) + +- [ ] **Step 3: Apply the mapping** + +In `dadb/src/main/kotlin/dadb/AdbConnection.kt`, replace the `open` method (lines 42-55): + +```kotlin + @Throws(IOException::class) + fun open(destination: String): AdbStream { + val localId = newId() + messageQueue.startListening(localId) + try { + adbWriter.writeOpen(localId, destination) + val message = messageQueue.take(localId, Constants.CMD_OKAY) + val remoteId = message.arg0 + return AdbStreamImpl(messageQueue, adbWriter, maxPayloadSize, localId, remoteId) + } catch (e: AdbStreamClosed) { + // adbd answered A_OPEN with A_CLSE: it refused this service. Connection is still alive. + messageQueue.stopListening(localId) + throw AdbStreamOpenException(destination, "adbd refused to open stream: $destination", e) + } catch (e: Throwable) { + messageQueue.stopListening(localId) + throw e + } + } +``` + +- [ ] **Step 4: Run the test to verify it passes** + +Run: `./gradlew :dadb:test --tests dadb.DadbTestImpl` +Expected: PASS (with emulator). Without an emulator, run `./gradlew :dadb:compileKotlin` and confirm it compiles, then proceed. + +- [ ] **Step 5: Commit** + +```bash +git add dadb/src/main/kotlin/dadb/AdbConnection.kt dadb/src/test/kotlin/dadb/DadbTest.kt +git commit -m "feat: map A_OPEN refusal to AdbStreamOpenException" +``` + +--- + +## Task 7: `AdbSync` — internal `AdbSyncFailException` + +Replace the bare `IOException`s in the sync codec with an internal signal the high-level methods can catch, and make `send` report a `FAIL` message symmetrically with `recv`. + +**Files:** +- Modify: `dadb/src/main/kotlin/dadb/AdbSync.kt:46-97` +- Test: `dadb/src/test/kotlin/dadb/DadbResultTest.kt` (new file; sync tests here) + +- [ ] **Step 1: Write the failing tests** + +Create `dadb/src/test/kotlin/dadb/DadbResultTest.kt`: + +```kotlin +package dadb + +import com.google.common.truth.Truth.assertThat +import okio.Buffer +import kotlin.test.Test +import kotlin.test.assertFailsWith + +internal class DadbResultTest { + + @Test + fun syncRecvFailThrowsSyncFailException() { + val stream = FakeAdbStream(syncFailBuffer("No such file or directory")) + val sync = AdbSyncStream(stream) + val e = assertFailsWith { sync.recv(Buffer(), "/missing") } + assertThat(e.reason).isEqualTo("No such file or directory") + } +} +``` + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `./gradlew :dadb:compileTestKotlin` +Expected: FAIL — unresolved `AdbSyncFailException`. + +- [ ] **Step 3: Add the exception and use it** + +In `dadb/src/main/kotlin/dadb/AdbSync.kt`, add the internal exception just below the imports (after line 23): + +```kotlin +/** Internal signal that an adbd sync operation returned FAIL (e.g. file not found / permission + * denied). Caught by Dadb.push/pull and folded into a [SyncResult.Failure]; never surfaced raw + * from the high-level API. Not an AdbException — the transport is healthy. */ +internal class AdbSyncFailException(val reason: String) : IOException(reason) +``` + +Replace the `send` tail (line 69-70): + +```kotlin + val packet = readPacket() + if (packet.id == FAIL) { + val message = stream.source.readString(packet.arg.toLong(), StandardCharsets.UTF_8) + throw AdbSyncFailException(message) + } + if (packet.id != OKAY) throw AdbProtocolException("Unexpected sync packet id: ${packet.id}") +``` + +Replace the `recv` FAIL/unexpected handling (lines 86-90): + +```kotlin + if (packet.id == FAIL) { + val message = stream.source.readString(packet.arg.toLong(), StandardCharsets.UTF_8) + throw AdbSyncFailException(message) + } + if (packet.id != DATA) throw AdbProtocolException("Unexpected sync packet id: ${packet.id}") +``` + +(`AdbSync.kt` already imports `java.io.IOException`, `okio.*`, and `StandardCharsets`; `AdbProtocolException` shares the package.) + +- [ ] **Step 4: Run the test to verify it passes** + +Run: `./gradlew :dadb:test --tests dadb.DadbResultTest` +Expected: PASS (1 test) + +- [ ] **Step 5: Commit** + +```bash +git add dadb/src/main/kotlin/dadb/AdbSync.kt dadb/src/test/kotlin/dadb/DadbResultTest.kt +git commit -m "feat: AdbSync throws internal AdbSyncFailException on FAIL" +``` + +--- + +## Task 8: `Dadb.push` / `Dadb.pull` → `SyncResult` + +**Files:** +- Modify: `dadb/src/main/kotlin/dadb/Dadb.kt:46-68` +- Test: `dadb/src/test/kotlin/dadb/DadbResultTest.kt` (add tests) + +- [ ] **Step 1: Write the failing tests** + +Add these methods to `DadbResultTest` in `dadb/src/test/kotlin/dadb/DadbResultTest.kt`: + +```kotlin + @Test + fun pullMissingFileReturnsSyncFailure() { + val dadb = FakeDadb { FakeAdbStream(syncFailBuffer("No such file or directory")) } + val result = dadb.pull(Buffer(), "/missing") + assertThat(result).isInstanceOf(SyncResult.Failure::class.java) + assertThat((result as SyncResult.Failure).reason).isEqualTo("No such file or directory") + } + + @Test + fun pushSuccessReturnsSyncSuccess() { + val dadb = FakeDadb { FakeAdbStream(syncOkayBuffer()) } + val result = dadb.push(Buffer().also { it.writeUtf8("hi") }, "/data/local/tmp/hi", 0b110_100_100, 0L) + assertThat(result).isInstanceOf(SyncResult.Success::class.java) + } +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `./gradlew :dadb:compileTestKotlin` +Expected: FAIL — `pull`/`push` return `Unit`, so `assertThat(result)` does not type-check. + +- [ ] **Step 3: Change the signatures to return `SyncResult`** + +In `dadb/src/main/kotlin/dadb/Dadb.kt`, replace the push/pull/openSync block (lines 46-74). Note `openSync` is unchanged except for its `@Throws` annotation, kept here for context: + +```kotlin + @Throws(AdbException::class) + fun push(src: File, remotePath: String, mode: Int = readMode(src), lastModifiedMs: Long = src.lastModified()): SyncResult { + return push(src.source(), remotePath, mode, lastModifiedMs) + } + + @Throws(AdbException::class) + fun push(source: Source, remotePath: String, mode: Int, lastModifiedMs: Long): SyncResult { + openSync().use { stream -> + return try { + stream.send(source, remotePath, mode, lastModifiedMs) + SyncResult.Success + } catch (e: AdbSyncFailException) { + SyncResult.Failure(e.reason) + } + } + } + + @Throws(AdbException::class) + fun pull(dst: File, remotePath: String): SyncResult { + return pull(dst.sink(append = false), remotePath) + } + + @Throws(AdbException::class) + fun pull(sink: Sink, remotePath: String): SyncResult { + openSync().use { stream -> + return try { + stream.recv(sink, remotePath) + SyncResult.Success + } catch (e: AdbSyncFailException) { + SyncResult.Failure(e.reason) + } + } + } + + @Throws(AdbException::class) + fun openSync(): AdbSyncStream { + val stream = open("sync:") + return AdbSyncStream(stream) + } +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `./gradlew :dadb:test --tests dadb.DadbResultTest` +Expected: PASS. The module still compiles at this point: changing `push`/`pull` to return `SyncResult` while `install`/`uninstall`/`root` still return `Unit` is fine (their call sites in `pmInstall`/`installMultiple`/`DadbTest` ignore the new return value, which is legal Kotlin). `AdbException` resolves because it is in the same package. + +> **Note for the implementer:** Tasks 8–12 each edit `Dadb.kt` and each leaves the module compiling and its targeted `DadbResultTest` cases green. The emulator-dependent assertions in `DadbTest` are validated once at the end of Task 12 / Final Verification. + +- [ ] **Step 5: Commit** + +```bash +git add dadb/src/main/kotlin/dadb/Dadb.kt dadb/src/test/kotlin/dadb/DadbResultTest.kt +git commit -m "feat: push/pull return SyncResult" +``` + +--- + +## Task 9: `Dadb.uninstall` → `UninstallResult` + +**Files:** +- Modify: `dadb/src/main/kotlin/dadb/Dadb.kt:201-207` +- Test: `dadb/src/test/kotlin/dadb/DadbResultTest.kt` (add tests) + +- [ ] **Step 1: Write the failing tests** + +Add to `DadbResultTest`: + +```kotlin + @Test + fun uninstallNonZeroExitReturnsFailureWithExitCode() { + val dadb = FakeDadb { + FakeAdbStream(shellV2Buffer(stdout = "Failure [DELETE_FAILED_INTERNAL_ERROR]\n", exitCode = 1)) + } + val result = dadb.uninstall("com.example.absent") + assertThat(result).isInstanceOf(UninstallResult.Failure::class.java) + result as UninstallResult.Failure + assertThat(result.exitCode).isEqualTo(1) + assertThat(result.reason).contains("DELETE_FAILED_INTERNAL_ERROR") + } + + @Test + fun uninstallZeroExitReturnsSuccess() { + val dadb = FakeDadb { FakeAdbStream(shellV2Buffer(stdout = "Success\n", exitCode = 0)) } + assertThat(dadb.uninstall("com.example")).isInstanceOf(UninstallResult.Success::class.java) + } +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `./gradlew :dadb:compileTestKotlin` +Expected: FAIL — `uninstall` returns `Unit`. + +- [ ] **Step 3: Change `uninstall`** + +In `dadb/src/main/kotlin/dadb/Dadb.kt`, replace `uninstall` (lines 201-207): + +```kotlin + @Throws(AdbException::class) + fun uninstall(packageName: String): UninstallResult { + val response = shell("cmd package uninstall $packageName") + return if (response.exitCode == 0) { + UninstallResult.Success + } else { + UninstallResult.Failure(reason = response.allOutput, exitCode = response.exitCode) + } + } +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `./gradlew :dadb:test --tests dadb.DadbResultTest` (or defer to Task 12's combined run, per the Task 8 note) +Expected: PASS + +- [ ] **Step 5: Commit** + +```bash +git add dadb/src/main/kotlin/dadb/Dadb.kt dadb/src/test/kotlin/dadb/DadbResultTest.kt +git commit -m "feat: uninstall returns UninstallResult" +``` + +--- + +## Task 10: `Dadb.install` (+ `pmInstall`) → `InstallResult` + +This also fixes a latent bug: the current `pmInstall` ignores the `pm install` result entirely. + +**Files:** +- Modify: `dadb/src/main/kotlin/dadb/Dadb.kt:76-110` +- Test: `dadb/src/test/kotlin/dadb/DadbResultTest.kt` (add tests) + +- [ ] **Step 1: Write the failing tests** (cmd-path; the pm-path is covered by the emulator suite) + +Add to `DadbResultTest`: + +```kotlin + @Test + fun installCmdPathFailureReturnsInstallFailure() { + val dadb = FakeDadb(features = setOf("cmd")) { + FakeAdbStream(Buffer().also { it.writeUtf8("Failure [INSTALL_FAILED_INVALID_APK]") }) + } + val result = dadb.install(Buffer().also { it.writeUtf8("apk") }, 3L) + assertThat(result).isInstanceOf(InstallResult.Failure::class.java) + assertThat((result as InstallResult.Failure).reason).contains("INSTALL_FAILED_INVALID_APK") + } + + @Test + fun installCmdPathSuccessReturnsSuccess() { + val dadb = FakeDadb(features = setOf("cmd")) { + FakeAdbStream(Buffer().also { it.writeUtf8("Success\n") }) + } + val result = dadb.install(Buffer().also { it.writeUtf8("apk") }, 3L) + assertThat(result).isInstanceOf(InstallResult.Success::class.java) + } +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `./gradlew :dadb:compileTestKotlin` +Expected: FAIL — `install` returns `Unit`. + +- [ ] **Step 3: Change `install` and `pmInstall`** + +In `dadb/src/main/kotlin/dadb/Dadb.kt`, replace `install(file)`, `install(source, size)`, and `pmInstall` (lines 76-110): + +```kotlin + @Throws(AdbException::class) + fun install(file: File, vararg options: String): InstallResult { + return if (supportsFeature("cmd")) { + install(file.source(), file.length(), *options) + } else { + pmInstall(file, *options) + } + } + + @Throws(AdbException::class) + fun install(source: Source, size: Long, vararg options: String): InstallResult { + if (supportsFeature("cmd")) { + execCmd("package", "install", "-S", size.toString(), *options).use { stream -> + stream.sink.writeAll(source) + stream.sink.flush() + val response = stream.source.readString(Charsets.UTF_8) + return if (response.startsWith("Success")) InstallResult.Success else InstallResult.Failure(response) + } + } else { + val tempFile = kotlin.io.path.createTempFile() + val fileSink = tempFile.sink().buffer() + fileSink.writeAll(source) + fileSink.flush() + return pmInstall(tempFile.toFile(), *options) + } + } + + private fun pmInstall(file: File, vararg options: String): InstallResult { + val remotePath = "/data/local/tmp/${file.name}" + when (val pushResult = push(file, remotePath)) { + is SyncResult.Failure -> return InstallResult.Failure("push failed: ${pushResult.reason}") + SyncResult.Success -> Unit + } + val response = shell("pm install ${options.joinToString(" ")} \"$remotePath\"") + return if (response.allOutput.startsWith("Success")) InstallResult.Success else InstallResult.Failure(response.allOutput) + } +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `./gradlew :dadb:test --tests dadb.DadbResultTest` (or defer to Task 12's combined run) +Expected: PASS + +- [ ] **Step 5: Commit** + +```bash +git add dadb/src/main/kotlin/dadb/Dadb.kt dadb/src/test/kotlin/dadb/DadbResultTest.kt +git commit -m "feat: install returns InstallResult and checks the pm-install result" +``` + +--- + +## Task 11: `Dadb.installMultiple` → `InstallResult` + +Translate every `throw IOException(...)` operation failure in both the `cmd` and `pm` paths into an `InstallResult.Failure` return. Also fixes the `Dadb.kt:145` message bug (`$commitStream` → the response). This path is verified by the emulator suite (`DadbTest.installMultiple…`); update those tests to the new return type. + +**Files:** +- Modify: `dadb/src/main/kotlin/dadb/Dadb.kt:112-199` +- Test: `dadb/src/test/kotlin/dadb/DadbTest.kt` (update existing emulator assertions) + +- [ ] **Step 1: Rewrite `installMultiple`** + +In `dadb/src/main/kotlin/dadb/Dadb.kt`, replace `installMultiple` (lines 112-199): + +```kotlin + @Throws(AdbException::class) + fun installMultiple(apks: List, vararg options: String): InstallResult { + // http://aospxref.com/android-12.0.0_r3/xref/packages/modules/adb/client/adb_install.cpp#538 + if (supportsFeature("cmd")) { + val totalLength = apks.map { it.length() }.reduce { acc, l -> acc + l } + execCmd("package", "install-create", "-S", totalLength.toString(), *options).use { createStream -> + val response = createStream.source.readString(Charsets.UTF_8) + if (!response.startsWith("Success")) return InstallResult.Failure("create session failed: $response") + + val pattern = """\[(\w+)]""".toRegex() + val sessionId = pattern.find(response)?.groups?.get(1)?.value + ?: return InstallResult.Failure("failed to parse session id: $response") + + var error: String? = null + apks.forEach { apk -> + execCmd("package", "install-write", "-S", apk.length().toString(), sessionId, apk.name, "-", *options).use { writeStream -> + writeStream.sink.writeAll(apk.source()) + writeStream.sink.flush() + val writeResponse = writeStream.source.readString(Charsets.UTF_8) + if (!writeResponse.startsWith("Success")) { + error = writeResponse + return@forEach + } + } + } + + val finalCommand = if (error == null) "install-commit" else "install-abandon" + execCmd("package", finalCommand, sessionId, *options).use { commitStream -> + val finalResponse = commitStream.source.readString(Charsets.UTF_8) + if (!finalResponse.startsWith("Success")) return InstallResult.Failure("failed to finalize session: $finalResponse") + } + + return if (error == null) InstallResult.Success else InstallResult.Failure(error!!) + } + } else { + val totalLength = apks.map { it.length() }.reduce { acc, l -> acc + l } + val response = shell("pm install-create -S $totalLength ${options.joinToString(" ")}") + if (!response.allOutput.startsWith("Success")) return InstallResult.Failure("pm create session failed: ${response.allOutput}") + + val pattern = """\[(\w+)]""".toRegex() + val sessionId = pattern.find(response.allOutput)?.groups?.get(1)?.value + ?: return InstallResult.Failure("failed to parse session id: ${response.allOutput}") + var error: String? = null + + val fileNames = apks.map { it.name } + val remotePaths = fileNames.map { "/data/local/tmp/$it" } + + apks.zip(remotePaths).forEachIndexed { index, pair -> + val apk = pair.first + val remotePath = pair.second + + when (val pushResult = push(apk, remotePath)) { + is SyncResult.Failure -> { error = "push failed: ${pushResult.reason}"; return@forEachIndexed } + SyncResult.Success -> Unit + } + + val writeResponse = shell("pm install-write -S ${apk.length()} $sessionId $index $remotePath") + if (!writeResponse.allOutput.startsWith("Success")) { + error = writeResponse.allOutput + return@forEachIndexed + } + } + + val finalCommand = if (error == null) "pm install-commit $sessionId" else "pm install-abandon $sessionId" + val finalResponse = shell(finalCommand) + if (!finalResponse.allOutput.startsWith("Success")) return InstallResult.Failure("failed to finalize session: ${finalResponse.allOutput}") + return if (error == null) InstallResult.Success else InstallResult.Failure(error!!) + } + } +``` + +- [ ] **Step 2: Update the emulator tests in `DadbTest.kt`** + +Find the `installMultiple` / `install` assertions in `dadb/src/test/kotlin/dadb/DadbTest.kt`. Where a test previously called `dadb.install(apk)` / `dadb.installMultiple(apks)` and expected no exception, assert success instead. For example, a test body like: + +```kotlin + localEmulator { dadb -> + dadb.install(apkFile) + } +``` + +becomes: + +```kotlin + localEmulator { dadb -> + assertThat(dadb.install(apkFile)).isInstanceOf(InstallResult.Success::class.java) + } +``` + +Apply the same `assertThat(...).isInstanceOf(InstallResult.Success::class.java)` to each `install`/`installMultiple` call site, and update any `uninstall`/`push`/`pull` call sites to their new result types (`UninstallResult.Success`, `SyncResult.Success`). Add `import com.google.common.truth.Truth.assertThat` if missing. + +- [ ] **Step 3: Verify compilation** + +Run: `./gradlew :dadb:compileKotlin :dadb:compileTestKotlin` +Expected: PASS (whole module compiles; combined test run happens in Task 12) + +- [ ] **Step 4: Commit** + +```bash +git add dadb/src/main/kotlin/dadb/Dadb.kt dadb/src/test/kotlin/dadb/DadbTest.kt +git commit -m "feat: installMultiple returns InstallResult; fix finalize message bug" +``` + +--- + +## Task 12: `Dadb.root` / `unroot` → `RootResult`, `@Throws` sweep, full suite + +**Files:** +- Modify: `dadb/src/main/kotlin/dadb/Dadb.kt:28-247` (root/unroot + remaining `@Throws` annotations) +- Test: `dadb/src/test/kotlin/dadb/DadbResultTest.kt` (add root tests) + +- [ ] **Step 1: Write the failing tests** + +Add to `DadbResultTest` in `dadb/src/test/kotlin/dadb/DadbResultTest.kt`: + +```kotlin + @Test + fun rootFailureReturnsRootFailure() { + // root: service replies with a non-restarting line, terminated by '\n'. + val dadb = FakeDadb { + FakeAdbStream(Buffer().also { it.writeUtf8("adbd cannot run as root in production builds\n") }) + } + val result = dadb.root() + assertThat(result).isInstanceOf(RootResult.Failure::class.java) + assertThat((result as RootResult.Failure).reason).contains("production builds") + } +``` + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `./gradlew :dadb:compileTestKotlin` +Expected: FAIL — `root` returns `Unit`. + +- [ ] **Step 3: Rewrite root/unroot and finish the `@Throws` sweep** + +In `dadb/src/main/kotlin/dadb/Dadb.kt`, replace `root` and `unroot` (lines 223-239): + +```kotlin + @Throws(AdbException::class) + fun root(): RootResult = restartAdbd("root:", root = true) { it.startsWith("restarting") || it.contains("already") } + + @Throws(AdbException::class) + fun unroot(): RootResult = restartAdbd("unroot:", root = false) { it.startsWith("restarting") || it.contains("not running as root") } + + private fun restartAdbd(service: String, root: Boolean, isSuccess: (String) -> Boolean): RootResult { + val response = restartAdb(this, service) + if (!isSuccess(response)) return RootResult.Failure(response) + waitRootOrClose(this, root) + return RootResult.Success + } +``` + +Then change the remaining `@Throws(IOException::class)` annotations on the **public interface methods** to `@Throws(AdbException::class)`: `open` (line 28), `shell` (33), `openShell` (40), `execCmd` (209), `abbExec` (216). Leave `tcpForward`'s `@Throws(InterruptedException::class)` unchanged. Add the import `import dadb.AdbException` is unnecessary (same package), but ensure `Dadb.kt` no longer needs the now-unused `IOException` import only if nothing else uses it — `readMode` still throws `RuntimeException` and the companion still references `IOException` in `waitRootOrClose`'s catch, so **keep** the `okio.*`/`java.io` imports as-is. + +> The `@Throws` annotation value must resolve; since `AdbException` is in package `dadb`, no import is needed. + +- [ ] **Step 4: Run the full test suite** + +Run: `./gradlew :dadb:test` +Expected: PASS for all non-emulator tests — `AdbExceptionTest`, `ResultTypesTest`, `AdbStreamTest`, `AdbConnectionHandshakeTest`, `DadbResultTest`, `MessageQueueTest`, `PKCS8Test`. Emulator-dependent suites (`DadbTestImpl`, `AdbServerTest`, `DadbImplTest`) require a device; if none is attached they will error on connect — that is expected in a non-emulator environment. Confirm the unit suites are green. + +- [ ] **Step 5: Commit** + +```bash +git add dadb/src/main/kotlin/dadb/Dadb.kt dadb/src/test/kotlin/dadb/DadbResultTest.kt +git commit -m "feat: root/unroot return RootResult; @Throws(AdbException) sweep" +``` + +--- + +## Task 13: Update docs & changelog + +**Files:** +- Modify: `README.md`, `CHANGELOG.md` + +- [ ] **Step 1: Update the README usage examples** + +In `README.md`, update the install example and any others to the new return types. Replace: + +```kotlin +Dadb.create("localhost", 5555).use { dadb -> + dadb.install(apkFile) +} +``` + +with: + +```kotlin +Dadb.create("localhost", 5555).use { dadb -> + when (val result = dadb.install(apkFile)) { + is InstallResult.Success -> println("installed") + is InstallResult.Failure -> println("install failed: ${result.reason}") + } +} +``` + +Add a short "Error handling" subsection after the usage examples: + +```markdown +### Error handling + +dadb distinguishes *transport* failures from *operation* failures: + +- **Transport failures throw** an `AdbException` (an `IOException`): `AdbConnectException`, + `AdbAuthException`, `AdbStreamOpenException`, `AdbConnectionClosedException`, `AdbProtocolException`. + Catch `AdbException` to decide whether to reconnect. +- **Operation outcomes are returned**, never thrown: `install`/`installMultiple` → `InstallResult`, + `uninstall` → `UninstallResult`, `push`/`pull` → `SyncResult`, `root`/`unroot` → `RootResult`. + A non-zero `shell` exit code stays a value on `AdbShellResponse`. +``` + +- [ ] **Step 2: Add a CHANGELOG entry** + +Prepend an entry to `CHANGELOG.md` describing the major version's breaking changes (typed `AdbException` hierarchy; result-returning `install`/`installMultiple`/`uninstall`/`push`/`pull`/`root`/`unroot`; mid-stream socket faults now throw `AdbConnectionClosedException` instead of EOF). Match the file's existing entry format. + +- [ ] **Step 3: Commit** + +```bash +git add README.md CHANGELOG.md +git commit -m "docs: document the adbd-aware error model" +``` + +--- + +## Final Verification + +- [ ] Run the unit suites and confirm green: + `./gradlew :dadb:test --tests dadb.AdbExceptionTest --tests dadb.ResultTypesTest --tests dadb.AdbStreamTest --tests dadb.AdbConnectionHandshakeTest --tests dadb.DadbResultTest --tests dadb.MessageQueueTest --tests dadb.PKCS8Test` +- [ ] Confirm the whole module compiles: `./gradlew :dadb:compileKotlin :dadb:compileTestKotlin` +- [ ] Grep for leftover bare operation-failure throws that should now be results: + `grep -n "throw IOException" dadb/src/main/kotlin/dadb/Dadb.kt dadb/src/main/kotlin/dadb/AdbSync.kt` — expect **no matches** in `Dadb.kt` and `AdbSync.kt`. +- [ ] (If an emulator is available) run `./gradlew :dadb:test` in full and confirm the emulator suites pass, including `open_invalidService_throwsStreamOpenException` and the updated `install`/`installMultiple` assertions. From b084b456897ad0ddc31686e756b50dd5db740fad Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 12:41:22 -0700 Subject: [PATCH 06/32] feat: add AdbException transport exception hierarchy Co-Authored-By: Claude Sonnet 4.6 --- dadb/src/main/kotlin/dadb/AdbException.kt | 51 +++++++++++++++++++ dadb/src/test/kotlin/dadb/AdbExceptionTest.kt | 49 ++++++++++++++++++ 2 files changed, 100 insertions(+) create mode 100644 dadb/src/main/kotlin/dadb/AdbException.kt create mode 100644 dadb/src/test/kotlin/dadb/AdbExceptionTest.kt diff --git a/dadb/src/main/kotlin/dadb/AdbException.kt b/dadb/src/main/kotlin/dadb/AdbException.kt new file mode 100644 index 0000000..82896e3 --- /dev/null +++ b/dadb/src/main/kotlin/dadb/AdbException.kt @@ -0,0 +1,51 @@ +/* + * Copyright (c) 2021 mobile.dev inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package dadb + +import java.io.IOException + +/** + * Root of all dadb transport/connection failures. Extends [IOException] because these are genuine, + * recoverable socket I/O faults, consistent with the okio stream surface dadb exposes. Every + * transport fault thrown from a public method is one of these subtypes — no bare IOException leaks. + */ +sealed class AdbException(message: String, cause: Throwable? = null) : IOException(message, cause) + +/** Could not establish the connection: TCP connect/timeout, or the A_CNXN handshake failed. + * Nothing ran — safe to retry by reconnecting. */ +class AdbConnectException(message: String, cause: Throwable? = null) : AdbException(message, cause) + +/** A_AUTH was rejected (no key, or device left UNAUTHORIZED). Reconnecting with the same key will + * not help — fix or accept the key. */ +class AdbAuthException(message: String, cause: Throwable? = null) : AdbException(message, cause) + +/** adbd refused to OPEN the stream (A_OPEN answered with A_CLSE): unknown/forbidden service string, + * or device offline. The underlying connection is still usable. */ +class AdbStreamOpenException( + val destination: String, + message: String, + cause: Throwable? = null +) : AdbException(message, cause) + +/** An established connection/stream died unexpectedly (socket EOF/RST mid-operation). Reconnect — + * but the operation MAY have partially executed; re-check state before retrying non-idempotent work. */ +class AdbConnectionClosedException(message: String, cause: Throwable? = null) : AdbException(message, cause) + +/** The peer sent a malformed or unexpected apacket (desync). No reliable way to re-sync — the + * connection must be closed and re-established. */ +class AdbProtocolException(message: String, cause: Throwable? = null) : AdbException(message, cause) diff --git a/dadb/src/test/kotlin/dadb/AdbExceptionTest.kt b/dadb/src/test/kotlin/dadb/AdbExceptionTest.kt new file mode 100644 index 0000000..3d568e9 --- /dev/null +++ b/dadb/src/test/kotlin/dadb/AdbExceptionTest.kt @@ -0,0 +1,49 @@ +/* + * Copyright (c) 2021 mobile.dev inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package dadb + +import com.google.common.truth.Truth.assertThat +import java.io.IOException +import kotlin.test.Test + +internal class AdbExceptionTest { + + @Test + fun allTypesAreIOExceptions() { + val exceptions: List = listOf( + AdbConnectException("x"), + AdbAuthException("x"), + AdbStreamOpenException("shell:", "x"), + AdbConnectionClosedException("x"), + AdbProtocolException("x"), + ) + exceptions.forEach { assertThat(it).isInstanceOf(IOException::class.java) } + } + + @Test + fun streamOpenExceptionCarriesDestination() { + val e = AdbStreamOpenException("exec:cmd package install", "refused") + assertThat(e.destination).isEqualTo("exec:cmd package install") + } + + @Test + fun preservesCause() { + val cause = IOException("socket reset") + assertThat(AdbConnectException("x", cause).cause).isSameInstanceAs(cause) + } +} From 04c87509d86466f0929fa13e038064f7b9edf001 Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 12:43:47 -0700 Subject: [PATCH 07/32] feat: add operation result types Co-Authored-By: Claude Sonnet 4.6 --- dadb/src/main/kotlin/dadb/InstallResult.kt | 28 +++++++++++ dadb/src/main/kotlin/dadb/RootResult.kt | 24 ++++++++++ dadb/src/main/kotlin/dadb/SyncResult.kt | 24 ++++++++++ dadb/src/main/kotlin/dadb/UninstallResult.kt | 25 ++++++++++ dadb/src/test/kotlin/dadb/ResultTypesTest.kt | 50 ++++++++++++++++++++ 5 files changed, 151 insertions(+) create mode 100644 dadb/src/main/kotlin/dadb/InstallResult.kt create mode 100644 dadb/src/main/kotlin/dadb/RootResult.kt create mode 100644 dadb/src/main/kotlin/dadb/SyncResult.kt create mode 100644 dadb/src/main/kotlin/dadb/UninstallResult.kt create mode 100644 dadb/src/test/kotlin/dadb/ResultTypesTest.kt diff --git a/dadb/src/main/kotlin/dadb/InstallResult.kt b/dadb/src/main/kotlin/dadb/InstallResult.kt new file mode 100644 index 0000000..c09c9ae --- /dev/null +++ b/dadb/src/main/kotlin/dadb/InstallResult.kt @@ -0,0 +1,28 @@ +/* + * Copyright (c) 2021 mobile.dev inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package dadb + +/** Outcome of [Dadb.install] / [Dadb.installMultiple]. */ +sealed interface InstallResult { + object Success : InstallResult + /** Raw pm/`cmd package` verdict, surfaced opaquely — exactly as the canonical adb client does + * (client/adb_install.cpp checks `strncmp("Success", buf, 7)` and prints the rest verbatim). + * `INSTALL_FAILED_*` codes are a frameworks/base concept, not the adbd contract, so they are + * not parsed into an enum. */ + data class Failure(val reason: String) : InstallResult +} diff --git a/dadb/src/main/kotlin/dadb/RootResult.kt b/dadb/src/main/kotlin/dadb/RootResult.kt new file mode 100644 index 0000000..54edc20 --- /dev/null +++ b/dadb/src/main/kotlin/dadb/RootResult.kt @@ -0,0 +1,24 @@ +/* + * Copyright (c) 2021 mobile.dev inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package dadb + +/** Outcome of [Dadb.root] / [Dadb.unroot]. [Failure.reason] is the adbd root:/unroot: response line. */ +sealed interface RootResult { + object Success : RootResult + data class Failure(val reason: String) : RootResult +} diff --git a/dadb/src/main/kotlin/dadb/SyncResult.kt b/dadb/src/main/kotlin/dadb/SyncResult.kt new file mode 100644 index 0000000..391237f --- /dev/null +++ b/dadb/src/main/kotlin/dadb/SyncResult.kt @@ -0,0 +1,24 @@ +/* + * Copyright (c) 2021 mobile.dev inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package dadb + +/** Outcome of [Dadb.push] / [Dadb.pull]. [Failure.reason] is the adbd sync FAIL message. */ +sealed interface SyncResult { + object Success : SyncResult + data class Failure(val reason: String) : SyncResult +} diff --git a/dadb/src/main/kotlin/dadb/UninstallResult.kt b/dadb/src/main/kotlin/dadb/UninstallResult.kt new file mode 100644 index 0000000..d16155f --- /dev/null +++ b/dadb/src/main/kotlin/dadb/UninstallResult.kt @@ -0,0 +1,25 @@ +/* + * Copyright (c) 2021 mobile.dev inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package dadb + +/** Outcome of [Dadb.uninstall]. `uninstall` is `cmd package uninstall` over the shell service, so + * [Failure] preserves adbd's actual process exit code and combined output — the wrapper hides nothing. */ +sealed interface UninstallResult { + object Success : UninstallResult + data class Failure(val reason: String, val exitCode: Int) : UninstallResult +} diff --git a/dadb/src/test/kotlin/dadb/ResultTypesTest.kt b/dadb/src/test/kotlin/dadb/ResultTypesTest.kt new file mode 100644 index 0000000..6d887d1 --- /dev/null +++ b/dadb/src/test/kotlin/dadb/ResultTypesTest.kt @@ -0,0 +1,50 @@ +/* + * Copyright (c) 2021 mobile.dev inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package dadb + +import com.google.common.truth.Truth.assertThat +import kotlin.test.Test + +internal class ResultTypesTest { + + @Test + fun installResultVariants() { + val ok: InstallResult = InstallResult.Success + val fail: InstallResult = InstallResult.Failure("Failure [INSTALL_FAILED_INVALID_APK]") + assertThat(ok).isInstanceOf(InstallResult.Success::class.java) + assertThat((fail as InstallResult.Failure).reason).contains("INSTALL_FAILED") + } + + @Test + fun uninstallFailureCarriesExitCodeAndReason() { + val fail = UninstallResult.Failure(reason = "Failure [DELETE_FAILED_INTERNAL_ERROR]", exitCode = 1) + assertThat(fail.exitCode).isEqualTo(1) + assertThat(fail.reason).contains("DELETE_FAILED") + } + + @Test + fun syncAndRootVariants() { + assertThat(SyncResult.Failure("No such file or directory").reason).contains("No such file") + assertThat(RootResult.Failure("adbd cannot run as root in production builds").reason) + .contains("production builds") + val syncOk: SyncResult = SyncResult.Success + val rootOk: RootResult = RootResult.Success + assertThat(syncOk).isInstanceOf(SyncResult.Success::class.java) + assertThat(rootOk).isInstanceOf(RootResult.Success::class.java) + } +} From c334300f142d4616afa07ae8cd789ce16dedb7e0 Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 12:45:53 -0700 Subject: [PATCH 08/32] test: add FakeDadb fixture and wire byte-builders Co-Authored-By: Claude Opus 4.8 (1M context) --- dadb/src/test/kotlin/dadb/FakeDadb.kt | 80 +++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 dadb/src/test/kotlin/dadb/FakeDadb.kt diff --git a/dadb/src/test/kotlin/dadb/FakeDadb.kt b/dadb/src/test/kotlin/dadb/FakeDadb.kt new file mode 100644 index 0000000..eaf274e --- /dev/null +++ b/dadb/src/test/kotlin/dadb/FakeDadb.kt @@ -0,0 +1,80 @@ +/* + * Copyright (c) 2021 mobile.dev inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package dadb + +import okio.Buffer +import java.nio.charset.StandardCharsets + +/** An [AdbStream] backed by in-memory buffers. [source] is pre-loaded with device bytes; + * [sink] captures whatever the code under test writes. */ +internal class FakeAdbStream( + override val source: Buffer, + override val sink: Buffer = Buffer() +) : AdbStream { + override fun close() {} +} + +/** A [Dadb] whose `open(destination)` returns a caller-supplied [AdbStream]. Records every + * destination opened, in order, in [opened]. */ +internal class FakeDadb( + private val features: Set = setOf("shell_v2", "cmd"), + private val streamFor: (destination: String) -> AdbStream +) : Dadb { + val opened = mutableListOf() + + override fun open(destination: String): AdbStream { + opened += destination + return streamFor(destination) + } + + override fun supportsFeature(feature: String) = feature in features + + override fun close() {} +} + +/** Builds a shell,v2 byte stream: optional stdout/stderr packets followed by an exit packet. */ +internal fun shellV2Buffer(stdout: String = "", stderr: String = "", exitCode: Int): Buffer { + val buffer = Buffer() + if (stdout.isNotEmpty()) { + val bytes = stdout.toByteArray() + buffer.writeByte(ID_STDOUT); buffer.writeIntLe(bytes.size); buffer.write(bytes) + } + if (stderr.isNotEmpty()) { + val bytes = stderr.toByteArray() + buffer.writeByte(ID_STDERR); buffer.writeIntLe(bytes.size); buffer.write(bytes) + } + buffer.writeByte(ID_EXIT); buffer.writeIntLe(1); buffer.writeByte(exitCode) + return buffer +} + +/** Builds a single sync `FAIL` packet (4-char id + LE length + message). */ +internal fun syncFailBuffer(message: String): Buffer { + val buffer = Buffer() + buffer.writeString("FAIL", StandardCharsets.UTF_8) + buffer.writeIntLe(message.toByteArray().size) + buffer.writeString(message, StandardCharsets.UTF_8) + return buffer +} + +/** Builds a single sync `OKAY` packet (success terminator for SEND). */ +internal fun syncOkayBuffer(): Buffer { + val buffer = Buffer() + buffer.writeString("OKAY", StandardCharsets.UTF_8) + buffer.writeIntLe(0) + return buffer +} From d33acc8948de71d87e65dbaaf63805018ef7417e Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 12:48:30 -0700 Subject: [PATCH 09/32] fix: surface mid-stream socket faults as AdbConnectionClosedException Split the single IOException catch in AdbStreamImpl.nextMessage into two handlers: AdbStreamClosed (peer A_CLSE) remains a clean EOF returning null, while any other IOException (socket EOF/RST) now throws AdbConnectionClosedException so callers can distinguish a dropped transport from a normal end-of-stream. Also updated testLargeRemoteWrite to append a proper A_CLSE packet after the WRTE, matching real device behaviour. Co-Authored-By: Claude Sonnet 4.6 --- dadb/src/main/kotlin/dadb/AdbStream.kt | 7 ++++- dadb/src/test/kotlin/dadb/AdbStreamTest.kt | 31 ++++++++++++++++++++-- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/dadb/src/main/kotlin/dadb/AdbStream.kt b/dadb/src/main/kotlin/dadb/AdbStream.kt index dfec53c..a75edb3 100644 --- a/dadb/src/main/kotlin/dadb/AdbStream.kt +++ b/dadb/src/main/kotlin/dadb/AdbStream.kt @@ -117,9 +117,14 @@ internal class AdbStreamImpl internal constructor( private fun nextMessage(command: Int): AdbMessage? { return try { messageQueue.take(localId, command) + } catch (e: AdbStreamClosed) { + // Peer sent A_CLSE: normal end-of-stream (e.g. shell/exec output finished). EOF is correct. + close() + null } catch (e: IOException) { + // Real socket fault (EOF/RST) mid-stream: the transport died, not a clean close. close() - return null + throw AdbConnectionClosedException("Connection lost while reading stream $localId", e) } } diff --git a/dadb/src/test/kotlin/dadb/AdbStreamTest.kt b/dadb/src/test/kotlin/dadb/AdbStreamTest.kt index a027269..518e55f 100644 --- a/dadb/src/test/kotlin/dadb/AdbStreamTest.kt +++ b/dadb/src/test/kotlin/dadb/AdbStreamTest.kt @@ -20,6 +20,7 @@ package dadb import com.google.common.truth.Truth import okio.Buffer import kotlin.test.Test +import kotlin.test.assertFailsWith internal class AdbStreamTest { @@ -33,9 +34,35 @@ internal class AdbStreamTest { Truth.assertThat(stream.source.readByteArray()).isEqualTo(payload) } + @Test + fun peerCloseIsCleanEof() { + // Reader yields a CLSE for localId=1 and nothing else. + val buffer = Buffer() + AdbWriter(buffer).write(Constants.CMD_CLSE, 2, 1, null, 0, 0) + val messageQueue = AdbMessageQueue(AdbReader(buffer)) + messageQueue.startListening(1) + val stream = AdbStreamImpl(messageQueue, AdbWriter(Buffer()), 1024, 1, 2) + + // A clean peer close reads as EOF (empty), NOT an exception. + Truth.assertThat(stream.source.readByteArray()).isEqualTo(ByteArray(0)) + } + + @Test + fun socketFaultThrowsConnectionClosed() { + // Empty reader: the next read hits EOF on the socket itself (not a protocol CLSE). + val messageQueue = AdbMessageQueue(AdbReader(Buffer())) + messageQueue.startListening(1) + val stream = AdbStreamImpl(messageQueue, AdbWriter(Buffer()), 1024, 1, 2) + + assertFailsWith { stream.source.readByteArray() } + } + private fun createAdbReader(localId: Int, remoteId: Int, writePayload: ByteArray): AdbReader { val source = Buffer() - AdbWriter(source).write(Constants.CMD_WRTE, remoteId, localId, writePayload, 0, writePayload.size) + val writer = AdbWriter(source) + writer.write(Constants.CMD_WRTE, remoteId, localId, writePayload, 0, writePayload.size) + // Append a proper A_CLSE so the stream ends cleanly, matching real device behaviour. + writer.write(Constants.CMD_CLSE, remoteId, localId, null, 0, 0) return AdbReader(source) } -} \ No newline at end of file +} From 6faccd2475a78d45691c278dd53a350bf52564b3 Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 12:53:35 -0700 Subject: [PATCH 10/32] feat: map adbd handshake failures to typed AdbException subtypes Co-Authored-By: Claude Sonnet 4.6 --- dadb/src/main/kotlin/dadb/AdbConnection.kt | 22 ++++++-- .../kotlin/dadb/AdbConnectionHandshakeTest.kt | 53 +++++++++++++++++++ 2 files changed, 70 insertions(+), 5 deletions(-) create mode 100644 dadb/src/test/kotlin/dadb/AdbConnectionHandshakeTest.kt diff --git a/dadb/src/main/kotlin/dadb/AdbConnection.kt b/dadb/src/main/kotlin/dadb/AdbConnection.kt index 57cd17e..ca5b9bf 100644 --- a/dadb/src/main/kotlin/dadb/AdbConnection.kt +++ b/dadb/src/main/kotlin/dadb/AdbConnection.kt @@ -83,12 +83,20 @@ internal class AdbConnection internal constructor( return connect(source, sink, keyPair, socket) } - private fun connect(source: Source, sink: Sink, keyPair: AdbKeyPair? = null, closeable: Closeable? = null): AdbConnection { + internal fun connect(source: Source, sink: Sink, keyPair: AdbKeyPair? = null, closeable: Closeable? = null): AdbConnection { val adbReader = AdbReader(source) val adbWriter = AdbWriter(sink) try { return connect(adbReader, adbWriter, keyPair, closeable) + } catch (e: AdbException) { + adbReader.close() + adbWriter.close() + throw e + } catch (e: IOException) { + adbReader.close() + adbWriter.close() + throw AdbConnectException("Connection handshake failed", e) } catch (t: Throwable) { adbReader.close() adbWriter.close() @@ -102,8 +110,8 @@ internal class AdbConnection internal constructor( var message = adbReader.readMessage() if (message.command == Constants.CMD_AUTH) { - checkNotNull(keyPair) { "Authentication required but no KeyPair provided" } - check(message.arg0 == Constants.AUTH_TYPE_TOKEN) { "Unsupported auth type: $message" } + if (keyPair == null) throw AdbAuthException("Authentication required but no key pair was provided") + if (message.arg0 != Constants.AUTH_TYPE_TOKEN) throw AdbProtocolException("Unsupported auth type: $message") val signature = keyPair.signPayload(message) adbWriter.writeAuth(Constants.AUTH_TYPE_SIGNATURE, signature) @@ -115,7 +123,11 @@ internal class AdbConnection internal constructor( } } - if (message.command != Constants.CMD_CNXN) throw IOException("Connection failed: $message") + if (message.command != Constants.CMD_CNXN) { + // A trailing AUTH means the device rejected our key / stayed unauthorized. + if (message.command == Constants.CMD_AUTH) throw AdbAuthException("Device rejected authentication (unauthorized)") + throw AdbConnectException("Connection failed: $message") + } val connectionString = parseConnectionString(String(message.payload)) val version = message.arg0 @@ -131,7 +143,7 @@ internal class AdbConnection internal constructor( .map { it.split("=") } .mapNotNull { if (it.size != 2) null else it[0] to it[1] } .toMap() - if ("features" !in keyValues) throw IOException("Failed to parse features from connection string: $connectionString") + if ("features" !in keyValues) throw AdbConnectException("Failed to parse features from connection string: $connectionString") val features = keyValues.getValue("features").split(",").toSet() return ConnectionString(features) } diff --git a/dadb/src/test/kotlin/dadb/AdbConnectionHandshakeTest.kt b/dadb/src/test/kotlin/dadb/AdbConnectionHandshakeTest.kt new file mode 100644 index 0000000..50404e6 --- /dev/null +++ b/dadb/src/test/kotlin/dadb/AdbConnectionHandshakeTest.kt @@ -0,0 +1,53 @@ +/* + * Copyright (c) 2021 mobile.dev inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package dadb + +import okio.Buffer +import kotlin.test.Test +import kotlin.test.assertFailsWith + +internal class AdbConnectionHandshakeTest { + + @Test + fun nonCnxnResponseThrowsConnectException() { + // Device replies with an unexpected OKAY instead of CNXN. + val device = Buffer() + AdbWriter(device).writeOkay(0, 0) + assertFailsWith { + AdbConnection.connect(device, Buffer(), null) + } + } + + @Test + fun authRequiredWithoutKeyPairThrowsAuthException() { + // Device demands auth; we have no key pair. + val device = Buffer() + AdbWriter(device).writeAuth(Constants.AUTH_TYPE_TOKEN, ByteArray(20)) + assertFailsWith { + AdbConnection.connect(device, Buffer(), null) + } + } + + @Test + fun truncatedHandshakeThrowsConnectException() { + // Empty device stream: the first readMessage hits EOF. + assertFailsWith { + AdbConnection.connect(Buffer(), Buffer(), null) + } + } +} From 57c0320be1fe5fc178ccbec79da57c8f30a36805 Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 12:54:13 -0700 Subject: [PATCH 11/32] feat: map A_OPEN refusal to AdbStreamOpenException Co-Authored-By: Claude Sonnet 4.6 --- dadb/src/main/kotlin/dadb/AdbConnection.kt | 4 ++++ dadb/src/test/kotlin/dadb/DadbTest.kt | 10 ++++++++++ 2 files changed, 14 insertions(+) diff --git a/dadb/src/main/kotlin/dadb/AdbConnection.kt b/dadb/src/main/kotlin/dadb/AdbConnection.kt index ca5b9bf..0d6d978 100644 --- a/dadb/src/main/kotlin/dadb/AdbConnection.kt +++ b/dadb/src/main/kotlin/dadb/AdbConnection.kt @@ -48,6 +48,10 @@ internal class AdbConnection internal constructor( val message = messageQueue.take(localId, Constants.CMD_OKAY) val remoteId = message.arg0 return AdbStreamImpl(messageQueue, adbWriter, maxPayloadSize, localId, remoteId) + } catch (e: AdbStreamClosed) { + // adbd answered A_OPEN with A_CLSE: it refused this service. Connection is still alive. + messageQueue.stopListening(localId) + throw AdbStreamOpenException(destination, "adbd refused to open stream: $destination", e) } catch (e: Throwable) { messageQueue.stopListening(localId) throw e diff --git a/dadb/src/test/kotlin/dadb/DadbTest.kt b/dadb/src/test/kotlin/dadb/DadbTest.kt index a656b26..76425b0 100644 --- a/dadb/src/test/kotlin/dadb/DadbTest.kt +++ b/dadb/src/test/kotlin/dadb/DadbTest.kt @@ -36,6 +36,7 @@ import kotlin.test.AfterTest import kotlin.test.BeforeTest import kotlin.test.Ignore import kotlin.test.Test +import kotlin.test.assertFailsWith internal abstract class DadbTest : BaseConcurrencyTest() { @@ -319,6 +320,15 @@ internal abstract class DadbTest : BaseConcurrencyTest() { } } + @Test + fun open_invalidService_throwsStreamOpenException() { + localEmulator { dadb -> + assertFailsWith { + dadb.open("definitely-not-a-real-service:") + } + } + } + protected abstract fun localEmulator(body: (dadb: Dadb) -> Unit) private fun readSocketAsync(host: String, port: Int): Future { From a48a64249b295d5974d08d8d4080e4dd6813d899 Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 13:01:16 -0700 Subject: [PATCH 12/32] feat: AdbSync throws internal AdbSyncFailException on FAIL Replace bare IOExceptions in AdbSyncStream.send/recv with a typed AdbSyncFailException so callers can distinguish FAIL (operation failure, transport healthy) from protocol/connection errors. Co-Authored-By: Claude Sonnet 4.6 --- dadb/src/main/kotlin/dadb/AdbSync.kt | 15 +++++++-- dadb/src/test/kotlin/dadb/DadbResultTest.kt | 34 +++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 dadb/src/test/kotlin/dadb/DadbResultTest.kt diff --git a/dadb/src/main/kotlin/dadb/AdbSync.kt b/dadb/src/main/kotlin/dadb/AdbSync.kt index 821145d..14e1762 100644 --- a/dadb/src/main/kotlin/dadb/AdbSync.kt +++ b/dadb/src/main/kotlin/dadb/AdbSync.kt @@ -22,6 +22,11 @@ import java.io.IOException import java.nio.charset.StandardCharsets import kotlin.jvm.Throws +/** Internal signal that an adbd sync operation returned FAIL (e.g. file not found / permission + * denied). Caught by Dadb.push/pull and folded into a [SyncResult.Failure]; never surfaced raw + * from the high-level API. Not an AdbException — the transport is healthy. */ +internal class AdbSyncFailException(val reason: String) : IOException(reason) + internal const val LIST = "LIST" internal const val RECV = "RECV" internal const val SEND = "SEND" @@ -67,7 +72,11 @@ class AdbSyncStream( stream.sink.flush() val packet = readPacket() - if (packet.id != OKAY) throw IOException("Unexpected sync packet id: ${packet.id}") + if (packet.id == FAIL) { + val message = stream.source.readString(packet.arg.toLong(), StandardCharsets.UTF_8) + throw AdbSyncFailException(message) + } + if (packet.id != OKAY) throw AdbProtocolException("Unexpected sync packet id: ${packet.id}") } @Throws(IOException::class) @@ -85,9 +94,9 @@ class AdbSyncStream( if (packet.id == DONE) break if (packet.id == FAIL) { val message = stream.source.readString(packet.arg.toLong(), StandardCharsets.UTF_8) - throw IOException("Sync failed: $message") + throw AdbSyncFailException(message) } - if (packet.id != DATA) throw IOException("Unexpected sync packet id: ${packet.id}") + if (packet.id != DATA) throw AdbProtocolException("Unexpected sync packet id: ${packet.id}") val chunkSize = packet.arg stream.source.readFully(buffer, chunkSize.toLong()) buffer.readAll(sink) diff --git a/dadb/src/test/kotlin/dadb/DadbResultTest.kt b/dadb/src/test/kotlin/dadb/DadbResultTest.kt new file mode 100644 index 0000000..cff1fdd --- /dev/null +++ b/dadb/src/test/kotlin/dadb/DadbResultTest.kt @@ -0,0 +1,34 @@ +/* + * Copyright (c) 2021 mobile.dev inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package dadb + +import com.google.common.truth.Truth.assertThat +import okio.Buffer +import kotlin.test.Test +import kotlin.test.assertFailsWith + +internal class DadbResultTest { + + @Test + fun syncRecvFailThrowsSyncFailException() { + val stream = FakeAdbStream(syncFailBuffer("No such file or directory")) + val sync = AdbSyncStream(stream) + val e = assertFailsWith { sync.recv(Buffer(), "/missing") } + assertThat(e.reason).isEqualTo("No such file or directory") + } +} From 6a1a88a15f2a2f797b338e1f38630f29c108f619 Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 13:05:01 -0700 Subject: [PATCH 13/32] feat: push/pull return SyncResult --- dadb/src/main/kotlin/dadb/Dadb.kt | 36 +++++++++++++-------- dadb/src/test/kotlin/dadb/DadbResultTest.kt | 15 +++++++++ 2 files changed, 38 insertions(+), 13 deletions(-) diff --git a/dadb/src/main/kotlin/dadb/Dadb.kt b/dadb/src/main/kotlin/dadb/Dadb.kt index afe8524..cc2d9ee 100644 --- a/dadb/src/main/kotlin/dadb/Dadb.kt +++ b/dadb/src/main/kotlin/dadb/Dadb.kt @@ -43,31 +43,41 @@ interface Dadb : AutoCloseable { return AdbShellStream(stream) } - @Throws(IOException::class) - fun push(src: File, remotePath: String, mode: Int = readMode(src), lastModifiedMs: Long = src.lastModified()) { - push(src.source(), remotePath, mode, lastModifiedMs) + @Throws(AdbException::class) + fun push(src: File, remotePath: String, mode: Int = readMode(src), lastModifiedMs: Long = src.lastModified()): SyncResult { + return push(src.source(), remotePath, mode, lastModifiedMs) } - @Throws(IOException::class) - fun push(source: Source, remotePath: String, mode: Int, lastModifiedMs: Long) { + @Throws(AdbException::class) + fun push(source: Source, remotePath: String, mode: Int, lastModifiedMs: Long): SyncResult { openSync().use { stream -> - stream.send(source, remotePath, mode, lastModifiedMs) + return try { + stream.send(source, remotePath, mode, lastModifiedMs) + SyncResult.Success + } catch (e: AdbSyncFailException) { + SyncResult.Failure(e.reason) + } } } - @Throws(IOException::class) - fun pull(dst: File, remotePath: String) { - pull(dst.sink(append = false), remotePath) + @Throws(AdbException::class) + fun pull(dst: File, remotePath: String): SyncResult { + return pull(dst.sink(append = false), remotePath) } - @Throws(IOException::class) - fun pull(sink: Sink, remotePath: String) { + @Throws(AdbException::class) + fun pull(sink: Sink, remotePath: String): SyncResult { openSync().use { stream -> - stream.recv(sink, remotePath) + return try { + stream.recv(sink, remotePath) + SyncResult.Success + } catch (e: AdbSyncFailException) { + SyncResult.Failure(e.reason) + } } } - @Throws(IOException::class) + @Throws(AdbException::class) fun openSync(): AdbSyncStream { val stream = open("sync:") return AdbSyncStream(stream) diff --git a/dadb/src/test/kotlin/dadb/DadbResultTest.kt b/dadb/src/test/kotlin/dadb/DadbResultTest.kt index cff1fdd..75f5f9f 100644 --- a/dadb/src/test/kotlin/dadb/DadbResultTest.kt +++ b/dadb/src/test/kotlin/dadb/DadbResultTest.kt @@ -31,4 +31,19 @@ internal class DadbResultTest { val e = assertFailsWith { sync.recv(Buffer(), "/missing") } assertThat(e.reason).isEqualTo("No such file or directory") } + + @Test + fun pullMissingFileReturnsSyncFailure() { + val dadb = FakeDadb { FakeAdbStream(syncFailBuffer("No such file or directory")) } + val result = dadb.pull(Buffer(), "/missing") + assertThat(result).isInstanceOf(SyncResult.Failure::class.java) + assertThat((result as SyncResult.Failure).reason).isEqualTo("No such file or directory") + } + + @Test + fun pushSuccessReturnsSyncSuccess() { + val dadb = FakeDadb { FakeAdbStream(syncOkayBuffer()) } + val result = dadb.push(Buffer().also { it.writeUtf8("hi") }, "/data/local/tmp/hi", 0b110_100_100, 0L) + assertThat(result).isInstanceOf(SyncResult.Success::class.java) + } } From b03f84bae1d92ab2ee535341d5565dabe4c82ee1 Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 13:05:31 -0700 Subject: [PATCH 14/32] feat: uninstall returns UninstallResult --- dadb/src/main/kotlin/dadb/Dadb.kt | 10 ++++++---- dadb/src/test/kotlin/dadb/DadbResultTest.kt | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/dadb/src/main/kotlin/dadb/Dadb.kt b/dadb/src/main/kotlin/dadb/Dadb.kt index cc2d9ee..adbe385 100644 --- a/dadb/src/main/kotlin/dadb/Dadb.kt +++ b/dadb/src/main/kotlin/dadb/Dadb.kt @@ -208,11 +208,13 @@ interface Dadb : AutoCloseable { } } - @Throws(IOException::class) - fun uninstall(packageName: String) { + @Throws(AdbException::class) + fun uninstall(packageName: String): UninstallResult { val response = shell("cmd package uninstall $packageName") - if (response.exitCode != 0) { - throw IOException("Uninstall failed: ${response.allOutput}") + return if (response.exitCode == 0) { + UninstallResult.Success + } else { + UninstallResult.Failure(reason = response.allOutput, exitCode = response.exitCode) } } diff --git a/dadb/src/test/kotlin/dadb/DadbResultTest.kt b/dadb/src/test/kotlin/dadb/DadbResultTest.kt index 75f5f9f..23bcef2 100644 --- a/dadb/src/test/kotlin/dadb/DadbResultTest.kt +++ b/dadb/src/test/kotlin/dadb/DadbResultTest.kt @@ -46,4 +46,22 @@ internal class DadbResultTest { val result = dadb.push(Buffer().also { it.writeUtf8("hi") }, "/data/local/tmp/hi", 0b110_100_100, 0L) assertThat(result).isInstanceOf(SyncResult.Success::class.java) } + + @Test + fun uninstallNonZeroExitReturnsFailureWithExitCode() { + val dadb = FakeDadb { + FakeAdbStream(shellV2Buffer(stdout = "Failure [DELETE_FAILED_INTERNAL_ERROR]\n", exitCode = 1)) + } + val result = dadb.uninstall("com.example.absent") + assertThat(result).isInstanceOf(UninstallResult.Failure::class.java) + result as UninstallResult.Failure + assertThat(result.exitCode).isEqualTo(1) + assertThat(result.reason).contains("DELETE_FAILED_INTERNAL_ERROR") + } + + @Test + fun uninstallZeroExitReturnsSuccess() { + val dadb = FakeDadb { FakeAdbStream(shellV2Buffer(stdout = "Success\n", exitCode = 0)) } + assertThat(dadb.uninstall("com.example")).isInstanceOf(UninstallResult.Success::class.java) + } } From f65d2fe14d1fbac636ac0a89c2153ef6ea644f1e Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 13:06:01 -0700 Subject: [PATCH 15/32] feat: install returns InstallResult and checks the pm-install result --- dadb/src/main/kotlin/dadb/Dadb.kt | 29 +++++++++++---------- dadb/src/test/kotlin/dadb/DadbResultTest.kt | 19 ++++++++++++++ 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/dadb/src/main/kotlin/dadb/Dadb.kt b/dadb/src/main/kotlin/dadb/Dadb.kt index adbe385..55122fe 100644 --- a/dadb/src/main/kotlin/dadb/Dadb.kt +++ b/dadb/src/main/kotlin/dadb/Dadb.kt @@ -83,40 +83,41 @@ interface Dadb : AutoCloseable { return AdbSyncStream(stream) } - @Throws(IOException::class) - fun install(file: File, vararg options: String) { - if (supportsFeature("cmd")) { + @Throws(AdbException::class) + fun install(file: File, vararg options: String): InstallResult { + return if (supportsFeature("cmd")) { install(file.source(), file.length(), *options) } else { pmInstall(file, *options) } } - @Throws(IOException::class) - fun install(source: Source, size: Long, vararg options: String) { + @Throws(AdbException::class) + fun install(source: Source, size: Long, vararg options: String): InstallResult { if (supportsFeature("cmd")) { execCmd("package", "install", "-S", size.toString(), *options).use { stream -> stream.sink.writeAll(source) stream.sink.flush() val response = stream.source.readString(Charsets.UTF_8) - if (!response.startsWith("Success")) { - throw IOException("Install failed: $response") - } + return if (response.startsWith("Success")) InstallResult.Success else InstallResult.Failure(response) } } else { val tempFile = kotlin.io.path.createTempFile() val fileSink = tempFile.sink().buffer() fileSink.writeAll(source) fileSink.flush() - pmInstall(tempFile.toFile(), *options) + return pmInstall(tempFile.toFile(), *options) } } - private fun pmInstall(file: File, vararg options: String) { - val fileName = file.name - val remotePath = "/data/local/tmp/$fileName" - push(file, remotePath) - shell("pm install ${options.joinToString(" ")} \"$remotePath\"") + private fun pmInstall(file: File, vararg options: String): InstallResult { + val remotePath = "/data/local/tmp/${file.name}" + when (val pushResult = push(file, remotePath)) { + is SyncResult.Failure -> return InstallResult.Failure("push failed: ${pushResult.reason}") + SyncResult.Success -> Unit + } + val response = shell("pm install ${options.joinToString(" ")} \"$remotePath\"") + return if (response.allOutput.startsWith("Success")) InstallResult.Success else InstallResult.Failure(response.allOutput) } @Throws(IOException::class) diff --git a/dadb/src/test/kotlin/dadb/DadbResultTest.kt b/dadb/src/test/kotlin/dadb/DadbResultTest.kt index 23bcef2..e7ffb62 100644 --- a/dadb/src/test/kotlin/dadb/DadbResultTest.kt +++ b/dadb/src/test/kotlin/dadb/DadbResultTest.kt @@ -64,4 +64,23 @@ internal class DadbResultTest { val dadb = FakeDadb { FakeAdbStream(shellV2Buffer(stdout = "Success\n", exitCode = 0)) } assertThat(dadb.uninstall("com.example")).isInstanceOf(UninstallResult.Success::class.java) } + + @Test + fun installCmdPathFailureReturnsInstallFailure() { + val dadb = FakeDadb(features = setOf("cmd")) { + FakeAdbStream(Buffer().also { it.writeUtf8("Failure [INSTALL_FAILED_INVALID_APK]") }) + } + val result = dadb.install(Buffer().also { it.writeUtf8("apk") }, 3L) + assertThat(result).isInstanceOf(InstallResult.Failure::class.java) + assertThat((result as InstallResult.Failure).reason).contains("INSTALL_FAILED_INVALID_APK") + } + + @Test + fun installCmdPathSuccessReturnsSuccess() { + val dadb = FakeDadb(features = setOf("cmd")) { + FakeAdbStream(Buffer().also { it.writeUtf8("Success\n") }) + } + val result = dadb.install(Buffer().also { it.writeUtf8("apk") }, 3L) + assertThat(result).isInstanceOf(InstallResult.Success::class.java) + } } From 1ea91cc04bea15eba0faa9d12186a661eb6de5c8 Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 13:08:14 -0700 Subject: [PATCH 16/32] feat: installMultiple returns InstallResult; fix finalize message bug --- dadb/src/main/kotlin/dadb/Dadb.kt | 61 +++++++++------------------ dadb/src/test/kotlin/dadb/DadbTest.kt | 40 +++++++++--------- 2 files changed, 41 insertions(+), 60 deletions(-) diff --git a/dadb/src/main/kotlin/dadb/Dadb.kt b/dadb/src/main/kotlin/dadb/Dadb.kt index 55122fe..298554c 100644 --- a/dadb/src/main/kotlin/dadb/Dadb.kt +++ b/dadb/src/main/kotlin/dadb/Dadb.kt @@ -120,26 +120,24 @@ interface Dadb : AutoCloseable { return if (response.allOutput.startsWith("Success")) InstallResult.Success else InstallResult.Failure(response.allOutput) } - @Throws(IOException::class) - fun installMultiple(apks: List, vararg options: String) { + @Throws(AdbException::class) + fun installMultiple(apks: List, vararg options: String): InstallResult { // http://aospxref.com/android-12.0.0_r3/xref/packages/modules/adb/client/adb_install.cpp#538 if (supportsFeature("cmd")) { - val totalLength = apks.map { it.length() }.reduce { acc, l -> acc + l } + val totalLength = apks.map { it.length() }.reduce { acc, l -> acc + l } execCmd("package", "install-create", "-S", totalLength.toString(), *options).use { createStream -> val response = createStream.source.readString(Charsets.UTF_8) - if (!response.startsWith("Success")) { - throw IOException("connect error for create: $response") - } + if (!response.startsWith("Success")) return InstallResult.Failure("create session failed: $response") + val pattern = """\[(\w+)]""".toRegex() - val sessionId = pattern.find(response)?.groups?.get(1)?.value ?: throw IOException("failed to create session") + val sessionId = pattern.find(response)?.groups?.get(1)?.value + ?: return InstallResult.Failure("failed to parse session id: $response") var error: String? = null - apks.forEach { apk-> - // install write every apk file to stream - execCmd("package", "install-write", "-S", apk.length().toString(), sessionId, apk.name, "-", *options).use { writeStream-> + apks.forEach { apk -> + execCmd("package", "install-write", "-S", apk.length().toString(), sessionId, apk.name, "-", *options).use { writeStream -> writeStream.sink.writeAll(apk.source()) writeStream.sink.flush() - val writeResponse = writeStream.source.readString(Charsets.UTF_8) if (!writeResponse.startsWith("Success")) { error = writeResponse @@ -148,48 +146,36 @@ interface Dadb : AutoCloseable { } } - // commit the session val finalCommand = if (error == null) "install-commit" else "install-abandon" - execCmd("package", finalCommand, sessionId, *options).use { commitStream-> + execCmd("package", finalCommand, sessionId, *options).use { commitStream -> val finalResponse = commitStream.source.readString(Charsets.UTF_8) - if (!finalResponse.startsWith("Success")) { - throw IOException("failed to finalize session: $commitStream") - } + if (!finalResponse.startsWith("Success")) return InstallResult.Failure("failed to finalize session: $finalResponse") } - if (error != null) { - throw IOException("Install failed: $error") - } + return if (error == null) InstallResult.Success else InstallResult.Failure(error!!) } } else { - val totalLength = apks.map { it.length() }.reduce { acc, l -> acc + l } - // step1: create session + val totalLength = apks.map { it.length() }.reduce { acc, l -> acc + l } val response = shell("pm install-create -S $totalLength ${options.joinToString(" ")}") - if (!response.allOutput.startsWith("Success")) { - throw IOException("pm create session failed: $response") - } + if (!response.allOutput.startsWith("Success")) return InstallResult.Failure("pm create session failed: ${response.allOutput}") val pattern = """\[(\w+)]""".toRegex() - val sessionId = pattern.find(response.allOutput)?.groups?.get(1)?.value ?: throw IOException("failed to create session") + val sessionId = pattern.find(response.allOutput)?.groups?.get(1)?.value + ?: return InstallResult.Failure("failed to parse session id: ${response.allOutput}") var error: String? = null val fileNames = apks.map { it.name } val remotePaths = fileNames.map { "/data/local/tmp/$it" } - // step2: write apk to the session apks.zip(remotePaths).forEachIndexed { index, pair -> val apk = pair.first val remotePath = pair.second - try { - // we should push the apk files to device, when push failed, it would stop the installation - push(apk, remotePath) - } catch (t: IOException) { - error = t.message - return@forEachIndexed + when (val pushResult = push(apk, remotePath)) { + is SyncResult.Failure -> { error = "push failed: ${pushResult.reason}"; return@forEachIndexed } + SyncResult.Success -> Unit } - // pm install-write -S APK_SIZE SESSION_ID INDEX PATH val writeResponse = shell("pm install-write -S ${apk.length()} $sessionId $index $remotePath") if (!writeResponse.allOutput.startsWith("Success")) { error = writeResponse.allOutput @@ -197,15 +183,10 @@ interface Dadb : AutoCloseable { } } - // step3: commit or abandon the session val finalCommand = if (error == null) "pm install-commit $sessionId" else "pm install-abandon $sessionId" val finalResponse = shell(finalCommand) - if (!finalResponse.allOutput.startsWith("Success")) { - throw IOException("failed to finalize session: $finalResponse") - } - if (error != null) { - throw IOException("Install failed: $error"); - } + if (!finalResponse.allOutput.startsWith("Success")) return InstallResult.Failure("failed to finalize session: ${finalResponse.allOutput}") + return if (error == null) InstallResult.Success else InstallResult.Failure(error!!) } } diff --git a/dadb/src/test/kotlin/dadb/DadbTest.kt b/dadb/src/test/kotlin/dadb/DadbTest.kt index 76425b0..4963c66 100644 --- a/dadb/src/test/kotlin/dadb/DadbTest.kt +++ b/dadb/src/test/kotlin/dadb/DadbTest.kt @@ -127,10 +127,10 @@ internal abstract class DadbTest : BaseConcurrencyTest() { val content = randomString() val source = ByteArrayInputStream(content.toByteArray()).source() - dadb.push(source, remotePath, 439, System.currentTimeMillis()) + assertThat(dadb.push(source, remotePath, 439, System.currentTimeMillis())).isInstanceOf(SyncResult.Success::class.java) val buffer = Buffer() - dadb.pull(buffer, remotePath) + assertThat(dadb.pull(buffer, remotePath)).isInstanceOf(SyncResult.Success::class.java) val pulledContent = buffer.readString(StandardCharsets.UTF_8) assertThat(pulledContent).isEqualTo(content) @@ -147,10 +147,10 @@ internal abstract class DadbTest : BaseConcurrencyTest() { dadb.shell("rm -rf $remoteDir") val source = ByteArrayInputStream(content.toByteArray()).source() - dadb.push(source, remotePath, 439, System.currentTimeMillis()) + assertThat(dadb.push(source, remotePath, 439, System.currentTimeMillis())).isInstanceOf(SyncResult.Success::class.java) val buffer = Buffer() - dadb.pull(buffer, remotePath) + assertThat(dadb.pull(buffer, remotePath)).isInstanceOf(SyncResult.Success::class.java) val pulledContent = buffer.readString(StandardCharsets.UTF_8) assertThat(pulledContent).isEqualTo(content) @@ -165,7 +165,7 @@ internal abstract class DadbTest : BaseConcurrencyTest() { dadb.shell("fallocate -l ${sizeMb}M $remotePath") val buffer = Buffer() - dadb.pull(buffer, remotePath) + assertThat(dadb.pull(buffer, remotePath)).isInstanceOf(SyncResult.Success::class.java) val pulledContent = buffer.readByteArray() assertThat(pulledContent).hasLength(sizeMb * 1024 * 1024) @@ -178,10 +178,10 @@ internal abstract class DadbTest : BaseConcurrencyTest() { val content = randomString() val localSrcFile = temporaryFolder.newFile().apply { writeText(content) } - dadb.push(localSrcFile, remotePathWithCJK, 439, System.currentTimeMillis()) + assertThat(dadb.push(localSrcFile, remotePathWithCJK, 439, System.currentTimeMillis())).isInstanceOf(SyncResult.Success::class.java) val localDstFile = temporaryFolder.newFile() - dadb.pull(localDstFile, remotePathWithCJK) + assertThat(dadb.pull(localDstFile, remotePathWithCJK)).isInstanceOf(SyncResult.Success::class.java) assertThat(localDstFile.readText()).isEqualTo(content) } @@ -193,10 +193,10 @@ internal abstract class DadbTest : BaseConcurrencyTest() { val content = randomString() val localSrcFile = temporaryFolder.newFile().apply { writeText(content) } - dadb.push(localSrcFile, remotePath, 439, System.currentTimeMillis()) + assertThat(dadb.push(localSrcFile, remotePath, 439, System.currentTimeMillis())).isInstanceOf(SyncResult.Success::class.java) val localDstFile = temporaryFolder.newFile() - dadb.pull(localDstFile, remotePath) + assertThat(dadb.pull(localDstFile, remotePath)).isInstanceOf(SyncResult.Success::class.java) assertThat(localDstFile.readText()).isEqualTo(content) } @@ -205,7 +205,7 @@ internal abstract class DadbTest : BaseConcurrencyTest() { @Test fun install() { localEmulator { dadb -> - dadb.install(TestApk.FILE) + assertThat(dadb.install(TestApk.FILE)).isInstanceOf(InstallResult.Success::class.java) val response = dadb.shell("pm list packages ${TestApk.PACKAGE_NAME}") assertShellResponse(response, 0, "package:${TestApk.PACKAGE_NAME}\n") } @@ -215,7 +215,7 @@ internal abstract class DadbTest : BaseConcurrencyTest() { fun installStream() { localEmulator { dadb -> val inputStream = FileInputStream(TestApk.FILE).source() - dadb.install(inputStream, TestApk.FILE.length()) + assertThat(dadb.install(inputStream, TestApk.FILE.length())).isInstanceOf(InstallResult.Success::class.java) val response = dadb.shell("pm list packages ${TestApk.PACKAGE_NAME}") assertShellResponse(response, 0, "package:${TestApk.PACKAGE_NAME}\n") } @@ -224,7 +224,7 @@ internal abstract class DadbTest : BaseConcurrencyTest() { @Test fun installMultiple() { localEmulator { dadb -> - dadb.installMultiple(TestApk.SPLIT_FILES) + assertThat(dadb.installMultiple(TestApk.SPLIT_FILES)).isInstanceOf(InstallResult.Success::class.java) val response = dadb.shell("pm path ${TestApk.SPLIT_PACKAGE_NAME} | wc -l") assertShellResponse(response, 0, "${TestApk.SPLIT_FILES.size}\n") } @@ -233,12 +233,12 @@ internal abstract class DadbTest : BaseConcurrencyTest() { @Test internal fun install_grantPermissions() { localEmulator { dadb -> - dadb.install(TestApk.FILE) + assertThat(dadb.install(TestApk.FILE)).isInstanceOf(InstallResult.Success::class.java) var response = dadb.shell("dumpsys package ${TestApk.PACKAGE_NAME}") assertThat(response.exitCode).isEqualTo(0) assertThat(response.output).doesNotContain("android.permission.RECORD_AUDIO: granted=true") - dadb.install(TestApk.FILE, "-g") + assertThat(dadb.install(TestApk.FILE, "-g")).isInstanceOf(InstallResult.Success::class.java) response = dadb.shell("dumpsys package ${TestApk.PACKAGE_NAME}") assertThat(response.exitCode).isEqualTo(0) assertThat(response.output).contains("android.permission.RECORD_AUDIO: granted=true") @@ -248,8 +248,8 @@ internal abstract class DadbTest : BaseConcurrencyTest() { @Test fun uninstall() { localEmulator { dadb -> - dadb.install(TestApk.FILE) - dadb.uninstall(TestApk.PACKAGE_NAME) + assertThat(dadb.install(TestApk.FILE)).isInstanceOf(InstallResult.Success::class.java) + assertThat(dadb.uninstall(TestApk.PACKAGE_NAME)).isInstanceOf(UninstallResult.Success::class.java) val response = dadb.shell("pm list packages ${TestApk.PACKAGE_NAME}") assertShellResponse(response, 0, "") } @@ -291,8 +291,8 @@ internal abstract class DadbTest : BaseConcurrencyTest() { @Ignore @Test fun root() { - localEmulator(Dadb::unroot) - localEmulator(Dadb::root) + localEmulator { assertThat(it.unroot()).isInstanceOf(RootResult.Success::class.java) } + localEmulator { assertThat(it.root()).isInstanceOf(RootResult.Success::class.java) } localEmulator { dadb -> val response = dadb.shell("getprop service.adb.root") assertShellResponse(response, 0, "1\n") @@ -302,8 +302,8 @@ internal abstract class DadbTest : BaseConcurrencyTest() { @Ignore @Test fun unroot() { - localEmulator(Dadb::root) - localEmulator(Dadb::unroot) + localEmulator { assertThat(it.root()).isInstanceOf(RootResult.Success::class.java) } + localEmulator { assertThat(it.unroot()).isInstanceOf(RootResult.Success::class.java) } localEmulator { dadb -> val response = dadb.shell("getprop service.adb.root") assertShellResponse(response, 0, "0\n") From 2440e0ba6c51836670ef1ed3614346a6844a9cba Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 13:09:20 -0700 Subject: [PATCH 17/32] feat: root/unroot return RootResult; @Throws(AdbException) sweep --- dadb/src/main/kotlin/dadb/Dadb.kt | 35 +++++++++------------ dadb/src/test/kotlin/dadb/DadbResultTest.kt | 10 ++++++ 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/dadb/src/main/kotlin/dadb/Dadb.kt b/dadb/src/main/kotlin/dadb/Dadb.kt index 298554c..c607ea6 100644 --- a/dadb/src/main/kotlin/dadb/Dadb.kt +++ b/dadb/src/main/kotlin/dadb/Dadb.kt @@ -25,19 +25,19 @@ import okio.* interface Dadb : AutoCloseable { - @Throws(IOException::class) + @Throws(AdbException::class) fun open(destination: String): AdbStream fun supportsFeature(feature: String): Boolean - @Throws(IOException::class) + @Throws(AdbException::class) fun shell(command: String): AdbShellResponse { openShell(command).use { stream -> return stream.readAll() } } - @Throws(IOException::class) + @Throws(AdbException::class) fun openShell(command: String = ""): AdbShellStream { val stream = open("shell,v2,raw:$command") return AdbShellStream(stream) @@ -200,36 +200,31 @@ interface Dadb : AutoCloseable { } } - @Throws(IOException::class) + @Throws(AdbException::class) fun execCmd(vararg command: String): AdbStream { if (!supportsFeature("cmd")) throw UnsupportedOperationException("cmd is not supported on this version of Android") val destination = (listOf("exec:cmd") + command).joinToString(" ") return open(destination) } - @Throws(IOException::class) + @Throws(AdbException::class) fun abbExec(vararg command: String): AdbStream { if (!supportsFeature("abb_exec")) throw UnsupportedOperationException("abb_exec is not supported on this version of Android") val destination = "abb_exec:${command.joinToString("\u0000")}" return open(destination) } - @Throws(IOException::class) - fun root() { - val response = restartAdb(this, "root:") - if (!response.startsWith("restarting") && !response.contains("already")) { - throw IOException("Failed to restart adb as root: $response") - } - waitRootOrClose(this, root = true) - } + @Throws(AdbException::class) + fun root(): RootResult = restartAdbd("root:", root = true) { it.startsWith("restarting") || it.contains("already") } - @Throws(IOException::class) - fun unroot() { - val response = restartAdb(this, "unroot:") - if (!response.startsWith("restarting") && !response.contains("not running as root")) { - throw IOException("Failed to restart adb as root: $response") - } - waitRootOrClose(this, root = false) + @Throws(AdbException::class) + fun unroot(): RootResult = restartAdbd("unroot:", root = false) { it.startsWith("restarting") || it.contains("not running as root") } + + private fun restartAdbd(service: String, root: Boolean, isSuccess: (String) -> Boolean): RootResult { + val response = restartAdb(this, service) + if (!isSuccess(response)) return RootResult.Failure(response) + waitRootOrClose(this, root) + return RootResult.Success } @Throws(InterruptedException::class) diff --git a/dadb/src/test/kotlin/dadb/DadbResultTest.kt b/dadb/src/test/kotlin/dadb/DadbResultTest.kt index e7ffb62..de71a85 100644 --- a/dadb/src/test/kotlin/dadb/DadbResultTest.kt +++ b/dadb/src/test/kotlin/dadb/DadbResultTest.kt @@ -83,4 +83,14 @@ internal class DadbResultTest { val result = dadb.install(Buffer().also { it.writeUtf8("apk") }, 3L) assertThat(result).isInstanceOf(InstallResult.Success::class.java) } + + @Test + fun rootFailureReturnsRootFailure() { + val dadb = FakeDadb { + FakeAdbStream(Buffer().also { it.writeUtf8("adbd cannot run as root in production builds\n") }) + } + val result = dadb.root() + assertThat(result).isInstanceOf(RootResult.Failure::class.java) + assertThat((result as RootResult.Failure).reason).contains("production builds") + } } From 45d48833786c4a147b6036b2cd3b0c017ea4df7c Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 13:14:36 -0700 Subject: [PATCH 18/32] docs: document the adbd-aware error model --- CHANGELOG.md | 7 +++++++ README.md | 16 +++++++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f1868e..823f9da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +### 2.0.0 + +* **Breaking:** redesigned error model. Transport failures now throw a typed `AdbException` hierarchy (`AdbConnectException`, `AdbAuthException`, `AdbStreamOpenException`, `AdbConnectionClosedException`, `AdbProtocolException`), all extending `IOException`. +* **Breaking:** operation outcomes are now returned instead of thrown: `install`/`installMultiple` return `InstallResult`, `uninstall` returns `UninstallResult`, `push`/`pull` return `SyncResult`, `root`/`unroot` return `RootResult`. +* **Breaking:** a connection dropped mid-stream now throws `AdbConnectionClosedException` instead of presenting as a clean end-of-stream. +* `pmInstall` (the non-`cmd` install path) now checks the `pm install` result instead of ignoring it. + ### 1.2.10 * Creating configuration for enabling keep alive on dadb socket diff --git a/README.md b/README.md index 7e77433..6657bd9 100644 --- a/README.md +++ b/README.md @@ -20,12 +20,26 @@ Connect to `emulator-5554` and install `apkFile`: ```kotlin Dadb.create("localhost", 5555).use { dadb -> - dadb.install(apkFile) + when (val result = dadb.install(apkFile)) { + is InstallResult.Success -> println("installed") + is InstallResult.Failure -> println("install failed: ${result.reason}") + } } ``` *Note: Connect to the odd adb daemon port (5555), not the even emulator console port (5554)* +### Error handling + +dadb distinguishes *transport* failures from *operation* failures: + +- **Transport failures throw** an `AdbException` (an `IOException`): `AdbConnectException`, + `AdbAuthException`, `AdbStreamOpenException`, `AdbConnectionClosedException`, `AdbProtocolException`. + Catch `AdbException` to decide whether to reconnect. +- **Operation outcomes are returned**, never thrown: `install`/`installMultiple` → `InstallResult`, + `uninstall` → `UninstallResult`, `push`/`pull` → `SyncResult`, `root`/`unroot` → `RootResult`. + A non-zero `shell` exit code stays a value on `AdbShellResponse`. + ### Discover a Device The following discovers and returns a connected device or emulator. If there are multiple it returns the first one found. From 8fe890fd6c81a4f1373058843bcbb1b77319b403 Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 13:18:25 -0700 Subject: [PATCH 19/32] test: cover AdbProtocolException on sync packet desync --- dadb/src/test/kotlin/dadb/DadbResultTest.kt | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/dadb/src/test/kotlin/dadb/DadbResultTest.kt b/dadb/src/test/kotlin/dadb/DadbResultTest.kt index de71a85..31e7b8c 100644 --- a/dadb/src/test/kotlin/dadb/DadbResultTest.kt +++ b/dadb/src/test/kotlin/dadb/DadbResultTest.kt @@ -32,6 +32,14 @@ internal class DadbResultTest { assertThat(e.reason).isEqualTo("No such file or directory") } + @Test + fun syncRecvUnexpectedPacketThrowsProtocolException() { + // adbd sends a packet id that is neither DATA, DONE, nor FAIL — a protocol desync. + val bogus = Buffer().also { it.writeUtf8("OKAY"); it.writeIntLe(0) } + val sync = AdbSyncStream(FakeAdbStream(bogus)) + assertFailsWith { sync.recv(Buffer(), "/somewhere") } + } + @Test fun pullMissingFileReturnsSyncFailure() { val dadb = FakeDadb { FakeAdbStream(syncFailBuffer("No such file or directory")) } From 18f144b73c1625ffe0396677e481016fb824cc9f Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 13:43:25 -0700 Subject: [PATCH 20/32] fix: wrap raw socket faults in open() as AdbConnectionClosedException MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit open() only translated AdbStreamClosed (A_OPEN→A_CLSE). A raw socket fault (EOF/RST) during the OPEN read fell through to the Throwable catch and leaked a bare IOException, violating the "no bare IOException leaks" contract and the @Throws(AdbException::class) signal on the public open(). Map a raw IOException here to AdbConnectionClosedException (per design spec §4), pass already-typed AdbExceptions through without re-wrapping, and tighten the internal annotation to @Throws(AdbException::class). Co-Authored-By: Claude Opus 4.8 (1M context) --- dadb/src/main/kotlin/dadb/AdbConnection.kt | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/dadb/src/main/kotlin/dadb/AdbConnection.kt b/dadb/src/main/kotlin/dadb/AdbConnection.kt index 0d6d978..523b140 100644 --- a/dadb/src/main/kotlin/dadb/AdbConnection.kt +++ b/dadb/src/main/kotlin/dadb/AdbConnection.kt @@ -39,7 +39,7 @@ internal class AdbConnection internal constructor( private val random = Random() private val messageQueue = AdbMessageQueue(adbReader) - @Throws(IOException::class) + @Throws(AdbException::class) fun open(destination: String): AdbStream { val localId = newId() messageQueue.startListening(localId) @@ -52,6 +52,14 @@ internal class AdbConnection internal constructor( // adbd answered A_OPEN with A_CLSE: it refused this service. Connection is still alive. messageQueue.stopListening(localId) throw AdbStreamOpenException(destination, "adbd refused to open stream: $destination", e) + } catch (e: AdbException) { + // Already a typed transport fault — don't re-wrap. + messageQueue.stopListening(localId) + throw e + } catch (e: IOException) { + // Raw socket fault (EOF/RST) while opening: the connection died mid-handshake. + messageQueue.stopListening(localId) + throw AdbConnectionClosedException("Connection lost while opening stream: $destination", e) } catch (e: Throwable) { messageQueue.stopListening(localId) throw e From d4f16a2cd5bd022f25864612c9960a5329c99002 Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 13:49:08 -0700 Subject: [PATCH 21/32] test: move AdbStreamOpenException test to the direct-adbd impl only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit open_invalidService_throwsStreamOpenException was in the abstract DadbTest, so it also ran under AdbServerTest (the adb-server-backed impl), where a refused service still surfaces a generic IOException — that path is concern #2, out of scope. Moved the test to DadbTestImpl (direct connection), where the AdbStreamOpenException mapping applies. Full suite green on an emulator. Co-Authored-By: Claude Opus 4.8 (1M context) --- dadb/src/test/kotlin/dadb/DadbTest.kt | 10 ---------- dadb/src/test/kotlin/dadb/DadbTestImpl.kt | 13 +++++++++++++ 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/dadb/src/test/kotlin/dadb/DadbTest.kt b/dadb/src/test/kotlin/dadb/DadbTest.kt index 4963c66..f3ccab1 100644 --- a/dadb/src/test/kotlin/dadb/DadbTest.kt +++ b/dadb/src/test/kotlin/dadb/DadbTest.kt @@ -36,7 +36,6 @@ import kotlin.test.AfterTest import kotlin.test.BeforeTest import kotlin.test.Ignore import kotlin.test.Test -import kotlin.test.assertFailsWith internal abstract class DadbTest : BaseConcurrencyTest() { @@ -320,15 +319,6 @@ internal abstract class DadbTest : BaseConcurrencyTest() { } } - @Test - fun open_invalidService_throwsStreamOpenException() { - localEmulator { dadb -> - assertFailsWith { - dadb.open("definitely-not-a-real-service:") - } - } - } - protected abstract fun localEmulator(body: (dadb: Dadb) -> Unit) private fun readSocketAsync(host: String, port: Int): Future { diff --git a/dadb/src/test/kotlin/dadb/DadbTestImpl.kt b/dadb/src/test/kotlin/dadb/DadbTestImpl.kt index fa858a5..39406b0 100644 --- a/dadb/src/test/kotlin/dadb/DadbTestImpl.kt +++ b/dadb/src/test/kotlin/dadb/DadbTestImpl.kt @@ -3,6 +3,7 @@ package dadb import org.junit.jupiter.api.Test import java.net.Socket import kotlin.test.assertFails +import kotlin.test.assertFailsWith import kotlin.test.assertNotNull internal class DadbTestImpl : DadbTest() { @@ -60,4 +61,16 @@ internal class DadbTestImpl : DadbTest() { } } + // AdbStreamOpenException is a direct-adbd concept; it lives here (direct connection) rather than + // in the shared DadbTest, because the adb-server path (AdbServerTest) still surfaces a generic + // IOException for a refused service (that path is out of scope for the error-model redesign). + @Test + fun open_invalidService_throwsStreamOpenException() { + localEmulator { dadb -> + assertFailsWith { + dadb.open("definitely-not-a-real-service:") + } + } + } + } From 914fa500e80ea1fd28ace4cdaa9d3070459590e5 Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 14:09:40 -0700 Subject: [PATCH 22/32] feat: add opt-in onSuccess/onFailure/orThrow helpers to result types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reduce call-site verbosity for callers who don't want an exhaustive `when`. Mirrors Kotlin stdlib Result's onSuccess/onFailure/getOrThrow. orThrow throws AdbOperationFailedException, deliberately NOT an AdbException: surfacing an operation outcome as an exception is a caller choice, and a transport-level `catch (AdbException)` (reconnect-on-death) must not swallow it — the two axes stay distinct even when opting in. Helpers are per-type because the Failure payloads differ (uninstall carries an exitCode; orThrow propagates it). Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 1 + .../dadb/AdbOperationFailedException.kt | 36 +++++++++ dadb/src/main/kotlin/dadb/InstallResult.kt | 17 ++++ dadb/src/main/kotlin/dadb/RootResult.kt | 17 ++++ dadb/src/main/kotlin/dadb/SyncResult.kt | 17 ++++ dadb/src/main/kotlin/dadb/UninstallResult.kt | 18 +++++ .../src/test/kotlin/dadb/ResultHelpersTest.kt | 78 +++++++++++++++++++ 7 files changed, 184 insertions(+) create mode 100644 dadb/src/main/kotlin/dadb/AdbOperationFailedException.kt create mode 100644 dadb/src/test/kotlin/dadb/ResultHelpersTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index 823f9da..1824308 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * **Breaking:** operation outcomes are now returned instead of thrown: `install`/`installMultiple` return `InstallResult`, `uninstall` returns `UninstallResult`, `push`/`pull` return `SyncResult`, `root`/`unroot` return `RootResult`. * **Breaking:** a connection dropped mid-stream now throws `AdbConnectionClosedException` instead of presenting as a clean end-of-stream. * `pmInstall` (the non-`cmd` install path) now checks the `pm install` result instead of ignoring it. +* Added opt-in result helpers — `onSuccess`/`onFailure`/`orThrow` on each `…Result` type. `orThrow` raises `AdbOperationFailedException`, which is **not** an `AdbException`, so a transport-level `catch (AdbException)` won't catch an opted-in operation-failure throw. ### 1.2.10 diff --git a/dadb/src/main/kotlin/dadb/AdbOperationFailedException.kt b/dadb/src/main/kotlin/dadb/AdbOperationFailedException.kt new file mode 100644 index 0000000..6bc71f1 --- /dev/null +++ b/dadb/src/main/kotlin/dadb/AdbOperationFailedException.kt @@ -0,0 +1,36 @@ +/* + * Copyright (c) 2021 mobile.dev inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package dadb + +/** + * Thrown by the opt-in `orThrow()` helpers when an operation returned a `Failure`. This is an + * *operation outcome* surfaced as an exception by caller choice — the transport is healthy. + * + * It is deliberately **not** an [AdbException]: a `catch (AdbException)` that exists to reconnect on + * transport death will not catch this, so the two axes (transport died vs. operation rejected) stay + * distinct even for callers who opt into the throwing convenience. Catch it explicitly when you use + * `orThrow()`. + * + * [exitCode] is populated only for operations whose failure carries one (e.g. `uninstall`). + */ +class AdbOperationFailedException( + val reason: String, + val exitCode: Int? = null, +) : RuntimeException( + if (exitCode != null) "Operation failed (exit $exitCode): $reason" else "Operation failed: $reason" +) diff --git a/dadb/src/main/kotlin/dadb/InstallResult.kt b/dadb/src/main/kotlin/dadb/InstallResult.kt index c09c9ae..9078000 100644 --- a/dadb/src/main/kotlin/dadb/InstallResult.kt +++ b/dadb/src/main/kotlin/dadb/InstallResult.kt @@ -26,3 +26,20 @@ sealed interface InstallResult { * not parsed into an enum. */ data class Failure(val reason: String) : InstallResult } + +/** Run [block] if this is a [InstallResult.Success]; returns the receiver for chaining. */ +inline fun InstallResult.onSuccess(block: () -> Unit): InstallResult { + if (this is InstallResult.Success) block() + return this +} + +/** Run [block] if this is a [InstallResult.Failure]; returns the receiver for chaining. */ +inline fun InstallResult.onFailure(block: (InstallResult.Failure) -> Unit): InstallResult { + if (this is InstallResult.Failure) block(this) + return this +} + +/** Fail-fast: throw [AdbOperationFailedException] if this is a [InstallResult.Failure]. */ +fun InstallResult.orThrow() { + if (this is InstallResult.Failure) throw AdbOperationFailedException(reason) +} diff --git a/dadb/src/main/kotlin/dadb/RootResult.kt b/dadb/src/main/kotlin/dadb/RootResult.kt index 54edc20..33a54dd 100644 --- a/dadb/src/main/kotlin/dadb/RootResult.kt +++ b/dadb/src/main/kotlin/dadb/RootResult.kt @@ -22,3 +22,20 @@ sealed interface RootResult { object Success : RootResult data class Failure(val reason: String) : RootResult } + +/** Run [block] if this is a [RootResult.Success]; returns the receiver for chaining. */ +inline fun RootResult.onSuccess(block: () -> Unit): RootResult { + if (this is RootResult.Success) block() + return this +} + +/** Run [block] if this is a [RootResult.Failure]; returns the receiver for chaining. */ +inline fun RootResult.onFailure(block: (RootResult.Failure) -> Unit): RootResult { + if (this is RootResult.Failure) block(this) + return this +} + +/** Fail-fast: throw [AdbOperationFailedException] if this is a [RootResult.Failure]. */ +fun RootResult.orThrow() { + if (this is RootResult.Failure) throw AdbOperationFailedException(reason) +} diff --git a/dadb/src/main/kotlin/dadb/SyncResult.kt b/dadb/src/main/kotlin/dadb/SyncResult.kt index 391237f..d311cce 100644 --- a/dadb/src/main/kotlin/dadb/SyncResult.kt +++ b/dadb/src/main/kotlin/dadb/SyncResult.kt @@ -22,3 +22,20 @@ sealed interface SyncResult { object Success : SyncResult data class Failure(val reason: String) : SyncResult } + +/** Run [block] if this is a [SyncResult.Success]; returns the receiver for chaining. */ +inline fun SyncResult.onSuccess(block: () -> Unit): SyncResult { + if (this is SyncResult.Success) block() + return this +} + +/** Run [block] if this is a [SyncResult.Failure]; returns the receiver for chaining. */ +inline fun SyncResult.onFailure(block: (SyncResult.Failure) -> Unit): SyncResult { + if (this is SyncResult.Failure) block(this) + return this +} + +/** Fail-fast: throw [AdbOperationFailedException] if this is a [SyncResult.Failure]. */ +fun SyncResult.orThrow() { + if (this is SyncResult.Failure) throw AdbOperationFailedException(reason) +} diff --git a/dadb/src/main/kotlin/dadb/UninstallResult.kt b/dadb/src/main/kotlin/dadb/UninstallResult.kt index d16155f..adb9c42 100644 --- a/dadb/src/main/kotlin/dadb/UninstallResult.kt +++ b/dadb/src/main/kotlin/dadb/UninstallResult.kt @@ -23,3 +23,21 @@ sealed interface UninstallResult { object Success : UninstallResult data class Failure(val reason: String, val exitCode: Int) : UninstallResult } + +/** Run [block] if this is a [UninstallResult.Success]; returns the receiver for chaining. */ +inline fun UninstallResult.onSuccess(block: () -> Unit): UninstallResult { + if (this is UninstallResult.Success) block() + return this +} + +/** Run [block] if this is a [UninstallResult.Failure]; returns the receiver for chaining. */ +inline fun UninstallResult.onFailure(block: (UninstallResult.Failure) -> Unit): UninstallResult { + if (this is UninstallResult.Failure) block(this) + return this +} + +/** Fail-fast: throw [AdbOperationFailedException] (carrying the process [exitCode]) if this is a + * [UninstallResult.Failure]. */ +fun UninstallResult.orThrow() { + if (this is UninstallResult.Failure) throw AdbOperationFailedException(reason, exitCode) +} diff --git a/dadb/src/test/kotlin/dadb/ResultHelpersTest.kt b/dadb/src/test/kotlin/dadb/ResultHelpersTest.kt new file mode 100644 index 0000000..99ce5dc --- /dev/null +++ b/dadb/src/test/kotlin/dadb/ResultHelpersTest.kt @@ -0,0 +1,78 @@ +/* + * Copyright (c) 2021 mobile.dev inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package dadb + +import com.google.common.truth.Truth.assertThat +import kotlin.test.Test +import kotlin.test.assertFailsWith + +internal class ResultHelpersTest { + + @Test + fun onSuccessRunsOnlyForSuccess() { + var ran = false + (InstallResult.Success as InstallResult).onSuccess { ran = true } + assertThat(ran).isTrue() + + ran = false + (InstallResult.Failure("nope") as InstallResult).onSuccess { ran = true } + assertThat(ran).isFalse() + } + + @Test + fun onFailureReceivesTheFailurePayload() { + var reason: String? = null + (SyncResult.Failure("No such file or directory") as SyncResult).onFailure { reason = it.reason } + assertThat(reason).isEqualTo("No such file or directory") + + reason = "untouched" + (SyncResult.Success as SyncResult).onFailure { reason = it.reason } + assertThat(reason).isEqualTo("untouched") + } + + @Test + fun helpersReturnReceiverForChaining() { + var sawFailure = false + val result: InstallResult = InstallResult.Failure("rejected") + result.onSuccess { error("should not run") }.onFailure { sawFailure = true } + assertThat(sawFailure).isTrue() + } + + @Test + fun orThrowIsNoOpOnSuccess() { + (RootResult.Success as RootResult).orThrow() + } + + @Test + fun orThrowRaisesNonAdbExceptionOnFailure() { + val e = assertFailsWith { + (InstallResult.Failure("Failure [INSTALL_FAILED_INVALID_APK]") as InstallResult).orThrow() + } + assertThat(e.reason).contains("INSTALL_FAILED") + // The whole point: an opted-in operation throw is NOT a transport fault. + assertThat(e).isNotInstanceOf(AdbException::class.java) + } + + @Test + fun uninstallOrThrowCarriesExitCode() { + val e = assertFailsWith { + (UninstallResult.Failure(reason = "DELETE_FAILED_INTERNAL_ERROR", exitCode = 1) as UninstallResult).orThrow() + } + assertThat(e.exitCode).isEqualTo(1) + } +} From 68efdadd79985d477a439daa774cbcf1142dc249 Mon Sep 17 00:00:00 2001 From: Stevie Clifton Date: Fri, 5 Jun 2026 14:26:28 -0700 Subject: [PATCH 23/32] chore: remove internal superpowers design docs from the branch Co-Authored-By: Claude Opus 4.8 (1M context) --- .../plans/2026-06-05-dadb-error-model.md | 1239 ----------------- .../2026-06-05-dadb-error-model-design.md | 232 --- 2 files changed, 1471 deletions(-) delete mode 100644 docs/superpowers/plans/2026-06-05-dadb-error-model.md delete mode 100644 docs/superpowers/specs/2026-06-05-dadb-error-model-design.md diff --git a/docs/superpowers/plans/2026-06-05-dadb-error-model.md b/docs/superpowers/plans/2026-06-05-dadb-error-model.md deleted file mode 100644 index eacb7d4..0000000 --- a/docs/superpowers/plans/2026-06-05-dadb-error-model.md +++ /dev/null @@ -1,1239 +0,0 @@ -# dadb Error Model Redesign — Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Replace dadb's undifferentiated `IOException` error model with an adbd-aware contract — typed transport exceptions (`AdbException` hierarchy) for connection failures, and operation-specific result types (`InstallResult`/`UninstallResult`/`SyncResult`/`RootResult`) for in-band outcomes. - -**Architecture:** A sealed `AdbException : IOException` hierarchy carries every transport/auth/protocol failure (one type per documented adbd `protocol.txt` failure point). Expected operation outcomes become return values; the named high-level `Dadb` methods return per-operation sealed result types. Internal stream codecs throw a private `AdbSyncFailException` for sync `FAIL`, which the high-level methods catch and fold into a result. See the design spec: `docs/superpowers/specs/2026-06-05-dadb-error-model-design.md`. - -**Tech Stack:** Kotlin/JVM, okio 2.10.0, JUnit Platform (`kotlin.test`), Google Truth. Unit tests drive the wire protocol with in-memory okio `Buffer`s (the existing `AdbStreamTest` pattern); a `FakeDadb`/`FakeAdbStream` fixture exercises the high-level `Dadb` default methods without an emulator. - -**Scope:** Direct-to-adbd error handling only. Out of scope (do NOT touch): the `dadb.adbserver.*` host-server path, `discover`/`list` restructuring, the swapped-timeout bug at `Dadb.kt:271`, and the concurrency/resource fixes noted in review. - -**Conventions for every task:** new files get the standard Apache-2.0 license header (copy it verbatim from any existing file in `dadb/src/main/kotlin/dadb/`). Run the build from the repo root with `./gradlew :dadb:test`. Compile-only checks use `./gradlew :dadb:compileKotlin :dadb:compileTestKotlin`. - ---- - -## File Structure - -**New main files:** -- `dadb/src/main/kotlin/dadb/AdbException.kt` — sealed `AdbException` + the five transport subtypes. -- `dadb/src/main/kotlin/dadb/InstallResult.kt`, `UninstallResult.kt`, `SyncResult.kt`, `RootResult.kt` — operation result types. - -**New test files:** -- `dadb/src/test/kotlin/dadb/FakeDadb.kt` — `FakeAdbStream`, `FakeDadb`, and the `shellV2Buffer`/`syncFailBuffer` byte-builders. -- `dadb/src/test/kotlin/dadb/AdbExceptionTest.kt`, `ResultTypesTest.kt`, `AdbConnectionHandshakeTest.kt`, `DadbResultTest.kt` — unit tests. - -**Modified main files:** -- `dadb/src/main/kotlin/dadb/AdbStream.kt` — `nextMessage` distinguishes clean close from socket fault. -- `dadb/src/main/kotlin/dadb/AdbConnection.kt` — handshake + `open` map failures to typed exceptions; one `private`→`internal` test seam. -- `dadb/src/main/kotlin/dadb/AdbSync.kt` — internal `AdbSyncFailException`; `send`/`recv` throw it on `FAIL`. -- `dadb/src/main/kotlin/dadb/Dadb.kt` — high-level methods return result types; `@Throws(AdbException::class)`. - -**Modified test files:** -- `dadb/src/test/kotlin/dadb/AdbStreamTest.kt` — add the `nextMessage` behavior tests. -- `dadb/src/test/kotlin/dadb/DadbTest.kt` — update emulator tests to the new return types; add `AdbStreamOpenException` test. - ---- - -## Task 1: `AdbException` hierarchy - -**Files:** -- Create: `dadb/src/main/kotlin/dadb/AdbException.kt` -- Test: `dadb/src/test/kotlin/dadb/AdbExceptionTest.kt` - -- [ ] **Step 1: Write the failing test** - -Create `dadb/src/test/kotlin/dadb/AdbExceptionTest.kt`: - -```kotlin -package dadb - -import com.google.common.truth.Truth.assertThat -import java.io.IOException -import kotlin.test.Test - -internal class AdbExceptionTest { - - @Test - fun allTypesAreIOExceptions() { - val exceptions: List = listOf( - AdbConnectException("x"), - AdbAuthException("x"), - AdbStreamOpenException("shell:", "x"), - AdbConnectionClosedException("x"), - AdbProtocolException("x"), - ) - exceptions.forEach { assertThat(it).isInstanceOf(IOException::class.java) } - } - - @Test - fun streamOpenExceptionCarriesDestination() { - val e = AdbStreamOpenException("exec:cmd package install", "refused") - assertThat(e.destination).isEqualTo("exec:cmd package install") - } - - @Test - fun preservesCause() { - val cause = IOException("socket reset") - assertThat(AdbConnectException("x", cause).cause).isSameInstanceAs(cause) - } -} -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `./gradlew :dadb:compileTestKotlin` -Expected: FAIL — unresolved references `AdbConnectException`, `AdbException`, etc. - -- [ ] **Step 3: Write the implementation** - -Create `dadb/src/main/kotlin/dadb/AdbException.kt` (prefix with the Apache-2.0 license header copied from an existing file): - -```kotlin -package dadb - -import java.io.IOException - -/** - * Root of all dadb transport/connection failures. Extends [IOException] because these are genuine, - * recoverable socket I/O faults, consistent with the okio stream surface dadb exposes. Every - * transport fault thrown from a public method is one of these subtypes — no bare IOException leaks. - */ -sealed class AdbException(message: String, cause: Throwable? = null) : IOException(message, cause) - -/** Could not establish the connection: TCP connect/timeout, or the A_CNXN handshake failed. - * Nothing ran — safe to retry by reconnecting. */ -class AdbConnectException(message: String, cause: Throwable? = null) : AdbException(message, cause) - -/** A_AUTH was rejected (no key, or device left UNAUTHORIZED). Reconnecting with the same key will - * not help — fix or accept the key. */ -class AdbAuthException(message: String, cause: Throwable? = null) : AdbException(message, cause) - -/** adbd refused to OPEN the stream (A_OPEN answered with A_CLSE): unknown/forbidden service string, - * or device offline. The underlying connection is still usable. */ -class AdbStreamOpenException( - val destination: String, - message: String, - cause: Throwable? = null -) : AdbException(message, cause) - -/** An established connection/stream died unexpectedly (socket EOF/RST mid-operation). Reconnect — - * but the operation MAY have partially executed; re-check state before retrying non-idempotent work. */ -class AdbConnectionClosedException(message: String, cause: Throwable? = null) : AdbException(message, cause) - -/** The peer sent a malformed or unexpected apacket (desync). No reliable way to re-sync — the - * connection must be closed and re-established. */ -class AdbProtocolException(message: String, cause: Throwable? = null) : AdbException(message, cause) -``` - -- [ ] **Step 4: Run test to verify it passes** - -Run: `./gradlew :dadb:test --tests dadb.AdbExceptionTest` -Expected: PASS (3 tests) - -- [ ] **Step 5: Commit** - -```bash -git add dadb/src/main/kotlin/dadb/AdbException.kt dadb/src/test/kotlin/dadb/AdbExceptionTest.kt -git commit -m "feat: add AdbException transport exception hierarchy" -``` - ---- - -## Task 2: Operation result types - -**Files:** -- Create: `dadb/src/main/kotlin/dadb/InstallResult.kt`, `UninstallResult.kt`, `SyncResult.kt`, `RootResult.kt` -- Test: `dadb/src/test/kotlin/dadb/ResultTypesTest.kt` - -- [ ] **Step 1: Write the failing test** - -Create `dadb/src/test/kotlin/dadb/ResultTypesTest.kt`: - -```kotlin -package dadb - -import com.google.common.truth.Truth.assertThat -import kotlin.test.Test - -internal class ResultTypesTest { - - @Test - fun installResultVariants() { - val ok: InstallResult = InstallResult.Success - val fail: InstallResult = InstallResult.Failure("Failure [INSTALL_FAILED_INVALID_APK]") - assertThat(ok).isInstanceOf(InstallResult.Success::class.java) - assertThat((fail as InstallResult.Failure).reason).contains("INSTALL_FAILED") - } - - @Test - fun uninstallFailureCarriesExitCodeAndReason() { - val fail = UninstallResult.Failure(reason = "Failure [DELETE_FAILED_INTERNAL_ERROR]", exitCode = 1) - assertThat(fail.exitCode).isEqualTo(1) - assertThat(fail.reason).contains("DELETE_FAILED") - } - - @Test - fun syncAndRootVariants() { - assertThat(SyncResult.Failure("No such file or directory").reason).contains("No such file") - assertThat(RootResult.Failure("adbd cannot run as root in production builds").reason) - .contains("production builds") - val syncOk: SyncResult = SyncResult.Success - val rootOk: RootResult = RootResult.Success - assertThat(syncOk).isInstanceOf(SyncResult.Success::class.java) - assertThat(rootOk).isInstanceOf(RootResult.Success::class.java) - } -} -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `./gradlew :dadb:compileTestKotlin` -Expected: FAIL — unresolved `InstallResult`, `UninstallResult`, `SyncResult`, `RootResult`. - -- [ ] **Step 3: Write the implementations** - -Create `dadb/src/main/kotlin/dadb/InstallResult.kt` (with license header): - -```kotlin -package dadb - -/** Outcome of [Dadb.install] / [Dadb.installMultiple]. */ -sealed interface InstallResult { - object Success : InstallResult - /** Raw pm/`cmd package` verdict, surfaced opaquely — exactly as the canonical adb client does - * (client/adb_install.cpp checks `strncmp("Success", buf, 7)` and prints the rest verbatim). - * `INSTALL_FAILED_*` codes are a frameworks/base concept, not the adbd contract, so they are - * not parsed into an enum. */ - data class Failure(val reason: String) : InstallResult -} -``` - -Create `dadb/src/main/kotlin/dadb/UninstallResult.kt` (with license header): - -```kotlin -package dadb - -/** Outcome of [Dadb.uninstall]. `uninstall` is `cmd package uninstall` over the shell service, so - * [Failure] preserves adbd's actual process exit code and combined output — the wrapper hides nothing. */ -sealed interface UninstallResult { - object Success : UninstallResult - data class Failure(val reason: String, val exitCode: Int) : UninstallResult -} -``` - -Create `dadb/src/main/kotlin/dadb/SyncResult.kt` (with license header): - -```kotlin -package dadb - -/** Outcome of [Dadb.push] / [Dadb.pull]. [Failure.reason] is the adbd sync FAIL message. */ -sealed interface SyncResult { - object Success : SyncResult - data class Failure(val reason: String) : SyncResult -} -``` - -Create `dadb/src/main/kotlin/dadb/RootResult.kt` (with license header): - -```kotlin -package dadb - -/** Outcome of [Dadb.root] / [Dadb.unroot]. [Failure.reason] is the adbd root:/unroot: response line. */ -sealed interface RootResult { - object Success : RootResult - data class Failure(val reason: String) : RootResult -} -``` - -- [ ] **Step 4: Run test to verify it passes** - -Run: `./gradlew :dadb:test --tests dadb.ResultTypesTest` -Expected: PASS (3 tests) - -- [ ] **Step 5: Commit** - -```bash -git add dadb/src/main/kotlin/dadb/InstallResult.kt dadb/src/main/kotlin/dadb/UninstallResult.kt dadb/src/main/kotlin/dadb/SyncResult.kt dadb/src/main/kotlin/dadb/RootResult.kt dadb/src/test/kotlin/dadb/ResultTypesTest.kt -git commit -m "feat: add operation result types" -``` - ---- - -## Task 3: Test fixtures (`FakeDadb`, byte-builders) - -This is test scaffolding (no red/green of its own); later tasks depend on it. It must compile against the `Dadb` interface and existing wire constants. - -**Files:** -- Create: `dadb/src/test/kotlin/dadb/FakeDadb.kt` - -- [ ] **Step 1: Write the fixture** - -Create `dadb/src/test/kotlin/dadb/FakeDadb.kt`: - -```kotlin -package dadb - -import okio.Buffer -import java.nio.charset.StandardCharsets - -/** An [AdbStream] backed by in-memory buffers. [source] is pre-loaded with device bytes; - * [sink] captures whatever the code under test writes. */ -internal class FakeAdbStream( - override val source: Buffer, - override val sink: Buffer = Buffer() -) : AdbStream { - override fun close() {} -} - -/** A [Dadb] whose `open(destination)` returns a caller-supplied [AdbStream]. Records every - * destination opened, in order, in [opened]. */ -internal class FakeDadb( - private val features: Set = setOf("shell_v2", "cmd"), - private val streamFor: (destination: String) -> AdbStream -) : Dadb { - val opened = mutableListOf() - - override fun open(destination: String): AdbStream { - opened += destination - return streamFor(destination) - } - - override fun supportsFeature(feature: String) = feature in features - - override fun close() {} -} - -/** Builds a shell,v2 byte stream: optional stdout/stderr packets followed by an exit packet. */ -internal fun shellV2Buffer(stdout: String = "", stderr: String = "", exitCode: Int): Buffer { - val buffer = Buffer() - if (stdout.isNotEmpty()) { - val bytes = stdout.toByteArray() - buffer.writeByte(ID_STDOUT); buffer.writeIntLe(bytes.size); buffer.write(bytes) - } - if (stderr.isNotEmpty()) { - val bytes = stderr.toByteArray() - buffer.writeByte(ID_STDERR); buffer.writeIntLe(bytes.size); buffer.write(bytes) - } - buffer.writeByte(ID_EXIT); buffer.writeIntLe(1); buffer.writeByte(exitCode) - return buffer -} - -/** Builds a single sync `FAIL` packet (4-char id + LE length + message). */ -internal fun syncFailBuffer(message: String): Buffer { - val buffer = Buffer() - buffer.writeString("FAIL", StandardCharsets.UTF_8) - buffer.writeIntLe(message.toByteArray().size) - buffer.writeString(message, StandardCharsets.UTF_8) - return buffer -} - -/** Builds a single sync `OKAY` packet (success terminator for SEND). */ -internal fun syncOkayBuffer(): Buffer { - val buffer = Buffer() - buffer.writeString("OKAY", StandardCharsets.UTF_8) - buffer.writeIntLe(0) - return buffer -} -``` - -- [ ] **Step 2: Verify it compiles** - -Run: `./gradlew :dadb:compileTestKotlin` -Expected: PASS (compiles; `FakeAdbStream` satisfies `AdbStream`, constants `ID_STDOUT`/`ID_STDERR`/`ID_EXIT` resolve from `AdbShell.kt`). - -- [ ] **Step 3: Commit** - -```bash -git add dadb/src/test/kotlin/dadb/FakeDadb.kt -git commit -m "test: add FakeDadb fixture and wire byte-builders" -``` - ---- - -## Task 4: `AdbStream.nextMessage` — clean close vs socket fault - -The core behavioral fix: a peer `A_CLSE` is a legitimate EOF; a real socket fault must surface as `AdbConnectionClosedException` instead of masquerading as a clean end-of-stream. - -**Files:** -- Modify: `dadb/src/main/kotlin/dadb/AdbStream.kt:117-124` -- Test: `dadb/src/test/kotlin/dadb/AdbStreamTest.kt` (add tests) - -- [ ] **Step 1: Write the failing tests** - -Append these two methods inside the `AdbStreamTest` class in `dadb/src/test/kotlin/dadb/AdbStreamTest.kt` (and add the imports `import kotlin.test.assertFailsWith` and `import java.io.IOException` at the top): - -```kotlin - @Test - fun peerCloseIsCleanEof() { - // Reader yields a CLSE for localId=1 and nothing else. - val buffer = Buffer() - AdbWriter(buffer).write(Constants.CMD_CLSE, 2, 1, null, 0, 0) - val messageQueue = AdbMessageQueue(AdbReader(buffer)) - messageQueue.startListening(1) - val stream = AdbStreamImpl(messageQueue, AdbWriter(Buffer()), 1024, 1, 2) - - // A clean peer close reads as EOF (empty), NOT an exception. - Truth.assertThat(stream.source.readByteArray()).isEqualTo(ByteArray(0)) - } - - @Test - fun socketFaultThrowsConnectionClosed() { - // Empty reader: the next read hits EOF on the socket itself (not a protocol CLSE). - val messageQueue = AdbMessageQueue(AdbReader(Buffer())) - messageQueue.startListening(1) - val stream = AdbStreamImpl(messageQueue, AdbWriter(Buffer()), 1024, 1, 2) - - assertFailsWith { stream.source.readByteArray() } - } -``` - -- [ ] **Step 2: Run tests to verify they fail** - -Run: `./gradlew :dadb:test --tests dadb.AdbStreamTest` -Expected: `peerCloseIsCleanEof` PASSES already (today's swallow returns EOF), `socketFaultThrowsConnectionClosed` FAILS (today's swallow also returns EOF, so `readByteArray()` returns empty instead of throwing). - -- [ ] **Step 3: Apply the fix** - -In `dadb/src/main/kotlin/dadb/AdbStream.kt`, replace the `nextMessage` method (lines 117-124): - -```kotlin - private fun nextMessage(command: Int): AdbMessage? { - return try { - messageQueue.take(localId, command) - } catch (e: IOException) { - close() - return null - } - } -``` - -with: - -```kotlin - private fun nextMessage(command: Int): AdbMessage? { - return try { - messageQueue.take(localId, command) - } catch (e: AdbStreamClosed) { - // Peer sent A_CLSE: normal end-of-stream (e.g. shell/exec output finished). EOF is correct. - close() - null - } catch (e: IOException) { - // Real socket fault (EOF/RST) mid-stream: the transport died, not a clean close. - close() - throw AdbConnectionClosedException("Connection lost while reading stream $localId", e) - } - } -``` - -(`AdbStream.kt` already imports `java.io.IOException`; `AdbStreamClosed` and `AdbConnectionClosedException` are in the same package — no new imports needed.) - -- [ ] **Step 4: Run tests to verify they pass** - -Run: `./gradlew :dadb:test --tests dadb.AdbStreamTest` -Expected: PASS (3 tests, including the original `testLargeRemoteWrite`) - -- [ ] **Step 5: Commit** - -```bash -git add dadb/src/main/kotlin/dadb/AdbStream.kt dadb/src/test/kotlin/dadb/AdbStreamTest.kt -git commit -m "fix: surface mid-stream socket faults as AdbConnectionClosedException" -``` - ---- - -## Task 5: `AdbConnection` handshake — typed failures - -Map handshake failures to typed exceptions and guarantee no bare `IOException` leaks from the handshake. Requires making the `(Source, Sink)` connect overload `internal` as a test seam. - -**Files:** -- Modify: `dadb/src/main/kotlin/dadb/AdbConnection.kt:86-137` -- Test: `dadb/src/test/kotlin/dadb/AdbConnectionHandshakeTest.kt` - -- [ ] **Step 1: Write the failing tests** - -Create `dadb/src/test/kotlin/dadb/AdbConnectionHandshakeTest.kt`: - -```kotlin -package dadb - -import okio.Buffer -import kotlin.test.Test -import kotlin.test.assertFailsWith - -internal class AdbConnectionHandshakeTest { - - @Test - fun nonCnxnResponseThrowsConnectException() { - // Device replies with an unexpected OKAY instead of CNXN. - val device = Buffer() - AdbWriter(device).writeOkay(0, 0) - assertFailsWith { - AdbConnection.connect(device, Buffer(), null) - } - } - - @Test - fun authRequiredWithoutKeyPairThrowsAuthException() { - // Device demands auth; we have no key pair. - val device = Buffer() - AdbWriter(device).writeAuth(Constants.AUTH_TYPE_TOKEN, ByteArray(20)) - assertFailsWith { - AdbConnection.connect(device, Buffer(), null) - } - } - - @Test - fun truncatedHandshakeThrowsConnectException() { - // Empty device stream: the first readMessage hits EOF. - assertFailsWith { - AdbConnection.connect(Buffer(), Buffer(), null) - } - } -} -``` - -- [ ] **Step 2: Run tests to verify they fail** - -Run: `./gradlew :dadb:compileTestKotlin` -Expected: FAIL — `connect(Source, Sink, ...)` is `private` (unresolved/inaccessible). - -- [ ] **Step 3: Make the test seam internal and map failures** - -In `dadb/src/main/kotlin/dadb/AdbConnection.kt`: - -(a) Change the visibility of the `(Source, Sink)` overload (line 86) from `private` to `internal`, and wrap its body so any non-`AdbException` `IOException` from the handshake becomes `AdbConnectException`: - -```kotlin - internal fun connect(source: Source, sink: Sink, keyPair: AdbKeyPair? = null, closeable: Closeable? = null): AdbConnection { - val adbReader = AdbReader(source) - val adbWriter = AdbWriter(sink) - - try { - return connect(adbReader, adbWriter, keyPair, closeable) - } catch (e: AdbException) { - adbReader.close() - adbWriter.close() - throw e - } catch (e: IOException) { - adbReader.close() - adbWriter.close() - throw AdbConnectException("Connection handshake failed", e) - } catch (t: Throwable) { - adbReader.close() - adbWriter.close() - throw t - } - } -``` - -(b) Replace the handshake body in the `(AdbReader, AdbWriter)` overload (lines 99-125) with typed mappings: - -```kotlin - private fun connect(adbReader: AdbReader, adbWriter: AdbWriter, keyPair: AdbKeyPair?, closeable: Closeable?): AdbConnection { - adbWriter.writeConnect() - - var message = adbReader.readMessage() - - if (message.command == Constants.CMD_AUTH) { - if (keyPair == null) throw AdbAuthException("Authentication required but no key pair was provided") - if (message.arg0 != Constants.AUTH_TYPE_TOKEN) throw AdbProtocolException("Unsupported auth type: $message") - - val signature = keyPair.signPayload(message) - adbWriter.writeAuth(Constants.AUTH_TYPE_SIGNATURE, signature) - - message = adbReader.readMessage() - if (message.command == Constants.CMD_AUTH) { - adbWriter.writeAuth(Constants.AUTH_TYPE_RSA_PUBLIC, keyPair.publicKeyBytes) - message = adbReader.readMessage() - } - } - - if (message.command != Constants.CMD_CNXN) { - // A trailing AUTH means the device rejected our key / stayed unauthorized. - if (message.command == Constants.CMD_AUTH) throw AdbAuthException("Device rejected authentication (unauthorized)") - throw AdbConnectException("Connection failed: $message") - } - - val connectionString = parseConnectionString(String(message.payload)) - val version = message.arg0 - val maxPayloadSize = message.arg1 - - return AdbConnection(adbReader, adbWriter, closeable, connectionString.features, version, maxPayloadSize) - } -``` - -(c) In `parseConnectionString` (line 134), change the feature-parse throw to a connect failure: - -```kotlin - if ("features" !in keyValues) throw AdbConnectException("Failed to parse features from connection string: $connectionString") -``` - -(`AdbConnection.kt` already imports `java.io.IOException`; the new exception types share the package.) - -- [ ] **Step 4: Run tests to verify they pass** - -Run: `./gradlew :dadb:test --tests dadb.AdbConnectionHandshakeTest` -Expected: PASS (3 tests) - -- [ ] **Step 5: Commit** - -```bash -git add dadb/src/main/kotlin/dadb/AdbConnection.kt dadb/src/test/kotlin/dadb/AdbConnectionHandshakeTest.kt -git commit -m "feat: map adbd handshake failures to typed AdbException subtypes" -``` - ---- - -## Task 6: `AdbConnection.open` — stream-open refusal - -When adbd answers `A_OPEN` with `A_CLSE`, surface `AdbStreamOpenException` (the connection is still alive) instead of the internal `AdbStreamClosed`. The `localId` is random, so this mapping is verified by an emulator test (opening a bogus service) plus the implementation review. - -**Files:** -- Modify: `dadb/src/main/kotlin/dadb/AdbConnection.kt:42-55` -- Test: `dadb/src/test/kotlin/dadb/DadbTest.kt` (add one emulator test) - -- [ ] **Step 1: Write the failing emulator test** - -Add this method inside the `DadbTest` class in `dadb/src/test/kotlin/dadb/DadbTest.kt` (it requires a running emulator on `localhost:5555`, like the other `localEmulator { }` tests; add `import kotlin.test.assertFailsWith` if not present): - -```kotlin - @Test - fun open_invalidService_throwsStreamOpenException() { - localEmulator { dadb -> - assertFailsWith { - dadb.open("definitely-not-a-real-service:") - } - } - } -``` - -- [ ] **Step 2: Run the test to verify it fails** - -Run: `./gradlew :dadb:test --tests dadb.DadbTestImpl` -Expected: FAIL — today this throws the internal `AdbStreamClosed` (an `IOException`), not `AdbStreamOpenException`. (If no emulator is available the test errors on connect; in that case verify the mapping by code review and proceed.) - -- [ ] **Step 3: Apply the mapping** - -In `dadb/src/main/kotlin/dadb/AdbConnection.kt`, replace the `open` method (lines 42-55): - -```kotlin - @Throws(IOException::class) - fun open(destination: String): AdbStream { - val localId = newId() - messageQueue.startListening(localId) - try { - adbWriter.writeOpen(localId, destination) - val message = messageQueue.take(localId, Constants.CMD_OKAY) - val remoteId = message.arg0 - return AdbStreamImpl(messageQueue, adbWriter, maxPayloadSize, localId, remoteId) - } catch (e: AdbStreamClosed) { - // adbd answered A_OPEN with A_CLSE: it refused this service. Connection is still alive. - messageQueue.stopListening(localId) - throw AdbStreamOpenException(destination, "adbd refused to open stream: $destination", e) - } catch (e: Throwable) { - messageQueue.stopListening(localId) - throw e - } - } -``` - -- [ ] **Step 4: Run the test to verify it passes** - -Run: `./gradlew :dadb:test --tests dadb.DadbTestImpl` -Expected: PASS (with emulator). Without an emulator, run `./gradlew :dadb:compileKotlin` and confirm it compiles, then proceed. - -- [ ] **Step 5: Commit** - -```bash -git add dadb/src/main/kotlin/dadb/AdbConnection.kt dadb/src/test/kotlin/dadb/DadbTest.kt -git commit -m "feat: map A_OPEN refusal to AdbStreamOpenException" -``` - ---- - -## Task 7: `AdbSync` — internal `AdbSyncFailException` - -Replace the bare `IOException`s in the sync codec with an internal signal the high-level methods can catch, and make `send` report a `FAIL` message symmetrically with `recv`. - -**Files:** -- Modify: `dadb/src/main/kotlin/dadb/AdbSync.kt:46-97` -- Test: `dadb/src/test/kotlin/dadb/DadbResultTest.kt` (new file; sync tests here) - -- [ ] **Step 1: Write the failing tests** - -Create `dadb/src/test/kotlin/dadb/DadbResultTest.kt`: - -```kotlin -package dadb - -import com.google.common.truth.Truth.assertThat -import okio.Buffer -import kotlin.test.Test -import kotlin.test.assertFailsWith - -internal class DadbResultTest { - - @Test - fun syncRecvFailThrowsSyncFailException() { - val stream = FakeAdbStream(syncFailBuffer("No such file or directory")) - val sync = AdbSyncStream(stream) - val e = assertFailsWith { sync.recv(Buffer(), "/missing") } - assertThat(e.reason).isEqualTo("No such file or directory") - } -} -``` - -- [ ] **Step 2: Run the test to verify it fails** - -Run: `./gradlew :dadb:compileTestKotlin` -Expected: FAIL — unresolved `AdbSyncFailException`. - -- [ ] **Step 3: Add the exception and use it** - -In `dadb/src/main/kotlin/dadb/AdbSync.kt`, add the internal exception just below the imports (after line 23): - -```kotlin -/** Internal signal that an adbd sync operation returned FAIL (e.g. file not found / permission - * denied). Caught by Dadb.push/pull and folded into a [SyncResult.Failure]; never surfaced raw - * from the high-level API. Not an AdbException — the transport is healthy. */ -internal class AdbSyncFailException(val reason: String) : IOException(reason) -``` - -Replace the `send` tail (line 69-70): - -```kotlin - val packet = readPacket() - if (packet.id == FAIL) { - val message = stream.source.readString(packet.arg.toLong(), StandardCharsets.UTF_8) - throw AdbSyncFailException(message) - } - if (packet.id != OKAY) throw AdbProtocolException("Unexpected sync packet id: ${packet.id}") -``` - -Replace the `recv` FAIL/unexpected handling (lines 86-90): - -```kotlin - if (packet.id == FAIL) { - val message = stream.source.readString(packet.arg.toLong(), StandardCharsets.UTF_8) - throw AdbSyncFailException(message) - } - if (packet.id != DATA) throw AdbProtocolException("Unexpected sync packet id: ${packet.id}") -``` - -(`AdbSync.kt` already imports `java.io.IOException`, `okio.*`, and `StandardCharsets`; `AdbProtocolException` shares the package.) - -- [ ] **Step 4: Run the test to verify it passes** - -Run: `./gradlew :dadb:test --tests dadb.DadbResultTest` -Expected: PASS (1 test) - -- [ ] **Step 5: Commit** - -```bash -git add dadb/src/main/kotlin/dadb/AdbSync.kt dadb/src/test/kotlin/dadb/DadbResultTest.kt -git commit -m "feat: AdbSync throws internal AdbSyncFailException on FAIL" -``` - ---- - -## Task 8: `Dadb.push` / `Dadb.pull` → `SyncResult` - -**Files:** -- Modify: `dadb/src/main/kotlin/dadb/Dadb.kt:46-68` -- Test: `dadb/src/test/kotlin/dadb/DadbResultTest.kt` (add tests) - -- [ ] **Step 1: Write the failing tests** - -Add these methods to `DadbResultTest` in `dadb/src/test/kotlin/dadb/DadbResultTest.kt`: - -```kotlin - @Test - fun pullMissingFileReturnsSyncFailure() { - val dadb = FakeDadb { FakeAdbStream(syncFailBuffer("No such file or directory")) } - val result = dadb.pull(Buffer(), "/missing") - assertThat(result).isInstanceOf(SyncResult.Failure::class.java) - assertThat((result as SyncResult.Failure).reason).isEqualTo("No such file or directory") - } - - @Test - fun pushSuccessReturnsSyncSuccess() { - val dadb = FakeDadb { FakeAdbStream(syncOkayBuffer()) } - val result = dadb.push(Buffer().also { it.writeUtf8("hi") }, "/data/local/tmp/hi", 0b110_100_100, 0L) - assertThat(result).isInstanceOf(SyncResult.Success::class.java) - } -``` - -- [ ] **Step 2: Run tests to verify they fail** - -Run: `./gradlew :dadb:compileTestKotlin` -Expected: FAIL — `pull`/`push` return `Unit`, so `assertThat(result)` does not type-check. - -- [ ] **Step 3: Change the signatures to return `SyncResult`** - -In `dadb/src/main/kotlin/dadb/Dadb.kt`, replace the push/pull/openSync block (lines 46-74). Note `openSync` is unchanged except for its `@Throws` annotation, kept here for context: - -```kotlin - @Throws(AdbException::class) - fun push(src: File, remotePath: String, mode: Int = readMode(src), lastModifiedMs: Long = src.lastModified()): SyncResult { - return push(src.source(), remotePath, mode, lastModifiedMs) - } - - @Throws(AdbException::class) - fun push(source: Source, remotePath: String, mode: Int, lastModifiedMs: Long): SyncResult { - openSync().use { stream -> - return try { - stream.send(source, remotePath, mode, lastModifiedMs) - SyncResult.Success - } catch (e: AdbSyncFailException) { - SyncResult.Failure(e.reason) - } - } - } - - @Throws(AdbException::class) - fun pull(dst: File, remotePath: String): SyncResult { - return pull(dst.sink(append = false), remotePath) - } - - @Throws(AdbException::class) - fun pull(sink: Sink, remotePath: String): SyncResult { - openSync().use { stream -> - return try { - stream.recv(sink, remotePath) - SyncResult.Success - } catch (e: AdbSyncFailException) { - SyncResult.Failure(e.reason) - } - } - } - - @Throws(AdbException::class) - fun openSync(): AdbSyncStream { - val stream = open("sync:") - return AdbSyncStream(stream) - } -``` - -- [ ] **Step 4: Run tests to verify they pass** - -Run: `./gradlew :dadb:test --tests dadb.DadbResultTest` -Expected: PASS. The module still compiles at this point: changing `push`/`pull` to return `SyncResult` while `install`/`uninstall`/`root` still return `Unit` is fine (their call sites in `pmInstall`/`installMultiple`/`DadbTest` ignore the new return value, which is legal Kotlin). `AdbException` resolves because it is in the same package. - -> **Note for the implementer:** Tasks 8–12 each edit `Dadb.kt` and each leaves the module compiling and its targeted `DadbResultTest` cases green. The emulator-dependent assertions in `DadbTest` are validated once at the end of Task 12 / Final Verification. - -- [ ] **Step 5: Commit** - -```bash -git add dadb/src/main/kotlin/dadb/Dadb.kt dadb/src/test/kotlin/dadb/DadbResultTest.kt -git commit -m "feat: push/pull return SyncResult" -``` - ---- - -## Task 9: `Dadb.uninstall` → `UninstallResult` - -**Files:** -- Modify: `dadb/src/main/kotlin/dadb/Dadb.kt:201-207` -- Test: `dadb/src/test/kotlin/dadb/DadbResultTest.kt` (add tests) - -- [ ] **Step 1: Write the failing tests** - -Add to `DadbResultTest`: - -```kotlin - @Test - fun uninstallNonZeroExitReturnsFailureWithExitCode() { - val dadb = FakeDadb { - FakeAdbStream(shellV2Buffer(stdout = "Failure [DELETE_FAILED_INTERNAL_ERROR]\n", exitCode = 1)) - } - val result = dadb.uninstall("com.example.absent") - assertThat(result).isInstanceOf(UninstallResult.Failure::class.java) - result as UninstallResult.Failure - assertThat(result.exitCode).isEqualTo(1) - assertThat(result.reason).contains("DELETE_FAILED_INTERNAL_ERROR") - } - - @Test - fun uninstallZeroExitReturnsSuccess() { - val dadb = FakeDadb { FakeAdbStream(shellV2Buffer(stdout = "Success\n", exitCode = 0)) } - assertThat(dadb.uninstall("com.example")).isInstanceOf(UninstallResult.Success::class.java) - } -``` - -- [ ] **Step 2: Run tests to verify they fail** - -Run: `./gradlew :dadb:compileTestKotlin` -Expected: FAIL — `uninstall` returns `Unit`. - -- [ ] **Step 3: Change `uninstall`** - -In `dadb/src/main/kotlin/dadb/Dadb.kt`, replace `uninstall` (lines 201-207): - -```kotlin - @Throws(AdbException::class) - fun uninstall(packageName: String): UninstallResult { - val response = shell("cmd package uninstall $packageName") - return if (response.exitCode == 0) { - UninstallResult.Success - } else { - UninstallResult.Failure(reason = response.allOutput, exitCode = response.exitCode) - } - } -``` - -- [ ] **Step 4: Run tests to verify they pass** - -Run: `./gradlew :dadb:test --tests dadb.DadbResultTest` (or defer to Task 12's combined run, per the Task 8 note) -Expected: PASS - -- [ ] **Step 5: Commit** - -```bash -git add dadb/src/main/kotlin/dadb/Dadb.kt dadb/src/test/kotlin/dadb/DadbResultTest.kt -git commit -m "feat: uninstall returns UninstallResult" -``` - ---- - -## Task 10: `Dadb.install` (+ `pmInstall`) → `InstallResult` - -This also fixes a latent bug: the current `pmInstall` ignores the `pm install` result entirely. - -**Files:** -- Modify: `dadb/src/main/kotlin/dadb/Dadb.kt:76-110` -- Test: `dadb/src/test/kotlin/dadb/DadbResultTest.kt` (add tests) - -- [ ] **Step 1: Write the failing tests** (cmd-path; the pm-path is covered by the emulator suite) - -Add to `DadbResultTest`: - -```kotlin - @Test - fun installCmdPathFailureReturnsInstallFailure() { - val dadb = FakeDadb(features = setOf("cmd")) { - FakeAdbStream(Buffer().also { it.writeUtf8("Failure [INSTALL_FAILED_INVALID_APK]") }) - } - val result = dadb.install(Buffer().also { it.writeUtf8("apk") }, 3L) - assertThat(result).isInstanceOf(InstallResult.Failure::class.java) - assertThat((result as InstallResult.Failure).reason).contains("INSTALL_FAILED_INVALID_APK") - } - - @Test - fun installCmdPathSuccessReturnsSuccess() { - val dadb = FakeDadb(features = setOf("cmd")) { - FakeAdbStream(Buffer().also { it.writeUtf8("Success\n") }) - } - val result = dadb.install(Buffer().also { it.writeUtf8("apk") }, 3L) - assertThat(result).isInstanceOf(InstallResult.Success::class.java) - } -``` - -- [ ] **Step 2: Run tests to verify they fail** - -Run: `./gradlew :dadb:compileTestKotlin` -Expected: FAIL — `install` returns `Unit`. - -- [ ] **Step 3: Change `install` and `pmInstall`** - -In `dadb/src/main/kotlin/dadb/Dadb.kt`, replace `install(file)`, `install(source, size)`, and `pmInstall` (lines 76-110): - -```kotlin - @Throws(AdbException::class) - fun install(file: File, vararg options: String): InstallResult { - return if (supportsFeature("cmd")) { - install(file.source(), file.length(), *options) - } else { - pmInstall(file, *options) - } - } - - @Throws(AdbException::class) - fun install(source: Source, size: Long, vararg options: String): InstallResult { - if (supportsFeature("cmd")) { - execCmd("package", "install", "-S", size.toString(), *options).use { stream -> - stream.sink.writeAll(source) - stream.sink.flush() - val response = stream.source.readString(Charsets.UTF_8) - return if (response.startsWith("Success")) InstallResult.Success else InstallResult.Failure(response) - } - } else { - val tempFile = kotlin.io.path.createTempFile() - val fileSink = tempFile.sink().buffer() - fileSink.writeAll(source) - fileSink.flush() - return pmInstall(tempFile.toFile(), *options) - } - } - - private fun pmInstall(file: File, vararg options: String): InstallResult { - val remotePath = "/data/local/tmp/${file.name}" - when (val pushResult = push(file, remotePath)) { - is SyncResult.Failure -> return InstallResult.Failure("push failed: ${pushResult.reason}") - SyncResult.Success -> Unit - } - val response = shell("pm install ${options.joinToString(" ")} \"$remotePath\"") - return if (response.allOutput.startsWith("Success")) InstallResult.Success else InstallResult.Failure(response.allOutput) - } -``` - -- [ ] **Step 4: Run tests to verify they pass** - -Run: `./gradlew :dadb:test --tests dadb.DadbResultTest` (or defer to Task 12's combined run) -Expected: PASS - -- [ ] **Step 5: Commit** - -```bash -git add dadb/src/main/kotlin/dadb/Dadb.kt dadb/src/test/kotlin/dadb/DadbResultTest.kt -git commit -m "feat: install returns InstallResult and checks the pm-install result" -``` - ---- - -## Task 11: `Dadb.installMultiple` → `InstallResult` - -Translate every `throw IOException(...)` operation failure in both the `cmd` and `pm` paths into an `InstallResult.Failure` return. Also fixes the `Dadb.kt:145` message bug (`$commitStream` → the response). This path is verified by the emulator suite (`DadbTest.installMultiple…`); update those tests to the new return type. - -**Files:** -- Modify: `dadb/src/main/kotlin/dadb/Dadb.kt:112-199` -- Test: `dadb/src/test/kotlin/dadb/DadbTest.kt` (update existing emulator assertions) - -- [ ] **Step 1: Rewrite `installMultiple`** - -In `dadb/src/main/kotlin/dadb/Dadb.kt`, replace `installMultiple` (lines 112-199): - -```kotlin - @Throws(AdbException::class) - fun installMultiple(apks: List, vararg options: String): InstallResult { - // http://aospxref.com/android-12.0.0_r3/xref/packages/modules/adb/client/adb_install.cpp#538 - if (supportsFeature("cmd")) { - val totalLength = apks.map { it.length() }.reduce { acc, l -> acc + l } - execCmd("package", "install-create", "-S", totalLength.toString(), *options).use { createStream -> - val response = createStream.source.readString(Charsets.UTF_8) - if (!response.startsWith("Success")) return InstallResult.Failure("create session failed: $response") - - val pattern = """\[(\w+)]""".toRegex() - val sessionId = pattern.find(response)?.groups?.get(1)?.value - ?: return InstallResult.Failure("failed to parse session id: $response") - - var error: String? = null - apks.forEach { apk -> - execCmd("package", "install-write", "-S", apk.length().toString(), sessionId, apk.name, "-", *options).use { writeStream -> - writeStream.sink.writeAll(apk.source()) - writeStream.sink.flush() - val writeResponse = writeStream.source.readString(Charsets.UTF_8) - if (!writeResponse.startsWith("Success")) { - error = writeResponse - return@forEach - } - } - } - - val finalCommand = if (error == null) "install-commit" else "install-abandon" - execCmd("package", finalCommand, sessionId, *options).use { commitStream -> - val finalResponse = commitStream.source.readString(Charsets.UTF_8) - if (!finalResponse.startsWith("Success")) return InstallResult.Failure("failed to finalize session: $finalResponse") - } - - return if (error == null) InstallResult.Success else InstallResult.Failure(error!!) - } - } else { - val totalLength = apks.map { it.length() }.reduce { acc, l -> acc + l } - val response = shell("pm install-create -S $totalLength ${options.joinToString(" ")}") - if (!response.allOutput.startsWith("Success")) return InstallResult.Failure("pm create session failed: ${response.allOutput}") - - val pattern = """\[(\w+)]""".toRegex() - val sessionId = pattern.find(response.allOutput)?.groups?.get(1)?.value - ?: return InstallResult.Failure("failed to parse session id: ${response.allOutput}") - var error: String? = null - - val fileNames = apks.map { it.name } - val remotePaths = fileNames.map { "/data/local/tmp/$it" } - - apks.zip(remotePaths).forEachIndexed { index, pair -> - val apk = pair.first - val remotePath = pair.second - - when (val pushResult = push(apk, remotePath)) { - is SyncResult.Failure -> { error = "push failed: ${pushResult.reason}"; return@forEachIndexed } - SyncResult.Success -> Unit - } - - val writeResponse = shell("pm install-write -S ${apk.length()} $sessionId $index $remotePath") - if (!writeResponse.allOutput.startsWith("Success")) { - error = writeResponse.allOutput - return@forEachIndexed - } - } - - val finalCommand = if (error == null) "pm install-commit $sessionId" else "pm install-abandon $sessionId" - val finalResponse = shell(finalCommand) - if (!finalResponse.allOutput.startsWith("Success")) return InstallResult.Failure("failed to finalize session: ${finalResponse.allOutput}") - return if (error == null) InstallResult.Success else InstallResult.Failure(error!!) - } - } -``` - -- [ ] **Step 2: Update the emulator tests in `DadbTest.kt`** - -Find the `installMultiple` / `install` assertions in `dadb/src/test/kotlin/dadb/DadbTest.kt`. Where a test previously called `dadb.install(apk)` / `dadb.installMultiple(apks)` and expected no exception, assert success instead. For example, a test body like: - -```kotlin - localEmulator { dadb -> - dadb.install(apkFile) - } -``` - -becomes: - -```kotlin - localEmulator { dadb -> - assertThat(dadb.install(apkFile)).isInstanceOf(InstallResult.Success::class.java) - } -``` - -Apply the same `assertThat(...).isInstanceOf(InstallResult.Success::class.java)` to each `install`/`installMultiple` call site, and update any `uninstall`/`push`/`pull` call sites to their new result types (`UninstallResult.Success`, `SyncResult.Success`). Add `import com.google.common.truth.Truth.assertThat` if missing. - -- [ ] **Step 3: Verify compilation** - -Run: `./gradlew :dadb:compileKotlin :dadb:compileTestKotlin` -Expected: PASS (whole module compiles; combined test run happens in Task 12) - -- [ ] **Step 4: Commit** - -```bash -git add dadb/src/main/kotlin/dadb/Dadb.kt dadb/src/test/kotlin/dadb/DadbTest.kt -git commit -m "feat: installMultiple returns InstallResult; fix finalize message bug" -``` - ---- - -## Task 12: `Dadb.root` / `unroot` → `RootResult`, `@Throws` sweep, full suite - -**Files:** -- Modify: `dadb/src/main/kotlin/dadb/Dadb.kt:28-247` (root/unroot + remaining `@Throws` annotations) -- Test: `dadb/src/test/kotlin/dadb/DadbResultTest.kt` (add root tests) - -- [ ] **Step 1: Write the failing tests** - -Add to `DadbResultTest` in `dadb/src/test/kotlin/dadb/DadbResultTest.kt`: - -```kotlin - @Test - fun rootFailureReturnsRootFailure() { - // root: service replies with a non-restarting line, terminated by '\n'. - val dadb = FakeDadb { - FakeAdbStream(Buffer().also { it.writeUtf8("adbd cannot run as root in production builds\n") }) - } - val result = dadb.root() - assertThat(result).isInstanceOf(RootResult.Failure::class.java) - assertThat((result as RootResult.Failure).reason).contains("production builds") - } -``` - -- [ ] **Step 2: Run the test to verify it fails** - -Run: `./gradlew :dadb:compileTestKotlin` -Expected: FAIL — `root` returns `Unit`. - -- [ ] **Step 3: Rewrite root/unroot and finish the `@Throws` sweep** - -In `dadb/src/main/kotlin/dadb/Dadb.kt`, replace `root` and `unroot` (lines 223-239): - -```kotlin - @Throws(AdbException::class) - fun root(): RootResult = restartAdbd("root:", root = true) { it.startsWith("restarting") || it.contains("already") } - - @Throws(AdbException::class) - fun unroot(): RootResult = restartAdbd("unroot:", root = false) { it.startsWith("restarting") || it.contains("not running as root") } - - private fun restartAdbd(service: String, root: Boolean, isSuccess: (String) -> Boolean): RootResult { - val response = restartAdb(this, service) - if (!isSuccess(response)) return RootResult.Failure(response) - waitRootOrClose(this, root) - return RootResult.Success - } -``` - -Then change the remaining `@Throws(IOException::class)` annotations on the **public interface methods** to `@Throws(AdbException::class)`: `open` (line 28), `shell` (33), `openShell` (40), `execCmd` (209), `abbExec` (216). Leave `tcpForward`'s `@Throws(InterruptedException::class)` unchanged. Add the import `import dadb.AdbException` is unnecessary (same package), but ensure `Dadb.kt` no longer needs the now-unused `IOException` import only if nothing else uses it — `readMode` still throws `RuntimeException` and the companion still references `IOException` in `waitRootOrClose`'s catch, so **keep** the `okio.*`/`java.io` imports as-is. - -> The `@Throws` annotation value must resolve; since `AdbException` is in package `dadb`, no import is needed. - -- [ ] **Step 4: Run the full test suite** - -Run: `./gradlew :dadb:test` -Expected: PASS for all non-emulator tests — `AdbExceptionTest`, `ResultTypesTest`, `AdbStreamTest`, `AdbConnectionHandshakeTest`, `DadbResultTest`, `MessageQueueTest`, `PKCS8Test`. Emulator-dependent suites (`DadbTestImpl`, `AdbServerTest`, `DadbImplTest`) require a device; if none is attached they will error on connect — that is expected in a non-emulator environment. Confirm the unit suites are green. - -- [ ] **Step 5: Commit** - -```bash -git add dadb/src/main/kotlin/dadb/Dadb.kt dadb/src/test/kotlin/dadb/DadbResultTest.kt -git commit -m "feat: root/unroot return RootResult; @Throws(AdbException) sweep" -``` - ---- - -## Task 13: Update docs & changelog - -**Files:** -- Modify: `README.md`, `CHANGELOG.md` - -- [ ] **Step 1: Update the README usage examples** - -In `README.md`, update the install example and any others to the new return types. Replace: - -```kotlin -Dadb.create("localhost", 5555).use { dadb -> - dadb.install(apkFile) -} -``` - -with: - -```kotlin -Dadb.create("localhost", 5555).use { dadb -> - when (val result = dadb.install(apkFile)) { - is InstallResult.Success -> println("installed") - is InstallResult.Failure -> println("install failed: ${result.reason}") - } -} -``` - -Add a short "Error handling" subsection after the usage examples: - -```markdown -### Error handling - -dadb distinguishes *transport* failures from *operation* failures: - -- **Transport failures throw** an `AdbException` (an `IOException`): `AdbConnectException`, - `AdbAuthException`, `AdbStreamOpenException`, `AdbConnectionClosedException`, `AdbProtocolException`. - Catch `AdbException` to decide whether to reconnect. -- **Operation outcomes are returned**, never thrown: `install`/`installMultiple` → `InstallResult`, - `uninstall` → `UninstallResult`, `push`/`pull` → `SyncResult`, `root`/`unroot` → `RootResult`. - A non-zero `shell` exit code stays a value on `AdbShellResponse`. -``` - -- [ ] **Step 2: Add a CHANGELOG entry** - -Prepend an entry to `CHANGELOG.md` describing the major version's breaking changes (typed `AdbException` hierarchy; result-returning `install`/`installMultiple`/`uninstall`/`push`/`pull`/`root`/`unroot`; mid-stream socket faults now throw `AdbConnectionClosedException` instead of EOF). Match the file's existing entry format. - -- [ ] **Step 3: Commit** - -```bash -git add README.md CHANGELOG.md -git commit -m "docs: document the adbd-aware error model" -``` - ---- - -## Final Verification - -- [ ] Run the unit suites and confirm green: - `./gradlew :dadb:test --tests dadb.AdbExceptionTest --tests dadb.ResultTypesTest --tests dadb.AdbStreamTest --tests dadb.AdbConnectionHandshakeTest --tests dadb.DadbResultTest --tests dadb.MessageQueueTest --tests dadb.PKCS8Test` -- [ ] Confirm the whole module compiles: `./gradlew :dadb:compileKotlin :dadb:compileTestKotlin` -- [ ] Grep for leftover bare operation-failure throws that should now be results: - `grep -n "throw IOException" dadb/src/main/kotlin/dadb/Dadb.kt dadb/src/main/kotlin/dadb/AdbSync.kt` — expect **no matches** in `Dadb.kt` and `AdbSync.kt`. -- [ ] (If an emulator is available) run `./gradlew :dadb:test` in full and confirm the emulator suites pass, including `open_invalidService_throwsStreamOpenException` and the updated `install`/`installMultiple` assertions. diff --git a/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md b/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md deleted file mode 100644 index a0a39a1..0000000 --- a/docs/superpowers/specs/2026-06-05-dadb-error-model-design.md +++ /dev/null @@ -1,232 +0,0 @@ -# dadb Error Model Redesign — Design Spec - -- **Date:** 2026-06-05 -- **Status:** Proposed (awaiting review) -- **Scope:** Direct-to-adbd error handling only (concern #1). Host-server / smart-socket separation (concern #2) is a **separate** effort and explicitly out of scope here. - ---- - -## 1. Problem - -`dadb` funnels nearly every failure through bare `java.io.IOException`. The only library-specific exception type, `AdbStreamClosed`, is `internal` and uncatchable by consumers. As a result, a caller cannot programmatically distinguish: - -- **"the connection to adbd is dead — reconnect"** (socket reset, handshake failed, auth rejected), from -- **"the connection is fine, but the operation I asked for failed"** (an apk was rejected, a remote file was missing, a command exited non-zero). - -The only way to tell them apart today is string-matching exception messages. Concretely, these throw `IOException` for *operation* failures while the connection is perfectly alive (`Dadb.kt`): install rejected (`:93`, `:150`, `:196`), pm session failures (`:121`, `:123`, `:159`, `:162`), finalize failures (`:145`, `:193`), uninstall non-zero — exit code discarded into a string (`:205`), root/unroot refused (`:227`, `:236`); plus sync `FAIL` for a missing/forbidden remote file (`AdbSync.kt:88`). - -There is also an **inverse** bug: `AdbStreamImpl.nextMessage` (`AdbStream.kt:117-124`) swallows a real socket fault mid-read and converts it into a clean EOF, so a dropped connection during `shell`/`pull` can masquerade as success — the opposite failure, same root cause (an undifferentiated `IOException` channel). - -## 2. Goal & principle - -Make the error model **adbd-aware**: every outcome maps to a distinct, documented place the ADB wire protocol (`packages/modules/adb/protocol.txt`) can produce it. One governing principle: - -> **An exception means "I could not get an outcome — the transport/connection/auth is in trouble." A returned result means "I got an outcome — here is whether the operation succeeded."** - -This mirrors what the most active implementations do: Google's `adblib` (distinct `AdbProtocolErrorException` vs `AdbFailResponseException`; exit codes as values), `adam` (`ShellCommandResult.exitCode`), and Rust `adb_client` (non-zero exit is `Ok(...)`). It also follows JVM convention: root the hierarchy at `IOException` (as `java.net` does with `SocketException`), and treat an expected "no" as data, not a throw (OkHttp's "a response is not an exception"; Kotlin's sealed-result guidance). - -## 3. Scope - -**In scope** — failure modes reachable by talking straight to adbd over the socket: - -| adbd failure mode (`protocol.txt` unless noted) | Maps to | -|---|---| -| TCP connect refused/timeout; `A_CNXN` handshake never completes / version mismatch / unparseable banner | `AdbConnectException` (throw) | -| `A_AUTH` rejected — keys refused or pubkey prompt denied; device stays UNAUTHORIZED | `AdbAuthException` (throw) | -| `A_OPEN` answered with `A_CLSE` — adbd refused to open the stream (bad/forbidden service, device offline). Connection still alive. | `AdbStreamOpenException` (throw) | -| Unexpected socket fault (EOF/RST) **mid-stream** | `AdbConnectionClosedException` (throw) | -| Malformed/unexpected apacket — checksum/magic/length desync | `AdbProtocolException` (throw) | -| `sync:` returns `FAIL` + msg — ENOENT, EACCES (`SYNC.TXT`) | `SyncResult.Failure` (**result**) | -| `shell,v2:` `kIdExit` exit code (`shell_service.cpp`) | `exitCode` in `AdbShellResponse` (**result**, already exists) | - -**Out of scope (deliberately):** -- The host-server **smart-socket** protocol (`SERVICES.TXT`, 4-hex-length `OKAY`/`FAIL`) and its failures (`device offline`, `not found`, `more than one device`). That is the adb-server path (`dadb.adbserver.*`) and belongs to concern #2. adbd never speaks the smart-socket protocol; it has no place in this model. -- Restructuring `discover()`/`list()`/`AdbServer` (concern #2). -- Concurrency/resource fixes found during review (separate work). - -## 4. Exception hierarchy - -All types extend a sealed `AdbException`, which extends `IOException`. This is a merit choice, not a compatibility one: these failures genuinely *are* socket/transport I/O faults and they are recoverable (reconnect) — the textbook case for a checked `IOException` (Effective Java item 70), mirroring `java.net`'s `SocketException : IOException`. It is also consistent with our own surface: `AdbStream` exposes okio `Source`/`Sink`, whose reads/writes already throw `IOException`, so a consumer streaming from adbd is already handling `IOException`. - -**Contract guarantee:** no bare `IOException` ever leaks from a public method — every transport fault is wrapped as one of the subtypes below. Public methods that can fail at the transport are therefore annotated `@Throws(AdbException::class)` (a strictly more precise Java signal than the current `@Throws(IOException::class)`). - -```kotlin -package dadb - -import java.io.IOException - -/** Root of all dadb transport/connection failures. Extends IOException because these are - * genuine, recoverable socket I/O faults, consistent with the okio stream surface. */ -sealed class AdbException(message: String, cause: Throwable? = null) : IOException(message, cause) - -/** Could not establish the connection: TCP connect/timeout, or the A_CNXN handshake failed. - * Nothing ran — safe to retry by reconnecting. */ -class AdbConnectException(message: String, cause: Throwable? = null) : AdbException(message, cause) - -/** A_AUTH was rejected (keys refused / on-device prompt denied; device UNAUTHORIZED). - * Reconnecting with the same key will not help — fix or accept the key. */ -class AdbAuthException(message: String, cause: Throwable? = null) : AdbException(message, cause) - -/** adbd refused to OPEN the stream (A_OPEN answered with A_CLSE): unknown/forbidden service - * string, or device offline. The underlying connection is still usable. */ -class AdbStreamOpenException(val destination: String, message: String, cause: Throwable? = null) - : AdbException(message, cause) - -/** An established connection/stream died unexpectedly (socket EOF/RST mid-operation). - * Reconnect — but the operation MAY have partially executed; re-check state before retrying - * anything non-idempotent. */ -class AdbConnectionClosedException(message: String, cause: Throwable? = null) - : AdbException(message, cause) - -/** The peer sent a malformed or unexpected apacket (checksum/magic/length desync). There is no - * reliable way to re-sync — the connection must be closed and re-established. */ -class AdbProtocolException(message: String, cause: Throwable? = null) : AdbException(message, cause) -``` - -**On retryability:** we deliberately do **not** add a `retryable: Boolean` flag. A flag would lie in the one case that matters most — `AdbConnectionClosedException` — where adbd's protocol gives no way to know whether a mid-stream `pm install` already committed (gRPC's "only retry if the server never saw the request" problem). Instead, retry semantics are encoded in the **type** (the JDBC `SQLTransientException` / adblib approach) and documented per type: `AdbConnectException` = nothing ran, safe to retry; `AdbConnectionClosedException` = re-check state first. - -### Mapping to current code - -- `AdbConnection.connect` (`AdbConnection.kt`): `IOException("Connection failed", :118)` and the feature-parse `IOException` (`:134`, an unparseable handshake banner) → `AdbConnectException` (both are connection-establishment failures); auth `checkNotNull`/`check` (`:105-106`) → `AdbAuthException`. Raw socket faults from `AdbReader`/`AdbWriter` during the handshake → wrapped as `AdbConnectException(cause = e)`. `AdbProtocolException` is reserved for an unexpected/malformed apacket on an **already-established** connection (desync), not for handshake-time failures. -- `AdbConnection.open` (`AdbConnection.kt:43-55`): when `take(localId, CMD_OKAY)` sees a peer `CLSE` (currently surfaces as the internal `AdbStreamClosed`), throw `AdbStreamOpenException(destination)`. A raw socket fault here → `AdbConnectionClosedException`. -- `AdbStreamImpl.nextMessage` (`AdbStream.kt:117-124`) — the behavioral fix, see §6. -- `AdbStreamClosed` (internal) is retained only as an **internal** signal that the peer closed a stream; it is translated to the appropriate public type at the boundary (open vs mid-stream). It is never thrown to consumers. - -## 5. Result types - -Expected, in-band adbd outcomes become return values, never exceptions. Each **named high-level operation returns its own domain result type**, so peer operations are peers in the API — `install` and `uninstall` both return an `…Result`, not one a result and the other a raw shell response. Each `Failure` carries the richest honest detail the underlying adbd interaction produced, so the wrapper never *hides* what adbd did even though it presents a uniform surface. - -### Two-axis consistency rule - -The `Dadb` methods sit at different distances from adbd — `open()` is 1:1 with an `A_OPEN`; `shell`/`push`/`pull`/`uninstall` are single-service; `install`/`installMultiple`/`root`/`unroot` are composites that orchestrate several adbd ops. Consistency comes from applying one rule across all of them, on two independent axes: - -- **Transport/connection failures are universal.** Every operation can throw the single `AdbException` hierarchy (§4), because every operation ultimately goes through `open()` and the stream. These are *never* operation-specific. -- **Outcome results are operation-specific.** Each named operation gets its own `…Result { Success | Failure(…) }`; peers share one type. A composite folds any constituent failure into *its own* verdict rather than leaking a lower-level result (e.g. an `install` whose `pm`-path push sync-FAILs returns `InstallResult.Failure`, not a `SyncResult`). - -This is the deliberate choice of API-surface uniformity ("Philosophy B") over mechanism-mirroring: `uninstall` gets a domain type even though it is one shell command, accepting a small amount of wrapping in exchange for `install`/`uninstall` being true peers. The non-obfuscation constraint is preserved by carrying adbd's real detail in the `Failure` payload. - -| Operation(s) | Result type | -|---|---| -| `install`, `installMultiple` | `InstallResult` | -| `uninstall` | `UninstallResult` | -| `push`, `pull` | `SyncResult` | -| `root`, `unroot` | `RootResult` | -| `shell`, `openShell` | `AdbShellResponse` / `AdbShellStream` (generic primitive — see below) | - -```kotlin -sealed interface InstallResult { // install(), installMultiple() - object Success : InstallResult - /** Raw pm/`cmd package` verdict, surfaced opaquely — exactly as the canonical adb client does: - * client/adb_install.cpp checks `strncmp("Success", buf, 7)` and prints any other response - * verbatim. The `INSTALL_FAILED_*` codes are a frameworks/base (PackageManager) concept, NOT - * part of the adbd contract, so dadb does not parse them into an enum. */ - data class Failure(val reason: String) : InstallResult -} - -sealed interface UninstallResult { // uninstall() - object Success : UninstallResult - /** `uninstall` is `cmd package uninstall` over the shell service. Failure preserves adbd's - * actual process `exitCode` and combined output (`reason`) so the domain wrapper hides - * nothing — the whole point of choosing a wrapper here is peer-symmetry with install, not - * concealment. */ - data class Failure(val reason: String, val exitCode: Int) : UninstallResult -} - -sealed interface SyncResult { // push(), pull() - object Success : SyncResult - /** The adbd sync FAIL message, e.g. "No such file or directory" (SYNC.TXT). */ - data class Failure(val reason: String) : SyncResult -} - -sealed interface RootResult { // root(), unroot() - object Success : RootResult - /** The adbd root:/unroot: service response line, e.g. - * "adbd cannot run as root in production builds". This service has no exit code. */ - data class Failure(val reason: String) : RootResult -} -``` - -**Granularity:** the named operations collapse to four domain types (`InstallResult`, `UninstallResult`, `SyncResult`, `RootResult`), peers sharing one type. Not a single generic `Result` — the `Failure` payloads genuinely differ (uninstall carries an exit code; the others do not), and a generic wrapper would erase those distinctions. - -**`shell`/`openShell` stay `AdbShellResponse`/`AdbShellStream`.** These are the *generic shell primitive* callers build their own commands on, not a named package-lifecycle operation, so they expose the shell service's outcome directly (`exitCode` as a value, never thrown). `shell` is the tool; `install`/`uninstall`/etc. are the abstractions built with it. - -**Division of labor — results vs throws within an operation.** Low-level stream codecs keep throwing internally; the high-level operation catches the *operation-failure* signal and returns a `Failure`, while letting *transport* exceptions propagate: - -- `AdbSyncStream.send`/`recv` (`AdbSync.kt`) throw a new internal `AdbSyncFailException(message)` for a sync `FAIL`/unexpected-id (replacing the bare `IOException` at `:70`, `:88`, `:90`). `Dadb.push`/`pull` catch it → `SyncResult.Failure(message)`; any `AdbConnectionClosedException`/`AdbProtocolException` propagates untouched. -- `Dadb.install`/`installMultiple` return `InstallResult` by inspecting the `cmd`/`pm` response instead of `throw IOException("Install failed: …")`. -- `Dadb.uninstall` runs the shell command and maps the `AdbShellResponse` into `UninstallResult` — `exitCode == 0` → `Success`, else `Failure(reason = response.allOutput, exitCode = response.exitCode)`. -- `Dadb.root`/`unroot` return `RootResult` by classifying the adbd service's response line instead of throwing. - -So every high-level op may still **throw** an `AdbException` (transport/auth died → no outcome), and otherwise **returns** a result (an outcome was obtained, success or failure). - -## 6. Behavioral fix: stop masquerading transport death as EOF - -`AdbStreamImpl.nextMessage` currently swallows *all* `IOException`s into a clean EOF. The fix distinguishes the two cases the protocol actually produces: - -```kotlin -private fun nextMessage(command: Int): AdbMessage? { - return try { - messageQueue.take(localId, command) - } catch (e: AdbStreamClosed) { - // Peer sent A_CLSE: normal end-of-stream (e.g. shell/exec output finished). EOF is correct. - close() - null - } catch (e: IOException) { - // Real socket fault (EOF/RST) mid-stream: the transport died, not a clean close. - close() - throw AdbConnectionClosedException("Connection lost while reading stream $localId", e) - } -} -``` - -A peer `A_CLSE` remains a legitimate EOF (it is how adbd signals a service's output is done); only a genuine socket fault now surfaces as `AdbConnectionClosedException`. The previous blanket swallow was a latent bug — silently presenting a dropped connection as a clean end-of-stream — so this is simply the correct behavior, not a reluctant break. - -## 7. Per-operation migration table - -| Operation | Today | After | -|---|---|---| -| `open(dest): AdbStream` | throws `IOException` (incl. internal `AdbStreamClosed`) | throws `AdbStreamOpenException` / `AdbConnectException` / `AdbConnectionClosedException` / `AdbAuthException` (all `AdbException`) | -| `shell(cmd): AdbShellResponse` | exit code in result; transport → `IOException` | unchanged; transport → typed `AdbException` | -| `install(...)`: `Unit` | throws `IOException` on rejection | **returns `InstallResult`**; transport → `AdbException` | -| `installMultiple(...)`: `Unit` | throws `IOException` on rejection | **returns `InstallResult`**; transport → `AdbException` | -| `uninstall(pkg)`: `Unit` | throws `IOException` if exit≠0 | **returns `UninstallResult`** (`Failure` carries `reason` + `exitCode`); transport → `AdbException` | -| `push(...)`: `Unit` | throws `IOException` on sync FAIL | **returns `SyncResult`**; transport → `AdbException` | -| `pull(...)`: `Unit` | throws `IOException` on sync FAIL/missing file | **returns `SyncResult`**; transport → `AdbException` | -| `root()` / `unroot()`: `Unit` | throws `IOException` if refused | **returns `RootResult`**; transport → `AdbException` | -| `AdbSyncStream.send`/`recv` | throws bare `IOException` | throws internal `AdbSyncFailException` (op) / `AdbException` (transport) | - -`AdbStreamClosed` becomes a non-public translation detail. Public methods that can fail at the transport are annotated `@Throws(AdbException::class)` (precise Java checked signal); methods that return result types only throw `AdbException` when the transport dies before an outcome can be obtained. - -## 8. Migration (major version) - -This ships as a **major** version bump. Backward compatibility is *not* a design constraint — the contract above is chosen on its merits, and consumers opt into the upgrade when ready. This section exists only to make that opt-in straightforward, not to soften the contract. - -Breaking changes consumers will see: - -- **Return types:** `install`, `installMultiple`, `uninstall`, `push`, `pull`, `root`, `unroot` change from `Unit` to result types; call sites must inspect the result (`when (result) { is …Success -> …; is …Failure -> … }`). -- **Exception types:** transport/auth failures are now `AdbException` subtypes instead of bare `IOException`/`IllegalStateException`. Code that switched on exception *messages* should switch on *types* instead. -- **Behavior:** a connection dropped mid-stream now throws `AdbConnectionClosedException` instead of presenting as a clean EOF (§6). - -Release notes will enumerate these with before/after snippets. (Incidental: because `AdbException : IOException`, a coarse `catch (IOException)` still compiles and catches — but that is a consequence of the correct base type, not a goal, and is not how callers should distinguish failures going forward.) - -No on-the-wire behavior of the protocol classes (`AdbReader`/`AdbWriter`/`AdbMessageQueue`/`AdbKeyPair`) changes — only how their failures are typed and surfaced. - -## 9. Testing strategy - -- **Unit (no emulator):** drive `AdbConnection.connect` and `open` against in-memory okio `Buffer`s (as `AdbStreamTest` already does) to assert each apacket failure maps to the right type — `A_CNXN` failure → `AdbConnectException`, `A_AUTH` rejection → `AdbAuthException`, `A_OPEN`→`A_CLSE` → `AdbStreamOpenException`, mid-stream socket fault → `AdbConnectionClosedException`, malformed apacket → `AdbProtocolException`. Cover the `nextMessage` split: peer `A_CLSE` → EOF (`read() == -1`), socket fault → throw. -- **Sync codec:** feed a `FAIL`+message packet → assert `AdbSyncStream.recv` throws `AdbSyncFailException` and `Dadb.pull` returns `SyncResult.Failure(message)`. -- **Emulator (existing `DadbTest` style):** install a bad/duplicate apk → `InstallResult.Failure`; uninstall an absent package → `UninstallResult.Failure` (non-zero `exitCode` preserved); pull a missing path → `SyncResult.Failure`; `unroot` on a production-style build → `RootResult.Failure`; happy paths → `Success`. - -## 10. Future / not now - -- Concern #2: split the direct-adbd module from the adb-server module; make binary-spawning opt-in; give server-backed connections a distinct type. The smart-socket error types (`AdbHostResponseException` carrying `service`/`failMessage`) belong to that effort, not this one. -- Optional `shellOrThrow`-style convenience wrappers, if callers ask for them after the result migration. - -## 11. References - -- AOSP `packages/modules/adb`: `protocol.txt` (A_CNXN/A_AUTH/A_OPEN/A_OKAY/A_WRTE/A_CLSE), `SERVICES.TXT` (smart-socket — out of scope), `SYNC.TXT` (sync FAIL), `shell_service.cpp` (`kIdExit`), `client/adb_install.cpp` (`strncmp("Success", buf, 7)` — failure text is opaque, not structured). -- `frameworks/base` `core/java/android/content/pm/PackageManager.java` — the `INSTALL_FAILED_*` constants. Noted only to document that they live in the framework, *not* the adb/adbd contract, which is why dadb does not parse them. -- Google `adblib`: `AdbProtocolErrorException` vs sealed `AdbFailResponseException`; shell-v2 exit code as a flow value. -- `adam` (Malinskiy): `ShellCommandResult.exitCode`; the cautionary conflation of transport-dead and FAIL into one `RequestRejectedException`. -- Rust `adb_client`: `RustADBError` — non-zero exit is `Ok`, FAIL string carried in `ADBRequestFailed`. -- Conventions: Effective Java items 70–73 (checked-for-recoverable, exception translation); JDBC `SQLTransientException`/`SQLNonTransientException`; OkHttp "a response is not an exception"; Kotlin sealed-result guidance (Elizarov). From 29c9dc4dc1361769fbd5fc86926e801004efabee Mon Sep 17 00:00:00 2001 From: pedro ernesto Date: Fri, 19 Jun 2026 10:33:36 -0300 Subject: [PATCH 24/32] Wrap write faults in AdbConnectionClosedException AdbWriter.write() is the single funnel for every apacket write, but a failed socket write (okio's write-timeout firing on a wedged adbd, or an EOF/RST mid-write) propagated as a raw IOException. That leaked past push()/install() and the AdbStream sink, which are declared @Throws(AdbException), so the 'no bare IOException leaks' contract did not hold on the write path. The read path was already wrapped in AdbStream.nextMessage. Catch IOException at the write funnel and rethrow as AdbConnectionClosedException so every transport fault speaks one typed vocabulary. AdbWriterTest covers the wedged-push (writeWrite) and read-ack (writeOkay) paths. --- dadb/src/main/kotlin/dadb/AdbWriter.kt | 40 ++++++++------ dadb/src/test/kotlin/dadb/AdbWriterTest.kt | 63 ++++++++++++++++++++++ 2 files changed, 87 insertions(+), 16 deletions(-) create mode 100644 dadb/src/test/kotlin/dadb/AdbWriterTest.kt diff --git a/dadb/src/main/kotlin/dadb/AdbWriter.kt b/dadb/src/main/kotlin/dadb/AdbWriter.kt index ef5dbcf..c5a3b3f 100644 --- a/dadb/src/main/kotlin/dadb/AdbWriter.kt +++ b/dadb/src/main/kotlin/dadb/AdbWriter.kt @@ -19,6 +19,7 @@ package dadb import okio.Sink import okio.buffer +import java.io.IOException import java.nio.ByteBuffer internal class AdbWriter(sink: Sink) : AutoCloseable { @@ -73,24 +74,31 @@ internal class AdbWriter(sink: Sink) : AutoCloseable { length: Int ) { log { "(${Thread.currentThread().name}) > ${AdbMessage(command, arg0, arg1, length, 0, 0, payload ?: ByteArray(0))}" } - synchronized(bufferedSink) { - bufferedSink.apply { - writeIntLe(command) - writeIntLe(arg0) - writeIntLe(arg1) - if (payload == null) { - writeIntLe(0) - writeIntLe(0) - } else { - writeIntLe(length) - writeIntLe(payloadChecksum(payload)) + try { + synchronized(bufferedSink) { + bufferedSink.apply { + writeIntLe(command) + writeIntLe(arg0) + writeIntLe(arg1) + if (payload == null) { + writeIntLe(0) + writeIntLe(0) + } else { + writeIntLe(length) + writeIntLe(payloadChecksum(payload)) + } + writeIntLe(command xor -0x1) + if (payload != null) { + write(payload, offset, length) + } + flush() } - writeIntLe(command xor -0x1) - if (payload != null) { - write(payload, offset, length) - } - flush() } + } catch (e: IOException) { + // A failed socket write means the transport is gone: okio's write-timeout firing on a + // wedged adbd, or an EOF/RST mid-write. Surface it as a typed AdbException here, the single + // funnel for every write, so no raw IOException leaks out of a write. + throw AdbConnectionClosedException("Connection lost while writing to device", e) } } diff --git a/dadb/src/test/kotlin/dadb/AdbWriterTest.kt b/dadb/src/test/kotlin/dadb/AdbWriterTest.kt new file mode 100644 index 0000000..8305d70 --- /dev/null +++ b/dadb/src/test/kotlin/dadb/AdbWriterTest.kt @@ -0,0 +1,63 @@ +/* + * Copyright (c) 2021 mobile.dev inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package dadb + +import com.google.common.truth.Truth.assertThat +import okio.Buffer +import okio.Sink +import okio.Timeout +import java.io.IOException +import java.net.SocketTimeoutException +import kotlin.test.Test +import kotlin.test.assertFailsWith + +internal class AdbWriterTest { + + // A sink that fails every write the way okio's socket write-timeout does on a wedged adbd. + private class ThrowingSink(private val error: IOException) : Sink { + override fun write(source: Buffer, byteCount: Long) { throw error } + override fun flush() {} + override fun timeout(): Timeout = Timeout.NONE + override fun close() {} + } + + @Test + fun payloadWrite_onWriteTimeout_surfacesAsAdbException() { + // The wedged-push path: AdbStream.sink.flush() -> writeWrite(...). Before the fix this leaked + // okio's raw SocketTimeoutException past push()/install(), which are declared @Throws(AdbException). + val cause = SocketTimeoutException("timeout") + val writer = AdbWriter(ThrowingSink(cause)) + + val thrown = assertFailsWith { + writer.writeWrite(localId = 1, remoteId = 2, payload = ByteArray(8), offset = 0, length = 8) + } + assertThat(thrown).isInstanceOf(AdbException::class.java) + assertThat(thrown.cause).isSameInstanceAs(cause) + } + + @Test + fun ackWrite_onTransportFault_surfacesAsAdbException() { + // The read-path ack: AdbStream.source.read() -> writeOkay(...) sits outside nextMessage's + // catch, so it leaked raw too. Same single funnel, same guarantee. + val writer = AdbWriter(ThrowingSink(IOException("broken pipe"))) + + assertFailsWith { + writer.writeOkay(localId = 1, remoteId = 2) + } + } +} From 0a3c6aebc23b852d63883075d7a6572ce15d4133 Mon Sep 17 00:00:00 2001 From: pedro ernesto Date: Fri, 19 Jun 2026 10:51:18 -0300 Subject: [PATCH 25/32] Update WriteTimeoutTest for the AdbException error model A wedged read (SO_TIMEOUT) and a wedged write (okio write-timeout) now surface as AdbConnectionClosedException carrying the SocketTimeoutException as their cause, since the error model wraps every transport fault: the read path via AdbStream.nextMessage, the write path via AdbWriter. Assert the typed exception and the timeout cause instead of a raw SocketTimeoutException. Verified against a booted emulator (full suite: 93 tests, 0 failures). --- dadb/src/test/kotlin/dadb/WriteTimeoutTest.kt | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/dadb/src/test/kotlin/dadb/WriteTimeoutTest.kt b/dadb/src/test/kotlin/dadb/WriteTimeoutTest.kt index 1c10c05..0b90c20 100644 --- a/dadb/src/test/kotlin/dadb/WriteTimeoutTest.kt +++ b/dadb/src/test/kotlin/dadb/WriteTimeoutTest.kt @@ -108,7 +108,7 @@ class WriteTimeoutTest { } @Test - fun `a wedged write fails fast with SocketTimeoutException and the connection recovers`() { + fun `a wedged write fails fast with AdbConnectionClosedException and the connection recovers`() { Relay(targetPort = 5555).use { relay -> // writeTimeoutMillis is the internal test seam; production always uses WRITE_TIMEOUT_MILLIS. val dadb = DadbImpl( @@ -126,9 +126,12 @@ class WriteTimeoutTest { // Wedge: relay stops draining the client side, so a large write stalls. relay.wedgeWrites() val bigCommand = "x".repeat(32 * 1024 * 1024) // overflows the OS socket buffers (incl. larger Linux defaults) - assertThrows { + // The error model surfaces the wedged write as a typed transport fault, carrying the + // underlying okio SocketTimeoutException as its cause (no bare IOException leaks). + val thrown = assertThrows { assertTimeoutPreemptively(Duration.ofSeconds(8)) { dadb.shell(bigCommand) } } + assertThat(thrown.causedBySocketTimeout()).isTrue() // The timed-out write closed the socket; once forwarding resumes, the next op rebuilds. relay.unwedgeWrites() @@ -150,12 +153,20 @@ class WriteTimeoutTest { // Wedge: adbd's responses stop reaching dadb, so the next op's read blocks until // SO_TIMEOUT fires (the connection stays open, so this is a timeout, not an EOF). relay.wedgeReads() - assertThrows { + // SO_TIMEOUT trips the read; the error model surfaces it as a typed transport fault + // carrying the SocketTimeoutException as its cause. + val thrown = assertThrows { assertTimeoutPreemptively(Duration.ofSeconds(8)) { dadb.shell("echo hi") } } + assertThat(thrown.causedBySocketTimeout()).isTrue() } finally { runCatching { dadb.close() } } } } } + +// A wedged read/write is bounded by a SocketTimeoutException deep down; the error model wraps it on +// the way out, so assert on the cause chain rather than the surfaced type carrying it directly. +private fun Throwable.causedBySocketTimeout(): Boolean = + generateSequence(this) { it.cause }.any { it is SocketTimeoutException } From 14e2d844a645011b0d63c3158d3b19fbfa39fd9f Mon Sep 17 00:00:00 2001 From: pedro ernesto Date: Fri, 19 Jun 2026 12:50:05 -0300 Subject: [PATCH 26/32] Add AdbTimeoutException for mid-operation timeouts A timeout is a distinct failure mode from a peer-driven EOF/RST: dadb's own deadline fires because adbd went unresponsive. Add a typed AdbTimeoutException and a causedByTimeout() helper that detects a SocketTimeoutException anywhere in the cause chain (write timeouts arrive raw, read timeouts arrive wrapped via the message queue). --- dadb/src/main/kotlin/dadb/AdbException.kt | 11 +++++++++++ dadb/src/test/kotlin/dadb/AdbExceptionTest.kt | 1 + 2 files changed, 12 insertions(+) diff --git a/dadb/src/main/kotlin/dadb/AdbException.kt b/dadb/src/main/kotlin/dadb/AdbException.kt index 82896e3..563cf1a 100644 --- a/dadb/src/main/kotlin/dadb/AdbException.kt +++ b/dadb/src/main/kotlin/dadb/AdbException.kt @@ -18,6 +18,7 @@ package dadb import java.io.IOException +import java.net.SocketTimeoutException /** * Root of all dadb transport/connection failures. Extends [IOException] because these are genuine, @@ -46,6 +47,16 @@ class AdbStreamOpenException( * but the operation MAY have partially executed; re-check state before retrying non-idempotent work. */ class AdbConnectionClosedException(message: String, cause: Throwable? = null) : AdbException(message, cause) +/** An operation exceeded its deadline with adbd unresponsive: a read past socketTimeout, or a write + * past the write timeout (okio closes the socket on expiry). Distinct from a peer-driven EOF/RST. + * Reconnect; like a dropped connection the operation may have partially executed. */ +class AdbTimeoutException(message: String, cause: Throwable? = null) : AdbException(message, cause) + /** The peer sent a malformed or unexpected apacket (desync). No reliable way to re-sync — the * connection must be closed and re-established. */ class AdbProtocolException(message: String, cause: Throwable? = null) : AdbException(message, cause) + +/** True if a SocketTimeoutException is anywhere in this throwable's cause chain. The write timeout + * arrives raw from okio; the read timeout arrives wrapped through the message queue. */ +internal fun Throwable.causedByTimeout(): Boolean = + generateSequence(this) { it.cause }.any { it is SocketTimeoutException } diff --git a/dadb/src/test/kotlin/dadb/AdbExceptionTest.kt b/dadb/src/test/kotlin/dadb/AdbExceptionTest.kt index 3d568e9..9e824d8 100644 --- a/dadb/src/test/kotlin/dadb/AdbExceptionTest.kt +++ b/dadb/src/test/kotlin/dadb/AdbExceptionTest.kt @@ -30,6 +30,7 @@ internal class AdbExceptionTest { AdbAuthException("x"), AdbStreamOpenException("shell:", "x"), AdbConnectionClosedException("x"), + AdbTimeoutException("x"), AdbProtocolException("x"), ) exceptions.forEach { assertThat(it).isInstanceOf(IOException::class.java) } From a8bceaa7e4e65e013be0a909dae2467f253ccb4b Mon Sep 17 00:00:00 2001 From: pedro ernesto Date: Fri, 19 Jun 2026 12:51:01 -0300 Subject: [PATCH 27/32] Route write timeouts to AdbTimeoutException A wedged write (okio's write timeout on a stalled adbd) is a timeout, not a dropped connection. Split the AdbWriter write funnel: timeouts become AdbTimeoutException, other write faults stay AdbConnectionClosedException. --- dadb/src/main/kotlin/dadb/AdbWriter.kt | 9 +++++---- dadb/src/test/kotlin/dadb/AdbWriterTest.kt | 15 +++++++-------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/dadb/src/main/kotlin/dadb/AdbWriter.kt b/dadb/src/main/kotlin/dadb/AdbWriter.kt index c5a3b3f..6ad6a1d 100644 --- a/dadb/src/main/kotlin/dadb/AdbWriter.kt +++ b/dadb/src/main/kotlin/dadb/AdbWriter.kt @@ -95,10 +95,11 @@ internal class AdbWriter(sink: Sink) : AutoCloseable { } } } catch (e: IOException) { - // A failed socket write means the transport is gone: okio's write-timeout firing on a - // wedged adbd, or an EOF/RST mid-write. Surface it as a typed AdbException here, the single - // funnel for every write, so no raw IOException leaks out of a write. - throw AdbConnectionClosedException("Connection lost while writing to device", e) + // A failed socket write means the transport is gone. A timeout is a stall (adbd stopped + // draining); anything else is a dropped connection. This is the single funnel for every + // write, so no raw IOException leaks out either way. + throw if (e.causedByTimeout()) AdbTimeoutException("Write timed out; device unresponsive", e) + else AdbConnectionClosedException("Connection lost while writing to device", e) } } diff --git a/dadb/src/test/kotlin/dadb/AdbWriterTest.kt b/dadb/src/test/kotlin/dadb/AdbWriterTest.kt index 8305d70..09fc3d4 100644 --- a/dadb/src/test/kotlin/dadb/AdbWriterTest.kt +++ b/dadb/src/test/kotlin/dadb/AdbWriterTest.kt @@ -37,23 +37,22 @@ internal class AdbWriterTest { } @Test - fun payloadWrite_onWriteTimeout_surfacesAsAdbException() { - // The wedged-push path: AdbStream.sink.flush() -> writeWrite(...). Before the fix this leaked - // okio's raw SocketTimeoutException past push()/install(), which are declared @Throws(AdbException). + fun payloadWrite_onWriteTimeout_surfacesAsAdbTimeout() { + // The wedged-push path: AdbStream.sink.flush() -> writeWrite(...). okio's write timeout on a + // stalled adbd is a timeout, not a dropped connection, so it surfaces as AdbTimeoutException. val cause = SocketTimeoutException("timeout") val writer = AdbWriter(ThrowingSink(cause)) - val thrown = assertFailsWith { + val thrown = assertFailsWith { writer.writeWrite(localId = 1, remoteId = 2, payload = ByteArray(8), offset = 0, length = 8) } - assertThat(thrown).isInstanceOf(AdbException::class.java) assertThat(thrown.cause).isSameInstanceAs(cause) } @Test - fun ackWrite_onTransportFault_surfacesAsAdbException() { - // The read-path ack: AdbStream.source.read() -> writeOkay(...) sits outside nextMessage's - // catch, so it leaked raw too. Same single funnel, same guarantee. + fun ackWrite_onNonTimeoutFault_surfacesAsConnectionClosed() { + // A non-timeout write fault (e.g. a broken pipe) is a dropped connection, not a stall, so it + // surfaces as AdbConnectionClosedException. Either way no raw IOException leaves the funnel. val writer = AdbWriter(ThrowingSink(IOException("broken pipe"))) assertFailsWith { From 2ec11bb1efe804b6d1751da783ba4370d89c2f20 Mon Sep 17 00:00:00 2001 From: pedro ernesto Date: Fri, 19 Jun 2026 12:51:50 -0300 Subject: [PATCH 28/32] Route read timeouts (open + stream) to AdbTimeoutException A read past socketTimeout while opening a stream or mid-stream is a stall, not a dropped connection. Route it to AdbTimeoutException at both read boundaries, keeping non-timeout faults as AdbConnectionClosedException and a peer A_CLSE as a clean end. --- dadb/src/main/kotlin/dadb/AdbConnection.kt | 5 +++-- dadb/src/main/kotlin/dadb/AdbStream.kt | 6 ++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/dadb/src/main/kotlin/dadb/AdbConnection.kt b/dadb/src/main/kotlin/dadb/AdbConnection.kt index 967c6de..51eb369 100644 --- a/dadb/src/main/kotlin/dadb/AdbConnection.kt +++ b/dadb/src/main/kotlin/dadb/AdbConnection.kt @@ -58,9 +58,10 @@ internal class AdbConnection internal constructor( messageQueue.stopListening(localId) throw e } catch (e: IOException) { - // Raw socket fault (EOF/RST) while opening: the connection died mid-handshake. + // Raw socket fault while opening: a timeout (adbd unresponsive) vs the connection dying. messageQueue.stopListening(localId) - throw AdbConnectionClosedException("Connection lost while opening stream: $destination", e) + throw if (e.causedByTimeout()) AdbTimeoutException("Timed out opening stream: $destination", e) + else AdbConnectionClosedException("Connection lost while opening stream: $destination", e) } catch (e: Throwable) { messageQueue.stopListening(localId) throw e diff --git a/dadb/src/main/kotlin/dadb/AdbStream.kt b/dadb/src/main/kotlin/dadb/AdbStream.kt index a75edb3..9f8f3f9 100644 --- a/dadb/src/main/kotlin/dadb/AdbStream.kt +++ b/dadb/src/main/kotlin/dadb/AdbStream.kt @@ -122,9 +122,11 @@ internal class AdbStreamImpl internal constructor( close() null } catch (e: IOException) { - // Real socket fault (EOF/RST) mid-stream: the transport died, not a clean close. + // Mid-stream fault: a timeout (adbd unresponsive) vs the transport dying. Either way the + // stream is done. close() - throw AdbConnectionClosedException("Connection lost while reading stream $localId", e) + throw if (e.causedByTimeout()) AdbTimeoutException("Read timed out on stream $localId; device unresponsive", e) + else AdbConnectionClosedException("Connection lost while reading stream $localId", e) } } From b3ef7ae31e4fb2f9782045ee56f100424dfe9132 Mon Sep 17 00:00:00 2001 From: pedro ernesto Date: Fri, 19 Jun 2026 14:10:35 -0300 Subject: [PATCH 29/32] Expect AdbTimeoutException in WriteTimeoutTest Both the wedged-write and wedged-read cases now assert the typed timeout. The type itself encodes that it was a timeout, so the cause-chain helper is no longer needed. --- dadb/src/test/kotlin/dadb/WriteTimeoutTest.kt | 21 ++++++------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/dadb/src/test/kotlin/dadb/WriteTimeoutTest.kt b/dadb/src/test/kotlin/dadb/WriteTimeoutTest.kt index 0b90c20..a4ed24e 100644 --- a/dadb/src/test/kotlin/dadb/WriteTimeoutTest.kt +++ b/dadb/src/test/kotlin/dadb/WriteTimeoutTest.kt @@ -25,7 +25,6 @@ import java.io.Closeable import java.net.InetSocketAddress import java.net.ServerSocket import java.net.Socket -import java.net.SocketTimeoutException import java.time.Duration import java.util.Collections import kotlin.concurrent.thread @@ -108,7 +107,7 @@ class WriteTimeoutTest { } @Test - fun `a wedged write fails fast with AdbConnectionClosedException and the connection recovers`() { + fun `a wedged write fails fast with AdbTimeoutException and the connection recovers`() { Relay(targetPort = 5555).use { relay -> // writeTimeoutMillis is the internal test seam; production always uses WRITE_TIMEOUT_MILLIS. val dadb = DadbImpl( @@ -126,12 +125,11 @@ class WriteTimeoutTest { // Wedge: relay stops draining the client side, so a large write stalls. relay.wedgeWrites() val bigCommand = "x".repeat(32 * 1024 * 1024) // overflows the OS socket buffers (incl. larger Linux defaults) - // The error model surfaces the wedged write as a typed transport fault, carrying the - // underlying okio SocketTimeoutException as its cause (no bare IOException leaks). - val thrown = assertThrows { + // okio's write timeout fires on the stalled write and surfaces as a typed timeout, + // not a raw SocketTimeoutException. + assertThrows { assertTimeoutPreemptively(Duration.ofSeconds(8)) { dadb.shell(bigCommand) } } - assertThat(thrown.causedBySocketTimeout()).isTrue() // The timed-out write closed the socket; once forwarding resumes, the next op rebuilds. relay.unwedgeWrites() @@ -153,20 +151,13 @@ class WriteTimeoutTest { // Wedge: adbd's responses stop reaching dadb, so the next op's read blocks until // SO_TIMEOUT fires (the connection stays open, so this is a timeout, not an EOF). relay.wedgeReads() - // SO_TIMEOUT trips the read; the error model surfaces it as a typed transport fault - // carrying the SocketTimeoutException as its cause. - val thrown = assertThrows { + // SO_TIMEOUT trips the read and surfaces as a typed timeout. + assertThrows { assertTimeoutPreemptively(Duration.ofSeconds(8)) { dadb.shell("echo hi") } } - assertThat(thrown.causedBySocketTimeout()).isTrue() } finally { runCatching { dadb.close() } } } } } - -// A wedged read/write is bounded by a SocketTimeoutException deep down; the error model wraps it on -// the way out, so assert on the cause chain rather than the surfaced type carrying it directly. -private fun Throwable.causedBySocketTimeout(): Boolean = - generateSequence(this) { it.cause }.any { it is SocketTimeoutException } From 49dde78ed890dc53bfdc8e726f39efae1c81d644 Mon Sep 17 00:00:00 2001 From: pedro ernesto Date: Fri, 19 Jun 2026 14:20:25 -0300 Subject: [PATCH 30/32] Address review: accurate causedByTimeout comment + read-timeout unit test The helper comment claimed the read timeout arrives wrapped through the message queue; it actually propagates raw (the chain walk still covers either case). Add AdbStreamTest.readTimeoutThrowsAdbTimeout to pin the timeout branch at the stream read boundary as a fast unit test (the non-timeout and clean-EOF branches were already covered). --- dadb/src/main/kotlin/dadb/AdbException.kt | 5 +++-- dadb/src/test/kotlin/dadb/AdbStreamTest.kt | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/dadb/src/main/kotlin/dadb/AdbException.kt b/dadb/src/main/kotlin/dadb/AdbException.kt index 563cf1a..47ba965 100644 --- a/dadb/src/main/kotlin/dadb/AdbException.kt +++ b/dadb/src/main/kotlin/dadb/AdbException.kt @@ -56,7 +56,8 @@ class AdbTimeoutException(message: String, cause: Throwable? = null) : AdbExcept * connection must be closed and re-established. */ class AdbProtocolException(message: String, cause: Throwable? = null) : AdbException(message, cause) -/** True if a SocketTimeoutException is anywhere in this throwable's cause chain. The write timeout - * arrives raw from okio; the read timeout arrives wrapped through the message queue. */ +/** True if a SocketTimeoutException is anywhere in this throwable's cause chain. Both the write + * timeout (okio's socket sink) and the read timeout (SO_TIMEOUT) arrive as a SocketTimeoutException; + * the chain walk keeps the check robust if a layer ever wraps one on the way out. */ internal fun Throwable.causedByTimeout(): Boolean = generateSequence(this) { it.cause }.any { it is SocketTimeoutException } diff --git a/dadb/src/test/kotlin/dadb/AdbStreamTest.kt b/dadb/src/test/kotlin/dadb/AdbStreamTest.kt index 518e55f..16ed778 100644 --- a/dadb/src/test/kotlin/dadb/AdbStreamTest.kt +++ b/dadb/src/test/kotlin/dadb/AdbStreamTest.kt @@ -19,6 +19,9 @@ package dadb import com.google.common.truth.Truth import okio.Buffer +import okio.Source +import okio.Timeout +import java.net.SocketTimeoutException import kotlin.test.Test import kotlin.test.assertFailsWith @@ -57,6 +60,22 @@ internal class AdbStreamTest { assertFailsWith { stream.source.readByteArray() } } + @Test + fun readTimeoutThrowsAdbTimeout() { + // Reader whose socket read trips SO_TIMEOUT instead of returning data or EOF: a stall, which + // surfaces as AdbTimeoutException rather than AdbConnectionClosedException. + val timingOutReader = AdbReader(object : Source { + override fun read(sink: Buffer, byteCount: Long): Long = throw SocketTimeoutException("Read timed out") + override fun timeout(): Timeout = Timeout.NONE + override fun close() {} + }) + val messageQueue = AdbMessageQueue(timingOutReader) + messageQueue.startListening(1) + val stream = AdbStreamImpl(messageQueue, AdbWriter(Buffer()), 1024, 1, 2) + + assertFailsWith { stream.source.readByteArray() } + } + private fun createAdbReader(localId: Int, remoteId: Int, writePayload: ByteArray): AdbReader { val source = Buffer() val writer = AdbWriter(source) From c84a010bf783eef6718f95659d59c30bfb1ed996 Mon Sep 17 00:00:00 2001 From: pedro ernesto Date: Fri, 19 Jun 2026 14:26:43 -0300 Subject: [PATCH 31/32] Simplify timeout detection to a direct is-check The timeout arrives raw at all three catch sites (okio throws SocketTimeoutException directly on writes, SO_TIMEOUT throws it directly on reads, the message queue does not wrap it), so the cause-chain helper guarded against wrapping that does not exist. Inline 'e is SocketTimeoutException' at AdbWriter, AdbConnection.open, and AdbStream.nextMessage, and drop the causedByTimeout helper. --- dadb/src/main/kotlin/dadb/AdbConnection.kt | 3 ++- dadb/src/main/kotlin/dadb/AdbException.kt | 7 ------- dadb/src/main/kotlin/dadb/AdbStream.kt | 3 ++- dadb/src/main/kotlin/dadb/AdbWriter.kt | 3 ++- 4 files changed, 6 insertions(+), 10 deletions(-) diff --git a/dadb/src/main/kotlin/dadb/AdbConnection.kt b/dadb/src/main/kotlin/dadb/AdbConnection.kt index 51eb369..d71eaab 100644 --- a/dadb/src/main/kotlin/dadb/AdbConnection.kt +++ b/dadb/src/main/kotlin/dadb/AdbConnection.kt @@ -25,6 +25,7 @@ import org.jetbrains.annotations.TestOnly import java.io.Closeable import java.io.IOException import java.net.Socket +import java.net.SocketTimeoutException import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicInteger @@ -60,7 +61,7 @@ internal class AdbConnection internal constructor( } catch (e: IOException) { // Raw socket fault while opening: a timeout (adbd unresponsive) vs the connection dying. messageQueue.stopListening(localId) - throw if (e.causedByTimeout()) AdbTimeoutException("Timed out opening stream: $destination", e) + throw if (e is SocketTimeoutException) AdbTimeoutException("Timed out opening stream: $destination", e) else AdbConnectionClosedException("Connection lost while opening stream: $destination", e) } catch (e: Throwable) { messageQueue.stopListening(localId) diff --git a/dadb/src/main/kotlin/dadb/AdbException.kt b/dadb/src/main/kotlin/dadb/AdbException.kt index 47ba965..2f3d957 100644 --- a/dadb/src/main/kotlin/dadb/AdbException.kt +++ b/dadb/src/main/kotlin/dadb/AdbException.kt @@ -18,7 +18,6 @@ package dadb import java.io.IOException -import java.net.SocketTimeoutException /** * Root of all dadb transport/connection failures. Extends [IOException] because these are genuine, @@ -55,9 +54,3 @@ class AdbTimeoutException(message: String, cause: Throwable? = null) : AdbExcept /** The peer sent a malformed or unexpected apacket (desync). No reliable way to re-sync — the * connection must be closed and re-established. */ class AdbProtocolException(message: String, cause: Throwable? = null) : AdbException(message, cause) - -/** True if a SocketTimeoutException is anywhere in this throwable's cause chain. Both the write - * timeout (okio's socket sink) and the read timeout (SO_TIMEOUT) arrive as a SocketTimeoutException; - * the chain walk keeps the check robust if a layer ever wraps one on the way out. */ -internal fun Throwable.causedByTimeout(): Boolean = - generateSequence(this) { it.cause }.any { it is SocketTimeoutException } diff --git a/dadb/src/main/kotlin/dadb/AdbStream.kt b/dadb/src/main/kotlin/dadb/AdbStream.kt index 9f8f3f9..ff3277e 100644 --- a/dadb/src/main/kotlin/dadb/AdbStream.kt +++ b/dadb/src/main/kotlin/dadb/AdbStream.kt @@ -19,6 +19,7 @@ package dadb import okio.* import java.lang.Integer.min +import java.net.SocketTimeoutException import java.nio.ByteBuffer interface AdbStream : AutoCloseable { @@ -125,7 +126,7 @@ internal class AdbStreamImpl internal constructor( // Mid-stream fault: a timeout (adbd unresponsive) vs the transport dying. Either way the // stream is done. close() - throw if (e.causedByTimeout()) AdbTimeoutException("Read timed out on stream $localId; device unresponsive", e) + throw if (e is SocketTimeoutException) AdbTimeoutException("Read timed out on stream $localId; device unresponsive", e) else AdbConnectionClosedException("Connection lost while reading stream $localId", e) } } diff --git a/dadb/src/main/kotlin/dadb/AdbWriter.kt b/dadb/src/main/kotlin/dadb/AdbWriter.kt index 6ad6a1d..830e5df 100644 --- a/dadb/src/main/kotlin/dadb/AdbWriter.kt +++ b/dadb/src/main/kotlin/dadb/AdbWriter.kt @@ -20,6 +20,7 @@ package dadb import okio.Sink import okio.buffer import java.io.IOException +import java.net.SocketTimeoutException import java.nio.ByteBuffer internal class AdbWriter(sink: Sink) : AutoCloseable { @@ -98,7 +99,7 @@ internal class AdbWriter(sink: Sink) : AutoCloseable { // A failed socket write means the transport is gone. A timeout is a stall (adbd stopped // draining); anything else is a dropped connection. This is the single funnel for every // write, so no raw IOException leaks out either way. - throw if (e.causedByTimeout()) AdbTimeoutException("Write timed out; device unresponsive", e) + throw if (e is SocketTimeoutException) AdbTimeoutException("Write timed out; device unresponsive", e) else AdbConnectionClosedException("Connection lost while writing to device", e) } } From f780773e2d93de62c628ee5cf754a9327e27e471 Mon Sep 17 00:00:00 2001 From: pedro ernesto Date: Fri, 19 Jun 2026 14:59:07 -0300 Subject: [PATCH 32/32] Wrap TCP connect failures in AdbConnectException; document AdbTimeoutException DadbImpl.newConnection let a raw java.net.ConnectException / SocketTimeoutException escape socket.connect() past the @Throws(AdbException) public API on the common device-offline / refused / connect-timeout case. Wrap it as AdbConnectException, per that type's contract (TCP connect/timeout, nothing established). Add a refused-port test. Also list AdbTimeoutException in the CHANGELOG 2.0.0 entry and note the timeout behavior, which the entry (written before that type existed) had omitted. --- CHANGELOG.md | 3 ++- dadb/src/main/kotlin/dadb/DadbImpl.kt | 9 ++++++++- dadb/src/test/kotlin/dadb/AdbExceptionTest.kt | 13 +++++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1824308..5822684 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,10 @@ ### 2.0.0 -* **Breaking:** redesigned error model. Transport failures now throw a typed `AdbException` hierarchy (`AdbConnectException`, `AdbAuthException`, `AdbStreamOpenException`, `AdbConnectionClosedException`, `AdbProtocolException`), all extending `IOException`. +* **Breaking:** redesigned error model. Transport failures now throw a typed `AdbException` hierarchy (`AdbConnectException`, `AdbAuthException`, `AdbStreamOpenException`, `AdbConnectionClosedException`, `AdbTimeoutException`, `AdbProtocolException`), all extending `IOException`. * **Breaking:** operation outcomes are now returned instead of thrown: `install`/`installMultiple` return `InstallResult`, `uninstall` returns `UninstallResult`, `push`/`pull` return `SyncResult`, `root`/`unroot` return `RootResult`. * **Breaking:** a connection dropped mid-stream now throws `AdbConnectionClosedException` instead of presenting as a clean end-of-stream. +* **Breaking:** a read or write that exceeds its timeout now throws `AdbTimeoutException` instead of a raw `SocketTimeoutException`. * `pmInstall` (the non-`cmd` install path) now checks the `pm install` result instead of ignoring it. * Added opt-in result helpers — `onSuccess`/`onFailure`/`orThrow` on each `…Result` type. `orThrow` raises `AdbOperationFailedException`, which is **not** an `AdbException`, so a transport-level `catch (AdbException)` won't catch an opted-in operation-failure throw. diff --git a/dadb/src/main/kotlin/dadb/DadbImpl.kt b/dadb/src/main/kotlin/dadb/DadbImpl.kt index 83857c8..419eba8 100644 --- a/dadb/src/main/kotlin/dadb/DadbImpl.kt +++ b/dadb/src/main/kotlin/dadb/DadbImpl.kt @@ -18,6 +18,7 @@ package dadb import org.jetbrains.annotations.TestOnly +import java.io.IOException import java.net.InetSocketAddress import java.net.Socket import kotlin.jvm.Throws @@ -87,7 +88,13 @@ internal class DadbImpl @Throws(IllegalArgumentException::class) constructor( val socketAddress = InetSocketAddress(host, port) val socket = Socket() socket.soTimeout = socketTimeout - socket.connect(socketAddress, connectTimeout) + try { + socket.connect(socketAddress, connectTimeout) + } catch (e: IOException) { + // A failed TCP connect (refused / unreachable / connect timeout) means nothing was + // established. Surface it as a typed AdbException, not a raw java.net exception. + throw AdbConnectException("Failed to connect to $host:$port", e) + } if (keepAlive) { socket.keepAlive = true } diff --git a/dadb/src/test/kotlin/dadb/AdbExceptionTest.kt b/dadb/src/test/kotlin/dadb/AdbExceptionTest.kt index 9e824d8..ddd546a 100644 --- a/dadb/src/test/kotlin/dadb/AdbExceptionTest.kt +++ b/dadb/src/test/kotlin/dadb/AdbExceptionTest.kt @@ -19,7 +19,9 @@ package dadb import com.google.common.truth.Truth.assertThat import java.io.IOException +import java.net.ServerSocket import kotlin.test.Test +import kotlin.test.assertFailsWith internal class AdbExceptionTest { @@ -47,4 +49,15 @@ internal class AdbExceptionTest { val cause = IOException("socket reset") assertThat(AdbConnectException("x", cause).cause).isSameInstanceAs(cause) } + + @Test + fun refusedConnectThrowsAdbConnectException() { + // Bind an ephemeral port then release it: nothing is listening, so the TCP connect is refused. + val closedPort = ServerSocket(0).use { it.localPort } + val dadb = DadbImpl(host = "localhost", port = closedPort, connectTimeout = 1000, socketTimeout = 1000) + + // A refused connect must surface as the typed AdbConnectException, not a raw java.net exception. + val thrown = assertFailsWith { dadb.shell("echo hi") } + assertThat(thrown.cause).isNotNull() + } }