diff --git a/.kiro/specs/scheduler-sim-discrete-clock/tasks.md b/.kiro/specs/scheduler-sim-discrete-clock/tasks.md index b555f2d1..bc25f081 100644 --- a/.kiro/specs/scheduler-sim-discrete-clock/tasks.md +++ b/.kiro/specs/scheduler-sim-discrete-clock/tasks.md @@ -11,7 +11,7 @@ Test-first throughout (red → green → refactor). Every sub-task leaves the bu - _Requirements: 2.3, 2.4, 2.5_ - _Boundary: CliCompat, scheduler test build_ -- [ ] 2. Core: pin the simulator clock, remove the REPL speed parameter, retire the CLI option +- [x] 2. Core: pin the simulator clock, remove the REPL speed parameter, retire the CLI option - [x] 2.1 (P) Pin the simulation-mode clock so it moves only at recorded timestamps - Write the tests first in the new test binary, each passing a non-zero speed through the still-present simulator argument so they fail on the current code: after start the clock equals the chosen snapshot's timestamp and stays equal after a real sleep, with the core holding the frozen speed setting; with a stand-in allocator that sleeps and a recorded session start that is not a whole second, the session start seen inside the session equals the recorded one exactly, and simulation output has that session start, a zero session duration and every decision timestamp equal to it, with at least one decision per session; a session start earlier than the preceding event, and events earlier than the start snapshot, set the clock to exactly those values and the replay completes; a mid-replay snapshot followed by the end marker leaves the clock at the last event's timestamp after the run and after a sleep, with the start timestamp pinned so the real skip branch runs; the same recording replayed twice gives identical logical timestamps and decisions in both outputs, ignoring only the wall-clock header field (design.md, Testing Strategy: Unit (SimClock), Integration (replay loop)). - Make simulator startup always select the frozen clock setting and drop the speed parameter from the start-clock function. The start-clock function keeps writing the shadow copy, now always the frozen setting, because `info config` and the startup test still read it; the simulator's speed argument is no longer read. Both are removed in 3.1. @@ -27,7 +27,7 @@ Test-first throughout (red → green → refactor). Every sub-task leaves the bu - Done when the REPL tests pass and no REPL output or help text offers a speed setting. - _Requirements: 1.5, 3.1, 3.2, 3.3, 3.4, 3.5_ - _Boundary: ReplSet, ReplHelp, ReplInfo_ -- [ ] 2.3 Retire `--sim-speed` on the command line and prove the default command line end to end +- [x] 2.3 Retire `--sim-speed` on the command line and prove the default command line end to end - Write the tests first in the new test binary, running the built scheduler binary as a child process on a fixture the test writes (session start not a whole second, a pending job before each session), draining its stdout before collecting the exit status: without the option it exits 0, its output has exact session starts, zero durations and decision timestamps equal to the session start, and its log has no warning; with each of `7`, `abc`, `-1` and an empty value (fresh output and log files per run) it exits 0, its output matches the default run except the wall-clock header field, and its log has exactly one warning naming the value; a repeated option logs one warning naming the last value; a trailing option with no value exits non-zero; an interactive run at end of input logs exactly one warning; `--help` describes the option as deprecated and mentions no real-time or as-fast-as-possible mode. - Stop defaulting, validating and passing a simulation speed; call the warning helper once right after the logger starts and, when the option was given, drain and restart the logger there while still single-threaded; update the option's help text and the comment that cites its numeric validation; the flag-meaning comments that describe the simulator speed argument are left to 3.1 (design.md, Modified Files: `main.c`; Error Handling). - Done when the end-to-end and help tests pass; the runs with `abc`, `-1`, an empty value and `7`, the repeated-option run, the interactive run and the help test fail against the pre-change command-line handling, while the run without the option is a regression guard that 2.1 already makes pass. diff --git a/src/scheduler/src/main.c b/src/scheduler/src/main.c index 49e8db9b..317f25b1 100644 --- a/src/scheduler/src/main.c +++ b/src/scheduler/src/main.c @@ -6,6 +6,7 @@ #include #include +#include "cli_compat.h" #include "client.h" #include "controller.capnp.h" #include "xce/cluster.h" @@ -89,9 +90,12 @@ static struct argp_option options[] = { { "sim-start-ts", OPT_SIM_START_TS, "TIMESTAMP", 0, "Simulation start timestamp (ns since epoch); default: the replay " "file's most recent snapshot" }, + /* scheduler-sim-discrete-clock Req 2.4/2.6: kept, still taking a + * value (a trailing --sim-speed stays an argp error), but never + * parsed; sched_cli_warn_deprecated_options() logs its one warning. */ { "sim-speed", OPT_SIM_SPEED, "N", 0, - "Simulation speed: 0=as-fast-as-possible, 1=real-time (default), " - "N>1=N x real-time" }, + "Deprecated, has no effect: the simulator clock moves only at " + "recorded timestamps. Will be removed in a future release." }, { "sim-verify", OPT_SIM_VERIFY, 0, 0, "Compare simulated decisions against the replay file's recorded ones" }, { "sim-alt-events", OPT_SIM_ALT_EVENTS, "FILE", 0, @@ -293,7 +297,7 @@ int main(int argc, char* argv[]) /* 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 + * record_dir above. Numeric validation mirrors --sim-start-ts'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(). */ @@ -324,6 +328,22 @@ int main(int argc, char* argv[]) mylog_init(logfile, loglevel, logsize, logcount); + /* scheduler-sim-discrete-clock Req 2.1-2.5: --sim-speed is accepted in + * both modes with any value and has no effect beyond this one startup + * warning. When it was given, drain the logger right away (tlog_exit() + * joins its writer thread only after every buffered line is on disk) + * and restart it, so the warning survives every later exit path: the + * --sim returns below, the REPL's exit(0) and normal-mode shutdown. + * Safe only here, while still single-threaded: no picker, allocator or + * driver thread exists before sched_sim_driver_run()/sched_init(), and + * tlog_exit() must not race another logging thread -- which is also why + * it is not called at the --sim returns or in the REPL's quit. */ + sched_cli_warn_deprecated_options(arguments.sim_speed); + if (arguments.sim_speed != NULL) { + mylog_fini(); + mylog_init(logfile, loglevel, logsize, logcount); + } + /* Req 5 AC 9 "Mode selection" / Req 5 AC 13 "Simulation mode startup" * (scheduler-core spec, task 11.1): --sim selects the simulation driver * (SimDriver) instead of the normal, controller-attached one. Checked @@ -336,7 +356,6 @@ int main(int argc, char* argv[]) sched_sim_args_t sim_args; xce_sched_sim_driver_t sim; uint64_t sim_start_ts = SCHED_SIM_START_TS_LATEST; - uint64_t sim_speed = 1; /* default: real-time */ int rc; if (arguments.sim_start_ts != NULL) { @@ -353,24 +372,9 @@ int main(int argc, char* argv[]) } } - if (arguments.sim_speed != NULL) { - char* endp = NULL; - - sim_speed = strtoull(arguments.sim_speed, &endp, 10); - if ((endp == NULL) || (*endp != '\0') - || (endp == arguments.sim_speed)) { - fprintf(stderr, - "xce_scheduler: --sim-speed expects a numeric " - "value, got \"%s\"\n", - arguments.sim_speed); - return 1; - } - } - memset(&sim_args, 0, sizeof(sim_args)); sim_args.replay_path = arguments.sim_file; sim_args.sim_start_ts = sim_start_ts; - sim_args.sim_speed = sim_speed; sim_args.verify = (arguments.sim_verify != 0); sim_args.alt_events_path = arguments.sim_alt_events; sim_args.output_path = arguments.sim_output; diff --git a/src/scheduler/testsuite/test_sim_discrete_clock.c b/src/scheduler/testsuite/test_sim_discrete_clock.c index e2a5115a..7c72cd71 100644 --- a/src/scheduler/testsuite/test_sim_discrete_clock.c +++ b/src/scheduler/testsuite/test_sim_discrete_clock.c @@ -29,9 +29,17 @@ * early-print path writes WARN lines to stdout, which the test-local * capture_stdout_around() below captures. * - * XCE_SCHEDULER_BIN (compile-time, from the scheduler executable's - * full_path() in testsuite/meson.build) is used by the end-to-end tests - * later tasks add to this file. + * Task 2.3 (Requirements 1.4, 1.7, 1.8, 2.1-2.6; design.md, Testing + * Strategy: CLI help and CLI end-to-end (a)-(e)): XCE_SCHEDULER_BIN + * (compile-time, from the scheduler executable's full_path() in + * testsuite/meson.build) runs as a child process on a fixture this file + * writes. Without --sim-speed the default command line produces exact + * session starts, zero durations and decision timestamps equal to the + * session start, with no warning; with any value (valid or not) the output + * is unchanged except recordingStartWall and the log holds exactly one + * warning naming the (last) value; a trailing --sim-speed is still an argp + * error; the REPL exit path keeps the warning; --help calls the option + * deprecated. */ #include @@ -43,6 +51,7 @@ #include #include #include +#include #include #include @@ -971,6 +980,274 @@ static void test_two_replays_produce_identical_output(void) } } +/* ---------------------------------------------------------------------- */ +/* CLI end to end and --help (task 2.3; Requirements 1.4, 1.7, 1.8, */ +/* 2.1-2.6). The built scheduler binary runs as a child process on the */ +/* two-session fixture; its stdout (decision lines) goes to /dev/null and */ +/* the exit status comes from WEXITSTATUS(pclose()). Every run gets a */ +/* fresh --sim-output and --logfile. */ +/* ---------------------------------------------------------------------- */ + +/* Runs `cmd` through the shell, drains its stdout to EOF and returns its + * exit status (-1 if it did not exit normally). */ +static int run_child(const char* cmd) +{ + FILE* p = popen(cmd, "r"); + char sink[512]; + int st; + + TEST_ASSERT_NOT_NULL_MESSAGE(p, "popen of the scheduler must succeed"); + while (fread(sink, 1, sizeof(sink), p) > 0) { + } + st = pclose(p); + TEST_ASSERT_TRUE_MESSAGE(st != -1, "pclose must succeed"); + return WIFEXITED(st) ? WEXITSTATUS(st) : -1; +} + +/* Runs the scheduler in batch simulation mode on `fixture` with `extra` + * appended to the command line; writes to fresh output and log files whose + * paths are returned. */ +static int run_sim_cli(const char* fixture, const char* extra, char* out, + char* log) +{ + char cmd[4 * PATH_MAX]; + + make_tmp_file(out, PATH_MAX, "cliout"); + make_tmp_file(log, PATH_MAX, "clilog"); + snprintf(cmd, sizeof(cmd), + XCE_SCHEDULER_BIN " --sim %s --sim-output %s --logfile %s %s " + "> /dev/null", + fixture, out, log, extra); + return run_child(cmd); +} + +/* Counts the log lines naming --sim-speed; *named is set to how many of + * them also contain `quoted`. An absent log counts as zero lines. */ +static int count_speed_log_lines(const char* log, const char* quoted, + int* named) +{ + FILE* f = fopen(log, "r"); + char line[4096]; + int n = 0; + + *named = 0; + if (f == NULL) { + return 0; + } + while (fgets(line, sizeof(line), f) != NULL) { + if (strstr(line, "--sim-speed") != NULL) { + n++; + if ((quoted != NULL) && (strstr(line, quoted) != NULL)) { + (*named)++; + } + } + } + fclose(f); + return n; +} + +static void assert_one_speed_warning(const char* log, const char* quoted) +{ + int named = 0; + + TEST_ASSERT_EQUAL_INT_MESSAGE(1, count_speed_log_lines(log, quoted, + &named), + "the log must hold exactly one " + "--sim-speed warning"); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, named, "the warning must name the " + "quoted value"); +} + +/* Output identical to the reference except ReplayHeader.recordingStartWall: + * header decoded and compared field by field, the rest byte for byte. */ +static void assert_same_output(const char* ref, const char* got) +{ + replay_header_t hdr[2]; + long len[2]; + long hdr_end[2]; + char* buf[2]; + int r; + + buf[0] = slurp_output(ref, &len[0], &hdr_end[0], &hdr[0]); + buf[1] = slurp_output(got, &len[1], &hdr_end[1], &hdr[1]); + + TEST_ASSERT_EQUAL_UINT64(hdr[0].recording_start_ts, + hdr[1].recording_start_ts); + TEST_ASSERT_EQUAL_STRING(hdr[0].recorder_version, hdr[1].recorder_version); + TEST_ASSERT_EQUAL_STRING(hdr[0].sched_policy, hdr[1].sched_policy); + TEST_ASSERT_EQUAL_PTR(NULL, hdr[0].cluster); + TEST_ASSERT_EQUAL_PTR(NULL, hdr[1].cluster); + TEST_ASSERT_EQUAL_INT64(len[0] - hdr_end[0], len[1] - hdr_end[1]); + TEST_ASSERT_EQUAL_MEMORY_MESSAGE(buf[0] + hdr_end[0], buf[1] + hdr_end[1], + (size_t)(len[0] - hdr_end[0]), + "--sim-speed must not change the " + "output after the header"); + for (r = 0; r < 2; r++) { + free_ReplayHeader(&hdr[r]); + free(buf[r]); + } +} + +/* (a) no --sim-speed: exit 0, exact session starts, zero durations, + * decision ts == beginRound (at least one decision per session), and no + * warning in the log. Returns the output path in `out` for (b). */ +static void run_default_cli(const char* fixture, char* out) +{ + const uint64_t begin[2] = { OUT_B1, OUT_B2 }; + char log[PATH_MAX]; + out_session_t sessions[4]; + int named = 0; + int n; + int k; + int i; + + TEST_ASSERT_EQUAL_INT_MESSAGE(0, run_sim_cli(fixture, "", out, log), + "the default command line must exit 0"); + n = read_output_sessions(out, sessions, 4); + TEST_ASSERT_EQUAL_INT(2, n); + for (k = 0; k < n; k++) { + TEST_ASSERT_EQUAL_UINT64(begin[k], sessions[k].begin_round); + TEST_ASSERT_EQUAL_UINT64(0ULL, sessions[k].duration_ns); + TEST_ASSERT_TRUE_MESSAGE(sessions[k].n_decisions >= 1, + "each session must decide at least once"); + for (i = 0; i < sessions[k].n_decisions; i++) { + TEST_ASSERT_EQUAL_UINT64(begin[k], sessions[k].decision_ts[i]); + } + } + TEST_ASSERT_EQUAL_INT_MESSAGE(0, count_speed_log_lines(log, NULL, &named), + "no --sim-speed warning without the " + "option"); + unlink(log); +} + +/* (a) 1.4, 1.7, 1.8, 2.5 on the default command line. */ +static void test_cli_default_exact_times_no_warning(void) +{ + char fixture[PATH_MAX]; + char out[PATH_MAX]; + + write_two_session_fixture(fixture, sizeof(fixture)); + run_default_cli(fixture, out); + unlink(out); +} + +/* (b) 2.1-2.4: any value, valid or not, exits 0 with output identical to + * the default run and exactly one warning naming the value. */ +static void test_cli_sim_speed_any_value_is_noop_with_one_warning(void) +{ + static const char* const extra[] = { "--sim-speed 7", "--sim-speed abc", + "--sim-speed -1", "--sim-speed=" }; + static const char* const quoted[] = { "\"7\"", "\"abc\"", "\"-1\"", + "\"\"" }; + char fixture[PATH_MAX]; + char ref[PATH_MAX]; + char out[PATH_MAX]; + char log[PATH_MAX]; + size_t i; + + write_two_session_fixture(fixture, sizeof(fixture)); + run_default_cli(fixture, ref); + for (i = 0; i < sizeof(extra) / sizeof(extra[0]); i++) { + TEST_ASSERT_EQUAL_INT_MESSAGE(0, run_sim_cli(fixture, extra[i], out, + log), + extra[i]); + assert_same_output(ref, out); + assert_one_speed_warning(log, quoted[i]); + unlink(out); + unlink(log); + } + unlink(ref); +} + +/* (c) 2.3: a repeated option logs one warning, naming the last value. */ +static void test_cli_repeated_sim_speed_names_last_value(void) +{ + char fixture[PATH_MAX]; + char out[PATH_MAX]; + char log[PATH_MAX]; + + write_two_session_fixture(fixture, sizeof(fixture)); + TEST_ASSERT_EQUAL_INT(0, run_sim_cli(fixture, + "--sim-speed 1 --sim-speed 9", out, + log)); + assert_one_speed_warning(log, "\"9\""); + unlink(out); + unlink(log); +} + +/* (d) 2.4: a trailing --sim-speed with no value is still an argp error. */ +static void test_cli_trailing_sim_speed_without_value_fails(void) +{ + char fixture[PATH_MAX]; + char cmd[2 * PATH_MAX]; + + write_two_session_fixture(fixture, sizeof(fixture)); + snprintf(cmd, sizeof(cmd), + XCE_SCHEDULER_BIN " --sim %s --sim-speed > /dev/null 2>&1", + fixture); + TEST_ASSERT_NOT_EQUAL_INT_MESSAGE(0, run_child(cmd), + "--sim-speed without a value must be " + "a command-line error"); +} + +/* (e) 2.3 on the REPL exit path: end of input -> quit -> exit(0), and the + * log holds exactly one warning. */ +static void test_cli_interactive_sim_speed_logs_one_warning(void) +{ + char fixture[PATH_MAX]; + char log[PATH_MAX]; + char cmd[3 * PATH_MAX]; + + write_two_session_fixture(fixture, sizeof(fixture)); + make_tmp_file(log, sizeof(log), "clilogi"); + snprintf(cmd, sizeof(cmd), + XCE_SCHEDULER_BIN " --sim %s -i --sim-speed 3 --logfile %s " + "< /dev/null > /dev/null", + fixture, log); + TEST_ASSERT_EQUAL_INT(0, run_child(cmd)); + assert_one_speed_warning(log, "\"3\""); + unlink(log); +} + +/* 2.6: --help describes --sim-speed as deprecated and offers no speed or + * real-time mode. */ +static void test_cli_help_describes_sim_speed_as_deprecated(void) +{ + FILE* p = popen(XCE_SCHEDULER_BIN " --help", "r"); + char* text; + size_t cap = 65536; + size_t len = 0; + size_t got; + char* entry; + char* eol; + + TEST_ASSERT_NOT_NULL(p); + text = (char*)calloc(1, cap); + TEST_ASSERT_NOT_NULL(text); + while ((got = fread(text + len, 1, cap - 1 - len, p)) > 0) { + len += got; + } + TEST_ASSERT_EQUAL_INT(0, WEXITSTATUS(pclose(p))); + + entry = strstr(text, "--sim-speed"); + TEST_ASSERT_NOT_NULL_MESSAGE(entry, "--help must list --sim-speed"); + eol = strchr(entry, '\n'); + if (eol != NULL) { + *eol = '\0'; + } + TEST_ASSERT_NOT_NULL_MESSAGE(strstr(entry, "Deprecated"), + "the --sim-speed entry must say " + "Deprecated"); + if (eol != NULL) { + *eol = '\n'; + } + TEST_ASSERT_NULL_MESSAGE(strstr(text, "real-time"), + "--help must not mention real-time"); + TEST_ASSERT_NULL_MESSAGE(strstr(text, "as-fast-as-possible"), + "--help must not mention as-fast-as-possible"); + free(text); +} + int main(void) { UNITY_BEGIN(); @@ -985,5 +1262,11 @@ int main(void) RUN_TEST(test_mid_replay_snapshot_and_end_leave_clock_at_last_event); RUN_TEST(test_session_start_and_output_times_equal_begin_round); RUN_TEST(test_two_replays_produce_identical_output); + RUN_TEST(test_cli_default_exact_times_no_warning); + RUN_TEST(test_cli_sim_speed_any_value_is_noop_with_one_warning); + RUN_TEST(test_cli_repeated_sim_speed_names_last_value); + RUN_TEST(test_cli_trailing_sim_speed_without_value_fails); + RUN_TEST(test_cli_interactive_sim_speed_logs_one_warning); + RUN_TEST(test_cli_help_describes_sim_speed_as_deprecated); return UNITY_END(); }