fix(runtime): use process-owned credential-home locks - #613
Conversation
|
@codex review Please review current head |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 379a59e3f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Please re-review current head The PR now explicitly requires landing the corrected combined tree and stopping any heartbeat-only preview processes first. Please verify whether the mixed-version finding remains a blocker under that rollout, and check the corrected tree for any other issues. The reasoning and compatibility evidence are in the existing review thread. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
379a59e to
5ab8e68
Compare
|
@codex review Please review rebased head The runtime suite (129 passed, 11 platform skips), types, formatting, build, and the built Node 22.13 paused-owner/crash-recovery regression passed again. Please check integration with the current base and the documented requirement to stop heartbeat-only operations before upgrading. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Follow-up to the now-merged #611, rebased onto
mainat79734fd92c9d78ea1e42b2e155c41820dc9d22f6.Reused PIDs are a real recovery problem, but an expired heartbeat does not prove that the owner exited. A paused process or an older client can still be using the credential home when heartbeat expiry admits another writer. This replaces heartbeat expiry with a process-owned lock.
Changes
Testing
Rebase validation at
5ab8e682a90633a1731c99cd58a580f43a7c1431:git range-diffconfirms the follow-up patch is unchanged. Production source, runtime tests, package fixture, README, and dependency lockfile match the previously validated implementation.git diff --check: passed.Before the rebase, the same implementation passed the full Bun suite (1,576 passed, 30 platform-specific skips), packed-artifact inspection, installed-package smoke, Node 24.15.0 validation, and all 27 CI checks. Eight concurrent Node/Bun processes completed 80 protected updates without overlap, and pre-heartbeat/current compatibility probes waited for release in both directions. Fresh CI and Codex review must be evaluated on the rebased head.
Risk and rollout
This PR now targets
maindirectly; the former parent PR #611 is already merged. Stop operations using the heartbeat-only implementation before upgrading: those clients do not participate in the native lock and can still steal a paused owner's directory, with or without a compatibility heartbeat.Legacy PID-only records cannot safely distinguish a live owner from a reused PID, so ambiguous records still require manual recovery after stopping all operations using the home. The persistent SQLite guard must not be removed while operations are running. Node 22 emits its built-in SQLite experimental warning; the supported Node range is unchanged. Cross-platform CI is required before landing.
Public disclosure review