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).
This commit is contained in:
parent
e819a50600
commit
2d533e652e
2 changed files with 262 additions and 1 deletions
|
|
@ -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
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue