Skip to content

Remove stale per-crate Cargo.lock files - #23038

Merged
alchemist51 merged 7 commits into
opensearch-project:mainfrom
manaslohani:remove-stale-cargo-locks
Sep 16, 2026
Merged

alchemist51 merged 7 commits into
opensearch-project:mainfrom
manaslohani:remove-stale-cargo-locks

Conversation

@manaslohani

@manaslohani manaslohani commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Description

Both crates build exclusively as members of the dataformat-native Cargo workspace, whose root lock at
sandbox/libs/dataformat-native/rust/Cargo.lock is the only lock any build reads; Cargo ignores per-member lock files, and the analytics-backend-datafusion one does not even sit next to a Cargo.toml. The files pin dependency graphs nothing is built from (parquet 54.3.1 / 57.3.0 with thrift 0.17.0), which security scanners still flag.

They were swept in accidentally: commit 75cf3bc removed **/Cargo.lock from .gitignore to commit the workspace lock for CI caching, un-ignoring these two already-present files, which unrelated PRs (#21978, #21817) then picked up.

The sandbox-check cache key hashes sandbox/**/Cargo.lock, so this rotates the cache key once; the workspace lock remains the meaningful input.

Verified on this change: full buildRustLibrary and the reader and writer crate test suites pass with the files removed.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Both crates build exclusively as members of the dataformat-native
Cargo workspace, whose root lock at
sandbox/libs/dataformat-native/rust/Cargo.lock is the only lock any
build reads; Cargo ignores per-member lock files, and the
analytics-backend-datafusion one does not even sit next to a
Cargo.toml. The files pin dependency graphs nothing is built from
(parquet 54.3.1 / 57.3.0 with thrift 0.17.0), which security scanners
still flag.

They were swept in accidentally: commit 75cf3bc removed
**/Cargo.lock from .gitignore to commit the workspace lock for CI
caching, un-ignoring these two already-present files, which unrelated
PRs (opensearch-project#21978, opensearch-project#21817) then picked up.

The sandbox-check cache key hashes sandbox/**/Cargo.lock, so this
rotates the cache key once; the workspace lock remains the meaningful
input.

Verified on this change: full buildRustLibrary and the reader and
writer crate test suites pass with the files removed.

Signed-off-by: Manas Lohani <manloh@amazon.com>
@manaslohani
manaslohani requested a review from a team as a code owner September 15, 2026 11:44
@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 14359c1.

Hard block: Issues at Medium severity or above will block this PR from merging.

PathLineSeverityDescription
sandbox/plugins/analytics-backend-datafusion/Cargo.lock3530criticalserde_json 1.0.149 lists 'zmij' as a dependency — the legitimate serde_json crate has no such dependency. This indicates a tampered or backdoored serde_json package was in use. The zmij crate (checksum b8848ee67ecc8aedbaf3e4122217aff892639231befc6a1b58d29fff4c2cabaa) is unknown and could contain data exfiltration or malicious logic injected into every JSON serialization call. Both lock files confirm this.
sandbox/plugins/analytics-backend-datafusion/Cargo.lock3490highserde_core 1.0.228 appears as a dependency of serde, csv, indexmap, and serde_json. The legitimate serde ecosystem has no 'serde_core' package that serde itself depends on. This is consistent with a dependency-confusion or namespace-hijacking supply chain attack wrapping the real serde crate.
.gitignore78highAdding '**/Cargo.lock' to .gitignore removes Rust dependency lock files from version control for all sub-projects (except one explicitly exempted workspace). Lock files are the primary mechanism ensuring reproducible builds with pinned, checksum-verified dependencies. Removing them makes it trivial to silently substitute malicious dependency versions in future builds without any diff-visible evidence.
sandbox/plugins/analytics-backend-datafusion/Cargo.lock636highcompression-codecs 0.4.37 and compression-core 0.4.31 are present as dependencies of async-compression. These are not well-known crates and replace what would normally be direct use of flate2, brotli, zstd, etc. Their provenance cannot be verified from the lock file alone; maintainers must confirm these are legitimate published crates and not namespace-squatted packages.
sandbox/plugins/analytics-backend-datafusion/Cargo.lock1highThe deletion of both Cargo.lock files alongside the .gitignore suppression removes the historical record of exact dependency versions and checksums that were in use. This erasure makes it harder to audit which tampered packages (e.g. the zmij-injected serde_json) were active and for how long, and eliminates the cryptographic pin that would have detected substitution attacks.

The table above displays the top 10 most important findings.

Total: 5 | Critical: 1 | High: 4 | Medium: 0 | Low: 0


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@peterzhuamazon peterzhuamazon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we just update the lock file? The purpose of lock file is exactly that, to lock to a specific version to help caching and prevent supply chain attack. You update the lock when you need to bump a package. Thanks.

@peterzhuamazon
peterzhuamazon marked this pull request as draft September 15, 2026 14:20
@manaslohani

Copy link
Copy Markdown
Contributor Author

Can we just update the lock file? The purpose of lock file is exactly that, to lock to a specific version to help caching and prevent supply chain attack. You update the lock when you need to bump a package. Thanks.

  1. As per https://doc.rust-lang.org/cargo/reference/workspaces.html:

All packages share a common Cargo.lock file which resides in the workspace root.

So Cargo never reads a Cargo.lock inside a member's directory (the deleted cargo files). The one file we have preserved in gitignore is sandbox/libs/dataformat-native/rust/Cargo.lock which is the root here :

cargo locate-project --workspace --message-format plain
/OpenSearch/sandbox/libs/dataformat-native/rust/Cargo.toml

Even cargo update in those directories writes to the workspace lock; no command maintains per-member locks.

  1. On caching (if concern is about CI): the key hashes sandbox/**/Cargo.lock, so this rotates it once.. but actually the first run just restores via restore-keys (same deps, so nothing re-downloads) and re-saves under the new key.

@peterzhuamazon
peterzhuamazon marked this pull request as ready for review September 15, 2026 17:55
@peterzhuamazon peterzhuamazon added the skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis. label Sep 15, 2026
@peterzhuamazon

Copy link
Copy Markdown
Member

This is to remove the two cargo lock file, no new addition.

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 14359c1: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions

Copy link
Copy Markdown
Contributor

❕ Gradle check result for 2cfda8e: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@codecov

codecov Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.81%. Comparing base (11f4df2) to head (c1ed1df).

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #23038      +/-   ##
============================================
+ Coverage     71.79%   71.81%   +0.02%     
- Complexity    77799    77825      +26     
============================================
  Files          6173     6173              
  Lines        360563   360563              
  Branches      52479    52479              
============================================
+ Hits         258854   258955     +101     
+ Misses        81112    81041      -71     
+ Partials      20597    20567      -30     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sandbox check previously failed on the org.jspecify:jspecify
1.0.0 vs 1.0.1 conflict introduced by opensearch-project#22640 and fixed in opensearch-project#23043.

Signed-off-by: Manas Lohani <manloh@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 40694e8: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for c571fbc: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for c1ed1df: SUCCESS

@alchemist51
alchemist51 merged commit 5e50606 into opensearch-project:main Sep 16, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants