diff --git a/app/app/build.gradle.kts b/app/app/build.gradle.kts index 0554d24..943f8e3 100644 --- a/app/app/build.gradle.kts +++ b/app/app/build.gradle.kts @@ -21,6 +21,24 @@ val keystoreProperties = Properties().apply { } val hasReleaseKeystore = keystorePropertiesFile.exists() +// Google Books API key. Lives in `local.properties` (gitignored) as +// GOOGLE_BOOKS_API_KEY=..., or in the environment for CI. Absent is a supported +// state: the build stays green and GoogleBooksClient falls back to the keyless +// endpoint, which is what a fresh clone without the key gets. +// The key is NOT a secret in the usual sense — it ships inside the APK and can be +// extracted — but it is restricted to the Books API, and it must never be +// committed. See docs/METADATA-SOURCES.md. +val localPropertiesFile = rootProject.file("local.properties") +val localProperties = Properties().apply { + if (localPropertiesFile.exists()) { + localPropertiesFile.inputStream().use { load(it) } + } +} +val googleBooksApiKey: String = + (localProperties["GOOGLE_BOOKS_API_KEY"] as String?) + ?: System.getenv("GOOGLE_BOOKS_API_KEY") + ?: "" + android { namespace = "org.modg.bookshelf" compileSdk = 37 @@ -34,6 +52,8 @@ android { versionName = "1.0" testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" + + buildConfigField("String", "GOOGLE_BOOKS_API_KEY", "\"$googleBooksApiKey\"") } signingConfigs { @@ -64,6 +84,7 @@ android { buildFeatures { compose = true + buildConfig = true } testOptions { diff --git a/app/app/src/main/java/org/modg/bookshelf/AppContainer.kt b/app/app/src/main/java/org/modg/bookshelf/AppContainer.kt index 0f8517f..c9505a0 100644 --- a/app/app/src/main/java/org/modg/bookshelf/AppContainer.kt +++ b/app/app/src/main/java/org/modg/bookshelf/AppContainer.kt @@ -114,7 +114,7 @@ class AppContainer(private val context: Context) { .build() } - val metadataRepository by lazy { MetadataRepository(metadataHttpClient, json) } + val metadataRepository by lazy { MetadataRepository(metadataHttpClient, json, BuildConfig.GOOGLE_BOOKS_API_KEY) } val syncEngine by lazy { SyncEngine( diff --git a/app/app/src/main/java/org/modg/bookshelf/data/metadata/GoogleBooksClient.kt b/app/app/src/main/java/org/modg/bookshelf/data/metadata/GoogleBooksClient.kt index 50182b3..cca949c 100644 --- a/app/app/src/main/java/org/modg/bookshelf/data/metadata/GoogleBooksClient.kt +++ b/app/app/src/main/java/org/modg/bookshelf/data/metadata/GoogleBooksClient.kt @@ -5,52 +5,83 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import kotlinx.serialization.SerializationException import kotlinx.serialization.json.Json +import okhttp3.HttpUrl import okhttp3.OkHttpClient import okhttp3.Request /** - * Google Books lookup — SPEC.md "Book metadata lookup" fallback source. No API key. + * Google Books lookup — SPEC.md "Book metadata lookup" fallback source. Takes an + * [apiKey] (see `AppContainer` / `BuildConfig.GOOGLE_BOOKS_API_KEY`) because a + * keyless request is permanently rate-limited: every anonymous caller on the + * internet shares one exhausted daily quota (docs/METADATA-SOURCES.md). A blank + * key (the default) sends the request keyless, unchanged from before — this + * matters for a fresh clone with no key configured, which must still build a + * working app rather than a broken one. * Never throws: every outcome, including transport failure, comes back as a * [SourceResult] rather than a swallowed null. */ class GoogleBooksClient( private val httpClient: OkHttpClient, json: Json, + private val apiKey: String = "", ) { private val json = Json(from = json) { ignoreUnknownKeys = true } /** - * Retries transient failures per [RetryPolicy]. Note this source's standing - * failure — keyless requests share one exhausted global quota and answer 429, - * which [RetryPolicy] deliberately does NOT retry, so today this costs nothing - * and changes nothing here. See docs/METADATA-SOURCES.md. + * Retries transient failures per [RetryPolicy]. A keyless 429 is the exhausted + * global daily quota, which [RetryPolicy] deliberately does NOT retry — see its + * KDoc. A keyed 429 is the much shorter per-user rate limit and is worth one + * more ask, so [retryRateLimitedOnce] follows whether a key is configured. */ suspend fun lookup(isbn13: String): SourceResult = - withContext(Dispatchers.IO) { withRetry { fetch(isbn13) } } + withContext(Dispatchers.IO) { + withRetry(retryRateLimitedOnce = apiKey.isNotBlank()) { fetch(isbn13) } + } /** Single un-retried attempt, for tests that need to count calls. */ internal suspend fun lookupOnce(isbn13: String): SourceResult = withContext(Dispatchers.IO) { fetch(isbn13) } - private fun fetch(isbn13: String): SourceResult = try { - val request = Request.Builder() - .url("https://www.googleapis.com/books/v1/volumes?q=isbn:$isbn13") - .build() - httpClient.newCall(request).execute().use { response -> - classify(response.code, response.body.string()) + private fun fetch(isbn13: String): SourceResult = redact( + try { + val request = Request.Builder().url(requestUrl(isbn13)).build() + httpClient.newCall(request).execute().use { response -> + classify(response.code, response.body.string(), response.header("Retry-After")) + } + } catch (e: IOException) { + SourceResult.fromException(e) + }, + ) + + /** + * Package-visible pure function — no socket involved — so it's exhaustively + * unit-testable offline: the key is appended (URL-encoded) only when non-blank, + * and omitted entirely for the keyless default. Built with [HttpUrl.Builder] + * rather than string concatenation so query-parameter encoding is correct by + * construction rather than by hand. + */ + internal fun requestUrl(isbn13: String): String { + val builder = HttpUrl.Builder() + .scheme("https") + .host("www.googleapis.com") + .addPathSegments("books/v1/volumes") + .addQueryParameter("q", "isbn:$isbn13") + if (apiKey.isNotBlank()) { + builder.addQueryParameter("key", apiKey) } - } catch (e: IOException) { - SourceResult.fromException(e) + return builder.build().toString() } /** * Package-visible pure function — no socket involved — so it's exhaustively * unit-testable offline (2xx-with-record, 2xx-without-record, 404, 429, 500, * malformed body). [parseResponse] is defined in terms of this so the two - * can never disagree about what a body means. + * can never disagree about what a body means. [retryAfterHeader] is the raw + * `Retry-After` header value, if any — threaded through so the retry loop can + * see it, but only 429 ever consults it (via [SourceResult.fromHttpCode]). */ - internal fun classify(httpCode: Int, body: String?): SourceResult { - if (httpCode !in 200..299) return SourceResult.fromHttpCode(httpCode) + internal fun classify(httpCode: Int, body: String?, retryAfterHeader: String? = null): SourceResult { + if (httpCode !in 200..299) return SourceResult.fromHttpCode(httpCode, retryAfterHeader) if (body.isNullOrBlank()) return SourceResult.Failed("empty body", FailureKind.MALFORMED) return try { val dto = json.decodeFromString(GoogleBooksResponseDto.serializer(), body) @@ -66,4 +97,20 @@ class GoogleBooksClient( /** Package-visible for offline fixture tests — parses a raw response body with no network involved. */ internal fun parseResponse(body: String): BookMetadata? = (classify(200, body) as? SourceResult.Found)?.metadata + + /** + * [SourceResult.Failed.reason] is rendered on the scan sheet — our only + * diagnostic channel from a real phone (docs/METADATA-SOURCES.md) — so it must + * never carry the API key. Some okhttp/JDK IOExceptions embed the full request + * URL, key included, in their own message (`UnknownHostException`, SSL errors, + * and okhttp's own "Canceled" IOException variants differ by platform), so + * this scrubs defensively rather than trusting that no exception type ever + * will. Package-visible so the scrub itself is unit-testable without a socket. + */ + internal fun redact(result: SourceResult): SourceResult = + if (result is SourceResult.Failed && apiKey.isNotBlank() && result.reason.contains(apiKey)) { + result.copy(reason = result.reason.replace(apiKey, "[REDACTED]")) + } else { + result + } } diff --git a/app/app/src/main/java/org/modg/bookshelf/data/metadata/MetadataRepository.kt b/app/app/src/main/java/org/modg/bookshelf/data/metadata/MetadataRepository.kt index 82680cf..b83b2f3 100644 --- a/app/app/src/main/java/org/modg/bookshelf/data/metadata/MetadataRepository.kt +++ b/app/app/src/main/java/org/modg/bookshelf/data/metadata/MetadataRepository.kt @@ -16,9 +16,9 @@ class MetadataRepository( private val openLibraryClient: OpenLibraryClient, private val googleBooksClient: GoogleBooksClient, ) { - constructor(httpClient: OkHttpClient, json: Json) : this( + constructor(httpClient: OkHttpClient, json: Json, googleBooksApiKey: String = "") : this( OpenLibraryClient(httpClient, json), - GoogleBooksClient(httpClient, json), + GoogleBooksClient(httpClient, json, googleBooksApiKey), ) suspend fun lookup(isbn: String): LookupResult { diff --git a/app/app/src/main/java/org/modg/bookshelf/data/metadata/RetryPolicy.kt b/app/app/src/main/java/org/modg/bookshelf/data/metadata/RetryPolicy.kt index 372daf1..66bcae4 100644 --- a/app/app/src/main/java/org/modg/bookshelf/data/metadata/RetryPolicy.kt +++ b/app/app/src/main/java/org/modg/bookshelf/data/metadata/RetryPolicy.kt @@ -27,6 +27,15 @@ object RetryPolicy { /** One original attempt plus two retries. Beyond this the marginal gain is noise. */ const val MAX_ATTEMPTS = 3 + /** + * Cap on how long we'll wait on a keyed 429's `Retry-After` before treating it + * as not worth honouring, and also the fixed backoff used when that header is + * absent or unparseable. The user is standing at a bookshelf: a per-user rate + * limit clears fast, so there's no reason to wait longer than this for the one + * retry [withRetry] grants it (see [isRetryable]'s KDoc on RATE_LIMITED). + */ + const val RATE_LIMIT_RETRY_CAP_MILLIS = 2_000L + /** * Stop starting NEW attempts once this much time has gone into a single source. * A backstop against pathological cases (every attempt hitting the slow tail), @@ -41,19 +50,37 @@ object RetryPolicy { * [FailureKind.TRANSPORT] and [FailureKind.SERVER_ERROR] are transient and * cheap to re-ask. The rest are not: * - TIMEOUT — the budget is already spent; see the class KDoc. - * - RATE_LIMITED — the source is explicitly asking us to stop. Hammering a - * quota is how an intermittent block becomes a permanent one, - * and METADATA-SOURCES.md records that happening to this - * project's IP during research. When the Google Books API key - * lands, revisit this: a keyed 429 is a per-second rate limit - * and IS worth one Retry-After-respecting retry, unlike - * today's keyless daily-quota 429, which never clears. + * - RATE_LIMITED — the source is explicitly asking us to stop, and NOT + * retryable here either: a keyless 429 is the exhausted + * shared daily quota (METADATA-SOURCES.md), which never + * clears within a session, so hammering it is pure waste and + * risks turning an intermittent block into a permanent one. + * A KEYED 429 is a different animal — the much shorter + * per-user rate limit — and IS worth one retry, but that's a + * call only [GoogleBooksClient] can make (it knows whether a + * key is configured), so it opts in per-call via + * [withRetry]'s `retryRateLimitedOnce` rather than by + * changing this blanket answer. * - CLIENT_ERROR — an identical request gets an identical answer. * - MALFORMED — same bytes, same parse failure. */ fun isRetryable(kind: FailureKind): Boolean = kind == FailureKind.TRANSPORT || kind == FailureKind.SERVER_ERROR + /** + * Parses an HTTP `Retry-After` header (the plain delta-seconds form; Google + * Books does not send the HTTP-date form) into a wait capped at + * [RATE_LIMIT_RETRY_CAP_MILLIS]. Returns null — "don't honour this" — if the + * header is missing, blank, negative, or not a plain integer; [withRetry] + * falls back to [RATE_LIMIT_RETRY_CAP_MILLIS] itself in that case, so either + * way the wait stays short and fixed rather than whatever the server asked for. + */ + fun retryAfterMillis(header: String?): Long? { + val seconds = header?.trim()?.toLongOrNull() ?: return null + if (seconds < 0) return null + return (seconds * 1000).coerceAtMost(RATE_LIMIT_RETRY_CAP_MILLIS) + } + /** * Backoff before attempt number [nextAttempt] (2-based: the wait before the * first retry is `backoffMillis(2)`). 250ms then 750ms, plus up to 40% jitter @@ -82,10 +109,21 @@ object RetryPolicy { * * [sleep] and [nowMillis] are injectable purely so tests can run the real policy * with no wall-clock delay; production callers use the defaults. + * + * [retryRateLimitedOnce] permits exactly one extra retry of a + * [FailureKind.RATE_LIMITED] failure, on top of whatever [RetryPolicy.isRetryable] + * already grants — see its KDoc. "Once" is enforced independently of + * [maxAttempts]: a second consecutive rate-limited failure always ends the loop, + * even if attempts remain in the budget. The wait before that one retry honours + * [SourceResult.Failed.retryAfterMillis] when the failure carries one, and + * [RetryPolicy.RATE_LIMIT_RETRY_CAP_MILLIS] otherwise — never the normal + * exponential [RetryPolicy.backoffMillis], which is tuned for transient transport + * errors, not for a server explicitly asking us to wait. */ suspend fun withRetry( maxAttempts: Int = RetryPolicy.MAX_ATTEMPTS, budgetMillis: Long = RetryPolicy.TOTAL_BUDGET_MILLIS, + retryRateLimitedOnce: Boolean = false, random: Random = Random.Default, nowMillis: () -> Long = { System.currentTimeMillis() }, sleep: suspend (Long) -> Unit = { delay(it) }, @@ -94,13 +132,21 @@ suspend fun withRetry( val started = nowMillis() var last: SourceResult = attempt() var attemptsMade = 1 + var rateLimitedRetryUsed = false while (attemptsMade < maxAttempts) { val failure = last as? SourceResult.Failed ?: return last - if (!RetryPolicy.isRetryable(failure.kind)) break + val rateLimitedRetry = failure.kind == FailureKind.RATE_LIMITED && + retryRateLimitedOnce && !rateLimitedRetryUsed + if (!RetryPolicy.isRetryable(failure.kind) && !rateLimitedRetry) break if (nowMillis() - started >= budgetMillis) break - sleep(RetryPolicy.backoffMillis(attemptsMade + 1, random)) + if (rateLimitedRetry) { + rateLimitedRetryUsed = true + sleep(failure.retryAfterMillis ?: RetryPolicy.RATE_LIMIT_RETRY_CAP_MILLIS) + } else { + sleep(RetryPolicy.backoffMillis(attemptsMade + 1, random)) + } last = attempt() attemptsMade++ } diff --git a/app/app/src/main/java/org/modg/bookshelf/data/metadata/SourceResult.kt b/app/app/src/main/java/org/modg/bookshelf/data/metadata/SourceResult.kt index 88bb71c..9327cf1 100644 --- a/app/app/src/main/java/org/modg/bookshelf/data/metadata/SourceResult.kt +++ b/app/app/src/main/java/org/modg/bookshelf/data/metadata/SourceResult.kt @@ -53,9 +53,16 @@ sealed interface SourceResult { * to the user as supplementary detail on the scan sheet and is our ONLY * diagnostic channel from a real phone — so it names the specific failure, not * a generic one. [kind] is the same fact in a form [RetryPolicy] can act on; - * nothing should ever parse [reason] to recover it. + * nothing should ever parse [reason] to recover it. [retryAfterMillis] is set + * only for a [FailureKind.RATE_LIMITED] failure whose response carried a usable + * `Retry-After` header (already parsed and capped — see [RetryPolicy]); a + * retryable failure without one falls back to a fixed backoff instead. */ - data class Failed(val reason: String, val kind: FailureKind = FailureKind.TRANSPORT) : SourceResult + data class Failed( + val reason: String, + val kind: FailureKind = FailureKind.TRANSPORT, + val retryAfterMillis: Long? = null, + ) : SourceResult companion object { /** @@ -89,11 +96,19 @@ sealed interface SourceResult { /** * Maps a non-2xx HTTP status to a specific failure. 429 is called out * separately from the rest of 4xx because it is the one client error that is - * about us rather than about the request, and because it is currently - * Google Books' permanent state — see docs/METADATA-SOURCES.md. + * about us rather than about the request, and because it is Google Books' + * permanent keyless state — see docs/METADATA-SOURCES.md. [retryAfterHeader] + * is the raw `Retry-After` header value, if any; only a 429 ever uses it, via + * [RetryPolicy.retryAfterMillis]. Passing it for another code is harmless + * (it's simply not consulted) — the parameter isn't restricted to 429 so + * callers don't need to know which codes care. */ - fun fromHttpCode(code: Int): Failed = when { - code == 429 -> Failed("http 429 (rate limited)", FailureKind.RATE_LIMITED) + fun fromHttpCode(code: Int, retryAfterHeader: String? = null): Failed = when { + code == 429 -> Failed( + "http 429 (rate limited)", + FailureKind.RATE_LIMITED, + retryAfterMillis = RetryPolicy.retryAfterMillis(retryAfterHeader), + ) code in 500..599 -> Failed("http $code (server error)", FailureKind.SERVER_ERROR) else -> Failed("http $code", FailureKind.CLIENT_ERROR) } diff --git a/app/app/src/test/java/org/modg/bookshelf/data/metadata/GoogleBooksClientTest.kt b/app/app/src/test/java/org/modg/bookshelf/data/metadata/GoogleBooksClientTest.kt index 77a6b0d..d8c1061 100644 --- a/app/app/src/test/java/org/modg/bookshelf/data/metadata/GoogleBooksClientTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/data/metadata/GoogleBooksClientTest.kt @@ -3,7 +3,9 @@ package org.modg.bookshelf.data.metadata import kotlinx.serialization.json.Json import okhttp3.OkHttpClient import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue import org.junit.Test /** @@ -87,6 +89,74 @@ class GoogleBooksClientTest { assertEquals(SourceResult.Failed("malformed json", FailureKind.MALFORMED), result) } + // --- requestUrl(): the key must be present (and encoded) only when configured. --- + + @Test + fun `requestUrl omits the key entirely when it is blank -- the keyless default`() { + val url = client.requestUrl("9780134685991") + + assertTrue(url.contains("9780134685991")) + assertFalse(url.contains("key=")) + } + + @Test + fun `requestUrl appends the key when one is configured`() { + val keyed = GoogleBooksClient(OkHttpClient(), Json, apiKey = "test-key-123") + val url = keyed.requestUrl("9780134685991") + + assertTrue(url.contains("key=test-key-123")) + } + + @Test + fun `requestUrl url-encodes a key that needs it`() { + val key = "a key/with&chars" + val keyed = GoogleBooksClient(OkHttpClient(), Json, apiKey = key) + val url = keyed.requestUrl("9780134685991") + + assertFalse("raw key must not appear unescaped in the url", url.contains(key)) + // "key=" is the last query parameter added, so everything after it is the + // encoded value -- decode it back and confirm it round-trips to the original, + // rather than pinning to one specific percent-encoding scheme. + val encodedKey = url.substringAfter("key=") + assertEquals(key, java.net.URLDecoder.decode(encodedKey, "UTF-8")) + } + + // --- classify() threads the Retry-After header into a 429's retryAfterMillis. --- + + @Test + fun `classify carries a parsed Retry-After into the 429 failure`() { + val result = client.classify(429, null, retryAfterHeader = "1") as SourceResult.Failed + assertEquals(1_000L, result.retryAfterMillis) + } + + @Test + fun `classify leaves retryAfterMillis null when no header is given`() { + val result = client.classify(429, null) as SourceResult.Failed + assertNull(result.retryAfterMillis) + } + + // --- redact(): the API key must never survive into a shown/logged reason. --- + + @Test + fun `redact scrubs the api key out of a reason that leaked the full keyed request url`() { + val key = "test-key-123" + val keyed = GoogleBooksClient(OkHttpClient(), Json, apiKey = key) + val leakedUrl = keyed.requestUrl("9780134685991") + check(leakedUrl.contains(key)) { "fixture assumption broken: url doesn't contain the key" } + + val leaking = SourceResult.Failed("network error: connect to $leakedUrl failed", FailureKind.TRANSPORT) + val redacted = keyed.redact(leaking) as SourceResult.Failed + + assertFalse("literal key must not survive redaction", redacted.reason.contains(key)) + assertTrue(redacted.reason.contains("[REDACTED]")) + } + + @Test + fun `redact is a no-op when there is no key configured or nothing to scrub`() { + val clean = SourceResult.Failed("tls connection reset", FailureKind.TRANSPORT) + assertEquals(clean, client.redact(clean)) + } + private fun fixture(name: String): String = checkNotNull(javaClass.classLoader.getResourceAsStream("fixtures/$name")) { "missing fixture $name" } .bufferedReader() diff --git a/app/app/src/test/java/org/modg/bookshelf/data/metadata/RetryPolicyTest.kt b/app/app/src/test/java/org/modg/bookshelf/data/metadata/RetryPolicyTest.kt index 996a4d2..1124418 100644 --- a/app/app/src/test/java/org/modg/bookshelf/data/metadata/RetryPolicyTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/data/metadata/RetryPolicyTest.kt @@ -131,6 +131,101 @@ class RetryPolicyTest { assertEquals(transport, result) } + // --- a keyed 429 gets exactly one extra retry; a keyless one gets none --- + + @Test + fun `a keyed 429 is retried exactly once`() = runTest { + var calls = 0 + val result = withRetry(retryRateLimitedOnce = true, sleep = {}) { + calls++ + if (calls == 1) SourceResult.fromHttpCode(429) else found + } + assertEquals(found, result) + assertEquals(2, calls) + } + + @Test + fun `a keyless 429 is not retried at all`() = runTest { + var calls = 0 + val result = withRetry(retryRateLimitedOnce = false, sleep = {}) { calls++; SourceResult.fromHttpCode(429) } + assertEquals(1, calls) + assertEquals(FailureKind.RATE_LIMITED, (result as SourceResult.Failed).kind) + } + + @Test + fun `a second consecutive 429 ends it -- one retry means one, not up to MAX_ATTEMPTS`() = runTest { + var calls = 0 + val result = withRetry(retryRateLimitedOnce = true, sleep = {}) { calls++; SourceResult.fromHttpCode(429) } + assertEquals(2, calls) + assertEquals( + SourceResult.Failed("http 429 (rate limited), 2 attempts", FailureKind.RATE_LIMITED), + result, + ) + } + + @Test + fun `the rate-limited retry waits on Retry-After when present and short`() = runTest { + val sleeps = mutableListOf() + var calls = 0 + withRetry(retryRateLimitedOnce = true, sleep = { sleeps.add(it) }) { + calls++ + if (calls == 1) SourceResult.fromHttpCode(429, "1") else found + } + assertEquals(listOf(1_000L), sleeps) + } + + @Test + fun `the rate-limited retry caps a long Retry-After instead of honouring it`() = runTest { + val sleeps = mutableListOf() + var calls = 0 + withRetry(retryRateLimitedOnce = true, sleep = { sleeps.add(it) }) { + calls++ + if (calls == 1) SourceResult.fromHttpCode(429, "120") else found + } + assertEquals(listOf(RetryPolicy.RATE_LIMIT_RETRY_CAP_MILLIS), sleeps) + } + + @Test + fun `the rate-limited retry falls back to the cap when Retry-After is absent`() = runTest { + val sleeps = mutableListOf() + var calls = 0 + withRetry(retryRateLimitedOnce = true, sleep = { sleeps.add(it) }) { + calls++ + if (calls == 1) SourceResult.fromHttpCode(429) else found + } + assertEquals(listOf(RetryPolicy.RATE_LIMIT_RETRY_CAP_MILLIS), sleeps) + } + + @Test + fun `the rate-limited retry falls back to the cap when Retry-After is unparseable`() = runTest { + val sleeps = mutableListOf() + var calls = 0 + withRetry(retryRateLimitedOnce = true, sleep = { sleeps.add(it) }) { + calls++ + if (calls == 1) SourceResult.fromHttpCode(429, "not-a-number") else found + } + assertEquals(listOf(RetryPolicy.RATE_LIMIT_RETRY_CAP_MILLIS), sleeps) + } + + // --- Retry-After parsing itself --- + + @Test + fun `retryAfterMillis honours a short header exactly`() { + assertEquals(1_000L, RetryPolicy.retryAfterMillis("1")) + } + + @Test + fun `retryAfterMillis caps a long header at the rate-limit cap`() { + assertEquals(RetryPolicy.RATE_LIMIT_RETRY_CAP_MILLIS, RetryPolicy.retryAfterMillis("120")) + } + + @Test + fun `retryAfterMillis is null when the header is absent, unparseable, or negative`() { + assertEquals(null, RetryPolicy.retryAfterMillis(null)) + assertEquals(null, RetryPolicy.retryAfterMillis("soon")) + assertEquals(null, RetryPolicy.retryAfterMillis("-5")) + } + @Test fun `backoff is short and jittered, never zero and never seconds long`() { val r = Random(1234) diff --git a/docs/HANDOFF.md b/docs/HANDOFF.md index da1eab4..dea52ba 100644 --- a/docs/HANDOFF.md +++ b/docs/HANDOFF.md @@ -498,3 +498,66 @@ is our ONLY diagnostic channel from a real phone.** Nothing may parse it. - H2 (108 turns, $4.00) followed the brief closely, ran builds in the foreground, and reported honestly, including flagging its own stacked-sheet judgement call. Cost ratio to H1 ($0.66, 5 turns) is roughly the ratio of work actually done. + +## Wave 7 — I-gbkey (Google Books API key): COMPLETE, verified by the orchestrator 2026-09-11 +Prompt: `tasks/I-gbkey.txt`. The user asked for the key on 09-11, which lifts the +standing "do not add it unasked" instruction recorded in wave 6. + +**Split of work, deliberately:** the ORCHESTRATOR did the build plumbing +(`app/app/build.gradle.kts`: read `GOOGLE_BOOKS_API_KEY` from `local.properties`, +enable `buildConfig`, emit `BuildConfig.GOOGLE_BOOKS_API_KEY`) and verified it +green BEFORE launching the worker, because workers on this project are barred from +build files. The Sonnet worker did the Kotlin against a tree where the key was +already available. Reuse this pattern for anything needing a build-file change. + +| Check | Result | +|---|---| +| `./tasks/gw assembleDebug` | exit 0 | +| `./tasks/gw testDebugUnitTest --rerun-tasks` | exit 0 — **189 tests**, 1 skipped, 0 failures (was 172) | +| test count source | summed from `TEST-*.xml`, not the console | +| `./tasks/gw verifyPaparazziDebug` | exit 0 — no pixels moved, as intended | +| `./tasks/gw assembleRelease` | exit 0 — 41,810,740 bytes | +| `grep "always 'false'"` on a `--rerun-tasks` build | **0 hits** | +| boundary check | clean — worker touched only `data/metadata` + the one AppContainer line | +| key-leak check | `git grep` finds no real key in the tree; tests use `test-key-123` | +| **live API check** | HTTP 200 for both previously-failing ISBNs and a control | + +The worker's report was honest: every claim re-verified, including the test count, +and it flagged its own judgement calls (how it reconciled the slightly ambiguous +`Retry-After` cap wording) rather than papering over them. 51 turns, $1.59. + +**The live check was not ceremony.** `HttpUrl.Builder` percent-encodes the colon, +so the app sends `q=isbn%3A...` where every earlier hand-run test sent `q=isbn:...`. +Offline tests cannot distinguish those. Verified: the API accepts both. + +**Both books that failed on the phone are in Google Books** — so the restored +fallback now covers exactly the Open Library TLS-reset failure mode that actually +broke those two scans. The app has been effectively single-sourced since it was +written and is now genuinely two-sourced. Details in `docs/METADATA-SOURCES.md` +§ "The key landed". + +### HAZARD #9 — `run-task.sh` reads the WORKER'S OWN OUTPUT for quota strings +`run-task.sh`'s quota detector greps the worker's result blob for +`usage limit|...|429|too many requests|...`. That blob includes `.result` — the +worker's own prose. **This wave's task was ABOUT HTTP 429**, so the moment the +worker finished and wrote a report mentioning 429, the runner declared +`QUOTA hit (wait #1)`, slept 600s, and was about to `--resume` a session that had +already SUCCEEDED — which would have burned quota redoing finished work and let a +fresh worker turn loose on a completed tree. + +Caught it by checking the log rather than trusting the state line: +`jq '{is_error, subtype, num_turns}' logs/I-gbkey.json` said +`is_error:false, subtype:"success", num_turns:51`. The orchestrator killed the +runner (PID from `ps -o pid,args -p `, per HAZARD #6 — not `pkill -f`) before +the sleep elapsed, and appended a note to `logs/.state` saying why. + +**Before believing any `QUOTA hit` line, check whether the worker actually +finished:** a non-empty `logs/.json` with `is_error:false` means it +SUCCEEDED and the runner is about to waste a session. + +The real fix, for whoever next touches the runner (write a NEW file; never edit +`run-task.sh` while workers are running): the detector must read only the +transport-level error stream, not `.result` prose — e.g. grep `$ERR` alone, or +`jq -r 'select(.is_error==true) | .result'`, rather than `cat "$LOG" "$ERR"`. +Leaving it as-is means any future wave whose subject matter mentions rate limits +or 429 will loop this way. diff --git a/docs/METADATA-SOURCES.md b/docs/METADATA-SOURCES.md index a97e1d1..46cd213 100644 --- a/docs/METADATA-SOURCES.md +++ b/docs/METADATA-SOURCES.md @@ -108,7 +108,7 @@ Worth saying plainly, because it bounds how much weight the numbers carry: ## The options -### A. Give Google Books an API key +### A. Give Google Books an API key — **DONE 2026-09-11, see the end of this file** Free, 1,000 requests/day, no billing account required. Turns the fallback from "silently 429" into a working source. Roughly a dozen lines: a key in `local.properties` → `BuildConfig` → `&key=` on the query. @@ -248,7 +248,8 @@ PERMANENT standing failure, so **every** Open Library hiccup became `Unavailable The app has effectively been single-sourced this whole time while reporting failures as though two sources had been consulted. -**The user has deliberately deferred the API key.** Do not implement it unasked. +**The user deferred the API key at the time; they asked for it on 2026-09-11 and it +is now implemented and verified.** See "The key landed" at the end of this file. ### Open Library: 13% failure, and our own timeout was manufacturing more @@ -292,3 +293,60 @@ scanning a box of books in sequence stays on the fast path after the first book. - The user's own 3-request sample showed 2 failures. That is consistent with 13% (p ~ 5%) but does not confirm it. If their phone reports "3 attempts" often, their network is worse than this one and the retry count deserves revisiting. + +## The key landed — 2026-09-11 + +The user obtained a restricted Google Books API key and it is wired in +(commit below). This closes option A, which had been the single biggest +outstanding win in this file. + +### Verified live, not just in tests + +The key was tested from this machine against the real API before any code was +written, and again afterwards in the exact URL shape the app now builds: + +| ISBN | Result | +|---|---| +| 9781883937386 *Hittite Warrior* | HTTP 200, 1 item | +| 9781883937676 *Shadow Hawk* | HTTP 200, 1 item | +| 9780140449136 *Crime and Punishment* (control) | HTTP 200, 1 item | + +**Both of the books that failed on the user's phone are in Google Books.** They +were always in Open Library too — what actually failed was the TLS-stage +connection reset documented above, not coverage. The significance is that the +fallback would now cover exactly that failure mode: an Open Library transport +error no longer leaves the lookup with nothing to fall back to. The app has been +effectively single-sourced since it was written; it is now genuinely two-sourced. + +Note the second URL test was not redundant. `HttpUrl.Builder` percent-encodes the +colon, so the app sends `q=isbn%3A9781883937386` where every hand-run test in this +file sent `q=isbn:9781883937386`. Offline unit tests cannot tell those apart and +the API accepts both — but that is a fact worth having measured rather than +assumed, because the failure mode would have been a feature that passes every +test and returns nothing on the phone. + +### What was built + +- The key lives in `app/local.properties` (gitignored) as `GOOGLE_BOOKS_API_KEY`, + read by `app/app/build.gradle.kts` into `BuildConfig.GOOGLE_BOOKS_API_KEY`, + falling back to an environment variable of the same name and then to empty. + **A blank key is a supported state** — a fresh clone builds a working app that + falls back to the keyless (429ing) endpoint rather than failing to build. +- `GoogleBooksClient` takes the key and appends it only when non-blank, with URL + construction in a pure `requestUrl()` so it is testable with no socket. +- **The key is scrubbed out of `SourceResult.Failed.reason`** before it can reach + the scan sheet. That string is rendered to the user and is our only diagnostic + channel from a real phone; some okhttp/JDK IOExceptions embed the full request + URL in their message, so the scrub is defensive rather than a response to an + observed leak. Do not remove it on the grounds that nothing currently leaks. +- The `RetryPolicy` RATE_LIMITED decision parked in its KDoc is now resolved: a + **keyed** 429 is the short per-user rate limit and gets exactly one retry, + honouring `Retry-After` capped at 2s; a **keyless** 429 is still the dead daily + quota and is still never retried. + +### What this does NOT fix + +The 17% of books with no cover art anywhere is unchanged, and so is Open +Library's ~13% transport failure rate. What changes is that a failure of one +source is now much more likely to be covered by the other instead of surfacing +as `Unavailable`. diff --git a/tasks/I-gbkey.txt b/tasks/I-gbkey.txt new file mode 100644 index 0000000..4cc81b7 --- /dev/null +++ b/tasks/I-gbkey.txt @@ -0,0 +1,210 @@ +You are implementing ONE feature in the Bookshelf Android app at ~/bookshelf. + +READ FIRST, in this order: + 1. ~/bookshelf/docs/SPEC.md — the authoritative contract, section "Book metadata + lookup". Do not contradict it and do not edit it. + 2. ~/bookshelf/docs/METADATA-SOURCES.md — why this work exists. Read section + "Measured again 2026-09-09" in full; it is the evidence base for every + decision below. + 3. The files you will change: data/metadata/GoogleBooksClient.kt, + data/metadata/MetadataRepository.kt, data/metadata/RetryPolicy.kt, + data/metadata/SourceResult.kt, AppContainer.kt, and their tests. + +## Background — what is already done, do NOT redo it + +The user has obtained a Google Books API key and put it in `app/local.properties` +as `GOOGLE_BOOKS_API_KEY=...`. That file is gitignored. + +The ORCHESTRATOR has ALREADY done the build plumbing, and it is verified working: +`app/app/build.gradle.kts` reads that property (falling back to the environment +variable of the same name, then to empty string), enables the `buildConfig` +feature, and emits `BuildConfig.GOOGLE_BOOKS_API_KEY`. `assembleDebug` is green +and the generated BuildConfig carries the real key. + +So: **the key is already available to Kotlin code as +`org.modg.bookshelf.BuildConfig.GOOGLE_BOOKS_API_KEY`.** You do not need to, and +MUST NOT, touch any build file. If you think you need to, stop and say so in your +report instead. + +The key has been tested live from this machine against all three of these ISBNs — +9781883937386, 9781883937676, 9780140449136 — and returned HTTP 200 with the +right titles. The key works. Your job is to make the app use it. + +## Why this matters (so you make the right call on ambiguity) + +Keyless Google Books returns HTTP 429 to EVERY caller on the internet, forever: +all keyless traffic bills to one shared anonymous Google Cloud project whose daily +quota is permanently exhausted. That is a measured fact, not a guess — see +METADATA-SOURCES.md. + +The knock-on is the part that actually hurt the user. `MetadataRepository.combine` +turns "at least one source Failed, none Found" into `Unavailable`. Google Books has +been a PERMANENT standing failure, so **every** Open Library hiccup surfaced to the +user as "one or more sources couldn't be reached". The app has effectively been +single-sourced this whole time while reporting as though two sources were +consulted. Making the key work restores the second source and, with it, the +honesty of `Unavailable`. + +## What to build + +### 1. GoogleBooksClient sends the key + +Give `GoogleBooksClient` an `apiKey: String` constructor parameter, defaulting to +`""`. Append `&key=` to the request URL ONLY when the key is non-blank. + +A blank key must keep today's exact behaviour (keyless request, which 429s). This +is not a hypothetical: a fresh clone of this repo has no `local.properties` key, +and its build must still produce a working app rather than a broken one. + +URL-encode the key (`java.net.URLEncoder` or okhttp's `HttpUrl.Builder`). Prefer +`HttpUrl.Builder` — okhttp is already a dependency and it encodes query parameters +correctly by construction. + +CRITICAL FOR TESTABILITY: there is no MockWebServer in this project and you must +not add one. Put URL construction in a pure, package-visible function that takes +the ISBN and returns the URL string, exactly the way `classify()` is already +factored out for the same reason: + + internal fun requestUrl(isbn13: String): String + +`fetch()` then calls it. That makes the whole feature unit-testable offline. + +### 2. The key must never leak into anything user-visible or logged + +`SourceResult.Failed.reason` is RENDERED ON THE SCAN SHEET and is our only +diagnostic channel from a real phone. A `reason` that carried the request URL +would put the API key on the user's screen and into any screenshot they send us. + + - Never build a `reason` from the request URL. + - `SourceResult.fromException` derives its reason from exception type/message. + Some okhttp/JDK IOExceptions DO include the full URL in their message + (`java.net.UnknownHostException`, SSL errors, and okhttp's own + `java.io.IOException: Canceled` variants differ by platform). Add a redaction + step so that ANY reason string passing through has an API key replaced with a + placeholder before it can be stored or shown. + - Write a unit test that asserts the key does not appear in the reason for a + failure whose exception message contains the full keyed URL. Assert on the + literal key string you pass in — a test that only checks for "AIza" is not + good enough, because the test key you construct should not look real. + +Do not log the URL anywhere either. (`metadataHttpClient` in AppContainer has no +logging interceptor today — do not add one.) + +### 3. Wire the key through (AppContainer + MetadataRepository) + +`MetadataRepository`'s convenience constructor `(httpClient, json)` builds both +clients itself. Add a `googleBooksApiKey: String = ""` parameter to it and pass it +to `GoogleBooksClient`. Keep the primary constructor (which takes the two clients) +as it is — the tests use it. + +In `AppContainer`, pass `BuildConfig.GOOGLE_BOOKS_API_KEY` when constructing +`MetadataRepository`. That single line is the ONLY change you may make to +AppContainer.kt, and AppContainer.kt is the only file outside `data/metadata` you +may touch at all. + +### 4. A keyed 429 is a different animal — make it retryable, once + +`RetryPolicy.isRetryable` deliberately refuses to retry `RATE_LIMITED`, and its +KDoc explains why AND explicitly parks this decision for the wave you are now +doing. Read that KDoc before writing anything here. + +The reasoning: today's keyless 429 is an exhausted DAILY quota that never clears +within a scanning session, so retrying is pure waste and risks turning an +intermittent block into a permanent one. A KEYED 429 is usually the per-user +per-100-seconds rate limit instead, which does clear, and is worth exactly one +more ask. + +Implement it narrowly: + - `withRetry` gains a parameter, default `false`, that permits ONE retry of a + RATE_LIMITED failure — e.g. `retryRateLimitedOnce: Boolean = false`. + Open Library's call site keeps the default and its behaviour must not change. + - `GoogleBooksClient` passes `apiKey.isNotBlank()` for it. Keyless stays + unretried, which preserves today's measured-correct behaviour. + - One retry means one: a second 429 is returned to the caller, never a third + attempt, regardless of `MAX_ATTEMPTS`. + - Respect a `Retry-After` header if the response carries one, but CAP the wait + at ~2 seconds. The user is standing at a bookshelf. If the header is absent, + unparseable, or over the cap, use a short fixed backoff instead of honouring + it. Note this means `classify()` — currently `(httpCode, body)` — needs the + header value to reach the retry decision; thread it through in whatever way + keeps `classify` a pure function, and keep its existing tests passing. + - Unit-test all of it with the injected `sleep`/`nowMillis` hooks `withRetry` + already has. No test may sleep on the real clock. + +Keep every existing guarantee in `withRetry` intact, including the "N attempts" +suffix on a surviving failure's `reason`. + +### 5. Update the stale comments you invalidate + +Several comments now describe a world that no longer exists — the KDoc at the top +of `GoogleBooksClient` says "No API key", its `lookup` KDoc explains why retrying +costs nothing today, and `RetryPolicy`'s RATE_LIMITED bullet parks the decision you +are implementing. Update each to describe the new behaviour. Do not leave a comment +that contradicts the code; this project treats that as a defect. + +Do NOT edit docs/SPEC.md, docs/HANDOFF.md or docs/METADATA-SOURCES.md — the +orchestrator owns those. + +## Constraints — these are hard + +- Kotlin. Match the surrounding code's style, naming and comment density. These + files are heavily commented with the EVIDENCE for each decision, not with + restatements of the code. Read neighbouring files before writing and follow that + convention. +- DO NOT touch: app/app/build.gradle.kts, gradle/libs.versions.toml, or any other + build file. The plumbing is done and verified. +- DO NOT touch anything under data/local, data/remote, data/repo, data/prefs, or + any ui/ package. AppContainer.kt is limited to the one line in §3. +- DO NOT read, print, cat, grep or echo `app/local.properties` or any file that + might contain the key, and never paste a real key into a source file, a test, or + your report. Tests must use an obviously-fake key like "test-key-123". +- DO NOT git commit, git add, or git push. The orchestrator commits. Leave your + work in the working tree. +- Build with `./tasks/gw ` — NEVER `./gradlew` directly (tasks/gw is a + flock-serialized wrapper). +- RUN BUILDS IN THE FOREGROUND. Do not background a Gradle build and end your turn + saying you will report later — two previous workers did exactly that and could + never report. A full build here takes 1-3 minutes; just wait for it. +- There is no emulator on this box (no KVM). You cannot run the app. + +## Definition of done + +All of these must pass, and you must run them yourself and paste the real output: + + ./tasks/gw assembleDebug -> exit 0 + ./tasks/gw testDebugUnitTest -> exit 0. 172 tests pass today (1 skipped, + LiveSyncTest). The count must go UP and + nothing may regress. + ./tasks/gw verifyPaparazziDebug -> exit 0. This change is not supposed to move + a single pixel; if a snapshot fails, you + changed something you should not have. + +Required new tests, with REAL assertions: + - requestUrl includes `key=` with a non-blank key, and omits it entirely when + the key is blank. + - a key needing URL-encoding is encoded. + - the key is redacted out of a Failed reason built from an exception whose + message contains the keyed URL (§2). + - a keyed 429 is retried exactly once; a keyless 429 is not retried at all; + a second 429 ends it. + - Retry-After is honoured when short, capped when long, ignored when absent or + unparseable. +Assertion-free tests are a spec violation on this project. + +Also: check the build log for Kotlin warnings on files you touched. A warning +reading "Check for instance is always 'false'" is NOT cosmetic — that exact +warning hid a bug that blanked every book cover in this app for months. If you see +it, you have written dead code; fix it. + +## Report + +End with a plain report covering: + - what you changed, file by file + - the verbatim tail of each of the three gradle commands + - the test count before and after + - anything you could NOT do, or did differently from these instructions, and why + - anything you noticed that looks wrong but was out of scope + +Be honest. Workers on this project have over-claimed success before, and the +orchestrator independently re-verifies everything, so an inflated report only +wastes a round trip.