Conversation
…ypes and richer context Refactor get_item_details to intercept each failure mode separately: - Network/transport errors (connect refused, timeout, DNS) - Non-2xx HTTP status codes (with response body) - JSON parse failures Populate lot (MediaLot) in all ImportFailedItem error paths so the media type is always visible in failure reports. Include the Audiobookshelf item.id and episode.id in error messages for cross-referencing. Fix commit_metadata failures to use ImportFailStep::DatabaseCommit instead of ItemDetailsFromSource.
WalkthroughAudiobookshelf importing now loads completion progress, filters podcast episodes to finished entries, fetches item details once per podcast, and reports lookup, validation, HTTP, parsing, and commit failures with item identifiers and processing metadata. ChangesAudiobookshelf podcast completion import
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Importer
participant AudiobookshelfAPI
participant Database
Importer->>AudiobookshelfAPI: request /me progress
AudiobookshelfAPI-->>Importer: completed media progress
Importer->>AudiobookshelfAPI: request podcast item details
AudiobookshelfAPI-->>Importer: podcast episodes
Importer->>Importer: filter completed episodes
Importer->>Database: commit matching metadata
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@crates/services/importer/audiobookshelf/src/lib.rs`:
- Around line 121-135: Update the InputTransformation failure messages in the
book import branches to include both the existing reason and the Audiobookshelf
record identifier item.id. Also update the episode database-commit failure near
the episode_id handling to include episode_id in its error message, preserving
the existing failure steps and identifiers.
- Around line 130-134: Reorder the fields in the affected ImportFailedItem
initializers to follow ascending source-line length, placing the shorter lot and
step fields before the longer error expressions. Apply this consistently to
every initializer in the importer, including the instances around the No ISBN
found path and the additional referenced locations, without changing values or
behavior.
- Around line 258-272: Update the response-body read in the Audiobookshelf
request flow to replace unwrap_or_default with error propagation using
with_context(...)? before the status check. Preserve the existing status
handling and JSON parsing behavior while reporting failures to read
response.text() with request URL context.
🪄 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: 88fb2da5-7e8b-4bcc-8eb0-bc3ab5b68bee
📒 Files selected for processing (1)
crates/services/importer/audiobookshelf/src/lib.rs
| return Err(ImportFailedItem { | ||
| identifier: title, | ||
| lot: Some(MediaLot::Book), | ||
| step: ImportFailStep::InputTransformation, | ||
| error: Some("No Google Books ID found".to_string()), | ||
| ..Default::default() | ||
| }); | ||
| } | ||
| }, | ||
| _ => { | ||
| return Err(ImportFailedItem { | ||
| identifier: title, | ||
| lot: Some(MediaLot::Book), | ||
| error: Some("No ISBN found".to_string()), | ||
| step: ImportFailStep::InputTransformation, | ||
| ..Default::default() | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include all source identifiers in failure messages.
The book failures at Lines 125 and 133 should include item.id. The episode database-commit failure at Line 186 should also include episode_id; otherwise failures cannot identify the exact Audiobookshelf record.
Also applies to: 184-189
🤖 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 `@crates/services/importer/audiobookshelf/src/lib.rs` around lines 121 - 135,
Update the InputTransformation failure messages in the book import branches to
include both the existing reason and the Audiobookshelf record identifier
item.id. Also update the episode database-commit failure near the episode_id
handling to include episode_id in its error message, preserving the existing
failure steps and identifiers.
| return Err(ImportFailedItem { | ||
| identifier: title, | ||
| lot: Some(MediaLot::Book), | ||
| error: Some("No ISBN found".to_string()), | ||
| step: ImportFailStep::InputTransformation, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Reorder struct fields by ascending line length.
The changed ImportFailedItem initializers should consistently sort their fields by source-line length; notably, move shorter lot/step fields before the longer error expressions. As per coding guidelines, Rust struct fields must be ordered by ascending line length.
Also applies to: 143-148, 160-169, 184-189, 213-220
🤖 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 `@crates/services/importer/audiobookshelf/src/lib.rs` around lines 130 - 134,
Reorder the fields in the affected ImportFailedItem initializers to follow
ascending source-line length, placing the shorter lot and step fields before the
longer error expressions. Apply this consistently to every initializer in the
importer, including the instances around the No ISBN found path and the
additional referenced locations, without changing values or behavior.
Source: Coding guidelines
| let request_url = format!("{url}/items/{id}"); | ||
| let response = client | ||
| .get(&request_url) | ||
| .query(&query) | ||
| .send() | ||
| .await? | ||
| .json::<audiobookshelf_models::Item>() | ||
| .await?; | ||
| .await | ||
| .with_context(|| format!("Network error requesting audiobookshelf API at {request_url}"))?; | ||
| let status = response.status(); | ||
| let body = response.text().await.unwrap_or_default(); | ||
| if !status.is_success() { | ||
| anyhow::bail!("Audiobookshelf returned HTTP {status} (url: {request_url}): {body}"); | ||
| } | ||
| let item = serde_json::from_str::<audiobookshelf_models::Item>(&body).with_context(|| { | ||
| format!("Failed to parse JSON from Audiobookshelf response for {request_url}") | ||
| })?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant file and surrounding implementation.
if [ -f "crates/services/importer/audiobookshelf/src/lib.rs" ]; then
echo "== file exists =="
wc -l crates/services/importer/audiobookshelf/src/lib.rs
echo "== relevant lines =="
sed -n '220,290p' crates/services/importer/audiobookshelf/src/lib.rs | nl -ba -v220
else
echo "missing target file"
fd -a 'lib\.rs$' crates/services/importer 2>/dev/null | sed -n '1,50p'
fi
echo "== imports and error types =="
rg -n "reqwest|response\.text|with_context|audiobookshelf|struct .*Error|type .*Result|use anyhow" crates/services/importer/audiobookshelf -SRepository: IgnisDa/ryot
Length of output: 284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/services/importer/audiobookshelf/src/lib.rs"
echo "== relevant lines =="
sed -n '220,290p' "$file" | awk '{ printf "%6d %s\n", 220+NR-1, $0 }'
echo "== imports and error types / response usage =="
rg -n "reqwest|response\.text|with_context|audiobookshelf|struct .*Error|type .*Result|use anyhow" crates/services/importer/audiobookshelf -S || true
echo "== dependency info =="
for f in Cargo.toml crates/services/importer/importer/Cargo.toml crates/services/importer/audiobookshelf/Cargo.toml; do
if [ -f "$f" ]; then
echo "-- $f --"
rg -n "reqwest|anyhow|serde_json|tokio" "$f" || true
fi
done
echo "== reqwest text result type availability in docs if installed =="
cargo metadata --format-version 1 --no-deps >/tmp/cargo-metadata.json 2>/tmp/cargo-metadata.err || true
if [ -f /tmp/cargo-metadata.json ]; then
python3 - <<'PY' /tmp/cargo-metadata.json
import json, sys
meta=json.load(open(sys.argv[1]))
packages=[p for p in meta.get("packages",[]) if p.get("name")=="reqwest"]
for p in packages:
print(p.get("name"), p.get("version"), p.get("source"))
PY
fiRepository: IgnisDa/ryot
Length of output: 4613
🌐 Web query:
reqwest Response text documentation returns Result String Error
💡 Result:
In the reqwest crate, the text method on the Response struct returns a Result<String, reqwest::Error> [1][2]. When calling this method, the response body is decoded into a String. It includes several behaviors to ensure robust decoding: - Encoding Determination: It prioritizes the charset parameter of the Content-Type header; if that is not present, it defaults to UTF-8 [1][3]. - Character Handling: Malformed sequences are replaced with the char::REPLACEMENT_CHARACTER [1][4]. - BOM Handling: Byte Order Marks (BOM) are detected and stripped from the returned String [1][2]. If the optional charset feature is disabled, the method only attempts to decode the response as UTF-8, regardless of the Content-Type header [1][3].
Citations:
- 1: https://docs.rs/reqwest/latest/reqwest/struct.Response.html
- 2: https://docs.rs/reqwest/%3E=0.11,%20%3C=0.12/struct.Response.html
- 3: https://docs.rs/reqwest/latest/reqwest/blocking/struct.Response.html
- 4: https://github.com/seanmonstar/reqwest/blob/d31ffbbf/src/async_impl/response.rs
Propagate response-body read failures.
response.text().await.unwrap_or_default() converts read failures into an empty body. Failures are then misreported as JSON parsing errors for successful responses, or lost for HTTP error responses. Preserve the read error with with_context(...)? before checking the status.
🤖 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 `@crates/services/importer/audiobookshelf/src/lib.rs` around lines 258 - 272,
Update the response-body read in the Audiobookshelf request flow to replace
unwrap_or_default with error propagation using with_context(...)? before the
status check. Preserve the existing status handling and JSON parsing behavior
while reporting failures to read response.text() with request URL context.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/services/importer/audiobookshelf/src/lib.rs (1)
159-164: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
ImportFailedItemstruct-literal fields not ordered by ascending line length. All three sites still violate the field-ordering guideline flagged in a prior review round.
crates/services/importer/audiobookshelf/src/lib.rs#L159-L164: reorder toidentifier,lot,step,error.crates/services/importer/audiobookshelf/src/lib.rs#L192-L197: reorder toidentifier,lot,step,error.crates/services/importer/audiobookshelf/src/lib.rs#L221-L229: reorder tolot,identifier,step,error(lot: None,is shorter thanidentifier: title,).As per coding guidelines, "When initializing structs (Rust) or object literals (TypeScript), order fields by ascending line length - shorter lines first, longer lines last."
🛠️ Proposed fixes
.map_err(|e| ImportFailedItem { identifier: title.clone(), - error: Some(format!("item_id={}: {e:#}", item.id)), lot: Some(MediaLot::Podcast), step: ImportFailStep::ItemDetailsFromSource, + error: Some(format!("item_id={}: {e:#}", item.id)), })?; ... .map_err(|e| ImportFailedItem { identifier: title.clone(), - error: Some(format!("item_id={}: {e:#}", item.id)), lot: Some(MediaLot::Podcast), step: ImportFailStep::DatabaseCommit, + error: Some(format!("item_id={}: {e:#}", item.id)), })?; ... return Err(ImportFailedItem { - identifier: title, - step: ImportFailStep::InputTransformation, lot: None, + identifier: title, + step: ImportFailStep::InputTransformation, error: Some(format!( "item_id={}: No ASIN, ISBN or iTunes ID found", item.id )), });🤖 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 `@crates/services/importer/audiobookshelf/src/lib.rs` around lines 159 - 164, Reorder the ImportFailedItem struct literals in crates/services/importer/audiobookshelf/src/lib.rs at lines 159-164 and 192-197 to identifier, lot, step, error, and at lines 221-229 to lot, identifier, step, error, preserving all field values and behavior.Source: Coding guidelines
♻️ Duplicate comments (2)
crates/services/importer/audiobookshelf/src/lib.rs (2)
260-276: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winResponse-body read failures are still swallowed before the status check.
response.text().await.unwrap_or_default()converts a body-read failure into an empty string. On success status, that empty body is then handed toserde_json::from_str, misreporting what is actually a transport/read failure as a JSON parse error. On a non-success status, the real read error is silently lost from thebail!message. Propagate it with.with_context(...)?before the status check, as previously flagged.🛠️ Proposed fix
- let status = response.status(); - let body = response.text().await.unwrap_or_default(); + let status = response.status(); + let body = response + .text() + .await + .with_context(|| format!("Failed to read response body from {request_url}"))?;🤖 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 `@crates/services/importer/audiobookshelf/src/lib.rs` around lines 260 - 276, Update the response-body read in the item-fetching function to propagate failures instead of using unwrap_or_default. Add contextual error handling to response.text().await and apply ? before the status check, preserving the existing HTTP-status and JSON parsing behavior for successfully read bodies.
137-151: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFailure messages still omit source-record identifiers needed to trace records. Both sites build
ImportFailedItem.errorwithout the full identifying context of the Audiobookshelf record that failed, a gap flagged in a previous review round and still unaddressed.
crates/services/importer/audiobookshelf/src/lib.rs#L137-L151: additem.idto the "No Google Books ID found" and "No ISBN found" error strings.crates/services/importer/audiobookshelf/src/lib.rs#L192-L197: addepisode_id(in addition to the already-presentitem.id) to thecommit_metadatafailure error string.🛠️ Proposed fix
- error: Some("No Google Books ID found".to_string()), + error: Some(format!("item_id={}: No Google Books ID found", item.id)), ... - error: Some("No ISBN found".to_string()), + error: Some(format!("item_id={}: No ISBN found", item.id)), ... - error: Some(format!("item_id={}: {e:#}", item.id)), + error: Some(format!( + "item_id={}, episode_id={}: {e:#}", + item.id, episode_id + )),🤖 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 `@crates/services/importer/audiobookshelf/src/lib.rs` around lines 137 - 151, The Audiobookshelf import failure messages lack complete source-record context. In crates/services/importer/audiobookshelf/src/lib.rs:137-151, update the “No Google Books ID found” and “No ISBN found” errors to include item.id; in crates/services/importer/audiobookshelf/src/lib.rs:192-197, update the commit_metadata failure error to include episode_id alongside the existing item.id.
🤖 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 `@crates/services/importer/audiobookshelf/src/lib.rs`:
- Around line 52-64: The /me response parsing currently panics via unwrap
instead of propagating errors. In the import flow around the MeResponse
deserialization, replace unwrap with error propagation using ? and add
contextual information with with_context, preserving the existing Result-based
handling.
---
Outside diff comments:
In `@crates/services/importer/audiobookshelf/src/lib.rs`:
- Around line 159-164: Reorder the ImportFailedItem struct literals in
crates/services/importer/audiobookshelf/src/lib.rs at lines 159-164 and 192-197
to identifier, lot, step, error, and at lines 221-229 to lot, identifier, step,
error, preserving all field values and behavior.
---
Duplicate comments:
In `@crates/services/importer/audiobookshelf/src/lib.rs`:
- Around line 260-276: Update the response-body read in the item-fetching
function to propagate failures instead of using unwrap_or_default. Add
contextual error handling to response.text().await and apply ? before the status
check, preserving the existing HTTP-status and JSON parsing behavior for
successfully read bodies.
- Around line 137-151: The Audiobookshelf import failure messages lack complete
source-record context. In
crates/services/importer/audiobookshelf/src/lib.rs:137-151, update the “No
Google Books ID found” and “No ISBN found” errors to include item.id; in
crates/services/importer/audiobookshelf/src/lib.rs:192-197, update the
commit_metadata failure error to include episode_id alongside the existing
item.id.
🪄 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: b1b842ea-dfdd-40e8-963a-df520bb2edf8
📒 Files selected for processing (2)
crates/models/external/src/lib.rscrates/services/importer/audiobookshelf/src/lib.rs
| let me = client | ||
| .get(format!("{url}/me")) | ||
| .send() | ||
| .await? | ||
| .json::<audiobookshelf_models::MeResponse>() | ||
| .await | ||
| .unwrap(); | ||
| let finished_podcast_episodes = me | ||
| .media_progress | ||
| .into_iter() | ||
| .filter(|progress| progress.is_finished) | ||
| .filter_map(|progress| Some((progress.library_item_id?, progress.episode_id?))) | ||
| .collect::<HashSet<_>>(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
/me response parsing panics on failure, unlike the rest of this PR.
.json::<MeResponse>().await.unwrap() panics the entire import job if the /me endpoint returns malformed JSON or an unexpected shape — a much larger blast radius than the per-item failures this PR otherwise carefully categorizes (network vs. status vs. parse errors) in get_item_details. Since import() already returns Result<ImportResult>, propagate this with ? and with_context instead.
🛠️ Proposed fix
- let me = client
- .get(format!("{url}/me"))
- .send()
- .await?
- .json::<audiobookshelf_models::MeResponse>()
- .await
- .unwrap();
+ let me = client
+ .get(format!("{url}/me"))
+ .send()
+ .await
+ .with_context(|| "Network error requesting Audiobookshelf /me endpoint")?
+ .json::<audiobookshelf_models::MeResponse>()
+ .await
+ .with_context(|| "Failed to parse JSON from Audiobookshelf /me response")?;📝 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.
| let me = client | |
| .get(format!("{url}/me")) | |
| .send() | |
| .await? | |
| .json::<audiobookshelf_models::MeResponse>() | |
| .await | |
| .unwrap(); | |
| let finished_podcast_episodes = me | |
| .media_progress | |
| .into_iter() | |
| .filter(|progress| progress.is_finished) | |
| .filter_map(|progress| Some((progress.library_item_id?, progress.episode_id?))) | |
| .collect::<HashSet<_>>(); | |
| let me = client | |
| .get(format!("{url}/me")) | |
| .send() | |
| .await | |
| .with_context(|| "Network error requesting Audiobookshelf /me endpoint")? | |
| .json::<audiobookshelf_models::MeResponse>() | |
| .await | |
| .with_context(|| "Failed to parse JSON from Audiobookshelf /me response")?; | |
| let finished_podcast_episodes = me | |
| .media_progress | |
| .into_iter() | |
| .filter(|progress| progress.is_finished) | |
| .filter_map(|progress| Some((progress.library_item_id?, progress.episode_id?))) | |
| .collect::<HashSet<_>>(); |
🤖 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 `@crates/services/importer/audiobookshelf/src/lib.rs` around lines 52 - 64, The
/me response parsing currently panics via unwrap instead of propagating errors.
In the import flow around the MeResponse deserialization, replace unwrap with
error propagation using ? and add contextual information with with_context,
preserving the existing Result-based handling.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/services/importer/audiobookshelf/src/lib.rs (1)
143-148: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winMissing
lotfield — likely a compile-breaking regression.This
ImportFailedItemliteral only setsidentifier,error, andstep; the requiredlot: Option<MediaLot>field is absent and there's no..Default::default()spread.ImportFailedItem's fields (identifier,step,lot,error) are all required in a struct literal unless the struct derivesDefaultand a..update syntax is used — neither is shown here. This will fail withE0063: missing field lotunlessImportFailedItemderivesDefault. This also contradicts the AI summary, which states both the Google Books ID and ISBN branches now explicitly setlot: Some(MediaLot::Book). It also still omitsitem.idin the error message, unresolved from a prior review round.🐛 Proposed fix
_ => { return Err(ImportFailedItem { identifier: title, - error: Some("No ISBN found".to_string()), - step: ImportFailStep::InputTransformation, + lot: Some(MediaLot::Book), + step: ImportFailStep::InputTransformation, + error: Some(format!("item_id={}: No ISBN found", item.id)), }); }#!/bin/bash # Confirm whether ImportFailedItem derives Default (would allow the omitted field to compile). rg -n -B5 'struct ImportFailedItem' crates/models/importer/src/lib.rs🤖 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 `@crates/services/importer/audiobookshelf/src/lib.rs` around lines 143 - 148, Update the ImportFailedItem construction in the ISBN-missing branch to include the required lot field as Some(MediaLot::Book), matching the Google Books ID branch. Also include item.id in the “No ISBN found” error message, while preserving the existing identifier and InputTransformation step.
🤖 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 `@crates/services/importer/audiobookshelf/src/lib.rs`:
- Around line 254-285: Update get_item_details and fetch_item_details error
handling so retries occur only for network failures or HTTP 5xx responses;
immediately return parse errors and other non-retryable statuses such as 4xx
responses. Preserve the existing retry count, backoff, and successful item
return behavior for retryable failures.
- Line 96: Replace the global buffer_unordered(1) limit in the importer stream
with scoped synchronization, such as bounded concurrency or a per-podcast lock.
Preserve concurrency between independent library items while ensuring each
podcast’s episode imports and commits remain coordinated during item-details
retries.
- Around line 309-322: Update build_client to handle HeaderValue::from_str
failure without unwrapping: propagate the invalid-token error to the importer so
it returns one failed import item instead of panicking. Adjust the function’s
return flow and its caller as needed while preserving normal client creation and
existing client-build behavior.
---
Duplicate comments:
In `@crates/services/importer/audiobookshelf/src/lib.rs`:
- Around line 143-148: Update the ImportFailedItem construction in the
ISBN-missing branch to include the required lot field as Some(MediaLot::Book),
matching the Google Books ID branch. Also include item.id in the “No ISBN found”
error message, while preserving the existing identifier and InputTransformation
step.
🪄 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: cde7441e-ebc3-4f04-8d4e-382f278792db
📒 Files selected for processing (1)
crates/services/importer/audiobookshelf/src/lib.rs
| let results: Vec<_> = stream::iter(finished_items.results.into_iter().enumerate()) | ||
| .map(|(idx, item)| process_item(idx, item, len, &client, &url, &services)) | ||
| .buffer_unordered(5) | ||
| .buffer_unordered(1) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Is there a documented reason to fully serialize reqwest requests instead of using bounded concurrency with buffer_unordered?
💡 Result:
There is no documented requirement or technical advantage to fully serializing reqwest requests as a default practice. In fact, reqwest is designed for asynchronous operation, and using concurrency (such as with buffer_unordered) is the standard approach to maximize throughput and minimize latency by hiding network wait times [1][2][3]. However, there are specific, documented scenarios where serializing requests or limiting concurrency is necessary or preferred over unbound parallelism: 1. Server-Side Rate Limiting and Resource Constraints: Sending too many concurrent requests can trigger rate limits (429 Too Many Requests) or overwhelm the destination server, leading to dropped connections or timeouts [4]. 2. Client-Side Resource Exhaustion: Without bound, you risk exhausting local system resources, such as file descriptors or socket limits [4]. Connection pooling in reqwest is highly efficient, but unbounded concurrency can still cause connection churn, especially when interacting with many different hosts [5][3]. 3. Avoiding Thundering Herd Problems: In some complex architectures, spawning too many futures at once can create contention issues, where tasks wake up repeatedly but fail to progress, wasting CPU cycles [6]. 4. Order Sensitivity: If your application logic depends on the requests completing in a strict sequential order (e.g., streaming a process where step B requires the output of step A), buffer_unordered will break this sequence. In such cases, standard sequential iteration or buffer_ordered is required [7]. 5. Complex Data Dependencies: If requests are not independent—for example, if they require shared mutable state or perform operations that are not thread-safe (though reqwest::Client handles connection pooling internally and safely across threads)—serialization may be the simplest way to avoid complex synchronization overhead [1][5]. To balance performance with these constraints, it is common to use buffer_unordered(n) with a carefully chosen n (bounded concurrency) or a tokio::sync::Semaphore to manually control the number of active requests without resorting to full serialization [4][8]. The reqwest::Client should generally be created once and shared (cloned) across tasks to benefit from connection pooling, regardless of whether you choose to run requests concurrently or sequentially [5][3].
Citations:
- 1: https://stackoverflow.com/questions/51044467/how-can-i-perform-parallel-asynchronous-http-get-requests-with-reqwest
- 2: https://stackoverflow.com/questions/70772751/http-request-not-being-sent-async
- 3: https://arcmutex.com/content/reqwest-connection-pooling-performance
- 4: https://users.rust-lang.org/t/reqwest-http-client-fails-when-too-much-concurrency/55644
- 5: https://stackoverflow.com/questions/79396900/fast-request-sending-in-rust
- 6: https://blog.polybdenum.com/2023/05/15/tokio-futuresunordered-and-the-thundering-herd-problem.html
- 7: Footgun lurking in
FuturesUnorderedand other concurrency-enabling streams rust-lang/futures-rs#2387 - 8: https://users.rust-lang.org/t/reqwest-multiple-concurrent-requests-by-priority/128415
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant importer implementation and retry flow.
fd -a 'lib.rs' . | sed 's#^\./##' | rg 'importer/audiobookshelf/src/lib.rs|audiobookshelf/src/lib.rs' || true
echo '--- outline ---'
ast-grep outline crates/services/importer/audiobookshelf/src/lib.rs --view compact || true
echo '--- relevant lines ---'
cat -n crates/services/importer/audiobookshelf/src/lib.rs | sed -n '1,180p'
echo '--- search get_item_details and retry/backoff symbols ---'
rg -n "get_item_details|buffer_unordered|retry|backoff|try|Result|async|match" crates/services/importer/audiobookshelf/src/lib.rsRepository: IgnisDa/ryot
Length of output: 9866
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- read slice around item processing continuation and get_item_details ---'
cat -n crates/services/importer/audiobookshelf/src/lib.rs | sed -n '150,320p'
echo '--- get_item_details usages and concurrent limits in audiobookshelf importer ---'
rg -n "get_item_details|buffer_unordered|buffer_max|Semaphore|semaphore|mutex|lock|finished_podcast|podcast" crates/services/importer/audiobookshelf/src/lib.rs crates -g '*.rs' | head -200
echo '--- determine whether get_item_details loop is exponential backoff and how many sleeps ---'
python3 - <<'PY'
from pathlib import Path
s = Path('crates/services/importer/audiobookshelf/src/lib.rs').read_text().splitlines()
for i, line in enumerate(s, 1):
if i < 250 or i > 310:
continue
print(f"{i:04d}: {line}")
PYRepository: IgnisDa/ryot
Length of output: 32688
Avoid serializing the entire importer with buffer_unordered(1).
Each podcast imports per-episode commits, while item-details fetching can retry forever with exponential sleeps (2s then 4s before the final attempt). Limit the risk with scoped synchronization instead of making every library item fetch and commit purely sequential; a bounded stream or per-podcast lock would preserve concurrency without exposing the same stale-during-retry failure window across the whole import pipeline.
🤖 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 `@crates/services/importer/audiobookshelf/src/lib.rs` at line 96, Replace the
global buffer_unordered(1) limit in the importer stream with scoped
synchronization, such as bounded concurrency or a per-podcast lock. Preserve
concurrency between independent library items while ensuring each podcast’s
episode imports and commits remain coordinated during item-details retries.
| const MAX_ITEM_DETAILS_ATTEMPTS: u32 = 3; | ||
|
|
||
| async fn get_item_details( | ||
| client: &Client, | ||
| url: &str, | ||
| id: &str, | ||
| episode: Option<String>, | ||
| ) -> Result<audiobookshelf_models::Item> { | ||
| let mut query = serde_json::json!({ "expanded": "1", "include": "progress" }); | ||
| if let Some(episode) = episode { | ||
| query["episode"] = serde_json::json!(episode); | ||
| let query = serde_json::json!({ "expanded": "1" }); | ||
| let request_url = format!("{url}/items/{id}"); | ||
| let mut last_error = None; | ||
| for attempt in 0..MAX_ITEM_DETAILS_ATTEMPTS { | ||
| match fetch_item_details(client, &request_url, &query).await { | ||
| Ok(item) => return Ok(item), | ||
| Err(error) => { | ||
| last_error = Some(error); | ||
| if attempt + 1 < MAX_ITEM_DETAILS_ATTEMPTS { | ||
| let sleep_time = u64::pow(2, attempt + 1); | ||
| ryot_log!( | ||
| debug, | ||
| "Attempt {}/{} failed for {}, retrying in {}s", | ||
| attempt + 1, | ||
| MAX_ITEM_DETAILS_ATTEMPTS, | ||
| request_url, | ||
| sleep_time | ||
| ); | ||
| sleep_for_n_seconds(sleep_time).await; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| let item = client | ||
| .get(format!("{url}/items/{id}")) | ||
| .query(&query) | ||
| Err(last_error.unwrap()) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Retry loop retries non-transient failures (parse errors, 4xx) the same as transient ones.
fetch_item_details errors from serde_json::from_str (malformed/unexpected JSON shape) and from non-success HTTP statuses (e.g. a deleted item returning 404) are indistinguishable from network errors here, so every permanent failure now burns the full 3-attempt exponential backoff (~2s + 4s ≈ 6s) before surfacing. Combined with buffer_unordered(1) (Line 96), this can meaningfully slow down imports for libraries containing any permanently-unreachable items. Consider only retrying on network errors or 5xx statuses, and failing fast on 4xx/parse errors.
[reliability_and_resilience]
🤖 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 `@crates/services/importer/audiobookshelf/src/lib.rs` around lines 254 - 285,
Update get_item_details and fetch_item_details error handling so retries occur
only for network failures or HTTP 5xx responses; immediately return parse errors
and other non-retryable statuses such as 4xx responses. Preserve the existing
retry count, backoff, and successful item return behavior for retryable
failures.
| fn build_client(bearer_token: &str, danger_accept_invalid_certs: bool) -> Client { | ||
| let mut headers = HeaderMap::new(); | ||
| headers.insert(USER_AGENT, HeaderValue::from_static(USER_AGENT_STR)); | ||
| headers.insert( | ||
| AUTHORIZATION, | ||
| HeaderValue::from_str(&format!("Bearer {bearer_token}")).unwrap(), | ||
| ); | ||
| ClientBuilder::new() | ||
| .default_headers(headers) | ||
| .timeout(Duration::from_secs(60)) | ||
| .danger_accept_invalid_certs(danger_accept_invalid_certs) | ||
| .build() | ||
| .unwrap() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does reqwest HeaderValue::from_str reject leading/trailing whitespace or control characters in API tokens?
💡 Result:
Yes, the HeaderValue::from_str method in the http crate (which reqwest uses) rejects strings containing leading or trailing whitespace [1][2]. When you attempt to create a HeaderValue from a string that starts or ends with whitespace (such as a space or tab), the operation will return an error [1][2]. This design choice follows the HTTP specification (RFC 7230/9110), which dictates that header field values should not include leading or trailing whitespace, as such characters are meant to be excluded by parsers [1][2]. In addition to rejecting leading and trailing whitespace, HeaderValue::from_str validates that the input contains only visible ASCII characters (bytes 32–126, where 32 is a space) [3][4][5]. Although the horizontal tab (byte 9) is technically allowed in header values by the underlying logic, the specific restriction against leading/trailing whitespace prevents its use at the beginning or end of the string [1][5]. If you are dealing with API tokens that may contain accidental whitespace, you should trim the string (e.g., using.trim in Rust) before passing it to HeaderValue::from_str [2].
Citations:
- 1: Deny leading/trailing whitespace in HeaderValue ( fixes #245 ). hyperium/http#256
- 2: Remove trailing whitespaces from header values hyperium/http#245
- 3: https://docs.rs/reqwest/latest/reqwest/header/struct.HeaderValue.html
- 4: https://github.com/hyperium/http/blob/master/src/header/value.rs
- 5: https://docs.rs/http/latest/src/http/header/value.rs.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg 'crates/services/importer/audiobookshelf/src/lib\.rs|Cargo\.toml|Cargo.lock' || true
echo "== inspect relevant lines =="
cat -n crates/services/importer/audiobookshelf/src/lib.rs | sed -n '260,350p'
echo "== imports and surrounding config/token usage =="
sed -n '1,120p' crates/services/importer/audiobookshelf/src/lib.rs
rg -n "api_key|bearer_token|build_client|ImportResult|ImportFailedItem" crates/services/importer/audiobookshelf/src/lib.rs
echo "== reqwest/http dependency versions =="
rg -n 'reqwest|http \||http =|http ' Cargo.toml Cargo.lockRepository: IgnisDa/ryot
Length of output: 15699
🌐 Web query:
http crate 1.4.0 HeaderValue::from_str leading whitespace leading/hard tabs v1 value parsing
💡 Result:
In the http crate, the HeaderValue::from_str method (and related constructors like from_bytes) strictly prohibits leading or trailing whitespace, as well as control characters (including tabs and other non-visible ASCII characters), by design [1][2][3]. 1. Strict Validation: The http crate enforces that a HeaderValue contains only visible ASCII characters (defined as the range 32-127 inclusive, excluding the DEL character 127) [1][4][5]. Because space (0x20) and horizontal tabs (0x09) are either outside this range or classified as non-visible/control characters in the context of this implementation, any input starting or ending with these characters, or containing them, will trigger a validation error [1][3]. 2. Motivation: This behavior is intentional. While HTTP specifications (such as RFC 7230/9110) note that parsers should ignore "Observed Whitespace" (OWS) around field values when extracting them from a raw byte stream, it is considered a requirement that a valid HeaderValue type itself should represent the "sanitized" or "trimmed" value [2][3]. Therefore, the library forces developers to ensure that the data provided to these constructors is already compliant and trimmed of leading/trailing whitespace [2][3]. 3. Historical Context: This behavior was formalized to prevent issues where invalid or ambiguous header values could be constructed [2][3]. Previous discussions and pull requests (e.g., #245, #256) confirmed that it is the responsibility of the library's constructors to validate that the provided string or byte slice adheres to these requirements [2][3]. If you are encountering errors when attempting to create a HeaderValue, ensure that the input string is trimmed of all leading and trailing whitespace and does not contain tabs or other non-printable ASCII characters before passing it to HeaderValue::from_str [1][6][3].
Citations:
- 1: https://docs.rs/http/latest/src/http/header/value.rs.html
- 2: Remove trailing whitespaces from header values hyperium/http#245
- 3: Deny leading/trailing whitespace in HeaderValue ( fixes #245 ). hyperium/http#256
- 4: https://docs.rs/http/latest/http/header/struct.HeaderValue.html
- 5: https://doc.cuprate.org/src/http/header/value.rs.html
- 6: https://github.com/hyperium/http/blob/master/src/header/value.rs
🌐 Web query:
rust http 1.4.0 Source code HttpValue v1 header value visible ASCII 9 tab
💡 Result:
In the Rust http crate (version 1.4.0), the HeaderValue type supports the horizontal tab character (ASCII 9, \t) as a valid part of a header value [1][2]. Internally, the crate's validation logic uses a helper function called is_visible_ascii to determine which characters are permitted [1][2]. This function defines visible ASCII characters as those in the range 32–126 (inclusive) and explicitly includes the horizontal tab character (\t, ASCII 9) [1][2]. The code snippet below demonstrates this implementation logic: fn is_visible_ascii(b: u8) -> bool { b >= 32 && b < 127 || b == b'\t' } Consequently, methods that create a HeaderValue (such as from_static or from_str) will successfully accept strings containing horizontal tab characters [1][2]. This aligns with HTTP specifications that permit visible ASCII characters, spaces, and horizontal tabs within header field values [3].
Citations:
- 1: https://doc.cuprate.org/src/http/header/value.rs.html
- 2: https://docs.rs/http/latest/src/http/header/value.rs.html
- 3: Horizontal tabs should be allowed in header values dotnet/aspnetcore#40599
🌐 Web query:
http crate 1.4.0 HeaderValue::from_str trailing whitespace control character behavior leading/hard tab not visible ASCII
💡 Result:
In the http crate (including version 1.4.0), the HeaderValue::from_str method enforces strict validation rules on input strings [1][2]. It explicitly permits only visible ASCII characters, defined as the range 32–127 (inclusive) [1][3]. Regarding your specific concerns: 1. Leading and Trailing Whitespace: The http crate prohibits leading and trailing whitespace in HeaderValue objects [4]. This was implemented to align with HTTP specifications (RFC 7230), which expect parsers to ignore whitespace around header values rather than including them as part of the value itself [4]. Attempting to construct a HeaderValue with leading or trailing spaces (ASCII 32 or horizontal tab, ASCII 9) will result in an error [4][2]. 2. Control Characters and Tabs: The implementation restricts HeaderValue to visible ASCII characters (32–127) [1][3]. Because it explicitly excludes characters below 32, control characters—including horizontal tabs (ASCII 9)—are disallowed [1][3]. While some earlier discussions and GitHub issues noted potential nuances regarding how "visible ASCII" is interpreted versus broader HTTP spec requirements (which historically allowed HTAB), the current implementation maintains this strict restriction [1][5]. If you need to include opaque octets (bytes outside the 32–127 range, such as 128–255), you should use HeaderValue::from_bytes instead of from_str [1][2]. Unlike from_str, from_bytes provides a path to construct values with bytes that do not fit the visible ASCII requirement [1][3].
Citations:
- 1: https://docs.rs/http/latest/http/header/struct.HeaderValue.html
- 2: https://github.com/hyperium/http/blob/master/src/header/value.rs
- 3: https://docs.rs/http/latest/src/http/header/value.rs.html
- 4: Deny leading/trailing whitespace in HeaderValue ( fixes #245 ). hyperium/http#256
- 5: HeaderValue::from_str does not validate that the string is valid ascii hyperium/http#519
Return a failed import item when the bearer token is not a valid HeaderValue.
HeaderValue::from_str(&format!("Bearer {bearer_token}")).unwrap() will panic for tokens containing leading/trailing whitespace or non-visible/control bytes. Since api_key is user-supplied, check the result here and add one failed item with the invalid-token error instead of crashing the import job. .build().unwrap() is comparatively low-risk because client build failures are TLS/layer initialization failures rather than config validation failures.
🤖 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 `@crates/services/importer/audiobookshelf/src/lib.rs` around lines 309 - 322,
Update build_client to handle HeaderValue::from_str failure without unwrapping:
propagate the invalid-token error to the importer so it returns one failed
import item instead of panicking. Adjust the function’s return flow and its
caller as needed while preserving normal client creation and existing
client-build behavior.
1 participant
Refactor get_item_details to intercept each failure mode separately:
Populate lot (MediaLot) in all ImportFailedItem error paths so the media type is always visible in failure reports. Include the Audiobookshelf item.id and episode.id in error messages for cross-referencing. Fix commit_metadata failures to use ImportFailStep::DatabaseCommit instead of ItemDetailsFromSource.
Summary by CodeRabbit