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

@@ -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
 

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 <errno.h>
+#include <fcntl.h>
 #include <limits.h>
 #include <stdint.h>
 #include <stdio.h>
@@ -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);