diff --git a/.kiro/specs/scheduler-core/tasks.md b/.kiro/specs/scheduler-core/tasks.md index 6ea97161..84e1152f 100644 --- a/.kiro/specs/scheduler-core/tasks.md +++ b/.kiro/specs/scheduler-core/tasks.md @@ -408,3 +408,4 @@ - 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. - Task 12.1's review found a licensing gap (fixed): the readline()-based REPL input loop originally linked GNU readline (GPL) directly into `xce_scheduler`, a shipped production daemon (unlike `xce_test`, a debug-only binary controller's own readline use was already confined to). Switched project-wide to libedit (BSD) — `nixSupport/xce.nix`, both `meson.build`s, `repl.c`, and `src/controller/testsuite/test.c` now use `editline/readline.h` (API-compatible: `readline()`/`add_history()`), no hand-written accessor changes needed anywhere. - Task 12.1's review also surfaced bug-1504 (fixed) and bug-1505 (deferred, out of boundary) — see `.wolf/buglog.json`. Verifying the licensing fix required a full clean rebuild (new nix dependency), which exposed a genuine, pre-existing, deterministic SIGSEGV in `write_SnapshotData` unrelated to task 12.1's own repl.c: `SnapshotData.internalState` was a capnp `Data` field, and this codebase's capnpc-c backend silently no-ops Data-field encoding in generated `encode_XXX()` helpers without zero-initializing the caller's stack struct first — a real, reproducible UB crash, not a flaky test. Fixed (bug-1504) by switching `internalState` to `List(UInt8)` (controller.capnp, typemap.h, test_e2e_capnp.c) — this same capnpc-c backend DOES correctly auto-generate real encode/decode for `List(UInt8)` (proven precedent: `FetchFileReply.content`/`bytesRead`), eliminating the whole no-op class for this field with no hand-written accessor code. Re-verified with a full clean rebuild: scheduler suite 34/34, no regressions. While chasing this, also hit a **second, unrelated instance of the same bug class** in the controller's HA/WAL-catchup subsystem (`WalCatchupEntry.payload`, bug-847's own territory) — confirmed systemic (affects any `:Data` field in `controller.capnp` fed through the generic codec, not a one-off) but deliberately **left unfixed** (bug-1505) as out of scheduler-core's boundary; needs its own project-wide `:Data`-field audit as a separate task. +- bug-1505 fixed (2026-08-07, follow-up outside any specific task's boundary — controller/HA subsystem, not scheduler-core): before fixing, confirmed zero production exposure — `xce_wal_batch_encode_raw()`/`wal_entry_to_wire()` (`request.c`) never call the generic `encode_WalCatchupEntry()` at all; they `memset()` their own struct and call `write_WalCatchupEntry()` directly with every field hand-set, so the uninitialized-`capn_ptr` crash was 100% test-only exposure (only `test_e2e_capnp.c` calls the generic path). Fixed the same way as bug-1504: `WalCatchupEntry.payload` (@2) and `batchPayloads` (@10) switched from `Data` to `List(UInt8)` (new `batchPayloadsLen` @11 companion; `payload` reuses the existing `wirePayloadLen` @9), `typemap.h`'s `wal_catchup_entry_t` field types updated, `request.c`'s `extract_data_bytes()` widened to accept a raw `capn_ptr` (it already only used `.p` internally) so its 4 call sites work regardless of which Data-shaped wrapper the accessor returns now. Verified `batchPayloads`' single-top-level-blob shape means this does NOT reintroduce bug-847's per-element cross-segment far-pointer loss (that issue is inherent to PER-ELEMENT data regardless of `Data` vs `List(UInt8)` spelling — `payload`, the per-element field, was already legacy/unpopulated in production before this fix). Strengthened `test_wal_catchup_append_term_fields_round_trip` with real payload/batchPayloads content assertions (previously never exercised — the test's `src.payload`/`batch_payloads` were left NULL, so the crash occurred on content the test didn't even care about). Verified: `controller_unit_test` no longer crashes (3/3 controller suite), scheduler suite unaffected (37/37, unrelated schema area). diff --git a/.wolf/buglog.json b/.wolf/buglog.json index 50a1ee16..e438a61b 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -14821,7 +14821,7 @@ "error_message": "controller_unit_test SIGSEGV in write_WalCatchupEntry (lib/controller.capnp.c), test_wal_catchup_append_term_fields_round_trip -- discovered incidentally while chasing bug-1504; NOT fixed, logged for its own follow-up.", "file": "src/schema/controller.capnp", "root_cause": "Same root cause as bug-1504 (Data-field codegen no-op leaves an uninitialized stack capn_ptr in the generated encode_XXX_ptr()'s local struct), a DIFFERENT field: WalCatchupEntry.payload (@2: Data), in the controller's HA/WAL-catchup subsystem (bug-847's own territory -- see controller.capnp's comment there), entirely unrelated to scheduler-core. Confirms this is a systemic, project-wide bug class affecting every `:Data` field in controller.capnp whose bytes are populated via the generic encode_XXX()/write_XXX() composite pipeline rather than fully hand-written low-level accessor code -- not a one-off. Known/suspected other Data fields with the same latent gap: ReplayHeader.configHash (dormant so far, no real consumer), CtrlHelloRequest.key, and possibly others not yet audited.", - "fix": "NOT FIXED. Deliberately deferred -- out of scheduler-core task 12.1's boundary (this is the controller's HA/WAL-replication subsystem), and a proper fix requires a project-wide audit of every `:Data` field in controller.capnp to decide case-by-case: convert to List(UInt8) (bug-1504's fix, when the field is genuinely a flat byte blob with no cross-segment/per-element concerns) vs. keep Data with real hand-written low-level accessor code at every encode/decode call site (matching the batchPayloads precedent, when List(UInt8) risks bug-847's per-element far-pointer loss for repeated/batch structures). Needs its own scoped task, not a one-off patch.", + "fix": "FIXED 2026-08-07. Investigated production exposure first: xce_wal_batch_encode_raw()/wal_entry_to_wire() (request.c) never call the generic encode_WalCatchupEntry() at all -- they memset() their own struct and call write_WalCatchupEntry() directly with every field hand-set, deliberately leaving payload unset (bug-847's own comment). So this bug was 100% test-only exposure, zero production risk, confirmed by grepping every caller of encode_WalCatchupEntry_ptr/decode_WalCatchupEntry (only test_e2e_capnp.c uses the generic path). Fix: same as bug-1504 -- changed WalCatchupEntry.payload (@2) and batchPayloads (@10) from Data to List(UInt8) in controller.capnp, with batchPayloadsLen (@11, new) as batchPayloads' maplistcount companion (payload reuses the existing wirePayloadLen @9). Verified batchPayloads' own top-level-single-blob shape means this does not reintroduce bug-847's per-element cross-segment far-pointer loss (that issue is inherent to PER-ELEMENT data regardless of Data vs List(UInt8) spelling -- payload, the per-element field, was already legacy/unpopulated in production before this fix and stays that way; batchPayloads is single top-level, never subject to that issue either way). Updated typemap.h's wal_catchup_entry_t field types (xce_data_t* -> uint8_t*, new batch_payloads_wire_len). Fixed request.c's extract_data_bytes() to accept a raw capn_ptr instead of capn_data (it already only ever used .p internally) so all 4 call sites (WalCatchupEntry.payload/batchPayloads, SnapshotTransfer.payload, per-sub-entry we.payload) work regardless of which Data-shaped wrapper type the accessor returns. Strengthened test_wal_catchup_append_term_fields_round_trip (test_e2e_capnp.c) with real payload/batchPayloads content assertions (previously never exercised at all, since the test's src.payload/batch_payloads were left NULL). Verified: controller_unit_test no longer crashes (3/3 controller suite), scheduler suite unaffected (37/37, unrelated schema area).", "tags": [ "controller", "ha", @@ -14839,7 +14839,7 @@ "bug-847" ], "occurrences": 1, - "last_seen": "2026-08-07T08:10:00Z" + "last_seen": "2026-08-07T09:15:00Z" }, { "id": "bug-1506", diff --git a/.wolf/cerebrum.md b/.wolf/cerebrum.md index b605e186..c12ae974 100644 --- a/.wolf/cerebrum.md +++ b/.wolf/cerebrum.md @@ -722,3 +722,9 @@ Root-caused a `committed_job_loss=1` chaotic-soak FAIL entirely from logs/timest ## Key Learning (2026-08-07, scheduler-core task 12.4) — a RED-phase probe for a "REPL never alters WHAT is processed, only WHEN" invariant must corrupt CONTENT, not TIMING: defeating repl_should_pause() (task 12.1's own already-verified probe, which flips the `step N` pause decision) does NOT turn a REPL-vs-batch decision-sequence equivalence test red, because it only changes when stdin is read/control returns — every session still eventually runs to completion regardless of pause schedule, and any script built on CONTINUE's sticky non-pausing behavior tolerates arbitrary pause timing. The correct probe has to touch a hook that can affect content and fires ONLY under `sim->interactive`: `xce_repl_on_event(sim, ev)` (repl_break.c) takes a non-const `xce_core_event_t*` and is called strictly before the event joins the batch `xce_core_push_events()` later applies — a one-line mutation there (e.g. corrupting a JOBS_PUSH job's jobid) is invisible to batch mode entirely and produces a genuine, real divergence in the recorded output file. Confirmed the probe correctly also failed 3 OTHER pre-existing REPL tests that exercise JOBS_PUSH under interactive mode (test_repl_execution_control.c/test_repl_snapshot_params.c/test_repl_breakpoints_inspection.c) — a real production bug at this hook site would legitimately be caught by the whole REPL test family, not just one file, which is expected and reassuring, not a signal of over-broad blast radius from the probe itself. ## Decision Log (2026-08-07, scheduler-core task 12.4) — task 12.4's "byte-identical decision sequences" is a DIFFERENT comparison than task 11.6's test_sim_equivalence.c: 11.6 compares the NORMAL-mode driver against the SIMULATION driver on the same recorded fixture; 12.4 compares the SAME simulation driver (sched_sim_driver_run()) against itself, run twice on the identical `.replay` fixture, differing only in `sim->interactive` (REPL-driven via scripted stdin vs straight batch). Both runs set `sched_sim_args_t.output_path` directly (confirmed unconditional on `interactive` — sim.c's fopen() gate has no interactive check), so no `set output` REPL command is needed; multi-session fixtures (more than one ReplaySession boundary, needed to meaningfully exercise BOTH `step` and `continue` per the task's literal text) require scanning the whole output file structurally for ReplaySession-shaped messages (sim.c's shared msg_root_has_shape(), each `.replay` message type has a distinct (word,ptr) struct shape) rather than reusing test_sim_equivalence.c's fixed-skip-count decode technique (valid only for exactly one session). + +## Key Learning (2026-08-07, bug-1505 fix) — before fixing a capnp Data-field no-op crash (see the earlier Key Learning on this bug class, scheduler-core task 12.1), always check whether ANY production caller reaches the generic `encode_XXX_ptr()` composite path at all, before assuming the fix is production-critical. `xce_wal_batch_encode_raw()`/`wal_entry_to_wire()` (`src/lib/request.c`) `memset()` their own wire struct then hand-write every field and call `write_WalCatchupEntry()` DIRECTLY (bypassing `encode_WalCatchupEntry()`/`encode_WalCatchupEntry_ptr()` entirely) — this codebase's own established pattern for any Data-shaped field it actually needs to populate correctly (also the pattern `xce_wal_batch_encode_raw()`'s own comment cites: "Bypasses encode_WalCatchupEntry ... Builds the Request manually"). `memset()`-first is what makes this safe: an unset field degrades to a legitimate null `capn_ptr`, not garbage. Grep every caller of the generic `encode_X_ptr`/`decode_X` pair before fixing — if only test code calls it, the fix is test-hygiene + defense-in-depth, not an emergency; still worth fixing (a future caller could hit it), but changes the urgency/framing when reporting to the user. + +## Key Learning (2026-08-07, bug-1505 fix) — when converting a capnp field from `Data` to `List(UInt8)` (the bug-1504/1505 fix pattern) fixes a field ALSO passed through hand-written low-level accessor code elsewhere (e.g. `extract_data_bytes(capn_data dp, ...)` in `request.c`, called on multiple Data-shaped fields including ones NOT being converted), the accessor's parameter type must widen too. `capn_data`/`capn_list8` are both trivial single-field `{ capn_ptr p; }` wrappers with zero behavioral difference — the cleanest fix is changing the accessor to take a raw `capn_ptr` and updating every call site to pass `.p` explicitly, rather than picking one wrapper type and forcing the other call sites to construct a throwaway wrapper struct just to satisfy the compiler. + +## Do-Not-Repeat (2026-08-07, bug-1505 fix) — before assuming a `List(UInt8)` conversion for a PER-ELEMENT field (inside a `List(Struct)`) is safe from bug-847's cross-segment far-pointer loss, re-read that bug's own root cause carefully: it's about per-element Data/List(UInt8) fields losing their far-pointer when a multi-entry batch spans capnp segments — a property of being PER-ELEMENT, completely independent of whether the schema spells the field `Data` or `List(UInt8)` (both are the identical wire type in capnp; the keyword only changes whether THIS project's capnpc-c backend auto-generates real composite encode/decode for it or silently no-ops it). Converting a per-element field's spelling does not reintroduce or worsen bug-847 — but also does not fix it. Only a genuinely TOP-LEVEL single blob (like `batchPayloads`) is inherently immune to that specific cross-segment issue, which is why bug-847's own original fix routed traffic through a top-level field instead of trying to "fix" the per-element one. diff --git a/src/controller/testsuite/test_e2e_capnp.c b/src/controller/testsuite/test_e2e_capnp.c index 7e3e2c54..34a71e06 100644 --- a/src/controller/testsuite/test_e2e_capnp.c +++ b/src/controller/testsuite/test_e2e_capnp.c @@ -195,11 +195,21 @@ static void test_request_vote_last_log_tuple_round_trip(void) /* ------------------------------------------------------------------ */ /* Test 5: WalCatchupEntry (replication envelope) round-trips the */ /* AppendEntries term fields (prev_log_index, prev_log_term, term) */ -/* (ha-term-safety task 1.1, Req 1.3/1.4). */ +/* (ha-term-safety task 1.1, Req 1.3/1.4), AND (bug-1505 fix) genuinely */ +/* round-trips payload/batchPayloads content through the GENERIC codec */ +/* now that both are List(UInt8) instead of Data -- this is the exact */ +/* path that used to SIGSEGV (write_WalCatchupEntry writing an */ +/* uninitialized capn_ptr for a no-op'd Data field) even though this */ +/* test never previously touched payload content at all (src.payload */ +/* stayed NULL); see controller.capnp's WalCatchupEntry.payload comment */ +/* for the full root cause. */ /* ------------------------------------------------------------------ */ static void test_wal_catchup_append_term_fields_round_trip(void) { + static const uint8_t entry_payload[] = { 0x11, 0x22, 0x33, 0x44, 0x55 }; + static const uint8_t batch_payload[] = { 0xAA, 0xBB, 0xCC, 0xDD }; + wal_catchup_entry_t src; memset(&src, 0, sizeof(src)); src.lsn = 42ULL; @@ -207,6 +217,12 @@ static void test_wal_catchup_append_term_fields_round_trip(void) src.term = 7ULL; /* NEW: this entry's term stamp */ src.prev_log_index = 41ULL; /* NEW: lsn - 1 */ src.prev_log_term = 6ULL; /* NEW: term at prev_log_index */ + /* bug-1505 fix: real content through the generic codec (List(UInt8), + * not Data) for both payload and batchPayloads. */ + src.payload = (uint8_t *)entry_payload; + src.wire_payload_len = (uint32_t)sizeof(entry_payload); + src.batch_payloads = (uint8_t *)batch_payload; + src.batch_payloads_wire_len = (uint32_t)sizeof(batch_payload); struct capn c; capn_init_malloc(&c); @@ -238,6 +254,20 @@ static void test_wal_catchup_append_term_fields_round_trip(void) TEST_ASSERT_EQUAL_UINT64(41ULL, dst.prev_log_index); /* survives the wire */ TEST_ASSERT_EQUAL_UINT64(6ULL, dst.prev_log_term); /* survives the wire */ + /* bug-1505 fix: payload/batchPayloads now genuinely round-trip through + * the generic codec (List(UInt8), not Data) -- this is the content the + * old no-op'd Data-field codec never actually carried. */ + TEST_ASSERT_EQUAL_UINT32((uint32_t)sizeof(entry_payload), + dst.wire_payload_len); + TEST_ASSERT_NOT_NULL(dst.payload); + TEST_ASSERT_EQUAL_MEMORY(entry_payload, dst.payload, sizeof(entry_payload)); + + TEST_ASSERT_EQUAL_UINT32((uint32_t)sizeof(batch_payload), + dst.batch_payloads_wire_len); + TEST_ASSERT_NOT_NULL(dst.batch_payloads); + TEST_ASSERT_EQUAL_MEMORY(batch_payload, dst.batch_payloads, + sizeof(batch_payload)); + capn_free(&d); free(buf); printf("test_wal_catchup_append_term_fields_round_trip: PASS\n"); diff --git a/src/include/typemap.h b/src/include/typemap.h index 4f47d537..8db7e5a5 100644 --- a/src/include/typemap.h +++ b/src/include/typemap.h @@ -385,8 +385,14 @@ struct snapshot_transfer_s { struct wal_catchup_entry { uint64_t lsn; uint8_t entry_type; - xce_data_t *payload; /* legacy decoded field — broken for - * raw bytes; kept NULL, do not use */ + /* bug-1505 fix: List(UInt8) now, not Data (see controller.capnp's + * WalCatchupEntry.payload comment for the full root cause) — a real + * uint8_t* + wire_payload_len pair via the generic codec, same shape + * bytes_read/content already establish elsewhere. Still legacy/unused + * by production (wal_entry_to_wire() in request.c deliberately never + * sets it — see that comment); this genuinely round-trips now instead + * of being a documented-broken placeholder, but nothing reads it. */ + uint8_t *payload; int n_entries; /* batch: number of sub-entries */ struct wal_catchup_entry **entries; /* batch: array of sub-entry pointers */ /* Raw WAL entry payload bytes — same pattern as repl_config_push_req_t. @@ -421,15 +427,23 @@ struct wal_catchup_entry { * send; used on decode to slice this entry's bytes out of the TOP-LEVEL * container's batch_payloads blob (below). */ uint32_t wire_payload_len; - /* bug-847: generic decoded placeholder for wire field batchPayloads @10 — - * same broken-for-raw-bytes limitation as `payload` above; kept NULL, do - * not use. Only meaningful on the TOP-LEVEL container (never on a nested - * sub-entry). The real bytes are hand-extracted by xce_decode() into - * raw_batch_payloads/raw_batch_payloads_len below, then sliced per + /* bug-1505 fix: List(UInt8) now, not Data (controller.capnp's + * WalCatchupEntry.batchPayloads comment has the full root cause — + * same class as `payload` above and SnapshotData.internalState, + * bug-1504). Generic decoded field for wire field batchPayloads @10; + * genuinely round-trips now via the generic codec, but still unused by + * production — the real bytes are hand-extracted by xce_decode() into + * raw_batch_payloads/raw_batch_payloads_len below (a separate, + * already-correct code path this fix does not touch), then sliced per * sub-entry using each entry's wire_payload_len into that entry's OWN * raw_payload/raw_payload_len (so every consumer keeps reading - * raw_payload exactly as before — this is purely a wire-transport fix). */ - xce_data_t *batch_payloads; + * raw_payload exactly as before). Only meaningful on the TOP-LEVEL + * container (never on a nested sub-entry). batch_payloads_wire_len is + * its own maplistcount companion (wire field batchPayloadsLen @11) — + * distinct from raw_batch_payloads_len below, which belongs to the + * separate hand-rolled extraction path. */ + uint8_t *batch_payloads; + uint32_t batch_payloads_wire_len; uint8_t *raw_batch_payloads; size_t raw_batch_payloads_len; }; diff --git a/src/lib/request.c b/src/lib/request.c index 3c22716d..f3b972e7 100644 --- a/src/lib/request.c +++ b/src/lib/request.c @@ -56,16 +56,25 @@ int xce_encode(xce_request_t* req, io_packet_t* pkt) return 1; } -/* extract_data_bytes: copy a capnp Data (List(UInt8)) field into a fresh +/* extract_data_bytes: copy a capnp Data/List(UInt8) field into a fresh * malloc'd buffer. Returns NULL (*out_len = 0) for absent/empty fields or * allocation failure. Used by xce_decode() to pre-extract raw payload - * bytes before capn_free() destroys the segment. */ -static uint8_t *extract_data_bytes(capn_data dp, size_t *out_len) + * bytes before capn_free() destroys the segment. + * + * Takes a raw capn_ptr rather than capn_data/capn_list8 (bug-1505 fix): + * both wrapper types are a single `.p` field with identical layout — this + * function already only ever touched `.p` — so accepting the shared + * underlying type lets every call site pass either wrapper's `.p` directly, + * regardless of whether the source schema field is spelled `Data` (still + * SnapshotTransfer.payload) or `List(UInt8)` (WalCatchupEntry.payload/ + * batchPayloads, converted from Data by bug-1505's schema fix — see + * controller.capnp's WalCatchupEntry comment for why). */ +static uint8_t *extract_data_bytes(capn_ptr p, size_t *out_len) { *out_len = 0; capn_list8 l; - l.p = dp.p; + l.p = p; if (l.p.type == CAPN_NULL) return NULL; @@ -199,7 +208,7 @@ int xce_decode(io_packet_t* pkt, xce_request_t* req) /* Top-level payload (single-entry mode) */ batch->raw_payload = - extract_data_bytes(WalCatchupEntry_get_payload(b_ptr), + extract_data_bytes(WalCatchupEntry_get_payload(b_ptr).p, &batch->raw_payload_len); /* bug-847: the top-level batch_payloads blob — every sub-entry's @@ -208,7 +217,7 @@ int xce_decode(io_packet_t* pkt, xce_request_t* req) * exhibit the per-element cross-segment pointer loss that scattered * per-sub-entry Data blobs did. */ batch->raw_batch_payloads = - extract_data_bytes(WalCatchupEntry_get_batchPayloads(b_ptr), + extract_data_bytes(WalCatchupEntry_get_batchPayloads(b_ptr).p, &batch->raw_batch_payloads_len); /* ha-term-safety task 3.2: the top-level container's AppendEntries @@ -249,7 +258,7 @@ int xce_decode(io_packet_t* pkt, xce_request_t* req) } } else { batch->entries[i]->raw_payload = - extract_data_bytes(we.payload, + extract_data_bytes(we.payload.p, &batch->entries[i]->raw_payload_len); } } @@ -263,7 +272,7 @@ int xce_decode(io_packet_t* pkt, xce_request_t* req) snapshot_transfer_t *chunk = req->body.snapshottransfermsg; SnapshotTransfer_ptr s_ptr = rq.body.snapshottransfermsg; chunk->raw_payload = - extract_data_bytes(SnapshotTransfer_get_payload(s_ptr), + extract_data_bytes(SnapshotTransfer_get_payload(s_ptr).p, &chunk->raw_payload_len); } diff --git a/src/schema/controller.capnp b/src/schema/controller.capnp index b103b069..9a7e12e0 100644 --- a/src/schema/controller.capnp +++ b/src/schema/controller.capnp @@ -954,7 +954,29 @@ struct SnapshotTransfer $C.mapname("snapshot_transfer_t") { struct WalCatchupEntry $C.mapname("wal_catchup_entry_t") { lsn @0 :UInt64 $C.mapname("lsn"); entryType @1 :UInt8 $C.mapname("entry_type"); - payload @2 :Data $C.mapname("payload"); + # payload is List(UInt8), NOT Data (bug-1505 fix, same class as bug-1504 on + # SnapshotData.internalState -- see that struct's own comment for the full + # root cause: this codebase's capnpc-c backend silently no-ops every Data + # field's encoding in generated encode_XXX() helpers without zero- + # initializing the caller's local struct first, leaving a genuinely + # uninitialized capn_ptr written onto the wire -- a real, reproducible + # SIGSEGV in write_WalCatchupEntry, hit by + # test_wal_catchup_append_term_fields_round_trip even though that test + # never touches payload content at all (src.payload stays NULL -- the + # crash is from the OUTPUT struct's uninitialized field, independent of + # the input). List(UInt8) gets a real generated encode/decode from this + # same backend (proven precedent: FetchFileReply.content/bytesRead), so + # no hand-written accessor is needed. Reuses wirePayloadLen (@9, below) + # as its maplistcount companion -- already exactly "this sub-entry's + # payload byte count" for the bug-847 slicing use below, same value. + # NOTE: production (xce_wal_batch_encode_raw/wal_entry_to_wire, request.c) + # never goes through this generic encode_WalCatchupEntry() path at all -- + # it memset()s its own struct and calls write_WalCatchupEntry() directly + # with every field hand-set (payload deliberately left at its post-memset + # zero value, per bug-847's own comment below) -- so this fix has zero + # production behavior impact, it only stops the generic codec (used by + # this one test) from crashing. + payload @2 :List(UInt8) $C.mapname("payload") $C.maplistcount("wire_payload_len"); entries @3 :List(WalCatchupEntry) $C.mapname("entries") $C.maplistcount("n_entries"); # Raft AppendEntries term fields (ha-term-safety task 1.1, Req 1.3/1.4). # This envelope is the replication-carrying message (WAL catch-up); the four @@ -996,8 +1018,25 @@ struct WalCatchupEntry $C.mapname("wal_catchup_entry_t") { # n_entries (0/1/N); sub-entries no longer carry their own @2 payload on # this path (kept for any other/legacy sender; decode still handles it # defensively as a fallback when batchPayloads is absent). + # + # NOTE (bug-1505 fix, same as `payload` above): this note's own "cross- + # segment loss" concern is about scattered PER-ELEMENT Data (or + # List(UInt8) -- identical wire representation either way in this + # codegen; the Data vs List(UInt8) schema keyword only changes whether + # THIS backend's generic encode_XXX() composite helper populates the + # field automatically, not the underlying wire type). batchPayloads + # itself is the single TOP-LEVEL blob this fix already routes around that + # exact problem with -- so switching ITS OWN keyword to List(UInt8) does + # not reintroduce bug-847's issue; it only stops the generic codec's + # SnapshotData.internalState-class no-op/uninitialized-capn_ptr crash + # (bug-1504) for this field too. batchPayloadsLen (@11, below) is its + # maplistcount companion, distinct from raw_batch_payloads_len + # (typemap.h) -- that field is populated by request.c's OWN hand-rolled + # xce_decode() extraction (a completely separate, already-correct code + # path this fix does not touch), not by this generic decode. wirePayloadLen @9 :UInt32 $C.mapname("wire_payload_len"); - batchPayloads @10 :Data $C.mapname("batch_payloads"); + batchPayloads @10 :List(UInt8) $C.mapname("batch_payloads") $C.maplistcount("batch_payloads_wire_len"); + batchPayloadsLen @11 :UInt32 $C.mapname("batch_payloads_wire_len"); } struct CtrlHelloRequest $C.mapname("ctrl_hello_req_t") {