SqliteOfflineQueue::setIdempotencyKey throws on a conflicting key; the class's own enqueue() dedups instead

[Yaraslaut]

The gap

SqliteOfflineQueue::setIdempotencyKey (the protected hook IOfflineQueue::enqueue(payload, key)'s default implementation calls after the one-arg enqueue()) does a raw UPDATE ... SET idempotency_key = ?. If another row already holds that non-empty key, the partial unique index (ix_queue_idem) rejects the write and the call throws SqliteOfflineQueueError:

SqliteOfflineQueue: setIdempotencyKey failed: UNIQUE constraint failed: morph_offline_queue.idempotency_key

SqliteOfflineQueue's own two-arg enqueue(payload, key) override never reaches this — it resolves a conflict with INSERT ... ON CONFLICT ... DO NOTHING plus a lookup, returning the existing row's id exactly like FileOfflineQueue and matching the dedup contract discussed in #175. The throwing path is reachable only when a caller holds an IOfflineQueue& and explicitly scope-qualifies the call, e.g. q.IOfflineQueue::enqueue(payload, key), bypassing normal virtual dispatch to reach the base class's default (payload-then-stamp) implementation instead of the derived override.

Verified

Confirmed independently (twice) against include/morph/offline/sqlite_offline_queue.hpp (current master), via a standalone compiled probe (cl.exe /std:c++latest, vcpkg sqlite3 from build/cl-debug):

SqliteOfflineQueue q{dbPath};
q.enqueue("first", "DUPKEY");
q.enqueue("third", "OTHERKEY");
q.IOfflineQueue::enqueue("fourth-via-base", "DUPKEY");  // throws

derived enqueue dedup: id1=1 id2=1 (equal=yes) size=1
  row id=1 payload="first" key="DUPKEY"
third (OTHERKEY) id3=3 size=2
base-qualified enqueue threw=yes what="SqliteOfflineQueue: setIdempotencyKey failed: UNIQUE constraint failed: morph_offline_queue.idempotency_key"
final size=3
base-qualified enqueue with fresh key: id5=5 (no throw)

This confirms:

• SqliteOfflineQueue::enqueue(payload, key) (sqlite_offline_queue.hpp:172-213) resolves a conflicting key via INSERT ... ON CONFLICT ... DO NOTHING plus a lookup — a silent no-op dedup that discards the new payload and returns the existing row's id.
• The protected hook setIdempotencyKey (sqlite_offline_queue.hpp:285-291) does a raw UPDATE, which the partial unique index ix_queue_idem rejects on the same conflict, throwing SqliteOfflineQueueError.
• This throw path is reachable only via an IOfflineQueue& with an explicit scope qualifier (q.IOfflineQueue::enqueue(...)) — a fresh (non-conflicting) key through the same base-qualified path does not throw, isolating the throw to the conflict case specifically.
• No shipped call path reaches it today. SyncWorker never calls enqueue directly (it only drains/replays), and setIdempotencyKey appears nowhere outside the three queue implementations and tests/test_file_offline_queue.cpp's own scope-qualified test for FileOfflineQueue. The scope-qualified call shape is exercised in-tree for the sibling implementation, just not for SqliteOfflineQueue's conflicting-key case.
• The asymmetry is SqliteOfflineQueue-only: FileOfflineQueue::setIdempotencyKey (file_offline_queue.hpp:265-273) unconditionally overwrites the in-memory map entry with no unique constraint to conflict against, and InMemoryOfflineQueue::setIdempotencyKey never dedups at all — neither has this asymmetry.

Spec gap

docs/spec/offline/offline.md line 180 documents setIdempotencyKey only as the hook the default two-arg enqueue uses to stamp the key onto an already-enqueued item ("Default no-op; InMemoryOfflineQueue records the key directly instead."). Nothing there, in the IOfflineQueue table, the SqliteOfflineQueue section, or the dedup-contract note (offline.md ~line 774-775) mentions that the hook can throw on a conflict. This is a genuine spec gap (no documented contract to disagree with), not a spec-vs-code disagreement.

Direction

Make setIdempotencyKey resolve a conflicting key the same way SqliteOfflineQueue::enqueue(payload, key) already does: no-op, keep the existing row, matching the class's own dedup contract instead of throwing. This is the smaller change, removes the asymmetry outright rather than just documenting it, and costs nothing to callers — the hook is protected, has no public contract to preserve, and (per the verification above) no shipped code depends on the current throwing behavior.

The alternative — make the derived enqueue(payload, key) throw instead, to match the base hook — is possible but less consistent with current usage: it would break the existing dedup contract that enqueue(payload, key) callers already rely on (see #175), just to preserve a throw path nothing currently exercises.

Once the fix lands, docs/spec/offline/offline.md line 180 (and the dedup-contract note around line 774-775) should be updated to state the conflict behavior explicitly.

Scope

This is include/morph/offline/sqlite_offline_queue.hpp only — FileOfflineQueue::setIdempotencyKey has no unique-index equivalent to conflict against, and InMemoryOfflineQueue::setIdempotencyKey never dedups at all, so neither of the other two implementations has this asymmetry.

Split out of #175's "Out of scope" section during that issue's triage, per the triage-issue skill's guidance not to fold a second distinct problem into an existing issue.

[Yaraslaut]

Triage: triage: valid

Second cycle. The body now leads with a single, buildable recommendation rather than an open menu, and re-verification against current master confirms the underlying premise still holds exactly as described.

Re-verified against current HEAD

include/morph/offline/sqlite_offline_queue.hpp:

• enqueue(payload, key) (lines 172-213): on a key conflict, uses INSERT ... ON CONFLICT(idempotency_key) WHERE idempotency_key <> '' DO NOTHING, detects the no-op via sqlite3_changes(_db) == 0, then looks up and returns the existing row's id — silent dedup, discarding the new payload.
• setIdempotencyKey (lines 285-291): a raw UPDATE morph_offline_queue SET idempotency_key = ? WHERE id = ? via stepOrThrow, with no conflict handling. The partial unique index ix_queue_idem (declared line 68, created line 128) rejects this on the same kind of conflict, and stepOrThrow turns that into a thrown SqliteOfflineQueueError.

Both match the issue's description precisely — no drift since the prior cycle. The doc comment already on setIdempotencyKey (lines 278-284) independently confirms the reachability claim: this hook is reachable only via the base IOfflineQueue::enqueue(payload, key) default through an IOfflineQueue&, not through this class's own override.

Valid-vs-rescope test

The corrected body now commits to one recommended, buildable fix: make setIdempotencyKey resolve a conflicting key the same way enqueue(payload, key) already does (no-op, keep the existing row), scoped to SqliteOfflineQueue only (the other two implementations don't have this asymmetry, for stated reasons). The alternative (make enqueue throw instead) is explicitly named and deprioritized with a concrete reason — it would break the dedup contract enqueue(payload, key) callers already rely on (#175) to preserve a throw path nothing shipped exercises. A documentation follow-up (docs/spec/offline/offline.md line 180 and ~774-775) is noted as a consequence of the fix, not as a competing resolution.

This is no longer an open three-way menu — it's a single direction with specific method, specific file, specific scope, and a stated rationale for not pursuing the alternative. That's implementable as written.

Verdict: triage: valid. Labeling triage: valid + area: offline.