feat(scheduler-core): implement periodic snapshot checkpointing (task 9.3)
Periodic checkpointing hooks into sched_record_write_events(): once per call, when record_snapshot_interval > 0 and record_event_count crosses a multiple of it, writes a fresh SnapshotEntry via the same write_snapshot_entry() helper sched_record_start() already uses (made non-static, zero duplication). Safe from the session_active precondition because this call site is only ever reached from xce_sched_driver_translate_pending_events(), which always runs strictly between sessions. Confirmed the write-failure error-handling half (AC 10) was already complete from task 9.2's sched_record_abort_on_error() -- added the missing end-to-end test using a genuine closed-fd write failure rather than a mocked return code, proving the driver keeps processing sessions afterward. 25/25 scheduler suite, clang-format clean. Two non-blocking follow-ups logged in tasks.md: --record-snapshot-interval has no CLI wiring yet (AC 11's own struct-field mechanism is fully tested, but nothing sets it in production), and test_record_lifecycle.c has grown past this suite's prior size precedent with a natural future split point identified.
This commit is contained in:
parent
ff8d4f416e
commit
7494542a60
6 changed files with 531 additions and 31 deletions
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue