diff --git a/.kiro/specs/scheduler-core/requirements.md b/.kiro/specs/scheduler-core/requirements.md index 97cca25b..eb2b98ab 100644 --- a/.kiro/specs/scheduler-core/requirements.md +++ b/.kiro/specs/scheduler-core/requirements.md @@ -1106,6 +1106,18 @@ if (arguments.sim_file) { **Implementation note**: The REPL is a single `fgets` loop in the simulation driver. It is not a separate thread — when the REPL blocks on stdin, the simulation loop is parked. This is correct and intentional: the operator is in control, and the simulation has no wall-clock deadlines. + **Amendment (2026-08-19, live-operator testing — task 12.5)**: driving the REPL against a real replay file surfaced five gaps in the v1 design above. All five are now part of this acceptance criterion, not a follow-up: + + 1. **`continue` with no breakpoints must not end the session.** The original pseudocode's `for each event in replay_file: ...` loop, once it reaches EOF with no breakpoint ever hit, simply falls out the bottom and the process exits — the operator never gets to inspect the final state. Corrected behavior: reaching EOF while `--interactive` prints the existing "[sim] replay complete. N_sessions processed." banner and then **re-enters the REPL command loop** instead of returning. State-inspection commands (`info job/queue/worker/resource/summary/session`) remain fully usable against the final simulated state. + + 2. **The simulator shall never terminate except via an explicit `quit` (or stdin EOF, which this REPL has always treated as an implicit `quit`).** This is the direct corollary of (1): once replay is complete and the REPL has re-entered its prompt, `step`/`continue` at that point are no-ops that print "[sim] replay already complete -- no more events to process. Use 'quit' to exit." and re-prompt, rather than being treated as "hand control back to a driver loop that no longer has anything to drive." + + 3. **`step`/`continue` shall echo the input events and resulting decisions for the current session, in the order they actually occur.** V1 only ever printed decisions (`sim_print_decision()`); the events that drove them (`JOB_PUSH`, `JOB_STATE_CHANGE`, `WORKERS_PUSH`, etc.) were completely silent unless a breakpoint happened to match one. An operator single-stepping through a session had no visibility into what changed before the resulting dispatch/pend decisions appeared. Corrected behavior: every event is echoed one line at a time as it is decoded (the same point breakpoint-matching already inspects it, `xce_repl_on_event()`), so the printed sequence is event(s) → BEGIN_ROUND → decision(s), matching real execution order. + + 4. **`info resource |all` shall report resource *metadata*, restricted to globally-shared resources — never a per-host aggregation.** V1's implementation summed `total`/`available` across every worker's per-host resource value for every resource, host-based ones included (e.g. `cpus`, `mem`) — a fabricated, meaningless number for a per-host consumable already correctly inspected via `info worker `, and it never surfaced the resource *definition* (description, value type, which of `XCE_RESF_BUILTIN`/`DYNAMIC`/`GLOBAL`/`SHARED`/`EXTERNAL`/`RELEASE`/`DEFAULT_HOST`/`CONSUMABLE`/`LIMIT` apply, reporting interval). Corrected behavior: `info resource ` prints the resource's definition metadata; `info resource all` lists only resources flagged `XCE_RESF_GLOBAL` or `XCE_RESF_SHARED` (host-based/per-worker resources are out of scope for this command — they are `info worker`'s job). A global/shared resource's current value is read from the core's single shared-instance pool (`core->sharedInst`, indexed by the resource's `idx` — the same pool `RESVALS_FULL`/`RESVALS_DELTA` events and snapshot save/load already treat as the defining store for these resources), never aggregated across workers. + + 5. **REPL command arguments accept an optional pair of surrounding quotes.** An operator copying a jobid straight out of `info jobs`' own output (e.g. `0-15-1787129769-0[0]`) and habitually quoting it (`info job "0-15-1787129769-0[0]"`) got a silent wrong-answer failure (`parse_jobid()`'s regex requires a whole-string anchor match, so the literal quote characters broke the match, and the `strtoull()` fallback parsed the leading `"` as a nonnumeric prefix, silently producing jobid 0) instead of an intelligible error. Corrected behavior: `repl_split_tokens()` strips one matching pair of leading/trailing `"` or `'` characters from each token before it reaches any command handler. + ### Requirement 6: Object sharing forward-compatibility **Objective:** As a maintainer, I want the core's data-ownership patterns to leave room for future resreq/pend-reason object sharing, so that a later optimization spec can introduce sharing without retrofitting these invariants under load-bearing code. diff --git a/.kiro/specs/scheduler-core/tasks.md b/.kiro/specs/scheduler-core/tasks.md index da8a759c..42bbc8a3 100644 --- a/.kiro/specs/scheduler-core/tasks.md +++ b/.kiro/specs/scheduler-core/tasks.md @@ -389,6 +389,13 @@ - _Depends: 12.2, 12.3_ - _Verified: 2026-08-07 — TEST-ONLY task, no new production code. New test_repl_equivalence.c (683 lines) runs the SAME sched_sim_driver_run() (sim_loop.c) against ONE hand-built `.replay` fixture (two queues at different priorities "high"=5/"normal"=1, four jobs split across THREE ReplaySession boundaries) twice: once in batch mode (interactive=false, straight through), once REPL-driven (interactive=true, stdin freopen()'d from a scripted "step 1\ncontinue\n" -- exactly 2 lines, sufficient and safe regardless of session count since CONTINUE mode never re-reads stdin once entered, avoiding the readline()-EOF-triggers-exit(0) trap). This is a DIFFERENT comparison than task 11.6's test_sim_equivalence.c (which compares normal-mode driver vs simulation driver) -- here both sides are the identical simulation driver, differing only in `sim->interactive`. Both runs set `sched_sim_args_t.output_path` directly (confirmed unconditional on `interactive` by reading sim.c's `args->output_path != NULL` gate), so both write a full `.replay`-format decision stream to their own file with no `set output` REPL command needed. Since the fixture has THREE session boundaries (not task 11.6's one), decoding uses a new decode_all_sessions() that scans the whole output file structurally via sim.c's shared `msg_root_has_shape()` (ReplaySession's distinct (1,2) word/pointer-count shape) rather than a fixed message-skip count. Comparator reuses sim_verify.c's leaf-level structural comparators (`sim_verify_job_alloc_eq()`/`sim_verify_job_preason_eq()`, both `static` but visible via white-box #include) through a duplicated `replay_decision_content_eq()`, same technique test_sim_equivalence.c's own function of that name establishes. Non-tautology guards: session 1 asserted to have EXACTLY 2 decisions (an unambiguous floor -- only 2 jobs could possibly be pending that early), job 501 independently pinned as present in session 1's batch-side content, total batch decisions asserted >= 4, and every decision's type independently pinned as PEND (matching sim_picker_thread_fn()'s documented DISPATCH gap) BEFORE the cross-run equality check -- so a buggy always-`return 1` comparator could not pass silently. Observed (not hand-predicted) that a PEND'd job persists as a picker candidate into later sessions (session 1: jobs 501/502 -> 2 decisions; session 2: 501/503/502 -> 3; session 3: 501/503/502/504 -> 4; total 9 on both sides), confirming the fixture is genuinely non-trivial without needing a fragile hardcoded per-session prediction. Synthetic RED-phase probe: analysis first showed that defeating `repl_should_pause()` (task 12.1's own already-verified probe target) would NOT turn this test red, because that only changes *when* stdin is read/control returns, never *what* gets processed -- every session still eventually completes regardless of pause timing, and this file's own CONTINUE-is-sticky script design tolerates any pause schedule. The correct probe for THIS invariant ("never alters ... what events are processed") had to touch content, and only under `sim->interactive`: temporarily added one line to `xce_repl_on_event()` (repl_break.c) -- which takes a non-const `xce_core_event_t*` and fires (via repl.h's `sim->interactive`-gated wrapper) strictly before an event joins the batch `xce_core_push_events()` later applies -- corrupting a JOBS_PUSH job's jobid (`+= 1000000`) whenever interactive. Rebuilt, ran the full scheduler suite: exactly 4/37 failed (this new test plus three pre-existing REPL tests that also exercise JOBS_PUSH under interactive mode -- sched_repl_execution_control_test, sched_repl_snapshot_params_test, sched_repl_breakpoints_inspection_test -- all correctly detecting the same real divergence), all 33 others stayed green. This test's own failure output: "REPL step/continue-to-completion must produce a byte-identical decision to batch mode at this session and position" (jobid 501 on the batch side vs 1000501 on the REPL side, directly visible in the two runs' printed PEND lines). Reverted (diff-confirmed byte-identical to the pre-probe file), rebuilt, reconfirmed 37/37 scheduler suite (was 36/36), whole-project build clean. +- [ ] 12.5 REPL live-operator-testing fixes (Requirement 5.22 amendment) + - Fix the five gaps a live operator session surfaced (see requirements.md's AC 22 "Amendment (2026-08-19)" for the full rationale on each): (1) `continue` reaching EOF with no breakpoint hit re-enters the REPL instead of exiting the process; (2) `step`/`continue` issued after replay is already complete re-prompt with a message instead of being treated as "hand control to a driver loop"; (3) `step`/`continue` echo each input event (not just decisions) as it is processed, in real event/decision order; (4) `info resource |all` reports resource definition metadata restricted to `XCE_RESF_GLOBAL`/`XCE_RESF_SHARED` resources, reading the shared value from `core->sharedInst` instead of aggregating host-based resources across every worker; (5) `repl_split_tokens()` strips one matching pair of surrounding quote characters from each token. + - Observable: `--sim --interactive` against a replay file with no breakpoints set, driven by `continue`, stays at the `(sim) ` prompt after "[sim] replay complete" instead of the process exiting -- `info summary`/`info job`/etc. still work at that point, and only `quit` (or stdin EOF) ends the process; a `step` while events are pending prints each event's own one-line summary before that session's decisions; `info resource all` lists only global/shared resources with metadata fields, never a host-based resource's fabricated total/available; `info job ""` (quoted) parses identically to the unquoted form. + - _Requirements: 5.22_ + - _Depends: 12.4_ + - _Bugs: bug-1593 (info resource host-vs-shared), bug-1594 (continue/no-quit exit), bug-1595 (events not echoed in step/continue), bug-1596 (quoted REPL argument parsing)_ + - [x] 13. Run full cross-phase HA soak regression - Run sequential and chaotic HA soak rounds against the complete feature (all three phases integrated), confirming zero job loss, divergence, or liveness stall attributable to this feature across the full run - Observable: the soak run completes with a clean pass across all rounds, matching the zero-regression bar set by the Phase 1 and Phase 2 soak runs diff --git a/src/scheduler/include/core.h b/src/scheduler/include/core.h index c5526fa3..181f3e0d 100644 --- a/src/scheduler/include/core.h +++ b/src/scheduler/include/core.h @@ -137,11 +137,17 @@ struct xce_sched_core_s { * Atomically updated by the driver AS A GROUP before each event * dispatch (design.md's State Management note) -- xce_core_now() reads * them lock-free, no mutex, safe from both the picker and allocator - * threads. xce_sched_core_init() seeds these to AC 3's Normal-mode row - * (virtual_clock_base=0, real_clock_base=0, speed_num=1, speed_den=1): - * degraded-to-real-wall-clock default, today's existing production - * behavior, preserved until a future simulation driver (Phase 3) - * explicitly reconfigures them. + * threads. xce_sched_core_init() seeds these to a neutral + * (virtual_clock_base=0, real_clock_base=0, speed_num=1, speed_den=1) + * default -- NOT a real wall-clock anchor, despite this field's older + * doc comment claiming otherwise (bug-1584): with both anchors at 0, + * xce_core_now() degrades to raw CLOCK_MONOTONIC-since-boot + * nanoseconds, not epoch time. A real driver must seed a genuine + * wall-clock anchor itself -- the normal-mode driver does so via + * xce_core_set_wallclock_anchor() (scheduler.c's sched_init(), right + * after xce_sched_core_init()); the simulation driver does so via + * sim.c's sim_set_virtual_clock(). Only bare-core test fixtures that + * never call either keep the neutral (0,0,1,1) default. */ _Atomic uint64_t virtual_clock_base; _Atomic uint64_t real_clock_base; @@ -457,6 +463,22 @@ extern int xce_core_end_session(xce_sched_core_t*); */ extern uint64_t xce_core_now(xce_sched_core_t*); +/* + * xce_core_set_wallclock_anchor - bug-1584 fix. Seeds virtual_clock_base to + * real epoch time (CLOCK_REALTIME) and real_clock_base to the corresponding + * CLOCK_MONOTONIC reading, so a subsequent xce_core_now() tracks real + * wall-clock time from this instant -- the normal-mode driver's counterpart + * to sim.c's sim_set_virtual_clock() (which anchors virtual_clock_base to a + * `.replay` file's snapshot ts instead). xce_sched_core_init() alone does + * NOT do this -- it leaves both anchors at 0, under which xce_core_now() + * returns raw CLOCK_MONOTONIC-since-boot nanoseconds, not epoch time (see + * this file's own clock-field doc comment above, and core_clock.c's function + * header comment, for the full reasoning). Called once by scheduler.c's + * sched_init(), immediately after xce_sched_core_init() succeeds. No-op if + * core is NULL. + */ +extern void xce_core_set_wallclock_anchor(xce_sched_core_t*); + /* * xce_core_set_active_for_logging / xce_core_register_tlog_format - Req 5 * AC 3's tlog integration (task 4.5). See core_clock.c's own file header diff --git a/src/scheduler/include/repl.h b/src/scheduler/include/repl.h index 556cda38..1abdf4c2 100644 --- a/src/scheduler/include/repl.h +++ b/src/scheduler/include/repl.h @@ -95,6 +95,20 @@ extern void xce_repl_on_begin_round(xce_sched_sim_driver_t* sim, extern void xce_repl_on_decision(xce_sched_sim_driver_t* sim, xce_decision_t* d); +/* + * xce_repl_on_replay_complete (task 12.5, Req 5 AC 22 amendment, bug-1594) - + * called once, by sched_sim_driver_run() (sim_loop.c), immediately after the + * replay loop reaches EOF or the AC 20 end-of-stream marker -- the ONE point + * in the whole REPL integration that requirements.md's original pseudocode + * let fall through to process exit with no re-entry into the command loop. + * Prints the existing "[sim] replay complete" banner, marks + * sim->repl_state->replay_complete, and re-enters repl_command_loop() so the + * operator can inspect final state; the only remaining exit paths at that + * point are an explicit `quit` or stdin EOF (both already funnel through + * repl_cmd_quit()). Implemented in repl.c. + */ +extern void xce_repl_on_replay_complete(xce_sched_sim_driver_t* sim); + static inline void xce_repl_pre_replay_pause(xce_sched_sim_driver_t* sim) { if (sim->interactive) { @@ -133,4 +147,11 @@ static inline void xce_repl_check_decision(xce_sched_sim_driver_t* sim, } } +static inline void xce_repl_check_replay_complete(xce_sched_sim_driver_t* sim) +{ + if (sim->interactive) { + xce_repl_on_replay_complete(sim); + } +} + #endif diff --git a/src/scheduler/src/core.c b/src/scheduler/src/core.c index 17055cd2..82d3596f 100644 --- a/src/scheduler/src/core.c +++ b/src/scheduler/src/core.c @@ -127,15 +127,18 @@ int xce_sched_core_init(xce_sched_core_t* core) core->nextid = 1; core->configver = 0; - /* Req 5 AC 3 (scheduler-core spec, task 4.5): seed the clock atoms to - * AC 3's Normal-mode row. virtual_clock_base/real_clock_base are - * already correct from the memset() above (both 0); speed_num/ - * speed_den must be set explicitly to 1/1 -- memset() would otherwise - * leave them at 0/1, which is the FROZEN/discrete-simulation row, not - * Normal -- so a freshly initialized core degrades to real wall-clock - * time via xce_core_now() (today's existing production behavior), - * preserved until a future simulation driver (Phase 3) explicitly - * reconfigures these fields as a group. */ + /* Req 5 AC 3 (scheduler-core spec, task 4.5): seed the clock atoms to a + * neutral default. virtual_clock_base/real_clock_base are already + * correct from the memset() above (both 0); speed_num/speed_den must be + * set explicitly to 1/1 -- memset() would otherwise leave them at 0/1, + * which is the FROZEN/discrete-simulation row. This is NOT a real + * wall-clock anchor (bug-1584): with both bases at 0, xce_core_now() + * degrades to raw CLOCK_MONOTONIC-since-boot nanoseconds, not epoch + * time. A real driver seeds a genuine anchor itself -- normal mode via + * xce_core_set_wallclock_anchor() (scheduler.c's sched_init()), + * simulation mode via sim.c's sim_set_virtual_clock() -- both called + * once, right after this function returns. Bare-core test fixtures that + * call neither keep this neutral default, same as before. */ atomic_store_explicit(&(core->speed_num), 1, memory_order_release); atomic_store_explicit(&(core->speed_den), 1, memory_order_release); diff --git a/src/scheduler/src/core_clock.c b/src/scheduler/src/core_clock.c index 15eebdfd..f85f347d 100644 --- a/src/scheduler/src/core_clock.c +++ b/src/scheduler/src/core_clock.c @@ -119,6 +119,68 @@ uint64_t xce_core_now(xce_sched_core_t* core) return vbase + (elapsed * num) / den; } +/* + * xce_core_set_wallclock_anchor - bug-1584 fix. The normal-mode driver's + * counterpart to sim.c's sim_set_virtual_clock() (see that function's own + * header comment for why a driver-level clock_gettime() call is exempt from + * core.c's zero-wall-clock isolation boundary -- the same reasoning applies + * here). xce_sched_core_init() seeds virtual_clock_base/real_clock_base to + * (0,0) as a neutral default -- NOT a real wall-clock anchor, despite this + * file's and core.h's older comments claiming otherwise: with both anchors + * at 0, xce_core_now()'s elapsed computation (wall_ns - real_clock_base) + * degrades to the CLOCK_MONOTONIC reading itself, i.e. nanoseconds since an + * arbitrary reference point (typically system boot) -- NOT nanoseconds + * since the Unix epoch, which is what every consumer of xce_core_now()'s + * output for a real timestamp (recording.c's record_start_ts, every + * ReplayEvent/SnapshotEntry `ts` field, controller.capnp's + * SchedAdminReply.startedAt, documented there as "nanoseconds since epoch") + * actually needs. Left unseeded, `xceadmin scheduler record status` renders + * `started at:` as a date near 1970-01-01 (system uptime interpreted as an + * epoch offset) instead of the real start time. + * + * Two clock_gettime() reads, back to back: CLOCK_REALTIME gives the epoch + * value virtual_clock_base anchors to; CLOCK_MONOTONIC gives the + * real_clock_base reading xce_core_now()'s own elapsed computation is + * defined against (mixing a REALTIME real_clock_base with a MONOTONIC + * xce_core_now() read would drift under NTP adjustments -- this keeps the + * elapsed side monotonic-only). The nanosecond-scale gap between the two + * reads is the same inherent two-read slop sim_set_virtual_clock() already + * accepts. + * + * Called once by the normal-mode driver (scheduler.c's sched_init()) right + * after xce_sched_core_init() succeeds, before anything can observe + * xce_core_now(). Simulation mode and every core.c-including test fixture + * that constructs a core directly are unaffected -- they never call this + * function, so they keep xce_sched_core_init()'s (0,0,1,1) default (or, for + * simulation, sim_set_virtual_clock()'s own anchors) exactly as before. + */ +void xce_core_set_wallclock_anchor(xce_sched_core_t* core) +{ + struct timespec real_ts; + struct timespec mono_ts; + uint64_t epoch_ns; + uint64_t mono_ns; + + if (core == NULL) { + return; + } + + clock_gettime(CLOCK_REALTIME, &real_ts); + clock_gettime(CLOCK_MONOTONIC, &mono_ts); + + epoch_ns = ((uint64_t)real_ts.tv_sec * CORE_CLOCK_NSEC_PER_SEC) + + (uint64_t)real_ts.tv_nsec; + mono_ns = ((uint64_t)mono_ts.tv_sec * CORE_CLOCK_NSEC_PER_SEC) + + (uint64_t)mono_ts.tv_nsec; + + atomic_store_explicit(&(core->virtual_clock_base), epoch_ns, + memory_order_release); + atomic_store_explicit(&(core->real_clock_base), mono_ns, + memory_order_release); + atomic_store_explicit(&(core->speed_num), 1, memory_order_release); + atomic_store_explicit(&(core->speed_den), 1, memory_order_release); +} + /* * g_active_core_for_logging - see this file's header comment for the full * tlog_reg_format_func() signature-mismatch reasoning. NULL until a driver diff --git a/src/scheduler/src/core_events.c b/src/scheduler/src/core_events.c index ea66759b..6e9909c6 100644 --- a/src/scheduler/src/core_events.c +++ b/src/scheduler/src/core_events.c @@ -32,6 +32,38 @@ * xce_core_push_events()'s own doc comments (the public contract). */ +/* + * dup_val - deep-copies an xce_val_t (implemented in snapshot_wire.c; + * declared here via a plain `extern`, same narrow cross-.c-file idiom + * snapshot_load.c/snapshot.c already use for this exact symbol -- see + * snapshot_wire.c's own doc comment). + * + * bug-1585 root cause (live-reproduced heap-use-after-free, ASAN-confirmed + * against a real controller+scheduler+xcecd_virtual deployment, and + * regression-locked by test_resval_ownership.c): the RESVALS_FULL/ + * RESVALS_DELTA cases below call `zarray_put(&(zarray), idx, rv->val)` -- + * zarray_put_ex() (src/lib/zarray.c) stores that pointer directly + * (`array->data[idx] = data[i]`, no copy) and adopts it as the zarray's own + * property, to be handed to the zarray's killer (kill_sched_resval, + * schedobj.c) whenever this same index is later replaced. But driver.c's + * release_translated_payload() unconditionally frees the ENTIRE decoded + * ResValList/ResourceVal array (including this same `rv->val`) immediately + * after xce_core_push_events() returns, regardless of whether a value was + * adopted here. The result: the very next periodic resval report for the + * same worker+resource -- which happens every resource_sync interval on + * literally any live deployment -- evicts the now-dangling first value via + * zarray_put(), crashing the scheduler with a heap-use-after-free inside + * free_Value(). dup_val() is the same deep-copy-before-adopt fix this + * codebase already established for the identical hazard on the + * snapshot-load side (snapshot_load.c: `zarray_put(&(core->sharedInst), + * r->idx, dup_val(rv->val))` / `zarray_put(&(w->resvals), r->idx, + * dup_val(nv->val))`) -- applied here to the live event-application path, + * so the zarray owns an independent copy and driver.c's unconditional free + * of the original decoded payload never touches memory the core still + * holds. + */ +extern xce_val_t* dup_val(const xce_val_t*); + /* * core_build_job/core_build_queue/core_build_worker - construct core-owned * scheduling objects from the wire (xce_job_t/xce_queue_t/xce_worker_t) @@ -747,7 +779,11 @@ int xce_core_push_events(xce_sched_core_t* core, xce_core_event_t* events, continue; } - zarray_put(&(core->sharedInst), res->idx, rv->val); + /* bug-1585: dup_val(), not a shallow `rv->val` store -- see + * this file's own top-of-file comment on the `dup_val` + * extern declaration for the full ownership hazard this + * avoids. */ + zarray_put(&(core->sharedInst), res->idx, dup_val(rv->val)); } break; } @@ -783,7 +819,11 @@ int xce_core_push_events(xce_sched_core_t* core, xce_core_event_t* events, continue; } - zarray_put(&(w->resvals), res->idx, rv->val); + /* bug-1585: dup_val(), not a shallow `rv->val` store -- see + * this file's own top-of-file comment on the `dup_val` + * extern declaration for the full ownership hazard this + * avoids. */ + zarray_put(&(w->resvals), res->idx, dup_val(rv->val)); /* resource-value-verification spec (task 4.1): the only * place the live scheduler daemon observes and applies a @@ -829,8 +869,13 @@ int xce_core_push_events(xce_sched_core_t* core, xce_core_event_t* events, xce_sched_job_t* job = sched_get_job(core, jobid); if (job == NULL) { - WARN("%s JOB_STATE_CHANGE: unknown jobid %llu", __func__, - (unsigned long long)jobid); + char jobidStr[64]; + + /* bug-1591: see repl_info.c's repl_info_worker_one() + * comment on fmt_jobid(). */ + fmt_jobid(jobidStr, sizeof(jobidStr), jobid); + WARN("%s JOB_STATE_CHANGE: unknown jobid %s", __func__, + jobidStr); break; } @@ -864,8 +909,12 @@ int xce_core_push_events(xce_sched_core_t* core, xce_core_event_t* events, job = sched_get_job(core, updated->jobid); if (job == NULL) { - WARN("%s JOB_UPDATE: unknown jobid %llu", __func__, - (unsigned long long)updated->jobid); + char jobidStr[64]; + + /* bug-1591: see repl_info.c's repl_info_worker_one() + * comment on fmt_jobid(). */ + fmt_jobid(jobidStr, sizeof(jobidStr), updated->jobid); + WARN("%s JOB_UPDATE: unknown jobid %s", __func__, jobidStr); break; } @@ -936,8 +985,12 @@ int xce_core_push_events(xce_sched_core_t* core, xce_core_event_t* events, job = sched_get_job(core, rusages->jobid); if (job == NULL) { - WARN("%s JOB_RUSAGE: unknown jobid %llu", __func__, - (unsigned long long)rusages->jobid); + char jobidStr[64]; + + /* bug-1591: see repl_info.c's repl_info_worker_one() + * comment on fmt_jobid(). */ + fmt_jobid(jobidStr, sizeof(jobidStr), rusages->jobid); + WARN("%s JOB_RUSAGE: unknown jobid %s", __func__, jobidStr); break; } diff --git a/src/scheduler/src/driver.c b/src/scheduler/src/driver.c index d3661428..57549251 100644 --- a/src/scheduler/src/driver.c +++ b/src/scheduler/src/driver.c @@ -647,17 +647,22 @@ int xce_sched_driver_on_decision(void* driver_ctx, xce_decision_t* decision) break; } - case XCE_DECISION_PREEMPT: + case XCE_DECISION_PREEMPT: { + char jobidStr[64]; + + /* bug-1591: see repl_info.c's repl_info_worker_one() comment on + * fmt_jobid(). */ + fmt_jobid(jobidStr, sizeof(jobidStr), + (decision->u.preempt != NULL) ? decision->u.preempt->jobid + : 0); ERROR("%s PREEMPT decision publishing not yet implemented " - "(scheduler-preemption spec territory) -- jobid=%llu " + "(scheduler-preemption spec territory) -- jobid=%s " "dropped, no xcecli_preempt() client API exists to publish " "it", - __func__, - (unsigned long long)((decision->u.preempt != NULL) - ? decision->u.preempt->jobid - : 0)); + __func__, jobidStr); rc = -ENOSYS; break; + } default: /* XCE_DECISION_NONE / XCE_DECISION_NEW_SESSION / any future op: diff --git a/src/scheduler/src/main.c b/src/scheduler/src/main.c index 5b9765b0..153f3de5 100644 --- a/src/scheduler/src/main.c +++ b/src/scheduler/src/main.c @@ -30,6 +30,14 @@ const char* argp_program_bug_address = BUG_EMAIL; * short-option letters above -- same convention as the controller's * main.c, e.g. its OPT_PEERS). */ #define OPT_RECORD_DIR 256 +/* bug-1585 follow-up (scheduler-core spec tasks.md, task 9.3 review note): + * `record_snapshot_interval` (xce_scheduler_t, scheduler.h) has been read by + * recording_events.c's sched_record_write_events() since task 9.3 landed, + * but had NO way to ever be set non-zero in production -- no CLI flag, no + * admin-request field. Wired here, mirroring --record-dir's exact pattern + * (OPT_RECORD_DIR immediately above) -- same long-option-id/parse/assign + * shape, same "argv is the only way this gets provisioned" contract. */ +#define OPT_RECORD_SNAPSHOT_INTERVAL 262 /* Req 5 AC 13 (scheduler-core spec, task 11.1): the remaining simulation- * mode CLI flags task 5.2's own --sim option left unparsed ("Phase 3 -- not * yet implemented"). --sim-verify and --interactive are boolean flags (no @@ -47,6 +55,9 @@ struct arguments { char* peers; /* --peers value (optional) */ char* sim_file; /* --sim value (optional; selects simulation mode) */ char* record_dir; /* --record-dir value (optional; Req 5 AC 10, task 9.1) */ + char* record_snapshot_interval; /* --record-snapshot-interval value + * (optional; Req 5 AC 11, task 9.3 + * follow-up, bug-1585) */ /* Req 5 AC 13 (scheduler-core spec, task 11.1): the rest of simulation * mode's CLI surface, all optional except --sim itself. Raw strings * here, same as sim_file/record_dir above -- numeric/bool @@ -90,6 +101,22 @@ static struct argp_option options[] = { { "record-dir", OPT_RECORD_DIR, "DIR", 0, "Directory for scheduler session recordings " "(default: /tmp)" }, + /* Req 5 AC 11 (scheduler-core spec, task 9.3 follow-up, bug-1585): N + * events between periodic snapshot checkpoints during an active + * recording (0 = disabled, the default -- matches + * xce_scheduler_t.record_snapshot_interval's own default of 0 via + * sched_init()'s memset). Without this, a `.replay` file only ever gets + * ONE SnapshotEntry (written at `record start` time) -- necessarily + * empty of any job/queue/worker state that hadn't yet been submitted at + * that instant, since sched_record_start() runs synchronously off an + * admin request with no other state-mutation in between. A later + * `xce_scheduler -i -s FILE` load then legitimately starts at that + * empty baseline (see repl_print_banner()'s own updated comment) -- + * this flag is how an operator gets a periodically-refreshed starting + * snapshot instead. */ + { "record-snapshot-interval", OPT_RECORD_SNAPSHOT_INTERVAL, "N", 0, + "Write a fresh snapshot checkpoint every N recorded events while " + "recording is active (default: 0, disabled)" }, { 0 } }; @@ -131,6 +158,9 @@ static error_t parse_opt(int key, char* arg, struct argp_state* state) case OPT_RECORD_DIR: arguments->record_dir = arg; break; + case OPT_RECORD_SNAPSHOT_INTERVAL: + arguments->record_snapshot_interval = arg; + break; default: return ARGP_ERR_UNKNOWN; } @@ -185,6 +215,12 @@ int main(int argc, char* argv[]) * directory when --record-dir is not given -- see scheduler.h's * xce_scheduler_t.record_dir field comment. */ char* record_dir = "/tmp"; + /* Req 5 AC 11 (scheduler-core spec, task 9.3 follow-up, bug-1585): + * default when --record-snapshot-interval is not given -- matches + * xce_scheduler_t.record_snapshot_interval's own zero-value default + * (sched_init()'s memset), i.e. periodic checkpointing disabled unless + * explicitly requested. */ + uint64_t record_snapshot_interval = 0; xce_client_config_t config; xce_cluster_config_t cluster_cfg = { 0 }; const xce_cluster_config_t* cluster_cfg_ptr = NULL; @@ -208,6 +244,28 @@ int main(int argc, char* argv[]) record_dir = arguments.record_dir; } + /* Req 5 AC 11 (scheduler-core spec, task 9.3 follow-up, bug-1585): + * --record-snapshot-interval is the ONLY way record_snapshot_interval + * is ever provisioned -- same "argv is the sole source" contract as + * record_dir above. Numeric validation mirrors --sim-speed's own + * strtoull() pattern elsewhere in this function (below, in the --sim + * branch) -- rejects a non-numeric or partially-numeric argument with a + * clear error instead of silently truncating via atoi(). */ + if (arguments.record_snapshot_interval != NULL) { + char* endp = NULL; + + record_snapshot_interval + = strtoull(arguments.record_snapshot_interval, &endp, 10); + if ((endp == NULL) || (*endp != '\0') + || (endp == arguments.record_snapshot_interval)) { + fprintf(stderr, + "xce_scheduler: --record-snapshot-interval expects a " + "numeric value, got \"%s\"\n", + arguments.record_snapshot_interval); + return 1; + } + } + /* Ensure the recording directory exists. Best-effort: sched_validate_ * record_filename() will still fail with a clear message if the dir * can't be resolved after this. Silently succeeds when it already @@ -337,6 +395,14 @@ int main(int argc, char* argv[]) * task only wires the value through to a sensible home. */ sched.record_dir = record_dir; + /* Req 5 AC 11 (scheduler-core spec, task 9.3 follow-up, bug-1585): the + * resolved --record-snapshot-interval value, consumed by + * recording_events.c's sched_record_write_events() (task 9.3's own + * periodic-checkpoint mechanism, already implemented and tested -- + * this flag is the only thing that was missing to ever set it non-zero + * in a real deployment). */ + sched.record_snapshot_interval = record_snapshot_interval; + /* Req 5 AC 6 (scheduler-core spec, task 5.2): register the driver's * decision callback -- replaces main.c's former inline * `switch (decision->op)`. Registered once, before the first diff --git a/src/scheduler/src/recording_session.c b/src/scheduler/src/recording_session.c index 1aa2adc5..ae298c7c 100644 --- a/src/scheduler/src/recording_session.c +++ b/src/scheduler/src/recording_session.c @@ -169,13 +169,19 @@ void sched_record_session_add_decision(sched_record_session_ctx_t* ctx, * rationale, which this recording path mirrors: log loud, write * nothing to the recording for this decision, never fabricate wire * data for a shape this codebase cannot yet produce for real. */ - ERROR("%s PREEMPT decision recording not yet implemented " - "(scheduler-preemption spec territory) -- jobid=%llu dropped " - "from replay file", - __func__, - (unsigned long long)((decision->u.preempt != NULL) - ? decision->u.preempt->jobid - : 0)); + { + char jobidStr[64]; + + /* bug-1591: see repl_info.c's repl_info_worker_one() comment + * on fmt_jobid(). */ + fmt_jobid(jobidStr, sizeof(jobidStr), + (decision->u.preempt != NULL) ? decision->u.preempt->jobid + : 0); + ERROR("%s PREEMPT decision recording not yet implemented " + "(scheduler-preemption spec territory) -- jobid=%s " + "dropped from replay file", + __func__, jobidStr); + } free(rd); return; diff --git a/src/scheduler/src/repl.c b/src/scheduler/src/repl.c index 99e4827e..878da86d 100644 --- a/src/scheduler/src/repl.c +++ b/src/scheduler/src/repl.c @@ -2,6 +2,7 @@ #include #include #include +#include #include /* libedit (BSD) -- readline()/add_history() */ @@ -124,6 +125,24 @@ int repl_split_tokens(const char* line, char tokens[][XCE_REPL_TOKEN_LEN], p++; } tokens[n][i] = '\0'; + + /* bug-1596 (task 12.5): strip one matching pair of surrounding + * quote characters -- an operator pasting a value straight out of + * another command's own output (e.g. a jobid from `info jobs`) and + * habitually quoting it otherwise gets a silent wrong-answer + * failure downstream (parse_jobid()'s whole-string-anchored regex + * rejects the literal quote characters, and a numeric strtoull() + * fallback then parses the leading quote as a nonnumeric prefix, + * silently producing 0) instead of an intelligible error. No + * escaping/embedded-space support -- just symmetric outer-quote + * removal, sufficient for every argument this REPL's commands + * accept (jobids, breakpoint filters, names). */ + if ((i >= 2) && (tokens[n][0] == tokens[n][i - 1]) + && ((tokens[n][0] == '"') || (tokens[n][0] == '\''))) { + memmove(&(tokens[n][0]), &(tokens[n][1]), (size_t)(i - 2)); + tokens[n][i - 2] = '\0'; + } + n++; } @@ -176,12 +195,49 @@ static int repl_count_jobs(xce_sched_core_t* core) return count; } +/* + * repl_format_ts_human - bug-1587 fix: renders a raw xce_core_now()-style + * nanoseconds-since-epoch value as a human-readable UTC timestamp, purely a + * calendar conversion (gmtime_r()) with no clock read of its own -- same + * "not a wall-clock call" reasoning core_clock.c's ns_to_tlog_time() + * documents for the identical conversion. `sim->snapshot_ts` is genuinely + * epoch nanoseconds for a normal-mode-recorded .replay file (bug-1584); a + * simulation-authored .replay file's snapshot ts is whatever epoch value + * its own source core was anchored to when it wrote the file, so this + * rendering is well-defined either way -- never misleading, at worst an + * arbitrary-looking date for a purely synthetic recording. + */ +void repl_format_ts_human(uint64_t ns, char* buf, size_t bufsz) +{ + time_t secs = (time_t)(ns / 1000000000ULL); + struct tm gmt; + + gmtime_r(&secs, &gmt); + strftime(buf, bufsz, "%Y-%m-%dT%H:%M:%SZ", &gmt); +} + static void repl_print_banner(const xce_sched_sim_driver_t* sim) { - int njobs = repl_count_jobs((xce_sched_core_t*)&(sim->core)); + int njobs = repl_count_jobs((xce_sched_core_t*)&(sim->core)); + char ts_human[32]; - fprintf(stdout, "[sim] loaded: %d jobs, snapshot @ ts=%llu\n", njobs, - (unsigned long long)sim->snapshot_ts); + repl_format_ts_human(sim->snapshot_ts, ts_human, sizeof(ts_human)); + + fprintf(stdout, "[sim] loaded: %d jobs, snapshot @ ts=%llu (%s)\n", njobs, + (unsigned long long)sim->snapshot_ts, ts_human); + /* bug-1585: this count is the LOADED SNAPSHOT's state only -- whichever + * SnapshotEntry sim_locate_and_load_snapshot() picked (the most recent + * one at/before --sim-start-ts, normally the file's last). A recording + * with no periodic checkpointing (--record-snapshot-interval unset/0 on + * the recording scheduler) only ever has ONE SnapshotEntry, written at + * `record start` time -- often before any job was submitted, so 0 jobs + * here is expected, not a bug. Jobs/queues/workers/resvals that arrived + * via the recorded event stream AFTER this snapshot only appear once + * you `step`/`continue` past them -- querying `info jobs`/`info + * summary` immediately here only ever shows this starting baseline. */ + fprintf(stdout, + "[sim] (jobs above reflect this snapshot only -- step/continue " + "to replay later events)\n"); fprintf(stdout, "[sim] ready. type 'help' for commands.\n"); } @@ -314,6 +370,21 @@ static bool repl_dispatch(xce_sched_sim_driver_t* sim, const char* line) return false; /* whitespace-only -- silently re-prompt */ } + if ((strcmp(cmd, "step") == 0) || (strcmp(cmd, "continue") == 0)) { + /* bug-1594 (task 12.5): once replay has already reached EOF/end-of- + * stream, xce_repl_on_replay_complete() (called from sim_loop.c) is + * blocked in a NESTED repl_command_loop() -- returning true here + * would unwind that nested loop straight back to + * sched_sim_driver_run()'s return, silently exiting the process + * exactly as before this fix. Re-prompt instead; only `quit`/EOF + * may end the process past this point. */ + if (sim->repl_state->replay_complete) { + fprintf(stdout, "[sim] replay already complete -- no more events " + "to process. Use 'quit' to exit.\n"); + return false; + } + } + if (strcmp(cmd, "step") == 0) { return repl_cmd_step(sim, line); } @@ -430,3 +501,31 @@ void xce_repl_post_session_check(xce_sched_sim_driver_t* sim) repl_command_loop(sim); } } + +/* + * xce_repl_on_replay_complete - see repl.h's doc comment for the full + * rationale (task 12.5, Req 5 AC 22 amendment, bug-1594). Called exactly + * once, after the replay loop's last event/session has been processed. + * Blocks in repl_command_loop() until `quit`/EOF -- never returns while the + * process is still running. + */ +void xce_repl_on_replay_complete(xce_sched_sim_driver_t* sim) +{ + xce_sched_repl_state_t* rs = sim->repl_state; + + if (rs == NULL) { + return; + } + + fprintf(stdout, "[sim] replay complete. %llu session(s) processed.\n", + (unsigned long long)rs->session_count); + if (rs->output_path != NULL) { + fprintf(stdout, "[sim] output written to: %s\n", rs->output_path); + } + fprintf(stdout, "[sim] simulation finished -- state remains available " + "for inspection (info job/queue/worker/resource/summary). type " + "'quit' to exit.\n"); + + rs->replay_complete = true; + repl_command_loop(sim); +} diff --git a/src/scheduler/src/repl_break.c b/src/scheduler/src/repl_break.c index ac72b89b..9575d9a1 100644 --- a/src/scheduler/src/repl_break.c +++ b/src/scheduler/src/repl_break.c @@ -560,6 +560,117 @@ bool repl_cmd_delete(xce_sched_sim_driver_t* sim, const char* line) return false; } +/* + * repl_print_event_summary - task 12.5 (Req 5 AC 22 amendment, bug-1595): + * one human-readable line per decoded ReplayEvent, printed UNCONDITIONALLY + * (not gated on any breakpoint match) so `step`/`continue` give the + * operator visibility into what actually happened before the resulting + * decisions appear -- v1 only ever echoed decisions (sim_print_decision(), + * sim_loop.c), leaving every input event silent unless a breakpoint + * happened to match it. Only ever called from xce_repl_on_event() below, + * itself only reached when sim->interactive (repl.h's own gate) -- zero + * output added to batch `--sim` mode. + */ +static void repl_print_event_summary(const xce_core_event_t* ev) +{ + switch (ev->type) { + case XCE_CORE_EVENT_CONFIG_PUSH: + fprintf(stdout, "[sim] event: CONFIG_PUSH\n"); + break; + + case XCE_CORE_EVENT_JOBS_PUSH: { + int i; + + fprintf(stdout, "[sim] event: JOB_PUSH njobs=%d jobids=[", + ev->u.jobs_push.njobs); + for (i = 0; i < ev->u.jobs_push.njobs; i++) { + xce_job_t* j = ev->u.jobs_push.jobs[i]; + char jobidStr[64]; + + if (j == NULL) { + continue; + } + fmt_jobid(jobidStr, sizeof(jobidStr), j->jobid); + fprintf(stdout, "%s%s", (i > 0) ? "," : "", jobidStr); + } + fprintf(stdout, "]\n"); + break; + } + + case XCE_CORE_EVENT_WORKERS_PUSH: { + int i; + + fprintf(stdout, "[sim] event: WORKERS_PUSH nworkers=%d names=[", + ev->u.workers_push.nworkers); + for (i = 0; i < ev->u.workers_push.nworkers; i++) { + xce_worker_t* w = ev->u.workers_push.workers[i]; + + if (w == NULL) { + continue; + } + fprintf(stdout, "%s%s", (i > 0) ? "," : "", + (w->name != NULL) ? w->name : "(unnamed)"); + } + fprintf(stdout, "]\n"); + break; + } + + case XCE_CORE_EVENT_RESOURCES_PUSH: + fprintf(stdout, "[sim] event: RESOURCES_PUSH n=%d\n", + ev->u.resources_push.n); + break; + + case XCE_CORE_EVENT_RESVALS_FULL: + fprintf(stdout, "[sim] event: RESVALS_FULL n=%d\n", + ev->u.resvals_full.n); + break; + + case XCE_CORE_EVENT_RESVALS_DELTA: + fprintf(stdout, "[sim] event: RESVALS_DELTA n=%d\n", + (ev->u.resvals_delta.deltas != NULL) + ? ev->u.resvals_delta.deltas->count + : 0); + break; + + case XCE_CORE_EVENT_JOB_STATE_CHANGE: { + char jobidStr[64]; + + fmt_jobid(jobidStr, sizeof(jobidStr), ev->u.job_state_change.jobid); + fprintf(stdout, "[sim] event: JOB_STATE_CHANGE jobid=%s state=%s\n", + jobidStr, + repl_job_state_name(ev->u.job_state_change.new_state)); + break; + } + + case XCE_CORE_EVENT_JOB_UPDATE: { + char jobidStr[64]; + + fmt_jobid(jobidStr, sizeof(jobidStr), ev->u.job_update.jobid); + fprintf(stdout, "[sim] event: JOB_UPDATE jobid=%s\n", jobidStr); + break; + } + + case XCE_CORE_EVENT_QUEUE_UPDATE: + fprintf(stdout, "[sim] event: QUEUE_UPDATE queue=%s\n", + (ev->u.queue_update.queue != NULL) + ? ev->u.queue_update.queue->queue + : "(unknown)"); + break; + + case XCE_CORE_EVENT_JOB_RUSAGE: { + char jobidStr[64]; + + fmt_jobid(jobidStr, sizeof(jobidStr), ev->u.job_rusage.jobid); + fprintf(stdout, "[sim] event: JOB_RUSAGE jobid=%s\n", jobidStr); + break; + } + + default: + fprintf(stdout, "[sim] event: (unrecognized type %d)\n", (int)ev->type); + break; + } +} + /* * xce_repl_on_event - the per-decoded-ReplayEvent breakpoint-check hook * (task 12.2, Req 5 AC 22's `check_breakpoints(event)` pseudocode call, @@ -585,6 +696,11 @@ void xce_repl_on_event(xce_sched_sim_driver_t* sim, xce_core_event_t* ev) rs->event_index++; + /* task 12.5, bug-1595: unconditional echo, printed before the + * breakpoint check below so the operator sees "event happened" before + * (if it also matches) "breakpoint hit". */ + repl_print_event_summary(ev); + bp = repl_find_event_breakpoint(rs, ev); if (bp != NULL) { fprintf(stdout, "[sim] hit breakpoint %d @ event #%llu: %s\n", bp->id, @@ -662,8 +778,13 @@ void xce_repl_on_decision(xce_sched_sim_driver_t* sim, xce_decision_t* d) bp = repl_find_decision_breakpoint(&(sim->core), rs, d); if (bp != NULL) { - fprintf(stdout, "[sim] hit breakpoint %d @ decision: %s jobid=%llu\n", bp->id, - decision_op_name(d->op), (unsigned long long)decision_jobid(d)); + char jobidStr[64]; + + /* bug-1591: see repl_info.c's repl_info_worker_one() comment on + * fmt_jobid() -- same fix, same rationale. */ + fmt_jobid(jobidStr, sizeof(jobidStr), decision_jobid(d)); + fprintf(stdout, "[sim] hit breakpoint %d @ decision: %s jobid=%s\n", + bp->id, decision_op_name(d->op), jobidStr); repl_command_loop(sim); } } diff --git a/src/scheduler/src/repl_info.c b/src/scheduler/src/repl_info.c index 3335d8ab..eb4c853b 100644 --- a/src/scheduler/src/repl_info.c +++ b/src/scheduler/src/repl_info.c @@ -66,8 +66,10 @@ extern const char* find_resource_name_by_idx(xce_resource_t**, int, int); * most "advanced" lifecycle bit wins, mirroring repl_break.c's own * repl_job_state_matches() check order (used by `break ... state=` * filters) so a filter and this display never disagree about which single - * word describes a given state. */ -static const char* repl_job_state_name(uint32_t state) + * word describes a given state. Non-static (repl_internal.h) since task + * 12.5's per-event summary line (repl_break.c) shares it for + * JOB_STATE_CHANGE. */ +const char* repl_job_state_name(uint32_t state) { if (JOB_IS_RUN(state)) { return "RUN"; @@ -227,8 +229,13 @@ static void repl_info_queue_one(xce_sched_sim_driver_t* sim, } if ((job->queue != NULL) && (strcmp(job->queue->name, q->name) == 0) && JOB_IS_PEND(job->state)) { - fprintf(stdout, "[sim] queued job: jobid=%llu priority=%d\n", - (unsigned long long)job->jobid, job->priority); + char jobidStr[64]; + + /* bug-1591: see repl_info_worker_one()'s own comment on + * fmt_jobid() (below) -- same fix, same rationale. */ + fmt_jobid(jobidStr, sizeof(jobidStr), job->jobid); + fprintf(stdout, "[sim] queued job: jobid=%s priority=%d\n", + jobidStr, job->priority); } } iter_destroy(&it); @@ -315,23 +322,57 @@ static void repl_info_worker_one(xce_sched_sim_driver_t* sim, xce_val_t* avail_v = (xce_val_t*)zarray_get(&(w->resvals), i); xce_val_t* used_v = (xce_val_t*)zarray_get(&(w->resInUse), i); const char* rname = find_resource_name_by_idx(res_arr, res_n, i); - double avail; - double used; if ((avail_v == NULL) || (rname == NULL)) { continue; } - /* bug-1568: xce_val_t.which holds a Value_u_which tag - * (controller.capnp.h), NOT an XCE_VAL_* resource-definition - * value-type constant (xce/types.h, a different enum for - * xce_resource_t.valtype) -- comparing against the wrong one here - * made every genuinely-numeric resval silently fall back to 0.0. */ - avail = (avail_v->which == Value_u_fvalue) ? avail_v->u.fvalue : 0.0; - used = ((used_v != NULL) && (used_v->which == Value_u_fvalue)) - ? used_v->u.fvalue - : 0.0; - fprintf(stdout, "[sim] resource %s: total=%.2f available=%.2f\n", rname, - avail + used, avail); + + /* bug-1590: xce_val_t.which (a Value_u_which wire tag, + * controller.capnp.h) tells us the value's REAL type -- bug-1568 + * fixed comparing it against the wrong enum for the numeric case, + * but left every non-numeric resource (Value_u_svalue/ + * Value_u_bvalue -- e.g. this worker's `type`/`model` string + * resources, or any boolean-typed resource) silently coerced to + * 0.0 by the old unconditional %.2f formatting below. "total"/ + * "available" (a consumable-quantity concept: how much of this + * worker's capacity is free vs. in use) is only meaningful for a + * NUMERIC resource -- a string or boolean resource is a single + * static-ish per-worker attribute with no "in use" counterpart, so + * it gets its own "value=" line instead of a fabricated + * total/available pair. */ + switch (avail_v->which) { + case Value_u_svalue: + fprintf(stdout, "[sim] resource %s: value=%s\n", rname, + avail_v->u.svalue ? avail_v->u.svalue : "(null)"); + break; + + case Value_u_bvalue: + fprintf(stdout, "[sim] resource %s: value=%s\n", rname, + avail_v->u.bvalue ? "true" : "false"); + break; + + case Value_u_fvalue: { + double avail = avail_v->u.fvalue; + double used = ((used_v != NULL) && (used_v->which == Value_u_fvalue)) + ? used_v->u.fvalue + : 0.0; + + fprintf(stdout, + "[sim] resource %s: total=%.2f available=%.2f\n", rname, + avail + used, avail); + break; + } + + default: + /* ivalue32/uvalue32/ivalue64/uvalue64 -- not a shape this + * codebase's resource system currently produces for a resval + * (xce_resource_t.valtype, xce/types.h, only ever numeric/ + * string/boolean); reported honestly rather than guessed at, + * same "silently skipped ... rather than guessed at" posture + * core_events.c's bug-1573 fix already established for this + * identical ambiguity. */ + break; + } } free(res_arr); @@ -353,8 +394,16 @@ static void repl_info_worker_one(xce_sched_sim_driver_t* sim, if ((ha != NULL) && (ha->host != NULL) && (strcmp(ha->host, w->name) == 0)) { - fprintf(stdout, "[sim] assigned job: jobid=%llu\n", - (unsigned long long)job->jobid); + char jobidStr[64]; + + /* bug-1591: fmt_jobid() renders the raw idgen()-packed + * jobid in the same human-readable form already used by + * every other jobid display in this codebase (scheduler.c/ + * handler.c/plugins, worker, xce_test's waldump, and + * job.c's log lines since bug-1588) -- this REPL command + * was one of the few remaining raw-integer holdouts. */ + fmt_jobid(jobidStr, sizeof(jobidStr), job->jobid); + fprintf(stdout, "[sim] assigned job: jobid=%s\n", jobidStr); break; } } @@ -396,109 +445,131 @@ static void repl_info_worker(xce_sched_sim_driver_t* sim, const char* arg) } } +/* XCE_RESF_FLAG_TABLE - every xce/types.h XCE_RESF_* bit, human name. + * Shared by repl_info_resource_one()'s metadata line (task 12.5, bug-1593). */ +static const struct { + int bit; + const char* name; +} XCE_RESF_FLAG_TABLE[] = { + { XCE_RESF_BUILTIN, "BUILTIN" }, { XCE_RESF_DYNAMIC, "DYNAMIC" }, + { XCE_RESF_GLOBAL, "GLOBAL" }, { XCE_RESF_SHARED, "SHARED" }, + { XCE_RESF_EXTERNAL, "EXTERNAL" }, { XCE_RESF_RELEASE, "RELEASE" }, + { XCE_RESF_DEFAULT_HOST, "DEFAULT_HOST" }, + { XCE_RESF_CONSUMABLE, "CONSUMABLE" }, { XCE_RESF_LIMIT, "LIMIT" }, +}; + +/* repl_resource_flags_str - renders `flags` as a comma-joined list of the + * XCE_RESF_* names it carries ("GLOBAL,SHARED"), or "(none)" if none set. */ +static void repl_resource_flags_str(int flags, char* buf, size_t bufsz) +{ + size_t i; + size_t n = 0; + + buf[0] = '\0'; + for (i = 0; i < (sizeof(XCE_RESF_FLAG_TABLE) / sizeof(XCE_RESF_FLAG_TABLE[0])); + i++) { + int written; + + if ((flags & XCE_RESF_FLAG_TABLE[i].bit) == 0) { + continue; + } + written = snprintf(buf + n, bufsz - n, "%s%s", (n > 0) ? "," : "", + XCE_RESF_FLAG_TABLE[i].name); + if ((written > 0) && ((size_t)written < (bufsz - n))) { + n += (size_t)written; + } + } + if (n == 0) { + snprintf(buf, bufsz, "(none)"); + } +} + /* - * repl_info_resource_one - AC 22's "info resource ": "Dump one - * resource: type, total capacity, available, allocation count." `r` is the - * DEFINITION object (xce/types.h's xce_resource_t -- valtype/ordtype/idx, - * no per-worker numbers of its own, per snapshot.c's own "resources -> - * core->resmgr ... already the wire type natively" note); total/available - * are aggregated across every worker's resvals/resInUse at `r->idx`, the - * same idx-indexed convention repl_info_worker_one() above already - * establishes. "allocation count" counts distinct jobs currently holding - * any per-host xce_alloc_t naming this resource (xce/types.h). + * repl_info_resource_one - AC 22 amendment (task 12.5, bug-1593): "info + * resource " reports the resource DEFINITION's own metadata (desc, + * value type, XCE_RESF_* flags, reporting interval) -- never a per-host + * aggregation, which is repl_info_worker_one()'s job. A resource flagged + * XCE_RESF_GLOBAL or XCE_RESF_SHARED additionally gets its current value, + * read directly from the core's single defining pool (`core->sharedInst`, + * indexed by `r->idx` -- the same pool RESVALS_FULL/RESVALS_DELTA event + * handling and snapshot save/load already treat as authoritative for these + * resources, core_events.c/snapshot.c). A host-based resource (the common + * case -- cpus, mem, ...) has no single cluster-wide value to report here; + * its per-host values remain `info worker `'s job. */ static void repl_info_resource_one(xce_sched_sim_driver_t* sim, xce_resource_t* r) { - iterator_t it; - xce_sched_worker_t* w = NULL; - xce_sched_job_t* job; - double total = 0.0; - double available = 0.0; - int nallocs = 0; - int rc; + char flags_str[160]; + const char* valtype_str; - rc = iter_make(&it, sim->core.workers); - if (rc >= 0) { - for (;;) { - xce_val_t* avail_v; - xce_val_t* used_v; + switch (r->valtype) { + case XCE_VAL_STRING: + valtype_str = "string"; + break; + case XCE_VAL_BOOLEAN: + valtype_str = "boolean"; + break; + case XCE_VAL_NUMERIC: + default: + valtype_str = "numeric"; + break; + } + repl_resource_flags_str(r->flags, flags_str, sizeof(flags_str)); - rc = iter_next(&it, (void**)&w); - if ((rc < 0) || (rc == RTTI_ITER_NO_DATA)) { - break; - } - if (r->idx >= w->resvals.count) { - continue; - } - avail_v = (xce_val_t*)zarray_get(&(w->resvals), r->idx); - used_v = (xce_val_t*)zarray_get(&(w->resInUse), r->idx); - /* bug-1568: same Value_u_fvalue vs XCE_VAL_NUMERIC type-tag - * mismatch as repl_info_worker_one() above. */ - if ((avail_v != NULL) && (avail_v->which == Value_u_fvalue)) { - available += avail_v->u.fvalue; - total += avail_v->u.fvalue; - } - if ((used_v != NULL) && (used_v->which == Value_u_fvalue)) { - total += used_v->u.fvalue; - } - } - iter_destroy(&it); + fprintf(stdout, + "[sim] resource %s: desc=%s valtype=%s flags=%s interval=%d\n", + r->name, (r->desc != NULL) ? r->desc : "(none)", valtype_str, + flags_str, r->interval); + + if ((r->flags & (XCE_RESF_GLOBAL | XCE_RESF_SHARED)) == 0) { + fprintf(stdout, "[sim] host-based resource -- see 'info worker " + "' for per-host values\n"); + return; } - job = NULL; - rc = iter_make(&it, sim->core.jobs); - if (rc >= 0) { - for (;;) { - int i; - int matched = 0; - - rc = iter_next(&it, (void**)&job); - if ((rc < 0) || (rc == RTTI_ITER_NO_DATA)) { - break; - } - if (job->allocs == NULL) { - continue; - } - for (i = 0; i < job->allocs->nhosts; i++) { - xce_host_alloc_t* ha = job->allocs->allocs[i]; - int j; - - if (ha == NULL) { - continue; - } - for (j = 0; j < ha->nallocs; j++) { - if ((ha->allocs[j] != NULL) - && (ha->allocs[j]->resname != NULL) - && (strcmp(ha->allocs[j]->resname, r->name) == 0)) { - matched = 1; - break; - } - } - if (matched) { - break; - } - } - if (matched) { - nallocs++; - } - } - iter_destroy(&it); + if ((r->idx < 0) || (r->idx >= sim->core.sharedInst.count)) { + fprintf(stdout, "[sim] value=(none)\n"); + return; } - fprintf(stdout, "[sim] resource %s: valtype=%s total=%.2f available=%.2f " - "allocation_count=%d\n", - r->name, - (r->valtype == XCE_VAL_NUMERIC) - ? "numeric" - : (r->valtype == XCE_VAL_STRING) ? "string" : "boolean", - total, available, nallocs); + { + xce_val_t* v = (xce_val_t*)zarray_get(&(sim->core.sharedInst), r->idx); + + if (v == NULL) { + fprintf(stdout, "[sim] value=(none)\n"); + return; + } + + switch (v->which) { + case Value_u_svalue: + fprintf(stdout, "[sim] value=%s\n", + v->u.svalue ? v->u.svalue : "(null)"); + break; + case Value_u_bvalue: + fprintf(stdout, "[sim] value=%s\n", + v->u.bvalue ? "true" : "false"); + break; + case Value_u_fvalue: + fprintf(stdout, "[sim] value=%.2f\n", v->u.fvalue); + break; + default: + fprintf(stdout, "[sim] value=(unrecognized wire type)\n"); + break; + } + } } /* repl_info_resource - `info resource ` dispatch. resmgr has no * exported sched_get_*() accessor (unlike jobs/queues/workers, core.h) -- * key_find() directly, the same accessor core_events.c's own - * QUEUE_UPDATE/RESOURCES_PUSH handling already uses against core->resmgr. */ + * QUEUE_UPDATE/RESOURCES_PUSH handling already uses against core->resmgr. + * bug-1593 (task 12.5): `all` is scoped to cluster-wide (GLOBAL/SHARED) + * resources only -- a host-based resource never appears in this listing, it + * remains `info worker`'s job. A named lookup (`info resource `) + * still resolves any resource, host-based included, so an operator can + * check its metadata/flags even though no cluster-wide value is shown for + * it (repl_info_resource_one()'s own host-based branch above). */ static void repl_info_resource(xce_sched_sim_driver_t* sim, const char* arg) { if (strcmp(arg, "all") == 0) { @@ -514,6 +585,9 @@ static void repl_info_resource(xce_sched_sim_driver_t* sim, const char* arg) if ((rc < 0) || (rc == RTTI_ITER_NO_DATA)) { break; } + if ((r->flags & (XCE_RESF_GLOBAL | XCE_RESF_SHARED)) == 0) { + continue; + } repl_info_resource_one(sim, r); } iter_destroy(&it); @@ -634,9 +708,14 @@ static void repl_info_summary(xce_sched_sim_driver_t* sim) static void repl_info_session(xce_sched_sim_driver_t* sim) { xce_sched_repl_state_t* rs = sim->repl_state; + char ts_human[32]; - fprintf(stdout, "[sim] session: ts=%llu event_index=%llu sessions=%llu\n", - (unsigned long long)xce_core_now(&(sim->core)), + repl_format_ts_human(xce_core_now(&(sim->core)), ts_human, + sizeof(ts_human)); + + fprintf(stdout, + "[sim] session: ts=%llu (%s) event_index=%llu sessions=%llu\n", + (unsigned long long)xce_core_now(&(sim->core)), ts_human, (unsigned long long)((rs != NULL) ? rs->event_index : 0), (unsigned long long)((rs != NULL) ? rs->session_count : 0)); @@ -669,6 +748,7 @@ static void repl_info_config(xce_sched_sim_driver_t* sim) { xce_sched_repl_state_t* rs = sim->repl_state; const char* output_desc; + char ts_human[32]; if (sim->record_file == NULL) { output_desc = "off"; @@ -678,12 +758,15 @@ static void repl_info_config(xce_sched_sim_driver_t* sim) output_desc = "(active, path unknown)"; } + repl_format_ts_human(xce_core_now(&(sim->core)), ts_human, + sizeof(ts_human)); + fprintf(stdout, "[sim] config: speed=%llu output=%s verify=%s " - "events_processed=%llu current_ts=%llu sessions=%llu\n", + "events_processed=%llu current_ts=%llu (%s) sessions=%llu\n", (unsigned long long)sim->sim_speed_num, output_desc, sim->verify ? "on" : "off", (unsigned long long)((rs != NULL) ? rs->event_index : 0), - (unsigned long long)xce_core_now(&(sim->core)), + (unsigned long long)xce_core_now(&(sim->core)), ts_human, (unsigned long long)((rs != NULL) ? rs->session_count : 0)); } diff --git a/src/scheduler/src/repl_internal.h b/src/scheduler/src/repl_internal.h index 57e552eb..a0b79078 100644 --- a/src/scheduler/src/repl_internal.h +++ b/src/scheduler/src/repl_internal.h @@ -172,6 +172,19 @@ struct xce_sched_repl_state_s { * event_index to report "N events * written". Meaningless while * output_path == NULL. */ + + /* + * Task 12.5 field (Req 5 AC 22 amendment, bug-1594): set once, by + * xce_repl_on_replay_complete() (repl.c), the moment sim_loop.c's + * replay loop reaches EOF/end-of-stream under --interactive. Once set, + * repl_dispatch()'s `step`/`continue` branches stop treating those + * commands as "hand control back to the driver loop" (there is no more + * driver loop to hand control to -- xce_repl_on_replay_complete() is + * itself already blocked inside a nested repl_command_loop() call at + * that point) and instead print an informational re-prompt. Never + * cleared -- a REPL session only ever reaches replay completion once. + */ + bool replay_complete; }; /* repl_split_tokens/XCE_REPL_MAX_TOKENS/XCE_REPL_TOKEN_LEN (repl.c) - @@ -184,6 +197,13 @@ struct xce_sched_repl_state_s { extern int repl_split_tokens(const char* line, char tokens[][XCE_REPL_TOKEN_LEN], int max_tokens); +/* repl_format_ts_human (repl.c) - bug-1587 fix: renders a raw + * xce_core_now()-style nanoseconds-since-epoch value as a human-readable + * UTC timestamp ("YYYY-MM-DDTHH:MM:SSZ") into buf (must be >= 21 bytes). + * Shared by every REPL/sim `ts=` print site (repl.c/repl_info.c/ + * repl_params.c) so they render consistently. */ +extern void repl_format_ts_human(uint64_t ns, char* buf, size_t bufsz); + /* repl_command_loop (repl.c) - the blocking prompt loop. Breakpoint hits * (repl_break.c's xce_repl_on_event()/on_begin_round()/on_decision()) call * back into it directly, the same re-entry xce_repl_post_session_check() @@ -207,4 +227,11 @@ extern bool repl_cmd_info(xce_sched_sim_driver_t* sim, const char* line); extern bool repl_cmd_snapshot(xce_sched_sim_driver_t* sim, const char* line); extern bool repl_cmd_set(xce_sched_sim_driver_t* sim, const char* line); +/* repl_job_state_name (repl_info.c) - single readable label ("RUN"/"PEND"/ + * "EXIT"/"DONE"/"UNKNOWN") for a job's state bitmask. Shared with + * repl_break.c's task 12.5 per-event summary line (JOB_STATE_CHANGE) so + * `info job` and the event echo never disagree about which word describes + * a given state. */ +extern const char* repl_job_state_name(uint32_t state); + #endif diff --git a/src/scheduler/src/repl_params.c b/src/scheduler/src/repl_params.c index 3ad2d182..b92de91f 100644 --- a/src/scheduler/src/repl_params.c +++ b/src/scheduler/src/repl_params.c @@ -156,8 +156,13 @@ bool repl_cmd_snapshot(xce_sched_sim_driver_t* sim, const char* line) return false; } - fprintf(stdout, "[sim] snapshot written: %s (ts=%llu)\n", path, - (unsigned long long)ts); + { + char ts_human[32]; + + repl_format_ts_human(ts, ts_human, sizeof(ts_human)); + fprintf(stdout, "[sim] snapshot written: %s (ts=%llu, %s)\n", path, + (unsigned long long)ts, ts_human); + } return false; } diff --git a/src/scheduler/src/scheduler.c b/src/scheduler/src/scheduler.c index 06b90b63..84d59e6b 100644 --- a/src/scheduler/src/scheduler.c +++ b/src/scheduler/src/scheduler.c @@ -655,6 +655,17 @@ int sched_init(xce_scheduler_t* sched, xce_client_config_t* config, return rc; } + /* bug-1584: xce_sched_core_init() alone leaves the clock atoms at the + * neutral (0,0,1,1) default, under which xce_core_now() returns raw + * CLOCK_MONOTONIC-since-boot nanoseconds, not epoch time -- every + * timestamp this normal-mode driver stamps via xce_core_now() + * (recording.c's record_start_ts, every recorded ReplayEvent/ + * SnapshotEntry ts, SchedAdminReply.startedAt) would otherwise render + * as a date near 1970-01-01. Seed a real wall-clock anchor here, the + * normal-mode counterpart to sim.c's sim_set_virtual_clock() -- before + * anything below can possibly call xce_core_now(). */ + xce_core_set_wallclock_anchor(&(sched->core)); + sched->core.pend_updates[0] = (dataq_t*)calloc(1, sizeof(dataq_t)); dataq_init(sched->core.pend_updates[0], sched_pend_update_killer); diff --git a/src/scheduler/src/sim_loop.c b/src/scheduler/src/sim_loop.c index ac9c8e2d..23b35730 100644 --- a/src/scheduler/src/sim_loop.c +++ b/src/scheduler/src/sim_loop.c @@ -374,18 +374,23 @@ extern void sim_bootstrap_picker(xce_sched_sim_driver_t* sim); */ static void sim_print_decision(const xce_decision_t* d) { + char jobidStr[64]; + + /* bug-1591: fmt_jobid() renders the raw idgen()-packed jobid in the + * same human-readable form already used elsewhere in this codebase + * (see repl_info.c's repl_info_worker_one() comment). */ switch (d->op) { case XCE_DECISION_DISPATCH: - printf("[sim] DISPATCH jobid=%llu\n", - (unsigned long long)d->u.alloc->jobid); + fmt_jobid(jobidStr, sizeof(jobidStr), d->u.alloc->jobid); + printf("[sim] DISPATCH jobid=%s\n", jobidStr); break; case XCE_DECISION_PEND: - printf("[sim] PEND jobid=%llu\n", - (unsigned long long)d->u.reason->jobid); + fmt_jobid(jobidStr, sizeof(jobidStr), d->u.reason->jobid); + printf("[sim] PEND jobid=%s\n", jobidStr); break; case XCE_DECISION_PREEMPT: - printf("[sim] PREEMPT jobid=%llu\n", - (unsigned long long)d->u.preempt->jobid); + fmt_jobid(jobidStr, sizeof(jobidStr), d->u.preempt->jobid); + printf("[sim] PREEMPT jobid=%s\n", jobidStr); break; default: break; @@ -741,6 +746,12 @@ int sched_sim_driver_run(xce_sched_sim_driver_t* sim, break; } + /* Req 5 AC 22 amendment (task 12.5, bug-1594): under --interactive, park + * in the REPL instead of falling straight through to process exit -- + * see repl.h's doc comment on xce_repl_on_replay_complete(). No-op in + * batch mode. */ + xce_repl_check_replay_complete(sim); + /* Req 5 AC 19 (task 11.5): flush+close the --sim-output file on a clean * run completion so its contents are durably on disk before this * function returns -- no sim-driver "stop"/fini function exists to do diff --git a/src/scheduler/src/sim_verify.c b/src/scheduler/src/sim_verify.c index f9603a30..a2f8f185 100644 --- a/src/scheduler/src/sim_verify.c +++ b/src/scheduler/src/sim_verify.c @@ -288,14 +288,20 @@ int sim_decision_eq(const replay_decision_t* expected, * could never emit for real cannot be meaningfully structurally * verified either -- log loud and report a mismatch, rather than * silently treating an unreachable-today comparison as a "pass". */ - ERROR("%s PREEMPT decision comparison is not supported " - "(scheduler-preemption spec territory -- no live path " - "produces this decision type today) -- jobid=%llu reported " - "as a mismatch", - __func__, - (unsigned long long)((actual->u.preempt != NULL) - ? actual->u.preempt->jobid - : 0ULL)); + { + char jobidStr[64]; + + /* bug-1591: see repl_info.c's repl_info_worker_one() comment + * on fmt_jobid(). */ + fmt_jobid(jobidStr, sizeof(jobidStr), + (actual->u.preempt != NULL) ? actual->u.preempt->jobid + : 0ULL); + ERROR("%s PREEMPT decision comparison is not supported " + "(scheduler-preemption spec territory -- no live path " + "produces this decision type today) -- jobid=%s reported " + "as a mismatch", + __func__, jobidStr); + } return 0; default: @@ -319,11 +325,17 @@ static void sim_verify_describe_replay_decision(const replay_decision_t* d, return; } + /* bug-1591: fmt_jobid() renders every jobid below in the same + * human-readable form already used elsewhere in this codebase (see + * repl_info.c's repl_info_worker_one() comment). */ switch (d->body_which) { case ReplayDecision_body_dispatch: if ((d->body.dispatch != NULL) && (d->body.dispatch->alloc != NULL)) { - snprintf(buf, buf_sz, "DISPATCH jobid=%llu nhosts=%d", - (unsigned long long)d->body.dispatch->alloc->jobid, + char jobidStr[64]; + + fmt_jobid(jobidStr, sizeof(jobidStr), + d->body.dispatch->alloc->jobid); + snprintf(buf, buf_sz, "DISPATCH jobid=%s nhosts=%d", jobidStr, d->body.dispatch->alloc->nhosts); } else { snprintf(buf, buf_sz, "DISPATCH "); @@ -332,8 +344,10 @@ static void sim_verify_describe_replay_decision(const replay_decision_t* d, case ReplayDecision_body_pend: if (d->body.pend != NULL) { - snprintf(buf, buf_sz, "PEND jobid=%llu main_reason=%d", - (unsigned long long)d->body.pend->jobid, + char jobidStr[64]; + + fmt_jobid(jobidStr, sizeof(jobidStr), d->body.pend->jobid); + snprintf(buf, buf_sz, "PEND jobid=%s main_reason=%d", jobidStr, (d->body.pend->reason != NULL) ? d->body.pend->reason->main_reason : -1); @@ -343,10 +357,14 @@ static void sim_verify_describe_replay_decision(const replay_decision_t* d, break; case ReplayDecision_body_preempt: - snprintf(buf, buf_sz, "PREEMPT jobid=%llu", - (d->body.preempt != NULL) - ? (unsigned long long)d->body.preempt->jobid - : 0ULL); + { + char jobidStr[64]; + + fmt_jobid(jobidStr, sizeof(jobidStr), + (d->body.preempt != NULL) ? d->body.preempt->jobid + : 0ULL); + snprintf(buf, buf_sz, "PREEMPT jobid=%s", jobidStr); + } break; default: @@ -368,28 +386,41 @@ static void sim_verify_describe_decision(const xce_decision_t* d, char* buf, return; } + /* bug-1591: see sim_verify_describe_replay_decision() above's identical + * fmt_jobid() comment. */ switch (d->op) { case XCE_DECISION_DISPATCH: - snprintf(buf, buf_sz, "DISPATCH jobid=%llu nhosts=%d", - (d->u.alloc != NULL) ? (unsigned long long)d->u.alloc->jobid - : 0ULL, - (d->u.alloc != NULL) ? d->u.alloc->nhosts : 0); + { + char jobidStr[64]; + + fmt_jobid(jobidStr, sizeof(jobidStr), + (d->u.alloc != NULL) ? d->u.alloc->jobid : 0ULL); + snprintf(buf, buf_sz, "DISPATCH jobid=%s nhosts=%d", jobidStr, + (d->u.alloc != NULL) ? d->u.alloc->nhosts : 0); + } break; case XCE_DECISION_PEND: - snprintf(buf, buf_sz, "PEND jobid=%llu main_reason=%d", - (d->u.reason != NULL) ? (unsigned long long)d->u.reason->jobid - : 0ULL, - ((d->u.reason != NULL) && (d->u.reason->reasons != NULL)) - ? d->u.reason->reasons->main_reason - : -1); + { + char jobidStr[64]; + + fmt_jobid(jobidStr, sizeof(jobidStr), + (d->u.reason != NULL) ? d->u.reason->jobid : 0ULL); + snprintf(buf, buf_sz, "PEND jobid=%s main_reason=%d", jobidStr, + ((d->u.reason != NULL) && (d->u.reason->reasons != NULL)) + ? d->u.reason->reasons->main_reason + : -1); + } break; case XCE_DECISION_PREEMPT: - snprintf(buf, buf_sz, "PREEMPT jobid=%llu", - (d->u.preempt != NULL) - ? (unsigned long long)d->u.preempt->jobid - : 0ULL); + { + char jobidStr[64]; + + fmt_jobid(jobidStr, sizeof(jobidStr), + (d->u.preempt != NULL) ? d->u.preempt->jobid : 0ULL); + snprintf(buf, buf_sz, "PREEMPT jobid=%s", jobidStr); + } break; default: diff --git a/src/scheduler/testsuite/meson.build b/src/scheduler/testsuite/meson.build index e5b6fa01..b7852c25 100644 --- a/src/scheduler/testsuite/meson.build +++ b/src/scheduler/testsuite/meson.build @@ -325,6 +325,21 @@ sched_driver_translate_test = executable( dependencies: [scheduler_test_deps], install: false) +# Regression test for bug-1585's true root cause: a live heap-use-after-free +# in the RESVALS_FULL/RESVALS_DELTA event-application path (core_events.c), +# triggered by every second periodic resval report for the same +# worker+resource on ANY real deployment. Same white-box #include set as +# sched_driver_translate_test (its sibling), but with a REAL (non-stubbed) +# kill_sched_resval -- see test_resval_ownership.c's own header comment for +# why that distinction is exactly what makes this test able to catch the bug +# sched_driver_translate_test's own no-op stub cannot. +sched_resval_ownership_test = executable( + 'sched_resval_ownership_test', + ['test_resval_ownership.c'], + include_directories:[incdir], + dependencies: [scheduler_test_deps], + install: false) + # Contract test for the normal-mode driver's decision-publish callback # (scheduler-core spec, task 5.2, Req 5 AC 6 / AC 9's publishing half): # xce_sched_driver_on_decision() forwards DISPATCH/PEND decisions to @@ -944,6 +959,9 @@ test('scheduler core full cycle test', test('scheduler driver translate test', sched_driver_translate_test, suite: 'scheduler', env: ['ASAN_OPTIONS=detect_leaks=0']) +test('scheduler resval ownership test', + sched_resval_ownership_test, suite: 'scheduler', + env: ['ASAN_OPTIONS=detect_leaks=0']) test('scheduler driver decision cb test', sched_driver_decision_cb_test, suite: 'scheduler', env: ['ASAN_OPTIONS=detect_leaks=0']) diff --git a/src/scheduler/testsuite/test_clock.c b/src/scheduler/testsuite/test_clock.c index 4db982c4..583b95c2 100644 --- a/src/scheduler/testsuite/test_clock.c +++ b/src/scheduler/testsuite/test_clock.c @@ -246,6 +246,53 @@ static void test_xce_sched_core_init_seeds_normal_mode_defaults(void) 1, atomic_load_explicit(&(core.speed_den), memory_order_acquire)); } +/* ---------------------------------------------------------------------- */ +/* xce_core_set_wallclock_anchor() -- normal-mode epoch seeding (bug-1584) */ +/* ---------------------------------------------------------------------- */ + +static uint64_t test_epoch_ns(void) +{ + struct timespec ts; + + clock_gettime(CLOCK_REALTIME, &ts); + return ((uint64_t)ts.tv_sec * TEST_NSEC_PER_SEC) + (uint64_t)ts.tv_nsec; +} + +static void test_xce_core_set_wallclock_anchor_seeds_real_epoch_time(void) +{ + /* bug-1584: xce_sched_core_init() alone leaves xce_core_now() returning + * raw CLOCK_MONOTONIC-since-boot nanoseconds (see + * test_xce_sched_core_init_seeds_normal_mode_defaults above) -- NOT + * epoch time, which every real consumer of a normal-mode timestamp + * needs (recording.c's record_start_ts, SchedAdminReply.startedAt, + * documented in controller.capnp as "nanoseconds since epoch"). + * xce_core_set_wallclock_anchor() is the fix: bracket its effect on + * xce_core_now() between two back-to-back CLOCK_REALTIME reads taken + * immediately before/after the call -- deterministic (real time only + * moves forward) and immune to sleep-based flakiness. Before this fix, + * `result` would be a small monotonic-since-boot value, far below + * `before` (a real epoch value ~1.7e18 ns as of 2026) -- this bracket + * fails loudly on that regression rather than passing by coincidence. */ + xce_sched_core_t core; + uint64_t before, result, after; + + fixture_init(&core); + + before = test_epoch_ns(); + xce_core_set_wallclock_anchor(&core); + result = xce_core_now(&core); + after = test_epoch_ns(); + + TEST_ASSERT_TRUE(result >= before); + TEST_ASSERT_TRUE(result <= after); +} + +static void test_xce_core_set_wallclock_anchor_null_core_is_a_safe_noop(void) +{ + /* Mirrors xce_core_now(NULL)'s defensive contract -- no crash. */ + xce_core_set_wallclock_anchor(NULL); +} + /* ---------------------------------------------------------------------- */ /* tlog format integration (Req 5 AC 3) */ /* ---------------------------------------------------------------------- */ @@ -451,6 +498,8 @@ int main(void) RUN_TEST(test_xce_core_now_fast_simulation_scales_up_by_speed); RUN_TEST(test_xce_core_now_slow_simulation_scales_down_by_speed); RUN_TEST(test_xce_sched_core_init_seeds_normal_mode_defaults); + RUN_TEST(test_xce_core_set_wallclock_anchor_seeds_real_epoch_time); + RUN_TEST(test_xce_core_set_wallclock_anchor_null_core_is_a_safe_noop); RUN_TEST(test_tlog_format_shows_frozen_virtual_time_not_infotime); RUN_TEST(test_tlog_format_frozen_time_is_stable_across_calls); RUN_TEST(test_tlog_format_falls_back_to_infotime_when_no_active_core); diff --git a/src/scheduler/testsuite/test_core_events.c b/src/scheduler/testsuite/test_core_events.c index 33ba3b0f..34bee2d4 100644 --- a/src/scheduler/testsuite/test_core_events.c +++ b/src/scheduler/testsuite/test_core_events.c @@ -67,6 +67,39 @@ void kill_sched_worker(void* arg) { (void)arg; } void kill_sched_user(void* arg) { (void)arg; } void kill_sched_project(void* arg) { (void)arg; } void kill_sched_resval(xce_val_t* val) { (void)val; } + +/* dup_val - real reimplementation of snapshot_wire.c's function of the same + * name (verbatim logic), needed because core_events.c's RESVALS_FULL/ + * RESVALS_DELTA handlers now call it (bug-1585 fix) and this file's + * white-box #include of ../src/core_events.c does not pull in + * snapshot_wire.c (same "reimplement the one small function instead of the + * whole file" rationale this file's own sched_resolve_job_picker copy + * already documents). This file's own kill_sched_resval is a no-op stub, so + * the deep copy's contents are never inspected here -- only link-time + * presence matters for this file's own tests. */ +xce_val_t* dup_val(const xce_val_t* src) +{ + xce_val_t* v; + + if (src == NULL) { + return NULL; + } + + v = (xce_val_t*)calloc(1, sizeof(*v)); + if (v == NULL) { + return NULL; + } + + v->which = src->which; + if (src->which == XCE_VAL_STRING) { + v->u.svalue + = (src->u.svalue != NULL) ? makeString(src->u.svalue) : NULL; + } else { + v->u = src->u; + } + + return v; +} void plugin_unregister(void* arg) { (void)arg; } /* Real reimplementation of schedobj.c's sched_resolve_job_picker -- see diff --git a/src/scheduler/testsuite/test_core_full_cycle.c b/src/scheduler/testsuite/test_core_full_cycle.c index b89da616..353f83a6 100644 --- a/src/scheduler/testsuite/test_core_full_cycle.c +++ b/src/scheduler/testsuite/test_core_full_cycle.c @@ -134,6 +134,39 @@ void kill_sched_worker(void* arg) { (void)arg; } void kill_sched_user(void* arg) { (void)arg; } void kill_sched_project(void* arg) { (void)arg; } void kill_sched_resval(xce_val_t* val) { (void)val; } + +/* dup_val - real reimplementation of snapshot_wire.c's function of the same + * name (verbatim logic), needed because core_events.c's RESVALS_FULL/ + * RESVALS_DELTA handlers now call it (bug-1585 fix) and this file's + * white-box #include of ../src/core_events.c does not pull in + * snapshot_wire.c (same "reimplement the one small function instead of the + * whole file" rationale this file's own sched_resolve_job_picker copy + * already documents). This file's own kill_sched_resval is a no-op stub, so + * the deep copy's contents are never inspected here -- only link-time + * presence matters for this file's own tests. */ +xce_val_t* dup_val(const xce_val_t* src) +{ + xce_val_t* v; + + if (src == NULL) { + return NULL; + } + + v = (xce_val_t*)calloc(1, sizeof(*v)); + if (v == NULL) { + return NULL; + } + + v->which = src->which; + if (src->which == XCE_VAL_STRING) { + v->u.svalue + = (src->u.svalue != NULL) ? makeString(src->u.svalue) : NULL; + } else { + v->u = src->u; + } + + return v; +} void plugin_unregister(void* arg) { (void)arg; } /* core_reset_workers()'s (core_session.c) linker dependency -- see file diff --git a/src/scheduler/testsuite/test_driver_decision_cb.c b/src/scheduler/testsuite/test_driver_decision_cb.c index 741711b6..93a918e0 100644 --- a/src/scheduler/testsuite/test_driver_decision_cb.c +++ b/src/scheduler/testsuite/test_driver_decision_cb.c @@ -67,6 +67,39 @@ void kill_sched_worker(void* arg) { (void)arg; } void kill_sched_user(void* arg) { (void)arg; } void kill_sched_project(void* arg) { (void)arg; } void kill_sched_resval(xce_val_t* val) { (void)val; } + +/* dup_val - real reimplementation of snapshot_wire.c's function of the same + * name (verbatim logic), needed because core_events.c's RESVALS_FULL/ + * RESVALS_DELTA handlers now call it (bug-1585 fix) and this file's + * white-box #include of ../src/core_events.c does not pull in + * snapshot_wire.c (same "reimplement the one small function instead of the + * whole file" rationale this file's own sched_resolve_job_picker copy + * already documents). This file's own kill_sched_resval is a no-op stub, so + * the deep copy's contents are never inspected here -- only link-time + * presence matters for this file's own tests. */ +xce_val_t* dup_val(const xce_val_t* src) +{ + xce_val_t* v; + + if (src == NULL) { + return NULL; + } + + v = (xce_val_t*)calloc(1, sizeof(*v)); + if (v == NULL) { + return NULL; + } + + v->which = src->which; + if (src->which == XCE_VAL_STRING) { + v->u.svalue + = (src->u.svalue != NULL) ? makeString(src->u.svalue) : NULL; + } else { + v->u = src->u; + } + + return v; +} void plugin_unregister(void* arg) { (void)arg; } /* Link-satisfying stubs: as of scheduler-core spec task 5.2, diff --git a/src/scheduler/testsuite/test_driver_session_ordering.c b/src/scheduler/testsuite/test_driver_session_ordering.c index d4dc169c..c36c0d77 100644 --- a/src/scheduler/testsuite/test_driver_session_ordering.c +++ b/src/scheduler/testsuite/test_driver_session_ordering.c @@ -165,6 +165,39 @@ void kill_sched_user(void* arg) { (void)arg; } void kill_sched_project(void* arg) { (void)arg; } void kill_sched_resval(xce_val_t* val) { (void)val; } +/* dup_val - real reimplementation of snapshot_wire.c's function of the same + * name (verbatim logic), needed because core_events.c's RESVALS_FULL/ + * RESVALS_DELTA handlers now call it (bug-1585 fix) and this file's + * white-box #include of ../src/core_events.c does not pull in + * snapshot_wire.c (same "reimplement the one small function instead of the + * whole file" rationale this file's own sched_resolve_job_picker copy + * already documents). This file's own kill_sched_resval is a no-op stub, so + * the deep copy's contents are never inspected here -- only link-time + * presence matters for this file's own tests. */ +xce_val_t* dup_val(const xce_val_t* src) +{ + xce_val_t* v; + + if (src == NULL) { + return NULL; + } + + v = (xce_val_t*)calloc(1, sizeof(*v)); + if (v == NULL) { + return NULL; + } + + v->which = src->which; + if (src->which == XCE_VAL_STRING) { + v->u.svalue + = (src->u.svalue != NULL) ? makeString(src->u.svalue) : NULL; + } else { + v->u = src->u; + } + + return v; +} + /* core_reset_workers()'s (core_session.c) linker dependency -- see file * header comment; never invoked (no worker is ever added here). */ void sched_worker_release_host_ctx(xce_sched_worker_t* w) { (void)w; } diff --git a/src/scheduler/testsuite/test_driver_translate.c b/src/scheduler/testsuite/test_driver_translate.c index cad98570..6ccd7004 100644 --- a/src/scheduler/testsuite/test_driver_translate.c +++ b/src/scheduler/testsuite/test_driver_translate.c @@ -65,6 +65,39 @@ void kill_sched_worker(void* arg) { (void)arg; } void kill_sched_user(void* arg) { (void)arg; } void kill_sched_project(void* arg) { (void)arg; } void kill_sched_resval(xce_val_t* val) { (void)val; } + +/* dup_val - real reimplementation of snapshot_wire.c's function of the same + * name (verbatim logic), needed because core_events.c's RESVALS_FULL/ + * RESVALS_DELTA handlers now call it (bug-1585 fix) and this file's + * white-box #include of ../src/core_events.c does not pull in + * snapshot_wire.c (same "reimplement the one small function instead of the + * whole file" rationale this file's own sched_resolve_job_picker copy + * already documents). This file's own kill_sched_resval is a no-op stub, so + * the deep copy's contents are never inspected here -- only link-time + * presence matters for this file's own tests. */ +xce_val_t* dup_val(const xce_val_t* src) +{ + xce_val_t* v; + + if (src == NULL) { + return NULL; + } + + v = (xce_val_t*)calloc(1, sizeof(*v)); + if (v == NULL) { + return NULL; + } + + v->which = src->which; + if (src->which == XCE_VAL_STRING) { + v->u.svalue + = (src->u.svalue != NULL) ? makeString(src->u.svalue) : NULL; + } else { + v->u = src->u; + } + + return v; +} void plugin_unregister(void* arg) { (void)arg; } /* Link-satisfying stubs for scheduler.c's sched_publish_dispatch()/ @@ -673,19 +706,29 @@ static void test_drain_translates_and_pushes_all_ten_op_types(void) r3->u.resourceupdate.resources = resources; enqueue(&core, r3); - /* 4. UPDATE_RESVALS_FULL -> RESVALS_FULL. NOTE: core_events.c's - * RESVALS_FULL case does `zarray_put(&core->sharedInst, res->idx, - * rv->val)` -- a SHALLOW store of the SAME `val` pointer (verified by - * reading zarray_put_ex()'s source, src/lib/zarray.c) -- and this - * function's cleanup then frees `full_resval` (and, via - * free_ResourceVal->free_Value_ptr, `full_val` too), exactly mirroring - * scheduler.c's existing sched_pend_update_killer()'s identical - * RESVALS_FULL cleanup. This is a PRE-EXISTING production ownership - * characteristic (the same hazard already exists in sched_update()'s - * own live path today, untouched by this task) -- so after the drain - * below, core->sharedInst's stored pointer for "cpu" is only safe to - * check for non-NULL (the slot was populated), never to dereference. */ - full_val = (xce_val_t*)calloc(1, sizeof(*full_val)); + /* 4. UPDATE_RESVALS_FULL -> RESVALS_FULL. NOTE (bug-1585, FIXED): this + * comment used to document a PRE-EXISTING production ownership hazard -- + * core_events.c's RESVALS_FULL case used to do `zarray_put(&core-> + * sharedInst, res->idx, rv->val)`, a SHALLOW store of the SAME `val` + * pointer this function's cleanup then frees via + * free_ResourceVal->free_Value_ptr, exactly mirroring scheduler.c's + * sched_pend_update_killer()'s identical (also now-fixed) RESVALS_FULL + * cleanup -- meaning core->sharedInst's stored pointer for "cpu" used to + * be a dangling pointer the moment this drain returned, unsafe to + * dereference (only a non-NULL check was safe). core_events.c's + * RESVALS_FULL/RESVALS_DELTA handlers now `dup_val()` before + * `zarray_put()`, so the core owns an independent copy and this + * function's unconditional free of `full_val`/`delta_val` below is + * exactly what should happen -- core->sharedInst's stored value is now + * genuinely safe to read back and assert on (see + * test_resval_ownership.c for the dedicated regression test proving + * this end-to-end under a REAL, non-stubbed kill_sched_resval). `which` + * must be set explicitly here (XCE_VAL_NUMERIC) -- dup_val() now + * actually inspects it, and a bare calloc() leaves it 0 (== + * XCE_VAL_STRING), which would make dup_val() misread `u.ivalue32`'s + * bit pattern as a `char*` and crash in makeString(). */ + full_val = (xce_val_t*)calloc(1, sizeof(*full_val)); + full_val->which = XCE_VAL_NUMERIC; full_val->u.ivalue32 = 16; full_resval = (xce_resval_t*)calloc(1, sizeof(*full_resval)); full_resval->name = strdup("cpu"); @@ -698,9 +741,10 @@ static void test_drain_translates_and_pushes_all_ten_op_types(void) enqueue(&core, r4); /* 5. UPDATE_RESVALS -> RESVALS_DELTA: attributes to worker "hostA". - * Same shallow-store-then-free hazard as 4 above (per-worker resvals - * zarray instead of core->sharedInst) -- same "non-NULL only" caveat. */ - delta_val = (xce_val_t*)calloc(1, sizeof(*delta_val)); + * Same fix as 4 above (per-worker resvals zarray instead of + * core->sharedInst) -- `which` set explicitly for the same reason. */ + delta_val = (xce_val_t*)calloc(1, sizeof(*delta_val)); + delta_val->which = XCE_VAL_NUMERIC; delta_val->u.ivalue32 = 4; delta_resval = (xce_resval_t*)calloc(1, sizeof(*delta_resval)); delta_resval->name = strdup("cpu"); diff --git a/src/scheduler/testsuite/test_repl_breakpoints_inspection.c b/src/scheduler/testsuite/test_repl_breakpoints_inspection.c index cdc7a9d8..51a51529 100644 --- a/src/scheduler/testsuite/test_repl_breakpoints_inspection.c +++ b/src/scheduler/testsuite/test_repl_breakpoints_inspection.c @@ -52,6 +52,7 @@ */ #include #include +#include #include #include #include @@ -638,6 +639,8 @@ static void test_info_job_unknown_reports_not_found(void) static void test_info_queue_reports_status_and_queued_jobs(void) { xce_sched_sim_driver_t sim; + char jobid_marker[80]; + char jobid_buf[64]; char* captured; memset(&sim, 0, sizeof(sim)); @@ -645,10 +648,16 @@ static void test_info_queue_reports_status_and_queued_jobs(void) captured = capture_dispatch(&sim, "info queue normal"); + /* bug-1591: repl_info_queue_one() (repl_info.c) now renders jobids + * via fmt_jobid(), not raw %llu -- this marker must match that same + * human-readable form. */ + fmt_jobid(jobid_buf, sizeof(jobid_buf), 4242ULL); + snprintf(jobid_marker, sizeof(jobid_marker), "jobid=%s", jobid_buf); + TEST_ASSERT_NOT_NULL(strstr(captured, "queue normal")); TEST_ASSERT_NOT_NULL(strstr(captured, "priority=5")); TEST_ASSERT_NOT_NULL(strstr(captured, "status=open")); - TEST_ASSERT_NOT_NULL_MESSAGE(strstr(captured, "jobid=4242"), + TEST_ASSERT_NOT_NULL_MESSAGE(strstr(captured, jobid_marker), "the fixture's own PEND job must appear " "in the queue's queued-job listing"); @@ -736,6 +745,316 @@ static void test_info_worker_reports_resource_values(void) printf("test_info_worker_reports_resource_values: PASS\n"); } +/* + * bug-1590: repl_info_worker_one() forced EVERY resource value through the + * numeric "total=%.2f available=%.2f" format, silently coercing any + * string- or boolean-valued resource (e.g. `type`/`model`, or any + * XCE_VAL_BOOLEAN resource) to 0.00 -- indistinguishable from a genuinely + * zero numeric value. Regression: a string and a boolean resource must + * each render their real value via a "value=..." line, never a fabricated + * total/available pair. + */ +static void test_info_worker_reports_string_and_boolean_resource_values(void) +{ + xce_sched_sim_driver_t sim; + xce_core_event_t events[2]; + xce_resource_t wire_resource_str; + xce_resource_t wire_resource_bool; + xce_worker_t wire_worker; + xce_worker_t* worker_ptrs[1]; + xce_val_t val_str; + xce_val_t val_bool; + xce_name_val_t nv_str; + xce_name_val_t nv_bool; + xce_name_val_t* nv_ptrs[2]; + char* captured; + int rc; + + memset(&sim, 0, sizeof(sim)); + rc = xce_sched_core_init(&(sim.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + memset(&wire_resource_str, 0, sizeof(wire_resource_str)); + wire_resource_str.name = "type"; + wire_resource_str.valtype = XCE_VAL_STRING; + TEST_ASSERT_NOT_NULL(makeResource(sim.core.resmgr, &wire_resource_str)); + + memset(&wire_resource_bool, 0, sizeof(wire_resource_bool)); + wire_resource_bool.name = "dockerized"; + wire_resource_bool.valtype = XCE_VAL_BOOLEAN; + TEST_ASSERT_NOT_NULL(makeResource(sim.core.resmgr, &wire_resource_bool)); + + memset(&val_str, 0, sizeof(val_str)); + val_str.which = Value_u_svalue; + val_str.u.svalue = "linux86"; + memset(&nv_str, 0, sizeof(nv_str)); + nv_str.name = "type"; + nv_str.val = &val_str; + + memset(&val_bool, 0, sizeof(val_bool)); + val_bool.which = Value_u_bvalue; + val_bool.u.bvalue = 1; + memset(&nv_bool, 0, sizeof(nv_bool)); + nv_bool.name = "dockerized"; + nv_bool.val = &val_bool; + + nv_ptrs[0] = &nv_str; + nv_ptrs[1] = &nv_bool; + + memset(&wire_worker, 0, sizeof(wire_worker)); + wire_worker.name = "hostA"; + wire_worker.status = 0; + wire_worker.nresvals_to_sched = 2; + wire_worker.resvals_to_sched = nv_ptrs; + worker_ptrs[0] = &wire_worker; + + memset(events, 0, sizeof(events)); + events[0].type = XCE_CORE_EVENT_WORKERS_PUSH; + events[0].u.workers_push.nworkers = 1; + events[0].u.workers_push.workers = worker_ptrs; + + rc = xce_core_push_events(&(sim.core), events, 1); + TEST_ASSERT_EQUAL_INT(0, rc); + + captured = capture_dispatch(&sim, "info worker hostA"); + + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "resource type: value=linux86"), + "a string-valued resource must display its real string value, " + "not a fabricated total/available pair (bug-1590)"); + TEST_ASSERT_NULL_MESSAGE(strstr(captured, "resource type: total="), + "a string-valued resource must never be " + "rendered with a total=/available= pair"); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "resource dockerized: value=true"), + "a boolean-valued resource must display true/false, not a " + "fabricated total/available pair (bug-1590)"); + TEST_ASSERT_NULL_MESSAGE(strstr(captured, "resource dockerized: total="), + "a boolean-valued resource must never be " + "rendered with a total=/available= pair"); + + free(captured); + + printf("test_info_worker_reports_string_and_boolean_resource_values: " + "PASS\n"); +} + +/* + * bug-1590: repl_info_resource_one() (the "info resource " + * aggregate-across-workers command) had the same defect one layer up -- + * total/available aggregation silently excluded any non-numeric resource, + * while `valtype=string`/`valtype=boolean` was already labeled correctly + * two fields over. Regression: the resource's actual value must be + * reported, not silently dropped from the summary. + */ +static void test_info_resource_reports_string_value(void) +{ + xce_sched_sim_driver_t sim; + xce_core_event_t events[1]; + xce_resource_t wire_resource_str; + xce_val_t val_str; + xce_resval_t rv_str; + xce_resval_t* rv_ptrs[1]; + char* captured; + int rc; + + memset(&sim, 0, sizeof(sim)); + rc = xce_sched_core_init(&(sim.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + /* bug-1593 (task 12.5): 'info resource' now reads a GLOBAL/SHARED + * resource's value from core->sharedInst -- pushed here via + * RESVALS_FULL (mirrors core_events.c's own RESVALS_FULL handler), + * not via a per-worker WORKERS_PUSH resval anymore. */ + memset(&wire_resource_str, 0, sizeof(wire_resource_str)); + wire_resource_str.name = "license_pool"; + wire_resource_str.valtype = XCE_VAL_STRING; + wire_resource_str.flags = XCE_RESF_GLOBAL; + TEST_ASSERT_NOT_NULL(makeResource(sim.core.resmgr, &wire_resource_str)); + + memset(&val_str, 0, sizeof(val_str)); + val_str.which = Value_u_svalue; + val_str.u.svalue = "linux86"; + + memset(&rv_str, 0, sizeof(rv_str)); + rv_str.name = "license_pool"; + rv_str.val = &val_str; + rv_ptrs[0] = &rv_str; + + memset(events, 0, sizeof(events)); + events[0].type = XCE_CORE_EVENT_RESVALS_FULL; + events[0].u.resvals_full.n = 1; + events[0].u.resvals_full.resvals = rv_ptrs; + + rc = xce_core_push_events(&(sim.core), events, 1); + TEST_ASSERT_EQUAL_INT(0, rc); + + captured = capture_dispatch(&sim, "info resource license_pool"); + + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "resource license_pool: desc=(none) " + "valtype=string flags=GLOBAL interval=0"), + "a GLOBAL resource's definition metadata must be printed " + "(bug-1593)"); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "value=linux86"), + "a GLOBAL/SHARED string resource's real value must be reported, " + "read from core->sharedInst (bug-1590/bug-1593)"); + TEST_ASSERT_NULL_MESSAGE(strstr(captured, "total="), + "a string resource must never be rendered " + "with a total=/available= pair"); + TEST_ASSERT_NULL_MESSAGE(strstr(captured, "host-based resource"), + "a GLOBAL resource must never be reported as " + "host-based"); + + free(captured); + + printf("test_info_resource_reports_string_value: PASS\n"); +} + +/* + * bug-1593 (task 12.5): `info resource all` must list only resources + * flagged XCE_RESF_GLOBAL/XCE_RESF_SHARED -- a host-based resource (no + * such flag, e.g. cpus/mem) is `info worker`'s job, never this command's. + */ +static void test_info_resource_all_lists_only_global_shared_resources(void) +{ + xce_sched_sim_driver_t sim; + xce_core_event_t events[1]; + xce_resource_t wire_resource_shared; + xce_resource_t wire_resource_hostbased; + xce_val_t val; + xce_resval_t rv; + xce_resval_t* rv_ptrs[1]; + char* captured; + int rc; + + memset(&sim, 0, sizeof(sim)); + rc = xce_sched_core_init(&(sim.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + memset(&wire_resource_shared, 0, sizeof(wire_resource_shared)); + wire_resource_shared.name = "licenses"; + wire_resource_shared.valtype = XCE_VAL_NUMERIC; + wire_resource_shared.flags = XCE_RESF_SHARED; + TEST_ASSERT_NOT_NULL( + makeResource(sim.core.resmgr, &wire_resource_shared)); + + memset(&wire_resource_hostbased, 0, sizeof(wire_resource_hostbased)); + wire_resource_hostbased.name = "mem"; + wire_resource_hostbased.valtype = XCE_VAL_NUMERIC; + /* flags left 0 -- host-based, no GLOBAL/SHARED bit. */ + TEST_ASSERT_NOT_NULL( + makeResource(sim.core.resmgr, &wire_resource_hostbased)); + + memset(&val, 0, sizeof(val)); + val.which = Value_u_fvalue; + val.u.fvalue = 8.0; + memset(&rv, 0, sizeof(rv)); + rv.name = "licenses"; + rv.val = &val; + rv_ptrs[0] = &rv; + + memset(events, 0, sizeof(events)); + events[0].type = XCE_CORE_EVENT_RESVALS_FULL; + events[0].u.resvals_full.n = 1; + events[0].u.resvals_full.resvals = rv_ptrs; + + rc = xce_core_push_events(&(sim.core), events, 1); + TEST_ASSERT_EQUAL_INT(0, rc); + + captured = capture_dispatch(&sim, "info resource all"); + + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "resource licenses:"), + "a SHARED resource must appear in 'info resource all' (bug-1593)"); + TEST_ASSERT_NULL_MESSAGE( + strstr(captured, "resource mem:"), + "a host-based resource must NOT appear in 'info resource all' -- " + "that is info worker's job, not info resource's (bug-1593)"); + + free(captured); + + printf("test_info_resource_all_lists_only_global_shared_resources: " + "PASS\n"); +} + +/* + * bug-1593 (task 12.5): a host-based resource's own NAMED lookup + * (`info resource `, as opposed to `all`) still resolves it, so an + * operator can check its metadata/flags -- but reports no fabricated + * cluster-wide value, directing to `info worker` instead. + */ +static void test_info_resource_named_lookup_reports_host_based_note(void) +{ + xce_sched_sim_driver_t sim; + xce_resource_t wire_resource; + char* captured; + int rc; + + memset(&sim, 0, sizeof(sim)); + rc = xce_sched_core_init(&(sim.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + memset(&wire_resource, 0, sizeof(wire_resource)); + wire_resource.name = "cpus"; + wire_resource.valtype = XCE_VAL_NUMERIC; + TEST_ASSERT_NOT_NULL(makeResource(sim.core.resmgr, &wire_resource)); + + captured = capture_dispatch(&sim, "info resource cpus"); + + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "resource cpus: desc=(none) valtype=numeric " + "flags=(none) interval=0"), + "a host-based resource's metadata must still be reported by a " + "named lookup (bug-1593)"); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "host-based resource -- see 'info worker ' " + "for per-host values"), + "a host-based resource must direct the operator to 'info worker', " + "never a fabricated cluster-wide value (bug-1593)"); + + free(captured); + + printf("test_info_resource_named_lookup_reports_host_based_note: " + "PASS\n"); +} + +/* + * bug-1596 (task 12.5): repl_split_tokens() must strip one matching pair + * of surrounding quote characters -- an operator quoting an argument out + * of habit (e.g. pasted straight from another command's own output) must + * not get a silent wrong-answer failure. + */ +static void test_info_job_accepts_quoted_jobid_argument(void) +{ + xce_sched_sim_driver_t sim; + char* captured_dquoted; + char* captured_squoted; + + memset(&sim, 0, sizeof(sim)); + build_fixture_core(&(sim.core), 777); + + captured_dquoted = capture_dispatch(&sim, "info job \"777\""); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured_dquoted, "job 777"), + "a double-quoted jobid argument must resolve the same job as the " + "unquoted form (bug-1596)"); + TEST_ASSERT_NULL_MESSAGE( + strstr(captured_dquoted, "no such job"), + "must not silently mis-parse the quote characters into jobid 0"); + free(captured_dquoted); + + captured_squoted = capture_dispatch(&sim, "info job '777'"); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured_squoted, "job 777"), + "a single-quoted jobid argument must resolve the same job too"); + TEST_ASSERT_NULL_MESSAGE(strstr(captured_squoted, "no such job"), ""); + free(captured_squoted); + + printf("test_info_job_accepts_quoted_jobid_argument: PASS\n"); +} + static void test_info_summary_reports_counts(void) { xce_sched_sim_driver_t sim; @@ -989,20 +1308,102 @@ static void write_text_file(const char* path, const char* content) fclose(f); } -static xce_sched_sim_driver_t g_d_sim; -static sched_sim_args_t g_d_args; -static int g_d_rc; - -static void run_driver_for_group_d_capture(void) +static char* read_whole_file(const char* path) { - g_d_rc = sched_sim_driver_run(&g_d_sim, &g_d_args); + FILE* f = fopen(path, "rb"); + long len; + char* buf; + + TEST_ASSERT_NOT_NULL(f); + TEST_ASSERT_EQUAL_INT(0, fseek(f, 0, SEEK_END)); + len = ftell(f); + TEST_ASSERT_TRUE(len >= 0); + rewind(f); + + buf = (char*)malloc((size_t)len + 1); + TEST_ASSERT_NOT_NULL(buf); + if (len > 0) { + TEST_ASSERT_EQUAL_INT((int)len, (int)fread(buf, 1, (size_t)len, f)); + } + buf[len] = '\0'; + fclose(f); + + return buf; +} + +/* + * task 12.5 (bug-1594): a completed --interactive run now re-enters the + * REPL at replay completion instead of returning, so any Group D script + * that runs to the fixture's true EOF needs an explicit trailing `quit` + * and must run in a FORKED child -- otherwise the implicit-EOF-quit (or + * the scripted `quit` itself) calls exit(0) inside this shared unity test + * binary. Same fork+capture-file pattern test_repl_execution_control.c's + * own Test 3 and test_repl_snapshot_params.c already establish. + */ +static void run_group_d_child(const char* replay_path, + const char* script_path, + const char* capture_path) +{ + xce_sched_sim_driver_t sim; + sched_sim_args_t args; + + if (freopen(script_path, "r", stdin) == NULL) { + _exit(120); + } + if (freopen(capture_path, "w", stdout) == NULL) { + _exit(121); + } + + memset(&sim, 0, sizeof(sim)); + memset(&args, 0, sizeof(args)); + args.replay_path = replay_path; + args.sim_start_ts = SCHED_SIM_START_TS_LATEST; + args.sim_speed = 0; + args.interactive = true; + + (void)sched_sim_driver_run(&sim, &args); + + fflush(stdout); + _exit(111); /* unreachable -- the script's trailing `quit` must + * already have exited */ +} + +/* Forks run_group_d_child(), waits for it, and returns the captured + * stdout as a malloc'd buffer (caller frees). Asserts a clean exit(0). */ +static char* run_group_d_and_capture(const char* replay_path, + const char* script_path, + const char* capture_path) +{ + pid_t pid = fork(); + int status; + + TEST_ASSERT_TRUE_MESSAGE(pid >= 0, "fork() must succeed"); + + if (pid == 0) { + run_group_d_child(replay_path, script_path, capture_path); + _exit(112); /* unreachable */ + } + + TEST_ASSERT_EQUAL_INT_MESSAGE(pid, waitpid(pid, &status, 0), + "waitpid must reap the REPL child"); + TEST_ASSERT_TRUE_MESSAGE(WIFEXITED(status), + "the REPL child must exit normally via " + "quit's exit(0), not crash/signal"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, WEXITSTATUS(status), + "quit's exit(0) is the only correct " + "exit path"); + + return read_whole_file(capture_path); } static void test_event_breakpoint_pauses_repl_at_matching_event(void) { char replay_path[PATH_MAX]; - char script_dir[PATH_MAX]; + char aux_dir[PATH_MAX]; char script_path[PATH_MAX]; + char capture_path[PATH_MAX]; + char pend_marker[80]; + char jobid_buf[64]; FILE* f; char* captured; char* bp_pos; @@ -1020,26 +1421,25 @@ static void test_event_breakpoint_pauses_repl_at_matching_event(void) append_end_of_stream_marker(f); fclose(f); - make_tmp_dir(script_dir, sizeof(script_dir), "_evbpscript"); - snprintf(script_path, sizeof(script_path), "%s/script.txt", script_dir); + make_tmp_dir(aux_dir, sizeof(aux_dir), "_evbpaux"); + snprintf(script_path, sizeof(script_path), "%s/script.txt", aux_dir); + snprintf(capture_path, sizeof(capture_path), "%s/capture.out", aux_dir); + /* task 12.5 (bug-1594): trailing `quit` -- the second `continue` now + * runs this one-session fixture all the way to replay completion, + * which re-enters the REPL instead of returning. */ write_text_file(script_path, - "break JOB_STATE_CHANGE jobid=222\ncontinue\ncontinue\n"); + "break JOB_STATE_CHANGE jobid=222\ncontinue\ncontinue\nquit\n"); - TEST_ASSERT_NOT_NULL(freopen(script_path, "r", stdin)); + captured = run_group_d_and_capture(replay_path, script_path, capture_path); - memset(&g_d_sim, 0, sizeof(g_d_sim)); - memset(&g_d_args, 0, sizeof(g_d_args)); - g_d_args.replay_path = replay_path; - g_d_args.sim_start_ts = SCHED_SIM_START_TS_LATEST; - g_d_args.sim_speed = 0; - g_d_args.interactive = true; - - captured = capture_stdout_around(run_driver_for_group_d_capture); - - TEST_ASSERT_EQUAL_INT(0, g_d_rc); + /* bug-1591: sim_print_decision() (sim_loop.c) now renders jobids via + * fmt_jobid(), not raw %llu -- this marker must match that same + * human-readable form. */ + fmt_jobid(jobid_buf, sizeof(jobid_buf), 4242ULL); + snprintf(pend_marker, sizeof(pend_marker), "PEND jobid=%s", jobid_buf); bp_pos = strstr(captured, "hit breakpoint 1 @ event #4: JOB_STATE_CHANGE"); - pend_pos = strstr(captured, "PEND jobid=4242"); + pend_pos = strstr(captured, pend_marker); TEST_ASSERT_NOT_NULL_MESSAGE( bp_pos, "the breakpoint must fire exactly at event #4 (jobid=222), " @@ -1064,8 +1464,12 @@ static void test_event_breakpoint_pauses_repl_at_matching_event(void) static void test_decision_breakpoint_pauses_after_decision_output(void) { char replay_path[PATH_MAX]; - char script_dir[PATH_MAX]; + char aux_dir[PATH_MAX]; char script_path[PATH_MAX]; + char capture_path[PATH_MAX]; + char pend_marker[80]; + char bp_marker[112]; + char jobid_buf[64]; FILE* f; char* captured; char* pend_pos; @@ -1081,25 +1485,26 @@ static void test_decision_breakpoint_pauses_after_decision_output(void) append_end_of_stream_marker(f); fclose(f); - make_tmp_dir(script_dir, sizeof(script_dir), "_decbpscript"); - snprintf(script_path, sizeof(script_path), "%s/script.txt", script_dir); - write_text_file(script_path, "break DECISION type=PEND\ncontinue\ncontinue\n"); + make_tmp_dir(aux_dir, sizeof(aux_dir), "_decbpaux"); + snprintf(script_path, sizeof(script_path), "%s/script.txt", aux_dir); + snprintf(capture_path, sizeof(capture_path), "%s/capture.out", aux_dir); + /* task 12.5 (bug-1594): trailing `quit` -- see the event-breakpoint + * test above for why. */ + write_text_file(script_path, + "break DECISION type=PEND\ncontinue\ncontinue\nquit\n"); - TEST_ASSERT_NOT_NULL(freopen(script_path, "r", stdin)); + captured = run_group_d_and_capture(replay_path, script_path, capture_path); - memset(&g_d_sim, 0, sizeof(g_d_sim)); - memset(&g_d_args, 0, sizeof(g_d_args)); - g_d_args.replay_path = replay_path; - g_d_args.sim_start_ts = SCHED_SIM_START_TS_LATEST; - g_d_args.sim_speed = 0; - g_d_args.interactive = true; + /* bug-1591: sim_print_decision() (sim_loop.c) and repl_break.c's + * breakpoint-hit message now render jobids via fmt_jobid(), not raw + * %llu -- these markers must match that same human-readable form. */ + fmt_jobid(jobid_buf, sizeof(jobid_buf), 555ULL); + snprintf(pend_marker, sizeof(pend_marker), "PEND jobid=%s", jobid_buf); + snprintf(bp_marker, sizeof(bp_marker), + "hit breakpoint 1 @ decision: PEND jobid=%s", jobid_buf); - captured = capture_stdout_around(run_driver_for_group_d_capture); - - TEST_ASSERT_EQUAL_INT(0, g_d_rc); - - pend_pos = strstr(captured, "PEND jobid=555"); - bp_pos = strstr(captured, "hit breakpoint 1 @ decision: PEND jobid=555"); + pend_pos = strstr(captured, pend_marker); + bp_pos = strstr(captured, bp_marker); TEST_ASSERT_NOT_NULL_MESSAGE(pend_pos, "the fixture's own PEND decision " "must have run"); @@ -1115,6 +1520,84 @@ static void test_decision_breakpoint_pauses_after_decision_output(void) printf("test_decision_breakpoint_pauses_after_decision_output: PASS\n"); } +/* + * bug-1595 (task 12.5): `step`/`continue` must echo each input event, in + * the order it is decoded, BEFORE the session's own resulting decision -- + * v1 only ever printed decisions, leaving every event silent unless a + * breakpoint happened to match it. + */ +static void test_step_echoes_events_before_session_decisions(void) +{ + char replay_path[PATH_MAX]; + char aux_dir[PATH_MAX]; + char script_path[PATH_MAX]; + char capture_path[PATH_MAX]; + char pend_marker[80]; + char jobid_buf[64]; + FILE* f; + char* captured; + char* jobpush_pos; + char* statechange_pos; + char* pend_pos; + + f = write_header_and_snapshot(replay_path, sizeof(replay_path), + "_evorder"); + TEST_ASSERT_NOT_NULL(f); + + append_queue_update_event(f, 1000ULL, "normal"); + append_jobs_push_event(f, 1500ULL, 6161ULL, "normal"); + /* Unrelated jobid (7171, not 6161) -- deliberately does NOT match the + * job just pushed above. append_jobstatechange_event() is used here + * purely as a second event type to verify echo ordering against + * (see its own header comment: core_events.c WARN+skips an unknown + * jobid rather than fail). Reusing 6161 here would actually transition + * the real pushed job PEND->RUN before the session runs, so the picker + * would never offer it as a PEND candidate and the session's own + * decision this test asserts on below would never be produced. */ + append_jobstatechange_event(f, 1600ULL, 7171ULL); + append_replay_session(f, 2000ULL); + append_end_of_stream_marker(f); + fclose(f); + + make_tmp_dir(aux_dir, sizeof(aux_dir), "_evorderaux"); + snprintf(script_path, sizeof(script_path), "%s/script.txt", aux_dir); + snprintf(capture_path, sizeof(capture_path), "%s/capture.out", aux_dir); + /* "step 1" pauses right after session 1 completes -- never reaches + * true replay EOF, so a plain trailing `quit` is enough here (same + * precedent test_step_1_processes_exactly_one_session_then_quit_exits, + * test_repl_execution_control.c, already establishes). */ + write_text_file(script_path, "step 1\nquit\n"); + + captured = run_group_d_and_capture(replay_path, script_path, capture_path); + + fmt_jobid(jobid_buf, sizeof(jobid_buf), 6161ULL); + snprintf(pend_marker, sizeof(pend_marker), "PEND jobid=%s", jobid_buf); + + jobpush_pos = strstr(captured, "[sim] event: JOB_PUSH"); + statechange_pos = strstr(captured, "[sim] event: JOB_STATE_CHANGE"); + pend_pos = strstr(captured, pend_marker); + + TEST_ASSERT_NOT_NULL_MESSAGE(jobpush_pos, + "a JOBS_PUSH event must be echoed " + "(bug-1595)"); + TEST_ASSERT_NOT_NULL_MESSAGE(statechange_pos, + "a JOB_STATE_CHANGE event must be echoed " + "(bug-1595)"); + TEST_ASSERT_NOT_NULL_MESSAGE(pend_pos, + "the session's own decision must still " + "be printed"); + TEST_ASSERT_TRUE_MESSAGE(jobpush_pos < statechange_pos, + "events must be echoed in the order they are " + "decoded"); + TEST_ASSERT_TRUE_MESSAGE(statechange_pos < pend_pos, + "events must be echoed BEFORE the session's " + "resulting decision (bug-1595)"); + + free(captured); + + printf("test_step_echoes_events_before_session_decisions: PASS\n"); +} + int main(void) { UNITY_BEGIN(); @@ -1135,11 +1618,17 @@ int main(void) RUN_TEST(test_info_job_unknown_reports_not_found); RUN_TEST(test_info_queue_reports_status_and_queued_jobs); RUN_TEST(test_info_worker_reports_resource_values); + RUN_TEST(test_info_worker_reports_string_and_boolean_resource_values); + RUN_TEST(test_info_resource_reports_string_value); + RUN_TEST(test_info_resource_all_lists_only_global_shared_resources); + RUN_TEST(test_info_resource_named_lookup_reports_host_based_note); + RUN_TEST(test_info_job_accepts_quoted_jobid_argument); RUN_TEST(test_info_summary_reports_counts); RUN_TEST(test_info_session_reports_position_after_synthetic_events); RUN_TEST(test_event_breakpoint_pauses_repl_at_matching_event); RUN_TEST(test_decision_breakpoint_pauses_after_decision_output); + RUN_TEST(test_step_echoes_events_before_session_decisions); return UNITY_END(); } diff --git a/src/scheduler/testsuite/test_repl_equivalence.c b/src/scheduler/testsuite/test_repl_equivalence.c index 1abc7aa6..2765b73a 100644 --- a/src/scheduler/testsuite/test_repl_equivalence.c +++ b/src/scheduler/testsuite/test_repl_equivalence.c @@ -107,6 +107,7 @@ #include #include #include +#include #include #include @@ -571,12 +572,14 @@ test_repl_step_continue_to_completion_matches_batch_decision_sequence(void) make_tmp_dir(script_dir, sizeof(script_dir), "_script"); snprintf(script_path, sizeof(script_path), "%s/script.txt", script_dir); - write_text_file(script_path, "step 1\ncontinue\n"); + /* task 12.5 (bug-1594): trailing `quit` -- `continue` now re-enters + * the REPL at replay completion instead of returning, so this side + * must run in a FORKED child (matching test_repl_execution_control.c's + * own Test 3 pattern); otherwise quit's exit(0) would tear down this + * whole test binary before the batch-vs-REPL comparison below ever + * runs. */ + write_text_file(script_path, "step 1\ncontinue\nquit\n"); - TEST_ASSERT_NOT_NULL_MESSAGE(freopen(script_path, "r", stdin), - "freopen of scripted stdin must succeed"); - - memset(&sim_repl, 0, sizeof(sim_repl)); memset(&args_repl, 0, sizeof(args_repl)); args_repl.replay_path = fixture_path; args_repl.sim_start_ts = SCHED_SIM_START_TS_LATEST; @@ -584,8 +587,30 @@ test_repl_step_continue_to_completion_matches_batch_decision_sequence(void) args_repl.output_path = repl_out_path; args_repl.interactive = true; - rc = sched_sim_driver_run(&sim_repl, &args_repl); - TEST_ASSERT_EQUAL_INT(0, rc); + { + pid_t pid = fork(); + int status; + + TEST_ASSERT_TRUE_MESSAGE(pid >= 0, "fork() must succeed"); + + if (pid == 0) { + if (freopen(script_path, "r", stdin) == NULL) { + _exit(120); + } + memset(&sim_repl, 0, sizeof(sim_repl)); + (void)sched_sim_driver_run(&sim_repl, &args_repl); + _exit(111); /* unreachable -- quit must already have exited */ + } + + TEST_ASSERT_EQUAL_INT_MESSAGE(pid, waitpid(pid, &status, 0), + "waitpid must reap the REPL child"); + TEST_ASSERT_TRUE_MESSAGE(WIFEXITED(status), + "the REPL child must exit normally via " + "quit's exit(0), not crash/signal"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, WEXITSTATUS(status), + "quit's exit(0) is the only correct " + "exit path"); + } /* ---- Decode every ReplaySession from both output files and compare * the decision sequence, session by session, decision by decision. ---- */ diff --git a/src/scheduler/testsuite/test_repl_execution_control.c b/src/scheduler/testsuite/test_repl_execution_control.c index 295e600f..6389a6ee 100644 --- a/src/scheduler/testsuite/test_repl_execution_control.c +++ b/src/scheduler/testsuite/test_repl_execution_control.c @@ -455,25 +455,61 @@ static void test_should_pause_never_fires_in_continue_mode(void) /* -- Observable (a), proved by output ORDER, not merely presence. */ /* ---------------------------------------------------------------------- */ -static xce_sched_sim_driver_t g_t2_sim; -static sched_sim_args_t g_t2_args; -static int g_t2_rc; - -static void run_driver_for_t2_capture(void) +/* + * task 12.5 (bug-1594): a completed --interactive run now re-enters the + * REPL (xce_repl_on_replay_complete(), repl.c) instead of returning, so a + * "continue"-to-EOF script that has no further scripted line will hit + * stdin EOF inside that re-entered repl_command_loop() -- which + * repl_command_loop() has always treated as an implicit `quit` (exit(0)). + * Before task 12.5 this test ran the driver IN-PROCESS (capture_stdout_ + * around()) because sched_sim_driver_run() genuinely returned once EOF was + * reached; now it must fork, exactly like Test 3 below, since exit(0) + * would otherwise tear down this whole unity binary mid-suite. + */ +static void child_prepause_then_quit(const char* replay_path, + const char* script_path, + const char* capture_path) { - g_t2_rc = sched_sim_driver_run(&g_t2_sim, &g_t2_args); + xce_sched_sim_driver_t sim; + sched_sim_args_t args; + + if (freopen(script_path, "r", stdin) == NULL) { + _exit(120); + } + if (freopen(capture_path, "w", stdout) == NULL) { + _exit(121); + } + + memset(&sim, 0, sizeof(sim)); + memset(&args, 0, sizeof(args)); + args.replay_path = replay_path; + args.sim_start_ts = SCHED_SIM_START_TS_LATEST; + args.sim_speed = 0; + args.interactive = true; + + (void)sched_sim_driver_run(&sim, &args); + + /* Unreachable in a correct implementation -- the script's trailing + * `quit` (or, absent that, stdin EOF) must already have called + * exit(0) from inside the post-replay-completion pause. */ + fflush(stdout); + _exit(111); } static void test_pre_replay_pause_blocks_before_any_event_is_processed(void) { char replay_path[PATH_MAX]; - char script_dir[PATH_MAX]; + char aux_dir[PATH_MAX]; char script_path[PATH_MAX]; + char capture_path[PATH_MAX]; + char pend_marker[80]; + char jobid_buf[64]; FILE* f; + pid_t pid; + int status; char* captured; char* ready_pos; char* pend_pos; - FILE* saved_stdin_check; f = write_header_and_snapshot(replay_path, sizeof(replay_path), "_prepause"); @@ -485,30 +521,43 @@ static void test_pre_replay_pause_blocks_before_any_event_is_processed(void) append_end_of_stream_marker(f); fclose(f); - make_tmp_dir(script_dir, sizeof(script_dir), "_prepausescript"); - snprintf(script_path, sizeof(script_path), "%s/script.txt", script_dir); - /* A single "continue" -- 12.1's own scope means continue never - * re-pauses, so the run reaches EOF cleanly without reading stdin - * again. */ - write_text_file(script_path, "continue\n"); + make_tmp_dir(aux_dir, sizeof(aux_dir), "_prepauseaux"); + snprintf(script_path, sizeof(script_path), "%s/script.txt", aux_dir); + snprintf(capture_path, sizeof(capture_path), "%s/capture.out", aux_dir); + /* "continue" runs to EOF (12.1's own scope: continue never re-pauses + * mid-stream absent a breakpoint); task 12.5 then re-enters the REPL + * at replay completion, so an explicit trailing `quit` is required to + * end the child deterministically. */ + write_text_file(script_path, "continue\nquit\n"); - saved_stdin_check = freopen(script_path, "r", stdin); - TEST_ASSERT_NOT_NULL_MESSAGE(saved_stdin_check, - "freopen of scripted stdin must succeed"); + pid = fork(); + TEST_ASSERT_TRUE_MESSAGE(pid >= 0, "fork() must succeed"); - memset(&g_t2_sim, 0, sizeof(g_t2_sim)); - memset(&g_t2_args, 0, sizeof(g_t2_args)); - g_t2_args.replay_path = replay_path; - g_t2_args.sim_start_ts = SCHED_SIM_START_TS_LATEST; - g_t2_args.sim_speed = 0; - g_t2_args.interactive = true; + if (pid == 0) { + child_prepause_then_quit(replay_path, script_path, capture_path); + _exit(112); /* unreachable -- child_prepause_then_quit never + * returns */ + } - captured = capture_stdout_around(run_driver_for_t2_capture); + TEST_ASSERT_EQUAL_INT_MESSAGE(pid, waitpid(pid, &status, 0), + "waitpid must reap the REPL child"); + TEST_ASSERT_TRUE_MESSAGE(WIFEXITED(status), + "the REPL child must exit normally via quit's " + "exit(0), not crash/signal"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, WEXITSTATUS(status), + "quit's exit(0) is the only correct exit " + "path"); - TEST_ASSERT_EQUAL_INT(0, g_t2_rc); + captured = read_whole_file(capture_path); + + /* bug-1591: sim_print_decision() (sim_loop.c) now renders jobids via + * fmt_jobid(), not raw %llu -- this marker must match that same + * human-readable form. */ + fmt_jobid(jobid_buf, sizeof(jobid_buf), 4242ULL); + snprintf(pend_marker, sizeof(pend_marker), "PEND jobid=%s", jobid_buf); ready_pos = strstr(captured, "[sim] ready. type 'help' for commands."); - pend_pos = strstr(captured, "PEND jobid=4242"); + pend_pos = strstr(captured, pend_marker); TEST_ASSERT_NOT_NULL_MESSAGE(ready_pos, "pre-replay pause banner must be printed"); @@ -526,6 +575,204 @@ static void test_pre_replay_pause_blocks_before_any_event_is_processed(void) "PASS\n"); } +/* + * task 12.5 (bug-1594) regression: `continue` reaching EOF with no + * breakpoints must NOT exit the process -- it must re-enter the REPL so + * the operator can inspect final state, and only `quit` (or stdin EOF, + * pre-existing documented behavior) may end it. + */ +static void child_continue_then_inspect(const char* replay_path, + const char* script_path, + const char* capture_path) +{ + xce_sched_sim_driver_t sim; + sched_sim_args_t args; + + if (freopen(script_path, "r", stdin) == NULL) { + _exit(120); + } + if (freopen(capture_path, "w", stdout) == NULL) { + _exit(121); + } + + memset(&sim, 0, sizeof(sim)); + memset(&args, 0, sizeof(args)); + args.replay_path = replay_path; + args.sim_start_ts = SCHED_SIM_START_TS_LATEST; + args.sim_speed = 0; + args.interactive = true; + + (void)sched_sim_driver_run(&sim, &args); + + fflush(stdout); + _exit(111); /* unreachable -- the script's `quit` must already have + * exited */ +} + +static void test_continue_with_no_breakpoints_stays_alive_after_replay_complete( + void) +{ + char replay_path[PATH_MAX]; + char aux_dir[PATH_MAX]; + char script_path[PATH_MAX]; + char capture_path[PATH_MAX]; + FILE* f; + pid_t pid; + int status; + char* captured; + + f = write_header_and_snapshot(replay_path, sizeof(replay_path), + "_contnoexit"); + TEST_ASSERT_NOT_NULL(f); + + append_queue_update_event(f, 1000ULL, "normal"); + append_jobs_push_event(f, 1500ULL, 9090ULL, "normal"); + append_replay_session(f, 2000ULL); + append_end_of_stream_marker(f); + fclose(f); + + make_tmp_dir(aux_dir, sizeof(aux_dir), "_contnoexitaux"); + snprintf(script_path, sizeof(script_path), "%s/script.txt", aux_dir); + snprintf(capture_path, sizeof(capture_path), "%s/capture.out", aux_dir); + /* No breakpoints set. After `continue` runs to EOF, the REPL must + * still be alive to process `info summary` before `quit`. */ + write_text_file(script_path, "continue\ninfo summary\nquit\n"); + + pid = fork(); + TEST_ASSERT_TRUE_MESSAGE(pid >= 0, "fork() must succeed"); + + if (pid == 0) { + child_continue_then_inspect(replay_path, script_path, capture_path); + _exit(112); + } + + TEST_ASSERT_EQUAL_INT_MESSAGE(pid, waitpid(pid, &status, 0), + "waitpid must reap the REPL child"); + TEST_ASSERT_TRUE_MESSAGE( + WIFEXITED(status), + "the child must exit normally via quit's exit(0) -- a hang or " + "crash means the REPL did not survive replay completion"); + TEST_ASSERT_EQUAL_INT(0, WEXITSTATUS(status)); + + captured = read_whole_file(capture_path); + + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "[sim] replay complete."), + "replay completion must still print its banner"); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "1 jobs (1 pending, 0 running)"), + "info summary issued AFTER replay completion must still execute " + "against final state -- proves the REPL did not exit on EOF " + "(bug-1594)"); + + free(captured); + + printf("test_continue_with_no_breakpoints_stays_alive_after_replay_" + "complete: PASS\n"); +} + +/* + * task 12.5 (bug-1594) regression: `step`/`continue` issued AFTER replay + * is already complete must re-prompt with an informational message, not + * be treated as "hand control back to a driver loop" (there is none left). + */ +static void child_continue_twice_after_complete(const char* replay_path, + const char* script_path, + const char* capture_path) +{ + xce_sched_sim_driver_t sim; + sched_sim_args_t args; + + if (freopen(script_path, "r", stdin) == NULL) { + _exit(120); + } + if (freopen(capture_path, "w", stdout) == NULL) { + _exit(121); + } + + memset(&sim, 0, sizeof(sim)); + memset(&args, 0, sizeof(args)); + args.replay_path = replay_path; + args.sim_start_ts = SCHED_SIM_START_TS_LATEST; + args.sim_speed = 0; + args.interactive = true; + + (void)sched_sim_driver_run(&sim, &args); + + fflush(stdout); + _exit(111); +} + +static void test_continue_after_replay_complete_reprompts_instead_of_exiting( + void) +{ + char replay_path[PATH_MAX]; + char aux_dir[PATH_MAX]; + char script_path[PATH_MAX]; + char capture_path[PATH_MAX]; + FILE* f; + pid_t pid; + int status; + char* captured; + + f = write_header_and_snapshot(replay_path, sizeof(replay_path), + "_contafter"); + TEST_ASSERT_NOT_NULL(f); + + append_queue_update_event(f, 1000ULL, "normal"); + append_jobs_push_event(f, 1500ULL, 8080ULL, "normal"); + append_replay_session(f, 2000ULL); + append_end_of_stream_marker(f); + fclose(f); + + make_tmp_dir(aux_dir, sizeof(aux_dir), "_contafteraux"); + snprintf(script_path, sizeof(script_path), "%s/script.txt", aux_dir); + snprintf(capture_path, sizeof(capture_path), "%s/capture.out", aux_dir); + /* Second `continue`/`step` are issued AFTER replay is already + * complete -- both must re-prompt, not attempt to resume anything. */ + write_text_file(script_path, "continue\ncontinue\nstep\nquit\n"); + + pid = fork(); + TEST_ASSERT_TRUE_MESSAGE(pid >= 0, "fork() must succeed"); + + if (pid == 0) { + child_continue_twice_after_complete(replay_path, script_path, + capture_path); + _exit(112); + } + + TEST_ASSERT_EQUAL_INT_MESSAGE(pid, waitpid(pid, &status, 0), + "waitpid must reap the REPL child"); + TEST_ASSERT_TRUE_MESSAGE(WIFEXITED(status), + "the child must exit normally via quit's " + "exit(0)"); + TEST_ASSERT_EQUAL_INT(0, WEXITSTATUS(status)); + + captured = read_whole_file(capture_path); + + { + const char* needle = "[sim] replay already complete -- no more " + "events to process. Use 'quit' to exit."; + char* first = strstr(captured, needle); + char* second = (first != NULL) + ? strstr(first + 1, needle) + : NULL; + + TEST_ASSERT_NOT_NULL_MESSAGE( + first, "a `continue` issued after replay completion must " + "print the re-prompt message, not silently resume/exit " + "(bug-1594)"); + TEST_ASSERT_NOT_NULL_MESSAGE( + second, "a `step` issued after replay completion must ALSO " + "print the re-prompt message"); + } + + free(captured); + + printf("test_continue_after_replay_complete_reprompts_instead_of_" + "exiting: PASS\n"); +} + /* ---------------------------------------------------------------------- */ /* Test 3: "step 1" against a three-session fixture processes EXACTLY one */ /* session before re-pausing -- proved end to end via a forked child that */ @@ -572,6 +819,10 @@ static void test_step_1_processes_exactly_one_session_then_quit_exits(void) char aux_dir[PATH_MAX]; char script_path[PATH_MAX]; char capture_path[PATH_MAX]; + char pend_marker_111[80]; + char pend_marker_222[80]; + char pend_marker_333[80]; + char jobid_buf[64]; FILE* f; pid_t pid; int status; @@ -620,12 +871,25 @@ static void test_step_1_processes_exactly_one_session_then_quit_exits(void) captured = read_whole_file(capture_path); - TEST_ASSERT_NOT_NULL_MESSAGE(strstr(captured, "PEND jobid=111"), + /* bug-1591: sim_print_decision() (sim_loop.c) now renders jobids via + * fmt_jobid(), not raw %llu -- these markers must match that same + * human-readable form. */ + fmt_jobid(jobid_buf, sizeof(jobid_buf), 111ULL); + snprintf(pend_marker_111, sizeof(pend_marker_111), "PEND jobid=%s", + jobid_buf); + fmt_jobid(jobid_buf, sizeof(jobid_buf), 222ULL); + snprintf(pend_marker_222, sizeof(pend_marker_222), "PEND jobid=%s", + jobid_buf); + fmt_jobid(jobid_buf, sizeof(jobid_buf), 333ULL); + snprintf(pend_marker_333, sizeof(pend_marker_333), "PEND jobid=%s", + jobid_buf); + + TEST_ASSERT_NOT_NULL_MESSAGE(strstr(captured, pend_marker_111), "session 1's decision must have run"); - TEST_ASSERT_NULL_MESSAGE(strstr(captured, "PEND jobid=222"), + TEST_ASSERT_NULL_MESSAGE(strstr(captured, pend_marker_222), "session 2 must NEVER have run -- step 1 " "must pause strictly after session 1"); - TEST_ASSERT_NULL_MESSAGE(strstr(captured, "PEND jobid=333"), + TEST_ASSERT_NULL_MESSAGE(strstr(captured, pend_marker_333), "session 3 must NEVER have run either"); free(captured); @@ -642,6 +906,8 @@ int main(void) RUN_TEST(test_should_pause_never_fires_in_continue_mode); RUN_TEST(test_pre_replay_pause_blocks_before_any_event_is_processed); RUN_TEST(test_step_1_processes_exactly_one_session_then_quit_exits); + RUN_TEST(test_continue_with_no_breakpoints_stays_alive_after_replay_complete); + RUN_TEST(test_continue_after_replay_complete_reprompts_instead_of_exiting); return UNITY_END(); } diff --git a/src/scheduler/testsuite/test_resval_ownership.c b/src/scheduler/testsuite/test_resval_ownership.c new file mode 100644 index 00000000..a1a0b726 --- /dev/null +++ b/src/scheduler/testsuite/test_resval_ownership.c @@ -0,0 +1,549 @@ +/* + * test_resval_ownership.c - regression test for bug-1585's true root cause: + * a heap-use-after-free crash in the normal-mode driver's periodic resource- + * value (resval) update path, live-reproduced via gdb/ASAN against a real + * controller+scheduler+xcecd_virtual deployment (see .wolf/buglog.json + * bug-1585 for the full incident/backtrace). + * + * Root cause: core_events.c's XCE_CORE_EVENT_RESVALS_FULL and + * XCE_CORE_EVENT_RESVALS_DELTA handlers do `zarray_put(&(zarray), idx, + * rv->val)` -- a SHALLOW store of the wire-decoded `xce_val_t*` pointer + * (verified by reading zarray_put_ex(), src/lib/zarray.c: `array->data[idx] + * = data[i]`, no copy). zarray now considers itself the sole owner of that + * pointer, and will hand it to the zarray's killer (kill_sched_resval, + * schedobj.c) the NEXT time this same index is replaced. But driver.c's + * release_translated_payload() (XCE_SCHED_OP_UPDATE_RESVALS/_RESVALS_FULL + * cases) UNCONDITIONALLY frees the entire decoded ResValList/ResourceVal + * array immediately after xce_core_push_events() returns -- including the + * SAME `rv->val` the zarray just adopted by reference. The result: the very + * next periodic resval report for the same worker+resource (which happens + * every resource_sync interval on any live deployment, i.e. inevitably) + * triggers zarray_put's eviction of the now-dangling first value, which + * calls kill_sched_resval() -> free_Value() -> a heap-use-after-free READ + * on an already-fully-freed xce_val_t. This reliably crashes the live + * scheduler process within one resval-report cycle of any real deployment, + * which is why a `.replay` recording captured from a live session so often + * turns out empty/truncated (bug-1585): the scheduler dies before or during + * the interesting part of the session. + * + * This exact hazard was already NOTICED and explicitly documented as a + * "PRE-EXISTING production ownership characteristic" in + * test_driver_translate.c's test_drain_translates_and_pushes_all_ten_op_types + * comment (task 5.1) -- but never fixed, because that file's own + * kill_sched_resval is a pure no-op stub ("fixtures ... never torn down, so + * these killers are never actually invoked" -- true for the OTHER kvmgr + * datakillers, which fire only on core teardown, but NOT true for a + * zarray's killer, which fires on every REPLACEMENT of an occupied slot, + * not just teardown). That blind spot is exactly why no existing test + * caught this: this file uses the REAL kill_sched_resval (mirroring + * schedobj.c's implementation exactly) so the use-after-free is genuinely + * exercised, under ASAN, the same way the live crash was. + * + * Fix (core_events.c): both call sites now `dup_val(rv->val)` before + * zarray_put -- the same deep-copy-before-adopt convention this codebase + * ALREADY establishes elsewhere for the identical hazard (snapshot_load.c: + * `zarray_put(&(core->sharedInst), r->idx, dup_val(rv->val))` and + * `zarray_put(&(w->resvals), r->idx, dup_val(nv->val))`). After the fix, + * the zarray owns an independent copy; driver.c's unconditional free of the + * original decoded payload no longer touches memory the core still holds. + * + * White-box #include of ../src/core.c, ../src/core_clock.c, + * ../src/core_events.c, and ../src/driver.c -- same convention as + * test_driver_translate.c (this file's sibling), including its full stub + * set for driver.c/core_events.c's link-time externs this file's own tests + * never exercise (session/plugin/recording/publish -- see each stub's own + * comment, copied verbatim). The two stubs that matter for THIS file's + * Observable, unlike test_driver_translate.c, are NOT no-ops: + * + * - kill_sched_resval: the REAL implementation (schedobj.c:260 verbatim + * -- free_Value(val), no separate free(val); schedobj.c is not pulled + * in directly for the same "avoid its large stub surface" reason + * test_core_events.c/test_driver_translate.c already document for + * sched_resolve_job_picker). + * - dup_val: the REAL implementation (snapshot_wire.c:37 verbatim, same + * "reimplement the one small function instead of pulling in the whole + * file" rationale) -- core_events.c's fix (once applied) calls this by + * `extern` declaration, same idiom snapshot_load.c/snapshot.c already + * use for the identical symbol. + */ +#include +#include +#include +#include + +#include + +#include "controller.capnp.h" +#include "kvmgr.h" +#include "scheduler.h" +#include "xstring.h" + +/* Pull in the real xce_sched_core_init()/xce_core_push_events()/ + * xce_sched_driver_translate_pending_events() under test into this TU -- + * same white-box #include set as test_driver_translate.c. */ +#include "../src/core.c" +#include "../src/core_clock.c" +#include "../src/core_events.c" +#include "../src/driver.c" + +void kill_sched_job(void* arg) { (void)arg; } +void kill_sched_queue(void* arg) { (void)arg; } +void kill_sched_worker(void* arg) { (void)arg; } +void kill_sched_user(void* arg) { (void)arg; } +void kill_sched_project(void* arg) { (void)arg; } +void plugin_unregister(void* arg) { (void)arg; } + +/* REAL implementation -- schedobj.c:260 verbatim (see file header comment: + * this is the whole point of this test file). Deliberately reimplemented + * rather than pulling in schedobj.c itself (large stub surface, same + * rationale test_core_events.c/test_driver_translate.c already document for + * sched_resolve_job_picker). */ +void kill_sched_resval(xce_val_t* val) +{ + if (val == NULL) { + return; + } + + free_Value(val); +} + +/* REAL implementation -- snapshot_wire.c:37 verbatim. This is the fix under + * test: core_events.c's RESVALS_FULL/RESVALS_DELTA handlers must dup_val() + * before zarray_put() so the zarray owns an independent copy, not the same + * pointer driver.c's release_translated_payload() is about to free. */ +xce_val_t* dup_val(const xce_val_t* src) +{ + xce_val_t* v; + + if (src == NULL) { + return NULL; + } + + v = (xce_val_t*)calloc(1, sizeof(*v)); + if (v == NULL) { + return NULL; + } + + v->which = src->which; + if (src->which == XCE_VAL_STRING) { + v->u.svalue + = (src->u.svalue != NULL) ? makeString(src->u.svalue) : NULL; + } else { + v->u = src->u; + } + + return v; +} + +/* Link-satisfying stubs for scheduler.c's sched_publish_dispatch()/ + * sched_publish_pend_reason() -- copied verbatim from test_driver_translate.c + * (see that file's header comment); never invoked by this file's tests. */ +int sched_publish_dispatch(xce_scheduler_t* sched, xce_job_dispatch_t* d) +{ + (void)sched; + (void)d; + return 0; +} + +int sched_publish_pend_reason(xce_scheduler_t* sched, xce_preason_set_t* r) +{ + (void)sched; + (void)r; + return 0; +} + +/* Link-satisfying stub -- copied verbatim from test_driver_translate.c. + * xce_sched_driver_translate_pending_events() calls this behind a NULL + * `record_ctx` check; every call site below passes NULL, so never actually + * invoked at runtime. */ +int sched_record_write_events(xce_scheduler_t* sched, xce_core_event_t* events, + int n) +{ + (void)sched; + (void)events; + (void)n; + return 0; +} + +int sched_record_start(xce_scheduler_t* sched, const char* record_file) +{ + (void)sched; + (void)record_file; + return 0; +} + +int sched_record_stop(xce_scheduler_t* sched) +{ + (void)sched; + return 0; +} + +sched_record_session_ctx_t* sched_record_session_begin(xce_scheduler_t* sched, + uint64_t begin_round_ts) +{ + (void)sched; + (void)begin_round_ts; + return NULL; +} + +void sched_record_session_add_decision(sched_record_session_ctx_t* ctx, + uint64_t ts, xce_decision_t* decision) +{ + (void)ctx; + (void)ts; + (void)decision; +} + +int sched_record_session_end(xce_scheduler_t* sched, + sched_record_session_ctx_t* ctx, uint64_t end_ts) +{ + (void)sched; + (void)ctx; + (void)end_ts; + return 0; +} + +int xce_core_begin_session(xce_sched_core_t* core) +{ + (void)core; + return 0; +} + +int xce_core_signal_go(xce_sched_core_t* core) +{ + (void)core; + return 0; +} + +int xce_core_end_session(xce_sched_core_t* core) +{ + (void)core; + return 0; +} + +int plugin_begin_session(void* arg) +{ + (void)arg; + return 0; +} + +int plugin_end_session(void* arg) +{ + (void)arg; + return 0; +} + +int sched_account_jobs(xce_scheduler_t* sched) +{ + (void)sched; + return 0; +} + +/* Real reimplementation of schedobj.c's sched_resolve_job_picker -- copied + * verbatim from test_driver_translate.c's own copy (same rationale: no + * picker plugin is registered in this file's fixture, so this always + * returns -EINVAL and xce_core_push_events() proceeds with picker=NULL -- + * every case in core_events.c already NULL-guards its picker hook call). */ +int sched_resolve_job_picker(xce_sched_core_t* core, xce_sched_plugin_t** out) +{ + xce_sched_plugin_t* plugin = NULL; + int rc = 0; + + if ((core == NULL) || (out == NULL)) { + return -EINVAL; + } + + *out = NULL; + + if (core->job_pick_plugin == NULL) { + return -EINVAL; + } + + lock(core->plugins, RTTI_LOCK_READ); + rc = key_find(core->plugins, core->job_pick_plugin, (void**)&plugin); + unlock(core->plugins); + + if (rc <= 0) { + return (rc < 0) ? rc : -ENOENT; + } + + if ((plugin == NULL) || (plugin->picker_iface == NULL) + || (plugin->picker_iface->next_job == NULL) + || (plugin->picker_iface->begin_session == NULL) + || (plugin->picker_iface->end_session == NULL) + || (plugin->picker_iface->on_dispatch == NULL) + || (plugin->picker_iface->on_pend == NULL)) { + return -EINVAL; + } + + *out = plugin; + + return 0; +} + +void setUp(void) { return; } +void tearDown(void) { return; } + +/* ---------------------------------------------------------------------- */ +/* Fixture -- identical to test_driver_translate.c's own. */ +/* ---------------------------------------------------------------------- */ + +static void fixture_init(xce_sched_core_t* core) +{ + int rc; + + rc = xce_sched_core_init(core); + TEST_ASSERT_EQUAL_INT(0, rc); + + core->pend_updates[0] = (dataq_t*)calloc(1, sizeof(dataq_t)); + dataq_init(core->pend_updates[0], NULL); + core->pend_updates[1] = (dataq_t*)calloc(1, sizeof(dataq_t)); + dataq_init(core->pend_updates[1], NULL); +} + +static void enqueue(xce_sched_core_t* core, xce_sched_update_request_t* req) +{ + dataq_enq(core->pend_updates[0], &(req->link)); +} + +static xce_sched_update_request_t* make_req(xce_sched_op_t op) +{ + xce_sched_update_request_t* req + = (xce_sched_update_request_t*)calloc(1, sizeof(*req)); + + req->op = op; + return req; +} + +/* Builds one heap-allocated worker->name-tagged resval delta, exactly the + * shape decode_ResourceVal_list() (controller.capnp.c) produces on the real + * wire-decode path -- see test_driver_translate.c's own + * make_wire_job()-adjacent comment for why heap allocation (not a stack + * local) matters here: this is what release_translated_payload()'s real + * free_ResValList_ptr() call expects to free. */ +static xce_resval_list_t* make_resval_delta(const char* host, + const char* resname, + double fvalue) +{ + xce_val_t* val = (xce_val_t*)calloc(1, sizeof(*val)); + xce_resval_t* rv = (xce_resval_t*)calloc(1, sizeof(*rv)); + xce_resval_t** vals = (xce_resval_t**)calloc(1, sizeof(*vals)); + xce_resval_list_t* list = (xce_resval_list_t*)calloc(1, sizeof(*list)); + + val->which = XCE_VAL_NUMERIC; + val->u.fvalue = fvalue; + + rv->name = strdup(resname); + rv->host = strdup(host); + rv->val = val; + + vals[0] = rv; + list->count = 1; + list->vals = vals; + + return list; +} + +/* ---------------------------------------------------------------------- */ +/* RESVALS_DELTA: two successive drains targeting the SAME worker+resource */ +/* slot. RED (pre-fix): the second drain's zarray_put() eviction of the */ +/* first drain's already-freed value crashes under ASAN */ +/* (heap-use-after-free in free_Value, matching bug-1585's live backtrace).*/ +/* GREEN (post-fix): both drains succeed, and the worker's live resval */ +/* reads back the SECOND drain's value (8.0), proving no data corruption. */ +/* ---------------------------------------------------------------------- */ + +static void test_resvals_delta_second_drain_does_not_use_after_free(void) +{ + xce_sched_core_t core; + xce_worker_t* wire_worker; + xce_worker_t** worker_ptrs; + xce_resource_t* wire_resource; + xce_resource_t** resource_ptrs; + xce_resource_t* registered_res = NULL; + xce_sched_worker_t* w; + xce_val_t* got; + int rc; + + fixture_init(&core); + + /* Heap-allocated, heap-allocated-string wire payloads -- AND + * heap-allocated wrapper ARRAYS (not a stack array of pointers) -- + * driver.c's REAL release_translated_payload() (this file links the + * genuine driver.c, not a stub) calls the real capnp-generated + * free_Worker_ptr()/free_Resource_ptr() on each element AND + * free(req->u.workerupdate.workers) on the array itself, exactly like a + * genuinely wire-decoded payload -- see test_driver_translate.c's + * make_wire_job() comment for why a stack local or string literal here + * would crash immediately (it does: this is precisely the pitfall this + * comment warns about, for both the element and the array). */ + wire_worker = (xce_worker_t*)calloc(1, sizeof(*wire_worker)); + wire_worker->name = strdup("hostA"); + worker_ptrs = (xce_worker_t**)calloc(1, sizeof(*worker_ptrs)); + worker_ptrs[0] = wire_worker; + + wire_resource = (xce_resource_t*)calloc(1, sizeof(*wire_resource)); + wire_resource->name = strdup("mem"); + resource_ptrs = (xce_resource_t**)calloc(1, sizeof(*resource_ptrs)); + resource_ptrs[0] = wire_resource; + + { + xce_sched_update_request_t* rw = make_req(XCE_SCHED_OP_UPDATE_WORKER); + rw->u.workerupdate.nworkers = 1; + rw->u.workerupdate.workers = worker_ptrs; + enqueue(&core, rw); + + xce_sched_update_request_t* rr + = make_req(XCE_SCHED_OP_UPDATE_RESOURCES); + rr->u.resourceupdate.nresources = 1; + rr->u.resourceupdate.resources = resource_ptrs; + enqueue(&core, rr); + + /* xce_sched_driver_translate_pending_events() returns the number of + * events translated on success (test_driver_translate.c's own + * established convention -- e.g. its 10-op-type drain test asserts + * `TEST_ASSERT_EQUAL_INT(10, rc)`), not a bare 0/-errno status. Two + * requests (worker + resource) were enqueued above. */ + rc = xce_sched_driver_translate_pending_events(&core, NULL); + TEST_ASSERT_EQUAL_INT(2, rc); + } + + /* makeResource() (resobj.c) deep-copies into a FRESH xce_resource_t + * instance and assigns ITS OWN idx from the resmgr's key_add() return + * value -- wire_resource.idx (the source struct pushed above) is never + * mutated in place, so the authoritative idx must be read back from the + * resmgr, not assumed. */ + rc = key_find(core.resmgr, "mem", (void**)®istered_res); + TEST_ASSERT_TRUE(rc > 0); + TEST_ASSERT_NOT_NULL(registered_res); + + /* Drain #1: first periodic resval report -- adopts value 4.0. */ + { + xce_sched_update_request_t* r1 = make_req(XCE_SCHED_OP_UPDATE_RESVALS); + r1->u.resvalupdate.resvals = make_resval_delta("hostA", "mem", 4.0); + enqueue(&core, r1); + + rc = xce_sched_driver_translate_pending_events(&core, NULL); + TEST_ASSERT_EQUAL_INT(1, rc); + } + + /* Drain #2: second periodic resval report for the SAME worker+resource + * -- this is exactly what a live deployment's resource_sync thread does + * every interval. Pre-fix, zarray_put()'s eviction of drain #1's value + * (already freed by drain #1's own release_translated_payload()) is a + * heap-use-after-free -- ASAN aborts the process right here. */ + { + xce_sched_update_request_t* r2 = make_req(XCE_SCHED_OP_UPDATE_RESVALS); + r2->u.resvalupdate.resvals = make_resval_delta("hostA", "mem", 8.0); + enqueue(&core, r2); + + rc = xce_sched_driver_translate_pending_events(&core, NULL); + TEST_ASSERT_EQUAL_INT(1, rc); + } + + w = sched_get_worker(&core, "hostA"); + TEST_ASSERT_NOT_NULL(w); + + got = (xce_val_t*)zarray_get(&(w->resvals), registered_res->idx); + TEST_ASSERT_NOT_NULL(got); + TEST_ASSERT_EQUAL_INT(XCE_VAL_NUMERIC, got->which); + TEST_ASSERT_EQUAL_FLOAT(8.0f, (float)got->u.fvalue); +} + +/* ---------------------------------------------------------------------- */ +/* RESVALS_FULL: same hazard, core->sharedInst instead of a per-worker */ +/* zarray (core_events.c's XCE_CORE_EVENT_RESVALS_FULL case, line ~750). */ +/* ---------------------------------------------------------------------- */ + +static void test_resvals_full_second_drain_does_not_use_after_free(void) +{ + xce_sched_core_t core; + xce_resource_t* wire_resource; + xce_resource_t** resource_ptrs; + xce_resource_t* registered_res = NULL; + xce_val_t* got; + int rc; + + fixture_init(&core); + + wire_resource = (xce_resource_t*)calloc(1, sizeof(*wire_resource)); + wire_resource->name = strdup("slots"); + resource_ptrs = (xce_resource_t**)calloc(1, sizeof(*resource_ptrs)); + resource_ptrs[0] = wire_resource; + + { + xce_sched_update_request_t* rr + = make_req(XCE_SCHED_OP_UPDATE_RESOURCES); + rr->u.resourceupdate.nresources = 1; + rr->u.resourceupdate.resources = resource_ptrs; + enqueue(&core, rr); + + rc = xce_sched_driver_translate_pending_events(&core, NULL); + TEST_ASSERT_EQUAL_INT(1, rc); + } + + /* See test_resvals_delta_second_drain_does_not_use_after_free()'s + * identical comment: makeResource() assigns idx on its own fresh + * instance, never on wire_resource itself. */ + rc = key_find(core.resmgr, "slots", (void**)®istered_res); + TEST_ASSERT_TRUE(rc > 0); + TEST_ASSERT_NOT_NULL(registered_res); + + /* Drain #1: first full resval snapshot -- adopts value 16.0. */ + { + xce_val_t* val1 = (xce_val_t*)calloc(1, sizeof(*val1)); + xce_resval_t* rv1 = (xce_resval_t*)calloc(1, sizeof(*rv1)); + xce_resval_t** vals1 = (xce_resval_t**)calloc(1, sizeof(*vals1)); + + val1->which = XCE_VAL_NUMERIC; + val1->u.fvalue = 16.0; + rv1->name = strdup("slots"); + rv1->val = val1; + vals1[0] = rv1; + + xce_sched_update_request_t* r1 + = make_req(XCE_SCHED_OP_UPDATE_RESVALS_FULL); + r1->u.resvalfullupdate.nvals = 1; + r1->u.resvalfullupdate.vals = vals1; + enqueue(&core, r1); + + rc = xce_sched_driver_translate_pending_events(&core, NULL); + TEST_ASSERT_EQUAL_INT(1, rc); + } + + /* Drain #2: second full resval snapshot for the SAME resource -- same + * live hazard as the RESVALS_DELTA test above. */ + { + xce_val_t* val2 = (xce_val_t*)calloc(1, sizeof(*val2)); + xce_resval_t* rv2 = (xce_resval_t*)calloc(1, sizeof(*rv2)); + xce_resval_t** vals2 = (xce_resval_t**)calloc(1, sizeof(*vals2)); + + val2->which = XCE_VAL_NUMERIC; + val2->u.fvalue = 32.0; + rv2->name = strdup("slots"); + rv2->val = val2; + vals2[0] = rv2; + + xce_sched_update_request_t* r2 + = make_req(XCE_SCHED_OP_UPDATE_RESVALS_FULL); + r2->u.resvalfullupdate.nvals = 1; + r2->u.resvalfullupdate.vals = vals2; + enqueue(&core, r2); + + rc = xce_sched_driver_translate_pending_events(&core, NULL); + TEST_ASSERT_EQUAL_INT(1, rc); + } + + got = (xce_val_t*)zarray_get(&(core.sharedInst), registered_res->idx); + TEST_ASSERT_NOT_NULL(got); + TEST_ASSERT_EQUAL_INT(XCE_VAL_NUMERIC, got->which); + TEST_ASSERT_EQUAL_FLOAT(32.0f, (float)got->u.fvalue); +} + +int main(void) +{ + UNITY_BEGIN(); + RUN_TEST(test_resvals_delta_second_drain_does_not_use_after_free); + RUN_TEST(test_resvals_full_second_drain_does_not_use_after_free); + return UNITY_END(); +} diff --git a/src/scheduler/testsuite/test_sim_alt_events.c b/src/scheduler/testsuite/test_sim_alt_events.c index 371abc0b..59f99509 100644 --- a/src/scheduler/testsuite/test_sim_alt_events.c +++ b/src/scheduler/testsuite/test_sim_alt_events.c @@ -380,17 +380,23 @@ static char* run_sim_and_capture_stdout(const char* base_path, static void test_baseline_run_uses_base_file_events(void) { - char base_jobid_marker[32]; - char alt_jobid_marker[32]; + char base_jobid_marker[80]; + char alt_jobid_marker[80]; + char jobid_buf[64]; char path[PATH_MAX]; FILE* f; int rc; char* captured; - snprintf(base_jobid_marker, sizeof(base_jobid_marker), "jobid=%llu", - (unsigned long long)BASE_JOBID); - snprintf(alt_jobid_marker, sizeof(alt_jobid_marker), "jobid=%llu", - (unsigned long long)ALT_JOBID); + /* bug-1591: sim_print_decision() (sim_loop.c) now renders jobids via + * fmt_jobid(), not raw %llu -- these markers must match that same + * human-readable form, computed the same way, not hardcoded. */ + fmt_jobid(jobid_buf, sizeof(jobid_buf), BASE_JOBID); + snprintf(base_jobid_marker, sizeof(base_jobid_marker), "jobid=%s", + jobid_buf); + fmt_jobid(jobid_buf, sizeof(jobid_buf), ALT_JOBID); + snprintf(alt_jobid_marker, sizeof(alt_jobid_marker), "jobid=%s", + jobid_buf); f = write_header_and_snapshot(path, sizeof(path), "_baseline"); TEST_ASSERT_NOT_NULL(f); @@ -424,8 +430,9 @@ static void test_baseline_run_uses_base_file_events(void) static void test_alt_events_run_uses_alt_file_events_not_base(void) { - char base_jobid_marker[32]; - char alt_jobid_marker[32]; + char base_jobid_marker[80]; + char alt_jobid_marker[80]; + char jobid_buf[64]; char base_path[PATH_MAX]; char alt_path[PATH_MAX]; FILE* base_f; @@ -433,10 +440,14 @@ static void test_alt_events_run_uses_alt_file_events_not_base(void) int rc; char* captured; - snprintf(base_jobid_marker, sizeof(base_jobid_marker), "jobid=%llu", - (unsigned long long)BASE_JOBID); - snprintf(alt_jobid_marker, sizeof(alt_jobid_marker), "jobid=%llu", - (unsigned long long)ALT_JOBID); + /* bug-1591: see test_baseline_run_uses_base_file_events()'s identical + * fmt_jobid() comment. */ + fmt_jobid(jobid_buf, sizeof(jobid_buf), BASE_JOBID); + snprintf(base_jobid_marker, sizeof(base_jobid_marker), "jobid=%s", + jobid_buf); + fmt_jobid(jobid_buf, sizeof(jobid_buf), ALT_JOBID); + snprintf(alt_jobid_marker, sizeof(alt_jobid_marker), "jobid=%s", + jobid_buf); /* Base file: header + empty snapshot, then its OWN (never-to-be-read, * once --sim-alt-events is set) events for BASE_JOBID -- present to diff --git a/src/scheduler/testsuite/test_sim_event_loop.c b/src/scheduler/testsuite/test_sim_event_loop.c index 4d85fadc..a453e8d8 100644 --- a/src/scheduler/testsuite/test_sim_event_loop.c +++ b/src/scheduler/testsuite/test_sim_event_loop.c @@ -695,6 +695,8 @@ static void test_job_present_only_in_initial_snapshot_still_reaches_the_picker(void) { char path[PATH_MAX]; + char pend_marker[80]; + char jobid_buf[64]; FILE* f; int rc; char* captured; @@ -714,9 +716,15 @@ test_job_present_only_in_initial_snapshot_still_reaches_the_picker(void) captured = run_sim_and_capture_stdout(path, &rc); + /* bug-1591: sim_print_decision() (sim_loop.c) now renders jobids via + * fmt_jobid(), not raw %llu -- this marker must match that same + * human-readable form. */ + fmt_jobid(jobid_buf, sizeof(jobid_buf), 4242ULL); + snprintf(pend_marker, sizeof(pend_marker), "PEND jobid=%s", jobid_buf); + TEST_ASSERT_EQUAL_INT(0, rc); TEST_ASSERT_NOT_NULL_MESSAGE( - strstr(captured, "PEND jobid=4242"), + strstr(captured, pend_marker), "a job present only in the replay file's INITIAL snapshot must " "still reach the real picker and receive a live PEND decision " "(bug-1567)"); diff --git a/src/scheduler/testsuite/test_sim_verify.c b/src/scheduler/testsuite/test_sim_verify.c index 7f112fcf..b11672e6 100644 --- a/src/scheduler/testsuite/test_sim_verify.c +++ b/src/scheduler/testsuite/test_sim_verify.c @@ -482,8 +482,16 @@ static void log_one_mismatch_for_capture(void) static void test_log_mismatch_reports_logical_time_and_decision_detail(void) { + char jobid_marker[80]; + char jobid_buf[64]; char* buf = capture_stdout_around(log_one_mismatch_for_capture); + /* bug-1591: sim_verify_describe_{replay_,}decision() now render jobids + * via fmt_jobid(), not raw %llu -- this marker must match that same + * human-readable form. */ + fmt_jobid(jobid_buf, sizeof(jobid_buf), 9001ULL); + snprintf(jobid_marker, sizeof(jobid_marker), "jobid=%s", jobid_buf); + TEST_ASSERT_NOT_NULL_MESSAGE(strstr(buf, "MISMATCH"), "log_mismatch must report a MISMATCH"); TEST_ASSERT_NOT_NULL_MESSAGE( @@ -494,7 +502,7 @@ static void test_log_mismatch_reports_logical_time_and_decision_detail(void) "log_mismatch must report the decision's position within the " "session"); TEST_ASSERT_NOT_NULL_MESSAGE( - strstr(buf, "jobid=9001"), + strstr(buf, jobid_marker), "log_mismatch must report actionable decision detail (jobid)"); free(buf); @@ -831,6 +839,8 @@ static void test_verify_against_self_reports_zero_mismatches(void) static void test_verify_against_altered_copy_reports_exact_mismatch(void) { char path[PATH_MAX]; + char jobid_marker[80]; + char jobid_buf[64]; FILE* f; int rc; char* captured; @@ -846,13 +856,19 @@ static void test_verify_against_altered_copy_reports_exact_mismatch(void) captured = run_sim_and_capture_stdout(path, true, &rc); + /* bug-1591: sim_verify_describe_{replay_,}decision() now render + * jobids via fmt_jobid(), not raw %llu -- this marker must match that + * same human-readable form. */ + fmt_jobid(jobid_buf, sizeof(jobid_buf), 777ULL); + snprintf(jobid_marker, sizeof(jobid_marker), "jobid=%s", jobid_buf); + TEST_ASSERT_EQUAL_INT(0, rc); TEST_ASSERT_NOT_NULL_MESSAGE( strstr(captured, "MISMATCH"), "verifying against a deliberately-altered recorded decision must " "report a mismatch"); TEST_ASSERT_NOT_NULL_MESSAGE( - strstr(captured, "jobid=777"), + strstr(captured, jobid_marker), "the mismatch report must name the specific job"); free(captured);