mirror of
https://github.com/esengine/DeepSeek-Reasonix.git
synced 2026-11-07 10:45:59 -10:00
fix(session): a conflict stops turning into a second conversation
Changing the model produced a duplicate session and a notice about a conflict copy. The transcript in that copy was byte-identical to the one it forked from, written by the same process under its own lease: nothing was in conflict and nothing was at risk. The runtime had simply lost track of its own file — a ledger digest describing content nobody ever wrote — and could not recognise the transcript as its own, so it read itself as a foreign writer. The fork then copied the same mismatch onto both files, so the next switch forked again. The lease already decides who may write: a released one fails the save closed rather than forking. It now decides the other half too. A live authority covering the path means the file is this runtime's, whatever the ledger says, and the write lands and restamps it. Nothing about it forks. What the write replaces is kept beside the session under a name no listing globs, so no bytes are lost to the change and no second conversation appears for the user to wonder about. This trades away a case: where another writer really did reach a leased file, its write is now archived rather than promoted to a conversation of its own. That is the intent — a second identical session in the list has never been the right way to tell a user something happened.
This commit is contained in:
1 parent
b18d8ef3aa
commit
004557bbfd
6 files changed
+258
-57
No files matched your search
@@ -117,7 +117,8 @@ type snapshotWriteDecision struct {
|
||||
// repairLog is set when the on-disk event log was damaged (torn tail with
|
||||
// a lost suffix, or nothing decodable): the safe write shape is a full
|
||||
// rewrite that also compacts the log back to a healthy single event.
|
||||
repairLog bool
|
||||
repairLog bool
|
||||
supersedes bool // lease-authorized write over a disagreeing transcript
|
||||
// ledgerStale is set when the on-disk transcript already matches the
|
||||
// snapshot but the meta ledger still describes older content — the
|
||||
// aftermath of a save whose bytes landed and whose revision record then
|
||||
@@ -419,6 +420,7 @@ func (s *Session) saveLocked(path string, mode sessionSaveMode) error {
|
||||
return err
|
||||
}
|
||||
}
|
||||
keepSupersededTranscript(path, decision.supersedes)
|
||||
if err := writeSessionMessages(path, msgs); err != nil {
|
||||
return err
|
||||
}
|
||||
@@ -587,9 +589,9 @@ func (s *Session) checkSnapshotWrite(path string, next []provider.Message, nextD
|
||||
return snapshotWriteDecision{revision: currentRevision, repairLog: current.eventLogDamaged}, nil
|
||||
}
|
||||
}
|
||||
// Bound controllers: missing/stale authority must not fork recovery.
|
||||
if err := s.authorityErrorForPath(path); err != nil {
|
||||
return snapshotWriteDecision{}, err
|
||||
// Who holds the lease decides; see leaseholderDecision.
|
||||
if decision, handled, err := s.leaseholderDecision(path, currentRevision, current.eventLogDamaged); handled {
|
||||
return decision, err
|
||||
}
|
||||
if messagesHavePrefix(existing, next) || messagesHavePrefixWithCompatibleSystem(existing, next) ||
|
||||
(rawDiffers && (messagesHavePrefix(raw, next) || messagesHavePrefixWithCompatibleSystem(raw, next))) {
|
||||
|
||||
@@ -302,3 +302,107 @@ func TestSaveSnapshotZeroByteCheckpointHealsFromWAL(t *testing.T) {
|
||||
t.Fatalf("checkpoint size = %v err=%v, want healed non-empty", info, err)
|
||||
}
|
||||
}
|
||||
|
||||
// #4103: a model switch forked a byte-identical "conflict copy". The rebuilt
|
||||
// controller carries no persistence baseline and holds a reshape rather than an
|
||||
// extension, which together used to read as a foreign writer. The lease says
|
||||
// otherwise: nobody else may write this file.
|
||||
func TestSaveSnapshotLeaseHeldWithoutBaselineRewritesInsteadOfForking(t *testing.T) {
|
||||
path := filepath.Join(t.TempDir(), "session.jsonl")
|
||||
base := NewSession("sys")
|
||||
base.Add(provider.Message{Role: provider.RoleUser, Content: "ask"})
|
||||
base.Add(provider.Message{
|
||||
Role: provider.RoleAssistant, Content: "answering",
|
||||
ToolCalls: []provider.ToolCall{{ID: "call_1", Name: "edit", Arguments: `{"path":"f"}`}},
|
||||
})
|
||||
if err := base.Save(path); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
// The rebuilt runtime: same conversation, reshaped tool row, and no
|
||||
// persisted baseline of its own — exactly what boot leaves after a switch.
|
||||
rebuilt := NewSession("sys")
|
||||
rebuilt.Add(provider.Message{Role: provider.RoleUser, Content: "ask"})
|
||||
rebuilt.Add(provider.Message{
|
||||
Role: provider.RoleAssistant, Content: "answering",
|
||||
ToolCalls: []provider.ToolCall{{
|
||||
ID: "call_1", Name: "edit", Arguments: `{"path":"f"}`,
|
||||
Diff: "@@ -1 +1 @@\n-a\n+b\n", Added: 1, Removed: 1,
|
||||
}},
|
||||
})
|
||||
|
||||
lease, err := TryAcquireSessionLease(path)
|
||||
if err != nil {
|
||||
t.Fatalf("TryAcquireSessionLease: %v", err)
|
||||
}
|
||||
defer lease.Release()
|
||||
auth, err := lease.IssueWriteAuthority(NextSessionWriteGeneration())
|
||||
if err != nil {
|
||||
t.Fatalf("IssueWriteAuthority: %v", err)
|
||||
}
|
||||
rebuilt.BindWriteAuthority(auth)
|
||||
|
||||
if err := rebuilt.SaveSnapshot(path); err != nil {
|
||||
t.Fatalf("SaveSnapshot under a held lease = %v, want the write to land", err)
|
||||
}
|
||||
if errors.Is(err, ErrSessionSnapshotConflict) {
|
||||
t.Fatal("a leaseholder still forked a recovery branch")
|
||||
}
|
||||
|
||||
// The file is the runtime's now, and the ledger describes what is in it.
|
||||
back, err := LoadSession(path)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
// Load normalizes the dangling tool call into a placeholder result, so the
|
||||
// reloaded transcript is one longer; the reshaped row is what matters.
|
||||
got := back.Snapshot()
|
||||
if len(got) < 3 {
|
||||
t.Fatalf("reloaded %d messages, want at least the three written", len(got))
|
||||
}
|
||||
if len(got[2].ToolCalls) == 0 || got[2].ToolCalls[0].Diff == "" {
|
||||
t.Fatalf("the rewrite did not persist the reshaped tool row: %+v", got[2].ToolCalls)
|
||||
}
|
||||
raw, _, _, err := loadSessionMessages(path)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
digest, err := digestSessionMessages(raw)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
_, ledger, err := sessionContentRevision(path)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if ledger != digestString(digest) {
|
||||
t.Fatalf("ledger digest %s does not describe the transcript beside it (%s)", ledger, digestString(digest))
|
||||
}
|
||||
}
|
||||
|
||||
// Without a lease the same shape is still refused: removing the fork must not
|
||||
// turn "somebody else owns this file" into a silent overwrite.
|
||||
func TestSaveSnapshotWithoutLeaseStillRefusesAReshape(t *testing.T) {
|
||||
path := filepath.Join(t.TempDir(), "session.jsonl")
|
||||
base := NewSession("sys")
|
||||
base.Add(provider.Message{Role: provider.RoleUser, Content: "ask"})
|
||||
base.Add(provider.Message{
|
||||
Role: provider.RoleAssistant, Content: "answering",
|
||||
ToolCalls: []provider.ToolCall{{ID: "call_1", Name: "edit", Arguments: `{"path":"f"}`}},
|
||||
})
|
||||
if err := base.Save(path); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
rebuilt := NewSession("sys")
|
||||
rebuilt.Add(provider.Message{Role: provider.RoleUser, Content: "ask"})
|
||||
rebuilt.Add(provider.Message{
|
||||
Role: provider.RoleAssistant, Content: "answering",
|
||||
ToolCalls: []provider.ToolCall{{
|
||||
ID: "call_1", Name: "edit", Arguments: `{"path":"f"}`,
|
||||
Diff: "@@ -1 +1 @@\n-a\n+b\n", Added: 1, Removed: 1,
|
||||
}},
|
||||
})
|
||||
if err := rebuilt.SaveSnapshot(path); !errors.Is(err, ErrSessionSnapshotConflict) {
|
||||
t.Fatalf("SaveSnapshot without a lease = %v, want ErrSessionSnapshotConflict", err)
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,71 @@
|
||||
// Superseding a transcript instead of forking a second conversation.
|
||||
package agent
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"os"
|
||||
"time"
|
||||
|
||||
"reasonix/internal/store"
|
||||
)
|
||||
|
||||
// supersedingDecision is the write a leaseholder is entitled to over a
|
||||
// transcript that disagrees with it. Holding a live lease and still failing to
|
||||
// recognise the file means this runtime lost track of its own — not that
|
||||
// somebody else wrote it — and forking there hands the user a second identical
|
||||
// conversation whose copy carries the same mismatch into the next save.
|
||||
func supersedingDecision(revision int64, repairLog bool) snapshotWriteDecision {
|
||||
return snapshotWriteDecision{revision: revision, repairLog: repairLog, supersedes: true}
|
||||
}
|
||||
|
||||
// leaseholderDecision answers a save the content checks could not vouch for by
|
||||
// asking who holds the lease: a stale or missing authority fails closed, a live
|
||||
// one supersedes. handled is false only for a session that never bound any
|
||||
// authority — the low-level CAS path the store's own tests exercise.
|
||||
func (s *Session) leaseholderDecision(path string, revision int64, repairLog bool) (snapshotWriteDecision, bool, error) {
|
||||
if err := s.authorityErrorForPath(path); err != nil {
|
||||
return snapshotWriteDecision{}, true, err
|
||||
}
|
||||
if s.hasValidWriteAuthority(path) {
|
||||
return supersedingDecision(revision, repairLog), true, nil
|
||||
}
|
||||
return snapshotWriteDecision{}, false, nil
|
||||
}
|
||||
|
||||
// keepSupersededTranscript copies what is on disk aside before a leaseholder
|
||||
// writes over it. Holding the lease makes the write authoritative, but bytes
|
||||
// nobody chose to discard are still worth keeping: the alternative used to be
|
||||
// a recovery branch, which kept them by turning them into a second identical
|
||||
// conversation in the user's session list.
|
||||
//
|
||||
// Failures are warnings. A save whose transcript is authoritative must not be
|
||||
// held up by an archive of what it replaced.
|
||||
func keepSupersededTranscript(path string, superseding bool) {
|
||||
if !superseding || path == "" {
|
||||
return
|
||||
}
|
||||
current, err := os.ReadFile(path)
|
||||
if err != nil || len(current) == 0 {
|
||||
return
|
||||
}
|
||||
dest := store.SessionSuperseded(path)
|
||||
if dest == "" {
|
||||
return
|
||||
}
|
||||
header := fmt.Sprintf("// superseded at %s by the runtime holding this session's lease\n",
|
||||
time.Now().UTC().Format(time.RFC3339Nano))
|
||||
f, err := os.OpenFile(dest, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o600)
|
||||
if err != nil {
|
||||
slog.Warn("session: could not keep the superseded transcript", "path", dest, "err", err)
|
||||
return
|
||||
}
|
||||
defer f.Close()
|
||||
if _, err := f.WriteString(header); err != nil {
|
||||
slog.Warn("session: could not keep the superseded transcript", "path", dest, "err", err)
|
||||
return
|
||||
}
|
||||
if _, err := f.Write(current); err != nil {
|
||||
slog.Warn("session: could not keep the superseded transcript", "path", dest, "err", err)
|
||||
}
|
||||
}
|
||||
@@ -12,6 +12,7 @@ import (
|
||||
"reasonix/internal/control"
|
||||
"reasonix/internal/event"
|
||||
"reasonix/internal/provider"
|
||||
"reasonix/internal/store"
|
||||
"reasonix/internal/tool"
|
||||
)
|
||||
|
||||
@@ -112,13 +113,13 @@ func primarySessionFiles(paths []string) []string {
|
||||
return out
|
||||
}
|
||||
|
||||
// TestSwitchModelContinuesRecoveryPathAfterSnapshotConflict is the serve twin
|
||||
// of the desktop rebuild fix: when the pre-switch Snapshot hits a conflict and
|
||||
// retargets the old controller to a recovery branch, the rebuilt controller
|
||||
// must continue on that recovery path. Capturing prevPath before Snapshot
|
||||
// bound the just-recovered transcript back to the original file, so every
|
||||
// later save re-conflicted and derived yet another recovery branch.
|
||||
func TestSwitchModelContinuesRecoveryPathAfterSnapshotConflict(t *testing.T) {
|
||||
// #4103: a model switch used to answer a snapshot conflict by forking the
|
||||
// transcript, and the user got a second identical conversation in their list.
|
||||
// The lease is what says whose file this is: holding it makes the write
|
||||
// authoritative, so the switch stays on the one session. What the disk held is
|
||||
// kept beside it under a name no session listing globs, so the bytes survive
|
||||
// without becoming a conversation.
|
||||
func TestSwitchModelStaysOnOneSessionThroughASnapshotConflict(t *testing.T) {
|
||||
t.Setenv("REASONIX_HOME", t.TempDir())
|
||||
dir := t.TempDir()
|
||||
originalPath := filepath.Join(dir, "switch-conflict.jsonl")
|
||||
@@ -167,61 +168,54 @@ func TestSwitchModelContinuesRecoveryPathAfterSnapshotConflict(t *testing.T) {
|
||||
t.Fatalf("switchModel: %v", err)
|
||||
}
|
||||
|
||||
recoveryPath := built.SessionPath()
|
||||
if recoveryPath == "" || recoveryPath == originalPath || !strings.Contains(filepath.Base(recoveryPath), "-recovery-") {
|
||||
t.Fatalf("switched session path = %q, want recovery path distinct from %q", recoveryPath, originalPath)
|
||||
if got := built.SessionPath(); got != originalPath {
|
||||
t.Fatalf("switched session path = %q, want the original %q", got, originalPath)
|
||||
}
|
||||
if s.ctl() != built {
|
||||
t.Fatal("switchModel did not publish the rebuilt controller")
|
||||
}
|
||||
if got, want := leases.HeldPath(), agent.CanonicalSessionPath(recoveryPath); got != want {
|
||||
t.Fatalf("lease after pre-switch recovery = %q, want %q", got, want)
|
||||
if got, want := leases.HeldPath(), agent.CanonicalSessionPath(originalPath); got != want {
|
||||
t.Fatalf("lease after the switch = %q, want %q", got, want)
|
||||
}
|
||||
assertNoRecoveryBranch(t, dir, "after the switch")
|
||||
|
||||
// The bytes the write replaced are kept, and not as a session.
|
||||
kept := store.SessionSuperseded(originalPath)
|
||||
if body, err := os.ReadFile(kept); err != nil {
|
||||
t.Fatalf("superseded transcript not kept: %v", err)
|
||||
} else if !strings.Contains(string(body), "disk second") {
|
||||
t.Fatal("the superseded copy does not hold what was overwritten")
|
||||
}
|
||||
|
||||
// And a later divergence does the same thing: still one session.
|
||||
diskAgain, err := agent.LoadSession(originalPath)
|
||||
if err != nil {
|
||||
t.Fatalf("load transcript for external change: %v", err)
|
||||
}
|
||||
diskAgain.Add(provider.Message{Role: provider.RoleUser, Content: "disk third"})
|
||||
if err := diskAgain.Save(originalPath); err != nil {
|
||||
t.Fatalf("save external change: %v", err)
|
||||
}
|
||||
built.Executor().Session().Add(provider.Message{Role: provider.RoleUser, Content: "local third"})
|
||||
if err := built.Snapshot(); err != nil {
|
||||
t.Fatalf("Snapshot after a second divergence: %v", err)
|
||||
}
|
||||
if got := built.SessionPath(); got != originalPath {
|
||||
t.Fatalf("path after a second divergence = %q, want the original %q", got, originalPath)
|
||||
}
|
||||
assertNoRecoveryBranch(t, dir, "after a second divergence")
|
||||
}
|
||||
|
||||
// assertNoRecoveryBranch is the whole point of #4103: no run of this should
|
||||
// ever leave a second conversation in the directory.
|
||||
func assertNoRecoveryBranch(t *testing.T, dir, when string) {
|
||||
t.Helper()
|
||||
matches, err := filepath.Glob(filepath.Join(dir, "*-recovery-*.jsonl"))
|
||||
if err != nil {
|
||||
t.Fatalf("glob recovery branches: %v", err)
|
||||
}
|
||||
matches = primarySessionFiles(matches)
|
||||
if len(matches) != 1 || matches[0] != recoveryPath {
|
||||
t.Fatalf("recovery branches after switch = %v, want only %q", matches, recoveryPath)
|
||||
}
|
||||
|
||||
// The rebuilt controller adopted the recovery file's baseline, so its next
|
||||
// snapshot must not derive a second recovery branch.
|
||||
if err := built.Snapshot(); err != nil {
|
||||
t.Fatalf("Snapshot after switch: %v", err)
|
||||
}
|
||||
matches, err = filepath.Glob(filepath.Join(dir, "*-recovery-*.jsonl"))
|
||||
if err != nil {
|
||||
t.Fatalf("glob recovery branches after snapshot: %v", err)
|
||||
}
|
||||
matches = primarySessionFiles(matches)
|
||||
if len(matches) != 1 || matches[0] != recoveryPath {
|
||||
t.Fatalf("recovery branches after follow-up snapshot = %v, want only %q", matches, recoveryPath)
|
||||
}
|
||||
|
||||
// A later ordinary autosave on the rebuilt controller must use the same
|
||||
// ownership callback. Force another divergence after the switch and verify
|
||||
// the keeper follows the second recovery before the controller commits it.
|
||||
diskAfterSwitch, err := agent.LoadSession(recoveryPath)
|
||||
if err != nil {
|
||||
t.Fatalf("load recovery transcript for external change: %v", err)
|
||||
}
|
||||
diskAfterSwitch.Add(provider.Message{Role: provider.RoleUser, Content: "disk third"})
|
||||
if err := diskAfterSwitch.Save(recoveryPath); err != nil {
|
||||
t.Fatalf("save external recovery transcript change: %v", err)
|
||||
}
|
||||
built.Executor().Session().Add(provider.Message{Role: provider.RoleUser, Content: "local third"})
|
||||
if err := built.Snapshot(); err != nil {
|
||||
t.Fatalf("Snapshot rebuilt controller after divergence: %v", err)
|
||||
}
|
||||
secondRecoveryPath := built.SessionPath()
|
||||
if secondRecoveryPath == recoveryPath || !strings.Contains(filepath.Base(secondRecoveryPath), "-recovery-") {
|
||||
t.Fatalf("rebuilt controller path = %q, want recovery path distinct from %q", secondRecoveryPath, recoveryPath)
|
||||
}
|
||||
if got, want := leases.HeldPath(), agent.CanonicalSessionPath(secondRecoveryPath); got != want {
|
||||
t.Fatalf("lease after rebuilt-controller recovery = %q, want %q", got, want)
|
||||
if got := primarySessionFiles(matches); len(got) != 0 {
|
||||
t.Fatalf("recovery branches %s = %v, want none", when, got)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -151,6 +151,17 @@ func SessionConflictLog(sessionPath string) string {
|
||||
return sessionStem(sessionPath) + ".conflicts.jsonl"
|
||||
}
|
||||
|
||||
// SessionSuperseded holds what a leaseholder's overriding write replaced. The
|
||||
// suffix deliberately does not end in ".jsonl": a session listing globs for
|
||||
// those, and the point of keeping these bytes this way is that no second
|
||||
// conversation shows up next to the one the user is in.
|
||||
func SessionSuperseded(sessionPath string) string {
|
||||
if sessionPath == "" {
|
||||
return ""
|
||||
}
|
||||
return sessionStem(sessionPath) + ".superseded.jsonl.bak"
|
||||
}
|
||||
|
||||
// SessionLockFile is the advisory save lock (<id>.jsonl.lock).
|
||||
func SessionLockFile(sessionPath string) string {
|
||||
if sessionPath == "" {
|
||||
|
||||
@@ -1,6 +1,9 @@
|
||||
package store
|
||||
|
||||
import "testing"
|
||||
import (
|
||||
"path/filepath"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestSessionSidecarLayout(t *testing.T) {
|
||||
const p = "/home/u/.reasonix/sessions/abc.jsonl"
|
||||
@@ -110,3 +113,19 @@ func TestSessionSidecarFiles(t *testing.T) {
|
||||
t.Error("SessionSidecarFiles(\"\") should be nil")
|
||||
}
|
||||
}
|
||||
|
||||
// #4103: the superseded copy exists so a leaseholder's overriding write loses
|
||||
// no bytes. It must never be a session — that is the whole difference between
|
||||
// it and the recovery branch it replaces.
|
||||
func TestSupersededTranscriptIsNotASession(t *testing.T) {
|
||||
name := filepath.Base(SessionSuperseded("/tmp/sessions/20260820-1-model.jsonl"))
|
||||
if name == "" {
|
||||
t.Fatal("no superseded path")
|
||||
}
|
||||
if IsSessionTranscriptName(name) {
|
||||
t.Fatalf("%q lists as a conversation; the whole point is that it does not", name)
|
||||
}
|
||||
if IsSessionTranscriptName(filepath.Base(SessionConflictLog("/tmp/sessions/x.jsonl"))) {
|
||||
t.Fatal("the conflict log lists as a conversation")
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user