Repository navigation
Release v1.5.6 - #330
Merged
Merged
Release v1.5.6#330
Conversation
The post-stabilization reconcile verifies rule text, which cannot tell a live redirect from an anchor pf has stopped evaluating. Sleep/wake QA caught exactly that split: references intact, anchor rules intact, post-load verification passed, and every query through the system resolver timing out while the direct listener answered. Nothing else probed. The interception probe monitor stands down while stabilization owns pf and is never re-armed afterwards, so functional recovery waited for the periodic watchdog - 11 seconds in the captured run, up to a full 30-second interval - on a host whose link and default route were already back. The watchdog's probe then failed once, forced a reload, and public and VPN split-DNS both recovered immediately. Probe once at the end of stabilization and, if it fails, force exactly one reload and confirm with one more probe. Not the probe monitor: that keeps probing for ~7.5s and can force a reload per failed probe, where this path needs a single bounded repair before handing back to the watchdog. Skipped when a monitor already owns probing, when intercept state is gone, or during exec backoff. Hand ownership over deterministically rather than skipping on sight. A probe monitor started by an ignored network change claimed functional-probe ownership before checking whether it could work, then stood down because stabilization still owned pf; the verifier read that claimed flag as "somebody is probing" and skipped, so neither path probed and recovery fell back to the watchdog anyway. The monitor now checks eligibility before claiming, and the verifier waits out a holder that releases, yielding only to one that keeps probing. Extract the completion block into finishPFStabilization so the wiring is testable, and cover the bounded repair, the healthy path that must not reload, a prober that claims and stands down, a prober that keeps working, and a monitor that must not claim ownership while stabilizing.
Fixes unauthenticated metrics exposure be defaulting to 127.0.0.1 when no host is provided. Logs a warning when bound to non-loopback addresses.
processCDFlags retries the resolver-config fetch indefinitely by design: a device that has no working network at boot must eventually come up. The loop had no cancellation, so a stop request arriving while the API is unreachable was ignored - the process kept retrying long after the service reported itself stopped, doing work on behalf of a service the OS considers stopped. Thread a context through processCDFlags and derive it from p.stopCh, in both the startup preflight and the config-reload path. The loop now returns as soon as the context is cancelled, checked both before a retry and after backoff returns (backoff can wake up on cancellation). A stop during preflight exits the way a normal stop does, without Fatal, so the service manager does not treat it as a failed start and apply its restart policy to a service the operator just asked to stop. Bind the two API requests themselves as well, so a stop does not have to wait out an in-flight request. Without this the loop honours a stop only between attempts, which leaves up to defaultTimeout (20s) of a request the service is no longer interested in - the same "still working after Service stopped" the loop change exists to end, one layer down. Doing so means a context parameter on FetchResolverConfig, FetchResolverUID, UpdateCustomLastFailed and SendLogs, since all four reach a request builder. The callers that have no context pass context.Background(), which is what master effectively does at those sites: its loggerCtx carries a logger, not cancellation. doWithFallback needs no parameter, because it clones the request with req.Context() and so inherits the binding. This also repairs internal/controld/controld_test.go, which is behind //go:build controld and had already been written against the context-taking signature, so it could not compile. Cover the cancellation paths; removing either check makes the tests hang until timeout. Sampling the stop state is the whole point of runAPIPreflight rather than doing this inline. A stop and a failure need opposite handling - one exits quietly, the other self-uninstalls a deleted device, surfaces the error to a mobile app, and reports a failed start - so the two must not be confused. Reading it from the context after cancelling would report "stopped" for every failure, since CancelFunc sets ctx.Err() regardless of whether anyone asked to stop; the stop channel is read directly instead, which also does not depend on the context watcher goroutine having been scheduled.
Both API requests and binary downloads retry against a hard-coded IP when the attempt via hostname fails. Both then overwrote the first error with the fallback's, so only the last failure was reported. That discarded the diagnosis. When the hostname attempt is denied locally - WSAEACCES on Windows, "An attempt was made to access a socket in a way forbidden by its access permissions", which means the host is blocking ctrld - and the direct-ip fallback fails with an unreachable IPv6 route, what surfaces to the operator is "dial tcp6: no route to host": a routing problem that does not exist, while the error naming the real cause is visible only in debug logs. Report both failures instead, keeping the error chain intact so errors.Is still matches either one. Also switch the final wrap in doWithRetry from %v to %w, which had been flattening the chain even when a single error was reported. This also changes retry classification, which is worth stating explicitly because it is not obvious from "report both errors". processCDFlags decides whether to keep backing off with errUrlNetworkError, which uses errors.As - and errors.As returns the *first* match in the tree. Wrapping the hostname attempt first therefore hands the predicate that attempt's failure, where previously only the fallback's error survived to be classified. The effect is intended. A locally denied socket (WSAEACCES) is not a transient network error, so preflight now fails fast and reports instead of retrying against a firewall that is not going to clear on its own. The case that justifies retrying forever, a network unreachable on both attempts at boot, is unchanged. Both classifications are pinned by tests, along with the wrap order they depend on at each composition site, so reversing it fails loudly rather than silently restoring the old behaviour.
Rollback ran os.Remove(bin) while the replacement service was still running from that image. Windows locks a running executable, so the remove failed with "Access is denied" - and it was fatal, so the os.Rename that restores the previous binary never ran. The upgrade ended with the broken replacement still installed and the working binary stranded at its _previous name. Readiness failing is not evidence the process exited: the service manager can report a started service whose process never became operational. So rollback now stops the service and waits until the manager reports it stopped before touching the executable, then cleans up DNS the way the restart path's Cleanup task does. Restoring is now conditional on the previous binary reporting a version, since a _previous file that exists but produces no version output would trade a service that starts and hangs for one that cannot start at all. When it is unusable, rollback keeps it for inspection, leaves the installed binary alone, and says so instead of pressing on. The --version probe is bounded by a timeout so a binary that hangs cannot hang the upgrade. Remaining failures are reported rather than fatal, so each one says what state the host was left in. os.Remove is retried while the path stays locked, since Windows releases an image lock asynchronously after the process exits. The helpers live in a new file rather than in commands.go, and the rollback is extracted into rollbackToPreviousBinary() so it can be covered: the stop happens while the executable is still present, an unusable previous binary is kept without swapping or restarting, and a failed stop aborts before anything is modified. Reversing the stop and the remove fails these tests. The version probe is called through a variable so those tests do not have to stage a runnable executable. Staging one is not portable: oldBin is bin+"_previous", so a fixture named "ctrld" yields the extension-less "ctrld_previous", which Windows refuses to execute, and a symlink to the test binary needs a privilege Windows does not grant by default. The probe itself is still covered against the real test binary. Production is unaffected: ctrld.exe _previous does have an extension, and os/exec only appends PATHEXT entries when a path has none at all - noted at binaryVersion so the suffix is not renamed into something extension-less by accident.
… thinks "ctrld status" reported the service manager's view and nothing else, so it printed "Service is running" and exited 0 for a process that was alive and registered as started but had never got past startup: no control socket, no DNS listener, no policy applied. The one command an operator reaches for first confirmed the service was fine while the host had no working DNS. Probe the control server's /started endpoint before reporting success. That endpoint only answers once the onStarted hooks have completed, which is after the listeners are up, so a successful probe means the process is serving rather than merely alive. A service that is registered as running but cannot confirm startup is now reported as such, with a pointer to the log, and exits 3 - distinct from stopped (1) and unknown (2), because it needs a different response. A probe blocked by permissions is not evidence of a broken service: an unprivileged caller still gets "Service is running", with a note that startup was not verified. The probe is bounded by a short timeout so status stays fast. Document the exit codes in the command's help, and cover the probe (ready, not finished starting, no socket, timed out) and the classification, including that an unreadable socket is not reported as a failure. The not-ready verdict is only reported when the probe could have found the daemon's socket. socketDir() is caller-relative on unix - the system directory when writable, the caller's home otherwise - so an unprivileged "ctrld status" looks somewhere the root-owned daemon never listened and gets ENOENT, which is "wrong path", not "not ready". Since only darwin has an elevation PreRun and the root-level alias has none, that is the normal invocation; reporting exit 3 there would have told a monitoring check to restart healthy daemons. Such a caller now gets the service manager's view with startup reported as unverified. Windows and mobile resolve the same directory for every caller, so the verdict stays fully available on the platform the hung start was seen on. A successful probe is still conclusive whoever ran it.
The warning reported err, the resolver-config fetch error, which is nil on every path that reaches it - so a rejected custom config was logged with no reason attached. cfgErr holds the validation failure.
For fixing nclient4 panic. See: insomniacslk/dhcp#583
cuonglm
force-pushed
the
release-branch-v1.5.6
branch
2 times, most recently
from
August 21, 2026 08:24
97cc8bb to
9989a39
Compare
While at it, also removing the unmatched //lint line.
cuonglm
force-pushed
the
release-branch-v1.5.6
branch
from
August 21, 2026 08:39
9989a39 to
30acb84
Compare
tryUpdateListenerConfig treated an explicit --intercept-mode off the same as an empty flag and fell back to the persisted config value. run() selects the listener strategy before it clears the persisted mode, so the first start after a revert to standard mode selected the intercept strategy from a stale dns/hard value while setDNS kept interception off. Extract the resolution into listenerInterceptMode and make an explicit off final, the same contract as setDNS. Add a regression test that fails without the fix.
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
2 participants
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.
Minor Release
This contains new features, security hardening and bug fixes.
Security
Added
ctrld status. The command now confirms that ctrld is actually serving rather than only registered as running, and reports a distinct exit code for a service that never finished startingChanged
--intercept-mode offactually turn interception off, instead of leaving a previously persisted mode in the config for the next service start to pick upFixed
upstream.oscan reach a VPN-only resolver under a full tunnel instead of failing withno route to hostinsomniacslk/dhcpto c76316d to fix annclient4panic (nclient4: reject frames whose IPv4 payload cannot hold a UDP header insomniacslk/dhcp#583)