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).