Skip to content

test(api): install rustls CryptoProvider in setup_app#6

Closed
doctatortot wants to merge 2 commits into
ourmem:mainfrom
doctatortot:upstream-pr/test-crypto-provider
Closed

test(api): install rustls CryptoProvider in setup_app#6
doctatortot wants to merge 2 commits into
ourmem:mainfrom
doctatortot:upstream-pr/test-crypto-provider

Conversation

@doctatortot
Copy link
Copy Markdown
Contributor

rustls 0.23 requires a CryptoProvider to be installed at process startup before any TLS handshake. main() does this; tests bypass main(), so any test reaching object_store via LanceStore::new fails with no process-level CryptoProvider available.

Add a Once-guarded install_crypto_provider() helper in the api tests module and call it at the top of setup_app(). Once::call_once handles the constraint that install_default may be called at most once per process (any subsequent attempts return Err).

I hit this while working on #4 and worked around it by scoping CI to store:: only. This unblocks the ~20 api tests so future test additions don't have to dodge the same hole.

No production behavior change — tests only.

fn install_crypto_provider() {
    use std::sync::Once;
    static INIT: Once = Once::new();
    INIT.call_once(|| {
        let _ = rustls::crypto::ring::default_provider().install_default();
    });
}

doctatortot and others added 2 commits May 18, 2026 06:00
Fork-overlay for our forgejo runner — not relevant to upstream since
they use GitHub Actions. Builds into /dev/shm on kbuild (24G tmpfs),
rust-cache pinned to v2.7.7 for node20 act_runner compatibility, scoped
tests to store:: to avoid pre-existing failures in api/space tests.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
rustls 0.23 requires a CryptoProvider to be installed at process
startup before any TLS handshake. main() handles this with
`rustls::crypto::ring::default_provider().install_default()`, but tests
bypass main(), so any test reaching object_store via LanceStore::new
fails with 'no process-level CryptoProvider available'.

Add a Once-guarded install_crypto_provider() helper in the api tests
module and call it at the top of setup_app(). Once::call_once handles
the constraint that install_default may be called at most once per
process (any subsequent attempts return Err).

Affected: ~20 tests in omem-server/src/api/mod.rs that all flow through
setup_app(). store:: tests aren't affected (they don't go through the
S3/object_store code path).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@doctatortot
Copy link
Copy Markdown
Contributor Author

The failing check is cargo fmt -- --check reporting 229 diffs across 30+ files — none of which are in this PR's diff. The fmt complaints on api/mod.rs are all at lines 476+, while my change is at lines 54-70. Same fmt state exists on main, so I think this just hasn't been run-and-committed in a while.

A few ways to handle:

  1. Merge as-is — same fmt situation existed when feat(store): make Lance vector dimension configurable, derive from embedder #4 landed, and the actual change here is a 14-line Once-guarded init in test setup. No production code touched, no fmt drift in the diff.
  2. Want me to open a separate chore: cargo fmt PR first? Happy to. It'd be whitespace-only but unblocks every future contribution.
  3. Squash fmt into this PR — possible but it would balloon the diff to ~250 hunks.

Whichever you prefer. Otherwise standing by to address any actual review feedback on the test fix itself.

yhyyz added a commit that referenced this pull request May 19, 2026
- test(api): install rustls CryptoProvider in setup_app (PR #6)
  Fixes ~20 api test failures from missing TLS provider

- fix(retrieve): replace min-max RRF normalization with scale-and-clamp (PR #9)
  Preserves absolute quality signal; weak matches no longer inflate to 1.0
  RRF_SCALE=61 so ideal dual-leg rank-1 maps to ~1.0. Closes #7

- feat(api): expose memory_type on create and update (PR #10)
  POST /v1/memories accepts memory_type (pinned|insight|session)
  PUT /v1/memories/:id can change memory_type
  Default remains 'pinned' for backwards compat

Co-authored-by: doctatortot <doctatortot@users.noreply.github.com>
@yhyyz
Copy link
Copy Markdown
Contributor

yhyyz commented May 19, 2026

Applied in 628b757 — thanks @doctatortot!

We took the install_crypto_provider() fix from your PR (the api/mod.rs part). We skipped the .forgejo/ workflow file since we use GitHub Actions, but appreciate you noting the CI gap.

This unblocked the api tests — they now pass (except 5 pre-existing space membership issues unrelated to your change). Also ran cargo fmt across the whole project in a prior commit to clear the 229-file formatting debt you flagged. 👍

@yhyyz yhyyz closed this May 19, 2026
@yhyyz
Copy link
Copy Markdown
Contributor

yhyyz commented May 19, 2026

BTW — we also tracked down the 5 space-membership test failures you flagged as pre-existing. Root cause was not a LanceDB timing issue but a URL encoding bug in the tests: space IDs have the format team/{uuid} (contain /), which breaks axum's single-segment {id} route matching when used raw in the URI.

Fix: use percent_encoding::utf8_percent_encode(&space_id, NON_ALPHANUMERIC) in test URIs — same semantics as encodeURIComponent() in the production JS clients.

All 379 tests pass now, 0 failures. Commit: 3472c74

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.

2 participants