Take the state lock on the next write, not on a poll (#1027)

changelog.d/1027-takeover-on-next-write.fixed.md

@@ -0,0 +1 @@
+A window that showed the Bot team read-only because another dsh held `state.lock` now takes the lock on its next write once that window closes, instead of staying read-only until a `state` read happened to refresh it (#1027). A change made outside a request goes through `mutate()`, which follows what the other host left before applying the change, so neither side's work is lost.

core/src/host/store.js

@@ -427,10 +427,31 @@ export function install(rt) {
     return warnReadOnly()
   }
 
+  // A window that was read-only because the other dsh held state.lock takes the lock on
+  // its next write once that file is gone, instead of waiting for a later state call
+  // (#1027). Taking over adopts nothing by itself: whoever called save() may already have
+  // changed `state`, and adopt() replaces it whole (#1032).
+  const takeover = () => {
+    claimLock()
+    if (holder !== undefined) return false
+    warnedReadOnly = false
+    return true
+  }
+
   const save = () => {
     if (stopped) return writing
     indexSessions()
     // A reading host keeps the file's revision, so its window does not skip the next change.
+    if (holder !== undefined && !lockBroken && stuck === undefined) {
+      // The other dsh may have closed since this window last looked: take the free lock
+      // instead of dropping the write (#1027). Nothing is adopted here — a caller may have
+      // already changed `state`, and adopt() replaces it whole (#1032). The write-path
+      // entry that adopts *before* a change is `mutate()`, just below.
+      if (!takeover()) {
+        warnReadOnly()
+        return writing
+      }
+    }
     if (!writable()) return writing
     state.revision += 1
     state.exchanges = state.exchanges.slice(-MAX_EXCHANGES)
@@ -466,6 +487,17 @@ export function install(rt) {
     })
   }
 
+  // The write-path entry for a change made outside a request (a plugin, a scheduled task,
+  // a delivery): take over the lock and adopt what the other host left *before* the change
+  // is made, then write it. Adopting first is what keeps both sides: a change already in
+  // `state` cannot be adopted around without losing it (#1032).
+  const mutate = async (change) => {
+    await refresh()
+    const result = await change()
+    await save()
+    return result
+  }
+
   ctx.effect(() => () => {
     stopped = true
     return writing.finally(() => lock.release())
@@ -543,7 +575,7 @@ export function install(rt) {
   const isAdminOf = (room, id) => id !== undefined && room.admin === id
 
   Object.assign(rt, {
-    state, load, save, refresh, readOnly,
+    state, load, save, refresh, readOnly, mutate,
     botOf, roomOf, isMain, chatOf, selfOf, isBotSession,
     chatOwners, groupOwners, pastOwners,
     groupSessionsOf, dropGroupSession, findBot, findRooms, isAdminOf,

core/test/home-lock.test.js

@@ -5,7 +5,7 @@ import { syncBuiltinESMExports } from 'node:module'
 import { hostname, tmpdir } from 'node:os'
 import { join } from 'node:path'
 import { test } from 'node:test'
-import { apply } from '../index.js'
+import { createRuntime, provideFlyBot } from '../index.js'
 import { homeLock } from '../src/host/home-lock.js'
 import { fakeContext } from './fake-ctx.js'
 
@@ -118,10 +118,14 @@ function start(t, home) {
     route = options
     return register(options)
   }
-  apply(ctx, { home })
+  // createRuntime + provideFlyBot is what apply() does, and it keeps the runtime so a
+  // test can reach a save that no request drives (#1027).
+  const rt = createRuntime(ctx, { home })
+  provideFlyBot(ctx, rt.flybot)
   t.after(() => ctx.dispose())
   const call = async (endpoint, payload) => (await route.fetch(new Request('http://localhost/api/bot', { method: 'POST', body: JSON.stringify({ endpoint, payload }) }))).json()
   call.ctx = ctx
+  call.rt = rt
   return call
 }
 
@@ -186,6 +190,65 @@ test('a reading host follows the other host\'s keys, never writes them, and keep
   assert.deepEqual(saved, ['FIRST_KEY', 'MINE', 'SECOND_KEY', 'THIRD_KEY'])
 })
 
+test('#1027: a save in a stale read-only window takes the lock and writes', async (t) => {
+  const home = tempHome(t)
+  writeFileSync(join(home, 'state.json'), JSON.stringify(TEAM))
+  writeFileSync(join(home, 'state.lock'), JSON.stringify({ pid: process.ppid, host: hostname(), token: 'other' }))
+  const call = start(t, home)
+  assert.match((await call('state', {})).value.readOnly, /open in another dsh/)
+
+  // The other host closes; this window has not read since.
+  rmSync(join(home, 'state.lock'))
+
+  // A save no request drives (a plugin, a scheduled task, a retitle) takes the free lock
+  // and writes, instead of being dropped until some later state call refreshes.
+  await call.rt.save()
+  assert.equal(JSON.parse(readFileSync(join(home, 'state.lock'), 'utf8')).pid, process.pid)
+  assert.equal(JSON.parse(readFileSync(join(home, 'state.json'), 'utf8')).revision, 8)
+  assert.equal((await call('state', {})).value.readOnly, null)
+})
+
+test('#1032: a change made before save() lands; taking over does not replace it with what the other host left', async (t) => {
+  const home = tempHome(t)
+  writeFileSync(join(home, 'state.json'), JSON.stringify(TEAM))
+  writeFileSync(join(home, 'state.lock'), JSON.stringify({ pid: process.ppid, host: hostname(), token: 'other' }))
+  const call = start(t, home)
+  assert.match((await call('state', {})).value.readOnly, /open in another dsh/)
+
+  // The other host saves once more and closes while this window is read-only, and a writer
+  // here changes state before its save — the shape every host path uses.
+  writeFileSync(join(home, 'state.json'), JSON.stringify({ ...TEAM, revision: 8, prefs: { theme: 'light' } }))
+  rmSync(join(home, 'state.lock'))
+  call.rt.state.prefs.theme = 'dark'
+  await call.rt.save()
+
+  // This window writes what it has (revision 7 in memory, so 8 on disk); the change is not
+  // replaced by the copy the other host left, and the revision it bumps is its own.
+  const saved = JSON.parse(readFileSync(join(home, 'state.json'), 'utf8'))
+  assert.equal(saved.prefs.theme, 'dark')
+  assert.equal(saved.revision, 8)
+  assert.equal((await call('state', {})).value.readOnly, null)
+})
+
+test('#1032: mutate() adopts what the other host left before the change, so both survive', async (t) => {
+  const home = tempHome(t)
+  writeFileSync(join(home, 'state.json'), JSON.stringify(TEAM))
+  writeFileSync(join(home, 'state.lock'), JSON.stringify({ pid: process.ppid, host: hostname(), token: 'other' }))
+  const call = start(t, home)
+  assert.match((await call('state', {})).value.readOnly, /open in another dsh/)
+
+  writeFileSync(join(home, 'state.json'), JSON.stringify({ ...TEAM, revision: 8, prefs: { theme: 'light' } }))
+  rmSync(join(home, 'state.lock'))
+  // The write-path entry: take over and adopt first, then change, then write.
+  await call.rt.mutate(() => { call.rt.state.prefs.locale = 'zh' })
+
+  const saved = JSON.parse(readFileSync(join(home, 'state.json'), 'utf8'))
+  assert.equal(saved.prefs.theme, 'light')
+  assert.equal(saved.prefs.locale, 'zh')
+  assert.equal(saved.revision, 9)
+  assert.equal((await call('state', {})).value.readOnly, null)
+})
+
 test('read-only host (#14): FlyBot queries answer, plugin reads answer, writes and polling stop', async (t) => {
   const home = tempHome(t)
   writeFileSync(join(home, 'state.json'), JSON.stringify(TEAM))

docs/subsystems/core-host.md

@@ -113,6 +113,8 @@ A member's own line calls the members it names in two shapes where the call used
 
 - The saved team (`state.json`) is versioned. Version 2 (#47) carries the identity schema — `members` records, a per-Bot `ownerId`, the owner's `designatedChiefId`. A legacy version-1 file is migrated on load with a one-time `state.json.v1` backup; an existing backup counts only when it is a readable file holding a parseable version-1 state, and anything else (or any malformed ownership) fails closed (read-only). An older binary refuses a newer version read-only. This is schema foundation only — team mode stays disabled, see [team](team.md). On top of it, member auth (#48) adds host module `src/host/member-auth.js`, installed after `store` in `MODULES` (`core/index.js`): `invitations` and `authSessions` maps live on state (no `STATE_VERSION` bump), the channel resolves the actor from the `x-flybot-member-session` header, and endpoints are `auth-invite` / `auth-redeem` / `auth-revoke` / `auth-whoami` / `auth-remove-member` / `auth-revoke-invite` with `body.actorId` ignored; `auth-invite` alone also opens for an anonymous caller on a host with no sessions yet (bootstrap first invite), `auth-revoke-invite` kills an unredeemed invitation (redeeming it afterwards fails with `revoked`), `removeMember` refuses a member that still owns a Bot (`has_bots`), and the auth mutations are serialized behind an in-process lock so concurrent `removeMember` calls cannot both pass the last-owner check. Sign-in surface only — still not team mode; details in [team](team.md). On top of it, owner-scoped access (#49) adds host module `src/host/authz.js`, installed after member-auth: when any `authSessions` exist, the channel `authorize()` default-denies cross-owner Bot/room/session access, blocks shared non-isolatable resources (secrets, connectors, folder picks, browser bridge, update-install, diagnostics, set-prefs, `add-agent` — the DSH Agent is one per deployment and carries no owner) with `bot/blocked-shared` when more than one member exists, stamps `create-bot` with `actor.memberId`, and filters `view()`/`live()` to the actor; with no sessions the single-human behavior is unchanged. Session targets resolve through the store's owner maps — a Bot id via `botOf`, current chats and group Sessions and earlier parts via `selfOf`/`groupOwners`/`pastOwners` (a shared room Session also resolves for its current participants, #50) — and unresolvable ids deny. `approval-answer` gates on the approval's own Bot or room (resolved by approval id), never on caller-supplied session fields. `flybot/*` plugin endpoints (src/host/hooks.js) reach any signed-in actor for reads; plugin writes follow the shared rule. `view()`/`live()` narrowing (`rt.ownedView`/`rt.ownedLive` in authz.js) keeps per-session entries (`questions`, `exchangeTurns`) the actor owns, and deployment-wide surfaces (agents, secrets, connectors, memory reviews) only while a single member exists — the same rule as the shared writes; rooms the actor belongs to and the Bots in them (#50) stay visible. `create-room` refuses a request with neither a member Bot nor any participant. On top of it, room membership (#50) adds host module `src/host/room-membership.js`, installed after groups: a room may carry typed `participants` (`human` / local `bot` / granted remote `bot` with `handle` + owner fields, roles `admin` / `member`, `joinedSeq` / `leftSeq`); log lines carry `authorId` + `authorKind`; history is the membership window (current members and the owner of an invited Bot see `seq >=` their interval; leave withdraws access; rejoin starts a new interval); local cross-owner rooms accept each side's *granted* Bots (#90: default deny, Chief included); a friend's Bot from `social/remote-bots` (`cleanRemote`) may be added and ungranted Bots stay private, and a room with a friend's Bot holds only granted local Bots (a Bot that joined by room code, `viaCode`, only for that room); `view()` labels remotes with an owner badge and shows an invited owner **Shared room from \<inviter\>**. Face-to-face rooms (#83) reuse the same `participants` / membership-window / owner-badge presentation with `origin: 'face-to-face'`: room-only remotes may sit in the room without a person-grant or a friendship, `presentRoom` treats the viewer as a member (not Shared room from), leave skips the two-Bot floor, and `fanoutRoomLine` goes to `rt.faceRoomFanout` instead of friend `flybotSend`. Still not team mode — see [team](team.md). Per-Bot grants (#90) add host module `src/host/reach.js`, installed after room-membership: `state.grants[botId]` is the list of independent grants (one Bot → one grantee, optionally room-scoped); default deny, Chief included; a Bot whose owner is not a member has none. Grants are set with `rt.setGrants`, exposed as `BotView.granted`, `flybot.bot.hasGrant` / `hasAnyGrant`, `flybot.grants` and the `bot.grants` event. `reach()` is the owner policy: a friend's Bot may reach only local Bots granted to that sender, only a granted Bot (or one code-joined in that room) may message a friend's Bot, another owner's Bot is reached only if both sides have matching grants or both are members of the room the message belongs to. It is not a middleware: hooks.js runs it before the `bot.send` chain, again on approval release, and on every deliver with a sender or remote origin (resumed, retried, deferred, re-sent), so no verdict or `bypass` can skip it and a queued message is judged by what is true when it is delivered. Revoking a grant, or replacing its Chief, removes it from rooms that have friends' Bots or several owners at once (`rt.evictUnshared`). Approval binding (#52) completes the held-send half of this: `state.flybot.approvals` records carry `requestId`, a sha256 `version` over the request content, server-derived `ownerId` and `origin` (`via`/`hop`/audience) — claims in the message (`meta`, text) never confer authority. `flybot/decide` is allowed through authz for any signed-in actor and the per-record check happens in hooks (`decide` requires the sender Bot's owner's session, `bot/forbidden` otherwise, `bot/unauthorized` without a session); `release()` revalidates before dispatch and marks the record `stale` when the request changed, the sender or target Bot is gone (also invalidated eagerly on `deleteBot`), or the Bot changed owners; a demoted Chief's sender-side holds are invalidated eagerly on `setMain` (not inside `staleReason`), and `release()` re-checks immediately before dispatch so an in-flight approve cannot go out after a concurrent invalidation. Unknown `invalidateFor` field values throw. `flybot/approvals` lists only the actor's own holds while enforcement is on; decided stale records keep their attribution in state but appear in no member's list. Which statuses still wait for that decision is one rule in `src/shared/approval-waiting.js` (#754): `decide()`, `heldOf()` in the chat view, the in-flight filter that invalidates holds, `PENDING_MAX` and the `APPROVALS_KEPT` cleanup all ask `isApprovalWaiting()`, and `channel.js` counts a row's waiting replies with it, so a `pending_approval` hold (Bot Social's own spelling) is decidable, drawn as waiting and never dropped mid-wait.
 
+- One writer per home (`src/host/home-lock.js`): the first dsh to load `~/.dsh` takes `state.lock` (pid, host, a random token in the file, its mtime bumped on a timer) and a second window on the same data only reads, answering `bot/read-only` and naming the other host. Takeover is **on the next write**, not on a poll (#1027): a window that was read-only claims the free lock inside `save()` too, and a change made outside a request (a plugin, a scheduled task, a delivery) goes through `rt.mutate(fn)`, which takes over and adopts what the other host left *before* the change is made and written. `save()` itself never adopts: `adopt()` replaces `rt.state` whole, so adopting there would discard a change the caller has already made (`#1032`). A lock file still naming a live holder on this host (its pid answers) keeps the window read-only, which is the #14 rule.
+
 - `list_bots` (`src/host/tools.js`) tags a Bot with `N to approve` while one of its outbound sends is held for the owner's decision (#587): a count of that Bot's pending held sends (`state.flybot.approvals`), never the held message's text, so a coordinator reading the roster sees who is blocked and the tag clears once the owner decides. Covered by the roster case in `core/test/flybot.test.js`.
 - The Main Bot list is deployment-wide (#613): `set-main` requires the actor to own `body.id` and, when `body.replace` names the Main Bot whose place the target takes, that Bot too, so a member cannot swap another owner's Main Bot for their own or demote it (both are `bot/forbidden`); `roster.setMain` keeps the same invariant for every caller, refusing a `replace` across owners and leaving `state.mainBotIds` unchanged. A swap inside one owner's team, the plain promotion of a Bot the caller owns, and the deployment-wide list itself are unchanged.