fix(scheduler): plugin_end_session() guards on the wrong field (bug-1682/1492)

/xce-audit-invariants scheduler (dimension 1, driver state-equivalence,
re-run after scheduler-sim-live-allocation) found plugin_end_session()
still checked `plugin->begin_session_fn != NULL` before invoking
`plugin->end_session_fn(plugin)` -- a copy-paste of
plugin_begin_session()'s own (correct) guard. Two failure modes: a
plugin registering only end_session_fn silently never has it invoked;
a plugin registering only begin_session_fn crashes via a NULL
function-pointer call. Currently inert in production (no live plugin
hits either shape) but a real latent defect in exactly the
session-boundary code the driver-equivalence invariant governs.

This exact defect had been independently filed twice: bug-1492
(2026-08-04, scheduler-core task 5.2's review) and bug-1682
(2026-08-24, found adjacent to bug-1681's fix) -- neither investigation
cross-referenced the other. Both entries also carried a "fixed" tag
inconsistent with their own "not yet fixed" narrative text, caught by
this audit's independent code+text cross-check.

Fixed the one-line guard; added test_end_session_invokes_end_session_fn_
when_only_it_is_set() (test_plugin_hook_dispatch.c) proving the silent
non-invocation, confirmed RED (84/85) against the buggy guard and GREEN
(85/85) after the fix. Updated both buglog entries to FIXED with
cross-references, and corrected bug-1611's stale narrative to reflect
its narrowing (not elimination) by scheduler-sim-live-allocation.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC
This commit is contained in:
Rongsong Shen 2026-09-19 11:44:58 +00:00
parent 2f99406788
commit a643966f50
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
4 changed files with 63 additions and 11 deletions

File diff suppressed because one or more lines are too long

View file

@ -150,6 +150,7 @@
- **(2026-08-26) To produce a clean "before/after" pair of binaries for live E2E verification when OTHER unrelated files have uncommitted in-flight changes you must not touch or rebuild against**, use `git stash push -- <only the files for this fix>` (explicit pathspec, NOT a bare `git stash`) to isolate exactly the fix's own diff — rebuild for the "before" binary, `git stash pop` to restore the fix, rebuild again for the "after" binary. This leaves every other uncommitted file (the unrelated in-flight `ha.c`/`job.c`/`persist.c` work in this session) completely undisturbed throughout, unlike a bare `git stash` which would have stashed everything. For a PYTHON-only fix specifically, this same before/after swap works without any rebuild at all — just `cp` the two scheduler.py variants in and out of `src/xceadmin/src/xceadmin/` and re-run the SAME editable-install venv's `xceadmin` entry point; a `pip install -e` venv reads live from the source tree, no reinstall needed between swaps.
- **(2026-08-26, bug-1713) In this sandbox's local-loopback 3-node deploy, the FIRST-BOOT election-settling window varies wildly run to run (sometimes near-instant, sometimes ~10-12s via a term-1 election-timeout-then-PreVote-failover fallback path) — a fixed "wait for start-all.sh's health-check, then fire N requests" tight loop is NOT a reliable way to catch a transient `E_XCE_NOT_READY`/`E_XCE_NOMASTER` window: by the time start-all.sh's own 1s-granularity health-check first observes `CTRL_PRIMARY` in a log and returns, `ha_has_write_quorum()`'s `standby_endpoints` (populated synchronously inside `raft_cb_become_leader` from the raft library's own connected-voter set) is typically ALREADY satisfied, so the gate is already open. A REAL FAILOVER (kill -9 the current primary, then hammer the survivors with NO sleep, no health-check wait) reproduced the exact live symptom reliably instead — got a genuine bare `-10013` at ~t+8.6s post-kill on an unfixed client, and confirmed zero such failures surfaced across the equivalent window on the fixed client. The bug's own root-cause writeup already named this as an equally-valid trigger ("...or any time a real failover is in flight") — prefer the failover trigger over racing first-boot timing when a live repro is needed and cold-boot timing proves flaky.
- **(2026-08-26) A hand-rolled multi-connection mock TCP/TLS server in a test file needs a genuinely FINITE accept loop (`for handler in handlers:`), not `while self._handlers:` guarding a list that's never mutated — the latter is an always-true condition that leaves the server thread blocked in `accept()` indefinitely once the test's real client stops connecting, which does not fail the test but silently adds `thread.join(timeout=N)`'s FULL timeout to every such test's teardown (measured: 3 tests each added a flat 5s of pure teardown wait with zero effect on pass/fail).** Match the mock's handler-list length exactly to the number of connections the test actually expects (assert on `connection_count` for the "stops on its own budget" property instead of over-provisioning handlers "just in case") and additionally set a short `socket.settimeout()` on the listener as a safety net so a hypothetical future regression that causes MORE connections than expected still fails within one timeout window instead of silently reintroducing the same multi-second stall.
- **(2026-09-19, `/xce-audit-invariants scheduler` after `scheduler-sim-live-allocation`) A buglog entry's `tags` array can say `"fixed"` while its own `fix` text says `"Not yet fixed"` — a self-contradiction that survives silently unless something actually re-reads the entry, because most tooling in this project checks tags, not the narrative field.** Found on bug-1682 (and its unnoticed duplicate, bug-1492 — same defect, `plugin_end_session()` guarding on `begin_session_fn` instead of `end_session_fn`, independently filed twice 7 weeks apart because bug-1681's investigation never cross-referenced the earlier entry) via an audit subagent that read BOTH the code and the buglog text, not just the tags — it caught that `plugin.c:441` was still visibly broken despite the tags claiming otherwise. **Lesson 1**: when closing out a large feature and bulk-stamping buglog entries as fixed/superseded, verify each stamped entry's `fix` narrative actually matches the tag before writing it — a tag-only update is a claim with no evidence behind it. **Lesson 2**: before filing a new bug for a defect discovered "while fixing X," grep buglog.json for the exact symptom text first (e.g. `grep -i "begin_session_fn.*end_session_fn"`) — this exact one-line guard/call mismatch was independently re-discovered and re-filed as a "new" bug once already in this project's history.
## Do-Not-Repeat
- **A TDD-implementer SUBAGENT (spawned by a kiro-impl coordinator) has NO external wakeup mechanism — arming a `Monitor` tool call or a `Bash run_in_background` call and then ending the turn to "wait for a notification" just stops the subagent forever; the coordinator has to manually resume it.** (2026-08-26, xce-test-rc-file task 1.2, recurred across multiple tasks in this same spec before being called out explicitly.) Only the top-level session genuinely receives async task-notification events between turns; a subagent's turn ending IS the end of its execution for that dispatch. When a validation command (e.g. `controller_unit_test`) risks exceeding the default 120-300s Bash timeout, pass an explicit large `timeout` (e.g. 300000-600000ms) to a PLAIN FOREGROUND `Bash` call and let the tool call itself block until real output returns — never `run_in_background`, never `Monitor`, never end a turn on an unfinished command in this role. If a command is auto-moved to background because the timeout already elapsed, immediately re-run it in the foreground with a longer timeout rather than trying to "wait" for the backgrounded one.

View file

@ -438,7 +438,7 @@ int plugin_end_session(xce_sched_core_t* core)
break;
}
if (plugin->begin_session_fn != NULL) {
if (plugin->end_session_fn != NULL) {
plugin->end_session_fn(plugin);
}
}

View file

@ -282,6 +282,53 @@ static void test_plugin_register_sets_honest_core_reference(void)
TEST_ASSERT_EQUAL_PTR(&sched, plugin.sched);
}
/* ---------------------------------------------------------------------- */
/* bug-1682: plugin_end_session() guards its call with */
/* `plugin->begin_session_fn != NULL` instead of `plugin->end_session_fn != */
/* NULL` (plugin.c, a copy-paste of plugin_begin_session()'s own guard). */
/* Two failure modes this produces: (a) a plugin registering ONLY */
/* end_session_fn never has it invoked (silently skipped, this test); (b) a */
/* plugin registering ONLY begin_session_fn crashes via a NULL */
/* function-pointer call the first time a round ends (xce-audit-invariants, */
/* scheduler dimension 1, 2026-09-19 -- currently inert in production since */
/* no live plugin registers begin_session_fn without end_session_fn, but a */
/* latent defect in exactly the session-boundary code the driver- */
/* equivalence invariant governs). This test exercises failure mode (a), */
/* which is safely reproducible without crashing the test binary. */
/* ---------------------------------------------------------------------- */
static int g_end_session_calls;
static int mock_end_session_fn(void* arg)
{
(void)arg;
g_end_session_calls++;
return 0;
}
static void test_end_session_invokes_end_session_fn_when_only_it_is_set(void)
{
xce_sched_core_t core;
xce_sched_plugin_t plugin;
int rc;
core_fixture_init(&core);
make_mock_plugin(&plugin, "end_session_only");
/* Deliberately leave begin_session_fn NULL -- only end_session_fn is
* set, isolating plugin_end_session()'s own guard from
* plugin_begin_session()'s (already-correct) one. */
plugin.begin_session_fn = NULL;
plugin.end_session_fn = mock_end_session_fn;
rc = plugin_register_core(&core, &plugin);
TEST_ASSERT_TRUE(rc >= 0);
g_end_session_calls = 0;
rc = plugin_end_session(&core);
TEST_ASSERT_EQUAL_INT(0, rc);
TEST_ASSERT_EQUAL_INT(1, g_end_session_calls);
}
int main(void)
{
UNITY_BEGIN();
@ -289,5 +336,6 @@ int main(void)
RUN_TEST(test_dispatch_fanout_simulation_mode_shaped_container);
RUN_TEST(test_plugin_register_core_sets_honest_core_reference);
RUN_TEST(test_plugin_register_sets_honest_core_reference);
RUN_TEST(test_end_session_invokes_end_session_fn_when_only_it_is_set);
return UNITY_END();
}