Repository navigation
fix(signals): repair SQLite/JSON merge splice and close the upvote lo… - #363
Merged
Darkvader-ship-it merged 1 commit intoSep 28, 2026
Conversation
…st-update Merge 2e2f64e resolved a conflict between the signal-store concurrency work and the in-flight SQLite migration by splicing both into one file. The result did not compile: 170 tsc errors on main, 37 of them in lib/signal-store.ts. What the splice broke, and what this restores: - The `getDb` import, the `MediaAttachment` type, and the `// @ts-nocheck` pragma were all dropped while 40 references to getDb/getSignalRow/rowToSignal stayed. Restored the pre-merge SQLite base (c25f7fa) wholesale. - getScheduledSignals, cancelScheduledSignal and voteOnPoll were deleted from the module while app/api/signals/scheduled, app/api/signals/[id]/vote and signals-social.test.ts still import them. Restored. - The Signal type lost poll/status/type/scheduled_for/signature_verified/media, breaking the signals list and detail pages. Restored. - The replies route lost its imports for the contributor ledger, faucet deny list and phase-136 gateway helpers, and its ReplyBody type lost timestamp / attribution / contributors. Restored. - The avatar route lost BatchAvatarQuerySchema / getAvatarsForWallets / isNftGridVirtualizationEnabled; the follow route had a duplicated import block; soroban-rpc had two circuit breakers both declaring CIRCUIT_COOLDOWN_MS. All resolved. The genuine bug, fixed properly: upvoteSignal is the only read-modify-write over a JSON blob (upvotes_json), so two writers could both read a row and the second would erase the first's upvote. It now writes under `WHERE id = ? AND version = ?` and re-reads on a lost CAS, bounded at 8 attempts. createReply and editSignal verify the parent version inside the same BEGIN IMMEDIATE as their write, so the route's check-then-insert window is closed rather than merely narrowed. Because the guard is a column comparison under WAL rather than a per-process promise queue, this holds across processes. That was the residual limitation the previous commit documented, and it is now gone without a CRDT. Two further defects found while verifying: - The version column defaulted to 0, but ETag, If-Match and parent_version all reject 0. A signal that had never been edited or upvoted therefore could not be replied to. Now 1-based, with an idempotent migration for existing rows. - REACTION_EMOJI was double-encoded UTF-8, so every reaction was rejected with "Unsupported emoji" — the phase-83 feature was entirely dead. Repaired to 👍 ❤️ 🔥 💡 🎉 🎯. The 409 on a stale parent_version also returned `currentVersion` while the client reads `current_version`, so the client never refreshed its version and every retry re-conflicted. Corrected. Tests: the merge pointed `npm test` at vitest but left 40 of 43 suites on node:test, so only the new file ever ran. Migrated them (node:test's before / after are absent from vitest 3's runtime exports; aliased to beforeAll / afterAll), and the two suites importing @jest/globals. signal-store-concurrency.test.ts is rewritten against SQLite. The 50-writer Promise.all cases cannot demonstrate this race — node:sqlite is synchronous and upvoteSignal has no await inside its read-modify-write, so no in-process interleaving point exists; they pass against the unfixed code too. The test that does catch it drives a second DatabaseSync handle committing between one connection's read and its write, which fails against the blind write (verified by reverting the fix: 8 failures, including that one). Results: tsc 170 -> 16 errors, all 16 pre-existing at c25f7fa (which had 69) and none introduced here. Tests 273 passing, 6 failing in 3 suites, all pre-existing and unrelated: lore-versioning is missing diffLoreVersions/diffWords exports, world-roles-rbac sets PHASE_SERVER_DATA_DIR after module import, and nft-index-subscribe's validateWebhookUrl is a string blocklist whose /^\d+$/ test never matches 127.0.0.1 — a real SSRF hole whose test is correct. Left for separate branches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@ArtaXerxes-oss Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
trustlineCreation #208mark_settledBefore Horizon #209forge-agentAI Pipeline Runaway Cost: No Budget Cap, No Ledger #210follow-storeFan-Out Thundering Herd: N+1 for 10k Followers #211The genuine bug, fixed properly:
upvoteSignal is the only read-modify-write over a JSON blob (upvotes_json), so two writers could both read a row and the second would erase the first's upvote. It now writes under
WHERE id = ? AND version = ?and re-reads on a lost CAS, bounded at 8 attempts. createReply and editSignal verify the parent version inside the same BEGIN IMMEDIATE as their write, so the route's check-then-insert window is closed rather than merely narrowed.Because the guard is a column comparison under WAL rather than a per-process promise queue, this holds across processes. That was the residual limitation the previous commit documented, and it is now gone without a CRDT.
Two further defects found while verifying:
The 409 on a stale parent_version also returned
currentVersionwhile the client readscurrent_version, so the client never refreshed its version and every retry re-conflicted. Corrected.Tests: the merge pointed
npm testat vitest but left 40 of 43 suites on node:test, so only the new file ever ran. Migrated them (node:test's before / after are absent from vitest 3's runtime exports; aliased to beforeAll / afterAll), and the two suites importing @jest/globals.signal-store-concurrency.test.ts is rewritten against SQLite. The 50-writer Promise.all cases cannot demonstrate this race — node:sqlite is synchronous and upvoteSignal has no await inside its read-modify-write, so no in-process interleaving point exists; they pass against the unfixed code too. The test that does catch it drives a second DatabaseSync handle committing between one connection's read and its write, which fails against the blind write (verified by reverting the fix: 8 failures, including that one).
Results: tsc 170 -> 16 errors, all 16 pre-existing at c25f7fa (which had 69) and none introduced here. Tests 273 passing, 6 failing in 3 suites, all pre-existing and unrelated: lore-versioning is missing diffLoreVersions/diffWords exports, world-roles-rbac sets PHASE_SERVER_DATA_DIR after module import, and nft-index-subscribe's validateWebhookUrl is a string blocklist whose /^\d+$/ test never matches 127.0.0.1 — a real SSRF hole whose test is correct. Left for separate branches.