add kiro skills

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

@@ -0,0 +1,192 @@
+# 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`.