Skip to content

fix(session): publish rollback invalidation before replacement root - #1295

Merged
yzxoi merged 2 commits into
devfrom
synergy/session-fa8b6c
Sep 1, 2026
Merged

fix(session): publish rollback invalidation before replacement root#1295
yzxoi merged 2 commits into
devfrom
synergy/session-fa8b6c

Conversation

@yzxoi

@yzxoi yzxoi commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

After a rewind, the frontend prefix-hides everything at/after the rollback cut while the session projection still claims redo is available. Session.updateMessage published the replacement root's message.updated before flipping the persisted projection, so the new branch — including the latest assistant reply — could arrive while canUnrollback was still true and stay hidden until a refresh re-fetched the authoritative page. Tool cards stayed visible because they ride unsequenced streaming events, matching the reported symptom.

Change

Session.updateMessage now awaits publishRollbackInvalidation before publishing the written message's message.updated, so the invalidation's session.updated (canUnrollback: false) is always sequenced ahead of the replacement root.

Verification

  • New regression test in packages/synergy/test/session/rollback.test.ts asserts the bus publish order (fails before, passes after).
  • Full rollback suite: 22 pass, 0 fail.
  • Pre-commit gates (lint, decision check, doc check) all pass.
  • Added a bug-fix decision record.

The frontend prefix-hides everything after a rewind cut while the session
projection still claims redo is available. The replacement root's
message.updated could arrive before the flip that invalidates redo, so the
new branch (including the latest assistant reply) stayed hidden until a
refresh re-fetched the authoritative page. Sequence the projection flip
before the message publish.

Co-authored-by: synergy-agent <299070056+synergy-agent@users.noreply.github.com>
@synergy-agent

synergy-agent Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Review complete. All checks run green, the regression test is genuine (verified red/green), and the ordering claim holds up against both the bus implementation and the frontend prefix-cut semantics. Here is my review.


Review: #1295 — publish rollback invalidation before replacement root

Verdict: sound, no blocking findings. The change is minimal (a 5-line reorder in Session.updateMessage), the diagnosis matches the code, and the regression test genuinely fails against the old ordering.

What I verified

Root cause matches the code. packages/synergy/src/session/index.ts:1137-1155 (publishRollbackInvalidation) flips canUnrollback via a persisted Session.update, and it was previously invoked after Bus.publish(MessageV2.Event.Updated, ...) (index.ts:1167-1169). The frontend prefix-cut in packages/app/src/components/session/session-message-order.ts:22-44 hides everything at/after the cut while canUnrollback is true and no post-cut root outside the dropped set is loaded; packages/app/src/pages/session.tsx:412-420 applies it against the session record's rollback(). So a replacement root arriving before the flip could be prefix-hidden until a re-fetch — exactly the reported symptom.

The reorder actually guarantees the ordering. Bus.publish (packages/synergy/src/bus/index.ts:62-129) stamps seq/epoch synchronously at dispatch time and awaits all non-streaming subscriber tasks before returning; publishInfo (index.ts:355-380) calls it inline (not fire-and-forget). Since updateMessage now awaits the flip, the flip's session.updated is sequenced ahead of the root's message.updated on the wire.

The flip can't be swallowed by publish dedup. publishCompareKey (packages/synergy/src/session/publish-dedup.ts:17-27) strips only time.updated; decideSessionPublish publishes immediately on any key change (:43). canUnrollback: true → false is a key change, so the 1000 ms throttle never suppresses it.

No deadlock on the hot path. updateMessage holds no SessionMutation lock when calling the flip's Session.update (updateInternal acquires it at index.ts:852), and MessageV2.writeInfo's lock (message-v2.ts:1429-1432) is a different key — no reentrancy. The once-per-session guard (rollbackInvalidationPending, index.ts:1135, :1143) plus early returns for assistant/non-root/no-rollback writes keep the added cost off every other write.

Tests are real evidence.

  • With the PR ordering: bun test test/session/rollback.test.ts22 pass, 0 fail (matches the PR's claim).
  • Temporarily restoring the old ordering: exactly the new test fails (rollback invalidation publishes before the replacement root's message.updated), 21 pass / 1 fail. The test asserts flip-before-message through the bus, not source text.
  • bun run decision:check passes; the record at docs/decisions/implemented/bug-fix/2026-08-31-rollback-invalidation-before-replacement-root.md follows the implemented skeleton (Problem/Decision/Alternatives/Consequences), and its cited earlier commit a843a91c8 exists and indeed shipped the flip plus the frontend newBranchLoaded degradation.

One non-blocking observation

If Session.update inside publishRollbackInvalidation throws, the error is swallowed (index.ts:1150-1152 warns) and updateMessage proceeds to publish the root under a still-true canUnrollback — the original bug window persists for that write. This is pre-existing behavior, unchanged by this PR, and the frontend newBranchLoaded degradation (session-message-order.ts:37-42) remains the residual safety net. Not a blocker; worth knowing that the fix's guarantee is best-effort on the flip's persistence succeeding.

Synergy: review complete — no blocking findings, 1 non-blocking observation

@yzxoi
yzxoi merged commit 9a3106a into dev Sep 1, 2026
10 checks passed
@yzxoi
yzxoi deleted the synergy/session-fa8b6c branch September 1, 2026 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant