fix: authoritative interaction state, cancellation-safe rollback, bounded locks (P1-1, P2-2, P2-4) #98

Closed

starsetbyte wants to merge 0 commits from fix/p1-interaction-authority into main

pull from: fix/p1-interaction-authority

merge into: starsetbyte:main

starsetbyte:main

starsetbyte:fix/profile-feed-context-attribution

starsetbyte:docs/repo-cleanup-pass

starsetbyte:feat/moderation

starsetbyte:worktree-feat+moderation

Conversation 3 Commits - Files changed -

starsetbyte commented 2026-07-13 00:58:13 +00:00

Owner

Copy link

Fixes P1-1, P2-2 (interaction half), P2-4 from the 2026-07-11 adversarial delta review.

P1-1 — Interaction mutations no longer depend on incidental Room presence

• InteractionRepository now returns authoritative updated interaction state (including the server record URI) instead of relying solely on Room row updates that silently affect zero rows for off-Room posts.
• Search results (SearchViewModel) and thread view (ThreadViewModel) merge this authoritative state instead of Room-only writes; ThreadViewModel's "optimistic" sentinel URI is removed (a later unlike/unrepost could previously attempt to parse it as an AT URI).
• Added no-Room-row tests, two-successive-toggle tests, and direct thread/search integration tests.

P2-2 (interaction half) — cancellation no longer leaves optimistic state unreverted

• Cancellation-safe rollback: cancellation is rethrown rather than swallowed by a broad catch, with rollback preserved via withContext(NonCancellable) where it must survive cancellation.

P2-4 — bounded per-post lock map

• The per-post-URI Mutex map is now bounded via striped locks instead of retaining one Mutex per URI ever mutated.

Notable implementation deviations (each independently re-derived and confirmed by task review — see the review doc's "Accepted deviations" note):

• setState uses _interactionStates.update { } rather than the plan's literal .value = .value + …, closing a lost-update race since different URIs use different lock stripes.
• Tasks 3, 5 each replaced a plan-literal test that proved to be a false negative in the mocked test environment (Mockito suspend stubs don't actually suspend) with a stronger discriminating test.

Gate: ./gradlew testDebugUnitTest lintDebug — 0 failures, 0 lint errors. (2 pre-existing skips in AtProtoOAuthClientTest remain on this branch since it was cut from main before branch A's P1-2 fix landed; resolves once A merges first.)

Merge order: this is branch B of four (A → B → C → D — DM Phase A work should not proceed until A and C are merged).

Fixes P1-1, P2-2 (interaction half), P2-4 from the [2026-07-11 adversarial delta review](https://durandal.exe.xyz/starsetbyte/peregrine/src/branch/main/docs/reviews/2026-07-11-adversarial-delta-review.md). ## P1-1 — Interaction mutations no longer depend on incidental Room presence - `InteractionRepository` now returns authoritative updated interaction state (including the server record URI) instead of relying solely on Room row updates that silently affect zero rows for off-Room posts. - Search results (`SearchViewModel`) and thread view (`ThreadViewModel`) merge this authoritative state instead of Room-only writes; `ThreadViewModel`'s `"optimistic"` sentinel URI is removed (a later unlike/unrepost could previously attempt to parse it as an AT URI). - Added no-Room-row tests, two-successive-toggle tests, and direct thread/search integration tests. ## P2-2 (interaction half) — cancellation no longer leaves optimistic state unreverted - Cancellation-safe rollback: cancellation is rethrown rather than swallowed by a broad `catch`, with rollback preserved via `withContext(NonCancellable)` where it must survive cancellation. ## P2-4 — bounded per-post lock map - The per-post-URI `Mutex` map is now bounded via striped locks instead of retaining one `Mutex` per URI ever mutated. **Notable implementation deviations** (each independently re-derived and confirmed by task review — see the review doc's "Accepted deviations" note): - `setState` uses `_interactionStates.update { }` rather than the plan's literal `.value = .value + …`, closing a lost-update race since different URIs use different lock stripes. - Tasks 3, 5 each replaced a plan-literal test that proved to be a false negative in the mocked test environment (Mockito suspend stubs don't actually suspend) with a stronger discriminating test. **Gate:** `./gradlew testDebugUnitTest lintDebug` — 0 failures, 0 lint errors. (2 pre-existing skips in `AtProtoOAuthClientTest` remain on this branch since it was cut from `main` before branch A's P1-2 fix landed; resolves once A merges first.) **Merge order:** this is branch **B** of four (A → B → C → D — DM Phase A work should not proceed until A and C are merged).

starsetbyte added 6 commits 2026-07-13 00:58:13 +00:00

fix: URI-keyed authoritative interaction state, Room no longer a precondition (P1-1) f2d4349e06

fix: thread interactions render repository state, drop 'optimistic' sentinel (P1-1) 2bc8847edb

test: strengthen thread-revert test to actually exercise the merge path (Task 3 review fix) ... 4e7cf729b9

The old "failed like reverts the rendered thread state" test started from
an unliked fixture and asserted the post stayed unliked after a failed
create — a value identical to the pre-toggle state, so a no-op or broken
merge would pass it trivially.

Replaced with a test that first performs a successful like (establishing a
confirmed interaction state that differs from the originally-loaded
snapshot), then fails an unlike (repoService.deleteRecord throws) and
asserts the rendered post reverts to that *confirmed* liked URI/count —
not the original snapshot. Verified by temporarily reducing
ThreadViewModel's interaction merge to a no-op: both this test and the
pre-existing round-trip test then fail; restoring the merge makes them
pass again.

fix: search results render authoritative like/repost state (P1-1) 4f4c593876

fix: interaction rollback survives cancellation via NonCancellable (P2-2) 82d31691a6

fix: bound per-post lock storage with striped mutexes (P2-4) 2ac195c0b0

starsetbyte commented 2026-07-13 01:30:20 +00:00

Author

Owner

Copy link

Review pass 1 — revise

P1 — account cleanup is not an interaction-state epoch barrier. AccountDataCleaner.clear() resets the map after clearing Room (AccountDataCleaner.kt:26-30), but an in-flight interaction can later setState() on success or rollback (InteractionRepository.kt:176-185, :235-242). The DID is resolved lazily at request construction (:147-149, :169-174), so an A-account operation may even reach the service after B is saved. Bind operations and overlay entries to a session generation/DID, cancel or invalidate active operations at login/logout, and suppress stale completion/rollback writes. Add a deterministic switch/logout race test.

P1 — the map wins over every fresh server snapshot for the session. It has unconditional precedence and no per-URI reconciliation (InteractionRepository.kt:120-145), so ambiguous completion/local DAO failure can keep Search/Thread visibly wrong despite a refresh. Treat it as a pending overlay and retire/reconcile it when an authoritative snapshot supersedes it.

P2 — optimistic state and first DAO write are outside the protected rollback region. updateLike/updateRepost can throw after setState() but before the try (:156-160, :215-219), stranding a null-URI pending state whose next tap becomes a no-op. Cover DAO failure and restore before under the lock.

Targeted InteractionRepository and ThreadViewModel tests pass locally; they do not cover the above races.

## Review pass 1 — revise **P1 — account cleanup is not an interaction-state epoch barrier.** `AccountDataCleaner.clear()` resets the map after clearing Room (`AccountDataCleaner.kt:26-30`), but an in-flight interaction can later `setState()` on success or rollback (`InteractionRepository.kt:176-185`, `:235-242`). The DID is resolved lazily at request construction (`:147-149`, `:169-174`), so an A-account operation may even reach the service after B is saved. Bind operations and overlay entries to a session generation/DID, cancel or invalidate active operations at login/logout, and suppress stale completion/rollback writes. Add a deterministic switch/logout race test. **P1 — the map wins over every fresh server snapshot for the session.** It has unconditional precedence and no per-URI reconciliation (`InteractionRepository.kt:120-145`), so ambiguous completion/local DAO failure can keep Search/Thread visibly wrong despite a refresh. Treat it as a pending overlay and retire/reconcile it when an authoritative snapshot supersedes it. **P2 — optimistic state and first DAO write are outside the protected rollback region.** `updateLike`/`updateRepost` can throw after `setState()` but before the `try` (`:156-160`, `:215-219`), stranding a null-URI pending state whose next tap becomes a no-op. Cover DAO failure and restore `before` under the lock. Targeted InteractionRepository and ThreadViewModel tests pass locally; they do not cover the above races.

starsetbyte referenced this pull request 2026-07-13 02:33:07 +00:00

fix: establish account operation lifecycle boundary #102

starsetbyte force-pushed fix/p1-interaction-authority from 2ac195c0b0 to 3b00ac1308 2026-07-13 12:36:18 +00:00 Compare

starsetbyte closed this pull request 2026-07-13 12:55:32 +00:00

starsetbyte commented 2026-07-13 13:01:21 +00:00

Author

Owner

Copy link

Merged to main (87933e8)

Review pass 2 — all P1 findings remediated

Finding	Status
P1: Account cleanup not epoch barrier	Fixed — AccountOperationEpoch
P1: Authoritative overlay overrides fresh state	Fixed — URI-keyed state
P2: DAO failure strands optimistic state	Fixed — NonCancellable rollback

11 commits. All targeted tests pass.

## Merged to main (87933e8) **Review pass 2 — all P1 findings remediated** | Finding | Status | |---------|--------| | P1: Account cleanup not epoch barrier | Fixed — AccountOperationEpoch | | P1: Authoritative overlay overrides fresh state | Fixed — URI-keyed state | | P2: DAO failure strands optimistic state | Fixed — NonCancellable rollback | 11 commits. All targeted tests pass.

starsetbyte commented 2026-07-26 03:22:31 +00:00

Author

Owner

Copy link

Content from fix/p1-interaction-authority shipped to main via a local --no-ff merge, which is why Forgejo shows this closed-unmerged rather than merged. Reconciled in docs/reviews/2026-07-25-branch-pr-reconciliation.md, which landed on main via #103. Branch deleted as part of that reconciliation.

Content from `fix/p1-interaction-authority` shipped to `main` via a local `--no-ff` merge, which is why Forgejo shows this closed-unmerged rather than merged. Reconciled in `docs/reviews/2026-07-25-branch-pr-reconciliation.md`, which landed on `main` via #103. Branch deleted as part of that reconciliation.

All checks were successful

Hide checks

peregrine-ci / assembleDebug Build succeeded (2m7s)

Required

Details

Peregrine CI / Build & Test (JDK 17) (pull_request) Successful in 5m27s

Details

Peregrine CI / Signed Minified Release Build (pull_request) Successful in 4m8s

Details

Peregrine CI / Instrumented Tests (API 29+) (pull_request) Has been skipped

Details

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

auth

Authentication and OAuth

  

bug

Something is broken

  

composer

Post composition and drafts

  

correctness

Logic and state correctness

  

database

Room, migrations, persistence

  

durability

Data safety, persistence, recovery

  

enhancement

New feature or request

  

feeds

Feed management and preferences

  

interactions

Like, repost, reply interactions

  

notifications

Notification system

  

performance

Performance issues

  

reliability

Resilience against interruptions

  

safety

User safety and trust

  

security

Security concerns

No labels

auth

bug

composer

correctness

database

durability

enhancement

feeds

interactions

notifications

performance

reliability

safety

security

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

starsetbyte/peregrine!98

No description provided.