From 05ac57b183e467d2be2334852c56dcb4d415159e Mon Sep 17 00:00:00 2001 From: "lei.chen" Date: Thu, 27 Aug 2026 13:11:09 +0000 Subject: [PATCH] fix(worker): restore getpwnam execuser-existence check (bug-1507 merge regression) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scheduler-framework merge (4e894d21) reverted the bug-1507 fix from c091a8da: job_start_local_task fell back to nobody only when execuser was EMPTY, not when it does not exist on the worker host — a submit-user missing from /etc/passwd makes systemd fail the worker unit with EXIT_USER (217) and every job dies. Restored the getpwnam() existence check (+ #include ). All 21 worker tests pass; fallback log message verified in the built xcecd. Also records the merge-overwrite lesson in cerebrum Do-Not-Repeat: after any branch-sync merge, re-verify each previously-fixed bug's marker symbol survived (git show : | grep -c vs ^). --- .wolf/buglog.json | 10 ++++++---- .wolf/cerebrum.md | 1 + .wolf/memory.md | 1 + src/worker/src/controller_connector.c | 19 ++++++++++++++++++- 4 files changed, 26 insertions(+), 5 deletions(-) diff --git a/.wolf/buglog.json b/.wolf/buglog.json index e14adba4..cb848afa 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -14867,20 +14867,22 @@ "error_message": "\u4f5c\u4e1a\u63d0\u4ea4\u540e\u7acb\u5373 EXIT: systemd \u5355\u5143 xcecd-worker-.service \u5931\u8d25, status=217/USER (\u670d\u52a1\u7528\u6237\u4e0d\u5b58\u5728). \u7528\u6237 1-0-1785897786-0 \u7b49\u5168\u90e8\u5931\u8d25.", "file": "src/worker/src/controller_connector.c (job_start_local_task)", "root_cause": "apiserver bug-1500 \u4fee\u590d\u8bbe\u7f6e si.execuser=resolveExecUser(job)=\"admin\" (\u63d0\u4ea4\u7528\u6237). \u4f46 admin \u5728 worker \u4e3b\u673a /etc/passwd \u4e0d\u5b58\u5728. worker \u628a limits->user_name \u8bbe\u4e3a execuser, start_systemd_job \u751f\u6210 User=admin \u5355\u5143 -> systemd EXIT_USER 217 -> \u4f5c\u4e1a\u5931\u8d25. \u4e4b\u524d execuser \u4e3a\u7a7a\u65f6\u9ed8\u8ba4 nobody(\u5b58\u5728) \u80fd\u8dd1, bug-1500 \u90e8\u7f72\u540e\u53d8\u6210 admin \u53cd\u800c\u5168\u5931\u8d25.", - "fix": "job_start_local_task \u7528 getpwnam() \u68c0\u67e5 execuser \u662f\u5426\u5b58\u5728; \u4e0d\u5b58\u5728\u5219\u56de\u9000 nobody. \u4f5c\u4e1a\u4ee5 nobody \u8fd0\u884c(\u7528\u6237\u5df2\u786e\u8ba4\u73b0\u9636\u6bb5\u53ef\u4ee5).", + "fix": "job_start_local_task \u7528 getpwnam() \u68c0\u67e5 execuser \u662f\u5426\u5b58\u5728; \u4e0d\u5b58\u5728\u5219\u56de\u9000 nobody. \u4f5c\u4e1a\u4ee5 nobody \u8fd0\u884c(\u7528\u6237\u5df2\u786e\u8ba4\u73b0\u9636\u6bb5\u53ef\u4ee5). [2026-08-26 UPDATE] MERGE REGRESSION: the getpwnam() existence check was reverted by merge commit 4e894d21 (Rongsong scheduler-framework sync) \u2014 controller_connector.c now falls back to nobody only when execuser is empty, not when it does not exist (getpwnam gone from whole tree, verified 0 hits; chatbot^ had 1). admin exists on t1 /etc/passwd so not currently triggering, but latent \u2014 restore the getpwnam check (c091a8da had it). [2026-08-26] RESTORED: re-applied getpwnam() check in job_start_local_task (+ #include ), aligned to c091a8da. Worker builds clean, all 21 worker tests pass (controller_connector_test 15/0, resource_sync 2/0, xcecd_startup 2/0, dbus_workers 2/0). Verified the fallback log message is in the built xcecd binary.", "tags": [ "worker", "execuser", "systemd", "217", "nobody", - "regression" + "regression", + "merge-regression", + "needs-restore" ], "related_bugs": [ "bug-1500" ], - "occurrences": 1, - "last_seen": "2026-08-05" + "occurrences": 2, + "last_seen": "2026-08-26T12:00:00Z" }, { "id": "bug-1508", diff --git a/.wolf/cerebrum.md b/.wolf/cerebrum.md index fbfe58c8..ccf04eb1 100644 --- a/.wolf/cerebrum.md +++ b/.wolf/cerebrum.md @@ -118,6 +118,7 @@ ## Do-Not-Repeat +- (2026-08-26, scheduler-framework merge) **A git merge that syncs another developer's branch CAN silently REVERT our already-committed fixes** when the other branch has an older/different version of the same file — the file-level resolution picks the other side, and our fix is gone with no conflict marker. This session found TWO such regressions from `4e894d21` (Rongsong scheduler-framework sync): bug-1490 (clib.go `ControlJob` lost `jobCtrlDiscriminant()`, so every kill/stop/resume failed `-EINVAL`) and bug-1507 (controller_connector.c lost the `getpwnam()` execuser-existence check, so a submit-user missing from the worker host's /etc/passwd kills every job with systemd 217). **Post-merge checklist — do this on EVERY branch-sync merge before committing/running tests:** (1) enumerate our files/fixes that the other branch also touches (diff our tip vs theirs: `git log --all --oneline -S ` for each fix's signature symbol, or `git diff ... -- `); (2) for each fix's marker symbol, assert it survived the merge: `git show : | grep -c ` vs `git show ^: | grep -c ` — 0-after-1-before means the merge reverted it; (3) look specifically for defensive/fallback branches (`getpwnam` fallback, union-discriminant mapping, null-guards) — exactly the kind of safety code a sync commonly clobbers. For xce: our tracked worker/controller fixes are listed in `.wolf/buglog.json` tags `fixed` — after any future merge, re-verify each one's marker symbol is still present. - (2026-07-29, bug-1482) When a code-review finding says "X is never freed, add a free() call" citing a `_for_test` wrapper's own doc comment as the ownership contract, VERIFY the doc comment against the actual callee before complying literally — don't just trust it. `myreq_handler_for_test`'s comment claimed "the caller still owns/frees `pkt` itself", but `myreq_handler` unconditionally calls `xce_decode(pkt, ...)` as its first step, and `xce_decode()` (src/lib/request.c:274-275) itself unconditionally does `free_packet(pkt); FREEUP(pkt);` before returning — pkt is fully consumed/freed internally, no leak exists. Blindly adding the suggested `free_packet(pkt); free(pkt);` after the call produced an immediate ASAN heap-use-after-free/double-free (independent of `detect_leaks`, since UAF detection isn't gated by leak detection) — caught only by actually rebuilding and running the authoritative test command, not by code inspection alone. Fix was to correct the misleading doc comment instead of adding the free. **Lesson: for any wrapper that decodes/consumes a buffer (xce_decode-style), trace the actual free call sites transitively before trusting an ownership comment — a stale/wrong doc comment is itself a bug, and "add the missing free" is not always the right fix for a reported leak.** - (2026-07-19, bug-1452) `commit_engine_fini(&ce)` MUST run AFTER `persist_stop(p)`, never before, whenever the `persist_t` was `persist_start`'d (i.e. has a live `fsync_worker` thread). `commit_engine_fini`'s own header comment says this explicitly ("Callers are expected to have stopped the flusher (persist_stop) first") but `test_persist_truncate.c`'s three continuation tests called it backwards (`commit_engine_fini` then `teardown_persist`→`persist_stop`). `persist_set_durable_cb(p, NULL, NULL)` inside `commit_engine_fini` is an unsynchronized plain-pointer unhook — while the flusher thread is still alive it can already be mid-flight in `commit_on_durable_advance`/`commit_advance_locked` (no lock held, per persist.c:398-399's documented lock-ordering) and race the teardown: the guard `ce->p == NULL` check passes on a still-valid read, then a SEPARATE later re-read of `ce->p` inside `commit_advance_locked` (commit.c:208) observes the value `commit_engine_fini` just nulled — SEGV at exactly `offsetof(persist_t, committed_lsn)` (0x38). This was misdiagnosed for a day as caused by the g_drive_async_demotion/catchup_offload flags (bug-1450/1451) because flipping them changed thread-scheduling timing enough to trigger the always-latent race more reliably — the SAME crash signature was independently seen on 2026-07-07, before those flags existed, proving the flags were never causal. Fix: call `persist_stop(p)` immediately before `commit_engine_fini(&ce)`; the teardown helper's own internal `persist_stop` becomes a safe no-op second call. **Same backwards ordering also exists in `test_demotion_convergence.c` (line 233 `commit_engine_fini` before line 235 `persist_stop`) against a real `persist_start`'d fixture — not yet fixed, flag for a follow-up sweep of every `commit_engine_fini` call site in the controller test suite.** - (2026-07-03, raft-apply-decoupling task 1.5) `ninja -C build`'s INCREMENTAL rebuild in this worktree's shared `build/` dir can silently produce a stale/wrong `controller_unit_test` binary that reports a build success but gives WRONG test results for the actual HEAD source — distinct from bug-924 (which is the `meson test` driver hanging on overlayfs fdatasync). Symptom that tipped this off: `test_config_repl`'s `test_commit_advance_retry_clears_push_before_commit_fence` was independently measured as 70/70-passing by one task's reviewer and 10/10-failing by the next task's implementer, on what both believed was the same commit. Root-caused by isolating each measurement in its own throwaway `git worktree add ` + fresh `meson setup --buildtype=debug -Db_sanitize=address` + `ninja -C ` — proved the SAME commit passes cleanly with a fresh build and fails deterministically only in the main worktree's incrementally-rebuilt `build/`. Fix was `rm -rf build && meson setup build ... && ninja -C build` in the main worktree. **Lesson: whenever a test result looks surprising or contradicts a very recent measurement on the same commit, do NOT immediately conclude a source regression — first rule out a stale incremental build by either a full `rm -rf build` rebuild or an isolated `git worktree add` + fresh setup.** Never mutate the current worktree in place to "check an old commit" (e.g. `git checkout -- .`) — it silently reverts working-tree files against current HEAD; use `git worktree add` instead. diff --git a/.wolf/memory.md b/.wolf/memory.md index a7ac1ae3..548db044 100644 --- a/.wolf/memory.md +++ b/.wolf/memory.md @@ -7406,3 +7406,4 @@ | 15:5x | controller full suite 3 FAILs root-caused: Rongsong WAL_JOB_DISPATCH replay tests assert JOB_STAT_RUN after persist_recover, but our bug-1509 B3 (mark_all_run_jobs_unknown_in_memory in persist_recover, p->cluster set) ORs JOB_STAT_UNKWN|TRANSIENT onto RUN. Reconciled by updating the 3 tests (test_wal_job_dispatch_fact.c x2, test_dispatch_commit.c x1) to assert JOB_IS_RUN (base RUN recovered, Rongsong's intent incl. bug-1678 allocs) + UNKWN flag set (our B3). Both groups pass. | controller/testsuite/*.c | 6+4 tests OK | ~2k | | 10:3x | merged 集群重启+验证：nix build '.#xce' 产出 w96pv5-xcecore（含 pending_after 修复）；AppRun/runtime 被 nix-store --gc 清掉 → package.sh 用旧 AppImage 提取的 AppRun + 拆出的 runtime ELF 前缀打包，但 `find(b'hsqs')` 命中了 runtime 内部数据 → runtime 被截断（22460B），新 AppImage --appimage-extract 崩 SIGILL。绕过：unsquashfs -d 解包 squashfs 到 /opt/xce/appimage/squashfs-root + touch marker 让 start-cluster.sh 跳过 --appimage-extract（t1 用真实 nix store w96pv5）。集群起来：jobs submit(含 from_host)→RUN→DONE→kill 全通，hosts max_slot=8/NUM_RUN/HOST_TYPE 正常。**新发现 merge 回归**：Rongsong clib.go 覆盖了我们的 bug-1490 修复（ControlJob 的 ctrl.action 应映射到 capnp JobControl union 判别值 0/1，不是 Go JOB_* 常量 1/2/3/8）→ 所有 kill/stop 都 -22 EINVAL。已恢复 `ctrl.action = C.int(jobCtrlDiscriminant(action))`（helper 在 convert_pure.go:30 还活着）+ 重建 apiserver 到 /tmp/apiserver_merged2 + 重启 apiserver，kill 验证通过。待办：重建 xce(含 clib.go 修复)+打包 AppImage 做干净部署；修 AppImage runtime（从 nixpkgs 重建 AppRun+runtime）。 | persist.c, clib.go, start-cluster.sh, package.sh | 集群 OK；bug-1490 回归已修 | ~4k | | 11:5x | 遗留项处理完成：(1) 重建 xce 含 clib.go 修复 → 新 xcecore zjxxy909，apiserver 已从 store 干净 build 重启（非 /tmp 补丁），submit/kill/hosts 验证通过。(2) AppImage runtime 修复：nixpkgs 无 classic appimageTools（都被 GC/新版风格），下载 appimagetool（github continuous, 8.8MB）→ APPIMAGE_EXTRACT_AND_RUN=1 绕过 FUSE → 从 AppDir（AppRun+mountroot+99 store paths 闭包+desktop+icon）构建出正常 AppImage（--appimage-extract 可用）。注意：nix copy --to file:// 会建 binary cache 布局不是 nix/store 布局；cp -al 对目录 hardlink 失败、cp -a 保留只读权限难清理，用 cp -r 重建 AppDir。 | AppImage zjxxy, apiserver store build | 全绿 | ~3k | +| 12:1x | bug-1507 merge 回归修复：恢复 getpwnam() execuser 存在性检查（+pwd.h），对齐 c091a8da。worker 全测试 21/0 通过。cerebrum 加 Do-Not-Repeat：merge 同步会静默覆盖我们的修复（bug-1490 clib.go + bug-1507 getpwnam 两个实例）——post-merge 必须核对每个 fixed bug 的 marker symbol 是否幸存（git show :|grep -c vs ^）。 | controller_connector.c, cerebrum.md | 修复+教训记录 | ~500 | diff --git a/src/worker/src/controller_connector.c b/src/worker/src/controller_connector.c index c1665b94..97528144 100755 --- a/src/worker/src/controller_connector.c +++ b/src/worker/src/controller_connector.c @@ -8,6 +8,7 @@ #include "xcelog.h" #include #include +#include #include #include #include @@ -1000,7 +1001,23 @@ static int job_start_local_task(xce_request_t* request) } if (job_ctx->startinfo->execuser != NULL) { - job_ctx->limits->user_name = strdup(job_ctx->startinfo->execuser); + /* bug-1507 (merge-regression-restored): the exec user is the + * submitting account, which is not necessarily a real /etc/passwd + * account on this worker host, and systemd then fails the worker + * unit with EXIT_USER (217), killing every job. Fall back to + * nobody so the job still runs. (Restored from c091a8da — the + * scheduler-framework merge 4e894d21 had reverted this check.) */ + struct passwd* pw = getpwnam(job_ctx->startinfo->execuser); + if (pw != NULL) { + job_ctx->limits->user_name + = strdup(job_ctx->startinfo->execuser); + } else { + job_ctx->limits->user_name = strdup("nobody"); + XCECD_LOG(XCE_LOG_WARN, + "exec user '%s' does not exist on this host, " + "running job as nobody", + job_ctx->startinfo->execuser); + } } else { job_ctx->limits->user_name = strdup("nobody"); XCECD_LOG(XCE_LOG_WARN,