fix: stop op autolaunching a session bus on every call - #1
Merged
Conversation
op links the 99designs/keyring secret-service backend, and go-dbus autolaunches a private `dbus-daemon --fork --session` whenever DBUS_SESSION_BUS_ADDRESS is unset. The daemon user has no login session and so has no bus, so every single op invocation started one. Each daemon calls setsid() and outlives the op process that caused it: twenty list_items calls left exactly twenty behind, and an evening of ordinary use had 99 sitting at about 2.4 MB each. They do stay inside the unit's cgroup, so a restart collects them, but between restarts the unit walks toward TasksMax for nothing. Pointing the variable at an address that cannot connect is the whole fix. go-dbus autolaunches only when it is empty or the literal "autolaunch:"; given anything else it tries that address, fails, and reports no session bus, which is the truth. The keyring falls back to its file backend and nothing is lost, because authentication here is a service-account token read from a file rather than a stored login session. The guard goes on all four env builders, not just the op one. keeper, the notify hook and a caller's own command each build their own child environment and each could invoke something that wants a bus.
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.
The
sandbrokeruser owned 103 processes tonight. Ninety-nine weredbus-daemon --session, one peropinvocation, about 312 MB. Alist_fieldscall hung past 123 seconds and had to be backgrounded, which is how it surfaced.oplinks the99designs/keyringsecret-service backend, and its go-dbus client runsdbus-launch --autolaunchwhenDBUS_SESSION_BUS_ADDRESSis unset. The daemon that starts callssetsid()and outlives theopprocess that caused it. The service user has no login session and therefore no bus, so every single call started one. Caught directly: a/procpoller recordedop item listat 20:43:52.56 and a new sandbroker-owned session bus at 20:43:52.57, and twenty brokered calls against an idle vault took its cgroup from one bus to twenty-one, exactly.The fix sets an inert
DBUS_SESSION_BUS_ADDRESSon every child environment. go-dbus autolaunches only when that variable is empty or the literalautolaunch:; given a real address it attempts a connect, fails with ENOENT, and reports no session bus, which is the truth here.ProtectSystem=strictkeeps the filesystem read-only so nothing can later appear at that path. Nothing is lost by it: authentication is a service-account token read from a file, never a stored login session, so the keyring falling back to its file backend costs nothing. Twenty Dev calls also got faster, 7.1s to 5.9s,dbus-launchhaving cost roughly 60ms each.It is applied to all four child-environment builders rather than only the
opone, since each could invoke something that wants a bus, andDBUS_SESSION_BUS_ADDRESSjoinsRESERVED_ENVso a caller cannot rebind it and undo the fix.One correction to the initial diagnosis, which claimed the daemons escaped the unit's cgroup. They did not. Every orphan was inside the slice and
dbus-daemoncatches SIGTERM; a restart drained 44 processes to 2. That survival could not be reproduced, and the likely explanation is that Production was taking a call every twenty seconds, so the count regrew within seconds. Growth was linear per call and bounded byTasksMax=256, not unbounded, and containment was never the missing piece.Not claimed: this does not explain the 123-second hang. At 99 daemons the unit sat around 121 tasks against a 256 limit, and 60ms per call does not add up to 123 seconds. It may be a separate problem, now easier to see.
Test plan
tests/test_childenv.py, six tests intests/run.sh; the load-bearing one runs twenty commands through the realrunner.run()against a stub applying go-dbus's own rule and asserts the process count is flatwithout_session_buswithreturn envgivesAssertionError: 20 != 0 : 20 call(s) out of 20 left a detached process behind; restored from a file copy, md5 identical across all five sourcesopinvocations across both vaults, steady state four processes and about 95 MBbash tests/run.sh/opt/sandbroker/lib/sandbroker/