| author |  Julius Marminge <julius0216@outlook.com> 2026-02-26 00:34:47-08:00 |
| committer |  Julius Marminge <jmarminge@gmail.com> 2026-02-26 13:20:36-08:00 |
| log | c6d1f92ba9f2a820cdbee71ff6b9499d1886bba6 |
| tree | 468b12b9362d4b8fe70f51dd3382e8f7377d7664 |
| parent | 07df7fb73bf98367f9eedc3796c0b4a0afb3c919 |
fix concurrent reads to keybindingings config
3 files changed, 85 insertions(+), 32 deletions(-)
apps
server
.plans/16c-pr89-remediation-checklist.md+4-4
| ... | @@ -313,8 +313,8 @@ Counts: active `51` (`valid=33`, `partially-valid=18`), closed-invalid `6` | ... | @@ -313,8 +313,8 @@ Counts: active `51` (`valid=33`, `partially-valid=18`), closed-invalid `6` |
| 313 | - Threads: PRRT_kwDORLtfbc5w1GPo | 313 | - Threads: PRRT_kwDORLtfbc5w1GPo |
| 314 | - Audit note: Chunk-by-chunk UTF-8 decode can still corrupt split multibyte characters. | 314 | - Audit note: Chunk-by-chunk UTF-8 decode can still corrupt split multibyte characters. |
| 315 | 315 | ||
| 316 | - [ ] `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. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> | 316 | - [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. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> |
| 317 | - Status: `TODO` | 317 | - Status: `DONE` |
| 318 | - Verdict: `partially-valid` | 318 | - Verdict: `partially-valid` |
| 319 | - Severity: `Low` | 319 | - Severity: `Low` |
| 320 | - Area: `Edge-case parsing/platform behavior` | 320 | - Area: `Edge-case parsing/platform behavior` |
| ... | @@ -322,8 +322,8 @@ Counts: active `51` (`valid=33`, `partially-valid=18`), closed-invalid `6` | ... | @@ -322,8 +322,8 @@ Counts: active `51` (`valid=33`, `partially-valid=18`), closed-invalid `6` |
| 322 | - Threads: PRRT_kwDORLtfbc5wxvIB | 322 | - Threads: PRRT_kwDORLtfbc5wxvIB |
| 323 | - Audit note: Parser/encoder mismatch remains, but encoder path currently low-use. | 323 | - Audit note: Parser/encoder mismatch remains, but encoder path currently low-use. |
| 324 | 324 | ||
| 325 | - [ ] `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. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> | 325 | - [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. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> |
| 326 | - Status: `TODO` | 326 | - Status: `DONE` |
| 327 | - Verdict: `valid` | 327 | - Verdict: `valid` |
| 328 | - Severity: `Low` | 328 | - Severity: `Low` |
| 329 | - Area: `Edge-case parsing/platform behavior` | 329 | - Area: `Edge-case parsing/platform behavior` |
apps/server/src/keybindings.test.ts+52-4
| ... | @@ -1,4 +1,4 @@ | ... | @@ -1,4 +1,4 @@ |
| 1 | import { KeybindingRule, KeybindingsConfig } from "@t3tools/contracts"; | 1 | import { KeybindingCommand, KeybindingRule, KeybindingsConfig } from "@t3tools/contracts"; |
| 2 | import { NodeServices } from "@effect/platform-node"; | 2 | import { NodeServices } from "@effect/platform-node"; |
| 3 | import { assert, it } from "@effect/vitest"; | 3 | import { assert, it } from "@effect/vitest"; |
| 4 | import { assertFailure } from "@effect/vitest/utils"; | 4 | import { assertFailure } from "@effect/vitest/utils"; |
| ... | @@ -9,6 +9,7 @@ import { | ... | @@ -9,6 +9,7 @@ import { |
| 9 | Keybindings, | 9 | Keybindings, |
| 10 | KeybindingsConfigError, | 10 | KeybindingsConfigError, |
| 11 | KeybindingsLive, | 11 | KeybindingsLive, |
| 12 | ResolvedKeybindingFromConfig, | ||
| 12 | compileResolvedKeybindingRule, | 13 | compileResolvedKeybindingRule, |
| 13 | parseKeybindingShortcut, | 14 | parseKeybindingShortcut, |
| 14 | } from "./keybindings"; | 15 | } from "./keybindings"; |
| ... | @@ -39,9 +40,7 @@ const toDetailResult = <A, R>(effect: Effect.Effect<A, KeybindingsConfigError, R | ... | @@ -39,9 +40,7 @@ const toDetailResult = <A, R>(effect: Effect.Effect<A, KeybindingsConfigError, R |
| 39 | const writeKeybindingsConfig = (configPath: string, rules: readonly KeybindingRule[]) => | 40 | const writeKeybindingsConfig = (configPath: string, rules: readonly KeybindingRule[]) => |
| 40 | Effect.gen(function* () { | 41 | Effect.gen(function* () { |
| 41 | const fileSystem = yield* FileSystem.FileSystem; | 42 | const fileSystem = yield* FileSystem.FileSystem; |
| 42 | const encoded = yield* Schema.encodeEffect(KeybindingsConfigJson)( | 43 | const encoded = yield* Schema.encodeEffect(KeybindingsConfigJson)(rules); |
| 43 | rules, | ||
| 44 | ); | ||
| 45 | yield* fileSystem.writeFileString(configPath, encoded); | 44 | yield* fileSystem.writeFileString(configPath, encoded); |
| 46 | }); | 45 | }); |
| 47 | 46 | ||
| ... | @@ -104,6 +103,25 @@ it.layer(NodeServices.layer)("keybindings", (it) => { | ... | @@ -104,6 +103,25 @@ it.layer(NodeServices.layer)("keybindings", (it) => { |
| 104 | }), | 103 | }), |
| 105 | ); | 104 | ); |
| 106 | 105 | ||
| 106 | it.effect("encodes resolved plus-key shortcuts", () => | ||
| 107 | Effect.gen(function* () { | ||
| 108 | const encoded = yield* Schema.encodeEffect(ResolvedKeybindingFromConfig)({ | ||
| 109 | command: "terminal.toggle", | ||
| 110 | shortcut: { | ||
| 111 | key: "+", | ||
| 112 | metaKey: false, | ||
| 113 | ctrlKey: false, | ||
| 114 | shiftKey: false, | ||
| 115 | altKey: false, | ||
| 116 | modKey: true, | ||
| 117 | }, | ||
| 118 | }); | ||
| 119 | |||
| 120 | assert.equal(encoded.key, "mod++"); | ||
| 121 | assert.equal(encoded.command, "terminal.toggle"); | ||
| 122 | }), | ||
| 123 | ); | ||
| 124 | |||
| 107 | it.effect("rejects invalid rules", () => | 125 | it.effect("rejects invalid rules", () => |
| 108 | Effect.sync(() => { | 126 | Effect.sync(() => { |
| 109 | assert.isNull( | 127 | assert.isNull( |
| ... | @@ -299,4 +317,34 @@ it.layer(NodeServices.layer)("keybindings", (it) => { | ... | @@ -299,4 +317,34 @@ it.layer(NodeServices.layer)("keybindings", (it) => { |
| 299 | assert.isTrue(loadedAfterUpsert.some((entry) => entry.command === "terminal.toggle")); | 317 | assert.isTrue(loadedAfterUpsert.some((entry) => entry.command === "terminal.toggle")); |
| 300 | }).pipe(Effect.provide(makeKeybindingsLayer())), | 318 | }).pipe(Effect.provide(makeKeybindingsLayer())), |
| 301 | ); | 319 | ); |
| 320 | |||
| 321 | it.effect("serializes concurrent upserts to avoid lost updates", () => | ||
| 322 | Effect.gen(function* () { | ||
| 323 | const { keybindingsConfigPath } = yield* ServerConfig; | ||
| 324 | yield* writeKeybindingsConfig(keybindingsConfigPath, []); | ||
| 325 | |||
| 326 | const commands = Array.from( | ||
| 327 | { length: 20 }, | ||
| 328 | (_, index): KeybindingCommand => `script.concurrent-${index}.run`, | ||
| 329 | ); | ||
| 330 | yield* Effect.gen(function* () { | ||
| 331 | const keybindings = yield* Keybindings; | ||
| 332 | yield* Effect.all( | ||
| 333 | commands.map((command, index) => | ||
| 334 | keybindings.upsertKeybindingRule({ | ||
| 335 | key: `mod+${String.fromCharCode(97 + index)}`, | ||
| 336 | command, | ||
| 337 | }), | ||
| 338 | ), | ||
| 339 | { concurrency: "unbounded", discard: true }, | ||
| 340 | ); | ||
| 341 | }); | ||
| 342 | |||
| 343 | const persisted = yield* readKeybindingsConfig(keybindingsConfigPath); | ||
| 344 | const persistedCommands = new Set(persisted.map((entry) => entry.command)); | ||
| 345 | for (const command of commands) { | ||
| 346 | assert.isTrue(persistedCommands.has(command), `expected persisted command ${command}`); | ||
| 347 | } | ||
| 348 | }).pipe(Effect.provide(makeKeybindingsLayer())), | ||
| 349 | ); | ||
| 302 | }); | 350 | }); |
apps/server/src/keybindings.ts+29-24
| ... | @@ -32,6 +32,7 @@ import { | ... | @@ -32,6 +32,7 @@ import { |
| 32 | SchemaTransformation, | 32 | SchemaTransformation, |
| 33 | ServiceMap, | 33 | ServiceMap, |
| 34 | } from "effect"; | 34 | } from "effect"; |
| 35 | import * as Semaphore from "effect/Semaphore"; | ||
| 35 | import { ServerConfig } from "./config"; | 36 | import { ServerConfig } from "./config"; |
| 36 | 37 | ||
| 37 | export class KeybindingsConfigError extends Schema.TaggedErrorClass<KeybindingsConfigError>()( | 38 | export class KeybindingsConfigError extends Schema.TaggedErrorClass<KeybindingsConfigError>()( |
| ... | @@ -354,7 +355,8 @@ function encodeShortcut(shortcut: KeybindingShortcut): string | null { | ... | @@ -354,7 +355,8 @@ function encodeShortcut(shortcut: KeybindingShortcut): string | null { |
| 354 | if (shortcut.ctrlKey) modifiers.push("ctrl"); | 355 | if (shortcut.ctrlKey) modifiers.push("ctrl"); |
| 355 | if (shortcut.altKey) modifiers.push("alt"); | 356 | if (shortcut.altKey) modifiers.push("alt"); |
| 356 | if (shortcut.shiftKey) modifiers.push("shift"); | 357 | if (shortcut.shiftKey) modifiers.push("shift"); |
| 357 | if (!shortcut.key || shortcut.key.includes("+")) return null; | 358 | if (!shortcut.key) return null; |
| 359 | if (shortcut.key !== "+" && shortcut.key.includes("+")) return null; | ||
| 358 | const key = shortcut.key === " " ? "space" : shortcut.key; | 360 | const key = shortcut.key === " " ? "space" : shortcut.key; |
| 359 | return [...modifiers, key].join("+"); | 361 | return [...modifiers, key].join("+"); |
| 360 | } | 362 | } |
| ... | @@ -433,6 +435,7 @@ const makeKeybindings = Effect.gen(function* () { | ... | @@ -433,6 +435,7 @@ const makeKeybindings = Effect.gen(function* () { |
| 433 | const fs = yield* FileSystem.FileSystem; | 435 | const fs = yield* FileSystem.FileSystem; |
| 434 | const path = yield* Path.Path; | 436 | const path = yield* Path.Path; |
| 435 | const cachedResolvedConfig = yield* Ref.make<ResolvedKeybindingsConfig | null>(null); | 437 | const cachedResolvedConfig = yield* Ref.make<ResolvedKeybindingsConfig | null>(null); |
| 438 | const upsertSemaphore = yield* Semaphore.make(1); | ||
| 436 | 439 | ||
| 437 | const loadCustomKeybindingsConfig = Effect.fn(function* (): Effect.fn.Return< | 440 | const loadCustomKeybindingsConfig = Effect.fn(function* (): Effect.fn.Return< |
| 438 | readonly KeybindingRule[], | 441 | readonly KeybindingRule[], |
| ... | @@ -509,29 +512,31 @@ const makeKeybindings = Effect.gen(function* () { | ... | @@ -509,29 +512,31 @@ const makeKeybindings = Effect.gen(function* () { |
| 509 | return { | 512 | return { |
| 510 | loadResolvedKeybindingsConfig: loadResolvedFromCacheOrDisk, | 513 | loadResolvedKeybindingsConfig: loadResolvedFromCacheOrDisk, |
| 511 | upsertKeybindingRule: (rule) => | 514 | upsertKeybindingRule: (rule) => |
| 512 | Effect.gen(function* () { | 515 | upsertSemaphore.withPermits(1)( |
| 513 | const customConfig = yield* loadCustomKeybindingsConfig(); | 516 | Effect.gen(function* () { |
| 514 | const nextConfig = [ | 517 | const customConfig = yield* loadCustomKeybindingsConfig(); |
| 515 | ...customConfig.filter((entry) => entry.command !== rule.command), | 518 | const nextConfig = [ |
| 516 | rule, | 519 | ...customConfig.filter((entry) => entry.command !== rule.command), |
| 517 | ]; | 520 | rule, |
| 518 | const cappedConfig = | 521 | ]; |
| 519 | nextConfig.length > MAX_KEYBINDINGS_COUNT | 522 | const cappedConfig = |
| 520 | ? nextConfig.slice(-MAX_KEYBINDINGS_COUNT) | 523 | nextConfig.length > MAX_KEYBINDINGS_COUNT |
| 521 | : nextConfig; | 524 | ? nextConfig.slice(-MAX_KEYBINDINGS_COUNT) |
| 522 | if (nextConfig.length > MAX_KEYBINDINGS_COUNT) { | 525 | : nextConfig; |
| 523 | yield* Effect.logWarning("truncating keybindings config to max entries", { | 526 | if (nextConfig.length > MAX_KEYBINDINGS_COUNT) { |
| 524 | path: keybindingsConfigPath, | 527 | yield* Effect.logWarning("truncating keybindings config to max entries", { |
| 525 | maxEntries: MAX_KEYBINDINGS_COUNT, | 528 | path: keybindingsConfigPath, |
| 526 | }); | 529 | maxEntries: MAX_KEYBINDINGS_COUNT, |
| 527 | } | 530 | }); |
| 528 | yield* writeConfigAtomically(cappedConfig); | 531 | } |
| 529 | const nextResolved = mergeWithDefaultKeybindings( | 532 | yield* writeConfigAtomically(cappedConfig); |
| 530 | compileResolvedKeybindingsConfig(cappedConfig), | 533 | const nextResolved = mergeWithDefaultKeybindings( |
| 531 | ); | 534 | compileResolvedKeybindingsConfig(cappedConfig), |
| 532 | yield* Ref.set(cachedResolvedConfig, nextResolved); | 535 | ); |
| 533 | return nextResolved; | 536 | yield* Ref.set(cachedResolvedConfig, nextResolved); |
| 534 | }), | 537 | return nextResolved; |
| 538 | }), | ||
| 539 | ), | ||
| 535 | } satisfies KeybindingsShape; | 540 | } satisfies KeybindingsShape; |
| 536 | }); | 541 | }); |
| 537 | 542 |