Hash SQL message deduplication keys to prevent message_id overflow#6618
Hash SQL message deduplication keys to prevent message_id overflow#6618tim-smart wants to merge 5 commits into
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: a1f96bc The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughSqlMessageStorage now hashes composed deduplication keys into SHA-256 ChangesSQL message deduplication
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Sequence Diagram(s)sequenceDiagram
participant Client
participant SqlMessageStorage
participant Crypto
participant SQLDatabase
Client->>SqlMessageStorage: Save envelope with composed primary key
SqlMessageStorage->>Crypto: Hash primary key with SHA-256
Crypto-->>SqlMessageStorage: 64-character digest
SqlMessageStorage->>SQLDatabase: Query or insert hashed message_id
SqlMessageStorage->>SQLDatabase: Fallback lookup for legacy plaintext message_id
SQLDatabase-->>SqlMessageStorage: Existing or new message row
SqlMessageStorage-->>Client: Deduplication result
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Comment |
Bundle Size Analysis
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/effect/src/unstable/cluster/SqlMessageStorage.ts (2)
278-369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated SELECT text across
selectByMessageIdandinsertEnvelope's pg/mysql/orElse branches.The new
selectByMessageIdquery is identical to the conflict-resolutionSELECTalready inlined in the pg (Lines 298-303), mysql (Lines 312-318), and orElse/sqlite (Lines 359-364) branches ofinsertEnvelope. Consider having those branches callselectByMessageIdinstead of repeating the same SQL literal, to avoid future drift between the copies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effect/src/unstable/cluster/SqlMessageStorage.ts` around lines 278 - 369, Reuse the existing selectByMessageId query in the pg, mysql, and orElse conflict-resolution paths of insertEnvelope instead of repeating the identical SELECT SQL literals. Pass message_id through the helper while preserving each branch’s current insert and result-handling behavior.
95-118: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffPermanent dual-read fallback doubles DB round trips for the common (short-key) case.
mayHaveLegacyRowis true for essentially all keys, forever — there's no way to disable the legacy plaintext fallback once historic rows have aged out. Combined with its usage insaveEnvelope(Lines 454-465) andrequestIdForPrimaryKey(Lines 548-556), every short-key save now costs aSELECTbefore theINSERT(previously a singleINSERT ... ON CONFLICT), and every lookup potentially costs twoSELECTs. This is a permanent 2x round-trip tax on the hottest path in the storage layer, not a transitional one.Worth considering a way to opt out of the legacy fallback (e.g., an option threaded through
make/layer) once an operator confirms no legacy plaintextmessage_idrows remain, since this overhead never turns itself off.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effect/src/unstable/cluster/SqlMessageStorage.ts` around lines 95 - 118, Make the legacy plaintext message_id fallback configurable rather than unconditionally enabled for short keys. Add an opt-out option to the storage construction path, including make/layer, and update saveEnvelope and requestIdForPrimaryKey to skip legacy reads when disabled while preserving the current dual-read behavior by default.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/effect/src/unstable/cluster/SqlMessageStorage.ts`:
- Around line 278-369: Reuse the existing selectByMessageId query in the pg,
mysql, and orElse conflict-resolution paths of insertEnvelope instead of
repeating the identical SELECT SQL literals. Pass message_id through the helper
while preserving each branch’s current insert and result-handling behavior.
- Around line 95-118: Make the legacy plaintext message_id fallback configurable
rather than unconditionally enabled for short keys. Add an opt-out option to the
storage construction path, including make/layer, and update saveEnvelope and
requestIdForPrimaryKey to skip legacy reads when disabled while preserving the
current dual-read behavior by default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 72861d84-d62a-4bd7-b9cf-cfc0742da241
📒 Files selected for processing (10)
.changeset/hash-sql-message-dedupe-keys.mdpackages/effect/src/unstable/cluster/Envelope.tspackages/effect/src/unstable/cluster/SingleRunner.tspackages/effect/src/unstable/cluster/SqlMessageStorage.tspackages/platform-bun/src/BunClusterHttp.tspackages/platform-bun/src/BunClusterSocket.tspackages/platform-node/src/NodeClusterHttp.tspackages/platform-node/src/NodeClusterSocket.tspackages/platform-node/test/cluster/MessageStorageTest.tspackages/platform-node/test/cluster/SqlMessageStorage.test.ts
sqlite does not enforce VARCHAR widths, so legacy plaintext rows there can hold composed keys longer than 255 characters; only skip the fallback read on the width-enforcing dialects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The composed key's components are already readable via the entity_type, entity_id, tag, and payload columns, so the PR is now a pure code change with no schema change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Keys within 255 characters stay byte-identical plaintext, keeping deduplication compatible with rows written by previous versions with no fallback reads. The plaintext fallback now covers only over-length legacy keys on sqlite, whose TEXT column stored them before digests existed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
The composed request deduplication key (
entityType/entityId/tag/primaryKey) can legally exceed the 255-charactermessage_idcolumn:entity_type VARCHAR(150)+entity_id VARCHAR(255)+tag VARCHAR(50)alone allow 458 characters of address before the RPC primary key is appended. Any sufficiently long combination overflows the column (or silently violates its purpose viaINSERT IGNORE), surfacing asPersistenceErrors from deep inside SQL storage — the failure reported in #6317 via long workflow deferred keys.SqlMessageStoragenow conditionally hashes the composed key at the SQL boundary:Encoding.encodeHex) — fixed-size, fits every dialect's indexable column width, collision probability negligible (birthday bound 2^128). Digests never contain/while composed keys always do, so the two encodings cannot collide.Envelope.primaryKeyByAddressstays synchronous and plaintext; memory storage and the no-op storage are unchanged.Legacy rows with over-length keys (SQLite only)
On the width-enforcing dialects (pg/mysql/mssql), keys longer than 255 characters could never be stored, so no legacy handling is needed. SQLite's
message_idisTEXT, so pre-upgrade SQLite deployments can hold plaintext keys longer than 255 characters; for exactly that case (long key, non-width-enforcing dialect), save-time deduplication andrequestIdForPrimaryKeyfall back to a plaintext read when the digest misses.Layer requirements
SqlMessageStorage.make/layer/layerWithnow requireCrypto.Crypto; digest failures are mapped intoPersistenceErrorlike every other storage failure.SingleRunner.layerpropagates the requirement (SqlClient | Crypto.Crypto).NodeClusterHttp/NodeClusterSocket/BunClusterHttp/BunClusterSocketprovideNodeCrypto.layer/BunCrypto.layerinternally, so their public requirements are unchanged.ClusterWorkflowEngine.layeris untouched.Tests
Added to the pg/mysql/sqlite matrix in
SqlMessageStorage.test.ts:requestIdForPrimaryKey;message_idis verified to be a 64-char hex digestTEXTcolumn) still deduplicate via the fallbackAll 36 tests pass across the three dialects; memory storage tests are unchanged and pass.
Closes #6317
Closes EFF-48
🤖 Generated with Claude Code