diff --git a/.wolf/cerebrum.md b/.wolf/cerebrum.md index a8813a77..5c6cb370 100644 --- a/.wolf/cerebrum.md +++ b/.wolf/cerebrum.md @@ -61,6 +61,9 @@ - **`repl_info.c` (scheduler REPL state-inspection dumps) was already OVER this project's 800-line cap (883 lines) before this session even started** -- accumulated from three prior sessions' bug fixes (bug-1587/1590/1591) each adding a little without ever triggering a new sibling-file split. A task-12.5 rewrite of the resource-display functions brought it down to 856, still over. Flag this to the user rather than silently ignoring it or unilaterally doing a 4th-generation file split (repl.c -> repl_break.c/repl_info.c -> repl_params.c precedent) without being asked -- a split touches meson.build, test_sim_isolation.c's per-file scan macros, AND every white-box test TU's #include list (real, mechanical work, not worth taking on speculatively). - **A `repl_split_tokens()`-style whitespace tokenizer with zero quote-awareness silently breaks any downstream parser that requires a whole-string-anchored regex match** (e.g. `parse_jobid()`'s `pmatch[0].rm_so==0 && pmatch[0].rm_eo==len` check) when a user quotes an argument out of habit (common when copy-pasting a value from another command's own output). The numeric fallback path (`strtoull()`) then silently parses the leading quote character as "not a digit" and returns 0 rather than erroring -- producing a confusing wrong-answer, not a clear parse failure. Worth checking for in any REPL/CLI tokenizer this codebase adds. See bug-1596. +- **A pointer returned by `makeString()` (rtti-managed, `lib/xstring.c`) must always be released via `dereference()`, never `FREEUP()`/plain `free()`** — `makeInstance()`/`objAlloc()` returns an offset into a combined header+data block, not the block's own `malloc()`-returned base address, and the string is refcounted (freed only when the refcount hits zero), not directly owned. bug-1600 (2026-08-19, production, not a test fixture — see also bug-1503's earlier test-fixture instance of the identical mismatch): `lib/group.c`'s `makeGroup()` "existing group" update branch (hit every time a config-push re-applies a group name that already has members, e.g. any retried/duplicate config push — no thread concurrency required) called `FREEUP(m->u.name)` on a `makeString()`-allocated field, while the SAME FILE's `group_killer()` destructor released the identical field correctly via `dereference(m->u.name)`. This caused the real, reproducible (8 confirmed core dumps) `xce_scheduler` `free(): invalid pointer` SIGABRT crash during controller-failover config-push retries. **When adding or auditing any free/release path for a field populated by `makeString()`, grep the same file/struct for an existing destructor and match its release call — don't assume `FREEUP()` is safe just because it's used for adjacent plain-`calloc`'d fields in the same struct.** +- **`lib/group.c`'s `makeGroup()` "existing group" update path takes only `RTTI_LOCK_READ` (a genuine shared `pthread_rwlock_rdlock`, confirmed via `kvmgr.c`) while mutating shared group member-list state, and the scheduler's entire config-push handler chain (`sched_init_config()` → `plugin_config_update()` → ... → `makeGroup()`) holds no serializing lock at all — while `scheduler_req_handler()` genuinely runs on whichever of the scheduler's 4 vcpu worker pthreads picked up that vtask (`vtaskmgr_init(..., 4)`).** `xce_ctx_reconnect()` reuses the same `ctx->ep` struct and `endpoint_init()` unconditionally resets `ep->busy=0`, so a reconnect racing with a still-running config-push handler can let a second, concurrently-dispatched config-push run `makeGroup()` on the same group object at the same time as the first. This is a REAL latent concurrency bug distinct from bug-1600's allocator mismatch (which alone was sufficient to explain the observed crash and was the one fixed) — flagged as a follow-up, not yet fixed; would need a write-lock scope redesign since `init_group_by_config()`'s callees (`get_group()`) take their own internal read lock, so naively holding a write lock across the whole function would self-deadlock on the non-recursive rwlock. + ## Do-Not-Repeat - Don't run `clang-format -i` on whole existing files here — the repo isn't clang-format-clean at baseline, so a full reformat buries the real diff; recovering via `git checkout HEAD -- ` on a file with other uncommitted work destroys that work irrecoverably. - Before citing a bug as "still open" or a spec as "blocked," cross-check `.wolf/buglog.json`'s actual fix status and the spec's own tasks.md/commit history — blocked_on notes and prior narrative claims (including this file's own past entries) can be stale. **Self-caught recurrence (2026-08-19)**: while reconciling OTHER stale doc references, wrote a new tasks.md status line asserting "tasks 2.1-2.6 still carry no `_Verified:` annotations" by trusting a 2026-08-07 validation finding's own wording verbatim, without re-checking the file — they'd actually been backfilled the very next day (commit `d7b49690`, 2026-08-08). A dispatched verification agent caught it via `git log`/direct read. Lesson holds even for a claim you are ABOUT to write, not just one you're citing from memory: grep/read the actual current state of the specific lines before asserting their status, every time, no exceptions for "this one's probably still true." diff --git a/.wolf/memory.md b/.wolf/memory.md index c1b9b300..fce45d4d 100644 --- a/.wolf/memory.md +++ b/.wolf/memory.md @@ -2493,3 +2493,19 @@ | 20:00 | Edited src/scheduler/testsuite/test_repl_breakpoints_inspection.c | 1→2 lines | ~39 | | 20:00 | Session end: 59 writes across 21 files (test_repl_breakpoints_inspection.c, test_sim_startup.c, test_sim_event_loop.c, meson.build, test_core_isolation.c) | 56 reads | ~622851 tok | | 20:02 | Edited .kiro/steering/roadmap.md | 1→2 lines | ~394 | +| 20:03 | Session end: 60 writes across 21 files (test_repl_breakpoints_inspection.c, test_sim_startup.c, test_sim_event_loop.c, meson.build, test_core_isolation.c) | 56 reads | ~623792 tok | +| 20:21 | Session end: 60 writes across 21 files (test_repl_breakpoints_inspection.c, test_sim_startup.c, test_sim_event_loop.c, meson.build, test_core_isolation.c) | 56 reads | ~623792 tok | +| 20:26 | Session end: 60 writes across 21 files (test_repl_breakpoints_inspection.c, test_sim_startup.c, test_sim_event_loop.c, meson.build, test_core_isolation.c) | 57 reads | ~635646 tok | +| 20:29 | Session end: 60 writes across 21 files (test_repl_breakpoints_inspection.c, test_sim_startup.c, test_sim_event_loop.c, meson.build, test_core_isolation.c) | 59 reads | ~635646 tok | +| 20:37 | Session end: 60 writes across 21 files (test_repl_breakpoints_inspection.c, test_sim_startup.c, test_sim_event_loop.c, meson.build, test_core_isolation.c) | 60 reads | ~644876 tok | + +## Session: 2026-08-20 20:42 + +| Time | Action | File(s) | Outcome | ~Tokens | +|------|--------|---------|---------|--------| +| 20:55 | Edited src/lib/testsuite/test_group.c | modified test_reconfigure_same_group_members() | ~370 | +| 20:55 | Session end: 1 writes across 1 files (test_group.c) | 19 reads | ~25243 tok | +| 20:57 | Session end: 1 writes across 1 files (test_group.c) | 19 reads | ~25243 tok | +| 21:01 | Edited src/lib/group.c | FREEUP() → dereference() | ~24 | +| 21:02 | Session end: 2 writes across 2 files (test_group.c, group.c) | 19 reads | ~25269 tok | +| 21:06 | Fixed production heap-corruption crash: makeGroup() FREEUP() vs dereference() allocator mismatch | src/lib/group.c, src/lib/testsuite/test_group.c | RED->GREEN confirmed via ASAN, logged bug-1600 | ~15000 | diff --git a/src/lib/group.c b/src/lib/group.c index b4c70e22..fdcf5dfc 100644 --- a/src/lib/group.c +++ b/src/lib/group.c @@ -231,7 +231,7 @@ xce_group_t* makeGroup(var mgr, xce_group_config_t* config, continue; } if (m->is_group == 0) { - FREEUP(m->u.name); + dereference(m->u.name); } FREEUP(m); @@ -244,7 +244,7 @@ xce_group_t* makeGroup(var mgr, xce_group_config_t* config, continue; } if (m->is_group == 0) { - FREEUP(m->u.name); + dereference(m->u.name); } FREEUP(m); diff --git a/src/lib/testsuite/test_group.c b/src/lib/testsuite/test_group.c index 0943835a..6e47e29d 100644 --- a/src/lib/testsuite/test_group.c +++ b/src/lib/testsuite/test_group.c @@ -255,12 +255,51 @@ static void test_nest2(void) destroy(mgr); } +static void test_reconfigure_same_group_members(void) +{ + var mgr; + xce_group_config_t gdata1 = { + .name = "g1", + .members = { .count = 1, .data = (char*[]) { "rshen" } } + }; + xce_group_config_t gdata2 = { + .name = "g1", + .members = { .count = 1, .data = (char*[]) { "alice" } } + }; + + xce_group_t* g = NULL; + int rc = 0; + + groupmgr_init(&mgr, 16, 16); + + g = makeGroup(mgr, &gdata1, NULL); + + TEST_ASSERT_TRUE(g != NULL); + + /* Re-applying config for the SAME group name (e.g. a repeated config + * push) exercises makeGroup()'s existing-group update path, which must + * release the old members without corrupting the heap. */ + g = makeGroup(mgr, &gdata2, NULL); + + TEST_ASSERT_TRUE(g != NULL); + + rc = is_in_group(g, "alice"); + + TEST_ASSERT_TRUE(rc != 0); + + rc = is_in_group(g, "rshen"); + + TEST_ASSERT_TRUE(rc == 0); + + destroy(mgr); +} + void test_group(void) { printf("\n>> group test\n\n"); xstring_table_init(139); - + RUN_TEST(test_all); RUN_TEST(test_empty); RUN_TEST(test_all_1); @@ -271,6 +310,7 @@ void test_group(void) RUN_TEST(test_exclude); RUN_TEST(test_nest); RUN_TEST(test_nest2); + RUN_TEST(test_reconfigure_same_group_members); xstring_table_fini(); }