Split pool.remove into user-remove vs internal-retire (W11) - #1775
Conversation
…retire (W11) buildRemotePool's `remove` carried two distinct facts: a USER removing a host (which should persist the departure) and the pool shedding a dead session on its OWN initiative (#1708's guest re-serve pump-death, which must NOT persist — only an explicit user remove should forget a host). With one verb wired to `persist`, an internal shed permanently un-remembers a host the user never removed. Split them — two verbs, not a `persist` flag: - `remove(host)` — user intent: persist first (the `persist` hook fires), then tear down. Unchanged behaviour. - `retire(host)` — internal shed: the SAME teardown, but NO persist, so the host leaves the live pool yet stays in the persisted set and a membership store re-seeds it next boot. With no persist step it can't reject, so a fire-and-forget `void pool.retire(h)` needs no `.catch`. The destructive teardown is factored into one shared `tearDownEntry` (the two verbs differ only in the persist step before it). kolu's #1708 guest pump-death path now calls `pool.retire`, not `pool.remove`. On master this is behaviour-neutral (kolu passes no `persist` hook, so retire and remove are identical) — it lands first so W10 (host-membership persistence, #1772) can rebase onto a correct contract, per srid's W11 ruling (padi.mdx, #1770). Additive API change (a new method); drishti consumes buildRemotePool and calls only existing verbs, and its app typechecks green against this — no paired drishti change forced. Reference docs updated (ref-surface-remote.mdx).
… entries Architecture review (C6 state-and-time) caught that retire's "stays remembered" contract was not actually delivered: persist derived its set from live `entries.keys()`, which retire deliberately desyncs, so the FIRST add/remove after a retire recomputed the file from live keys and silently dropped the retired host BEFORE any reboot — meaning W10 would rebase onto a broken contract (F2 not really fixed). Track the intended-persisted membership as its OWN ordered Set: add inserts, remove deletes, retire LEAVES IT UNTOUCHED, and persist writes from THAT set (not live keys). A retired host now provably survives every later add/remove until a reboot re-seeds it. Also dedup the add path so re-adding a retired host (still in the persisted set) doesn't write a duplicate. Two new tests pin both. Behaviour-neutral for a no-persist consumer (drishti): persistedMembership tracks entries exactly when retire is never called.
…ve membership Reword the persist hook docstring so it states its argument is the intended persisted membership, which after a retire can include a shed-but-remembered host absent from hosts()/has(); persist must not cross-check or prune against live membership. Agreed by the lowy ⇄ hickey lens debate (finding lowy-2, raised by lowy). Not pushed or merged.
Thread verb into the WebSocket close reason (`host ${verb}`) so a retired host closes with reason "host retired", matching the adjacent log line instead of the hardcoded "host removed".
Agreed by the lowy ⇄ hickey lens debate (finding lowy-3, raised by lowy). Not pushed or merged.
Address codex's five findings on the W11 retire/remove verb split. - F1 (major, fixed): remove() early-returned when no live entry existed, so a host already retired (gone from entries, still in persistedMembership) could never be forgotten by the user's explicit remove — it re-seeded next boot. remove() now no-ops only when the host is absent from the REMEMBERED set, always persists + drops the remembered claim, and tears down only when a live entry remains. Regression test added. - F2 (major, fixed): "retire can't reject" was false — notifyMembership fanned out listeners unguarded, so a throwing subscriber (serveHostMap reconcile/fire) skipped session destroy AND rejected the queued task → void pool.retire() goes fatal. Each listener is now isolated (loud log, fan-out continues), making the premise structurally true; kolu's no-.catch call site stands. Regression test added. - F3 (minor, fixed): module header, subscribe() docstring, and the fleet-safety guide now document retire and separate live vs remembered membership. - F4 (minor, fixed): tearDownEntry comment + ref-surface-remote.mdx corrected — the verb labels the socket close reason too (not "only the log line"), the two verbs aren't "identical" without persist, and links evict on either verb. - F5 (nit, fixed): ISOLATION test comment reframed around the shared teardown guard and kolu's new void pool.retire() shed path. check + fmt green; 24 surface-remote hostFanout tests pass.
Close the two remaining doc findings from codex's round-2 verdict (F1, F2,
F5 already resolved in round 1; F3, F4 were held open).
- F3 (partial→fixed): registerConnection JSDoc now names retire alongside
remove (both close the socket via the shared tearDownEntry); the fleet
guide no longer claims a nonexistent `{ reason: "removed" }` /
`{ reason: "retired" }` stream payload — it describes the real wire facts:
a clean typed stream completion plus a 1000 WebSocket close whose reason
string names the verb (`host removed` / `host retired`).
- F4 (fixed): RemotePool.retire JSDoc no longer says the two verbs are
"identical" without a persist hook — they are the same on disk but differ
on the wire (socket close reason + log line name the verb), matching the
already-corrected website reference.
Doc/comment-only; no runtime change. just check green.
⚖️ Lowy ⇄ Hickey lens debate✅ Consensus after 1 round(s) · lowy + hickey · base Independent findings: lowy=4, hickey=3 Applied (3)
Agreed — no change (4)
|
Codex ⇄ Claude debate✅ Consensus after 3 round(s) · codex reviewed at Round 1codex — approved: The two-verb design, separate persisted membership, framework placement, and behavior-neutral landing are defensible. However, two major correctness defects remain: a later user removal can be discarded after retirement, and Findings:
claude — Fixed the two correctness gaps (retire→remove losing the departure; retire able to reject through a throwing membership listener) and tightened the four doc/comment inaccuracies; added two regression tests.
commit: Round 2codex — approved: The two correctness fixes are sound and their regression tests cover the reported failures. F5 is also fully corrected. The documentation cleanup remains incomplete: one public socket-lifecycle comment and the Findings:
codex on the rebuttal: F1: accepted; the remembered-membership check and conditional teardown resolve the race. F2: accepted; per-listener isolation plus the regression test addresses the reported rejection path. F3: held open because the socket JSDoc was missed and the updated guide introduces a nonexistent claude — Closed the two remaining doc findings: named
Gate: commit: Round 3codex — approved: All five findings are resolved. The correctness fixes remain intact, and commit 007ef9e accurately closes the remaining documentation gaps: socket tracking names both teardown verbs, the guide describes clean completion rather than nonexistent payloads, and the exported JSDoc distinguishes disk behavior from wire-visible close reasons and logging. Findings:
codex on the rebuttal: F1, F2, and F5 remain resolved. F3: accepted; both the missed socket JSDoc and invented reason payloads are corrected. F4: accepted; the public interface now precisely distinguishes disk behavior from wire-visible behavior. No disputes remain. |
👮 Code-policeTwo passes (rules + fact-check) over Rules pass — no violations (24 rules checked). Called out as exemplary on Fact-check pass — sound, all five load-bearing claims verified against the code:
24/24 unit tests green. One pre-existing non-defect noted (a throwing |
Darwin e2e — retuned sample (srid-ratified, in progress)Per srid's ruling, the two prior Exact tree under sample (honesty over tidiness):
This is a diagnostic sample of the retune, not #1775's official check. Result appended when it settles. |
|
Retune sample result: FAILED — wedged even at PAR=3. |
|
Control run (srid-ratified, in progress). To prove #1775's innocence dynamically, a control sample runs |
CI on the final HEAD
|
| node | |
|---|---|
ci::e2e@x86_64-linux |
✔ 5m37s |
ci::unit@x86_64-linux |
✔ 1m54s |
ci::smoke · ci::biome · ci::fmt · ci::nix · ci::flake-check · ci::home-manager · ci::pnpm-hash-fresh · ci::atlas-sync · surface-example builds |
✔ all |
⚠️ Darwin — NOT claimed on this HEAD
- Darwin non-e2e (unit / smoke / biome / fmt / builds): the earlier all-green numbers ran on
98517e415, not this HEAD — they are not carried over. A re-run ondd43e1c64is needed to assert them (currently blocked: rasam's odu/ssh transport is dropping mid-run —agent exited code=255— two consecutive control runs died on it). - Darwin e2e: stays under the e2e(darwin): local padi link liveness-timeout force-cycles an alive-but-slow padi mid-run → worker queue-drain #1776 disposition (the local-padi liveness force-cycle; reproduced 3×, filed e2e(darwin): local padi link liveness-timeout force-cycles an alive-but-slow padi mid-run → worker queue-drain #1776, tracked docs(atlas): flaky-tracker row — darwin e2e local-padi liveness force-cycle (#1776) #1777).
Provenance (straight record)
dd43e1c64(this HEAD) =98517e415+ master-merge (#1771boundToPid +#1774statepip).- The earlier 26/27 ran on
98517e415, which is post-gauntlet —persistedMembership(ac11544cb) and both codex fixes (eec8c24f5,007ef9e66) are ancestors of it. So the untested delta ondd43e1c64is the master-merge, not the gauntlet fixes.
Note: #1771 (test daemons die with their run) is now in this tree — the bare-host daemon-leak cure — so a fresh darwin sample is worth it once rasam's transport is stable.
…-cycle (#1776) (#1777) Adds a Flaky Test Tracker row for the `aarch64-darwin` e2e flake diagnosed as [#1776](#1776): the local padi link's **liveness probe times out under load** and force-cycles an alive-but-slow padi, so the per-scenario `Before`-hook `killAll` 500s during the down-window and the worker queue-drains. Surfaced while diagnosing #1775's darwin e2e (which is otherwise green on linux and every non-e2e darwin node). Verified the flake is **not** #1775's code (behaviour-neutral, changed paths unreachable by the single-host suite), **not** the startup readiness gate (already correct — `waitForPadiLive` gates every server start), and **not** the teardown race #1719. Docs-only; dist regenerated. _Generated by [`/be`](https://github.com/srid/agency) on Claude Code (model `claude-opus-4-8`)._
> ## 📚 Stacked on #1775 (the retire-verb split) > > This now sits on **#1775**, which delivers the framework fix W10 needed. The root cause — one verb (`pool.remove`) carrying both *user-remove* (persist) and *system-retire* (#1708 pump-death, must not persist) — is fixed there: kolu's guest pump-death path now calls **`pool.retire`**, which tears the host out of the live pool **without** persisting, so a transient guest fault no longer un-remembers a host. W10's earlier `.catch` guard on `pool.remove` was dropped in the rebase (retire provably can't reject). > > **Carried findings, resolved:** F2 (a guest pump-death permanently forgot a host) — fixed by the split's `retire`. F3 (a dead re-serve mirror stranded on retire) — kolu's per-host `reServeSurface` cache is evicted by `pruneToMembers` wired to `pool.subscribe`, which fires on `retire`'s membership drop, so the mirror is pruned. > > **Merge order:** split **#1775** first, then this. Base is `w11-retire-split`; it retargets to `master` automatically once #1775 merges. Tracked as **W11** in the padi note (#1770). > > _CI + evidence run on the final (rebased) tree — pending a free pu box (shared-pool saturation)._ --- **The remote hosts you add from the selector strip now survive a kolu restart.** Today a restart forgets everything but the local default and you re-add your fleet by hand; after this, every remembered host reappears in the strip and reconnects through the normal connect pipeline — a host that has since gone away shows its honest **failed** chip with the cause rather than silently vanishing. The only way a host leaves the strip is your explicit remove. Membership is a **server** fact — kolu-server's pool (`buildRemotePool`) is its one writer — so its memory lives beside that authority, never in the browser, where localStorage would fork the one list into per-device copies. The persisted artifact is a value replaced whole: a zod-schemed `{ version: 1, hosts: string[] }` JSON of **encoded host keys only**, beside `conf`'s `config.json` under `KOLU_STATE_DIR`, written atomically (tmp + rename, async so it never blocks the serving loop). ### The seam this wires (srid-approved over the brief's sketch) The brief sketched a hand-rolled write *inside the add/removeHost handlers*. But the pool already ships a purpose-built, transactional `persist` hook (from #1714) that kolu's own `buildRemotePool` call never wired. Hand-rolling would duplicate it — the "parallel hand-rolled mechanism" the *reuse the existing source of truth* rule names as a defect — so we **wired the existing seam** instead: | | Handler-side write (brief's sketch) | Pool `persist` hook (this PR) | | --- | --- | --- | | Ordering | writes *after* `pool.add` commits → disk/memory can diverge on failure | write ordered **before** the in-memory commit | | Concurrency | two rapid strip-adds race the file | serialized through the pool's one mutation queue | | Failure | host live but unpersisted | just-built session **rolled back** | ### Boot: seed, don't replay ``` parseKoluPadiHostSeed() ─┐ ├─▶ dedup ─▶ initialHosts ─▶ buildEntry ─▶ W6 connect pipeline loadPersistedHosts(file) ─┘ ``` Persisted hosts are merged into `initialHosts` (deduped against the env seed via order-preserving `new Set`) rather than re-added via a post-build `pool.add` loop. Both flow the identical connect pipeline, but seeding at construction *doesn't re-fire `persist`* — so the file is only ever rewritten by a genuine runtime add/remove, and **an interrupted boot can't truncate it**. ### Design philosophy - **Fail fast** — a file that exists but fails the schema *crashes the boot* naming the path (delete it to recover). Never start-with-empty-fleet, which would silently eat the user's hosts (`caught-error-must-not-collapse-to-empty`). This is why it's a **separate** file, not the `conf` store: `conf` data is reconstructible so a corrupt file resets to defaults; a fleet is not. Only `ENOENT` reads as "fresh install"; a permission error surfaces. - **Electricity** — persistence policy is app-level (`packages/server` only); the durability *mechanism* is the framework's `persist` contract. No framework change in *this* PR — that's exactly the split PR's job. - **Reuse the source of truth** — the pool's `persist` seam, `KOLU_STATE_DIR` (via a narrowed export from `state.ts`), the pool's own `encodeHostKey` vocabulary, and a shared `isEncodedHostKey` promoted into `kolu-common` (de-duping a copy the client held). ### Review gauntlet outcome - **architecture-first-principles** — 1 confirmed defect **fixed** (W10 made `pool.remove` rejectable; guarded the fire-and-forget guest-retire so a disk-write failure can't crash the whole server) + 3 minors folded in. It also surfaced the deeper verb-conflation that became the block above. - **lens-debate (lowy ⇄ hickey)** — consensus, 1 round, 1 fix (key the persist-exclusion off `LOCAL_HOST`, not the boot-default). - **codex-debate** — *stopped as non-converging* after 4 rounds (findings grew 13→16, never approved) when its author spiralled into an out-of-scope retry/overlay mechanism that generated its own blocking bugs. Reset to the lens-approved tree and triaged codex's **round-1** review (of the clean diff) by hand — the legit in-scope findings are applied (fail-fast read vs `existsSync` masking `EACCES`, `0600` mode on the ssh-target file, APM-source sync, atlas doc-sync). The full round-1–4 codex trail is posted as a comment for audit. - **simplify** — efficiency + altitude clean; applied a reuse de-dup and dropped dead test state. - **code-police** — 1 fix (async fs on the persist path). Two findings **recorded, not actioned** because they contradict the ratified plan: *prefer-focused-library* (use `conf`) — the plan mandates a zod JSON tmp+rename kept outside conf's ladder; *persisted-schema-stays-tolerant* (filter, don't crash) — the plan mandates a fail-fast crash-with-path, with a dedicated test. Surfaced for srid. Plan of record: the W10 section of `docs/atlas/src/content/atlas/padi.mdx` (#1770); the block resolution is minted there as **W11**. _Generated by [`/be`](https://github.com/srid/agency) on Claude Code (model `claude-opus-4-8`)._ --- ### Carried finding → resolved in the W11 split **F3 (codex round-1, residual):** if the internal guest-retire's teardown fails, a **dead re-serve mirror can be left stranded in both cache layers** (`reServes` in `index.ts` and the pool's `entries`). This is legitimately the **W11 split's** world — the retire path is being rebuilt there (the non-persisting internal retire), so the mirror-eviction ordering is fixed as part of that rebuild, not patched here. Named here so it isn't dropped; tracked on the W11 PR. _Disposition table for the full codex trail is in the coordinator report; the round-1 in-scope fixes are applied on this branch (fail-fast read, 0600 + fsync durability, env-seed provenance, strict schema, doc-sync)._
**Status: ratified (2026-07-13).** srid delegated ratification to the coordinator under /perfection-review; the note now carries `status: accepted` and reads as the current snapshot only — no revision history in the note body (git and this PR hold the journey). **This note is now the campaign plan of record for the surface-framework consolidation.** The original audit (Codex, `gpt-5`) was written deliberately blind to the existing Atlas plans — that was methodology, an independent read of the three working trees. This revision is the reconciliation it deferred: the coordinator's reviewed ruling on the proposal, ratified by srid, merged in as one plan. ### What merged in - **The ratified [reactive bridge](https://kolu.dev/atlas/surface-reactive-bridge.html)** — phase 0 shipped in W5 (#1759); its phases 1–4 are now PRs 7–10 of this plan's sequence (one numbering scheme; the bridge note carries a superseded-pointer). - **The [consolidation ledger](https://kolu.dev/atlas/padi-cleanup.html)'s campaign 1** — L28 rung 2 → PR 3, L20·L21 → PR 5, L1 → PR 8, L8's hygiene residue as the tail commit; L22 and L24 dispositioned (L24 already shipped as #1749). - **A staleness sweep** — the audit's pins predate six merged PRs (W9 #1764, W10 #1772, W11 #1775, W12 #1784, #1783, #1791). Every code claim was re-grounded against current kolu/drishti/odu tips or cut; e.g. the "six polled-query state machines" claim died against W9's per-host ownership shape, and odu already adopted `implementSurface` since the audit. ### The dispositions | proposed | ruling | | --- | --- | | supervised `SurfaceRuntime` (final router · `done` · idempotent `close`) | **kernel — PR 1** | | bound procedures as first-class client members | **kernel — PR 2** | | opaque `membershipId` per map add (+ typed connection key, `clockNow` at admit) | **kernel — PR 3** | | total, schema-valid `failureOf` (no fabricated `"other"`) | **kernel — PR 4** | | mirror consumes collection `deltas` (deletes drishti's parallel stream) | **kernel — PR 5** | | `firstFrameOrThrow`-style adoptions | **kernel — PR 6** | | `Feed` member kind | **dropped** — it is the bridge's `scan` wearing a wire protocol; the append-heavy wire concern (terminal bytes, logs) becomes a named bridge design question, not a fourth member kind | | `@kolu/surface-suite` | **deferred** — a thin composition leaf earns no receptacle; its two real moves (final router, typed key) are in the kernel; revival condition: the surface-app dependency direction | | Odu lease stack (`SurfaceLease` … `SurfacePort`) | **Odu-local** — five concepts for one caller fails prove-then-extract; graduates at a second consumer; `SurfacePort` dies with it | | `createLiveQuery` | **obsolete** — W9's `hostCodeTab.ts` ownership shape supersedes the string-keyed singleton; the createResource instinct may refine inside it | | `notificationSurfaceApp` / `bootSurfaceApp` | **needs grounding** against W5's shipped `createNotify`/SW seam before any PR is cut | The note is restructured as user-facing / architecture / implementation, with the sequenced ten-PR list (kernel first, every framework PR drishti-paired per `.claude/rules/surface.md`) and grep-able done-criteria. Status stays `proposed` — srid merges and flips. ### Validation - `just atlas::build` (foreground, exit 0) - `just atlas::check-sync` (sync + idempotency both green) 🤖 Generated with [Claude Code](https://claude.com/claude-code)
1 participant
buildRemotePool'sremovecarried two different facts under one verb — a user removing a host (which should persist the departure) and the pool shedding a dead session on its own initiative (#1708's guest re-serve pump-death). Onceremoveis wired to apersisthook, that conflation means an internal shed permanently un-remembers a host the user never removed — the defect the W10 host-persistence PR surfaced and is now blocked on. This splits the verb so persistence follows intent.Two verbs, not a
persistflagremove(host)persisthook fires first)retire(host)The destructive teardown is factored into one shared
tearDownEntry; the two verbs differ only in whether theypersistbefore it. Becauseretirehas no persist step and swallows its own teardown fault, it can't reject — so a fire-and-forgetvoid pool.retire(h)needs no.catch(unlikeremove, whose persist step can). kolu's #1708 guest pump-death path now callspool.retire.Drishti gate
Additive API change — a new
retiremethod onRemotePool. Drishti consumesbuildRemotePool/RemotePool(hostRegistry.ts,admin-router.ts) but calls only existing verbs (add/remove/reconnect/recheckAll/has/subscribe/destroyAll) — neverretire. Drishti's app typechecks green against this new surface-remote (verified locally by hydrating the change into drishti's tree). No paired drishti PR is forced — no drishti code changes are needed.Carried finding (from W10's review)
F3: if
retire's teardown fails, a dead re-serve mirror could be left in kolu'sreServescache and the pool'sentries. The teardown ordering here (drop membership + notify before destroy, destroy swallowed) is the pool half; kolu's per-hostreServeSurfacecache eviction is keyed topool.subscribemembership, so a retired host's mirror is pruned bypruneToMemberson the membership drop. Tracked so it isn't lost.Tests:
retireruns the same teardown asremovebut doesn't persist; a no-op on an unknown host; swallows a destroy fault and resolves (proving the fire-and-forget is safe).Generated by
/beon Claude Code (modelclaude-opus-4-8).