Skip to content

fix(file-lock): keep the Windows holder record out of *.json scans - #6164

Open
LIHUA919 wants to merge 2 commits into
loopx-project:mainfrom
LIHUA919:codex/windows-lock-holder-sidecar
Open

LIHUA919 wants to merge 2 commits into
loopx-project:mainfrom
LIHUA919:codex/windows-lock-holder-sidecar

Conversation

@LIHUA919

Copy link
Copy Markdown
Contributor

Goal And Delivered Outcome

  • Outcome basis / optional anchor: Bug report [Bug]: ephemeral lock holder sidecars race directory walks and match *.json globs, causing Windows-only test flakes #6128, including the root-cause comment that identifies the native chat admission crash.
  • Goal/source and gap: On Windows, lock_holder_path writes the holder record to a persistent sidecar named <file>.lock.holder.json beside the locked state file. It survives release, and scans that collect *.json state records read it as state. Native chat admission (ChatExternalConversations.pending()) crashes with KeyError: 'binding_id', and Windows-only test walkers flake. POSIX keeps the record inside *.lock, so Linux CI never sees it.
  • Observable before → after, with the validation row that proves it:
    • Before: with a holder sidecar beside a request journal, the second external request admission raises KeyError: 'binding_id', and glob("*.json") beside a locked file lists the sidecar.
    • After: admission succeeds and pending() lists only request journals; the Windows sidecar is <file>.lock.holder and never matches *.json. Proven by the regression_parity rows below.
  • Issue/task and intended base: Closes [Bug]: ephemeral lock holder sidecars race directory walks and match *.json globs, causing Windows-only test flakes #6128. Base main.

Author Declaration

  • Written by: model_agent (Claude Opus 5.5, Anthropic, via Claude Code), operated by @LIHUA919.

Implemented against

Criterion (spec clause) Disposition Symbol / path Test or command
The Windows holder sidecar must not be named like a state file implemented lock_holder_path in loopx/file_lock.py tests/test_file_lock.py::test_windows_holder_sidecar_is_not_a_json_state_file
Existing installs: legacy *.lock.holder.json records are migrated and still read while an older process may hold the lock implemented lock_holder_paths, _read_latest_holder_record, _persist_holder_record test_windows_holder_removes_a_legacy_json_sidecar_on_acquire, test_windows_holder_reader_prefers_the_latest_acquisition
Product walkers that must stay .json-shaped filter the holder name defensively (external chat requests) implemented ChatExternalConversations.pending tests/test_chat_conversation_bindings.py::test_external_request_listing_ignores_lock_holder_records
Lock-artifact name lists stay complete (machine-state classification, goal deletion safety check) implemented loopx/paths.py, loopx/control_plane/goals/deletion_service.py existing suites listed under Validation
POSIX holder layout and liveness semantics unchanged implemented lock_holder_liveness test_posix_holder_liveness_reads_the_lock_file_record
Patch every other *.json walker individually out_of_scope — The rename fixes them going forward; see Coverage and gaps
  • Self-check before submission:
    • Verified:
      • I read every lock_holder_path caller and every module that both takes exclusive_file_lock and globs *.json.
      • Each new test fails on the base and passes on the head.
    • Assumed, not verified:
      • Windows behavior is simulated on Linux by giving loopx.file_lock an os proxy with name == "nt".
      • Kernel locking still uses the host backend, so msvcrt itself was not exercised.

Scope And Continuation

  • Completed scope and remaining work: Complete within this scope.
  • Slice boundary / successor: N/A. A real native-Windows run of the two suites named in the issue would add confirmation; the reporter offered help.

Validation

  • Tested revision: ce95406da4b3bccd7fa69fc0923035b22fe5a9cf (the code is identical to the parent 5347064f7; the head adds only the docs commit)
  • Run state: finished
  • Input classes: synthetic
Check kind Result Public-safe evidence / limitation
regression_parity passed The four new tests/test_file_lock.py tests fail on base file_lock.py (three fail outright; the latest-record test guards the new two-name reader) and pass on head.
regression_parity passed tests/test_chat_conversation_bindings.py::test_external_request_listing_ignores_lock_holder_records fails on the base with the exact KeyError: 'binding_id' from the issue, and passes on head. It exercises the real admission path with the ordinary fixture.
unit passed Focused suites on the rebased head: test_file_lock.py, test_file_lock_cross_process.py, test_chat_conversation_bindings.py, test_collaboration_sidecar_scan.py, control_plane/test_legacy_coordination_writer_fence.py: 44 passed, 2 skipped (platform skips).
unit passed Broader run on the pre-rebase head: all test files referencing lock_holder, deletion_service, _runtime_root_has_machine_state or external_conversations gave 665 passed, 1 failed. The failure was test_delegation_stop_nested_host.py::...[sqlite-paused] under -n 4 load; it passed 3/3 serial runs on the head and 3/3 on the base, so it is unrelated.
static passed ruff on changed files; mypy (configured strict set); git diff --check; scripts/generate_semantic_inventory.py --changed-from HEAD found no new vocabulary carriers.
static passed loopx canary premerge --from-git-diff: risk-profile smokes 8/8 and the public boundary scan passed. Its semantic-vocabulary-drift-smoke initially flagged stale line numbers in project_registry_io_manifest_v1.json; this PR regenerates the manifest (two paths.py line shifts, no classification change) and the smoke now passes. I did not rerun the full premerge command after regenerating.
real_entrypoint not_run No native Windows host was available.
  • Coverage and gaps:
    • The rename closes the whole class going forward for every *.json walker. Older Windows installs keep one legacy record per lock until that lock is next acquired; external chat admission filters it explicitly.
    • Mixed-version upgrade window: an older process does not see the new name, so its diagnostics report absent for records written by a newer process. Holder records are advisory, and the kernel lock stays authoritative.
    • Legacy cleanup is best-effort: an OSError leaves the record for the next holder.

Frontend / Visual Evidence

  • UI impact: none
  • Before: N/A
  • After: N/A
  • States and viewports shown: N/A
  • Source data: none
  • Attention review: N/A, no UI change.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Refactoring (no functional changes)
  • Documentation update
  • Test update

LoopX Area

  • Control plane (goals, todos, quota, scheduler, registry, runtime)
  • Benchmark boundary (adapters, runners, verifiers, scoring, evidence)
  • Capability or extension (providers, adapters, skills)
  • Public docs or presentation surface (README, protocols, dashboard)
  • Build, packaging, installer, or CI
  • Host or runtime integration

Technical Direction

  • Direction / acceptance reference, when applicable: Core control-plane hardening (Windows runtime correctness).

Shared-authority RFC fixture impact

N/A. This PR makes no claim against the TypeScript migration or shared Goal Authority RFCs.

Boundary Checklist

  • Neither the diff nor this PR body/comments/attachments disclose private state, credentials, raw traces or verifier output, internal links, or local machine paths (including .loopx/, .codex/goals/, and live ACTIVE_GOAL_STATE.md).
  • I did not duplicate maintainer-owned benchmark work unless a maintainer split out a public issue for it.
  • I kept the change scoped to the linked issue/task.
  • I completed the visual evidence section for UI changes, or marked UI impact none.
  • Every commit includes a DCO Signed-off-by trailer (git commit -s).

Future-facing pass: applied. lock_holder_paths gives the two name-list owners (paths.py, deletion_service.py) one helper instead of hand-spelled names. I considered and deferred a shared state-directory walk helper across all *.json walkers, because the rename removes the hazard for them.

Changes runtime behavior of loopx/**, so this is left for maintainer review and merge.

🤖 Generated with Claude Code

LIHUA919 and others added 2 commits October 10, 2026 11:58
On Windows the lock holder record is a persistent sidecar beside the
locked file. It was named `<file>.lock.holder.json`, so directory scans
that collect `*.json` state records read lock metadata as state: native
chat admission crashed with `KeyError: 'binding_id'` and walker tests
flaked on Windows only.

Name the sidecar `<file>.lock.holder`. Readers still accept the legacy
name and prefer the most recently acquired record, so an older process
holding the lock during an upgrade stays visible; each new holder
removes the legacy record while it owns the kernel lock. The machine
state and goal deletion lock-artifact checks name both forms, and
external chat request listing reads only request-ref journals so a
legacy record left before migration cannot reach admission.

Fixes loopx-project#6128

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Lihua <78465742+LIHUA919@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Lihua <78465742+LIHUA919@users.noreply.github.com>

@loopx-agent loopx-agent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer: model_agent | gpt-6.1-sol | OpenAI | runtime_reported | xhigh

Approval conclusion

精确 head ce95406da4b3bccd7fa69fc0923035b22fe5a9cf;不可变基线 04bd639b4d3b6f55bba5025f266ef175806150dc。未发现阻塞该有界修复的问题。新旧目录的 native Chat 受理、完成与重放已通过真实 owner;升级期间旧 holder 的诊断和 kernel 互斥保留。

动机

Windows 上通过既有外部消息入口继续项目 Chat 的用户,以及等待锁并按 PID/operation 排查的操作者。 相同请求和目录:基线 fresh/legacy 都在受理时 KeyError;当前受理完成并同请求重放一致。旧版本进程仍持锁时,当前超时继续显示真实旧 holder PID/operation。 新旧目录的 native Chat 受理、完成与重放已通过真实 owner;升级期间旧 holder 的诊断和 kernel 互斥保留。 本 PR 对 issue #6128 提供实际可用收益,不以改名或测试数量代替受理结果。本次可独立交付 native Chat fresh/legacy 修复、旧 holder 诊断兼容和两个现有 artifact 消费者更新。旧版自身无法读新版名字、历史 Lark delivery walkers 和原生 Windows 系统调用确认属于明确剩余边界;不能把这次批准解读为全部 Windows 迁移完成。

改动思路

复用 file_lock 的 IO owner,把两个名字和 latest acquisition 读取归到同一 helper;paths/deletion 复用该名称 owner。native Chat 仅在既有24hex request_ref 边界筛选。没有新 capability/provider、控制面决策源或授权。 单纯改名不足以兼容正在持锁的旧进程;等待者必须在无法迁移时仍读到旧身份。独立实际两进程对照证明,这个 head 的兼容 reader 达到了该要求。重读 #6165 的关闭说明 后采用更早的当前 PR,不把关闭的重复实现或它的未发布结论继承为本 PR 证据。

具体改动

全 diff 八文件 +215/-28:锁 sidecar 改为 .lock.holder;新旧名字统一到 reader,按 acquisition time 选最近记录,并在取得 kernel lock 后 best-effort 清旧名;native Chat pending 按已有24hex request_ref 筛选;机器状态识别和 orphan-source 删除安全检查使用两个名字;已有 IO manifest 仅刷新 paths.py 两处行号;新增 file-lock 与实际 Chat regression 测试;协议公开解释持久名字和迁移。没有新 capability、配置、CLI、前端或授权流程。

依据改动前 docs/reference/protocols/file-lock-acquisition-v0.md(revision 04bd639b4d3b6f55bba5025f266ef175806150dc)的 Holder And Incident Records 与 Operator Recovery:正常释放保留 release time,超时保留身份并先检查后手工重试,kernel lock 始终是所有权权威。两条均 implemented:旧进程真实持锁时 B/H 超时都有其实际 PID/agent/operation;释放后 B/H 均为 released,并有 PID/release time。

关键代码讲解

  • lock_holder_paths 返回 Windows 两个名字、POSIX 原单一路径;消费者无需手写另一份列表。
  • _persist_holder_record 保留 atomic 写入;仅在已持锁时清旧 sibling,清理失败不授予接管。
  • _read_latest_holder_record 复用原字段过滤,区分 absent/unreadable;liveness 和 timeout 共用它,最新旧 holder 不会被更早的 released 新记录遮住。
  • pending 在读 binding/status 前排除非 request journal。相同合法请求重放仍只创建一个 Session。
  • _runtime_root_has_machine_state 与 _orphan_source_lock_error 接纳完整名字集合;真实隔离 registry probe 验证旧/新名不会误宣告 machine state,两种名字的 symlink 都拒绝,unknown 文件仍属 machine state。

对主干的风险

没有阻塞 finding。实际 B/H Chat 对照:基线 fresh/legacy 两种都 KeyError('binding_id');当前两种均 accepted/completed、同请求重放一致、一个 Session、pending 无 holder。真实旧 child 持 kernel flock、新 waiter 超时的测试验证互斥与诊断;child 正常退出后的 released 读回保留身份。没有向 active Goal、真实授权或用户消息写入故障数据。

本地验证:110项 focused 通过、2项平台 skip;50项 activation/deletion 通过;88项 routing/doctor 通过、3项平台 skip;2项实际 Chat 对照 head 通过而基线失败;ruff、diff-check、changed-diff advisory、完整 semantic smoke 通过,基线同一完整 semantic smoke 也通过。IO manifest 的两处行号按当前 source 验证,未降低扫描范围或门槛。mypy 配置的19个 source files 通过。未查询、轮询或等待 CI。

保留限制:测试使用 Windows filename branch 与实际 POSIX kernel/PID substrate,未运行原生 msvcrt/PID API;模型 executor 是替身,生产 typed owner、store、受理/完成/重放实际执行。历史 Lark delivery 目录若仍有旧 holder,health 和 pending_delivery_paths 在基线与当前同样 KeyError('binding_id') / KeyError('status')。这是明确未修的原有调用方,不是此 head 引入的回归;该 PR 的说明已将其它 walkers 单独迁移列为 out_of_scope,不能宣称本次完成所有历史 Lark 目录升级。旧版本自身也无法读新名字,kernel 权威和手工检查要求仍保留。

我的整体评价

APPROVE。long_horizon 与 user_experience 在这次有界 native Chat/锁诊断交付上 improved:已有用户不需新配置即可受理和重放,等待旧 owner 时仍能继续正确恢复。未来整理已通过 lock_holder_paths 与共用 reader 落在原 owner,两个 artifact 消费者也一起采用;没有必要追加全局 walker 框架或语言迁移。本次可独立交付 native Chat fresh/legacy 修复、旧 holder 诊断兼容和两个现有 artifact 消费者更新。旧版自身无法读新版名字、历史 Lark delivery walkers 和原生 Windows 系统调用确认属于明确剩余边界;不能把这次批准解读为全部 Windows 迁移完成。 批准不授予 merge、bypass 或 provider 写入权限。

English verdict: APPROVE - ce95406; real fresh/legacy Chat admission and replay repaired, old live/released holder identity and routing/deletion safety preserved.110+50+88 focused cases pass,5 platform skips; semantic B/H passes. Native Windows APIs and historical Lark journal migration remain outside this bounded result;CI not consulted.

@BigDataDZ BigDataDZ left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent verification on a real native Windows 10 host (zh-CN, cp936, Python 3.12, Node 22.23.3) at head ce95406da - I am also the reporter of #6128, and this PR resolves it.

Verified by execution on real Windows (no simulation needed):

  • lock_holder_path now returns <lock>.lock.holder - no .json suffix, so state-directory scans and glob("*.json") can no longer read lock metadata as state (the exact mechanism from #6128).
  • tests/test_file_lock.py → 20 passed, 3 skipped on Windows, including the new Windows holder layout tests (sidecar not state, legacy cleanup on acquire, latest-acquisition preference).
  • The new regression test_external_request_listing_ignores_lock_holder_records passes: pending() ignores a planted legacy .json.lock.holder.json and returns only the two real request journals.
  • Both #6128 observations are resolved: the original repro test_canonical_stopped_goal_rejects_peer_request_before_any_write - which failed flakily before the fix when the walker matched the persisted .lock.holder.json - now passes 3/3 deterministic runs.

What I could not fully exercise, with reason: the lark steward suite (test_lark_private_manager_returns.py) still errors on my host, but the signature changed from the #6128 KeyError: 'binding_id' (now fixed by the pending() journal filter) to an IndexError in the test fixture at store.list()[0], with the runtime reporting that repository npm dev dependencies are not installed on this host. That is an environment limitation here, not attributable to this diff (the PR touches none of the delegate/admission flow). Worth knowing that this suite needs npm ci --ignore-scripts before it can run on a bare Windows checkout.

The upgrade-window handling (legacy .json holders remain readable, preferring the most recently acquired record; a new holder removes the legacy name while it owns the kernel lock) covers the mixed-version case raised in #6128 discussion, and the RFC contract doc is updated to match.

Approving at head ce95406da on the evidence above. This closes the reporter side of #6128.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: ephemeral lock holder sidecars race directory walks and match *.json globs, causing Windows-only test flakes

3 participants