Skip to content

home-lock.test.js:184 flake: a write after the other host releases state.lock can still answer read-only #1027

Closed

Description

@Birfy

Owner: Dev3
Owner note: filed by Triage from Reviewer's report on #1023; Reviewer suggested Dev3 because #1012 was the same shape (a wait that gave up early).

What happened

The merge_queue Test run for #1023 (40125343913, entry 7a13f7f1) failed in core/test/home-lock.test.js:158 — a reading host follows the other host's keys, never writes them, and keeps them after taking over — at :184:

  true !== false        ← actual: false, expected: true
  at core/test/home-lock.test.js:184   assert.equal((await call('secret-set', { name: 'MINE', value: 'mine-value-1' })).ok, true)

The whole test took 31.4 ms, so this is not a slow runner. #1023 changes plugins/social only; core/ is byte-identical across the queue entry, the PR head and main 7225692e (db9be7d5 in all three), and the same core/ was green on the PR head at 19:20 — so it is a race in the takeover path, not that pull request. It was requeued once and passed.

The two candidate mechanisms (separate them before fixing)

Every endpoint call refreshes first: core/src/host/channel.js:276 await rt.refresh(), then refuses with bot/read-only when rt.readOnly() is set (:277-279). So a failed secret-set can come from either side, and the assertion cannot tell them apart:

  1. Endpoint gate — refresh() (core/src/host/store.js:381-407) did not retake the lock, so readOnly() (:64-69) still names the other host. The retake lives only in refresh()'s hadHolder branch (:385-392), never in writable() (:416-427), which for a cached holder just returns warnReadOnly() — its comment says "retake it when nobody else holds it (its file was deleted)" but the code only handles holder === undefined.
  2. Save path — commit() → rt.save() returns false because writable() saw holder !== undefined and never re-claimed; lock.owns() is false after rmSync(state.lock) at :183.

Both are timing-dependent on the lock file's presence during the two reads inside homeLock.claim() (core/src/host/home-lock.js:60-90: owns() → writeFileSync(..., { flag: 'wx' }) → on EEXIST read() → live()), and live() has a 60 s staleMs for another host — a leftover lock whose mtime the test never bumps is live by that rule, which is a second way to stay read-only.

Expected

A window that is read-only because the other dsh holds state.lock becomes writable on its next write once that lock file is gone — not on some later poll, and not only when a state call happens to adopt. If the design really is "takeover is refresh-driven", then say so in docs/subsystems/core-host.md and name the poll that drives it; a user closing the other window and getting "open in another dsh" is a bug either way.

Acceptance

  1. Instrument first: make the failing call tell the two mechanisms apart (log whether readOnly() was set at the gate or save() returned false), then reproduce: run core/test/home-lock.test.js in a high-frequency loop (≥300×) and, if it does not fail, add the interleaving the CI saw — rmSync(state.lock) between the state refresh and the secret-set — with a delay sweep; record the rate and the mechanism.
  2. Fix the mechanism the measurement names, in product code if it is (1)/(2): a write that finds the lock free must re-claim and adopt before saving, so the first write after takeover succeeds rather than a later poll landing it. Keep the reader semantics (#14): while the other host is live this host still never writes.
  3. Test: the new case fails on today's code and passes after — state which line it hits.
  4. No changelog.d/ fragment unless the fix changes user-visible behaviour (then one, fixed); Docs-not-needed: in the commit message if arch-docs fires without a doc change.

Not the same as

Activity

  1. added
    bugSomething isn't working
    agent-readyMaintainer-approved spec: an agent may take it. Only maintainers set this; never in a template
    area:automationScheduled tasks and automation
    on Oct 24, 2026
  2. Birfy commented on Oct 24, 2026

    @Birfy
    CollaboratorAuthor

    Claiming as Dev3. Branch dev3/fix/1027-home-lock-reclaim, cut from current main db56b817.

    Order Triage asked for, and why it matters: the two ways secret-set can answer ok:false are still indistinguishable in the log, so instrumenting comes first.

    1. Instrument once: channel.js:276 rt.refresh() then the entry refusal (:277-279) versus store.js writable() returning false inside save(). One run of the failing case names which one happened.
    2. Then the 300x loop with an rmSync of state.lock between two reads, so the takeover is the trigger and not the wall clock.
    3. Fix "the next write succeeds by itself": writable() (store.js:416-427) is where the comment promises a re-claim that the code only does for holder === undefined; claiming lives in refresh() (:381-407). The next write should re-claim, not wait for a poll.

    Red first, then the smallest change that makes the same loop pass; knife = put the old writable() back and the loop goes red again.

  3. added
    agent-workingAn agent has claimed this issue (the claim comment names the branch)
    on Oct 24, 2026
  4. self-assigned this
    on Oct 24, 2026
  5. removed
    agent-readyMaintainer-approved spec: an agent may take it. Only maintainers set this; never in a template
    on Oct 25, 2026
  6. removed
    agent-workingAn agent has claimed this issue (the claim comment names the branch)
    on Oct 25, 2026
  7. Birfy commented on Oct 25, 2026

    @Birfy
    CollaboratorAuthor

    done — close-out checked on merge commit 02a77af6 (current main).

    • #1027 is closed/completed; #1032 merged with merge commit 02a77af6, head is still the PASS'd tip 80b701ce.
    • changelog.d/1027-takeover-on-next-write.fixed.md is on main.
    • docs/subsystems/core-host.md:116 carries the "one writer per home" bullet in the new wording: takeover happens on the next write, not on a poll; rt.mutate(fn) is the adopt-before-change write entry; save() itself never adopts.
    • what landed on main is code, not only docs: core/src/host/store.js:434 takeover() (claims the lock, adopts nothing), :450 save() claims then writes this window's memory, :494 mutate(change) = refresh (take over + adopt) → change → save, exported at :578; core/test/home-lock.test.js carries the three cases (+65); patches/ds-bot/0001/0002 carry the mirror.
    • labels: agent-working removed from #1027, agent-pr removed from #1032.

    The two non-blocking findings from the review are now issues instead of comment-only: #1041 (a save() takeover logs nothing) and #1042 (the changelog promises mutate(), which has no production caller yet).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area:automationScheduled tasks and automationbugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions