myxce/.kiro/specs/worker-join-refactor/research.md

Research Log: worker-join-refactor

Summary

Discovery scope: Light discovery (extension of existing C subsystem).

Key findings:

1. Latent deadlock in current code: handle_worker_join acquires lock(c->workermgr, WRITE) then calls add_worker → makeWorker, which also calls lock(workermgr, WRITE). my_rwlock_t is pthread_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."
2. makeWorker in 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 the workermgr write lock when calling makeWorker.
3. override_worker already preserves admin bits: The XCE_WORKER_ADMIN_MASK (LOCKEDU | CLOSED | HIDDEN | CLOSED_BY_ADMIN) is preserved by override_worker in wobj.c. The rejoin update path must replicate this guarantee when updating attrs in-place, bypassing makeWorker.

---

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:

1. Saves admin bits (status & XCE_WORKER_ADMIN_MASK)
2. Calls kill_worker (frees name, attrs, dispatch_window, thresholds, resvals)
3. Calls copy_worker (re-allocates from new src)
4. 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_UNAVAIL
• w->ep = ep
• hconfig hosttype/model override
• add_hostattrs_resvals (resource vals for type, model, cpuf, etc.)
• set_host_slots (mxj or maxcpus)
• dispatch_window_config copy
• thresholds copy
• 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: calls makeWorker + apply_join_state
• int 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_worker body.
• Risk: Low. Existing add_worker contains the exact logic to extract. Tests in test_worker_join.c specify 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:

1. Add apply_join_state as 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 of add_worker.
2. Implement init_worker — build a xce_worker_t from name/attrs, call makeWorker, then call apply_join_state under per-worker write lock.
3. Implement rejoin_worker — save admin bits, update attrs/ep in-place, call apply_join_state, restore admin bits.
4. 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:

1. Implement apply_join_state first (tasks 2 / task 1.1 green)
2. Implement init_worker (task 3.1 / task 1.2 green)
3. Implement rejoin_worker (task 3.2 / task 1.3 green)
4. Remove add_worker (task 4.1 cleanup)
5. Run make test in 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-gap across 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), and apply_join_state (static helper) are all implemented. add_worker declaration removed from worker.h. The double-lock deadlock (handle_worker_join held workermgr WRITE lock while makeWorker re-acquired it) is eliminated.
• No remaining gaps: handle_worker_join now has explicit key_find → branch → init_worker/rejoin_worker → apply_join_state flow visible at the call site. BEFORE_EVENT/AFTER_EVENT hooks fire at the same event points. on_worker_join called after successful join. Admin status bits (LOCKEDU | CLOSED | HIDDEN | CLOSED_BY_ADMIN) preserved in rejoin_worker.
• Build verification: Run /xce-build controller to confirm test_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_worker are implemented in src/controller/worker.c:279–397; add_worker is 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 controller run 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_state visibility is toggled by XCE_TESTING (worker.c:282–288) — tests link against the non-static symbol. This is the established testsuite pattern; keep the meson -DXCE_TESTING arg in sync.
• TOCTOU window between READ-lock key_find and makeWorker remains by design; makeWorker_ degrades to override_worker (admin-bit-preserving since febb4a6), 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

1. Run /xce-build controller to confirm the 7 test_worker_join cases and the rest of the controller testsuite pass (closes Req 5.4).
2. On green, update spec.json phase to implemented (or run /kiro-verify-completion worker-join-refactor).
3. 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 controller run on current HEAD (worker.c/join.c unmodified since 93ed282, but HA-branch changes to ha.c/raft.c in the working tree share the controller test binary — confirm no link/test interference).