gsadmin/pad
1
0
Fork 0
mirror of https://github.com/PerpetualSoftware/pad.git synced 2026-09-10 15:05:40 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(server): write first, apply second on the collab applier path (TASK-2989 / BUG-2840 half A) (#1318)

Browse Source 
* test(server): measure BUG-2840 half A's premise before designing a fix

Half A's plan makes step one an experiment, not a design: the claim that a
refused PATCH still lands its content was a reading of the snapshot branch
rather than an observation, and the shape of the fix depends on which half
actually bites.

Measured, on the applier path with a live room: a PATCH carrying content and a
stale expected_updated_at answers 409, leaves items.content untouched, and adds
an op-log row that outlives the request. The caller's refusal is true of the
row and false of the collaborative document.

The first version of this test was CIRCULAR and reported the premise confirmed.
It drove a ?source=collab-snapshot PATCH carrying the refused string, which
proves only that a snapshot write writes what it is given. The server cannot
close that loop at all: collab here is a dumb relay that persists opaque Yjs
updates and never parses them, so nothing server-side derives markdown from a
room's document — in production that markdown comes from a live tab's Y.Doc.
What IS observable server-side is durable collab state created by a request
that was refused, which is what this now measures.

Two details that make the harness faithful rather than convenient:

- The fake applier emits a binary op as well as the ack. A real applier is a
  browser tab that applies the markdown and broadcasts the resulting update;
  acking alone would leave no durable trace, so the experiment would have been
  measuring a peer that does not exist.
- Readiness is detected by the observable difference between the two paths — a
  succeeding probe PATCH that leaves items.content untouched proves the applier
  answered — because no exported accessor for electable connections exists and
  the manager's state is not reachable from this package.

The test asserts today's behaviour, defect included, so the fix has a baseline
to move. It skips with an explicit "premise NOT established" message if the
harness ever stops reproducing the applier writing durable state, rather than
passing vacuously.

Refs: BUG-2840

* feat(server): write first, apply second on the collab applier path (TASK-2989 / BUG-2840 half A)

PLAN-2975 decisions 2-4. A refused PATCH no longer changes the item.

The applier path used to push content into the live Y.Doc before the
row write, so any of the four typed refusals answered 4xx while the
collaborative document had already moved and the next collab-snapshot
flush carried the refused content into items.content. The reorder is
possible because TASK-2987's HasElectableApplier answers which path the
request is on without taking it.

routeContentUpdate owns the re-decision deliberately: the predecessor
retried ErrRoomActiveDuringPrune inside applyContentViaCollab and
re-called ApplyExternalContent, which could succeed through a freshly
joined applier and return nil, after which the row write still ran last
and reproduced the defect. Re-deciding before anything is written makes
that impossible rather than unlikely.

Two typed 409s join the structured family. content_not_applied answers
the hybrid the reorder creates - row write committed, content not in the
document - naming the landed fields and the new updated_at so a
content-only retry does not trip OCC. room_settling answers the standoff
where PruneAndApply blocks on any writer while election also demands
unfrozen and replay-done: the predecessor gave up after three tries and
wrote past the live peer, losing the write on its next flush.
applier_ambiguous is untouched; its outcome is unknown and a claim
either way would be false.

The measurement harness is inverted rather than deleted: it asserted the
defect and would have become a SKIP, which reads as a pass.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn

* refactor(server): retire the route-flipping helper chain the reorder replaced (TASK-2989)

applyContentViaCollab, applyContentViaCollabOnce, directWriteFn,
applyContentMaxRetries and isDeterministicWriteFailure are dead once the
router owns the decision, and golangci-lint said so. Removing them is
the point rather than tidying: that chain retried
ErrRoomActiveDuringPrune internally and re-called ApplyExternalContent,
which could succeed through a freshly joined applier and let the row
write run last after all.

Two things ported rather than dropped. isDeterministicWriteFailure's
closed-set warning moves onto writeTypedItemRefusal, which inherits the
job of recognising every typed permanent refusal. Its regression test is
ported too, unchanged in property: a refusal the handler does not
recognise is treated as recoverable and the request re-derives it by
another route, which BUG-2804 measured as a rename cascade run twice.

CONVE-23 sweep: my own comment on HasElectableApplier, merged four hours
ago, said the fallback could write content past live peers. This unit
made that false. It now states what the sentence was true of and what
replaced it, rather than being quietly deleted.

The structural guard needed teaching, not weakening: it counts the
handler's refusal blocks and failed closed when one moved into a shared
function. It now scans both files and says why three is still three.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn

* test(server): pin the settle budget itself, which every other test bypassed (TASK-2989)

Found by mutation: applierSettleBudget = 0 survived the whole suite. The
decision tests pass their own budget, so the constant had no coverage at
all — and a zero budget makes the retryable refusal the normal answer
for any room with a writer still anchoring.

The floor is the measurement the constant was sized from rather than a
number: 47ms, just above the 46.41ms worst anchoring time measured for
this deployment.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn

* test(server): bound the standoff subtest so a broken deadline fails instead of hanging (TASK-2989)

The only exit from the standoff branch is the deadline, so the mutant
that makes it unreachable spins and the failure arrives as a package
timeout with no --- FAIL line — which a mutation harness reads as 'the
package broke' rather than as a detection. Measured: that is exactly
what M5 produced.

Same shape as the waiter rule: a failure mode indistinguishable from the
waiting mode is not a signal.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn

* fix(server): content_not_applied must not assert a timeout did not land (TASK-2989)

Codex round 1, and the sharpest finding in it. ApplyExternalContent
returns ErrAllAppliersTimedOut only AFTER an applier_request has gone
out on the wire, so the elected peer may have applied the markdown and
persisted its ops while the ack was lost or merely late. Answering
content_landed:false there states as fact something the server cannot
know — the same overclaim the ruling avoided by leaving
applier_ambiguous alone, arriving one door over.

The discriminator already existed upstream and needed no new machinery:
electAndApply returns ErrNoApplierAvailable when anyWriteSucceeded is
false (nothing reached a peer) and ErrAllAppliersTimedOut when something
did. The envelope now carries content_outcome, and content_landed is
ABSENT rather than false when the outcome is unknown, because a caller
that reads false may act on a premise nothing supports.

Three smaller round-1 items. The settle budget's comment now says it
bounds how long the route keeps ASKING, not how long the request takes —
the deadline is only consulted between attempts and PruneAndApply can
block on the per-item lock. A comment on fullWriteHandled still named
applyContentViaCollab, which this unit deleted; my own sweep missed it.
The ported classifier test now inspects the recorder rather than only
the boolean, since a mutant could return true while writing the wrong
status.

Verified and NOT changed: nil-ing content on the row write does not
newly suppress version bracketing. main already set input.Content = nil
on the applier path before its row write, so that behaviour is identical
before and after the reorder.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn

* fix(collab,server): the not-applied claim was still false on two post-wire paths (TASK-2989)

Codex round 2, and it refuted the reasoning I gave in round 1's fix
rather than just finding another case. I said the discriminator already
existed upstream because electAndApply tracks anyWriteSucceeded. It
does — PER ELECTION — and two paths escape it:

  - a restore storm returns ErrNoApplierAvailable after up to
    applierMaxRestartsAfterRestore elections, each of which may have put
    an applier_request on the wire, with the per-election flag discarded
    at every restart;
  - a registerPendingAck failure on a retry attempt returns a raw error
    after an earlier attempt had already sent one.

Both would have answered content_landed:false about content that may
have landed. Same shape as the finding they follow: a reason that was
sufficient-sounding and one file short of true.

Fixed at the source where the source can know it — ApplyExternalContent
now carries sentAny across restarts, so ErrNoApplierAvailable means what
its callers read it to mean — and by construction everywhere else:
classifyApplyOutcome is a whitelist, so only the two sentinels that mean
nothing reached a peer may make the claim and every other error,
including ones nobody has written yet, degrades to unknown.

Cancellation: the re-decision wait is the only new blocking wait this
branch adds, and it now ends when the caller goes away. The rest of the
path was context-blind on main and stays that way; threading a context
into the store and the applier round-trip is a different change.

Not fixed here, deliberately: the ambiguous-commit double-write. Codex
confirmed against main that it has the identical shape there, so it is
pre-existing and gets filed rather than folded into this unit.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn

* docs(collab): sweep the prose my own round-2 fix falsified (TASK-2989)

Codex round 3, one P3, and it is CONVE-23 arriving for the third time in
this unit. Carrying sentAny across the restart loop changed which
sentinel a restore storm returns, and left two comments describing the
old behaviour: the cap's doc still said exhaustion falls back with
ErrNoApplierAvailable, and the sentinel's own doc still said every
attempt timed out.

Both now say what the sentinel MEANS rather than how it usually arises —
bytes reached a peer and the outcome is unknown — because that is the
half two callers depend on: the op-log prune stays suppressed, and the
PATCH handler reports the content outcome as unknown rather than
not-applied.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn

* fix(server): restore the UNIQUE-constraint 409 the applier path used to inherit (TASK-2989)

Codex round 5, and a regression rather than a gap. The ordinary error
block maps a UNIQUE-constraint race — two updates that both pass
checkUniqueFields and then both hit the partial unique index on
invocation_slug — to a 409. Before the reorder the applier path's row
write ran through that block and inherited the mapping. Routing it
through a helper built from 'the four typed refusals' dropped the arm and
turned a benign race into a 500 on that route alone.

The irony is the lesson, and it belongs on the record: writeTypedItemRefusal
exists BECAUSE this handler's refusal set has been under-counted three
times, and I under-counted it again while building the thing meant to
stop that — by taking the population from the errors that have a Go type
rather than from the block that actually answers them.

The new arm's first version panicked on a nil error, since it
dereferences where the typed arms use errors.As. The existing nil
control leg caught it immediately, which is the entire reason that leg
is there.

The structural guard now DERIVES its file set — every non-test file in
the package that calls UpdateItemWithParentLink — instead of listing two
names, so a future block in a third file cannot sit unmapped while the
test passes.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn

* test(server): the guard now requires the fifth arm, scoped to the block's own function (TASK-2989)

Codex round 6. Two gaps in the guard as it stood: it verified only the
four typed arms, so removing the UNIQUE-constraint mapping from either
ordinary block still passed; and its file set matched on the store call
text, so a file reaching the store through a wrapper would not be
scanned at all.

The file set is now the UNION of files calling UpdateItemWithParentLink
and files calling any of the arms — a refusal block lives where the arms
are called, whatever it calls the store through.

The fifth-arm check is scoped to the ENCLOSING FUNCTION, and that is the
part worth reading. The first version asked whether a UNIQUE literal
appeared between one block's start and the next block's start in
token.Pos. Those windows span whole files, so the gap between the last
block of one file and the first block of the next swallowed every
literal in between — two in handlers_items.go belonging to the create
and restore paths, one in handlers_items_bulk.go. All three mutation
controls survived it. It asserted nothing, and it passed, which is the
only reason I looked.

Committed BEFORE the controls run this time. The previous round's
controls used git checkout -- internal/ against uncommitted guard work
and deleted it; the tree read clean afterwards, which is the ambiguity —
clean means the mutation was reverted OR the mutation and my work both
were.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn

* test(server): scope the fifth-arm check to the block statement, not the function (TASK-2989)

Per-function was the second wrong containment and the controls said so:
handleUpdateItem holds TWO refusal blocks with a UNIQUE arm each, so
neutralising either hid behind the other and survived. Only the
writeTypedItemRefusal control was detected — the check covered one of
the three blocks it claimed to cover.

Innermost enclosing BlockStmt is the containment that matches what the
sentence means by 'the block's own arm'.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn

* test(server): the fifth-arm check reads if-conditions, not any literal in the block (TASK-2989)

Codex round 7. Scanning the whole BlockStmt for a matching string
literal let an unrelated nested closure — or a message string quoting
the phrase — satisfy the guard after the real mapping had been deleted.
That is the guard passing for a reason unrelated to what it asserts,
which is the failure this whole check exists to prevent one level down.

It now reads only IfStmt conditions, which is the shape the arm actually
has.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn
This commit is contained in:
xarmian
2026-09-10 08:54:41 -04:00
committed by GitHub
parent ab4607fed1
commit dc70ff3d7f
9 changed files with 1555 additions and 380 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/collab/applier.go
+69 -32
View File
@@ -209,9 +209,20 @@ var (
// direct write.
ErrNoApplierAvailable = errors.New("collab: no live conn available to apply")
// ErrAllAppliersTimedOut — every attempt timed out without an
// ack. Caller falls back to direct write and (depending on
// preference) logs a warn so operators can see degraded sessions.
// ErrAllAppliersTimedOut — an applier_request REACHED a peer and the
// round-trip was never confirmed. The usual cause is what the name says
// (every attempt timed out without an ack); since PLAN-2975 it also covers
// a restore storm that exhausted its re-elections after sending, which is
// not a timeout but has the identical meaning to a caller: bytes went out
// and the outcome is unknown.
//
// That "bytes went out" is the load-bearing half, and two callers depend on
// it rather than on the timeout wording. The op-log prune deliberately does
// NOT fire here (a peer may hold a Y.Doc derived from the log), and the
// items PATCH handler reports the content outcome as UNKNOWN rather than
// not-applied, because the peer may have applied the markdown with its ack
// lost or late. ErrNoApplierAvailable is the sentinel that means nothing
// reached a peer; keep the two distinct when adding a return path.
ErrAllAppliersTimedOut = errors.New("collab: all designated appliers timed out")
// ErrApplierAmbiguous — a legacy (non-bracket-capable) applier round-trip was
@@ -252,8 +263,17 @@ func (m *RoomManager) ApplyExternalContent(itemID string, markdown string) error
return ErrNoActiveRoom
}
// sentAny is tracked ACROSS restarts, not just within one election, because the
// sentinel this function ends on is read by callers as a claim about whether the
// markdown ever reached a peer (PLAN-2975: content_not_applied says so on the
// wire). electAndApply tracks it per election and a superseded election discards
// it, so without this the restore-storm return below would answer "no applier was
// available" — i.e. nothing was sent — after up to applierMaxRestartsAfterRestore
// elections that may each have put an applier_request on the wire.
var sentAny bool
for restart := 0; restart < applierMaxRestartsAfterRestore; restart++ {
err, superseded := m.electAndApply(room, itemID, markdown)
err, superseded, sent := m.electAndApply(room, itemID, markdown)
sentAny = sentAny || sent
if !superseded {
return err
}
@@ -261,7 +281,12 @@ func (m *RoomManager) ApplyExternalContent(itemID string, markdown string) error
// (electAndApply already waited it out). Re-elect from scratch: a fresh
// request_id + tried set against the post-restore room.
}
// Restore storm: fall back to a direct write rather than spin.
// Restore storm: fall back to a direct write rather than spin. Which sentinel is
// not cosmetic — ErrNoApplierAvailable asserts nothing reached a peer, and after
// a storm in which a request DID go out that assertion is false.
if sentAny {
return ErrAllAppliersTimedOut
}
return ErrNoApplierAvailable
}
@@ -291,16 +316,16 @@ func (m *RoomManager) ApplyExternalContent(itemID string, markdown string) error
// decision 2), which is why every judgement call here resolves toward true.
//
// That asymmetry is a statement about THIS function's answer, not a guarantee about
// everything downstream of it, and the difference matters. A caller that reorders on
// a true and then meets an apply failure inherits whatever its fallback does — and
// the fallback that exists today can itself write content past live peers: when
// applyContentViaCollab exhausts its ErrRoomActiveDuringPrune retries, the PATCH
// handler falls through to a plain direct write while a live writer may be holding a
// Y.Doc that will outvote it on the next flush (internal/server/handlers_collab.go's
// retry cap, consumed by handleUpdateItem's "any other error path" fall-through).
// That predates this function and is unchanged by it; it is named here because a
// reader would otherwise take "a false positive costs a partial answer" as a claim
// about the whole path rather than about this return value (codex round 2, PLAN-2975).
// everything downstream of it. When this comment was written the caller's fallback
// could itself write content past live peers — a three-try give-up that fell through
// to a plain direct write while a live writer held a Y.Doc that would outvote it on
// the next flush — so the sentence above was true of this return value and false of
// the path. PLAN-2975 unit 2 removed that fallback: an unsettled room now answers a
// retryable room_settling refusal, and an apply that fails after the row write
// answers content_not_applied rather than a success the content never reached. The
// distinction is kept because it is still the right way to read this function: a
// false positive costs the CALLER an honest partial answer, and what the caller does
// with that is the caller's contract, not this one's.
//
// That is why an in-progress version restore answers TRUE rather than consulting the
// conns. ForceRefreshRoom freezes every conn for the duration, and pickApplier skips
@@ -342,11 +367,16 @@ func (m *RoomManager) HasElectableApplier(itemID string) bool {
}
// electAndApply runs one designated-applier election (first attempt + one retry)
// against the room, gated on the restore coordinator. It returns (err, superseded):
// superseded==true means a version restore interrupted the election and has resolved,
// so ApplyExternalContent should restart; err is meaningless in that case. When
// superseded==false, err is the final outcome (nil on success, or a sentinel).
func (m *RoomManager) electAndApply(room *Room, itemID, markdown string) (error, bool) {
// against the room, gated on the restore coordinator. It returns
// (err, superseded, sentAny): superseded==true means a version restore interrupted
// the election and has resolved, so ApplyExternalContent should restart; err is
// meaningless in that case. When superseded==false, err is the final outcome (nil on
// success, or a sentinel).
//
// sentAny reports whether an applier_request actually reached a peer during THIS
// election. The caller accumulates it across restarts so the sentinel it finally
// returns does not claim nothing was sent when something was (PLAN-2975).
func (m *RoomManager) electAndApply(room *Room, itemID, markdown string) (error, bool, bool) {
// A FRESH request_id per election: a late ack from a superseded prior election
// (same conn, re-picked after a rollback) can't be mistaken for this one's.
requestID := uuid.NewString()
@@ -370,7 +400,7 @@ func (m *RoomManager) electAndApply(room *Room, itemID, markdown string) (error,
// !waited return ADMITS us; finishAdmission MUST run before we wait on the
// outcome so a concurrent beginRestore drains us (P1).
if room.enterApplierGate() {
return nil, true
return nil, true, anyWriteSucceeded
}
applier := room.pickApplier(tried)
@@ -378,9 +408,9 @@ func (m *RoomManager) electAndApply(room *Room, itemID, markdown string) (error,
// No more candidates left.
room.finishAdmission()
if !anyWriteSucceeded {
return ErrNoApplierAvailable, false
return ErrNoApplierAvailable, false, anyWriteSucceeded
}
return ErrAllAppliersTimedOut, false
return ErrAllAppliersTimedOut, false, anyWriteSucceeded
}
tried[applier.conn] = struct{}{}
@@ -390,7 +420,7 @@ func (m *RoomManager) electAndApply(room *Room, itemID, markdown string) (error,
room.finishAdmission()
if registerErr != nil {
// Race: room closed between pickApplier and registration.
return registerErr, false
return registerErr, false, anyWriteSucceeded
}
// Send the applier_request as a TextMessage. y-protocol
@@ -412,7 +442,7 @@ func (m *RoomManager) electAndApply(room *Room, itemID, markdown string) (error,
payload, err := json.Marshal(msg)
if err != nil {
room.cancelPendingAck(requestID)
return fmt.Errorf("marshal applier_request: %w", err), false
return fmt.Errorf("marshal applier_request: %w", err), false, anyWriteSucceeded
}
// Bounded write (BUG-2276 residual 2, P1a): a dead/slow peer wedged in a
// deadline-free writeLoop write holds writeMu, which would otherwise block
@@ -447,7 +477,7 @@ func (m *RoomManager) electAndApply(room *Room, itemID, markdown string) (error,
case applierPersisted:
// The applier's setContent durably landed (no frame in its apply
// bracket was frozen-dropped). All peers are on the new state.
return nil, false
return nil, false, anyWriteSucceeded
case applierAmbiguous:
// Legacy conn caught by a restore: setContent MIGHT have landed but we
// can't confirm. FAIL-SAFE — do NOT re-apply (that could clobber peer
@@ -460,7 +490,7 @@ func (m *RoomManager) electAndApply(room *Room, itemID, markdown string) (error,
"item_id", itemID,
"client_id", applier.id,
)
return ErrApplierAmbiguous, false
return ErrApplierAmbiguous, false, anyWriteSucceeded
default: // applierNotPersisted
// A version restore froze this apply, so setContent did NOT land. This
// is a DURABLE determination (see restore_coord.go), not a timing guess
@@ -472,7 +502,7 @@ func (m *RoomManager) electAndApply(room *Room, itemID, markdown string) (error,
"item_id", itemID,
"client_id", applier.id,
)
return nil, true
return nil, true, anyWriteSucceeded
}
case <-time.After(timeouts[attempt]):
room.cancelPendingAck(requestID)
@@ -492,15 +522,22 @@ func (m *RoomManager) electAndApply(room *Room, itemID, markdown string) (error,
if !anyWriteSucceeded {
// applierMaxAttempts exhausted without ever putting bytes on
// the wire. Same recovery profile as no-applier-found.
return ErrNoApplierAvailable, false
return ErrNoApplierAvailable, false, anyWriteSucceeded
}
return ErrAllAppliersTimedOut, false
return ErrAllAppliersTimedOut, false, anyWriteSucceeded
}
// applierMaxRestartsAfterRestore caps how many times ApplyExternalContent re-elects
// after being superseded by a restore, so a pathological back-to-back restore storm
// can't spin the election forever. On exhaustion the caller falls back to a direct
// write (ErrNoApplierAvailable) — safe graceful degradation.
// can't spin the election forever.
//
// On exhaustion the sentinel depends on whether any election put an applier_request
// on the wire (PLAN-2975). Nothing sent → ErrNoApplierAvailable, and the caller's
// direct write with an op-log prune is safe graceful degradation. Something sent →
// ErrAllAppliersTimedOut, which suppresses the prune and reports the content outcome
// as unknown: a peer may hold a Y.Doc built from the log, and may have applied the
// markdown. Answering the no-applier sentinel there would assert that nothing reached
// a peer, which is exactly the false claim this distinction exists to prevent.
const applierMaxRestartsAfterRestore = 5
// applierWriteDeadlineVar bounds the applier_request write so a dead/slow peer
internal/collab/applier_testhooks.go
+25
View File
@@ -0,0 +1,25 @@
package collab
import "time"
// SetApplierTimeoutsForTesting shrinks the designated-applier round-trip's
// first-attempt and retry budgets, returning a function that restores them.
//
// The budgets are 30s and 15s in production, which is correct there and makes any
// cross-package test of an apply FAILURE take three quarters of a minute. Package
// collab's own tests have shrunk these vars directly since TASK-1257; this exports
// the same lever for internal/server, which needs it to drive the content_not_applied
// answer (PLAN-2975) through the real HTTP handler rather than asserting it from a
// unit test that never touches the route.
//
// Test-only by contract, not by build tag: it is called from _test.go files, and the
// restore function is what keeps a shrunk budget from leaking into a sibling test in
// the same binary. NOT SAFE to call while an applier round-trip is in flight — the
// vars are read without synchronisation, exactly as they were before this existed.
func SetApplierTimeoutsForTesting(first, retry time.Duration) func() {
origFirst, origRetry := applierFirstTimeoutVar, applierRetryTimeoutVar
applierFirstTimeoutVar, applierRetryTimeoutVar = first, retry
return func() {
applierFirstTimeoutVar, applierRetryTimeoutVar = origFirst, origRetry
}
}
internal/server/handlers_collab.go
+12 -212
View File
@@ -1,7 +1,6 @@
package server
import (
"database/sql"
"errors"
"log/slog"
"math/rand"
@@ -11,7 +10,6 @@ import (
"github.com/PerpetualSoftware/pad/internal/collab"
"github.com/PerpetualSoftware/pad/internal/models"
"github.com/PerpetualSoftware/pad/internal/store"
"github.com/go-chi/chi/v5"
"github.com/gorilla/websocket"
)
@@ -407,217 +405,19 @@ func isAccessDenial(err error) bool {
return errors.As(err, &sErr)
}
// applyContentViaCollab routes an external content update through a
// connected browser tab via the collab room manager's
// designated-applier protocol. Returns nil when an applier acked
// successfully (caller should suppress the direct items.content
// write); any error means "fall back to direct write".
// The write-first-apply-second router in handlers_items_content_route.go replaced the
// helper chain that used to live here (applyContentViaCollab / applyContentViaCollabOnce
// / directWriteFn / applyContentMaxRetries / isDeterministicWriteFailure), removed in
// PLAN-2975 unit 2.
//
// Errors are categorised + logged at the right level so operators
// can see when degraded paths fire. Returning the error rather than
// silently swallowing lets callers add their own telemetry / metrics
// later without re-deriving the categorisation.
//
// Retries internally when the no-room classification races a fresh
// Join (PruneAndApply returns ErrRoomActiveDuringPrune): the room
// is now active, so we re-call ApplyExternalContent against the
// live peer. Capped at applyContentMaxRetries to prevent runaway
// loops if joins keep landing during the prune attempts.
const applyContentMaxRetries = 3
// directWriteFn is the caller's items.content writer. Invoked only
// on the no-room/no-applier paths, INSIDE the per-item setup lock
// (so a fresh Join cannot replay stale op-log between prune and
// content write).
//
// It receives the op-log prune as a hook to run INSIDE its own write
// transaction rather than performing the prune itself, so the two
// move together or not at all (BUG-2840 half B). The caller must run
// the hook when it is non-nil; a write that commits without it leaves
// a stale op-log that a later Join would replay over the content just
// written, which is the hazard the prune exists to prevent.
type directWriteFn func(pruneOpLog func(tx *sql.Tx) error) error
func (s *Server) applyContentViaCollab(r *http.Request, itemID, markdown string, directWrite directWriteFn) error {
if s.collab == nil {
return errors.New("collab not configured")
}
for attempt := 0; attempt < applyContentMaxRetries; attempt++ {
err := s.applyContentViaCollabOnce(r, itemID, markdown, directWrite)
if !errors.Is(err, collab.ErrRoomActiveDuringPrune) {
return err
}
// A fresh Join slipped in during PruneAndApply's check.
// Loop and re-try ApplyExternalContent against the now-
// active room rather than direct-writing past the live
// peer (whose Y.Doc would otherwise outvote the direct
// write on next flush). Per Codex review round 6.
}
slog.Warn("collab: exhausted prune retries; falling back to direct write",
"item_id", itemID,
)
return collab.ErrRoomActiveDuringPrune
}
// isDeterministicWriteFailure reports whether an error from a direct-write
// callback is a settled answer rather than a transient condition.
//
// These are the errors handleUpdateItem already treats as FINAL at every call
// site: a rejection, a conflict, and two refusals. None can come out
// differently on a retry, so a fallback path that swallows one and retries is
// doing the work twice to reach the same answer — and, worse, may reach it by
// a route that reports it differently.
//
// THIS LIST IS A CLOSED SET THAT KEEPS GETTING REOPENED. It said "these three"
// until BUG-2833 added a fourth store-level refusal on the same write path, and
// nothing failed when the new error was omitted — the request still reached an
// answer, just twice and by the other route. Anyone adding a typed, permanent
// refusal to store.UpdateItem owes this function an entry and the sentence
// above a recount.
func isDeterministicWriteFailure(err error) bool {
if err == nil {
return false
}
if _, ok := asOpenChildrenGuardError(err); ok {
return true
}
if _, ok := asUpdateConflictError(err); ok {
return true
}
var badTitle *store.InvalidItemTitleError
if errors.As(err, &badTitle) {
return true
}
var tooLarge *store.ItemRenameCascadeTooLargeError
return errors.As(err, &tooLarge)
}
func (s *Server) applyContentViaCollabOnce(r *http.Request, itemID, markdown string, directWrite directWriteFn) error {
err := s.collab.ApplyExternalContent(itemID, markdown)
switch {
case err == nil:
// Caller suppresses the direct write.
return nil
case errors.Is(err, collab.ErrNoActiveRoom),
errors.Is(err, collab.ErrNoApplierAvailable):
// No live editors — direct write is the right thing. We
// also prune the op-log here: any prior collab state is
// strictly older than the items.content the caller is
// about to write, and replaying it on the next collab
// session would resurrect stale content and silently
// overwrite this update on the next 5s flush. Common
// triggers for this path are (a) CLI/MCP/API updates
// outside any co-edit session, (b) raw-mode toggles that
// destroy the in-tab provider before saving, and (c) raw
// saves that hit the server while the room is still in
// its 60s grace TTL with zero conns (returns
// ErrNoApplierAvailable).
//
// PruneAndApply runs the prune under the per-item setup
// lock so a fresh Join racing in this exact window can't
// load the soon-to-be-pruned op-log under our feet.
// Pruning is safe in both no-conn cases — there are no
// peers in memory whose Y.Doc would diverge.
// ErrAllAppliersTimedOut is intentionally NOT pruned
// because peers may still be alive there.
paErr := s.collab.PruneAndApply(itemID, func() error {
// The prune runs INSIDE the write's own transaction, via
// the hook, so a refused or failed write rolls it back
// (BUG-2840 half B). It used to run first, in its own
// statement: the justification for pruning is that "any
// prior collab state is strictly older than the
// items.content the caller is about to write", and that
// premise is FALSE on every path where the write then
// refuses — the ops were destroyed and nothing replaced
// them. That is not hypothetical on this branch: it fires
// on ErrNoApplierAvailable, i.e. a room inside its grace
// TTL with zero connections, which is exactly the state
// where the op-log holds a closed tab's edits that never
// reached items.content.
//
// Same shape, and the same reasoning, as PruneItemOpLogTx's
// use by version-restore, whose comment already says a
// split prune/commit "leaves a divergent state on any
// failure" in EITHER order. This path was the remaining
// split.
//
// Still under the per-item lock, so a concurrent Join
// cannot slip between prune and write, replay an empty
// op-log, then overwrite the fresh write from its stale
// Y.Doc state (Codex review round 8).
return directWrite(func(tx *sql.Tx) error {
return s.store.PruneItemOpLogTx(tx, itemID)
})
})
switch {
case paErr == nil:
// Prune + direct write completed atomically.
// Caller suppresses any subsequent items.content write.
return nil
case errors.Is(paErr, collab.ErrRoomActiveDuringPrune):
// A peer joined between ApplyExternalContent's check
// and PruneAndApply's re-check. Surface the error so
// the outer retry loop in applyContentViaCollab
// re-routes through the now-active applier — direct-
// writing past a live peer would let its Y.Doc
// outvote our update on the next flush. Per Codex
// review round 6.
return paErr
default:
// A DETERMINISTIC failure from directWrite is the caller's answer,
// not a collab routing problem, so it must survive this branch
// (BUG-2804 / codex R2). Returning `err` here discards it and hands
// the caller the original collab error instead, which reads as
// "couldn't route through an applier" — recoverable — so the caller
// falls through to its own direct write and re-derives the identical
// refusal from scratch. Measured: a refused rename ran the whole
// cascade TWICE, 64 rewritten bodies built for one request.
//
// Scoped to errors that cannot come out differently on a second
// attempt. Everything else keeps returning `err`, preserving the
// graceful-degradation contract this branch exists for: a prune
// failure or a transient write fault should still fall through.
if isDeterministicWriteFailure(paErr) {
return paErr
}
slog.Warn("collab: failed to prune op-log on direct-write fallback",
"item_id", itemID,
"error", paErr,
)
return err
}
case errors.Is(err, collab.ErrAllAppliersTimedOut):
slog.Warn("collab: all designated appliers timed out; falling back to direct items.content write",
"item_id", itemID,
"actor", actorIDFromRequest(r),
)
return err
default:
slog.Warn("collab: applier path failed; falling back to direct items.content write",
"item_id", itemID,
"error", err,
)
return err
}
}
// actorIDFromRequest returns a non-empty identity string for the
// caller when one is available — user id, token id, or empty. Used
// in slog calls where we want SOMETHING actor-shaped without
// caring about the precise auth path.
func actorIDFromRequest(r *http.Request) string {
if u := currentUser(r); u != nil {
return u.ID
}
if tw := tokenWorkspaceID(r); tw != "" {
return "token-ws:" + tw
}
return ""
}
// It is worth saying WHY rather than leaving a gap: that chain retried
// ErrRoomActiveDuringPrune internally and re-called ApplyExternalContent, which could
// succeed through a freshly joined applier and return nil — after which the handler's
// row write ran last, which is BUG-2840 half A. The retry budget now sits in
// settleContentRoute, above the write, where a re-decision cannot leave content in the
// document ahead of a refusal. isDeterministicWriteFailure's job — classifying the
// typed, permanent refusals — is writeTypedItemRefusal's now, and it carries that
// function's closed-set warning with it.
// statusError lets authorizeCollabAccess return a typed error that
// carries the HTTP status + payload pieces handleCollab should write.
internal/server/handlers_collab_applier_refusal_test.go
+198
View File
@@ -0,0 +1,198 @@
package server
import (
"encoding/json"
"net/http"
"net/http/httptest"
"testing"
"time"
"github.com/PerpetualSoftware/pad/internal/collab"
"github.com/gorilla/websocket"
)
// BUG-2840 half A: this file MEASURED the defect, and now asserts the property that
// replaced it (PLAN-2975 unit 2).
//
// The filing claims that on the APPLIER path a refused PATCH still lands its
// content — applyContentViaCollab pushes the markdown into the live Y.Doc,
// the handler clears input.Content, the row write then refuses, and the
// content reaches items.content anyway on the next collab-snapshot flush.
//
// That claim was a READING of the snapshot branch, not an observation, and the
// plan says so: "Both need measuring before a fix is designed — the shape of
// the fix depends on which half actually bites." So this file measures. It
// asserts what the code DOES today, including the part that is a defect, so
// that the fix has a baseline to move and a reviewer can see the premise was
// established rather than assumed.
// applierEcho answers applier_request frames with an ack, which is what makes
// ApplyExternalContent succeed and puts the handler on the applier path. Ported
// from internal/collab's own test helper; that one cannot be reused here
// because reaching the defect requires going through the HTTP handler.
func applierEcho(t *testing.T, conn *websocket.Conn) func() {
t.Helper()
done := make(chan struct{})
go func() {
defer close(done)
for {
mt, data, err := conn.ReadMessage()
if err != nil {
return
}
if mt != websocket.TextMessage {
continue
}
var ctl collab.ControlMessage
if err := json.Unmarshal(data, &ctl); err != nil {
continue
}
if ctl.Type != collab.ControlMessageApplierRequest {
continue
}
// A real applier is a browser tab: it applies the markdown to
// its Y.Doc and BROADCASTS the resulting update, which the relay
// persists to the op-log. The ack alone would make
// ApplyExternalContent succeed while leaving no durable trace, so
// this emits an op too — otherwise the experiment would measure a
// peer that does not exist.
if err := conn.WriteMessage(websocket.BinaryMessage, []byte{0x00, 0x01, 0x02}); err != nil {
return
}
payload, _ := json.Marshal(collab.ControlMessage{
Type: collab.ControlMessageApplierAck,
RequestID: ctl.RequestID,
})
if err := conn.WriteMessage(websocket.TextMessage, payload); err != nil {
return
}
}
}()
return func() { _ = conn.Close(); <-done }
}
// waitForApplierPath blocks until a content PATCH is actually taking the
// applier path, and returns once it is.
//
// The readiness signal is the observable difference between the two paths
// rather than an internal field: on the APPLIER path the markdown goes to the
// Y.Doc and items.content is left alone, while on the direct-write path
// items.content changes. So a SUCCEEDING probe PATCH that leaves items.content
// untouched is proof the applier answered — no exported accessor for
// "electable conns" exists, and reaching into the manager's unexported state
// is not available from this package.
func waitForApplierPath(t *testing.T, srv *Server, wsSlug, itemSlug, itemID string) {
t.Helper()
deadline := time.Now().Add(3 * time.Second)
for attempt := 0; time.Now().Before(deadline); attempt++ {
probe := "applier-path probe"
rr := doRequest(srv, "PATCH", "/api/v1/workspaces/"+wsSlug+"/items/"+itemSlug,
map[string]interface{}{"content": probe})
if rr.Code != http.StatusOK {
t.Fatalf("probe PATCH failed: %d %s", rr.Code, rr.Body.String())
}
item, err := srv.store.GetItem(itemID)
if err != nil {
t.Fatalf("GetItem: %v", err)
}
if item.Content != probe {
return // content did not land in the row: the applier took it
}
time.Sleep(5 * time.Millisecond)
}
t.Fatal("no applier path within 3s: every probe PATCH wrote items.content directly")
}
// TestBUG2840HalfA_RefusedPatchLeavesTheDocumentUntouched asserts the property the
// reorder buys: a refused PATCH on the applier path changes NOTHING — not the row,
// and not the collaborative document.
//
// Until PLAN-2975 unit 2 this same test asserted the DEFECT (that the refused request
// left op-log rows behind) and skipped if it could not reproduce it. The inversion is
// deliberate and is the only honest way to reuse it: leaving the old assertion in
// place would have turned the fix into a SKIP, and a skip reads as a pass in the
// summary line.
func TestBUG2840HalfA_RefusedPatchLeavesTheDocumentUntouched(t *testing.T) {
srv := testServerWithCollab(t)
ts := httptest.NewServer(srv)
t.Cleanup(ts.Close)
slug := createWSWithCollections(t, srv)
item := createTaskWithFields(t, srv, slug, "Item", `{"status":"open"}`)
conn, resp, err := dialCollab(t, ts.URL, item.ID, nil, "")
if err != nil {
status := ""
if resp != nil {
status = resp.Status
}
t.Fatalf("dialCollab: %v (%s)", err, status)
}
stop := applierEcho(t, conn)
t.Cleanup(stop)
waitForApplierPath(t, srv, slug, item.Slug, item.ID)
before, err := srv.store.GetItem(item.ID)
if err != nil {
t.Fatalf("GetItem: %v", err)
}
beforeOps := countOpLog(t, srv, item.ID)
const refused = "content the caller was told was not written"
rr := doRequest(srv, "PATCH", "/api/v1/workspaces/"+slug+"/items/"+item.Slug,
map[string]interface{}{
"expected_updated_at": "2000-01-01T00:00:00Z",
"content": refused,
})
if rr.Code != http.StatusConflict {
t.Fatalf("expected 409 from the stale token, got %d: %s", rr.Code, rr.Body.String())
}
after, err := srv.store.GetItem(item.ID)
if err != nil {
t.Fatalf("GetItem: %v", err)
}
if after.Content != before.Content {
t.Errorf("items.content moved on a refused PATCH: %q -> %q", before.Content, after.Content)
}
// WHAT THIS CAN AND CANNOT MEASURE, stated because the first version of
// this test got it wrong in a way that looked right.
//
// The plan's step 4 was to drive a ?source=collab-snapshot PATCH and check
// that items.content ends up holding the refused content. Written
// literally, that is CIRCULAR: the snapshot PATCH carries its markdown in
// the request body, so a test that supplies the refused string proves only
// that a snapshot write writes what it is given. The first run of this
// test did exactly that and reported the premise confirmed.
//
// The server cannot close the loop itself. Collab here is a DUMB RELAY —
// it persists opaque Yjs updates and never parses them — so nothing
// server-side can derive markdown from the room's document. The markdown
// in a real snapshot PATCH comes from a live TAB's Y.Doc.
//
// What IS observable, and what the defect reduces to on this side of the
// wire: a refused PATCH leaves DURABLE COLLAB STATE created by that same
// refused request. Any tab that joins afterwards replays it.
afterOps := countOpLog(t, srv, item.ID)
t.Logf("op-log rows before refusal=%d, after refusal=%d; items.content before=%q after=%q",
beforeOps, afterOps, before.Content, after.Content)
// THE PROPERTY. Before the reorder the refused request pushed its markdown into
// the live Y.Doc first, and the relay persisted the resulting update — so the
// refusal left DURABLE collab state that any later joiner replays and flushes
// back into items.content. The caller's 409 was true of the row and false of the
// document.
//
// Now the row write runs FIRST and refuses before ApplyExternalContent is ever
// called, so there is nothing to persist. Op-log rows unchanged is what "the
// document did not move" reduces to on this side of the wire: the server cannot
// read the document itself, because collab here is a dumb relay that never parses
// the opaque Yjs updates it stores.
if afterOps != beforeOps {
t.Errorf("a refused PATCH left %d new op-log row(s) behind (%d -> %d). The refusal is true of "+
"the row and false of the collaborative document: a tab joining later replays those ops "+
"and flushes them back as items.content, landing the content the caller was told was "+
"rejected.", afterOps-beforeOps, beforeOps, afterOps)
}
}
internal/server/handlers_items.go
+43 -102
View File
@@ -14,7 +14,6 @@ import (
"github.com/go-chi/chi/v5"
"github.com/PerpetualSoftware/pad/internal/collab"
"github.com/PerpetualSoftware/pad/internal/events"
"github.com/PerpetualSoftware/pad/internal/items"
"github.com/PerpetualSoftware/pad/internal/models"
@@ -1645,12 +1644,15 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) {
//
// Field-only PATCHes (input.Content == nil) skip this branch
// entirely; they continue straight to UpdateItem unchanged.
// fullWriteHandled is set when applyContentViaCollab's directWrite
// callback ran the FULL UpdateItem (content + title + fields +
// everything) inside the per-item lock. In that case we must not
// re-run UpdateItem below — we'd duplicate the write and could
// produce two version-history rows. Instead we re-fetch the
// post-write snapshot for the response. Per Codex review round 9.
// fullWriteHandled is set when routeContentUpdate already ran the FULL
// UpdateItem — either through the direct path's PruneAndApply callback (content
// + title + fields + everything, inside the per-item lock) or through the
// applier path's row write. In either case we must not re-run UpdateItem below:
// we would duplicate the write and could produce two version-history rows.
// Instead the router hands back the post-write snapshot for the response.
// (Originally Codex round 9 on applyContentViaCollab's directWrite callback;
// that helper was retired with PLAN-2975's reorder and the invariant moved
// rather than went away.)
var fullWriteHandled bool
var fullWriteUpdated *models.Item
// `?source=collab-snapshot` opts out of the applier-routing path so
@@ -1880,105 +1882,44 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) {
}
if input.Content != nil && s.collab != nil && !collabSnapshot {
// applyContentViaCollab calls directWrite ONLY on the no-
// room/no-applier paths (where pruning the op-log is safe
// and we need to land items.content under the per-item
// lock). The callback owns the full UpdateItem so a mixed
// content + title + fields PATCH stays atomic — Round 8's
// content-only split lost atomicity and broke
// Store.UpdateItem's content-versioning peek at Title.
// Per Codex review round 9.
err := s.applyContentViaCollab(r, item.ID, *input.Content, func(pruneOpLog func(*sql.Tx) error) error {
// The op-log prune rides INSIDE this write's transaction
// (BUG-2840 half B): composed onto the precheck hook, which
// UpdateItemWithParentLink runs inside the tx, so a refusal
// from the guard or from the update itself rolls the prune
// back. Before this the prune was a separate statement that
// ran FIRST, and a refused write left the op-log emptied with
// nothing written to supersede it.
precheck := openChildrenPrecheck
if pruneOpLog != nil {
inner := precheck
precheck = func(tx *sql.Tx, existing *models.Item) error {
// Guard first — a preference, not an enforced invariant,
// and mutation-checked as such: swapping these two
// survives the suite because both run in ONE transaction,
// so a refusal rolls the prune back either way. The order
// buys two smaller things: the DELETE is not done for a
// write that is about to refuse, and an error from the
// prune cannot mask the guard's refusal as the caller's
// answer. Neither is observable without a failing prune,
// so this comment claims a preference rather than a rule
// nothing enforces.
if inner != nil {
if err := inner(tx, existing); err != nil {
return err
}
}
return pruneOpLog(tx)
}
}
updated, uerr := s.store.UpdateItemWithParentLink(item.ID, input, precheck, parentLink)
if uerr != nil {
return uerr
}
// PLAN-2975: WRITE FIRST, APPLY SECOND on the applier path.
//
// Before this, the content half was pushed into the live Y.Doc by
// ApplyExternalContent and the row write ran afterwards, so any of the
// four typed refusals below answered 4xx while the content was already
// in the collaborative document and would reach items.content on the
// next ?source=collab-snapshot flush (BUG-2840 half A). A refused PATCH
// must not change the item.
//
// The ordering is only possible because the handler can now learn which
// path it is on WITHOUT applying (TASK-2987's HasElectableApplier);
// previously the only way to discover the applier path was to take it.
contentToApply := *input.Content
route, updated, routeErr := s.routeContentUpdate(w, r, item, &input, openChildrenPrecheck, parentLink, contentToApply)
switch route {
case contentRouteHandled:
// The refusal or the settling answer has already been written.
return
case contentRouteApplierWrote:
// The row write committed and the apply succeeded: content is in the
// document and reaches items.content on the next flush.
fullWriteHandled = true
fullWriteUpdated = updated
return nil
})
if err == nil {
// Either the applier path acked (content propagated via
// Y.Doc; UpdateItem still needs to run for other fields)
// or directWrite ran the full UpdateItem inside the
// lock (fullWriteHandled tracks which).
input.Content = nil
} else if details, ok := asOpenChildrenGuardError(err); ok {
// IDEA-1494 R2: the open-children guard fired inside the
// directWrite callback. Don't let applyContentViaCollab's
// "any error falls through" policy retry the write —
// rejection is final.
writeOpenChildrenError(w, itemRefOrSlug(*item), details)
return
} else if conflict, ok := asUpdateConflictError(err); ok {
// TASK-2022: optimistic-concurrency conflict from inside the
// directWrite callback is also final — a retry would just lose
// the same race again.
writeUpdateConflictError(w, itemRefOrSlug(*item), conflict)
return
} else if writeItemRenameCascadeTooLarge(w, err) {
// FINAL, like the guard and conflict arms above. The refusal is
// deterministic, so the fall-through to the direct write below would
// re-run the entire cascade — every linking body read and projected a
// second time — and refuse identically (codex R2).
return
} else if writeInvalidItemTitle(w, err) {
// FINAL for the same reason, and the pair with
// isDeterministicWriteFailure: that function is what stops
// applyContentViaCollab swallowing this error, and THIS arm is what
// consumes it once it arrives. Without both, a refused title falls
// through to the direct write below and is re-derived from scratch.
//
// BUG-2833 / codex R2. The comment on writeItemRenameCascadeTooLarge
// says this handler has THREE error blocks and that mapping only the
// plain one is a population error rather than a typo. I mapped two,
// then a reviewer named a third — and this is the fourth unit to make
// the identical mistake in the identical function. CONVE-18: the
// instance a reviewer names is a sample; grep the class.
return
} else if errors.Is(err, collab.ErrApplierAmbiguous) {
// BUG-2276 residual 2 (P1, mixed-deploy window): a legacy (non-bracket-
// capable) applier was caught by a concurrent version restore and its
// outcome can't be confirmed. FAIL-SAFE — do NOT fall through to a direct
// write (which could clobber the live doc / lose the write); return a
// retryable 409 so the external caller retries once clients converge.
writeError(w, http.StatusConflict, "applier_ambiguous",
"A concurrent version restore made this edit's outcome ambiguous; please retry.")
return
case contentRouteDirectWrote:
// PruneAndApply ran the full write (content included) under the
// per-item lock, for a room with no live writer at all.
fullWriteHandled = true
fullWriteUpdated = updated
input.Content = nil
case contentRouteFallThrough:
// A transient, non-deterministic failure that is NOT a lost-write
// hazard: no live writer holds a diverging Y.Doc, so the ordinary
// row write below still carries the content. routeErr is logged by
// the router.
_ = routeErr
}
// Any other error path (e.g. ErrAllAppliersTimedOut, retry
// exhaustion) falls through to direct write — graceful
// degradation. The helper logs the specifics so operators
// see degraded paths.
}
var updated *models.Item
internal/server/handlers_items_content_route.go
+407
View File
@@ -0,0 +1,407 @@
package server
import (
"context"
"database/sql"
"errors"
"log/slog"
"net/http"
"sort"
"strings"
"time"
"github.com/PerpetualSoftware/pad/internal/collab"
"github.com/PerpetualSoftware/pad/internal/models"
"github.com/PerpetualSoftware/pad/internal/store"
)
// Write-first-apply-second routing for a content PATCH (PLAN-2975, BUG-2840 half A).
//
// The defect this closes: the applier path used to push content into the live Y.Doc
// BEFORE the row write, so a refused write answered 4xx while the collaborative
// document had already moved. The refusal was true of the row and false of the
// document, and the next ?source=collab-snapshot flush carried the refused content
// into items.content.
//
// The ordering is possible because HasElectableApplier (TASK-2987) answers "would
// this route through an applier?" without applying. It is a HINT — an apply can
// still fail after a yes — which is why contentRouteHandled exists: an apply that
// fails after the row write has committed answers the typed content_not_applied 409
// rather than a 200 that would read as success.
type contentRoute int
const (
// contentRouteHandled — a response has already been written (a typed refusal,
// content_not_applied, or room_settling). The caller must return immediately.
contentRouteHandled contentRoute = iota
// contentRouteApplierWrote — the row write committed and the apply succeeded.
contentRouteApplierWrote
// contentRouteDirectWrote — PruneAndApply ran the full write for a room with no
// live writer.
contentRouteDirectWrote
// contentRouteFallThrough — nothing was written and no live writer holds a
// diverging document, so the caller's ordinary row write should carry the
// content as it always did.
contentRouteFallThrough
)
// applierSettleBudget bounds the wait for a room that is neither settled enough to
// elect an applier nor empty enough to write directly.
//
// RECEIPT. Measured on this deployment's shape (TASK-2989): dial the collab socket
// through the real HTTP/WS handler chain against the real SQLite store, then poll
// HasElectableApplier until it answers true. n=5 per bucket, loopback:
//
// rows=0 mean 3.23ms max 5.60ms
// rows=10 mean 2.45ms max 3.08ms
// rows=100 mean 2.85ms max 3.81ms
// rows=1000 mean 6.33ms max 8.51ms
// rows=5000 mean 38.75ms max 46.41ms
//
// Flat at ~3ms up to 100 op-log rows (fixed handshake cost), then roughly linear at
// about 7.7µs/row. Worst single observation across all 25 runs: 46.41ms. The budget
// is ~10x that, and the headroom is spent deliberately on what the measurement does
// NOT cover: Postgres rather than SQLite, a real browser across a real network, and
// a store under concurrent load.
//
// It does NOT cover a conn that has not dialled yet, or one wedged behind a stalled
// write — those are meant to reach the room_settling refusal rather than be waited
// out. Error direction is safe in both senses: too long merely delays a request that
// was going to be refused, and too short converts a room that would have settled
// into a retryable refusal. Nothing is written on either side of the bound.
//
// IT IS A RE-DECISION BUDGET, NOT A HARD REQUEST BOUND, and the difference is worth
// stating because the name suggests otherwise (codex round 1, this unit). The
// deadline is only consulted between attempts, and an attempt calls PruneAndApply,
// which can itself block on the per-item lock or on appendMu while a restore holds
// the room. A request can therefore exceed this budget under contention. What the
// budget bounds is how long the route keeps ASKING, not how long the request takes.
const applierSettleBudget = 500 * time.Millisecond
// applierSettlePoll is the re-decision interval inside that budget. HasElectableApplier
// is two uncontended mutex acquisitions, so polling costs nothing measurable; 10ms
// keeps the common case (a conn that anchors in ~3ms) from paying a full extra tick.
const applierSettlePoll = 10 * time.Millisecond
// routeContentUpdate decides which ordering a content PATCH takes and executes it.
//
// It owns the re-decision deliberately. The predecessor helper retried
// ErrRoomActiveDuringPrune INSIDE applyContentViaCollab and re-called
// ApplyExternalContent, which could succeed through a freshly-joined applier and
// return nil — after which the handler's row write still ran last, reproducing the
// very defect this change removes. Re-deciding HERE, before anything is written, is
// what makes that impossible rather than unlikely.
func (s *Server) routeContentUpdate(
w http.ResponseWriter,
r *http.Request,
item *models.Item,
input *models.ItemUpdate,
openChildrenPrecheck func(*sql.Tx, *models.Item) error,
parentLink *store.ParentLinkUpdate,
content string,
) (contentRoute, *models.Item, error) {
var updated *models.Item
outcome, paErr := settleContentRoute(
r.Context(),
func() bool { return s.collab.HasElectableApplier(item.ID) },
func() error {
return s.collab.PruneAndApply(item.ID, func() error {
precheck := composePruneWithPrecheck(s, item.ID, openChildrenPrecheck)
u, uerr := s.store.UpdateItemWithParentLink(item.ID, *input, precheck, parentLink)
if uerr != nil {
return uerr
}
updated = u
return nil
})
},
applierSettleBudget, applierSettlePoll,
)
switch outcome {
case settleElectApplier:
return s.applierFirstWrite(w, item, input, openChildrenPrecheck, parentLink, content)
case settleDirectWrote:
return contentRouteDirectWrote, updated, nil
case settleUnsettled:
slog.Info("collab: room neither electable nor peerless within the settle budget; refusing",
"item_id", item.ID,
"budget", applierSettleBudget,
)
writeRoomSettlingError(w, itemRefOrSlug(*item))
return contentRouteHandled, nil, nil
default: // settleDirectFailed
if s.writeTypedItemRefusal(w, item, paErr) {
return contentRouteHandled, nil, nil
}
slog.Warn("collab: direct-write path failed; falling through to the ordinary row write",
"item_id", item.ID,
"error", paErr,
)
return contentRouteFallThrough, nil, paErr
}
}
// settleOutcome is what one pass of the route decision concluded.
type settleOutcome int
const (
// settleElectApplier — an applier is electable; take the reordered path.
settleElectApplier settleOutcome = iota
// settleDirectWrote — the room had no live writer and the direct write ran.
settleDirectWrote
// settleUnsettled — the budget expired with the room neither electable nor
// peerless. NOTHING has been written.
settleUnsettled
// settleDirectFailed — the direct write itself failed; the error is returned.
settleDirectFailed
)
// settleContentRoute is the route decision, extracted from its I/O so the budget
// expiry is testable without a seam into the conn anchoring machinery — which is
// where a test-only lever would otherwise have to reach, and which PLAN-2975 fences
// off as its own unit.
//
// The standoff it bounds is not a race the server can win by trying harder.
// PruneAndApply blocks on ANY conn with canWrite; election additionally requires the
// conn to be unfrozen and past its replay. A room whose only writer has joined and
// not yet anchored satisfies the first and fails the second, so neither path can run,
// and only that conn anchoring resolves it — which this request does not control.
//
// Re-deciding here, before anything is written, is also what closes the route-flip
// the predecessor had: retrying inside applyContentViaCollab re-called
// ApplyExternalContent, which could succeed through a freshly joined applier and let
// the row write run last after all.
func settleContentRoute(
ctx context.Context,
hasElectableApplier func() bool,
tryDirectWrite func() error,
budget, poll time.Duration,
) (settleOutcome, error) {
deadline := time.Now().Add(budget)
for {
// The caller going away ends the wait immediately. This is the only NEW
// blocking wait the reorder introduces — the rest of this path was already
// context-blind on main and stays that way, deliberately, since threading a
// context into the store and the applier round-trip is a change of a
// different size (codex round 2). Bounding what this unit ADDED is the part
// that belongs to this unit.
if err := ctx.Err(); err != nil {
return settleUnsettled, nil
}
if hasElectableApplier() {
return settleElectApplier, nil
}
err := tryDirectWrite()
switch {
case err == nil:
return settleDirectWrote, nil
case errors.Is(err, collab.ErrRoomActiveDuringPrune):
// A writer exists but is not electable, or is about to become so.
// PruneAndApply returns this BEFORE it calls applyFn, so nothing has
// been written and waiting is free of consequence.
if time.Now().After(deadline) {
return settleUnsettled, nil
}
select {
case <-time.After(poll):
case <-ctx.Done():
return settleUnsettled, nil
}
default:
return settleDirectFailed, err
}
}
}
// applierFirstWrite is the reordered applier path: the row write commits WITHOUT
// content, then the apply runs against a committed row and no held transaction
// (PLAN-2975 decision 3).
func (s *Server) applierFirstWrite(
w http.ResponseWriter,
item *models.Item,
input *models.ItemUpdate,
openChildrenPrecheck func(*sql.Tx, *models.Item) error,
parentLink *store.ParentLinkUpdate,
content string,
) (contentRoute, *models.Item, error) {
rowInput := *input
// The content half travels through the applier, not the row. Every store content
// write is gated on Content != nil, so a nil here means the row write touches
// neither items.content nor the version chain's content bracket.
rowInput.Content = nil
updated, uerr := s.store.UpdateItemWithParentLink(item.ID, rowInput, openChildrenPrecheck, parentLink)
if uerr != nil {
// THE POINT OF THE WHOLE CHANGE: this refusal happens before
// ApplyExternalContent is ever called, so there is no Y.Doc write, no
// op-log row, and nothing for a later flush to carry.
if s.writeTypedItemRefusal(w, item, uerr) {
return contentRouteHandled, nil, nil
}
writeInternalError(w, uerr)
return contentRouteHandled, nil, nil
}
if aerr := s.collab.ApplyExternalContent(item.ID, content); aerr != nil {
if errors.Is(aerr, collab.ErrApplierAmbiguous) {
// Untouched by this change, deliberately. A legacy round-trip caught by
// a restore MIGHT have persisted; claiming "content was not applied"
// would be a false statement, so it keeps its own retryable answer.
writeError(w, http.StatusConflict, "applier_ambiguous",
"A concurrent version restore made this edit's outcome ambiguous; please retry.")
return contentRouteHandled, nil, nil
}
outcome := classifyApplyOutcome(aerr)
slog.Warn("collab: row write landed but the apply did not confirm; answering content_not_applied",
"item_id", item.ID,
"content_outcome", outcome,
"error", aerr,
)
writeContentNotAppliedError(w, itemRefOrSlug(*item), landedFieldNames(input), updated.UpdatedAt, outcome, aerr.Error())
return contentRouteHandled, nil, nil
}
return contentRouteApplierWrote, updated, nil
}
// classifyApplyOutcome decides whether a failed apply DEMONSTRABLY left the content
// out of the collaborative document, or merely failed to confirm.
//
// It is written as a WHITELIST — unknown unless proven otherwise — and that is the
// correction from codex round 2 rather than the original shape. The first version
// asked whether the error was a timeout and called everything else "not applied", on
// the reasoning that ApplyExternalContent's own anyWriteSucceeded tracking already
// separated the two. That reasoning was one file short of true: anyWriteSucceeded is
// tracked PER ELECTION, and two paths escaped it — a restore storm returning after
// several elections that may each have sent a request, and a registerPendingAck
// failure on a retry attempt returning a raw error after an earlier attempt had
// already put bytes on the wire. Both would have answered "content_landed: false"
// about content that may well have landed.
//
// The storm path is fixed at its source (ApplyExternalContent now carries sentAny
// across restarts). This function covers the rest by construction: only the two
// sentinels that MEAN nothing reached a peer are allowed to make the claim, and every
// other error — sentinel, wrapped, or entirely unforeseen — is unknown. A new error
// added upstream therefore degrades to the honest answer rather than to a false one.
func classifyApplyOutcome(err error) string {
switch {
case errors.Is(err, collab.ErrNoActiveRoom), errors.Is(err, collab.ErrNoApplierAvailable):
return contentOutcomeNotApplied
default:
return contentOutcomeUnknown
}
}
// composePruneWithPrecheck rides the op-log prune inside the write's own transaction
// (BUG-2840 half B) by composing it onto the precheck hook UpdateItemWithParentLink
// runs there, so a refusal from either rolls the prune back.
func composePruneWithPrecheck(s *Server, itemID string, inner func(*sql.Tx, *models.Item) error) func(*sql.Tx, *models.Item) error {
return func(tx *sql.Tx, existing *models.Item) error {
if inner != nil {
if err := inner(tx, existing); err != nil {
return err
}
}
return s.store.PruneItemOpLogTx(tx, itemID)
}
}
// writeTypedItemRefusal writes the structured refusal for any of the four typed,
// permanent failures store.UpdateItem can return, and reports whether it did.
//
// It exists because this handler's refusal set is a CLASS that has been under-counted
// three separate times (BUG-2804 and BUG-2833 each added an arm a previous unit had
// missed, and isDeterministicWriteFailure carries a comment saying so). One function
// consulted by every ordering is what stops the count drifting again: anyone adding a
// typed refusal to store.UpdateItem changes this and every path inherits it.
func (s *Server) writeTypedItemRefusal(w http.ResponseWriter, item *models.Item, err error) bool {
// A nil error is not a refusal. The typed arms below all tolerate nil (errors.As
// and the write helpers check), but the string-matching arm dereferences, so
// without this a caller asking "is this a refusal?" about success panics. Caught
// by the nil control leg in TestWriteTypedItemRefusalIncludesTitleRefusal the
// moment that arm was added — which is what the control is for.
if err == nil {
return false
}
if details, ok := asOpenChildrenGuardError(err); ok {
writeOpenChildrenError(w, itemRefOrSlug(*item), details)
return true
}
if conflict, ok := asUpdateConflictError(err); ok {
writeUpdateConflictError(w, itemRefOrSlug(*item), conflict)
return true
}
if writeItemRenameCascadeTooLarge(w, err) {
return true
}
if writeInvalidItemTitle(w, err) {
return true
}
// The FIFTH arm, and the one this function was built without — found by codex
// round 5 as a REGRESSION, not a gap. The ordinary path maps a UNIQUE-constraint
// race to a 409 (a concurrent update that passes checkUniqueFields and then hits
// the partial unique index on invocation_slug), and before the reorder the
// applier path's row write ran through that block and inherited it. Routing the
// applier path through a helper built from "the four typed refusals" turned a
// benign race into a 500 on that path only.
//
// The irony is the lesson: this function exists BECAUSE this handler's refusal
// set has been under-counted three times, and building it I under-counted the set
// again — by taking the count from the typed errors rather than from the block
// that actually answers them. The population is what the ordinary path maps, not
// what has a Go type.
//
// It stays a string match here for the same reason it is one there: the store
// returns the driver's error verbatim and SQLite and Postgres word it
// differently. Kept LAST, after every typed arm, because a substring match can
// swallow a typed refusal whose message happens to contain the text.
if strings.Contains(err.Error(), "UNIQUE constraint") || strings.Contains(err.Error(), "duplicate key") {
writeError(w, http.StatusConflict, "conflict",
"An item conflicts with an existing record (duplicate slug, title, or invocation slug)")
return true
}
return false
}
// landedFieldNames names what the row write actually stored, so a content_not_applied
// answer can tell the caller which half of its PATCH is done and must not be re-sent
// blind. Content is never listed: by construction it is the half that did not land.
func landedFieldNames(input *models.ItemUpdate) []string {
var names []string
if input.Title != nil {
names = append(names, "title")
}
if input.Fields != nil {
names = append(names, "fields")
}
for k := range input.FieldsPatch {
names = append(names, "fields."+k)
}
if input.Tags != nil {
names = append(names, "tags")
}
if input.Pinned != nil {
names = append(names, "pinned")
}
if input.SortOrder != nil {
names = append(names, "sort_order")
}
if input.ParentID != nil {
names = append(names, "parent_id")
}
if input.AssignedUserID != nil || input.ClearAssignedUser {
names = append(names, "assigned_user_id")
}
if input.AgentRoleID != nil || input.ClearAgentRole {
names = append(names, "agent_role_id")
}
// Deterministic order: this is a wire value, and a set iterated in map order
// would make the response non-reproducible for no reason.
sort.Strings(names)
return names
}
internal/server/handlers_items_content_route_test.go
+437
View File
@@ -0,0 +1,437 @@
package server
import (
"context"
"encoding/json"
"errors"
"fmt"
"net/http"
"net/http/httptest"
"testing"
"time"
"github.com/PerpetualSoftware/pad/internal/collab"
"github.com/gorilla/websocket"
)
// Route tests for PLAN-2975's write-first-apply-second ordering.
//
// The lexical guard in handlers_items_title_test.go proves the refusal ARMS are
// present, ordered and reachable in every block. These prove the ORDERING: that a
// refusal on the applier path leaves the collaborative document alone, and that an
// apply which fails after the row write says so instead of answering success.
// silentApplier accepts the applier_request and never acks, so the round-trip runs
// out its (shrunk) budget and ApplyExternalContent fails — the condition
// content_not_applied exists to answer. It deliberately does NOT emit an op, so a
// failure to apply leaves no durable trace either.
func silentApplier(t *testing.T, conn *websocket.Conn) func() {
t.Helper()
done := make(chan struct{})
go func() {
defer close(done)
for {
if _, _, err := conn.ReadMessage(); err != nil {
return
}
}
}()
return func() { _ = conn.Close(); <-done }
}
func decodeErrorEnvelope(t *testing.T, body []byte) (code string, details map[string]any) {
t.Helper()
var env struct {
Error struct {
Code string `json:"code"`
Message string `json:"message"`
Details map[string]any `json:"details"`
} `json:"error"`
}
if err := json.Unmarshal(body, &env); err != nil {
t.Fatalf("decode error envelope: %v (body %s)", err, body)
}
return env.Error.Code, env.Error.Details
}
// TestApplierPathRefusalLeavesNoOpLogRow_OpenChildren is the SECOND refusal arm
// driven behaviourally.
//
// The measurement harness covers the optimistic-concurrency arm. This covers the
// open-children guard, which reaches the store through a different mechanism (a
// precheck inside the write transaction rather than a token comparison), so a reorder
// that happened to order one correctly and not the other would show up here.
//
// BOUNDARY, stated rather than left as an apparent gap: the other two arms
// (rename-cascade-too-large, invalid title) have no HTTP trigger on this path — the
// handlers' own pre-checks catch every title refusal reachable over the wire, and the
// store-sourced versions fire only on a concurrent rename inside the lock window,
// which needs a store seam that does not exist. Those two are covered lexically by
// TestUpdateItemErrorBlocksMapEveryStoreRefusal, and that is the whole coverage they
// have.
func TestApplierPathRefusalLeavesNoOpLogRow_OpenChildren(t *testing.T) {
srv := testServerWithCollab(t)
ts := httptest.NewServer(srv)
t.Cleanup(ts.Close)
slug := createWSWithCollections(t, srv)
// The link is established at CREATE time via fields.parent, which is the shape
// the guard's own tests use; a parent_id PATCH answers 200 and leaves no link,
// so a hand-rolled version of this setup measures an unguarded parent.
parent, _ := seedParentAndChildren(t, srv, slug, []string{"open"})
conn, resp, err := dialCollab(t, ts.URL, parent.ID, nil, "")
if err != nil {
status := ""
if resp != nil {
status = resp.Status
}
t.Fatalf("dialCollab: %v (%s)", err, status)
}
stop := applierEcho(t, conn)
t.Cleanup(stop)
waitForApplierPath(t, srv, slug, parent.Ref, parent.ID)
before, err := srv.store.GetItem(parent.ID)
if err != nil {
t.Fatalf("GetItem: %v", err)
}
beforeOps := countOpLog(t, srv, parent.ID)
rr := doRequest(srv, "PATCH", "/api/v1/workspaces/"+slug+"/items/"+parent.Ref,
map[string]interface{}{
"fields": `{"status":"completed"}`,
"content": "content the caller was told was not written",
})
if rr.Code != http.StatusConflict {
t.Fatalf("expected the open-children guard to refuse with 409, got %d: %s", rr.Code, rr.Body.String())
}
if code, _ := decodeErrorEnvelope(t, rr.Body.Bytes()); code != "open_children" {
t.Fatalf("want code open_children, got %q", code)
}
after, err := srv.store.GetItem(parent.ID)
if err != nil {
t.Fatalf("GetItem: %v", err)
}
if after.Content != before.Content {
t.Errorf("items.content moved on a refused PATCH: %q -> %q", before.Content, after.Content)
}
if afterOps := countOpLog(t, srv, parent.ID); afterOps != beforeOps {
t.Errorf("the open-children refusal left %d new op-log row(s) (%d -> %d): the content reached "+
"the live document even though the caller was told the write was refused",
afterOps-beforeOps, beforeOps, afterOps)
}
}
// TestApplyFailureAfterRowWriteAnswersContentNotApplied pins the hybrid outcome the
// reorder creates and the ruled answer to it.
//
// The pre-check is a HINT: it can say yes and the apply can still fail. When it does,
// the row write has already committed, so the response must say BOTH halves — the
// fields landed, the content did not — and must not be a 2xx a client can read as
// success.
func TestApplyFailureAfterRowWriteAnswersContentNotApplied(t *testing.T) {
restore := collab.SetApplierTimeoutsForTesting(150*time.Millisecond, 150*time.Millisecond)
t.Cleanup(restore)
srv := testServerWithCollab(t)
ts := httptest.NewServer(srv)
t.Cleanup(ts.Close)
slug := createWSWithCollections(t, srv)
item := createTaskWithFields(t, srv, slug, "Item", `{"status":"open"}`)
conn, resp, err := dialCollab(t, ts.URL, item.ID, nil, "")
if err != nil {
status := ""
if resp != nil {
status = resp.Status
}
t.Fatalf("dialCollab: %v (%s)", err, status)
}
stop := silentApplier(t, conn)
t.Cleanup(stop)
// The conn must be electable, or this measures the direct-write path instead.
deadline := time.Now().Add(3 * time.Second)
for !srv.collab.HasElectableApplier(item.ID) {
if time.Now().After(deadline) {
t.Fatal("the conn never became electable; this test would have measured the direct path")
}
time.Sleep(2 * time.Millisecond)
}
before, err := srv.store.GetItem(item.ID)
if err != nil {
t.Fatalf("GetItem: %v", err)
}
rr := doRequest(srv, "PATCH", "/api/v1/workspaces/"+slug+"/items/"+item.Slug,
map[string]interface{}{
"title": "Renamed by the same request",
"content": "content that never reaches the document",
})
if rr.Code != http.StatusConflict {
t.Fatalf("an apply that fails after the row write must not answer %d — a 2xx here reads as "+
"success while the content is not in the document. Body: %s", rr.Code, rr.Body.String())
}
code, details := decodeErrorEnvelope(t, rr.Body.Bytes())
if code != "content_not_applied" {
t.Fatalf("want code content_not_applied, got %q (body %s)", code, rr.Body.String())
}
// The silent applier ACCEPTS the request and never acks, so this is the TIMED-OUT
// case: the request went out on the wire and the peer might have applied it. The
// response must say the outcome is unknown rather than assert it did not land —
// asserting that would state as fact something the server cannot know (codex
// round 1).
if got, _ := details["content_outcome"].(string); got != "unknown" {
t.Errorf("content_outcome = %q, want \"unknown\": ErrAllAppliersTimedOut is only returned "+
"after an applier_request reached a peer, so the content may in fact have been applied", got)
}
if _, present := details["content_landed"]; present {
t.Error("content_landed must be ABSENT when the outcome is unknown: a caller that reads " +
"false may act on a premise the server cannot support")
}
if _, ok := details["actual_updated_at"].(string); !ok {
t.Error("actual_updated_at missing: without it a content-only retry trips OCC on a timestamp " +
"this very request moved")
}
fields, _ := details["landed_fields"].([]any)
var sawTitle bool
for _, f := range fields {
if s, _ := f.(string); s == "title" {
sawTitle = true
}
}
if !sawTitle {
t.Errorf("landed_fields %v does not name the title this request DID store; the caller cannot "+
"tell which half of its PATCH is done", fields)
}
// The row half really did land — otherwise the 409 would be honest by accident.
after, err := srv.store.GetItem(item.ID)
if err != nil {
t.Fatalf("GetItem: %v", err)
}
if after.Title == before.Title {
t.Errorf("the row write did not land (title still %q), so this test proved nothing about the "+
"hybrid outcome it exists to pin", after.Title)
}
if after.Content != before.Content {
t.Errorf("items.content moved (%q -> %q) even though the apply failed", before.Content, after.Content)
}
}
// TestSettleContentRouteBoundsTheStandoff covers the decision the end-to-end tests
// cannot reach.
//
// The standoff state — a room whose only writer has joined but not finished replaying
// — is not constructible from this package without a lever into the conn anchoring
// machinery, which PLAN-2975 fences off as its own unit. So the decision is tested
// where it lives, with the two I/O calls injected.
func TestSettleContentRouteBoundsTheStandoff(t *testing.T) {
t.Run("elects an applier immediately when one exists", func(t *testing.T) {
calls := 0
out, err := settleContentRoute(
context.Background(),
func() bool { return true },
func() error { calls++; return nil },
time.Second, time.Millisecond,
)
if out != settleElectApplier || err != nil {
t.Fatalf("want settleElectApplier/nil, got %v/%v", out, err)
}
if calls != 0 {
t.Errorf("the direct write must not be attempted when an applier is electable; called %d times", calls)
}
})
t.Run("writes directly when the room has no live writer", func(t *testing.T) {
out, err := settleContentRoute(
context.Background(),
func() bool { return false },
func() error { return nil },
time.Second, time.Millisecond,
)
if out != settleDirectWrote || err != nil {
t.Fatalf("want settleDirectWrote/nil, got %v/%v", out, err)
}
})
t.Run("gives up on a room that never settles, having written nothing", func(t *testing.T) {
attempts := 0
start := time.Now()
// Bounded by the test rather than trusted to return: the ONLY exit from the
// standoff branch is the deadline, so a broken deadline check loops forever
// and the failure arrives as a 10-minute package timeout with no --- FAIL
// line — which a mutation harness reads as "the package broke", not as a
// detection. Measured: that is exactly what the mutant making the deadline
// unreachable produced before this select was added.
type result struct {
out settleOutcome
err error
}
done := make(chan result, 1)
go func() {
o, e := settleContentRoute(
context.Background(),
func() bool { return false },
func() error { attempts++; return collab.ErrRoomActiveDuringPrune },
60*time.Millisecond, 5*time.Millisecond,
)
done <- result{o, e}
}()
var out settleOutcome
var err error
select {
case r := <-done:
out, err = r.out, r.err
case <-time.After(5 * time.Second):
t.Fatal("settleContentRoute never returned: a room that never settles must reach a " +
"terminal answer, not spin holding the request open")
}
if out != settleUnsettled || err != nil {
t.Fatalf("want settleUnsettled/nil, got %v/%v", out, err)
}
if elapsed := time.Since(start); elapsed < 60*time.Millisecond {
t.Errorf("gave up after %s, before the budget expired: a room that would have settled is "+
"refused early", elapsed)
}
if attempts < 2 {
t.Errorf("only %d attempt(s): the budget must be spent RE-DECIDING, not sleeping once", attempts)
}
})
t.Run("takes the applier path when the writer anchors inside the budget", func(t *testing.T) {
attempts := 0
out, err := settleContentRoute(
context.Background(),
func() bool { return attempts >= 2 },
func() error { attempts++; return collab.ErrRoomActiveDuringPrune },
time.Second, time.Millisecond,
)
if out != settleElectApplier || err != nil {
t.Fatalf("a writer that anchors inside the budget must route to the applier, got %v/%v", out, err)
}
})
t.Run("surfaces a direct-write failure instead of retrying it", func(t *testing.T) {
boom := errors.New("store exploded")
attempts := 0
out, err := settleContentRoute(
context.Background(),
func() bool { return false },
func() error { attempts++; return boom },
time.Second, time.Millisecond,
)
if out != settleDirectFailed || !errors.Is(err, boom) {
t.Fatalf("want settleDirectFailed/boom, got %v/%v", out, err)
}
if attempts != 1 {
t.Errorf("a failure that is not the standoff must not be retried; attempted %d times", attempts)
}
})
}
// TestApplierSettleBudgetCoversTheMeasuredAnchoringWindow pins the CONSTANT, which
// every other test in this file bypasses by passing its own budget.
//
// Found by mutation: setting applierSettleBudget to 0 survived the whole suite. A
// zero budget makes settleContentRoute refuse on its first pass, so every room with a
// writer still anchoring answers room_settling instead of waiting the few milliseconds
// it needs — correct in the sense that nothing is written, and useless in the sense
// that the retryable refusal is the normal answer.
//
// The floor is the measurement the budget was sized from (TASK-2989, real store and
// real WS chain, n=5 per bucket): worst single observation 46.41ms at 5000 op-log
// rows. A budget below that refuses rooms the measurement says would have settled.
// The ceiling is judgement, not measurement: a PATCH that blocks for seconds is worse
// than one that asks the caller to retry.
func TestApplierSettleBudgetCoversTheMeasuredAnchoringWindow(t *testing.T) {
const measuredWorstAnchor = 47 * time.Millisecond
if applierSettleBudget < measuredWorstAnchor {
t.Errorf("applierSettleBudget is %s, below the %s worst anchoring time measured for this "+
"deployment: a room that the measurement says would have settled is refused instead",
applierSettleBudget, measuredWorstAnchor)
}
if applierSettleBudget > 5*time.Second {
t.Errorf("applierSettleBudget is %s: a content PATCH that blocks this long is worse than a "+
"retryable refusal", applierSettleBudget)
}
if applierSettlePoll <= 0 || applierSettlePoll >= applierSettleBudget {
t.Errorf("applierSettlePoll %s must be positive and smaller than the budget %s, or the budget "+
"is spent sleeping rather than re-deciding", applierSettlePoll, applierSettleBudget)
}
}
// TestClassifyApplyOutcomeIsUnknownUnlessProven covers the discriminator directly,
// including the cases the end-to-end tests cannot reach.
//
// Codex round 2 found the first version of this classification wrong: it asked
// whether the error was a timeout and called everything else "not applied", which is
// a false claim for a restore storm that sent requests across several elections, and
// for a registerPendingAck failure returning a raw error after an earlier attempt had
// already put bytes on the wire. Those are the last two subtests, and neither is
// constructible through the HTTP handler.
func TestClassifyApplyOutcomeIsUnknownUnlessProven(t *testing.T) {
cases := []struct {
name string
err error
want string
}{
{"no room at all — nothing could have been sent", collab.ErrNoActiveRoom, contentOutcomeNotApplied},
{"no electable applier — nothing reached a peer", collab.ErrNoApplierAvailable, contentOutcomeNotApplied},
{"wrapped no-room still reads through errors.Is", fmt.Errorf("apply: %w", collab.ErrNoActiveRoom), contentOutcomeNotApplied},
{"timed out — the request went out and may have been applied", collab.ErrAllAppliersTimedOut, contentOutcomeUnknown},
{"ambiguous — outcome unknown by construction", collab.ErrApplierAmbiguous, contentOutcomeUnknown},
{"a raw registerPendingAck error after an earlier send", errors.New("collab: room closing"), contentOutcomeUnknown},
{"an error nobody has written yet", errors.New("something new upstream"), contentOutcomeUnknown},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
if got := classifyApplyOutcome(tc.err); got != tc.want {
t.Errorf("classifyApplyOutcome(%v) = %q, want %q", tc.err, got, tc.want)
}
})
}
}
// TestSettleContentRouteStopsWhenTheCallerGoesAway pins the only new blocking wait
// this change introduces. Without it a cancelled request keeps re-deciding for the
// full budget against a room nobody is waiting on any more.
func TestSettleContentRouteStopsWhenTheCallerGoesAway(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
attempts := 0
done := make(chan settleOutcome, 1)
go func() {
out, _ := settleContentRoute(
ctx,
func() bool { return false },
func() error { attempts++; return collab.ErrRoomActiveDuringPrune },
10*time.Second, 5*time.Millisecond,
)
done <- out
}()
// Let it take at least one lap, so cancellation is what ends it rather than the
// loop never having started.
time.Sleep(20 * time.Millisecond)
cancel()
select {
case out := <-done:
if out != settleUnsettled {
t.Errorf("a cancelled request must end unsettled, got %v", out)
}
case <-time.After(2 * time.Second):
t.Fatal("settleContentRoute ignored cancellation and kept re-deciding; the 10s budget here " +
"is far longer than any caller would wait")
}
if attempts == 0 {
t.Error("the loop never ran, so this proved nothing about cancelling it")
}
}
internal/server/handlers_items_open_children_guard.go
+119
View File
@@ -70,6 +70,7 @@ import (
"errors"
"fmt"
"net/http"
"strconv"
"strings"
"time"
@@ -350,6 +351,124 @@ func writeUpdateConflictEnvelope(w http.ResponseWriter, ref, expectedUpdatedAt s
})
}
// contentNotAppliedRetryAfterSeconds is the Retry-After hint on a room_settling
// refusal. One second: the wait that preceded it already covered the measured
// anchoring window with an order of magnitude to spare, so a room still unsettled
// after it is waiting on something slower than replay — a slow network, a wedged
// conn, a store under load — and a sub-second retry would just re-refuse.
const contentNotAppliedRetryAfterSeconds = 1
// writeContentNotAppliedError emits the pad-structured-error/v1 envelope for the
// outcome the write-first-apply-second ordering creates (PLAN-2975 decision 2): the
// row write COMMITTED and the content did not reach the collaborative document.
//
// It is a 409 rather than a 200-with-a-warning, and that is the ruled shape rather
// than a stylistic choice. The two failure modes are not symmetric: a client that
// ignores an advisory on a 200 believes the content landed and loses the information
// silently, while a client that meets a non-2xx retries — the fields it re-sends hit
// optimistic concurrency and converge, and the content it re-sends applies. The
// property being bought is that a response after which the content is not in the
// document is never readable as success.
//
// `landedFields` names what the row write did store, so the caller can tell that the
// non-content half of its PATCH is done and must not be re-sent blind.
// `actualUpdatedAt` is the post-write value: a content-only retry that echoes it back
// as expected_updated_at will not trip the OCC check on a timestamp this very request
// moved. `reason` carries the underlying apply failure so an operator can tell a
// timed-out applier from an evicted one.
//
// `contentOutcome` is the part a first draft of this got WRONG, and the reason it is
// a parameter rather than a constant false (codex round 1, this unit). An apply that
// TIMED OUT is not the same as one that never happened: ApplyExternalContent only
// returns ErrAllAppliersTimedOut after an applier_request has actually gone out on
// the wire, and the elected peer may have applied the markdown and persisted its ops
// while the ack was lost or merely late. Answering `content_landed: false` there
// states as fact something the server cannot know — the same overclaim the ruling
// avoided by leaving applier_ambiguous alone. So the envelope reports what the server
// can actually distinguish, and the discriminator already exists upstream:
// ErrNoActiveRoom / ErrNoApplierAvailable mean nothing ever reached a peer
// (anyWriteSucceeded == false), while ErrAllAppliersTimedOut means something did.
func writeContentNotAppliedError(w http.ResponseWriter, ref string, landedFields []string, actualUpdatedAt time.Time, contentOutcome, reason string) {
if landedFields == nil {
landedFields = []string{}
}
msg := fmt.Sprintf(
"%s was updated, but its content could not be applied to the live collaborative document; retry the content on its own.",
ref)
details := map[string]any{
"ref": ref,
"landed_fields": landedFields,
// Full RFC3339Nano for the same reason writeUpdateConflictEnvelope uses it:
// this value is meant to be round-tripped back as the caller's
// expected_updated_at token.
"actual_updated_at": actualUpdatedAt.UTC().Format(time.RFC3339Nano),
"apply_reason": reason,
"content_outcome": contentOutcome,
}
switch contentOutcome {
case contentOutcomeNotApplied:
details["content_landed"] = false
default: // contentOutcomeUnknown
// content_landed is DELIBERATELY ABSENT rather than false: the request went
// out and may have been applied. A caller that retries converges either way
// — a re-applied identical markdown is a no-op diff — but a caller that
// reads content_landed:false may take an action premised on the content
// being gone, and that premise would be unfounded.
msg = fmt.Sprintf(
"%s was updated, but the outcome of applying its content to the live collaborative document is unknown; retry the content on its own.",
ref)
}
writeJSON(w, http.StatusConflict, map[string]any{
"error": map[string]any{
"code": "content_not_applied",
"message": msg,
"details": details,
},
})
}
// contentOutcome values for writeContentNotAppliedError's details.
const (
// contentOutcomeNotApplied — no applier_request ever reached a peer, so the
// content demonstrably did not land.
contentOutcomeNotApplied = "not_applied"
// contentOutcomeUnknown — a request went out and was not acked in time. The
// peer may have applied it.
contentOutcomeUnknown = "unknown"
)
// writeRoomSettlingError emits the pad-structured-error/v1 envelope for a room that
// is neither settled enough to elect an applier nor empty enough to write directly
// (PLAN-2975 decision 2, standoff clause).
//
// That state is real and is not a race the server can resolve by trying harder:
// PruneAndApply blocks on any conn with canWrite, while election additionally
// requires the conn to be unfrozen and past its replay. A room whose only writer has
// joined but not finished replaying satisfies the first and fails the second, so the
// direct path refuses and the applier path has nobody to elect. Only the conn
// anchoring resolves it, which this request does not control.
//
// The predecessor behaviour was to give up after three attempts and write
// items.content directly past that live peer — a write the peer's next flush
// overwrites. A refusal the caller can retry is strictly better than a write that is
// silently lost, which is why this refusal replaces it on the applier route. NOTHING
// has been written when this fires: PruneAndApply returns before it calls applyFn.
func writeRoomSettlingError(w http.ResponseWriter, ref string) {
w.Header().Set("Retry-After", strconv.Itoa(contentNotAppliedRetryAfterSeconds))
writeJSON(w, http.StatusConflict, map[string]any{
"error": map[string]any{
"code": "room_settling",
"message": fmt.Sprintf(
"%s has a collaborator connecting right now; nothing was changed. Retry in a moment.",
ref),
"details": map[string]any{
"ref": ref,
"retry_after_seconds": contentNotAppliedRetryAfterSeconds,
},
},
})
}
// asUpdateConflictError reports whether err is (or wraps) a
// store.UpdateConflictError and returns it. Handlers use it to branch the
// generic upstream-error path into the structured 409 above.
internal/server/handlers_items_title_test.go
+245 -34
View File
@@ -9,6 +9,7 @@ import (
"go/token"
"net/http"
"net/http/httptest"
"os"
"sort"
"strings"
"testing"
@@ -261,36 +262,83 @@ func TestCreateItemCheckedMapsStoreTitleRefusalTo400(t *testing.T) {
}
}
// TestIsDeterministicWriteFailureIncludesTitleRefusal regresses codex round 1
// P1 on the collab fallback.
// TestWriteTypedItemRefusalIncludesTitleRefusal regresses codex round 1 P1 on the
// collab fallback, PORTED from isDeterministicWriteFailure when PLAN-2975 unit 2
// replaced that classifier with writeTypedItemRefusal.
//
// isDeterministicWriteFailure decides whether a direct-write failure is a
// settled answer. A permanent refusal it does not recognise is returned as a
// generic collab-routing error, so the caller falls through to its own direct
// write and re-derives the identical refusal from scratch — BUG-2804 measured
// that as running a whole rename cascade twice for one request, and reported
// the answer by the other route.
// The property is unchanged and is why the port was worth doing rather than deleting
// the test with the function: a permanent refusal the handler does not RECOGNISE is
// treated as a recoverable routing error, so the request falls through to another
// write path and re-derives the identical refusal from scratch — BUG-2804 measured
// that as running a whole rename cascade twice for one request, and answering by the
// other route.
//
// The store gained a fourth such refusal with the item-title bound and nothing
// failed when it was left out, which is exactly why this test exists: the
// omission is invisible from the outside.
func TestIsDeterministicWriteFailureIncludesTitleRefusal(t *testing.T) {
if !isDeterministicWriteFailure(&store.InvalidItemTitleError{Reason: "Title is required"}) {
// The store gained a fourth such refusal with the item-title bound and nothing failed
// when it was left out, which is exactly why this exists: the omission is invisible
// from the outside.
func TestWriteTypedItemRefusalIncludesTitleRefusal(t *testing.T) {
srv := testServer(t)
item := &models.Item{ID: "item-1", Ref: "TASK-1", Slug: "task-1"}
// The recorder is inspected, not discarded: the classifier's answer is only half
// the contract — a mutant that returns true while writing the wrong status would
// pass a boolean-only assertion (codex round 1, this unit).
refused := func(err error) bool {
rec := httptest.NewRecorder()
got := srv.writeTypedItemRefusal(rec, item, err)
if got {
if rec.Code < 400 || rec.Code > 499 {
t.Errorf("a recognised refusal wrote status %d; a refusal must answer 4xx or the "+
"caller cannot tell it from success", rec.Code)
}
if rec.Body.Len() == 0 {
t.Error("a recognised refusal wrote no body; the structured envelope is the contract")
}
} else if rec.Body.Len() != 0 {
t.Errorf("an unrecognised error wrote a body (%s) while reporting not-handled; the "+
"caller will write a second response on top of it", rec.Body.String())
}
return got
}
if !refused(&store.InvalidItemTitleError{Reason: "Title is required"}) {
t.Error("an invalid-title refusal is permanent: retrying the same title always refuses")
}
// Through a wrapper, since the call path wraps on the way up.
if !isDeterministicWriteFailure(fmt.Errorf("update item: %w", &store.InvalidItemTitleError{Reason: "Title is too long"})) {
if !refused(fmt.Errorf("update item: %w", &store.InvalidItemTitleError{Reason: "Title is too long"})) {
t.Error("must see through wrappers")
}
// Controls. Without these, a mutant returning true unconditionally would
// pass — and that mutant would break the graceful-degradation contract the
// function exists to protect, by treating a transient prune failure as
// final.
if isDeterministicWriteFailure(nil) {
// Controls. Without these a mutant returning true unconditionally would pass —
// and that mutant would swallow every transient failure as a settled refusal,
// answering a 4xx for a condition that would have succeeded on retry.
if refused(nil) {
t.Error("nil is not a failure")
}
if isDeterministicWriteFailure(errors.New("transient prune failure")) {
t.Error("an unrecognised error must stay recoverable so the fallback still degrades gracefully")
if refused(errors.New("transient prune failure")) {
t.Error("an unrecognised error must stay recoverable so the route still degrades gracefully")
}
// The FIFTH arm. Before PLAN-2975 the applier path's row write fell through the
// ordinary error block and inherited its UNIQUE-constraint mapping; routing that
// path through this helper dropped it, turning a benign race into a 500 on that
// route only (codex round 5, a regression rather than a gap). Both wordings are
// asserted because the store hands back the driver's message verbatim and SQLite
// and Postgres word it differently — testing one would leave the other route to
// the 500.
for _, msg := range []string{
"UNIQUE constraint failed: items.slug",
`pq: duplicate key value violates unique constraint "items_invocation_slug_idx"`,
} {
rec := httptest.NewRecorder()
if !srv.writeTypedItemRefusal(rec, item, errors.New(msg)) {
t.Errorf("a unique-constraint race (%q) must be recognised; unrecognised it answers 500 "+
"for a request the server understood and declined", msg)
continue
}
if rec.Code != http.StatusConflict {
t.Errorf("a unique-constraint race answered %d, want 409 to match the create path and "+
"the ordinary update path", rec.Code)
}
}
}
@@ -339,16 +387,19 @@ func TestIsDeterministicWriteFailureIncludesTitleRefusal(t *testing.T) {
// the lock window. Driving that from a test needs a store-level seam that does
// not exist. The helper itself is unit-tested; these arms' job is to call it.
func TestUpdateItemErrorBlocksMapEveryStoreRefusal(t *testing.T) {
fset := token.NewFileSet()
file, err := parser.ParseFile(fset, "handlers_items.go", nil, 0)
if err != nil {
t.Fatalf("parse handlers_items.go: %v", err)
}
// The arms, in the order every block must apply them. Order is part of the
// contract, not style: the UNIQUE-constraint arm that closes each block
// matches on error TEXT, so a typed arm placed after it can be swallowed by
// a substring match rather than reached.
// TWO FILES since PLAN-2975, and the second one is why the expected block
// count changed rather than the guard weakening.
//
// The write-first-apply-second reorder took the applier branch's inline block
// out of handlers_items.go and replaced it with a call to writeTypedItemRefusal,
// which maps all four arms once and is consulted by every ordering. That is a
// STRONGER shape than three parallel blocks — the failure this guard exists to
// catch is an arm mapped in some routes and not others, and a single shared
// function cannot drift against itself — but it moves one block into another
// file, so a scan of handlers_items.go alone now sees two blocks and fails
// closed. It failed closed when the reorder landed, which is the guard working;
// teaching it the new shape is the response, and the count below is the part a
// future restructuring will trip again on purpose.
want := []string{
"asOpenChildrenGuardError",
"asUpdateConflictError",
@@ -360,6 +411,60 @@ func TestUpdateItemErrorBlocksMapEveryStoreRefusal(t *testing.T) {
wantArm[w] = true
}
// THE FILE SET IS DERIVED, NOT LISTED (codex round 5). A hardcoded pair passes
// while an unmapped block sits in a third file, which is the same
// under-counting this guard exists to catch — so the sources are every
// non-test file in the package that calls UpdateItemWithParentLink, and a file
// that starts calling it joins the scan by doing so.
fset := token.NewFileSet()
entries, err := os.ReadDir(".")
if err != nil {
t.Fatalf("read package dir: %v", err)
}
var files []*ast.File
var sources []string
for _, e := range entries {
name := e.Name()
if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") {
continue
}
src, rerr := os.ReadFile(name)
if rerr != nil {
t.Fatalf("read %s: %v", name, rerr)
}
// The predicate is a UNION, and fail-open is the failure mode it is fighting
// (codex round 6): a file reaching the store through a wrapper or a variable
// would not match the call text, so a file that calls any of the refusal ARMS
// is scanned too. A refusal block lives where the arms are called, whatever it
// calls the store through.
text := string(src)
relevant := strings.Contains(text, "UpdateItemWithParentLink(")
for _, arm := range want {
if strings.Contains(text, arm+"(") {
relevant = true
}
}
if !relevant {
continue
}
f, perr := parser.ParseFile(fset, name, src, 0)
if perr != nil {
t.Fatalf("parse %s: %v", name, perr)
}
files = append(files, f)
sources = append(sources, name)
}
if len(files) == 0 {
t.Fatal("no non-test file in this package calls UpdateItemWithParentLink; the scan found " +
"nothing to check, so every assertion below would be vacuous")
}
t.Logf("scanning %v", sources)
// The arms, in the order every block must apply them. Order is part of the
// contract, not style: the UNIQUE-constraint arm that closes each block
// matches on error TEXT, so a typed arm placed after it can be swallowed by
// a substring match rather than reached.
// ---- membership + order, per block ----
//
// PER-BLOCK, not per-file (codex round 2): counting calls across the whole
@@ -381,7 +486,12 @@ func TestUpdateItemErrorBlocksMapEveryStoreRefusal(t *testing.T) {
pos token.Pos
}
var refs []armRef
ast.Inspect(file, func(n ast.Node) bool {
inspectAll := func(fn func(ast.Node) bool) {
for _, f := range files {
ast.Inspect(f, fn)
}
}
inspectAll(func(n ast.Node) bool {
call, ok := n.(*ast.CallExpr)
if !ok {
return true
@@ -399,10 +509,12 @@ func TestUpdateItemErrorBlocksMapEveryStoreRefusal(t *testing.T) {
var blocks [][]string
var lines []int
var startPos []token.Pos
for _, r := range refs {
if r.name == want[0] {
blocks = append(blocks, nil)
lines = append(lines, fset.Position(r.pos).Line)
startPos = append(startPos, r.pos)
}
if len(blocks) == 0 {
t.Fatalf("arm %q at line %d precedes any %q — the block-splitting assumption is wrong",
@@ -416,13 +528,23 @@ func TestUpdateItemErrorBlocksMapEveryStoreRefusal(t *testing.T) {
// BUG-2833 door sweep), so single-arm blocks are not update blocks.
var updateBlocks [][]string
var updateLines []int
var updateStart []token.Pos
for i, b := range blocks {
if len(b) > 1 {
updateBlocks = append(updateBlocks, b)
updateLines = append(updateLines, lines[i])
updateStart = append(updateStart, startPos[i])
}
}
// Three: two inline blocks remaining in handlers_items.go, plus
// writeTypedItemRefusal's single shared block. The number is unchanged from
// before PLAN-2975 by coincidence — what changed is that one of the three is now
// reached by every content-PATCH ordering instead of being copied per route.
//
// The constant is deliberately brittle. A restructuring that changes the count
// should stop here and be looked at, because that is the moment a refusal
// silently stops being mapped on one route.
const wantBlocks = 3
if len(updateBlocks) != wantBlocks {
t.Fatalf("found %d UpdateItem error block(s) at lines %v, want %d — the instrument's block "+
@@ -446,6 +568,88 @@ func TestUpdateItemErrorBlocksMapEveryStoreRefusal(t *testing.T) {
}
}
// ---- the fifth arm: the UNIQUE-constraint race ----
//
// It is not in `want` because it is not a call to a named helper — it is a string
// match on the driver's message, so the AST walk above cannot see it. It is
// checked anyway, and separately, because dropping it is exactly the regression
// codex round 5 found: the applier path inherited this mapping from the ordinary
// block until the reorder routed around it, and a benign race answered 500 on one
// route and 409 on the other.
//
// SCOPED TO THE ENCLOSING FUNCTION, which is what made it discriminate. The first
// version asked whether a UNIQUE literal appeared between this block's start and
// the next block's start in token.Pos. Those windows span whole FILES — the gap
// between the last block of one file and the first block of the next swallows
// every literal in between, including two in handlers_items.go belonging to the
// create and restore paths and one in handlers_items_bulk.go. ALL THREE mutation
// controls survived that version; it asserted nothing. A block's arm lives in the
// block's own function, so that is the containment to test.
// The containment is the innermost BLOCK STATEMENT holding the block's first arm,
// not the enclosing function. Per-function was the second wrong answer and the
// controls said so: handleUpdateItem holds TWO refusal blocks with an arm each, so
// neutralising either one hid behind the other and survived. Only the
// writeTypedItemRefusal control was detected, i.e. the check covered one of the
// three blocks it claimed to cover.
innermostBlock := func(pos token.Pos) *ast.BlockStmt {
var best *ast.BlockStmt
inspectAll(func(n ast.Node) bool {
b, ok := n.(*ast.BlockStmt)
if !ok {
return true
}
if pos < b.Pos() || pos > b.End() {
return true
}
if best == nil || b.Pos() > best.Pos() {
best = b
}
return true
})
return best
}
// The arm is an IF CONDITION, and the check reads only conditions (codex round
// 7). Accepting any string literal in the block would let an unrelated nested
// closure, or a message string that happened to quote the phrase, satisfy the
// guard after the real mapping had been deleted — the guard passing for a reason
// that has nothing to do with what it claims.
hasUniqueArm := func(b *ast.BlockStmt) bool {
found := false
ast.Inspect(b, func(n ast.Node) bool {
ifStmt, ok := n.(*ast.IfStmt)
if !ok || ifStmt.Cond == nil {
return true
}
ast.Inspect(ifStmt.Cond, func(c ast.Node) bool {
lit, ok := c.(*ast.BasicLit)
if !ok || lit.Kind != token.STRING {
return true
}
if strings.Contains(lit.Value, "UNIQUE constraint") {
found = true
}
return true
})
return true
})
return found
}
for i, lo := range updateStart {
b := innermostBlock(lo)
if b == nil {
t.Errorf("the error block at line %d is not inside any block statement; the containment "+
"this check relies on does not hold", updateLines[i])
continue
}
if !hasUniqueArm(b) {
t.Errorf("the error block at line %d has no UNIQUE-constraint arm in its own scope. A "+
"concurrent slug/title collision answers 409 on the routes that map it and 500 on "+
"the ones that do not, for the identical store error — which is the regression that "+
"put this check here.", updateLines[i])
}
}
// ---- reachability ----
//
// Counting call expressions detects a DELETED arm but not a disabled one:
@@ -465,7 +669,7 @@ func TestUpdateItemErrorBlocksMapEveryStoreRefusal(t *testing.T) {
//
// Anything else is either a disabling mutation or a restructuring that
// deserves to be looked at deliberately.
ast.Inspect(file, func(n ast.Node) bool {
inspectAll(func(n ast.Node) bool {
ifStmt, ok := n.(*ast.IfStmt)
if !ok {
return true
@@ -497,7 +701,14 @@ func TestUpdateItemErrorBlocksMapEveryStoreRefusal(t *testing.T) {
// let the next arm write another, or let the request continue to the
// generic 500 (codex round 6). The body's last statement is a bare return
// in all three blocks; anything else is a behaviour change worth looking at.
ast.Inspect(file, func(n ast.Node) bool {
//
// BOUNDARY, stated because the shared block weakened this leg and pretending
// otherwise would be the exact overclaim this file's header warns about: in
// writeTypedItemRefusal the arms end in `return true`, a handled-FLAG its
// callers act on, not a return from the request. This walk therefore proves
// the arm stops the FUNCTION, and the caller honouring the flag is checked
// behaviourally by the route tests rather than here.
inspectAll(func(n ast.Node) bool {
ifStmt, ok := n.(*ast.IfStmt)
if !ok {
return true