Repository navigation
fix(linux): repair disabled session bus in daemon and PTY environments - #21120
oliver-mee wants to merge 4 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between a8d0c428545c6ced8857d3328f559855674721cd and d7db8d3. 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughA new helper repairs Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The reviewed changes are mergeable after normal checks; no concrete session-bus regression remains identified. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Linux daemon processes and terminals can now receive a working user-bus address where they previously received a disabled marker. That repairs the reported failure, but it may conflict with deployments that deliberately disable bus use for certain processes. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline.
Reviewed changes
- Session-bus repair helper — new Linux-only
repairDisabledSessionBusEnvrewrites an exactDBUS_SESSION_BUS_ADDRESS=disabled:tounix:path=/run/user/<uid>/bus, but only when that socket actually exists. - Daemon fork env — the inline fork env became
daemonForkEnv(), which repairs the environment inherited by the forked daemon so the marker never originates there. - Daemon PTY env —
createDaemonPtyEnvironmentrepairs the merged env too, which also catches adisabled:value arriving throughopts.env. - Tests — six cases: repair, missing socket, real address, absent var, non-Linux, and a live no-throw smoke test.
I verified the load-bearing external claims: Chromium really does setenv("DBUS_SESSION_BUS_ADDRESS", "disabled:", 0) when the var is unset at launch and it does surface in the Electron main process, and unix:path=/run/user/<uid>/bus matches the address systemd's own dbus.socket publishes via %t ($XDG_RUNTIME_DIR). Nice, tightly scoped fix.
ℹ️ Local PTY fallback still propagates the marker
The repair covers the daemon lane, but buildLocalPtySpawnEnvironment builds its env from the Electron process's process.env with no equivalent repair, so the marker still reaches shells whenever the daemon lane degrades to LocalPtyProvider (degraded-new-pty-fallback, or onDaemonError in main-process-pty-startup.ts). The reported headless-serve case is fixed while the daemon runs, but the PR's "every spawned PTY shell" claim does not hold on that fallback lane.
Technical details
# Local PTY spawn env is not repaired
## Affected sites
- `src/main/providers/local-pty-spawn-environment.ts:20` — spreads `stripInheritedBuildModeEnv(process.env)` with no `repairDisabledSessionBusEnv` call.
- `src/main/ipc/pty/host-env/assembly.ts:37` (`buildPtyHostEnv`) — the local lane's `baseEnv` includes `process.env`; the daemon lane's does not (see its own note at line 45).
## Required outcome
- The local PTY lane should produce the same session-bus address as the daemon lane, so a fallback spawn does not reintroduce the bug the PR fixes.
## Open questions for the human
- Is the local fallback lane intentionally out of scope (e.g. it is already non-persistent degraded mode), or should this be a follow-up? If in scope, calling `repairDisabledSessionBusEnv` in `buildLocalPtySpawnEnvironment` (or once in `buildPtyHostEnv` for the local path) is the smallest change.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
1bf345c to
c95d446
Compare
|
Rebased onto current The branch was rebuilt on the new base rather than replayed, so the commit SHAs changed and the four commits are now two. #19430 moved the daemon launch out of One thing worth flagging from the rebase: with the marker left in place, Validated locally: the related suites pass ( |
There was a problem hiding this comment.
ℹ️ No new issues in this delta. The changes are a rebase onto current
mainplus one genuinely new wiring site. One prior minor suggestion (dbus-session-bus-env.ts:14) remains open for the author.
Reviewed changes
- Rebased onto current
main— base movedde4dab9→122b8c2, and the four prior commits were rewritten into two. The helper and thecreateDaemonPtyEnvironmentcall are byte-identical to the last review. - Daemon repair relocated into the refactored spawn path — the repair moved from the old
daemon-launched-child.tsdaemonForkEnv()intospawnDaemonChildProcessindaemon-launched-child-spawn.ts, computed once ontodaemonEnv. systemd-run --user --scopearm now repaired as well — becausedaemonEnvfeeds both the directforkProcessarm andbuildDurableDaemonScopeCommand(...).env, the scoped launch no longer inherits thedisabled:marker, so it can no longer fail to connect and fall back to the unscoped path for that reason.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 26d2f9c1-ba85-40be-9f5a-8f41d17f714f
📥 Commits
Reviewing files that changed from the base of the PR and between c95d446768efba59825aa80f1094977f61ac11f9 and 069ff8d9902188a06e943372634a438702dea6f6.
📒 Files selected for processing (6)
src/main/daemon/daemon-cgroup-scope.tssrc/main/daemon/daemon-launched-child-spawn.test.tssrc/main/providers/local-pty-spawn-environment.tssrc/main/pty/dbus-session-bus-env-wiring.test.tssrc/main/pty/dbus-session-bus-env.test.tssrc/main/pty/dbus-session-bus-env.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior Pullfrog review (c95d446), this run reviewed commit 069ff8d:
- Reused the durable-scope bus resolver —
repairDisabledSessionBusEnvnow callsresolveUserRuntimeDir(and importsCANONICAL_USER_RUNTIME_DIR) instead of its ownexistsSyncpath check, so it prefers the per-UID bus, falls back toXDG_RUNTIME_DIR, and uses anisSocket()probe. This addresses the remaining inline thread's stale-path weakness and adds the XDG fallback. - Covered the local PTY lane —
buildLocalPtySpawnEnvironmentnow callsrepairDisabledSessionBusEnv, closing the fallback-lane gap raised as a body concern in the initial review. - Rewrote the helper tests against real Unix sockets — the previous
socketExistsseam is gone; cases now bind a real listener and exercise the injected platform/runtime-dir seams, including a plain-file-at-buscase that provesisSocket(). - Added per-path wiring and daemon-launch tests — a mocked-helper wiring suite asserts each of the three spawn paths hands on the repaired address, and
daemon-launched-child-spawn.test.tscovers both the directforkProcessandsystemd-run --scopearms.
I ran the four related suites (dbus-session-bus-env, dbus-session-bus-env-wiring, daemon-launched-child-spawn, daemon-cgroup-scope: 39 pass) and typecheck:node (clean). The prior inline thread is addressed and resolved.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Headless serve inherits Chromium's DBUS_SESSION_BUS_ADDRESS=disabled: marker. Add a Linux-only helper that swaps the exact marker for the user bus socket when that socket exists, and leaves every other value alone. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Apply the repair to the daemon launch env, which feeds both the direct fork and the systemd-run --user --scope arm, and to the daemon PTY env. With the marker left in place, systemd-run --user fails with "Connection refused" and the scoped launch falls back to the unscoped path. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…pawn path Resolve the bus with the resolver the durable daemon scope already uses (isSocket check, per-UID runtime dir first, XDG_RUNTIME_DIR as fallback) instead of a second existsSync check against the per-UID path. Repair the local, non-daemon PTY env too, which runs the same scrubbers as the daemon path. Tests now exercise the helper against real sockets through injected platform and runtime-dir seams, and wiring tests fail if any of the three spawn paths stops calling it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Skip Unix-socket fixtures on Windows, where filesystem IPC paths are not valid. Document the test doubles to satisfy the changed-function docstring check.
a8d0c42 to
d7db8d3
Compare
|
Rebased onto current Two conflicts, both from changes that landed next to ours:
The import in Checks after the rebase, on Linux arm64:
Still reproduces on v1.4.215 headless |
1 participant
ELI5
When Orca runs headless on Linux, every terminal it opens gets a broken setting that tells tools there is no user session bus. So
systemctl --userfails in those terminals, although the bus is fine. This PR puts the real bus address back before the terminal starts.What Changed
Before: headless
orca servestarts with no session bus in its env, so Chromium setsDBUS_SESSION_BUS_ADDRESS=disabled:in its own env. Service hardening can set the same marker. Orca passes the marker into the forked terminal daemon and into every PTY shell and agent. In those shells,systemctl --userandsystemd-run --userfail with "Failed to connect to bus: Connection refused", while the bus at/run/user/<uid>/busis healthy.After: those shells get
DBUS_SESSION_BUS_ADDRESS=unix:path=<runtime dir>/bus, and user-bus tools work.Mechanism:
repairDisabledSessionBusEnvinsrc/main/pty/dbus-session-bus-env.ts. It is Linux only, and it changes the variable only when the value is exactlydisabled:. It finds the bus withresolveUserRuntimeDirfromdaemon-cgroup-scope.ts, the same resolver the durable daemon scope uses: anisSocket()check, the per-UID runtime dir first,XDG_RUNTIME_DIRas a fallback. When no bus is reachable, the env does not change. That resolver and its constant are now exported, with no change in behaviour.daemon-launched-child-spawn.ts(this feeds both the direct fork and thesystemd-run --user --scopearm), the daemon PTY env inpty-subprocess/spawn-environment.ts, and the local PTY env inproviders/local-pty-spawn-environment.ts.Why
A real address, not a deleted variable: shells can carry an
XDG_RUNTIME_DIRthat points at a private hardening dir with no bus in it (the case theresolveUserRuntimeDircomment describes), so a shell tool cannot rediscover the bus on its own. The strictdisabled:match leaves every other value alone, including a real address and an absent variable.This also affects #19430. Its durable-scope launch runs
systemd-run --user --scopewith the daemon env. With the marker in that env,systemd-run --userfails (reproduced on systemd 255:DBUS_SESSION_BUS_ADDRESS=disabled: systemd-run --user --scope --collect trueexits 1 with "Connection refused", and the same command without the variable works). So under headless serve the scoped launch falls back to the unscoped path. The legacy migration and the scope death watch already clear the marker, butbuildDurableDaemonScopeCommanddoes not. The repair on the daemon launch env covers that arm too.Alternatives considered: delete the marker (as the migration and death watch do), which fails in shells for the reason above. Or a separate path check, which was the first version of this PR and duplicated the existing resolver with a weaker
existsSynccheck.Linked Issue
Fixes #21119
Visual Proof
N/A. No UI changes. The change is to the environment of spawned processes on Linux.
Testing
Local run, Linux arm64:
dbus-session-bus-env.test.ts: the helper against real Unix sockets through injected platform and runtime-dir seams (per-UID bus, per-UID over a hardenedXDG_RUNTIME_DIR,XDG_RUNTIME_DIRfallback, no bus, a plain file namedbus, real and absent addresses, non-Linux). Unix-socket cases are skipped on Windows, where Node requires named-pipe endpoints.dbus-session-bus-env-wiring.test.tsanddaemon-launched-child-spawn.test.ts: each of the three spawn paths hands on the repaired address. Removing any one of the three calls makes these tests fail. I checked this for each call.src/main/pty, daemon PTYs, local PTYs, daemon launch and scope: 45 files passed, 547 tests passed and 22 were skipped on Linux arm64.pnpm run typecheck:nodepassed.pnpm run check:code-quality:changedpassed on all 8 changed files. The commit hook'soxlint, React Doctor and formatting checks passed.orca serveshowsdisabled:, andsystemctl --userfails. Setting the repaired value by hand makessystemctl --user is-system-runningprintrunning.Not tested: a full end-to-end serve run with this build. The marker failure was manually reproduced before the fix, but I did not run a post-fix serve session. Repo CI has not run on this PR yet because it waits for maintainer approval.
AI Disclosure
Earlier revisions: Muse Spark via OpenCode. Rebase onto #19430, the move to the shared resolver, the local PTY path and the new tests: Claude Opus 5.5 (
claude-opus-5-5) with agentic tool use (shell, file edits, test runs). The Windows test guard and docstring follow-up used OpenAI GPT-6 Luna (gpt-6-luna) via OpenCode. A human directed and reviewed the work.Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
The repair is Linux only and a no-op off Linux. It runs only when the value is exactly
disabled:, so desktop sessions with a real bus do not change. It makes onestat()call per spawn only in that case. No SSH, mobile or relay code changes.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)The last box is not ticked: related tests, the node typecheck and changed-code quality gate passed. Full
pnpm tcwas terminated with exit code 137 in the container. Fullpnpm lint,pnpm testandpnpm buildwere not run.🤖 Generated with Muse Spark, Claude Opus 5.5, and OpenAI GPT-6 Luna
🧑💻 Ideated, directed and reviewed by a human, @oliver-mee