diff --git a/.kiro/specs/scheduler-core/tasks.md b/.kiro/specs/scheduler-core/tasks.md index 1914c24e..3c1d8cb2 100644 --- a/.kiro/specs/scheduler-core/tasks.md +++ b/.kiro/specs/scheduler-core/tasks.md @@ -234,12 +234,13 @@ - _Depends: 9.1_ - _Verified: 2026-08-05 — added recording-state fields directly to xce_scheduler_t (no new xce_sched_driver_t type, matching task 9.1's established deviation from design.md's sketch). sched_record_start()/sched_record_stop() (recording.c) implement AC 10 steps 1 and 7 exactly: validate via sched_validate_record_filename() (task 9.1), open with the TOCTOU-hardened sequence (O_NOFOLLOW, no O_TRUNC, fstat()+st_nlink==1 check on the opened fd, ftruncate() only after the check passes — matching recording.h's task-9.1-era note to this task), xce_sched_core_save_snapshot() → write ReplayHeader then SnapshotEntry, set recording=true; stop writes ReplayEvent{body=endSession}, flushes, closes, clears state. driver.c gains a record_ctx void* param on xce_sched_driver_translate_pending_events() (deliberately NOT read from core->driver_ctx, which isn't guaranteed xce_scheduler_t* in test fixtures) for per-event-batch ReplayEvent writes (new recording_events.c), plus sched_record_session_begin()/_add_decision()/_end() hooks in xce_sched_driver_run_normal_mode_round() for per-session ReplaySession writes (new recording_session.c) — both boolean-gated on sched->recording, zero I/O/allocation when inactive, verified by tracing both call sites. recording.c split three ways (recording.c=543, recording_events.c=177, recording_session.c=244 lines) to stay under the 800-line cap, mirroring snapshot.c's precedent. Two real bugs were found and fixed during implementation, independently confirmed by review: bug-1498 (an initial O_TRUNC-in-open() would have truncated a hard-linked victim file before the st_nlink check could reject it — fixed by deferring truncation to a post-check ftruncate()) and bug-1499 (an edit transiently dropped xce_core_signal_go(), a 3-way picker/allocator/driver deadlock — restored, caught by test_driver_session_ordering.c). New test_record_lifecycle.c (826 lines, in line with this suite's established over-800 precedent for test files) decodes the full start→events→session→stop cycle with the real generated capnp decoders and asserts the exact 5-message sequence plus no 6th message; two TOCTOU regression tests use real symlink(2)/link(2) syscalls against a real victim file and assert its content is untouched after rejection; double-start/stop-when-not-recording both return -EALREADY cleanly. Review found one non-blocking follow-up (open() lacks O_NONBLOCK, so a FIFO planted in the same TOCTOU window would hang the calling thread — DoS, not corruption; logged below, not fixed in this task) and confirmed zero scope leakage into tasks 9.3/9.4/group 10. 25/25 scheduler suite, clang-format clean — independently re-verified by me via a fresh `meson compile`/`meson test --suite scheduler` run before marking complete. Note: two implementer-agent runs ended up editing this worktree concurrently after a spurious "agent died" signal turned out to be wrong; a stabilization wait (repeated md5sum) plus two independent fresh-build verifications (mine and a dedicated fix/verify agent's) confirmed the settled state was consistent before review._ -- [ ] 9.3 Implement periodic snapshot checkpointing and recording error handling +- [x] 9.3 Implement periodic snapshot checkpointing and recording error handling - Append a fresh snapshot entry to the record file every configured number of recorded events (0 disables this) - On a write failure mid-recording (disk full, permission error): log the error, cleanly tear down recording state, and continue normal scheduling — recording failure must never stop or crash the scheduler - Observable: recording through a configured interval produces the expected number of extra snapshot entries in the file; simulating a write failure leaves the scheduler still processing sessions afterward with recording cleanly disabled - _Requirements: 5.11_ - _Depends: 9.2_ + - _Verified: 2026-08-05 — the write-failure error-handling half was already built in task 9.2 (`sched_record_abort_on_error()`, matching AC 10's clause verbatim: log, fflush+fclose, free+NULL path, recording=false); this task added the missing end-to-end test (`test_write_failure_disables_recording_and_driver_keeps_running`) using a genuine closed-fd write failure (fflush + setvbuf(_IONBF) + close(fd), confirmed by review running the binary directly and observing a real `rc=-5` abort log line, then a real subsequent session completing) rather than a mocked return code. Periodic checkpointing (the new work) hooks into `sched_record_write_events()`: once per call, `record_snapshot_interval > 0 && record_event_count % record_snapshot_interval == 0` triggers a fresh `SnapshotEntry` via the same `write_snapshot_entry()` helper `sched_record_start()` already uses (made non-static, zero duplication) — safe from task 8.2's `session_active` precondition because this call site is only ever reached from `xce_sched_driver_translate_pending_events()`, confirmed the sole production caller and confirmed to always run strictly before `xce_core_begin_session()`. `record_snapshot_interval == 0` genuinely produces zero checkpoints. The "once per call, not once per crossed boundary" semantics matches requirements.md's own embedded pseudocode verbatim, not an implementer shortcut. 13/13 new test binary, 25/25 scheduler suite, clang-format clean — independently re-verified by me via a fresh `meson compile`/`meson test --suite scheduler` run before marking complete. Two follow-ups flagged by review, logged below rather than folded in: `--record-snapshot-interval` has no CLI wiring anywhere (AC 11 cannot be configured non-zero in production as currently scoped — this task's own Observable is satisfied via the struct field directly, which is sufficient for this task, but the operator-facing gap remains); `test_record_lifecycle.c` grew to 1232 lines (826→1232), past this suite's prior precedent, with a natural AC10/AC11 split point identified but not acted on._ - [ ] 9.4 Implement the scheduler-side admin request handler - Decode a forwarded `SchedAdminRequest` arriving on the scheduler's controller-facing endpoint, dispatch by action to the record-start/stop/status functions built in 9.2/9.3, and encode+send the `SchedAdminReply` back to the controller on the same endpoint — this is the scheduler-side counterpart to the controller's forwarding dispatch (task 10.1), without which 10.1 has nothing to forward to @@ -384,4 +385,6 @@ - Same pattern compounded across tasks 4.4 (`core_session.c`) and 4.5 (`core_clock.c`, which also genuinely NEEDS to be outside the wall-clock-call scan since it's the one file allowed to call `clock_gettime`) — `test_core_isolation.c` now covers only `core.c`, while three sibling files (`core_events.c`/`core_session.c`/`core_clock.c`) sit outside its mechanical audit, each manually verified clean but not enforced by a test. Worth a dedicated follow-up task extending the forbidden-header/forbidden-symbol half of the audit (not the wall-clock half, which `core_clock.c` must stay exempt from) to all `core_*.c` files before Phase 3 adds more. - Task 5.2's review surfaced a genuine, pre-existing bug in `plugin.c`'s `plugin_end_session()` (predates this entire spec — `git blame` traces it to commit `638173dec`, 2026-02-26; untouched by any scheduler-core task's diff): the per-plugin guard checks `plugin->begin_session_fn != NULL` but then calls `plugin->end_session_fn(plugin)` — a guard/call mismatch. Confirmed live impact: `plugins/fcfs.c` registers `.begin_session_fn = NULL`/`.end_session_fn = fcfs_end_session`, so `fcfs_end_session()` (resets the legacy `job_elect_fn`-style iteration cursor) is never invoked today. Only matters if a deployment's `job_pick_plugin` config selects the legacy `xce_sched_fcfs` name rather than the new default `builtin_fcfs` (both fields NULL on `builtin_fcfs`, unaffected). Out of scope to fix here — needs its own bug-log entry / small fix task, not folded into scheduler-core. - Task 9.2's review found a non-blocking follow-up, not fixed in that task: `sched_record_start()`'s TOCTOU-hardened `open()` (`recording.c`, `O_WRONLY|O_CREAT|O_NOFOLLOW`) lacks `O_NONBLOCK`. A FIFO planted at the resolved path in the same race window the symlink/hard-link defenses already close (between `sched_validate_record_filename()` returning and `open()` running) would hang the calling thread indefinitely — a DoS (thread hang), not data corruption, since the existing `st_nlink`/`S_ISREG` checks still reject it once `open()` eventually returns. Adding `O_NONBLOCK` to the open flags is zero-risk for the regular-file case and closes this; worth a small follow-up before task 9.2's code sees production traffic. +- Task 9.3's review found a genuine task-breakdown gap, not fixable within 9.3's own boundary: `record_snapshot_interval` (the field task 9.3's periodic-checkpoint mechanism reads) has no CLI or admin-request wiring anywhere in the codebase — `--record-snapshot-interval` appears in AC 11's own normative text but in zero task's actual scope. Neither task 9.4 (admin request handler — dispatches `SchedAdminRequest` actions, which don't carry an interval field either, per controller.capnp's `SchedAdminRequest{action, recordFile}`) nor group 10 (controller-side dispatch) owns scheduler startup CLI flags; only `main.c`'s argp table does, following task 9.1's own `--record-dir` precedent. As currently scoped, AC 11's mechanism is fully implemented and tested (via the struct field directly) but has no path to ever be configured non-zero in a real deployment. Needs a small follow-up task before this spec is considered complete: add a `--record-snapshot-interval N` CLI flag to `main.c`, wired to `sched.record_snapshot_interval`, mirroring `--record-dir`'s exact pattern (task 9.1). +- Task 9.3's review also flagged `test_record_lifecycle.c`'s size (826→1232 lines across tasks 9.2+9.3) as past this suite's established over-800 precedent (prior max `test_driver_translate.c`≈867) — the content itself is genuine, non-duplicative coverage (not padding), and a natural split point exists along the AC10 (lifecycle/TOCTOU)/AC11 (periodic-checkpoint/error-handling) seam by extracting shared fixture helpers into a common header. Non-blocking; worth a future cleanup pass, not urgent. - Cross-task interaction, flagged before task 9.4 exists — resolution decided (not a fail/retry, a deferral): task 9.2's `sched_record_start()` calls `xce_sched_core_save_snapshot()` as its first action (AC 10 step 1) — the same function task 8.2 gated to return `-EBUSY` whenever `session_active != 0`. `main.c`'s driver loop is single-threaded (`while(1) { xce_sched_driver_run_normal_mode_round(&sched); ...; sleep(1); }`); incoming requests (including the future `SchedAdminRequest`) arrive asynchronously via `scheduler_req_handler` (handler.c) on the I/O side, same as job/queue/worker updates. **Decision**: task 9.4 must NOT call `sched_record_start()` directly from the request-handling thread (that both risks racing `session_active` and violates this codebase's async-I/O rule against blocking/file-I/O in a request-handler callback). Instead, `recordStart` must be enqueued the same way job/queue/worker updates already are (`pend_updates` dataq or an equivalent dedicated pending-admin-request slot) and picked up by `xce_sched_driver_translate_pending_events()` — which already runs at the top of every round, strictly before `begin_session()`, so it is guaranteed between-sessions by the same construction argument design.md uses for `core_push_events()`. This means recording "takes effect" at the next scheduling round, not synchronously on request arrival. Consequence for task 9.4: since AC 10 specifies a synchronous request-reply carrying the resolved filename, the actual `SchedAdminReply` send must be deferred until the driver thread performs the queued start (not sent eagerly from the handler thread) — needs a completion signal back to the waiting request context (e.g. a condition variable, or replying once `xce_sched_driver_translate_pending_events()` reports the deferred start applied). Given this deferral makes the direct `-EBUSY` race effectively unreachable from the admin path, the AC 10 error-codes table doesn't need a new "busy" entry for this case — `sched_record_start()` keeps its `-EBUSY` return as a defensive/fail-loud contract check (task 9.2's job, no torn state), it just should no longer be a path task 9.4's dispatch can actually trigger by construction. diff --git a/src/scheduler/include/recording.h b/src/scheduler/include/recording.h index e5b66b09..68abe15d 100644 --- a/src/scheduler/include/recording.h +++ b/src/scheduler/include/recording.h @@ -211,6 +211,20 @@ extern int sched_record_stop(xce_scheduler_t* sched); * marker -- the file is presumably broken already) and a negative errno is * returned; the caller (driver.c) does not need to react further -- normal * scheduling continues either way (Req 5 AC 10, "Error handling"). + * + * Periodic checkpoint (Req 5 AC 11, task 9.3): after the whole batch's + * events are written, if `sched->record_snapshot_interval > 0` and the + * (cumulative, since recording started) `sched->record_event_count` has + * just landed exactly on a multiple of it, a fresh `SnapshotEntry` message + * is appended (via recording.c's shared write_snapshot_entry(), the SAME + * builder sched_record_start() uses for the initial one -- DRY, never + * duplicated). `record_snapshot_interval == 0` disables this entirely. + * Checked ONCE per call against the post-batch count, not once per + * multiple crossed within one large batch -- see recording_events.c's own + * comment on sched_record_write_events() for the full boundary-semantics + * rationale. A checkpoint write failure disables recording via + * sched_record_abort_on_error(), the exact same path as any other + * recording write failure -- never special-cased. */ extern int sched_record_write_events(xce_scheduler_t* sched, xce_core_event_t* events, int n); diff --git a/src/scheduler/include/scheduler.h b/src/scheduler/include/scheduler.h index 099c0e00..0488c2a1 100644 --- a/src/scheduler/include/scheduler.h +++ b/src/scheduler/include/scheduler.h @@ -145,8 +145,11 @@ typedef struct { * by task 9.3's periodic-checkpoint trigger; incremented by * recording.c's event-batch writer, task 9.2). */ uint64_t record_event_count; - /* N events between periodic snapshot checkpoints (0 = disabled). - * Reserved for task 9.3 -- inert (never read) as of task 9.2. */ + /* N events between periodic snapshot checkpoints (0 = disabled). Read + * by recording_events.c's sched_record_write_events() (task 9.3): + * every time cumulative `record_event_count` lands exactly on a + * multiple of this value, a fresh SnapshotEntry is appended to the + * record file (Req 5 AC 11). */ uint64_t record_snapshot_interval; /* logical time (xce_core_now()) recording started; 0 = not recording. */ uint64_t record_start_ts; diff --git a/src/scheduler/src/recording.c b/src/scheduler/src/recording.c index e588a06c..d5ac8c67 100644 --- a/src/scheduler/src/recording.c +++ b/src/scheduler/src/recording.c @@ -14,16 +14,22 @@ #include "utils.h" /* - * recording.c - scheduler-core spec, tasks 9.1/9.2. See recording.h for the - * full per-function contract/rationale. Task 9.1 added ONLY - * sched_validate_record_filename() below. Task 9.2 (this revision) adds - * the record start/stop lifecycle (sched_record_start()/sched_record_stop()) - * plus the shared message-write/error-abort helpers + * recording.c - scheduler-core spec, tasks 9.1/9.2/9.3. See recording.h for + * the full per-function contract/rationale. Task 9.1 added ONLY + * sched_validate_record_filename() below. Task 9.2 added the record + * start/stop lifecycle (sched_record_start()/sched_record_stop()) plus the + * shared message-write/error-abort helpers * (write_capnp_message()/sched_record_abort_on_error()) two sibling files * -- recording_events.c (the per-event-batch recording-write hook) and * recording_session.c (the per-session ReplaySession accumulator) -- also - * use. Split across three files (this project's 800-line-per-file - * convention, CLAUDE.md/coding-style.md) the same way task 8.1's + * use. Task 9.3 (this revision) additionally shares write_snapshot_entry() + * below the same way: recording_events.c's sched_record_write_events() now + * calls it (via a plain `extern`, same idiom) to append a periodic + * checkpoint SnapshotEntry once `sched->record_snapshot_interval` events + * have been recorded (Req 5 AC 11) -- see recording_events.c for the + * boundary-crossing semantics and recording.h for the full contract. Split + * across three files (this project's 800-line-per-file convention, + * CLAUDE.md/coding-style.md) the same way task 8.1's * snapshot.c/snapshot_wire.c/snapshot_load.c split: shared, narrow helpers * are non-`static` here and redeclared via a plain `extern` at each * consumer's own use site, matching snapshot_wire.c's own documented idiom @@ -313,10 +319,30 @@ static int write_replay_header(FILE* f, xce_scheduler_t* sched, } /* - * write_snapshot_entry - builds and writes the initial SnapshotEntry - * message (Req 5 AC 20's msg 2, "MUST be the first data message after - * ReplayHeader"). Takes a full core snapshot via - * xce_sched_core_save_snapshot() (core.h, task 8.1) and writes it. + * write_snapshot_entry - builds and writes a SnapshotEntry message. Used + * for two distinct call sites, both writing the exact same message shape + * (Req 5 AC 20's msg 2 / Req 5 AC 11): + * + * 1. sched_record_start() (this file, task 9.2) -- the initial + * SnapshotEntry, "MUST be the first data message after ReplayHeader". + * 2. sched_record_write_events() (recording_events.c, task 9.3) -- a + * periodic checkpoint SnapshotEntry, appended every + * `sched->record_snapshot_interval` recorded events. Non-`static` and + * redeclared via `extern` there, same shared-helper idiom this file's + * own header comment documents for write_capnp_message()/ + * sched_record_abort_on_error() -- DRY: the capnp-building logic is + * never duplicated. + * + * Both call sites take a full core snapshot via + * xce_sched_core_save_snapshot() (core.h, task 8.1) and write it. That + * function itself enforces the `core->session_active == 0` precondition + * (returns -EBUSY otherwise, core.h's own doc comment) -- both call sites + * are safe by construction: sched_record_start() runs before any session + * begins, and sched_record_write_events() is only ever invoked from + * xce_sched_driver_translate_pending_events() (driver.c), which itself only + * ever runs strictly between sessions (driver.c's own doc comment on + * xce_sched_driver_run_normal_mode_round()), so `session_active` is always 0 + * at both call sites in production. * * `snap`'s contents are deliberately never freed here: snapshot.c's own * "Ownership notes" (file header comment) document that `config` and each @@ -328,7 +354,7 @@ static int write_replay_header(FILE* f, xce_scheduler_t* sched, * save_snapshot() result unfreed for the same reason, detect_leaks=0 * across this whole suite) -- writing one is out of this task's boundary. */ -static int write_snapshot_entry(FILE* f, xce_sched_core_t* core, uint64_t ts) +int write_snapshot_entry(FILE* f, xce_sched_core_t* core, uint64_t ts) { struct capn c; struct capn_segment* cs; diff --git a/src/scheduler/src/recording_events.c b/src/scheduler/src/recording_events.c index bd72b697..22a1e6dd 100644 --- a/src/scheduler/src/recording_events.c +++ b/src/scheduler/src/recording_events.c @@ -8,23 +8,28 @@ #include "utils.h" /* - * recording_events.c - scheduler-core spec, task 9.2 (Req 5 AC 10, "On each - * core_push_events() call ... For each event in the batch, serialize and - * write the corresponding ReplayEvent message"). Split out of recording.c - * (this project's 800-line-per-file convention, CLAUDE.md/coding-style.md) - * -- see recording.c's own file header comment, "Split across three - * files", for the full rationale. This file owns the per-event-batch - * recording-write hook driver.c's xce_sched_driver_translate_pending_events() - * calls into. + * recording_events.c - scheduler-core spec, tasks 9.2/9.3 (Req 5 AC 10, "On + * each core_push_events() call ... For each event in the batch, serialize + * and write the corresponding ReplayEvent message"; Req 5 AC 11, "the + * driver shall periodically append fresh snapshots to the replay file -- + * default: every N events"). Split out of recording.c (this project's + * 800-line-per-file convention, CLAUDE.md/coding-style.md) -- see + * recording.c's own file header comment, "Split across three files", for + * the full rationale. This file owns the per-event-batch recording-write + * hook driver.c's xce_sched_driver_translate_pending_events() calls into, + * plus (task 9.3) the periodic-checkpoint trigger layered onto that same + * hook. * - * write_capnp_message()/sched_record_abort_on_error() are defined - * (non-`static`) in recording.c and redeclared here via a plain `extern`, - * matching this codebase's existing cross-.c-file idiom for a narrow - * shared dependency (snapshot_wire.c's own documented precedent, itself - * citing core_session.c's `extern void sched_worker_release_host_ctx(...)`). + * write_capnp_message()/sched_record_abort_on_error()/write_snapshot_entry() + * are defined (non-`static`) in recording.c and redeclared here via a + * plain `extern`, matching this codebase's existing cross-.c-file idiom for + * a narrow shared dependency (snapshot_wire.c's own documented precedent, + * itself citing core_session.c's `extern void + * sched_worker_release_host_ctx(...)`). */ extern int write_capnp_message(FILE* f, struct capn* c); extern void sched_record_abort_on_error(xce_scheduler_t* sched, int rc); +extern int write_snapshot_entry(FILE* f, xce_sched_core_t* core, uint64_t ts); /* * write_one_replay_event - builds and writes one ReplayEvent message from @@ -152,6 +157,38 @@ static int write_one_replay_event(FILE* f, xce_core_event_t* ev, return rc; } +/* + * sched_record_write_events - see recording.h's declaration comment for the + * full contract. This function's tail implements Req 5 AC 11's periodic + * checkpoint: task 9.3. + * + * Boundary semantics ("every N events", documented choice): after writing + * this call's whole batch, the check is `record_snapshot_interval > 0 && + * record_event_count > 0 && record_event_count % record_snapshot_interval + * == 0` against the POST-batch, cumulative `record_event_count` -- i.e. + * "has the running total, since recording started, just landed exactly on + * a multiple of N" -- evaluated ONCE PER CALL, not once per boundary + * crossed within the call. Matches requirements.md's own annotated + * pseudocode for this step verbatim (a single `if`, not a loop over + * crossed multiples). Consequence, deliberate and documented: if a single + * batch is large enough to cross more than one multiple of N (e.g. N=3 and + * one call's batch pushes record_event_count from 0 straight to 6), only + * ONE checkpoint is written for that call, not one per crossed boundary -- + * periodic checkpointing is a diagnostic/replay-optimization aid, not a + * strict per-event guarantee, so this is a proportionate simplification + * (same discipline this file's sibling driver.c documents for its own + * batch-level simplifications), not a correctness gap: the checkpoint + * cadence measured in wall-clock/event-arrival terms is unaffected for the + * common case of one-or-few-events-per-call this driver's normal-mode loop + * actually produces, and simulation replay never depends on periodic + * checkpoints existing at every possible offset -- Req 5 AC 13's "most + * recent SnapshotEntry with ts <= start_ts" scan tolerates any subset. + * + * On a periodic-checkpoint write failure, recording is disabled via + * sched_record_abort_on_error() -- the exact same shared path every other + * recording write failure in this codebase uses, never special-cased + * (Req 5 AC 10's "Error handling" clause applies uniformly). + */ int sched_record_write_events(xce_scheduler_t* sched, xce_core_event_t* events, int n) { @@ -173,5 +210,16 @@ int sched_record_write_events(xce_scheduler_t* sched, xce_core_event_t* events, } } + if ((sched->record_snapshot_interval > 0) && (sched->record_event_count > 0) + && ((sched->record_event_count % sched->record_snapshot_interval) + == 0)) { + int rc = write_snapshot_entry(sched->record_file, &(sched->core), + xce_core_now(&(sched->core))); + if (rc < 0) { + sched_record_abort_on_error(sched, rc); + return rc; + } + } + return 0; } diff --git a/src/scheduler/testsuite/test_record_lifecycle.c b/src/scheduler/testsuite/test_record_lifecycle.c index ddcbb8b8..6b9bb472 100644 --- a/src/scheduler/testsuite/test_record_lifecycle.c +++ b/src/scheduler/testsuite/test_record_lifecycle.c @@ -1,9 +1,10 @@ /* * test_record_lifecycle.c - contract test for the record start/stop - * lifecycle and the driver-loop recording-write hooks (scheduler-core - * spec, task 9.2, Req 5 AC 10). + * lifecycle, the driver-loop recording-write hooks, periodic snapshot + * checkpointing, and recording error handling (scheduler-core spec, tasks + * 9.2/9.3, Req 5 AC 10/AC 11). * - * Observable under test (this task's own text): + * Observable under test (task 9.2's own text): * - start -> the file begins with a decodable ReplayHeader then a * decodable SnapshotEntry. * - start -> submit an event batch -> complete one session -> stop @@ -14,6 +15,27 @@ * - a TOCTOU-style symlink/hardlink swap between path validation and the * eventual open() is rejected, not silently followed. * + * Observable under test (task 9.3's own text, added this revision): + * - recording through a configured `record_snapshot_interval` produces + * the expected NUMBER of extra periodic SnapshotEntry messages in the + * file (interval=3, 7 events across 7 calls -> 2 extra checkpoints; + * interval=0 -> zero extra checkpoints regardless of event count; one + * large single-call batch crossing two interval multiples -> exactly + * ONE checkpoint, proving/documenting this implementation's "once per + * call" boundary choice -- see recording_events.c's own comment on + * sched_record_write_events() for the full rationale). + * - simulating a write failure leaves the scheduler still processing + * sessions afterward with recording cleanly disabled: a genuine fd-level + * write failure (not a simulated return code -- see the fflush()+ + * setvbuf(_IONBF)+close(fd) sequence in + * test_write_failure_disables_recording_and_driver_keeps_running(), and + * .wolf/cerebrum.md's matching Key Learning for why plain fd-closing + * alone is not sufficient against a buffered stdio stream) triggers + * sched_record_abort_on_error()'s cleanup (recording=false, + * record_file/record_file_path both NULL), then a full real + * normal-mode round still completes with no crash/hang and no further + * recording I/O attempted. + * * White-box #include of ../src/core.c, ../src/core_clock.c, * ../src/core_events.c, ../src/core_session.c, ../src/driver.c, * ../src/plugin.c (same file set/rationale as @@ -809,6 +831,383 @@ static void test_toctou_hardlink_planted_after_validation_rejected(void) TEST_ASSERT_EQUAL_STRING(victim_content, readback); } +/* ---------------------------------------------------------------------- */ +/* Test 6: periodic snapshot checkpointing (task 9.3, Req 5 AC 11) -- */ +/* interval N produces the expected NUMBER of extra SnapshotEntry */ +/* messages, decoded and counted (not just file-size-checked) from the */ +/* real generated capnp decoders, same convention as the tests above. */ +/* ---------------------------------------------------------------------- */ + +/* Builds and submits one synthetic JOB_STATE_CHANGE event via + * sched_record_write_events() -- the direct entry point recording_events.c + * exposes, bypassing the need to drive a real session (xce_sched_core_t's + * `session_active` stays 0 throughout, which is exactly what + * write_snapshot_entry()'s xce_sched_core_save_snapshot() call requires -- + * core.h's own documented precondition -- so calling this hook directly, + * without ever running a session, is a faithful, simpler stand-in for the + * real driver.c call site). `jobid` varies per call purely so decoded + * events are individually distinguishable in assertions below. */ +static void submit_one_jobstate_event(xce_scheduler_t* sched, uint64_t jobid) +{ + xce_core_event_t ev; + + memset(&ev, 0, sizeof(ev)); + ev.type = XCE_CORE_EVENT_JOB_STATE_CHANGE; + ev.u.job_state_change.jobid = jobid; + + TEST_ASSERT_EQUAL_INT(0, sched_record_write_events(sched, &ev, 1)); +} + +/* Reads exactly one capnp message off `f` and decodes it as a ReplayEvent, + * asserting it is a jobStateChange event for `expected_jobid`. */ +static void expect_next_msg_is_jobstate_event(FILE* f, uint64_t expected_jobid) +{ + struct capn c; + ReplayEvent_ptr p; + struct ReplayEvent wire; + replay_event_t dst; + + TEST_ASSERT_EQUAL_INT(0, capn_init_fp(&c, f, 0)); + p.p = capn_getp(capn_root(&c), 0, 1); + read_ReplayEvent(&wire, p); + memset(&dst, 0, sizeof(dst)); + decode_ReplayEvent(&dst, &wire); + TEST_ASSERT_EQUAL_INT((int)ReplayEvent_body_jobStateChange, + (int)dst.body_which); + TEST_ASSERT_NOT_NULL(dst.body.job_state_change); + TEST_ASSERT_EQUAL_UINT64(expected_jobid, dst.body.job_state_change->jobid); + free_ReplayEvent(&dst); + capn_free(&c); +} + +/* Reads exactly one capnp message off `f` and decodes it as a + * SnapshotEntry, asserting its `data` field is present (same structural + * check test 1 above already establishes for the initial SnapshotEntry). */ +static void expect_next_msg_is_snapshot_entry(FILE* f) +{ + struct capn c; + SnapshotEntry_ptr snap; + SnapshotData_ptr sdata; + + TEST_ASSERT_EQUAL_INT(0, capn_init_fp(&c, f, 0)); + snap.p = capn_getp(capn_root(&c), 0, 1); + sdata = SnapshotEntry_get_data(snap); + TEST_ASSERT_FALSE(sdata.p.type == CAPN_NULL); + capn_free(&c); +} + +/* Reads exactly one capnp message off `f` and decodes it as the + * end-of-recording marker, ReplayEvent{endSession}. */ +static void expect_next_msg_is_end_session_marker(FILE* f) +{ + struct capn c; + ReplayEvent_ptr p; + struct ReplayEvent wire; + replay_event_t dst; + + TEST_ASSERT_EQUAL_INT(0, capn_init_fp(&c, f, 0)); + p.p = capn_getp(capn_root(&c), 0, 1); + read_ReplayEvent(&wire, p); + memset(&dst, 0, sizeof(dst)); + decode_ReplayEvent(&dst, &wire); + TEST_ASSERT_EQUAL_INT((int)ReplayEvent_body_endSession, + (int)dst.body_which); + free_ReplayEvent(&dst); + capn_free(&c); +} + +/* Skips the ReplayHeader + initial SnapshotEntry that sched_record_start() + * always writes first (Req 5 AC 20) -- every test below only cares about + * what comes AFTER those two, so this consumes them structurally (same + * "structural check only" pattern test 2 above already uses for its own + * ReplaySession message). */ +static void skip_header_and_initial_snapshot(FILE* f) +{ + struct capn c; + + TEST_ASSERT_EQUAL_INT(0, capn_init_fp(&c, f, 0)); /* ReplayHeader */ + capn_free(&c); + TEST_ASSERT_EQUAL_INT(0, + capn_init_fp(&c, f, 0)); /* initial SnapshotEntry */ + capn_free(&c); +} + +/* interval=3, 7 events submitted ONE PER CALL (7 separate + * sched_record_write_events() calls) -- record_event_count runs + * 1,2,3,4,5,6,7, landing exactly on the interval's multiples at 3 and 6. + * Expect exactly 2 extra periodic SnapshotEntry messages, positioned + * immediately after the 3rd and 6th events respectively (this task's own + * Observable, using this task's own example numbers). */ +static void +test_periodic_checkpoint_interval_produces_expected_snapshot_count(void) +{ + char record_dir[PATH_MAX]; + xce_scheduler_t sched; + char path_copy[PATH_MAX]; + FILE* f; + struct capn c; + int i; + + make_tmp_dir(record_dir, sizeof(record_dir), "_periodic"); + fixture_init(&sched, record_dir); + sched.record_snapshot_interval = 3ULL; + + TEST_ASSERT_EQUAL_INT(0, sched_record_start(&sched, "periodic.replay")); + strcpy(path_copy, sched.record_file_path); + + for (i = 0; i < 7; i++) { + submit_one_jobstate_event(&sched, (uint64_t)(100 + i)); + } + TEST_ASSERT_EQUAL_UINT64(7ULL, sched.record_event_count); + TEST_ASSERT_TRUE(sched.recording); /* no abort fired along the way */ + + TEST_ASSERT_EQUAL_INT(0, sched_record_stop(&sched)); + + f = fopen(path_copy, "r"); + TEST_ASSERT_NOT_NULL(f); + skip_header_and_initial_snapshot(f); + + expect_next_msg_is_jobstate_event(f, 100ULL); + expect_next_msg_is_jobstate_event(f, 101ULL); + expect_next_msg_is_jobstate_event(f, 102ULL); + expect_next_msg_is_snapshot_entry(f); /* count==3, extra checkpoint #1 */ + expect_next_msg_is_jobstate_event(f, 103ULL); + expect_next_msg_is_jobstate_event(f, 104ULL); + expect_next_msg_is_jobstate_event(f, 105ULL); + expect_next_msg_is_snapshot_entry(f); /* count==6, extra checkpoint #2 */ + expect_next_msg_is_jobstate_event(f, 106ULL); + expect_next_msg_is_end_session_marker(f); + + /* Nothing more to read -- exactly 2 extra SnapshotEntry messages, no + * more, no fewer. */ + TEST_ASSERT_NOT_EQUAL(0, capn_init_fp(&c, f, 0)); + + fclose(f); + printf("test_periodic_checkpoint_interval_produces_expected_snapshot_count:" + " PASS\n"); +} + +/* record_snapshot_interval == 0 (the default/disabled case) -- many events + * recorded, zero extra SnapshotEntry messages produced regardless. */ +static void test_periodic_checkpoint_interval_zero_disables_checkpointing(void) +{ + char record_dir[PATH_MAX]; + xce_scheduler_t sched; + char path_copy[PATH_MAX]; + FILE* f; + struct capn c; + int i; + + make_tmp_dir(record_dir, sizeof(record_dir), "_nointerval"); + fixture_init(&sched, record_dir); + TEST_ASSERT_EQUAL_UINT64(0ULL, + sched.record_snapshot_interval); /* default */ + + TEST_ASSERT_EQUAL_INT(0, sched_record_start(&sched, "nointerval.replay")); + strcpy(path_copy, sched.record_file_path); + + for (i = 0; i < 10; i++) { + submit_one_jobstate_event(&sched, (uint64_t)(200 + i)); + } + TEST_ASSERT_EQUAL_UINT64(10ULL, sched.record_event_count); + + TEST_ASSERT_EQUAL_INT(0, sched_record_stop(&sched)); + + f = fopen(path_copy, "r"); + TEST_ASSERT_NOT_NULL(f); + skip_header_and_initial_snapshot(f); + + for (i = 0; i < 10; i++) { + expect_next_msg_is_jobstate_event(f, (uint64_t)(200 + i)); + } + expect_next_msg_is_end_session_marker(f); + + /* No SnapshotEntry anywhere in that stream -- exactly the 10 events + * plus the end marker, nothing more. */ + TEST_ASSERT_NOT_EQUAL(0, capn_init_fp(&c, f, 0)); + + fclose(f); + printf("test_periodic_checkpoint_interval_zero_disables_checkpointing:" + " PASS\n"); +} + +/* interval=3, but all 6 events are submitted in ONE single + * sched_record_write_events() call (one batch) -- record_event_count jumps + * straight from 0 to 6 in that single call, crossing BOTH the count==3 and + * count==6 multiples within it. Documents/proves this task's chosen + * "once per call" semantics: only ONE checkpoint is written for the call, + * not one per crossed boundary (see recording_events.c's own comment on + * sched_record_write_events() for the full rationale) -- this is the exact + * scenario where "once per crossed boundary" and "once per call" would + * observably differ, and this test proves which one this implementation + * chose. */ +static void +test_periodic_checkpoint_single_batch_crossing_two_boundaries_checkpoints_once( + void) +{ + char record_dir[PATH_MAX]; + xce_scheduler_t sched; + char path_copy[PATH_MAX]; + xce_core_event_t events[6]; + FILE* f; + struct capn c; + int i; + + make_tmp_dir(record_dir, sizeof(record_dir), "_batchboundary"); + fixture_init(&sched, record_dir); + sched.record_snapshot_interval = 3ULL; + + TEST_ASSERT_EQUAL_INT(0, + sched_record_start(&sched, "batchboundary.replay")); + strcpy(path_copy, sched.record_file_path); + + memset(events, 0, sizeof(events)); + for (i = 0; i < 6; i++) { + events[i].type = XCE_CORE_EVENT_JOB_STATE_CHANGE; + events[i].u.job_state_change.jobid = (uint64_t)(300 + i); + } + TEST_ASSERT_EQUAL_INT(0, sched_record_write_events(&sched, events, 6)); + TEST_ASSERT_EQUAL_UINT64(6ULL, sched.record_event_count); + TEST_ASSERT_TRUE(sched.recording); + + TEST_ASSERT_EQUAL_INT(0, sched_record_stop(&sched)); + + f = fopen(path_copy, "r"); + TEST_ASSERT_NOT_NULL(f); + skip_header_and_initial_snapshot(f); + + for (i = 0; i < 6; i++) { + expect_next_msg_is_jobstate_event(f, (uint64_t)(300 + i)); + } + expect_next_msg_is_snapshot_entry(f); /* exactly ONE, not two */ + expect_next_msg_is_end_session_marker(f); + + TEST_ASSERT_NOT_EQUAL(0, capn_init_fp(&c, f, 0)); + + fclose(f); + printf("test_periodic_checkpoint_single_batch_crossing_two_boundaries_" + "checkpoints_once: PASS\n"); +} + +/* A periodic checkpoint's OWN write can fail independently of the + * per-event writes that precede it in the same call -- proven here by + * forcing xce_sched_core_save_snapshot() (which write_snapshot_entry() + * calls first) to fail via core.h's own documented `session_active != 0` + * precondition, entirely independent of file-descriptor health. Proves + * task 9.3's "route through the SAME sched_record_abort_on_error() path, + * do not special-case it" requirement for the periodic-checkpoint branch + * specifically (the general write-failure test below already covers the + * per-event-write branch). */ +static void +test_periodic_checkpoint_write_failure_disables_recording_cleanly(void) +{ + char record_dir[PATH_MAX]; + xce_scheduler_t sched; + xce_core_event_t ev; + + make_tmp_dir(record_dir, sizeof(record_dir), "_ckptfail"); + fixture_init(&sched, record_dir); + sched.record_snapshot_interval = 1ULL; /* every single event checkpoints */ + + TEST_ASSERT_EQUAL_INT(0, sched_record_start(&sched, "ckptfail.replay")); + TEST_ASSERT_TRUE(sched.recording); + + /* Force xce_sched_core_save_snapshot()'s precondition check to fail + * (core.h: "-EBUSY ... checked BEFORE any mutation") -- no real session + * is ever driven; this only fakes the flag write_snapshot_entry()'s + * save_snapshot() call reads. */ + sched.core.session_active = 1; + + memset(&ev, 0, sizeof(ev)); + ev.type = XCE_CORE_EVENT_JOB_STATE_CHANGE; + ev.u.job_state_change.jobid = 400ULL; + + /* The event itself writes fine; the periodic checkpoint that fires + * immediately after it (interval=1) is what fails and aborts. */ + TEST_ASSERT_TRUE(sched_record_write_events(&sched, &ev, 1) < 0); + + TEST_ASSERT_FALSE(sched.recording); + TEST_ASSERT_NULL(sched.record_file); + TEST_ASSERT_NULL(sched.record_file_path); + + sched.core.session_active = 0; /* restore -- avoid tripping other state */ +} + +/* ---------------------------------------------------------------------- */ +/* Test 7: write-failure end-to-end (task 9.3's own Observable) -- */ +/* a genuine write failure (the underlying fd closed out from under the */ +/* buffered FILE*, then forced unbuffered so the very next fwrite() hits */ +/* the syscall directly instead of silently buffering past the break) */ +/* cleanly disables recording via sched_record_abort_on_error(), AND the */ +/* driver loop keeps working afterward -- a full real session still */ +/* completes with no crash/hang, recording simply staying inactive. */ +/* ---------------------------------------------------------------------- */ + +static void test_write_failure_disables_recording_and_driver_keeps_running(void) +{ + char record_dir[PATH_MAX]; + xce_scheduler_t sched; + xce_core_event_t ev; + int fd; + uint64_t event_count_before_failure; + + make_tmp_dir(record_dir, sizeof(record_dir), "_writefail"); + fixture_init(&sched, record_dir); + + TEST_ASSERT_EQUAL_INT(0, sched_record_start(&sched, "writefail.replay")); + TEST_ASSERT_TRUE(sched.recording); + TEST_ASSERT_NOT_NULL(sched.record_file); + + /* Commit the header+initial-snapshot bytes sched_record_start() already + * buffered to the still-healthy fd, then force the stream unbuffered so + * every subsequent fwrite() hits the underlying fd directly (no stdio + * buffering to mask a failure), THEN close that fd out from under the + * FILE* -- "close the underlying fd out from under the FILE* before a + * write", per this task's own suggested technique. */ + 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)); + + memset(&ev, 0, sizeof(ev)); + ev.type = XCE_CORE_EVENT_JOB_STATE_CHANGE; + ev.u.job_state_change.jobid = 500ULL; + + /* The write genuinely fails now (EBADF on the closed fd, surfaced + * immediately since the stream is unbuffered) -- proving a REAL I/O + * failure, not a simulated return code. */ + TEST_ASSERT_TRUE(sched_record_write_events(&sched, &ev, 1) < 0); + + /* sched_record_abort_on_error()'s cleanup fired: recording cleanly + * disabled, file handle closed+NULLed, path freed+NULLed -- this + * task's own Observable, "recording cleanly disabled". */ + TEST_ASSERT_FALSE(sched.recording); + TEST_ASSERT_NULL(sched.record_file); + TEST_ASSERT_NULL(sched.record_file_path); + + event_count_before_failure = sched.record_event_count; + + /* "the scheduler still processing sessions afterward" -- drive one full + * real normal-mode round (real picker thread + stand-in allocator + * thread, same helper the earlier full-cycle test uses) through the + * now-recording-inactive scheduler. No crash, no hang, and no further + * recording I/O is attempted (recording_inactive_hooks test above + * already proves the zero-I/O contract for `recording == false`; this + * proves it holds true specifically AFTER a real abort, not just from a + * fixture that never started recording). */ + run_one_real_session(&sched); + + TEST_ASSERT_FALSE(sched.recording); + TEST_ASSERT_NULL(sched.record_file); + TEST_ASSERT_NULL(sched.record_file_path); + TEST_ASSERT_EQUAL_UINT64(event_count_before_failure, + sched.record_event_count); + + printf("test_write_failure_disables_recording_and_driver_keeps_running:" + " PASS\n"); +} + int main(void) { UNITY_BEGIN(); @@ -821,6 +1220,13 @@ int main(void) RUN_TEST(test_null_sched_returns_einval); RUN_TEST(test_toctou_symlink_planted_after_validation_rejected); RUN_TEST(test_toctou_hardlink_planted_after_validation_rejected); + RUN_TEST( + test_periodic_checkpoint_interval_produces_expected_snapshot_count); + RUN_TEST(test_periodic_checkpoint_interval_zero_disables_checkpointing); + RUN_TEST( + test_periodic_checkpoint_single_batch_crossing_two_boundaries_checkpoints_once); + RUN_TEST(test_periodic_checkpoint_write_failure_disables_recording_cleanly); + RUN_TEST(test_write_failure_disables_recording_and_driver_keeps_running); return UNITY_END(); }