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.