Skip to content

fix: stop the OOM/502 outage at the root — PDS restore guard, bounded native memory - #197

Merged
nperez0111 merged 8 commits into
mainfrom
feat/bound-refetchbooks-fanout
Aug 2, 2026
Merged

fix: stop the OOM/502 outage at the root — PDS restore guard, bounded native memory#197
nperez0111 merged 8 commits into
mainfrom
feat/bound-refetchbooks-fanout

Conversation

@nperez0111

@nperez0111 nperez0111 commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Every one of the 66 requests over 30s in a measured 6h production window carried oauth_restore: "failed" — one user's PDS started blackholing packets, oauthClient.restore() had no timeout, and it hung holding the cross-process refresh lock while every other request for that DID in every worker burned the lock's full 37.5s poll budget at three synchronous SQLite statements per poll, until workers stopped calling accept() (Caddy's 166,450 dial tcp: i/o timeout, the dominant class of the outage's 171,145 502s). This PR adds guardedRestore — a 5s timeout plus a circuit breaker keyed by authorization-server host, resolved from a local KV read — bounds the lock wait to 3s with exponential backoff, and stops destroying sessions on transient failures; then removes the unbounded native memory that made the ~148 OOM kills possible: DB_MMAP_SIZE now defaults to 0, the OG cache is deleted outright (Cloudflare already serves every repeat from the edge, so over 48h the origin saw 1,189 requests across 1,134 distinct cards — a perfect origin cache could have served 4%), etag() no longer buffers whole downloads, uploads check size before arrayBuffer(), library re-sync fans out at 3 instead of one search per record, and the KV is VACUUMed on startup (measured at 1.94 GB on disk holding 34.7 MB of live rows).

It also makes the failures visible for the first time — worker_exit classification moves into a tested server/worker-exit.ts, since bunfig's test root was src and that is exactly how a number-keyed lookup against Bun's string signalCode shipped "SIGSIGKILL" and a permanently false likely_oom through every one of those kills — and adds external/array_buffers gauges and a per-worker label, without which the SO_REUSEPORT workers alias into one silently-alternating series. The two hot scans over 356k rows are indexed (FTS5 for search; a hive_book_author join table for author lookup, taking /authors/:author from ~511ms to 1ms with identical counts), and the enrich queue gains the terminal state it needed to converge — previously a row at max attempts was deleted without recording anything on the book, so the next crawler page view re-enqueued it forever. Tracing turned out to have been live the whole time (13.7M spans): the repo's compose.yaml pointed OPEN_OBSERVE_URL at localhost while the deployment used openobserve, which is how the pipeline was wrongly written off as dead, so that config is fixed and route spans are now named per matched route rather than all sharing "hono-middleware".

Two open items deliberately left out: fetchGoodreadsViaWaf books both the plain-HTTP fetch and the WAF solve against a single circuit breaker, so a degraded solver starves a path that still succeeded 4,671 times in 24h; and mem_limit can go back from 6g to 3g once the acceptance criteria in the plan hold.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added faster, more accurate book and author search.
    • Added conditional downloads for unchanged personal books.
    • Added clearer enrichment retry handling and improved library synchronization.
    • Added richer operational diagnostics and monitoring.
  • Bug Fixes

    • Prevented oversized personal-book uploads with a 100 MB limit.
    • Improved resilience when authentication services are unavailable.
    • Reduced duplicate cover-image rendering and improved service connectivity handling.
    • Improved worker failure reporting and memory diagnostics.

… memory

A dead PDS could take down every worker. `oauthClient.restore()` had no
timeout: when one user's PDS started blackholing packets, the refresh hung
while holding the cross-process lock (whose heartbeat kept renewing it, so it
was never evicted as stale), and every other request for that DID — in every
worker — burned the lock's full 37.5s poll budget at three synchronous SQLite
statements per poll. Workers stopped calling accept(); Caddy logged 166,450
`dial tcp: i/o timeout`, the dominant class of the outage's 171,145 502s.
Measured in production: all 66 requests over 30s in a 6h window carried
`oauth_restore: "failed"`, and nothing else.

Root fix:

- `guardedRestore` (src/auth/restore-guard.ts) wraps every restore in a 5s
  timeout and a CircuitBreaker keyed by the authorization-server host, read
  from the stored session's `tokenSet.iss` — a local KV read, never a network
  call. Once a host trips, requests fail instantly instead of dispatching.
- refresh-lock waits 3s with exponential backoff rather than 250 flat 150ms
  polls: ~21 SQLite statements per waiter instead of ~750.
- `getSessionAgent` only destroys the session when the PDS actually rejected
  our credentials. A timeout used to silently log the user out.

Bounded the unbounded native memory that made the OOM kills possible:

- `DB_MMAP_SIZE` defaults to 0. Mapping a 1.6 GB database into every worker
  moved RSS by 971 MB per full-table scan, to save ~390ms.
- OG cards cache in the shared SQLite KV, not a per-process unbounded Map of
  webp bytes held for 7 days. Production traffic is a crawler sweeping the
  catalog — 674 distinct cards in 3h at a 4.4% hit rate, so it only ever grew.
- `etag()` skips `/library/books/*` and `/opds/books/*`, which set their own
  strong ETag from `contentHash`. It clones and drains one tee branch through
  the digest while nothing reads the other: 134 MB of arrayBuffers for a
  120 MB download, and streaming defeated outright.
- Uploads check `file.size` before `arrayBuffer()`; the XRPC procedure gets
  the same cap, which it previously lacked entirely.
- Library re-sync fans out at most 3 searches instead of one per record.
- The KV is VACUUMed on startup and switched to auto_vacuum=INCREMENTAL:
  measured at 1.94 GB on disk holding 34.7 MB of live rows, 98.1% free pages.

Made the failures visible. `worker_exit` classification lived in cluster.ts
where no test could reach it — bunfig's test root was `src`, which is how
`signalName` shipped a number-keyed lookup against Bun's *string* signalCode,
emitting "SIGSIGKILL" and a permanently false `likely_oom` through ~148 OOM
kills. It now lives in server/worker-exit.ts with tests, and memory is sampled
from procfs while workers are alive, since procfs is gone by the time onExit
fires. Metrics gain `external`/`array_buffers` (where this app's real
allocations live) and a per-worker label, without which the SO_REUSEPORT
workers alias into one silently-alternating series.

Indexed the two hot scans over 356k rows: FTS5 for search, and a
`hive_book_author` join table for author lookup — `/authors/:author` planned a
full scan plus a temp B-tree at ~511ms and now returns identical counts at
1ms. Deliberately not FTS5 for authors: that is exact identity, not text
search; "Stephen King" must not also match "Stephen Kingsley".

The enrich queue could not converge: a row at max attempts was deleted without
recording anything on the book, so the next page view re-enqueued it — a
perpetual-motion machine with a crawler walking the catalog. Terminal state
now lives on `hive_book`, with a shared 7d cooldown inside
`enqueueEnrichmentBatch` rather than at each call site.

Tracing was live all along (13.7M spans); the repo's compose.yaml pointed
`OPEN_OBSERVE_URL` at localhost while the deployment used `openobserve`, and
that mismatch is how the pipeline was wrongly written off as dead. Fixed the
config, and named the route spans — 82.6% of spans were literally
"hono-middleware", which defeated grouping entirely.

`google.ts`/`isbndb.ts` are deleted as dead code. The WAF solver stays: over
7 days it produced 356 direct successes plus 2,023 cached-token successes, so
~24% of successful enrichments depended on it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@nperez0111, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 39 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0510ddf6-b831-4ea7-b88c-d128492b6a89

📥 Commits

Reviewing files that changed from the base of the PR and between d66d4fc and 362bf68.

📒 Files selected for processing (5)
  • AGENTS.md
  • server/cluster.ts
  • src/context.ts
  • src/middleware/otel-middleware.ts
  • src/pages/bookInfo.tsx
📝 Walkthrough

Walkthrough

This PR adds worker exit and memory diagnostics, OAuth restore protection, FTS5 and author indexing, enrichment cooldowns, KV vacuuming, upload limits, conditional downloads, OG render deduplication, tracing updates, structured logging, and updated operational documentation.

Changes

Worker supervision and memory diagnostics

Layer / File(s) Summary
Worker-exit classification
server/worker-exit.ts, server/worker-exit.test.ts
Adds signal normalization, OOM detection, process-memory parsing, structured exit events, and tests.
Cluster worker integration
server/cluster.ts, Dockerfile
Samples worker memory, classifies exits, logs restart details, and copies the classifier into the runtime image.
Memory diagnostics and worker metrics
src/routes/debug.ts, src/metrics.ts
Adds /memory diagnostics and worker-scoped runtime metrics.

OAuth restore resilience

Layer / File(s) Summary
Refresh-lock timeout and backoff
src/auth/refresh-lock.ts, src/auth/refresh-lock.test.ts
Uses deadline-based exponential polling and validates timeout behavior.
Restore circuit breaker
src/auth/restore-guard.ts, src/auth/restore-guard.test.ts
Adds bounded per-host breakers, restore timeouts, error classification, cooldowns, and state inspection.
Guarded restoration integration
src/auth/storage.ts, src/context.ts, src/auth/pds-outage.test.ts
Derives issuer-host keys, protects restores, records telemetry, and preserves sessions for transient failures.

Book indexing and search

Layer / File(s) Summary
FTS5, author, and enrichment schema
src/db.ts, src/types.ts, src/db.authors.test.ts
Adds FTS5, normalized author rows, enrichment state columns, triggers, and schema tests.
Normalized author query paths
src/pages/*, src/routes/og.tsx, src/utils/authorMatching.ts, src/utils/authorMatching.test.ts
Replaces serialized author pattern matching with exact indexed joins.
FTS query and resynchronization search
src/utils/ftsQuery.ts, src/utils/ftsQuery.test.ts, src/routes/lib.ts
Adds FTS query validation, indexed local search, bounded refetch concurrency, and tests.

SQLite KV maintenance

Layer / File(s) Summary
KV vacuum lifecycle
src/sqlite-kv.ts, src/sqlite-kv.vacuum.test.ts, src/context.ts, src/workers/*
Returns the raw SQLite handle, adds full and incremental vacuuming, wires maintenance into startup and cleanup, and updates KV consumers.

Enrichment queue convergence

Layer / File(s) Summary
Enrichment cooldown and queue metrics
src/utils/enrichQueue.ts, src/utils/enrichQueue.test.ts, src/scrapers/goodreads.ts, src/types.ts, src/db.ts
Tracks attempts and failures, applies a seven-day cooldown, publishes queue statistics, and tests retry convergence.

Upload and streaming controls

Layer / File(s) Summary
Personal-library upload validation
src/utils/personalLibrary.ts, src/utils/personalLibrary.test.ts, src/routes/library.tsx, src/routes/opds.ts, src/xrpc/router.ts
Adds the shared 100 MiB limit, validates sizes before buffering, and supports conditional downloads with ETags.
Streaming response ETag behavior
src/app.ts
Excludes ebook and import routes from global ETag middleware.

OG rendering and shedding

Layer / File(s) Summary
In-flight OG rendering
src/routes/og.tsx, src/workers/og-render/client.ts, src/metrics.ts
Removes persistent OG caching, shares concurrent renders, and records queue shedding.

Tracing, logging, and build configuration

Layer / File(s) Summary
Tracing and request events
server/plugins/otel-sdk.ts, src/middleware/otel-middleware.ts, src/middleware/wide-event.ts
Refines instrumentation, route span names, HTTP attributes, and worker event labels.
OpenObserve worker logging
src/workers/open-observe-worker.ts, compose.yaml
Uses the container endpoint and emits structured failure and disable events.
Test and runtime configuration
bunfig.toml, package.json, src/env.ts, src/workers/*
Expands test discovery, updates memory configuration, and adapts runtime KV initialization.
AGENTS.md operational documentation
AGENTS.md
Documents the current architecture, routes, storage, indexing, OAuth, tracing, worker behavior, scrapers, and test commands.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

Poem

A rabbit checks the worker’s flight,
And guards the PDS through day and night.
FTS finds books in tidy rows,
While queues cool their retry woes.
ETags whisper, “Not modified!”
Logs hop neatly, well-structured.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 52.46% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the primary outage fixes: PDS restore protection and bounded native memory usage.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/bound-refetchbooks-fanout

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

…ache

Measured over 48h of production traffic, the origin sees an almost perfectly
unique stream of OG requests: 1,189 across 1,134 distinct cards, 1,081 of them
requested exactly once, max 4 repeats. A *perfect* origin cache could have
served 4% of them. That is not a coincidence — these responses carry
`public, max-age=…` and Cloudflare serves the repeats from the edge (verified:
MISS then HIT on a real card), so by construction the origin only ever sees
misses.

We were paying for that 4% twice over. First with ocache's unbounded per-process
`Map` of webp bytes, which is one of the things that OOM-killed the workers.
Then, in this branch, with an `og_cache` KV table — a base64 round-trip
inflating every card 33% into SQLite TEXT, a sweep on the 15-min timer, two
gauges, and another writer to the file we already have to VACUUM. Rendering on
demand costs ~600ms p50 on a worker thread, ~25 times an hour.

So: no server-side cache. `src/utils/ogCache.ts` and its test are deleted, along
with the `og:` mount, the sweep, and `bookhive_og_cache_entries`/`_bytes`. That
also retires the import-cycle workaround the module existed to dodge — nothing
in `src/context.ts` reaches into the OG path any more.

What survives is `renderOnce` in `src/routes/og.tsx`: concurrent requests for
the *same* cold card share one render rather than starting N. It holds promises,
never bytes, and always clears in `finally` — the one failure mode the edge
cache genuinely cannot cover, and the same class of unbounded fan-out this
branch is about everywhere else.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 22

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@AGENTS.md`:
- Around line 615-618: Update the Architecture at a Glance diagram near the
scraper overview to remove Google and ISBNdb from the active scraper list, or
explicitly label them as historical, while keeping only currently supported
scrapers as active runtime guidance.

In `@compose.yaml`:
- Around line 97-101: In compose.yaml, add a healthcheck to the openobserve
service and update the server service’s dependency condition to require
openobserve.service_healthy before startup. Preserve the existing
OPEN_OBSERVE_URL and ensure the dependency uses the healthcheck result rather
than simple container startup.

In `@server/cluster.ts`:
- Around line 90-94: Update logWorkerExit to return the classified worker-exit
event produced by classifyWorkerExit, then use its likely_oom field when
composing the restart message instead of recomputing the SIGKILL/null-exit
condition. Keep the structured and human-readable logs consistent with the
single classification result.

In `@src/app.ts`:
- Around line 121-128: Update ETAG_EXCLUDED_PREFIXES and the etag middleware
guard to explicitly bypass both "/import" and "/import/" paths, regardless of
route registration order. Add a regression test covering the import SSE response
and verifying it does not pass through etagMiddleware.

In `@src/auth/restore-guard.ts`:
- Around line 43-75: Ensure getBreaker always enforces MAX_BREAKERS by selecting
an eviction candidate even when no breaker is closed. Remove the closed-state
filter or add a bounded fallback that evicts the least recently used entry when
all breakers are open or half_open, while preserving the existing
least-recently-used selection.

In `@src/context.ts`:
- Around line 397-404: Update restoreGuardKey to avoid exposing the raw DID when
getStoredSessionIssuerHost returns no value: derive a hashed or truncated
fallback key before it is used in SESSION_FAIL breaker state and returned by
restoreGuardStates(). Preserve the existing issuer-host key behavior for stored
sessions and keep the result stable for the same DID.

In `@src/db.ts`:
- Around line 710-760: Update the post-VACUUM flow in server/cluster.ts and
src/context.ts to rebuild the external-content hive_book_fts index after
sqlite.exec("VACUUM") runs on env.DB_PATH. Execute INSERT INTO
hive_book_fts(hive_book_fts) VALUES ('rebuild') on the same database connection,
or exclude the main database from VACUUM; preserve migration 019’s FTS setup.

In `@src/middleware/otel-middleware.ts`:
- Around line 20-23: Update the initial span-name fallback in the middleware to
use only ctx.req.method, rather than combining the method with ctx.req.path;
retain ctx.req.path unchanged for ATTR_URL_PATH and preserve the existing
matched-route renaming via updateName.

In `@src/pages/bookInfo.tsx`:
- Around line 208-211: Trim the first author value at its source in the author
parsing flow, using parseAuthors or equivalent trimming before assigning
firstAuthor. Reuse that normalized value for both the hive_book_author equality
filter and the /authors/:author link so related-book queries and
getBooksByAuthor receive the same trimmed author.

In `@src/routes/lib.ts`:
- Around line 27-33: Configure the module-scoped searchSlots Semaphore with
finite maxPending and acquireTimeoutMs values so refetchBooks cannot accumulate
unbounded waiters. Update the promises pushed by refetchBooks to catch and
swallow semaphore shed/timeout rejections, preserving Promise.all completion for
the remaining book searches instead of aborting the entire re-sync.

In `@src/routes/library.tsx`:
- Around line 78-83: Add a zValidator("form", ...) middleware before the /upload
handler, defining the form schema with the required file validation and size
constraint. Keep the existing body-size limiter registered before this
validator, and update the handler to obtain the validated fields through
c.req.valid("form") instead of manually parsing FormData or performing ad-hoc
file checks.
- Around line 78-83: Enforce upload limits before request bodies are
materialized: in src/routes/library.tsx at lines 78-83, add Hono bodyLimit with
maxSize MAX_FILE_SIZE and the requested error response before c.req.formData(),
while retaining the file.size and bytes.length checks; in src/xrpc/router.ts at
lines 1656-1668, reject oversized bodies before request.arrayBuffer() using a
bounded stream/body-limit path or the app-level bodyLimit, while retaining
existing size checks.

In `@src/routes/og.tsx`:
- Around line 383-403: Update the totalRow count query to inner join hive_book
using the same hive_book_author.hiveId-to-hive_book.id relationship as the
avgRow and books queries, while preserving the existing author filter and count
behavior.

In `@src/sqlite-kv.ts`:
- Around line 140-146: Update incrementalVacuumKv to coerce pages to an integer
before interpolating it into the PRAGMA statement, and add an optional log
callback following the existing vacuumKvIfBloated pattern. Invoke the callback
from the catch with the reclaim error while preserving the current no-op
behavior for unsupported auto_vacuum modes.

In `@src/sqlite-kv.vacuum.test.ts`:
- Around line 64-66: Add a test for vacuumKvIfBloated that forces the database
exec/VACUUM operation to throw, then asserts the error is swallowed and noop
receives the "kv VACUUM failed" log message. Keep the existing empty-database
test unchanged and use the current test helpers or mocking approach.

In `@src/utils/enrichQueue.ts`:
- Around line 278-290: Update the exhausted-count query in the enrich queue
metrics flow to count rows in hive_book where enrichFailedAt is set, rather than
deleted enrich_queue rows; if the metric represents active cooldowns, apply the
established cooldown cutoff. Preserve the existing enrichQueueDepth label update
and add a regression test that exhausts a book and verifies a nonzero exhausted
metric.

In `@src/utils/ftsQuery.ts`:
- Around line 24-26: Update the comment above the cleaned assignment to describe
only collapsing internal whitespace and trimming surrounding whitespace; remove
the claim that FTS5 token-separator characters are stripped from the ends, while
leaving the input.replace behavior unchanged.

In `@src/utils/ogCache.test.ts`:
- Around line 92-101: Add a symmetric test beside the existing “cache write
fails” case in the test covering cachedOgRender: configure the storage’s get
operation to reject, invoke cachedOgRender with a renderer returning known
bytes, and assert the image is still returned with the expected byte length.
Keep the test focused on the read-failure fallback path.

In `@src/utils/ogCache.ts`:
- Around line 58-72: Make swallowed KV failures observable: in
src/utils/ogCache.ts lines 58-72, increment the existing failure counter from
both kv.get and kv.set catch paths while preserving best-effort behavior. In
src/sqlite-kv.ts lines 140-146, update incrementalVacuumKv to accept an optional
log callback and invoke it with the caught error before returning, so
maintenance failures are recorded without failing the caller.
- Around line 85-92: Update publishOgCacheStats to retain the cheap row count
but stop calculating ogCacheBytes with sum(length(value)) from og_cache. Use the
existing byte-accounting mechanism or metadata maintained during cache writes to
populate the byte metric without scanning cached payloads or synchronously
reading every row.

In `@src/utils/personalLibrary.ts`:
- Around line 84-91: Update streamPersonalBook and its download consumers to
evaluate If-None-Match before opening or streaming the ebook; when it matches
contentHash, return a 304 response containing the existing ETag and
Cache-Control headers, otherwise preserve the current streaming response.

In `@src/workers/og-render/client.ts`:
- Around line 64-67: Change the shedding log in the queue-backpressure path to
use logger.warn instead of logger.error, while preserving the existing
ogRenderShedTotal increment and log context. Leave the genuine og_render_timeout
error logging unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 63fad0d2-a88c-425a-9da6-39ca2f6acdfb

📥 Commits

Reviewing files that changed from the base of the PR and between 80a700f and 2248fe6.

📒 Files selected for processing (52)
  • AGENTS.md
  • Dockerfile
  • bunfig.toml
  • compose.yaml
  • package.json
  • server/cluster.ts
  • server/plugins/otel-sdk.ts
  • server/worker-exit.test.ts
  • server/worker-exit.ts
  • src/app.ts
  • src/auth/pds-outage.test.ts
  • src/auth/refresh-lock.test.ts
  • src/auth/refresh-lock.ts
  • src/auth/restore-guard.test.ts
  • src/auth/restore-guard.ts
  • src/auth/storage.ts
  • src/context.ts
  • src/db.authors.test.ts
  • src/db.ts
  • src/env.ts
  • src/metrics.ts
  • src/middleware/otel-middleware.ts
  • src/middleware/wide-event.ts
  • src/pages/authorBooks.tsx
  • src/pages/authorDirectory.tsx
  • src/pages/bookInfo.tsx
  • src/routes/debug.ts
  • src/routes/lib.ts
  • src/routes/library.tsx
  • src/routes/og.tsx
  • src/scrapers/goodreads.ts
  • src/scrapers/google.ts
  • src/scrapers/index.ts
  • src/scrapers/isbndb.ts
  • src/sqlite-kv.ts
  • src/sqlite-kv.vacuum.test.ts
  • src/types.ts
  • src/utils/authorMatching.test.ts
  • src/utils/authorMatching.ts
  • src/utils/enrichQueue.test.ts
  • src/utils/enrichQueue.ts
  • src/utils/ftsQuery.test.ts
  • src/utils/ftsQuery.ts
  • src/utils/imageProxy.ts
  • src/utils/ogCache.test.ts
  • src/utils/ogCache.ts
  • src/utils/personalLibrary.ts
  • src/workers/import/context.ts
  • src/workers/ingester-worker.ts
  • src/workers/og-render/client.ts
  • src/workers/open-observe-worker.ts
  • src/xrpc/router.ts
💤 Files with no reviewable changes (4)
  • src/scrapers/index.ts
  • src/utils/authorMatching.ts
  • src/scrapers/isbndb.ts
  • src/scrapers/google.ts

Comment thread AGENTS.md Outdated
Comment thread compose.yaml
Comment on lines +97 to +101
# Must be container-reachable: inside this container `localhost` is the
# app itself, not OpenObserve. Both the logs transport and the OTLP trace
# exporter derive their endpoints from this, so a wrong value silently
# disables observability rather than failing loudly.
OPEN_OBSERVE_URL: "http://openobserve:5080"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '30,115p' compose.yaml

for file in src/workers/open-observe-worker.ts server/plugins/otel-sdk.ts src/logger/index.ts; do
  if [ -f "$file" ]; then
    printf '\n--- %s ---\n' "$file"
    rg -n -C 8 'OPEN_OBSERVE_URL|ECONNREFUSED|connection refused|retry|disable|fetch|OTLP' "$file" || true
  fi
done

Repository: nperez0111/bookhive

Length of output: 9227


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- compose services relevant ---'
python3 - <<'PY'
import yaml
from pathlib import Path
p=Path('compose.yaml')
data=yaml.safe_load(p.read_text())
for name in ['server','openobserve','otel-collector']:
    svc=data.get('services',{}).get(name,{})
    print(name, {
        'depends_on': svc.get('depends_on'),
        'healthcheck': svc.get('healthcheck'),
        'ports': svc.get('ports'),
    })
PY

printf '\n--- openobserve worker flush tail ---\n'
sed -n '85,140p' src/workers/open-observe-worker.ts

printf '\n--- otel plugin lifecycle tail ---\n'
sed -n '20,120p' server/plugins/otel-sdk.ts

printf '\n--- logger setup context ---\n'
sed -n '1,80p' src/logger/index.ts

Repository: nperez0111/bookhive

Length of output: 308


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- compose services relevant ---'
python3 - <<'PY'
import json
from pathlib import Path

path = Path('compose.yaml')
text = path.read_text().splitlines()
# extract relevant service blocks as YAML is not available; simple state machine
state = []
for i,line in enumerate(text,1):
    if line.startswith('  server:') or line.startswith('  openobserve:') or line.startswith('  otel-collector:'):
        state.append([i, line.strip().rstrip(':'), {}])
    elif len(state):
        k=line.strip().split(':', 1)[0]
        if k in ('depends_on','healthcheck') and state[-1][1] in ('server','openobserve','otel-collector'):
            state[-1][2][k]=True
        elif k and k[0] != ',' and k[0] not in '-#':
            if state[-1][1] == 'otel-collector' and state[-1][2] == {}:
                # avoid config keys after depends_on line if parsed inline
                pass
for item in state:
    if not item[2]:
        print(item[1], {'depends_on': None, 'healthcheck': None})
    elif item[2].get('healthcheck'):
        print(item[1], {'depends_on': item[2].get('depends_on'), 'healthcheck': True})
    else:
        print(item[1], {'depends_on': item[2].get('depends_on'), 'healthcheck': False})
PY

printf '\n--- openobserve worker flush tail ---\n'
sed -n '85,140p' src/workers/open-observe-worker.ts

printf '\n--- otel plugin lifecycle tail ---\n'
sed -n '20,120p' server/plugins/otel-sdk.ts

printf '\n--- logger setup context ---\n'
sed -n '1,80p' src/logger/index.ts

Repository: nperez0111/bookhive

Length of output: 4942


Tie server startup to OpenObserve readiness.

server targets http://openobserve:5080, but openobserve has no healthcheck and server does not depend on it; otel-collector uses dependent so it can start before server and openobserve are both running. Add a healthcheck to openobserve:latest and make server depend on openobserve.service_healthy, or use an explicit startup health-check wrapper, so logs and traces are not configured against a service that is too early to accept connections.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@compose.yaml` around lines 97 - 101, In compose.yaml, add a healthcheck to
the openobserve service and update the server service’s dependency condition to
require openobserve.service_healthy before startup. Preserve the existing
OPEN_OBSERVE_URL and ensure the dependency uses the healthcheck result rather
than simple container startup.

Comment thread server/cluster.ts Outdated
Comment thread src/app.ts Outdated
Comment thread src/auth/restore-guard.ts
Comment thread src/utils/ogCache.test.ts Outdated
Comment on lines +92 to +101
it("still returns the image when the cache write fails", async () => {
const broken = createStorage({ driver: memoryDriver() });
broken.set = async () => {
throw new Error("disk full");
};
const key = ogCacheKey("book", { id: "bk_nowrite" });

const result = await cachedOgRender(broken, key, 60, async () => bytesOf(12));
expect(result.byteLength).toBe(12);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add the symmetric read-failure case.

This case covers a failing kv.set. cachedOgRender also swallows a failing kv.get at line 58 of src/utils/ogCache.ts, and no case covers that path. A read fault is the more likely production fault, because it happens on every request rather than only on a miss.

💚 Proposed test
   it("still returns the image when the cache write fails", async () => {
     const broken = createStorage({ driver: memoryDriver() });
     broken.set = async () => {
       throw new Error("disk full");
     };
     const key = ogCacheKey("book", { id: "bk_nowrite" });
 
     const result = await cachedOgRender(broken, key, 60, async () => bytesOf(12));
     expect(result.byteLength).toBe(12);
   });
+
+  it("still returns the image when the cache read fails", async () => {
+    const broken = createStorage({ driver: memoryDriver() });
+    broken.get = async () => {
+      throw new Error("no such table: og_cache");
+    };
+    const key = ogCacheKey("book", { id: "bk_noread" });
+
+    const result = await cachedOgRender(broken, key, 60, async () => bytesOf(9));
+    expect(result.byteLength).toBe(9);
+  });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it("still returns the image when the cache write fails", async () => {
const broken = createStorage({ driver: memoryDriver() });
broken.set = async () => {
throw new Error("disk full");
};
const key = ogCacheKey("book", { id: "bk_nowrite" });
const result = await cachedOgRender(broken, key, 60, async () => bytesOf(12));
expect(result.byteLength).toBe(12);
});
it("still returns the image when the cache write fails", async () => {
const broken = createStorage({ driver: memoryDriver() });
broken.set = async () => {
throw new Error("disk full");
};
const key = ogCacheKey("book", { id: "bk_nowrite" });
const result = await cachedOgRender(broken, key, 60, async () => bytesOf(12));
expect(result.byteLength).toBe(12);
});
it("still returns the image when the cache read fails", async () => {
const broken = createStorage({ driver: memoryDriver() });
broken.get = async () => {
throw new Error("no such table: og_cache");
};
const key = ogCacheKey("book", { id: "bk_noread" });
const result = await cachedOgRender(broken, key, 60, async () => bytesOf(9));
expect(result.byteLength).toBe(9);
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/ogCache.test.ts` around lines 92 - 101, Add a symmetric test beside
the existing “cache write fails” case in the test covering cachedOgRender:
configure the storage’s get operation to reject, invoke cachedOgRender with a
renderer returning known bytes, and assert the image is still returned with the
expected byte length. Keep the test focused on the read-failure fallback path.

Comment thread src/utils/ogCache.ts Outdated
Comment thread src/utils/ogCache.ts Outdated
Comment on lines +85 to +92
export async function publishOgCacheStats(kvDb: KvDb): Promise<void> {
const row = await sql<{
n: number;
b: number | null;
}>`SELECT count(*) AS n, sum(length(value)) AS b FROM og_cache`.execute(kvDb);
ogCacheEntries.set(Number(row.rows[0]?.n ?? 0));
ogCacheBytes.set(Number(row.rows[0]?.b ?? 0));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check whether the dbstat virtual table is available in the Bun SQLite build,
# and confirm how ogCacheBytes / ogCacheEntries are declared.

set -uo pipefail

echo "=== metrics declarations ==="
fd -t f 'metrics.ts' src | xargs rg -n -C3 'ogCacheBytes|ogCacheEntries|ogRenderShedTotal'

echo
echo "=== bun version ==="
bun --version 2>/dev/null || echo "bun not available in sandbox"

echo
echo "=== dbstat availability ==="
cat > /tmp/dbstat-probe.ts <<'EOF'
import { Database } from "bun:sqlite";
const db = new Database(":memory:");
db.exec("CREATE TABLE og_cache (id TEXT PRIMARY KEY, value TEXT NOT NULL)");
db.exec("INSERT INTO og_cache VALUES ('a', 'b')");
try {
  console.log("dbstat OK:", db.query("SELECT sum(pgsize) AS b FROM dbstat WHERE name='og_cache'").get());
} catch (e) {
  console.log("dbstat UNAVAILABLE:", (e as Error).message);
}
EOF
bun /tmp/dbstat-probe.ts 2>&1 || echo "probe could not run"

Repository: nperez0111/bookhive

Length of output: 969


🏁 Script executed:

#!/bin/bash
set -uo pipefail

echo "=== locate relevant files ==="
git ls-files | rg '(^|/)src/(utils/ogCache\.ts|context\.ts|sqlite-kv\.ts|.*metrics.*)$' || true

echo
echo "=== ogCache declarations and publishOgCacheStats ==="
sed -n '1,140p' src/utils/ogCache.ts

echo
echo "=== publishOgCacheStats call sites ==="
rg -n -C 6 'publishOgCacheStats' src || true

echo
echo "=== sqlite helper/table definitions ==="
rg -n -C 4 'og_cache|dbstat|page_count|page_size' src || true

echo
echo "=== package/runtime config hints ==="
for f in package.json bun.lockb bun.lockb.bak; do
  if [ -f "$f" ]; then
    echo "--- $f"
    if [ "$f" = package.json ]; then cat "$f" | sed -n '1,220p'; else ls -l "$f"; fi
  fi
done

echo
echo "=== read-only SQLite probe using node-sqlite3 or better-sqlite3 if available, or sqlite3 CLI ==="
node - <<'JS'
const sqlite3 = require('better-sqlite3');
const fs = require('fs');
const db = new sqlite3(':memory:');
db.exec(`CREATE TABLE og_cache (id TEXT PRIMARY KEY, value TEXT NOT NULL)`);
const buf = Buffer.alloc(102400, 0x42);
db.exec(`INSERT INTO og_cache VALUES ('x', ${JSON.stringify(buf.toString('base64'))})`);
console.log(hasDbstat(db));
db.close();
function hasDbstat(db) {
  const stmt = db.prepare('SELECT sum(pgsize) AS b FROM dbstat WHERE name=?');
  try { return stmt.get('og_cache'); } catch (e) { return e && e.message; }
}
JS
sqlite3 :memory: 'CREATE TABLE og_cache(id TEXT PRIMARY KEY, value TEXT NOT NULL); INSERT INTO og_cache VALUES(\'a\',\'b\'); SELECT sum(pgsize) AS b FROM dbstat WHERE name = \'og_cache\';' 2>&1 || true

Repository: nperez0111/bookhive

Length of output: 17125


🌐 Web query:

Bun sqlite database module dbstat virtual table SQLITE_ENABLE_DBSTAT_VTAB

💡 Result:

The bun:sqlite module in Bun includes the dbstat virtual table, as it is compiled with the SQLITE_ENABLE_DBSTAT_VTAB option enabled [1][2]. The dbstat virtual table is a read-only eponymous virtual table used to retrieve information about disk space usage in an SQLite database [3]. Because it is eponymous, you do not need to execute CREATE VIRTUAL TABLE to instantiate it; you can query the dbstat module directly as if it were a table [3]. While Bun's bun:sqlite documentation mentions support for CREATE VIRTUAL TABLE [4][5], general support for custom virtual tables (where a JavaScript generator yields rows to SQLite) is a separate, requested feature currently tracked in the Bun issue tracker [6]. However, the built-in dbstat virtual table provided by the underlying SQLite library is available for use within Bun [1][2]. To use it, you can execute queries against the dbstat table directly in your SQL: sql SELECT * FROM dbstat; This will return information such as the table name, page number, page type, and page size for the database file [3].

Citations:


🌐 Web query:

Bun sqlite synchronous query documentation

💡 Result:

The bun:sqlite module in Bun provides a synchronous API for interacting with SQLite databases [1][2]. There is no native asynchronous API for bun:sqlite; all database operations executed through this module are blocking and run synchronously on the main thread [1][3][4]. Key points regarding the synchronous nature of Bun's SQLite implementation: API Design: The API is designed to be high-performance and synchronous, inspired by better-sqlite3 [1][2]. Blocking Behavior: Because the API is synchronous, long-running queries will block the event loop, which can prevent other tasks (such as UI updates or handling other requests) from executing during that time [3][4]. Workarounds: Users who require non-blocking behavior often move SQLite operations to separate worker threads in user space to offload the blocking I/O from the main event loop [3][4]. Methods: Standard methods such as.query,.run,.get, and.all all operate synchronously [5][6][2]. For further details, you can consult the official Bun documentation for the bun:sqlite module [1][2].

Citations:


Don’t sum length(value) to compute og_cache bytes

sum(length(value)) reads every cached OG row and value on the 15-minute timer. This blocks the primary worker because bun:sqlite is synchronous, and it drags the cached image payloads through the SQLite page cache while this PR is focused on page-cache pressure.

Keep the cheap row count. Use byte accounting instead of a length(value) scan.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/ogCache.ts` around lines 85 - 92, Update publishOgCacheStats to
retain the cheap row count but stop calculating ogCacheBytes with
sum(length(value)) from og_cache. Use the existing byte-accounting mechanism or
metadata maintained during cache writes to populate the byte metric without
scanning cached payloads or synchronously reading every row.

Comment thread src/utils/personalLibrary.ts Outdated
Comment thread src/workers/og-render/client.ts Outdated
nperez0111 and others added 5 commits August 2, 2026 22:53
Verified against the codebase and fixed:
- /sitemap.xml listed before mainRouter (actually after)
- Middleware order missing CORP override, Cache-Control default, registerMetrics
- google.ts/isbndb.ts claimed deleted (exist but unused)
- personal_book, personal_shelf, personal_shelf_item missing from DB table list
- Build commands missing bunx --bun wrappers and src dir arg
- bookMetadata/, bookMeta.ts, xml.ts, buildUrl.ts undocumented

829 → 454 lines: removed incident postmortem narratives, production
measurements, and implementation details derivable from the code while
keeping all structural info, constraints, and "don't do X" rules.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
These files exist on disk but are not imported anywhere.
Kept for potential future use; imageProxy allowlist still
references images.isbndb.com for historical hive_book rows.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…r VACUUM

Two of these were real bugs, one of them mine.

`streamPersonalBook` set a strong ETag but nothing ever read `If-None-Match`.
Excluding `/library/books/*` and `/opds/books/*` from hono's `etag()` removed
the middleware that turns a validator into a 304, and the compensating header
does not restore it — so every e-reader re-downloaded every book on every sync.
It now answers the conditional request before opening the file (handling `W/`,
comma lists and `*`), and both routes return 304.

`getBreaker` enforced MAX_BREAKERS by evicting the LRU *closed* breaker. During
a mass PDS outage nothing is closed, so nothing was evicted and the map grew one
entry per host forever — an unbounded leak inside the leak guard, reachable in
exactly the scenario the module exists for. Falls back to the overall LRU. The
regression test fails against the old logic.

`hive_book_fts` is external-content keyed by `hive_book`'s implicit rowid (`id`
is TEXT), and SQLite documents that VACUUM may renumber those. Measured on 3.51
it does not, but FTS5's `'integrity-check'` cannot detect this desync — verified
against a deliberately shifted content table — so the failure would be silent
wrong search results with nothing to alarm on. A `'rebuild'` after VACUUM costs
~1s at 356k rows, once, inside the startup barrier VACUUM already runs in.

Also: `bodyLimit` on `/library/upload`, because `c.req.formData()` materialises
the whole multipart body before the `file.size` check can fire; `/import` is now
excluded from etag by prefix rather than only by mount order, since a reorder
would hang the SSE stream forever; the enrich `exhausted` gauge counts
`hive_book.enrichFailedAt` in the cooldown instead of queue rows at MAX_ATTEMPTS
(deleted as they exhaust, so it read 0 by construction); `firstAuthor` in
bookInfo is trimmed to match what mig 020's trigger stores; the OG author count
joins `hive_book` like its sibling queries; refetch searches swallow their own
failures so one Goodreads timeout no longer aborts a whole library re-sync;
cluster.ts reuses one classification instead of re-deriving `likely_oom`; the
otel fallback span name drops the raw path that would blow up cardinality.

Skipped, with reasons: an openobserve healthcheck (the image is distroless — no
sh, no curl, no wget, verified on the host — so any check would report
permanently unhealthy) and the matching `depends_on` (gating app startup on its
telemetry backend makes observability a SPOF); hashing the DID in the breaker
fallback key (does not change cardinality, which was the real concern and is
fixed above, and DIDs are public identifiers); zValidator on the multipart
upload (bodyLimit addresses the actual allocation risk; the field checks are
explicit already).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
They were restored in fdabf65, so AGENTS.md claiming they were deleted is now
the inaccuracy. Both are listed in the file table and described as tracked but
not wired up: nothing imports them and `findBookDetails` has no fallback branch
that reaches them, so Goodreads is still the only scraper that runs. Said
explicitly, because "there is an ISBNdb scraper in the tree" reads as "Goodreads
failures degrade to ISBNdb" and they do not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Timed the whole migration path against a backup of the production database
(356,675 books, 1.62 GB) on the host, since none of it had ever been run at that
scale:

  019 FTS create + backfill      3.2s
  020 author table + backfill    4.1s
  020 indexes                    0.4s
  021 enrich columns             ~0s
  VACUUM                        22.3s
  FTS rebuild after VACUUM       3.6s
                                -----
                                33.6s   (healthcheck barrier allows ~150s)

The barrier is fine. But VACUUM is two thirds of it and reclaims **nothing**:
`freelist_count` on that file is 0. The main DB is essentially append-only — the
ingester and enrichment insert, almost nothing deletes — so there are no free
pages to give back. That was 22s of startup on every deploy shipping a
migration, for zero bytes, and it is also the only reason the FTS rebuild is
needed at all.

Now gated on the same freelist ratio as `vacuumKvIfBloated`, so it stays correct
for a future migration that does free a lot, and costs a PRAGMA read otherwise.
The KV VACUUM is untouched and still unconditional — that file is the
delete-heavy one and went 1.94 GB to 34.7 MB.

Also verified on the real data that the post-VACUUM join still resolves
correctly (a 'gatsby' match returns The Great Gatsby, not some neighbouring
row), so the rowid-stability assumption holds on 3.45 as well as 3.51.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/middleware/otel-middleware.ts (1)

42-53: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Close spans from a finally path.

If next() throws, the post-processing block is skipped and span.end() at line 71 is never reached, so error spans can remain unfinished and miss route/status enrichment. Move span finalization into a finally block while keeping the exception recording from catch and rethrowing.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/middleware/otel-middleware.ts` around lines 42 - 53, Update the
middleware’s span lifecycle so the route/status enrichment and span.end()
execute from a finally block even when next() throws. Preserve the catch block’s
exception recording and rethrow the original error after recording it.
src/utils/personalLibrary.ts (2)

79-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract a shared response helper for PersonalBookDownload results. Both download routes translate a PersonalBookDownload result into an HTTP response with the identical three-branch pattern (missing → 404, notModified → 304, else stream). Centralizing this in src/utils/personalLibrary.ts prevents the two routes from drifting if this response logic changes later (for example, adding a header to the 304 branch).

  • src/utils/personalLibrary.ts#L79-L126: add an exported helper, e.g. respondToDownload(c, download, notFoundBody), that implements the shared branch logic and returns the appropriate c.body(...) call.
  • src/routes/library.tsx#L247-L257: replace the manual !download/notModified/stream branches with a call to the new shared helper.
  • src/routes/opds.ts#L528-L535: replace the manual !download/notModified/stream branches with a call to the new shared helper, passing its own not-found body.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/personalLibrary.ts` around lines 79 - 126, In
src/utils/personalLibrary.ts lines 79-126, add an exported helper such as
respondToDownload that handles missing downloads with the supplied not-found
body, notModified results with a 304 response and headers, and stream results
with their headers. In src/routes/library.tsx lines 247-257 and
src/routes/opds.ts lines 528-535, replace each duplicated three-branch response
flow with the shared helper, passing the route-specific not-found body.

10-17: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Add the XRPC personal-book download route for GET /books/:hash/download.

src/routes/library.tsx and src/routes/opds.ts both use streamPersonalBook(...) with the new PersonalBookDownload contract, including if-none-match/304 handling. src/xrpc/router.ts does not define this endpoint, so XRPC clients can request these books while the route is still missing.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/personalLibrary.ts` around lines 10 - 17, Add the missing XRPC GET
endpoint for /books/:hash/download in the router, using the PersonalBookDownload
contract and streamPersonalBook(...) consistently with the implementations in
library.tsx and opds.ts. Preserve their if-none-match handling, including
returning 304 when the resource is unchanged, and return the streamed
personal-book response for valid requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@AGENTS.md`:
- Line 139: Update the GET /:hiveId documentation to remove the claim that
?force-refresh=true performs inline enrichment; describe forced refresh as
queued enrichment instead, consistent with the existing route contract and
enqueueEnrichment usage.

In `@server/cluster.ts`:
- Around line 45-65: Clear the worker index entry in lastMemory immediately
after logWorkerExit captures the exit event, preventing samples from being
reused by a restarted process. Update the restart message near the exit handling
around logWorkerExit to read anon_kb from the captured exit event rather than
directly from lastMemory.

In `@src/context.ts`:
- Around line 161-165: Update the catch block around the hive_book_fts rebuild
after VACUUM to rethrow err immediately after logger.error, ensuring startup
aborts when rebuilding fails and the worker cannot serve with a stale
external-content index.
- Around line 222-225: Update the DELETE flow in the surrounding context method
to await the asynchronous DELETE operation before calling
incrementalVacuumKv(kvSqlite). Invoke incrementalVacuumKv only after the DELETE
completes successfully, preserving the existing vacuum behavior and avoiding
execution when the DELETE fails.

In `@src/pages/bookInfo.tsx`:
- Around line 144-149: Normalize all tab-separated author segments once near
firstAuthor into a trimmed author array, then derive firstAuthor from that array
and use the same array when rendering author links. Preserve the canonical
tab-separated source format and ensure every generated link uses the trimmed
author value.

In `@src/sqlite-kv.ts`:
- Around line 143-154: Make incremental-vacuum errors observable from the
production timer by updating the incrementalVacuumKv call in the context timer
to pass the available application logger as its log callback. Preserve the
existing optional callback behavior and error message in incrementalVacuumKv,
ensuring SQLITE_BUSY and disk failures are emitted through the production
logger.

---

Outside diff comments:
In `@src/middleware/otel-middleware.ts`:
- Around line 42-53: Update the middleware’s span lifecycle so the route/status
enrichment and span.end() execute from a finally block even when next() throws.
Preserve the catch block’s exception recording and rethrow the original error
after recording it.

In `@src/utils/personalLibrary.ts`:
- Around line 79-126: In src/utils/personalLibrary.ts lines 79-126, add an
exported helper such as respondToDownload that handles missing downloads with
the supplied not-found body, notModified results with a 304 response and
headers, and stream results with their headers. In src/routes/library.tsx lines
247-257 and src/routes/opds.ts lines 528-535, replace each duplicated
three-branch response flow with the shared helper, passing the route-specific
not-found body.
- Around line 10-17: Add the missing XRPC GET endpoint for /books/:hash/download
in the router, using the PersonalBookDownload contract and
streamPersonalBook(...) consistently with the implementations in library.tsx and
opds.ts. Preserve their if-none-match handling, including returning 304 when the
resource is unchanged, and return the streamed personal-book response for valid
requests.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 749ef9aa-573e-4046-a60a-a6956bf35c8e

📥 Commits

Reviewing files that changed from the base of the PR and between 2248fe6 and 81e0029.

📒 Files selected for processing (22)
  • AGENTS.md
  • compose.yaml
  • server/cluster.ts
  • src/app.ts
  • src/auth/restore-guard.test.ts
  • src/auth/restore-guard.ts
  • src/context.ts
  • src/metrics.ts
  • src/middleware/otel-middleware.ts
  • src/pages/bookInfo.tsx
  • src/routes/lib.ts
  • src/routes/library.tsx
  • src/routes/og.tsx
  • src/routes/opds.ts
  • src/sqlite-kv.ts
  • src/sqlite-kv.vacuum.test.ts
  • src/utils/enrichQueue.test.ts
  • src/utils/enrichQueue.ts
  • src/utils/ftsQuery.ts
  • src/utils/personalLibrary.test.ts
  • src/utils/personalLibrary.ts
  • src/workers/og-render/client.ts
💤 Files with no reviewable changes (1)
  • src/metrics.ts

Comment thread AGENTS.md
- DELETE `/:hiveId` → delete book record from PDS + DB
- POST `/` → add/update book (zValidator form incl. `bookProgress`); per-DID `book_lock` KV, 429 if locked
- GET `/:hiveId/comments` → `src/pages/comments.tsx` — comments/reviews section
- GET `/:hiveId` → `src/pages/bookInfo.tsx` — book detail. `hiveId` must match `^bk_[A-Za-z0-9]+$`. Stale books (>30d) queued for enrichment; `?force-refresh=true` enriches inline with 15s ceiling

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Remove the inline-enrichment claim from the route documentation.

Line 55 says enrichment is queued and never inline. This line says ?force-refresh=true enriches inline. The conflicting guidance can reintroduce request-bound scraper work. Document forced refresh as a queued operation, or update both sections and the route implementation to the same contract.

As per coding guidelines, routes must not perform inline enrichment when queued enrichment is appropriate; use enqueueEnrichment or enqueueEnrichmentBatch instead.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@AGENTS.md` at line 139, Update the GET /:hiveId documentation to remove the
claim that ?force-refresh=true performs inline enrichment; describe forced
refresh as queued enrichment instead, consistent with the existing route
contract and enqueueEnrichment usage.

Source: Coding guidelines

Comment thread server/cluster.ts
Comment thread src/context.ts Outdated
Comment on lines +161 to +165
try {
sqlite.exec(`INSERT INTO hive_book_fts(hive_book_fts) VALUES('rebuild')`);
} catch (err) {
logger.error({ err }, "hive_book_fts rebuild after VACUUM failed");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Stop startup when the FTS rebuild fails.

If the rebuild fails after VACUUM, this code logs the error and continues. The external-content index can then reference stale hive_book rowids and return incorrect books from search. Rethrow the error after logging it so the worker does not serve with an invalid index.

As per coding guidelines, “Rebuild hive_book_fts after the startup VACUUM, on the primary worker, because VACUUM may change implicit rowids used by the external-content index.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/context.ts` around lines 161 - 165, Update the catch block around the
hive_book_fts rebuild after VACUUM to rethrow err immediately after
logger.error, ensuring startup aborts when rebuilding fails and the worker
cannot serve with a stale external-content index.

Source: Coding guidelines

Comment thread src/context.ts Outdated
Comment thread src/pages/bookInfo.tsx Outdated
Comment thread src/sqlite-kv.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@AGENTS.md`:
- Around line 314-320: The documentation in AGENTS.md incorrectly says the
main-database VACUUM uses the same freelist ratio as the KV. Update that phrase
to state it is gated on the main database’s freelist ratio, while preserving the
clarification that the KV VACUUM remains unconditional and the main-database
VACUUM must stay conditional.

In `@src/sqlite-kv.ts`:
- Around line 158-160: Remove the unused exported readPragma helper from
sqlite-kv.ts. Do not alter the currently used SQL-building helpers or make
changes based solely on the documented src/context.ts callers.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e214ea6b-3f21-4354-a132-4c45b9b05279

📥 Commits

Reviewing files that changed from the base of the PR and between 81e0029 and d66d4fc.

📒 Files selected for processing (3)
  • AGENTS.md
  • src/context.ts
  • src/sqlite-kv.ts

Comment thread AGENTS.md Outdated
Comment thread src/sqlite-kv.ts
Comment on lines +158 to +160
export function readPragma(sqlite: DatabaseSync, name: string): number {
const row = sqlite.query(`PRAGMA ${name}`).get() as Record<string, number> | null;
return row ? (Object.values(row)[0] ?? 0) : 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

🧩 Analysis chain

🏁 Script executed:

rg -n -C 2 '\breadPragma\s*\(' --glob '*.ts'

Repository: nperez0111/bookhive

Length of output: 157


🏁 Script executed:

#!/bin/bash
set -u

echo "--- tracked files matching sqlite-kv.ts / context.ts ---"
git ls-files | rg '(^|/)(sqlite-kv|context)\.ts$|sqlite'

echo
echo "--- locate readPragma text in tracked TS files ---"
rg -n -C 2 'readPragma|PRAGMA' --glob '*.ts' || true

echo
echo "--- file snippets if sqlite-kv exists ---"
for f in $(git ls-files | rg '(^|/)sqlite-kv\.ts$'); do
  echo "### $f"
  wc -l "$f"
  sed -n '1,220p' "$f" | cat -n
done

Repository: nperez0111/bookhive

Length of output: 412


🏁 Script executed:

#!/bin/bash
set -u

echo "--- src/sqlite-kv.ts line count and relevant section ---"
wc -l src/sqlite-kv.ts
sed -n '130,180p' src/sqlite-kv.ts | cat -n

echo
echo "--- src/context.ts relevant sqlite-kv imports/calls section ---"
wc -l src/context.ts
sed -n '1,220p' src/context.ts | cat -n

echo
echo "--- all tracked occurrences of identifiers in src/sqlite-kv.ts ---"
python3 - <<'PY'
import re, pathlib
p=pathlib.Path('src/sqlite-kv.ts')
text=p.read_text()
print("contains readPragma:", "readPragma" in text)
for name in ["readPragma","writePragma","PRAGMA","page_count","freelist_count","auto_vacuum"]:
    print(f"name={name}: count={text.count(name)}")
PY

Repository: nperez0111/bookhive

Length of output: 13635


Remove the unused readPragma helper before relying on the current callers.

The helper is exported but has no current callsite, so the documented src/context.ts callers do not justify it. Keep unused exported SQL-building helpers unless they are reused.

🧰 Tools
🪛 ast-grep (0.45.0)

[error] 158-158: Avoid SQL injection
Context: sqlite.query(PRAGMA ${name})
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').

(sql-injection-typescript)

🪛 OpenGrep (1.26.0)

[ERROR] 159-159: SQL query built via string concatenation or template literal passed to query()/execute(). Use parameterized queries instead.

(coderabbit.sql-injection.raw-query-concat-js)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sqlite-kv.ts` around lines 158 - 160, Remove the unused exported
readPragma helper from sqlite-kv.ts. Do not alter the currently used
SQL-building helpers or make changes based solely on the documented
src/context.ts callers.

Source: Linters/SAST tools

The otel middleware ended its span *after* the try/catch, so a throwing `next()`
rethrew straight past `span.end()`. Those spans were never exported and stayed
pinned in the SDK — losing the trace for exactly the requests worth tracing.
Moved to `finally`, with the route/status enrichment guarded so a telemetry
throw can never replace the request's real error.

The author trim only covered `firstAuthor`; the rendered "by ..." list still
built `/authors/<name>` from raw tab-separated segments, so any padded name
linked somewhere that matches nothing in `hive_book_author`. Split once into a
trimmed array and derive both from it.

The 15-minute KV sweep fired `incrementalVacuumKv` alongside an un-awaited
DELETE, so the comment claiming it "hands back the pages that DELETE just freed"
was false — it only ever reclaimed the previous cycle's. Awaited, and it now
gets the logger it always accepted, so a SQLITE_BUSY or full disk on that timer
is visible instead of swallowed.

`lastMemory` is cleared once the exit event has read it. Worker indices are
reused by the restarted process and the sampler tick is up to 15s away, so a
worker dying inside that window reported its predecessor's memory — the most
misleading possible number during a crash loop. The restart line now reads
`anon_kb` off the captured event rather than re-reading the map.

Corrected two claims of my own while confirming the reviewer's: the KV VACUUM is
*not* unconditional (it skips when the ratio is low and auto_vacuum is already
INCREMENTAL — effectively one VACUUM when this first ships, bloat-driven after),
and the main-DB gate reads its own freelist, not the KV's.

Skipped:
- `?force-refresh=true` "enriches inline" — accurate as documented; the handler
  awaits `enrichBookWithDetailedData` under a 15s `withTimeout`.
- Removing exported `readPragma` — it is imported and called by src/context.ts;
  removing it breaks the build.
- Rethrowing a failed FTS rebuild — it only runs on the rare VACUUM, and the
  desync it guards against is one we measured does not occur. Crashing the
  primary worker there trades a hypothetical stale index for a certain outage:
  the supervisor gives up after 5 restarts in 60s.
- `respondToDownload` helper and a new XRPC download endpoint — the first is
  indirection for two call sites; the second is a new lexicon + endpoint, not a
  review fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@nperez0111
nperez0111 merged commit 3756f60 into main Aug 2, 2026
2 checks passed
@nperez0111
nperez0111 deleted the feat/bound-refetchbooks-fanout branch August 2, 2026 23:20
nperez0111 added a commit that referenced this pull request Aug 3, 2026
`fetchGoodreadsViaWaf` did two things behind one gate: a plain GET that succeeds
~98% of the time, and a WAF challenge solve that costs ~4 requests, a 1.3 MB
script, a Worker and proof-of-work. The circuit breaker was fed by solve outcomes
and gated the page fetch, so when AWS WAF stopped honouring our tokens it sat open
and refused the path that still worked.

Measured over one 6h production window: 8,606 `circuit_open` refusals across 6,840
distinct books, breaker open in 254 of 360 minutes, while the requests it did let
through succeeded 95.6% of the time. Worse, `enrich_queue` counted a refusal as an
attempt, so 2,854 books — 98% of everything the queue gave up on — were tombstoned
for 7 days apiece without a single request being sent on their behalf.

Split the two operations so they share no state, and delete the breaker rather
than adding a second one. The invariant is now structural:

  No book is ever failed without a request to Goodreads having been sent
  and answered.

`fetchGoodreadsViaWaf` has no early-return branch before the fetch — not a
threshold tuned so it rarely fires, no branch at all. A test drives 100
consecutive failures of every kind and asserts 100 requests went out.

What each thing the breaker protected does now:

- Cost of pointless solves: single-flight plus one attempt per token lifetime.
  `SOLVE_MIN_INTERVAL_MS = TOKEN_MAX_AGE_MS` is derived, not tuned — solving more
  often than a token lasts cannot produce anything we don't already have. Worst
  case 15 attempts/hour; the breaker, open 70% of the time, allowed 16.
- Request rate: `ENRICH_CONCURRENCY`/`DRAIN_INTERVAL_MS` already cap us at 36
  fetches/min whether Goodreads is healthy or dead. The breaker was a second
  limiter on an already-bounded path; all it changed was which books got destroyed.
- Memory (the 2026-08-01 OOM): tighter. At most one solver Worker per process
  instead of a pool of four plus 32 queued waiters, terminated on every path
  rather than retired after 50 solves, and page bodies no longer cross the Worker
  boundary at all. The worker now takes a challenge page and returns a token.

`enrich_queue` learns retry/defer/dead. `attempts` counts answers from Goodreads,
so only an answer spends one; a defer costs nothing and re-queues on a schedule
that doubles hourly to a 6h ceiling, because a flat retry would let a few hundred
stuck books eat most of the 36 fetches/min. Defers are bounded by a 7-day ceiling
from `enqueuedAt`, which survives re-enqueue. `book_not_found_upstream` — which
only the parser can assert, from `getBookByLegacyId` resolving to null — tombstones
on the first attempt instead of the fourth.

Note on why solving fails: it worked from this host until 2026-07-29 (5-30/day, no
token failures). Volume spiked ~20x on 07-30, token failures appeared the same day,
and there have been zero solve successes since 08-01 — predating #196/#197, so not
a regression. A/B on 2026-08-03 with identical code, 4/4 each: from Hetzner a
solved token comes back 202 + `x-amzn-waf-action: challenge`; from a residential IP
the same token clears the WAF (403 from the origin, no waf-action). Reputation, not
crypto — so the solver stays and keeps making one cheap attempt per token lifetime.

`CircuitBreaker` itself is unchanged and still used by `auth/restore-guard.ts`,
where refusing genuinely is cheaper for a waiting user than failing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant