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, which is what
the Firewall Mode incident produced.
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 resets DNS the way the restart path's Cleanup task does.
That also leaves the host in a known state - a stopped ctrld holds no WFP or
NRPT enforcement, so a replacement that was blocking traffic stops blocking it
at rollback instead of at the next reboot.
Restoring is now conditional on the previous binary reporting a version. The
incident's _previous file existed but produced no version output; renaming that
over the current binary would have traded 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 return errors rather than calling Fatal, so each one reports
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.
Extract the rollback into rollbackToPreviousBinary() and cover it: 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.
Restore the previously merged macOS VPN DNS post-settle refresh,
unchanged-exemption suppression, forced pf state flush, and self-upgrade
test after they were removed by the firewall-mode merge.
Also keep Windows NRPT pointed at loopback when the listener is configured
on a wildcard address.
The test:windows CI job intermittently failed to clean up .testbin with
"Access to the path '...cmd_cli.test.exe' is denied". This was previously
attributed to Windows Defender scanning the large unsigned test binaries,
and mitigated with Defender exclusions and cleanup retries. That was
treating a symptom.
Root cause: performUpgrade() self-upgrades by running
exec.Command(os.Executable(), "upgrade", "prod", "-vv") as a detached,
windowless child. In the real ctrld binary this re-execs ctrld and is
correct. Under `go test`, os.Executable() is the test binary itself, and
`go test` stops flag parsing at the first positional arg ("upgrade") and
ignores the rest -- so the child silently re-runs the entire test suite.
That child hits the upgrade tests again and spawns more detached children,
recursively: a fork bomb of hidden processes that pins the runner's
CPU/memory and keeps the test binary's image file locked. Windows refuses
to delete the image of a running process, hence the "Access is denied"
during after_script. Whether any children are still alive when cleanup
runs is a timing race, which is why the failure was flaky.
Two tests reached this path: Test_performUpgrade (directly) and
Test_selfUpgradeCheck (via selfUpgradeCheck -> performUpgrade on the
"upgrade allowed" case).
Fix:
- prog.go: extract the command construction into a package-level
newUpgradeCmd var. Production behavior is unchanged.
- main_test.go: stub newUpgradeCmd once in TestMain so the whole test
binary self-execs with `-test.run=^$` (matches no tests, exits
immediately) instead of re-running the suite. This covers every test
that reaches performUpgrade, present and future, while still exercising
the cmd.Start() success path.
Replace github.com/rs/zerolog with go.uber.org/zap throughout the codebase
to improve performance and provide better structured logging capabilities.
Key changes:
- Replace zerolog imports with zap and zapcore
- Implement custom Logger wrapper in log.go to maintain zerolog-like API
- Add LogEvent struct with chained methods (Str, Int, Err, Bool, etc.)
- Update all logging calls to use the new zap-based wrapper
- Replace JSON encoders with Console encoders for better readability
Benefits:
- Better performance with zap's optimized logging
- Consistent structured logging across all components
- Maintained zerolog-like API for easy migration
- Proper field context preservation for debugging
- Multi-core logging architecture for better output control
All tests pass and build succeeds.
So setting up logging for ctrld binary and ctrld packages could be done
more easily, decouple the required setup for interactive vs daemon
running.
This is the first step toward replacing rs/zerolog libary with a
different logging library.
Dev Scribe