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

# Design Document: worker-join-refactor

## Overview

This refactor restructures the worker join path in the XCE controller to make the check → create-or-update → status/resource-update sequence explicit and readable at the `handle_worker_join` call site.

The current implementation delegates the create-or-update decision to `add_worker`, which in turn delegates it to `makeWorker` in `wobj.c`. This hides the branching and introduces a latent deadlock: `handle_worker_join` holds the `workermgr` write lock while calling `add_worker → makeWorker`, which attempts to re-acquire the same lock. Since `my_rwlock_t` is `pthread_rwlock_t` (non-reentrant), this deadlocks. The refactor eliminates the outer lock, makes existence-checking visible at the join handler, and introduces two narrowly-scoped helpers for the create and rejoin paths.

### Goals

- Make the check → create-or-update → status/resource-update control flow visible in `handle_worker_join`
- Eliminate the double workermgr write lock acquisition (latent deadlock fix)
- Preserve all observable behavior: hooks, `on_worker_join`, admin-bit preservation

### Non-Goals

- Changes to `makeWorker` or any other function in `src/lib/wobj.c`
- Changes to other join handlers (`handle_client_join`, `handle_scheduler_join`, etc.)
- Job dispatch, WAL, or persist subsystem behavior changes

---

## Boundary Commitments

### This Spec Owns

- `handle_worker_join` in `src/controller/join.c` — rewritten to expose explicit branching
- `add_worker` in `src/controller/worker.c` — replaced by `init_worker`, `rejoin_worker`, and `apply_join_state`
- `src/controller/include/worker.h` — updated exported declarations

### Out of Boundary

- `makeWorker` / `makeWorker_` / `override_worker` in `src/lib/wobj.c` — must not be modified
- Other join handler functions in `join.c`
- Persist/WAL hook internals

### Allowed Dependencies

- `key_find` from `kvmgr` (via RTTI) for existence check
- `makeWorker` from `wobj.c` for first-time worker creation
- `find_matched_hostconfig`, `add_hostattrs_resvals`, `set_host_slots`, `mark_host_res` from `worker.c` (already present)
- `xce_run_hooks` for BEFORE/AFTER event hooks
- `on_worker_join` for post-join config push

### Revalidation Triggers

- Changes to `makeWorker` behavior (create vs update path) require re-checking `init_worker`
- Changes to `XCE_WORKER_ADMIN_MASK` definition require re-checking the admin-bit preservation in `rejoin_worker`
- Adding new hconfig fields requires updating `apply_join_state`

---

## Architecture

### Existing Architecture Analysis

**Current flow in `handle_worker_join` (join.c)**:

```
lock(workermgr, WRITE)
    run_hooks(BEFORE_EVENT)
    add_worker()                     ← delegates create-or-update decision
        makeWorker()
            lock(workermgr, WRITE)   ← DEADLOCK: already held by caller
            key_find + create-or-override
            unlock(workermgr)
        per-worker lock
            clear UNAVAIL, set ep, apply hconfig
        per-worker unlock
    vget(workermgr, idx, &w)
    run_hooks(AFTER_EVENT)
    xce_reply(...)
unlock(workermgr)
on_worker_join(...)
```

**Problems**:
1. Double write lock on `workermgr` deadlocks (pthread_rwlock_t is non-reentrant)
2. Create-or-update branch hidden inside `makeWorker_` — invisible at call site
3. `xce_reply` called while holding the write lock (unnecessary)

### Architecture Pattern & Boundary Map

```mermaid
graph TB
    HWJ[handle_worker_join]
    FHC[find_matched_hostconfig]
    HooksBefore[run_hooks BEFORE]
    KeyFind[key_find workermgr READ lock]
    InitW[init_worker]
    RejoinW[rejoin_worker]
    AJS[apply_join_state]
    MakeW[makeWorker wobj.c]
    HooksAfter[run_hooks AFTER]
    OWJ[on_worker_join]

    HWJ --> FHC
    HWJ --> HooksBefore
    HWJ --> KeyFind
    KeyFind -->|ENOENT| InitW
    KeyFind -->|found| RejoinW
    InitW --> MakeW
    InitW --> AJS
    RejoinW --> AJS
    HWJ --> HooksAfter
    HWJ --> OWJ
```

**Refactored flow**:
- `handle_worker_join` owns the explicit `key_find` branch decision (Req 1)
- `init_worker` owns first-time registration (Req 2); delegates creation locking to `makeWorker`
- `rejoin_worker` owns in-place attribute/endpoint update (Req 3); uses per-worker write lock
- `apply_join_state` is a static shared helper for status and resource update (Req 4)
- No outer `workermgr` write lock in `handle_worker_join` — eliminates deadlock

**Locking model**:

| Scope | Lock | Holder | Purpose |
|-------|------|--------|---------|
| Existence check | workermgr READ | `handle_worker_join` (brief) | Safe key_find |
| Worker creation | workermgr WRITE | `makeWorker` (internal) | key_add atomicity |
| Worker update | per-worker WRITE | `rejoin_worker` / `apply_join_state` | Field mutation |

### Technology Stack

| Layer | Choice | Role | Notes |
|-------|--------|------|-------|
| Language | C99 | Implementation | Existing codebase standard |
| Locking | pthread_rwlock_t (my_rwlock_t) | Concurrency | Existing primitive; non-reentrant |
| Object management | RTTI (key_find, key_add, makeInstance) | Worker lifecycle | wobj.c / kvmgr.c |
| Hooks | xce_run_hooks | Persistence integration | Unchanged hook fire points |

---

## File Structure Plan

### Modified Files

- `src/controller/join.c` — `handle_worker_join`: remove outer workermgr write lock; add explicit `key_find` check; call `init_worker` or `rejoin_worker` based on result
- `src/controller/worker.c` — remove `add_worker`; add `apply_join_state` (static), `init_worker`, `rejoin_worker`
- `src/controller/include/worker.h` — replace `add_worker` declaration with `init_worker` and `rejoin_worker`

---

## System Flows

```mermaid
sequenceDiagram
    participant JC as handle_worker_join
    participant WC as worker.c helpers
    participant WO as makeWorker (wobj.c)
    participant HS as hooksys

    JC->>JC: find_matched_hostconfig (fail fast, no lock)
    JC->>HS: run_hooks BEFORE_EVENT WORKER_JOIN
    JC->>JC: lock(workermgr READ)
    JC->>JC: key_find(workermgr, name, &w)
    JC->>JC: unlock(workermgr READ)

    alt host not found (ENOENT)
        JC->>WC: init_worker(c, ep, name, attrs, hconfig)
        WC->>WO: makeWorker(workermgr, &tmp_host)
        Note over WO: lock WRITE, create entry, unlock WRITE
        WC->>WC: apply_join_state under per-worker WRITE lock
        WC-->>JC: idx
    else host found
        JC->>WC: rejoin_worker(c, w, ep, attrs, hconfig)
        WC->>WC: per-worker WRITE lock
        WC->>WC: update hostattrs + ep (preserve admin bits)
        WC->>WC: apply_join_state (status, resources)
        WC->>WC: unlock per-worker
        WC-->>JC: idx
    else key_find error (not ENOENT)
        JC-->>JC: reject join, return error
    end

    JC->>JC: set priv->u.idx = idx
    JC->>JC: xce_reply (outside any lock)
    JC->>HS: run_hooks AFTER_EVENT WORKER_JOIN
    JC->>WC: on_worker_join(c, ep)
```

**Key decisions**:
- `find_matched_hostconfig` fails fast before any lock (Req 4.4 — reject if NULL)
- Hooks fire outside the workermgr lock, bracketing the create-or-update step
- `xce_reply` and `on_worker_join` execute after all locks are released

---

## Requirements Traceability

| Requirement | Summary | Component | Interface | Flow |
|-------------|---------|-----------|-----------|------|
| 1.1 | key_find before create-or-update | `handle_worker_join` | — | Sequence: key_find step |
| 1.2 | Branch on key_find result | `handle_worker_join` | — | alt/else branches |
| 1.3 | Reject on non-ENOENT error | `handle_worker_join` | — | else-error branch |
| 2.1 | Create via makeWorker | `init_worker` | `init_worker` signature | create branch |
| 2.2 | Log failure, set reply status, no on_worker_join | `handle_worker_join` | — | error exit path |
| 2.3 | Assign priv->u.idx | `handle_worker_join` | — | after successful join |
| 3.1 | Update hostattrs on rejoin | `rejoin_worker` | `rejoin_worker` signature | update branch |
| 3.2 | Update ep on rejoin | `rejoin_worker` | — | update branch |
| 3.3 | Preserve admin bits | `rejoin_worker` | — | ADMIN_MASK pattern |
| 4.1 | Clear XCE_WORKER_UNAVAIL | `apply_join_state` | — | shared path |
| 4.2 | Apply hconfig overrides | `apply_join_state` | — | shared path |
| 4.3 | Update host-attr resource values | `apply_join_state` | — | shared path |
| 4.4 | Reject if find_matched_hostconfig NULL | `handle_worker_join` | — | fail-fast before lock |
| 5.1 | BEFORE/AFTER hook order preserved | `handle_worker_join` | — | hook call sites |
| 5.2 | on_worker_join still called | `handle_worker_join` | — | post-join step |
| 5.3 | Critical section as short as current | Locking model | — | per-lock scoping |
| 5.4 | Existing tests pass | All | — | behavioral preservation |

---

## Components and Interfaces

### Summary

| Component | Layer | Intent | Req Coverage | Key Dependencies |
|-----------|-------|--------|--------------|-----------------|
| `handle_worker_join` | join.c | Explicit branch controller | 1.1–1.3, 2.2–2.3, 4.4, 5.1–5.3 | `key_find` (P0), `init_worker` (P0), `rejoin_worker` (P0) |
| `init_worker` | worker.c | First-time registration | 2.1–2.3 | `makeWorker` (P0), `apply_join_state` (P0) |
| `rejoin_worker` | worker.c | Reconnect update | 3.1–3.3 | `apply_join_state` (P0), per-worker lock (P0) |
| `apply_join_state` | worker.c (static) | Status + resource update | 4.1–4.3 | `add_hostattrs_resvals` (P0), `set_host_slots` (P0) |

---

### join.c

#### `handle_worker_join` (rewritten)

| Field | Detail |
|-------|--------|
| Intent | Explicit check → create-or-update → status/resource sequence for worker join |
| Requirements | 1.1, 1.2, 1.3, 2.2, 2.3, 4.4, 5.1, 5.2, 5.3 |

**Responsibilities & Constraints**
- Calls `find_matched_hostconfig` first; rejects join with appropriate error code if NULL (Req 4.4)
- Fires BEFORE_EVENT hook before the key_find step
- Acquires a brief workermgr READ lock for `key_find`; releases before any write path
- Branches explicitly on key_find result: `init_worker` or `rejoin_worker`
- Fires AFTER_EVENT hook after the create/update completes, outside any lock
- Calls `xce_reply` and `on_worker_join` outside any lock
- Does NOT hold the workermgr write lock at any point (prevents double-lock deadlock)

**Dependencies**
- Inbound: `handle_join_` dispatches to this function (P0)
- Outbound: `find_matched_hostconfig` — hostconfig lookup (P0)
- Outbound: `xce_run_hooks` — BEFORE/AFTER_EVENT (P0)
- Outbound: `key_find(c->workermgr, ...)` — existence check (P0)
- Outbound: `init_worker` — first-time join path (P0)
- Outbound: `rejoin_worker` — rejoin path (P0)
- Outbound: `on_worker_join` — config push post-join (P0)

**Contracts**: Service [x] / State [x]

##### Service Interface (C)

```c
/* Unchanged external signature */
static int handle_worker_join(cluster_t* c, endpoint_t* ep,
                              xce_request_t* request);
```

- Preconditions: `request->body.joinmsg` non-NULL; `ep->data` non-NULL
- Postconditions: on success, `priv->u.idx` set; `on_worker_join` invoked; BEFORE/AFTER hooks fired
- Error return: negative errno; reply status set to error code; `on_worker_join` not called

**Implementation Notes**
- Integration: The outer `lock(c->workermgr, RTTI_LOCK_WRITE)` present in the current code is **removed**. The `vget` lookup for the AFTER_EVENT hook (currently inside the lock) moves to after the create/update call.
- Validation: All early-exit paths (hostconfig NULL, key_find non-ENOENT error, init/rejoin failure) must set `reply.status` and call `xce_reply` before returning.
- Risks: `priv->u.idx` must be set before `on_worker_join` and after the lock is released; the existing order is preserved.

---

### worker.c

#### `apply_join_state` (static)

| Field | Detail |
|-------|--------|
| Intent | Shared status and resource update applied to both new and rejoining workers |
| Requirements | 4.1, 4.2, 4.3 |

**Responsibilities & Constraints**
- Called under the per-worker write lock (caller must hold it)
- Clears `XCE_WORKER_UNAVAIL` from `w->host.status` (Req 4.1)
- Sets `w->ep = ep`
- Applies hconfig hosttype/model overrides (dereference + makeString pattern)
- Calls `add_hostattrs_resvals` (Req 4.3)
- Computes mxj slots and calls `set_host_slots` (Req 4.2)
- Replaces `dispatch_window_config` and `thresholds` from hconfig
- Loops over `hconfig->nresources` calling `mark_host_res` (bool resources)

**Contracts**: Service [x]

##### Service Interface (C)

```c
/* static — internal to worker.c */
static int apply_join_state(cluster_t* c, worker_t* w, endpoint_t* ep,
                            xce_hostconfig_t* hconfig);
```

- Preconditions: caller holds per-worker WRITE lock; `w`, `hconfig` non-NULL
- Postconditions: worker status, ep, and all resource values updated
- Returns: 0 on success; negative errno on resource update failure

---

#### `init_worker`

| Field | Detail |
|-------|--------|
| Intent | Register a new worker entry for a first-time joining host |
| Requirements | 2.1, 2.2, 2.3 |

**Responsibilities & Constraints**
- Builds a temporary `xce_worker_t` with name and attrs
- Calls `makeWorker(c->workermgr, &tmp)` — delegating write-lock acquisition and allocation to wobj.c
- On NULL return: logs failure, returns `-errno` (caller sets reply status)
- On success: acquires per-worker write lock, calls `apply_join_state`, unlocks
- Returns the worker index (`w->idx`)

**Dependencies**
- Outbound: `makeWorker` (wobj.c) — allocation + key_add (P0)
- Outbound: `apply_join_state` — status/resource setup (P0)

**Contracts**: Service [x]

##### Service Interface (C)

```c
/* exported — declared in worker.h */
int init_worker(cluster_t* c, endpoint_t* ep, const char* host,
                hostattrs_t* attrs, xce_hostconfig_t* hconfig);
```

- Preconditions: caller does NOT hold workermgr write lock; `host` non-NULL
- Postconditions: new worker registered in workermgr; status and resources initialized
- Returns: worker index (≥ 0) on success; negative errno on failure
- Error cases: `makeWorker` allocation failure → return `-errno` without calling `on_worker_join`

---

#### `rejoin_worker`

| Field | Detail |
|-------|--------|
| Intent | Update an existing worker entry when the same host reconnects |
| Requirements | 3.1, 3.2, 3.3 |

**Responsibilities & Constraints**
- Accepts a `worker_t*` found by the caller's `key_find`
- Acquires per-worker write lock
- Saves admin status bits: `admin_status = w->host.status & XCE_WORKER_ADMIN_MASK`
- Frees existing `hosttype` and `hostmodel` strings; copies new ones from `attrs`
- Copies numeric attrs fields (`cpufactor`, `maxcpus`, `maxmem`, `maxswap`, `maxtmp`, `ndisks`)
- Restores `w->host.status` admin bits (Req 3.3)
- Calls `apply_join_state` (clears UNAVAIL, sets ep, applies hconfig overrides, updates resources)
- Releases per-worker write lock
- Returns `w->idx`

**Dependencies**
- Outbound: `apply_join_state` — status/resource setup (P0)
- Outbound: `dereference` / `makeString` — string memory management (P0)

**Contracts**: Service [x]

##### Service Interface (C)

```c
/* exported — declared in worker.h */
int rejoin_worker(cluster_t* c, worker_t* w, endpoint_t* ep,
                  hostattrs_t* attrs, xce_hostconfig_t* hconfig);
```

- Preconditions: caller holds a reference on `w`; does NOT hold per-worker write lock
- Postconditions: `w->host.attrs` updated; `w->ep` set to `ep`; admin bits preserved; resources updated
- Returns: `w->idx` (≥ 0); negative errno on `apply_join_state` failure
- Invariant: `w->host.status & XCE_WORKER_ADMIN_MASK` unchanged from pre-call value

```c
#define XCE_WORKER_ADMIN_MASK \
    (XCE_WORKER_LOCKEDU | XCE_WORKER_CLOSED | XCE_WORKER_HIDDEN \
     | XCE_WORKER_CLOSED_BY_ADMIN)
```

---

### worker.h

#### Exported Declaration Changes

Remove:
```c
extern int add_worker(void*, endpoint_t*, const char*, hostattrs_t*);
```

Add:
```c
extern int init_worker(cluster_t*, endpoint_t*, const char*, hostattrs_t*,
                       xce_hostconfig_t*);
extern int rejoin_worker(cluster_t*, worker_t*, endpoint_t*, hostattrs_t*,
                         xce_hostconfig_t*);
```

---

## Error Handling

### Error Strategy

Fail fast, explicit error propagation, no silent fallbacks.

### Error Categories

| Error | Source | Handler | Reply Status |
|-------|--------|---------|-------------|
| `find_matched_hostconfig` returns NULL | No matching hostconfig | Reject join; set `reply.status = -errno` | `-errno` |
| `key_find` returns error other than -ENOENT | workermgr internal error | Reject join (Req 1.3) | key_find return code |
| `init_worker` returns error | `makeWorker` allocation failure | Log, reject join (Req 2.2) | error code |
| `rejoin_worker` returns error | `apply_join_state` failure | Log, reject join | error code |
| `apply_join_state` partial failure | Resource update failure | Log with PERROR; continue (matches existing behavior) | 0 |

### Monitoring

- Existing `ERROR` / `TRACE` macros used throughout — no changes to log verbosity
- `PERROR("update_resval")` in `update_resvals` preserved for resource update failures

---

## Testing Strategy

### Unit Tests (src/controller/testsuite/)

1. **First-time join creates worker**: call `init_worker` with a new hostname; verify `key_find` succeeds afterwards, `XCE_WORKER_UNAVAIL` is clear, and resource values are set.
2. **Rejoin updates attrs and ep**: call `init_worker`, then call `rejoin_worker` with different attrs; verify updated attrs, updated ep, and original admin bits preserved.
3. **Admin bits preserved on rejoin**: set `XCE_WORKER_CLOSED` before rejoin; verify flag still set after `rejoin_worker`.
4. **NULL hostconfig rejects join**: configure cluster with no matching host config; verify `handle_worker_join` path returns error without modifying workermgr.
5. **Non-ENOENT key_find error rejected**: simulate `key_find` returning `-EIO`; verify join rejected and `on_worker_join` not called.

### Integration Tests

1. **BEFORE/AFTER hook order**: verify hooks fire in the correct order relative to workermgr state changes using existing `test_e2e_hooks.c` patterns.
2. **on_worker_join called after successful join**: verify `worker_push_config` executes on both first-time and rejoin paths.
3. **Existing testsuite passes without modification**: `make test` in `src/frontend/` and testsuite build must remain green.

### Behavioral Preservation

The existing unit tests in `src/controller/testsuite/` exercise `add_worker` indirectly through cluster setup helpers. Test fixtures that call `add_worker` directly (if any) must be updated to call `init_worker` with the added `hconfig` parameter. All observable behaviors — hook ordering, resource initialization, admin-bit preservation, `on_worker_join` invocation — are explicitly covered by the traceability table and unit tests above.