Skip to content

fix: harden Windows file-backed locking - #19

Open
mangod12 wants to merge 1 commit into
Delego-Dev:mainfrom
mangod12:fix/windows-file-backed-locking
Open

fix: harden Windows file-backed locking#19
mangod12 wants to merge 1 commit into
Delego-Dev:mainfrom
mangod12:fix/windows-file-backed-locking

Conversation

@mangod12

@mangod12 mangod12 commented Jun 15, 2026

Copy link
Copy Markdown

What & why

This fixes a Windows-specific gap in Delego's documented file-backed consistency class.

Relevant spec sections:

  • spec.md §5, Consistency class: rate_limit exactness depends on a serialized single-writer audit ledger, and implementations MUST document which consistency class they provide.
  • spec.md §11, Concurrency: a single logical chain has one writer at a time; concurrent appenders can fork the chain, so the implementation must be clear about its concurrency guarantees.

Delego documents that, without the daemon, file-backed ledger and approval writes are serialized by an OS file lock and are corruption-safe for writers sharing one home on one host. On Windows, the implementation did not reliably provide that behavior.

The Windows lock path used msvcrt.locking() directly. Under threaded contention, msvcrt.locking(fd, LK_LOCK, 1) can raise OSError: [Errno 36] Resource deadlock avoided; file_lock() then still attempted _release(fd) even though the lock was never acquired, causing PermissionError and aborting the write. In practice, concurrent audit appends and approval creates could be dropped instead of serialized.

There was a second Windows fallback issue: CLI approve / deny / pending probe for a daemon before using file-backed state, but daemon probing touched socket.AF_UNIX unconditionally. On Windows this crashed before the intended file-backed fallback could run.

What changed

  • Windows file_lock() now serializes same-process threads before taking the msvcrt byte lock.
  • Lock acquisition retries until the byte lock is available.
  • Cleanup only unlocks after a successful acquire.
  • daemon_running() returns False on platforms without Unix domain sockets, allowing CLI commands to fall back to direct file-backed handling.
  • Added regression tests for both the failed-lock cleanup behavior and CLI fallback without socket.AF_UNIX.
  • Updated README/CHANGELOG to document the Windows fallback behavior.

This restores the documented file-backed write-integrity behavior. It does not change the protocol or normative authorization semantics.

Reproduction

On Windows / Python 3.12 before this fix:

python -m venv .venv
.\.venv\Scripts\python -m pip install -e ".[dev]"
.\.venv\Scripts\python -m pytest tests/test_cli.py tests/test_concurrency.py -q

Observed failures included:

  • test_concurrent_appends_keep_chain_valid: expected 25 audit receipts, fewer were present.
  • test_concurrent_approval_creates_are_intact: expected 25 approval records, fewer were present.
  • test_approve_echoes_what_was_approved: crashed with AttributeError("module 'socket' has no attribute 'AF_UNIX'").

AI assistance disclosure (required)

  • No AI assistance.
  • AI-assisted. Tool(s) and how used: OpenAI Codex was used interactively to reproduce the Windows failures, inspect prior PR/spec context, draft the patch, and run local verification.
  • AI-generated, human-reviewed. I have read every line and am accountable for it.

Checklist

  • Forked the repo; this PR comes from a branch in my fork.
  • pytest passes locally.
  • python examples/demo.py still shows all eight scenarios + tamper detection.
  • Tests added/updated for the behaviour changed (or explained why none are needed).
  • Updated README.md and CHANGELOG.md for any behaviour change.
  • I have not weakened any design invariant (see CONTRIBUTING.md / ARCHITECTURE.md):
    no LLM in the authorization path; no credential custody; fail-closed; approvals
    bound to fingerprint + intent and single-use; append-only signed audit chain;
    fixed evaluation order.

Protocol / spec impact

  • No normative/protocol behaviour changed.
  • Normative behaviour changed — the wire spec
    and its CTK vectors are updated (or a linked spec PR does so), and
    __protocol_version__ stays <= the spec version.

Local verification

  • python -m pytest -q -> 67 passed, 7 skipped (daemon tests skipped on Windows: Unix domain sockets unavailable)
  • python examples/demo.py -> all 8 scenarios passed, including tamper detection
  • python -m pytest tests/test_cli.py tests/test_concurrency.py -q -> 9 passed
  • repeated python -m pytest tests/test_concurrency.py -q 8 times -> 5 passed each run
  • python -m compileall -q delego tests -> passed
  • spec repo: python conformance.py -> passed
  • spec repo: python validate.py -> passed

@mangod12
mangod12 requested a review from koishore as a code owner June 15, 2026 09:14
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.

1 participant