G0 D4: ingestd CSV → Parquet → catalogd register · 2 scrum fixes

Phase G0 Day 4 ships ingestd: multipart CSV upload, Arrow schema
inference per ADR-010 (default-to-string on ambiguity), single-pass
streaming CSV → Parquet via pqarrow batched writer (Snappy compressed,
8192 rows per batch), PUT to storaged at content-addressed key
datasets/<name>/<fp_hex>.parquet, register manifest with catalogd.
Acceptance smoke 6/6 PASS including idempotent re-ingest (proves
inference is deterministic — same CSV always produces same fingerprint)
and schema-drift → 409 (proves catalogd's gate fires on ingest traffic).

Schema fingerprint is SHA-256 over (name, type) tuples in header order
using ASCII record/unit separators (0x1e/0x1f) so column names with
commas can't collide. Nullability intentionally NOT in the fingerprint
— a column gaining nulls isn't a schema change.

Cross-lineage scrum on shipped code:
  - Opus 4.7 (opencode):                       4 WARN + 3 INFO (after 2 self-retracted BLOCKs)
  - Kimi K2-0905 (openrouter):                 1 BLOCK + 2 WARN + 1 INFO
  - Qwen3-coder (openrouter):                  2 BLOCK + 2 WARN + 2 INFO

Fixed (2, both Opus single-reviewer):
  C-DRIFT: PUT-then-register on fixed datasets/<name>/data.parquet
    meant a schema-drift ingest overwrote the live parquet BEFORE
    catalogd's 409 fired → storaged inconsistent with manifest.
    Fix: content-addressed key datasets/<name>/<fp_hex>.parquet.
    Drift writes to a different file (orphan in G2 GC scope); the
    live data is never corrupted.
  C-WCLOSE: pqarrow.NewFileWriter not Closed on error paths leaks
    buffered column data + OS resources per failed ingest.
    Fix: deferred guarded close with wClosed flag.

Dismissed (5, all false positives):
  Qwen BLOCK "csv.Reader needs LazyQuotes=true for multi-line" — false,
    Go csv handles RFC 4180 multi-line quoted fields by default
  Qwen BLOCK "row[i] OOB" — already bounds-checked at schema.go:73
    and csv.go:201
  Kimi BLOCK "type assertion panic if pqarrow reorders fields" —
    speculative, no real path
  Kimi WARN + Qwen WARN×2 "RecordBuilder leak on early error" —
    false convergent. Outer defer rb.Release() captures the current
    builder; in-loop release runs before reassignment. No leak.

Deferred (6 INFO + accepted-with-rationale on 3 WARN): sample
boundary type mismatch (G0 cap bounds peak), string-match
paranoia on http.MaxBytesError, multipart double-buffer (G2 spool-
to-disk), separator validation, body close ordering, etc.

The D4 scrum produced fewer real findings than D3 (2 vs 6) — both
were architectural hazards smoke wouldn't catch because the smoke's
"schema drift → 409" assertion was passing even in the corrupted-
state world. The 409 fires correctly; what was wrong was the PUT
having already mutated the live parquet before the validation check.
Opus's PUT-then-register read of the order is exactly the kind of
architectural insight the cross-lineage scrum is designed to surface.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

docs/PHASE_G0_KICKOFF.md

@@ -681,3 +681,147 @@ caught it; the fix is a 4-line candidate-then-swap pattern but the
 class of bug (in-memory advances ahead of disk) is exactly the
 distributed-systems hazard ADR-020 was added to prevent at a
 different layer.
+
+---
+
+## D4 — actual run results (2026-04-28 evening)
+
+Phase G0 Day 4 executed end-to-end. Output of `scripts/d4_smoke.sh`:
+
+```
+[d4-smoke] POST /ingest?name=d4_workers (5 rows, 5 cols):
+  ✓ ingest fresh → row_count=5, existing=false, key=datasets/d4_workers/247165...parquet
+[d4-smoke] mc shows the parquet on MinIO:
+  ✓ <fp>.parquet present in lakehouse-go-primary/datasets/d4_workers/
+[d4-smoke] catalogd manifest matches:
+  ✓ manifest row_count=5, fp matches, 1 object at datasets/d4_workers/<fp>.parquet
+[d4-smoke] re-ingest same CSV → existing=true:
+  ✓ idempotent re-ingest: existing=true, same dataset_id, same fingerprint
+[d4-smoke] schema-drift CSV → 409:
+  ✓ schema drift → 409 Conflict
+[d4-smoke] D4 acceptance gate: PASSED
+```
+
+What landed:
+- `internal/ingestd/schema.go` — Arrow schema inference per ADR-010
+  ("default to string on ambiguity"). Detects int64, float64, bool,
+  string. Empty cells → nullable flag. `Fingerprint()` is a
+  deterministic SHA-256 over `(name, type)` tuples in header order
+  using ASCII record/unit separators (`0x1e`/`0x1f`) so column names
+  with commas don't collide. Nullability intentionally NOT in the
+  fingerprint — gaining/losing nulls isn't a schema change.
+- `internal/ingestd/csv.go` — single-pass CSV → Arrow → Parquet.
+  Buffers the first `DefaultSampleRows=1000` rows for inference,
+  then replays them + streams the rest into the pqarrow writer in
+  `DefaultBatchSize=8192`-row record batches. Snappy compression on
+  the parquet output. Empty cells → null on numeric/bool, empty
+  string on string columns. Per scrum C-WCLOSE: deferred guarded
+  `w.Close()` so error paths flush + free buffered column data.
+- `internal/ingestd/catalog_client.go` — symmetric in shape with
+  `internal/catalogd/store_client.go`. POST /catalog/register, drain-
+  on-error keep-alive hygiene, `ErrFingerprintConflict` sentinel for
+  the 409 path.
+- `cmd/ingestd/main.go` — POST `/ingest?name=X` with multipart form.
+  Pipeline: parse multipart → `IngestCSV` → PUT parquet to storaged at
+  `datasets/<name>/<fp_hex>.parquet` (content-addressed per scrum
+  C-DRIFT) → catalogd `/catalog/register`. 256 MiB body cap matches
+  storaged's PUT cap. 5-minute upstream timeout for large ingests.
+- New `[ingestd]` config block: `bind` + `storaged_url` + `catalogd_url`
+- `scripts/d4_smoke.sh` — 6 acceptance probes. Generates a CSV that
+  exercises every inference path (clean int, string, ADR-010 fallback
+  on `salary` with one `N/A`, mixed-case bool literals, float64).
+
+Content-addressed parquet keys: `datasets/<name>/<fp_hex>.parquet`.
+Per scrum C-DRIFT (Opus WARN): the prior `data.parquet` shape meant
+a schema-drift ingest would PUT-overwrite the live parquet BEFORE
+catalogd's 409 fired, leaving storaged inconsistent with the catalog.
+Fingerprint-keyed paths mean drift attempts write to a different file
+that becomes an orphan; the live data is never corrupted. Same-fp
+re-ingest still overwrites the same key (idempotent).
+
+Next: D5 — queryd DuckDB SELECT over registered datasets.
+
+---
+
+## D4 — code scrum review (3-lineage parallel pass)
+
+| Pass | Reviewer | Latency | Findings |
+|---|---|---|---|
+| 1 | `opencode/claude-opus-4-7` | ~25s | 4 WARN + 3 INFO (+ 2 BLOCKs Opus self-retracted in-flight after re-reading) |
+| 2 | `openrouter/moonshotai/kimi-k2-0905` | ~25s | 1 BLOCK + 2 WARN + 1 INFO |
+| 3 | `openrouter/qwen/qwen3-coder` | ~30s | 2 BLOCK + 2 WARN + 2 INFO |
+
+Total: 16 distinct findings. Notably, Opus retracted two of its own
+BLOCKs after re-reading the code — the first time we've seen
+self-correction in the scrum stream. The actual reviewable output
+was 4 WARN + 3 INFO from Opus, and the analysis itself was visible.
+
+### Convergent findings (≥2 reviewers — high confidence)
+
+**No real convergent findings.** Kimi and Qwen both flagged a
+"RecordBuilder leak on early error mid-stream" but on close reading
+the existing code is correct: the deferred `rb.Release()` at the
+outer scope captures whatever value `rb` holds at function exit, and
+the in-loop `rb.Release()` runs before `rb` is reassigned to a new
+builder. No leak, regardless of where errors occur. Both reviewers
+landed on the same wrong intuition by different paths.
+
+### Single-reviewer findings — applied
+
+| # | Severity | Finding | Reviewer | Disposition |
+|---|---|---|---|---|
+| C-DRIFT | WARN | PUT-before-register on a fixed `datasets/<name>/data.parquet` key means a schema-drift ingest **overwrites the good parquet** before catalogd's 409 fires. After: storaged holds the new (wrong-schema) parquet, manifest still has the old fingerprint, queryd reads stale-schema data | Opus | **Fixed** — content-addressed key shape `datasets/<name>/<fp_hex>.parquet`. Drift writes to a different file (orphan); live parquet is never overwritten by a non-matching schema |
+| C-WCLOSE | WARN | `pqarrow.NewFileWriter` not Closed on error paths — buffered column data + OS resources leak per failed ingest | Opus | **Fixed** — `wClosed` flag + deferred guarded close. Success-path explicit Close still runs and is the only place the close error surfaces; defer fires only on error returns |
+
+### Single-reviewer findings — accepted with rationale
+
+| # | Severity | Finding | Reviewer | Disposition |
+|---|---|---|---|---|
+| O-WARN1 | WARN | Schema sample is 1000 rows; ambiguous values past row 1000 fail the entire ingest with 400 instead of widening the inference | Opus | **Accepted** — design call. G0's 256 MiB cap caps reasonable-CSVs well below the boundary. Long-term fix is two-pass infer or downgrade-to-string-on-failure; out of scope for D4 |
+| O-WARN2 | WARN | String-match on `"http: request body too large"` is paranoia; trust `errors.As` only | Opus | **Accepted** — `errors.As` IS the load-bearing check; the string-match is a documented safety net we keep across the codebase (also in storaged D2, also called out then) |
+| K-WARN2 | WARN | Multipart parse buffers entire upload, then `IngestCSV` reads it again — double in-memory cost | Kimi | **Accepted** — known G0 limitation. ParseMultipartForm with the `64<<20` threshold spills to temp file above 64 MiB so the doubling only hits below that; 256 MiB upload cap means peak is bounded |
+
+### Single-reviewer findings — deferred
+
+| # | Severity | Finding | Reviewer | Disposition |
+|---|---|---|---|---|
+| O-INFO×3 | INFO | `0x1e/0x1f` separator validation, body-close ordering, `FieldsPerRecord=-1` swallowing CSV truncation | Opus | **Deferred** — small, no risk if skipped |
+| K-INFO×1 | INFO | Fingerprint ignores nullability — already documented intentional choice | Kimi | **Deferred** |
+| Q-INFO×2 | INFO | Lowercase isBoolLiteral micro-opt; align multipart-parse limit with body cap | Qwen | **Deferred** |
+
+### Dismissed (false positives)
+
+| # | Severity | Finding | Reviewer | Why dismissed |
+|---|---|---|---|---|
+| F1 | Q-BLOCK | csv.Reader needs `LazyQuotes=true` for multi-line quoted fields | Qwen | False — `LazyQuotes` is for unescaped quotes WITHIN fields. Go's `csv.Reader` correctly handles RFC 4180 multi-line quoted fields by default; smoke would have surfaced this if true |
+| F2 | Q-BLOCK | `row[i]` OOB panic on inconsistent rows | Qwen | False — already bounds-checked at `schema.go:73` (`if i >= len(row) { continue }`) and `csv.go:201` (`if i < len(row) { cell = row[i] }`) |
+| F3 | K-BLOCK | Type assertion panic if pqarrow reorders fields | Kimi | Speculative — pqarrow doesn't reorder schema; no real path |
+| F4 | K-WARN + Q-WARN×2 | "RecordBuilder leak on early error mid-stream" (false convergent) | Kimi + Qwen | Both wrong, by different theories. The outer `defer rb.Release()` captures the current value of `rb` at function exit; the in-loop `rb.Release()` runs before reassignment. No path leaks |
+
+### Cumulative D4 disposition
+
+- **3-lineage parallel pass: 16 distinct findings**
+- **Fixed: 2** (both Opus single-reviewer; no real convergent findings this round)
+- **Accepted-with-rationale: 3**
+- **Deferred: 6** (mostly INFO)
+- **Dismissed (false positives): 5** (2 Qwen BLOCKs + 1 Kimi BLOCK + 1 false convergent leak finding)
+- Build clean, vet clean, all tests pass, smoke 6/6 PASS after every fix round.
+
+The D4 scrum produced fewer real findings than D3 (2 vs 6) — both
+real findings were Opus-only, both were WARN-level architectural
+hazards that smoke wouldn't catch (PUT-then-register order failure
+on schema drift; resource leak on writer error path). Kimi and Qwen
+delivered a false convergent finding on a non-issue, plus several
+hallucinated BLOCKs (LazyQuotes for multi-line, OOB on bounds-checked
+indexing). Opus's self-retraction of two BLOCKs in-flight is a
+healthier reviewer behavior than confidently-wrong dismissals would
+have been.
+
+The C-DRIFT fix (content-addressed parquet keys) is the kind of
+finding that's invisible to integration tests because the smoke's
+"schema drift → 409" assertion was passing in the corrupted-state
+world too — the 409 fires correctly; what was wrong was the live
+data getting overwritten before the 409 fired. Opus reading the
+PUT-then-register order and noticing that PUT mutates state before
+the validation check is exactly the kind of architectural code-read
+the scrum exists for.