Fix/getobjectlength #920
Merged
Fix/getobjectlength #920
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ed compression predicate and metadata improvements (#915) * Initial plan * Fix GetObject regression by excluding error responses from compression The issue was that CompressionLayer was attempting to compress error responses, which could cause Content-Length header mismatches. By excluding 4xx and 5xx responses from compression, we ensure error responses (like NoSuchKey) are sent correctly without body truncation. Co-authored-by: houseme <[email protected]> * Enhance NoSuchKey fix with improved compression predicate and comprehensive tests - Enhanced ShouldCompress predicate with size-based exclusion (< 256 bytes) - Added detailed documentation explaining the compression logic - Added debug logging for better observability - Created comprehensive test suite with 4 test cases: - test_get_deleted_object_returns_nosuchkey - test_head_deleted_object_returns_nosuchkey - test_get_nonexistent_object_returns_nosuchkey - test_multiple_gets_deleted_object - Added extensive inline documentation and comments - Created docs/fix-nosuchkey-regression.md with full analysis Co-authored-by: houseme <[email protected]> * Add compression best practices documentation Added comprehensive guide covering: - Best practices for HTTP response compression - Common pitfalls and solutions - Performance considerations and trade-offs - Testing guidelines and examples - Monitoring and alerting recommendations - Migration guide for existing services Co-authored-by: houseme <[email protected]> * fix * fmt * fmt * Fix/objectdelete (#917) * fix getobject content length resp * fix delete object --------- Co-authored-by: houseme <[email protected]> * Add comprehensive analysis of NoSuchKey fix and related improvements Created detailed documentation analyzing: - HTTP compression layer fix (primary issue) - Content-length calculation fix from PR #917 - Delete object metadata fixes from PR #917 - How all components work together - Complete scenario walkthrough - Performance impact analysis - Testing strategy and deployment checklist This ties together all the changes in the PR branch including the merged improvements from PR #917. Co-authored-by: houseme <[email protected]> * replace `once_cell` to `std` * fmt --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: houseme <[email protected]> Co-authored-by: houseme <[email protected]> Co-authored-by: weisd <[email protected]>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes Issue #901, a critical regression in RustFS 1.0.69 where GetObject operations on deleted or non-existent objects returned networking errors instead of the expected NoSuchKey S3 error. The fix implements intelligent compression filtering and includes a migration from the external once_cell crate to Rust's standard library synchronization primitives.
Key Changes
- Implemented
ShouldCompresspredicate to exclude error responses (4xx/5xx) and small responses (<256 bytes) from HTTP compression - Migrated from
once_cellcrate to stdlibOnceLockandLazyLockacross the lock crate - Added comprehensive end-to-end tests for deleted object scenarios with 4 test cases
- Extensive documentation covering the fix rationale, best practices, and deployment strategy
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| rustfs/src/server/http.rs | Added ShouldCompress predicate implementation to prevent compression of error responses and small payloads, fixing Content-Length mismatch issues |
| crates/lock/src/lib.rs | Migrated from once_cell::sync::OnceCell to std::sync::OnceLock for global lock manager singleton |
| crates/lock/src/guard.rs | Replaced once_cell::sync::Lazy with std::sync::LazyLock for unlock runtime initialization |
| crates/lock/src/fast_lock/types.rs | Changed hash cache from once_cell::unsync::OnceCell to std::sync::OnceLock in OptimizedObjectKey |
| crates/lock/src/fast_lock/optimized_notify.rs | Updated notification pool from once_cell::sync::Lazy to std::sync::LazyLock |
| crates/lock/Cargo.toml | Removed once_cell dependency from lock crate |
| crates/e2e_test/src/reliant/get_deleted_object_test.rs | Added 4 comprehensive test cases covering GetObject and HeadObject on deleted and non-existent objects |
| crates/e2e_test/src/reliant/mod.rs | Registered new test module for deleted object tests |
| Cargo.toml | Removed once_cell from workspace dependencies |
| Cargo.lock | Updated lock file to reflect once_cell removal |
| docs/nosuchkey-fix-comprehensive-analysis.md | Comprehensive 396-line analysis document covering root cause, solution, testing, and deployment strategy |
| docs/fix-nosuchkey-regression.md | Focused documentation on the NoSuchKey fix with implementation details and testing guidance |
| docs/compression-best-practices.md | Best practices guide for HTTP response compression with code examples and monitoring strategies |
| docs/COMPLETE_SUMMARY.md | Updated existing summary document with minor formatting improvements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type of Change
Related Issues
#901Summary of Changes
Checklist
make pre-commitImpact
Additional Notes
Thank you for your contribution! Please ensure your PR follows the community standards (CODE_OF_CONDUCT.md) and sign the CLA if this is your first contribution.