Investigation & fixes (failover committed-job loss): - root-cause bug-851 on an owned 3-node cluster: chaotic failover causes silent committed-prefix divergence (WAL hdr.term always 0; two leaders per term; no (term,lsn) reconcile). Evidence in repro-851/ + INVESTIGATION. - live-validated fixes 848/849/850 (recovery-on-promotion, advertise real applied_lsn, persist-layer contiguity guard); add waldump WAL inspector. Specs (strict raft.tla conformance — library owns the log): - TLA-gap-analysis.md: impl vs raft/raft.tla, gaps G1-G6. - raft-log-replication, raft-commit-safety, raft-sim-conformance: extend src/lib/raft to own log/commit/verification via callbacks (specs generated, cross-spec review passed). - ha-term-safety: controller-side adapter + config marker (R1-R7); design done. - config-upgrade-snapshot: R8 descoped here (deps entity-bearing-snapshots, bug-857). - roadmap + buglog (851/854/857) + .wolf knowledge updated.
30 KiB
Research Log: worker-join-refactor
Summary
Discovery scope: Light discovery (extension of existing C subsystem).
Key findings:
- Latent deadlock in current code:
handle_worker_joinacquireslock(c->workermgr, WRITE)then callsadd_worker → makeWorker, which also callslock(workermgr, WRITE).my_rwlock_tispthread_rwlock_t(non-reentrant), so this double-acquisition deadlocks. The outer lock was added for persistence-hook work on this branch and is the root cause of the "confusing locking structure." makeWorkerin wobj.c is out of scope but must still be called: It manages the create-or-update decision internally (makeWorker_). For the create path, the refactored design must NOT hold theworkermgrwrite lock when callingmakeWorker.override_workeralready preserves admin bits: TheXCE_WORKER_ADMIN_MASK(LOCKEDU | CLOSED | HIDDEN | CLOSED_BY_ADMIN) is preserved byoverride_workerin wobj.c. The rejoin update path must replicate this guarantee when updating attrs in-place, bypassingmakeWorker.
Research Log
Topic 1: Lock Reentrancy
Question: Is my_rwlock_t reentrant? Is the existing double-lock intentional?
Investigation: src/include/mylock.h defines my_rwlock_t as pthread_rwlock_t. POSIX pthread_rwlock_wrlock on an already-held write lock by the same thread is undefined behavior (most implementations deadlock). The outer write lock in handle_worker_join was introduced on branch persistent_ha for the AFTER_EVENT persist hook (the vget lookup comment confirms this). This is a newly introduced bug.
Implication: The refactored design must eliminate the outer workermgr write lock in handle_worker_join before any path reaches makeWorker.
Topic 2: Existence Check and Locking Strategy
Question: How should the explicit key_find check be locked?
Investigation: worker_ctrl uses lock(workermgr, READ) → key_find → reference(w) → unlock. Other paths use read locks for existence checks and upgrade to per-object write locks for mutation. The workermgr write lock is only needed for structural changes (insert/delete of entries), not for updating existing worker fields.
Decision: Use a brief READ lock for the key_find check. For the create path, delegate write locking to makeWorker. For the rejoin path, use a per-worker write lock.
TOCTOU note: A brief window exists between the read-lock check and makeWorker's write lock. This is benign: makeWorker_ calls key_find internally under the write lock and, if the worker was created by another thread between our check and our call, will call override_worker (update) instead of makeInstance (create). The behavior remains correct.
Topic 3: Rejoin Update — Replicating override_worker Semantics
Question: For the rejoin path (explicit update), how should attrs be updated in-place without calling override_worker (which is static in wobj.c)?
Investigation: override_worker in wobj.c:
- Saves admin bits (
status & XCE_WORKER_ADMIN_MASK) - Calls
kill_worker(frees name, attrs, dispatch_window, thresholds, resvals) - Calls
copy_worker(re-allocates from new src) - Restores admin bits and idx
For the rejoin path in worker.c, we can replicate this semantic without calling override_worker directly. The update needs to: free old hostattrs fields (hosttype/model strings), copy new attrs, update ep, and preserve admin bits on host.status. The existing dereference/makeString pattern is already used in add_worker for hosttype/model override, so the pattern is established.
Decision: Implement rejoin_worker in worker.c that replicates the admin-bit-preserving update logic inline, avoiding the static override_worker dependency.
Topic 4: Resource and Status Update — Shared Path
Question: Can the status/resource update logic in add_worker (req 4) be shared between create and rejoin paths?
Investigation: The current add_worker applies the following after makeWorker returns, under a per-worker write lock:
w->host.status &= ~XCE_WORKER_UNAVAILw->ep = ep- hconfig hosttype/model override
add_hostattrs_resvals(resource vals for type, model, cpuf, etc.)set_host_slots(mxj or maxcpus)dispatch_window_configcopythresholdscopy- bool resource marking loop
This block is identical for both create and rejoin. Extracting it into static int apply_join_state(cluster_t*, worker_t*, endpoint_t*, xce_hostconfig_t*) eliminates duplication.
Topic 5: Hook Firing and Lock Relationship
Question: Must BEFORE_EVENT and AFTER_EVENT hooks fire while the workermgr write lock is held?
Investigation: The current code fires both hooks under the outer workermgr write lock. However, mark_worker_unavail and mark_worker_back fire hooks while holding only the per-worker write lock (not the workermgr lock). The persist hook signature takes (c, w) — it reads from w directly, not through the workermgr index. There is no architectural requirement for the workermgr write lock to be held during hook dispatch.
Decision: Hooks fire in handle_worker_join outside any workermgr lock. BEFORE_EVENT fires before key_find. AFTER_EVENT fires after the create/update completes. This matches the behavioral contract from req 5.1.
Architecture Decision: Removing add_worker, Adding init_worker + rejoin_worker
The current add_worker is a monolithic function that internally calls makeWorker (which hides the create-or-update branch). The refactored design replaces it with:
static int apply_join_state(cluster_t*, worker_t*, endpoint_t*, xce_hostconfig_t*)— shared status/resource update logic (per-worker write lock)int init_worker(cluster_t*, endpoint_t*, const char*, hostattrs_t*, xce_hostconfig_t*)— first-time join: callsmakeWorker+apply_join_stateint rejoin_worker(cluster_t*, worker_t*, endpoint_t*, hostattrs_t*, xce_hostconfig_t*)— rejoin: updates attrs/ep in-place +apply_join_state
add_worker declaration in worker.h is replaced with init_worker and rejoin_worker.
Risk Assessment
| Risk | Likelihood | Mitigation |
|---|---|---|
| TOCTOU between key_find and makeWorker | Very low (same host reconnecting twice simultaneously) | makeWorker_ handles gracefully via override |
| Admin bits not preserved in rejoin path | Medium if not explicit | Explicit mask applied in rejoin_worker |
| Hook ordering regression | Low | Hooks remain in handle_worker_join, same relative order |
| Existing test breakage | Low | Tests exercise add_worker indirectly; signature change requires test fixture update |
Gap Analysis (2026-05-20)
Analysis Summary
- Scope: Narrow brownfield refactor — one C file (
worker.c) with three missing function bodies. All surrounding scaffolding is already in place. - Key challenge: None blocking; the design and test contract are locked. Only implementation is missing.
- Recommendation: Option A (extend existing
worker.c) is the only appropriate approach. - Effort: S (1–3 days). All design decisions and lock semantics resolved; code to write is a mechanical extraction from the existing
add_workerbody. - Risk: Low. Existing
add_workercontains the exact logic to extract. Tests intest_worker_join.cspecify every assertion.
Current Implementation State
| Component | File | Status |
|---|---|---|
handle_worker_join (rewritten) |
join.c |
✅ Done — explicit key_find branch, calls init_worker/rejoin_worker |
worker.h declarations |
include/worker.h |
✅ Done — init_worker, rejoin_worker exported; add_worker removed |
| TDD tests | testsuite/test_worker_join.c |
✅ Done — 7 tests for apply_join_state, init_worker, rejoin_worker |
| Test registered | testsuite/controller_test.c |
✅ Done — test_worker_join registered |
apply_join_state (static helper) |
worker.c |
❌ Missing — body not yet extracted |
init_worker |
worker.c |
❌ Missing — stub only; function body not yet written |
rejoin_worker |
worker.c |
❌ Missing — stub only; function body not yet written |
add_worker removal |
worker.c |
❌ Pending — old function body still present (unused by join.c) |
Requirement-to-Asset Map
| Requirement | Asset Needed | Status | Gap Tag |
|---|---|---|---|
| 1.1 Explicit key_find lookup | handle_worker_join explicit key_find |
✅ Present in join.c | — |
| 1.2 Branch on key_find result | init_worker/rejoin_worker dispatch in join.c |
✅ Present in join.c | — |
| 1.3 Reject non-ENOENT errors | Error branch in join.c | ✅ Present in join.c | — |
| 2.1 Create via makeWorker | init_worker calling makeWorker |
❌ Missing implementation | Missing |
| 2.2 Log failure, no on_worker_join | Error path in join.c | ✅ Present in join.c | — |
| 2.3 Assign priv->u.idx | join.c post-join assignment | ✅ Present in join.c | — |
| 3.1 Update hostattrs on rejoin | rejoin_worker attrs copy |
❌ Missing implementation | Missing |
| 3.2 Update ep on rejoin | rejoin_worker ep assignment |
❌ Missing implementation | Missing |
| 3.3 Preserve admin bits | XCE_WORKER_ADMIN_MASK in rejoin_worker |
❌ Missing implementation | Missing |
| 4.1 Clear UNAVAIL | apply_join_state status clear |
❌ Missing implementation | Missing |
| 4.2 Apply hconfig overrides | apply_join_state hosttype/model/slots |
❌ Missing implementation | Missing |
| 4.3 Update host-attr resources | apply_join_state calling add_hostattrs_resvals |
❌ Missing implementation | Missing |
| 4.4 Reject if hostconfig NULL | join.c fail-fast before lock | ✅ Present in join.c | — |
| 5.1 BEFORE/AFTER hook order | join.c hook call sites | ✅ Present in join.c | — |
| 5.2 on_worker_join still called | join.c post-join call | ✅ Present in join.c | — |
| 5.3 Critical section brevity | Locking model in join.c | ✅ Present in join.c | — |
| 5.4 Existing tests pass | Test suite build | ❌ Pending — add_worker still present, init/rejoin stubs fail tests | Constraint |
Implementation Approach Options
Option A: Extend Existing worker.c (Recommended)
Rationale: All design decisions are locked. The logic to implement already exists verbatim in add_worker. The task is extraction and reorganization:
- Add
apply_join_stateas a static helper — extract the~XCE_WORKER_UNAVAIL+ ep + hconfig override +add_hostattrs_resvals+set_host_slots+dispatch_window_config+thresholds+ bool resources loop from the bottom half ofadd_worker. - Implement
init_worker— build axce_worker_tfrom name/attrs, callmakeWorker, then callapply_join_stateunder per-worker write lock. - Implement
rejoin_worker— save admin bits, update attrs/ep in-place, callapply_join_state, restore admin bits. - Remove
add_worker(now dead code).
Files changed: src/controller/worker.c only.
Trade-offs:
- ✅ Minimal surface — one file, pure extraction
- ✅ Tests already written and specify exact postconditions
- ✅ Lock semantics fully resolved in design.md
- ❌ None — this is the obvious approach
Option B: New file (not appropriate)
These are not new responsibilities — they replace add_worker in the same file. Creating a new file adds navigation overhead with no benefit.
Option C: Hybrid (not applicable)
No external integration or phased rollout needed. Pure in-file refactor.
Implementation Complexity & Risk
- Effort: S (1–3 days) — logic is a mechanical extraction from
add_worker; all design decisions resolved - Risk: Low — no new patterns, no new dependencies, no new locking decisions; tests specify exact assertions
Recommendations for Design Phase
Design phase is already approved. Proceed directly to implementation:
- Implement
apply_join_statefirst (tasks 2 / task 1.1 green) - Implement
init_worker(task 3.1 / task 1.2 green) - Implement
rejoin_worker(task 3.2 / task 1.3 green) - Remove
add_worker(task 4.1 cleanup) - Run
make testin controller testsuite (task 5 validation)
No further research needed. All unknowns from the original research log are resolved in design.md.
Cross-Spec Gap Audit (2026-06-08)
Batch
/kiro-validate-gapacross 9 specs on persistent_ha branch.
Analysis Summary
- worker-join-refactor is ~100% complete. All 5 task groups, all subtasks (1.1–5) are
[x]in tasks.md. - Prior gaps RESOLVED:
init_worker(worker.c:339),rejoin_worker(worker.c:366), andapply_join_state(static helper) are all implemented.add_workerdeclaration removed fromworker.h. The double-lock deadlock (handle_worker_joinheldworkermgrWRITE lock whilemakeWorkerre-acquired it) is eliminated. - No remaining gaps:
handle_worker_joinnow has explicitkey_find→ branch →init_worker/rejoin_worker→apply_join_stateflow visible at the call site.BEFORE_EVENT/AFTER_EVENThooks fire at the same event points.on_worker_joincalled after successful join. Admin status bits (LOCKEDU | CLOSED | HIDDEN | CLOSED_BY_ADMIN) preserved inrejoin_worker. - Build verification: Run
/xce-build controllerto confirmtest_worker_join.c(7 test cases) pass. - Recommendation: The spec can be marked
implemented.
Gap Analysis — Validation Pass (2026-06-09)
Analysis Summary
- Status: Implementation is complete and committed (
93ed282 feat(worker-join-refactor): implement apply_join_state, init_worker, rejoin_worker). Working tree is clean for all spec-owned files. All tasks 1.1–5 are[x]in tasks.md. - Code verification: Every requirement was re-checked against the actual source on branch
persistent_ha.handle_worker_join(src/controller/join.c:33–119) now exposes the explicit check → create-or-update → status/resource-update flow;apply_join_state/init_worker/rejoin_workerare implemented in src/controller/worker.c:279–397;add_workeris fully removed (no remaining references anywhere in src/). - Only open item: Req 5.4 (full testsuite green) could not be re-confirmed in this pass — no build artifact exists in the worktree. One
/xce-build controllerrun is the remaining verification step.
Current State Investigation
| Asset | Location | Finding |
|---|---|---|
handle_worker_join |
src/controller/join.c:33 |
Rewritten. Fail-fast find_matched_hostconfig (no lock) → BEFORE_EVENT hook → brief workermgr READ lock + key_find + reference(w) → branch (-ENOENT→init_worker, found→rejoin_worker, other error→reject) → vget for AFTER_EVENT hook → xce_reply outside any lock → priv->u.idx set → on_worker_join. Outer workermgr WRITE lock is gone — the latent double-lock deadlock is eliminated. |
apply_join_state |
src/controller/worker.c:283–337 |
Static (exported under -DXCE_TESTING, which meson.build:148 sets for the test binary). Clears XCE_WORKER_UNAVAIL, sets w->ep, applies hosttype/model "!"-guarded overrides, add_hostattrs_resvals, mxj/maxcpus set_host_slots, dispatch-window + thresholds replacement, bool-resource loop. Precondition (per-worker WRITE lock held by caller) documented in comment. |
init_worker |
src/controller/worker.c:339–364 |
Builds temp xce_worker_t, calls makeWorker (which owns its own workermgr WRITE lock), -errno on NULL with ERROR log, then per-worker WRITE lock + apply_join_state, returns w->idx. |
rejoin_worker |
src/controller/worker.c:366–397 |
Per-worker WRITE lock; saves admin_status = status & XCE_WORKER_ADMIN_MASK (LOCKEDU|CLOSED|HIDDEN|CLOSED_BY_ADMIN, defined worker.c:275); dereference/makeString replacement of hosttype/hostmodel; copies cpufactor/maxcpus/maxmem/maxswap/maxtmp/ndisks; apply_join_state; restores admin bits; returns w->idx. |
worker.h |
src/controller/include/worker.h:11–14 |
init_worker and rejoin_worker exported with xce_hostconfig_t* param; add_worker declaration removed (only unrelated add_worker_to_xlist remains in cluster.h). |
| Tests | src/controller/testsuite/test_worker_join.c |
7 RUN_TEST cases (3 × apply_join_state, 2 × init_worker, 2 × rejoin_worker), registered in controller_test.c:154 and built via meson.build:103. |
| Out-of-scope guard | src/lib/wobj.c |
makeWorker/override_worker untouched by the refactor commit (wobj.c admin-mask logic at lines 85–105 predates it, commit febb4a6). Boundary respected. |
Requirement-to-Asset Map
| Req | Summary | Asset (file:line) | Status |
|---|---|---|---|
| 1.1 | key_find before create-or-update | join.c:75–79 (READ lock + key_find + reference) | Implemented |
| 1.2 | Branch on lookup result | join.c:81–97 (error / NULL / found branches) | Implemented |
| 1.3 | Reject non-ENOENT key_find error | join.c:81–84 (idx = rc, reply.status set, returned) |
Implemented |
| 2.1 | Create via makeWorker | worker.c:349 (makeWorker(c->workermgr, &tmp)) |
Implemented |
| 2.2 | Log failure, set reply status, skip on_worker_join | worker.c:350–353 + join.c:87–89, 107–112 (if (idx < 0) return idx before on_worker_join) |
Implemented |
| 2.3 | Assign priv->u.idx | join.c:115 (before on_worker_join) |
Implemented |
| 3.1 | Update hostattrs on rejoin | worker.c:376–386 (string + numeric attr copy) | Implemented |
| 3.2 | Update ep on rejoin | worker.c:294 (w->ep = ep inside apply_join_state, called from rejoin) |
Implemented |
| 3.3 | Preserve admin bits | worker.c:374, 390 (save mask / restore after apply_join_state) | Implemented |
| 4.1 | Clear XCE_WORKER_UNAVAIL | worker.c:293 | Implemented |
| 4.2 | Apply hconfig overrides (type/model/mxj/window/thresholds/bool res) | worker.c:296–336 | Implemented |
| 4.3 | Host-attr resource values | worker.c:308 (add_hostattrs_resvals) |
Implemented |
| 4.4 | Reject when hostconfig NULL, workermgr untouched | join.c:60–68 (fail-fast before BEFORE_EVENT hook and before any lock) | Implemented |
| 5.1 | BEFORE/AFTER hook order at same event points | join.c:71–72 (BEFORE, pre-key_find) and join.c:103–104 (AFTER, post-update, w via vget) | Implemented |
| 5.2 | on_worker_join after successful join (both paths) | join.c:116 (single post-branch call site) | Implemented |
| 5.3 | Critical section no longer than before | Locking narrowed vs. pre-refactor: brief READ for lookup, makeWorker-internal WRITE for create, per-worker WRITE for mutation; xce_reply outside all locks | Implemented |
| 5.4 | Existing testsuite passes unmodified | test_worker_join.c registered; no build dir in worktree to confirm a green run in this pass | Unknown — verify with /xce-build controller |
Constraints noted (no action required):
apply_join_statevisibility is toggled byXCE_TESTING(worker.c:282–288) — tests link against the non-static symbol. This is the established testsuite pattern; keep the meson-DXCE_TESTINGarg in sync.- TOCTOU window between READ-lock
key_findandmakeWorkerremains by design;makeWorker_degrades tooverride_worker(admin-bit-preserving sincefebb4a6), so behavior stays correct.
Minor observation (LOW, optional): in the join.c error branch (rc < 0 && rc != -ENOENT), if key_find ever returned an error while also setting w, the reference(w) taken at join.c:77 would not be released. Current key_find semantics make this unreachable (w stays NULL on error), so this is a defensive-coding nit only.
Implementation Approach Options
Option A: Close out the spec as-is (Recommended)
Verify req 5.4 with one /xce-build controller run; if green, mark the spec implemented in spec.json and archive.
- ✅ Zero code change; everything else is verified at source level
- ✅ Matches the prior audit conclusion (2026-06-08) — now re-confirmed line-by-line
- ❌ None
Option B: Add the defensive dereference in the key_find error branch before closing
One-line hardening (if (w) { dereference(w); w = NULL; } in the error branch) plus rebuild.
- ✅ Removes the theoretical reference leak
- ❌ Touches a committed, behavior-frozen handler for an unreachable path; out of the spec's acceptance criteria
Option C: Fold verification into the next controller-wide build
Skip a dedicated build; rely on the next /xce-build from any active spec (e.g., HA work on this branch) to exercise test_worker_join.
- ✅ No extra build cycle
- ❌ Spec closure blocked on unrelated work; a regression would be attributed late
Effort & Risk
- Effort: S — remaining work is one build/test run (Option A) or that plus a one-line nit (Option B); no design or implementation gaps remain.
- Risk: Low — pure verification of already-committed code following established patterns; all 16 functional criteria confirmed at source level; only the test-run evidence is missing.
Recommendations
- Run
/xce-build controllerto confirm the 7test_worker_joincases and the rest of the controller testsuite pass (closes Req 5.4). - On green, update
spec.jsonphase toimplemented(or run/kiro-verify-completion worker-join-refactor). - Optionally apply the Option B one-line defensive dereference if the file is touched again for other reasons; do not reopen the spec for it.
Research Needed
- None for this spec. Single carry-forward verification item: green
/xce-build controllerrun on current HEAD (worker.c/join.c unmodified since93ed282, but HA-branch changes to ha.c/raft.c in the working tree share the controller test binary — confirm no link/test interference).
Gap Re-Audit (2026-06-18)
Mode: POST-IMPLEMENTATION GAP AUDIT (spec.json phase=implemented; tasks 12/12 [x]). Method: EARS requirement → on-disk code verification via codegraph + targeted Read. Cites file:line at current HEAD. Scope reminder (roadmap line 26): this spec covers attribute/endpoint update on rejoin only. The job-state resync (running/finished jobs re-reported on rejoin) is the SEPARATE, not-yet-started spec worker-rejoin-resync (planned, no effort). That is explicitly NOT a gap of THIS spec — see Boundary note below.
Requirement-to-Asset Map
| Req | Statement (abbrev) | Asset (file:line) | Status |
|---|---|---|---|
| 1.1 | key_find before create-or-update |
handle_worker_join brief READ lock + key_find — join.c:91-95 |
CLOSED |
| 1.2 | Branch on lookup result (new vs exists) | if (w==NULL) init_worker … else rejoin_worker — join.c:101-113 |
CLOSED |
| 1.3 | Reject on non-ENOENT key_find error |
if (rc<0 && rc!=-ENOENT){ … idx=rc; } — join.c:97-100 |
CLOSED |
| 2.1 | Create new entry via makeWorker |
init_worker builds tmp xce_worker_t, calls makeWorker(c->workermgr,&tmp) — worker.c:339-354 |
CLOSED |
| 2.2 | On create fail: log, set status, no on_worker_join |
NULL→ERROR+return -errno; caller sets reply.status=idx, if(idx<0) return idx before on_worker_join — worker.c:351-353, join.c:104-105/138-147 |
CLOSED |
| 2.3 | Assign new idx to priv->u.idx |
priv->u.idx = idx; before on_worker_join — join.c:146 |
CLOSED |
| 3.1 | Update hostattrs on rejoin |
rejoin_worker frees+makeString hosttype/hostmodel, copies numeric attrs — worker.c:376-386 |
CLOSED |
| 3.2 | Update ep on rejoin |
apply_join_state sets w->ep = ep (worker.c:295), invoked by rejoin path — worker.c:388 |
CLOSED |
| 3.3 | Preserve admin status bits | admin_status = w->host.status & XCE_WORKER_ADMIN_MASK (worker.c:374) restored `w->host.status |
= admin_status` (worker.c:390); mask = LOCKEDU|CLOSED|HIDDEN|CLOSED_BY_ADMIN — worker.c:275-277 |
| 4.1 | Clear XCE_WORKER_UNAVAIL |
w->host.status &= ~XCE_WORKER_UNAVAIL; — worker.c:294 |
CLOSED |
| 4.2 | Apply hconfig overrides (type/model/mxj slots/dispatch window/thresholds/bool res) | apply_join_state worker.c:297-334: hosttype/model "!"-guarded override, set_host_slots(mxj, fallback maxcpus), dispatch_window dup, thresholds dup, mark_host_res loop |
CLOSED |
| 4.3 | Update host-attr resource values (type,model,cpuf,maxcpus,maxmem,maxswap,maxtmp,ndisks,slots) | add_hostattrs_resvals (worker.c:223-250, 8 attrs) + set_host_slots (slots) — called worker.c:307,312 |
CLOSED |
| 4.4 | Reject if find_matched_hostconfig==NULL, no workermgr mutation |
fail-fast before any lock: hconfig==NULL → reply.status=-errno/-EINVAL; return — join.c:77-84 |
CLOSED |
| 5.1 | BEFORE before mutation, AFTER after | HOOK_BEFORE_EVENT at join.c:87-88 (before key_find); HOOK_AFTER_EVENT at join.c:120-121 (after create/update, outside lock) |
CLOSED |
| 5.2 | on_worker_join after successful join (both paths) |
on_worker_join(c, ep) — join.c:147 (reached for both init and rejoin when idx>=0) |
CLOSED |
| 5.3 | Critical section as short as current | No outer workermgr WRITE lock; only brief READ lock for key_find (join.c:91-95); mutation under per-worker WRITE lock in init/rejoin (worker.c:356-358, 372-392) | CLOSED |
| 5.4 | Existing tests pass; add_worker removed |
Controller add_worker (old join path) deleted from worker.c & worker.h:11-14 (only init_worker/rejoin_worker declared); new test_worker_join.c registered in controller_test.c:37,166. NOTE: residual unrelated add_worker symbols exist — add_worker_to_xlist (misc.c:11) and worker-daemon add_worker (xcecd_worker_utils.c:12); both are distinct functions, not the refactored one |
CLOSED (verify build) |
Score: 16/16 acceptance criteria CLOSED in code. All 12 tasks have on-disk evidence. Design's planned architecture (explicit branch, no outer write lock, static apply_join_state, init_worker/rejoin_worker) matches the implementation exactly.
Implementation deltas vs design (not gaps — improvements/additions)
validate_hostgate added (join.c:65-72) — rejects unknown hosts with-EPERMbefore hostconfig lookup. Not in the original design; a correct hardening, no requirement conflict.persist_commit_barrieradded (join.c:129-136) — the success reply is gated on the join WAL entry being majority-committed (ha-commit-barrier-rollout Req 1.1-1.4). This is the cross-spec commit-durability integration; barrier failure becomes the error reply and skipson_worker_join, preserving Req 2.2/5.2 semantics. Landed after this spec's design was written; consistent with it.apply_join_statelinkage isstaticin production, non-staticunder#ifdef XCE_TESTING(worker.c:283-289) — enables the unit test to call it directly while keeping production encapsulation. Matches design intent.reference(w)/dereference(w)discipline around the found worker (join.c:93-94, 109) — refcount safety across the lock release between key_find and rejoin. Correct; not specified but required given the read-lock-then-release pattern.
Residual / Deferred Gaps
No open requirement gaps for this spec. All criteria are met in code. Residual items are verification-only or out-of-boundary:
- R1 (verification-only, carried from prior audit): green build/test on current HEAD. worker.c/join.c are unmodified by HA-branch work, but join.c now depends on
persist_commit_barrierand the controller test binary is shared with HA-branch changes (ha.c/raft.c) in the working tree. Action: run/xce-build controllerand confirmtest_worker_join(7 cases) plustest_commit_barrier_replypass and no link interference. Effort: S (~1 build). Risk: Low. - R2 (latent, MEDIUM — was flagged in prior audit body):
apply_join_stateresource-update failures are swallowed.update_resvalsreturns 0 unconditionally (worker.c:133-146; logs viaPERRORbut discards rc), andmark_host_resrc in theapply_join_stateloop (worker.c:333-334) is ignored. So Req 4.3 "update all resource values" can partially fail silently and the join still reports success. This MATCHES pre-refactor behavior (Error Handling table, design.md:423 — "log with PERROR; continue"), so it is not a regression and not a spec gap — it is a pre-existing design-accepted behavior. Logged here only as a known limitation. - R3 (out-of-boundary — DO NOT count as this spec's gap): worker job-state resync on rejoin.
rejoin_workerupdates attributes/endpoint/resources only. It does NOT recover running/finished job facts that were lost during the disconnect/failover window.xce_join_reqcarries onlyhostattrs_t(no job state). This is owned by the separate worker-rejoin-resync spec (roadmap line 26: "planned long ago, no effort yet"; dependencies: ha-consistency-invariants commit barrier + idempotency guards). Roadmap explicitly records this spec as "Extends: worker-join-refactor (implemented — covers attribute/endpoint update on rejoin only)." Boundary is clean and intentional.
Options for the one actionable open item (R1)
| Option | Description | Effort | Risk |
|---|---|---|---|
| A (recommended) | Run /xce-build controller on current HEAD; confirm test_worker_join + test_commit_barrier_reply green and no link interference with HA-branch ha.c/raft.c changes. Record result; close R1. |
S (~1 build cycle) | Low |
| B | Defer until next controller build for any reason; treat R1 as a standing CI gate rather than a spec action. | None now | Low (build drift risk if join.c's persist_commit_barrier dep changes upstream) |
| C | Additionally harden R2: make update_resvals/mark_host_res propagate rc so a join with failed resource setup fails loud (per fail-loud preference). Re-scopes Req 4.3 error semantics — would need a spec amendment since current behavior is design-accepted. |
M (touches worker.c shared helpers + tests; affects restore_worker/mark_worker_unavail callers) |
Medium (behavior change to a shared path; out of original scope) |
Recommendation: Option A. The spec is functionally complete (16/16 criteria CLOSED); only a build/test confirmation on the integrated HEAD remains. R2 is a pre-existing accepted behavior — pursue Option C only if a separate fail-loud hardening spec is opened. R3 belongs to worker-rejoin-resync.