Skip to content

fix(service_loader): stop silently swallowing credential/config parse errors - #84

Merged
jhamill34 merged 4 commits into
mainfrom
claude/github-issues-backlog-groom-57d0xg
Aug 26, 2026
Merged

fix(service_loader): stop silently swallowing credential/config parse errors#84
jhamill34 merged 4 commits into
mainfrom
claude/github-issues-backlog-groom-57d0xg

Conversation

@jhamill34

Copy link
Copy Markdown
Owner

Summary

  • ServiceLoader::load() used if let Ok(x) = ... on the results of load_credentials/load_configuration, which discarded any error identically — whether the file was simply absent (fine to skip) or present but malformed (a real bug that should surface).
  • Replaced both with a match that only swallows ServiceLoader::Io when its io::ErrorKind is NotFound; every other error (malformed JSON, bad protobuf JSON, permission errors, etc.) now propagates via ?, matching the discipline used everywhere else in this function.
  • The one real caller (binary/apid/src/workers/loader.rs) already logs Err from load(), so newly-surfaced errors get reported instead of vanishing — no caller changes needed.

Fixes #17

Test plan

  • Added 3 regression tests to service_loader: missing credentials/config files are still skipped gracefully; malformed credentials now return Err; malformed config now returns Err.
  • cargo test -p service_loader — 28/28 passing.
  • Verified via git stash/pop that the new tests actually require the fix (they fail against the pre-fix code).
  • cargo build --workspace --all-features — clean.

Generated by Claude Code

claude added 3 commits August 26, 2026 13:20
… errors

load() used if-let-Ok on load_credentials/load_configuration, discarding
any error the same way whether the file was simply absent (fine) or
present but malformed (a real bug that should surface). Now only a
missing-file io::ErrorKind::NotFound is swallowed; everything else
propagates.

Fixes #17
CI's fmt check flagged the previous commit's formatting.
…ogic

collect_operations() hand-duplicated the same get/post/put/patch/delete/
head/options/trace block eight times; replaced with a loop over a
verb-to-HttpMethodType table.

service_writer::handle_path_items() had the mirror-image 8-arm match
building the JSON path-item key; replaced with EnumFull's descriptor
name (already used elsewhere in this file for InType), lowercased.

Also folded the near-identical allOf/anyOf/oneOf branches in
handle_schema() into one handle_composed_schema() helper.

Added regression tests for handle_path_items (grouping by path/verb,
rejecting an unset method) and handle_schema's composed-schema output.

Fixes #6

Copy link
Copy Markdown
Owner Author

The test check failed on commit 45e075c, but it's not this PR's failure: tests::async_api_caller_reuses_the_same_http_client_across_calls (runners/api_caller/src/lib.rs:1204) asserts an exact TCP-accept count across two sequential requests to confirm connection-pool reuse — a timing-sensitive assertion in a crate this PR never touches (only usecases/service_loader and usecases/service_writer are modified across all 3 commits here). The same test, with identical api_caller code, passed on the immediately prior commit (23a816b) in this same PR, which is strong evidence this is a flake rather than a regression.

I don't have permission to re-run the failed job here (rerun-failed-jobs returned a 403), so I can't confirm with a clean re-run. No fix for the flake exists yet, and making that assertion more robust is out of scope for this PR (it's unrelated to the credential/config-loading fix and the HTTP-verb dedup this PR is about) — flagging it here rather than widening the diff to touch it. Will keep watching this PR; a push to re-trigger CI will happen if there's a legitimate follow-up commit, otherwise someone with CI-rerun permission may want to just re-run the job.


Generated by Claude Code

…flake

Both connection-pooling tests sent two requests and asserted the second
reused the first's pooled connection, but never read the first response's
body. reqwest only returns a connection to its pool once the body is fully
drained, so the assertion raced that release against the second request —
observed failing in CI (#84) with an unrelated diff.
Reading the body via .bytes() before the next request removes the race.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018VzKriyNnMHmqQ6UxPhsv2

Copy link
Copy Markdown
Owner Author

Update: found and fixed the actual race behind the flaky test failure (commit 1f1eafe). Both reuses_the_same_http_client_across_calls and async_api_caller_reuses_the_same_http_client_across_calls in runners/api_caller/src/lib.rs sent two requests and asserted the second reused the first's pooled connection, but never read the first response's body — reqwest only returns a connection to its pool once the body is fully drained, so the assertion was racing that release against the second request firing. Added .bytes()/.bytes().await to drain each response before the next call. Verified with 20 back-to-back local runs (previously flaky, now consistently passing) plus the full api_caller suite (6/6).

This is a one-off flaky-test fix, not part of the original scope (credential/config error handling + HTTP-verb dedup) — flagging it clearly as its own commit rather than folding it into the others.


Generated by Claude Code

@jhamill34
jhamill34 merged commit 56fd15d into main Aug 26, 2026
5 checks passed
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.

service_loader: ServiceLoader::load() silently discards credential/config parse errors

2 participants