Skip to content

fix(runtime): use process-owned credential-home locks - #613

Open
mldangelo-oai wants to merge 1 commit into
mainfrom
mdangelo/codex/credential-lock-ownership
Open

fix(runtime): use process-owned credential-home locks#613
mldangelo-oai wants to merge 1 commit into
mainfrom
mdangelo/codex/credential-lock-ownership

Conversation

@mldangelo-oai

@mldangelo-oai mldangelo-oai commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to the now-merged #611, rebased onto main at 79734fd92c9d78ea1e42b2e155c41820dc9d22f6.

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

  • Hold an exclusive SQLite transaction in the process preparing the scan runtime, using Node's or Bun's built-in SQLite. No new dependency, helper process, CLI surface, or lock timeout.
  • Keep the existing owner directory for older clients. Mark new records so an available native lock lets us reclaim them regardless of PID reuse. Preserve conservative handling of live legacy PID-only records.
  • Keep the guard file between operations, validate its path and permissions, and close the connection on cancellation and acquisition/release errors.
  • Replace heartbeat timer mocks with a real stalled/crashed-owner regression used by both Bun tests and the installed Node package smoke test. Document legacy recovery.

Testing

Rebase validation at 5ab8e682a90633a1731c99cd58a580f43a7c1431:

  • git range-diff confirms the follow-up patch is unchanged. Production source, runtime tests, package fixture, README, and dependency lockfile match the previously validated implementation.
  • Runtime suite: 129 passed, 11 platform-specific skips, 0 failed.
  • Typecheck, formatting, build, and git diff --check: passed.
  • Built-runtime paused-owner/crash-recovery regression on Node 22.13.0: 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 main directly; 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

  • No customer, partner, prospect, or user identities, data, or identifying details are included.
  • No credentials, personal data, private source, scan findings, or nonpublic links or tickets are included.
  • I reviewed the branch name, title, description, commits, changes, comments, logs, screenshots, attachments, and links for public disclosure.

@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review

Please review current head 379a59e3f83106c4d05c479d5571086072e63a11, especially native lock lifetime, paused/crashed owners, conservative legacy PID handling, cancellation/error cleanup, and Node/Bun behavior on Windows and Unix. This is stacked on #611; the regression fixture also runs against the installed npm package.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 22, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-08-22T08:00:40.528545Z 5ab8e68 Manual request
🔒 Security Review Completed 2026-08-22T08:00:06.833119Z 5ab8e68 Draft marked ready
ℹ️ 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" or "@codex security review".

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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".

Comment thread sdk/typescript/src/runtime.ts
@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review

Please re-review current head 379a59e3f83106c4d05c479d5571086072e63a11 with the clarified rollout requirements. The code is unchanged and all 27 CI checks passed. #611 is an unmerged review base, not a release to deploy before this PR. The latest released tag (npm-v0.1.16) has PID-only locking; two-process probes with that pre-heartbeat protocol and this head waited correctly in both directions.

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.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

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".

@mldangelo-oai
mldangelo-oai changed the base branch from mdangelo/codex/pr611-base to main August 22, 2026 07:57
@github-actions github-actions Bot added the bug Something isn't working label Aug 22, 2026
@mldangelo-oai
mldangelo-oai force-pushed the mdangelo/codex/credential-lock-ownership branch from 379a59e to 5ab8e68 Compare August 22, 2026 07:57
@mldangelo-oai
mldangelo-oai marked this pull request as ready for review August 22, 2026 07:58
@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review

Please review rebased head 5ab8e682a90633a1731c99cd58a580f43a7c1431 against main. #611 is now merged, so this PR is no longer based on the temporary mirror. git range-diff confirms the follow-up patch is unchanged.

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.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: 5ab8e682a9

ℹ️ 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".

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant