diff --git a/internal/tools/perforce.go b/internal/tools/perforce.go index 4f7b1f3..e1c466b 100644 --- a/internal/tools/perforce.go +++ b/internal/tools/perforce.go @@ -53,6 +53,12 @@ type Describe struct { Status string `json:"status"` Files []DescribedFile `json:"files"` FileCount int `json:"file_count"` + + // The changelist's real file count, before max_files or max_bytes were + // applied. FileCount is what was RETURNED; this is what EXISTS. A consumer + // comparing the two can see exactly how much a truncated record is missing + // instead of only knowing that it is short (#1904). + DepotFileCount int `json:"depot_file_count"` } // ChangeSummary is one entry of p4.changes. @@ -82,14 +88,28 @@ func (p *Perforce) Execute(ctx context.Context, verb string, args map[string]any func (p *Perforce) describe(ctx context.Context, change int64, maxFiles, maxBytes int) (any, int, bool, error) { // -s omits the diffs entirely; this is the content boundary enforced at the // tool invocation, not only in the schema. - recs, n, err := p.run(ctx, maxBytes, "describe", "-s", strconv.FormatInt(change, 10)) + recs, n, err := p.run(ctx, "describe", "-s", strconv.FormatInt(change, 10)) if err != nil { return nil, n, false, err } if len(recs) == 0 { return nil, n, false, fmt.Errorf("perforce: changelist %d not found", change) } - r := recs[0] + out, truncated := describeFromRecord(recs[0], change, maxFiles, maxBytes) + return out, n, truncated, nil +} + +// describeFromRecord shapes one p4 -Mj describe record into a Describe, +// applying both size bounds. Split out from describe() so the bounds - which +// are the whole subject of #1904 - can be exercised without a live p4 server. +// +// TWO bounds apply, and both TRUNCATE rather than fail (PROTOCOL.md, +// "Ordering, replay, and size": "the connector truncates and sets +// truncated: true rather than streaming unbounded"). max_bytes used to +// short-circuit in run() with an error instead, which is why changelist 118 on +// project 108 - 503 files, an ordinary asset reorganisation for a game studio - +// never synced at all rather than syncing partially. +func describeFromRecord(r map[string]any, change int64, maxFiles, maxBytes int) (Describe, bool) { out := Describe{ Change: change, User: str(r, "user"), @@ -98,30 +118,70 @@ func (p *Perforce) describe(ctx context.Context, change int64, maxFiles, maxByte Description: str(r, "desc"), Status: str(r, "status"), } + // p4 -Mj returns indexed keys: depotFile0, action0, type0, rev0, ... + // + // depotFileCount is the changelist's REAL file count, counted before either + // bound is applied. Without it a truncated record cannot be told from a + // complete one by size alone, and the consumer has no way to know how much + // it is missing - FileCount below is only what was returned. truncated := false + budget := maxBytes + if budget <= 0 { + budget = DefaultDescribeMaxBytes + } + + depotFileCount := 0 for i := 0; ; i++ { df := str(r, "depotFile"+strconv.Itoa(i)) if df == "" { break } + depotFileCount++ + + if truncated { + continue // keep counting; stop collecting + } if len(out.Files) >= maxFiles { truncated = true - break + continue } - out.Files = append(out.Files, DescribedFile{ + + file := DescribedFile{ DepotFile: df, Action: str(r, "action"+strconv.Itoa(i)), Type: str(r, "type"+strconv.Itoa(i)), Rev: str(r, "rev"+strconv.Itoa(i)), - }) + } + // Charged against the budget before appending, so the bound holds + // rather than being discovered one record after it was crossed. + cost := file.approxJSONBytes() + if cost > budget { + truncated = true + continue + } + budget -= cost + + out.Files = append(out.Files, file) } out.FileCount = len(out.Files) - return out, n, truncated, nil + out.DepotFileCount = depotFileCount + return out, truncated +} + +// approxJSONBytes estimates what this record costs in the encoded response. +// An estimate is the right tool: the exact cost depends on the encoder's +// escaping, and re-marshalling every record to find out would make the bound +// more expensive than the work it bounds. It counts the field values plus a +// fixed allowance for the keys, braces, quotes and commas, and it rounds UP, +// so the real response is never larger than the budget implies. +func (f DescribedFile) approxJSONBytes() int { + const envelope = 64 // {"depot_file":"","action":"","type":"","rev":""}, + return envelope + len(f.DepotFile) + len(f.Action) + len(f.Type) + len(f.Rev) } func (p *Perforce) changes(ctx context.Context, path string, max, maxBytes int) (any, int, bool, error) { - recs, n, err := p.run(ctx, maxBytes, "changes", "-m", strconv.Itoa(max), path) + recs, n, err := p.run(ctx, "changes", "-m", strconv.Itoa(max), path) if err != nil { return nil, n, false, err } @@ -138,9 +198,60 @@ func (p *Perforce) changes(ctx context.Context, path string, max, maxBytes int) return out, n, false, nil } +// Size bounds. +// +// These used to be metadata-sized (64 KiB, roughly 500 file records) and they +// decided which changelists were allowed to be CORRECT rather than protecting +// anything: ButterStack already stores the full file list for every changelist +// under the limit, so the cap never stopped the data existing in the product. +// An asset reorganisation of a few hundred files is ordinary for a game studio +// and is exactly what this daemon exists to observe. +// +// So the defaults are now sized for the work, and the true ceiling is a +// separate, much larger memory bound. +const ( + // DefaultDescribeMaxBytes is the response budget for p4.describe when the + // caller names none. ~4 MiB is roughly 30,000 file records: past any real + // changelist, well short of a memory problem. + DefaultDescribeMaxBytes = 4 << 20 + + // MaxToolOutputBytes is how much raw p4 output the daemon will hold for a + // single command, so a pathological changelist cannot exhaust it. This is + // the sanity bound; it is not a response-shaping knob and no caller can + // raise it. + MaxToolOutputBytes = 64 << 20 +) + +// cappedBuffer is a bytes.Buffer that stops growing past limit and records +// that it did. Writing into a plain buffer and checking its length afterwards +// would already have allocated whatever p4 produced, which is the allocation +// the ceiling exists to prevent. +type cappedBuffer struct { + buf bytes.Buffer + limit int + overflowed bool +} + +func (c *cappedBuffer) Write(p []byte) (int, error) { + if c.overflowed { + return len(p), nil // absorb the rest; the command is already doomed + } + if remaining := c.limit - c.buf.Len(); len(p) > remaining { + c.overflowed = true + if remaining > 0 { + c.buf.Write(p[:remaining]) + } + return len(p), nil + } + return c.buf.Write(p) +} + +func (c *cappedBuffer) Bytes() []byte { return c.buf.Bytes() } +func (c *cappedBuffer) Len() int { return c.buf.Len() } + // run invokes p4 with -Mj (one JSON object per record) and returns the parsed // records. args is appended to a fixed prefix; nothing in args is interpreted. -func (p *Perforce) run(ctx context.Context, maxBytes int, args ...string) ([]map[string]any, int, error) { +func (p *Perforce) run(ctx context.Context, args ...string) ([]map[string]any, int, error) { argv := append([]string{ "-p", p.cfg.Port, "-u", p.cfg.User, @@ -152,18 +263,34 @@ func (p *Perforce) run(ctx context.Context, maxBytes int, args ...string) ([]map cmd := exec.CommandContext(ctx, p.cfg.Binary, argv...) // argv array; no shell cmd.Env = p.env() - var stdout, stderr bytes.Buffer - cmd.Stdout, cmd.Stderr = &stdout, &stderr + // The per-verb max_bytes budget is NOT enforced here. It is a response- + // shaping bound and belongs where records are assembled, so a large + // changelist truncates with truncated: true rather than failing outright. + // What is enforced here is a memory ceiling: a bound on what the daemon is + // willing to hold, not on what the caller asked for. It is measured in + // megabytes and a changelist that trips it is genuinely pathological. + stdout := &cappedBuffer{limit: MaxToolOutputBytes} + var stderr bytes.Buffer + cmd.Stdout, cmd.Stderr = stdout, &stderr if err := cmd.Run(); err != nil { return nil, stdout.Len(), fmt.Errorf("perforce: %s", failureDetail(stdout.Bytes(), stderr.String(), err)) } - if maxBytes > 0 && stdout.Len() > maxBytes { - return nil, stdout.Len(), fmt.Errorf("perforce: response exceeded max_bytes") + if stdout.overflowed { + return nil, stdout.Len(), fmt.Errorf("perforce: output exceeded the %d-byte tool ceiling", MaxToolOutputBytes) } + // Decode from a reader over the bytes rather than from the buffer itself. + // Decoding CONSUMES a bytes.Buffer, so the previous `json.NewDecoder(&stdout)` + // left it empty and the `stdout.Len()` reported below was therefore always + // 0 - every successful command reported `bytes: 0` to the broker and into + // the audit log. Found while removing the max_bytes hard-fail above, which + // was the only caller that read the length before the decode drained it. + raw := stdout.Bytes() + n := len(raw) + var recs []map[string]any - dec := json.NewDecoder(&stdout) + dec := json.NewDecoder(bytes.NewReader(raw)) for { var m map[string]any if err := dec.Decode(&m); err != nil { @@ -171,7 +298,7 @@ func (p *Perforce) run(ctx context.Context, maxBytes int, args ...string) ([]map } recs = append(recs, m) } - return recs, stdout.Len(), nil + return recs, n, nil } // env builds a minimal environment. The ticket goes in P4PASSWD rather than on diff --git a/internal/tools/perforce_describe_test.go b/internal/tools/perforce_describe_test.go new file mode 100644 index 0000000..20c5221 --- /dev/null +++ b/internal/tools/perforce_describe_test.go @@ -0,0 +1,147 @@ +package tools + +import ( + "strconv" + "testing" +) + +// #1904: max_bytes used to short-circuit with an error instead of truncating, +// contradicting PROTOCOL.md ("the connector truncates and sets truncated: true +// rather than streaming unbounded"). That is why changelist 118 on project 108 +// - 503 files, an ordinary asset reorganisation - never synced at all rather +// than syncing partially. +// +// describe() reads a p4 -Mj record, so these drive it through that record shape +// rather than through a live p4. +func describeRecord(fileCount int) map[string]any { + r := map[string]any{ + "change": "118", + "user": "ryan", + "client": "bsg-cp-01", + "time": "1757260800", + "desc": "audio stack, asset reorg", + "status": "submitted", + } + for i := 0; i < fileCount; i++ { + r["depotFile"+strconv.Itoa(i)] = "//pilot_light/art/texture_" + strconv.Itoa(i) + ".png" + r["action"+strconv.Itoa(i)] = "add" + r["type"+strconv.Itoa(i)] = "binary" + r["rev"+strconv.Itoa(i)] = "1" + } + return r +} + +func collect(t *testing.T, fileCount, maxFiles, maxBytes int) (Describe, bool) { + t.Helper() + out, truncated := describeFromRecord(describeRecord(fileCount), 118, maxFiles, maxBytes) + return out, truncated +} + +func TestDescribeTruncatesOnMaxFiles(t *testing.T) { + out, truncated := collect(t, 503, 200, DefaultDescribeMaxBytes) + + if !truncated { + t.Fatal("a file list cut short by max_files must report truncated") + } + if out.FileCount != 200 { + t.Errorf("FileCount = %d, want 200 (what was returned)", out.FileCount) + } + if out.DepotFileCount != 503 { + t.Errorf("DepotFileCount = %d, want 503 (what exists) - without this a consumer cannot tell how much is missing", out.DepotFileCount) + } +} + +// The behaviour change this issue is about: a byte budget shapes the response, +// it does not fail the command. +func TestDescribeTruncatesOnMaxBytesRatherThanFailing(t *testing.T) { + out, truncated := collect(t, 503, 100000, 4096) + + if !truncated { + t.Fatal("a file list cut short by max_bytes must report truncated, not error") + } + if out.FileCount == 0 { + t.Fatal("a tight byte budget must still return the records that fit, not nothing") + } + if out.FileCount >= 503 { + t.Fatalf("FileCount = %d: the byte budget was not applied", out.FileCount) + } + if out.DepotFileCount != 503 { + t.Errorf("DepotFileCount = %d, want 503", out.DepotFileCount) + } +} + +// Changelist 118's own shape. Under the old 65536 cap this was the failing +// case; it must now come back whole. +func TestDescribeReturnsAFiveHundredFileChangelistWhole(t *testing.T) { + out, truncated := collect(t, 503, 100000, DefaultDescribeMaxBytes) + + if truncated { + t.Error("503 files is an ordinary changelist and must not truncate at the default budget") + } + if out.FileCount != 503 { + t.Errorf("FileCount = %d, want 503", out.FileCount) + } + if out.DepotFileCount != out.FileCount { + t.Errorf("a complete record must have DepotFileCount == FileCount, got %d vs %d", out.DepotFileCount, out.FileCount) + } +} + +func TestDescribeCompleteListIsNotMarkedTruncated(t *testing.T) { + out, truncated := collect(t, 12, 200, DefaultDescribeMaxBytes) + + if truncated { + t.Error("a list under both bounds must not be marked truncated") + } + if out.FileCount != 12 || out.DepotFileCount != 12 { + t.Errorf("FileCount/DepotFileCount = %d/%d, want 12/12", out.FileCount, out.DepotFileCount) + } +} + +// The budget must hold, not be discovered one record after it was crossed. +func TestDescribeStaysWithinItsByteBudget(t *testing.T) { + const budget = 8192 + out, _ := collect(t, 503, 100000, budget) + + total := 0 + for _, f := range out.Files { + total += f.approxJSONBytes() + } + if total > budget { + t.Errorf("returned records cost %d bytes against a %d budget", total, budget) + } +} + +// A zero budget means "no caller preference", not "return nothing". +func TestDescribeZeroMaxBytesUsesTheDefault(t *testing.T) { + out, truncated := collect(t, 503, 100000, 0) + + if truncated || out.FileCount != 503 { + t.Errorf("max_bytes=0 should fall back to the default budget; got truncated=%v FileCount=%d", truncated, out.FileCount) + } +} + +func TestCappedBufferStopsAtItsLimit(t *testing.T) { + b := &cappedBuffer{limit: 10} + + n, err := b.Write([]byte("12345")) + if n != 5 || err != nil || b.overflowed { + t.Fatalf("under the limit: n=%d err=%v overflowed=%v", n, err, b.overflowed) + } + + // Writes past the limit are absorbed, not errored: the command is already + // doomed and returning an error here would surface as a write failure + // rather than as the size problem it is. + n, err = b.Write([]byte("678901234567890")) + if err != nil { + t.Fatalf("a write past the limit must be absorbed, got %v", err) + } + if n != 15 { + t.Errorf("Write must report the full length it was given, got %d", n) + } + if !b.overflowed { + t.Error("overflow must be recorded") + } + if b.Len() != 10 { + t.Errorf("buffer grew to %d, past its %d limit - the allocation the ceiling exists to prevent", b.Len(), 10) + } +} diff --git a/internal/vocab/vocab.go b/internal/vocab/vocab.go index 1ff5178..fbd2a41 100644 --- a/internal/vocab/vocab.go +++ b/internal/vocab/vocab.go @@ -241,21 +241,31 @@ var Vocabulary = []Verb{ // ---- Perforce ----------------------------------------------------------- { + // Sizing (#1904). The previous 65536/1000 pair was metadata-sized and + // decided which changelists were allowed to be CORRECT: every describe + // in production silently stopped at 200 files, and a 503-file asset + // reorganisation - ordinary for a game studio, and exactly what this + // daemon exists to observe - failed outright on max_bytes. p4.describe + // carries NO file contents (-s), so a larger bound exposes nothing a + // smaller one was protecting; it only lets the record be complete. Name: "p4.describe", Class: ClassPaths, Compiled: true, Tool: "perforce", - DefaultMaxBytes: 65536, + DefaultMaxBytes: 4 << 20, Args: []Arg{ {Name: "change", Kind: KindInt, Required: true, Min: 1, Max: 1 << 40, Doc: "changelist number, integer-validated before the p4 call"}, - {Name: "max_files", Kind: KindInt, Min: 1, Max: 1000, - Doc: "cap on the returned file list"}, + {Name: "max_files", Kind: KindInt, Min: 1, Max: 100000, + Doc: "cap on the returned file list; the response truncates and sets truncated:true rather than failing"}, {Name: "include_diff", Kind: KindBool, MustBeFalse: true, Doc: "content class; not available in v0 at any config setting"}, }, Doc: "p4 describe -s , invoked as an argv array with no shell", }, { + // p4.changes returns one summary per change (no file lists), so its + // bound is reached far later than p4.describe's. Raised on the same + // principle rather than left as the only metadata-sized P-class cap. Name: "p4.changes", Class: ClassPaths, Compiled: true, Tool: "perforce", - DefaultMaxBytes: 65536, + DefaultMaxBytes: 1 << 20, Args: []Arg{ {Name: "path", Kind: KindDepotPath, Required: true, Scope: ScopeDepot, MaxLen: 1024, Doc: "depot path; prefix-matched against depot_scope, no wildcard above the prefix"}, diff --git a/internal/vocab/vocab_test.go b/internal/vocab/vocab_test.go index 1a2755d..fefcd9f 100644 --- a/internal/vocab/vocab_test.go +++ b/internal/vocab/vocab_test.go @@ -129,7 +129,10 @@ func TestArgumentTypeValidationAtTheFrameBoundary(t *testing.T) { {"teamcity.build.get", `{"build_id":0}`, ReasonArgumentRange}, {"teamcity.build.get", `{}`, ReasonMissingArgument}, {"teamcity.build.get", `[1,2]`, ReasonMalformedArgs}, - {"p4.describe", `{"change":7,"max_files":100000}`, ReasonArgumentRange}, + // The bound moved from 1000 to 100000 in #1904; this still asserts that + // a bound EXISTS and is enforced at the frame boundary, which is what + // the case is for. + {"p4.describe", `{"change":7,"max_files":100001}`, ReasonArgumentRange}, {"p4.changes", `{"path":"//depot/game/...","max":9999}`, ReasonArgumentRange}, {"p4.changes", `{"path":42}`, ReasonArgumentType}, }