fix(outlook): retry LDAP user searches with fresh connections - #4441
Merged
Merged
Conversation
On long on-prem Exchange syncs the admin-user LDAP query can run days after the normal-user search. Reusing a cached ldap3 connection caused Connection reset by peer and aborted the sync. Use a new connection per search, unbind after each attempt, and retry transient failures with the same backoff settings as Office365Users. Co-authored-by: Cursor <cursoragent@cursor.com>
14 tasks
There was a problem hiding this comment.
🟡 Changes recommended
The notice lists an incorrect google-auth version, and cleanup behavior lacks a regression assertion.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds resilient LDAP user enumeration for long-running Outlook Server syncs.
Changes:
- Creates and unbinds a fresh LDAP connection per search attempt.
- Retries transient LDAP failures with exponential backoff.
- Adds retry and connection-lifecycle tests.
File summaries
| File | Description |
|---|---|
connectors/sources/outlook/client.py |
Implements fresh, retryable LDAP searches. |
tests/sources/test_outlook.py |
Tests retries and fresh connections. |
NOTICE.txt |
Updates dependency notices. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Jan-Kazlouski-elastic
added a commit
that referenced
this pull request
Sep 4, 2026
## Summary `test_close_with_client_session` was making real AWS `ListBuckets` HTTP calls with dummy credentials. That caused slow, flaky failures under `--fail-slow=1` when network latency exceeded 1s. The test now mocks the S3 client (same pattern as `test_ping`) and asserts that `close()` tears down the `AsyncExitStack` used for the client session. ## Checklists #### Pre-Review Checklist - [x] this PR does NOT contain credentials of any kind, such as API keys or username/passwords (double check `config.yml.example`) - [x] this PR has a meaningful title - [x] this PR links to all relevant github issues that it fixes or partially addresses - [x] if there is no GH issue, please create it. Each PR should have a link to an issue - [x] this PR has a thorough description - [x] Covered the changes with automated tests - [x] Tested the changes locally - [ ] Added a label for each target release version (example: `v7.13.2`, `v7.14.0`, `v8.0.0`) - [ ] For bugfixes: backport safely to all minor branches still receiving patch releases - [x] Considered corresponding documentation changes - [x] Contributed any configuration settings changes to the configuration reference - [x] if you added or changed Rich Configurable Fields for a Native Connector, you made a corresponding PR in [Kibana](https://github.com/elastic/kibana/blob/main/packages/kbn-search-connectors/types/native_connectors.ts) #### Changes Requiring Extra Attention - [ ] Security-related changes (encryption, TLS, SSRF, etc) - [ ] New external service dependencies added. ## Related Pull Requests * #4441 — Outlook LDAP retry fix (where the flake was first observed) ## Release Note n/a — test-only change. Made with [Cursor](https://cursor.com) --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Wrap LDAP unbind in a defensive try/except so cleanup failures do not mask search errors or block retries, and assert unbind runs on each retry attempt in the transient-error test.
Contributor
Author
|
Thank you @erikcurrin-elastic |
Jan-Kazlouski-elastic
enabled auto-merge (squash)
September 4, 2026 22:09
Contributor
Author
|
Greetings @artem-shelkovnikov Would you please take a look at this PR as it is waiting for a review for a long time? |
|
|
||
| @cached_property | ||
| def _create_connection(self): | ||
| def _create_ldap_connection(self): |
Member
There was a problem hiding this comment.
I'd personally inline this into the method that does LDAP search just in case so that nobody would think of returning back @cached_property and causing trouble
artem-shelkovnikov
approved these changes
Sep 9, 2026
artem-shelkovnikov
left a comment
artem-shelkovnikov
left a comment
Member
There was a problem hiding this comment.
LGTM, left non-blocking comment
This was referenced Sep 9, 2026
Jan-Kazlouski-elastic
added a commit
that referenced
this pull request
Sep 9, 2026
…4441) (#4463) Backports the following commits to 9.4: - fix(outlook): retry LDAP user searches with fresh connections (#4441) Co-authored-by: Jan-Kazlouski-elastic <jan.kazlouski@elastic.co> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Jan-Kazlouski-elastic
added a commit
that referenced
this pull request
Sep 9, 2026
…4441) (#4464) Backports the following commits to 9.5: - fix(outlook): retry LDAP user searches with fresh connections (#4441) Co-authored-by: Jan-Kazlouski-elastic <jan.kazlouski@elastic.co> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
2 tasks
Jan-Kazlouski-elastic
added a commit
that referenced
this pull request
Sep 9, 2026
…4441) (#4465) Backports #4441 to `8.19`. Related to elastic/sdh-search#1898 ## Summary - Replace the cached LDAP connection with a fresh `Connection` per search. - Add `_ldap_search()` with `@retryable` backoff and `skipped_exceptions=[UsersFetchFailed]`. - Unbind after each search attempt (with defensive cleanup). - Refactor `_fetch_normal_users` / `_fetch_admin_users` to call the shared helper. ## Backport notes - The 8.19 branch still uses the monolithic `connectors/sources/outlook.py` layout (rather than the `outlook/` package split into `client.py`/`datasource.py` on `main`), so the changes were manually adapted to that single module. Behaviour is identical to the original PR. - The incidental `NOTICE.txt` version bump from `make install` is intentionally excluded. ## Test plan - [x] `make clean install autoformat lint test` - [x] `pytest tests/sources/test_outlook.py`
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of https://github.com/elastic/sdh-search/issues/1898
Problem
On large on-prem Exchange deployments the Outlook Server connector enumerates users from Active Directory via LDAP before opening each mailbox over EWS.
ExchangeUserskept a single cachedldap3connection for both the normal-user and admin-user searches.get_user_accounts()consumes users lazily: it yields each normal user, the sync processes that mailbox (often for hours), then continues. The admin-user LDAP search can therefore run days after the first bind. AD or intermediate network gear often drops the idle TCP session, and the next search fails with errors such as:Unlike the Office365 Graph path, LDAP had no retry layer. A single transient failure aborted the entire sync. A customer on SDH #1898 hit this after indexing 4.2M+ documents over ~45 hours.
Fix
Connectionper search._ldap_search()with the existing@retryablesettings (RETRIES,RETRY_INTERVAL, exponential backoff) andskipped_exceptions=[UsersFetchFailed]so empty LDAP results are not retried._fetch_normal_users/_fetch_admin_usersto call the shared helper.Checklists
Pre-Review Checklist
config.yml.example)v7.13.2,v7.14.0,v8.0.0)Changes Requiring Extra Attention
Related Pull Requests
mailattributeRelease Note
Outlook Server connector: LDAP user enumeration now opens a fresh Active Directory connection per search and retries transient network failures instead of aborting long syncs when the admin-user query runs on a stale connection.
Made with Cursor