diff --git a/.kiro/specs/xce-test-cli-ux/tasks.md b/.kiro/specs/xce-test-cli-ux/tasks.md index a5039ea7..eabd5556 100644 --- a/.kiro/specs/xce-test-cli-ux/tasks.md +++ b/.kiro/specs/xce-test-cli-ux/tasks.md @@ -17,7 +17,7 @@ - _Boundary: Flag Parsing Layer_ - _Depends: 1.1_ -- [ ] 1.3 Correct the SIGINT installation and expose an interruption accessor +- [x] 1.3 Correct the SIGINT installation and expose an interruption accessor - Replace the process's existing signal-based SIGINT installation with an explicit installation that clears the automatic-restart behavior, so a blocking read reliably returns an interrupted result instead of silently retrying - Expose a small accessor over the existing interrupt flag so the interruptible-call adapter (built next, in 1.4/1.5) can poll it without depending on the flag's storage details - Observable completion: a small standalone test that sends an interrupt signal to the process during a blocking read confirms the read returns immediately with an interrupted result rather than continuing to block @@ -178,3 +178,4 @@ ## Implementation Notes - `src/tools/xce-build.sh controller asan xce.build --incremental` writes its build dir at `src/xce.build/` (relative to the script's own location), not a repo-root `xce.build/` — meson logs are at `src/xce.build/meson-logs/testlog.txt`. (task 1.1) - New Unity test files need two wiring points, not just meson.build's source list: also register an `extern void test_(void);` + `TEST(test_);` in `src/controller/testsuite/controller_test.c`, mirroring the existing `test_rcfile` registration, or the suite silently never runs. (task 1.1) +- After task 1.3's SIGINT/EINTR fix alone (before task 3.2 wires the REPL loop to check `xcetest_interrupted()`), a real Ctrl-C at `xce_test`'s top-level interactive prompt now causes the process to EXIT (readline's EINTR-driven NULL return is currently treated the same as EOF) rather than gracefully redisplaying the prompt. This is expected/deferred, not a regression — no current test exercises that path — but task 3.2's implementer must fix it as part of wiring the REPL loop, not just the RPC call sites. (task 1.3) diff --git a/src/controller/meson.build b/src/controller/meson.build index 31b5854f..6711eaaa 100644 --- a/src/controller/meson.build +++ b/src/controller/meson.build @@ -170,6 +170,7 @@ controller_unit_test_src = [ 'testsuite/rcfile.c', 'testsuite/test_flagspec.c', 'testsuite/flagspec.c', + 'testsuite/test_xcetest_interrupt.c', 'testsuite/test_persist_snapshot_entities.c', 'testsuite/test_apply_seam.c', 'testsuite/test_apply_committed_entry.c', diff --git a/src/controller/testsuite/controller_test.c b/src/controller/testsuite/controller_test.c index 0415bcee..5114d24a 100644 --- a/src/controller/testsuite/controller_test.c +++ b/src/controller/testsuite/controller_test.c @@ -72,6 +72,7 @@ extern void test_snapdiff(void); extern void test_configcompare(void); extern void test_rcfile(void); extern void test_flagspec(void); +extern void test_xcetest_interrupt(void); extern void test_drive_async_config(void); extern void test_slowlane(void); extern void test_commit_admission_pool(void); @@ -253,6 +254,7 @@ int main(int argc, char* argv[]) TEST(test_configcompare); TEST(test_rcfile); TEST(test_flagspec); + TEST(test_xcetest_interrupt); TEST(test_drive_async_config); TEST(test_slowlane); TEST(test_commit_admission_pool); diff --git a/src/controller/testsuite/test.c b/src/controller/testsuite/test.c index 3cd90c3d..43391331 100644 --- a/src/controller/testsuite/test.c +++ b/src/controller/testsuite/test.c @@ -34,6 +34,7 @@ #include "resval_typeresolve.h" /* resval_resolve_type (resource-value-verification task 2.1) */ #include "xce/cluster.h" /* xce_parse_peers, xce_cluster_config_t */ #include "xce/types.h" +#include "xcetest_interrupt.h" /* xcetest_install_sigint, xcetest_interrupted (xce-test-cli-ux task 1.3) */ typedef int (*cmd_func_t)(int, char*[]); @@ -705,6 +706,13 @@ static struct argp argp = { options, parse_opt, args_doc, doc }; void sigbreak(int signo) { break_out = 1; } +/* xce-test-cli-ux task 1.3, Requirement 9.9: read-only accessor over + * `break_out` for code outside this file (interruptcall.c, task 1.4) to + * poll -- see xcetest_interrupt.h's file-header comment for the full + * contract (pure read, no reset side effect, break_out's own reset stays + * test.c-private). */ +int xcetest_interrupted(void) { return break_out; } + static char* get_setting(char* key) { int i; @@ -4454,7 +4462,16 @@ int main(int argc, char* argv[]) mylog_init(logfile, loglevel, logsize, logcount); - signal(SIGINT, sigbreak); + /* xce-test-cli-ux task 1.3, Requirement 9.9: replaces the former bare + * `signal(SIGINT, sigbreak);` call. glibc's signal() installs with + * SA_RESTART SET (BSD semantics), which silently restarts a blocking + * slow syscall (read(), etc.) interrupted by SIGINT instead of + * returning EINTR -- see xcetest_interrupt.h's file header for the + * full rationale. xcetest_install_sigint() installs the same + * sigbreak() handler via sigaction() with SA_RESTART explicitly + * cleared, so a later blocking read (e.g. the confirmation prompt + * built on top of this task) reliably observes EINTR. */ + xcetest_install_sigint(sigbreak); memset(&run_jobs, 0x0, sizeof(run_jobs)); memset(&run_tasks, 0x0, sizeof(run_tasks)); diff --git a/src/controller/testsuite/test_xcetest_interrupt.c b/src/controller/testsuite/test_xcetest_interrupt.c new file mode 100644 index 00000000..c96da1c4 --- /dev/null +++ b/src/controller/testsuite/test_xcetest_interrupt.c @@ -0,0 +1,152 @@ +/* + * test_xcetest_interrupt.c -- proves, via a REAL, OS-delivered SIGINT + * hitting a genuinely blocked read(), that xcetest_install_sigint() + * (xcetest_interrupt.h, xce-test-cli-ux task 1.3, Requirement 9.9) fixes + * the restart-vs-EINTR bug the bare `signal(SIGINT, sigbreak)` call in + * test.c's main() used to have. + * + * Why a fork()-based process-level test rather than a plain unit test: + * this is inherently about kernel/libc syscall-restart semantics, which + * only a REAL signal delivered to a REAL process blocked in a REAL slow + * syscall can demonstrate -- calling sigbreak() directly as a plain + * function call would prove nothing about SA_RESTART. Each test below + * forks a short-lived child that installs a SIGINT handler via the given + * installer function, blocks in read() on a pipe that is never written + * to (so it can only unblock via a signal or its own 1-second alarm + * safety net), and reports which happened via its exit code -- the exact + * fork/report pattern already established in this directory (see + * test_ha_role_lifecycle.c's noop_classifies_retain()). + */ +#include +#include +#include +#include +#include +#include + +#include + +#include "xcetest_interrupt.h" + +/* Child-side exit codes, decoded by the parent via WEXITSTATUS(). */ +#define CHILD_EXIT_EINTR_OBSERVED 0 /* read() returned -1/EINTR promptly */ +#define CHILD_EXIT_NO_EINTR 1 /* read() returned, but not via EINTR */ +#define CHILD_EXIT_TIMED_OUT 2 /* read() never returned before the + * 1-second alarm fired -- i.e. the + * interrupted syscall was silently + * restarted instead */ + +/* 1-second safety net: if the installation under test lets the blocked + * read() silently restart, this makes the child self-terminate quickly + * (reporting CHILD_EXIT_TIMED_OUT) instead of hanging the whole suite. + * Uses plain signal() -- fine here, since the handler calls _exit() + * directly and never returns, so SIGALRM's own restart-vs-EINTR + * disposition never comes into play. */ +static void child_alarm_handler(int signo) +{ + (void)signo; + _exit(CHILD_EXIT_TIMED_OUT); +} + +/* Stands in for test.c's sigbreak(): a real, non-default SIGINT handler. + * Only its mere existence (vs SIG_DFL/SIG_IGN) matters for read()'s + * restart-vs-EINTR decision -- this test is about the *installation*, + * not what the handler itself does. */ +static void child_sigint_noop_handler(int signo) +{ + (void)signo; +} + +static void install_via_plain_signal(void (*handler)(int)) +{ + signal(SIGINT, handler); +} + +/* Forks a child that installs SIGINT via `installer`, then blocks in + * read() on an always-empty pipe. The parent sends a real SIGINT to the + * child shortly after fork (giving it time to install the handler and + * enter the blocking read()) -- genuine OS signal delivery, not a direct + * call to any handler function. Returns the child's CHILD_EXIT_* exit + * code. */ +static int run_forked_blocked_read(void (*installer)(void (*)(int))) +{ + int pipefd[2]; + pid_t pid; + pid_t waited; + int status = 0; + + TEST_ASSERT_EQUAL_INT(0, pipe(pipefd)); + + pid = fork(); + TEST_ASSERT_TRUE(pid >= 0); + + if (pid == 0) { + /* child */ + char buf[1]; + ssize_t n; + + close(pipefd[1]); /* only the PARENT's copy of the write end must + * stay open -- that's what keeps this pipe + * "empty but not at EOF" for the duration of + * the child's read() below. */ + + signal(SIGALRM, child_alarm_handler); + alarm(1); + + installer(child_sigint_noop_handler); + + errno = 0; + n = read(pipefd[0], buf, sizeof(buf)); + + if (n == -1 && errno == EINTR) { + _exit(CHILD_EXIT_EINTR_OBSERVED); + } + _exit(CHILD_EXIT_NO_EINTR); + } + + /* parent */ + close(pipefd[0]); + + usleep(200000); /* give the child time to install its handler and + * reach the blocking read() before we signal it */ + TEST_ASSERT_EQUAL_INT(0, kill(pid, SIGINT)); + + waited = waitpid(pid, &status, 0); + close(pipefd[1]); + + TEST_ASSERT_EQUAL_INT(pid, waited); + TEST_ASSERT_TRUE(WIFEXITED(status)); + + return WEXITSTATUS(status); +} + +/* RED-phase documentation, kept as a permanent regression check: proves + * the ROOT CAUSE this task fixes is real in this environment, not + * assumed. glibc's signal() installs with SA_RESTART set by default, so + * a single SIGINT delivered while a process is blocked in read() does + * NOT make that read() return -- the kernel transparently restarts it, + * and the read() keeps blocking (here, until this test's own 1-second + * alarm safety net fires). */ +static void test_plain_signal_install_lets_sigint_restart_blocked_read(void) +{ + int rc = run_forked_blocked_read(install_via_plain_signal); + TEST_ASSERT_EQUAL_INT(CHILD_EXIT_TIMED_OUT, rc); +} + +/* GREEN: the fix under test. test.c's main() now installs SIGINT via + * this exact function instead of the bare signal(SIGINT, sigbreak) call + * it used before this task (Requirement 9.9). A real SIGINT delivered + * while blocked in read() must make that read() return immediately with + * errno == EINTR, not silently retry. */ +static void test_install_sigint_clears_sa_restart_so_blocked_read_returns_eintr(void) +{ + int rc = run_forked_blocked_read(xcetest_install_sigint); + TEST_ASSERT_EQUAL_INT(CHILD_EXIT_EINTR_OBSERVED, rc); +} + +void test_xcetest_interrupt(void) +{ + UnitySetTestFile(__FILE__); + RUN_TEST(test_plain_signal_install_lets_sigint_restart_blocked_read); + RUN_TEST(test_install_sigint_clears_sa_restart_so_blocked_read_returns_eintr); +} diff --git a/src/controller/testsuite/xcetest_interrupt.h b/src/controller/testsuite/xcetest_interrupt.h new file mode 100644 index 00000000..5448a311 --- /dev/null +++ b/src/controller/testsuite/xcetest_interrupt.h @@ -0,0 +1,84 @@ +#pragma once +/* + * xcetest_interrupt.h -- tiny, storage-detail-hiding surface over xce_test's + * process-wide SIGINT handling (xce-test-cli-ux spec, task 1.3; + * Requirement 9.9). + * + * ROOT CAUSE this task fixes: test.c's main() used to install its SIGINT + * handler via the bare libc call `signal(SIGINT, sigbreak);`. glibc's + * signal() uses BSD semantics, which install the handler with SA_RESTART + * SET by default -- a slow syscall (read(), etc.) that is interrupted by + * the signal is transparently RESTARTED by the kernel instead of + * returning EINTR to the caller. That silently defeats Requirement 9.9: + * a later task's confirmation-prompt read (confirm.c) needs a real Ctrl-C + * to make its blocking read() return immediately with EINTR, not keep + * waiting for input as if nothing happened. See research.md's "Decision: + * Confirmation prompt's Ctrl-C-as-decline requires SA_RESTART cleared on + * SIGINT" for the full rationale and alternatives considered. + * + * THE FIX: xcetest_install_sigint() below installs the given handler via + * sigaction() with sa_flags = 0 (i.e. explicitly WITHOUT SA_RESTART) and + * an empty sa_mask. test.c's main() now calls + * `xcetest_install_sigint(sigbreak);` instead of the old signal() call. + * + * WHY THIS LIVES IN A HEADER, AS `static inline`, RATHER THAN AS A + * NORMAL EXPORTED FUNCTION DEFINED IN test.c: test.c pulls in a large + * dependency graph (libedit, cJSON, libxce, Cap'n Proto, OpenSSL) that + * this project's Unity-based `controller_unit_test` binary does not + * link, and test.c itself defines main() -- both make it impractical to + * link test.c into a small, fast, signal-level unit test. Defining the + * installer here means test.c's main() and this task's test + * (test_xcetest_interrupt.c) call the EXACT SAME code, not two + * hand-copies that could silently drift apart -- the test genuinely + * proves what main() actually installs. + * + * xcetest_interrupted() is a small read-only accessor over test.c's + * PRIVATE `static int break_out` flag (still declared/defined in test.c, + * unmodified in shape or storage -- only this accessor is new), so code + * outside test.c -- the Interruptible Call Adapter, `interruptcall.c`, + * task 1.4 -- can poll whether a SIGINT has been observed without + * depending on break_out's storage details. + * + * CONTRACT (task 1.4's implementer: read this before use): + * - Returns nonzero once sigbreak() has run (i.e. a SIGINT was + * delivered) since break_out was last 0; returns 0 otherwise. + * - This is a PURE READ -- calling it does NOT clear break_out as a + * side effect. It is safe to call any number of times from a polling + * loop (e.g. "poll break_out every fifty milliseconds", per + * design.md's Interruptible Call Adapter) without consuming the + * interrupt. + * - No reset/clear accessor is exposed yet. test.c itself already + * resets break_out to 0 at one existing call site (cmd_hello's + * repeat loop, immediately before that loop begins) by writing the + * static directly, since it has that access. If task 1.4's design + * needs an interruptible operation to start from a guaranteed-clear + * flag and it runs from OUTSIDE test.c, it will need a new + * xcetest_clear_interrupted()-style accessor -- none exists yet + * because no task through 1.3 needed one; do not assume one is + * silently available. + */ + +#include +#include + +/* Installs `handler` as the process's SIGINT disposition via sigaction(), + * with sa_flags = 0 (i.e. explicitly WITHOUT SA_RESTART) and an empty + * sa_mask (no other signal is additionally blocked while `handler` + * runs). This is the one and only installation path for xce_test's + * SIGINT handling -- see the file header above for why a blocking slow + * syscall must observe EINTR rather than a silent restart. */ +static inline void xcetest_install_sigint(void (*handler)(int)) +{ + struct sigaction sa; + + memset(&sa, 0, sizeof(sa)); + sa.sa_handler = handler; + sa.sa_flags = 0; /* explicitly WITHOUT SA_RESTART */ + sigemptyset(&sa.sa_mask); + + sigaction(SIGINT, &sa, NULL); +} + +/* See "CONTRACT" in the file header above. Implemented in test.c, over + * its private `static int break_out`. */ +int xcetest_interrupted(void);