Two write handlers in one file roll back differently

Type

Bug — inconsistent error path

Found while

SAPE-19

Triage

Defer

Status

Open

Severity

Medium — the window is narrow, and what it leaves behind is sticky

Description

Two handlers in internal/server write to one database through one store, under the same write lock, and they treat a failed write differently.

declare rolls back (internal/server/declare.go:99-110):

stored, err := db.store.DeclareOperation(caller, asked.Operation)
if err != nil {
	if back := db.store.Rollback(); back != nil {
		return nil, fmt.Errorf("%w (and rolling it back failed: %v)", err, back)
	}
	return nil, err
}

invoke does not (internal/server/server.go:757-762):

db.mutex.Lock()
defer db.mutex.Unlock()

result, err := db.store.Invoke(caller, asked.Command, asked.Version, asked.Arguments)
if err != nil {
	return nil, err
}

Evidence

• grep -n Rollback internal/server/*.go finds exactly one non-test call in the package: declare.go:107.
• declare's own comment names the window it is closing: "DeclareOperation validates before it writes, so the ordinary refusal has left nothing behind. The one that has is the narrow window after tree.Put and before record returns: half a declaration, uncommitted, on a database the next caller would commit for us."
• The write path invoke reaches does roll back for itself in one place: runBatch (internal/store/batch.go:81,91). That covers a batch's own error returns; it does not cover the non-batch write actions, nor a failure that returns before runBatch's error path.
• What makes leftovers sticky rather than transient is recorded at internal/server/server.go:195-200: runBatch calls Rollback on its error path and not with a defer, so a panic partway through skips it and leaves s.pages.Pending() true — and runBatch's first line refuses to run anything on a database with Pending() true, for every caller, until the process restarts.
• Note for whoever picks this up: the summary says "in one file", and they are in two — server.go and declare.go. One database file, one package, two handlers.

Triage

Does it lose data, break a promise on a public surface, or stop somebody installing and running? Not demonstrably. Committed data is never at risk — the concern is uncommitted bytes left pending after a refusal, and the next successful writer commits or rolls them as part of its own transaction. No wire shape changes either way, and the fix is invisible to every client.

Defer, with a caveat stated plainly: this was deferred because nobody has produced the failing case, not because the reasoning proves there is none. The honest status is "the window is narrow and unmeasured". If somebody does produce a case where a refused invoke leaves Pending() true, the triage answer changes to yes — that is the data-loss branch, and it would be a bug, not debt.

If deferred, what it costs to wait

• Not compatibility — this is entirely internal, and a rollback added later is invisible.
• The cost is diagnosis. The symptom is a database that refuses every write with ErrUncommitted until the process restarts, arbitrarily long after the refusal that caused it. That is the most expensive kind of bug report to receive: a wrong answer that outlives its cause.
• The cheap step while it waits is a measurement, not a fix: a fault-injected write failure through invoke, asserting Pending() is false afterwards. If it passes, this issue shrinks to a consistency tidy-up. If it fails, it is promoted.
• Whichever way it goes, the two handlers should end up with one shape. Two answers to "what does a failed write leave behind" in one package is how the next handler gets written with a third.