From 2d533e652e7bbe6070832c39348ccd08b0f88b3b Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Wed, 5 Aug 2026 14:35:52 +0000 Subject: [PATCH] test(scheduler-core): recording lifecycle integration tests (task 9.5) Extends test_admin_request.c with 3 tests closing the gap between what task 9.4's own suite already proved through scheduler_req_handler() and what this task's text specifically asks for: a ".."-traversal rejection and a real symlink(2)-escape rejection, both submitted as genuine SchedAdminRequests (not internal calls), and a disk-full simulation (fflush + setvbuf(_IONBF) + close(fd), matching task 9.3's technique) proving sched_record_abort_on_error()'s cleanup is visible through a subsequent real recordStatus request and that recording recovers cleanly afterward. Hard-link-aliasing coverage deliberately not re-added through the admin path -- task 9.1/9.2 already exhaustively prove that rejection branch, and the routing plumbing this task exists to prove is already retired by the dotdot/symlink cases. 17/17 in sched_admin_request_test, 26/26 scheduler suite, clang-format clean. Closes out Group 9 (Core: Recording Control). --- .kiro/specs/scheduler-core/tasks.md | 3 +- src/scheduler/testsuite/test_admin_request.c | 260 +++++++++++++++++++ 2 files changed, 262 insertions(+), 1 deletion(-) diff --git a/.kiro/specs/scheduler-core/tasks.md b/.kiro/specs/scheduler-core/tasks.md index 4b3e88bd..1bd60a02 100644 --- a/.kiro/specs/scheduler-core/tasks.md +++ b/.kiro/specs/scheduler-core/tasks.md @@ -249,11 +249,12 @@ - _Depends: 9.3_ - _Verified: 2026-08-05 — this is the scheduler's first-ever request-reply opcode (`OPCode_schedAdmin`, no prior precedent). `sched_handle_admin_request()` (new, handler.c) decodes `SchedAdminRequest` and dispatches by action: `recordStatus` is answered synchronously (new `sched_record_status()`, recording.c, pure read); `recordStart`/`recordStop` are deferred — enqueued as a new `XCE_SCHED_OP_ADMIN_RECORD` op onto the existing `pend_updates` queue (same mechanism every controller-sourced update already uses) rather than called directly from the request-handling thread, because `sched_record_start()`'s `xce_sched_core_save_snapshot()` call requires task 8.2's `session_active==0` precondition, only guaranteed on the driver thread — confirmed by review as a genuine (not theoretical) concern: `vtaskmgr.c` spawns real pthreads for request handling, distinct from `main.c`'s single driver-loop thread. `driver.c`'s `xce_sched_driver_translate_pending_events()` intercepts the deferred op, performs the real start/stop, and sends the reply via a new `xcecli_reply_schedadmin()` (src/lib/client.c/client.h) correlated by a plain `seq` copied by value before the original request is freed — confirmed safe since `xcecli_ctx_t` is a single unmultiplexed connection. Every reply-field branch (ok/err for each action) independently verified against requirements.md's semantics table, including the subtlety that `recordStop`'s `file` value must be captured before `sched_record_stop()` frees `record_file_path`. Round 1 review found one real bug beyond either implementer session's own testing: `sched_record_status()`, reachable from the vtask thread, read `recording`/`record_file_path`/`record_event_count`/`record_start_ts` with no lock while the driver thread's `sched_record_stop()` mutates and frees those same fields unsynchronized — a genuine cross-thread use-after-free, the codebase's first such race, missed by the ASan-only test config (ASan doesn't catch races). Fixed with a new dedicated `mylock_t record_lock` (deliberately separate from `core.lock`), scoped strictly to the in-memory field reads/writes only (never across `fopen()`/`fwrite()`/capnp I/O — confirmed no vtask-blocks-on-disk-I/O hazard reintroduced), with `sched_record_status()` now `strdup()`ing `record_file_path` while holding the lock rather than returning a borrowed live pointer. Round 2 review independently confirmed the fix: exhaustive grep found every access site now correctly locked (with `recording_session.c`'s driver-thread-only functions and `handle_deferred_admin_record()`'s post-start/stop reads correctly reasoned as safe without the lock, both same-thread-only by construction); confirmed non-recursive `mylock_t` and no nested acquisition (no new deadlock); independently reproduced the fix's necessity by temporarily stripping the lock and confirming the new lock-contention test fails exactly as predicted, then restoring and reconfirming green. 26/26 scheduler suite (14/14 in the new `sched_admin_request_test`), clang-format clean — independently re-verified by me via a fresh `meson compile`/`meson test --suite scheduler` run before marking complete. New `xcecli_reply_schedadmin()` addition to shared `src/lib/client.c`/`src/include/client.h` scoped purely to this one function, confirmed not touching `src/controller/` (task 10.1's exclusive boundary)._ -- [ ] 9.5 Recording lifecycle integration tests +- [x] 9.5 Recording lifecycle integration tests - Test the full start/stop/status round trip against a real filesystem (through the request handler from 9.4, not by calling the internal functions directly), including the path-validation rejection cases and a simulated disk-full condition - Observable: the integration test suite passes, including assertions on the exact error returned for each rejection case - _Requirements: 5.10, 5.11_ - _Depends: 9.4_ + - _Verified: 2026-08-05 — audited task 9.4's existing `test_admin_request.c` (already 14 tests driving everything through the real `scheduler_req_handler()`) against task 9.1's exhaustive path-validation suite and task 9.3's write-failure technique to find genuine gaps rather than duplicating coverage: only the bare path-separator rejection had been proven through the admin/handler path, and the disk-full scenario had never been proven with both the initiating `recordStart` and the verifying `recordStatus` driven through real admin requests. Added 3 tests (903→1163 lines): a `..`-traversal rejection (distinct `strstr("..")` branch) and a real-`symlink(2)`-escape rejection, both submitted via genuine `SchedAdminRequest`s through `scheduler_req_handler()` and asserted via the same reply-spy the rest of the file uses; and a write-failure test using task 9.3's exact `fflush()`+`setvbuf(_IONBF)`+`close(fd)` technique to force a genuine EIO, confirming `sched_record_abort_on_error()`'s cleanup is visible through a subsequent real `recordStatus` admin request and that a further `recordStart` afterward succeeds cleanly — the specific thing beyond task 9.3's internal-level proof. Hard-link-aliasing coverage through the admin path deliberately omitted as reasonable scope: task 9.1's 3-round adversarial hunt and task 9.2's direct-call TOCTOU test already exhaustively prove that specific rejection branch, and the routing/plumbing this task exists to prove is structurally identical regardless of which validation branch fires — already retired by the dotdot/symlink cases. Review independently verified every claim against source (confirmed distinct code branches, confirmed a real `rc=-5` EIO in test output, confirmed the write-failure test's single-threaded sequence never touches `record_lock` directly and cannot deadlock/race since all mutation flows through already-locked library calls) and confirmed the `record_start_ts`-unchanged-on-abort behavior matches `sched_record_stop()`'s pre-existing identical semantics and requirements.md's own reply-field table ("original start (historical)") — not a bug. One minor non-blocking note: the symlink-escape test verifies "untouched" via `stat()`+`size==0` rather than this suite's stronger write-content-then-readback pattern; logically sufficient, flagged for awareness only. 17/17 in `sched_admin_request_test`, 26/26 scheduler suite, clang-format clean — independently re-verified by me via a fresh `meson compile`/`meson test --suite scheduler` run before marking complete. Closes out Group 9 (Core: Recording Control) entirely — tasks 9.1-9.5 all complete._ - [ ] 10. Integration: Controller Admin Opcode and xceadmin CLI diff --git a/src/scheduler/testsuite/test_admin_request.c b/src/scheduler/testsuite/test_admin_request.c index e258b7ff..7ecaf193 100644 --- a/src/scheduler/testsuite/test_admin_request.c +++ b/src/scheduler/testsuite/test_admin_request.c @@ -28,6 +28,23 @@ * - An invalid recordFile (path separator) surfaces all the way through * the deferred path as {ok=false, error="invalid record file name"}, * never just an internal log line, and never actually opens a file. + * - The same rejection holds for a ".."-traversal recordFile and for a + * pre-planted symlink inside record_dir that escapes outside it -- + * both submitted through this same real admin request path (task + * 9.1's sched_validate_record_filename() contract), not just proven at + * the internal validation-function level + * (test_recording_path_validation.c) or the single path-separator case + * above (task 9.5's genuine-gap audit). + * - A genuine write failure (the underlying fd closed out from under the + * open sched->record_file, task 9.3's own real-I/O technique) that + * occurs while recording -- itself started through this same real + * admin request path -- cleanly disables recording via + * sched_record_abort_on_error(); a subsequent recordStatus through + * this SAME real handler entry point correctly reports + * recording=false; and a further admin request afterward still + * succeeds cleanly (task 9.5's disk-full Observable, proven through + * the request-handler path specifically, not just the internal + * driver-loop path task 9.3 already proved). * - An unrecognized SchedAdminAction returns -EINVAL, logs, and neither * enqueues anything nor sends any reply -- no crash. * @@ -68,6 +85,7 @@ * would be a stack-use-after-return read the very next line. */ #include +#include #include #include #include @@ -304,6 +322,17 @@ static void make_tmp_dir(char* out, size_t out_len, const char* suffix) strcpy(out, tmpl); } +/* Same helper as test_recording_path_validation.c's own touch_file() -- + * creates an empty regular file, used below to build a genuine symlink + * target outside record_dir for the symlink-escape-through-the-admin-path + * test. */ +static void touch_file(const char* path) +{ + int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0644); + TEST_ASSERT_TRUE(fd >= 0); + close(fd); +} + /* `record_dir` must outlive `sched` (caller-owned storage), same convention * as test_record_lifecycle.c's fixture_init(). No picker plugin is * registered -- unlike test_record_lifecycle.c's fixture, no test in this @@ -718,6 +747,127 @@ static void test_recordstart_invalid_filename_replies_error(void) TEST_ASSERT_EQUAL_UINT64(0, g_last_started_at); } +/* ---------------------------------------------------------------------- */ +/* More recordStart path-validation rejection cases through the SAME real */ +/* admin request path (task 9.5's genuine-gap audit): the test above only */ +/* covers the path-separator case. sched_validate_record_filename() */ +/* (recording.c, task 9.1) also rejects a ".."-traversal filename and a */ +/* pre-planted symlink-escape -- test_recording_path_validation.c already */ +/* proves both at the internal function level; these two prove the SAME */ +/* rejections surface correctly once the deferred recordStart is actually */ +/* attempted by xce_sched_driver_translate_pending_events(), through the */ +/* real scheduler_req_handler() entry point, exactly like the */ +/* path-separator test above. */ +/* ---------------------------------------------------------------------- */ + +static void test_recordstart_dotdot_filename_replies_error(void) +{ + char record_dir[PATH_MAX]; + xce_scheduler_t sched; + int rc; + + make_tmp_dir(record_dir, sizeof(record_dir), "_start_dotdot"); + fixture_init(&sched, record_dir); + + /* Contains ".." -- rejected before any filesystem access, same + * short-circuiting string check as the path-separator case above but a + * genuinely DIFFERENT branch inside sched_validate_record_filename() + * (strstr(filename, "..") rather than strchr(filename, '/')). */ + rc = send_admin_request(&sched, SchedAdminAction_recordStart, + "../evil.replay", 1111); + TEST_ASSERT_EQUAL_INT(0, rc); + + rc = xce_sched_driver_translate_pending_events(&(sched.core), &sched); + TEST_ASSERT_EQUAL_INT(0, rc); + + TEST_ASSERT_FALSE(sched.recording); + TEST_ASSERT_NULL(sched.record_file_path); + TEST_ASSERT_NULL(sched.record_file); + + TEST_ASSERT_EQUAL_INT(1, g_reply_calls); + TEST_ASSERT_FALSE(g_last_ok); + TEST_ASSERT_FALSE(g_last_error_null); + TEST_ASSERT_EQUAL_STRING("invalid record file name", g_last_error); + TEST_ASSERT_FALSE(g_last_recording); + TEST_ASSERT_TRUE(g_last_file_null); + TEST_ASSERT_EQUAL_UINT64(0, g_last_event_count); + TEST_ASSERT_EQUAL_UINT64(0, g_last_started_at); + + /* The driver loop keeps working after the rejection: a subsequent + * recordStatus request through the SAME real handler entry point + * (synchronous, not deferred) succeeds cleanly, proving the rejected + * recordStart left no dangling state that would wedge later requests. */ + rc = send_admin_request(&sched, SchedAdminAction_recordStatus, NULL, 1112); + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_INT(2, g_reply_calls); + TEST_ASSERT_TRUE(g_last_ok); + TEST_ASSERT_FALSE(g_last_recording); +} + +static void test_recordstart_symlink_escape_replies_error(void) +{ + char record_dir[PATH_MAX]; + char outside_dir[PATH_MAX]; + char outside_target[PATH_MAX]; + char link_path[PATH_MAX]; + xce_scheduler_t sched; + struct stat st; + int rc; + + make_tmp_dir(record_dir, sizeof(record_dir), "_start_symlink"); + make_tmp_dir(outside_dir, sizeof(outside_dir), "_start_symlink_dst"); + fixture_init(&sched, record_dir); + + /* Same symlink-escape shape as test_recording_path_validation.c's + * test_symlink_inside_record_dir_escaping_outside_is_rejected: a + * pre-existing symlink INSIDE record_dir, named exactly like an + * innocent-looking filename, whose target lives OUTSIDE record_dir. + * The filename component alone ("looks_safe") violates neither the + * '/' nor the ".." rule -- only the canonical-path containment check + * on the pre-existing symlink's resolved target catches this. Proven + * here through the real admin request path, not just at the internal + * sched_validate_record_filename() level. */ + snprintf(outside_target, sizeof(outside_target), "%s/secret", outside_dir); + touch_file(outside_target); + + snprintf(link_path, sizeof(link_path), "%s/looks_safe", record_dir); + TEST_ASSERT_EQUAL_INT(0, symlink(outside_target, link_path)); + + rc = send_admin_request(&sched, SchedAdminAction_recordStart, "looks_safe", + 2222); + TEST_ASSERT_EQUAL_INT(0, rc); + + rc = xce_sched_driver_translate_pending_events(&(sched.core), &sched); + TEST_ASSERT_EQUAL_INT(0, rc); + + TEST_ASSERT_FALSE(sched.recording); + TEST_ASSERT_NULL(sched.record_file_path); + TEST_ASSERT_NULL(sched.record_file); + + TEST_ASSERT_EQUAL_INT(1, g_reply_calls); + TEST_ASSERT_FALSE(g_last_ok); + TEST_ASSERT_FALSE(g_last_error_null); + TEST_ASSERT_EQUAL_STRING("invalid record file name", g_last_error); + TEST_ASSERT_FALSE(g_last_recording); + TEST_ASSERT_TRUE(g_last_file_null); + TEST_ASSERT_EQUAL_UINT64(0, g_last_event_count); + TEST_ASSERT_EQUAL_UINT64(0, g_last_started_at); + + /* The escaped-to outside file was never opened, let alone truncated -- + * proves the rejection happened before any fopen()/ftruncate() on the + * resolved-outside path. */ + TEST_ASSERT_EQUAL_INT(0, stat(outside_target, &st)); + TEST_ASSERT_EQUAL_INT(0, (int)st.st_size); + + /* Driver loop keeps working after the rejection, same proof as the + * ".."-traversal case above. */ + rc = send_admin_request(&sched, SchedAdminAction_recordStatus, NULL, 2223); + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_INT(2, g_reply_calls); + TEST_ASSERT_TRUE(g_last_ok); + TEST_ASSERT_FALSE(g_last_recording); +} + /* ---------------------------------------------------------------------- */ /* recordStop -- deferred, not synchronous. */ /* ---------------------------------------------------------------------- */ @@ -802,6 +952,112 @@ static void test_recordstop_while_not_recording_replies_error(void) TEST_ASSERT_EQUAL_UINT64(0, g_last_started_at); } +/* ---------------------------------------------------------------------- */ +/* Disk-full simulation through the admin/request-handler path (task 9.5). */ +/* Mirrors test_record_lifecycle.c's */ +/* test_write_failure_disables_recording_and_driver_keeps_running() -- */ +/* same real fflush()+setvbuf(_IONBF)+close(fd) write-failure technique, */ +/* task 9.3's own Observable -- but with BOTH the initiating recordStart */ +/* AND the verifying recordStatus driven through scheduler_req_handler(), */ +/* the real production request-handler entry point, rather than task */ +/* 9.3's direct internal sched_record_start()/sched_record_status() calls. */ +/* ---------------------------------------------------------------------- */ + +static void +test_write_failure_through_admin_path_disables_recording_cleanly(void) +{ + char record_dir[PATH_MAX]; + xce_scheduler_t sched; + xce_core_event_t ev; + int rc; + int fd; + int calls_before; + uint64_t original_started_at; + + make_tmp_dir(record_dir, sizeof(record_dir), "_diskfull"); + fixture_init(&sched, record_dir); + + /* 1. recordStart, genuinely through the admin request path: deferred, + * then performed by a real driver round -- same pattern as + * test_recordstart_is_deferred_then_performed_by_driver_round above. */ + rc = send_admin_request(&sched, SchedAdminAction_recordStart, + "diskfull.replay", 3001); + TEST_ASSERT_EQUAL_INT(0, rc); + + rc = xce_sched_driver_translate_pending_events(&(sched.core), &sched); + TEST_ASSERT_EQUAL_INT(0, rc); + + TEST_ASSERT_TRUE(sched.recording); + TEST_ASSERT_NOT_NULL(sched.record_file); + TEST_ASSERT_EQUAL_INT(1, g_reply_calls); + TEST_ASSERT_TRUE(g_last_ok); + /* sched_record_abort_on_error() (recording.c) deliberately does NOT + * reset record_start_ts -- only sched_record_start() ever (re)stamps + * it, on the NEXT successful start (recording.c's own field-write + * list). So a post-abort recordStatus must still report THIS original + * started-at, not 0 -- captured here for that later assertion. */ + original_started_at = g_last_started_at; + + /* 2. Force a genuine write failure on the now-open sched->record_file -- + * the same real technique test_record_lifecycle.c uses: flush what + * recordStart already buffered, force the stream unbuffered, then close + * the underlying fd out from under the FILE* so the very next fwrite() + * hits a genuine EBADF, not a simulated return code. */ + TEST_ASSERT_EQUAL_INT(0, fflush(sched.record_file)); + TEST_ASSERT_EQUAL_INT(0, setvbuf(sched.record_file, NULL, _IONBF, 0)); + fd = fileno(sched.record_file); + TEST_ASSERT_TRUE(fd >= 0); + TEST_ASSERT_EQUAL_INT(0, close(fd)); + + /* 3. Drive the failing write via one real event batch -- the same + * sched_record_write_events() call + * xce_sched_driver_translate_pending_events() itself invokes for a + * translated batch (this file's own + * test_recordstatus_while_recording_reflects_live_state above already + * establishes this direct call as this suite's real event-batch + * trigger). */ + memset(&ev, 0, sizeof(ev)); + ev.type = XCE_CORE_EVENT_JOB_STATE_CHANGE; + ev.u.job_state_change.jobid = 42ULL; + TEST_ASSERT_TRUE(sched_record_write_events(&sched, &ev, 1) < 0); + + /* (a) sched_record_abort_on_error()'s cleanup genuinely fired. */ + TEST_ASSERT_FALSE(sched.recording); + TEST_ASSERT_NULL(sched.record_file); + TEST_ASSERT_NULL(sched.record_file_path); + + /* (b) A subsequent recordStatus request through the SAME real handler + * entry point correctly reports the abort. */ + calls_before = g_reply_calls; + rc = send_admin_request(&sched, SchedAdminAction_recordStatus, NULL, 3002); + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_INT(calls_before + 1, g_reply_calls); + TEST_ASSERT_EQUAL_UINT64(3002, g_last_seq); + TEST_ASSERT_TRUE(g_last_ok); + TEST_ASSERT_FALSE(g_last_recording); + TEST_ASSERT_TRUE(g_last_file_null); + TEST_ASSERT_EQUAL_UINT64(0, g_last_event_count); + TEST_ASSERT_EQUAL_UINT64(original_started_at, g_last_started_at); + + /* (c) The scheduler keeps processing normally afterward: a further + * admin request -- a brand new recordStart -- succeeds cleanly through + * the SAME real handler + driver-round path, genuinely creating a new + * file on disk. */ + calls_before = g_reply_calls; + rc = send_admin_request(&sched, SchedAdminAction_recordStart, + "recovery.replay", 3003); + TEST_ASSERT_EQUAL_INT(0, rc); + + rc = xce_sched_driver_translate_pending_events(&(sched.core), &sched); + TEST_ASSERT_EQUAL_INT(0, rc); + + TEST_ASSERT_EQUAL_INT(calls_before + 1, g_reply_calls); + TEST_ASSERT_TRUE(g_last_ok); + TEST_ASSERT_TRUE(sched.recording); + TEST_ASSERT_NOT_NULL(sched.record_file_path); + TEST_ASSERT_TRUE(strstr(sched.record_file_path, "recovery.replay") != NULL); +} + /* ---------------------------------------------------------------------- */ /* Unrecognized action / NULL argument guards. */ /* ---------------------------------------------------------------------- */ @@ -890,10 +1146,14 @@ int main(void) RUN_TEST(test_recordstart_empty_file_auto_generates_name); RUN_TEST(test_recordstart_while_already_recording_replies_error); RUN_TEST(test_recordstart_invalid_filename_replies_error); + RUN_TEST(test_recordstart_dotdot_filename_replies_error); + RUN_TEST(test_recordstart_symlink_escape_replies_error); RUN_TEST(test_recordstop_is_deferred_then_performed_by_driver_round); RUN_TEST(test_recordstop_while_not_recording_replies_error); + RUN_TEST(test_write_failure_through_admin_path_disables_recording_cleanly); + RUN_TEST(test_unrecognized_action_returns_einval_no_crash_no_reply); RUN_TEST(test_null_sched_returns_einval); RUN_TEST(test_null_request_returns_einval);