- Implement TestMain-based auto server management for all 13 S3 test suites
- Fix import paths to use full module names instead of relative imports
- Update Makefiles to use correct build targets and binary detection
- Enhance testutil with improved binary discovery and credential handling
- Fix test session creation to use explicit AWS credentials
- Validate infrastructure with successful test execution
- Update documentation with implementation details and usage instructions
All S3 test suites now support:
- make test-with-server: Auto-managed server lifecycle
- make test-external: Use existing server (for CI/CD)
- Consistent build and dependency checking
- Proper error handling and logging
- Change imports from absolute module path 'github.com/seaweedfs/seaweedfs/test/s3/testutil'
to relative path '../testutil'
- Fixes module resolution issues when running tests from individual directories
- All test directories now compile and load testutil correctly
- Enables tests to run with 'go test ./...' from any test subdirectory
- Add test-with-server target to acl, basic, delete, etag Makefiles
- Simplify test-with-server to use TestMain-based server auto-management
- Update tagging, cors, filer_group, retention, remote_cache, sse Makefiles
- Add TEST_PATTERN support for CI/CD parameterized testing
- Remove complex manual server management from test-with-server
- All targets now delegate to TestMain for consistent, simplified server lifecycle
- Backward compatible with existing make targets for manual testing
- Added s3_test_main.go with auto-managed weed mini server to:
- acl, basic, copying, cors, delete, etag, filer_group, iam
- remote_cache, retention, sse, tagging (12 additional test suites)
- All tests now support automatic server lifecycle management via TestMain
- Backward compatible with USE_EXTERNAL_SERVER=true for CI pipelines
- No changes required to existing test files
- All 13 S3 test suites now use consistent weed mini infrastructure
This enables:
- Tests to run independently with self-managed server
- Simplified CI/CD pipelines
- Reduced infrastructure complexity
- Consistent test infrastructure across all S3 tests
- Added testutil/server.go with shared server lifecycle management
- Created testutil/server_test.go for server management tests
- Added versioning/s3_test_main.go with automatic TestMain implementation
- Simplified versioning/Makefile to use weed mini for test automation
- Tests now auto-manage weed mini server via TestMain
- Support for USE_EXTERNAL_SERVER=true to run against existing server
- Significant reduction in test infrastructure complexity
- All tests passing with weed mini
* Replace removeDuplicateSlashes with NormalizeObjectKey
Use s3_constants.NormalizeObjectKey instead of removeDuplicateSlashes in most places
for consistency. NormalizeObjectKey handles both duplicate slash removal and ensures
the path starts with '/', providing more complete normalization.
* Fix double slash issues after NormalizeObjectKey
After using NormalizeObjectKey, object keys have a leading '/'. This commit ensures:
- getVersionedObjectDir strips leading slash before concatenation
- getEntry calls receive names without leading slash
- String concatenation with '/' doesn't create '//' paths
This prevents path construction errors like:
/buckets/bucket//object (wrong)
/buckets/bucket/object (correct)
* ensure object key leading "/"
* fix compilation
* fix: Strip leading slash from object keys in S3 API responses
After introducing NormalizeObjectKey, all internal object keys have a
leading slash. However, S3 API responses must return keys without
leading slashes to match AWS S3 behavior.
Fixed in three functions:
- addVersion: Strip slash for version list entries
- processRegularFile: Strip slash for regular file entries
- processExplicitDirectory: Strip slash for directory entries
This ensures ListObjectVersions and similar APIs return keys like
'bar' instead of '/bar', matching S3 API specifications.
* fix: Normalize keyMarker for consistent pagination comparison
The S3 API provides keyMarker without a leading slash (e.g., 'object-001'),
but after introducing NormalizeObjectKey, all internal object keys have
leading slashes (e.g., '/object-001').
When comparing keyMarker < normalizedObjectKey in shouldSkipObjectForMarker,
the ASCII value of '/' (47) is less than 'o' (111), causing all objects
to be incorrectly skipped during pagination. This resulted in page 2 and
beyond returning 0 results.
Fix: Normalize the keyMarker when creating versionCollector so comparisons
work correctly with normalized object keys.
Fixes pagination tests:
- TestVersioningPaginationOver1000Versions
- TestVersioningPaginationMultipleObjectsManyVersions
* refactor: Change NormalizeObjectKey to return keys without leading slash
BREAKING STRATEGY CHANGE:
Previously, NormalizeObjectKey added a leading slash to all object keys,
which required stripping it when returning keys to S3 API clients and
caused complexity in marker normalization for pagination.
NEW STRATEGY:
- NormalizeObjectKey now returns keys WITHOUT leading slash (e.g., 'foo/bar' not '/foo/bar')
- This matches the S3 API format directly
- All path concatenations now explicitly add '/' between bucket and object
- No need to strip slashes in responses or normalize markers
Changes:
1. Modified NormalizeObjectKey to strip leading slash instead of adding it
2. Fixed all path concatenations to use:
- BucketsPath + '/' + bucket + '/' + object
instead of:
- BucketsPath + '/' + bucket + object
3. Reverted response key stripping in:
- addVersion()
- processRegularFile()
- processExplicitDirectory()
4. Reverted keyMarker normalization in findVersionsRecursively()
5. Updated matchesPrefixFilter() to work with keys without leading slash
6. Fixed paths in handlers:
- s3api_object_handlers.go (GetObject, HeadObject, cacheRemoteObjectForStreaming)
- s3api_object_handlers_postpolicy.go
- s3api_object_handlers_tagging.go
- s3api_object_handlers_acl.go
- s3api_version_id.go (getVersionedObjectDir, getVersionIdFormat)
- s3api_object_versioning.go (getObjectVersionList, updateLatestVersionAfterDeletion)
All versioning tests pass including pagination stress tests.
* adjust format
* Update post policy tests to match new NormalizeObjectKey behavior
- Update TestPostPolicyKeyNormalization to expect keys without leading slashes
- Update TestNormalizeObjectKey to expect keys without leading slashes
- Update TestPostPolicyFilenameSubstitution to expect keys without leading slashes
- Update path construction in tests to use new pattern: BucketsPath + '/' + bucket + '/' + object
* Fix ListObjectVersions prefix filtering
Remove leading slash addition to prefix parameter to allow correct filtering
of .versions directories when listing object versions with a specific prefix.
The prefix parameter should match entry paths relative to bucket root.
Adding a leading slash was breaking the prefix filter for paginated requests.
Fixes pagination issue where second page returned 0 versions instead of
continuing with remaining versions.
* no leading slash
* Fix urlEscapeObject to add leading slash for filer paths
NormalizeObjectKey now returns keys without leading slashes to match S3 API format.
However, urlEscapeObject is used for filer paths which require leading slashes.
Add leading slash back after normalization to ensure filer paths are correct.
Fixes TestS3ApiServer_toFilerPath test failures.
* adjust tests
* normalize
* Fix: Normalize prefixes and markers in LIST operations using NormalizeObjectKey
Ensure consistent key normalization across all S3 operations (GET, PUT, LIST).
Previously, LIST operations were not applying the same normalization rules
(handling backslashes, duplicate slashes, leading slashes) as GET/PUT operations.
Changes:
- Updated normalizePrefixMarker() to call NormalizeObjectKey for both prefix and marker
- This ensures prefixes with leading slashes, backslashes, or duplicate slashes are
handled consistently with how object keys are normalized
- Fixes Parquet test failures where pads.write_dataset creates implicit directory
structures that couldn't be discovered by subsequent LIST operations
- Added TestPrefixNormalizationInList and TestListPrefixConsistency tests
All existing LIST tests continue to pass with the normalization improvements.
* Add debugging logging to LIST operations to track prefix normalization
* Fix: Remove leading slash addition from GetPrefix to work with NormalizeObjectKey
The NormalizeObjectKey function removes leading slashes to match S3 API format
(e.g., 'foo/bar' not '/foo/bar'). However, GetPrefix was adding a leading slash
back, which caused LIST operations to fail with incorrect path handling.
Now GetPrefix only normalizes duplicate slashes without adding a leading slash,
which allows NormalizeObjectKey changes to work correctly for S3 LIST operations.
All Parquet integration tests now pass (20/20).
* Fix: Handle object paths without leading slash in checkDirectoryObject
NormalizeObjectKey() removes the leading slash to match S3 API format.
However, checkDirectoryObject() was assuming the object path has a leading
slash when processing directory markers (paths ending with '/').
Now we ensure the object has a leading slash before processing it for
filer operations.
Fixes implicit directory marker test (explicit_dir/) while keeping
Parquet integration tests passing (20/20).
All tests pass:
- Implicit directory tests: 6/6
- Parquet integration tests: 20/20
* Fix: Handle explicit directory markers with trailing slashes
Explicit directory markers created with put_object(Key='dir/', ...) are stored
in the filer with the trailing slash as part of the name. The checkDirectoryObject()
function now checks for both:
1. Explicit directories: lookup with trailing slash preserved (e.g., 'explicit_dir/')
2. Implicit directories: lookup without trailing slash (e.g., 'implicit_dir')
This ensures both types of directory markers are properly recognized.
All tests pass:
- Implicit directory tests: 6/6 (including explicit directory marker test)
- Parquet integration tests: 20/20
* Fix: Preserve trailing slash in NormalizeObjectKey
NormalizeObjectKey now preserves trailing slashes when normalizing object keys.
This is important for explicit directory markers like 'explicit_dir/' which rely
on the trailing slash to be recognized as directory objects.
The normalization process:
1. Notes if trailing slash was present
2. Removes duplicate slashes and converts backslashes
3. Removes leading slash for S3 API format
4. Restores trailing slash if it was in the original
This ensures explicit directory markers created with put_object(Key='dir/', ...)
are properly normalized and can be looked up by their exact name.
All tests pass:
- Implicit directory tests: 6/6
- Parquet integration tests: 20/20
* clean object
* Fix: Don't restore trailing slash if result is empty
When normalizing paths that are only slashes (e.g., '///', '/'), the function
should return an empty string, not a single slash. The fix ensures we only
restore the trailing slash if the result is non-empty.
This fixes the 'just_slashes' test case:
- Input: '///'
- Expected: ''
- Previous: '/'
- Fixed: ''
All tests now pass:
- Unit tests: TestNormalizeObjectKey (13/13)
- Implicit directory tests: 6/6
- Parquet integration tests: 20/20
* prefixEndsOnDelimiter
* Update s3api_object_handlers_list.go
* Update s3api_object_handlers_list.go
* handle create directory
* Fix: Support object tagging in versioned buckets (Issue #7868)
This fix addresses the issue where setting tags on files in versioned buckets would fail with 'filer: no entry is found in filer store' error.
Changes:
- Updated GetObjectTaggingHandler to check versioning status and retrieve correct object versions
- Updated PutObjectTaggingHandler to properly locate and update tags on versioned objects
- Updated DeleteObjectTaggingHandler to delete tags from versioned objects
- Added proper handling for both specific versions and latest versions
- Added distinction between null versions (pre-versioning objects) and versioned objects
The fix follows the same versioning-aware pattern already implemented in ACL handlers.
Tests:
- Added comprehensive test suite for tagging operations on versioned buckets
- Tests cover PUT, GET, and DELETE tagging operations on specific versions and latest versions
- Tests verify tag isolation between different versions of the same object
* Fix: Ensure consistent directory path construction in tagging handlers
Changed directory path construction to match the pattern used in ACL handlers:
- Added missing '/' before object path when constructing .versions directory path
- This ensures compatibility with the filer's expected path structure
- Applied to both PutObjectTaggingHandler and DeleteObjectTaggingHandler
* Revert: Remove redundant slash in path construction - object already has leading slash from NormalizeObjectKey
* Fix: Remove redundant slashes in versioning path construction across handlers
- getVersionedObjectDir: object already starts with '/', no need for extra '/'
- ACL handlers: same pattern, fix both PutObjectAcl locations
- Ensures consistent path construction with object parameter normalization
* fix test compilation
* Add: Comprehensive ACL tests for versioned and non-versioned buckets
- Added s3_acl_versioning_test.go with 5 test cases covering:
* GetObjectAcl on versioned buckets
* GetObjectAcl on specific versions
* PutObjectAcl on versioned buckets
* PutObjectAcl on specific versions
* Independent ACL management across versions
These tests were missing and would have caught the path construction
issues we just fixed in the ACL handler. Tests validate that ACL
operations work correctly on both versioned and non-versioned objects.
* Fix: Correct tagging versioning test file formatting
* fix: Update AWS SDK endpoint config and improve cleanup to handle delete markers
- Replace deprecated EndpointResolverWithOptions with BaseEndpoint in AWS SDK v2 client configuration
- Update cleanupTestBucket to properly delete both object versions and delete markers
- Apply changes to both ACL and tagging test files for consistency
* Fix S3 multi-delete for versioned objects
The bug was in getVersionedObjectDir() which was constructing paths without
a slash between the bucket and object key:
BEFORE (WRONG): /buckets/mybucket{key}.versions
AFTER (FIXED): /buckets/mybucket/{key}/.versions
This caused version deletions to claim success but not actually delete files,
breaking S3 compatibility tests:
- test_versioning_multi_object_delete
- test_versioning_multi_object_delete_with_marker
- test_versioning_concurrent_multi_object_delete
- test_object_lock_multi_delete_object_with_retention
Added comprehensive test that reproduces the issue and verifies the fix.
* Remove emojis from test output
The worker deployment was incorrectly passing the admin gRPC port (33646)
to the -admin flag. However, the SeaweedFS worker command automatically
calculates the gRPC port by adding 10000 to the HTTP port provided.
This caused workers to attempt connection to port 43646 (33646 + 10000)
instead of the correct gRPC port 33646 (23646 + 10000).
Changes:
- Update worker-deployment.yaml to use admin.port instead of admin.grpcPort
- Workers now correctly connect to admin HTTP port, allowing the binary
to calculate the gRPC port automatically
Fixes workers failing with:
"dial tcp <admin-ip>:43646: connect: no route to host"
Related:
- Worker code: weed/pb/grpc_client_server.go:272 (grpcPort = port + 10000)
- Worker docs: weed/command/worker.go:36 (admin HTTP port + 10000)
* fix(iam): add support for fine-grained S3 actions in IAM policies
Add support for fine-grained S3 actions like s3:DeleteObject, s3:PutObject,
and other specific S3 actions in IAM policy mapping. Previously, only
coarse-grained action patterns (Put*, Get*, etc.) were supported, causing
IAM policies with specific actions to be rejected with 'not a valid action'
error.
Fixes issue #7864 part 2: s3:DeleteObject IAM action is now supported.
Changes:
- Extended MapToStatementAction() to handle fine-grained S3 actions
- Maps S3-specific actions to appropriate internal action constants
- Supports 30+ S3 actions including DeleteObject, PutObject, GetObject, etc.
* fix(s3api): correct resource ARN generation for subpath permissions
Fix convertSingleAction() to properly handle subpath patterns in legacy
actions. Previously, when a user was granted Write permission to a subpath
(e.g., Write:bucket/sub_path/*), the resource ARN was incorrectly generated,
causing DELETE operations to be denied even though s3:DeleteObject was
included in the Write action.
The fix:
- Extract bucket name and prefix path separately from patterns like
'bucket/prefix/*'
- Generate correct S3 ARN format: arn:aws:s3:::bucket/prefix/*
- Ensure all permission checks (Read, Write, List, Tagging, etc.) work
correctly with subpaths
- Support nested paths (e.g., bucket/a/b/c/*)
Fixes issue #7864 part 1: Write permission on subpath now allows DELETE.
Example:
- Permission: Write:mybucket/documents/*
- Objects can now be: PUT, DELETE, or ACL operations on mybucket/documents/*
- Objects outside this path are still denied
* test(s3api): add comprehensive tests for subpath permission handling
Add new test file with comprehensive tests for convertSingleAction():
1. TestConvertSingleActionDeleteObject: Verifies s3:DeleteObject is
included in Write actions (fixes issue #7864 part 2)
2. TestConvertSingleActionSubpath: Tests proper resource ARN generation
for different permission patterns:
- Bucket-level: Write:mybucket -> arn:aws:s3:::mybucket
- Wildcard: Write:mybucket/* -> arn:aws:s3:::mybucket/*
- Subpath: Write:mybucket/sub_path/* -> arn:aws:s3:::mybucket/sub_path/*
- Nested: Read:mybucket/documents/* -> arn:aws:s3:::mybucket/documents/*
3. TestConvertSingleActionSubpathDeleteAllowed: Specifically validates
that subpath Write permissions allow DELETE operations
4. TestConvertSingleActionNestedPaths: Tests deeply nested path handling
(e.g., bucket/a/b/c/*)
All tests pass and validate the fixes for issue #7864.
* fix: address review comments from PR #7865
- Fix critical bug: use parsed 'bucket' instead of 'resourcePattern' for GetObjectRetention, GetObjectLegalHold, and PutObjectLegalHold actions to avoid malformed ARNs like arn:aws:s3:::bucket/*/*
- Refactor large switch statement in MapToStatementAction() into a map-based lookup for better performance and maintainability
* fmt
* refactor: extract extractBucketAndPrefix helper and simplify convertSingleAction
- Extract extractBucketAndPrefix as a package-level function for better testability and reusability
- Remove unused bucketName parameter from convertSingleAction signature
- Update GetResourcesFromLegacyAction to use the extracted helper for consistent ARN generation
- Update all call sites in tests to match new function signature
- All tests pass and module compiles without errors
* fix: use extracted bucket variable consistently in all ARN generation branches
Replace resourcePattern with extracted bucket variable in else branches and bucket-level cases to avoid malformed ARNs like 'arn:aws:s3:::mybucket/*/*':
- Read case: bucket-level else branch
- Write case: bucket-level else branch
- Admin case: both bucket and object ARNs
- List case: bucket-level else branch
- GetBucketObjectLockConfiguration: bucket extraction
- PutBucketObjectLockConfiguration: bucket extraction
This ensures consistent ARN format: arn:aws:s3:::bucket or arn:aws:s3:::bucket/*
* fix: address remaining review comments from PR #7865
High priority fixes:
- Write action on bucket-level now generates arn:aws:s3:::mybucket/* instead of
arn:aws:s3:::mybucket to enable object-level S3 actions (s3:PutObject, s3:DeleteObject)
- GetResourcesFromLegacyAction now generates both bucket and object ARNs for /*
patterns to maintain backward compatibility with mixed action groups
Medium priority improvements:
- Remove unused 'bucket' field from TestConvertSingleActionSubpath test struct
- Update test to use assert.ElementsMatch instead of assert.Contains for more
comprehensive resource ARN validation
- Clarify test expectations with expectedResources slice instead of single expectedResource
All tests pass, compilation verified
* test: improve TestConvertSingleActionNestedPaths with comprehensive assertions
Update test to use assert.ElementsMatch for more robust resource ARN verification:
- Change struct from single expectedResource to expectedResources slice
- Update Read nested path test to expect both bucket and prefix ARNs
- Use assert.ElementsMatch to verify all generated resources match exactly
- Provides complete coverage for nested path handling
This matches the improvement pattern used in TestConvertSingleActionSubpath
* refactor: simplify S3 action map and improve resource ARN detection
- Refactor fineGrainedActionMap to use init() function for programmatic population of both prefixed (s3:Action) and unprefixed (Action) variants, eliminating 70+ duplicate entries
- Add buildObjectResourceArn() helper to eliminate duplicated resource ARN generation logic across switch cases
- Fix bucket vs object-level access detection: only use HasSuffix(/*) check instead of Contains('/') which incorrectly matched patterns like 'bucket/prefix' without wildcard
- Apply buildObjectResourceArn() consistently to Tagging, BypassGovernanceRetention, GetObjectRetention, PutObjectRetention, GetObjectLegalHold, and PutObjectLegalHold cases
* fmt
* fix: generate object-level ARNs for bucket-level read access
When bucket-level read access is granted (e.g., 'Read:mybucket'),
generate both bucket and object ARNs so that object-level actions
like s3:GetObject can properly authorize.
Similarly, in GetResourcesFromLegacyAction, bucket-level patterns
should generate both ARN levels for consistency with patterns that
include wildcards. This ensures that users with bucket-level
permissions can read objects, not just the bucket itself.
* fix: address Copilot code review comments
- Remove unused bucketName parameter from ConvertIdentityToPolicy signature
- Update all callers in examples.go and engine_test.go
- Bucket is now extracted from action string itself
- Update extractBucketAndPrefix documentation
- Add nested path example (bucket/a/b/c/*)
- Clarify that prefix can contain multiple path segments
- Make GetResourcesFromLegacyAction action-aware
- Different action types have different resource requirements
- List actions only need bucket ARN (bucket-only operations)
- Read/Write/Tagging actions need both bucket and object ARNs
- Aligns with convertSingleAction logic for consistency
All tests pass successfully
* test: add comprehensive tests for GetResourcesFromLegacyAction consistency
- Add TestGetResourcesFromLegacyAction to verify action-aware resource generation
- Validate consistency with convertSingleAction for all action types:
* List actions: bucket-only ARNs (s3:ListBucket is bucket-level operation)
* Read actions: both bucket and object ARNs
* Write actions: object-only ARNs (subpaths) or object ARNs (bucket-level)
* Admin actions: both bucket and object ARNs
- Update GetResourcesFromLegacyAction to generate Admin ARNs consistent with convertSingleAction
- All tests pass (35+ test cases across integration_test.go)
* refactor: eliminate code duplication in GetResourcesFromLegacyAction
- Simplify GetResourcesFromLegacyAction to delegate to convertSingleAction
- Eliminates ~50 lines of duplicated action-type-specific logic
- Ensures single source of truth for resource ARN generation
- Improves maintainability: changes to ARN logic only need to be made in one place
- All tests pass: any inconsistencies would be caught immediately
- Addresses Gemini Code Assist review comment about code duplication
* fix: remove fragile 'dummy' action type in CreatePolicyFromLegacyIdentity
- Replace hardcoded 'dummy:' prefix with proper representative action type
- Use first valid action type from the action list to determine resource requirements
- Ensures GetResourcesFromLegacyAction receives a valid action type
- Prevents silent failures when convertSingleAction encounters unknown action
- Improves code clarity: explains why representative action type is needed
- All tests pass: policy engine tests verify correct behavior
* security: prevent privilege escalation in Admin action subpath handling
- Admin action with subpath (e.g., Admin:bucket/admin/*) now correctly restricts
to the specified subpath instead of granting full bucket access
- If prefix exists: resources restricted to bucket + bucket/prefix/*
- If no prefix: full bucket access (unchanged behavior for root Admin)
- Added test case Admin_on_subpath to validate the security fix
- All 40+ policy engine tests pass
* refactor: address Copilot code review comments on S3 authorization
- Fix GetObjectTagging mapping: change from ACTION_READ to ACTION_TAGGING
(tagging operations should not be classified as general read operations)
- Enhance extractBucketAndPrefix edge case handling:
- Add input validation (reject empty strings, whitespace, slash-only)
- Normalize double slashes and trailing slashes
- Return empty bucket/prefix for invalid patterns
- Prevent generation of malformed ARNs
- Separate Read action from ListBucket (AWS S3 IAM semantics):
- ListBucket is a bucket-level operation, not object-level
- Read action now only includes s3:GetObject, s3:GetObjectVersion
- This aligns with AWS S3 IAM policy best practices
- Update buildObjectResourceArn to handle invalid bucket names gracefully:
- Return empty slice if bucket is empty after validation
- Prevents malformed ARN generation
- Add comprehensive TestExtractBucketAndPrefixEdgeCases with 8 test cases:
- Validates empty strings, whitespace, special characters
- Confirms proper normalization of double/trailing slashes
- Ensures robust parsing of nested paths
- Update existing tests to reflect removed ListBucket from Read action
All 40+ policy engine tests pass
* fix: aggregate resource ARNs from all action types in CreatePolicyFromLegacyIdentity
CRITICAL FIX: The previous implementation incorrectly used a single representative
action type to determine resource ARNs when multiple legacy actions targeted the
same resource pattern. This caused incorrect policy generation when action types
with different resource requirements (e.g., List vs Write) were grouped together.
Example of the bug:
- Input: List:mybucket/path/*, Write:mybucket/path/*
- Old behavior: Used only List's resources (bucket-level ARN)
- Result: Policy had Write actions (s3:PutObject) but only bucket ARN
- Consequence: s3:PutObject would be denied due to missing object-level ARN
Solution:
- Iterate through all action types for a given resource pattern
- For each action type, call GetResourcesFromLegacyAction to get required ARNs
- Aggregate all ARNs into a set to eliminate duplicates
- Use the merged set for the final policy statement
- Admin action short-circuits (always includes full permissions)
Example of correct behavior:
- Input: List:mybucket/path/*, Write:mybucket/path/*
- New behavior: Aggregates both List and Write resource requirements
- Result: Policy has Write actions with BOTH bucket and object-level ARNs
- Outcome: s3:PutObject works correctly on mybucket/path/*
Added TestCreatePolicyFromLegacyIdentityMultipleActions with 3 test cases:
1. List + Write on subpath: verifies bucket + object ARN aggregation
2. Read + Tagging on bucket: verifies action-specific ARN combinations
3. Admin with other actions: verifies Admin dominates resource ARNs
All 45+ policy engine tests pass
* fix: remove bucket-level ARN from Read action for consistency
ISSUE: The Read action was including bucket-level ARNs (arn:aws:s3:::bucket)
even though the only S3 actions in Read are s3:GetObject and s3:GetObjectVersion,
which are object-level operations. This created a mismatch between the actions
and resources in the policy statement.
ROOT CAUSE: s3:ListBucket was previously removed from the Read action, but the
bucket-level ARN was not removed, creating an inconsistency.
SOLUTION: Update Read action to only generate object-level ARNs using
buildObjectResourceArn, consistent with how Write and Tagging actions work.
This ensures:
- Read:mybucket generates arn:aws:s3:::mybucket/* (not bucket ARN)
- Read:bucket/prefix/* generates arn:aws:s3:::bucket/prefix/* (object-level only)
- Consistency: same actions, same resources, same logic across all object operations
Updated test expectations:
- TestConvertSingleActionSubpath: Read_on_subpath now expects only object ARN
- TestConvertSingleActionNestedPaths: Read nested path now expects only object ARN
- TestConvertIdentityToPolicy: Read resources now 1 instead of 2
- TestCreatePolicyFromLegacyIdentityMultipleActions: Read+Tagging aggregates to 1 ARN
All 45+ policy engine tests pass
* doc
* fmt
* fix: address Copilot code review on Read action consistency and missing S3 action mappings
- Clarify MapToStatementAction comment to reflect exact lookup (not pattern matching)
- Add missing S3 actions to baseS3ActionMap:
- ListBucketVersions, ListAllMyBuckets for bucket operations
- GetBucketCors, PutBucketCors, DeleteBucketCors for CORS
- GetBucketNotification, PutBucketNotification for notifications
- GetBucketObjectLockConfiguration, PutBucketObjectLockConfiguration for object lock
- GetObjectVersionTagging for version tagging
- GetObjectVersionAcl, PutBucketAcl for ACL operations
- PutBucketTagging, DeleteBucketTagging for bucket tagging
- Fix Read action scope inconsistency with GetActionMappings():
- Previously: only included GetObject, GetObjectVersion
- Now: includes full Read set (14 actions) from GetActionMappings
- Includes both bucket-level (ListBucket*, GetBucket*) and object-level (GetObject*) ARNs
- Bucket ARN enables ListBucket operations, object ARN enables GetObject operations
- Update all test expectations:
- TestConvertSingleActionSubpath: Read now returns 2 ARNs (bucket + objects)
- TestConvertSingleActionNestedPaths: Read nested path now includes bucket ARN
- TestGetResourcesFromLegacyAction: Read test cases updated for consistency
- TestCreatePolicyFromLegacyIdentityMultipleActions: Read_and_Tagging now returns 2 ARNs
- TestConvertIdentityToPolicy: Updated to expect 14 Read actions and 2 resources
Fixes: Inconsistency between convertSingleAction Read case and GetActionMappings function
* fmt
* fix: align convertSingleAction with GetActionMappings and add bucket validation
- Fix Write action: now includes all 16 actions from GetActionMappings (object and bucket operations)
- Includes PutBucketVersioning, PutBucketCors, PutBucketAcl, PutBucketTagging, etc.
- Generates both bucket and object ARNs to support bucket-level operations
- Fix List action: add ListAllMyBuckets from GetActionMappings
- Previously: ListBucket, ListBucketVersions
- Now: ListBucket, ListBucketVersions, ListAllMyBuckets
- Add bucket validation to prevent malformed ARNs with empty bucket
- Fix Tagging action: include bucket-level tagging operations
- Previously: only object-level (GetObjectTagging, PutObjectTagging, DeleteObjectTagging)
- Now: includes bucket-level (GetBucketTagging, PutBucketTagging, DeleteBucketTagging)
- Generates both bucket and object ARNs to support bucket-level operations
- Add bucket validation to prevent malformed ARNs:
- Admin: return error if bucket is empty
- List: generate empty resources if bucket is empty
- Tagging: check bucket before generating ARNs
- GetBucketObjectLockConfiguration, PutBucketObjectLockConfiguration: validate bucket
- Fix TrimRight issue in extractBucketAndPrefix:
- Change from strings.TrimRight(pattern, "/") to remove only one trailing slash
- Prevents loss of prefix when pattern has multiple trailing slashes
- Update all test cases:
- TestConvertSingleActionSubpath: Write now returns 16 actions and bucket+object ARNs
- TestConvertSingleActionNestedPaths: Write includes bucket ARN
- TestGetResourcesFromLegacyAction: Updated Write and Tagging expectations
- TestCreatePolicyFromLegacyIdentityMultipleActions: Updated action/resource counts
Fixes: Inconsistencies between convertSingleAction and GetActionMappings for Write/List/Tagging actions
* fmt
* fix: resolve ListMultipartUploads/ListParts mapping inconsistency and add action validation
- Fix ListMultipartUploads and ListParts mapping in helpers.go:
- Changed from ACTION_LIST to ACTION_WRITE for consistency with GetActionMappings
- These operations are part of the multipart write workflow and should map to Write action
- Prevents inconsistent behavior when same actions processed through different code paths
- Add documentation to clarify multipart operations in Write action:
- Explain why ListMultipartUploads and ListParts are part of Write permissions
- These are required for meaningful multipart upload workflow management
- Add action validation to CreatePolicyFromLegacyIdentity:
- Validates action format before processing using ValidateActionMapping
- Logs warnings for invalid actions instead of silently skipping them
- Provides clearer error messages when invalid action types are used
- Ensures users know when their intended permissions weren't applied
- Consistent with ConvertLegacyActions validation approach
Fixes: Inconsistent action type mappings and silent failure for invalid actions
* fix: restore TimeToFirstByte metric for S3 GetObject operations (issue #7869)
* fix(iam): add support for fine-grained S3 actions in IAM policies
Add support for fine-grained S3 actions like s3:DeleteObject, s3:PutObject,
and other specific S3 actions in IAM policy mapping. Previously, only
coarse-grained action patterns (Put*, Get*, etc.) were supported, causing
IAM policies with specific actions to be rejected with 'not a valid action'
error.
Fixes issue #7864 part 2: s3:DeleteObject IAM action is now supported.
Changes:
- Extended MapToStatementAction() to handle fine-grained S3 actions
- Maps S3-specific actions to appropriate internal action constants
- Supports 30+ S3 actions including DeleteObject, PutObject, GetObject, etc.
* fix(s3api): correct resource ARN generation for subpath permissions
Fix convertSingleAction() to properly handle subpath patterns in legacy
actions. Previously, when a user was granted Write permission to a subpath
(e.g., Write:bucket/sub_path/*), the resource ARN was incorrectly generated,
causing DELETE operations to be denied even though s3:DeleteObject was
included in the Write action.
The fix:
- Extract bucket name and prefix path separately from patterns like
'bucket/prefix/*'
- Generate correct S3 ARN format: arn:aws:s3:::bucket/prefix/*
- Ensure all permission checks (Read, Write, List, Tagging, etc.) work
correctly with subpaths
- Support nested paths (e.g., bucket/a/b/c/*)
Fixes issue #7864 part 1: Write permission on subpath now allows DELETE.
Example:
- Permission: Write:mybucket/documents/*
- Objects can now be: PUT, DELETE, or ACL operations on mybucket/documents/*
- Objects outside this path are still denied
* test(iam): add tests for fine-grained S3 action mappings
Extend TestMapToStatementAction with test cases for fine-grained S3 actions:
- s3:DeleteObject
- s3:PutObject
- s3:GetObject
- s3:ListBucket
- s3:PutObjectAcl
- s3:GetObjectAcl
Ensures the new action mapping support is working correctly.
* test(s3api): add comprehensive tests for subpath permission handling
Add new test file with comprehensive tests for convertSingleAction():
1. TestConvertSingleActionDeleteObject: Verifies s3:DeleteObject is
included in Write actions (fixes issue #7864 part 2)
2. TestConvertSingleActionSubpath: Tests proper resource ARN generation
for different permission patterns:
- Bucket-level: Write:mybucket -> arn:aws:s3:::mybucket
- Wildcard: Write:mybucket/* -> arn:aws:s3:::mybucket/*
- Subpath: Write:mybucket/sub_path/* -> arn:aws:s3:::mybucket/sub_path/*
- Nested: Read:mybucket/documents/* -> arn:aws:s3:::mybucket/documents/*
3. TestConvertSingleActionSubpathDeleteAllowed: Specifically validates
that subpath Write permissions allow DELETE operations
4. TestConvertSingleActionNestedPaths: Tests deeply nested path handling
(e.g., bucket/a/b/c/*)
All tests pass and validate the fixes for issue #7864.
* fix: address review comments from PR #7865
- Fix critical bug: use parsed 'bucket' instead of 'resourcePattern' for GetObjectRetention, GetObjectLegalHold, and PutObjectLegalHold actions to avoid malformed ARNs like arn:aws:s3:::bucket/*/*
- Refactor large switch statement in MapToStatementAction() into a map-based lookup for better performance and maintainability
* fmt
* refactor: extract extractBucketAndPrefix helper and simplify convertSingleAction
- Extract extractBucketAndPrefix as a package-level function for better testability and reusability
- Remove unused bucketName parameter from convertSingleAction signature
- Update GetResourcesFromLegacyAction to use the extracted helper for consistent ARN generation
- Update all call sites in tests to match new function signature
- All tests pass and module compiles without errors
* fix: use extracted bucket variable consistently in all ARN generation branches
Replace resourcePattern with extracted bucket variable in else branches and bucket-level cases to avoid malformed ARNs like 'arn:aws:s3:::mybucket/*/*':
- Read case: bucket-level else branch
- Write case: bucket-level else branch
- Admin case: both bucket and object ARNs
- List case: bucket-level else branch
- GetBucketObjectLockConfiguration: bucket extraction
- PutBucketObjectLockConfiguration: bucket extraction
This ensures consistent ARN format: arn:aws:s3:::bucket or arn:aws:s3:::bucket/*
* fix: address remaining review comments from PR #7865
High priority fixes:
- Write action on bucket-level now generates arn:aws:s3:::mybucket/* instead of
arn:aws:s3:::mybucket to enable object-level S3 actions (s3:PutObject, s3:DeleteObject)
- GetResourcesFromLegacyAction now generates both bucket and object ARNs for /*
patterns to maintain backward compatibility with mixed action groups
Medium priority improvements:
- Remove unused 'bucket' field from TestConvertSingleActionSubpath test struct
- Update test to use assert.ElementsMatch instead of assert.Contains for more
comprehensive resource ARN validation
- Clarify test expectations with expectedResources slice instead of single expectedResource
All tests pass, compilation verified
* test: improve TestConvertSingleActionNestedPaths with comprehensive assertions
Update test to use assert.ElementsMatch for more robust resource ARN verification:
- Change struct from single expectedResource to expectedResources slice
- Update Read nested path test to expect both bucket and prefix ARNs
- Use assert.ElementsMatch to verify all generated resources match exactly
- Provides complete coverage for nested path handling
This matches the improvement pattern used in TestConvertSingleActionSubpath
* refactor: simplify S3 action map and improve resource ARN detection
- Refactor fineGrainedActionMap to use init() function for programmatic population of both prefixed (s3:Action) and unprefixed (Action) variants, eliminating 70+ duplicate entries
- Add buildObjectResourceArn() helper to eliminate duplicated resource ARN generation logic across switch cases
- Fix bucket vs object-level access detection: only use HasSuffix(/*) check instead of Contains('/') which incorrectly matched patterns like 'bucket/prefix' without wildcard
- Apply buildObjectResourceArn() consistently to Tagging, BypassGovernanceRetention, GetObjectRetention, PutObjectRetention, GetObjectLegalHold, and PutObjectLegalHold cases
* fmt
* fix: generate object-level ARNs for bucket-level read access
When bucket-level read access is granted (e.g., 'Read:mybucket'),
generate both bucket and object ARNs so that object-level actions
like s3:GetObject can properly authorize.
Similarly, in GetResourcesFromLegacyAction, bucket-level patterns
should generate both ARN levels for consistency with patterns that
include wildcards. This ensures that users with bucket-level
permissions can read objects, not just the bucket itself.
* fix: address Copilot code review comments
- Remove unused bucketName parameter from ConvertIdentityToPolicy signature
- Update all callers in examples.go and engine_test.go
- Bucket is now extracted from action string itself
- Update extractBucketAndPrefix documentation
- Add nested path example (bucket/a/b/c/*)
- Clarify that prefix can contain multiple path segments
- Make GetResourcesFromLegacyAction action-aware
- Different action types have different resource requirements
- List actions only need bucket ARN (bucket-only operations)
- Read/Write/Tagging actions need both bucket and object ARNs
- Aligns with convertSingleAction logic for consistency
All tests pass successfully
* test: add comprehensive tests for GetResourcesFromLegacyAction consistency
- Add TestGetResourcesFromLegacyAction to verify action-aware resource generation
- Validate consistency with convertSingleAction for all action types:
* List actions: bucket-only ARNs (s3:ListBucket is bucket-level operation)
* Read actions: both bucket and object ARNs
* Write actions: object-only ARNs (subpaths) or object ARNs (bucket-level)
* Admin actions: both bucket and object ARNs
- Update GetResourcesFromLegacyAction to generate Admin ARNs consistent with convertSingleAction
- All tests pass (35+ test cases across integration_test.go)
* refactor: eliminate code duplication in GetResourcesFromLegacyAction
- Simplify GetResourcesFromLegacyAction to delegate to convertSingleAction
- Eliminates ~50 lines of duplicated action-type-specific logic
- Ensures single source of truth for resource ARN generation
- Improves maintainability: changes to ARN logic only need to be made in one place
- All tests pass: any inconsistencies would be caught immediately
- Addresses Gemini Code Assist review comment about code duplication
* fix: remove fragile 'dummy' action type in CreatePolicyFromLegacyIdentity
- Replace hardcoded 'dummy:' prefix with proper representative action type
- Use first valid action type from the action list to determine resource requirements
- Ensures GetResourcesFromLegacyAction receives a valid action type
- Prevents silent failures when convertSingleAction encounters unknown action
- Improves code clarity: explains why representative action type is needed
- All tests pass: policy engine tests verify correct behavior
* security: prevent privilege escalation in Admin action subpath handling
- Admin action with subpath (e.g., Admin:bucket/admin/*) now correctly restricts
to the specified subpath instead of granting full bucket access
- If prefix exists: resources restricted to bucket + bucket/prefix/*
- If no prefix: full bucket access (unchanged behavior for root Admin)
- Added test case Admin_on_subpath to validate the security fix
- All 40+ policy engine tests pass
* refactor: address Copilot code review comments on S3 authorization
- Fix GetObjectTagging mapping: change from ACTION_READ to ACTION_TAGGING
(tagging operations should not be classified as general read operations)
- Enhance extractBucketAndPrefix edge case handling:
- Add input validation (reject empty strings, whitespace, slash-only)
- Normalize double slashes and trailing slashes
- Return empty bucket/prefix for invalid patterns
- Prevent generation of malformed ARNs
- Separate Read action from ListBucket (AWS S3 IAM semantics):
- ListBucket is a bucket-level operation, not object-level
- Read action now only includes s3:GetObject, s3:GetObjectVersion
- This aligns with AWS S3 IAM policy best practices
- Update buildObjectResourceArn to handle invalid bucket names gracefully:
- Return empty slice if bucket is empty after validation
- Prevents malformed ARN generation
- Add comprehensive TestExtractBucketAndPrefixEdgeCases with 8 test cases:
- Validates empty strings, whitespace, special characters
- Confirms proper normalization of double/trailing slashes
- Ensures robust parsing of nested paths
- Update existing tests to reflect removed ListBucket from Read action
All 40+ policy engine tests pass
* fix: aggregate resource ARNs from all action types in CreatePolicyFromLegacyIdentity
CRITICAL FIX: The previous implementation incorrectly used a single representative
action type to determine resource ARNs when multiple legacy actions targeted the
same resource pattern. This caused incorrect policy generation when action types
with different resource requirements (e.g., List vs Write) were grouped together.
Example of the bug:
- Input: List:mybucket/path/*, Write:mybucket/path/*
- Old behavior: Used only List's resources (bucket-level ARN)
- Result: Policy had Write actions (s3:PutObject) but only bucket ARN
- Consequence: s3:PutObject would be denied due to missing object-level ARN
Solution:
- Iterate through all action types for a given resource pattern
- For each action type, call GetResourcesFromLegacyAction to get required ARNs
- Aggregate all ARNs into a set to eliminate duplicates
- Use the merged set for the final policy statement
- Admin action short-circuits (always includes full permissions)
Example of correct behavior:
- Input: List:mybucket/path/*, Write:mybucket/path/*
- New behavior: Aggregates both List and Write resource requirements
- Result: Policy has Write actions with BOTH bucket and object-level ARNs
- Outcome: s3:PutObject works correctly on mybucket/path/*
Added TestCreatePolicyFromLegacyIdentityMultipleActions with 3 test cases:
1. List + Write on subpath: verifies bucket + object ARN aggregation
2. Read + Tagging on bucket: verifies action-specific ARN combinations
3. Admin with other actions: verifies Admin dominates resource ARNs
All 45+ policy engine tests pass
* fix: remove bucket-level ARN from Read action for consistency
ISSUE: The Read action was including bucket-level ARNs (arn:aws:s3:::bucket)
even though the only S3 actions in Read are s3:GetObject and s3:GetObjectVersion,
which are object-level operations. This created a mismatch between the actions
and resources in the policy statement.
ROOT CAUSE: s3:ListBucket was previously removed from the Read action, but the
bucket-level ARN was not removed, creating an inconsistency.
SOLUTION: Update Read action to only generate object-level ARNs using
buildObjectResourceArn, consistent with how Write and Tagging actions work.
This ensures:
- Read:mybucket generates arn:aws:s3:::mybucket/* (not bucket ARN)
- Read:bucket/prefix/* generates arn:aws:s3:::bucket/prefix/* (object-level only)
- Consistency: same actions, same resources, same logic across all object operations
Updated test expectations:
- TestConvertSingleActionSubpath: Read_on_subpath now expects only object ARN
- TestConvertSingleActionNestedPaths: Read nested path now expects only object ARN
- TestConvertIdentityToPolicy: Read resources now 1 instead of 2
- TestCreatePolicyFromLegacyIdentityMultipleActions: Read+Tagging aggregates to 1 ARN
All 45+ policy engine tests pass
* doc
* fmt
* fix: address Copilot code review on Read action consistency and missing S3 action mappings
- Clarify MapToStatementAction comment to reflect exact lookup (not pattern matching)
- Add missing S3 actions to baseS3ActionMap:
- ListBucketVersions, ListAllMyBuckets for bucket operations
- GetBucketCors, PutBucketCors, DeleteBucketCors for CORS
- GetBucketNotification, PutBucketNotification for notifications
- GetBucketObjectLockConfiguration, PutBucketObjectLockConfiguration for object lock
- GetObjectVersionTagging for version tagging
- GetObjectVersionAcl, PutBucketAcl for ACL operations
- PutBucketTagging, DeleteBucketTagging for bucket tagging
- Fix Read action scope inconsistency with GetActionMappings():
- Previously: only included GetObject, GetObjectVersion
- Now: includes full Read set (14 actions) from GetActionMappings
- Includes both bucket-level (ListBucket*, GetBucket*) and object-level (GetObject*) ARNs
- Bucket ARN enables ListBucket operations, object ARN enables GetObject operations
- Update all test expectations:
- TestConvertSingleActionSubpath: Read now returns 2 ARNs (bucket + objects)
- TestConvertSingleActionNestedPaths: Read nested path now includes bucket ARN
- TestGetResourcesFromLegacyAction: Read test cases updated for consistency
- TestCreatePolicyFromLegacyIdentityMultipleActions: Read_and_Tagging now returns 2 ARNs
- TestConvertIdentityToPolicy: Updated to expect 14 Read actions and 2 resources
Fixes: Inconsistency between convertSingleAction Read case and GetActionMappings function
* fmt
* fix: align convertSingleAction with GetActionMappings and add bucket validation
- Fix Write action: now includes all 16 actions from GetActionMappings (object and bucket operations)
- Includes PutBucketVersioning, PutBucketCors, PutBucketAcl, PutBucketTagging, etc.
- Generates both bucket and object ARNs to support bucket-level operations
- Fix List action: add ListAllMyBuckets from GetActionMappings
- Previously: ListBucket, ListBucketVersions
- Now: ListBucket, ListBucketVersions, ListAllMyBuckets
- Add bucket validation to prevent malformed ARNs with empty bucket
- Fix Tagging action: include bucket-level tagging operations
- Previously: only object-level (GetObjectTagging, PutObjectTagging, DeleteObjectTagging)
- Now: includes bucket-level (GetBucketTagging, PutBucketTagging, DeleteBucketTagging)
- Generates both bucket and object ARNs to support bucket-level operations
- Add bucket validation to prevent malformed ARNs:
- Admin: return error if bucket is empty
- List: generate empty resources if bucket is empty
- Tagging: check bucket before generating ARNs
- GetBucketObjectLockConfiguration, PutBucketObjectLockConfiguration: validate bucket
- Fix TrimRight issue in extractBucketAndPrefix:
- Change from strings.TrimRight(pattern, "/") to remove only one trailing slash
- Prevents loss of prefix when pattern has multiple trailing slashes
- Update all test cases:
- TestConvertSingleActionSubpath: Write now returns 16 actions and bucket+object ARNs
- TestConvertSingleActionNestedPaths: Write includes bucket ARN
- TestGetResourcesFromLegacyAction: Updated Write and Tagging expectations
- TestCreatePolicyFromLegacyIdentityMultipleActions: Updated action/resource counts
Fixes: Inconsistencies between convertSingleAction and GetActionMappings for Write/List/Tagging actions
* fmt
* fix: resolve ListMultipartUploads/ListParts mapping inconsistency and add action validation
- Fix ListMultipartUploads and ListParts mapping in helpers.go:
- Changed from ACTION_LIST to ACTION_WRITE for consistency with GetActionMappings
- These operations are part of the multipart write workflow and should map to Write action
- Prevents inconsistent behavior when same actions processed through different code paths
- Add documentation to clarify multipart operations in Write action:
- Explain why ListMultipartUploads and ListParts are part of Write permissions
- These are required for meaningful multipart upload workflow management
- Add action validation to CreatePolicyFromLegacyIdentity:
- Validates action format before processing using ValidateActionMapping
- Logs warnings for invalid actions instead of silently skipping them
- Provides clearer error messages when invalid action types are used
- Ensures users know when their intended permissions weren't applied
- Consistent with ConvertLegacyActions validation approach
Fixes: Inconsistent action type mappings and silent failure for invalid actions
* fix: use keyed fields in struct literals
- Replace unsafe reflect.StringHeader/SliceHeader with safe unsafe.String/Slice (weed/query/sqltypes/unsafe.go)
- Add field names to Type_ScalarType struct literals (weed/mq/schema/schema_builder.go)
- Add Duration field name to FlexibleDuration struct literals across test files
- Add field names to bson.D struct literals (weed/filer/mongodb/mongodb_store_kv.go)
Fixes go vet warnings about unkeyed struct literals.
* fix: remove unreachable code
- Remove unreachable return statements after infinite for loops
- Remove unreachable code after if/else blocks where all paths return
- Simplify recursive logic by removing unnecessary for loop (inode_to_path.go)
- Fix Type_ScalarType literal to use enum value directly (schema_builder.go)
- Call onCompletionFn on stream error (subscribe_session.go)
Files fixed:
- weed/query/sqltypes/unsafe.go
- weed/mq/schema/schema_builder.go
- weed/mq/client/sub_client/connect_to_sub_coordinator.go
- weed/filer/redis3/ItemList.go
- weed/mq/client/agent_client/subscribe_session.go
- weed/mq/broker/broker_grpc_pub_balancer.go
- weed/mount/inode_to_path.go
- weed/util/skiplist/name_list.go
* fix: avoid copying lock values in protobuf messages
- Use proto.Merge() instead of direct assignment to avoid copying sync.Mutex in S3ApiConfiguration (iamapi_server.go)
- Add explicit comments noting that channel-received values are already copies before taking addresses (volume_grpc_client_to_master.go)
The protobuf messages contain sync.Mutex fields from the message state, which should not be copied.
Using proto.Merge() properly merges messages without copying the embedded mutex.
* fix: correct byte array size for uint32 bit shift operations
The generateAccountId() function only needs 4 bytes to create a uint32 value.
Changed from allocating 8 bytes to 4 bytes to match the actual usage.
This fixes go vet warning about shifting 8-bit values (bytes) by more than 8 bits.
* fix: ensure context cancellation on all error paths
In broker_client_subscribe.go, ensure subscriberCancel() is called on all error return paths:
- When stream creation fails
- When partition assignment fails
- When sending initialization message fails
This prevents context leaks when an error occurs during subscriber creation.
* fix: ensure subscriberCancel called for CreateFreshSubscriber stream.Send error
Ensure subscriberCancel() is called when stream.Send fails in CreateFreshSubscriber.
* ci: add go vet step to prevent future lint regressions
- Add go vet step to GitHub Actions workflow
- Filter known protobuf lock warnings (MessageState sync.Mutex)
These are expected in generated protobuf code and are safe
- Prevents accumulation of go vet errors in future PRs
- Step runs before build to catch issues early
* fix: resolve remaining syntax and logic errors in vet fixes
- Fixed syntax errors in filer_sync.go caused by missing closing braces
- Added missing closing brace for if block and function
- Synchronized fixes to match previous commits on branch
* fix: add missing return statements to daemon functions
- Add 'return false' after infinite loops in filer_backup.go and filer_meta_backup.go
- Satisfies declared bool return type signatures
- Maintains consistency with other daemon functions (runMaster, runFilerSynchronize, runWorker)
- While unreachable, explicitly declares the return satisfies function signature contract
* fix: add nil check for onCompletionFn in SubscribeMessageRecord
- Check if onCompletionFn is not nil before calling it
- Prevents potential panic if nil function is passed
- Matches pattern used in other callback functions
* docs: clarify unreachable return statements in daemon functions
- Add comments documenting that return statements satisfy function signature
- Explains that these returns follow infinite loops and are unreachable
- Improves code clarity for future maintainers
* feat(worker): add metrics HTTP server and debug profiling support
- Add -metricsPort flag to enable Prometheus metrics endpoint
- Add -metricsIp flag to configure metrics server bind address
- Implement /metrics endpoint for Prometheus-compatible metrics
- Implement /health endpoint for Kubernetes readiness/liveness probes
- Add -debug flag to enable pprof debugging server
- Add -debug.port flag to configure debug server port
- Fix stats package import naming conflict by using alias
- Update usage examples to show new flags
Fixes#7843
* feat(helm): add worker metrics and health check support
- Update worker readiness probe to use httpGet on /health endpoint
- Update worker liveness probe to use httpGet on /health endpoint
- Add metricsPort flag to worker command in deployment template
- Support both httpGet and tcpSocket probe types for backward compatibility
- Update values.yaml with health check configuration
This enables Kubernetes pod lifecycle management for worker components through
proper health checks on the new metrics HTTP endpoint.
* feat(mini): align all services to share single debug and metrics servers
- Disable S3's separate debug server in mini mode (port 6060 now shared by all)
- Add metrics server startup to embedded worker for health monitoring
- All services now share the single metrics port (9327) and single debug port (6060)
- Consistent pattern with master, filer, volume, webdav services
* fix(worker): fix variable shadowing in health check handler
- Rename http.ResponseWriter parameter from 'w' to 'rw' to avoid shadowing
the outer 'w *worker.Worker' parameter
- Prevents potential bugs if future code tries to use worker state in handler
- Improves code clarity and follows Go best practices
* fix(worker): remove unused worker parameter in metrics server
- Change 'w *worker.Worker' parameter to '_' as it's not used
- Clarifies intent that parameter is intentionally unused
- Follows Go best practices and improves code clarity
* fix(helm): fix trailing backslash syntax errors in worker command
- Fix conditional backslash placement to prevent shell syntax errors
- Only add backslash when metricsPort OR extraArgs are present
- Prevents worker pod startup failures due to malformed command arguments
- Ensures proper shell command parsing regardless of configuration state
* refactor(worker): use standard stats.StartMetricsServer for consistency
- Replace custom metrics server implementation with stats.StartMetricsServer
to match pattern used in master, volume, s3, filer_sync components
- Simplifies code and improves maintainability
- Uses glog.Fatal for errors (consistent with other SeaweedFS components)
- Remove unused net/http and prometheus/promhttp imports
- Automatically provides /metrics and /health endpoints via standard implementation
fix: consolidate Helm chart release with container image build
Resolve issue #7855 by consolidating the Helm chart release workflow
with the container image build workflow. This ensures perfect alignment:
1. Container images are built and pushed to GHCR
2. Images are copied from GHCR to Docker Hub
3. Helm chart is published only after step 2 completes
Previously, the Helm chart was published immediately on tag push before
images were available in Docker Hub, causing deployment failures.
Changes:
- Added helm-release job to container_release_unified.yml that depends
on copy-to-dockerhub job
- Removed helm_chart_release.yml workflow (consolidated into unified release)
Benefits:
- No race conditions between image push and chart publication
- Users can deploy immediately after release
- Single source of truth for release process
- Clearer job dependencies and execution flow
* fix: use path instead of filepath to handle urls in weed admin file browser
* test: add comprehensive tests for file browser path handling
- Test breadcrumb generation for various path scenarios
- Test path handling with forward slashes (URL compatibility)
- Test parent path calculation for Windows compatibility
- Test file extension handling using path.Ext
- Test bucket path detection logic
These tests verify that the switch from filepath to path package works
correctly and handles URLs properly across all platforms.
* refactor: simplify fullPath construction using path.Join
Replace verbose manual path construction with path.Join which:
- Handles trailing slashes automatically
- Is more concise and readable
- Is more robust for edge cases
* fix: normalize path in ShowFileBrowser and rename generateBreadcrumbs parameter
Critical fix:
- Add util.CleanWindowsPath() normalization to path parameter in ShowFileBrowser
handler, matching the pattern used in other file operation handlers
(lines 273, 464)
- This ensures Windows-style backslashes are converted to forward slashes
before processing, fixing path handling issues on Windows
Consistency improvement:
- Rename path parameter to dir in generateBreadcrumbs function
- Aligns with parameter rename in GetFileBrowser for consistent naming
throughout the file
* test: improve coverage for Windows path handling and production code behavior
Address reviewer feedback by enhancing test quality:
1. Improved test documentation:
- Added clear comments explaining what each test validates
- Clarified that some tests validate expected behavior vs production code
- Documented the Windows path normalization flow
2. Enhanced actual production code testing:
- TestGenerateBreadcrumbs: Calls actual production function
- TestBreadcrumbPathFormatting: Validates production output format
- TestDirectoryNavigation: Integration-style test for complete flow
3. Added new test functions for better coverage:
- TestPathJoinHandlesEdgeCases: Verifies path.Join behavior
- TestWindowsPathNormalizationBehavior: Documents expected normalization
- TestDirectoryNavigation: Complete navigation flow test
4. Improved test organization:
- Fixed duplicate field naming issues
- Better test names for clarity
- More comprehensive edge case coverage
These improvements ensure the fix for issue #7628 (Windows path handling)
is properly validated across the complete flow from handler to path logic.
* test: use actual util.CleanWindowsPath function in Windows path normalization test
Address reviewer feedback by testing the actual production function:
- Import util package for CleanWindowsPath
- Call the real util.CleanWindowsPath() instead of reimplementing logic
- Ensures test validates actual implementation, not just expected behavior
- Added more test cases for edge cases (simple path, deep nesting)
This change validates that the Windows path normalization in the
ShowFileBrowser handler (handlers/file_browser_handlers.go:64)
works correctly with the actual util.CleanWindowsPath function.
* style: fix indentation in TestPathJoinHandlesEdgeCases
Align t.Errorf statement inside the if block with proper indentation.
The error message now correctly aligns with the if block body,
maintaining consistent indentation throughout the function.
* test: restore backslash validation check in TestPathJoinHandlesEdgeCases
---------
Co-authored-by: Chris Lu <chris.lu@gmail.com>
* refactor: Convert versioning to three-state string model matching AWS S3
- Change VersioningEnabled bool to VersioningStatus string in S3Bucket struct
- Add GetVersioningStatus() function returning empty string (never enabled), 'Enabled', or 'Suspended'
- Update StoreVersioningInExtended() to delete key instead of setting 'Suspended'
- Ensures Admin UI and S3 API use consistent versioning state representation
* fix: Add validation for bucket quota and Object Lock configuration
- Prevent buckets with quota enabled but size=0 (validation check)
- Fix Object Lock mode handling to only pass mode when setDefaultRetention is true
- Ensures proper extended attribute storage for Object Lock configuration
- Matches AWS S3 behavior for Object Lock setup
* feat: Handle versioned objects in bucket details view
- Recognize .versions directories as versioned objects in listBucketObjects()
- Extract size and mtime from extended attribute metadata (ExtLatestVersionSizeKey, ExtLatestVersionMtimeKey)
- Add length validation (8 bytes) before parsing extended attribute byte arrays
- Update GetBucketDetails() and GetS3Buckets() to use new GetVersioningStatus()
- Properly display versioned objects without .versions suffix in bucket details
* ui: Update bucket management UI to show three-state versioning and Object Lock
- Change versioning display from binary (Enabled/Disabled) to three-state (Not configured/Enabled/Suspended)
- Update Object Lock display to show 'Not configured' instead of 'Disabled'
- Fix bucket details modal to use bucket.versioning_status instead of bucket.versioning_enabled
- Update displayBucketDetails() JavaScript to handle three versioning states
* chore: Regenerate template code for bucket UI changes
- Generated from updated s3_buckets.templ
- Reflects three-state versioning and Object Lock UI improvements
The GrpcMaxConnectionAge and GrpcMaxConnectionAgeGrace constants have a troubled
history - they were removed in 2022 due to gRPC issues, reverted later, and
recently re-added. However, they are not essential to the core worker reconnection
fix which was solved through proper goroutine ordering.
The Docker Swarm DNS handling mentioned in the comments is not critical, and
these parameters have caused problems in the past. Removing them simplifies
the configuration without losing functionality.
* feat: add automatic port detection and fallback for mini command
- Added port availability detection using TCP binding tests
- Implemented port fallback mechanism searching for available ports
- Support for both HTTP and gRPC port handling
- IP-aware port checking using actual service bind address
- Dual-interface verification (specific IP and wildcard 0.0.0.0)
- All services (Master, Volume, Filer, S3, WebDAV, Admin) auto-reallocate to available ports
- Enables multiple mini instances to run simultaneously without conflicts
* fix: use actual bind IP for service health checks
- Previously health checks were hardcoded to localhost (127.0.0.1)
- This caused failures when services bind to actual IP (e.g., 10.21.153.8)
- Now health checks use the same IP that services are binding to
- Fixes Volume and other service health check failures on non-localhost IPs
* refactor: improve port detection logic and remove gRPC handling duplication
- findAvailablePortOnIP now returns 0 on failure instead of unavailable port
Allows callers to detect when port finding fails and handle appropriately
- Remove duplicate gRPC port handling from ensureAllPortsAvailableOnIP
All gRPC port logic is now centralized in initializeGrpcPortsOnIP
- Log final port configuration only after all ports are finalized
Both HTTP and gRPC ports are now correctly initialized before logging
- Add error logging when port allocation fails
Makes debugging easier when ports can't be found
* refactor: fix race condition and clean up port detection code
- Convert parallel HTTP port checks to sequential to prevent race conditions
where multiple goroutines could allocate the same available port
- Remove unused 'sync' import since WaitGroup is no longer used
- Add documentation to localhost wrapper functions explaining they are
kept for backwards compatibility and future use
- All gRPC port logic is now exclusively handled in initializeGrpcPortsOnIP
eliminating any duplication in ensureAllPortsAvailableOnIP
* refactor: address code review comments - constants, helper function, and cleanup
- Define GrpcPortOffset constant (10000) to replace magic numbers throughout
the code for better maintainability and consistency
- Extract bindIp determination logic into getBindIp() helper function
to eliminate code duplication between runMini and startMiniServices
- Remove redundant 'calculatedPort = calculatedPort' assignment that had no effect
- Update all gRPC port calculations to use GrpcPortOffset constant
(lines 489, 886 and the error logging at line 501)
* refactor: remove unused wrapper functions and update documentation
- Remove unused localhost wrapper functions that were never called:
- isPortOpen() - wrapper around isPortOpenOnIP with hardcoded 127.0.0.1
- findAvailablePort() - wrapper around findAvailablePortOnIP with hardcoded 127.0.0.1
- ensurePortAvailable() - wrapper around ensurePortAvailableOnIP with hardcoded 127.0.0.1
- ensureAllPortsAvailable() - wrapper around ensureAllPortsAvailableOnIP with hardcoded 127.0.0.1
Since this is new functionality with no backwards compatibility concerns,
these wrapper functions were not needed. The comments claiming they were
'kept for future use or backwards compatibility' are no longer valid.
- Update documentation to reference GrpcPortOffset constant instead of hardcoded 10000:
- Update comment in ensureAllPortsAvailableOnIP to use GrpcPortOffset
- Update admin.port.grpc flag help text to reference GrpcPortOffset
Note: getBindIp() is actually being used and should be retained (contrary to
the review comment suggesting it was unused - it's called in both runMini
and startMiniServices functions)
* refactor: prevent HTTP/gRPC port collisions and improve error handling
- Add upfront reservation of all calculated gRPC ports before allocating HTTP ports
to prevent collisions where an HTTP port allocation could use a port that will
later be needed for a gRPC port calculation.
Example scenario that is now prevented:
- Master HTTP reallocated from 9333 to 9334 (original in use)
- Filer HTTP search finds 19334 available and assigns it
- Master gRPC calculated as 9334 + GrpcPortOffset = 19334 → collision!
Now: reserved gRPC ports are tracked upfront and HTTP port search skips them.
- Improve admin server gRPC port fallback error handling:
- Change from silent V(1) verbose log to Warningf to make the error visible
- Update comment to clarify this indicates a problem in the port initialization sequence
- Add explanation that the fallback calculation may cause bind failure
- Update ensureAllPortsAvailableOnIP comment to clarify it avoids reserved ports
* fix: enforce reserved ports in HTTP allocation and improve admin gRPC fallback
Critical fixes for port allocation safety:
1. Make findAvailablePortOnIP and ensurePortAvailableOnIP aware of reservedPorts:
- Add reservedPorts map parameter to both functions
- findAvailablePortOnIP now skips reserved ports when searching for alternatives
- ensurePortAvailableOnIP passes reservedPorts through to findAvailablePortOnIP
- This prevents HTTP ports from being allocated to ports reserved for gRPC
2. Update ensureAllPortsAvailableOnIP to pass reservedPorts:
- Pass the reservedPorts map to ensurePortAvailableOnIP calls
- Maintains the map updates (delete/add) for accuracy as ports change
3. Replace blind admin gRPC port fallback with proper availability checks:
- Previous code just calculated *miniAdminOptions.port + GrpcPortOffset
- New code checks both the calculated port and finds alternatives if needed
- Uses the same availability checking logic as initializeGrpcPortsOnIP
- Properly logs the fallback process and any port changes
- Will fail gracefully if no available ports found (consistent with other services)
These changes eliminate two critical vulnerabilities:
- HTTP port allocation can no longer accidentally claim gRPC ports
- Admin gRPC port fallback no longer blindly uses an unchecked port
* fix: prevent gRPC port collisions during multi-service fallback allocation
Critical fix for gRPC port allocation safety across multiple services:
Problem: When multiple services need gRPC port fallback allocation in sequence
(e.g., Master gRPC unavailable → finds alternative, then Filer gRPC unavailable
→ searches from calculated port), there was no tracking of previously allocated
gRPC ports. This could allow two services to claim the same port.
Scenario that is now prevented:
- Master gRPC: calculated 19333 unavailable → finds 19334 → assigns 19334
- Filer gRPC: calculated 18888 unavailable → searches from 18889, might land on
19334 if consecutive ports in range are unavailable (especially with custom
port configurations or in high-port-contention environments)
Solution:
- Add allocatedGrpcPorts map to track gRPC ports allocated within the function
- Check allocatedGrpcPorts before using calculated port for each service
- Pass allocatedGrpcPorts to findAvailablePortOnIP when finding fallback ports
- Add allocatedGrpcPorts[port] = true after each successful allocation
- This ensures no two services can allocate the same gRPC port
The fix handles both:
1. Calculated gRPC ports (when grpcPort == 0)
2. Explicitly set gRPC ports (when user provides -service.port.grpc value)
While default port spacing makes collision unlikely, this fix is essential for:
- Custom port configurations
- High-contention environments
- Edge cases with many unavailable consecutive ports
- Correctness and safety guarantees
* feat: enforce hard-fail behavior for explicitly specified ports
When users explicitly specify a port via command-line flags (e.g., -s3.port=8333),
the server should fail immediately if the port is unavailable, rather than silently
falling back to an alternative port. This prevents user confusion and makes misconfiguration
failures obvious.
Changes:
- Modified ensurePortAvailableOnIP() to check if a port was explicitly passed via isFlagPassed()
- If an explicit port is unavailable, return error instead of silently allocating alternative
- Updated ensureAllPortsAvailableOnIP() to handle the returned error and fail startup
- Modified runMini() to check error from ensureAllPortsAvailableOnIP() and return false on failure
- Default ports (not explicitly specified) continue to fallback to available alternatives
This ensures:
- Explicit ports: fail if unavailable (e.g., -s3.port=8333 fails if 8333 is taken)
- Default ports: fallback to alternatives (e.g., s3.port without flag falls back to 8334 if 8333 taken)
* fix: accurate error messages for explicitly specified unavailable ports
When a port is explicitly specified via CLI flags but is unavailable, the error message
now correctly reports the originally requested port instead of reporting a fallback port
that was calculated internally.
The issue was that the config file applied after CLI flag parsing caused isFlagPassed()
to return true for ports loaded from the config file (since flag.Visit() was called during
config file application), incorrectly marking them as explicitly specified.
Solution: Capture which port flags were explicitly passed on the CLI BEFORE the config file
is applied, storing them in the explicitPortFlags map. This preserves the accurate
distinction between user-specified ports and defaults/config-file ports.
Example:
- User runs: weed mini -dir=. -s3.port=22
- Now correctly shows: 'port 22 for S3 (specified by flag s3.port) is not available'
- Previously incorrectly showed: 'port 8334 for S3...' (some calculated fallback)
* fix: respect explicitly specified ports and prevent config file override
When a port is explicitly specified via CLI flags (e.g., -s3.port=8333),
the config file options should NOT override it. Previously, config file
options would be applied if the flag value differed from default, but
this check wasn't sufficient to prevent override in all cases.
Solution: Check the explicitPortFlags map before applying any config file
port options. If a port was explicitly passed on the CLI, skip applying
the config file option for that port.
This ensures:
- Explicit ports take absolute precedence over config file ports
- Config file ports are only used if port wasn't specified on CLI
- Example: 'weed mini -s3.port=8333' will use 8333, never the config file value
* fix: don't print usage on port allocation error
When a port allocation fails (e.g., explicit port is unavailable), exit
immediately without showing the usage example. This provides cleaner
error output when the error is expected (port conflict).
* refactor: clean up code quality issues
Remove no-op assignment (calculatedPort = calculatedPort) that had no effect.
The variable already holds the correct value when no alternative port is
found.
Improve documentation for the defensive gRPC port initialization fallback
in startAdminServer. While this code shouldn't execute in normal flow
because ensureAllPortsAvailableOnIP is called earlier in runMini, the
fallback handles edge cases where port initialization may have been skipped
or failed silently due to configuration changes or error handling paths.
* fix: improve worker reconnection robustness and prevent handleOutgoing hang
- Add dedicated streamFailed signaling channel to abort registration waits early when stream dies
- Add per-connection regWait channel to route RegistrationResponse separately from shared incoming channel, avoiding race where other consumers steal the response
- Refactor handleOutgoing() loop to use select on streamExit/errCh, ensuring old handlers exit cleanly on reconnect (prevents stale senders competing with new stream)
- Buffer msgCh to reduce shutdown edge cases
- Add cleanup of streamFailed and regWait channels on reconnect/disconnect
- Fixes registration timeout and potential stream lifecycle hangs on aggressive server max_age recycling
* fix: prevent deadlock when stream error occurs - make cmds send non-blocking
If managerLoop is blocked (e.g., waiting on regWait), a blocking send to cmds
will deadlock handleIncoming. Make the send non-blocking to prevent this.
* fix: address code review comments on mini.go port allocation
- Remove flawed fallback gRPC port initialization and convert to fatal error
(ensures port initialization issues are caught immediately instead of silently
failing with an empty reserved ports map)
- Extract common port validation logic to eliminate duplication between
calculated and explicitly set gRPC port handling
* Fix critical race condition and improve error handling in worker client
- Capture channel pointers before checking for nil (prevents TOCTOU race with reconnect)
- Use async fallback goroutine for cmds send to prevent error loss when manager is busy
- Consistently close regWait channel on disconnect (matches streamFailed behavior)
- Complete cleanup of channels on failed registration
- Improve error messages for clarity (replace 'timeout' with 'failed' where appropriate)
* Add debug logging for registration response routing
Add glog.V(3) and glog.V(2) logs to track successful and dropped registration
responses in handleIncoming, helping diagnose registration issues in production.
* Update weed/worker/client.go
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Ensure stream errors are never lost by using async fallback
When handleIncoming detects a stream error, queue ActionStreamError to managerLoop
with non-blocking send. If managerLoop is busy and cmds channel is full, spawn an
async goroutine to queue the error asynchronously. This ensures the manager is
always notified of stream failures, preventing the connection from remaining in an
inconsistent state (connected=true while stream is dead).
* Refactor handleOutgoing to eliminate duplicate error handling code
Extract error handling and cleanup logic into helper functions to avoid duplication
in nested select statements. This improves maintainability and reduces the risk of
inconsistencies when updating error handling logic.
* Prevent goroutine leaks by adding timeouts to blocking cmds sends
Add 2-second timeouts to both handleStreamError and the async fallback goroutine
when sending ActionStreamError to cmds channel. This prevents the handleOutgoing
and handleIncoming goroutines from blocking indefinitely if the managerLoop is
no longer receiving (e.g., during shutdown), preventing resource leaks.
* Properly close regWait channel in reconnect to prevent resource leaks
Close the regWait channel before setting it to nil in reconnect(), matching the
pattern used in handleDisconnect(). This ensures any goroutines waiting on this
channel during reconnection are properly signaled, preventing them from hanging.
* Use non-blocking async pattern in handleOutgoing error reporting
Refactor handleStreamError to use non-blocking send with async fallback goroutine,
matching the pattern used in handleIncoming. This allows handleOutgoing to exit
immediately when errors occur rather than blocking for up to 2 seconds, improving
responsiveness and consistency across handlers.
* fix: drain regWait channel before closing to prevent message loss
- Add drain loop before closing regWait in reconnect() cleanup
- Add drain loop before closing regWait in handleDisconnect() cleanup
- Ensures no pending RegistrationResponse messages are lost during channel closure
* docs: add comments explaining regWait buffered channel design
- Document that regWait buffer size 1 prevents race conditions
- Explain non-blocking send pattern between sendRegistration and handleIncoming
- Clarify timing of registration response handling in handleIncoming
* fix: improve error messages and channel handling in sendRegistration
- Clarify error message when stream fails before registration sent
- Use two-value receive form to properly detect closed channels
- Better distinguish between closed channel and nil value scenarios
* refactor: extract drain and close channel logic into helper function
- Create drainAndCloseRegWaitChannel() helper to eliminate code duplication
- Replace 3 copies of drain-and-close logic with single function call
- Improves maintainability and consistency across cleanup paths
---------
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Fix: Eliminate duplicate versioned objects in S3 list operations
- Move versioned directory processing outside of pagination loop to process only once
- Add deduplication during .versions directory collection phase
- Fix directory handling to not add directories to results in recursive mode
- Directly add versioned entries to contents array instead of using callback
Fixes issue where AWS S3 list operations returned duplicated versioned objects
(e.g., 1000 duplicate entries from 4 unique objects). Now correctly returns only
the unique logical entries without duplication.
Verified with:
aws s3api list-objects --endpoint-url http://localhost:8333 --bucket pm-itatiaiucu-01
Returns exactly 4 entries (ClientInfo.xml and Repository from 2 Veeam backup folders)
* Refactor: Process .versions directories immediately when encountered
Instead of collecting .versions directories and processing them after the
pagination loop, process them immediately when encountered during traversal.
Benefits:
- Simpler code: removed versionedDirEntry struct and collection array
- More efficient: no need to store and iterate through collected entries
- Same O(V) complexity but with less memory overhead
- Clearer logic: processing happens in one pass during traversal
Since each .versions directory is only visited once during recursive
traversal (we never traverse into them), there's no need for deferred
processing or deduplication.
* Add comprehensive tests for versioned objects list
- TestListObjectsWithVersionedObjects: Tests listing with various delimiters
- TestVersionedObjectsNoDuplication: Core test validating no 250x duplication
- TestVersionedObjectsWithDeleteMarker: Tests delete marker filtering
- TestVersionedObjectsMaxKeys: Tests pagination with versioned objects
- TestVersionsDirectoryNotTraversed: Ensures .versions never traversed
- Fix existing test signature to match updated doListFilerEntries
* style: Fix formatting alignment in versioned objects tests
* perf: Optimize path extraction using string indexing
Replace multiple strings.Split/Join calls with efficient strings.Index
slicing to extract bucket-relative path from directory string.
Reduces unnecessary allocations and improves performance in versioned
objects listing path construction.
* refactor: Address code review feedback from Gemini Code Assist
1. Fix misleading comment about versioned directory processing location.
Versioned directories are processed immediately in doListFilerEntries,
not deferred to ListObjectsV1Handler.
2. Simplify path extraction logic using explicit bucket path construction
instead of index-based string slicing for better readability and
maintainability.
3. Add clarifying comment to test callback explaining why production logic
is duplicated - necessary because listFilerEntries is not easily testable
with filer client injection.
* fmt
* refactor: Address code review feedback from Copilot
- Fix misleading comment about versioned directory processing location
(note that processing happens within doListFilerEntries, not at top level)
- Add maxKeys validation checks in all test callbacks for consistency
- Add maxKeys check before calling eachEntryFn for versioned objects
- Improve test documentation to clarify testing approach and avoid apologetic tone
* refactor: Address code review feedback from Gemini Code Assist
- Remove redundant maxKeys check before eachEntryFn call on line 541
(the loop already checks maxKeys <= 0 at line 502, ensuring quota exists)
- Fix pagination pattern consistency in all test callbacks
- TestVersionedObjectsNoDuplication: Use cursor.maxKeys <= 0 check and decrement
- TestVersionedObjectsWithDeleteMarker: Use cursor.maxKeys <= 0 check and decrement
- TestVersionsDirectoryNotTraversed: Use cursor.maxKeys <= 0 check and decrement
- Ensures consistent pagination logic across all callbacks matching production behavior
* refactor: Address code review suggestions for code quality
- Adjust log verbosity from V(5) to V(4) for file additions to reduce noise
while maintaining useful debug output during troubleshooting
- Remove unused isRecursive parameter from doListFilerEntries function
signature and all call sites (not used for any logic decisions)
- Consolidate redundant comments about versioned directory handling
to reduce documentation duplication
These changes improve code maintainability and clarity.
* fmt
* refactor: Add pagination test and optimize stream processing
- Add comprehensive test validation to TestVersionedObjectsMaxKeys
that verifies truncation is correctly set when maxKeys is exhausted
with more entries available, ensuring proper pagination state
- Optimize stream processing in doListFilerEntries by using 'break'
instead of 'continue' when quota is exhausted (cursor.maxKeys <= 0)
This avoids receiving and discarding entries from the stream when
we've already reached the requested limit, improving efficiency
Fixes#7230
When a cluster goes down during file replication, the chunk upload process
would fail after a limited number of retries. Once the remote cluster came
back online, those failed uploads were never retried, leaving the clusters
out-of-sync.
This change enables the RetryForever flag in the UploadOption when
replicating chunks between filers. This ensures that upload operations
will keep retrying indefinitely, and once the remote cluster comes back
online, the pending uploads will automatically succeed.
Users no longer need to manually run fs.meta.save and fs.meta.load as
a workaround for out-of-sync clusters.
* feat: add automatic port detection and fallback for mini command
- Added port availability detection using TCP binding tests
- Implemented port fallback mechanism searching for available ports
- Support for both HTTP and gRPC port handling
- IP-aware port checking using actual service bind address
- Dual-interface verification (specific IP and wildcard 0.0.0.0)
- All services (Master, Volume, Filer, S3, WebDAV, Admin) auto-reallocate to available ports
- Enables multiple mini instances to run simultaneously without conflicts
* fix: use actual bind IP for service health checks
- Previously health checks were hardcoded to localhost (127.0.0.1)
- This caused failures when services bind to actual IP (e.g., 10.21.153.8)
- Now health checks use the same IP that services are binding to
- Fixes Volume and other service health check failures on non-localhost IPs
* refactor: improve port detection logic and remove gRPC handling duplication
- findAvailablePortOnIP now returns 0 on failure instead of unavailable port
Allows callers to detect when port finding fails and handle appropriately
- Remove duplicate gRPC port handling from ensureAllPortsAvailableOnIP
All gRPC port logic is now centralized in initializeGrpcPortsOnIP
- Log final port configuration only after all ports are finalized
Both HTTP and gRPC ports are now correctly initialized before logging
- Add error logging when port allocation fails
Makes debugging easier when ports can't be found
* refactor: fix race condition and clean up port detection code
- Convert parallel HTTP port checks to sequential to prevent race conditions
where multiple goroutines could allocate the same available port
- Remove unused 'sync' import since WaitGroup is no longer used
- Add documentation to localhost wrapper functions explaining they are
kept for backwards compatibility and future use
- All gRPC port logic is now exclusively handled in initializeGrpcPortsOnIP
eliminating any duplication in ensureAllPortsAvailableOnIP
* refactor: address code review comments - constants, helper function, and cleanup
- Define GrpcPortOffset constant (10000) to replace magic numbers throughout
the code for better maintainability and consistency
- Extract bindIp determination logic into getBindIp() helper function
to eliminate code duplication between runMini and startMiniServices
- Remove redundant 'calculatedPort = calculatedPort' assignment that had no effect
- Update all gRPC port calculations to use GrpcPortOffset constant
(lines 489, 886 and the error logging at line 501)
* refactor: remove unused wrapper functions and update documentation
- Remove unused localhost wrapper functions that were never called:
- isPortOpen() - wrapper around isPortOpenOnIP with hardcoded 127.0.0.1
- findAvailablePort() - wrapper around findAvailablePortOnIP with hardcoded 127.0.0.1
- ensurePortAvailable() - wrapper around ensurePortAvailableOnIP with hardcoded 127.0.0.1
- ensureAllPortsAvailable() - wrapper around ensureAllPortsAvailableOnIP with hardcoded 127.0.0.1
Since this is new functionality with no backwards compatibility concerns,
these wrapper functions were not needed. The comments claiming they were
'kept for future use or backwards compatibility' are no longer valid.
- Update documentation to reference GrpcPortOffset constant instead of hardcoded 10000:
- Update comment in ensureAllPortsAvailableOnIP to use GrpcPortOffset
- Update admin.port.grpc flag help text to reference GrpcPortOffset
Note: getBindIp() is actually being used and should be retained (contrary to
the review comment suggesting it was unused - it's called in both runMini
and startMiniServices functions)
* refactor: prevent HTTP/gRPC port collisions and improve error handling
- Add upfront reservation of all calculated gRPC ports before allocating HTTP ports
to prevent collisions where an HTTP port allocation could use a port that will
later be needed for a gRPC port calculation.
Example scenario that is now prevented:
- Master HTTP reallocated from 9333 to 9334 (original in use)
- Filer HTTP search finds 19334 available and assigns it
- Master gRPC calculated as 9334 + GrpcPortOffset = 19334 → collision!
Now: reserved gRPC ports are tracked upfront and HTTP port search skips them.
- Improve admin server gRPC port fallback error handling:
- Change from silent V(1) verbose log to Warningf to make the error visible
- Update comment to clarify this indicates a problem in the port initialization sequence
- Add explanation that the fallback calculation may cause bind failure
- Update ensureAllPortsAvailableOnIP comment to clarify it avoids reserved ports
* fix: enforce reserved ports in HTTP allocation and improve admin gRPC fallback
Critical fixes for port allocation safety:
1. Make findAvailablePortOnIP and ensurePortAvailableOnIP aware of reservedPorts:
- Add reservedPorts map parameter to both functions
- findAvailablePortOnIP now skips reserved ports when searching for alternatives
- ensurePortAvailableOnIP passes reservedPorts through to findAvailablePortOnIP
- This prevents HTTP ports from being allocated to ports reserved for gRPC
2. Update ensureAllPortsAvailableOnIP to pass reservedPorts:
- Pass the reservedPorts map to ensurePortAvailableOnIP calls
- Maintains the map updates (delete/add) for accuracy as ports change
3. Replace blind admin gRPC port fallback with proper availability checks:
- Previous code just calculated *miniAdminOptions.port + GrpcPortOffset
- New code checks both the calculated port and finds alternatives if needed
- Uses the same availability checking logic as initializeGrpcPortsOnIP
- Properly logs the fallback process and any port changes
- Will fail gracefully if no available ports found (consistent with other services)
These changes eliminate two critical vulnerabilities:
- HTTP port allocation can no longer accidentally claim gRPC ports
- Admin gRPC port fallback no longer blindly uses an unchecked port
* fix: prevent gRPC port collisions during multi-service fallback allocation
Critical fix for gRPC port allocation safety across multiple services:
Problem: When multiple services need gRPC port fallback allocation in sequence
(e.g., Master gRPC unavailable → finds alternative, then Filer gRPC unavailable
→ searches from calculated port), there was no tracking of previously allocated
gRPC ports. This could allow two services to claim the same port.
Scenario that is now prevented:
- Master gRPC: calculated 19333 unavailable → finds 19334 → assigns 19334
- Filer gRPC: calculated 18888 unavailable → searches from 18889, might land on
19334 if consecutive ports in range are unavailable (especially with custom
port configurations or in high-port-contention environments)
Solution:
- Add allocatedGrpcPorts map to track gRPC ports allocated within the function
- Check allocatedGrpcPorts before using calculated port for each service
- Pass allocatedGrpcPorts to findAvailablePortOnIP when finding fallback ports
- Add allocatedGrpcPorts[port] = true after each successful allocation
- This ensures no two services can allocate the same gRPC port
The fix handles both:
1. Calculated gRPC ports (when grpcPort == 0)
2. Explicitly set gRPC ports (when user provides -service.port.grpc value)
While default port spacing makes collision unlikely, this fix is essential for:
- Custom port configurations
- High-contention environments
- Edge cases with many unavailable consecutive ports
- Correctness and safety guarantees
* feat: enforce hard-fail behavior for explicitly specified ports
When users explicitly specify a port via command-line flags (e.g., -s3.port=8333),
the server should fail immediately if the port is unavailable, rather than silently
falling back to an alternative port. This prevents user confusion and makes misconfiguration
failures obvious.
Changes:
- Modified ensurePortAvailableOnIP() to check if a port was explicitly passed via isFlagPassed()
- If an explicit port is unavailable, return error instead of silently allocating alternative
- Updated ensureAllPortsAvailableOnIP() to handle the returned error and fail startup
- Modified runMini() to check error from ensureAllPortsAvailableOnIP() and return false on failure
- Default ports (not explicitly specified) continue to fallback to available alternatives
This ensures:
- Explicit ports: fail if unavailable (e.g., -s3.port=8333 fails if 8333 is taken)
- Default ports: fallback to alternatives (e.g., s3.port without flag falls back to 8334 if 8333 taken)
* fix: accurate error messages for explicitly specified unavailable ports
When a port is explicitly specified via CLI flags but is unavailable, the error message
now correctly reports the originally requested port instead of reporting a fallback port
that was calculated internally.
The issue was that the config file applied after CLI flag parsing caused isFlagPassed()
to return true for ports loaded from the config file (since flag.Visit() was called during
config file application), incorrectly marking them as explicitly specified.
Solution: Capture which port flags were explicitly passed on the CLI BEFORE the config file
is applied, storing them in the explicitPortFlags map. This preserves the accurate
distinction between user-specified ports and defaults/config-file ports.
Example:
- User runs: weed mini -dir=. -s3.port=22
- Now correctly shows: 'port 22 for S3 (specified by flag s3.port) is not available'
- Previously incorrectly showed: 'port 8334 for S3...' (some calculated fallback)
* fix: respect explicitly specified ports and prevent config file override
When a port is explicitly specified via CLI flags (e.g., -s3.port=8333),
the config file options should NOT override it. Previously, config file
options would be applied if the flag value differed from default, but
this check wasn't sufficient to prevent override in all cases.
Solution: Check the explicitPortFlags map before applying any config file
port options. If a port was explicitly passed on the CLI, skip applying
the config file option for that port.
This ensures:
- Explicit ports take absolute precedence over config file ports
- Config file ports are only used if port wasn't specified on CLI
- Example: 'weed mini -s3.port=8333' will use 8333, never the config file value
* fix: don't print usage on port allocation error
When a port allocation fails (e.g., explicit port is unavailable), exit
immediately without showing the usage example. This provides cleaner
error output when the error is expected (port conflict).
* fix: increase worker registration timeout for reconnections
Increase the worker registration timeout from 10 seconds to 30 seconds.
The 10-second timeout was too aggressive for reconnections when the admin
server might be busy processing other operations. Reconnecting workers need
more time to:
1. Re-establish the gRPC connection
2. Send the registration message
3. Wait for the admin server to process and respond
This prevents spurious "registration timeout" errors during long-running
mini instances when brief network hiccups or admin server load cause delays.
* refactor: clean up code quality issues
Remove no-op assignment (calculatedPort = calculatedPort) that had no effect.
The variable already holds the correct value when no alternative port is
found.
Improve documentation for the defensive gRPC port initialization fallback
in startAdminServer. While this code shouldn't execute in normal flow
because ensureAllPortsAvailableOnIP is called earlier in runMini, the
fallback handles edge cases where port initialization may have been skipped
or failed silently due to configuration changes or error handling paths.
* fix: prevent panic on close of closed channel in worker client reconnection
- Use idiomatic Go pattern of setting channels to nil after closing instead of flags
- Extract repeated safe-close logic into safeCloseChannel() helper method
- Call safeCloseChannel() in attemptConnection(), reconnect(), and handleDisconnect()
- In safeCloseChannel(), check if channel is not nil, close it, and set to nil
- Also set streamExit to nil in attemptConnection() when registration fails
- This follows Go best practices for channel management and prevents double-close panics
- Improved code maintainability by eliminating duplication
* fix: prevent panic on close of closed channel in worker client reconnection
- Use idiomatic Go pattern of setting channels to nil after closing instead of flags
- Extract repeated safe-close logic into safeCloseChannel() helper method
- Call safeCloseChannel() in attemptConnection(), reconnect(), and handleDisconnect()
- In safeCloseChannel(), check if channel is not nil, close it, and set to nil
- Also set streamExit to nil in attemptConnection() when registration fails
- Document thread-safety assumptions: function is safe in current usage (serialized
in managerLoop) but would need synchronization if used in concurrent contexts
- This follows Go best practices for channel management and prevents double-close panics
- Improved code maintainability by eliminating duplication
* feat: auto-configure optimal volume size limit based on available disk space
- Add calculateOptimalVolumeSizeMB() function with OS-independent disk detection
- Reuses existing stats.NewDiskStatus() which works across Linux, macOS, Windows, BSD, Solaris
- Algorithm: available disk / 100, rounded up to nearest power of 2 (64MB, 128MB, 256MB, 512MB, 1024MB)
- Volume size capped to maximum of 1GB (1024MB) for better stability
- Minimum volume size is 64MB
- Uses efficient bits.Len() for power-of-2 rounding instead of floating-point operations
- Only auto-calculates volume size if user didn't specify a custom value via -master.volumeSizeLimitMB
- Respects user-specified values without override
- Master logs whether value was auto-calculated or user-specified
- Welcome message displays the configured volume size with correct format string ordering
- Removed unused autoVolumeSizeMB variable (logging handles source tracking)
Fixes: #0
* Refactor: Consolidate volume size constants and use robust flag detection for mini mode
This commit addresses all code review feedback on the auto-optimal volume size feature:
1. **Consolidate hardcoded defaults into package-level constants**
- Moved minVolumeSizeMB=64 and maxVolumeSizeMB=1024 from local function-scope
constants to package-level constants for consistency and maintainability
- All three volume size constants (min, default, max) now defined in one place
2. **Implement robust flag detection using flag.Visit()**
- Added isFlagPassed() helper function using flag.Visit() to check if a CLI
flag was explicitly passed on the command line
- Replaces the previous implementation that checked if current value equals
default (which could incorrectly assume user intent if default was specified)
- Now correctly detects user override regardless of the actual value
3. **Restructure power-of-2 rounding logic for clarity**
- Changed from 'only round if above min threshold' to 'always round to power-of-2
first, then apply min/max constraints'
- More robust: works correctly even if min/max constants are adjusted in future
- Clearer intent: all non-zero values go through consistent rounding logic
4. **Fix import ordering**
- Added 'flag' import (aliased to fla9 package) to support isFlagPassed()
- Added 'math/bits' import to support power-of-2 rounding
Benefits:
- Better code organization with all volume size limits in package constants
- Correct user override detection that doesn't rely on value equality checks
- More maintainable rounding logic that's easier to understand and modify
- Consistent with SeaweedFS conventions (uses fla9 package like other commands)
* fix: Address code review feedback for volume size calculation
This commit resolves three code review comments for better code quality and robustness:
1. **Handle comma-separated directories in -dir flag**
- The -dir flag accepts comma-separated list of directories, but the volume size
calculation was passing the entire string to util.ResolvePath()
- Now splits on comma and uses the first directory for disk space calculation
- Added explanatory comment about the multi-directory support
- Ensures the optimal size calculation works correctly in all scenarios
2. **Change disk detection failure from verbose log to warning**
- When disk status cannot be determined, the warning is now logged via
glog.Warningf() instead of glog.V(1).Infof()
- Makes the event visible in default logs without requiring verbose mode
- Better alerting for operators about fallback to default values
3. **Avoid recalculating availableMB/100 and define bytesPerMB constant**
- Added bytesPerMB = 1024*1024 constant for clarity and reusability
- Replaced hardcoded (1024 * 1024) with bytesPerMB constant
- Store availableMB/100 in initialOptimalMB variable to avoid recalculation
- Log message now references initialOptimalMB instead of recalculating
- Improves maintainability and reduces redundant computation
All three changes maintain the same logic while improving code quality and
robustness as requested by the reviewer.
* fix: Address rounding logic, logging clarity, and disk capacity measurement issues
This commit resolves three additional code review comments to improve robustness
and clarity of the volume size calculation:
1. **Fix power-of-2 rounding logic for edge cases**
- The previous condition 'if optimalMB > 0' created a bug: when optimalMB=1,
bits.Len(0)=0, resulting in 1<<0=1, which is below minimum (64MB)
- Changed to explicitly handle zero case first: 'if optimalMB == 0'
- Separate zero-handling from power-of-2 rounding ensures correct behavior:
* optimalMB=0 → set to minVolumeSizeMB (64)
* optimalMB>=1 → apply power-of-2 rounding
- Then apply min/max constraints unconditionally
- More explicit and easier to reason about correctness
2. **Use total disk capacity instead of free space for stable configuration**
- Changed from diskStatus.Free (available space) to diskStatus.All (total capacity)
- Free space varies based on current disk usage at startup time
- This caused inconsistent volume sizes: same disk could get different sizes
depending on how full it is when the service starts
- Using total capacity ensures predictable, stable configuration across restarts
- Better aligns with the intended behavior of sizing based on disk capacity
- Added explanatory comments about why total capacity is more appropriate
3. **Improve log message clarity and accuracy**
- Updated message to clearly show:
* 'total disk capacity' instead of vague 'available disk'
* 'capacity/100 before rounding' to match actual calculation
* 'clamped to [min,max]' instead of 'capped to max' to show both bounds
* Includes min and max values in log for context
- More accurate and helpful for operators troubleshooting volume sizing
These changes ensure the volume size calculation is both correct and predictable.
* feat: Save mini configuration to file for persistence and documentation
This commit adds persistent configuration storage for the 'weed mini' command,
saving all non-default parameters to a JSON configuration file for:
1. **Configuration Documentation**
- All parameters actually passed on the command line are saved
- Provides a clear record of the running configuration
- Useful for auditing and understanding how the system is configured
2. **Persistence of Auto-Calculated Values**
- The auto-calculated optimal volume size (master.volumeSizeLimitMB) is saved
with a note indicating it was auto-calculated
- On restart, if the auto-calculated value exists, it won't be recalculated
- Users can delete the auto-calculated entry to force recalculation on next startup
- Provides stable, predictable configuration across restarts
3. **Configuration File Location**
- Saved to: <data-folder>/.seaweedfs/mini.config.json
- Uses the first directory from comma-separated -dir list
- Directory is created automatically if it doesn't exist
- JSON format for easy parsing and manual editing
4. **Implementation Details**
- Uses flag.Visit() to collect only explicitly passed flags
- Distinguishes between user-specified and auto-calculated values
- Includes helpful notes in the JSON file
- Graceful handling of save errors (logs warnings, doesn't fail startup)
The configuration file includes all parameters such as:
- IP and port settings (master, filer, volume, admin)
- Data directories and metadata folders
- Replication and collection settings
- S3 and IAM configurations
- Performance tuning parameters (concurrency limits, timeouts, etc.)
- Auto-calculated volume size (if applicable)
Example mini.config.json output:
{
"debug": "true",
"dir": "/data/seaweedfs",
"master.port": "9333",
"filer.port": "8888",
"volume.port": "9340",
"master.volumeSizeLimitMB.auto": "256",
"_note_auto_calculated": "This value was auto-calculated. Remove it to recalculate on next startup."
}
This allows operators to:
- Review what configuration was active
- Replicate the configuration on other systems
- Understand the startup behavior
- Control when auto-calculation occurs
* refactor: Change configuration file format to match command-line options format
Update the saved configuration format from JSON to shell-compatible options format
that matches how options are expected to be passed on the command line.
Configuration file: .seaweedfs/mini.options
Format: Each line contains a command-line option in the format -name=value
Benefits:
- Format is compatible with shell scripts and can be sourced
- Can be easily converted to command-line options
- Human-readable and editable
- Values with spaces are properly quoted
- Includes helpful comments explaining auto-calculated values
- Directly usable with weed mini command
The file can be used in multiple ways:
1. Extract options: cat .seaweedfs/mini.options | grep -v '^#' | tr '\n' ' '
2. Inline in command: weed mini \$(cat .seaweedfs/mini.options | grep -v '^#')
3. Manual review: cat .seaweedfs/mini.options
* refactor: Save mini.options directly to -dir folder
* docs: Update PR description with accurate algorithm and examples
Update the function documentation comments to accurately reflect the implemented
algorithm and provide real-world examples with actual calculated outputs.
Changes:
- Clarify that algorithm uses total disk capacity (not free space)
- Document exact calculation: capacity/100, round to power of 2, clamp to [64,1024]
- Add realistic examples showing input disk sizes and resulting volume sizes:
* 10GB disk → 64MB (minimum)
* 100GB disk → 64MB (minimum)
* 1TB disk → 64MB (minimum)
* 6.4TB disk → 64MB
* 12.8TB disk → 128MB
* 100TB disk → 1024MB (maximum)
* 1PB disk → 1024MB (maximum)
- Include note that values are rounded to next power of 2 and capped at 1GB
This helps users understand the volume size calculation and predict what size
will be set for their specific disk configurations.
* feat: integrate configuration file loading into mini startup
- Load mini.options file at startup if it exists
- Apply loaded configuration options before normal initialization
- CLI flags override file-based configuration
- Exclude 'dir' option from being saved (environment-specific)
- Configuration file format: option=value without leading dashes
- Auto-calculated volume size persists with recalculation marker
* Add 'weed mini' command for S3 beginners and small/dev use cases
This new command simplifies starting SeaweedFS by combining all components
in one process with optimized settings for development and small deployments.
Features:
- Starts master, volume, filer, S3, WebDAV, and admin in one command
- Volume size limit: 64MB (optimized for small files)
- Volume max: 0 (auto-configured based on free disk space)
- Pre-stop seconds: 1 (faster shutdown for development)
- Master peers: none (single master mode by default)
- Includes admin UI with one worker for maintenance tasks
- Clean, user-friendly startup message with all endpoint URLs
Usage:
weed mini # Use default temp directory
weed mini -dir=/data # Custom data directory
This makes it much easier for:
- Developers getting started with SeaweedFS
- Testing and development workflows
- Learning S3 API with SeaweedFS
- Small deployments that don't need complex clustering
* Change default volume server port to 9340 to avoid popular port 8080
* Fix nil pointer dereference by initializing all required volume server fields
Added missing VolumeServerOptions field initializations:
- id, publicUrl, diskType
- maintenanceMBPerSecond, ldbTimeout
- concurrentUploadLimitMB, concurrentDownloadLimitMB
- pprof, idxFolder
- inflightUploadDataTimeout, inflightDownloadDataTimeout
- hasSlowRead, readBufferSizeMB
This resolves the panic that occurred when starting the volume server.
* Fix multiple nil pointer dereferences in mini command
Added missing field initializations for:
- Master options: raftHashicorp, raftBootstrap, telemetryUrl, telemetryEnabled
- Filer options: filerGroup, saveToFilerLimit, concurrentUploadLimitMB,
concurrentFileUploadLimit, localSocket, showUIDirectoryDelete,
downloadMaxMBps, diskType, allowedOrigins, exposeDirectoryData, tusBasePath
- Volume options: id, publicUrl, diskType, maintenanceMBPerSecond, ldbTimeout,
concurrentUploadLimitMB, concurrentDownloadLimitMB, pprof, idxFolder,
inflightUploadDataTimeout, inflightDownloadDataTimeout, hasSlowRead, readBufferSizeMB
- WebDAV options: tlsPrivateKey, tlsCertificate, filerRootPath
- Admin options: master
These initializations are required to avoid runtime panics when starting components.
* Fix remaining S3 option nil pointers in mini command
* Update mini command: 256MB volume size and add S3 access instructions for beginners
* mini: set default master.volumeSizeLimitMB to 128MB and update help/banner text
* mini: shorten S3 help text to a concise pointer to docs/Admin UI
* mini: remove duplicated component bullet list, use concise sentence
* mini: tidy help alignment and update example usage
* mini: default -dir to current directory
* mini: load initial S3 credentials from env and write IAM config
* mini: use AWS env vars for initial S3 creds; instruct to create via Admin UI if absent
* Improve startup synchronization with channel-based coordination
- Replace fragile time.Sleep delays with robust channel-based synchronization
- Implement proper service dependency ordering (Master → Volume → Filer → S3/WebDAV/Admin)
- Add sync.WaitGroup for goroutine coordination
- Add startup readiness logging for better visibility
- Implement 10-second timeout for admin server startup
- Remove arbitrary sleep delays for faster, more reliable startup
- Services now start deterministically based on dependencies, not timing
This makes the startup process more reliable and eliminates race conditions on slow systems or under load.
* Refactor service startup logic for better maintainability
Extract service startup into dedicated helper functions:
- startMiniServices(): Orchestrates all service startup with dependency coordination
- startServiceWithCoordination(): Starts services with readiness signaling
- startServiceWithoutReady(): Starts services without readiness signaling
- startS3Service(): Encapsulates S3 initialization logic
Benefits:
- Reduced code duplication in runMini()
- Clearer separation of concerns
- Easier to add new services or modify startup sequence
- More testable code structure
- Improved readability with explicit service names and logging
* Remove unused serviceStartupInfo struct type
- Delete the serviceStartupInfo struct that was defined but never used
- Improves code clarity by removing dead code
- All service startup is now handled directly by helper functions
* Preserve existing IAM config file instead of truncating
- Use os.Stat to check if IAM config file already exists
- Only create and write configuration if file doesn't exist
- Log appropriate messages for each case:
* File exists: skip writing, preserve existing config
* File absent: create with os.OpenFile and write new config
* Stat error: log error without overwriting
- Set *miniIamConfig only when new file is successfully created
- Use os.O_CREATE|os.O_WRONLY flags for safe file creation
- Handles file operations with proper error checking and cleanup
* Fix CodeQL security issue: prevent logging of sensitive S3 credentials
- Add createdInitialIAM flag to track when initial IAM config is created from env vars
- Set flag in startS3Service() when new IAM config is successfully written
- Update welcome message to inform user of credential creation without exposing secrets
- Print only the username (mini) and config file location to user
- Never print access keys or secret keys in clear text
- Maintain security while keeping user informed of what was created
- Addresses CodeQL finding: Clear-text logging of sensitive information
* Fix three code review issues in weed mini command
1. Fix deadlock in service startup coordination:
- Run blocking service functions (startMaster, startFiler, etc.) in separate goroutines
- This allows readyChan to be closed and prevents indefinite blocking
- Services now start concurrently instead of sequentially blocking the coordinator
2. Use shared grace.StartDebugServer for consistency:
- Replace inline debug server startup with grace.StartDebugServer
- Improves code consistency with other commands (master, filer, etc.)
- Removes net/http import which is no longer needed
3. Simplify IAM config file cleanup with defer:
- Use 'defer f.Close()' instead of multiple f.Close() calls
- Ensures file is closed regardless of which code path is taken
- Improves robustness and code clarity
* fmt
* Fix: Remove misleading 'service is ready' logs
The previous fix removed 'go' from service function calls but left misleading
'service is ready' log messages. The service helpers now correctly:
- Call fn() directly (blocking) instead of 'go fn()' (non-blocking)
- Remove the 'service is ready' message that was printed before the service
actually started running
- Services run as blocking goroutines within the coordinator goroutine,
which keeps them alive while the program runs
- The readiness channels still work correctly because they're closed when
the coordinator finishes waiting for dependencies
* Update mini.go
* Fix four code review issues in weed mini command
1. Use restrictive file permissions (0600) for IAM config:
- Changed from 0644 to 0600 when creating iam_config.json
- Prevents world-readable access to sensitive AWS credentials
- Protects AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY
2. Remove unused sync.WaitGroup:
- Removed WaitGroup that was never waited on
- All services run as blocking goroutines in the coordinator
- Main goroutine blocks indefinitely with select{}
- Removes unnecessary complexity without changing behavior
3. Merge service startup helper functions:
- Combined startServiceWithCoordination and startServiceWithoutReady
- Made readyChan optional (nil for services without readiness signaling)
- Reduces code duplication and improves maintainability
- Both services now use single startServiceWithCoordination function
4. Fix admin server readiness check:
- Removed misleading timeout channel that never closed at startup
- Replaced with simple 2-second sleep before worker startup
- startAdminServer() blocks indefinitely, so channel would only close on shutdown
- Explicit sleep is clearer about the startup coordination intent
* Fix three code quality issues in weed mini command
1. Define volume configuration as named constants:
- Added miniVolumeMaxDataVolumeCounts = "0"
- Added miniVolumeMinFreeSpace = "1"
- Added miniVolumeMinFreeSpacePercent = "1"
- Removed local variable assignments in Volume startup
- Improves maintainability and documents configuration intent
2. Fix deadlock in startServiceWithCoordination:
- Changed from 'defer close(readyChan)' with blocking fn() to running fn() in goroutine
- Close readyChan immediately after launching service goroutine
- Prevents deadlock where fn() never returns, blocking defer execution
- Allows dependent services to start without waiting for blocking call
3. Improve admin server readiness check:
- Replaced fixed 2-second sleep with polling the gRPC port
- Polls up to 20 times (10 seconds total) with 500ms intervals
- Uses net.DialTimeout to check if port is available
- Properly handles IPv6 addresses using net.JoinHostPort
- Logs progress and warnings about connection status
- More robust than sleep against server startup timing variations
4. Add net import for network operations (IPv6 support)
Also fixed IAM config file close error handling to properly check error
from f.Close() and log any failures, preventing silent data loss on NFS.
* Document environment variable setup for S3 credentials
Updated welcome message to explain two ways to create S3 credentials:
1. Environment variables (recommended for quick setup):
- Set AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY
- Run 'weed mini -dir=/data'
- Creates initial 'mini' user credentials automatically
2. Admin UI (for managing multiple users and policies):
- Open http://localhost:23646 (Admin UI)
- Add identities to create new S3 credentials
This gives users clear guidance on the easiest way to get started with S3
credentials while also explaining the more advanced option for multiple users.
* Print welcome message after all services are running
Moved the welcome message printing from immediately after startMiniServices()
to after all services have been started and are ready. This ensures users see
the welcome message only after startup is complete, not mixed with startup logs.
Changes:
- Extract welcome message logic into printWelcomeMessage() function
- Call printWelcomeMessage() after startMiniServices() completes
- Change message from 'are starting' to 'are running and ready to use'
- This provides cleaner startup output without interleaved logs
* Wait for all services to complete before printing welcome message
The welcome message should only appear after all services are fully running and
the worker is connected. This prevents the message from appearing too early before
startup logs complete.
Changes:
- Pass allServicesReady channel through startMiniServices()
- Add adminReadyChan to track when admin/worker startup completes
- Signal allServicesReady when admin service is fully ready
- Wait for allServicesReady in runMini() before printing welcome message
- This ensures clean output: startup logs first, then welcome message once ready
Now the user sees all startup activity, then a clear welcome message when
everything is truly ready to use.
* Fix welcome message timing: print after worker is fully started
The welcome message was printing too early because allServicesReady was being
closed when the Admin service goroutine started, not when it actually completed.
The Admin service launches startMiniAdminWithWorker() which is a blocking call
that doesn't return until the worker is fully connected.
Now allServicesReady is passed through to startMiniWorker() which closes it
after the worker successfully starts and connects to the admin server.
This ensures the welcome message only appears after:
- Master is ready
- Volume server is ready
- Filer is ready
- S3 service is ready
- WebDAV service is ready
- Admin server is ready
- Worker is connected and running
All startup logs appear first, then the clean welcome message at the end.
* Wait for S3 and WebDAV services to be ready before showing welcome message
The welcome message was printing before S3 and WebDAV servers had fully
initialized. Now the readiness flow is:
1. Master → ready
2. Volume → ready
3. Filer → ready
4. S3 → ready (signals s3ReadyChan)
5. WebDAV → ready (signals webdavReadyChan)
6. Admin/Worker → starts, then waits for both S3 and WebDAV
7. Welcome message prints (all services truly ready)
Changes:
- Add s3ReadyChan and webdavReadyChan to service startup
- Pass S3 and WebDAV ready channels through to Admin service
- Admin/Worker waits for both S3 and WebDAV before closing allServicesReady
- This ensures welcome message appears only when all services are operational
* Admin service should wait for Filer, S3, and WebDAV to be ready
Admin service depends on Filer being operational since it uses the filer
for credential storage. It also makes sense to wait for S3 and WebDAV
since they are user-facing services that should be ready before Admin.
Updated dependencies:
- Admin now waits for: Master, Filer, S3, WebDAV
- This ensures all critical services are operational before Admin starts
- Welcome message will print only after all services including Admin are ready
* Add initialization delay for S3 and WebDAV services
S3 and WebDAV servers need extra time to fully initialize and start listening
after their service functions are launched. Added a 1-second delay after
launching S3 and WebDAV goroutines before signaling readiness.
This ensures the welcome message doesn't print until both services have
emitted their startup logs and are actually serving requests.
* Increase service initialization wait times for more reliable startup
- Increase S3 and WebDAV initialization delay from 1s to 2s to ensure they emit startup logs before welcome message
- Add 1s initialization delay for Filer to ensure it's listening
- Increase admin gRPC polling timeout from 10s to 20s to ensure admin server is fully ready
- This ensures welcome message prints only after all services are fully initialized and ready to accept requests
* Increase service wait time to 10 seconds for reliable startup
All services now wait 10 seconds after launching to ensure they are fully initialized and ready before signaling readiness to dependent services. This ensures the welcome message prints only after all services have fully started.
* Replace fixed 10s delay with intelligent port polling for service readiness
Instead of waiting a fixed 10 seconds for each service, now polls the service
port to check if it's actually accepting connections. This eliminates unnecessary
waiting and allows services to signal readiness as soon as they're ready.
- Polls each service port with up to 30 attempts (6 seconds total)
- Each attempt waits 200ms before retrying
- Stops polling immediately once service is ready
- Falls back gracefully if service is unknown
- Significantly faster startup sequence while maintaining reliability
* Replace channel-based coordination with HTTP pinging for service readiness
Instead of using channels to coordinate service startup, now uses HTTP GET requests
to ping each service endpoint to check if it's ready to accept connections.
Key changes:
- Removed all readiness channels (masterReadyChan, volumeReadyChan, etc.)
- Simplified startMiniServices to use sequential HTTP polling for each service
- startMiniService now just starts the service with logging
- waitForServiceReady uses HTTP client to ping service endpoints (max 6 seconds)
- waitForAdminServerReady uses HTTP GET to check admin server availability
- startMiniAdminWithWorker and startMiniWorker simplified without channel parameters
Benefits:
- Cleaner, more straightforward code
- HTTP pinging is more reliable than TCP port probing
- Services signal readiness through their HTTP endpoints
- Eliminates channel synchronization complexity
* log level
* Remove overly specific comment from volume size limit in welcome message
The '(good for small files)' comment is too limiting. The 128MB volume size
limit works well for general use cases, not just small files. Simplified the
message to just show the value.
* Ensure allServicesReady channel is always closed via defer
Add 'defer close(allServicesReady)' at the start of startMiniAdminWithWorker
to guarantee the channel is closed on ALL exit paths (normal and error).
This prevents the caller waiting on <-allServicesReady from ever hanging,
while removing the explicit close() at the successful end prevents panic
from double-close.
This makes the code more robust by:
- Guaranteeing channel closure even if worker setup fails
- Eliminating the possibility of caller hanging on errors
- Following Go defer patterns for resource cleanup
* Enhance health check polling for more robust service coordination
The service startup already uses HTTP health checks via waitForServiceReady()
to verify services are actually accepting connections. This commit improves
the health check implementation:
Changes:
- Elevated success logging to Info level so users see when services become ready
- Improved error messages to clarify that health check timeouts are not fatal
- Services continue startup even if health checks timeout (they may still work)
- Consistent handling of health check results across all services
This provides better visibility into service startup while maintaining the
existing robust coordination via HTTP pinging rather than just TCP port checks.
* Implement stricter error handling for robust mini server startup
Apply all PR review feedback to ensure the mini server fails fast and clearly
when critical components cannot start:
Changes:
1. Remove redundant miniVolumeMinFreeSpacePercent constant
- Simplified util.MustParseMinFreeSpace() call to use single parameter
2. Make service readiness checks fatal errors:
- Master, Volume, Filer, S3, WebDAV health check failures now return errors
- Prevents partially-functional servers from running
- Caller can handle errors gracefully instead of continuing with broken state
3. Make admin server readiness fatal:
- Admin gRPC availability is critical for worker startup
- Use glog.Fatalf to terminate with clear error message
4. Improve IAM config error handling:
- Treat all file operation failures (stat, open, write, close) as fatal
- Prevents silent failures in S3 credential setup
- User gets immediate feedback instead of authentication issues later
5. Use glog.Fatalf for critical worker setup errors:
- Failed to create worker directory, task directories, or worker instance
- Failed to create admin client or start worker
- Ensures mini server doesn't run in broken state
This ensures deterministic startup: services succeed completely or fail with
clear, actionable error messages for the user.
* Make health checks non-fatal for graceful degradation and improve IAM file handling
Address PR feedback to make the mini command more resilient for development:
1. Make health check failures non-fatal
- Master, Volume, Filer, S3, WebDAV health checks now log warnings but allow startup
- Services may still work even if health check endpoints aren't immediately available
- Aligns with intent of a dev-focused tool that should be forgiving of timing issues
- Only prevents startup if startup coordination or critical errors occur
2. Improve IAM config file handling
- Refactored to guarantee file is always closed using separate error variables
- Opens file once and handles write/close errors independently
- Maintains strict error handling while improving code clarity
- All file operation failures still cause fatal errors (as intended)
This makes startup more graceful while maintaining critical error handling for
fundamental failures like missing directories or configuration errors.
* Fix code quality issues in weed mini command
- Fix pointer aliasing: use value copy (*miniBindIp = *miniIp) instead of pointer assignment
- Remove misleading error return from waitForServiceReady() function
- Simplify health check callers to call waitForServiceReady() directly without error handling
- Remove redundant S3 option assignments already set in init() block
- Remove unused allServicesReady parameter from startMiniWorker() function
* Refactor welcome message to use template strings and add startup delay
- Convert welcome message to constant template strings for cleaner code
- Separate credentials instructions into dedicated constant
- Add 500ms delay after worker startup to allow full initialization before welcome message
- Improves output cleanliness by avoiding log interleaving with welcome message
* Fix code style issues in weed mini command
- Fix indentation in IAM config block (lines 424-432) to align with surrounding code
- Remove unused adminServerDone channel that was created but never read
* Address code review feedback for robustness and resource management
- Use defer f.Close() for IAM file handling to ensure file is closed in all code paths, preventing potential file descriptor leaks
- Use 127.0.0.1 instead of *miniIp for service readiness checks to ensure checks always target localhost, improving reliability in environments with firewalls or complex network configurations
- Simplify error handling in waitForAdminServerReady by using single error return instead of separate write/close error variables
* Fix function declaration formatting
- Separate closing brace of startS3Service from startMiniAdminWithWorker declaration with blank line
- Move comment to proper position above function declaration
- Run gofmt for consistent formatting
* Fix IAM config pointer assignment when file already exists
- Add missing *miniIamConfig = iamPath assignment when IAM config file already exists
- Ensures S3 service is properly pointed to the existing IAM configuration
- Retains logging to inform user that existing configuration is being preserved
* Improve pointer assignments and worker synchronization
- Simplify grpcPort and dataDir pointer assignments by directly dereferencing and assigning values instead of taking address of local variables
- Replace time.Sleep(500ms) with proper TCP-based polling to wait for worker gRPC port readiness
- Add waitForWorkerReady function that polls worker's gRPC port with max 6-second timeout
- Add net package import for TCP connection checks
- Improves code idiomaticity and synchronization robustness
* Refactor and simplify error handling for maintainability
- Remove unused error return from startMiniServices (always returned nil)
- Update runMini caller to not expect error from startMiniServices
- Refactor init() into component-specific helper functions:
* initMiniCommonFlags() for common options
* initMiniMasterFlags() for master server options
* initMiniFilerFlags() for filer server options
* initMiniVolumeFlags() for volume server options
* initMiniS3Flags() for S3 server options
* initMiniWebDAVFlags() for WebDAV server options
* initMiniAdminFlags() for admin server options
- Significantly improves code readability and maintainability
- Each component's flags are now in dedicated, focused functions
* Make lock_manager.RenewInterval configurable in LiveLock
- Add renewInterval field to LiveLock struct
- Modify StartLongLivedLock to accept renewInterval parameter
- Update all call sites to pass lock_manager.RenewInterval
- Default to lock_manager.RenewInterval if zero is passed
* S3 metrics: reduce collection interval to half of bucketSizeMetricsInterval
Since S3 metrics collection is not critical, check more frequently but
only collect when holding the distributed lock. This allows faster
detection of any issues while avoiding overhead on non-leader instances.
* Remove unused lock_manager import from bucket_size_metrics.go
* Refactor: Make lockTTL the primary parameter, derive renewInterval from it
Instead of configurable renew interval, lockTTL is now the input parameter.
The renewal interval is automatically derived as lockTTL / 2, ensuring that
locks are renewed well before expiration.
Changes:
- Replace renewInterval parameter with lockTTL
- Rename LiveLock.renewInterval field to lockTTL
- Calculate renewInterval as lockTTL / 2 inside the goroutine
- Update all call sites to pass lockTTL values
- Simplify sleep logic to use consistent renewInterval for both states
This approach is more intuitive and guarantees safe renewal windows.
* When locked, renew more aggressively to actively keep the lock
When holding the lock, sleep for renewInterval/2 to renew more frequently.
When seeking the lock, sleep for renewInterval to retry with normal frequency.
This ensures we actively maintain lock ownership while being less aggressive
when competing for the lock.
* Simplify: use consistent renewInterval for all lock states
Since renewInterval is already lockTTL / 2, there's no need to differentiate
between locked and unlocked states. Both use the same interval for consistency.
* Adjust sleep intervals for different lock states
- Locked instances sleep for renewInterval (lockTTL/2) to renew the lock
- Unlocked instances sleep for 5*renewInterval (2.5*lockTTL) to retry acquisition less frequently
* s3api: fix bucket-root listing w/ delimiter
* test: improve mock robustness for bucket-root listing test
- Make testListEntriesStream implement interface explicitly without embedding
- Add prefix filtering logic to testFilerClient to simulate real filer behavior
- Special-case prefix='/' to not filter for bucket root compatibility
- Add required imports for metadata and strings packages
This addresses review comments about test mock brittleness and accuracy.
* test: add clarifying comment for mock filtering behavior
Add detailed comment explaining which ListEntriesRequest parameters
are implemented (Prefix) vs ignored (Limit, StartFromFileName, etc.)
in the test mock to improve code documentation and future maintenance.
* logging
* less logs
* less check if already locked
* Fix worker reconnection race condition causing context canceled errors
Fixes#7824
This commit fixes critical connection stability issues between admin server
and workers that manifested as rapid reconnection cycles with 'context canceled'
errors, particularly after 24+ hours of operation in containerized environments.
Root Cause:
-----------
Race condition where TWO goroutines were calling stream.Recv() on the same
gRPC bidirectional stream concurrently:
1. sendRegistrationSync() started a goroutine that calls stream.Recv()
2. handleIncoming() also calls stream.Recv() in a loop
Per gRPC specification, only ONE goroutine can call Recv() on a stream at a
time. Concurrent Recv() calls cause undefined behavior, manifesting as
'context canceled' errors and stream corruption.
The race occurred during worker reconnection:
- Sometimes sendRegistrationSync goroutine read the registration response first (success)
- Sometimes handleIncoming read it first, causing sendRegistrationSync to timeout
- This left the stream in an inconsistent state, triggering 'context canceled' error
- The error triggered rapid reconnection attempts, creating a reconnection storm
Why it happened after 24 hours:
Container orchestration systems (Docker Swarm/Kubernetes) periodically restart
pods. Over time, workers reconnect multiple times. Each reconnection had a chance
of hitting the race condition. Eventually the race manifested and caused the
connection storm.
Changes:
--------
weed/worker/client.go:
- Start handleIncoming and handleOutgoing goroutines BEFORE sending registration
- Use sendRegistration() instead of sendRegistrationSync()
- Ensures only ONE goroutine (handleIncoming) calls stream.Recv()
- Eliminates race condition entirely
weed/admin/dash/worker_grpc_server.go:
- Clean up old connection when worker reconnects with same ID
- Cancel old connection context to stop its goroutines
- Prevents resource leaks and stale connection accumulation
Impact:
-------
Before: Random 'context canceled' errors during reconnection, rapid reconnection
cycles, resource leaks, requires manual restart to recover
After: Reliable reconnection, single Recv() goroutine, proper cleanup,
stable operation over 24+ hours
Testing:
--------
Build verified successful with no compilation errors.
How to reproduce the bug:
1. Start admin server and worker
2. Restart admin server (simulates container recreation)
3. Worker reconnects
4. Race condition may manifest, causing 'context canceled' error
5. Observe rapid reconnection cycles in logs
The fix is backward compatible and requires no configuration changes.
* Add MaxConnectionAge to gRPC server for Docker Swarm DNS handling
- Configure MaxConnectionAge and MaxConnectionAgeGrace for gRPC server
- Expand error detection in shouldInvalidateConnection for better cache invalidation
- Add connection lifecycle logging for debugging
* Add topology validation and nil-safety checks
- Add validation guards in UpdateTopology to prevent invalid updates
- Add nil-safety checks in rebuildIndexes
- Add GetDiskCount method for diagnostic purposes
* Fix worker registration race condition
- Reorder goroutine startup in WorkerStream to prevent race conditions
- Add defensive cleanup in unregisterWorker with panic-safe channel closing
* Add comprehensive topology update logging
- Enhance UpdateTopologyInfo with detailed logging of datacenter/node/disk counts
- Add metrics logging for topology changes
* Add periodic diagnostic status logging
- Implement topologyStatusLoop running every 5 minutes
- Add logTopologyStatus function reporting system metrics
- Run as background goroutine in maintenance manager
* Enhance master client connection logging
- Add connection timing logs in tryConnectToMaster
- Add reconnection attempt counting in KeepConnectedToMaster
- Improve diagnostic visibility for connection issues
* Remove unused sendRegistrationSync function
- Function is no longer called after switching to asynchronous sendRegistration
- Contains the problematic concurrent stream.Recv() pattern that caused race conditions
- Cleanup as suggested in PR review
* Clarify comment for channel closing during disconnection
- Improve comment to explain why channels are closed and their effect
- Make the code more self-documenting as suggested in PR review
* Address code review feedback: refactor and improvements
- Extract topology counting logic to shared helper function
CountTopologyResources() to eliminate duplication between
topology_management.go and maintenance_integration.go
- Use gRPC status codes for more robust error detection in
shouldInvalidateConnection(), falling back to string matching
for transport-level errors
- Add recover wrapper for channel close consistency in
cleanupStaleConnections() to match unregisterWorker() pattern
* Update grpc_client_server.go
* Fix data race on lastSeen field access
- Add mutex protection around conn.lastSeen = time.Now() in WorkerStream method
- Ensures thread-safe access consistent with cleanupStaleConnections
* Fix goroutine leaks in worker reconnection logic
- Close streamExit in reconnect() before creating new connection
- Close streamExit in attemptConnection() when sendRegistration fails
- Prevents orphaned handleOutgoing/handleIncoming goroutines from previous connections
- Ensures proper cleanup of goroutines competing for shared outgoing channel
* Minor cleanup improvements for consistency and clarity
- Remove redundant string checks in shouldInvalidateConnection that overlap with gRPC status codes
- Add recover block to Stop() method for consistency with other channel close operations
- Maintains valuable DNS and transport-specific error detection while eliminating redundancy
* Improve topology update error handling
- Return descriptive errors instead of silently preserving topology for invalid updates
- Change nil topologyInfo case to return 'rejected invalid topology update: nil topologyInfo'
- Change empty DataCenterInfos case to return 'rejected invalid topology update: empty DataCenterInfos (had X nodes, Y disks)'
- Keep existing glog.Warningf calls but append error details to logs before returning errors
- Allows callers to distinguish rejected updates and handle them appropriately
* Refactor safe channel closing into helper method
- Add safeCloseOutgoingChannel helper method to eliminate code duplication
- Replace repeated recover blocks in Stop, unregisterWorker, and cleanupStaleConnections
- Improves maintainability and ensures consistent error handling across all channel close operations
- Maintains same panic recovery behavior with contextual source identification
* Make connection invalidation string matching case-insensitive
- Convert error string to lowercase once for all string.Contains checks
- Improves robustness by catching error message variations from different sources
- Eliminates need for separate 'DNS resolution' and 'dns' checks
- Maintains same error detection coverage with better reliability
* Clean up warning logs in UpdateTopology to avoid duplicating error text
- Remove duplicated error phrases from glog.Warningf messages
- Keep concise contextual warnings that don't repeat the fmt.Errorf content
- Maintain same error returns for backward compatibility
* Add robust validation to prevent topology wipeout during master restart
- Reject topology updates with 0 nodes when current topology has nodes
- Prevents transient empty topology from overwriting valid state
- Improves resilience during master restart scenarios
- Maintains backward compatibility for legitimate empty topology updates
* fix: EC rebalance fails with replica placement 000
This PR fixes several issues with EC shard distribution:
1. Pre-flight check before EC encoding
- Verify target disk type has capacity before encoding starts
- Prevents encoding shards only to fail during rebalance
- Shows helpful error when wrong diskType is specified (e.g., ssd when volumes are on hdd)
2. Fix EC rebalance with replica placement 000
- When DiffRackCount=0, shards should be distributed freely across racks
- The '000' placement means 'no volume replication needed' because EC provides redundancy
- Previously all racks were skipped with error 'shards X > replica placement limit (0)'
3. Add unit tests for EC rebalance slot calculation
- TestECRebalanceWithLimitedSlots: documents the limited slots scenario
- TestECRebalanceZeroFreeSlots: reproduces the 0 free slots error
4. Add Makefile for manual EC testing
- make setup: start cluster and populate data
- make shell: open weed shell for EC commands
- make clean: stop cluster and cleanup
* fix: default -rebalance to true for ec.encode
The -rebalance flag was defaulting to false, which meant ec.encode would
only print shard moves but not actually execute them. This is a poor
default since the whole point of EC encoding is to distribute shards
across servers for fault tolerance.
Now -rebalance defaults to true, so shards are actually distributed
after encoding. Users can use -rebalance=false if they only want to
see what would happen without making changes.
* test/erasure_coding: improve Makefile safety and docs
- Narrow pkill pattern for volume servers to use TEST_DIR instead of
port pattern, avoiding accidental kills of unrelated SeaweedFS processes
- Document external dependencies (curl, jq) in header comments
* shell: refactor buildRackWithEcShards to reuse buildEcShards
Extract common shard bit construction logic to avoid duplication
between buildEcShards and buildRackWithEcShards helper functions.
* shell: update test for EC replication 000 behavior
When DiffRackCount=0 (replication "000"), EC shards should be
distributed freely across racks since erasure coding provides its
own redundancy. Update test expectation to reflect this behavior.
* erasure_coding: add distribution package for proportional EC shard placement
Add a new reusable package for EC shard distribution that:
- Supports configurable EC ratios (not hard-coded 10+4)
- Distributes shards proportionally based on replication policy
- Provides fault tolerance analysis
- Prefers moving parity shards to keep data shards spread out
Key components:
- ECConfig: Configurable data/parity shard counts
- ReplicationConfig: Parsed XYZ replication policy
- ECDistribution: Target shard counts per DC/rack/node
- Rebalancer: Plans shard moves with parity-first strategy
This enables seaweed-enterprise custom EC ratios and weed worker
integration while maintaining a clean, testable architecture.
* shell: integrate distribution package for EC rebalancing
Add shell wrappers around the distribution package:
- ProportionalECRebalancer: Plans moves using distribution.Rebalancer
- NewProportionalECRebalancerWithConfig: Supports custom EC configs
- GetDistributionSummary/GetFaultToleranceAnalysis: Helper functions
The shell layer converts between EcNode types and the generic
TopologyNode types used by the distribution package.
* test setup
* ec: improve data and parity shard distribution across racks
- Add shardsByTypePerRack helper to track data vs parity shards
- Rewrite doBalanceEcShardsAcrossRacks for two-pass balancing:
1. Balance data shards (0-9) evenly, max ceil(10/6)=2 per rack
2. Balance parity shards (10-13) evenly, max ceil(4/6)=1 per rack
- Add balanceShardTypeAcrossRacks for generic shard type balancing
- Add pickRackForShardType to select destination with room for type
- Add unit tests for even data/parity distribution verification
This ensures even read load during normal operation by spreading
both data and parity shards across all available racks.
* ec: make data/parity shard counts configurable in ecBalancer
- Add dataShardCount and parityShardCount fields to ecBalancer struct
- Add getDataShardCount() and getParityShardCount() methods with defaults
- Replace direct constant usage with configurable methods
- Fix unused variable warning for parityPerRack
This allows seaweed-enterprise to use custom EC ratios while
defaulting to standard 10+4 scheme.
* Address PR 7812 review comments
Makefile improvements:
- Save PIDs for each volume server for precise termination
- Use PID-based killing in stop target with pkill fallback
- Use more specific pkill patterns with TEST_DIR paths
Documentation:
- Document jq dependency in README.md
Rebalancer fix:
- Fix duplicate shard count updates in applyMovesToAnalysis
- All planners (DC/rack/node) update counts inline during planning
- Remove duplicate updates from applyMovesToAnalysis to avoid double-counting
* test/erasure_coding: use mktemp for test file template
Use mktemp instead of hardcoded /tmp/testfile_template.bin path
to provide better isolation for concurrent test runs.
* proto: add RaftLeadershipTransfer RPC for forced leader change
Add new gRPC RPC and messages for leadership transfer:
- RaftLeadershipTransferRequest: optional target_id and target_address
- RaftLeadershipTransferResponse: previous_leader and new_leader
This enables graceful leadership transfer before master maintenance,
reducing errors in filers during planned maintenance windows.
Ref: https://github.com/seaweedfs/seaweedfs/issues/7527
* proto: regenerate Go files for RaftLeadershipTransfer
Generated from master.proto changes.
* master: implement RaftLeadershipTransfer gRPC handler
Add gRPC handler for leadership transfer with support for:
- Transfer to any eligible follower (when target_id is empty)
- Transfer to a specific server (when target_id and target_address are provided)
Uses hashicorp/raft LeadershipTransfer() and LeadershipTransferToServer() APIs.
Returns the previous and new leader in the response.
* shell: add cluster.raft.leader.transfer command
Add weed shell command for graceful leadership transfer:
- Displays current cluster status before transfer
- Supports auto-selection of target (any eligible follower)
- Supports targeted transfer with -id and -address flags
- Provides clear feedback on success/failure with troubleshooting tips
Usage:
cluster.raft.leader.transfer
cluster.raft.leader.transfer -id <server_id> -address <grpc_address>
* master: add unit tests for raft gRPC handlers
Add tests covering:
- RaftLeadershipTransfer with no raft initialized
- RaftLeadershipTransfer with target_id but no address
- RaftListClusterServers with no raft initialized
- RaftAddServer with no raft initialized
- RaftRemoveServer with no raft initialized
These tests verify error handling when raft is not configured.
* shell: add tests for cluster.raft.leader.transfer command
Add tests covering:
- Command name and help text validation
- HasTag returns false for ResourceHeavy
- Validation of -id without -address
- Argument parsing with unknown flags
* master: clarify that leadership transfer requires -raftHashicorp
The default raft implementation (seaweedfs/raft, a goraft fork) does not
support graceful leadership transfer. This feature is only available when
using hashicorp raft (-raftHashicorp=true).
Update error messages and help text to make this requirement clear:
- gRPC handler returns specific error for goraft users
- Shell command help text notes the requirement
- Added test for goraft case
* test: use strings.Contains instead of custom helper
Replace custom contains/containsHelper functions with the standard
library strings.Contains for better maintainability.
* shell: return flag parsing errors instead of swallowing them
- Return the error from flag.Parse() instead of returning nil
- Update test to explicitly assert error for unknown flags
* test: document integration test scenarios for Raft leadership transfer
Add comments explaining:
- Why these unit tests only cover 'Raft not initialized' scenarios
- What integration tests should cover (with multi-master cluster)
- hashicorp/raft uses concrete types that cannot be easily mocked
* fix: address reviewer feedback on tests and leader routing
- Remove misleading tests that couldn't properly validate their
documented behavior without a real Raft cluster:
- TestRaftLeadershipTransfer_GoraftNotSupported
- TestRaftLeadershipTransfer_ValidationTargetIdWithoutAddress
- Change WithClient(false) to WithClient(true) for RaftLeadershipTransfer
RPC to ensure the request is routed to the current leader
* Improve cluster.raft.transferLeader command
- Rename command from cluster.raft.leader.transfer to cluster.raft.transferLeader
- Add symmetric validation: -id and -address must be specified together
- Handle case where same leader is re-elected after transfer
- Add test for -address without -id validation
- Add docker compose file for 5-master raft cluster testing
fix: S3 remote storage cold-cache read fails with 'size reported but no content available' (#7815)
When a remote-only entry's initial caching attempt times out or fails,
streamFromVolumeServers() now detects this case and retries caching
synchronously before streaming, similar to how the filer server handles
remote-only entries.
Changes:
- Modified streamFromVolumeServers() to check entry.IsInRemoteOnly()
before treating missing chunks as a data integrity error
- Added doCacheRemoteObject() as the core caching function (calls filer gRPC)
- Added buildRemoteObjectPath() helper to reduce code duplication
- Refactored cacheRemoteObjectWithDedup() and cacheRemoteObjectForStreaming()
to reuse the shared functions
- Added integration tests for remote storage scenarios
Fixes https://github.com/seaweedfs/seaweedfs/issues/7815
* mount: skip directory caching on file lookup and write
When opening or creating a file in a directory that hasn't been cached yet,
don't list the entire directory. Instead:
- For reads: fetch only the single file's metadata directly from the filer
- For writes: create on filer but skip local cache insertion
This fixes a performance issue where opening a file in a directory
with millions of files would hang because EnsureVisited() had to
list all entries before the open could complete.
The directory will still be cached when explicitly listed (ReadDir),
but individual file operations now bypass the full directory caching.
Key optimizations:
- Extract shared lookupEntry() method to eliminate code duplication
- Skip EnsureVisited on Lookup (file open)
- Skip cache insertion on Mknod, Mkdir, Symlink, Link if dir not cached
- Skip cache update on file sync/flush if dir not cached
- If directory IS cached and entry not found, return ENOENT immediately
Fixes#7145
* mount: add error handling for meta cache insert/update operations
Handle errors from metaCache.InsertEntry and metaCache.UpdateEntry calls
instead of silently ignoring them. This prevents silent cache inconsistencies
and ensures errors are properly propagated.
Files updated:
- filehandle_read.go: handle InsertEntry error in downloadRemoteEntry
- weedfs_file_sync.go: handle InsertEntry error in doFlush
- weedfs_link.go: handle UpdateEntry and InsertEntry errors in Link
- weedfs_symlink.go: handle InsertEntry error in Symlink
* mount: use error wrapping (%w) for consistent error handling
Use %w instead of %v in fmt.Errorf to preserve the original error,
allowing it to be inspected up the call stack with errors.Is/As.
2025-12-18 21:19:15 -08:00
Chris LuGitHubgemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
* Add consistent -debug and -debug.port flags to commands
Add -debug and -debug.port flags to weed master, weed volume, weed s3,
weed mq.broker, and weed filer.sync commands for consistency with
weed filer.
When -debug is enabled, an HTTP server starts on the specified port
(default 6060) serving runtime profiling data at /debug/pprof/.
For mq.broker, replaced the older -port.pprof flag with the new
-debug and -debug.port pattern for consistency.
* Update weed/util/grace/pprof.go
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
---------
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
* fix: achieve single-scan efficiency for S3 versioned object listing
When listing objects in a versioning-enabled bucket, the original code
triggered multiple getEntry calls per versioned object (up to 12 with
retries), causing excessive 'find' operations visible in Grafana and
leading to high memory usage.
This fix achieves single-scan efficiency by caching list metadata
(size, ETag, mtime, owner) directly in the .versions directory:
1. Add new Extended keys for caching list metadata in .versions dir
2. Update upload/copy/multipart paths to cache metadata when creating versions
3. Update getLatestVersionEntryFromDirectoryEntry to use cached metadata
(zero getEntry calls when cache is available)
4. Update updateLatestVersionAfterDeletion to maintain cache consistency
Performance improvement for N versioned objects:
- Before: N×1 to N×12 find operations per list request
- After: 0 extra find operations (all metadata from single scan)
This matches the efficiency of normal (non-versioned) object listing.
* Update s3api_object_versioning.go
* s3api: fix ETag handling for versioned objects and simplify delete marker creation
- Add Md5 attribute to synthetic logicalEntry for single-part uploads to ensure
filer.ETag() returns correct value in ListObjects response
- Simplify delete marker creation by initializing entry directly in mkFile callback
- Add bytes and encoding/hex imports for ETag parsing
* s3api: preserve default attributes in delete marker mkFile callback
Only modify Mtime field instead of replacing the entire Attributes struct,
preserving default values like Crtime, FileMode, Uid, and Gid that mkFile
initializes.
* s3api: fix ETag handling in newListEntry for multipart uploads
Prioritize ExtETagKey from Extended attributes before falling back to
filer.ETag(). This properly handles multipart upload ETags (format: md5-parts)
for versioned objects, where the synthetic entry has cached ETag metadata
but no chunks to calculate from.
* s3api: reduce code duplication in delete marker creation
Extract deleteMarkerExtended map to be reused in both mkFile callback
and deleteMarkerEntry construction.
* test: add multipart upload versioning tests for ETag verification
Add tests to verify that multipart uploaded objects in versioned buckets
have correct ETags when listed:
- TestMultipartUploadVersioningListETag: Basic multipart upload with 2 parts
- TestMultipartUploadMultipleVersionsListETag: Multiple multipart versions
- TestMixedSingleAndMultipartVersionsListETag: Mix of single-part and multipart
These tests cover a bug where synthetic entries for versioned objects
didn't include proper ETag handling for multipart uploads.
* test: add delete marker test for multipart uploaded versioned objects
TestMultipartUploadDeleteMarkerListBehavior verifies:
- Delete marker creation hides object from ListObjectsV2
- ListObjectVersions shows both version and delete marker
- Version ETag (multipart format) is preserved after delete marker
- Object can be accessed by version ID after delete marker
- Removing delete marker restores object visibility
* refactor: address code review feedback
- test: use assert.ElementsMatch for ETag verification (more idiomatic)
- s3api: optimize newListEntry ETag logic (check ExtETagKey first)
- s3api: fix edge case in ETag parsing (>= 2 instead of > 2)
* s3api: prevent stale cached metadata and preserve existing extended attrs
- setCachedListMetadata: clear old cached keys before setting new values
to prevent stale data when new version lacks certain fields (e.g., owner)
- createDeleteMarker: merge extended attributes instead of overwriting
to preserve any existing metadata on the entry
* s3api: extract clearCachedVersionMetadata to reduce code duplication
- clearCachedVersionMetadata: clears only metadata fields (size, mtime, etag, owner, deleteMarker)
- clearCachedListMetadata: now reuses clearCachedVersionMetadata + clears ID/filename
- setCachedListMetadata: uses clearCachedVersionMetadata (not clearCachedListMetadata
because caller has already set ID/filename)
* s3api: share timestamp between version entry and cache entry
Capture versionMtime once before mkFile and reuse for both:
- versionEntry.Attributes.Mtime in the mkFile callback
- versionEntryForCache.Attributes.Mtime for list caching
This keeps list vs. HEAD LastModified timestamps aligned.
* s3api: remove amzAccountId variable shadowing in multipart upload
Extract amzAccountId before mkFile callback and reuse in both places,
similar to how versionMtime is handled. Avoids confusion from
redeclaring the same variable.
* fix: S3 versioning memory leak in ListObjectVersions pagination
This commit fixes a memory leak issue in S3 versioning buckets where
ListObjectVersions with pagination (key-marker set) would collect ALL
versions in the bucket before filtering, causing O(N) memory usage.
Root cause:
- When keyMarker was set, maxCollect was set to 0 (unlimited)
- This caused findVersionsRecursively to traverse the entire bucket
- All versions were collected into memory, sorted, then filtered
Fix:
- Updated findVersionsRecursively to accept keyMarker and versionIdMarker
- Skips objects/versions before the marker during recursion (not after)
- Always respects maxCollect limit (never unlimited)
- Memory usage is now O(maxKeys) instead of O(total versions)
Refactoring:
- Introduced versionCollector struct to encapsulate collection state
- Extracted helper methods for cleaner, more testable code:
- matchesPrefixFilter: prefix matching logic
- shouldSkipObjectForMarker: keyMarker filtering
- shouldSkipVersionForMarker: versionIdMarker filtering
- processVersionsDirectory: .versions directory handling
- processExplicitDirectory: S3 directory object handling
- processRegularFile: pre-versioning file handling
- collectVersions: main recursive collection loop
- processDirectory: directory entry dispatch
This reduces the high QPS on 'find' and 'prefixList' operations
by skipping irrelevant objects during traversal.
Fixes customer-reported memory leak with high find/prefixList QPS
in Grafana for S3 versioning buckets.
* s3: infer version ID format from ExtLatestVersionIdKey metadata
Simplified version format detection:
- Removed ExtVersionIdFormatKey - no longer needed
- getVersionIdFormat() now infers format from ExtLatestVersionIdKey
- Uses isNewFormatVersionId() to check if latest version uses inverted format
This approach is simpler because:
- ExtLatestVersionIdKey is already stored in .versions directory metadata
- No need for separate format metadata field
- Format is naturally determined by the existing version IDs
Fixes#7810
The raft library would panic when prevLogIndex was beyond the end of
the log after compaction. The fix in raft v1.1.6 returns nil instead,
triggering the snapshot fallback mechanism.
* Fix Worker and Admin CA in helm chart
* Fix Worker and Admin CA in helm chart - add security.toml modification
* Fix Worker and Admin CA in helm chart - fix security.toml modification error
* Fix Worker and Admin CA in helm chart - fix errors in volume mounts
* Fix Worker and Admin CA in helm chart - address review comments
- Remove worker-cert from admin pod (principle of least privilege)
- Remove admin-cert from worker pod (principle of least privilege)
- Remove overly broad namespace wildcards from admin-cert dnsNames
- Remove overly broad namespace wildcards from worker-cert dnsNames
---------
Co-authored-by: chrislu <chris.lu@gmail.com>
Fix nil pointer dereference panic when starting master.follower.
The init() function was missing initialization for:
- maxParallelVacuumPerServer
- telemetryUrl
- telemetryEnabled
These fields are dereferenced in toMasterOption() causing a panic.
Fixes#7806
fix: authenticate before parsing form in IAM API (#7802)
The AuthIam middleware was calling ParseForm() before AuthSignatureOnly(),
which consumed the request body before signature verification could hash it.
For IAM requests (service != 's3'), the signature verification needs to hash
the request body. When ParseForm() was called first, the body was already
consumed, resulting in an empty body hash and SignatureDoesNotMatch error.
The fix moves authentication before form parsing. The streamHashRequestBody
function preserves the body after reading, so ParseForm() works correctly
after authentication.
Fixes#7802
The test was using a static bucket name 'test-iam-bucket' that could conflict
with buckets created by other tests or previous runs. Each test framework
creates new RSA keys for JWT signing, so the 'admin-user' identity differs
between runs. When the bucket exists from a previous test, the new admin
cannot access or delete it, causing AccessDenied errors.
Changed to use GenerateUniqueBucketName() which ensures each test run gets
its own bucket, avoiding cross-test conflicts.
* fix: ListBuckets returns empty for users with bucket-specific permissions (#7796)
The ListBucketsHandler was using sequential AND logic where ownership check
happened before permission check. If a user had 'List:bucketname' permission
but didn't own the bucket (different AmzIdentityId or missing owner metadata),
the bucket was filtered out before the permission check could run.
Changed to OR logic: a bucket is now visible if the user owns it OR has
explicit permission to list it. This allows users with bucket-specific
permissions like 'List:geoserver' to see buckets they have access to,
even if they don't own them.
Changes:
- Modified ListBucketsHandler to check both ownership and permission,
including bucket if either check passes
- Renamed isBucketVisibleToIdentity to isBucketOwnedByIdentity for clarity
- Added comprehensive tests in TestListBucketsIssue7796
Fixes#7796
* address review comments: optimize permission check and add integration test
- Skip permission check if user is already the owner (performance optimization)
- Add integration test that simulates the complete handler filtering logic
to verify the combination of ownership OR permission check works correctly
* add visibility assertions to each sub-test for self-contained verification
Each sub-test now verifies the final outcome using isOwner || canList logic,
making tests more robust and independently verifiable.
* fix: sync replica entries before ec.encode and volume.tier.move (#7797)
This addresses the data inconsistency risk in multi-replica volumes.
When ec.encode or volume.tier.move operates on a multi-replica volume:
1. Find the replica with the highest file count (the 'best' one)
2. Copy missing entries from other replicas INTO this best replica
3. Use this union replica for the destructive operation
This ensures no data is lost due to replica inconsistency before
EC encoding or tier moving.
Added:
- command_volume_replica_check.go: Core sync and select logic
- command_volume_replica_check_test.go: Test coverage
Modified:
- command_ec_encode.go: Call syncAndSelectBestReplica before encoding
- command_volume_tier_move.go: Call syncAndSelectBestReplica before moving
Fixes#7797
* test: add integration test for replicated volume sync during ec.encode
* test: improve retry logic for replicated volume integration test
* fix: resolve JWT issue in integration tests by using empty security.toml
* address review comments: add readNeedleMeta, parallelize status fetch, fix collection param, fix test issues
* test: use collection parameter consistently in replica sync test
* fix: convert weed binary path to absolute to work with changed working directory
* fix: remove skip behavior, keep tests failing on missing binary
* fix: always check recency for each needle, add divergent replica test
fix: add debug logging for JWT validation failures (#7788)
When JWT file ID validation fails during replication, add a log message
showing both the expected and actual file IDs to help diagnose issues.
Ref #7788
* Fix volume repeatedly toggling between crowded and uncrowded
Fixes#6712
The issue was that removeFromCrowded() was called in removeFromWritable(),
which is invoked whenever a volume temporarily becomes unwritable (due to
replica count fluctuations, heartbeat issues, or read-only state changes).
This caused unnecessary toggling:
1. Volume becomes temporarily unwritable → removeFromWritable() →
removeFromCrowded() logs 'becomes uncrowded'
2. Volume becomes writable again
3. CollectDeadNodeAndFullVolumes() runs → setVolumeCrowded() logs
'becomes crowded'
The fix:
- Remove removeFromCrowded() call from removeFromWritable()
- Only clear crowded status when volume is fully unregistered from
the layout (when location.Length() == 0 in UnRegisterVolume)
This ensures transient state changes don't cause log spam and the
crowded status accurately reflects the volume's size relative to
the grow threshold.
* Refactor test to use subtests for better readability
Address review feedback: use t.Run subtests to make the test's intent
clearer by giving each verification step a descriptive name.
* Fix S3 server panic when -s3.port.https equals -s3.port
When starting the S3 server with -s3.port.https=8333 (same as default
-s3.port), the server would panic with nil pointer dereference because:
1. The HTTP listener was already bound to port 8333
2. NewIpAndLocalListeners for HTTPS failed but error was discarded
3. ServeTLS was called on nil listener causing panic
This fix:
- Adds early validation to prevent using same port for HTTP and HTTPS
- Properly handles the error from NewIpAndLocalListeners for HTTPS
Fixes#7792
* Fix volume repeatedly toggling between crowded and uncrowded
Fixes#6712
The issue was that removeFromCrowded() was called in removeFromWritable(),
which is invoked whenever a volume temporarily becomes unwritable (due to
replica count fluctuations, heartbeat issues, or read-only state changes).
This caused unnecessary toggling:
1. Volume becomes temporarily unwritable → removeFromWritable() →
removeFromCrowded() logs 'becomes uncrowded'
2. Volume becomes writable again
3. CollectDeadNodeAndFullVolumes() runs → setVolumeCrowded() logs
'becomes crowded'
The fix:
- Remove removeFromCrowded() call from removeFromWritable()
- Only clear crowded status when volume is fully unregistered from
the layout (when location.Length() == 0 in UnRegisterVolume)
This ensures transient state changes don't cause log spam and the
crowded status accurately reflects the volume's size relative to
the grow threshold.
* Refactor test to use subtests for better readability
Address review feedback: use t.Run subtests to make the test's intent
clearer by giving each verification step a descriptive name.
* s3: fix remote object not caching
* s3: address review comments for remote object caching
- Fix leading slash in object name by using strings.TrimPrefix
- Return cached entry from CacheRemoteObjectToLocalCluster to get updated local chunk locations
- Reuse existing helper function instead of inline gRPC call
* s3/filer: add singleflight deduplication for remote object caching
- Add singleflight.Group to FilerServer to deduplicate concurrent cache operations
- Wrap CacheRemoteObjectToLocalCluster with singleflight to ensure only one
caching operation runs per object when multiple clients request the same file
- Add early-return check for already-cached objects
- S3 API calls filer gRPC with timeout and graceful fallback on error
- Clear negative bucket cache when bucket is created via weed shell
- Add integration tests for remote cache with singleflight deduplication
This benefits all clients (S3, HTTP, Hadoop) accessing remote-mounted objects
by preventing redundant cache operations and improving concurrent access performance.
Fixes: https://github.com/seaweedfs/seaweedfs/discussions/7599
* fix: data race in concurrent remote object caching
- Add mutex to protect chunks slice from concurrent append
- Add mutex to protect fetchAndWriteErr from concurrent read/write
- Fix incorrect error check (was checking assignResult.Error instead of parseErr)
- Rename inner variable to avoid shadowing fetchAndWriteErr
* fix: address code review comments
- Remove duplicate remote caching block in GetObjectHandler, keep only singleflight version
- Add mutex protection for concurrent chunk slice and error access (data race fix)
- Use lazy initialization for S3 client in tests to avoid panic during package load
- Fix markdown linting: add language specifier to code fence, blank lines around tables
- Add 'all' target to Makefile as alias for test-with-server
- Remove unused 'util' import
* style: remove emojis from test files
* fix: add defensive checks and sort chunks by offset
- Add nil check and type assertion check for singleflight result
- Sort chunks by offset after concurrent fetching to maintain file order
* fix: improve test diagnostics and path normalization
- runWeedShell now returns error for better test diagnostics
- Add all targets to .PHONY in Makefile (logs-primary, logs-remote, health)
- Strip leading slash from normalizedObject to avoid double slashes in path
---------
Co-authored-by: chrislu <chris.lu@gmail.com>
Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>
s3: fix fallback owner lookup to use specific version
Address review feedback: the fallback logic was incorrectly using
getLatestObjectVersion which returns the wrong owner when different
versions have different owners.
Fix by using getSpecificObjectVersion with the version.VersionId to
fetch the correct entry for the specific version being processed.
This also simplifies the code by removing the separate null version
handling since getSpecificObjectVersion already handles that case.
When paginating with keyMarker, we must collect all versions first because
filtering happens after sorting. Previously, we limited collection to maxKeys+1
which caused us to miss versions beyond the marker when there were many versions
before it.
The ListObjectVersions API was receiving key-marker and version-id-marker
parameters but not using them to filter results. This caused infinite
pagination loops when clients tried to paginate through results.
Fix by adding filtering logic after sorting:
- Skip versions with key < keyMarker (already returned in previous pages)
- For key == keyMarker, skip versions with versionId >= versionIdMarker
- Include versions with key > keyMarker or (key == keyMarker and versionId < versionIdMarker)
This respects the S3 sort order (key ascending, versionId descending for same key)
and correctly returns only versions that come AFTER the marker position.
Add pagination stress tests (>1000 versions) to the S3 versioning stress
test job in GitHub CI. These tests run on master branch pushes to validate
that ListObjectVersions correctly handles objects with more than 1000
versions using pagination.
* s3: add pagination to getObjectVersionList and reduce memory
This commit makes two improvements to S3 version listing:
1. Add pagination to getObjectVersionList:
- Previously hardcoded limit of 1000 versions per object
- Now paginates through all versions using startFrom marker
- Fixes correctness issue where objects with >1000 versions would
have some versions missing from list results
2. Reduce memory by not retaining full Entry:
- Replace Entry field with OwnerID string in ObjectVersion struct
- Extract owner ID when creating ObjectVersion
- Avoids retaining Chunks array which can be large for big files
- Clear seenVersionIds map after use to help GC
3. Update getObjectOwnerFromVersion:
- Use new OwnerID field instead of accessing Entry.Extended
- Maintains backward compatibility with fallback lookups
* s3: propagate errors from list operation instead of returning partial results
Address review feedback: when s3a.list fails during version listing,
the function was logging at V(2) level and returning partial results
with nil error. This hides the error and could lead to silent data
inconsistencies.
Fix by:
1. Log at Warningf level for better visibility
2. Return nil versions slice with the error to prevent partial results
from being processed as complete
* s3: fix memory leak in ListObjectVersions with early termination
This fixes a critical memory leak in S3 versioned bucket listing operations:
1. Add maxCollect parameter to findVersionsRecursively() to stop collecting
versions once we have enough for the response + truncation detection
2. Add early termination checks throughout the recursive traversal to
prevent scanning entire buckets when only a small number of results
are requested
3. Use clear() on tracking maps after collection to help GC reclaim memory
4. Create new slice with exact capacity when truncating results instead of
re-slicing, which allows GC to reclaim the excess backing array memory
5. Pre-allocate result slice with reasonable initial capacity to reduce
reallocations during collection
Before this fix, listing versions on a bucket with many objects and versions
would load ALL versions into memory before pagination, causing OOM crashes.
Fixes memory exhaustion when calling ListObjectVersions on large versioned buckets.
* s3: fix pre-allocation capacity to be consistent with maxCollect
Address review feedback: the previous capping logic caused an inconsistency
where initialCap was capped to 1000 but maxCollect was maxKeys+1, leading to
unnecessary reallocations when maxKeys was 1000.
Fix by:
1. Cap maxKeys to 1000 (S3 API limit) at the start of the function
2. Use maxKeys+1 directly for slice capacity, ensuring consistency with
the maxCollect parameter passed to findVersionsRecursively
2025-12-15 22:43:09 -08:00
Chris LuGitHubgemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
* fix: correctly detect missing source file during volume copy
The previous fix (commit 5c27522) incorrectly used progressedBytes == 0 to
detect if the source file didn't exist. This was wrong because it would also
delete files when the source file exists but is empty.
This fix:
1. Server side: Send ModifiedTsNs even for empty files, so the client knows
the source file exists
2. Client side: Check modifiedTsNs == 0 instead of progressedBytes == 0 to
determine if source file didn't exist
Now the logic correctly handles:
- Source file doesn't exist → No ModifiedTsNs sent → Remove empty file
- Source file exists but is empty → ModifiedTsNs sent → Keep empty file
- Source file exists with content → ModifiedTsNs sent → Keep file with content
Fixes#7777
* Update weed/server/volume_grpc_copy.go
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
---------
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
This is a follow-up fix to PR #7332 which partially addressed the issue.
The problem is that size=0 needles are in a gray area:
- IsValid() returns false for size=0 (because size must be > 0)
- IsDeleted() returns false for size=0 (because size must be < 0 or == TombstoneFileSize)
PR #7332 only fixed 2 places, but several other places still had the same bug:
1. needle_map_memory.go:doLoading - line 43 still used oldSize.IsValid()
2. needle_map_memory.go:DoOffsetLoading - used during vacuum and incremental loading
3. needle_map_leveldb.go:generateLevelDbFile - used when generating LevelDB needle maps
4. needle_map_leveldb.go:DoOffsetLoading - used during incremental loading for LevelDB
5. needle_map/compact_map.go:delete - couldn't delete size=0 entries because:
- The condition 'size > 0' failed for size=0
- Even if it passed, negating 0 gives 0 (not marking as deleted)
Changes:
- Changed size.IsValid() to !size.IsDeleted() in doLoading and DoOffsetLoading functions
- Fixed compact_map delete to use TombstoneFileSize for size=0 entries
Fixes#7293
Sort lists of filers, volume servers, masters, and message brokers
by address to ensure consistent ordering on page refresh.
This fixes the non-deterministic ordering caused by iterating over
Go maps with range.
Fixes#7781
* Fix EC Volumes page header styling to match admin theme
Fixes#7779
The EC Volumes page was rendering with bright Bootstrap default colors
instead of the admin dark theme because it was structured as a standalone
HTML document with its own DOCTYPE, head, and body tags.
This fix converts the template to be a content fragment (like other
properly styled templates such as cluster_ec_shards.templ) so it
correctly inherits the admin.css styling when rendered within the layout.
* Address review comments: fix URL interpolation and falsy value check
- Fix collection filter link to use templ.URL() for proper interpolation
- Change updateUrl() falsy check from 'if (params[key])' to
'if (params[key] != null)' to handle 0 and false values correctly
* Address additional review comments
- Use erasure_coding.TotalShardsCount constant instead of hardcoded '14'
for shard count displays (lines 88 and 214)
- Improve error handling in repairVolume() to check response.ok before
parsing JSON, preventing confusing errors on non-JSON responses
- Remove unused totalSize variable in formatShardRangesWithSizes()
- Simplify redundant pagination conditions
* Remove unused code: displayShardLocationsHTML, groupShardsByServerWithSizes, formatShardRangesWithSizes
These functions and templates were defined but never called anywhere
in the codebase. Removing them reduces code maintenance burden.
* Address review feedback: improve code quality
- Add defensive JSON response validation in repairVolume function
- Replace O(n²) bubble sorts with Go's standard sort.Ints and sort.Slice
- Document volume status thresholds explaining EC recovery logic:
* Critical: unrecoverable (more than DataShardsCount missing)
* Degraded: high risk (more than half DataShardsCount missing)
* Incomplete: reduced redundancy (more than half ParityShardsCount missing)
* Minor: fully recoverable with good margin
* Fix redundant shard count display in Healthy Volumes card
Changed from 'Complete (14/14 shards)' to 'All 14 shards present' since
the numerator and denominator were always the same value.
* Use templ.URL for default collection link for consistency
* Fix Clear Filter link to stay on EC Volumes page
Changed href from /cluster/ec-shards to /cluster/ec-volumes so clearing
the filter stays on the current page instead of navigating away.
* feat: add S3 bucket size and object count metrics
Adds periodic collection of bucket size metrics:
- SeaweedFS_s3_bucket_size_bytes: logical size (deduplicated across replicas)
- SeaweedFS_s3_bucket_physical_size_bytes: physical size (including replicas)
- SeaweedFS_s3_bucket_object_count: object count (deduplicated)
Collection runs every 1 minute via background goroutine that queries
filer Statistics RPC for each bucket's collection.
Also adds Grafana dashboard panels for:
- S3 Bucket Size (logical vs physical)
- S3 Bucket Object Count
* address PR comments: fix bucket size metrics collection
1. Fix collectCollectionInfoFromMaster to use master VolumeList API
- Now properly queries master for topology info
- Uses WithMasterClient to get volume list from master
- Correctly calculates logical vs physical size based on replication
2. Return error when filerClient is nil to trigger fallback
- Changed from 'return nil, nil' to 'return nil, error'
- Ensures fallback to filer stats is properly triggered
3. Implement pagination in listBucketNames
- Added listBucketPageSize constant (1000)
- Uses StartFromFileName for pagination
- Continues fetching until fewer entries than limit returned
4. Handle NewReplicaPlacementFromByte error and prevent division by zero
- Check error return from NewReplicaPlacementFromByte
- Default to 1 copy if error occurs
- Add explicit check for copyCount == 0
* simplify bucket size metrics: remove filer fallback, align with quota enforcement
- Remove fallback to filer Statistics RPC
- Use only master topology for collection info (same as s3.bucket.quota.enforce)
- Updated comments to clarify this runs the same collection logic as quota enforcement
- Simplified code by removing collectBucketSizeFromFilerStats
* use s3a.option.Masters directly instead of querying filer
* address PR comments: fix dashboard overlaps and improve metrics collection
Grafana dashboard fixes:
- Fix overlapping panels 55 and 59 in grafana_seaweedfs.json (moved 59 to y=30)
- Fix grid collision in k8s dashboard (moved panel 72 to y=48)
- Aggregate bucket metrics with max() by (bucket) for multi-instance S3 gateways
Go code improvements:
- Add graceful shutdown support via context cancellation
- Use ticker instead of time.Sleep for better shutdown responsiveness
- Distinguish EOF from actual errors in stream handling
* improve bucket size metrics: multi-master failover and proper error handling
- Initial delay now respects context cancellation using select with time.After
- Use WithOneOfGrpcMasterClients for multi-master failover instead of hardcoding Masters[0]
- Properly propagate stream errors instead of just logging them (EOF vs real errors)
* improve bucket size metrics: distributed lock and volume ID deduplication
- Add distributed lock (LiveLock) so only one S3 instance collects metrics at a time
- Add IsLocked() method to LiveLock for checking lock status
- Fix deduplication: use volume ID tracking instead of dividing by copyCount
- Previous approach gave wrong results if replicas were missing
- Now tracks seen volume IDs and counts each volume only once
- Physical size still includes all replicas for accurate disk usage reporting
* rename lock to s3.leader
* simplify: remove StartBucketSizeMetricsCollection wrapper function
* fix data race: use atomic operations for LiveLock.isLocked field
- Change isLocked from bool to int32
- Use atomic.LoadInt32/StoreInt32 for all reads/writes
- Sync shared isLocked field in StartLongLivedLock goroutine
* add nil check for topology info to prevent panic
* fix bucket metrics: use Ticker for consistent intervals, fix pagination logic
- Use time.Ticker instead of time.After for consistent interval execution
- Fix pagination: count all entries (not just directories) for proper termination
- Update lastFileName for all entries to prevent pagination issues
* address PR comments: remove redundant atomic store, propagate context
- Remove redundant atomic.StoreInt32 in StartLongLivedLock (AttemptToLock already sets it)
- Propagate context through metrics collection for proper cancellation on shutdown
- collectAndUpdateBucketSizeMetrics now accepts ctx
- collectCollectionInfoFromMaster uses ctx for VolumeList RPC
- listBucketNames uses ctx for ListEntries RPC
When copying/evacuating empty volumes, the .idx file may not exist on disk
(this is allowed by checkIdxFile for volumes with only super block in .dat).
This fix:
1. Uses os.IsNotExist() instead of err == os.ErrNotExist for proper
wrapped error checking in CopyFile
2. Treats missing source file as success when StopOffset == 0 (empty file)
3. Allows checkCopyFiles to pass when idx file doesn't exist but
IdxFileSize == 0 (empty volume)
Fixes volumeServer.evacuate and volume.fix.replication for empty volumes.
* fix: add S3 bucket traffic sent metric tracking
The BucketTrafficSent() function was defined but never called, causing
the S3 Bucket Traffic Sent Grafana dashboard panel to not display data.
Added BucketTrafficSent() calls in the streaming functions:
- streamFromVolumeServers: for inline and chunked content
- streamFromVolumeServersWithSSE: for encrypted range and full object requests
The traffic received metric already worked because BucketTrafficReceived()
was properly called in putToFiler() for both regular and multipart uploads.
* feat: add S3 API Calls per Bucket panel to Grafana dashboards
Added a new panel showing API calls per bucket using the existing
SeaweedFS_s3_request_total metric aggregated by bucket.
Updated all Grafana dashboard files:
- other/metrics/grafana_seaweedfs.json
- other/metrics/grafana_seaweedfs_k8s.json
- other/metrics/grafana_seaweedfs_heartbeat.json
- k8s/charts/seaweedfs/dashboards/seaweedfs-grafana-dashboard.json
* address PR comments: use actual bytes written for traffic metrics
- Use actual bytes written from w.Write instead of expected size for inline content
- Add countingWriter wrapper to track actual bytes for chunked content streaming
- Update streamDecryptedRangeFromChunks to return actual bytes written for SSE
- Remove redundant nil check that caused linter warning
- Fix duplicate panel id 86 in grafana_seaweedfs.json (changed to 90)
- Fix overlapping panel positions in grafana_seaweedfs_k8s.json (rebalanced x positions)
* fix grafana k8s dashboard: rebalance S3 panels to avoid overlap
- Panel 86 (S3 API Calls per Bucket): w:6, x:0, y:15
- Panel 67 (S3 Request Duration 95th): w:6, x:6, y:15
- Panel 68 (S3 Request Duration 80th): w:6, x:12, y:15
- Panel 65 (S3 Request Duration 99th): w:6, x:18, y:15
All four S3 panels now fit in a single row (y:15) with width 6 each.
Filer row header at y:22 and subsequent panels remain correctly positioned.
* add input validation and clarify comments in adjustRangeForPart
- Add validation that partStartOffset <= partEndOffset at function start
- Add clarifying comments for suffix-range handling where clientEnd
temporarily holds the suffix length before being reassigned
* align pluginVersion for panel 86 to 10.3.1 in k8s dashboard
* track partial writes for accurate egress traffic accounting
- Change condition from 'err == nil' to 'written > 0' for inline content
- Move BucketTrafficSent before error check for chunked content streaming
- Track traffic even on partial SSE range writes
- Track traffic even on partial full SSE object copies
This ensures egress traffic is counted even when writes fail partway through,
providing more accurate bandwidth metrics.
* s3: warm bucket config cache on startup for multi-filer consistency
In multi-filer clusters, the bucket configuration cache (storing Object Lock,
versioning, and other settings) was not being pre-populated on S3 API server
startup. This caused issues where:
1. After server restart, Object Lock and versioning settings appeared lost
until the bucket was accessed (lazy loading)
2. In multi-filer clusters, race conditions during bucket creation could
result in inconsistent Object Lock configuration
This fix warms the bucketConfigCache during BucketRegistry initialization,
ensuring all bucket configurations (including Object Lock and versioning)
are immediately available after restart without waiting for first access.
The fix piggybacks on the existing BucketRegistry.init() which already
iterates through all buckets, adding a call to update the config cache
with each bucket's extended attributes.
* s3: add visibility logging for bucket config cache warming
- Add bucket count tracking during initialization
- Log error if bucket listing fails
- Log INFO message with count of warmed buckets on successful init
This improves observability for the cache warming process and addresses
review feedback about error handling visibility.
* s3: fix bucket deletion not invalidating config cache
Bug fix: The metadata subscription handler had an early return when
NewEntry was nil, which skipped the onBucketMetadataChange call for
bucket deletions. This caused deleted buckets to remain in the config
cache.
The fix moves onBucketMetadataChange before the nil check so it's
called for all events (create, update, delete). The IAM and circuit
breaker updates still require NewEntry content, so they remain after
the check.
* s3: handle config file deletions for IAM and circuit breaker
Refactored the metadata subscription handlers to properly handle all
event types (create, update, delete) for IAM and circuit breaker configs:
- Renamed onIamConfigUpdate -> onIamConfigChange
- Renamed onCircuitBreakerConfigUpdate -> onCircuitBreakerConfigChange
- Both handlers now check for deletions (newEntry == nil && oldEntry != nil)
- On config file deletion, reset to empty config by loading empty bytes
- Simplified processEventFn to call all handlers unconditionally
- Each handler checks for nil entries internally
This ensures that deleting identity.json or circuit_breaker.json will
clear the in-memory config rather than leaving stale data.
* s3: restore NewParentPath handling for rename/move operations
The directory resolution logic was accidentally removed. This restores
the check for NewParentPath which is needed when files are renamed or
moved - in such cases, NewParentPath contains the destination directory
which should be used for directory matching in the handlers.
* filer: improve FoundationDB performance by disabling batch by default
This PR addresses a performance issue where FoundationDB filer was achieving
only ~757 ops/sec with 12 concurrent S3 clients, despite FDB being capable
of 17,000+ ops/sec.
Root cause: The write batcher was waiting up to 5ms for each operation to
batch, even though S3 semantics require waiting for durability confirmation.
This added artificial latency that defeated the purpose of batching.
Changes:
- Disable write batching by default (batch_enabled = false)
- Each write now commits immediately in its own transaction
- Reduce batch interval from 5ms to 1ms when batching is enabled
- Add batch_enabled config option to toggle behavior
- Improve batcher to collect available ops without blocking
- Add benchmarks comparing batch vs no-batch performance
Benchmark results (16 concurrent goroutines):
- With batch: 2,924 ops/sec (342,032 ns/op)
- Without batch: 4,625 ops/sec (216,219 ns/op)
- Improvement: +58% faster
Configuration:
- Default: batch_enabled = false (optimal for S3 PUT latency)
- For bulk ingestion: set batch_enabled = true
Also fixes ARM64 Docker test setup (shell compatibility, fdbserver path).
* fix: address review comments - use atomic counter and remove duplicate batcher
- Use sync/atomic.Uint64 for unique filenames in concurrent benchmarks
- Remove duplicate batcher creation in createBenchmarkStoreWithBatching
(initialize() already creates batcher when batchEnabled=true)
* fix: add realistic default values to benchmark store helper
Set directoryPrefix, timeout, and maxRetryDelay to reasonable defaults
for more realistic benchmark conditions.
* s3: fix PutObject ETag format for multi-chunk uploads
Fix issue #7768: AWS S3 SDK for Java fails with 'Invalid base 16
character: -' when performing PutObject on files that are internally
auto-chunked.
The issue was that SeaweedFS returned a composite ETag format
(<md5hash>-<count>) for regular PutObject when the file was split
into multiple chunks due to auto-chunking. However, per AWS S3 spec,
the composite ETag format should only be used for multipart uploads
(CreateMultipartUpload/UploadPart/CompleteMultipartUpload API).
Regular PutObject should always return a pure MD5 hash as the ETag,
regardless of how the file is stored internally.
The fix ensures the MD5 hash is always stored in entry.Attributes.Md5
for regular PutObject operations, so filer.ETag() returns the pure
MD5 hash instead of falling back to ETagChunks() composite format.
* test: add comprehensive ETag format tests for issue #7768
Add integration tests to ensure PutObject ETag format compatibility:
Go tests (test/s3/etag/):
- TestPutObjectETagFormat_SmallFile: 1KB single chunk
- TestPutObjectETagFormat_LargeFile: 10MB auto-chunked (critical for #7768)
- TestPutObjectETagFormat_ExtraLargeFile: 25MB multi-chunk
- TestMultipartUploadETagFormat: verify composite ETag for multipart
- TestPutObjectETagConsistency: ETag consistency across PUT/HEAD/GET
- TestETagHexValidation: simulate AWS SDK v2 hex decoding
- TestMultipleLargeFileUploads: stress test multiple large uploads
Java tests (other/java/s3copier/):
- Update pom.xml to include AWS SDK v2 (2.20.127)
- Add ETagValidationTest.java with comprehensive SDK v2 tests
- Add README.md documenting SDK versions and test coverage
Documentation:
- Add test/s3/SDK_COMPATIBILITY.md documenting validated SDK versions
- Add test/s3/etag/README.md explaining test coverage
These tests ensure large file PutObject (>8MB) returns pure MD5 ETags
(not composite format), which is required for AWS SDK v2 compatibility.
* fix: lower Java version requirement to 11 for CI compatibility
* address CodeRabbit review comments
- s3_etag_test.go: Handle rand.Read error, fix multipart part-count logging
- Makefile: Add 'all' target, pass S3_ENDPOINT to test commands
- SDK_COMPATIBILITY.md: Add language tag to fenced code block
- ETagValidationTest.java: Add pagination to cleanup logic
- README.md: Clarify Go SDK tests are in separate location
* ci: add s3copier ETag validation tests to Java integration tests
- Enable S3 API (-s3 -s3.port=8333) in SeaweedFS test server
- Add S3 API readiness check to wait loop
- Add step to run ETagValidationTest from s3copier
This ensures the fix for issue #7768 is continuously tested
against AWS SDK v2 for Java in CI.
* ci: add S3 config with credentials for s3copier tests
- Add -s3.config pointing to docker/compose/s3.json
- Add -s3.allowDeleteBucketNotEmpty for test cleanup
- Set S3_ACCESS_KEY and S3_SECRET_KEY env vars for tests
* ci: pass S3 config as Maven system properties
Pass S3_ENDPOINT, S3_ACCESS_KEY, S3_SECRET_KEY via -D flags
so they're available via System.getProperty() in Java tests
Modernized all S3 integration tests to use **weed mini** - a single optimized binary for simplified testing infrastructure. This eliminates the need to manually manage separate master, volume, and filer server processes.
## Feature Branch
- **Branch**: `feature/modernize-s3-tests`
- **PR**: #7876
- **Status**: ✅ Complete - All changes committed and pushed
## Infrastructure Changes
### 1. Shared Server Management (`test/s3/testutil/server.go`)
**Purpose**: Centralized server lifecycle management for all test suites
**Key Features**:
- Auto-discovers weed binary at runtime
- Creates isolated test data directory (`/tmp/weed-test-data`)
- Configurable S3 port (default 8333)
- HTTP health check polling for startup verification
| `testETagConsistency` | Verify ETag consistency across PUT/HEAD/GET |
| `testMultipleLargeFileUploads` | Stress test multiple large uploads |
### Background: Issue #7768
AWS SDK v2 for Java includes checksum validation that decodes the ETag as hexadecimal. When SeaweedFS returned composite ETags (`<md5>-<count>`) for regular `PutObject` with internally auto-chunked files, the SDK failed with:
```
java.lang.IllegalArgumentException: Invalid base 16 character: '-'
```
**Per AWS S3 specification:**
- `PutObject`: ETag is always a pure MD5 hex string (32 chars)
- `CompleteMultipartUpload`: ETag is composite format (`<md5>-<partcount>`)
The fix ensures SeaweedFS follows this specification.
## Project Structure
```
src/
├── main/java/com/seaweedfs/s3/
│ ├── PutObject.java # Example PutObject with SDK v1
AWS SDK v2 for Java includes strict ETag validation in `ChecksumsEnabledValidator.validatePutObjectChecksum`. It decodes the ETag as a hexadecimal MD5 hash using `Base16Codec.decode()`.
**Impact**: If the ETag contains non-hexadecimal characters (like `-` in composite format), the SDK fails with:
```text
java.lang.IllegalArgumentException: Invalid base 16 character: '-'
```
**Resolution**: SeaweedFS now correctly returns:
- **PutObject**: Pure MD5 hex ETag (32 characters) regardless of internal chunking
**Test Coverage**: `test/s3/etag/` and `other/java/s3copier/ETagValidationTest.java`
### AWS SDK v1 vs v2 Differences
| Feature | SDK v1 | SDK v2 |
|---------|--------|--------|
| ETag hex validation | No | Yes (strict) |
| Checksum validation | Basic | Enhanced |
| Async support | Limited | Full |
| Default retry behavior | Lenient | Stricter |
### Large File Handling
SeaweedFS auto-chunks files larger than **8MB** for efficient storage. This is transparent to clients, but previously affected ETag format. The current implementation ensures:
1. Regular `PutObject` (any size): Returns pure MD5 ETag
2. Multipart upload: Returns composite ETag per AWS S3 specification
## Test Categories by File Size
| Category | Size | Chunks | ETag Format |
|----------|------|--------|-------------|
| Small | < 256KB | 1 (inline) | Pure MD5 |
| Medium | 256KB - 8MB | 1 | Pure MD5 |
| Large | 8MB - 24MB | 2-3 | Pure MD5 |
| Extra Large | > 24MB | 4+ | Pure MD5 |
| Multipart | N/A | Per part | Composite |
## Running SDK Compatibility Tests
### Go Tests
```bash
# Run all ETag tests
cd test/s3/etag && make test
# Run large file tests only
cd test/s3/etag && make test-large
```
### Java Tests
```bash
# Run all Java SDK tests
cd other/java/s3copier && mvn test
# Run only ETag validation tests
cd other/java/s3copier && mvn test -Dtest=ETagValidationTest
```
### Python Tests
```bash
# Run PyArrow S3 tests
cd test/s3/parquet && make test
```
## Adding New SDK Tests
When adding tests for new SDKs, ensure:
1. **Large file tests (>8MB)**: Critical for verifying ETag format with auto-chunking
2. **Multipart upload tests**: Verify composite ETag format
3. **Checksum validation**: Test SDK-specific checksum validation if applicable
4. **Document SDK version**: Add to this compatibility matrix
## Known Issues and Workarounds
### Issue: Older SDK Versions
Some very old SDK versions (e.g., AWS SDK v1 for Java < 1.11.x) may have different behavior. Testing with the versions listed above is recommended.
### Issue: Custom Checksum Algorithms
AWS SDK v2 supports SHA-256 and CRC32 checksums in addition to MD5. SeaweedFS currently returns MD5-based ETags. For checksums other than MD5, use the `x-amz-checksum-*` headers.
This test suite verifies that SeaweedFS returns correct ETag formats for S3 operations, ensuring compatibility with AWS S3 SDKs.
## Background
**GitHub Issue #7768**: AWS S3 SDK for Java v2 was failing with `Invalid base 16 character: '-'` when performing `PutObject` on large files.
### Root Cause
SeaweedFS internally auto-chunks large files (>8MB) for efficient storage. Previously, when a regular `PutObject` request resulted in multiple internal chunks, SeaweedFS returned a composite ETag format (`<md5>-<count>`) instead of a pure MD5 hash.
go test -v -timeout=$(TEST_TIMEOUT) -run "$(TEST_PATTERN)" .
@echo "✅ Filer group tests completed"
# Run tests with automatic server management
test-with-server:start-server
@echo "Server started successfully, running filer group tests..."
@echo "Test pattern: $(TEST_PATTERN)"
@echo "Test timeout: $(TEST_TIMEOUT)"
@trap "$(MAKE) stop-server" EXIT;\
$(MAKE)test||(echo"❌ Tests failed, showing server logs:"&&echo"=== Last 50 lines of server logs ==="&& tail -50 weed-test.log &&echo"=== End of server logs ==="&&exit 1)
@$(MAKE) stop-server
@echo "✅ Tests completed and server stopped"
# Run tests with auto-managed server via TestMain
test-with-server:check-deps
@echo "Running filer group tests with auto-managed server..."
@if [ -z "$(TEST_PATTERN)"];then\
go test -v -timeout=$(TEST_TIMEOUT) ./...;\
else\
go test -v -timeout=$(TEST_TIMEOUT) -run "$(TEST_PATTERN)" ./...;\
Some files were not shown because too many files have changed in this diff
Show More
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Chris Lu
Chris LuandGitHub
Sheya BernsteinandGitHub
Lisandro PinandGitHub
