fix(launchagent): bound the LoopX service logs - #5076
Conversation
huangruiteng
left a comment
There was a problem hiding this comment.
Request changes conclusion (author-owned PR; GitHub blocks formal self-review)
Exact head: b96dfd810ae9b328f9eb4bc19c8d74149b83488e; immutable merge base: 3e443ad7c285973c40be883293970be6c0c51e06.
动机
macOS status/chat 服务的日志跨 KeepAlive 重启持续追加,旧噪声停止后也没有回收。需要在既有 LaunchAgent 所有者内保留一代历史、让后续启动回收空间,并向用户如实读回安装策略。目标依据是 installer 的日志/启动合同、既有 LaunchAgent smoke 和用户可观察的保留结果;不依赖作者单个主机的私有日志。
整个 PR 只有 installer + 既有 smoke 两文件、+80/-2,是合适的维护切片。作者明确限定“每次服务启动检查”,不承诺单个持续运行进程的实时字节上限。本评审接受该边界,持续进程的无限输出不是这里凭空增加的阻塞项。
改动思路
在每个生成 wrapper 中检查 stdout/stderr,超过阈值时 copy-and-truncate;这保留 launchd 已打开的 append descriptor 对应 inode。仅在 installer 旋转会漏过 KeepAlive,rename 会把输出送入旧 inode;复用现有 wrapper 比新建 daemon/定时任务小得多。最强反对理由是它现在把备份失败变成静默丢日志,且“active policy”读回并非安装事实。
具体改动
关键代码讲解
scripts/macos-dashboard-launchagent.sh:77的log_rotation_prelude产生两路日志检查,阈值来自调用 installer 时的LOOPX_LOG_MAX_BYTES,默认 10 MiB。write_plists:245–246将该 prelude 嵌入 status/chat 的/bin/zsh -c,之后仍使用原来的 Python、PATH、Codex home 与服务 argv;既有 control-plane-write opt-in 参数未改。print_status:453新增保留策略文本,却读取当前 status 进程的环境变量,而不是已落盘 wrapper。examples/macos-dashboard-launchagent-status-smoke.py:61的check_log_rotation使用真实 O_APPEND descriptor 验证 inode 和备份内容;两 agent 都进入该检查,under-threshold 控制保留历史。
已验证的阻塞问题:
- [P2] 备份复制成功后才能截断 live 日志。
log_rotation_prelude:79用分号连接cp -f ...与: > ...,并丢弃复制错误。因此权限拒绝、磁盘/IO 错误会丢失尚未备份的内容,wrapper 仍成功 exec 服务。独立验证调用真实 installer 生成 plist,再运行完整 zsh ProgramArguments,使用 macOS 实际 cp/stat/O_APPEND 文件;只隔离 launchctl 和最终服务。在两个 agent 上,2048 字节 live 日志对应不可写的.1目标,真实 cp 控制退出 1;head 的 live 仅剩 15 字节 service 输出、无可读备份、wrapper 退出 0。相同 base 输入保留 2048 字节历史加 service 输出。最小修复是复制成功才截断;失败保留原日志并给出可诊断的警告/重试策略,加入真实复制失败负例。 - [P2] status 应显示已安装的阈值。
print_status:453与write_plists消费不同时间的环境。在LOOPX_LOG_MAX_BYTES=1024安装后,用4096调用 status 会显示“exceeds 4096 bytes”,但 plist 仍按 1024 旋转;普通不带 override 的 status 同样会回到默认值。作者承诺报告 active policy,这个输出会误导日志保留和故障判断。应从安装的、由 installer 产生的明确配置读回;若仅展示本次调用的拟配置,明确标注并另报实际 installed policy。覆盖 override install → omitted/different-env status → restart/readback。
另一个输入边界值得在相同有界修复中处理:LOOPX_LOG_MAX_BYTES=invalid 的 install 退出 0 并公开打印 invalid bytes,没有 fail-fast;检查数值格式/范围后再生成 shell。该观察没有被夸大成已验证的远程代码攻击。
语义与 CI 对齐
默认每启动旋转是明确的行为变化,PR/help/status 和既有 smoke 已披露。它不属于 default-off 功能,因此不能用 feature-off 测试冒充 default parity;对真正共享的执行权限和 identity 路径进行了 base/head 比较,control-plane-write 默认关闭、已有 Codex home 绑定和 argv 保持原所有者。没有 peer 生命周期、Goal 权限、quota 或 generic control-plane 义务变化。日志保留属于主机 installer,无需新的 frontend/Lark 入口;CLI status 才是必须正确的伴随读回。
对主干的风险
独立 immutable base/head 执行既有 LaunchAgent smoke 均通过。相同隔离主机 fixture 经过真实 installer → plist → 完整 zsh wrapper → macOS 文件 backend,覆盖 status/chat 两 agent、两 stream、超过/低于阈值、重复大日志启动、带空格 HOME 路径、复制失败及不同 status 环境。正常 head 备份精确、inode 不变、后续输出到 live 路径;base 不旋转是预期变化。rename 语义 mutation 在临时脚本中执行,原生 smoke 因 live path 消失按独立 inode 不变量失败;当前 head 正常控制通过。没有安装或重启真实用户服务。
uv run --extra test python -m pytest -q -rs tests/test_dashboard_command.py tests/test_self_update_runtime_activation.py 在构建并验证 Chat bundle 后 61 passed, 1 skipped;skip 是明确的 native Windows update boundary,不是改动涉及的 macOS路径。最初缺少 bundle 时 base/head 同样有 5 个包装入口失败,构建准备完成后 head 已重跑通过;不把准备失败说成 PR 回归,也不把 skip 说成通过。bash -n、diff whitespace 和两文件边界扫描 errors=0。远端 CI 不获取、不轮询,遵循本 packet 的本地验证合同。
我的整体评价
REQUEST_CHANGES。 wrapper 放置和 inode 处理正确,代码量与问题相称;目前有效的重复启动结果不能抵消失败时删除唯一历史和虚假的策略读回。长期继续启动应保留可恢复证据,用户应能从 status 知道真正生效的设置。修复两项阻塞并复跑同一真实文件/配置反例后再审整个 PR。未来改动友好性检查已做:保留单一 prelude owner,并让同一安装配置驱动 wrapper 和 status;无需新日志服务或手工同步状态。未合并。
English verdict: REQUEST_CHANGES
The per-start copy-and-truncate design correctly preserves append descriptors. However, a failed backup silently destroys the live history, and status reports the caller environment rather than the installed retention policy. Fix both behaviors and rerun the native paired cases.
The status and chat LaunchAgents append to `~/Library/Logs/loopx/*.log` with no retention, so those files only ever grow. KeepAlive means they outlive every release, and a service that is noisy for a while leaves that output on disk indefinitely: on one host the two stdout logs had reached 59 MB and 49 MB. Rotate a log to `.1` once it passes a size limit, defaulting to 10 MiB and overridable with `LOOPX_LOG_MAX_BYTES`. The rotation runs inside each agent's own wrapper rather than in this installer, because launchd restarts the service on its own and those restarts never re-enter the installer -- which is exactly when the unbounded growth happens. It also copies the previous generation aside and truncates in place rather than renaming. launchd opens StandardOutPath before the wrapper runs and keeps appending to that descriptor, so a rename would send the service's output to the rotated copy and leave the live log missing. The smoke reproduces that descriptor and asserts the live path still receives post-rotation writes, so the rename mistake cannot come back. Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Retention truncated the live log whether or not the copy aside succeeded, and status reported the caller's environment rather than the installed policy. Both are corrected here, together with the input validation the first fix needs to be meaningful: - The wrapper only truncates after `cp` succeeded, and refuses to copy over an existing directory at the backup path (where `cp` would otherwise copy *into* it and report success). When the copy fails the live log keeps every byte, and the agent's error log gets a line naming the path so the operator can diagnose it; the next agent start retries. - `status` reads the retention size back out of the installed plist, so it reports what the running agents do. An explicitly different `LOOPX_LOG_MAX_BYTES` adds a note that it is not in effect yet, and a missing retention step is reported as unknown instead of as a default. - `install`/`restart` reject a `LOOPX_LOG_MAX_BYTES` that is not a positive byte count before writing a wrapper whose retention step could never match. The status smoke covers both failure injections for the copy (a directory at the backup path, and a logs directory that refuses new files), the installed -versus-override readback, and the rejected install leaving the previous wrapper in place. Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
b96dfd8 to
8299357
Compare
huangruiteng
left a comment
There was a problem hiding this comment.
Approval conclusion (author-owned PR; GitHub blocks formal self-approval)
Reviewed head: 829935702b53eab8d92eac5c91b9bcdbe3496f72
Base: main (rebased; the branch is no longer behind)
This head supersedes the Request-changes conclusion published on b96dfd810
(pullrequestreview-5326602187). Both blocking findings and the input boundary
raised in the same bounded fix are answered.
动机
macOS status/chat 服务的日志跨 KeepAlive 重启一直追加,旧噪声结束后也没有回收,所以需要在既有 LaunchAgent 所有者内保留一代历史。这个切片本身是合适的维护改动:两文件、复用既有 wrapper、不新建 daemon 或定时任务,作者也明确限定"每次服务启动检查"而不是承诺单一进程的实时字节上限。但它的价值取决于两个读回是否诚实:回收不能以丢掉唯一历史为代价,status 也不能报告一个没有生效的设置。上一个 head 在这两点上都是反向的——复制失败仍会截断日志,status 读的是调用者环境。本 head 把这两处改为按契约工作,判定为 goal_achieved。
改动思路
权威输入是安装器写下的 wrapper:launchd 在 wrapper 之前打开 StandardOutPath 并持续 append,因此只能用"复制一代 + 原地截断"(rename 会把服务输出送进旧 inode,并让 live 路径消失)。判定权保持在同一个 prelude 所有者里:它决定何时回收,而 status 的读回改为从已安装 plist 解析真实阈值,不再读环境。失败路径的归属也明确:复制失败时保留 live 日志、把可诊断的一行写进该 agent 的 err 日志,并在下次启动重试;安装参数非法时在写 wrapper 之前拒绝。副作用是 print_status 需要读一次 plist,代价可忽略;没有新增文件或状态。
具体改动
三处实质修改。其一,截断改为条件式:cp 成功才 : > live 文件,且当备份路径上已存在目录时直接不复制(cp -f 会把文件复制进那个目录并返回 0,按旧实现会"备份成功"地截断);失败时向 err 日志打印保持原日志并将在下次启动重试的提示。其二,status 从已安装的 plist 解析实际阈值,并在显式传入的 LOOPX_LOG_MAX_BYTES 与之不同时提示"未生效,直到下次 install/restart";没有 retention 步骤时报告 unknown 而不是回落到默认值。其三,install/restart 在用 LOOPX_LOG_MAX_BYTES 生成 wrapper 之前校验它必须是正的字节数,非法值以退出码 2 拒绝,不写入半成品 wrapper。验证侧把三种情形都做成真实文件系统输入:备份路径被目录占据、日志目录拒绝新建文件、安装时 override → 不带/不同环境读回,以及非法安装被拒后已装 wrapper 保持不变。
关键代码讲解
scripts/macos-dashboard-launchagent.sh:84log_rotation_prelude:生成每个 agent 启动时执行的回收步骤。旧版用cp ...; : >无条件截断并丢弃复制错误,权限拒绝或磁盘错误会丢掉尚未备份的内容;新版先判断大小与备份路径,再以cp的成功作为截断前提,失败时保留全部字节并写一行诊断信息到该 agent 的 err 日志。scripts/macos-dashboard-launchagent.sh:95installed_log_max_bytes:从已安装 plist 的 wrapper 文本里读回-gt <bytes>,即"实际生效"的策略;读不到即返回失败,让调用方报告 unknown,而不是假设默认值。scripts/macos-dashboard-launchagent.sh:104validate_log_max_bytes与:490main的安装分支:在生成 wrapper 前拒绝非正整数,避免写出永远不会匹配的 retention 步骤(旧版会打印invalid bytes并成功安装)。scripts/macos-dashboard-launchagent.sh:478print_status的读回分支:改报已安装阈值,仅在调用者显式给出不同值时追加"未生效"说明;没有 retention 步骤时明确报 unknown。examples/macos-dashboard-launchagent-status-smoke.py:93check_retention_keeps_the_log_when_the_backup_fails:用两种真实失败注入(目录占据备份路径、日志目录 0500 拒绝新建文件)断言 live 内容逐字节保留、备份不产生半成品、err 日志含提示,并保持 wrapper 退出 0 继续 exec 服务。
语义与 CI 对齐
按默认值每次启动回收是明确的行为变化,PR 描述、usage() 与 status 都已披露;它不属于 default-off 能力,因此这里比较的是 base/head 的真实文件与配置行为,而不是 feature-off parity。没有 peer 生命周期、Goal 权限、quota 或通用 control-plane 义务变化;日志保留属于主机 installer,CLI status 是必须正确的伴随读回。
对主干的风险
No blocking finding. 修复后没有阻塞性问题;以下是残留风险与最强缺失验证。
最大的风险是"回收动作反而成为数据丢失点",以及"策略读回与实际不符"。前者现在由复制成功作为截断前提覆盖,两种失败注入都验证 live 内容不变;后者由 plist 读回覆盖,override/不带 override 两种 status 都验证输出与实际安装一致。残留风险两点:一是备份路径上若长期存在目录,回收会持续跳过并在每次启动各写一行提示(有噪声但无数据丢失);二是持续运行且输出极大的单个进程仍不受每次启动检查的约束——这是作者明确接受、本评审也接受的边界,不属于本 PR 新增的缺口。最强缺失验证:没有在真实 launchd 上安装并观察 KeepAlive 重启后的文件状态(smoke 用真实 zsh wrapper、真实文件系统与真实 O_APPEND 描述符,但隔离了 launchctl),这一层由既有安装路径承担。上一轮的两项 finding 与输入边界均已修复并有对应断言,且我用变异验证确认过它们不是空断言。
我的整体评价
APPROVE。修复把回收从"先删后证"改成"证明备份成功才回收",并让 status 读回已安装策略而不是调用者环境,两处都是让这个切片真正可信的最小改动,代码量与原问题相称。outcome_impact 两个维度均为 improved——长期看日志保留不会在失败时抹掉证据、每次启动都重试,用户体验上 status 报的是实际生效的设置,故障时能看到一行可诊断的提示。剩余边界(持续进程无限输出、真实 launchd 端到端)已写明归属,不需要本 PR 承接。合并交维护者执行。
English verdict: APPROVE
|
Merge decision (maintainer repair at the merged head) Reviewed head: What changed since the previously reviewed head (
Validation at this head: the status smoke passes with two real copy-failure |
Problem
The status and chat LaunchAgents append to
~/Library/Logs/loopx/*.logwithno retention, so those files only ever grow.
KeepAlivemeans they outliveevery release, and a service that is noisy for a while leaves that output on
disk indefinitely.
Observed on one host:
status.out.logat 59 MB (1.87M lines) andchat.out.logat 49 MB, accumulated across 138 service starts. The bulk wasa repeated status render from an older release that no longer reproduces —
which is the point: nothing reclaimed that space once the noise stopped, and
nothing would have bounded it while it was happening.
Change
Rotate a log to
.1once it passes a size limit, defaulting to 10 MiB andoverridable with
LOOPX_LOG_MAX_BYTES.statusreports the active policy.Two constraints shaped where and how this runs, and both are the reason the
obvious implementations are wrong:
It runs inside each agent's wrapper, not in this installer. launchd
restarts these services on its own and those restarts never re-enter the
installer — which is exactly when the unbounded growth happens. Rotating at
install/restart time would have missed all 138 starts that produced the logs
above.
It truncates in place instead of renaming. launchd opens
StandardOutPathbefore the wrapper runs and keeps appending to thatdescriptor. A rename leaves that descriptor attached to the rotated inode, so
the service's output would silently follow the
.1copy and the live logwould stay missing until the next restart. Copying the previous generation
aside and truncating lets the append-mode descriptor fall back to offset zero.
Validation
examples/macos-dashboard-launchagent-status-smoke.pypasses, extended tocover both agents and both of their streams.
Mutation-checked. The smoke reproduces launchd's pre-opened append
descriptor: it opens the live log with
O_APPEND, runs the agent's ownrotation prelude, then writes through that descriptor and asserts the live
path received it while
.1kept the history. Replacing the copy-and-truncatewith
mv -ffails the smoke with the reason spelled out, so the renamemistake cannot come back. Confirmed by temporarily applying that regression.
The under-threshold case is covered too, so rotation stays retention rather
than a reset on every service start.
Scope
Retention is bounded per service start, which is what the observed growth
pattern needed. A single process that floods its log between restarts is
still unbounded by this change; that belongs to whatever is doing the
flooding, and the render loop behind the 59 MB above is already gone on the
current release.
Maintainer repair at the merged head
Reviewed head
829935702b53eab8d92eac5c91b9bcdbe3496f72, merge commit4906620cdcc1634648fd85a18be4dd9a2094cd1e; the merged tree equals the reviewed head.statusreads the retention size back out of the installed plist, notes an override that is not in effect, and reportsunknownwhen no retention step is installed.install/restartreject a non-positive-integerLOOPX_LOG_MAX_BYTESwith exit 2 before writing a wrapper.tests/test_dashboard_command.pyplustests/test_self_update_runtime_activation.py→ 62 passed, 1 skipped (native Windows boundary) with the chat bundle built.