Repository navigation
[client] Fixes DisableAutoConnect semantics - #6409
riccardomanfrin wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR removes the ChangesConnection Lifecycle Improvements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
DisableAutoConnect semantics
============================
Scope: governs ONLY the service-Start auto-connect decision. Once the
connection goroutine has been spawned (by any path), the flag is never
consulted again — the retry loop keeps trying to connect until ctx is
cancelled by Down / Stop / Logout.
Cases
-----
1. Service Start + DisableAutoConnect = true
- No connection goroutine spawned.
- clientRunning stays false.
- State set to StatusIdle.
- Daemon stays passive until an explicit Up RPC.
2. Service Start + DisableAutoConnect = false
- Spawn connectWithRetryRuns.
- clientRunning = true.
- Retry loop runs until ctx cancelled.
3. Up RPC (any value of DisableAutoConnect)
- Flag ignored. The user / admin explicitly asked to connect — by
definition not "auto".
- Spawn connectWithRetryRuns.
- clientRunning = true.
4. MDM-triggered restart (any value of DisableAutoConnect)
- Flag ignored. An MDM policy change applies new config to an
already-running engine; treated as an implicit Up.
- Spawn connectWithRetryRuns.
- clientRunning = true.
5. Down / Stop / Logout
- Cancels ctx → connectWithRetryRuns exits → close(giveUpChan).
- cleanupConnection clears clientRunning = false.
- DisableAutoConnect not involved.
Prepare for next step
Collapse error log into s.connect and rename it to more explicit connectOnce
f60d315 to
c2c23f6
Compare
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/server/server.go (1)
1657-1659: 💤 Low valueDuplicate logging with runOperation.
When
client.Run()fails, this logs "run client connection exited with error: %v", then the error propagates torunOperation(line 253) which logs the same message with ". Will retry in the background" appended. Consider removing this log since the caller already logs with more context, or differentiate the messages if both are needed for distinct debugging purposes.🤖 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 `@client/server/server.go` around lines 1657 - 1659, The log.Debugf in the error branch of client.Run is duplicating a higher-level message in runOperation; remove the duplicate logging inside the Run caller (the log.Debugf("run client connection exited with error: %v", err) in the block where client.Run(runningChan, s.logFile) returns an error) and simply return the error so runOperation (which logs with ". Will retry in the background") is the single source of that message; alternatively, if a lower-level log is still desired, change it to a lower-severity/trace message or add distinct context (e.g., include s.name or connection id) instead of mirroring runOperation.
🤖 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.
Nitpick comments:
In `@client/server/server.go`:
- Around line 1657-1659: The log.Debugf in the error branch of client.Run is
duplicating a higher-level message in runOperation; remove the duplicate logging
inside the Run caller (the log.Debugf("run client connection exited with error:
%v", err) in the block where client.Run(runningChan, s.logFile) returns an
error) and simply return the error so runOperation (which logs with ". Will
retry in the background") is the single source of that message; alternatively,
if a lower-level log is still desired, change it to a lower-severity/trace
message or add distinct context (e.g., include s.name or connection id) instead
of mirroring runOperation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0797369e-594a-4333-9043-93975dadf89d
📒 Files selected for processing (1)
client/server/server.go
riccardomanfrin
changed the title
|
mlsmaycon
left a comment
mlsmaycon
left a comment
There was a problem hiding this comment.
we can probably leave it as is with more docs. But I don't see any harm in keeping it here. If we keep it, let's add some docs for it.
2 participants
Describe your changes
Clarify the semantics of the
DisableAutoConnectflag in the clientdaemon. The flag previously short-circuited every code path that
considered spawning the connection goroutine — service Start, the Up
RPC handler, and future coming MDM-driven engine restart.
The flag's intent is narrower: it governs whether the daemon should
automatically connect at service start. An explicit user request,
or a policy refresh applied to an already-running engine, should not be
subordinated to the flag.
New semantics:
DisableAutoConnect=true→ daemon staysidle, no connection goroutine spawned,
clientRunningstaysfalse. The state RPC reportsStatusIdle.DisableAutoConnect=false→ daemon spawnsconnectWithRetryRuns, retry loop runs until ctx cancelled.DisableAutoConnect) → flag ignored.The user explicitly asked to connect — by definition not "auto".
DisableAutoConnect) →flag ignored. An MDM policy change applies new config to an
already-running engine; treated as an implicit Up.
cleanupConnectionclears
clientRunning.DisableAutoConnectis not involved.Code changes:
client/server/server.go: removed theDisableAutoConnectbranchinside
connectWithRetryRunsso the retry loop no longercollapses to a single-shot when the flag is set. The flag is now
only consulted at the top of
Start()(the early-return path thatsets
StatusIdle).Issue ticket number and link
No public issue. Semantic divergence between the flag name and its
behaviour surfaced during the MDM integration review
(#6374) —
DisableAutoConnectwas being read as a runtime kill switch in addition to a startup
gate. This PR confines it to its documented purpose so the MDM PR
does not have to choose between two reasonable but incompatible
interpretations.
Stack
Checklist
Documentation
Select exactly one:
The
DisableAutoConnectflag is already documented as astartup-only auto-connect gate (CLI help text and ConfigInput
field comment). The previous implementation diverged from the
documented intent; this PR aligns the implementation with the
existing docs, so no doc update is required. The behavioural
contract becomes: setting the flag prevents the boot-time
connection only — explicit Up requests and MDM-driven restarts
remain unaffected.
Docs PR URL (required if "docs added" is checked)
N/A
Summary by CodeRabbit