Repository navigation
Conversation
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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 c47f92f427a8b9dccf35e417fc1a15028c676c60 and bff26c4. 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe durable daemon-scope check now requires lingering or a launcher running under the matching user manager. If neither condition is met, it logs a one-time Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The durable daemon scope is now used only when the user manager outlives login sessions. Otherwise the daemon falls back to direct fork and logs a one-time hint. No unresolved defects were found, so the change looks ready to merge once CI passes. A live systemd logout test would still add confidence. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change avoids relying on a temporary login session for terminal persistence and does not add privileges. The existing fallback remains available, but its terminals can still stop with the service. Already-running terminals are not automatically relocated when migration is rejected. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 3 files. (2 skipped: 2 unsupported.)
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 |
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: 5a495fdb-49e8-44f2-836b-4c381a572e28
📥 Commits
Reviewing files that changed from the base of the PR and between cb36344 and c47f92f427a8b9dccf35e417fc1a15028c676c60.
📒 Files selected for processing (4)
docs/reference/headless-linux-server.mddocs/reference/orcad-operations.mdsrc/main/daemon/daemon-cgroup-scope.test.tssrc/main/daemon/daemon-cgroup-scope.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
There was a problem hiding this comment.
ℹ️ Minor suggestions only — two documentation-accuracy fixes in otherwise sound, well-tested code.
Reviewed changes
- Durable-scope gate now requires a persistent user manager —
isDurableDaemonScopeSupported()additionally checks, after the platform/boot/bus/systemd-rungates, that the user manager outlives the current sessions: lingering (/var/lib/systemd/linger/<user>) or the launcher's own cgroup containinguser@<uid>.service. Otherwise it logs oneenable-lingerhint per process and returns false, solaunchDaemonChilduses the existing direct-fork fallback. - New test seams threaded through migration —
lingerDir/ownCgroupPathtrail the existing seams andmigrateLegacyDaemonScopeforwards them; production callers pass nothing. - Tests — seven new cases in
daemon-cgroup-scope.test.tswith hermetic fixtures (real Unix socket, temp linger/cgroup dirs); I ran the file and all 32 pass. - Docs —
headless-linux-server.mdandorcad-operations.mdexplain that an open login is not a substitute for lingering.
I independently verified the load-bearing systemd assumption (session cgroups are siblings of user@<uid>.service, not children) and found no functional defect. The behavior change for desktops in a session-N.scope is already documented in the PR body and degrades to the proven fallback.
ℹ️ Nitpicks
daemon-cgroup-scope.ts:126still calls the probe "side-effect-free" in its JSDoc, but it now logs the once-per-processenable-lingerhint. Worth updating the contract sentence so the logging isn't mistaken for a bug.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…rsists The durable-scope probe accepted any reachable user bus. For `orca serve` as a system unit with User= and lingering off, that bus only exists while the account has a login session, and logind stops the user manager, with the daemon's orca-daemon-<nonce>.scope and every terminal in it, seconds after the last logout. Also require that the manager outlives the current sessions: lingering is on (logind's record in /var/lib/systemd/linger/, named by systemd's cescape() of the user name), or the launcher already runs under user@<uid>.service. Otherwise take the existing direct-fork fallback and log a one-time `loginctl enable-linger` hint, or a "cannot tell" line when there is no passwd entry or the record cannot be checked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
c47f92f to
bff26c4
Compare
|
Re: pullfrog review, nitpick on daemon-cgroup-scope.ts:126 Thanks, good catch. In bff26c4 the JSDoc reads "Best-effort, read-only capability probe that logs at most one hint per process", and the PR description no longer says side-effect-free. |
|
Re: coderabbit summary, pre-merge check "Docstring Coverage" Thanks. The repo has no docstring-coverage rule (AGENTS.md asks for brief, non-obvious comments only), so we're not adding docstrings for the threshold. The JSDoc wording you'd expect to matter (side effects, the desktop case) is being corrected in the next push. |
|
Re: coderabbit review, "Actionable comments posted: 1" Thanks. The one actionable comment (logind's escaped linger file name) is fixed in bff26c4; details in the inline thread. |
|
Re: greptile summary, findings 1 and 2 Thanks. Both findings are addressed in bff26c4. The hint no longer says lingering is off when it can't check (no passwd entry or an unreadable record). The test fixtures no longer read the host's passwd database. Details in the two inline threads. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes (delta since the prior pullfrog review at c47f92f)
- Linger-record handling extracted into
systemd-linger-record.ts—currentUsername()andisLingering()move out ofdaemon-cgroup-scope.ts, and a newlingerRecordName()reproduces systemd'scescape(). - Probe now separates "lingering is off" from "cannot tell" — a missing record logs the
loginctl enable-linger <user>hint, while an absent passwd entry or an unreadable linger dir logs a distinct "cannot tell whether lingering is on" line, once per process. - Prior doc feedback addressed — the
runsUnderOwnUserManagerdocstring and theorcad-operations.mdbullet now name the shapes that actually match (app-*.scopeunderuser@<uid>.service, or asystemctl --userunit) and state a plainsession-N.scopelogin does not count; the JSDoc no longer claims to be "side-effect-free". - Tests reworked and extended — the suite now mocks
node:osat file level with a name seam, adds fourcescape()fixture cases, an unreadable-dir case, and "cannot tell" assertions; existing fixtures inject a linger dir so they stay hermetic.
I independently verified the load-bearing external contract against systemd source: user_check_linger_file() (src/login/logind-user.c) reads /var/lib/systemd/linger/<cescape(user_name)>, method_set_user_linger() (src/login/logind-dbus.c) writes the same path from cescape(pw->pw_name), and cescape_char() (src/basic/escape.c) matches lingerRecordName byte-for-byte — including the named escapes and 3-digit octal range. isLingering's ENOENT→false / other-error→null mapping mirrors logind's access(p, F_OK). The new test fixtures (jos\303\251, DOMAIN\\orca, o\'brien, tab\there\"\177) are consistent with that contract, so the tests can fail if the escaping regresses. No functional defect found; the two prior threads are addressed and resolved.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

ELI5
On Linux, Orca puts its terminal helper (the daemon that owns every terminal) into a separate systemd "box" owned by the user's own systemd, so restarting the Orca service doesn't kill your terminals. But that user systemd only stays around after you log out if "lingering" is turned on. When Orca runs as a system service and lingering is off, Orca could still pick that box just because someone happened to be logged in at the time, and then every terminal died a few seconds after that person logged out. Now Orca only uses the box when it will actually last, and otherwise tells you, once, how to turn lingering on (or, if it cannot check, that it cannot tell).
What Changed
Before:
isDurableDaemonScopeSupported()said yes whenever systemd was PID 1, a user bus socket existed at/run/user/<uid>/bus(or$XDG_RUNTIME_DIR/bus), andsystemd-run --versionworked. Fororca serveas a system unit withUser=<account>and lingering off, that bus exists only while the account has a login session. If one was open when the daemon started, the daemon went intoorca-daemon-<nonce>.scopeunder that transient user manager, and systemd stopped the scope, and every terminal in it, about 10 s after the account's last logout. Orca logged nothing.After: the probe also requires that the user manager outlives the current login sessions. That holds when either:
/var/lib/systemd/linger/(username fromos.userInfo()). logind names the record by systemd'scescape()of the user name (src/login/logind-dbus.c,src/basic/escape.c), so Orca escapes the name the same way over its UTF-8 bytes: a backslash before\,",'and the seven C escapes (\a \b \f \n \r \t \v), and\ooooctal for every other byte below 0x20 or from 0x7f up.DOMAIN\orcais looked up asDOMAIN\\orca,joséasjos\303\251,o'brienaso\'brien; or/proc/self/cgroupwith the module's existingcgroupPathFromProc) containsuser@<uid>.service. That covers anapp-*.scopeunderuser@<uid>.serviceandorca serveas asystemctl --userunit, where the daemon can lose the manager no sooner than its launcher does. A plainsession-N.scopelogin does not count: it is a sibling ofuser@<uid>.service, not under it.Otherwise the probe returns false, so the daemon takes the existing direct-fork fallback, and Orca logs one line, once per process. When the linger record is missing:
When Orca cannot check, it does not claim that lingering is off. With no passwd entry for the UID it logs
... cannot tell whether lingering is on: UID <uid> has no passwd entry, so there is no user name to find its linger record by; see .... If the record lookup fails with anything other than ENOENT (an unreadable linger dir, say) it logs... cannot tell whether lingering is on for <user>: <linger dir> could not be read; see ....Mechanism details:
systemd-run --versionchecks, so the hint is only logged when lingering is the one thing missing (not on macOS, Windows, containers, or hosts withoutsystemd-run).readFileSync, oneaccess, oneuserInfo()) and logs at most one line per process. Every failure (unreadable cgroup file, no passwd entry for the UID, a linger lookup that fails with anything but ENOENT) resolves to "not supported". The module's "never throws, fail closed" contract is unchanged.currentUsername,isLingering, with thecescape()table) live in a new small module,src/main/daemon/systemd-linger-record.ts, which keepsdaemon-cgroup-scope.tsunder the repo's 300-line limit.isDurableDaemonScopeSupportedandmigrateLegacyDaemonScopegain two trailing test seams,lingerDirandownCgroupPath, next to the existingsystemdBootPath/canonicalRuntimeDirones. Production callers pass nothing and get the real paths.docs/reference/headless-linux-server.md: one short paragraph saying that an open login is not a substitute for lingering, and what Orca does instead.docs/reference/orcad-operations.md: the "All of these must hold" list gains the fourth condition (a user manager that outlives the login sessions: lingering, recorded as a file in/var/lib/systemd/linger/named after the C-escaped user name, or the launcher underuser@<uid>.serviceas anapp-*.scopeor asystemctl --userunit, where a plainsession-N.scopelogin does not count). The bus bullet no longer implies that being logged in replaces lingering.Why
The module header already states the requirement ("a reachable
--usermanager (a login session, orloginctl enable-linger <user>for a service account)"), and the headless doc already tells operators tosudo loginctl enable-linger orcaand says that without it the daemon falls back to the direct launch. The code only checked that the bus existed at probe time. A login session makes the bus exist only while that session lives: logind stopsuser@<uid>.serviceUserStopDelaySec(10 s by default) after the last session closes, and the manager stops every scope it owns. So without lingering the outcome depended on login timing, and the "logged in" outcome (the daemon dies at the next logout, silently) was worse than the fallback it was meant to improve on (the daemon dies at the next unit restart, which is documented).Alternatives considered:
loginctl show-user -p Linger, or D-Bus): a process spawn or a bus call on a path that must stay synchronous. logind answers theLingerproperty, and decides whether to keep the user manager running, withuser_check_linger_file()(src/login/logind-user-dbus.c,src/login/logind-user.c): anaccess()on/var/lib/systemd/linger/plus thecescape()d user name. Orca does the same lookup with the same escaping, without a spawn or a bus call. For the same user name it gets the same answer. The exception is a name that is not valid UTF-8: Orca reads the name with Node's defaultuserInfo(), which decodes it as UTF-8, so Orca then misses the record and falls back (systemd's own user-name check expects valid UTF-8, so such names are rare).user@<uid>.service(anapp-*.scopeor asystemctl --userunit), where it is correct today, because the launcher itself already lives and dies with that manager.Linked Issue
Fixes #24201
Visual Proof
N/A: no visual change. This is main-process daemon launch code with no renderer or UI surface; the only user-visible output is one log line.
Testing
New cases in
src/main/daemon/daemon-cgroup-scope.test.ts, all with injected fixtures (a real Unix socket as the bus, a temp linger dir, a temp cgroup file), so they never consult the host's own systemd. The suite also stubsos.userInfo()with its own user name, so no fixture reads the host's passwd database and the file passes on a UID without a passwd entry:is false when the user manager stops at the last logout: no lingering, launched outside it: bus present, systemd present,systemd-runworks, no linger file, launcher cgroup is a system service or an SSHsession-N.scope→ not supported.is true for a service outside the user manager when lingering keeps that manager alive→ supported.is true without lingering when the launcher already runs under this user's manager→ supported underuser@<uid>.service, not supported under another UID'suser@manager.names loginctl enable-linger once per process when lingering is what blocks the scope: no warning while lingering is on; two no-linger probes log exactly one single-line warning that says lingering is off for the user and namesloginctl enable-linger <user>and the headless doc.does not name enable-linger when an earlier gate already rules the scope out: with no linger file, adarwinplatform, a missing systemd boot marker, a runtime dir without a bus, and a failingsystemd-run --versioneach give "not supported" and log nothing.fails closed when its own cgroup file cannot be read and lingering is off→ not supported.fails closed when this UID has no passwd entry, whoever else is lingering, and says it cannot tell:os.userInfo()throws, and the linger dir has files for other names → not supported, and the one warning says it cannot tell whether lingering is on, not that it is off.finds logind's escaped linger record for <name>, not a raw-name file, forDOMAIN\orca,josé,o'brienand a name with a tab, a double quote and DEL: a record named as logind names it (DOMAIN\\orca,jos\303\251,o\'brien,tab\there\"\177, written out literally in the test) → supported; a file under the raw name → not supported.fails closed and says it cannot tell when the linger dir cannot be read(skipped as root): the record exists but its dir is mode 000 → not supported, and the warning says it cannot tell.On an earlier revision of this PR, cases 1, 3, 4, 6 and 7 failed on
mainwith only the tests applied (5 failed, 27 passed). Cases 2 and 5 pass onmaintoo; they guard that the fix keeps the scope where it is still correct and that the check stays after the other gates. Cases 4 and 7 have gained assertions since then, and cases 8 and 9 are new. On this revision, the existing "supported" cases and the legacy-migration case inject a lingering fixture so they stay hermetic.Local runs for this revision (Linux x64, Node 24.21.0, pnpm 12.0.0, on a shared machine under heavy load from other jobs):
vitest run --config config/vitest.config.ts src/main/daemon/daemon-cgroup-scope.test.ts: 37/37 pass. The same tests against the previous revision's source (c47f92f4, before the escaping and the "cannot tell" line): 6 failed, 31 passed. The six were cases 7 and 9 and the four names of case 8.unshare --user --map-user=54321): the previous revision's tests failed 7 of 32, every one fromuv_os_get_passwd returned ENOENTraised in the linger fixture. This revision passes 37/37.tscon a config that extendsconfig/tsconfig.node.jsonand lists the changed source files, the new module, their callers, the changed test file and one caller's test: 0 errors.oxlintandoxfmt --checkon the changed files, andpnpm run check:max-lines-ratchet: pass. The pre-commit hook (lint-staged: oxlint, the react-doctor oxlint config, oxfmt) ran on the commit.Wider runs were made on the previous revision only and not repeated for this one. All four full commands stopped at the local 8 GB / 1800 s cap:
pnpm testandpnpm linthit the runtime cap, andpnpm typecheckandpnpm buildran out of memory. Every failure in the part ofpnpm testthat ran was in a file that does not import the changed code, and the first 14 of the 16pnpm lintsteps passed.vitest run --config config/vitest.config.ts src/main/daemon src/main/orcadhad 3 failures there, all in files that do not import the changed module directly: two load timeouts (orcad-push-startup,orcad-bundle-native-load-order) and one timing-sensitive assertion (daemon-restore-scrollback-depth).pnpm run build:electron-vitepassed there.CI should run the full four commands.
Not tested: a live
orca serveon a systemd host with the logout sequence from the issue, and a real logind record for an account whose name logind escapes (the escaping follows systemd's source and is pinned by case 8). The reproduction in the issue was observed on 1.4.211; the post-fix behaviour on a real host is covered only by the unit tests above.AI Disclosure
Claude Opus 5.5 (claude-opus-5-5), Anthropic. Used for the investigation, the patch, the tests and this description, directed and reviewed by a human.
Review
AI coding-agent review summary:
platform !== 'linux'and/run/systemd/systemgates, so macOS and Windows return false exactly as before and never read the new paths.process.getuidabsence is handled (the cgroup branch is skipped).orca serveget the same rule, which is the point of the fix. The SSH-deployedorcadon Linux remotes also reaches this probe:src/main/ssh/orcad-remote-launch.tsstarts it withexec nohupinside the SSH login session, and it launches its terminal daemon through the samelaunchDaemonChildpath. On a remote without lingering, that daemon used to go into a scope under the login-triggered user manager, which stops about 10 s after the last SSH session closes whileorcaditself lives on in the session scope. Now it stays in the session scope next toorcad, and the one-line hint appears in the remoteorcad.log(whereorcad's stderr goes). On a remote with lingering on, nothing changes.accessand onegetpwuidper daemon launch (an infrequent event); no polling. The warning is logged once per process./proc/self/cgroupand of logind's linger record under/var/lib/systemd/linger/; the username comes from the OS, not from input, and is only C-escaped into a path join and used in a log line. No new privileges, no command execution added.session-N.scope(not underuser@<uid>.service) and without lingering now uses the direct-fork launch. The daemon there stays in the session scope, which with logind's defaultKillUserProcesses=nooutlives the user manager at logout, so it is no less durable than before. WithKillUserProcesses=yesthe session scope is stopped at logout, so the daemon dies then, about 10 s sooner than the old user-manager scope did. That host also kills the desktop launcher at logout, so the terminals end with the app in both cases.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.Not applicable: core daemon launch code, not the skill-installer system.
Notes
sudo loginctl enable-linger <account>, which the headless doc already asks for.resolveUserRuntimeDirfrom the same file; the two changes touch different functions and should rebase trivially.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)The last box is not ticked: the full commands did not fit in the local memory/runtime cap. The parts that cover this change pass (see Testing), and CI will run the full four commands.
🤖 Generated with Claude Code