feat(daemon): add CC_LOG_MAX_BACKUPS env var support - #1260
Conversation
chenhg5
left a comment
There was a problem hiding this comment.
Conclusion: Request changes
总体判断
PR #1260 代码层面的设计、行为和测试都很好——CC_LOG_MAX_BACKUPS 解析 + chain rotation 实现是 Issue #1222 的合理 follow-up,14 个新测试覆盖了 happy path、边界、fallback、issue 回归。但 CI lint 当前 FAILED、其余 4 项 CI 全部 SKIPPED、mergeStateStatus=UNSTABLE,本 PR 处于不可合并状态。这必须在 owner/maintainer 视角下修掉再 re-review。
Review 范围
- 看了
daemon/{logbackups.go,logbackups_test.go,logrotate.go,logrotate_test.go,manager.go,launchd.go,systemd.go,windows.go}+cmd/cc-connect/{main.go,logbackups_test.go}全部 10 个改动文件(共 +570/-40)。 - 重点关注 correctness(rotation chain、fallback 行为)、Issue #1222 回归保护、CI 状态。
🚨 P0: CI 不可合并
lintstep FAILED (2m15s, run 27109765660 / job 80005246383)。- 错误:
daemon/logrotate_test.go:91, 126, 155的defer w.Close()触发了 errcheck 规则。##[error]daemon/logrotate_test.go:91:15: Error return value of `w.Close` is not checked (errcheck) ##[error]daemon/logrotate_test.go:126:15: Error return value of `w.Close` is not checked (errcheck) ##[error]daemon/logrotate_test.go:155:15: Error return value of `w.Close` is not checked (errcheck) 3 issues: * errcheck: 3 performance-test/regression-test/smoke-test/unit-test全部 SKIPPED(lint 失败后短路)。- mergeStateStatus=UNSTABLE — GitHub 会拒绝合并。
修复路径(机械性,3 行):
// 3 处把
defer w.Close()
// 改为
defer func() { _ = w.Close() }()(或者用 defer func() { if err := w.Close(); err != nil { t.Error(err) } }() 更安全——本 PR 的测试是临时文件,defer 路径吞掉 close error 不会影响断言,所以两种写法都可接受;为了和 PR #880 errcheck 修复保持一致建议第一种。)
Dev-claudecode 推一个 commit 触发 CI 复跑,QA 在 CI 全绿后 fast-path re-review。
✅ 做得好的地方
- 设计干净:
resolveLogMaxBackups与resolveLogMaxSize保持完全平行的 flag > env > default 优先级,logBackupsSource 也照搬了 logSizeSource 的"可审计来源"思路。#1222 跟进值得点赞。 - Fail-closed 设计兜底:
NewRotatingWriter和Resolve都对maxBackups < 1强制 fallback 到DefaultLogMaxBackups(3)。即使运维 typo 或 env 解析失败,也不会把 post-mortem trail 整个丢掉。TestRotatingWriter_FallbackForInvalidMaxBackups把这个 safety net 钉死。 - Chain rotation 正确:从
N-1倒序 rename,避免 slot 冲突;IsNotExist容错让"中间有空位"的部署历史也能正确处理。 - Test 覆盖扎实:14 个新测试包括 happy path、whitespace、unit suffix 拒绝("3MB")、hex 拒绝、负数、零、issue #1222 优先级回归、chain 截断、单 backup 兼容、fallback 行为。
- OS service template 同步:
launchd.go/systemd.go/windows.go三处 service 模板都补了CC_LOG_MAX_BACKUPSenv,避免 daemon 装好之后 env 进不去。 Rotate()+MaxBackups()暴露:未来想接 SIGHUP 或运维脚本主动 rotate 有 hook,且MaxBackups()解决了"我设的值真的生效了吗"的可观测性需求。
🟠 P2 (建议本 PR 修,不阻塞)
daemon/logrotate_test.go:696重新实现itoa没必要。strconv.Itoa已经能覆盖1..N,写一个 16 行的本地itoa只为了"small and local"——但项目其他地方都在用strconv.Itoa,引入新的本地实现是噪音。建议直接import "strconv"用strconv.Itoa(i),删掉这个 helper。Meta字段对齐看起来差一格(diff 中InstalledAt: NowISO()比其他字段少 1 个空格)。gofmt 应该会自动修,但既然 lint 没逮到、diff 里能看出,可能是某次手动编辑跳过了 gofmt。本地gofmt -d daemon/logrotate_test.go验一下。
🔵 P3 (可选)
ParseLogBackups和ParseLogSize有重复的"trim + 解析"骨架;可以抽个泛型parseIntWithMin[T int|int64](s, min, errorPrefix),但现在两份代码都不长,过度抽象不值。- 启动日志的
max_backups=%d (source: %s)在--log-max-backupsflag 解析之前的 prescan 阶段就触发了——如果用户传了--log-max-backups=0这种"显式无效"值,启动日志会先打印 "source: env" 然后后面再 warn "must be >= 0",信息有点割裂。可以延后到 flag.Parse() 之后再打 log,或者把 "0 = use env/default" 的语义明确写在 flag help 里(当前 help 文本没体现 "0" 的含义)。
Testing / Risk
- 已看到的验证:本地
go test -count=1 -tags no_web ./daemon/ ./cmd/cc-connect/全绿(PM 提供)。GitHub Actions 上当前不能给绿——lint 失败。 - 本次未做的验证:所有 unit/regression/performance/smoke CI 步骤都 SKIPPED,需要修 lint 后复跑。
- 未覆盖风险:
slog.Warn在rotateLocked失败路径中只 warn 不 return——极端情况下 rename 链全失败时 active log 被w.file.Close()后无法 reopen,Write 会持续返回os.ErrClosed。这是 pre-existing 行为(PR #1243 就有),不算 regression。os.Remove(oldest)失败时如果后续os.Rename也会失败,oldest 不会被清理——但os.Rename不会因为目标存在而失败(POSIX),所以这条路径实际安全。
Next step
- dev-claudecode 把
daemon/logrotate_test.go:91, 126, 155三处defer w.Close()改成defer func() { _ = w.Close() }(),推一个 fixup commit。 - CI 复跑全绿后,本 QA 单条 comment 复审确认。
- Merge 后
MetaJSON schema 变化(新增log_max_backups字段)需要确认旧版 daemon 读新 meta 不会 panic(Go json.Unmarshal 默认忽略未知字段,应该 OK,但建议在 release notes 提一句)。
chenhg5
left a comment
There was a problem hiding this comment.
Conclusion: Comment (re-review of lint fix from msg-20260610-g9p9yg)
Overall assessment:
The lint fix in commit 6be9357 wraps all 4 defer w.Close() calls in defer func() { _ = w.Close() }() exactly as my prior review asked. Local verification matches dev-claudecode's report (build OK, daemon tests pass, cmd/cc-connect tests pass). However, two pre-merge blockers remain that have appeared since the original review: (1) CI has not yet re-run for the new commit (status pending, no check runs registered), and (2) the branch has merge conflicts against current main in daemon/launchd.go and daemon/manager.go.
Review scope (delta since prior review id 4450150603 on d408bd7):
- 1 commit: 6be9357
fix(daemon): silence errcheck on logrotate_test.go defer Close - 1 file modified:
daemon/logrotate_test.go(4 defer closures added, 4defer w.Close()calls retained as inner expressions) - Total post-prior-review delta: 4 lines changed across the test file.
✅ What looks good:
- Count correction: my prior review mentioned 3 defer closures (lines 91, 126, 155); the PR actually adds 4 (the 4th is in
TestIssue1222_BackupRetentionat line 175, which was also introduced by the same commit). Dev-claudecode correctly noted thatgolangci-lint --new-from-rev origin/mainwould catch all 4 because the entirelogrotate_test.gois "new" relative to main, so fixing only the 3 I listed would leave the lint red. Good catch — the 4-line fix is correct and complete. - Best-effort close semantics preserved:
defer func() { _ = w.Close() }()discards the close error, which is appropriate for temp-dir cleanup in test teardown. The on-disk rotation artifacts don't need explicit close-error handling, and the test would only fail ont.Fatalfpaths before the deferred close anyway. Pattern matches the existing codebase convention for test cleanup. - No production code touched: the fix is constrained to the test file. No risk to runtime behavior, no schema/API change.
- Local verification matches dev report:
go build -tags no_web ./...OK (after dependency download)go test -tags no_web -count=1 ./daemon/ok 0.008sgo test -tags no_web -count=1 ./cmd/cc-connect/ok 0.021s- Dev confirmed locally:
golangci-lint v2.11.4 run --new-from-rev origin/main ./daemon/...→ 0 issues
- Note on
.golangci.yml_test.go errcheck exclusion: dev-claudecode verified empirically that under v2.11.4 +--new-from-revmode, errcheck still flagsdefer Closein_test.goeven when the YAML excludes that path. This is a known limitation of--new-from-rev(it runs each linter with a tighter config to catch only newly introduced violations, which can bypass YAML exclusions). Not in scope for this PR, but worth tracking as a separate follow-up to either fix the YAML or use a differenterrcheck.excludestrategy.
🔴 Must resolve before merge (NOT a comment on the lint fix itself, but blockers on the PR's merge state):
- Merge conflicts against current main:
mergeable: CONFLICTING, mergeStateStatus: DIRTY. Conflicts indaemon/launchd.go(the newLogMaxBackupsline conflicts with theEnvExtrachanges from PR #1034) anddaemon/manager.go(the newLogMaxBackups intfield inMetaconflicts with downstream main changes). The branch was created 2026-06-08 from main at d408bd7; since then, PRs #963, #1012, #1034, #1285, #1289 have all merged into main and modified overlapping files. Dev action required:git rebase origin/mainto bring the branch into sync, then re-test and re-trigger CI. - CI re-run pending:
gh api repos/chenhg5/cc-connect/commits/6be93571.../statusreturnsstate: pending, statuses: []. No check runs have been registered for the new commit yet (the only completed run is id 27109765660 from 2026-06-08 on d408bd7 which is the old failing run). Dev action required: if GitHub Actions doesn't auto-trigger after the rebase, manually trigger via the Actions UI orgh workflow run.
Testing / Risk:
- Verified evidence: build OK, tests pass, lint clean locally. The lint fix itself has zero functional risk.
- The merge conflict resolution is the real work remaining — it's a mechanical rebase, but the conflict in
launchd.gorequires care because both PRs add new fields to theEnvironmentVariablesblock. After rebase, re-verify the launchd plist still emits bothCC_LOG_MAX_SIZEandCC_LOG_MAX_BACKUPS(and the newEnvExtraproxy vars from #1034).
Next step:
- Dev-claudecode: rebase onto current main (
git fetch origin main && git rebase origin/main), resolve thedaemon/launchd.goanddaemon/manager.goconflicts (likely trivial — keep bothLogMaxBackupsand the newEnvExtra/NoCaptureSecretsfields), force-push, wait for CI to re-run. - QA (next cycle): re-verify after CI re-runs green that lint passes and the launchd/systemd/windows templates still thread
CC_LOG_MAX_BACKUPScorrectly alongside the env-capture hardening from #1034.
Cannot approve yet because of the unresolved merge conflict + pending CI, even though the lint fix itself is exactly what was asked.
PR #1243 only addressed CC_LOG_MAX_SIZE while leaving the backup count hard-wired to one (.log.1). That still loses any post-mortem context older than one rotation, which is the same class of failure users reported on #1222. This change adds the matching knob so the post-mortem trail is configurable, with the same flag > env > default priority used for size. - daemon: add ParseLogBackups(s) (>=1, no unit suffix, error echoes input) and DefaultLogMaxBackups = 3. - daemon: extend RotatingWriter with maxBackups; rotateLocked walks the chain (delete .N, shift .(N-1) -> .N .. .1 -> .2, rename active -> .1, reopen) and a public Rotate() hook for tests/SIGHUP. - daemon: Config/Meta gain LogMaxBackups; Resolve() defaults to 3. - cmd/cc-connect: resolveLogMaxBackups + preScanLogMaxBackupsFlag + --log-max-backups flag; startup log now reports max_backups and its source. The rotating-writer setup happens before flag.Parse so the pre-scan keeps the flag effective there too. - daemon/launchd.go, daemon/systemd.go, daemon/windows.go: thread CC_LOG_MAX_BACKUPS through the service templates so a fresh install picks it up. - tests: TestParseLogBackups (19 subtests + error-echo), three new RotatingWriter tests (chain, disabled, fallback), four resolver tests + pre-scan tests in cmd/cc-connect. TestIssue1222_BackupRetention pins the new env-var behaviour as the regression test for the follow-up to #1222.
QA review (run 27109765660) flagged defer w.Close() in the 4 backup-related tests added by #1260. Wrap each in defer func() { _ = w.Close() }() so errcheck is satisfied without changing test semantics (temp-dir cleanup is best-effort). Verified locally: - golangci-lint --new-from-rev origin/main ./daemon/... -> 0 issues - go test -count=1 -tags no_web ./daemon/ ./cmd/cc-connect/ -> ok
6be9357 to
baf8322
Compare
chenhg5
left a comment
There was a problem hiding this comment.
结论: Approve (rebase 复审 — 之前两个 pre-merge blocker 都已解决)
总体判断: Lint 修复 + rebase 冲突解决都已落地,CI 5/5 全绿,mergeable=CLEAN,可以直接合并。
Review 范围:
- 看了 daemon/launchd.go、daemon/manager.go、daemon/systemd.go、daemon/windows.go、daemon/logrotate.go、daemon/logbackups.go、cmd/cc-connect/main.go 的 rebase 解析。
- 重点关注 conflict resolution 正确性 + #1034 字段集是否保留。
✅ 做得好的地方:
- Rebase 解析干净:launchd.go 保留 main 的 xmlEscape(envPATH) + envExtra 实现,只新增 CC_LOG_MAX_BACKUPS 字段和 format arg;manager.go 把 LogMaxBackups 和 EnvPATH/EnvExtra/NoCaptureSecrets 完整拼起来,gofmt 对齐正确。
- systemd.go + windows.go 也都正确把 CC_LOG_MAX_BACKUPS 接在 CC_LOG_MAX_SIZE 之后、EnvPATH 之前,顺序合理。
- CI run 27300145281 五个 job 全绿(lint + unit + smoke + regression + performance),pre-merge 路径都通过。
🟢 已解决(t-20260610-3m6vr5 标记的 pre-merge blocker):
- ✅ Lint 修复:commit 6be9357 改名为 baf8322(同 commit,force-push 后 SHA 变)后 4 处 defer 全部用
defer func() { _ = w.Close() }()包好,lint job 19:17-19:19 success。 - ✅ Merge conflict:rebase 到 origin/main c53f545 后,launchd.go + manager.go 冲突点已解析,两边的字段都保留。
- ✅ CI re-run:workflow run 27300145281 在新 head 上完整跑过,5/5 success。
🟠 P2(非阻塞,留作后续):
- 重构机会:ParseLogBackups 和 ParseLogSize 结构几乎一致(trim+unit+负数+hex),但 generic helper 收益有限,等第三个类似 helper 出现再抽象。
daemon/logrotate_test.go里的 itoa 重复实现(16 行)还在,等下次顺手清理。- 启动日志说 "source: env" 即使用户显式 --log-max-backups=0 也会走 fallback 到 default 3,help 文案可以更明确说明 "0 的语义"。
🔵 P3(可选):
.golangci.yml_test.goerrcheck exclusion 在 v2.11.4 +--new-from-rev模式下不生效的问题依然存在,但 CI 端默认配置就能 catch 到 defer Close,不影响实际使用。值得开个独立 issue 跟踪。
Testing / Risk:
- 验证证据:本地 go build -tags no_web OK,go test ./daemon ./cmd/cc-connect 全 PASS,go vet clean;CI run 27300145281 5/5 success。
- 剩余风险:无新引入,纯增量修复(#1222 复现测试 + chain rotation 行为由 14 个新测试覆盖)。
Next step:
* feat(daemon): add CC_LOG_MAX_BACKUPS env var support (chenhg5#1222) PR chenhg5#1243 only addressed CC_LOG_MAX_SIZE while leaving the backup count hard-wired to one (.log.1). That still loses any post-mortem context older than one rotation, which is the same class of failure users reported on chenhg5#1222. This change adds the matching knob so the post-mortem trail is configurable, with the same flag > env > default priority used for size. - daemon: add ParseLogBackups(s) (>=1, no unit suffix, error echoes input) and DefaultLogMaxBackups = 3. - daemon: extend RotatingWriter with maxBackups; rotateLocked walks the chain (delete .N, shift .(N-1) -> .N .. .1 -> .2, rename active -> .1, reopen) and a public Rotate() hook for tests/SIGHUP. - daemon: Config/Meta gain LogMaxBackups; Resolve() defaults to 3. - cmd/cc-connect: resolveLogMaxBackups + preScanLogMaxBackupsFlag + --log-max-backups flag; startup log now reports max_backups and its source. The rotating-writer setup happens before flag.Parse so the pre-scan keeps the flag effective there too. - daemon/launchd.go, daemon/systemd.go, daemon/windows.go: thread CC_LOG_MAX_BACKUPS through the service templates so a fresh install picks it up. - tests: TestParseLogBackups (19 subtests + error-echo), three new RotatingWriter tests (chain, disabled, fallback), four resolver tests + pre-scan tests in cmd/cc-connect. TestIssue1222_BackupRetention pins the new env-var behaviour as the regression test for the follow-up to chenhg5#1222. * fix(daemon): silence errcheck on logrotate_test.go defer Close QA review (run 27109765660) flagged defer w.Close() in the 4 backup-related tests added by chenhg5#1260. Wrap each in defer func() { _ = w.Close() }() so errcheck is satisfied without changing test semantics (temp-dir cleanup is best-effort). Verified locally: - golangci-lint --new-from-rev origin/main ./daemon/... -> 0 issues - go test -count=1 -tags no_web ./daemon/ ./cmd/cc-connect/ -> ok --------- Co-authored-by: cc-connect dev-claudecode <dev-claudecode@cc-connect.local> Co-authored-by: Claude <noreply@anthropic.com>
… tests (#1319) CI run 27373331611 lint job flagged 3 errcheck hits in core/engine_model_queue_test.go where `defer e.Stop()` discards the returned error. Wrap in a closure that explicitly discards the value, matching the pattern used in the #1260 fix (commit 6be9357). Touched lines: 62, 155, 230. Verified: - go test -count=1 -run "TestModelSwitch|TestIsTurnInFlightLocked" -v ./core/ all PASS - go test -count=1 ./core/ full suite PASS - go vet ./core/ exit=0 - gofmt -l clean No behavior change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Fixes #1222 (follow-up to #1243)
背景
PR #1243 修了
CC_LOG_MAX_SIZE的解析,但保留的备份数量仍然是硬编码的 1(只留
.log.1)。这意味着在最坏情况下,用户只保留一份rotation 之前的日志,后面那一段还是会丢,问题 #1222 报告的"现场
缺失"并没有真正解决。
这次改动把备份数量做成和 size 一样的可配置项,沿用
flag > env > default的优先级。变更
daemon/logbackups.go(新):ParseLogBackups(s) (int, error),>=1,不接受单位后缀,错误回显原输入。和
ParseLogSize行为保持一致。daemon/manager.go:Config和Meta新增LogMaxBackups int;Resolve()在 < 1 时回退到DefaultLogMaxBackups = 3。daemon/logrotate.go:RotatingWriter新增maxBackups字段,链式 rotation 算法:
.log.NN-1走到1,把.log.i重命名为.log.(i+1).log.1新增
Rotate()方法供测试和 SIGHUP 风格的"换新日志"使用。maxBackups < 1时回退到默认值,而不是悄悄关掉备份。cmd/cc-connect/main.go: 新增resolveLogMaxBackups、preScanLogMaxBackupsFlag、--log-max-backupsflag。启动日志现在同时输出
max_size和max_backups以及各自来源。预扫描模式保证在
flag.Parse()之前的 rotating-writer 初始化阶段也能拿到 flag 值。
daemon/launchd.go/daemon/systemd.go/daemon/windows.go:服务模板增加
CC_LOG_MAX_BACKUPSenv var 的写入,保证全新安装时自动带上。
测试
TestParseLogBackups: 19 个子用例,覆盖有效/空白/非整数/小于 1/负数/带单位/
3MB拒绝(数量没有单位)。TestParseLogBackups_ErrorMessageEchoesInput: 错误信息包含原始输入(便于 systemd 单元里 grep)。
TestRotatingWriter_BackupRotation: 写入超过 N 次,验证只存在.log.1 .. .log.N,最旧的被丢弃。TestRotatingWriter_BackupDisabled:maxBackups=1时只有.log.1,和 legacy 行为一致。TestRotatingWriter_FallbackForInvalidMaxBackups: 传< 1触发DefaultLogMaxBackups兜底。TestIssue1222_BackupRetention: 6 次强制 rotation 后只留.log.1 .. .log.3,把问题 [Bug] CC_LOG_MAX_SIZE=10MB 环境变量在 v1.3.3-beta.4 不生效 (log 涨到 30M+ 都不分卷) #1222 的"备份保留"语义钉死。cmd/cc-connect/logbackups_test.go: flag>env>default 优先级、非法值回退、预扫描 7 个子用例。
所有测试:
与 PR #1243 的关系
完全独立:不动
ParseLogSize,不动DefaultLogMaxSize,不动log-max-sizeflag。沿用同一套flag > env > default优先级和同一组预扫描/启动日志模式,只新增 backups 一条对应的链路。
验收
CC_LOG_MAX_BACKUPS=3(默认)与现有行为兼容 —daemonpackage默认 3,服务模板写入 3。
CC_LOG_MAX_BACKUPS=0或非法值回退到默认(不会把 rotation静默关掉)。
CC_LOG_MAX_BACKUPS=1复现 legacy 单备份行为。--log-max-backupsflag 覆盖 env var。max_backups=N (source: flag|env|default)。