From c6d1f92ba9f2a820cdbee71ff6b9499d1886bba6 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Thu, 26 Feb 2026 00:34:47 -0800 Subject: [PATCH] fix concurrent reads to keybindingings config --- .plans/16c-pr89-remediation-checklist.md | 8 ++-- apps/server/src/keybindings.test.ts | 56 ++++++++++++++++++++++-- apps/server/src/keybindings.ts | 53 ++++++++++++---------- 3 files changed, 85 insertions(+), 32 deletions(-) diff --git a/.plans/16c-pr89-remediation-checklist.md b/.plans/16c-pr89-remediation-checklist.md index f784dbd7fe05ddf157f94859a081a7de12b21779..74d16d5e851805da2050a3a8697cb20b588f4907 100644 --- a/.plans/16c-pr89-remediation-checklist.md +++ b/.plans/16c-pr89-remediation-checklist.md @@ -313,8 +313,8 @@ Counts: active `51` (`valid=33`, `partially-valid=18`), closed-invalid `6` - Threads: PRRT_kwDORLtfbc5w1GPo - Audit note: Chunk-by-chunk UTF-8 decode can still corrupt split multibyte characters. -- [ ] `C039` The `+` key can be parsed (via trailing `+` handling) but cannot be encoded because `shortcut.key.includes("+")` returns true for the literal `+` key. Consider checking `shortcut.key === "+"` separately and encoding it as `"space"` style (e.g., a special token), or adjusting the condition to allow the single `+` character.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent: - - Status: `TODO` +- [x] `C039` The `+` key can be parsed (via trailing `+` handling) but cannot be encoded because `shortcut.key.includes("+")` returns true for the literal `+` key. Consider checking `shortcut.key === "+"` separately and encoding it as `"space"` style (e.g., a special token), or adjusting the condition to allow the single `+` character.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent: + - Status: `DONE` - Verdict: `partially-valid` - Severity: `Low` - Area: `Edge-case parsing/platform behavior` @@ -322,8 +322,8 @@ Counts: active `51` (`valid=33`, `partially-valid=18`), closed-invalid `6` - Threads: PRRT_kwDORLtfbc5wxvIB - Audit note: Parser/encoder mismatch remains, but encoder path currently low-use. -- [ ] `C040` `upsertKeybindingRule` has a race condition: concurrent calls read the same file state, then the last write overwrites earlier changes. Consider wrapping the read-modify-write sequence with `Effect.Semaphore` to serialize access.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent: - - Status: `TODO` +- [x] `C040` `upsertKeybindingRule` has a race condition: concurrent calls read the same file state, then the last write overwrites earlier changes. Consider wrapping the read-modify-write sequence with `Effect.Semaphore` to serialize access.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent: + - Status: `DONE` - Verdict: `valid` - Severity: `Low` - Area: `Edge-case parsing/platform behavior` diff --git a/apps/server/src/keybindings.test.ts b/apps/server/src/keybindings.test.ts index 690e7d7d08e84feb48b798bdb7cabbae2f5c62fa..4dba9aa1baca1d53063ec5d78519c44002d27b41 100644 --- a/apps/server/src/keybindings.test.ts +++ b/apps/server/src/keybindings.test.ts @@ -1,4 +1,4 @@ -import { KeybindingRule, KeybindingsConfig } from "@t3tools/contracts"; +import { KeybindingCommand, KeybindingRule, KeybindingsConfig } from "@t3tools/contracts"; import { NodeServices } from "@effect/platform-node"; import { assert, it } from "@effect/vitest"; import { assertFailure } from "@effect/vitest/utils"; @@ -9,6 +9,7 @@ import { Keybindings, KeybindingsConfigError, KeybindingsLive, + ResolvedKeybindingFromConfig, compileResolvedKeybindingRule, parseKeybindingShortcut, } from "./keybindings"; @@ -39,9 +40,7 @@ const toDetailResult = (effect: Effect.Effect Effect.gen(function* () { const fileSystem = yield* FileSystem.FileSystem; - const encoded = yield* Schema.encodeEffect(KeybindingsConfigJson)( - rules, - ); + const encoded = yield* Schema.encodeEffect(KeybindingsConfigJson)(rules); yield* fileSystem.writeFileString(configPath, encoded); }); @@ -104,6 +103,25 @@ it.layer(NodeServices.layer)("keybindings", (it) => { }), ); + it.effect("encodes resolved plus-key shortcuts", () => + Effect.gen(function* () { + const encoded = yield* Schema.encodeEffect(ResolvedKeybindingFromConfig)({ + command: "terminal.toggle", + shortcut: { + key: "+", + metaKey: false, + ctrlKey: false, + shiftKey: false, + altKey: false, + modKey: true, + }, + }); + + assert.equal(encoded.key, "mod++"); + assert.equal(encoded.command, "terminal.toggle"); + }), + ); + it.effect("rejects invalid rules", () => Effect.sync(() => { assert.isNull( @@ -299,4 +317,34 @@ it.layer(NodeServices.layer)("keybindings", (it) => { assert.isTrue(loadedAfterUpsert.some((entry) => entry.command === "terminal.toggle")); }).pipe(Effect.provide(makeKeybindingsLayer())), ); + + it.effect("serializes concurrent upserts to avoid lost updates", () => + Effect.gen(function* () { + const { keybindingsConfigPath } = yield* ServerConfig; + yield* writeKeybindingsConfig(keybindingsConfigPath, []); + + const commands = Array.from( + { length: 20 }, + (_, index): KeybindingCommand => `script.concurrent-${index}.run`, + ); + yield* Effect.gen(function* () { + const keybindings = yield* Keybindings; + yield* Effect.all( + commands.map((command, index) => + keybindings.upsertKeybindingRule({ + key: `mod+${String.fromCharCode(97 + index)}`, + command, + }), + ), + { concurrency: "unbounded", discard: true }, + ); + }); + + const persisted = yield* readKeybindingsConfig(keybindingsConfigPath); + const persistedCommands = new Set(persisted.map((entry) => entry.command)); + for (const command of commands) { + assert.isTrue(persistedCommands.has(command), `expected persisted command ${command}`); + } + }).pipe(Effect.provide(makeKeybindingsLayer())), + ); }); diff --git a/apps/server/src/keybindings.ts b/apps/server/src/keybindings.ts index b433b05fd3fad19210ccdddc91832a804b94dc59..95e526319885460a4dd9104dbb2f955ece8fc72b 100644 --- a/apps/server/src/keybindings.ts +++ b/apps/server/src/keybindings.ts @@ -32,6 +32,7 @@ import { SchemaTransformation, ServiceMap, } from "effect"; +import * as Semaphore from "effect/Semaphore"; import { ServerConfig } from "./config"; export class KeybindingsConfigError extends Schema.TaggedErrorClass()( @@ -354,7 +355,8 @@ function encodeShortcut(shortcut: KeybindingShortcut): string | null { if (shortcut.ctrlKey) modifiers.push("ctrl"); if (shortcut.altKey) modifiers.push("alt"); if (shortcut.shiftKey) modifiers.push("shift"); - if (!shortcut.key || shortcut.key.includes("+")) return null; + if (!shortcut.key) return null; + if (shortcut.key !== "+" && shortcut.key.includes("+")) return null; const key = shortcut.key === " " ? "space" : shortcut.key; return [...modifiers, key].join("+"); } @@ -433,6 +435,7 @@ const makeKeybindings = Effect.gen(function* () { const fs = yield* FileSystem.FileSystem; const path = yield* Path.Path; const cachedResolvedConfig = yield* Ref.make(null); + const upsertSemaphore = yield* Semaphore.make(1); const loadCustomKeybindingsConfig = Effect.fn(function* (): Effect.fn.Return< readonly KeybindingRule[], @@ -509,29 +512,31 @@ const makeKeybindings = Effect.gen(function* () { return { loadResolvedKeybindingsConfig: loadResolvedFromCacheOrDisk, upsertKeybindingRule: (rule) => - Effect.gen(function* () { - const customConfig = yield* loadCustomKeybindingsConfig(); - const nextConfig = [ - ...customConfig.filter((entry) => entry.command !== rule.command), - rule, - ]; - const cappedConfig = - nextConfig.length > MAX_KEYBINDINGS_COUNT - ? nextConfig.slice(-MAX_KEYBINDINGS_COUNT) - : nextConfig; - if (nextConfig.length > MAX_KEYBINDINGS_COUNT) { - yield* Effect.logWarning("truncating keybindings config to max entries", { - path: keybindingsConfigPath, - maxEntries: MAX_KEYBINDINGS_COUNT, - }); - } - yield* writeConfigAtomically(cappedConfig); - const nextResolved = mergeWithDefaultKeybindings( - compileResolvedKeybindingsConfig(cappedConfig), - ); - yield* Ref.set(cachedResolvedConfig, nextResolved); - return nextResolved; - }), + upsertSemaphore.withPermits(1)( + Effect.gen(function* () { + const customConfig = yield* loadCustomKeybindingsConfig(); + const nextConfig = [ + ...customConfig.filter((entry) => entry.command !== rule.command), + rule, + ]; + const cappedConfig = + nextConfig.length > MAX_KEYBINDINGS_COUNT + ? nextConfig.slice(-MAX_KEYBINDINGS_COUNT) + : nextConfig; + if (nextConfig.length > MAX_KEYBINDINGS_COUNT) { + yield* Effect.logWarning("truncating keybindings config to max entries", { + path: keybindingsConfigPath, + maxEntries: MAX_KEYBINDINGS_COUNT, + }); + } + yield* writeConfigAtomically(cappedConfig); + const nextResolved = mergeWithDefaultKeybindings( + compileResolvedKeybindingsConfig(cappedConfig), + ); + yield* Ref.set(cachedResolvedConfig, nextResolved); + return nextResolved; + }), + ), } satisfies KeybindingsShape; }); -- 2.54.0