Skip to content

fix: recover stale run coordination locks - #29

Merged
RobertTLange merged 20 commits into
mainfrom
fix/issue-28-run-coordination
Aug 30, 2026
Merged

fix: recover stale run coordination locks#29
RobertTLange merged 20 commits into
mainfrom
fix/issue-28-run-coordination

Conversation

@RobertTLange

Copy link
Copy Markdown
Owner

What changed

  • replace PID-file run and node locks with renewable leases that recover after crashed owners
  • keep async message ownership with a detached worker and prevent takeover while its process tree is still alive
  • preserve signal forwarding and safely fail closed when Windows process-tree termination cannot be confirmed
  • retry transient Windows run-state replacement failures and clean random, exclusively-created temporary files
  • add Windows Node 22/24 run-storage CI, including a native exclusive-sharing-lock regression

Why

Stale run.lock and session.lock files could permanently block run coordination after a process crash. Windows could also transiently reject replacement of run.json, leaving coordination updates unreliable.

Closes #28.

How to test

npm run build
node --import tsx --test tests/run-storage.test.ts
node --import tsx --test --test-name-pattern='run message --async|CLI forwards parent signals' tests/run-coordination.test.ts tests/headless.test.ts
npm run pack:check
npm audit --omit=dev

Verification

  • run-storage: 24 passed locally, 1 native Windows test skipped until CI
  • focused async coordination and signal regressions passed
  • scoped coverage: 92.54% lines, 82.42% branches, 91.30% functions
  • 304 unaffected tests passed
  • package dry-run passed
  • dependency audit: 0 vulnerabilities
  • reliability, regression, and security reviews found no remaining material issues

npm run check is currently blocked by unrelated tmux tests with fixed May 2026 transcript timestamps and an existing cleanup-duration assertion. Both failure modes reproduce on the untouched da89bad base commit; they are not introduced by this PR.

@RobertTLange

Copy link
Copy Markdown
Owner Author

Review findings fixed and pushed, one scoped commit per fix:

  • 0cafee6 — preserve live node-lock owners after proper-lockfile lease cleanup; add post-acquire owner recheck.
  • f1da717 — transfer ownership to a gated, detached wrapper process; keep signal-resistant agents covered; scrub internal ownership env.
  • 37a0d67 — roll async startup failures from busy to failed, including partial state writes.
  • 8df0d75 — make Windows process probes fail closed; add stable Linux/macOS/Windows process-start identities and PID bounds.
  • 46cb02b — on Windows PID reuse, probe surviving descendants before releasing ownership.
  • 70ce60e — run the new process-probe suite in native Windows CI (Node 22 and 24).
  • ea5d952 — acknowledge durable async startup and repair parent/worker interruption windows without cancel/disconnect status races.

Validation:

  • Final read-only correctness, regression, reliability, security, and maintainability reviews: clear.
  • Build: passed.
  • Changed/focused suites: 53 passed, 1 Windows-only local skip.
  • Package dry run: passed.
  • npm audit --omit=dev: 0 vulnerabilities.
  • Full local npm run check was attempted; existing clock-sensitive tmux transcript fixtures failed/timed out in the current Aug 2026 environment. Affected suites pass; GitHub CI is now running as the authoritative full gate.

@RobertTLange

Copy link
Copy Markdown
Owner Author

CI follow-up fix pushed in 73113db (fix: pass Windows probe PIDs reliably).

Windows revealed that powershell.exe -Command consumed the trailing PID as command text, so $args[0] was empty. That made start-identity lookup miss the real process and descendant lookup start at PID 0.

Fix:

  • validate PIDs as positive uint32 values before building either probe
  • embed the validated decimal PID in the PowerShell script
  • keep process-tree ambiguity fail-closed
  • add command-shape regressions proving the PID is embedded and $args is absent

Validation: TypeScript build passes; run-storage suites pass 37/37 locally with the one native-Windows-only test skipped; diff check clean; four independent review passes found no actionable issues. Replacement CI is now running.

@RobertTLange

Copy link
Copy Markdown
Owner Author

Final review summary — all findings fixed and pushed.

Review findings and fixes:

  1. Live node owners could be bypassed after lease-directory cleanup; added unconditional owner-sidecar recheck and preserved live ownership (0cafee6).
  2. Async worker released the node lock before the detached agent finished; transferred ownership through the wrapper and kept the successor process tree authoritative (f1da717).
  3. Post-prepare async startup failures could leave nodes busy; persisted failed state before rethrow while preserving the original error (37a0d67).
  4. Windows process-probe errors/noise could release locks; made ambiguous results fail closed and accepted only explicit sentinels (8df0d75).
  5. Reused Windows wrapper PIDs could hide surviving descendants; probe descendants on identity mismatch (46cb02b).
  6. Native Windows process-probe behavior lacked CI coverage; added Node 22/24 Windows storage jobs and workflow guards (70ce60e).
  7. Parent success could race worker startup; added a started acknowledgement and repaired pre-start disconnects without overwriting explicit cancellation (ea5d952).
  8. Windows PowerShell -Command consumed trailing PID arguments, causing PID 0 probes; validate and embed decimal uint32 PIDs in generated scripts (73113db).
  9. A descendant probe could count its own PowerShell observer; exclude only the observer PID while retaining every other ambiguous descendant (a3e84f0).
  10. The native PID-reuse fixture used the test runner PID and inherited unrelated children; isolate it with a ready, identity-observable child process and race-safe cleanup (134eb8b).

Validation:

  • focused storage suites: 37 pass locally, 1 native-Windows-only skip
  • TypeScript build and diff check pass
  • review/security/reliability/regression passes clear after each fix
  • GitHub Actions run 33272517449: all 6 jobs green
    • Node 22 / Node 24 full checks, package contents, executable smoke tests
    • Windows storage Node 22 / Node 24
    • Python 3.10 / Python 3.14

No existing review threads were present to reply to or resolve. PR merge state: clean.

@RobertTLange
RobertTLange merged commit 5253494 into main Aug 30, 2026
11 of 12 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.

Run coordination: stale run.lock never reaped; renameSync fails with EPERM on Windows

1 participant