From d93c522c2bc26265fb6f5e074552d72e8f285350 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Fri, 7 Aug 2026 13:48:18 +0000 Subject: [PATCH] docs(scheduler-core): record feature-level validation findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /kiro-validate-impl pass: GO. Two non-blocking follow-ups found and documented — AC 3's tlog virtual-clock logging integration is built but never activated (root-caused to sim.c's sched_sim_driver_start, deferred pending a matching deactivation/fini design), and plugins/fcfs.c vs builtin_fcfs.c is functionally correct but undocumented against design.md and missing _Verified notes on 2.1-2.6. Co-Authored-By: Claude Sonnet 5 --- .kiro/specs/scheduler-core/tasks.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.kiro/specs/scheduler-core/tasks.md b/.kiro/specs/scheduler-core/tasks.md index cd1ed65f..a4a3df38 100644 --- a/.kiro/specs/scheduler-core/tasks.md +++ b/.kiro/specs/scheduler-core/tasks.md @@ -410,3 +410,7 @@ - Task 12.1's review found a licensing gap (fixed): the readline()-based REPL input loop originally linked GNU readline (GPL) directly into `xce_scheduler`, a shipped production daemon (unlike `xce_test`, a debug-only binary controller's own readline use was already confined to). Switched project-wide to libedit (BSD) — `nixSupport/xce.nix`, both `meson.build`s, `repl.c`, and `src/controller/testsuite/test.c` now use `editline/readline.h` (API-compatible: `readline()`/`add_history()`), no hand-written accessor changes needed anywhere. - Task 12.1's review also surfaced bug-1504 (fixed) and bug-1505 (deferred, out of boundary) — see `.wolf/buglog.json`. Verifying the licensing fix required a full clean rebuild (new nix dependency), which exposed a genuine, pre-existing, deterministic SIGSEGV in `write_SnapshotData` unrelated to task 12.1's own repl.c: `SnapshotData.internalState` was a capnp `Data` field, and this codebase's capnpc-c backend silently no-ops Data-field encoding in generated `encode_XXX()` helpers without zero-initializing the caller's stack struct first — a real, reproducible UB crash, not a flaky test. Fixed (bug-1504) by switching `internalState` to `List(UInt8)` (controller.capnp, typemap.h, test_e2e_capnp.c) — this same capnpc-c backend DOES correctly auto-generate real encode/decode for `List(UInt8)` (proven precedent: `FetchFileReply.content`/`bytesRead`), eliminating the whole no-op class for this field with no hand-written accessor code. Re-verified with a full clean rebuild: scheduler suite 34/34, no regressions. While chasing this, also hit a **second, unrelated instance of the same bug class** in the controller's HA/WAL-catchup subsystem (`WalCatchupEntry.payload`, bug-847's own territory) — confirmed systemic (affects any `:Data` field in `controller.capnp` fed through the generic codec, not a one-off) but deliberately **left unfixed** (bug-1505) as out of scheduler-core's boundary; needs its own project-wide `:Data`-field audit as a separate task. - bug-1505 fixed (2026-08-07, follow-up outside any specific task's boundary — controller/HA subsystem, not scheduler-core): before fixing, confirmed zero production exposure — `xce_wal_batch_encode_raw()`/`wal_entry_to_wire()` (`request.c`) never call the generic `encode_WalCatchupEntry()` at all; they `memset()` their own struct and call `write_WalCatchupEntry()` directly with every field hand-set, so the uninitialized-`capn_ptr` crash was 100% test-only exposure (only `test_e2e_capnp.c` calls the generic path). Fixed the same way as bug-1504: `WalCatchupEntry.payload` (@2) and `batchPayloads` (@10) switched from `Data` to `List(UInt8)` (new `batchPayloadsLen` @11 companion; `payload` reuses the existing `wirePayloadLen` @9), `typemap.h`'s `wal_catchup_entry_t` field types updated, `request.c`'s `extract_data_bytes()` widened to accept a raw `capn_ptr` (it already only used `.p` internally) so its 4 call sites work regardless of which Data-shaped wrapper the accessor returns now. Verified `batchPayloads`' single-top-level-blob shape means this does NOT reintroduce bug-847's per-element cross-segment far-pointer loss (that issue is inherent to PER-ELEMENT data regardless of `Data` vs `List(UInt8)` spelling — `payload`, the per-element field, was already legacy/unpopulated in production before this fix). Strengthened `test_wal_catchup_append_term_fields_round_trip` with real payload/batchPayloads content assertions (previously never exercised — the test's `src.payload`/`batch_payloads` were left NULL, so the crash occurred on content the test didn't even care about). Verified: `controller_unit_test` no longer crashes (3/3 controller suite), scheduler suite unaffected (37/37, unrelated schema area). +- **Feature-level validation (`/kiro-validate-impl`, 2026-08-07) — GO, with 2 new non-blocking follow-ups found:** + 1. **Req 5 AC 3's tlog logging-integration bullet is built but never activated.** `core_clock.c`'s `xce_core_register_tlog_format()`/`xce_core_set_active_for_logging()` (task 4.5) are unit-tested but have zero call sites in `main.c`/`sim.c`/`driver.c` — task 4.5's own header comment explicitly deferred this ("this task does not wire it into mylog_init()/main.c; that remains a later task's job"), but no later task in this spec ever picked it up. Root-caused where it *should* wire in: NOT `main.c`'s normal-mode path — normal mode seeds `virtual_clock_base=0, real_clock_base=0` (`core.c:139-140`), so `xce_core_now()` there returns raw `CLOCK_MONOTONIC` nanoseconds-since-boot, not epoch time; activating it for normal-mode logging would make log timestamps *worse*, not better. The correct site is `sim.c`'s `sched_sim_driver_start()`, right after `sim_set_virtual_clock()` (line 394) — `virtual_clock_base` is seeded to `snapshot_ts` there (a real epoch-ns value from the `.replay` file), so `xce_core_now()` becomes a meaningful simulated timestamp. Left unfixed here rather than patched under validation-gate time pressure because activation needs a matching deactivation: `g_active_core_for_logging` is a single process-wide pointer, `sched_sim_driver_run()`'s own header comment already documents "no sim-driver stop/fini function exists" (deliberate, accepted gap) with multiple early-return paths, and several test binaries (e.g. `test_repl_equivalence.c`) construct more than one stack-allocated `xce_sched_sim_driver_t` per process — activating without a paired deactivation risks a real ASAN stack-use-after-scope hazard the very next time an unrelated log call fires in that process. Needs a small dedicated follow-up task: wire activation into `sched_sim_driver_start()` AND deactivation into every `sched_sim_driver_run()` return path (or add the fini function its own comment says doesn't exist yet). + 2. **`plugins/fcfs.c` vs `plugins/builtin_fcfs.c` — functionally correct, undocumented.** design.md (component description) says `fcfs.c` is "rewritten (Phase 1) to `xce_job_picker_iface_t`, drops `job_elect_fn`." The actual implementation instead added a new parallel file, `plugins/builtin_fcfs.c`, and left `fcfs.c` untouched still registering the legacy `job_elect_fn` (`fcfs.c:353`) — confirmed deliberate, not an oversight: `scheduler.c:466`'s own comment says the new picker is registered "alongside the untouched legacy `xce_sched_fcfs`", and task 1.2's compatibility-wrapper requirement needs exactly this kind of legacy `job_elect_fn`-only plugin to exist as its live exercise target. `builtin_fcfs` is confirmed the registered default (`schedobj.c:364`). Functionally sound; the only gap is that tasks 2.1-2.6 carry no `_Verified:` annotations (unlike every other task group) and design.md's literal text was never updated to record this file-layout decision — a documentation/traceability gap, not a defect. Worth a follow-up: add `_Verified:` notes to 2.1-2.6 and correct design.md's component description. + - Both items are cosmetic/traceability, not correctness or determinism defects — the soak, equivalence, and integration evidence for this validation pass (15-round sequential + 15-round chaotic HA soak, both zero-loss/zero-divergence; full rebuild 41/41 meson + 15/15 manual scheduler + all Go/xceadmin tests green; independent requirements-coverage/design-alignment/cross-task-integration subagent passes) is unaffected by either.