feat(scheduler-core): implement REPL snapshot capture and simulation parameter commands (task 12.3)
Adds `snapshot [FILE]` and `set speed|output|verify ...`/`info config` (Req 5 AC 22 "Snapshots"/"Simulation parameters") to the interactive simulation REPL. `snapshot` reuses sim_record.c's existing sim_record_write_header_and_ snapshot() (task 11.5) rather than reimplementing capnp encoding, so the produced file is genuinely loadable via a fresh --sim invocation -- the task's own Observable. `set speed`/`set output`/`set verify` mutate fields sim_loop.c and core_clock.c already read fresh on every event/session, so no new hook points were needed and sim_loop.c stays untouched at exactly 800/800 lines (this project's hard per-file cap). New repl_params.c is a fourth sibling in the repl.c/repl_break.c/ repl_info.c split (task 12.2's precedent) -- neither existing file had headroom left under the 800-line cap for this task's own logic. Scheduler suite: 36/36 clean (was 35/35). Review independently reproduced the RED-phase probe and additionally verified the snapshot round-trip Observable is non-vacuous. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JPPXeKSUXUqcqvzNXY1rzJ
This commit is contained in:
parent
2fecea3592
commit
2329dbf2a8
18 changed files with 2755 additions and 110 deletions
|
|
@ -12,6 +12,9 @@
|
|||
- Keep test logs for analysis, don't skip intermediate output even if a test hangs
|
||||
|
||||
## Key Learnings
|
||||
- (2026-08-03, scheduler-core task 4.5, xce_core_now/tlog integration) Two spec-vs-reality mismatches resolved: (1) `test_core_isolation.c` (task 4.1) does a literal source-text scan of `core.c` ONLY for `"clock_gettime("`/`"time("`/`"gettimeofday("` with zero exception carve-out — so AC 3's mandated ONE sanctioned `clock_gettime(CLOCK_MONOTONIC)` call (inside `xce_core_now()`) CANNOT live in `core.c` without breaking that already-passing regression test; it must go in a sibling file (`core_clock.c`) that the isolation test never scans. This is a general pattern for this codebase: when a task's own AC deliberately re-introduces one instance of something an EARLIER task's regression test blanket-forbids, check whether that earlier test scans by FILE PATH (as this one does) rather than by symbol/module — if so, a new sibling file is the correct, low-risk resolution, not editing the old test's exception list. (2) requirements.md's own pseudocode for the tlog integration (`tlog_reg_format_func(fn, core_ptr)`) does not match the real vendored tlog.h API (`tlog_reg_format_func(tlog_format_func callback)` — ONE arg, no userptr-binding param; `userptr` is a hardcoded-NULL per-call-site argument threaded through `tlog_ext()`, per `utils.h`'s `tlog(level, format, ...)` macro). Resolved via a module-level `static _Atomic(xce_sched_core_t*) g_active_core_for_logging` set by a new `xce_core_set_active_for_logging()` setter, read by the registered format function instead of `info->time` — avoids rewriting every ERROR/WARN/INFO/TRACE call site scheduler-wide. Also: `xce_sched_core_init()`'s `memset(core,0,...)` leaves `speed_num`/`speed_den` at 0/1 (the FROZEN row), not the required Normal-mode 1/1 default — must explicitly `atomic_store_explicit` speed_num=1/speed_den=1 after the memset or a freshly-initialized core silently freezes instead of tracking real wall-clock time (would have been a silent behavior regression, not a build/test failure).
|
||||
- (2026-08-03, scheduler-core task 4.3, xce_core_push_events) `core.c`/`core.h`'s zero-controller-I/O boundary blocks reusing schedobj.c's `make_sched_job()`/`make_sched_queue()`/`make_sched_worker()` from core.c even though their bodies only ever touch `arg->core.*`: they take `void* arg` cast internally to `xce_scheduler_t*`, and calling them with a bare `xce_sched_core_t*` not actually embedded in a real `xce_scheduler_t` (as a unit test would build) is undefined behavior (foreign-type pointer reinterpretation), on top of core.c being forbidden from including scheduler.h to even name `xce_scheduler_t`. Fix pattern: write parallel core-scoped static helpers (`core_build_job`/`core_build_queue`/`core_build_worker`) that take `xce_sched_core_t*` directly and reuse the already-core-scoped accessors (`sched_get_user`/`sched_get_queue`/`sched_get_project`/`parse_resreq`) verbatim — same algorithm, not reinvented, just re-parameterized. Also: `sched_resolve_job_picker()` (schedobj.c) was changed from `xce_scheduler_t*` to `xce_sched_core_t*` and its extern declaration MOVED from scheduler.h to core.h (core.h needed `#include "plugin.h"` added for `xce_sched_plugin_t`) so a core.c-family file could call it without depending on scheduler.h — all 3 real call sites in scheduler.c and 9 in test_picker_wiring.c were trivially updated to pass `&sched->core` instead of `sched`, confirming this was the right call over duplicating the ~15-line resolution logic.
|
||||
- (2026-08-03, scheduler-core task 4.3, 800-line file cap) Implementing `xce_core_push_events()` (9 event types, per-type state mutation + picker hook dispatch) directly inside `core.c` pushed it from 407 to 977 lines, over this project's 800-line-per-file hard cap (CLAUDE.md/coding-style.md) even though design.md's File Structure Plan originally sketched it living in core.c. Split into a new sibling file `src/scheduler/src/core_events.c` (592 lines) instead — added to `src/scheduler/meson.build`'s source list, declarations stay in `core.h` (the public contract, unchanged), core.c reverted byte-identical to its pre-task state. For white-box tests that `#include "../src/core.c"` directly (test_core_isolation.c / test_core_jbuf_capacity_pin.c / the new test_core_events.c), a NEW file introducing a call to an as-yet-unlinked cross-file symbol (`sched_resolve_job_picker`, real impl in schedobj.c) breaks EVERY such test that includes core.c but not the new file/schedobj.c — must add/remove a matching stub in each. Lesson: when a task's own file is near the line cap, size out the FULL new function+helpers mentally (or write it, `wc -l`, and be ready to move it) BEFORE wiring it into shared headers/build files, since a same-file addition and a split-file addition have different declaration/stub footprints downstream.
|
||||
- (2026-07-28, bug-1479 ROOT-CAUSED not fixed) A permanent quorum-loss family member unrelated to admission control: when repl.c's `repl_broadcast` (live single-entry WAL tail-follow) gets an `ae_reject_logmismatch` NACK back from a standby (a genuinely NORMAL Raft outcome after rapid leader-election churn leaves a conflicting uncommitted tail entry at the same LSN with a different term — confirmed byte-precise via `xce_test waldump`: ctrl-2's own lsn=13 was its own deposed term=3 noop, while the canonical/surviving log has a DIFFERENT term=4 noop at the same lsn), NOTHING in ha.c's `repl_ack_decode_and_dispatch` NACK branch (`!success`) does anything but check for a higher observed term and then literally `WARN("...success=0 — ignoring")` — no `repl_on_ack` call (the ONLY site that decrements `pending_ack_count`), no nextIndex backoff-and-retry, no catch-up trigger. Consequence: `pending_ack_count` climbs 1-per-broadcast-attempt with zero decrements until `repl_broadcast`'s own `MAX_PENDING_ACKS=16` defensive eviction fires and marks the standby `in_catch_up=1` — which correctly excludes it from `repl_active_standby_count`/quorum, but NOTHING then drives an actual catch-up stream/snapshot-install for it (the only other `repl_complete_catchup` call site, config_repl.c's ACK-driven `catchup_lane_fn`, can never engage a standby that never sends a genuine ACK in the first place). Net effect: a standby with ONE conflicting log entry from election churn (routine, expected) becomes PERMANENTLY excluded from quorum with no self-healing path — in a 3-node cluster this is one unlucky election sequence away from total write-quorum loss. **General lesson**: when a symptom looks like "resource exhaustion" (admission cap, connection pool, whatever), always verify the actual counter's INCREMENT and DECREMENT sites are symmetric across EVERY reply outcome (success AND failure/reject), not just the happy path — a counter that only decrements on success but increments unconditionally will silently drift to its ceiling under ANY sustained failure condition, which looks identical to a genuine leak from the outside (repeated timeouts / repeated rejects) but has a completely different fix (handle the reject/failure path, not "find the missing release call"). Confirmed the ORIGINAL admission-slot-leak hypothesis for this bug was WRONG by checking `commit_resolve()`/`wait_release_locked()` — those DO release correctly on every timeout, 1:1 (17 timeouts, 17 releases) — before chasing the real mechanism.
|
||||
- (2026-07-28, waldump REPL usage) `xce_test` (built at `src/xce.build/controller/xce_test` or `src/build.dbg/controller/xce_test`) has no `waldump` CLI flag — `--help` only shows generic argp options. It's a REPL: pipe commands via stdin, e.g. `printf 'waldump <data_dir_or_wal_seg>\nquit\n' | ASAN_OPTIONS=detect_leaks=0 timeout 60 <path-to-xce_test>` (mirrors the `xtest()` shell helper in both `clean_failover_preservation.sh` and `run-ha-soak.sh`). Output columns are `LSN | TERM | TYPE | PLEN | PCRC | INFO` — the PLEN column is payload length, NOT a second type code (a WAL_CONFIG_MARKER shows as `UNKNOWN | 40 |` because waldump's type-name table has no case for type 50 yet — 40 there is the marker's 40-byte payload length, not its wire type number). This is a fast, byte-precise way to independently confirm/refute term or index divergence between two members' WALs without instrumenting production code.
|
||||
- (2026-07-27/28, bug-1475 FIXED) A term/version index that grows its backing array geometrically and zero-fills the newly-grown region to mean "absent" has a subtle, TIME-DEPENDENT bug: zero-filling with the same sentinel value used for a legitimate "recorded as literal 0" entry makes a deliberately-never-recorded slot (here: persist.c's per-LSN term_index, where a WAL_CHECKPOINT's own LSN is BY DESIGN never passed to record_entry_term) silently and RETROACTIVELY flip from "absent" to "present, value 0" the instant ANY later index is recorded and the array-growth watermark (`term_index_count`) crosses it — even though nothing ever explicitly wrote to that slot. Two readers that check the SAME never-recorded slot at DIFFERENT times (e.g. a sender that reads it AFTER appending later entries, vs. a receiver that reads it BEFORE appending anything past it) get DIFFERENT answers for what should be a stable, position-only fact — this produced a permanent AppendEntries logOk rejection (bug-1475: sender computed prev_log_term=1 (WAL_LEGACY_TERM), receiver computed prev_log_term=0 (absent), for the identical checkpoint LSN). **General fix pattern**: reserve a DIFFERENT sentinel (here `TERM_INDEX_UNSET = UINT64_MAX`, filled via `memset(...,0xFF,...)` on array growth) distinct from every legitimate stored value, so a never-explicitly-written slot reads "absent" PERMANENTLY, independent of later growth — never let "grown but unwritten" collide with a real, meaningful value (here, literal term 0 / WAL_LEGACY_TERM) in the same sentinel. **Do-not-repeat**: when hand-tracing (not instrumenting) a bug involving an in-memory index that has a documented "grows on demand, gaps read as absent" contract, don't trust that the gap actually STAYS absent across the FULL lifetime of the process/test — verify with real instrumentation whether "absent" is a stable property of the position or merely a snapshot of "not yet grown past." This was found only after two prior, plausible-looking production fixes (config_repl.c's tracker-advance + ha.c's JIT log_len resync, both already landed) still left the regression test RED — real fprintf instrumentation at the streamer, the JIT resync, and raft.c's logOk check (all added and fully removed afterward) pinned the exact moment prev_log_term flipped from 0 to 1. See `.wolf/buglog.json` bug-1475 for the full three-part fix and `test_bug1475_catchup_survives_midstream_checkpoint` (test_config_repl.c) for the regression proof.
|
||||
|
|
@ -125,6 +128,9 @@
|
|||
- (2026-06-22, ha-term-safety task 3.3 remediation) Do NOT run `clang-format -i` on whole XCE controller source files (ha.c/ha.h/test files): these files are NOT clang-format-clean against the repo's config, so a full reformat rewrites ~1700 lines and BURIES the targeted change (and violates a "preserve the existing diff" instruction). I then made it worse with `git checkout HEAD -- <files>` to undo the format — but the uncommitted task-3.3 work lived ONLY in the working tree, so that wiped it (no stash, unrecoverable from git). Recovery worked only because the full original content was already in my context. Lesson: (1) only format the exact changed regions or skip formatting; (2) NEVER `git checkout HEAD -- <file>` to discard formatting noise when the file also holds uncommitted feature work — instead revert just the format hunks, or stash first. The reflog showed branch HEAD was already at task-3.2 (754e586), and the initial-context "recent commit 2c5d298" was a stale snapshot — the uncommitted 3.3 diff sat on top of 3.2.
|
||||
- (2026-06-22, ACK peer-index, bug-960) The walAck branch of `repl_ack_decode_and_dispatch` (ha.c) fires for TWO endpoint types whose stored `peer_idx` means DIFFERENT things: promoted-vote eps (`vote_ep_ctx_t`, `ctx!=NULL`) carry a self-EXCLUDED *votes* index (relative); heartbeat standby eps (`standby_ep_ctx_t`, `ctx==NULL`) carry an ALREADY-ABSOLUTE candidate index. The authoritative branch is ha.c:6187-6189 (`sctx->ctx != NULL ? votes_idx : i`). The pre-existing ACK consumer `ha_walack_on_ack(c, vctx->peer_idx, …)` uses `peer_idx` DIRECTLY (no remap) — proof the ACK path's index is already absolute. Any code consuming the ACK `peer_idx` for the library (match_index[]/next_index[], which raft.c writes UNCHECKED) MUST branch on `vctx->ctx != NULL` before remapping, or it double-maps the heartbeat path → wrong-peer false quorum / OOB write. `self_idx == ha->my_priority` (raft_cfg.self_idx = my_priority everywhere).
|
||||
|
||||
- (2026-08-05, scheduler-core task 9.2, bug-1498) When hardening an `open()` call against a TOCTOU symlink/hard-link swap with `O_NOFOLLOW` + a post-open `fstat()`+`st_nlink==1` check, NEVER also pass `O_TRUNC` in that same `open()` call. `O_TRUNC` truncates atomically as PART of the `open(2)` syscall itself, before any userspace check can run — against a hard-linked victim (same inode, no symlink involved, so `O_NOFOLLOW` never fires), the file gets zeroed the instant `open()` succeeds, and the *correct* nlink check that follows only closes the fd after the damage is already done. Fix: open WITHOUT `O_TRUNC`, run the safety check, and only call `ftruncate(fd, 0)` once verified safe. Caught by a TOCTOU regression test that asserted the OUTSIDE victim file's content was untouched (not just that the return code was negative) — a bare "rc < 0" assertion would have missed this entirely, since the hardened sequence DID correctly reject the operation, it just did so after already corrupting the aliased file. **Lesson: when writing a TOCTOU/symlink-hardening open sequence, treat any destructive open flag (`O_TRUNC`, and by the same logic `O_EXCL`'s interaction with `O_CREAT` deserves the same scrutiny) as happening BEFORE your post-open safety check, not after — and when testing such a defense, always assert on the side-effect-free state of anything the attack could alias, not just the function's return code.**
|
||||
- (2026-08-05, scheduler-core task 9.2, bug-1499) When using the Edit tool to insert new code between two existing statements in a function, double-check that the LAST line of `old_string` (the line immediately preceding the insertion point) is preserved verbatim in `new_string` — it is easy to accidentally let it get silently swallowed by the replacement text, especially when the new block being inserted is long and multi-paragraph. Dropped `xce_core_signal_go(&(sched->core));` this way while inserting a recording-hook block into `xce_sched_driver_run_normal_mode_round()` (driver.c) → a 3-way deadlock across the picker thread (blocks on `cbuf_get(&core->wbuf)` forever), the stand-in allocator thread (never gets a jobid), and the main thread (`cbuf_get(&core->decisions)` never returns) — surfaced only as a 30s `meson test` TIMEOUT with no further diagnostic, and only in the ONE test file (`test_driver_session_ordering.c`) that actually drives a real picker/allocator thread pair through this function; every other white-box test in the suite stayed green because they never reach that rendezvous. **Lesson: after any Edit that touches a function with cross-thread rendezvous/signaling calls (`cbuf_put`/`cbuf_get`/condvar signals/etc.), diff the function's FULL body against its pre-edit form (not just the hunk you intended) before trusting the edit — a single accidentally-dropped signal call produces a silent deadlock with no compiler warning and no error message, only a hang.**
|
||||
- (2026-08-07, scheduler-core task 11.4, bug-1501 — RECURRENCE of bug-1493) Writing prose inside a `/* ... */` doc comment that contains an adjacent `*` followed by `/` (e.g. "xce_job_alloc_t*/xce_pendreason_t*" meaning "a pointer of type A or of type B") silently CLOSES the comment early — the C preprocessor has no concept of "still inside a comment, this looks like code now", so everything after the accidental `*/` up to the next real `*/` gets parsed as C, producing a cascade of nonsense syntax errors far from the true location. Caught immediately by the first build attempt (gcc pointed at line 59, the true bug), not by review. **Lesson: never write a bare `*/` sequence inside a block comment, even when it reads naturally as "pointer-of-A slash pointer-of-B" — always insert a word ("or") or a space between the two types instead. Before debugging a confusing cascade of syntax errors in a freshly-written/edited C file, grep that file for `[A-Za-z_]\*/[A-Za-z_]` FIRST — it is faster than reading the compiler's misleading downstream error trail.**
|
||||
- (2026-06-24, bug-1060 — RECURRENCE of the 2026-06-22 entry above) Ran `clang-format -i` on whole ha.c/ha.h/test_ha_election.c AGAIN → spurious whole-file reformat (3268/969/635 lines) that buried a ~180-line fix. Confirmed spurious by checking the repo's HEAD ha.c is NOT idempotent under the nix `clang-format` (a `.clang-format` exists at src/.clang-format but the pinned formatter version differs from whatever produced HEAD). Recovery THIS time succeeded because: (a) I had already printed the full pre-format `git diff ha.c` (5 hunks: bug-1028/1042/1043 pre-existing + my bug-1060) into context; (b) ha.h's only genuine new symbol vs HEAD was `ha_sync_raft_log_from_recovery` (found via `comm` on `grep -oE '\bha_[a-z_]+\b'` of HEAD-vs-current — everything else was pointer-style/comment reflow); (c) test_ha_election.c was NOT in the session-start `M` list so it held only my changes. Then `git checkout HEAD -- <3 files>` + re-applied the logical hunks by Edit. LESSON STANDS: do NOT clang-format whole files here; format only changed regions or skip. The `git checkout HEAD --` rescue is ONLY safe after you have positively accounted for EVERY uncommitted change in those files (capture the pre-format `git diff` FIRST, and confirm which files had pre-existing work via the session-start `git status`).
|
||||
|
||||
<!-- Mistakes made and corrected. Each entry prevents the same mistake recurring. -->
|
||||
|
|
@ -603,5 +609,112 @@ Ran `/xce-build all` (backgrounded) in this session while at least two OTHER Cla
|
|||
## Do-Not-Repeat (2026-07-31) — a Bash `run_in_background` task inside a background *subagent* invocation may never deliver its "you'll be notified when it completes" notification; don't wait passively on it
|
||||
As a subagent (spawned via the `Agent` tool, running in the background from the parent's perspective), a `Bash` call with `run_in_background: true` did not visibly notify this session on completion even after several idle turns — a peer/coordinator message eventually had to point out "you're a subagent, you don't get woken up by background task notifications, check `ps aux` and wait blocking instead," which held up on inspection (the process really had died, torn out by the concurrent-build race in the entry above). **Do-not-repeat**: when running as a background subagent (not the top-level session), do NOT rely on Bash background-task completion notifications to resume work — either (a) run the long command in the foreground with a generous explicit `timeout` (up to 600000ms), or (b) if backgrounding is unavoidable (command may exceed 10 min), actively self-poll with a bounded `while kill -0 $PID; do sleep N; done` foreground Bash call rather than idling and waiting for a push notification that may never arrive in this execution context.
|
||||
|
||||
## Key Learning (2026-08-02, scheduler-core task 2.1) — every `xce_job_picker_iface_t` hook's `void* picker` argument is the owning `xce_sched_plugin_t*`, not a picker-private handle — confirmed by the already-committed legacy wrapper, not just inferred
|
||||
`xce_job_picker_iface_t` (plugin.h) never says what `void* picker` actually points to. The job_elect_fn compatibility wrapper (`plugin.c`'s `legacy_picker_wrapper_iface`, task 1.2) FORCES the answer for the legacy-wrapped case: `legacy_picker_next_job()` casts `picker` straight to `xce_sched_plugin_t*` to reach `plugin->job_elect_fn`, and `test_plugin_picker_registration.c` confirms the call site passes `&plugin` (`plugin.picker_iface->next_job(&plugin)`). Since the eventual picker-thread driving code (tasks 2.4/2.5, group 4) must call every hook through ONE uniform site regardless of whether the registered picker is legacy-wrapped or native, `picker` == `xce_sched_plugin_t*` for EVERY picker, not just legacy ones — there is no other value that works for both. `builtin_fcfs_init`/`builtin_fcfs_fini` (task 2.1, `plugins/builtin_fcfs.c`) therefore store their private `fcfs_plugin_data_t*` in `plugin->data` (the existing `plugin_data_t data` field on `xce_sched_plugin_t`), exactly mirroring today's old-style `fcfs_init`/`fcfs_fini` (`plugins/fcfs.c`) convention — NOT a new dedicated field, NOT the private struct itself as the handle. **Do-not-repeat**: for tasks 2.2/2.3 (on_queue_update, on_job_push, next_job, etc.), don't re-derive this from scratch — every hook's `void* picker` first argument is `(xce_sched_plugin_t*)picker`, and `((xce_sched_plugin_t*)picker)->data` is where `fcfs_plugin_data_t*` lives.
|
||||
|
||||
## Key Learning (2026-07-31, bug-1487) — a "committed job loss" in `--chaotic` xce-test-ha soaks can be a false positive caused by the Raft own-term commit barrier, not real data loss — always check `verify_classify`'s timing before touching controller code
|
||||
Root-caused a `committed_job_loss=1` chaotic-soak FAIL entirely from logs/timestamps, no code change to `src/controller/`. Mechanism: a newly-elected primary cannot fold a PRIOR-term entry into its own `committed_lsn` watermark just because it holds it durably (Raft Figure-8 safety; `ha_arm_own_term_gate` in `ha.c` — every promotion appends an own-term `WAL_NOOP` and only advances `committed_lsn` past the inherited prefix once THAT noop itself gets a fresh majority ack under the new term). `expect-job` reads `c->jobmgr`, populated only by the apply pump once entries cross `committed_lsn` — so a genuinely majority-committed job can be transiently invisible on the brand-new primary until that fresh ack lands. In steady state this closes in <200ms, but `--chaotic` mode's own "restart the previous round's downed node with no convergence wait" design means the peer whose ack is needed can itself be mid-catch-up, stretching the window to several seconds (measured live: 6.232s). `run-ha-soak.sh`'s chaotic-mode `verify_classify()` ran its `expect-job` check immediately on primary detection with zero settle/retry (unlike `verify_no_loss`, which already had a bug-1422 3-attempt/2s retry, and unlike `wait_for_full_convergence`, which chaotic mode deliberately skips between kills). Fix was a bounded retry in `verify_classify` alone (5×2s), never touching `src/controller/`. **Do-not-repeat**: before treating any xce-test-ha `committed_job_loss` count as a real controller bug, first check (a) whether the SAME job shows up as present in every LATER verify/final-sweep in the same run (a real loss never comes back; this class of false-positive always does), and (b) whether `verify_classify`/whichever check flagged it has an equivalent settle/retry to its sibling `verify_no_loss` — chaotic and sequential modes have historically drifted out of sync on this (bug-1422 fixed sequential only; bug-1487 caught chaotic's matching gap 3 weeks later).
|
||||
|
||||
## Key Learning (2026-08-03, scheduler-core task 4.1) — extracting `xce_sched_core_t` from `xce_scheduler_t`: embed the new struct as a NAMED first member (`xce_sched_core_t core;`), never anonymous-union it in. `xce_scheduler_t` now holds `{ xce_sched_core_t core; xcecli_ctx_t apictx; xce_cluster_config_t cluster_cfg; }`, matching design.md's own future `xce_sched_driver_t` shape (same `core` field name). This makes the diff mechanical but large: every `sched->FIELD` where FIELD moved into core becomes `sched->core.FIELD` across scheduler.c/schedobj.c/resource.c/handler.c/plugin.c/main.c/plugins/*.c, and every accessor whose signature changed to `xce_sched_core_t*` (sched_get_job/add_job/get_queue/add_queue/get_worker/add_worker/get_user/add_user/get_project/add_project) needs its call sites updated to pass `&sched->core`. `xce_sched_core_t` needs its OWN `typedef struct xce_sched_core_s xce_sched_core_t;` inside core.h itself — don't rely solely on plugin.h's forward declaration, since include order (scheduler.h pulls in core.h BEFORE plugin.h) means the forward decl may not be visible yet. An identical repeated typedef is valid C11/gnu17 (same idiom as this codebase's `cluster_t`).
|
||||
|
||||
## Do-Not-Repeat (2026-08-03, scheduler-core task 4.1) — a whole-file `Write` rewrite of a large existing .c file (rather than targeted `Edit` calls) risks silently DROPPING a function that has nothing to do with the change (bug-1490: `reset_sched_obj()` vanished from a schedobj.c rewrite, only caught by a link error in `plugins/limit.c`). After any whole-file rewrite, diff the new file's function/symbol list against the original's before considering the file done — don't rely on the compiler/linker alone to catch it (a missed *declaration-only* drop, e.g. a header prototype, would not even produce a link error).
|
||||
|
||||
## Key Learning (2026-08-03, scheduler-core task 4.1) — white-box tests that `#include "../src/X.c"` directly and stub scheduler-internal externs are the dominant test pattern in `src/scheduler/testsuite/`; any signature change to a relocated/shared function (e.g. `sched_get_job`) must be propagated to EVERY test file's local stub/duplicate-implementation of that function, not just the production call sites — the compiler only catches a mismatch if the stub's declaration conflicts with a header-declared prototype already visible in that TU (grep for the old signature across `testsuite/*.c` before declaring a relocation done).
|
||||
|
||||
## Key Learning (2026-08-03, scheduler-core task 4.1) — proving "this file links zero symbols from library X" for an existing monolithic build (one big `libxce.a` bundling everything) is NOT achievable by checking header includes alone, since `xce/types.h` (included by nearly every scheduler file) itself pulls in `netio.h` for unrelated reasons. The real, build-time-enforceable proof is: compile the target file ALONE into its own `static_library()` (meson), then `nm --format=posix -u` that archive and assert its undefined-symbol table contains no forbidden-prefixed names — this reflects what the code actually CALLS, independent of what any header transitively declares. `link_with:` the isolated static_library into the test executable so meson tracks the build-order dependency even though the test never calls its symbols.
|
||||
|
||||
## Key Learning (2026-08-03, scheduler-core task 4.3 round 2) — `xce_core_push_events()`'s CONFIG_PUSH case must decompose the pushed `xce_conf4sched_t`'s embedded `resources[]`/`resvals[]`/`queues[]` arrays into core state, not just store the config pointer + bump `configver`. Round-1 implementation only did the latter; round-1 review REJECTED it because `scheduler.c`'s still-live `sched_init_config()` (called for this exact same payload type) always calls `sched_init_resources()`/`sched_init_sharedInst()`/`sched_init_queues()` on top of `plugin_config_update()`. Fix added `core_apply_config_resources()`/`core_apply_config_resvals()` (new, mirroring the scheduler.c originals exactly, parameterized on `xce_sched_core_t*` directly) and a shared `core_upsert_queue()` helper reused by BOTH the CONFIG_PUSH per-queue loop and the pre-existing QUEUE_UPDATE event case (previously QUEUE_UPDATE had its own inline sched_get_queue-or-build-and-add logic; factored out to avoid a third copy) — this makes CONFIG_PUSH's queues literally behave as batched QUEUE_UPDATE events, which is the documented Req 5 AC 4 design intent ("added or modified"). **Do-not-repeat**: when a core event type's payload type (`xce_conf4sched_t`, `xce_worker_t`, etc.) is ALSO consumed by an existing production code path elsewhere in the codebase, diff the new core-event handler's per-field behavior against that existing path's handling of the SAME struct field-by-field before considering the event handler complete — a "looks complete" handler that only touches the top-level fields (here: just `config`+`configver`) while ignoring nested arrays the existing path DOES decompose is exactly the kind of gap a fresh review catches instantly but an implementer close to the code can miss.
|
||||
|
||||
## Key Learning (2026-08-04, scheduler-core task 7.1) — `controller.capnp`'s `$C.codecgen` (file-level annotation) generates `encode_X`/`decode_X` functions for EVERY struct, and every one of those functions references the struct's `$C.mapname` target type by NAME in the generated `.c`/`.h` — that C type MUST already be declared (typically in `typemap.h`, pulled in via `$C.extraheader`) or the generated code fails to compile with "unknown type name". Verified empirically (built a scratch schema + ran the real `capnp compile -o c:...` + `gcc -c`): a struct WITHOUT `$C.mapname` doesn't dodge this — it just falls back to an auto-derived default name (`struct <StructName>_`) that is EQUALLY undeclared unless you supply it yourself. There is no `$C.nocodecgen`/opt-out annotation. **Practical rule for adding a new placeholder/stub struct to this schema**: give it `$C.mapname` and a matching (even if minimal/single-reserved-byte) C struct in `typemap.h`, same "frame-only, no live struct" convention already used for `capnp_snap_hdr_t`/`capnp_host_admin_state_t` — do NOT try to skip the C-side type just because the capnp struct itself has zero real fields yet.
|
||||
|
||||
## Key Learning (2026-08-04, scheduler-core task 7.1) — a `Data` (raw-bytes / `List(UInt8)`) capnp field's generated `encode_X`/`decode_X` codec function body is a confirmed NO-OP in this codebase's capnpc-c setup (verified by reading the actual generated `decode_UpgradeConfigRequest` — literally just a comment, no assignment). Every existing `Data` field in `controller.capnp` (`sha256`/`bytes`/`payload` on `UpgradeConfigRequest`/`ReplConfigPushRequest`/`SnapshotTransfer`/`WalCatchupEntry`) is ALWAYS populated by hand-written raw capnp-C-API code (`capn_new_list8`+`capn_setv8`, e.g. `request.c`'s `xce_replconfigpush_req_raw`) into a MEMSET-ZEROED native `struct X` local, never through the convenience `encode_X_ptr(cs,&p,&c_struct)` wrapper — that wrapper allocates its native `struct X d;` UNINITIALIZED and the no-op codec never fills the Data field, so `write_X(&d,ptr)` would write GARBAGE stack memory as a capnp pointer. Any new struct with a Data field (e.g. `ReplayHeader.configHash`) must follow the same pattern: either build it via low-level accessors (`new_X`/`X_set_dataField`) or manually memset+populate the native struct before `write_X`, never the bare `encode_X_ptr` convenience path.
|
||||
|
||||
## Key Learning (2026-08-04, scheduler-core task 7.1) — a nested-struct-typed field's generated getter (`Parent_get_child(p)`) calls `capn_getp(p.p, idx, 0)` with `resolve=0`, so it can return an UNRESOLVED far pointer (`.p.type == CAPN_FAR_POINTER == 5`) after a real segment→bytes→segment round-trip. Per-field accessors that index raw struct offsets (`capn_read64`/`capn_read32`, used by every plain-scalar `X_get_field`) do NOT auto-resolve it and will silently read zeros/garbage — unlike `capn_get_text`/`capn_get_data`, which DO resolve internally, so Text/Data fields "just work" and mask the bug. **Rule**: after getting ANY nested-struct pointer from a generated accessor, call `capn_resolve(&ptr.p)` before touching that struct's own scalar field accessors. Whether the far-pointer case actually triggers seems to depend on segment layout/allocation order (a small enough test can accidentally stay near-pointer and pass without the resolve call, which is exactly what happened here on the first pass — `test_replay_header_snapshot_entry_round_trip`'s `ReplayCluster` nested struct happened to pass without it, while the very next test's `SessionSummary` nested struct came back all-zero without it — same bug, different luck).
|
||||
|
||||
## Do-Not-Repeat (2026-08-04, scheduler-core task 7.1) — `typemap.h` and `test_e2e_capnp.c` are NOT clang-format-clean at baseline (hundreds/dozens of pre-existing `-Wclang-format-violations` even before any edit in this session, confirmed via `clang-format --dry-run --Werror` against the pre-edit file). This project's actual house style for these files is hand-aligned columns that `.clang-format`'s webkit-based config doesn't reproduce, and no build/test gate enforces clang-format compliance on them (the full `meson compile`/`meson test` run is clean either way). Don't waste a cycle trying to reformat new additions to strict clang-format output in these files — match the surrounding hand-aligned style instead, which is what the codebase actually treats as canonical.
|
||||
|
||||
## Key Learning (2026-08-04, scheduler-core task 7.2) — when extending a hand-aligned `:union` block (capnp) or matching C `union` (typemap.h) with new members whose ordinal/name is wider than the existing column width, do NOT reflow/realign the pre-existing member lines to make columns line up — that turns every existing line into a diff hunk (looks like a rewrite, not an addition) even though only whitespace changed. Leave the existing lines' bytes untouched and let the new, wider members be locally mis-aligned with the old column; `git diff` then shows a clean pure-addition (verified: `git diff` on both `controller.capnp`'s `ReplayEvent.body` and `typemap.h`'s `replay_event_t.body` showed 0 changed bytes on any pre-existing line after reverting an initial column-realignment pass). This matters most for schema files where reviewers explicitly gate on "existing ordinals/fields must be byte-identical."
|
||||
|
||||
## Key Learning (2026-08-04, scheduler-core task 7.2) — for a NEW capnp struct being added to `controller.capnp` that reuses an EXISTING nested struct with real substructure (here: `JobUpdateEvent.job: Job`, reusing the pre-existing `xce_job_t`-backed `Job`), the codec bridge (`encode_X_ptr`/`decode_X_ptr`/`free_X_ptr`, generated by `$C.codecgen`) recurses correctly through arbitrarily deep nested-pointer and list-of-pointer fields (`Job.submitData`→`JobSubmitData`, `Job.allocs`→`JobAlloc`→`List(HostAlloc)`→`List(ResAlloc)`, etc.) with ZERO hand-written accessor code needed — confirmed by reading the actual generated `encode_Job`/`decode_Job`/`decode_HostAlloc_list` bodies, which chain `encode_<Child>_ptr`/`decode_<Child>_ptr` calls automatically, and every generated `decode_<Type>_ptr` helper already calls `capn_resolve()` internally (unlike the raw low-level accessor path documented in task 7.1's far-pointer gotcha above). **Rule of thumb**: use the codec bridge (Test 6's `test_data_job_array_variant_round_trip` pattern) whenever the new/reused struct has a real backing C struct and no `Data` field anywhere in its transitive closure; reserve the low-level accessor API (Tests 7-8's pattern) for frame-only types with no live C struct yet, or anything touching a `Data` field (task 7.1's no-op-codec gotcha).
|
||||
|
||||
## Do-Not-Repeat (2026-08-04, scheduler-core task 7.3) — never write `TypeA*/TypeB` (a pointer-star immediately followed by a slash) inside a C `/* */` block comment. `typemap.h`'s prose "the same raw uint8_t*/size_t pattern" silently closed the comment early at the `*/` in `uint8_t*/size_t` — everything after (rest of the comment, the struct's opening brace) got parsed as real tokens, producing ~20 cascading "expected declaration specifiers" errors across `typemap.h`/`controller.capnp.h`/`controller.capnp.c` that look nothing like the actual root cause at a glance. Logged as bug-1493. When a struct/type suddenly "doesn't exist" right after a comment edit, grep that comment for a bare `*/` mid-word before suspecting anything else.
|
||||
|
||||
## Key Learning (2026-08-04, scheduler-core task 7.3) — when design.md's literal `$C.mapname` target for a NEW capnp struct names an EXISTING C type that lives in a component-private header (e.g. `xce_sched_user_t`/`xce_sched_project_t` in `src/scheduler/include/sched/types.h`, scheduler-only) rather than a shared header `typemap.h` can reach, do NOT reuse that exact name as the mapname target even though the field lists match perfectly. `typemap.h` is compiled into every binary linking `controller.capnp` (controller/apiserver-CGO/scheduler/xceadmin/worker), so the mapname target type must live somewhere `typemap.h` includes (`xce/types.h` or `typemap.h` itself) — but the scheduler-private header in question ALREADY `#include`s `typemap.h` (confirmed: `core_events.c`/`schedobj.c` include both in one TU), so adding a same-named typedef to `typemap.h` is a GUARANTEED duplicate-typedef compile error the instant both headers meet in one translation unit, not a hypothetical risk. Resolution: mint a fresh, differently-named C type in `typemap.h` (here `sched_snapshot_user_t`/`sched_snapshot_project_t`) with an IDENTICAL field layout, and document the deviation prominently in both the schema comment and typemap.h comment (field lists still match design.md's literal spec; only the C type name differs). This is the SAME pattern this task already used deliberately for `PreemptVictim`/`PreemptDecision` (fresh mapnames, distinct from decision.h's core-internal `xce_preempt_victim_t`/`xce_preempt_t`) — treat "component-private header defines the type design.md wants to reuse" as an automatic trigger for this pattern, not something to special-case per-type.
|
||||
|
||||
## Key Learning (2026-08-04, scheduler-core task 7.3) — a struct with exactly ONE `Data`-typed field mixed among many normal (scalar/Text/nested-struct/List-of-mapname'd-struct) fields can still safely use the full `encode_X_ptr`/`decode_X` codec bridge for ALL the normal fields (confirmed by reading the generated `encode_SnapshotData`/`decode_SnapshotData` bodies: every non-Data field gets a real assignment or recursive `encode_<Child>_ptr` call; the Data field line is literally just `/* internalState internal_state */`, a silent no-op comment, not a type-mismatch compile error as originally feared). The safe hybrid: call `encode_X_ptr(cs,&p,&native)` once to populate everything else (including recursively encoding nested `List(MapnameStruct)` fields straight from native `T**`+`int count` pairs — no manual list-building needed), THEN separately call the low-level `X_set_dataField(p, capn_data)` on the SAME already-allocated struct pointer `p` to fill in just the Data field (mirrors task 7.1's config_hash technique, but composed with the codec bridge instead of replacing it entirely). This is strictly less code than building the whole struct via low-level accessors just because one field can't use the bridge.
|
||||
|
||||
## Key Learning (2026-08-04, scheduler-core task 8.1) — a project's 800-line-per-file cap can be respected by a 3-way split when a 2-way split can't get either resulting file under the cap: `snapshot.c` (1076 lines: xce_sched_core_save_snapshot/xce_sched_core_load_snapshot + a ~290-line shared design-decision doc comment covering BOTH directions) was split into `snapshot.c` (save + the full shared doc comment, 621 lines), `snapshot_wire.c` (small helpers + SAVE-direction wire builders — matched the file's OWN pre-existing internal section-comment boundaries exactly, 258 lines), and `snapshot_load.c` (load + load-specific "picker rebuild" doc subsection, 335 lines). Cutting along the file's own existing `/* ---- Section Name ---- */` dividers (rather than an arbitrary line-count chop) is what made all three resulting files independently cohesive. Cross-file helper sharing used the SAME idiom `core_session.c` already established for a single symbol (`extern void sched_worker_release_host_ctx(...)`, redeclared at each consumer rather than a new private header) — but a shared MACRO constant (here `SCHED_SNAPSHOT_INTERNAL_VERSION`, needed by both save's writer and load's version-check reader) can't be `extern`-declared like a function, so it moved into the existing shared umbrella header (`core.h`) instead of being duplicated (duplicating a versioning constant risks silent drift between the two copies).
|
||||
|
||||
## Key Learning (2026-08-04, scheduler-core task 8.1) — test files in `src/scheduler/testsuite/` are NOT held to the same 800-line hard cap as production `.c` files: `test_picker_fcfs.c` (801 lines) and `test_driver_translate.c` (820 lines) are both pre-existing, committed, already over 800 — confirmed precedent that a cohesive single-Observable test file (all fixtures/stubs/the-one-big-test-function in one white-box TU) is accepted here even past the nominal cap, unlike production `.c` files where task 4.3's core_events.c split was enforced strictly. `test_snapshot_save_load.c` (846 lines) follows this test-file precedent rather than being force-split.
|
||||
|
||||
## Key Learning (2026-08-05, scheduler-core task 9.3) — reliably forcing a REAL (not simulated-return-code) `fwrite()` failure against a `FILE*` whose underlying fd was closed out from under it requires disabling stdio buffering FIRST: a plain `close(fileno(f))` alone is not enough, because a small `fwrite()` on a still-fully-buffered stream just copies bytes into the userspace buffer and reports success without ever reaching the (now-invalid) fd — the failure would only surface later, on the next `fflush()`/`fclose()`/buffer-full write, not at the `fwrite()` call site production code actually checks. The reliable sequence (used in `test_record_lifecycle.c`'s new write-failure tests): (1) `fflush(f)` to commit any already-buffered bytes to the still-good fd, (2) `setvbuf(f, NULL, _IONBF, 0)` to force every subsequent `fwrite()` to hit the syscall directly, THEN (3) `close(fileno(f))`. After this sequence, the very next `fwrite()` fails immediately and visibly. `fflush()`/`fclose()` on the now-broken stream afterward (e.g. inside a cleanup/abort handler) are themselves safe no-ops-that-report-EBADF, not crashes — glibc's `fclose()` frees the `FILE*`'s own heap state regardless of whether the underlying `close(fd)` syscall it issues succeeds.
|
||||
|
||||
## Key Learning (2026-08-05, scheduler-core task 9.3) — to test that ONE SPECIFIC write call inside a multi-write function fails while EARLIER writes in the same call succeed (e.g. proving a periodic-checkpoint write's own failure path, distinct from the per-event writes preceding it in the same `sched_record_write_events()` call), fd-breaking tricks can't discriminate between sub-writes within a single call — once the fd is broken, ALL subsequent writes in that call fail, not just the target one. Instead, find an independent precondition the LATER call alone depends on and flip only that: here, `xce_sched_core_save_snapshot()`'s documented `core->session_active == 0` precondition (core.h) is checked ONLY by the snapshot/checkpoint path, never by the plain per-event `ReplayEvent` writer, so setting `sched.core.session_active = 1` right before the triggering call makes exactly the periodic-checkpoint's `write_snapshot_entry()` fail (`-EBUSY`) while the preceding event write in the same call succeeds normally — a clean, deterministic, fd-independent way to isolate one sub-path's failure.
|
||||
|
||||
## Do-Not-Repeat (2026-08-04, scheduler-core task 8.1) — an `xce_val_t` constructed by hand (not through a real wire-decode path) MUST have `.which` set explicitly to the correct `XCE_VAL_*` tag before it can safely cross `dup_val()` (or any code that branches on `.which`) — `XCE_VAL_STRING` is literally `0` (`xce/types.h`), so a `memset`-zeroed `xce_val_t` with only `.u.ivalue32` set defaults to `.which == XCE_VAL_STRING`, and `dup_val()` will reinterpret the raw int32 bits as a `char*` and crash inside `makeString()`/`hash_string()` (SEGV at the literal small-integer address). `test_core_events.c`'s own fixtures get away with never setting `.which` only because their resval path (`core_apply_config_resvals`) does a raw `MOVE_PTR`, never branching on `.which` — that convention is NOT safe to copy into any test that actually exercises a `.which`-branching consumer like `dup_val()` (task 8.1's own `snapshot_wire.c`).
|
||||
|
||||
## Key Learning (2026-08-04, scheduler-core task 8.1) — `sched_get_user()`/`sched_get_project()` (`core.c`) had a real, pre-existing (tasks 4.1/4.3 era) get-or-create bug: both did `rc = key_find(...); if (rc < 0) return NULL;` before falling through to the create fallback, but `key_find()`'s actual "not found" signal is a NEGATIVE errno (`-ENOENT`, confirmed via direct instrumentation: `rc=-2` for absent, `rc=1` for present) — not `rc==0` as the surrounding code assumed. This made get-or-create unreachable for any genuinely new name, silently leaving `job->user`/`job->project` NULL for every job referencing a brand-new user/project (in BOTH the live `core_events.c` JOBS_PUSH path and the still-live legacy `schedobj.c` path), completely unnoticed because no existing test asserted `job->user`/`job->project` non-NULL for a freshly-referenced name. Caught by task 8.1's own cross-core equivalence test (RED phase). Fixed by narrowing the guard to `if ((rc < 0) && (rc != -ENOENT))` in both functions — logged as bug-1495. **Do-not-repeat**: when wrapping a `key_find()`-style lookup with get-or-create semantics in this codebase, verify the library's actual not-found return value empirically (small debug instrumentation) rather than assuming `rc==0` — this same wrong assumption was independently baked into two sibling functions.
|
||||
|
||||
## Key Learning (2026-08-05, scheduler-core task 9.4 fix round) — a vtask-worker-thread reader of `xce_scheduler_t` recording fields (`sched_record_status()`) races the single-threaded driver loop's writers (`sched_record_start`/`sched_record_stop`/`sched_record_write_events`/`sched_record_abort_on_error`) with zero prior synchronization; fixed by adding a NEW dedicated `mylock_t record_lock` field (not widening `core.lock`, since `core` vs driver-level state is a deliberate architectural boundary per this spec's own design) initialized in `sched_init()`/destroyed in `sched_fini()`, taken only around the struct-field reads/writes themselves (never across `fopen()`/`fwrite()`/`capn_write_mem()` I/O). Do-not-repeat pattern for future admin/status read paths in this codebase: a synchronous "status" reply that borrows a live heap pointer (`reply->file = sched->record_file_path`) into a struct that outlives the current call's lock scope is unsafe the moment ANY other thread can mutate that pointer — the safe idiom is `strdup()` the string's bytes WHILE HOLDING the lock, hand back a caller-owned copy, and document (both the header comment and the call site) that the caller must `free()` it. This differs from the same-stack-frame "borrow a local buffer" idiom `handle_deferred_admin_record()` uses for recordStop's `last_path` (driver.c) — that idiom only works when the snapshot and the eventual consumer share one stack frame; sched_record_status()'s caller lives in a different function (handler.c), so a stack-local buffer there would dangle across the return.
|
||||
|
||||
## Decision Log (2026-08-05, scheduler-core task 9.4 fix round) — chose a DETERMINISTIC pthread-based lock-contention test (hold `record_lock` on the main test thread, spawn a real second pthread calling `sched_record_status()`, assert it has NOT returned after a generous `usleep()`, then assert it completes once the lock is released) over a genuinely racy ASan-timing-dependent reproduction, following the SAME precedent task 9.2 already established for its TOCTOU test (test_record_lifecycle.c) of preferring a deterministic proof over a flaky race test. Verified the test is not vacuous by temporarily commenting out the `mylock_lock()`/`mylock_unlock()` calls in `sched_record_status()` and confirming the test genuinely FAILS (`Expected 0 Was 1`) before restoring the real fix — all 26/26 scheduler tests pass with the fix in place. A second, fully deterministic (no timing dependency at all) test proves the actual use-after-free is closed: take a status snapshot, then call the real `sched_record_stop()` (which `free()`s `sched->record_file_path` for real under ASan), and confirm the snapshot's `reply->file` — a separate `strdup()` allocation — survives untouched.
|
||||
|
||||
## Key Learning (2026-08-05, scheduler-core task 10.3) — `src/xceadmin/src/xcecommon/schema_files/controller.capnp` (the bundled schema copy `xcecommon.schema.load_schema()`'s default path resolves to) is a BUILD ARTIFACT, not tracked in git (`git ls-files` confirms) — `src/xceadmin/meson-build.sh` `cp`s the real source-of-truth `src/schema/controller.capnp` into it fresh on every `meson compile -C <builddir> xceadmin` run, inside an out-of-tree mirror. The live checkout's copy under `src/xceadmin/src/xcecommon/schema_files/` can be silently STALE relative to `src/schema/controller.capnp` (confirmed: it was still missing task 7.4's `SchedAdminAction`/`SchedAdminRequest`/`SchedAdminReply` additions when checked directly) — this is harmless for the real meson-driven test run (which always re-syncs), but would bite a bare `pytest src/xceadmin/tests` run from the live checkout without first re-syncing or setting `XCEADMIN_SCHEMA_PATH`. Never hand-edit this file; if editing `controller.capnp`, always edit `src/schema/controller.capnp` (already established in admin-config-tool's own task 1.3 note) and let the build re-sync it.
|
||||
|
||||
## Decision Log (2026-08-05, scheduler-core task 10.3) — `xceadmin.scheduler`'s `record_start/stop/status()` deliberately DO NOT follow `xceadmin.push.push_config()`'s established "return an application-level failure as data" convention (`PushResult.error` populated, never raised, caller/CLI decides). Task 10.3's own Observable text is explicit and different: "raises a typed error on connection failure, decode failure, or an error reply" — naming a business-level `SchedAdminReply{ok=false}` as a THIRD raising failure mode on equal footing with connection/decode failures. Implemented as a new `SchedAdminError(XceAdminError)` (`xcecommon/errors.py`) carrying `error`/`recording`/`file`/`event_count`/`started_at` (full reply state, not just the reason string, since e.g. "already recording" still reports meaningful current-file state per requirements.md's Reply field semantics table). design.md's older API Contract table (written before task breakdown) still lists Errors generically as "XceAdminError subclasses (connection, redirect, decode)" with no explicit error-reply row — treated as design.md being superseded by the more specific, later tasks.md Observable, not a conflict to resolve by asking. Future work on this file (task 10.4's `dump()`, task 10.5's CLI wiring) should NOT assume `push.py`'s return-as-data convention applies uniformly across this package — check each function's own task Observable text.
|
||||
|
||||
## Do-Not-Repeat (2026-08-06, scheduler-core task 10.5) — a `click` CLI command function must NEVER share its bare name with a directly-imported business-logic function it delegates to, even though nothing raises an ImportError. `cli.py`'s existing convention (`config_build`/`config_push` calling `build_cluster_config`/`push_config`) already dodges this by prefixing the CLI function with its group name — but it would be an easy trap to miss when adding `scheduler record start/stop/status` + `scheduler dump`, since `from xceadmin.scheduler import record_start, record_stop, record_status, dump` are the exact literal command names AC 10/20 use. Defining `def record_start(...):` under `@record.command("start")` that internally calls `record_start(...)` creates infinite self-recursion (Python resolves the free-variable name at call time from the module's current globals, which by then point at the new local `def`, not the import) — no error at import time, only a `RecursionError` the first time the command actually runs. Fix used here: keep the direct unaliased imports, but name every CLI command function with its full group-path prefix (`scheduler_record_start`/`scheduler_record_stop`/`scheduler_record_status`/`scheduler_dump`), passing the short literal command string explicitly to `@x.command("start")`/`@scheduler.command("dump")` (click infers the CLI-visible name from the decorator arg, not the Python function name, so the prefix never leaks into `--help` output). Same collision risk applies to the top-level `@cli.group() def scheduler()` vs. `import xceadmin.scheduler` — used `from xceadmin.scheduler import ...` (never `from xceadmin import scheduler`) for exactly this reason.
|
||||
|
||||
## Key Learning (2026-08-06, scheduler-core task 10.5) — requirements.md's "Status output" example under AC 10 is a load-bearing literal spec, not illustrative prose: every label (`recording:`/`file:`/`events:`/`started at:`) is left-padded to column 13 before the value (verified by counting characters in the raw markdown — `"recording:".ljust(13)`==`"file:".ljust(13)`==`"events:".ljust(13)`==`"started at:".ljust(13)`, all 13 wide). `startedAt` is documented as "nanoseconds since epoch" (real Unix epoch, not a scheduler-internal logical clock), rendered as timezone-less ISO-8601 (`2026-07-29T02:09:00`, i.e. `datetime.fromtimestamp(ns/1e9, tz=timezone.utc).strftime("%Y-%m-%dT%H:%M:%S")`). AC 10 also says `record start`/`record stop` return the SAME confirmation fields as `record status` "for confirmation" — so all three CLI subcommands should share one formatting helper, not three separate one-off `click.echo()` calls.
|
||||
|
||||
## Decision Log (2026-08-06, scheduler-core task 10.5) — `xceadmin.cli`'s established precedent (config_push) surfaces every `XceAdminError` subclass via bare `e.message`. `SchedAdminError` is a deliberate, documented exception to that precedent: its own docstring (`xcecommon/errors.py`, task 10.3) explicitly invites a richer CLI message ("so a caller that wants to display it ... does not have to re-query record status separately"), and `e.message` alone (`"scheduler admin request failed: already recording"`) does carry the reason but drops the `recording`/`file`/`event_count` state the exception was specifically built to carry. Chose: catch `SchedAdminError` in a branch ahead of the generic `XceAdminError` catch-all, and only add extra detail (current file + event count) when `e.recording` is true (i.e. only the "already recording" case has genuinely useful extra state to show — "not recording"/"scheduler not connected"/"request timeout"/"invalid record file name" all report `recording=false` and fall through to the plain `e.error` string, matching precedent for those). Every other `XceAdminError` subclass (including `ReplayFileError` for `scheduler dump`) still gets the bare `e.message` treatment, unchanged.
|
||||
|
||||
## Do-Not-Repeat (2026-08-06, scheduler-core task 11.1) — `xce_sched_core_fini()` (`core.c`) unconditionally calls `dataq_fini(core->pend_updates[0])`/`dataq_fini(core->pend_updates[1])`, and `dataq_fini()` itself SEGVs (`dataq_deq_head` NULL deref) if passed a NULL `dataq_t*`. `xce_sched_core_init()` deliberately leaves both slots NULL (core.c's own file header comment: the dataq killer callback stays driver-owned) — so ANY caller of `xce_sched_core_init()` that might later call `xce_sched_core_fini()` (including on an early-return error path) MUST separately `calloc(1, sizeof(dataq_t))` + `dataq_init(..., killer_or_NULL)` both slots immediately after init, even if that driver never actually enqueues anything into them (a NULL killer is fine in that case — matches every core.c-including test fixture's own established pattern, and scheduler.c's real `sched_init()`). Building `xce_sched_sim_driver_t` (task 11.1's own sim.c) hit this directly: the sim driver never uses `pend_updates` (task 11.2's event loop bypasses them entirely per Req 5 AC 18, translating `.replay` events straight into `xce_core_event_t` batches), but its own error-path cleanup calls to `xce_sched_core_fini()` crashed until the same alloc+init pair was added right after `xce_sched_core_init()`.
|
||||
|
||||
## Key Learning (2026-08-06, scheduler-core task 11.1) — the C-side equivalent of xceadmin's raw-byte `.replay` message-shape-peek technique (task 10.4, `xceadmin/scheduler.py`'s `_peek_message()`) needs no manual byte parsing: after `capn_init_fp(&c, f, 0)` decodes one message, `capn_getp(capn_root(&c), 0, 1)` (resolve=1) returns the REAL top-level payload's `capn_ptr` (every writer in this codebase stores its actual message at pointer slot 0 of the implicit root struct, per `write_capnp_message()`'s callers' `capn_setp(capn_root(&c), 0, ptr.p)` convention) — its `.datasz` (BYTES, divide by 8 for word count) and `.ptrs` fields give the exact `(dataWordCount, pointerCount)` shape tuple, which matches controller.capnp.h's own generated `<Type>_word_count`/`<Type>_pointer_count` constants 1:1 (verified: ReplayHeader=(1,5), SnapshotEntry=(1,1), ReplayEvent=(2,1), ReplaySession=(1,2), identical to the Python-side values already documented). This is how sim.c distinguishes "first message is ReplayHeader" / "first data message is SnapshotEntry" without any self-describing type tag (none exists in the wire format) and without a fragile trial-decode.
|
||||
|
||||
## Do-Not-Repeat (2026-08-06, scheduler-core task 11.1) — after `capn_free(&c)`, ANY `capn_ptr`/generated `<Type>_ptr` wrapper still holding a pointer INTO that freed arena becomes immediately dangling; re-reading a scalar field through it (even a cheap accessor like `SnapshotEntry_get_ts(sp)`) is a genuine ASan-catchable use-after-free, not just a style nit. In a scan loop that frees `c` per iteration, any value needed AFTER the free (e.g. "the timestamp of whichever candidate we kept") must be captured into a plain local BEFORE the `capn_free(&c)` call that owns it — never re-derived from the wire-struct view afterward, even if the pointer LOOKS like it should still be readable.
|
||||
|
||||
## Key Learning (2026-08-06, scheduler-core task 11.1) — `xce_sched_core_load_snapshot()` (snapshot_load.c, task 8.1) aliases `core->config = in->config` (a borrowed-reference assignment, not a deep copy, per its own documented "config -- borrowed reference" choice) — after a successful call, `in`'s `config` field (and therefore anything that owns/frees `in` as a whole, e.g. a decoded `replay_snapshot_entry_t` from a `.replay` file) must NEVER be freed, or `core->config` immediately dangles. Every OTHER field `load_snapshot()` restores (jobs/queues/workers/users/projects/resources/resvals) IS independently deep-copied (via `makeString()`/`dup_val()`/fresh `calloc()`s), so only `config` carries this hazard — but since there is no partial-free API, the safe rule (matching `recording.c`'s `write_snapshot_entry()` own documented choice on the SAVE side) is: never free the snapshot struct that was actually handed to `load_snapshot()` at all, and accept the leak of its non-aliased remainder (this whole test suite already runs under `ASAN_OPTIONS=detect_leaks=0` for exactly this reason). Any INTERMEDIATE candidate discarded BEFORE ever being passed to `load_snapshot()` (e.g. while scanning a `.replay` file for the best matching snapshot) is safe to free normally — the hazard only attaches to the one candidate that was actually loaded.
|
||||
|
||||
## Key Learning (2026-08-06, scheduler-core task 11.2) — the simulation driver (`xce_sched_sim_driver_t`, sim.h) deliberately has no `xce_scheduler_t*` (Req 5 AC 17), which means `plugin_register()`/`register_builtin_fcfs()` (both `xce_scheduler_t*`-typed) and `sched_job_picker()`/`sched_allocator()` (both `xce_scheduler_t*`-typed thread entry points, scheduler.c) are ALL unreachable from it. Task 11.1 punted registering any picker into `sim->core.plugins`; task 11.2 discovered this is load-bearing the moment a real event loop calls `xce_core_go()` and then blocks draining `core->decisions` — with zero threads ever consuming `core->wbuf`, that block is a permanent deadlock, not a degraded-but-safe path, regardless of whether the fixture has any real jobs. Fixed by: (1) a new generic `plugin_register_core(xce_sched_core_t*, xce_sched_plugin_t*)` (plugin.c/plugin.h) that does `init_fn()` + `key_add(core->plugins,...)` directly, skipping `plugin_register()`'s `plugin->sched = sched` bookkeeping (`plugin->sched` stays NULL — safe, since nothing calls `plugin_unregister()` on a sim-registered plugin, no sim-driver teardown function exists yet); (2) a new `builtin_fcfs_new_plugin()` factory (builtin_fcfs.c/plugin.h) extracted from `register_builtin_fcfs()`'s existing template-build step (behavior-preserving refactor, zero change to the real production registration path); (3) a NEW persistent core-scoped picker-driving thread (`sim_picker_thread_fn`, sim_loop.c) mirroring `sched_job_picker()`'s outer loop, spawned once per `sched_sim_driver_run()` call, never joined (no sim-driver fini exists). Real resource-matching allocation (`sched_make_allocation()`) is confirmed (independently, by an earlier task 4.6 cerebrum note: "sched_allocator Requires Full xce_scheduler_t* Context; Cannot Be Used in Pure-Core Contract Tests") to be unreachable without a genuine `xce_scheduler_t*` too, and has no pure-core equivalent — that extraction belongs to the separate, not-yet-designed `scheduler-allocation` spec. Task 11.2's thread therefore conservatively PENDs every picker candidate (real `on_pend()`/`make_pend_decision()`/`make_pendreason()` calls, never a fabricated DISPATCH+allocation) rather than block on or fake real resource matching.
|
||||
|
||||
## Do-Not-Repeat (2026-08-06, scheduler-core task 11.2) — a header's forward-declared opaque type (e.g. `plugin.h`'s `typedef struct xce_sched_core_s xce_sched_core_t;`, placed deliberately just above the picker-interface section so `xce_job_picker_iface_t`'s hooks can reference it) only satisfies function DECLARATIONS placed textually AFTER it in the same file — inserting a new `extern` prototype using that type earlier in the file (e.g. right after the `register_*` block, before the forward-declare) fails to compile with "unknown type name" everywhere the header is included, even though the type IS declared later in the very same file. Always grep/read for where a needed forward-declare actually sits before adding a new declaration that depends on it.
|
||||
|
||||
## Do-Not-Repeat (2026-08-06, scheduler-core task 11.2) — a new PUBLIC header (e.g. `sim.h`) must never gain a declaration whose parameter types (`struct capn`, `capn_ptr`) come from a heavy third-party runtime header (`controller.capnp.h`/capnp_c.h) unless every consumer of that public header already includes the heavy header first — `main.c` and other non-capnp-aware callers include `sim.h` directly. The fix used here: keep such a shared-but-not-really-public helper (`msg_root_has_shape`, shared between `sim.c` and the new `sim_loop.c`) as a non-static (external-linkage) function with NO declaration in the public header at all — instead, each `.c` file that calls it carries its own local `extern` prototype, the SAME "extern declared where consumed, not in a shared header" convention `recording_events.c`/`recording_session.c` already establish for `write_capnp_message()` (`recording.c`).
|
||||
|
||||
## Decision Log (2026-08-06, scheduler-core task 11.2) — chose to advance the simulation driver's virtual clock via a small NEW `sim_advance_virtual_clock()` living at driver level (`sim_loop.c`, `static`, not exported through `core.h`) rather than adding a general "jump virtual time" setter to `core_clock.c`'s public API. Rationale: `core_clock.c`'s own file header comment claims `xce_core_now()`'s `clock_gettime()` call is "the ONE wall-clock call in the entire scheduler-core module family" — adding a second wall-clock call site inside `core_clock.c` itself would make that claim stale/false. `sim.c`'s own task-11.1 `sim_set_virtual_clock()` already established the precedent that driver-level `clock_gettime()` calls (re-seeding `virtual_clock_base`/`real_clock_base`) are legitimate and OUTSIDE `test_core_isolation.c`'s scan boundary (scoped to `core.c`/`core.h` text only) — the new function is the same pattern, just re-anchoring speed_num/speed_den-agnostic (mid-run jump only, never touching the speed settings `sched_sim_driver_start()` already configured), used for every ReplayEvent's own timestamp AND every ReplaySession's `beginRound`.
|
||||
|
||||
## Do-Not-Repeat (2026-08-06, scheduler-core task 11.3) — NEVER run `clang-format -i` against a `meson.build` file. It is a C/C++ formatter with no meson/Python-ish grammar awareness; pointed at `meson.build`, it silently "succeeds" (no error) but reinterprets meson's syntax as C-ish tokens and mangles it into garbage (e.g. `scheduler_test_src = [...]` became `scheduler_test_src =\n [\n ...\n ]\n\n scheduler_test_deps\n = [...]`, and every `#` comment line got wrapped/mis-indented). Caught immediately only because the post-edit file-watcher hook echoed the corrupted content back in a system-reminder. Fix: `git checkout -- <file>` to discard the corruption (safe only if the file had no OTHER uncommitted changes worth keeping — re-apply any real edits via Edit afterward, never via another format pass). When a task says "run clang-format on changed/new files," scope the command explicitly to `*.c`/`*.h` files only, never a blanket pass over every changed path in the diff.
|
||||
|
||||
## Key Learning (2026-08-06, scheduler-core task 11.3) — a source-text isolation-scan test (the `test_core_isolation.c` genre: `strstr()` a forbidden symbol/type name across a whole file's raw text) breaks the moment the audited file's OWN doc comments legitimately name the forbidden symbol in prose while documenting the very boundary being enforced. Concretely: `sim.h`'s task-11.1 boundary comment says *"`xce_sched_sim_driver_t` below contains no `xcecli_ctx_t`, no `xce_cluster_config_t`..."* — a raw whole-file scan for those exact substrings false-positives on that sentence. `test_core_isolation.c` never hit this because `core.h`/`core.c`'s comments never happen to name a forbidden symbol by name. Fix used in `test_sim_isolation.c`: a small state-machine `strip_c_comments()` (block + line comments replaced with a space, string/char literal contents left untouched) run BEFORE every substring scan — comments never produce compiled symbols anyway, so stripping them first keeps the source-text layer consistent with what the nm(1) link-time layer actually measures. General rule for any future isolation/boundary audit test in this codebase: if the file under audit is expected to document its own forbidden-dependency list in prose (encouraged, good practice), the scanner MUST comment-strip first, not scan raw text.
|
||||
|
||||
## Key Learning (2026-08-06, scheduler-core task 11.3) — proving "the struct definition itself" has no forbidden field (Req 5 AC 17's literal wording) needs a NARROWER check than a whole-file scan: extract just the `typedef struct { ... } xce_sched_sim_driver_t;` body text (find the last `typedef struct {` strictly before the unique `} xce_sched_sim_driver_t;` end marker — `sim.h` has a SECOND, later, unrelated `typedef struct {` for `sched_sim_args_t`, so "first occurrence" would be wrong if the struct order ever changed, but "last occurrence before the known end marker" is robust either way) and scan only that substring. This also catches a real gap the generic FORBIDDEN_SYMBOLS prefix list (reused from `test_core_isolation.c`) does NOT cover: `xce_cluster_config_t` is a plain type name, not a call-site function-name prefix, and no existing FORBIDDEN_SYMBOLS entry is a substring/superstring match for it (`xce_cluster_config_validate` is a different, unrelated string) — Req 5 AC 17 names it explicitly, so it needs its own dedicated STRUCT_FORBIDDEN_TYPES list, separate from FORBIDDEN_SYMBOLS.
|
||||
|
||||
## Decision Log (2026-08-06, scheduler-core task 11.3) — verified self-review discipline for isolation/audit tests in this codebase (matches `test_core_isolation.c`'s own `any_undefined_seen` vacuous-pass guard bar): don't just assert the new test passes clean — temporarily inject one violation per layer (a forbidden struct field via an opaque forward-declared pointer type so it still compiles; a forbidden `#include "client.h"`; a forbidden call-site symbol reference via a locally-declared `extern` prototype so it links without pulling in the real client library), rebuild+run, confirm the EXACT expected test/assertion fails (not a different one, not a build break), then revert via `cp`-from-backup and `diff -q` to prove byte-identical restoration. For `test_sim_isolation.c` this caught the struct-field layer, the `#include` layer, and — usefully — proved the source-text-scan and nm(1) link-time layers are genuinely independent (both fired on the same injected `xcecli_hello` call, not just one).
|
||||
|
||||
## Do-Not-Repeat (2026-08-06, scheduler-core task 11.5) — recurrence of the accidental-`*/`-inside-a-comment bug (same family as bug-1493 in typemap.h): writing prose like `xce_scheduler_t*/scheduler.h dependency` inside a `/* ... */` block comment terminates the comment early at the literal `*/` substring, turning the REST of the doc comment into raw C tokens and producing dozens of cascading "stray '#'"/"missing terminating character" errors far from the true location. Caught immediately by the RED-phase build (not by inspection). When writing a doc comment that needs to say "type* / other-thing" (a type name immediately followed by a slash), always insert a space around the slash (`xce_scheduler_t* / scheduler.h`) — grep new/edited comment blocks for the regex `[A-Za-z0-9_]\*/` before building whenever a comment discusses pointer types near a path or ratio.
|
||||
|
||||
## Do-Not-Repeat (2026-08-07, scheduler-core task 12.3) — THIRD recurrence of the accidental-`*/`-inside-a-comment bug (same family as bug-1493/the 11.5 entry above): `xce_scheduler_t*/record_dir` in a new repl_params.c doc comment closed the comment early again, despite the lesson already being logged twice. This confirms the existing "grep before building" advice isn't being followed proactively — going forward: after writing ANY new multi-paragraph doc comment in this codebase (not just when a build error already occurred), run `grep -n '[A-Za-z0-9_]\*/' <file>` on it BEFORE the first build attempt, not after. Applied retroactively this session across all 4 touched repl*.c files before the final build — zero hits confirmed the fix was complete.
|
||||
|
||||
## Key Learning (2026-08-06, scheduler-core task 11.5) — this repo's top-level `xce.build` sometimes needs TWO consecutive `ninja -C xce.build <target>` invocations after a `meson.build` source-list change (adding a new `.c` file to an existing target, or a whole new `executable()`): the first invocation's "Regenerating build files" step reconfigures but the immediately-following `ninja` command in the SAME invocation can still report "unknown target" against the freshly-added target/source; a second, separate `ninja -C xce.build <target>` (or `ninja -C xce.build` with no target, or `ninja -C xce.build -t targets all` to confirm the target now exists) picks it up. Don't treat "unknown target" right after a meson.build edit as evidence the edit is wrong — retry the build once before debugging further. Also: `ninja -t targets` (no arg) only lists targets needed for the DEFAULT build, not the whole project — use `ninja -t targets all` to enumerate everything when hunting for a specific new target name.
|
||||
|
||||
## Decision Log (2026-08-06, scheduler-core task 11.5) — SUPERSEDED 2026-08-07, see Do-Not-Repeat below: the selective-`git stash` RED-phase technique described here did NOT actually reproduce when independently re-run (twice by the round-1 reviewer, once by the round-2 remediation pass) — it produced a hard `ninja` build failure, not the claimed 3-test-failure RED, because `test_sim_event_loop.c` (left un-stashed, since it's a test file) already calls `sim_process_replay_event_message()` with the NEW 5-arg signature while the stashed-back `sim_loop.c` only declares the OLD 4-arg one. The original claimed evidence (3 tests failing under `meson test`) was therefore not reproducible as described. Do not treat this stash technique as validated; see the replacement methodology below.
|
||||
|
||||
## Do-Not-Repeat (2026-08-07, scheduler-core task 11.5 review round 2) — `git stash` scoped to "behavioral production files only" is UNSAFE as a RED-phase probe whenever any test file in the same change already calls the NEW signature/behavior of a function whose OLD declaration lives in a stashed-back production file — the stash desyncs caller (test, not stashed) from callee declaration (production, stashed), producing a build failure that looks like a red herring, not a real RED. Validated replacement methodology (reproduced clean by both the round-1 reviewer and this round-2 remediation): make a DIRECT, TARGETED, IN-PLACE temporary edit to ONLY the specific lines implementing the Observable under test inside the one production `.c` file (never touch test files, never touch sibling production files, never use `git stash`) — e.g. hardcode a conditional selection to always take one branch, or wrap a side-effecting call's guard in `if (0 && ...)` to no-op it while leaving it compiling. Rebuild (`ninja -C xce.build`) + run the real suite (`meson test -C xce.build --suite <suite>`), capture the ACTUAL fail count/names/assertion messages verbatim (never guess or reuse a previous session's numbers). Revert each edit by restoring the EXACT original text (not a paraphrase), then prove zero residue two ways: `git diff <file>` shows only the pre-existing (pre-probe) diff, AND `grep` for whatever temporary marker/pattern you introduced (e.g. a "TEMP RED-PHASE PROBE" comment, `if (0 &&`) returns zero matches. Rebuild + rerun to confirm full green restored before reporting.
|
||||
|
||||
## Key Learning (2026-08-06, scheduler-core task 11.5) — recording_session.c's `sched_record_session_add_decision(ctx, ts, decision)` was ALREADY scheduler_t-free before this task (no `xce_scheduler_t*` parameter) — only `sched_record_session_begin()`/`sched_record_session_end()` gate on `sched->recording`/`sched->record_file`. To let a `xce_scheduler_t*`-less caller (the simulation driver, Req 5 AC 17) reuse the exact same accumulate-then-finalize protocol, extracted two new non-static functions — `sched_record_session_ctx_new(begin_round_ts)` (pure calloc+stamp, no gate) and `sched_record_session_finalize(FILE* f, ctx, end_ts)` (pure encode+write+free, target file passed explicitly instead of read off `sched->record_file`) — and turned the existing `_begin()`/`_end()` into thin wrappers around them (gate check, then delegate). This is a pure behavior-preserving refactor (verified identical control flow) that lets a second, unrelated caller reuse 100% of the wire-encoding logic via a local `extern` declaration (never `#include "recording.h"`, which pulls in `scheduler.h` → `client.h`/`xce/cluster.h`, exactly the forbidden dependency chain) instead of hand-deriving the `xce_decision_t` → `replay_decision_t` field mapping a second time.
|
||||
|
||||
## Do-Not-Repeat (2026-08-07, bug-1503, scheduler-core task 11.6) — `makeString()` (`lib/xstring.c`) is NOT a plain-heap `strdup()` equivalent: it is backed by the vendored `rtti` library's `makeInstance()` allocator, whose returned pointer sits SOME BYTES INTO a larger rtti-managed allocation (confirmed via ASan: "56 bytes inside of a 61-byte region"), not at the start of a plain `malloc()` block. `makeString()`-allocated strings are only safe when released by the SAME rtti-aware teardown path that produced them (e.g. `xce_sched_core_load_snapshot()`'s own deep-copy fields, later freed by that module's own kill/free functions) — never by a capnp-generated `free_X()`/`free_X_ptr()` function, which calls plain `free()` on each string field directly. Hit building `test_sim_equivalence.c`'s `make_queue()`/`make_job()` fixtures: populating `xce_queue_t.queue`/`xce_job_t.submit->{queue,user,project_name,resreq}` via `makeString()` then driving them through the REAL production cleanup path (`release_translated_payload()` → `free_QueueConf_ptr()`/`free_Job_ptr()`, driver.c/controller.capnp.c) crashed with an ASan bad-free. Fix: use `strdup()` for any string field a capnp-generated `free_X()` will later `free()` directly; reserve `makeString()` only for fields whose owning module's own teardown path is known to handle rtti-backed allocations.
|
||||
|
||||
## Key Learning (2026-08-07, scheduler-core task 11.6) — `sim_loop.c`'s `sim_picker_thread_fn()` (task 11.2) unconditionally PENDs every scheduling candidate it is handed — it can NEVER emit a live DISPATCH decision, under any replayed event sequence, by construction (its own doc comment calls this a "Known scope boundary": real resource-matching allocation needs a live `xce_scheduler_t*` the simulation driver deliberately does not have, Req 5 AC 17, and belongs to the not-yet-designed `scheduler-allocation` spec). `testsuite/meson.build`'s own comment on `sched_sim_verify_test` (task 11.4) already documented the same fact independently ("including DISPATCH, which sim_loop.c's own real picker thread can never produce live"). Practical consequence for task 11.6's normal↔sim equivalence test: a normal-mode recording containing ANY genuine DISPATCH decision will ALWAYS diverge when replayed in simulation (recorded=DISPATCH, simulated=PEND for the same job) — not a bug in the equivalence test, a confirmed, pre-existing, already-documented architectural gap. `test_sim_equivalence.c`'s fixture is therefore deliberately PEND-only (multiple jobs across two differently-prioritized queues, still a genuine non-trivial regression guard on event translation + picker ordering + PEND-reason content fidelity) rather than silently asserting equality on only a subset of a mixed fixture. If a future task closes the scheduler-allocation gap and gives the sim driver a real allocator, `test_sim_equivalence.c` should be revisited to add a genuine DISPATCH case.
|
||||
|
||||
## Decision Log (2026-08-07, scheduler-core task 11.6) — combining BOTH the normal-mode driver stack (`driver.c` + `recording*.c`) and the full simulation stack (`sim.c`/`sim_loop.c`/`sim_record.c`/`sim_verify.c`) as white-box `#include`s in ONE test TU (`test_sim_equivalence.c`) had no prior precedent in this test suite — every existing test file combines one side or the other, never both. Verified safe before writing the file: a mechanical scan of every `static` function name across all 17 combined `.c` files found zero collisions, and `scheduler.h` (needed for the normal-mode fixture) already coexists with `sim.c`/`sim_loop.c` in the SAME TU in `test_sim_record_output.c` today (that file includes `scheduler.h` for its own fixture code even though `sim.c`/`sim_loop.c` themselves stay `scheduler.h`-free per their own Req 5 AC 17 isolation boundary) — confirming the combination compiles before spending time debugging a hypothetical conflict.
|
||||
|
||||
## Key Learning (2026-08-07, scheduler-core task 12.1) — this codebase's capnpc-c backend silently no-ops `Data`-field encoding inside generated `encode_XXX()` composite helpers, AND the generated `encode_XXX_ptr()` wrapper never zero-initializes its local stack struct first — so any `:Data` field left unpopulated by the no-op'd encode step is written to the wire as UNINITIALIZED STACK GARBAGE, a real, reproducible SIGSEGV risk (`capn_setp`/`read_ptr` deref garbage), not a flaky test. Whether it actually crashes is pure luck of what garbage bytes land in that specific call site's stack frame — confirmed present in at least 2 known fields (`SnapshotData.internalState`, fixed as bug-1504; `WalCatchupEntry.payload`, found but deliberately deferred as bug-1505) across 2 unrelated subsystems (scheduler-core, controller HA/WAL). Fix that actually works (proven, not a workaround): switch the field from `:Data` to `:List(UInt8)` with a companion `UInt32 $C.maplistcount(...)` count field — this SAME capnpc-c backend DOES correctly auto-generate real `capn_new_list8()`/`capn_set8()` encode and `calloc()`+`capn_get8()` decode for `List(UInt8)` (working precedent already in the schema: `FetchFileReply.content`/`bytesRead`). No hand-written low-level accessor code needed after the type swap. Before this, `Data` fields required either "intentionally left unset" (silent data loss, e.g. `ReplayHeader.configHash`) or fully hand-written low-level `capn_setv`/`capn_new_data`-style accessor code bypassing the no-op'd generic codec entirely (e.g. `ReplLogEntry.batchPayloads`, bug-847's fix). Any future `:Data` field in `controller.capnp` should default to `List(UInt8)` instead unless there's a specific reason not to.
|
||||
|
||||
## Do-Not-Repeat (2026-08-07, scheduler-core task 12.1) — a `rm -rf xce.build && meson setup && meson compile` full clean rebuild can flip a genuinely pre-existing, dormant undefined-behavior bug (uninitialized stack memory, see Key Learning above) from silently-inert to deterministically-crashing, purely by shifting the whole binary's stack/register layout — this is NOT something the rebuild "caused," it's exposing UB that was always there. Don't assume a crash that only appears after a full clean rebuild is caused by whatever prompted the rebuild (here: an unrelated nix dependency swap, readline→libedit) — verify root cause with gdb before attributing blame; the actual crash site can be completely unrelated code untouched by the triggering change.
|
||||
|
||||
## Do-Not-Repeat (2026-08-07, scheduler-core task 12.1) — when a defensive guard is added based on an INITIAL crash theory that later turns out to be wrong (root-caused via gdb to something else entirely), re-evaluate whether the guard is still independently justified before keeping it. Here: a `core->config == NULL` fail-loud guard was added to `xce_sched_core_save_snapshot()` believing NULL config caused the SnapshotData crash — it didn't (the generated `encode_Conf4Sched_ptr()` already null-checks and safely emits `capn_null`); the real crash was the internalState `Data`-field UB above. The guard, though "defensively reasonable" in isolation, broke 8 other test fixtures across the suite that never set up a config (a much bigger regression than the 1 crash it prevented, which wasn't actually preventable via that path anyway) — reverted cleanly once the real root cause was found. Lesson: verify a defensive fix against the ACTUAL proven root cause (gdb backtrace + field values), not against a plausible-sounding first theory, before letting it ship.
|
||||
|
||||
## Do-Not-Repeat (2026-08-07, bug-1506/1507, scheduler-core task 12.2) — two makeString() gotchas hit building repl_break.c's breakpoint storage, both caught by this task's own RED-phase test run (not by inspection): (1) makeString() (lib/xstring.c) silently returns NULL rather than crashing when its backing xstr_table hasn't been initialized via xstring_table_init() (only ever called transitively by xce_sched_core_init(), core.c) — a test fixture that builds a bare xce_sched_sim_driver_t via memset(0) and drives repl_dispatch()'s `break` command WITHOUT first calling xce_sched_core_init(&sim.core) gets silently-NULL breakpoint token/filter fields instead of a loud failure (bug-1506). Always call xce_sched_core_init() first in any repl.c-family test fixture, even one that never touches core state otherwise — it's the same precondition the real driver flow always satisfies (sched_sim_driver_start() runs before xce_repl_start()). (2) Confirms and extends bug-1503's existing "makeString() is not strdup(), never free() its result directly" lesson: this task's own repl_breakpoint_free() called a bare FREEUP()/free() on a makeString()-produced xstring, producing "free(): invalid pointer" (glibc heap-corruption abort). The correct precedent for a field that is NEVER meant to be individually released is what kill_sched_job()/kill_sched_queue() (schedobj.c) already do: don't free it at all — makeString()'d fields on scheduler-core objects live for the process's lifetime, matching this whole test suite's `detect_leaks=0`-by-design convention. Only call free() on a makeString() result if you can point to the SPECIFIC rtti-aware teardown path that produced it (bug-1503's own narrower case); when in doubt, follow kill_sched_job()/kill_sched_queue()'s "never free it" default instead.
|
||||
|
|
|
|||
1136
.wolf/memory.md
1136
.wolf/memory.md
File diff suppressed because it is too large
Load diff
Loading…
Add table
Add a link
Reference in a new issue