diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 474900758..077e8b5cb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -157,6 +157,33 @@ jobs: - name: Run tests with all features env: HFS_REQUIRE_R5: "1" + # Un-skips the S3+ES composite suite (s3_es_tests.rs), which had + # never run in CI — nothing set this variable, so every run passed it + # vacuously (#390). + # + # Unconditional, on every agent. It was briefly gated to Linux, for + # one measured reason that no longer applies. Run 30303350382 pinned + # this job to the Windows agent with the gate removed: 8 of 10 tests + # panicked in `crc-fast-1.6.0/src/arch/mod.rs:121` with "x86 features + # missing (SSE4.1 && PCLMULQDQ)". That agent is a virtualized guest + # without those instructions, and crc-fast 1.6.0 gates its software + # fallback behind `#[cfg(target_arch = "x86")]` — so on x86_64 the + # fallback never compiles and the arm falls straight into a panic. + # The AWS S3 SDK reaches it on every upload via aws-sdk-s3 -> + # aws-smithy-checksums -> crc-fast; the only two tests that passed + # were the two that never PUT an object. + # + # crc-fast fixed the fallback in 1.7.1. aws-smithy-checksums 0.63.12 + # pinned `crc-fast ~1.6.0`, so reaching the fix meant moving + # aws-sdk-s3 to 1.122.0 (-> checksums 0.64.4 -> crc-fast 1.9.0), + # which this PR does. Elasticsearch was never a blocker: that same + # run had `s3_es_test_read_missing_returns_none` pass, which runs the + # whole `make_harness()` path including ES container start and + # `initialize()`. + # + # Keep this unconditional. Re-gating it to one OS is what let the + # suite pass vacuously for its entire existence (#390). + RUN_MINIO_S3_ES_TESTS: "1" run: | cargo test --workspace --all-features @@ -229,22 +256,24 @@ jobs: env: RUN_MINIO_S3_TESTS: "1" EXPECTED_MINIO_SETTINGS_TESTS: "5" - # The single self-hosted Windows runner is a virtualized guest whose - # CPU does not advertise SSE4.1 + PCLMULQDQ. The AWS S3 SDK's default - # integrity protection computes a CRC32 on every upload via the - # `crc-fast` crate (aws-sdk-s3 -> aws-smithy-checksums -> crc-fast), - # and crc-fast 1.6.0 has no software fallback on x86_64 — it panics - # ("x86 features missing (SSE4.1 && PCLMULQDQ)"), failing all four - # tests before they can verify anything. Opt out of the default - # checksum calc/validation so the SDK never enters that code path. - # This step verifies the If-Match / If-None-Match precondition - # contract, not checksums, so no coverage is lost; the Linux leg above - # still exercises the default checksum path. Both the test's SDK - # client and the S3 backend build config via - # `aws_config::defaults(BehaviorVersion::latest())`, which reads these - # env vars, so setting them here is sufficient. - AWS_REQUEST_CHECKSUM_CALCULATION: "WHEN_REQUIRED" - AWS_RESPONSE_CHECKSUM_VALIDATION: "WHEN_REQUIRED" + # This step used to set AWS_REQUEST_CHECKSUM_CALCULATION and + # AWS_RESPONSE_CHECKSUM_VALIDATION to "WHEN_REQUIRED". That was not a + # preference — it was a workaround for crc-fast 1.6.0, which gates its + # software fallback behind `#[cfg(target_arch = "x86")]` and so panics + # ("x86 features missing (SSE4.1 && PCLMULQDQ)") on this virtualized + # x86_64 guest, whose CPU advertises neither. The AWS S3 SDK reaches it + # on every upload via aws-sdk-s3 -> aws-smithy-checksums -> crc-fast. + # + # Opting out dodged the panic but also disabled the SDK's default + # integrity protection, so this leg tested a different SDK + # configuration than the Linux leg above — and the checksum path, the + # part that was actually broken, was the one thing Windows never + # exercised. crc-fast 1.9.0 (via aws-sdk-s3 1.122.0) has a working + # software fallback, so the panic is gone and both legs can run the + # same defaults. + # + # Do not re-add the opt-out to silence a checksum panic; that would + # mean the SDK has been downgraded below crc-fast 1.7.1. # `runs-on: [self-hosted]` is unpinned, so this job can land on a Windows # runner, where `run:` defaults to PowerShell and cannot parse this bash # script (`set -o pipefail`, `| tee`, `if ! grep`, `\` continuations). diff --git a/Cargo.lock b/Cargo.lock index dde08cc22..7de56789d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -576,8 +576,8 @@ dependencies = [ "aws-sdk-ssooidc", "aws-sdk-sts", "aws-smithy-async", - "aws-smithy-http 0.63.4", - "aws-smithy-json 0.62.4", + "aws-smithy-http", + "aws-smithy-json", "aws-smithy-runtime", "aws-smithy-runtime-api", "aws-smithy-types", @@ -638,7 +638,7 @@ dependencies = [ "aws-sigv4", "aws-smithy-async", "aws-smithy-eventstream", - "aws-smithy-http 0.63.4", + "aws-smithy-http", "aws-smithy-runtime", "aws-smithy-runtime-api", "aws-smithy-types", @@ -666,8 +666,8 @@ dependencies = [ "aws-runtime", "aws-smithy-async", "aws-smithy-eventstream", - "aws-smithy-http 0.63.4", - "aws-smithy-json 0.62.4", + "aws-smithy-http", + "aws-smithy-json", "aws-smithy-observability", "aws-smithy-runtime", "aws-smithy-runtime-api", @@ -683,9 +683,9 @@ dependencies = [ [[package]] name = "aws-sdk-s3" -version = "1.119.0" +version = "1.122.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1d65fddc3844f902dfe1864acb8494db5f9342015ee3ab7890270d36fbd2e01c" +checksum = "94c2ca0cba97e8e279eb6c0b2d0aa10db5959000e602ab2b7c02de6b85d4c19b" dependencies = [ "aws-credential-types", "aws-runtime", @@ -693,8 +693,9 @@ dependencies = [ "aws-smithy-async", "aws-smithy-checksums", "aws-smithy-eventstream", - "aws-smithy-http 0.62.6", - "aws-smithy-json 0.61.9", + "aws-smithy-http", + "aws-smithy-json", + "aws-smithy-observability", "aws-smithy-runtime", "aws-smithy-runtime-api", "aws-smithy-types", @@ -706,7 +707,7 @@ dependencies = [ "hmac 0.12.1", "http 0.2.12", "http 1.4.0", - "http-body 0.4.6", + "http-body 1.0.1", "lru", "percent-encoding", "regex-lite", @@ -724,8 +725,8 @@ dependencies = [ "aws-credential-types", "aws-runtime", "aws-smithy-async", - "aws-smithy-http 0.63.4", - "aws-smithy-json 0.62.4", + "aws-smithy-http", + "aws-smithy-json", "aws-smithy-observability", "aws-smithy-runtime", "aws-smithy-runtime-api", @@ -748,8 +749,8 @@ dependencies = [ "aws-credential-types", "aws-runtime", "aws-smithy-async", - "aws-smithy-http 0.63.4", - "aws-smithy-json 0.62.4", + "aws-smithy-http", + "aws-smithy-json", "aws-smithy-observability", "aws-smithy-runtime", "aws-smithy-runtime-api", @@ -772,8 +773,8 @@ dependencies = [ "aws-credential-types", "aws-runtime", "aws-smithy-async", - "aws-smithy-http 0.63.4", - "aws-smithy-json 0.62.4", + "aws-smithy-http", + "aws-smithy-json", "aws-smithy-observability", "aws-smithy-query", "aws-smithy-runtime", @@ -796,7 +797,7 @@ checksum = "68f6ae9b71597dc5fd115d52849d7a5556ad9265885ad3492ea8d73b93bbc46e" dependencies = [ "aws-credential-types", "aws-smithy-eventstream", - "aws-smithy-http 0.63.4", + "aws-smithy-http", "aws-smithy-runtime-api", "aws-smithy-types", "bytes", @@ -829,17 +830,18 @@ dependencies = [ [[package]] name = "aws-smithy-checksums" -version = "0.63.12" +version = "0.64.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "87294a084b43d649d967efe58aa1f9e0adc260e13a6938eb904c0ae9b45824ae" +checksum = "a764fa7222922f6c0af8eea478b0ef1ba5ce1222af97e01f33ca5e957bd7f3b9" dependencies = [ - "aws-smithy-http 0.62.6", + "aws-smithy-http", "aws-smithy-types", "bytes", "crc-fast", "hex", - "http 0.2.12", - "http-body 0.4.6", + "http 1.4.0", + "http-body 1.0.1", + "http-body-util", "md-5 0.10.6", "pin-project-lite", "sha1", @@ -858,28 +860,6 @@ dependencies = [ "crc32fast", ] -[[package]] -name = "aws-smithy-http" -version = "0.62.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "826141069295752372f8203c17f28e30c464d22899a43a0c9fd9c458d469c88b" -dependencies = [ - "aws-smithy-eventstream", - "aws-smithy-runtime-api", - "aws-smithy-types", - "bytes", - "bytes-utils", - "futures-core", - "futures-util", - "http 0.2.12", - "http 1.4.0", - "http-body 0.4.6", - "percent-encoding", - "pin-project-lite", - "pin-utils", - "tracing", -] - [[package]] name = "aws-smithy-http" version = "0.63.4" @@ -932,15 +912,6 @@ dependencies = [ "tracing", ] -[[package]] -name = "aws-smithy-json" -version = "0.61.9" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "49fa1213db31ac95288d981476f78d05d9cbb0353d22cdf3472cc05bb02f6551" -dependencies = [ - "aws-smithy-types", -] - [[package]] name = "aws-smithy-json" version = "0.62.4" @@ -976,7 +947,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8fd3dfc18c1ce097cf81fced7192731e63809829c6cbf933c1ec47452d08e1aa" dependencies = [ "aws-smithy-async", - "aws-smithy-http 0.63.4", + "aws-smithy-http", "aws-smithy-http-client", "aws-smithy-observability", "aws-smithy-runtime-api", @@ -1974,9 +1945,9 @@ dependencies = [ [[package]] name = "crc" -version = "3.4.0" +version = "3.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5eb8a2a1cd12ab0d987a5d5e825195d372001a4094a0376319d5a0ad71c1ba0d" +checksum = "9710d3b3739c2e349eb44fe848ad0b7c8cb1e42bd87ee49371df2f7acaf3e675" dependencies = [ "crc-catalog", ] @@ -1989,15 +1960,14 @@ checksum = "19d374276b40fb8bbdee95aef7c7fa6b5316ec764510eb64b8dd0e2ed0d7e7f5" [[package]] name = "crc-fast" -version = "1.6.0" +version = "1.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6ddc2d09feefeee8bd78101665bd8645637828fa9317f9f292496dbbd8c65ff3" +checksum = "2fd92aca2c6001b1bf5ba0ff84ee74ec8501b52bbef0cac80bf25a6c1d87a83d" dependencies = [ "crc", "digest 0.10.7", - "rand 0.9.4", - "regex", "rustversion", + "spin 0.10.1", ] [[package]] @@ -2797,7 +2767,7 @@ version = "0.11.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "da0e4dd2a88388a1f4ccc7c9ce104604dab68d9f408dc34cd45823d5a9069095" dependencies = [ - "spin", + "spin 0.9.8", ] [[package]] @@ -2812,6 +2782,12 @@ version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d9c4f5dac5e15c24eb999c26181a6ca40b39fe946cbe4c263c7209467bc83af2" +[[package]] +name = "foldhash" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "77ce24cb58228fbb8aa041425bb1050850ac19177686ea6e0f41a70416f56fdb" + [[package]] name = "foreign-types" version = "0.3.2" @@ -3118,7 +3094,18 @@ checksum = "9229cfe53dfd69f0609a49f65461bd93001ea1ef889cd5529dd176593f5338a1" dependencies = [ "allocator-api2", "equivalent", - "foldhash", + "foldhash 0.1.5", +] + +[[package]] +name = "hashbrown" +version = "0.16.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "841d1cc9bed7f9236f321df977030373f4a4163ae1a7dbfe1a51a2c1a51d9100" +dependencies = [ + "allocator-api2", + "equivalent", + "foldhash 0.2.0", ] [[package]] @@ -4602,11 +4589,11 @@ checksum = "5e5032e24019045c762d3c0f28f5b6b8bbf38563a65908389bf7978758920897" [[package]] name = "lru" -version = "0.12.5" +version = "0.16.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "234cf4f4a04dc1f57e24b96cc0cd600cf2af460d4161ac5ecdd0af8e1f3b2a38" +checksum = "7f66e8d5d03f609abc3a39e6f08e4164ebf1447a732906d39eb9b99b7919ef39" dependencies = [ - "hashbrown 0.15.5", + "hashbrown 0.16.1", ] [[package]] @@ -7180,6 +7167,12 @@ dependencies = [ "lock_api", ] +[[package]] +name = "spin" +version = "0.10.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "023a211cb3138dbc438680b32560ad89f699977624c9f8dbb95a47d5b4c07dd3" + [[package]] name = "spki" version = "0.6.0" diff --git a/crates/persistence/tests/s3_es_tests.rs b/crates/persistence/tests/s3_es_tests.rs index d7d2cba27..ee7a4fb8e 100644 --- a/crates/persistence/tests/s3_es_tests.rs +++ b/crates/persistence/tests/s3_es_tests.rs @@ -22,9 +22,9 @@ use serde_json::json; use helios_persistence::backends::elasticsearch::{ElasticsearchBackend, ElasticsearchConfig}; use helios_persistence::backends::s3::{S3Backend, S3BackendConfig, S3TenancyMode}; use helios_persistence::composite::{ - CompositeConfig, CompositeStorage, DynSearchProvider, DynStorage, + CompositeConfig, CompositeStorage, DynSearchProvider, DynStorage, SyncMode, }; -use helios_persistence::core::search::SearchProvider; +use helios_persistence::core::search::{SearchProvider, SearchResult}; use helios_persistence::core::{Backend, BackendKind, ResourceStorage}; use helios_persistence::error::{ResourceError, StorageError}; use helios_persistence::search::{SearchParameterLoader, TenantSearchRegistries}; @@ -277,10 +277,17 @@ async fn make_harness(scope: &str) -> S3EsHarness { .await .expect("initialize ES backend"); - // Composite + // Composite. Sync mode must be explicit: the default is Asynchronous, + // which requires start_sync_workers() (never called here) — without it + // every secondary sync fails with "async worker was never started", the + // composite swallows the error (primary succeeded), and nothing ever + // reaches ES, so every positive search assertion fails. Synchronous + // indexing is also what these tests want: create/update/delete must be + // searchable as soon as ES refreshes. let composite_config = CompositeConfig::builder() .primary("s3", BackendKind::S3) .search_backend("es", BackendKind::Elasticsearch) + .sync_mode(SyncMode::Synchronous) .build() .expect("build composite config"); @@ -304,6 +311,34 @@ fn tenant(id: &str) -> TenantContext { TenantContext::new(TenantId::new(id), TenantPermissions::full_access()) } +/// Poll a search until `pred` holds or ~10s elapse, returning the last result. +/// The ES index is configured with a 1ms refresh interval, but on a loaded CI +/// runner refresh visibility can lag well past any fixed sleep (#390), so +/// visibility assertions poll instead of sleeping. On timeout the last result +/// is returned and the caller's assertion reports the real failure. +async fn search_until( + harness: &S3EsHarness, + tenant: &TenantContext, + query: &SearchQuery, + pred: F, +) -> SearchResult +where + F: Fn(&SearchResult) -> bool, +{ + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + loop { + let results = harness + .composite + .search(tenant, query) + .await + .expect("search should succeed"); + if pred(&results) || std::time::Instant::now() >= deadline { + return results; + } + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + } +} + // ============================================================================ // Tests // ============================================================================ @@ -333,9 +368,6 @@ async fn s3_es_test_create_then_search() { .await .expect("create should succeed"); - // Allow ES refresh (1ms interval configured, but give a little time) - tokio::time::sleep(std::time::Duration::from_millis(100)).await; - let query = SearchQuery::new("Patient").with_parameter(SearchParameter { name: "family".to_string(), param_type: SearchParamType::String, @@ -345,11 +377,7 @@ async fn s3_es_test_create_then_search() { components: vec![], }); - let results = harness - .composite - .search(&tenant, &query) - .await - .expect("search should succeed"); + let results = search_until(&harness, &tenant, &query, |r| !r.resources.items.is_empty()).await; assert!( !results.resources.items.is_empty(), @@ -398,8 +426,6 @@ async fn s3_es_test_update_then_search() { .await .expect("update should succeed"); - tokio::time::sleep(std::time::Duration::from_millis(100)).await; - // Search by new family name — should find it let query_new = SearchQuery::new("Patient").with_parameter(SearchParameter { name: "family".to_string(), @@ -409,11 +435,10 @@ async fn s3_es_test_update_then_search() { chain: vec![], components: vec![], }); - let results = harness - .composite - .search(&tenant, &query_new) - .await - .expect("search by new family should succeed"); + let results = search_until(&harness, &tenant, &query_new, |r| { + r.resources.items.iter().any(|res| res.id() == created.id()) + }) + .await; assert!( results .resources @@ -447,16 +472,6 @@ async fn s3_es_test_delete_then_search() { .await .expect("create should succeed"); - tokio::time::sleep(std::time::Duration::from_millis(100)).await; - - harness - .composite - .delete(&tenant, "Patient", created.id()) - .await - .expect("delete should succeed"); - - tokio::time::sleep(std::time::Duration::from_millis(100)).await; - let query = SearchQuery::new("Patient").with_parameter(SearchParameter { name: "family".to_string(), param_type: SearchParamType::String, @@ -465,11 +480,21 @@ async fn s3_es_test_delete_then_search() { chain: vec![], components: vec![], }); - let results = harness + + // Wait until the create is searchable so the post-delete absence check + // can't pass vacuously against a lagging index. + search_until(&harness, &tenant, &query, |r| !r.resources.items.is_empty()).await; + + harness .composite - .search(&tenant, &query) + .delete(&tenant, "Patient", created.id()) .await - .expect("search after delete should not error"); + .expect("delete should succeed"); + + let results = search_until(&harness, &tenant, &query, |r| { + r.resources.items.iter().all(|res| res.id() != created.id()) + }) + .await; assert!( results .resources @@ -606,8 +631,6 @@ async fn s3_es_test_multi_tenant_isolation() { .await .expect("create in tenant-a should succeed"); - tokio::time::sleep(std::time::Duration::from_millis(100)).await; - let query = SearchQuery::new("Patient").with_parameter(SearchParameter { name: "family".to_string(), param_type: SearchParamType::String, @@ -617,6 +640,18 @@ async fn s3_es_test_multi_tenant_isolation() { components: vec![], }); + // tenant-a should see its own data. Waiting for this FIRST also makes the + // tenant-b absence check below meaningful: once the document is visible to + // tenant-a, its absence for tenant-b is isolation, not index lag. + let results_a = search_until(&harness, &tenant_a, &query, |r| { + !r.resources.items.is_empty() + }) + .await; + assert!( + !results_a.resources.items.is_empty(), + "tenant-a should see its own resource" + ); + // tenant-b should NOT see tenant-a's data let results_b = harness .composite @@ -627,17 +662,6 @@ async fn s3_es_test_multi_tenant_isolation() { results_b.resources.items.is_empty(), "tenant-b should not see tenant-a's resource" ); - - // tenant-a should see its own data - let results_a = harness - .composite - .search(&tenant_a, &query) - .await - .expect("tenant-a search should succeed"); - assert!( - !results_a.resources.items.is_empty(), - "tenant-a should see its own resource" - ); } /// Read returns the resource after it was written to S3. diff --git a/crates/sof/Cargo.toml b/crates/sof/Cargo.toml index b9e268892..c76c82a79 100644 --- a/crates/sof/Cargo.toml +++ b/crates/sof/Cargo.toml @@ -51,14 +51,14 @@ chrono = { workspace = true, features = ["serde"] } reqwest = { version = "0.12", features = ["json"] } url = "2.5" # Cross-chunk cache for streaming remote resolve(). -# Pinned to 0.12 to MATCH aws-sdk-s3, which pins `lru = "^0.12"` as a non-optional -# dependency (pulled by the optional `s3` feature in helios-persistence/-rest). -# This keeps a single lru version compiled across all feature combinations, -# including `s3` builds. The latest lru is 0.18, but raising this would compile two -# lru versions whenever `s3` is enabled, since the AWS SDK's `^0.12` pin (a -# third-party crate we don't control) is semver-incompatible with 0.18. Bump to the -# latest once the AWS SDK updates its lru pin — see ROADMAP.md ("Build & Dependencies"). -lru = "0.12" +# Pinned to MATCH aws-sdk-s3, which pins lru as a non-optional dependency (pulled by +# the optional `s3` feature in helios-persistence/-rest). Keeping the two in the same +# semver range means a single lru version compiles across all feature combinations, +# including `s3` builds. This tracked the SDK's `^0.12` until aws-sdk-s3 1.122.0 moved +# to `^0.16`, so it moves with it; raising it beyond what the SDK pins would compile +# two lru versions whenever `s3` is enabled. The latest lru is 0.18 — bump to it only +# when the SDK does. See ROADMAP.md ("Build & Dependencies"). +lru = "0.16" async-trait = "0.1" object_store = { version = "0.11", features = ["aws", "gcp", "azure"] } rayon = "1.8"