Skip to content

Add session-fenced collaborative claims - #34

Closed
Deicyde wants to merge 5 commits into
facebookresearch:mainfrom
VivienCabannes:split/13-collaborative-claims
Closed

Deicyde wants to merge 5 commits into
facebookresearch:mainfrom
VivienCabannes:split/13-collaborative-claims

Conversation

@Deicyde

@Deicyde Deicyde commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add explicit claim acquire, renew, release, list, and cleanup commands
  • fence collaborative leases with exact Git-ref CAS and per-worktree session receipts
  • separate durable article claims from resource claims and migrate legacy refs conservatively
  • pin local repositories and shared scratch state to retained directory generations
  • bound Git transfer, object promotion, subprocess time and output, scratch storage, ref batches, messages, and retries
  • reject implicit stealing and avoid remote mutation outside explicit claim commands

Depends on #28. The reviewable change is commit e511514; earlier commits belong to the dependency stack.

Validation

  • focused claims and CLI suite: 241 passed
  • Python 3.13 full suite: 829 passed, 4 skipped
  • Python 3.10 full suite: 830 passed, 3 skipped
  • installed-wheel local bare and file:// acquire/list/release smoke tests
  • make lint
  • make check-example
  • git diff --check
  • independent exact-diff review: SHIP

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Sep 21, 2026

@Deicyde Deicyde left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The CAS mechanics are careful, and all 241 focused tests pass, but I found six protocol/safety gaps on the exact head:

  1. A relative local origin is kept lexical, then the pinned worktree is closed before pin_claim_repository() resolves it (__main__.py:729-732, 1132-1137). An ancestor A-B-A swap in that gap made claim acquire write the claim ref to a different repository. Resolve and pin the derived local repository while the worktree binding is still retained.

  2. Markerless scratch migration is destructive by shape alone (claims.py:1042-1077, 1619-1631). Pointing --scratch at an unrelated empty bare repo silently installed Autoform's marker and erased custom config; a normal bare repo with refs/heads/main was still modified before the command rejected it. Validate owned namespaces and legacy-specific evidence before any write, or require explicit migration.

  3. Claim refs may be symbolic. ls-remote at claims.py:1677-1685 reports the target OID without preserving that fact, then push at 2326-2339 dereferences the ref. In a supported local bare repository, making a claim ref point at refs/heads/main caused acquire to create main and release to delete it. Refuse symbolic claim refs or use a mutation mechanism that cannot follow them.

  4. release() has no exact lease argument and trusts the latest receipt for the whole worktree (claims.py:2748-2768). After process A's lease expired and process B reacquired the key in the same default session, delayed A.release() returned true and deleted B's successor. Carry the lease_id/ClaimFence through release just as heartbeat renewal does.

  5. The v1 compatibility scan is not atomic with the v2 CAS (claims.py:2541-2553, 2622-2661). Installing a live historical v1 ref after the post-push scan lets acquire return true with simultaneous v1/v2 owners; holds() immediately returns false. The migration fence needs an atomic multi-ref transition, or mixed-client operation must be rejected by a stronger invariant.

  6. Process cleanup sends SIGKILL only when the direct child survives SIGTERM (claims.py:574-590). A leader that exits promptly while its grandchild ignores SIGTERM leaves that grandchild running after timeout/output-limit cleanup. Kill/check the process group after the grace period regardless of the leader's status.

These are independent of the #28 dependency. Validation: tests/test_claims.py tests/test_claim_cli.py passed, 241 tests.

@Deicyde Deicyde added the awaiting author Review is complete and author action is required label Oct 3, 2026
@Deicyde
Deicyde marked this pull request as draft October 3, 2026 00:47
@Deicyde

Deicyde commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

Moved to #72. VivienCabannes/autoform-bot is being deleted, so this PR now lives on the same-named branch in facebookresearch/autoform-bot (head e511514, unchanged). Please continue review on #72; new commits go to facebookresearch:split/13-collaborative-claims, not the fork.

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

Labels

awaiting author Review is complete and author action is required CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant