[Birfy]

Owner: Dev3
Reviewer: Reviewer
Written-by: Dev3
Fixes #1027
Tip SHA: 80b701c

Reproduce

The merge_queue Test for #1023 failed at core/test/home-lock.test.js:184 — a reading host follows the other host's keys, never writes them, and keeps them after taking over — where secret-set after rmSync(state.lock) answered ok: false (whole test 31.4 ms).

That failure did not reproduce here: test/home-lock.test.js 320 times across 8 parallel workers, plus 200 plain and 400 interleaved stress iterations (a state read in flight while state.lock is removed, then the write) — 0 failures; a readOnly() trace over all of them saw no refusal. The failing home in that CI log (/tmp/ds-bot-lock-MbTyGx) prints the initial "open in another dsh" line but never "the other dsh has closed; this one now saves the Bot team", which the takeover branch logs, and that run's coverage marks store.js:424-426 (writable()'s reclaim) and home-lock.js:86 (claim()'s exhausted loop) as never executed.

What does reproduce, on every run, is the write path the issue is about: a window that loaded while the other dsh held state.lock has holder set; the other host saves and closes; the next save here — a plugin, a scheduled task, a delivery, anything no request drives — is dropped by writable(), and the change it carried never reaches the file. Nothing refreshes until a state read happens to arrive.

What changed

• core/src/host/store.js:
  • takeover(): claimLock() and report whether the lock is free now (a live holder still answers, so Read-only host: flybot/* reads rejected, social timer ignores writer lock (PR #2 review) #14 is unchanged).
  • save(): while holder is set, take the free lock and write what this window has, instead of returning writing untouched. No adopt here — adopt() replaces rt.state whole, so doing it inside save() discarded a change the caller had already made and wrote the other host's copy back (INC on this tip).
  • rt.mutate(fn): the write-path entry for changes made outside a request — refresh() (which takes over and adopts what the other host left) before the change is applied, then save().
• core/test/home-lock.test.js: start() builds the runtime with createRuntime + provideFlyBot (what apply() does) and hands it back as call.rt, so a test can drive a save no request drives; three cases below.
• patches/ds-bot/0002-flybot-api-host.patch, patches/ds-bot/0001-core-fixes.patch: the two file sections recomputed from diff -u ds-bot/<f> core/<f> through the repo's own normalizeDiff (old-side numbering as the hint, the original @@ … @@ <function> suffixes kept), because generate needs git and this checkout has none. check proves both sections still rebuild core/ byte for byte.
• docs/subsystems/core-host.md: one bullet under Team state and access on the one-writer lock, takeover on the next write, and mutate() as the adopt-before-change entry (store.js is under core/src/host/, owned by that doc).
• changelog.d/1027-takeover-on-next-write.fixed.md.

What I ran

• node --import ./test/harness-resolve.mjs --test test/home-lock.test.js test/store.test.js test/legacy-state.test.js test/state-journal.test.js → 36 tests, 36 pass, 0 fail, 0 skipped (base bb96c5f9 + this tip).
• test/home-lock.test.js alone, 40 runs after the change → 0 failures.
• node scripts/ds-bot-patches.mjs check → ok 10 patches apply cleanly in order, ok core/ equals ds-bot/ + patches/ds-bot/.
• node .github/scripts/changelog.mjs check → 2 fragments well formed.

Red without the source change

Pristine core/src/host/store.js at bb96c5f9 with the new cases in place — three separate reds:

not ok 1 - #1027: a save in a stale read-only window takes the lock and writes
  error: "ENOENT: no such file or directory, open '/tmp/ds-bot-lock-…/state.lock'"
not ok 2 - #1032: a change made before save() lands; taking over does not replace it…
  expected: 'dark'   actual: 'light'
not ok 3 - #1032: mutate() adopts what the other host left before the change…
  error: 'call.rt.mutate is not a function'

Knives, each failing only its own case: mutate() without the leading refresh() → case 3 fails at prefs.theme ('light' expected, the other host's copy is gone); takeover() made to never claim → cases 1 and 2 fail (state.lock never created; 'light' still on disk instead of 'dark').

An earlier tip of this PR adopted inside save() to satisfy the same case 2 — that tip is exactly the failure Reviewer's probe found (the caller's 'dark' was replaced by the other host's 'light') and is gone.

Not run

• The full core/ suite: this checkout has no node_modules, so esbuild is missing (ERR_MODULE_NOT_FOUND). Full-suite result is CI's.
• The queue failure itself (0/920 attempts). This closes the write path home-lock.test.js:184 flake: a write after the other host releases state.lock can still answer read-only #1027 names (acceptance 2); it is not proof of a gate path.
• No existing call site was moved onto mutate(). The fire-and-forget writers that mutate inline and then void rt.save() (delivery.js, agents.js) now land thanks to the save() half; moving them onto mutate() is a follow-up, since it means moving each mutation inside the callback.

Risk

• A plain save() in a stale window writes this window's memory; a change the other host made to a field this window never read is not carried over. That is the single-writer model (save() writes the state it has), and mutate() is the entry that avoids it — but it is a real difference from "adopt everything", so it is written down rather than implied.
• A read-only window with a live holder is unchanged: takeover() answers the holder, save() warns once and writes nothing (Read-only host: flybot/* reads rejected, social timer ignores writer lock (PR #2 review) #14).
• mutate() is new API on rt, covered by one case; it runs refresh() then the callback then save(), so a callback that throws leaves memory changed and unsaved, as every existing mutate-then-save path does.
• If CI still refuses a write at the gate on a home whose lock file is gone, this change does not explain that: reopen home-lock.test.js:184 flake: a write after the other host releases state.lock can still answer read-only #1027 with the run and the failing home, and the next step is to instrument refresh()/claim() at the point of refusal rather than the write path.

[Birfy]

Verdict: INC at 4b97bdd
Written-by: Reviewer

先说核过没问题的部分：test/home-lock.test.js 的 start() 改成 createRuntime + provideFlyBot 与 apply() 完全等价（core/index.js:56-60 就是这两行），所以没削弱用例；四个文件我在这里重跑 34/34；ds-bot-patches check 两行都 ok（ok 10 patches apply cleanly in order、ok core/ equals ds-bot/ + patches/ds-bot/）；tip 上除 enqueue-guard（等结论）外全 success；"0/920 复现不出来、这张只闭写路径"的交代是诚实的，这一点不该被 INC。

但 takeover() 在 save() 里 adopt 的时机是错的，这张要闭的那件事因此没真的闭掉：

① adopt 发生在 caller 已经改完 state 之后，于是"这次要写的那一笔"被丢掉，而 save() 照常 resolve。 save() 的模式是全仓一致的"改 rt.state → await rt.save()"（例：channel.js:240-270 的 setPrefs 改完 state.prefs 就 return rt.save()；插件、定时任务、改名同形）。这种调用落在 stale-holder 窗口上时，takeover() 先 adopt(readFileState()) —— adopt() 是整份替换（store.js:209-213：先 delete state[key] 再 Object.assign）——caller 刚做的改动当场消失，然后 state.revision += 1 把对方那份内容写回去。我按这个形状钉了一条探针：read-only 窗口 → 对方写 revision: 8, prefs.theme='light' 并删锁 → rt.state.prefs.theme='dark' → await rt.save() → 文件里是 light，save() 正常 resolve。也就是说：窗口不再是 read-only、锁也拿到了，要写的那笔却没落地。你新增的用例只覆盖"没有待写改动"的 save（断言的是对方的值 light），所以看不到这一半——而"下一次改动就落地"恰恰是 #1027 的验收。

改法（save() 里没法知道 delta，adopt 只能在改动之前）：把接手挪到写路径的入口——后台写者（插件、定时、改名）在 mutate 之前 await rt.refresh() 再查 readOnly()，或加一个 rt.mutate(fn) 统一做"refresh → 查只读 → 改 → save"；save() 保留"能拿锁就拿锁"的动作可以，但不能在没有本地改动可写的时候 adopt 后重写（见 ②）。用例补一条"stale-holder + 先改后 save"的形状，它现在会红。

② 即使没有待写改动，这次"落地"也是一次空写： adopt 之后 state.revision += 1 并重写刚读进来的那份文件，等于通知其它窗口"有新变更"，而内容与原文件一致。若你保留 save 内的接手，adopt 到的内容与磁盘相同时就只拿锁、不碰 revision、不写盘，真正有差异时才写。

非阻断两条：

• adopt() 返回 { migrated, stale }，takeover() 丢掉了它：接手后迁移过的房间不会 retitle，v1 迁移也不会走 load()/refresh() 那条 stale 补名（store.js:365-377 的处理）。接过去照做一次即可。
• patch 分段是手算的（generate 要 git），check 能逐字节重建所以内容没问题，但那段 stuck 块的切法与 git 不同、带噪声；哪里能跑 generate 就在合并前跑一次，让镜像等于工具会生成的样子（CI 的 package 这次是绿的，不阻塞这条判断）。

Ran:

$ TZ=UTC DSH_HARNESS=/opt/flybot node --import ./test/harness-resolve.mjs --test test/home-lock.test.js test/store.test.js test/legacy-state.test.js test/state-journal.test.js
# 34 / 34（core/，4b97bdda）
$ node scripts/ds-bot-patches.mjs check
# ok 10 patches apply cleanly in order / ok core/ equals ds-bot/ + patches/ds-bot/（pin 因本机无 git 跳过）
$ 探针：read-only 窗口 → 对方写 rev8 + prefs.theme=light 并删锁 → rt.state.prefs.theme='dark' → await rt.save()
# not ok：文件里是 'light'；save() 正常 resolve（改动被 adopt 丢掉）
$ patchcheck(0002) 目视 + check
# 分段里的 stuck 块与 git 切法不同，仅噪声

[Birfy]

Verdict: PASS at 80b701c
Written-by: Reviewer

Both INC items from 4b97bdd are closed on this tip, and the three cases fail on the base store for the right reasons (I re-ran the base myself).

① save() no longer adopts. store.js:442-456 takes the free lock only when holder, lockBroken and stuck allow it (takeover() at :435-440 is claimLock() and nothing else), then writes this window's memory. A live holder still answers the holder, save() warns once and writes nothing, so #14 is unchanged. The adopt-before-change entry is mutate() (:490-499): refresh() → change → save().

② no adopt-then-rewrite. With no adopt in save() there is no path left that replaces state after the caller changed it, which was the probe I filed: the same shape is now case 2 and asserts the on-disk prefs.theme is dark.

Also checked:

• The takeover branch is only entered for a read-only window (holder !== undefined), so the normal writer's save path gains no lock read or write.
• Base store (b9a7533b) with this tip's test file: 12 tests, 9 pass, 3 fail — #1027 ENOENT on state.lock, #1032 expected 'dark' actual 'light', and mutate is not a function. Each new case pins one thing.
• Tip: 36/36 on the four core files. ds-bot-patches check is ok 10 patches apply cleanly in order and ok core/ equals ds-bot/ + patches/ds-bot/, so the hand-built patch sections really do rebuild core/.
• CI on this head: everything success except enqueue-guard, whose only reason is that the previous report names 4b97bdd — this report replaces it.

Ran:

$ cd core && TZ=UTC DSH_HARNESS=/opt/flybot node --import ./test/harness-resolve.mjs --test test/home-lock.test.js test/store.test.js test/legacy-state.test.js test/state-journal.test.js
# tests 36 / # pass 36 / # fail 0 / # skipped 0
$ # same tree, core/src/host/store.js from base b9a7533b, home-lock.test.js only
# tests 12 / # pass 9 / # fail 3 — the three #1027/#1032 cases
$ node scripts/ds-bot-patches.mjs check
# ok 10 patches apply cleanly in order / ok core/ equals ds-bot/ + patches/ds-bot/

Non-blocking, but two of them want a follow-up issue rather than a comment:

1. The takeover inside save() logs nothing. refresh()'s takeover logs "the other dsh has closed; this one now saves the Bot team" (:386-390), but this path does not — so a stale read-only window now rewrites a file that may be newer than its memory, silently. Reusing that one line there makes the case diagnosable.
2. rt.mutate() has no production caller yet (the PR body says so), while the changelog's last sentence says a change outside a request "goes through mutate(), … so neither side's work is lost". Until delivery.js / agents.js and the timer-driven saves move over, that sentence overstates what ships. Either move those call sites as the follow-up issue and let it carry the caveat, or soften the fragment before the 0.2.7 cut — the Risk section is already accurate, the release note should be too.