jfmonty2/bookshelf
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Second on-device feedback round: eight fixes, and a measured retry policy

Browse Source 
The ghost bookcase was an inset bug, not a data bug. LocationsScreen's list
branch dropped the Scaffold's innerPadding while its empty-state branch applied
it, so the first bookcase row rendered under the top app bar and was invisible.
Both of the user's bookcases were always real; they just could not see the first
one, so they made a second. Every other screen was checked for the same class of
bug — Locations was the only one.

Metadata lookup is now designed against a measurement rather than a guess
(docs/METADATA-SOURCES.md § "Measured again 2026-09-09"):

  - Google Books keyless is dead for everyone, permanently. The user's
    residential-IP test returned a quota error naming a shared anonymous PROJECT,
    not an IP, so the earlier "your phone may well get answers" guess is wrong and
    is now marked CORRECTED in place. Because combine() turns any Failed-with-no-
    Found into Unavailable, that standing failure meant every Open Library hiccup
    surfaced as "one or more sources couldn't be reached". The API key is
    deliberately deferred by the user; this commit leaves the source broken.

  - Our own timeouts were manufacturing failures. Over 30 live requests, 13%
    failed — all fast TLS resets under 2.5s — while successes ran to a median of
    4.3s and a max of 22.0s. Two of 26 successes exceeded the old 12s callTimeout,
    so ~8% of lookups that were about to work were cancelled and reported as
    unreachable. Timeouts are now 25s/20s/20s.

That asymmetry (cheap failures, expensive successes) is what RetryPolicy encodes.
It retries TRANSPORT and SERVER_ERROR with a 250ms/750ms jittered backoff, and
deliberately does not retry TIMEOUT (the budget is already spent) or RATE_LIMITED
(hammering a quota is how an intermittent block becomes a permanent one — this
project's IP has already been refused outright once during research).

SourceResult.Failed now carries a FailureKind alongside its human reason, and the
reason names the specific failure ("tls connection reset, 3 attempts") instead of
a generic "network error". That string was already threaded to the UI and dropped
on the floor; LookupFailedSheet now renders it. It is the only diagnostic channel
we have from a real phone, so nothing may parse it.

Also from the same feedback round:
  - Grouped ModalBottomSheet shelf picker, replacing two near-duplicate flat
    dropdowns that listed every bookcase x shelf pair. Sections per bookcase,
    empty bookcases say so, and the most recently used shelf is pinned on top.
  - The recent shelf persists across sessions (SettingsStore.LAST_SHELF_ID) and is
    cleared on sign-out. It is offered, never pre-selected: the user weighed that
    and chose one tap over the risk of silently mis-shelving a book.
  - Locations dialogs and the manual-ISBN dialog auto-focus their first field.
  - The library filter menu offers "Add a bookcase to enable filtering" instead of
    a lone "All books" that is already the active state and cannot be changed.
  - The Locations button is Material Symbols' "shelves" (a bookcase) instead of
    Warehouse (a barn). material-icons-extended 1.7.8 has no bookcase glyph.
  - The scan sheet drops "you can lower the book" — the ISBN echo already says it.

assembleDebug exit 0; testDebugUnitTest 172 tests, 1 skipped, 0 failures (was
138); verifyPaparazziDebug exit 0; assembleRelease exit 0, signed with the real
release key; zero "always 'false'" warnings on a --rerun-tasks rebuild.

Three soft spots are recorded in docs/HANDOFF.md and are NOT verified: the
ghost-bookcase Paparazzi snapshot renders a lookalike of the screen rather than
the screen, the auto-focus calls swallow their own failure and no emulator exists
here, and the picker opens as a sheet stacked on the save sheet.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LSnVqWdiQNEcPFRq1hGZAi
This commit is contained in:
Sprite
2026-09-09 17:45:09 +00:00
co-authored by claude
parent dd3a61fc33
commit d6d02f788c
44 changed files with 1740 additions and 127 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/HANDOFF.md
+87
View File
@@ -411,3 +411,90 @@ successfully. Use these instead, in this order:
2. `tail logs/<name>.state` — says SUCCESS / GIVING UP / WALL CLOCK explicitly.
3. `git status --porcelain` — is there actually work in the tree?
The sentinel is a convenience, not the record of truth. `logs/<name>.state` is.
## Wave 6 — second on-device feedback round: COMPLETE, verified 2026-09-09
The user tested the phone build again and sent eight items. All eight are done.
Two Sonnet workers (`tasks/H1-screens.txt`, `tasks/H2-picker.txt`) took the UI
work; the ORCHESTRATOR did the retry/backoff work itself in `data/metadata` and
`AppContainer`, because it needed the live measurement below to design it.
| Check | Result |
|---|---|
| `./tasks/gw assembleDebug` | exit 0 |
| `./tasks/gw testDebugUnitTest` | exit 0 — **172 tests**, 1 skipped, 0 failures (was 138) |
| `./tasks/gw verifyPaparazziDebug` | exit 0 |
| `grep "always 'false'"` on a `--rerun-tasks` rebuild | **0 hits** |
| `./tasks/gw assembleRelease` | exit 0 — 41,810,740 bytes, V2 signer `CN=Bookshelf, O=Montanaro` |
| boundary check | clean — neither worker touched a build file or the other's packages |
### The ghost bookcase was an inset bug, not a data bug
`LocationsScreen`'s list branch dropped the Scaffold's `innerPadding` while its
empty-state branch applied it, so the FIRST bookcase row rendered underneath the
top app bar and was invisible. Every symptom the user described follows from that:
invisible first bookcase, no empty state on re-entry (the list was genuinely
non-empty), a second bookcase created, both showing in the filter menu. **Both
records were always real and healthy** — the user should delete the spare.
Every other screen was checked for the same class of bug; Locations was the only
one. Fix: fold `innerPadding` into the LazyColumn's `contentPadding` (NOT
`Modifier.padding`, which would clip the scroll area instead of insetting it).
### Metadata: measured, not guessed
See `docs/METADATA-SOURCES.md` § "Measured again 2026-09-09" for the full data.
Two things that change how you should think about this app:
1. **Google Books keyless is dead for everyone, permanently.** The user's
residential-IP test returned a quota error naming `project_number:624717413613`
— a shared anonymous *project*, not an IP. The old note in METADATA-SOURCES.md
guessing that a residential IP "may well get answers" is now marked CORRECTED
in place. Because `combine()` turns any Failed-with-no-Found into `Unavailable`,
this standing failure meant **every** Open Library hiccup surfaced as
"one or more sources couldn't be reached". The app has been single-sourced all
along. **The user has deliberately deferred the API key — do not add it unasked.**
2. **Our own timeouts were manufacturing failures.** 30 live requests: 13% failed,
all fast TLS resets (<2.5s); successes had a median of 4.3s but a max of 22.0s,
and **2 of 26 successes exceeded the old 12s `callTimeout`**. Timeouts are now
25s/20s/20s. Failures are fast and successes are slow, so a short timeout buys
nothing on the failure path and costs real successes on the slow path.
`RetryPolicy` + `withRetry` (new, `data/metadata/`) retry TRANSPORT and
SERVER_ERROR only. It deliberately does NOT retry:
- **TIMEOUT** — the budget is already spent; retrying could triple the wait.
- **RATE_LIMITED** — hammering a quota is how an intermittent block becomes a
permanent one, and METADATA-SOURCES.md records that happening to this project's
IP. Revisit when the Google Books key lands: a *keyed* 429 is a per-second limit
and does deserve one Retry-After-respecting retry.
`SourceResult.Failed` now carries a `FailureKind` alongside its human `reason`, and
`reason` names the specific exception ("tls connection reset, 3 attempts") instead
of a generic "network error". **That string is now rendered on the scan sheet and
is our ONLY diagnostic channel from a real phone.** Nothing may parse it.
### Known-soft spots in wave 6 — do not mistake these for verified
1. **The Paparazzi "regression" snapshot for the ghost bookcase is a lookalike,
not the real screen.** `LocationsScreenPaparazziTest` hand-rolls its own
Scaffold+LazyColumn copy because the real `LocationsScreen` needs an
`AppContainer` (Room + DataStore). The orchestrator verified the REAL fix by
reading the diff; the PNG only proves the test's copy is right, and the two can
drift — the copy already omits the bottom inset the real screen adds. Splitting
a stateless `LocationsContent(state, callbacks)` out of the screen would make
this snapshot genuine. Worth doing before anyone trusts it as regression cover.
2. **The auto-focus calls are unverified.** Three dialogs now do
`LaunchedEffect(Unit) { runCatching { focusRequester.requestFocus() } }`. That
is the idiomatic form, but there is no emulator here and `runCatching` means a
too-early call fails SILENTLY rather than crashing. If a dialog opens unfocused
on the phone, that is why; the fix is to await a frame before requesting.
3. **The shelf picker opens as a bottom sheet stacked on top of the save sheet**
(H2's own flagged judgement call). It renders correctly in Paparazzi but
sheet-over-sheet is awkward on real Android. Watch it on the device.
### Worker lessons (both are repeats — the prompts already forbade them)
- **H1 backgrounded a Gradle build and ended its turn**, exactly the wave-4
failure, despite an explicit foreground-only instruction AND
`CLAUDE_CODE_PRINT_BG_WAIT_CEILING_MS=0` being set in `run-task.sh`. Its final
message was "I'll wait for this background build to complete." `run-task.sh`
still recorded SUCCESS because the process exited 0. **`.state` saying SUCCESS
means the process exited cleanly, NOT that the worker finished its task** — read
`logs/<name>.summary` and check that the result is an actual report. H1's work
was fine, but nobody verified it except the orchestrator.
- 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.
docs/METADATA-SOURCES.md
+77
View File
@@ -97,6 +97,10 @@ Worth saying plainly, because it bounds how much weight the numbers carry:
this host sharing a quota pool with other tenants; **your phone, on a
residential or mobile IP, may well get answers.** That's precisely why the
app needs to be able to tell us which it got.
> **CORRECTED 2026-09-09 — this guess was WRONG.** The user tested from a
> residential IP and got the same refusal, naming a shared *project* quota
> rather than an IP one. See "Measured again 2026-09-09" at the end of this
> file. Do not act on the sentence above.
- I don't know which three ISBNs you scanned. If you still have the books to
hand, those three numbers are worth more than another 60 sampled ones.
@@ -215,3 +219,76 @@ One thing worth deciding separately: 17% of books legitimately have no cover art
anywhere. The placeholder now looks deliberate rather than broken, but if you
want covers on everything, that's a different feature — photograph the book,
store it as the cover — and not a metadata-source problem at all.
## Measured again 2026-09-09, after the user tested from a residential IP
Two things were settled that the first round could only guess at.
### Google Books keyless is dead everywhere, not just from this box
The user ran the app's exact Google Books call from their home connection and got:
Quota exceeded for quota metric 'Queries' and limit 'Queries per day'
of service 'books.googleapis.com' for consumer 'project_number:624717413613'
That names a **Google Cloud project, not an IP**. Every keyless caller on the
internet is billed to that one shared anonymous project and its daily quota is
exhausted. So:
- The earlier caveat — "your phone, on a residential or mobile IP, may well get
answers" — is **WRONG**. Delete it from your mental model. It was tested and it
is not true.
- Backoff cannot help. This is a daily quota, not a per-second rate limit.
- A free API key is the only fix, and it is a complete one: it moves the app into
its own project with its own quota (free tier 1,000 req/day).
The knock-on is the part that actually hurt the user. `MetadataRepository.combine`
turns "any source Failed, none Found" into `Unavailable`. Google Books is a
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.
### Open Library: 13% failure, and our own timeout was manufacturing more
30 requests, the exact call `OpenLibraryClient` makes, 1.5s apart, from the sprite:
| | |
|---|---|
| failure rate | **13%** (4 of 30) |
| every failure | curl exit 35 — TLS-stage `Connection reset by peer` |
| failure latency | 0.23s, 0.31s, 0.59s, 2.46s — **all fast** |
| success latency | median **4.3s**, p75 6.0s, p90 **9.2s**, max **22.0s** |
| successes over the old 12s callTimeout | **2 of 26 (8%)** |
Two conclusions, and they point in opposite directions:
1. **Failures are cheap and transient**, so retrying is nearly free. 13% → ~1.7%
at two attempts → ~0.2% at three. This is why `RetryPolicy` exists and why its
backoff is milliseconds rather than the conventional seconds.
2. **Successes are slow and long-tailed**, and the app's own
`callTimeout(12s)`/`connectTimeout(10s)` were cutting off roughly 8% of
lookups that were about to succeed — then reporting them to the user as
"couldn't be reached". The app was generating a meaningful share of its own
failures. Timeouts are now 25s/20s/20s, above the 22.0s worst observed success.
The asymmetry is the whole design: a short timeout buys nothing on the failure
path (failures return in under 2.5s regardless) and costs real successes on the
slow path. That is also why `RetryPolicy.isRetryable` refuses to repeat a
TIMEOUT — a timeout means the budget was already spent, and the evidence says
slow requests mostly succeed if you let them finish.
Connection reuse is visible in the data and matters in real use: cold connects
ran 2-19s while warm ones ran 0.07s. OkHttp pools connections for 5 minutes, so
scanning a box of books in sequence stays on the fast path after the first book.
### What still is not known
- All 30 requests came from this datacenter IP. The user's phone may see a
different failure rate. The reason string now shown on the scan sheet
(`SourceResult.Failed.reason`, e.g. "tls connection reset, 3 attempts") is how
we find out — it is the only diagnostic channel we have from a real device.
- 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.