A read-only composed operation takes the write lock

Type

Debt — correct but pessimistic

Found while

SAPE-20

Triage

Defer

Status

Open

Severity

Low — no wrong answers, but it cancels SAPE-18 for the feature SAPE-20 sells

Description

SAPE-18 made reads share a database. A composed operation gets none of it, whatever it does, because composition is spelled action: batch and batch is classified as a write.

// internal/store/invoke.go:259
func writes(action string) bool {
	switch action {
	case ActionInsert, ActionPut, ActionUpdate, ActionDelete, ActionBatch:
		return true
	}
	return false
}

SharedRead asks writes(operation.Action) first and returns shared = false at once (invoke.go:470-472). invoke then drops the read lock, takes db.mutex.Lock() and serializes against every other call on that database (internal/server/server.go:754-760). A composed operation of three scans — the headline use — is a pure read that behaves like a write.

Evidence

• The classification is unconditional: ActionBatch is listed in writes() with no look at the steps.
• Nothing else distinguishes a read-only batch. validateOperation's ActionBatch branch (internal/store/ops.go:747) walks the steps and knows exactly which actions they are, but nothing derived from that walk is stored on the operation or consulted at call time.
• The correct-but-costly reading is deliberate elsewhere too: SharedRead's second condition refuses to share a read of a partitioned collection, because Collection.into opens and expires files. So the machinery to say "this looks like a read and is not" already exists and is used — it is the inverse case that is missing.
• Consequence measured at the design level rather than the clock: the benefit TestReadsRunTogether pins (1.6–2.1× for four readers, against 3.9–4.0 serialized) does not apply to any composed call.

Triage

Does it lose data, break a promise on a public surface, or stop somebody installing and running? No. Taking a stronger lock than needed is always safe. The wire does not change, no answer changes, and nobody is blocked.

Defer. The fix is a real design decision, not a one-liner: deciding "this batch only reads" means either recomputing it per call by walking the steps and their callees — the same recursive walk ceiling() does — or deriving it once at declaration time and storing it on the Operation. The second is better and it puts a new field on a stored declaration, which is a schema decision that deserves more than release-week attention.

If deferred, what it costs to wait

• It gets cheaper to fix in one respect: stored declarations are versioned and a derived field can be computed on read for old ones, so there is no migration either way. This is not a freezing surface.
• The cost is that the two features shipped in the same release partly cancel: SAPE-20's answer to "K round trips" is a composed read, and SAPE-18's answer to "one reader at a time" does not cover it. Anyone who adopts composition for read fan-in makes their concurrency worse, which is the opposite of both tickets' stated point.
• Cheap interim: say so where it is visible. Neither the CHANGELOG entry for composed operations nor SharedRead's doc mentions that a composed read is not shared.
• When it is done, SharedRead's partition rule must still apply to every leg, not just the parent — a composed read over a partitioned collection is still a writer wearing a reader's name.