Skip to content

B6 P2: the momentary file lock and the compare (ADR-0042 decision 10) - #733

Closed
markramm wants to merge 6 commits into
devfrom
feature/b6-p2-file-lock
Closed

markramm wants to merge 6 commits into
devfrom
feature/b6-p2-file-lock

Conversation

@markramm

@markramm markramm commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator

Refs #730

Harden lock-dir and stripe handling. P2 of B6: pyrite/utils/file_lock.py and atomic_write_text(expect=); nothing is wired into a writer yet (P3). Windows is unsupported: expect= writes fail closed there. The lock directory is per user in every environment; sharing across OS users needs PYRITE_LOCK_DIR naming one absolute root-owned sticky directory. Left: the lock_dir config setting and the index health warning (A1).


Fix round 1 (pushed 0506d2b): harden lock-dir and stripe handling

  • 32 tests in tests/test_file_lock.py pass at -n 4, and the pre-push test-affected run passed.
  • verify-red: 5 red · 27 import-only (the module is new). A guard-deletion table is the evidence: 14 guards, each with a named test that fails when only that guard is removed.
  • Added:
    • a bounded lock wait (LOCK_TIMEOUT, 10 s);
    • fork-safe in-process locks;
    • validation of expect (bytes, or a 64-hex digest);
    • case and Unicode folding of stripes;
    • tests for the unwritable-directory and owner-fallback write paths;
    • a finished-thread assertion.
  • Docs: the lock dir is per user by default and shared only when configured (PYRITE_LOCK_DIR), matching ADR-0042 §10a A1 as amended in process: retro 2026-10-03 decisions (feedback/ per entry; A1 shared only when configured; quality theme) #742.
  • Unverified: Windows (msvcrt). The shared sticky-directory path is tested as a mode only, not with a second OS user.
  • Left: the lock_dir config setting, the index health warning, and wiring (P3).

Guards: delete the guard alone, this test fails

Guard Test
compare in atomic_write_text 8 tests (chains, in-process, all compare tests)
stripe bound (% STRIPES) test_lock_files_are_bounded
in-process lock test_threads_exclude_each_other_when_the_os_lock_does_not
flock test_eight_processes_one_key_form_one_chain
O_NOFOLLOW on stripe test_a_symlinked_stripe_is_refused_and_its_target_untouched
O_EXCL (mode set only on created file) test_an_existing_stripe_keeps_its_mode
O_NOFOLLOW on directory test_a_symlinked_directory_is_refused
directory owner check test_a_directory_owned_by_someone_else_is_refused
shared-needs-sticky check test_a_world_writable_directory_without_sticky_is_refused
explicit directory mode test_a_restrictive_umask_does_not_leave_the_directory_unusable
regular-file check test_a_stripe_that_is_not_a_regular_file_is_refused
stripe opened via verified dir fd test_a_stripe_opens_relative_to_the_verified_directory
group-writable counts as shared test_a_group_writable_sticky_directory_counts_as_shared
one predicate for "others can write" (base avoided if group/other-writable) test_every_default_branch_is_per_user (incl. 2775 base)
fallback parent must not let others rename; fail closed test_the_fallbacks_parent_must_not_let_others_rename
per-user fallback (uid in path) test_every_default_branch_is_per_user
shared base directory detected test_every_default_branch_is_per_user
absolute PYRITE_LOCK_DIR test_a_relative_lock_dir_is_refused
Windows refused test_windows_is_unsupported_and_says_so
fork re-initialisation test_a_forked_child_does_not_inherit_a_locked_process_lock
case and Unicode fold test_stripes_fold_case_and_unicode_form
bounded wait test_a_wait_for_the_lock_is_bounded
expect str/type validation test_text_is_not_mistaken_for_a_digest
in-place paths compare the hard-link, unwritable-directory and owner-fallback tests in TestCompare / TestWritePaths

Stated limits (module docstring): another user can restrict a stripe they created, and any user can hold any stripe (LockTimeout); only the final component of the lock directory is checked; group-writable counts as shared; ACLs are not read; a forked child shares the open file description.

🤖 Generated with Claude Code


Fix round 2 (pushed 3abc710): harden lock-dir and stripe handling

  • Default lock dir is per user on every platform and in every environment. Where the base dir is world-writable or unset, it falls back to <tmp>/pyrite-locks-<uid>. test_every_default_branch_is_per_user covers all six branches with two uids.
  • Docs: the docstring and changelog now say that cross-user sharing works only with a root-owned sticky directory, and the stale "pending" wording is gone.
  • New tests for previously unchecked guards: the stripe opens relative to the verified dir fd, a group-writable sticky dir counts as shared, and a relative PYRITE_LOCK_DIR is refused. The dead S_ISDIR check is removed.
  • Limits:
    • stated and pinned: a symlinked parent is followed; a group-writable dir without sticky is refused.
    • stated: fork while holding the lock; ACLs.
    • Windows is unsupported for expect= and fails closed with an error naming it. The msvcrt code is removed.
  • Evidence: 39 tests pass; verify-red 5 red · 34 import-only (new module). Each guard deleted on its own makes a test fail, and the table is above.
  • Unsure: cross-user sharing is tested with modes and a simulated foreign owner, not with a second real OS user.
  • Left: the lock_dir config setting, the index health warning, and wiring (P3).
  • Note for ADR-0042 §10a A1: it still lists a Windows default. Correct the text when this lands.

Round 3, scoped and maintainer-approved (pushed df99ec6): harden lock-dir handling

  • One predicate: _others_can_rename is true for group- or other-write without the sticky bit. It is used for the lock dir and for the fallback parent, which is now checked and fails closed. _is_shared holds the underlying bits. The base check avoids any group or other write bit, sticky or not, which is stricter than the predicate.
  • Tests: a 2775 base is added to the per-user test. A new fallback-parent test covers 0777, 0770 and 2775 (refused) and 1777 and 0700 (accepted).
  • Evidence: 40 tests pass. verify-red: 6 red · 34 import-only. The guard table is updated above.
  • Text: "root-owned sticky", the Windows message, and the two shared-dir limits.

markramm and others added 5 commits October 3, 2026 13:02
… that lost a race writes nothing

ADR-0042 decision 10 with amendments A1 (per-host lock dir), A2 (1024
stripe files, bounded) and A5 (compare before the in-place truncate).
Not wired into any writer yet.

Refs #730

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@markramm
markramm force-pushed the feature/b6-p2-file-lock branch from 3abc710 to 83bcb6d Compare October 3, 2026 17:02
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@markramm

markramm commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by the maintainer's decision on Andon pyrite-security#97: the lock lives in <git-dir>/pyrite/locks (ADR-0042 §10a A1, as amended in #748). It is rebuilt on feature/b6-p2-git-dir-lock. The compare-under-lock, stripe-bound, fork-safety and expect-validation work here carries over; the lock-dir derivation from the environment does not.

Conductor (automated agent).

@markramm markramm closed this Oct 5, 2026
markramm added a commit that referenced this pull request Oct 6, 2026
lock_dir_for(path) is the one answer to where the lock is:
<git-dir>/pyrite/locks of the work tree holding realpath(path), beside
git's index.lock. It follows git's discovery rules (gitfile, commondir,
HEAD validity, bare/inside-git-dir refused, stop at a filesystem boundary)
without running git and without reading the environment: with GIT_DIR
exported, 'git rev-parse' names another repository for the same file, two
lock dirs, failing open. git rev-parse costs 22-27 ms a call on macOS and
may be absent on a server, so the tests use git as the reference instead,
comparing on every adversary layout.

The lock dir gets the git dir's mode (never wider, umask-independent);
a read-only git dir, a widened lock dir, a move across repositories and a
KB outside git are refused for expect= writes.

Carried over from #733: atomic_write_text(expect=) with FileChanged /
LockTimeout, the compare under the lock before the rename and before the
truncate on the three in-place paths (A5), expect validation, a bounded
wait, register_at_fork, N=1024 case- and Unicode-folded stripes opened
O_NOFOLLOW/O_EXCL relative to a verified dir fd, the process tests.
Dropped: the env-derived lock dir, PYRITE_LOCK_DIR, the uid fallback and
the shared-sticky-dir logic.

Refs #730

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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