Skip to content

fix(install): use the cross-platform file lock for workflow skill install - #3392

Merged
huangruiteng merged 1 commit into
huangruiteng:mainfrom
luantaraschi:fix/cross-platform-workflow-skill-install-lock
Aug 21, 2026
Merged

fix(install): use the cross-platform file lock for workflow skill install#3392
huangruiteng merged 1 commit into
huangruiteng:mainfrom
luantaraschi:fix/cross-platform-workflow-skill-install-lock

Conversation

@luantaraschi

Copy link
Copy Markdown
Contributor

Summary

  • _exclusive_install_lock imported fcntl unconditionally, so loopx workflow-skills --install aborted on native Windows before doing any work.
  • It now goes through exclusive_file_lock from loopx/file_lock.py, which is where the fcntl/msvcrt contract already lives, so mutual exclusion is preserved rather than dropped on the platform that lacks fcntl.
  • The lock file stays where it has always been. exclusive_file_lock appends .lock to the path it guards, so guarding <skills_dir>/.loopx-workflow-skills produces <skills_dir>/.loopx-workflow-skills.lock.

Issue Or Task

Validation

Run on native Windows 11, CPython 3.13.7, no WSL.

  • python -m pytest tests/test_workflow_skill_install.py -> 6 passed. Two of those four pre-existing tests were failing on Windows before this change, for the same reason the CLI was: test_install_is_idempotent_and_uninstall_removes_managed_skills and test_uninstall_preserves_locally_modified_skill.
  • Every test file that touches workflow_skill_install, file_lock, skill_install_readback or slash_command_install: 130 passed, 3 failed, 3 skipped. The same three fail on the unpatched tree, so they are not from this change: test_file_lock.py::test_stalled_holder_times_out_and_records_independent_incident, test_loopx_turn_journal_inspection.py::test_inspection_returns_versioned_allowlisted_projection_without_mutation, and test_windows_install.py::test_windows_installer_rolls_back_late_user_surface_failure (that last one wants pwsh on PATH).
  • python -m ruff check on the touched files and on the paths CONTRIBUTING lists: clean. python -m mypy: no issues in 12 source files. git diff --check: clean.
  • End to end against a scratch CODEX_HOME: workflow-skills --install succeeds, a second run reports every skill unchanged with entry: unchanged and after.ready: true, doctor reports skill_delivery_status: ready with all five required skills valid, and --uninstall removes them cleanly.

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

  • Core control-plane hardening

  • Long-horizon benchmark evidence

  • Operator surface and IM integration

  • Shared Goal Authority and cross-host coordination

  • Architecture and research incubator

  • Target base branch: main

  • Direction tracker or promotion unit:

Boundary Checklist

  • I did not commit .loopx/, .codex/goals/, live ACTIVE_GOAL_STATE.md, credentials, private benchmark traces, verifier output, raw agent sessions, internal document links, or local machine paths.
  • 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.
  • Every commit includes a DCO Signed-off-by trailer (git commit -s).

Two notes

The coverage follows the triage note about exercising the Windows backend. test_install_takes_the_windows_lock_branch_without_fcntl forces file_lock.fcntl to None and swaps in a byte-range stand-in, so the msvcrt branch is asserted on any host rather than only on Windows CI. test_install_serializes_on_the_shared_workflow_skill_lock records the lock and asserts the readback write happens inside the hold and that the guarded path is the historical one.

One behaviour difference worth calling out before review: the old lock was a blocking flock(LOCK_EX) with no deadline, while exclusive_file_lock uses the MUTATION policy's five second timeout and then raises LockAcquireTimeoutError with holder information. I kept the shared default rather than passing a custom timeout, since every other caller in the repository does the same and the install itself copies roughly 265 KB of skills, but if you would rather the installer wait longer than a concurrent install can take, that is a one-argument change.

While validating I also hit something unrelated: tests/test_contract_scan_unreadable_files.py cannot even be collected on Windows, because a module-level pytest.mark.skipif calls os.geteuid(), which does not exist there. I left it alone since it is outside this issue.

…tall

`_exclusive_install_lock` imported `fcntl` unconditionally, so
`loopx workflow-skills --install` raised ModuleNotFoundError on native
Windows CPython and the install could not run at all.

`exclusive_file_lock` in `loopx/file_lock.py` already owns the
fcntl/msvcrt contract the rest of the repository uses, so the installer
reuses it instead of growing a second backend. The lock file stays at
`<skills_dir>/.loopx-workflow-skills.lock`, since the helper appends
`.lock` to the path it guards.

Closes huangruiteng#3355

Signed-off-by: Luan Taraschi <130802253+luantaraschi@users.noreply.github.com>

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

结论:未发现阻塞项,建议合入(APPROVE)。评审基于 exact head 41638b80b8e2a683197ad701ddd82c24fb122f81

动机

#3355 的根因很明确:loopx workflow-skills --install 进入 _exclusive_install_lock() 后无条件导入 POSIX-only 的 fcntl,因此 native Windows 在任何复制或 readback 写入发生前就直接失败。受影响的是公开支持的 Windows/PyPI 安装路径,结果是工作流技能完全无法安装。较小的“Windows 上跳过锁”方案虽然能消除 import error,却会削弱并发安装时对技能目录和 readback 的互斥保护;本 PR 复用仓库已有的跨平台锁,是更小且语义更完整的修复。非目标保持清楚:不改变技能来源发现、安装内容、卸载语义或 readback schema。

改动思路

调用链仍是 CLI workflow-skillsworkflow_skill_install()_exclusive_install_lock() → 逐项复制、materialize $loopx entry、写入 install readback → 锁外重新 inspect。变化只发生在锁的实现所有权:安装器不再自己实现 fcntl.flock,而是把 <skills_dir>/.loopx-workflow-skills 交给 exclusive_file_lock();后者统一决定 POSIX fcntl 或 Windows msvcrt backend,并继续生成历史兼容的 .loopx-workflow-skills.lock

正向路径上,Windows 缺少 fcntl 时会走 msvcrt byte-range lock,持锁完成技能复制、entry materialization 和 readback 后释放;测试同时证明 readback 写入仍在锁内。负向路径上,若已有 holder 超过 MUTATION policy 的 5 秒期限,新调用会抛出带 holder/incident 信息的 LockAcquireTimeoutError,由 CLI/operator 重试,而不是像旧实现一样无限阻塞。

具体改动

  • 生产代码:loopx/workflow_skill_install.py 共 +9/-9。新增 _INSTALL_LOCK_STEM,并让 _exclusive_install_lock() 委托现有 exclusive_file_lock();没有新增模块或重复 backend。
  • 测试:tests/test_workflow_skill_install.py 新增 67 行,覆盖无 fcntl 时的 Windows backend 分支,以及安装 readback 确实发生在共享锁持有期间。
  • 文档、生成文件和机械搬移:无。

关键代码讲解

  1. _exclusive_install_lock():仍负责创建目标目录并定义安装事务边界,但把 kernel lock、超时、holder receipt 和释放交给共享 helper。输入是 skills_dir,副作用范围仍覆盖所有实际安装写入。
  2. exclusive_file_lock()(现有活跃实现):通过 typed LockAcquisitionPolicy.MUTATION 选择 5 秒超时和 retry 语义;_try_acquire_kernel_lock() 在 POSIX/Windows 间选择 fcntl/msvcrt,finally 路径记录 release 后释放 kernel lock。
  3. workflow_skill_install():这是活跃生产 caller;entry preview 通过后进入锁,复制全部 packaged skills、materialize entry 并写 readback,锁外 inspect 形成最终可观察结果。
  4. test_install_takes_the_windows_lock_branch_without_fcntl()test_install_serializes_on_the_shared_workflow_skill_lock():分别固定跨平台 backend 选择和安装事务边界,避免未来再次把 Windows 支持或 readback 保护拆掉。

对主干的风险

主要行为差异不是锁文件位置,而是等待策略:旧 flock(LOCK_EX) 可无限等待,新共享 MUTATION policy 最多等待 5 秒。PR 描述已明确披露该差异;对约 265 KB 的本地技能复制而言,共享默认值是合理且与仓库其他 mutation caller 一致的选择。最强回归场景是慢盘或异常 holder 让并发安装超过 5 秒,此时 blast radius 限于本次安装失败,不会在未持锁时继续写;holder/incident receipt 提供可观测性,恢复方式是检查 holder 后重试。若后续真实数据证明 5 秒不足,最小修复是在此 caller 显式传入更长 timeout,并增加慢 holder 回归测试。

typed-state 检查通过:policy 由 LockAcquisitionPolicy enum 表达,没有 substring denylist 或散落布尔分类。domain-neutrality 检查通过:通用锁错误与 policy 保持 goal-agnostic,workflow_skill_install 仅作为 operation label。默认行为变化已在 PR 正文披露;本 PR 不修改 work-lane 或强制 obligation,guidance-vs-obligation 不适用。

验证:本地 exact-head 运行 pytest -q tests/test_workflow_skill_install.py tests/test_file_lock.py 为 9 passed;ruff checkgit diff --check 通过。GitHub required checks 全绿,包括 native Windows PowerShell job;两个发布 job 按条件跳过。未在本机独立运行真实 Windows kernel backend,但作者的 native Windows 结果与 GitHub Windows CI共同覆盖了该剩余风险。

我的整体评价

这是范围合适、可回退且复用既有能力所有权的修复。76 行增量主要是两条持久回归测试,生产改动反而删除了平台专属实现;active caller、正负路径和兼容锁路径都有证据。没有发现阻塞性 correctness、权限、隐私或兼容问题,残余风险集中在共享 5 秒 timeout 的实际慢盘表现,已有明确失败模式和后续最小修复路径。因此对该 exact head 给出 APPROVE;合并仍由独立 merge policy 决定。

English verdict: APPROVE for exact head 41638b80b8e2a683197ad701ddd82c24fb122f81. The PR replaces the Windows-breaking unconditional fcntl import with the repository's typed cross-platform lock, preserves the historical lock path and install transaction boundary, and adds focused backend/serialization regressions. Local validation passed 9 focused tests, Ruff, and diff-check; required GitHub checks, including Windows PowerShell, are green. Residual risk is limited to the disclosed change from indefinite waiting to the shared 5-second mutation timeout.

@huangruiteng
huangruiteng merged commit e089e1a into huangruiteng:main Aug 21, 2026
7 checks passed
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.

workflow-skills --install crashes on native Windows: unconditional import fcntl in _exclusive_install_lock

2 participants