feat(scheduler-core): implement record start/stop lifecycle (task 9.2)
Adds recording-state fields directly to xce_scheduler_t (no new
xce_sched_driver_t type, per task 9.1's precedent). sched_record_start()/
sched_record_stop() implement AC 10: validate via task 9.1's path
validator, open with a TOCTOU-hardened sequence (O_NOFOLLOW, no O_TRUNC,
fstat()+st_nlink==1 check before ftruncate()), snapshot the core, write
ReplayHeader+SnapshotEntry on start, ReplayEvent{endSession} on stop.
driver.c gains boolean-gated recording hooks in
xce_sched_driver_translate_pending_events() (per-event-batch ReplayEvent
writes, new recording_events.c) and xce_sched_driver_run_normal_mode_round()
(per-session ReplaySession writes, new recording_session.c) -- zero I/O/
allocation when inactive.
Two real bugs found and fixed during implementation: bug-1498 (O_TRUNC
would have truncated a hard-linked victim file before the TOCTOU check
could reject it) and bug-1499 (a transient edit dropped
xce_core_signal_go(), causing a 3-way thread deadlock).
test_record_lifecycle.c decodes the full start->events->session->stop
cycle with the real capnp decoders and asserts the exact message
sequence; TOCTOU regression tests use real symlink(2)/link(2) syscalls.
25/25 scheduler suite, clang-format clean.
This commit is contained in:
parent
4e5547ee55
commit
ff8d4f416e
15 changed files with 2110 additions and 36 deletions
|
|
@ -224,7 +224,7 @@
|
|||
- _Depends: 8.1_
|
||||
- _Verified: 2026-08-04 — `--record-dir` CLI flag added to main.c/scheduler.h (record_dir field, never a request-path field). sched_validate_record_filename() (new recording.c/recording.h) rejects any `filename` containing `/` or `..` before any filesystem call, then resolves via realpath() and confirms canonical (not textual-prefix) containment beneath record_dir via path_is_contained(); rejects pre-existing non-regular entries (-EEXIST) and a non-directory record_dir (-ENOTDIR). Adversarial security review found and this task fixed two real CWE-59 inode-aliasing bypasses across 3 rounds: round 1 found a direct hard link inside record_dir aliasing an outside file passed validation (S_ISREG doesn't distinguish a hard-linked regular file from a genuine one) — fixed with an entry_st.st_nlink > 1 check (-EACCES) in the direct-entry branch, live RED→GREEN reproduced. Round 2 re-review found a compound variant still open: a symlink inside record_dir pointing at an in-directory file itself hard-linked outside record_dir was still accepted, since the S_ISLNK branch checked containment but never st_nlink on the resolved target — fixed by stat()'ing the already-realpath()-resolved target and applying the same S_ISREG && st_nlink>1 check, generalizing correctly to multi-hop symlink chains, live RED→GREEN reproduced. Round 3 re-review independently reproduced both fixes via a standalone PoC harness and additionally hunted for further variants (2-hop chains, symlink-to-symlink escape, FIFO/socket direct-naming) — all clean, no new bypass found; one non-blocking observation logged (symlink resolving to a contained-but-non-regular target is accepted rather than rejected, pre-existing since round 1, not the CWE-22/CWE-59 escape class, already covered by task 9.2's planned O_NOFOLLOW hardening) — APPROVED. TOCTOU residual risk (validation-to-eventual-fopen() window) explicitly documented in recording.h and deferred to task 9.2 (O_NOFOLLOW + fstat()-based st_nlink==1 re-check on the opened fd), not this task's scope. 18/18 recording subtests (16 original + 2 hard-link/compound regressions), 24/24 scheduler suite, clang-format clean — all independently re-verified by me via a fresh `meson compile`/`meson test --suite scheduler` run on current HEAD before marking complete._
|
||||
|
||||
- [ ] 9.2 Implement the record start/stop lifecycle
|
||||
- [x] 9.2 Implement the record start/stop lifecycle
|
||||
- On start: take a full snapshot, write the replay header followed by the initial snapshot entry, begin tracking recording state
|
||||
- On each event batch and each completed session while recording: write the corresponding event/session messages to the record file in order — extends the normal-mode driver main loop built in group 5 with recording insert points, it does not replace it
|
||||
- On stop: write the end-of-recording marker, flush and close the file, clear recording state
|
||||
|
|
@ -232,6 +232,7 @@
|
|||
- Observable: a start → submit events → complete a session → stop cycle produces a file beginning with header+snapshot-entry and ending with the end-of-recording marker, with events and one session in between, byte-decodable by the round-trip decoder from group 7
|
||||
- _Requirements: 5.10_
|
||||
- _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
|
||||
- Append a fresh snapshot entry to the record file every configured number of recorded events (0 disables this)
|
||||
|
|
@ -382,3 +383,5 @@
|
|||
- Task 4.3 split its implementation into a NEW file `core_events.c` (not `core.c`, which design.md's File Structure Plan literally names) because implementing `xce_core_push_events()` directly in `core.c` would have pushed it to 977 lines, over this project's 800-line hard cap. `core.h` (the public contract) matches design.md's intent; only the `.c` file location differs. Worth reflecting in design.md next time it's touched. Also: `test_core_isolation.c`'s nm(1)-based zero-controller-I/O boundary audit (Req 5 AC 3) covers only `core.c`, not `core_events.c` — manually verified clean (no netio/xceapi/client/delay/idgen usage) but not mechanically enforced; extending that test to cover `core_events.c` would be a reasonable low-risk follow-up before later tasks add more logic to this file.
|
||||
- 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.
|
||||
- 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