fix(db): PK lookups inside patch refresh past the identity map; corrected identity-map notes

server/src/db/repositories/Categories.repository.ts

@@ -30,14 +30,16 @@ export class CategoriesRepository extends EntityRepository<Categories> {
    * global") and was deleted rather than kept as dead code (Task 0
    * re-review, R6; see `_shared/owned-lookup.ts`'s docstring).
    *
-   * `refresh: true` (Task 0 review, I1 — the ruling every Plan 3 repository
-   * follows for a PK read). For this unrestricted (no `fields` option)
-   * findOne, MikroORM already re-queries and merges fresh data on every call
-   * regardless of `refresh` — its identity-map-skip optimisation only
-   * engages together with a `fields` restriction (the case
-   * `AppSettingsRepository.getValue` actually needed it for). `refresh`
-   * stays on this read to match the blanket ruling and to stay correct if a
-   * future caller adds `fields` here.
+   * `refresh: true` (Task 0 review, I1 — every PK-only `findOne` in a
+   * repository takes it) and it is load-bearing here, not decorative: `{ id
+   * }` is exactly the primary key, so MikroORM answers this call from the
+   * identity map with zero queries whenever the row is already loaded
+   * (Task 3 review, Important 2's correction — the short-circuit is driven
+   * by the filter being PK-only, not by a `fields` restriction, which was
+   * the wrong mechanism this docstring stated before the fix). Without
+   * `refresh`, a raw write to this row earlier in the same request would
+   * make this method return the stale snapshot instead of the current DB
+   * row.
    */
   async findById(id: number): Promise<CategoryRow | null> {
     const category = await this.findOne({ id }, { refresh: true });
@@ -84,13 +86,26 @@ export class CategoriesRepository extends EntityRepository<Categories> {
    * coalesce-away-empty-string behaviour. Returns `null` when no row matches
    * `id`, mirroring the legacy re-select's `undefined`.
    *
+   * The lookup `findOne({ id }, { refresh: true })` is PK-only, so without
+   * `refresh: true` MikroORM would answer it from the identity map instead
+   * of the DB (Task 3 review, Important 1): a category loaded earlier in
+   * this request (list/findById/an earlier patch) would stay pinned to that
+   * stale snapshot, so (a) an untouched column could read stale even though
+   * the DB has since changed underneath it, (b) `assign` could diff a patch
+   * against a stale value that happens to match the new one and emit no
+   * UPDATE at all — reporting success while silently dropping the write —
+   * and (c) a `remove()` earlier in the same request (a `nativeDelete`,
+   * which does not clear the identity map) would let `patch` fabricate a
+   * row for an id that no longer exists instead of returning `null`.
+   * `refresh: true` forces a real re-query every time, closing all three.
+   *
    * No re-`findOne`/`refresh` after `flush`: `assign` already mutated the
-   * managed entity in place, and no column here is DB-computed on UPDATE
-   * (unlike `created_at` on insert), so the in-memory entity already holds
-   * exactly what was written.
+   * managed (and now guaranteed-fresh) entity in place, and no column here
+   * is DB-computed on UPDATE (unlike `created_at` on insert), so the
+   * in-memory entity already holds exactly what was written.
    */
   async patch(id: number, changes: { name?: string; color?: string; icon?: string }): Promise<CategoryRow | null> {
-    const category = await this.findOne({ id });
+    const category = await this.findOne({ id }, { refresh: true });
     if (!category) return null;
     this.assign(category, changes);
     await this.getEntityManager().flush();

server/src/db/repositories/Tags.repository.ts

@@ -33,16 +33,17 @@ export class TagsRepository extends EntityRepository<Tags> {
    * directly).
    *
    * `refresh: true` (Task 0 review, I1 — the ruling every Plan 3 repository
-   * follows for a PK-scoped read). For this unrestricted (no `fields`
-   * option) findOne, MikroORM already re-queries and merges fresh data on
-   * every call regardless of `refresh` — its identity-map-skip optimisation
-   * only engages together with a `fields` restriction (the case
-   * `AppSettingsRepository.getValue` actually needed it for). `refresh`
-   * stays on this read to match the blanket ruling and to stay correct if a
-   * future caller adds `fields` here. `findOwnedByUser`'s optional `options`
-   * parameter (added here, not duplicated — Task 0's helper had no reads
-   * that needed it yet) passes straight through to the underlying
-   * `findOne`.
+   * follows for a PK-scoped read). This call's filter is `{ id, user }`,
+   * not PK-only, so MikroORM's identity-map short-circuit — which fires
+   * only when the filter is exactly the primary key, not (as an earlier
+   * version of this docstring said) when a `fields` restriction is present
+   * (Task 3 review, Important 2's correction) — never engages here anyway:
+   * this call already re-queries and merges fresh data on every call
+   * regardless of `refresh`. `refresh` stays threaded through to match the
+   * blanket ruling and to stay correct if a future caller narrows the
+   * filter to PK-only. `findOwnedByUser`'s optional `options` parameter
+   * (added here, not duplicated — Task 0's helper had no reads that needed
+   * it yet) passes straight through to the underlying `findOne`.
    */
   async findByIdAndUser(id: number, userId: number): Promise<TagRow | null> {
     const tag = await findOwnedByUser<Tags, 'user'>(this, id, 'user', userId, { refresh: true });
@@ -84,12 +85,22 @@ export class TagsRepository extends EntityRepository<Tags> {
    * coalesce-away-empty-string behaviour. Returns `null` when no row matches
    * `id`, mirroring the legacy re-select's `undefined`.
    *
+   * The lookup `findOne({ id }, { refresh: true })` is PK-only, so without
+   * `refresh: true` MikroORM would answer it from the identity map instead
+   * of the DB (Task 3 review, Important 1) — same three consequences as
+   * `CategoriesRepository.patch`: a stale untouched column, a patch that
+   * diffs against a stale value and silently drops the UPDATE, or (since
+   * `remove()` is a `nativeDelete` that does not clear the identity map) a
+   * fabricated row for an id already deleted earlier in the request instead
+   * of `null`. `refresh: true` forces a real re-query every time.
+   *
    * No re-`findOne`/`refresh` after `flush`, same reasoning as
-   * `CategoriesRepository.patch`: `assign` already mutated the managed
-   * entity in place, and no column here is DB-computed on UPDATE.
+   * `CategoriesRepository.patch`: `assign` already mutated the managed (and
+   * now guaranteed-fresh) entity in place, and no column here is
+   * DB-computed on UPDATE.
    */
   async patch(id: number, changes: { name?: string; color?: string }): Promise<TagRow | null> {
-    const tag = await this.findOne({ id });
+    const tag = await this.findOne({ id }, { refresh: true });
     if (!tag) return null;
     this.assign(tag, changes);
     await this.getEntityManager().flush();

server/src/db/repositories/_shared/owned-lookup.ts

@@ -73,14 +73,15 @@ export async function listForOwner<T extends { id: unknown }, K extends keyof T,
  *
  * `options.refresh` passes straight through to the underlying `findOne`
  * (Task 0 review, I1's PK-read ruling: a caller whose read can follow a
- * raw/native write on the same row within the same request sets it). For an
- * unrestricted call like this one (no `fields` option), MikroORM already
- * re-queries and merges fresh data on every call regardless of `refresh` —
- * its identity-map-skip optimisation, and the staleness it can cause without
- * `refresh`, only engages together with a `fields` restriction (the case
- * `AppSettingsRepository.getValue` actually hit). `refresh` is still
- * threaded through here to match the blanket ruling and to stay correct if a
- * caller ever adds `fields` on top of it.
+ * raw/native write on the same row within the same request sets it). This
+ * filter is `{ id, [ownerField]: ownerId }` — not PK-only — so MikroORM's
+ * identity-map short-circuit never engages here: that short-circuit fires
+ * only when the filter is exactly the primary key (Task 3 review, Important
+ * 2's correction — `fields` has nothing to do with it, unlike what an
+ * earlier version of this docstring claimed). `refresh` is a harmless
+ * no-op on every call this helper serves today; it stays threaded through
+ * to match the blanket ruling and to stay correct if a future caller
+ * narrows the filter to PK-only.
  */
 export async function findOwnedByUser<T extends { id: unknown }, K extends keyof T>(
   repo: EntityRepository<T>,

server/src/nest/categories/categories.service.ts

@@ -51,7 +51,11 @@ export class CategoriesService {
     if (name) changes.name = name;
     if (color) changes.color = color;
     if (icon) changes.icon = icon;
-    return (await this.categories.patch(Number(id), changes)) as unknown as Category;
+    // `patch` returns `null` for a non-existent id (Task 3 review, Minor 2);
+    // the legacy re-select returned `undefined` for the same case, and every
+    // caller pre-checks with a 404 first, so `?? undefined` restores that
+    // exact parity for the race-condition path the pre-check doesn't cover.
+    return ((await this.categories.patch(Number(id), changes)) ?? undefined) as unknown as Category;
   }
 
   async remove(id: string | number): Promise<void> {

server/src/nest/tags/tags.service.ts

@@ -49,7 +49,11 @@ export class TagsService {
     const changes: { name?: string; color?: string } = {};
     if (name) changes.name = name;
     if (color) changes.color = color;
-    return (await this.tags.patch(Number(id), changes)) as unknown as Tag;
+    // `patch` returns `null` for a non-existent id (Task 3 review, Minor 2);
+    // the legacy re-select returned `undefined` for the same case, and every
+    // caller pre-checks with a 404 first, so `?? undefined` restores that
+    // exact parity for the race-condition path the pre-check doesn't cover.
+    return ((await this.tags.patch(Number(id), changes)) ?? undefined) as unknown as Tag;
   }
 
   async remove(id: string | number): Promise<void> {

server/tests/unit/db/repositories/Categories.repository.test.ts

@@ -89,10 +89,11 @@ describe('CategoriesRepository', () => {
     });
 
     // I1 (Task 0 review): findById carries `refresh: true`, the ruling every
-    // Plan 3 PK read follows. This unrestricted (no `fields` option) findOne
-    // always re-queries and merges fresh data regardless of `refresh` — see
-    // CategoriesRepository's docstring — so this proves the (still correct)
-    // outcome in one query rather than a refresh-vs-no-refresh contrast.
+    // Plan 3 PK read follows — and it is load-bearing here (Task 3 review,
+    // Important 2's correction): `{ id }` is exactly the primary key, so
+    // without `refresh` this would be served from the identity map with
+    // zero queries and the stale pre-UPDATE color. `queries === 1` proves
+    // the real re-query happens.
     it('CATREPO-008: a raw UPDATE on the same row then findById reads the new value, in one query', async () => {
       const created = createCategory(testDb, { name: 'RepoStale', color: '#111111' });
       expect((await categories.findById(created.id))?.color).toBe('#111111'); // populate the identity map
@@ -151,6 +152,46 @@ describe('CategoriesRepository', () => {
     it('CATREPO-014: returns null for a non-existent id', async () => {
       expect(await categories.patch(99999999, { name: 'Nope' })).toBeNull();
     });
+
+    // Task 3 review, Important 1, consequence (a) — and Minor 1: the
+    // identity map is populated through `list()` here, not `findById`, so
+    // this doubles as the "list → raw UPDATE → patch" case the review
+    // asked for. Without `refresh: true` in `patch`'s lookup, `icon` would
+    // still read the pre-UPDATE 'OLD' (verified: fails without the fix).
+    it('CATREPO-018: an untouched column reflects a raw UPDATE made after the identity map was populated by list()', async () => {
+      const created = createCategory(testDb, { name: 'RepoRaceIcon', color: '#aaaaaa', icon: 'OLD' });
+      await categories.list(); // populate the identity map with icon: 'OLD'
+      testDb.prepare('UPDATE categories SET icon = ? WHERE id = ?').run('NEW', created.id);
+      const updated = await categories.patch(created.id, { color: '#bbbbbb' });
+      expect(updated?.icon).toBe('NEW');
+    });
+
+    // Task 3 review, Important 1, consequence (b): without `refresh: true`,
+    // MikroORM diffs the patch against the stale in-memory snapshot; when
+    // the concurrent raw write and the patch happen to agree on the value,
+    // no changeset is produced and the UPDATE is silently dropped even
+    // though the method reports success. The raw-row assertion is what
+    // catches it — `updated?.color` alone would still read '#111111'
+    // either way (verified: fails without the fix).
+    it('CATREPO-019: a patch matching a concurrent raw write is not silently dropped', async () => {
+      const created = createCategory(testDb, { name: 'RepoRaceColor', color: '#111111' });
+      await categories.findById(created.id); // populate the identity map with '#111111'
+      testDb.prepare('UPDATE categories SET color = ? WHERE id = ?').run('#333333', created.id);
+      const updated = await categories.patch(created.id, { color: '#111111' });
+      expect(updated?.color).toBe('#111111');
+      expect(rawCategory(created.id)).toMatchObject({ color: '#111111' });
+    });
+
+    // Task 3 review, Important 1, consequence (c): `remove` uses
+    // `nativeDelete`, which does not clear the identity map, so a stale
+    // patch afterwards could fabricate a row for an id that no longer
+    // exists instead of returning `null` (verified: fails without the fix).
+    it('CATREPO-020: patch after remove in the same request returns null, not a fabricated row', async () => {
+      const created = createCategory(testDb, { name: 'RepoRaceDelete', color: '#444444' });
+      await categories.findById(created.id); // populate the identity map
+      await categories.remove(created.id);
+      expect(await categories.patch(created.id, { color: '#555555' })).toBeNull();
+    });
   });
 
   describe('remove', () => {

server/tests/unit/db/repositories/Tags.repository.test.ts

@@ -94,10 +94,12 @@ describe('TagsRepository', () => {
 
     // I1 (Task 0 review): findByIdAndUser carries `{ refresh: true }` (via
     // the shared findOwnedByUser helper), the ruling every Plan 3 PK read
-    // follows. This unrestricted (no `fields` option) findOne always
-    // re-queries and merges fresh data regardless of `refresh` — see
-    // TagsRepository's docstring — so this proves the (still correct)
-    // outcome in one query rather than a refresh-vs-no-refresh contrast.
+    // follows. This call's filter is `{ id, user }`, not PK-only, so it
+    // always re-queries and merges fresh data regardless of `refresh` (Task
+    // 3 review, Important 2's correction: the identity-map short-circuit
+    // fires only for an exactly-PK filter, not for a `fields` restriction)
+    // — this proves the (still correct) outcome in one query rather than a
+    // refresh-vs-no-refresh contrast.
     it('TAGREPO-009: a raw UPDATE on the same row then findByIdAndUser reads the new value, in one query', async () => {
       const { user } = createUser(testDb);
       const created = createTag(testDb, user.id, { name: 'Stale', color: '#111111' });
@@ -155,6 +157,50 @@ describe('TagsRepository', () => {
     it('TAGREPO-015: returns null for a non-existent id', async () => {
       expect(await tags.patch(99999999, { name: 'Nope' })).toBeNull();
     });
+
+    // Task 3 review, Important 1, consequence (a) — and Minor 1: the
+    // identity map is populated through `listByUser()` here, not
+    // `findByIdAndUser`, so this doubles as the "list → raw UPDATE →
+    // patch" case the review asked for. Without `refresh: true` in
+    // `patch`'s lookup, `name` would still read the pre-UPDATE 'OLD'
+    // (verified: fails without the fix).
+    it('TAGREPO-019: an untouched column reflects a raw UPDATE made after the identity map was populated by listByUser()', async () => {
+      const { user } = createUser(testDb);
+      const created = createTag(testDb, user.id, { name: 'OLD', color: '#aaaaaa' });
+      await tags.listByUser(user.id); // populate the identity map with name: 'OLD'
+      testDb.prepare('UPDATE tags SET name = ? WHERE id = ?').run('NEW', created.id);
+      const updated = await tags.patch(created.id, { color: '#bbbbbb' });
+      expect(updated?.name).toBe('NEW');
+    });
+
+    // Task 3 review, Important 1, consequence (b): without `refresh: true`,
+    // MikroORM diffs the patch against the stale in-memory snapshot; when
+    // the concurrent raw write and the patch happen to agree on the value,
+    // no changeset is produced and the UPDATE is silently dropped even
+    // though the method reports success. The raw-row assertion is what
+    // catches it — `updated?.color` alone would still read '#111111'
+    // either way (verified: fails without the fix).
+    it('TAGREPO-020: a patch matching a concurrent raw write is not silently dropped', async () => {
+      const { user } = createUser(testDb);
+      const created = createTag(testDb, user.id, { name: 'RaceColor', color: '#111111' });
+      await tags.findByIdAndUser(created.id, user.id); // populate the identity map with '#111111'
+      testDb.prepare('UPDATE tags SET color = ? WHERE id = ?').run('#333333', created.id);
+      const updated = await tags.patch(created.id, { color: '#111111' });
+      expect(updated?.color).toBe('#111111');
+      expect(rawTag(created.id)).toMatchObject({ color: '#111111' });
+    });
+
+    // Task 3 review, Important 1, consequence (c): `remove` uses
+    // `nativeDelete`, which does not clear the identity map, so a stale
+    // patch afterwards could fabricate a row for an id that no longer
+    // exists instead of returning `null` (verified: fails without the fix).
+    it('TAGREPO-021: patch after remove in the same request returns null, not a fabricated row', async () => {
+      const { user } = createUser(testDb);
+      const created = createTag(testDb, user.id, { name: 'RaceDelete', color: '#444444' });
+      await tags.findByIdAndUser(created.id, user.id); // populate the identity map
+      await tags.remove(created.id);
+      expect(await tags.patch(created.id, { color: '#555555' })).toBeNull();
+    });
   });
 
   describe('remove', () => {

server/tests/unit/db/repositories/_shared/owned-lookup.test.ts

@@ -61,16 +61,18 @@ describe('findOwnedByUser', () => {
   });
 
   // Task 0 review I1's PK-read ruling, extended to this helper (Task 3):
-  // `options.refresh` passes through to `findOne`. For an unrestricted
-  // (full-row) findOne like this one, MikroORM already re-queries and merges
-  // fresh data on every call regardless of `refresh` (proven below: a raw
-  // write on the same row is visible either way) — `refresh`'s
-  // identity-map-skip optimisation (and the staleness it can cause without
-  // it) only engages when `options.fields` is also set, which is why
-  // AppSettingsRepository.getValue (a `fields: ['value']`-scoped findOne)
-  // needed it to fix a real bug and this one does not. It stays on this
-  // helper regardless, matching the Task 0 review's blanket PK-read ruling
-  // and staying correct if a future caller adds a `fields` restriction here.
+  // `options.refresh` passes through to `findOne`. This helper's filter is
+  // `{ id, [ownerField]: ownerId }`, not PK-only, so it always re-queries
+  // and merges fresh data regardless of `refresh` (proven below: a raw
+  // write on the same row is visible either way) — the identity-map
+  // short-circuit (and the staleness it can cause without `refresh`) fires
+  // only when the filter is exactly the primary key, which is what
+  // AppSettingsRepository.getValue's PK-only `findOne` hit and this
+  // multi-condition filter never does (Task 3 review, Important 2's
+  // correction — `fields` has nothing to do with it). `refresh` stays on
+  // this helper regardless, matching the Task 0 review's blanket PK-read
+  // ruling and staying correct if a future caller narrows the filter to
+  // PK-only.
   it('OWNEDLOOKUP-006: { refresh: true } sees a raw UPDATE on the same row in the same request, in one query', async () => {
     const { user: owner } = createUser(testDb);
     const tag = createTag(testDb, owner.id, { name: 'Old' });