Repository navigation
fix(media-use): serialize concurrent preference records - #5366
user-github-me wants to merge 8 commits into
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
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.mjs5/5 passes at head, and 17/17 together with the existing prefs-store tests. With main'sprefs-store.mjsswapped 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 recordprocesses (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/.lockfiles. - The standalone and CLI
prefs-store.mjscopies 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: thefinallyrmSync(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.withFileLocknever takes over a lock. Arecordkilled 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/*.lockin 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
left a comment
There was a problem hiding this comment.
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 wasskills-manifest.json, andgen-skills-manifest.ts --checknow reports it in sync (21 skills).mainhadn't touchedprefs-store.mjs,config-lock.mjsormemory.mdsince your base, so the merge didn't change the fix. prefs-store.mjs:79-82: thefinallycleanup is now wrapped in try/catch. I checked the four new EPERM/EBUSY tests inprefs-store.test.mjsby putting the unguardedrmSyncback. All four failed: two because a successful write turned into a failure, and two because the cleanup error hid theENOSPC. With your change they pass.skills/media-use/references/memory.md: it now documents recovery from an abandonedpreferences.json.lockand the.media/*.lockignore guidance. The claim that "a locked personal tier does not fail the project record" matchesrecordPreference, which catches the error fromrecordUserSighting(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
|
Thanks for this, @user-github-me. This PR now has merge conflicts with |
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
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
main, retaining its media-home resolution and regenerating the manifest for the seven published skills.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.