Artifact d110836fcff52ada165bfd4d4108c7357f5ff445bd5dabb968b7dd21023eb351:
- File
internal/xfer/encode.go
— part of check-in
[f2fa2cc751]
at
2026-07-19 22:42:56
on branch trunk
— fix: clone_seqno is server-to-client only (#96)
* fix: clone_seqno is server-to-client only (#74)
The clone client transmitted a `clone_seqno` card to carry its pagination cursor. That card travels the other way: canonical's page_xfer() only ever writes it (src/xfer.c:1571) and has no parser for it, so a real fossil server answered `bad command: clone_seqno N` and the clone died on round two. Repositories that fit in a single batch never reached the pagination path, which is why wapp and pikchr cloned while fossil and sqlite did not.
The request cursor belongs on the clone card itself — `clone VERSION SEQNO`, read from token 2 at src/xfer.c:1553 — and the response cursor belongs on `clone_seqno`, which the client only consumes. Give each direction a single owner:
- The client sends its cursor on the clone card and never emits a clone_seqno card. It already consumed the server's card correctly; it now also rejects a negative value, matching src/xfer.c:2769. - The server reads its cursor from the clone card and the CloneSeqNoCard parser is gone, so there is exactly one way to set the cursor rather than two.
Aligning with canonical also corrects the cursor's meaning. Ours was the last rid sent, queried with `rid > cursor`; canonical's is the rid of the next blob to send, looped inclusively with `while( seqno<=max )`. Now that the value crosses to a real server it has to mean what canonical means, so emitCloneBatch queries `rid >= cursor` and reports the first rid it did not send, or 0 once the snapshot is exhausted. That collapses the old more/lastSentRID pair into a single nextRID and keeps the truncate hook honest by re-owing the dropped rid instead of skipping it.
TestCloneMultiBatchAgainstCanonicalServer drives the client against a stand-in server that implements the canonical contract and rejects a client-sent clone_seqno the way fossil does; it reproduces `bad command: clone_seqno` against the old client. TestHandleCloneCursorFromCloneCard pins the server half, including the inclusive cursor.
Verified against fossil 2.28 over a loopback server: fossil and sqlite now transfer every artifact (67,499 and 130,994 blobs), and wapp and pikchr still clone in 3 rounds.
* fix: conform clone cursor validation to the spec in both directions (#74)
Review caught two rules applied to the wrong side of the wire — the same axis error the original bug was.
The client rejected a negative clone_seqno as fatal, citing src/xfer.c:2769. That line is the server's rule for the SEQNO on a clone REQUEST. The reply parser is governed by §3.2 and §8.2: NEXT is recorded only when it is decimal, and a visible but non-decimal NEXT is a defined receiver-tolerance exception that leaves the recorded sequence unchanged and does not stop parsing later reply cards. Aborting the clone was the one thing the spec rules out. The rule belongs in the decoder, where a non-decimal NEXT now decodes to an unknown card: the token is preserved, no consumer records it, and following cards still parse. strconv.Atoi accepted "-1" off the wire, so this was reachable, not theoretical.
Conversely the server clamped `clone 3 0` to a cursor of 1 where §8.1 mandates a fatal: clear accumulated output, roll back, emit `error invalid clone sequence number`, and parse no later request card. Nothing is written before processDataCards, so returning early is what the rollback amounts to. Seeing this case at all needs a presence flag, because `clone` and `clone 3 0` both parsed to SeqNo 0 and only the former is exempt. §8.1 also keys on a digit-only SEQNO and withholds the fatal from wrong arity, a VERSION below 2, and a SEQNO that fails digit-only recognition, so the flag records digit-only presence rather than merely arity.
Both card rules now turn on the spec's `decimal = 1*DIGIT` production via a shared predicate instead of on whatever strconv.Atoi tolerates.
Also promotes emitCloneBatch's `count > 1` truncate guard to a documented liveness constraint. Under the old exclusive `rid > cursor` semantics `count > 0` was safe; under the inclusive cursor, dropping the only card in a batch reports a cursor equal to the one we entered with and the client re-requests forever. A check before the reply card now names a stalled round at its source instead of letting it surface as an opaque MaxRounds failure, and the cursor precondition became an error return rather than a panic — it is enforced ~350 lines away in a network-facing handler.
Records the deliberate asymmetry that our server ignores an inbound clone_seqno where canonical answers `bad command`, and renames TestHandleCloneSeqNo, which no longer sends that card.
* fix: name the clone SEQNO flag for the shape it records (#74)
HasSeqNo described presence, but the field records whether the SEQNO token is digit-only: `clone 3 -1` carries a SEQNO and sets it false. §8.1 keys its fatal on that shape rather than on the parsed value, which is why a plain int cannot stand in for it. SeqNoIsDecimal says so.
Reading it as presence would also collapse `clone 0 0` — a distinct wire form that parses to the same Version and SeqNo as a bare `clone` — back into `clone` on re-encode. encodeClone already guarded against that; a round-trip test over every clone wire form now pins it, and fails on exactly that input when the guard is removed.
Also records why the §8.1 rollback is currently vacuous: no card handled before processDataCards writes anything, so returning early leaves nothing to undo. Nothing enforces that ordering, and §3.5's ci-lock would break it, so the dependency is written down next to the code that relies on it.
Corrects a test comment that claimed a continuation property it did not exercise — the malformed card sits in the trailer, so nothing follows it. That half of §8.2 is covered by the follow-on card in TestDecode_CloneSeqNoNonDecimal. (user: [email protected], size: 8674) [annotate] [blame] [check-ins using]
package xfer import ( "bytes" "compress/zlib" "fmt" "github.com/danmestas/libfossil/internal/deck" ) // EncodeCard writes the wire-format representation of a Card to w. func EncodeCard(w *bytes.Buffer, c Card) error { if w == nil { panic("xfer.EncodeCard: w must not be nil") } if c == nil { panic("xfer.EncodeCard: c must not be nil") } switch v := c.(type) { case *IGotCard: return encodeIGot(w, v) case *GimmeCard: return encodeGimme(w, v) case *PushCard: return encodePush(w, v) case *PullCard: return encodePull(w, v) case *CookieCard: return encodeCookie(w, v) case *ReqConfigCard: return encodeReqConfig(w, v) case *PrivateCard: return encodePrivate(w) case *CloneCard: return encodeClone(w, v) case *CloneSeqNoCard: return encodeCloneSeqNo(w, v) case *UVGimmeCard: return encodeUVGimme(w, v) case *PragmaCard: return encodePragma(w, v) case *LoginCard: return encodeLogin(w, v) case *ErrorCard: return encodeError(w, v) case *MessageCard: return encodeMessage(w, v) case *UVIGotCard: return encodeUVIGot(w, v) case *FileCard: return encodeFile(w, v) case *CFileCard: return encodeCFile(w, v) case *ConfigCard: return encodeConfig(w, v) case *UVFileCard: return encodeUVFile(w, v) case *SchemaCard: return encodeSchema(w, v) case *XIGotCard: return encodeXIGot(w, v) case *XGimmeCard: return encodeXGimme(w, v) case *XRowCard: return encodeXRow(w, v) case *XDeleteCard: return encodeXDelete(w, v) case *UnknownCard: return encodeUnknown(w, v) default: return fmt.Errorf("xfer: cannot encode %T", c) } } func encodeIGot(w *bytes.Buffer, c *IGotCard) error { w.WriteString("igot ") w.WriteString(c.UUID) if c.IsPrivate { w.WriteString(" 1") } w.WriteByte('\n') return nil } func encodeGimme(w *bytes.Buffer, c *GimmeCard) error { w.WriteString("gimme ") w.WriteString(c.UUID) w.WriteByte('\n') return nil } // encodePush writes "push <project-code> [<server-code>]\n". // // The Fossil C xfer parser (src/xfer.c xfer_push_card) requires at least the // project code; a bare "push\n" is rejected by real Fossil servers. The // project code must always be present — callers must populate PushCard.ProjectCode // before encoding. ServerCode is optional (omitted on the first round when no // prior session exists). // // Wire-format arg order matches Fossil C: project-code first, server-code second. func encodePush(w *bytes.Buffer, c *PushCard) error { if c.ProjectCode == "" { panic("xfer: encodePush: ProjectCode must not be empty") } w.WriteString("push ") w.WriteString(c.ProjectCode) if c.ServerCode != "" { w.WriteByte(' ') w.WriteString(c.ServerCode) } w.WriteByte('\n') return nil } // encodePull writes "pull <project-code> <server-code>\n". // // The Fossil C xfer parser requires both args on pull — a bare "pull\n" or // single-arg "pull <code>\n" is rejected. Both ProjectCode and ServerCode must // be non-empty before encoding. // // Wire-format arg order matches Fossil C: project-code first, server-code second. func encodePull(w *bytes.Buffer, c *PullCard) error { if c.ProjectCode == "" { panic("xfer: encodePull: ProjectCode must not be empty") } if c.ServerCode == "" { panic("xfer: encodePull: ServerCode must not be empty") } w.WriteString("pull ") w.WriteString(c.ProjectCode) w.WriteByte(' ') w.WriteString(c.ServerCode) w.WriteByte('\n') return nil } func encodeCookie(w *bytes.Buffer, c *CookieCard) error { w.WriteString("cookie ") w.WriteString(c.Value) w.WriteByte('\n') return nil } func encodeReqConfig(w *bytes.Buffer, c *ReqConfigCard) error { w.WriteString("reqconfig ") w.WriteString(c.Name) w.WriteByte('\n') return nil } func encodePrivate(w *bytes.Buffer) error { w.WriteString("private\n") return nil } func encodeClone(w *bytes.Buffer, c *CloneCard) error { if !c.SeqNoIsDecimal && c.Version == 0 && c.SeqNo == 0 { w.WriteString("clone\n") } else { fmt.Fprintf(w, "clone %d %d\n", c.Version, c.SeqNo) } return nil } func encodeCloneSeqNo(w *bytes.Buffer, c *CloneSeqNoCard) error { fmt.Fprintf(w, "clone_seqno %d\n", c.SeqNo) return nil } func encodeUVGimme(w *bytes.Buffer, c *UVGimmeCard) error { w.WriteString("uvgimme ") w.WriteString(c.Name) w.WriteByte('\n') return nil } func encodePragma(w *bytes.Buffer, c *PragmaCard) error { w.WriteString("pragma ") w.WriteString(c.Name) for _, v := range c.Values { w.WriteByte(' ') w.WriteString(v) } w.WriteByte('\n') return nil } func encodeLogin(w *bytes.Buffer, c *LoginCard) error { w.WriteString("login ") w.WriteString(deck.FossilEncode(c.User)) w.WriteByte(' ') w.WriteString(c.Nonce) w.WriteByte(' ') w.WriteString(c.Signature) w.WriteByte('\n') return nil } func encodeError(w *bytes.Buffer, c *ErrorCard) error { w.WriteString("error ") w.WriteString(deck.FossilEncode(c.Message)) w.WriteByte('\n') return nil } func encodeMessage(w *bytes.Buffer, c *MessageCard) error { w.WriteString("message ") w.WriteString(deck.FossilEncode(c.Message)) w.WriteByte('\n') return nil } func encodeUVIGot(w *bytes.Buffer, c *UVIGotCard) error { fmt.Fprintf(w, "uvigot %s %d %s %d\n", c.Name, c.MTime, c.Hash, c.Size) return nil } func encodeUnknown(w *bytes.Buffer, c *UnknownCard) error { w.WriteString(c.Command) for _, a := range c.Args { w.WriteByte(' ') w.WriteString(a) } w.WriteByte('\n') return nil } // --- Payload card encoders --- // encodeFile writes: file UUID SIZE \n CONTENT (no trailing \n) // or: file UUID DELTASRC SIZE \n CONTENT (delta variant) func encodeFile(w *bytes.Buffer, c *FileCard) error { w.WriteString("file ") w.WriteString(c.UUID) if c.DeltaSrc != "" { w.WriteByte(' ') w.WriteString(c.DeltaSrc) } fmt.Fprintf(w, " %d\n", len(c.Content)) w.Write(c.Content) // NO trailing newline return nil } // encodeCFile writes: cfile UUID USIZE CSIZE \n ZCONTENT (no trailing \n) // or: cfile UUID DELTASRC USIZE CSIZE \n ZCONTENT (delta variant) func encodeCFile(w *bytes.Buffer, c *CFileCard) error { // Compress content with zlib var zbuf bytes.Buffer zw := zlib.NewWriter(&zbuf) if _, err := zw.Write(c.Content); err != nil { return fmt.Errorf("xfer: cfile zlib write: %w", err) } if err := zw.Close(); err != nil { return fmt.Errorf("xfer: cfile zlib close: %w", err) } w.WriteString("cfile ") w.WriteString(c.UUID) if c.DeltaSrc != "" { w.WriteByte(' ') w.WriteString(c.DeltaSrc) } fmt.Fprintf(w, " %d %d\n", len(c.Content), zbuf.Len()) w.Write(zbuf.Bytes()) // NO trailing newline return nil } // encodeConfig writes: config NAME SIZE \n CONTENT \n // Note the trailing \n after CONTENT. func encodeConfig(w *bytes.Buffer, c *ConfigCard) error { fmt.Fprintf(w, "config %s %d\n", c.Name, len(c.Content)) w.Write(c.Content) w.WriteByte('\n') // trailing newline return nil } // encodeUVFile writes: uvfile NAME MTIME HASH SIZE FLAGS \n CONTENT // When deleted (0x0001) or content-omitted (0x0004), no payload follows. func encodeUVFile(w *bytes.Buffer, c *UVFileCard) error { fmt.Fprintf(w, "uvfile %s %d %s %d %d\n", c.Name, c.MTime, c.Hash, c.Size, c.Flags) if !uvFileOmitsContent(c.Flags) { w.Write(c.Content) } // NO trailing newline return nil } func encodeSchema(w *bytes.Buffer, c *SchemaCard) error { fmt.Fprintf(w, "schema %s %d %s %d %d\n", c.Table, c.Version, c.Hash, c.MTime, len(c.Content)) w.Write(c.Content) w.WriteByte('\n') return nil } func encodeXIGot(w *bytes.Buffer, c *XIGotCard) error { if c.Table == "" { panic("encodeXIGot: Table must not be empty") } if c.PKHash == "" { panic("encodeXIGot: PKHash must not be empty") } fmt.Fprintf(w, "xigot %s %s %d\n", c.Table, c.PKHash, c.MTime) return nil } func encodeXGimme(w *bytes.Buffer, c *XGimmeCard) error { if c.Table == "" { panic("encodeXGimme: Table must not be empty") } if c.PKHash == "" { panic("encodeXGimme: PKHash must not be empty") } fmt.Fprintf(w, "xgimme %s %s\n", c.Table, c.PKHash) return nil } func encodeXRow(w *bytes.Buffer, c *XRowCard) error { if c.Table == "" { panic("encodeXRow: Table must not be empty") } if c.PKHash == "" { panic("encodeXRow: PKHash must not be empty") } fmt.Fprintf(w, "xrow %s %s %d %d\n", c.Table, c.PKHash, c.MTime, len(c.Content)) w.Write(c.Content) w.WriteByte('\n') return nil } func encodeXDelete(w *bytes.Buffer, c *XDeleteCard) error { if c.Table == "" { panic("encodeXDelete: Table must not be empty") } if c.PKHash == "" { panic("encodeXDelete: PKHash must not be empty") } fmt.Fprintf(w, "xdelete %s %s %d %d\n", c.Table, c.PKHash, c.MTime, len(c.PKData)) w.Write(c.PKData) w.WriteByte('\n') return nil }