# 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`.