Skip to content

feat: add a reconnect to the initial ldap connection#928

Open
steveiliop56 wants to merge 2 commits into
mainfrom
feat/ldap-reconnect
Open

feat: add a reconnect to the initial ldap connection#928
steveiliop56 wants to merge 2 commits into
mainfrom
feat/ldap-reconnect

Conversation

@steveiliop56
Copy link
Copy Markdown
Member

@steveiliop56 steveiliop56 commented Jun 8, 2026

Summary by CodeRabbit

  • Bug Fixes
    • Improved LDAP service connection resilience with automatic reconnection retry logic on initial connection failures and heartbeat detection failures.
    • LDAP service now honors the application context for connection and retry behavior, ensuring more reliable, timely reconnections.

@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jun 8, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 8, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 450e8bac-70a1-46d6-85bb-0d3d51e0e79f

📥 Commits

Reviewing files that changed from the base of the PR and between ab6a2b4 and 9f73f79.

📒 Files selected for processing (2)
  • internal/bootstrap/service_bootstrap.go
  • internal/service/ldap_service.go

📝 Walkthrough

Walkthrough

LdapService now carries an injected context and NewLdapService requires ctx. Initial connect failure triggers reconnect(10s); heartbeat failures call reconnect(5s). reconnect(interval) uses ldap.ctx, sets backoff.InitialInterval from interval, and only closes non-nil connections before reconnecting.

Changes

LDAP reconnect & context wiring

Layer / File(s) Summary
LdapService ctx field and constructor wiring
internal/service/ldap_service.go, internal/bootstrap/service_bootstrap.go
Adds ctx context.Context to LdapService, updates NewLdapService to accept and store ctx, and updates service bootstrap to pass app.ctx to the constructor.
Startup reconnect call
internal/service/ldap_service.go
When initial ldap.connect() fails in NewLdapService, code now calls reconnect(10 * time.Second) and uses any reconnect error for startup failure reporting.
Heartbeat-triggered reconnect
internal/service/ldap_service.go
Heartbeat loop now calls reconnect(5 * time.Second) on heartbeat failure.
Reconnect implementation refactor
internal/service/ldap_service.go
reconnect(interval time.Duration) sets backoff.InitialInterval from interval, retries with ldap.ctx (max 3 tries), and only closes ldap.conn when non-nil before calling ldap.connect().

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I tunneled through code with a curious squeak,
Injected a ctx so reconnects won't leak,
Ten seconds to try when the start goes amiss,
Five seconds to nudge when the heartbeat goes bliss,
Hoppity hops — steady connections we seek.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a reconnect mechanism to the initial LDAP connection when it fails.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/ldap-reconnect

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 8, 2026

Codecov Report

❌ Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/service/ldap_service.go 0.00% 8 Missing ⚠️
internal/bootstrap/service_bootstrap.go 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/service/ldap_service.go`:
- Around line 66-70: The startup blocking retry in ldap_service.go (the call to
ldap.reconnect(...) inside the error branch after connection attempt) causes
~47.5s delay; either remove this startup reconnect entirely so setup continues
on LDAP failure (relying on existing heartbeat reconnect logic), or drastically
shorten it (e.g., call ldap.reconnect with a single short interval / 1–2 second
attempts or implement max 1-2 retries) and update the comment to reflect the
actual delay; locate the error handling around ldap.reconnect and adjust the
call and/or comment accordingly so BootstrapApp.setupServices will not block for
~47.5s when LDAP is unavailable.
- Around line 67-69: The logic in NewLdapService incorrectly returns nil even
when ldap.reconnect succeeds: adjust the error handling in the initial connect
branch (where err != nil) to call ldap.reconnect(10 * time.Second) and check its
return error; if reconnect returns nil then return the ldap pointer and nil
error, otherwise return nil and the wrapped reconnect error (fmt.Errorf("failed
to connect to ldap server: %w", err)). Ensure you reference the ldap variable
created in NewLdapService and only return nil when reconnect actually fails.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 90ca0a54-2e80-4d2f-8a87-9283a697d90d

📥 Commits

Reviewing files that changed from the base of the PR and between 426eac2 and ab6a2b4.

📒 Files selected for processing (1)
  • internal/service/ldap_service.go

Comment on lines +66 to 70
// Warn: This will hang the tinyauth startup for a good 45 seconds until it fails
if err != nil {
err = ldap.reconnect(10 * time.Second)
return nil, fmt.Errorf("failed to connect to ldap server: %w", err)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Startup now blocks for ~47.5 seconds when LDAP is unavailable.

The startup reconnect logic introduces a significant delay. With a 10-second initial interval, 1.5× multiplier, and 3 max tries, the total wait is approximately 47.5 seconds before failing. This blocks application startup whenever LDAP is unreachable.

Impact: Based on the upstream caller (BootstrapApp.setupServices), the application is designed to tolerate LDAP initialization failure and continue without it. The new blocking retry degrades startup time from immediate to ~47.5 seconds in environments where LDAP is down or misconfigured. This affects deployment, development, and testing workflows.

Suggestions:

  • Consider removing the startup reconnect entirely and rely only on the heartbeat reconnect (which already handles connection recovery after startup).
  • If startup reconnect is essential, use a much shorter total timeout (e.g., 1-2 tries with 2-3 second intervals) to fail faster.
  • Update or remove the "45 seconds" comment to reflect the actual ~47.5-second delay.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/ldap_service.go` around lines 66 - 70, The startup blocking
retry in ldap_service.go (the call to ldap.reconnect(...) inside the error
branch after connection attempt) causes ~47.5s delay; either remove this startup
reconnect entirely so setup continues on LDAP failure (relying on existing
heartbeat reconnect logic), or drastically shorten it (e.g., call ldap.reconnect
with a single short interval / 1–2 second attempts or implement max 1-2 retries)
and update the comment to reflect the actual delay; locate the error handling
around ldap.reconnect and adjust the call and/or comment accordingly so
BootstrapApp.setupServices will not block for ~47.5s when LDAP is unavailable.

Comment on lines 67 to 69
if err != nil {
err = ldap.reconnect(10 * time.Second)
return nil, fmt.Errorf("failed to connect to ldap server: %w", err)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Critical logic error: successful reconnect still returns nil service.

If the initial connection fails but reconnect succeeds, this code still returns a nil service pointer and an error. Line 69 executes unconditionally when the initial connect fails, regardless of whether the reconnect succeeded.

Impact: When reconnect succeeds, the LDAP connection is established (ldap.conn is valid and ldap pointer exists), but NewLdapService returns nil to the caller. Downstream code (like AuthService.GetLDAPUser) will report "ldap service not configured" even though the connection is active.

🐛 Proposed fix
 if err != nil {
 	err = ldap.reconnect(10 * time.Second)
-	return nil, fmt.Errorf("failed to connect to ldap server: %w", err)
+	if err != nil {
+		return nil, fmt.Errorf("failed to connect to ldap server: %w", err)
+	}
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if err != nil {
err = ldap.reconnect(10 * time.Second)
return nil, fmt.Errorf("failed to connect to ldap server: %w", err)
if err != nil {
err = ldap.reconnect(10 * time.Second)
if err != nil {
return nil, fmt.Errorf("failed to connect to ldap server: %w", err)
}
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/ldap_service.go` around lines 67 - 69, The logic in
NewLdapService incorrectly returns nil even when ldap.reconnect succeeds: adjust
the error handling in the initial connect branch (where err != nil) to call
ldap.reconnect(10 * time.Second) and check its return error; if reconnect
returns nil then return the ldap pointer and nil error, otherwise return nil and
the wrapped reconnect error (fmt.Errorf("failed to connect to ldap server: %w",
err)). Ensure you reference the ldap variable created in NewLdapService and only
return nil when reconnect actually fails.

@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant