lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat(scheduler-core): implement scheduler-side admin request handler (task 9.4)

Browse source 
Implements the scheduler's first request-reply opcode (OPCode_schedAdmin).
sched_handle_admin_request() (handler.c) decodes SchedAdminRequest and
dispatches by action: recordStatus answers synchronously (pure read);
recordStart/recordStop are deferred onto the existing pend_updates queue
(XCE_SCHED_OP_ADMIN_RECORD) since sched_record_start()'s snapshot call
requires the session_active==0 precondition only guaranteed on the driver
thread, not the async request-handling thread. driver.c's
xce_sched_driver_translate_pending_events() performs the deferred action
and sends the reply via a new xcecli_reply_schedadmin() (client.c/client.h),
correlated by seq copied before the original request is freed.

Adversarial review found a genuine cross-thread race: sched_record_status()
read driver-owned recording state (including a pointer the driver thread
frees) from the vtask thread with no lock -- the codebase's first such
race, missed by ASan (doesn't detect races). Fixed with a dedicated
record_lock scoped strictly to in-memory field access, never spanning file
I/O; sched_record_status() now strdup()s under the lock instead of
returning a borrowed live pointer. Verified with a real two-thread
lock-contention test (independently confirmed non-vacuous by removing the
lock and observing the test fail) plus a deterministic use-after-free
reproduction.

26/26 scheduler suite, clang-format clean.
This commit is contained in:
Rongsong Shen 2026-08-05 14:11:43 +00:00
parent 7494542a60
commit e819a50600
Signed by: rshen
SSH key fingerprint: SHA256:8Vf2pZ3h6rJa7EhGK+WaAQ9Mta/apZvteW6g9U/WKKk
19 changed files with 1761 additions and 115 deletions
Download patch file Download diff file Expand all files Collapse all files

64
src/scheduler/include/recording.h
View file

@ -170,6 +170,12 @@ extern int sched_validate_record_filename(const char* record_dir,
*
* `record_file` may be NULL or empty to auto-generate a timestamped name
* (`scheduler-replay-<UTC timestamp>.replay`) beneath `sched->record_dir`.
*
* Concurrency (task 9.4 fix round): step 4's field commit takes
* `sched->record_lock` (scheduler.h's field comments) -- see
* sched_record_status()'s doc comment below for the cross-thread reader
* this protects against. The lock is held only around the five assignments
* themselves, never across steps 1-3's file I/O.
*/
extern int sched_record_start(xce_scheduler_t* sched, const char* record_file);
@ -192,6 +198,13 @@ extern int sched_record_start(xce_scheduler_t* sched, const char* record_file);
* marker failed to write (state is still fully cleared either way, per the
* error-handling clause above -- the negative return is diagnostic only,
* not a signal that recording is still active).
*
* Concurrency (task 9.4 fix round): the fflush()+fclose()+state-clear
* sequence above takes `sched->record_lock` (scheduler.h's field
* comments) around the state-clear portion only -- see
* sched_record_status()'s doc comment below for the cross-thread reader
* this protects against. In particular, freeing `record_file_path` and
* setting it NULL happens atomically from that reader's point of view.
*/
extern int sched_record_stop(xce_scheduler_t* sched);
@ -225,6 +238,13 @@ extern int sched_record_stop(xce_scheduler_t* sched);
* 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.
*
* Concurrency (task 9.4 fix round): the per-event increment of
* `sched->record_event_count` takes `sched->record_lock` (scheduler.h's
* field comments), scoped to just the increment -- see
* sched_record_status()'s doc comment below for the cross-thread reader
* this protects against. `record_snapshot_interval` is driver-thread-only
* and not protected by this lock (scheduler.h's field comment).
*/
extern int sched_record_write_events(xce_scheduler_t* sched,
xce_core_event_t* events, int n);
@ -283,4 +303,48 @@ extern int sched_record_session_end(xce_scheduler_t* sched,
sched_record_session_ctx_t* ctx,
uint64_t end_ts);
/*
* sched_record_status - Req 5 AC 10 (scheduler-core spec, task 9.4,
* "Reply field semantics by action" -- the `recordStatus` column). A pure
* read of `sched`'s CURRENT recording-state fields into a
* `sched_admin_reply_t` the caller (handler.c's scheduler_req_handler())
* sends back over the wire immediately -- no file I/O, no state mutation,
* and (unlike sched_record_start()/sched_record_stop()) no session_active
* dependency, so it is safe to answer directly from the request-handling
* thread rather than deferring through the driver-thread pend_updates
* queue the way recordStart/recordStop must.
*
* Concurrency (task 9.4 fix round): this function is called from a vtask
* worker pthread (handler.c's scheduler_req_handler(), invoked with no
* lock held -- lib/xcectx.c), which runs concurrently with the
* single-threaded driver loop's sched_record_start()/sched_record_stop()/
* sched_record_write_events()/sched_record_abort_on_error() -- all of
* which mutate the same fields this function reads, with no other
* synchronization. This function therefore takes `sched->record_lock`
* (scheduler.h's field comments) around every read of those fields,
* INCLUDING the copy of `record_file_path` described next.
*
* `reply->file` is a fresh, caller-owned heap copy of
* `sched->record_file_path` (via strdup(), taken while still holding
* `sched->record_lock` -- copying the bytes before the lock is released is
* what makes this safe), NOT a borrowed alias of the live pointer. Unlike
* every other capnp encode call in this codebase, this reply crosses a
* function-return boundary before it is ever encoded, so a borrowed
* pointer into `sched`'s live, mutable state cannot be handed back safely
* -- the driver thread could free() it the instant `record_lock` is
* released, well before the caller's eventual xcecli_reply_schedadmin()
* call. The caller MUST free(reply->file) once it is done (NULL when not
* recording -- nothing to free).
*
* Returns 0 on success, populating every field of `*reply`
* (`reply->ok = true` unconditionally). Returns -EINVAL if `sched` or
* `reply` is NULL; `*reply` is left untouched in that case. Returns
* -ENOMEM if the internal strdup() of `record_file_path` fails; `*reply`
* is left zeroed (via the initial memset()) in that case -- this is the
* one way reading status can now itself fail, a direct consequence of no
* longer handing back a borrowed pointer.
*/
extern int sched_record_status(xce_scheduler_t* sched,
sched_admin_reply_t* reply);
#endif

88
src/scheduler/include/scheduler.h
View file

@ -39,7 +39,22 @@ typedef enum {
/* Notify-only single-job in-place field update. Distinct from
* XCE_SCHED_OP_UPDATE_JOB (the dataset bulk-create path) so a notify
* update to an existing job mutates it in place and never creates a job. */
XCE_SCHED_OP_UPDATE_JOB_FIELDS
XCE_SCHED_OP_UPDATE_JOB_FIELDS,
/* Scheduler admin recording control (scheduler-core spec, task 9.4, Req
* 5 AC 10): a `recordStart`/`recordStop` SchedAdminRequest, deferred
* from the request-handling thread (handler.c's scheduler_req_handler())
* to the driver thread. sched_record_start()/sched_record_stop() do
* real file I/O and sched_record_start() calls
* xce_sched_core_save_snapshot(), which requires the core's
* session_active precondition (task 8.2) -- a guarantee only held on
* the single-threaded driver loop (main.c), not the async request-
* handling thread. See xce_sched_update_request_t's `adminrecord`
* member below and driver.c's xce_sched_driver_translate_pending_events()
* for the full deferred-dispatch + deferred-reply design.
* `recordStatus` is a pure read of already-current struct fields (no
* file I/O, no session_active dependency) and is answered synchronously
* from the handler thread instead -- it never reaches this queue. */
XCE_SCHED_OP_ADMIN_RECORD
} xce_sched_op_t;
typedef struct {
@ -79,6 +94,27 @@ typedef struct {
struct {
xce_job_rusages_t* rusages;
} job_rusage;
struct {
/* SchedAdminAction_recordStart or SchedAdminAction_recordStop
* (controller.capnp.h) -- recordStatus never reaches this
* queue (answered synchronously, see xce_sched_op_t's own
* comment on XCE_SCHED_OP_ADMIN_RECORD above). */
int action;
/* strdup'd copy of SchedAdminRequest.recordFile for
* recordStart (NULL for auto-generate, and always NULL for
* recordStop, which ignores it) -- the original xce_request_t
* is freed by the handler-thread caller (xcectx.c's
* vtask_entry()) immediately after scheduler_req_handler()
* returns, so this must be an independent, owned copy for the
* driver thread to read later. Freed by the deferred
* dispatcher (driver.c) once the action completes. */
char* record_file;
/* The original request's seq -- the only field of the
* now-freed xce_request_t the deferred reply needs
* (init_reply_by_req()'s other field, opcode, is always
* OPCode_schedAdmin here, so it is not worth storing). */
uint64_t seq;
} adminrecord;
} u;
} xce_sched_update_request_t;
@ -132,27 +168,63 @@ typedef struct {
* writes -- see xce_sched_driver_translate_pending_events()'s and
* xce_sched_driver_run_normal_mode_round()'s own comments.
*
* Invariant: recording == true iff record_file != NULL. */
* Invariant: recording == true iff record_file != NULL.
*
* Concurrency (scheduler-core spec, task 9.4 fix round): `recording`,
* `record_file_path`, `record_file`, `record_event_count`, and
* `record_start_ts` are written on the single-threaded driver loop
* (main.c) but ALSO read by sched_record_status() (recording.c), which
* runs on whichever vtask worker pthread handled the synchronous
* `recordStatus` SchedAdminRequest (handler.c's
* scheduler_req_handler(), invoked with no lock held --
* lib/xcectx.c). That makes this a genuine cross-thread reader/writer
* pair with no other synchronization, so every read or write of these
* five fields MUST hold `record_lock` below -- see its own comment. */
bool recording;
/* strdup'd copy of the resolved record file path; owned by the record
* file lifecycle (strdup on start, free+NULL on stop or start/write
* failure) -- lets a future status query (task 9.4, out of this task's
* scope) answer with the current path even after record_file closes. */
* failure) -- lets a status query (task 9.4) answer with the current
* path even after record_file closes. Protected by `record_lock` (see
* `recording`'s comment above and `record_lock`'s own comment below) --
* sched_record_status() must copy this string's bytes (e.g. strdup())
* while still holding the lock rather than handing back the live
* pointer, since the driver thread may free() it the instant the lock
* is released. */
char* record_file_path;
/* replay file handle; NULL when not recording. */
/* replay file handle; NULL when not recording. Protected by
* `record_lock` for the same reason as `record_file_path` above. */
FILE* record_file;
/* count of ReplayEvent messages written since recording started (used
* by task 9.3's periodic-checkpoint trigger; incremented by
* recording.c's event-batch writer, task 9.2). */
* recording.c's event-batch writer, task 9.2). Protected by
* `record_lock`. */
uint64_t record_event_count;
/* 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). */
* record file (Req 5 AC 11). Driver-thread-only (never read by
* sched_record_status()) -- not protected by `record_lock`. */
uint64_t record_snapshot_interval;
/* logical time (xce_core_now()) recording started; 0 = not recording. */
/* logical time (xce_core_now()) recording started; 0 = not recording.
* Protected by `record_lock`. */
uint64_t record_start_ts;
/* Dedicated mutex (scheduler-core spec, task 9.4 fix round) guarding
* `recording`/`record_file_path`/`record_file`/`record_event_count`/
* `record_start_ts` -- see `recording`'s comment above for the
* cross-thread reader/writer hazard this closes. Deliberately a NEW,
* separate lock rather than reusing `core.lock`: `core`
* (xce_sched_core_t) is the conceptually distinct core-vs-driver piece
* of state this struct's own header comment describes, and these
* recording fields are driver-level (like `record_dir`/`apictx`/
* `cluster_cfg` above), so widening core.lock's scope to also cover
* them would blur that boundary for no benefit. Held only around the
* struct-field reads/writes themselves -- never across the actual file
* I/O (fopen()/fwrite()/capn_write_mem() etc.) -- matching this
* codebase's low-contention-lock-scope convention. Initialized by
* sched_init(), destroyed by sched_fini() (scheduler.c) -- same
* lifecycle pairing as `core.lock` (core.c). */
mylock_t record_lock;
} xce_scheduler_t;
extern int sched_init(xce_scheduler_t*, xce_client_config_t*,