fix(runtime): use process-owned credential-home locks - #613
fix(runtime): use process-owned credential-home locks#613mldangelo-oai wants to merge 4 commits into
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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". |
Let the stalled holder exit if its controller dies. Shorten credential-lock documentation and remove the outdated release-failure explanation.
|
@codex review Please review current head The runtime suite passed (129 tests, 11 platform-specific skips), as did types, formatting, and build. The normal paused-owner/crash-recovery fixture passed on Node 22.13 and Node 24.15. Parent-exit probes passed on Node 22, Node 24, and Bun; a before-change control still reproduced the orphaned holder. The broader seeded and random-order test suites and fresh CI are still running. |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
|
@codex review Please review current head |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
kmbroai
left a comment
kmbroai
left a comment
There was a problem hiding this comment.
Critical review
Reviewed 0557442c1d425b45229b506cc88cc3ba2d21ae4d. A process-owned lock addresses a real correctness defect in heartbeat-based ownership. No additional blocking code defect found, but the mixed-version rollout limitation must remain explicit.
Necessity and correctness
A paused process can still own and mutate the credential home. A stale heartbeat is therefore not proof that admitting another writer is safe. Holding an exclusive SQLite transaction in the process performing the protected work removes that expiry inference and releases ownership when the process exits.
The acquisition and release paths retain home identity checks, validate the persistent guard file, handle contention without a production expiry timeout, and close the database after acquisition errors and release failures. The protocol marker allows orphaned new-format owner records to be reclaimed only after acquiring the native lock. Conservative treatment of live legacy PID-only records is appropriate; a reused PID cannot safely be distinguished by age alone.
Simplification and compatibility
SQLite may look heavy for a mutex, but using the SQLite implementation already built into both supported runtimes avoids another dependency or helper process and provides the needed crash semantics. Keep the implementation limited to lock ownership; it should not become another state database or heartbeat service. The legacy directory remains justified only as a compatibility boundary, not a second independent source of ownership for new clients.
The old heartbeat-only client still cannot be made safe by this new client's lock: it does not participate in the SQLite protocol. A compatibility heartbeat would also fail when the new owner pauses. The PR's instruction to stop heartbeat-only operations before upgrading is therefore a real requirement, not an optional caution. Please put that upgrade requirement alongside the shipped README lock guidance as well; package users will not necessarily read the PR description. I verified that #611 is merged and the npm-v0.1.18/current-main snapshot contains the heartbeat implementation, so the older thread's characterization of it as only an unreleased preview is no longer current.
Verification
Ran the credential-lock subset with seed 12345: 9 passed, 0 failed, including the real stalled-owner/crash-with-reused-PID fixture, Bun 1.3.14/Linux with locally available dependencies and a normal 022 umask outside the sandbox. I did not rerun the built Node package smoke, native Windows locking, or a mixed-version live deployment. No credential lock files were manually removed outside synthetic fixtures.
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Blocking on the released-client compatibility issue in the existing credential-lock thread. The mixed-version probe reproduces concurrent ownership with an active new holder; the matched old/old control remains exclusive. The new-only lock tests and CI pass but do not cover this regression.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c69e65db5
ℹ️ 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".
|
|
||
| // Released 0.1.18 clients wait one heartbeat before reclaiming an aged | ||
| // directory, even when its recorded owner is still running. | ||
| await new Promise((resolve) => setTimeout(resolve, 5_000)); |
There was a problem hiding this comment.
On a loaded CI host, the holder and legacy client are separate processes, so this 5,000 ms timer can wake before the holder's 5,000 ms heartbeat callback even though its deadline was scheduled slightly later. The fixture then reports legacy acquired and fails both the runtime test and installed-package smoke test despite correct locking behavior. Synchronize on an observed heartbeat or inject the timing dependency rather than relying on cross-process timer ordering.
AGENTS.md reference: sdk/typescript/AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed this fixture race on 4c69e65db56b0034eddbd64c28071bfff7ec0dab. Using the unmodified holder/legacy modes and the fixture's forced-stale mtime, a controlled 376 ms suspension across the heartbeat deadline produced legacy acquired and replaced the owner. The actual released npm-v0.1.18 contender with natural timestamps stayed blocked for eight seconds despite a 1.1-second suspension. This demonstrates a synthetic test-ordering failure, not another production-locking regression; spontaneous CI frequency is unmeasured.
Please synchronize the fixture's heartbeat observation while retaining coverage that fails without the compatibility heartbeat. Separately, the original active-holder regression is fixed: the real mixed-version probe stayed exclusive for 38 seconds and released cleanly; 11 focused lock tests and the built Node fixture pass. Approval is held for this test concern and passing required CI.
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
3 participants
Summary
A stale heartbeat does not prove that a credential-home owner has exited: a paused process may still hold credentials, and PID reuse can prevent safe crash recovery. Hold ownership in an exclusive SQLite transaction while preserving compatibility with released 0.1.18 clients that still use the directory heartbeat.
Changes
Testing
Validated commit
4c69e65db56b0034eddbd64c28071bfff7ec0dab:bun test --timeout 30000 tests-ts/runtime.test.ts --seed 12345: 129 passed, 11 platform-specific skips, 0 failed.pnpm run types,pnpm run format,pnpm run build, andgit diff --check: passed.pnpm pack --pack-destination ../../distfollowed bypnpm run check:package <packed tarball>: passed, including 255 archive entries, public imports, NodeNext types, the installed Node credential-lock regression, 111 bundled plugin files, the bundled Codex executable, and a nested worker.12345: 1,593 passed, 30 skipped, 1 failed; the failed unrelated deep-scan reducer-deadline test exceeded its 30-second limit under concurrent local load. The identical reducer test passed in 11.28 seconds when rerun separately with the same 30-second limit.Risk and rollout
Released 0.1.18 clients do not participate in the native SQLite protocol. The compatibility heartbeat prevents those clients from taking over an active newer owner's lock, but a paused newer owner cannot refresh its heartbeat and an older client can still reclaim its directory. Finish operations using older versions before upgrading; do not rely on mixed-version protection when an owner may pause.
Legacy PID-only records cannot distinguish a live owner from a reused PID, so ambiguous records still require manual recovery after stopping all operations using the credential home. Never remove the persistent SQLite guard while an operation is running. Supported Node versions and the public CLI remain unchanged; Node may emit its existing built-in SQLite experimental warning. Cross-platform CI remains required before landing.
Public disclosure review