fix(storage): make empty-table shard keys dialect-consistent

[Kiran01bm]

Make the shard-scoped task loaders treat an empty table name identically on MySQL and PostgreSQL by COALESCE-ing the NULL table segment in the shard-key match.

Why

insertTask stores an empty table name as NULL, and the shard-scoped loaders (GetByApplyID, GetByApplyOperationID) rebuild storage.ShardOperationKey's namespace/shard/table key in SQL with CONCAT. MySQL's CONCAT returns NULL when any operand is NULL, so an empty-table shard row could never match its work operation there — while PostgreSQL's concat() skips NULLs and produces ns/shard/, which matches the Go key construction. The two dialects silently disagreed on which rows are drive work.

This is latent hardening, not a repair of live data: every task writer has guarded against empty table names since it shipped, so no deployed database can hold a row this fix newly recovers. The change closes the dialect gap ahead of any future writer that emits an empty-table shard row.

What

• CONCAT(t.namespace, '/', t.shard, '/', COALESCE(t.table_name, '')) at both loader sites, aligning both dialects with storage.ShardOperationKey's Go semantics.
• A new Tasks parity subtest (ShardScopedLoaders_EmptyTableNameKey) pinning that an empty-table shard row matches a work operation keyed ns/shard/ through both loaders, on both dialects, and that a same-operation bystander row for another table stays excluded — so each loader's operation_key limb is pinned for both under- and over-matching. The subtest fails on MySQL without the COALESCE.

Before / after

Before (task row: namespace=ns, shard=s, table_name=NULL; op key "ns/s/"):

  MySQL:     CONCAT(ns,'/',s,'/',NULL)  -> NULL      -> row excluded
  PostgreSQL: concat(ns,'/',s,'/',NULL) -> "ns/s/"   -> row included
                                     ^ dialects diverge

After:

  MySQL:     CONCAT(ns,'/',s,'/',COALESCE(NULL,'')) -> "ns/s/" -> row included
  PostgreSQL: concat(ns,'/',s,'/',COALESCE(NULL,'')) -> "ns/s/" -> row included
                                     ^ both match storage.ShardOperationKey(ns, s, "")

[Copilot]

Pull request overview

This pull request fixes a cross-dialect storage mismatch in how shard-scoped task loaders match apply_operations.operation_key against a shard task’s (namespace, shard, table_name) key when table_name is empty (stored as NULL). By COALESCE-ing t.table_name to '' inside the SQL CONCAT, MySQL and PostgreSQL now both produce the same namespace/shard/ key string and include the intended drive task rows consistently.

Changes:

• Updated shard-scoped task loader queries to use COALESCE(t.table_name, '') when rebuilding namespace/shard/table keys.
• Added a parity subtest ensuring GetByApplyID and GetByApplyOperationID both match shard tasks whose table_name is stored as NULL (empty table segment) across dialects.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
pkg/storage/internal/sqlstore/tasks.go	Fixes shard key reconstruction by coalescing table_name so MySQL and PostgreSQL key matching is consistent when table_name is NULL.
pkg/storage/storagetest/tasks.go	Adds a parity test that pins the intended behavior for shard-scoped loaders when the table segment is empty (stored as NULL).

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by their agent. Reviewed at head f085ad39, in a worktree, with the new parity subtest run on both dialects against real MySQL 8.0 and PostgreSQL 16 containers.

Verdict: clean — nothing blocks. The divergence is real and the proof is clean: reverting the COALESCE fails the MySQL lane and leaves PostgreSQL passing, which is exactly the asymmetry the body describes, demonstrated rather than argued. Two loader sites is the complete set, and table_name is the only nullable operand in the key, so the fix is complete. One thing worth adjusting, and it's about how the change reads rather than what it does.

#	Finding	Severity
1	The row shape is unreachable through the production write path, but the body reads as a live divergence	doc / framing

---

1. The write path already refuses the row this fixes the read path for

UpsertShardProgress is the only writer of per-shard rows — two call sites, both in pkg/tern — and it refuses an empty table name before writing anything, with a comment that names this exact NULL behavior:

// A per-shard row must identify its table and shard. An empty table_name
// would store NULL and never match the lookup (re-inserting every pass), and
// an empty shard would collide with the unsharded single-shard sentinel.
if task.TableName == "" {
    return fmt.Errorf("upsert shard progress for operation %d shard %q requires a table name", ...)
}

So no (shard != '', table_name IS NULL) row can exist through the production path today, and the new subtest reaches the state by going around that guard via Tasks().Create. That's a fine way to pin SQL semantics — but it means the state under test is one the writer forbids.

The body currently reads as a live bug: "The two dialects silently disagreed on which rows are drive work." In the SQL they did; in the data they couldn't, because the guard upstream prevents the row. A reviewer sizing the urgency of this — or someone triaging a shard row that went missing — will go looking for the live path and not find one.

I'd still land it: keeping the SQL equal to ShardOperationKey's Go semantics is worth having on its own, and it's the kind of latent divergence that only ever surfaces at the worst moment. It just deserves an honest label. The SQL comment is the better home for it than the PR body — one clause saying the shape is currently prevented by UpsertShardProgress's guard, and that the COALESCE keeps the read path correct if that ever changes, tells the next reader everything the current comment leaves them to reconstruct.

---

Action items

1. (Finding 1) Say in the SQL comment (and soften in the body) that UpsertShardProgress refuses this row shape today, so the COALESCE is the read path staying correct rather than a live divergence being repaired.

---

Verified — tried to break, couldn't

The asymmetric proof. Reverting both COALESCEs to bare t.table_name and re-running the new subtest on both lanes:

Lane	Result
TestStorageParity/Tasks/ShardScopedLoaders_EmptyTableNameKey (MySQL 8.0)	🔴 FAIL
TestPostgresStorageParity/Tasks/ShardScopedLoaders_EmptyTableNameKey (PostgreSQL 16)	🟢 PASS

One dialect, not both — so the divergence was genuinely in MySQL's CONCAT, PostgreSQL was already correct, and the family now holds MySQL to the Go construction. Both containers really started; this isn't a skipped lane.

The bystander row earns its place. Dropping the table segment from the key entirely (CONCAT(ns, '/', shard, '/')) makes the empty-table row match and pulls in the orders row, and that fails on both dialects. Without the bystander, that mutation would have survived — the test would have proved "the row is found" but not "only the right row is found". Nice addition.

The fix covers the complete set. table_name is the only nullable column of the three: the schema declares namespace and shard as NOT NULL DEFAULT '' on both dialects, and insertTask passes them raw while only table_name goes through nullString. So no other operand can turn the CONCAT into NULL. Both key-matching sites are patched — GetByApplyID and GetByApplyOperationID — and the third shard-scoped loader, GetShardProgressByApplyOperationID, filters on shard != '' with no key match at all, so it needs nothing.

The write path stays consistent with the read path. UpsertShardProgress's own lookup uses table_name = ? through nullString, which would miss a NULL row on either dialect and re-insert every pass — exactly what its comment predicts. The guard above it is what keeps that unreachable, so patching only the read path leaves no new asymmetry between the two.

Ran locally at head: go build ./..., and the new subtest green on both dialects. CI 34/34. No test deletions or weakened assertions. Leak check on the body and diff clean, terminology clean.

This review was generated by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/1156, follow-up commit

#	Concern	Status
1	GetByApplyOperationID's operation_key limb has no cross-dialect over-match negative — a single-row fixture can't detect a predicate that matches too much (mutation M5c survives the parity suite)	fixed — f085ad3 adds a same-operation bystander row (task_shard_notable_bystander, table orders) to ShardScopedLoaders_EmptyTableNameKey; empirically re-verified: re-applying M5c (deleting the operation_key limb from GetByApplyOperationID) now fails the subtest on both MySQL and Postgres, and the restored code passes both
2	The COALESCE comment says "on every dialect" without naming MySQL as the dialect that requires it — a Postgres-side reader could "simplify" the guard away	fixed — f085ad3 rewrites the comment at both loader sites: "MySQL's CONCAT returns NULL when any operand is NULL (Postgres's concat() ignores NULLs)"
3	PR reads as a live-defect repair; it is latent hardening (no deployed row can reach this shape), which changes how reviewers/on-call should size it	fixed — PR body updated to state explicitly that every writer has guarded empty table names since it shipped, no deployed database holds a recoverable row, and the change closes the gap ahead of a future writer
4	Collation asymmetry under the same predicate: MySQL compares operation_key under utf8mb4_0900_ai_ci, Postgres exactly — case-variant keys would diverge	reply — acknowledged, no action: per the review's own analysis this is genuinely unreachable (no writer can emit case-variant shard keys, unlike the latent-and-waiting NULL case), and a collation migration on operation_key is out of scope for this PR; if a future writer introduces case-variant keys, that feature owns the parity coverage