Skip to content

fix(loop-worktree): prevent manifest entry loss during concurrent ope… - #430

Merged
cobusgreyling merged 2 commits into
cobusgreyling:mainfrom
AIMindCrafter:fix/loop-worktree-concurrent-manifest
Jul 29, 2026
Merged

fix(loop-worktree): prevent manifest entry loss during concurrent ope…#430
cobusgreyling merged 2 commits into
cobusgreyling:mainfrom
AIMindCrafter:fix/loop-worktree-concurrent-manifest

Conversation

@AIMindCrafter

Copy link
Copy Markdown
Contributor

…rations (#408)

Summary

Changes

  • New pattern or starter (followed templates/pattern-template.md + updated registry.yaml)
  • Doc / example improvement
  • Tool change (loop-audit)
  • Story (includes real failure or surprise + lesson)

Checklist (from CONTRIBUTING)

  • All required sections present for patterns
  • Links work from README, patterns/README, starters/README, docs/index
  • No secrets, tokens, internal company URLs
  • STATE.md* examples use .example suffix
  • Safety-related content references docs/safety.md
  • Ran node tools/loop-audit/dist/cli.js . (or on the starter) and addressed findings

Testing / Dogfood

  • loop-audit passes on affected starters or this repo
  • Manual review of generated state / skill output

Screenshots / Examples (if UI or command output)


This template enforces the high bar this reference is known for.

…oncurrent-manifest

# Conflicts:
#	tools/loop-worktree/package-lock.json
@github-actions

Copy link
Copy Markdown
Contributor

Thanks @AIMindCrafter for contributing a docs improvement — visible, reviewable PRs like this grow the reference for everyone.

What happens next

  • Maintainer aims for same-day review on story, adopter, and scoped docs/example PRs (CONTRIBUTING.md).
  • good first issue PRs: comment on the linked issue so we can assign and close on merge.

More ways to help

— loop-engineering maintainers

@amarjaleelbanbhan

Copy link
Copy Markdown
Contributor

The mutex + atomic-write approach here matches the design discussed on #408 — nice touch adding rollback in createWorktree (force-removing the worktree/branch if writeManifest fails after git worktree add succeeds), that covers a failure-mode I hadn't handled in my own pass at this.

One gap against the issue's acceptance criteria: the three new tests in worktree.test.mjs call createWorktree/markWorktree concurrently via Promise.allSettled within a single Node process. Node's event loop only interleaves at await points, so that doesn't exercise real OS-level contention on .manifest.mutex across separate processes — the issue explicitly asks for spawning sibling node processes (e.g. execFile against dist/cli.js, same pattern cli.test.mjs already uses) to cover the actual race. Worth adding before merge to be confident the mutex holds up outside a single event loop.

@AIMindCrafter
AIMindCrafter force-pushed the fix/loop-worktree-concurrent-manifest branch from 5ca09ab to 6ac1890 Compare July 29, 2026 18:17

@cobusgreyling cobusgreyling 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.

Review

Solid fix for concurrent manifest loss (#408).

What works

  • File-lock mutex (wx + stale recovery + 30s timeout) serializes create/mark/cleanup/gc mutations
  • Atomic write via temp file + rename for writeManifest
  • Rollback of git worktree + branch if manifest write fails after git worktree add
  • Tests cover concurrent different IDs, same ID (one success / no orphan), and concurrent create+mark

Follow-up (non-blocking for this bugfix)

  • Bump @cobusgreyling/loop-worktree patch (e.g. 1.3.1) + publish so npm consumers get the fix; main alone does not ship until tagged.

Decision: approve.

@cobusgreyling
cobusgreyling merged commit 961fb48 into cobusgreyling:main Jul 29, 2026
4 checks passed
@cobusgreyling

Copy link
Copy Markdown
Owner

Merged — thank you @AIMindCrafter!

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.

3 participants