Repository navigation
Conversation
#217 stopped a v2-primary daemon from running the legacy mirror on /api/mark-read, so it wrote no v2 read cursor at all. It now writes one natively (v2wire.MarkReadV2): - The conversation id resolves the way v2 reads do: the v2 id, or a legacy-form id through remote_conversation_id. The alias rule moves into v2read.ResolveConversation, which the read path now calls too. An id that resolves to nothing writes nothing. - The cursor goes on the account's local installation device (GetLocalInstallationDevice), whatever its id. An account with none gets one under the migration's derived id, now shared as v2keys.LocalInstallationDeviceID. - LastReadMessageID stays nil, as in the mirror. read_cursors references messages with no ON DELETE action, so a cursor on the newest message pins it, and the outbox's echo-duplicate delete on reconcile then fails with FOREIGN KEY constraint failed. - UpsertReadCursor keeps it monotone in read time. The write is best effort: a failure is logged and the response stays 200. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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
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.
Stacked on #217, which is open, so the base is #217's branch. I'll retarget this to
mainand rebase once #217 merges.What was missing
#217 stopped a v2-primary daemon from running the legacy mirror on
POST /api/mark-read. The mirror can't resolve the v2 conversation ids a v2-primary UI sends. For a legacy id the v2 store lacks, it would add a legacy-keyed conversation row to the primary store. The cost was that a v2-primary daemon wrote no v2 read cursor at all. Nothing consumes v2 read cursors yet:v2readmapsUnreadCount: 0, and the web UI calls mark-read only whenUnreadCount > 0. So no visible behavior regressed, and today only API callers reach this path on v2-primary. The S5b/S8 unread derivation will need the cursors.Change
On v2-primary,
/api/mark-readnow callsv2wire.MarkReadV2, which writes the cursor natively:v2read.ResolveConversation, the alias rule v2 reads already used, now exported. The v2 id resolves to itself. Any other value is tried asremote_conversation_idacross accounts, normalized per platform (sosignal: +1555…matches), and the most recently active match wins. The read path (Source.resolveConversationID) now calls the same function, so a stored legacy id writes to the thread it reads. An id that matches nothing writes nothing and returnsErrNotFound.GetLocalInstallationDevice(account): the device ingest advances receipt cursors for, whatever its id (9bcc1343…for Google on a migrated store). An account with no local device gets one throughEnsureLocalInstallationDevice, under the migration's derived id. That id is now one helper,v2keys.LocalInstallationDeviceID, used by both the migration and this path. Nothing here assumeslocal-primary:<account>.UpsertReadCursorwithLastReadMessageID: nilandLastReadAtMS = UpdatedAtMS = now. The upsert is monotone in read time.Failed to write v2 read cursorwithconv_idat warn, and the response stays 200. No read receipt goes to the phone.Legacy-primary daemons still run the mirror, unchanged. Both writes now run under
context.WithoutCancel(r.Context()), so a client that hangs up mid-request doesn't drop the v2 write after the legacy write has landed.Why
LastReadMessageIDstays nilI considered pointing the cursor at the conversation's newest message. Every other cursor writer records a position: migration, ingest receipts, and the outbox read path. A NULL written at a later time also replaces a migrated cursor's position. But naming the newest message is not safe today:
read_cursorshasFOREIGN KEY (conversation_id, last_read_message_id) REFERENCES messages(conversation_id, message_id), with noON DELETEaction, andforeign_keys(ON)is set on every connection.OutboxRepository.repointLocalMessagedeletes a send's echo duplicate on reconcile. The echo is exactly the row most likely to be the newest message when the user opens the thread.TestOutboxReconcileConfirmMergesCollisionAndPreservesLocalAnchorscenario with a read cursor on the echo duplicate, then ranReconcileConfirm. It failed withdelete echo duplicate "message-echo-duplicate": … FOREIGN KEY constraint failed (787), so the send stays unreconciled.repair_idspace.go) already skips moving or deleting any message a cursor references (MessageHasReadCursor), so a moving pointer at the newest message would also pin rows that repair needs to fix.A NULL position dated at the request says what mark-read means ("everything up to now is read", which is the legacy
unread_count = 0). It matches the mirror and theReadCursordoc ("may be nil for … cursors that only approximate a position"). Once the FK or the deletes handle cursors, S5b/S8 can switch this to a position.Invariants (property-tested)
MarkReadV2never lowers a cursor'slast_read_at_ms. A call dated before the stored cursor leaves it byte-identical, including a positioned cursor from another writer.{LastReadMessageID: nil, LastReadAtMS: at, UpdatedAtMS: at}on (local installation device, resolved conversation).ResolveConversationagrees with a brute-force reading of the alias rule, and with the conversation the v2 read path serves for the same key (differential).LocalInstallationDeviceID(account)to an account that had no local installation device.ErrNotFound. The HTTP response stays 200 whatever happens to the v2 write.Tests
internal/web/markread_v2primary_test.go: the store is built with the realmigration.Transform, and the fixture pins derived devices and migrated cursors that name a message.TestMarkReadV2PrimaryWritesNativeCursorOnMigratedStore: Google, WhatsApp and Signal threads, each by v2 id and by legacy alias. The cursor lands on the derived device and no other row changes.TestMarkReadV2PrimaryUnknownConversationLogsAndWritesNothing: a legacy id the v2 store lacks returns 200, logs the warning, and writes nothing.TestMarkReadV2PrimaryWithoutV2StoreStays200.TestMarkReadV2PrimaryCursorMonotoneProperty(testing/quick, 15 migrated stores × up to 8 requests): random threads get positioned cursors from another writer, dated up to a day before or after now, and then random v2-id or alias requests run. Cursors dated after a request survive it, others become{nil, t, t}withtinside the request window, and nothing else changes.internal/v2wire/read_native_test.go: migrated-store cases with the v2 id, the legacy alias and a padded alias. Unknown, empty and post-cutover legacy ids write nothing. Invalid input (nil or canceled context, nil store, non-positive time) writes nothing. Device creation covers an account with no device and one whose only local device is non-current.TestMarkReadV2CursorPropertiesruns 60 random account, device and cursor shapes, each with up to 12 calls inside a 2-second window, so equal, older and newer calls are common. It checks a model after every call and replays the calls in reverse.internal/v2read/alias_resolve_test.go: example cases, plusTestResolveConversationMatchesReferenceAndReadPath(40 random stores whose remote ids collide across accounts and whose recency ties). It compares a reference implementation withSource.GetConversation.internal/v2keys:LocalInstallationDeviceIDagainst the migration's recorded device goldens.Mutation check: five mutants, each killed by both the
v2wireand thewebsuites:GetConversation).local-primary:<account>.WHEREinUpsertReadCursor.Runbook
docs/agent-runbook.md, in the cutover section: the mirror bullet now points to the native write instead of saying a v2-primary daemon writes nothing. A new bullet describes how the id is resolved and which device the cursor lands on, why the message is NULL, the log line, and a read-only query for the latest cursors.Verification
GOWORK=off go test -race -count=1 ./internal/v2read/ ./internal/v2wire/ ./internal/v2keys/ ./internal/migration/ ./internal/web/passes.go build ./...andgo vet(touched packages and./cmd/) are clean../...locally, to spare the shared build cache while host disk is low; CI runs it.Not in this PR
repointLocalMessagedeletes the echo duplicate without checkingMessageHasReadCursor, unlike id-space repair. This PR adds no positioned cursor, but any writer that positions one on an unreconciled echo would wedge that send's reconcile. Ingest receipts and the outbox read path both position cursors. I haven't checked whether either can land on an echo, so this needs a look before S5b/S8 relies on positioned cursors.20261009-111730-om217-review) found that the legacy mirror doesn't normalize Signal natural keys. That fix belongs to Reuse the migrated local device in the legacy mirror; stop UpsertDevice FK scans #217, and this PR doesn't touchmirror.go.🤖 Generated with Claude Code