Skip to content

fix(media-use): serialize concurrent preference records - #5366

Open
user-github-me wants to merge 8 commits into
heygen-com:mainfrom
user-github-me:fix/concurrent-preference-records
Open

user-github-me wants to merge 8 commits into
heygen-com:mainfrom
user-github-me:fix/concurrent-preference-records

Conversation

@user-github-me

@user-github-me user-github-me commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

What changed

Preference records serialize each read-modify-write with the existing per-file lock. Concurrent records keep every project preference and shared personal sighting, so confirmations can promote reliably after two projects. Temporary files are cleaned up without masking an original write error.

The standalone and packaged copies stay identical. The media guide and skill reference explain recovery from an abandoned lock and keeping transient lock files out of version control. A locked personal tier leaves the project record intact and defers personal promotion.

Fixes #5333.

Validation

  • Refreshed against current main, retaining its media-home resolution and regenerating the manifest for the seven published skills.
  • Fresh full workspace build and CLI typecheck passed.
  • 27 preference, concurrent-writer, config-lock and copy-parity tests passed, including held/abandoned locks and EPERM/EBUSY cleanup cases.
  • Skills manifest, lint, formatting, test reachability, normal commit hooks and both comment checks passed.

The Windows engine/CLI lane and its full workflow passed on the current commit f072486a. The earlier browser timeout is superseded by this successful run.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, this fixes #5333 and I'd like to land it. I'll approve as soon as the conflict below is resolved.

What I checked:

  • prefs-store.concurrent.test.mjs 5/5 passes at head, and 17/17 together with the existing prefs-store tests. With main's prefs-store.mjs swapped back in, all 5 new tests fail in 5 of 5 runs. The ready barrier keeps the 8-writer race tight, and the held-lock cases are deterministic.
  • An out-of-tree probe ran 25 trials of 8 parallel node prefs.mjs record processes (2 projects × 4 keys). Main had 8 ENOENT exits, 18 lost project keys and 14 lost user sightings, with 13/25 trials losing data. This head had no failures, no lost keys or sightings, and no leftover .tmp/.lock files.
  • The standalone and CLI prefs-store.mjs copies are byte-identical, and the manifest hash at head matches the tree.

Before merge: the branch conflicts with main only in skills-manifest.json. Please rebase and rerun bun run --cwd packages/cli gen:skills-manifest, since main's media-use hash has moved since your base.

Non-blocking notes:

  • prefs-store.mjs:79-80: the finally rmSync(tmp, { force: true }) is unguarded. An EPERM/EBUSY there, most likely on Windows, would mask the original error or fail a write that already succeeded. Wrapping it in try/catch would be safer.
  • withFileLock never takes over a lock. A record killed inside the critical section leaves .media/preferences.json.lock, and later records for that project fail with "Settings are locked…" until someone deletes the file. That matches the helper's config.json policy and your abandoned-lock test, so I'm fine with it. It may be worth a mention in the prefs docs, and maybe .media/*.lock in the project's ignore guidance, since .media/ is committed.

This supersedes #5460, which uses the same lock but only patches the standalone copy.

— Rames

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving at e4c6891f. Thanks for working through both notes. The conflict is resolved and both non-blocking points are addressed.

Since my last review at 4b778034:

  • Merged current main (d5cdf418). The only conflict was skills-manifest.json, and gen-skills-manifest.ts --check now reports it in sync (21 skills). main hadn't touched prefs-store.mjs, config-lock.mjs or memory.md since your base, so the merge didn't change the fix.
  • prefs-store.mjs:79-82: the finally cleanup is now wrapped in try/catch. I checked the four new EPERM/EBUSY tests in prefs-store.test.mjs by putting the unguarded rmSync back. All four failed: two because a successful write turned into a failure, and two because the cleanup error hid the ENOSPC. With your change they pass.
  • skills/media-use/references/memory.md: it now documents recovery from an abandoned preferences.json.lock and the .media/*.lock ignore guidance. The claim that "a locked personal tier does not fail the project record" matches recordPreference, which catches the error from recordUserSighting (prefs-store.mjs:191-195).

At this head, the CLI and standalone prefs-store.mjs files are byte-identical, and prefs-store.test.mjs plus prefs-store.concurrent.test.mjs pass 21/21.

CI: the one red job, CLI: npx shim (ubuntu-latest), was cancelled because bun install hung for 10 minutes. Its macOS and Windows runs passed, so it only needs a rerun.

— Rames

@jrusso1020

Copy link
Copy Markdown
Collaborator

Thanks for this, @user-github-me. This PR now has merge conflicts with main. Could you rebase it onto the latest main? Once it's conflict-free we'll review it and get it into the merge queue.

@mintlify

mintlify Bot commented Oct 24, 2026

Copy link
Copy Markdown

Preview approval required

This pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit f072486.

Approve preview

This branch has not been deployed

No deployments
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.

media-use prefs.mjs: parallel record calls fail with ENOENT on a shared preferences.json.tmp and lose keys

2 participants