2c8a1ea6ccd6bfaeba2882b6078fcc78d29a0680
7 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Chris Lu
|
ced2236cc6 |
Adjust rename events metadata format (#8854)
* rename metadata events * fix subscription filter to use NewEntry.Name for rename path matching The server-side subscription filter constructed the new path using OldEntry.Name instead of NewEntry.Name when checking if a rename event's destination matches the subscriber's path prefix. This could cause events to be incorrectly filtered when a rename changes the file name. * fix bucket events to handle rename of bucket directories onBucketEvents only checked IsCreate and IsDelete. A bucket directory rename via AtomicRenameEntry now emits a single rename event (both OldEntry and NewEntry non-nil), which matched neither check. Handle IsRename by deleting the old bucket and creating the new one. * fix replicator to handle rename events across directory boundaries Two issues fixed: 1. The replicator filtered events by checking if the key (old path) was under the source directory. Rename events now use the old path as key, so renames from outside into the watched directory were silently dropped. Now both old and new paths are checked, and cross-boundary renames are converted to create or delete. 2. NewParentPath was passed to the sink without remapping to the sink's target directory structure, causing the sink to write entries at the wrong location. Now NewParentPath is remapped alongside the key. * fix filer sync to handle rename events crossing directory boundaries The early directory-prefix filter only checked resp.Directory (old parent). Rename events now carry the old parent as Directory, so renames from outside the source path into it were dropped before reaching the existing cross-boundary handling logic. Check both old and new directories against sourcePath and excludePaths so the downstream old-key/new-key logic can properly convert these to create or delete operations. * fix metadata event path matching * fix metadata event consumers for rename targets * Fix replication rename target keys Logical rename events now reach replication sinks with distinct source and target paths.\n\nHandle non-filer sinks as delete-plus-create on the translated target key, and make the rename fallback path create at the translated target key too.\n\nAdd focused tests covering non-filer renames, filer rename updates, and the fallback path.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix filer sync rename path scoping Use directory-boundary matching instead of raw prefix checks when classifying source and target paths during filer sync.\n\nAlso apply excludePaths per side so renames across excluded boundaries downgrade cleanly to create/delete instead of being misclassified as in-scope updates.\n\nAdd focused tests for boundary matching and rename classification.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix replicator directory boundary checks Use directory-boundary matching instead of raw prefix checks when deciding whether a source or target path is inside the watched tree or an excluded subtree.\n\nThis prevents sibling paths such as /foo and /foobar from being misclassified during rename handling, and preserves the earlier rename-target-key fix.\n\nAdd focused tests for boundary matching and rename classification across sibling/excluded directories.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix etc-remote rename-out handling Use boundary-safe source/target directory membership when classifying metadata events under DirectoryEtcRemote.\n\nThis prevents rename-out events from being processed as config updates, while still treating them as removals where appropriate for the remote sync and remote gateway command paths.\n\nAdd focused tests for update/removal classification and sibling-prefix handling.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Defer rename events until commit Queue logical rename metadata events during atomic and streaming renames and publish them only after the transaction commits successfully.\n\nThis prevents subscribers from seeing delete or logical rename events for operations that later fail during delete or commit.\n\nAlso serialize notification.Queue swaps in rename tests and add failure-path coverage.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Skip descendant rename target lookups Avoid redundant target lookups during recursive directory renames once the destination subtree is known absent.\n\nThe recursive move path now inserts known-absent descendants directly, and the test harness exercises prefixed directory listing so the optimization is covered by a directory rename regression test.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Tighten rename review tests Return filer_pb.ErrNotFound from the bucket tracking store test stub so it follows the FilerStore contract, and add a webhook filter case for same-name renames across parent directories.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix HardLinkId format verb in InsertEntryKnownAbsent error HardLinkId is a byte slice. %d prints each byte as a decimal number which is not useful for an identifier. Use %x to match the log line two lines above. * only skip descendant target lookup when source and dest use same store moveFolderSubEntries unconditionally passed skipTargetLookup=true for every descendant. This is safe when all paths resolve to the same underlying store, but with path-specific store configuration a child's destination may map to a different backend that already holds an entry at that path. Use FilerStoreWrapper.SameActualStore to check per-child and fall back to the full CreateEntry path when stores differ. * add nil and create edge-case tests for metadata event scope helpers * extract pathIsEqualOrUnder into util.IsEqualOrUnder Identical implementations existed in both replication/replicator.go and command/filer_sync.go. Move to util.IsEqualOrUnder (alongside the existing FullPath.IsUnder) and remove the duplicates. * use MetadataEventTargetDirectory for new-side directory in filer sync The new-side directory checks and sourceNewKey computation used message.NewParentPath directly. If NewParentPath were empty (legacy events, older filer versions during rolling upgrades), sourceNewKey would be wrong (/filename instead of /dir/filename) and the UpdateEntry parent path rewrite would panic on slice bounds. Derive targetDir once from MetadataEventTargetDirectory, which falls back to resp.Directory when NewParentPath is empty, and use it consistently for all new-side checks and the sink parent path. |
||
|
Chris Lu
|
086ab3e28c |
Fix webhook duplicate deliveries and POST to GET conversion (#7668)
* Fix webhook duplicate deliveries and POST to GET conversion Fixes #7667 This commit addresses two critical issues with the webhook notification system: 1. Duplicate webhook deliveries based on worker count 2. POST requests being converted to GET when following redirects Issue 1: Multiple webhook deliveries ------------------------------------ Problem: The webhook queue was creating multiple handlers (one per worker) that all subscribed to the same topic. With Watermill's gochannel, each handler creates a separate subscription, and all subscriptions receive their own copy of every message, resulting in duplicate webhook calls equal to the worker count. Solution: Use a single handler instead of multiple handlers to ensure each webhook event is sent only once, regardless of worker configuration. Issue 2: POST to GET conversion with intelligent redirect handling ------------------------------------------------------------------ Problem: When webhook endpoints returned redirects (301/302/303), Go's default HTTP client would automatically follow them and convert POST requests to GET requests per HTTP specification. Solution: Implement intelligent redirect handling that: - Prevents automatic redirects to preserve POST method - Manually follows redirects by recreating POST requests - Caches the final redirect destination for performance - Invalidates cache and retries on failures (network or HTTP errors) - Provides automatic recovery from cached endpoint failures Benefits: - Webhooks are now sent exactly once per event - POST method is always preserved through redirects - Reduced latency through redirect destination caching - Automatic failover when cached destinations become unavailable - Thread-safe concurrent webhook delivery Testing: - Added TestQueueNoDuplicateWebhooks to verify single delivery - Added TestHttpClientFollowsRedirectAsPost for redirect handling - Added TestHttpClientUsesCachedRedirect for caching behavior - Added cache invalidation tests for error scenarios - All 18 webhook tests pass successfully * Address code review comments - Add maxWebhookRetryDepth constant to avoid magic number - Extract cache invalidation logic into invalidateCache() helper method - Fix redirect handling to properly follow redirects even on retry attempts - Remove misleading comment about nWorkers controlling handler parallelism - Fix test assertions to match actual execution flow - Remove trailing whitespace in test file All tests passing. * Refactor: use setFinalURL() instead of invalidateCache() Replace invalidateCache() with more explicit setFinalURL() function. This is cleaner as it makes the intent clear - we're setting the URL (either to a value or to empty string to clear it), rather than having a separate function just for clearing. No functional changes, all tests passing. * Add concurrent webhook delivery using nWorkers configuration Webhooks were previously sent sequentially (one-by-one), which could be a performance bottleneck for high-throughput scenarios. Now nWorkers configuration is properly used to control concurrent webhook delivery. Implementation: - Added semaphore channel (buffered to nWorkers capacity) - handleWebhook acquires semaphore slot before sending (blocks if at capacity) - Releases slot after webhook completes - Allows up to nWorkers concurrent webhook HTTP requests Benefits: - Improved throughput for slow webhook endpoints - nWorkers config now has actual purpose (was validated but unused) - Default 5 workers provides good balance - Configurable from 1-100 workers based on needs Example performance improvement: - Before: 500ms webhook latency = ~2 webhooks/sec max - After (5 workers): 500ms latency = ~10 webhooks/sec - After (10 workers): 500ms latency = ~20 webhooks/sec All tests passing. * Replace deprecated AddNoPublisherHandler with AddConsumerHandler AddNoPublisherHandler is deprecated in Watermill. Use AddConsumerHandler instead, which is the current recommended API for handlers that only consume messages without publishing. No functional changes, all tests passing. * Drain response bodies to enable HTTP connection reuse Added drainBody() calls in all code paths to ensure response bodies are consumed before returning. This is critical for HTTP keep-alive connection reuse. Without draining: - Connections are closed after each request - New TCP handshake + TLS handshake for every webhook - Higher latency and resource usage With draining: - Connections are reused via HTTP keep-alive - Significant performance improvement for repeated webhooks - Lower latency (no handshake overhead) - Reduced resource usage Implementation: - Added drainBody() helper that reads up to 1MB (prevents memory issues) - Drain on success path (line 161) - Drain on error responses before retry (lines 119, 152) - Drain on redirect responses before following (line 118) - Already had drainResponse() for network errors (line 99) All tests passing. * Use existing CloseResponse utility instead of custom drainBody Replaced custom drainBody() function with the existing util_http.CloseResponse() utility which is already used throughout the codebase. This provides: - Consistent behavior with rest of the codebase - Better logging (logs bytes drained via CountingReader) - Full body drainage (not limited to 1MB) - Cleaner code (no duplication) CloseResponse properly drains and closes the response body to enable HTTP keep-alive connection reuse. All tests passing. * Fix: Don't overwrite original error when draining response Before: err was being overwritten by drainResponse() result After: Use drainErr to avoid losing the original client.Do() error This was a subtle bug where if drainResponse() succeeded (returned nil), we would lose the original network error and potentially return a confusing error message. All tests passing. * Optimize HTTP client: reuse client and remove redundant timeout 1. Reuse single http.Client instance instead of creating new one per request - Reduces allocation overhead - More efficient for high-volume webhooks 2. Remove redundant timeout configuration - Before: timeout set on both context AND http.Client - After: timeout only on context (cleaner, context fires first anyway) Performance benefits: - Reduced GC pressure (fewer client allocations) - Better connection pooling (single transport instance) - Cleaner code (no redundancy) All tests passing. |
||
|
Ibrahim Konsowa
|
315fcc70b2 | fix: dead letter message log message (#7072) | ||
|
Chris Lu
|
69553e5ba6 | convert error fromating to %w everywhere (#6995) | ||
|
Chris Lu
|
a524b4f485 |
Object locking need to persist the tags and set the headers (#6994)
* fix object locking read and write No logic to include object lock metadata in HEAD/GET response headers No logic to extract object lock metadata from PUT request headers * add tests for object locking * Update weed/s3api/s3api_object_handlers_put.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update weed/s3api/s3api_object_handlers.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor * add unit tests * sync versions * Update s3_worm_integration_test.go * fix legal hold values * lint * fix tests * racing condition when enable versioning * fix tests * validate put object lock header * allow check lock permissions for PUT * default to OFF legal hold * only set object lock headers for objects that are actually from object lock-enabled buckets fix --- FAIL: TestAddObjectLockHeadersToResponse/Handle_entry_with_no_object_lock_metadata (0.00s) * address comments * fix tests * purge * fix * refactoring * address comment * address comment * Update weed/s3api/s3api_object_handlers_put.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update weed/s3api/s3api_object_handlers_put.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update weed/s3api/s3api_object_handlers.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * avoid nil * ensure locked objects cannot be overwritten --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> |
||
|
Ibrahim Konsowa
|
d78aa3d2de |
[Notifications] Improving webhook notifications (#6965)
* worker setup * fix tests * start worker * graceful worker drain * retry queue * migrate queue to watermill * adding filters and improvements * add the event type to the webhook message * eliminating redundant JSON serialization * resolve review comments * trigger actions * fix tests * typo fixes * read max_backoff_seconds from config * add more context to the dead letter * close the http response on errors * drain the http response body in case not empty * eliminate exported typesπ |
||
|
Ibrahim Konsowa
|
93bbaa1fb4 |
[Notifications] Support webhook notifications (#6962)
Add webhook notification support |