Skip to content

Profile records carry an id but no write provenance, so a guard cannot tell a replaced profile from a concurrent write #2355

Open

Description

@easonLiangWorldedtech

Measured while clearing a review thread on PR #1562 (head 82f0a9af7), read from the commit blobs rather than a working tree.

A profile record has an id but no provenance. saveConfig resolves the id as config.id || existingId || this.generateId() (line 468) and then replaces the whole entry under its name: providerProfiles.apiConfigs[name] = { ...filteredConfig, id } (line 480). The map is keyed by name, so a name has exactly one id at a time and a rewrite that does not carry the old id drops it out of the section. getProfile reads by name (lines 498-505); looking a profile up by id means scanning entries for a matching apiConfig.id (lines 507-513). No timestamp, revision, writer or origin field exists on the record, so nothing in the section says who wrote it or when.

Consequence for error handling: after a failed read-back, the guard in src/extension/api.ts (line 810) branches on exactly one identity signal, currentRecord.id !== id, and the reasoning is already written down at lines 811-814: identical settings under a different id is a replacement rather than this call own write, because saveConfig keeps an existing id when it overwrites, so a new id can only come from a writer that replaced the record. Skipping the restore there is the data-preserving side of the trade.

What the id signal does and does not separate, case by case:

  • A concurrent save that does not carry an id re-derives it from existingId, so the id is unchanged, the guard does not fire, and content drift is caught separately by the movedKeys branch (lines 820-824). That case is distinguishable.
  • A rename writes the record at the new name while carrying the id forward (webviewMessageHandler.ts:2603 destructures id from the previous record and passes it in), so a rename does not look like a replacement either.
  • An import replaces the whole section at once (importExport.ts:240 calls providerSettingsManager.import(providerProfiles)), so an import that carries a different id for the same name produces exactly the observation the guard is written for - it catches it, but cannot say whether the writer was an import or any other write that supplied its own id.
  • An import that carries the same id as the record it replaces produces the same observation as this call own write: identity matches and nothing else in the record says who wrote it. Only the movedKeys comparison notices, and only if the content differs. That is the real hole.

So the gap is narrower than "identity cannot help": identity already routes the replacement case, and the unresolvable residue is a same-id rewrite by another writer plus the inability to tell an import from any other id-carrying write. Both trace to the same missing fact - the record does not say which write produced it.

If the imported replacement should be protected, the change belongs in the store rather than in the guard: saveConfig needs to record which write replaced a record (provenance on the entry, or a per-name id history) so a caller can branch on a fact instead of guessing. Recorded here so the guard choice is reviewed as a design decision. Related to issue #2354 (originating view takes the sanitised payload while siblings take the persisted record); both came out of review threads on PR #1562.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions