G0 D3: catalogd Parquet manifests + ADR-020 idempotent register · 6 scrum fixes
Phase G0 Day 3 ships catalogd: Arrow Parquet manifest codec, in-memory registry with the ADR-020 idempotency contract (same name+fingerprint reuses dataset_id; different fingerprint → 409 Conflict), HTTP client to storaged for persistence, and rehydration on startup. Acceptance smoke 6/6 PASSES end-to-end including rehydrate-across-restart — the load-bearing test that the catalog/storaged service split actually preserves state. dataset_id derivation diverges from Rust: UUIDv5(namespace, name) instead of v4 surrogate. Same name on any box generates the same dataset_id; rehydrate after disk loss converges to the same identity rather than silently re-issuing. Namespace pinned at a8f3c1d2-4e5b-5a6c-9d8e-7f0a1b2c3d4e — every dataset_id ever issued depends on these bytes. Cross-lineage scrum on shipped code: - Opus 4.7 (opencode): 1 BLOCK + 5 WARN + 3 INFO - Kimi K2-0905 (openrouter, validated D2): 2 BLOCK + 2 WARN + 1 INFO - Qwen3-coder (openrouter): 2 BLOCK + 2 WARN + 2 INFO Fixed: C1 list-offsets BLOCK (3-way convergent) → ValueOffsets(0) + bounds C2 Rehydrate mutex held across I/O → swap-under-brief-lock pattern S1 split-brain on persist failure → candidate-then-swap S2 brittle string-match for 400 vs 500 → ErrEmptyName/ErrEmptyFingerprint sentinels S3 Get/List shallow-copy aliasing → cloneManifest deep copy S4 keep-alive socket leak on error paths → drainAndClose helper Dismissed (false positives, all single-reviewer): Kimi BLOCK "Decode crashes on empty Parquet" — already handled Kimi INFO "safeKey double-escapes" — wrong, splitting before escape is required Qwen INFO "rb.NewRecord() error unchecked" — API returns no error Deferred to G1+: name validation regex, per-call deadlines, Snappy compression, list pagination continuation tokens (storaged caps at 10k with sentinel for now). Build clean, vet clean, all tests pass, smoke 6/6 PASS after every fix round. arrow-go/v18 + google/uuid added; Go 1.24 → 1.25 forced by arrow-go's minimum. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
root
@@ -557,3 +557,127 @@ its D1 contribution where it caught two real BLOCKs that Opus missed.
|
||||
Lineage rotation is real; on a given PR, one lineage may be the only
|
||||
one finding bugs and another may be confidently wrong. The convergence
|
||||
filter (≥2 reviewers) is the right gate.
|
||||
|
||||
---
|
||||
|
||||
## D3 — actual run results (2026-04-28 evening)
|
||||
|
||||
Phase G0 Day 3 executed end-to-end. Output of `scripts/d3_smoke.sh`:
|
||||
|
||||
```
|
||||
[d3-smoke] POST /catalog/register (fresh):
|
||||
✓ fresh register → existing=false, dataset_id=200a05a8-...
|
||||
[d3-smoke] GET /catalog/manifest/<name>:
|
||||
✓ manifest dataset_id matches
|
||||
[d3-smoke] GET /catalog/list (1 entry):
|
||||
✓ list count=1
|
||||
[d3-smoke] restart catalogd → rehydrate from Parquet:
|
||||
✓ rehydrated dataset_id matches across restart
|
||||
[d3-smoke] re-register (same name + same fingerprint) → existing=true:
|
||||
✓ existing=true, same dataset_id, objects replaced (count=2)
|
||||
[d3-smoke] re-register (different fingerprint) → 409:
|
||||
✓ different fingerprint → 409 Conflict
|
||||
[d3-smoke] D3 acceptance gate: PASSED
|
||||
```
|
||||
|
||||
What landed:
|
||||
- `internal/catalogd/manifest.go` — Arrow Parquet codec for the
|
||||
Manifest type (dataset_id/name/schema_fingerprint/objects-as-list-of-
|
||||
struct/created_at_ns/updated_at_ns/row_count). One row per Parquet
|
||||
file. `DatasetIDForName` derives a deterministic UUIDv5 from name
|
||||
(namespace `a8f3c1d2-4e5b-5a6c-9d8e-7f0a1b2c3d4e`); same name on
|
||||
any box yields the same dataset_id.
|
||||
- `internal/catalogd/registry.go` — in-memory `map[name]*Manifest`
|
||||
with the ADR-020 contract: same name+fingerprint reuses
|
||||
dataset_id, replaces objects, bumps `updated_at`; different
|
||||
fingerprint → `ErrFingerprintConflict`. Single mutex serializes
|
||||
Register across persistence to close the check→insert TOCTOU.
|
||||
Rehydrate runs storaged I/O OUTSIDE the lock, swaps in the new
|
||||
map under the lock.
|
||||
- `internal/catalogd/store_client.go` — HTTP client over
|
||||
storaged's GET/PUT/DELETE/list. `safeKey` URL-escapes path
|
||||
segments while preserving `/`. Error paths drain body before
|
||||
close to keep the keep-alive pool healthy.
|
||||
- `cmd/catalogd/main.go` — POST /catalog/register, GET
|
||||
/catalog/manifest/{name}, GET /catalog/list. Sentinel-error
|
||||
detection (`errors.Is(err, ErrEmptyName)`) for 400s. 4 MiB body
|
||||
cap on register payloads.
|
||||
- New `[catalogd]` config: `bind` + `storaged_url`. Default
|
||||
`http://127.0.0.1:3211`.
|
||||
- `scripts/d3_smoke.sh` — 6 acceptance probes including
|
||||
rehydrate-across-restart (the load-bearing ADR-020 contract test).
|
||||
- `arrow-go/v18` v18.6.0 + `google/uuid` v1.6.0 added; Go 1.24 → 1.25
|
||||
forced by arrow-go's minimum.
|
||||
|
||||
UUIDv5 vs Rust v4: the Go rewrite picks deterministic-from-name. Same
|
||||
dataset name on any box converges to the same dataset_id; rehydrate
|
||||
after disk loss can't silently issue new IDs and break downstream
|
||||
cross-references. Rust's surrogate-UUIDv4 is what the prior system
|
||||
used; the divergence is intentional and documented at
|
||||
`internal/catalogd/manifest.go:34`.
|
||||
|
||||
Next: D4 — ingestd CSV → Parquet → catalogd register loop.
|
||||
|
||||
---
|
||||
|
||||
## D3 — code scrum review (3-lineage parallel pass)
|
||||
|
||||
After D3 shipped, the actual code went through the same 3-lineage
|
||||
parallel scrum as D1/D2.
|
||||
|
||||
| Pass | Reviewer | Latency | Findings |
|
||||
|---|---|---|---|
|
||||
| 1 | `opencode/claude-opus-4-7` | ~30s | 1 BLOCK + 5 WARN + 3 INFO = 9 |
|
||||
| 2 | `openrouter/moonshotai/kimi-k2-0905` | ~30s | 2 BLOCK + 2 WARN + 1 INFO = 5 |
|
||||
| 3 | `openrouter/qwen/qwen3-coder` | ~25s | 2 BLOCK + 2 WARN + 2 INFO = 6 |
|
||||
|
||||
Total: 20 distinct findings (some convergent across reviewers).
|
||||
Kimi route shopping from D2 paid off — `openrouter/moonshotai/kimi-k2-0905`
|
||||
delivered structured output on first attempt, no max_tokens cap, no
|
||||
empty-content reasoning trap.
|
||||
|
||||
### Convergent findings (≥2 reviewers — high confidence)
|
||||
|
||||
| # | Severity | Finding | Reviewers | Disposition |
|
||||
|---|---|---|---|---|
|
||||
| C1 | BLOCK×3 | `Decode` indexes `listArr.Offsets()[0]/[1]` directly — fragile under array slicing/non-zero offset; panics on malformed Parquet (multi-row reader chunks, single-offset corrupt files) | Opus + Kimi + Qwen | **Fixed** — switched to `listArr.ValueOffsets(0)` (Arrow API that accounts for array offset) + bounds check on `start/end` against `structArr.Len()` |
|
||||
| C2 | WARN×2 | `Rehydrate` holds the registry mutex across network I/O to storaged — slow storaged blocks all `Register/Get/List`; future re-sync endpoints stall | Opus + Kimi | **Fixed** — list/get/decode happen outside the lock; the completed map is swapped into `r.byKey` under a brief lock |
|
||||
|
||||
### Single-reviewer findings (lineage-specific catches)
|
||||
|
||||
| # | Severity | Finding | Reviewer | Disposition |
|
||||
|---|---|---|---|---|
|
||||
| S1 | WARN | Idempotent re-register mutates the in-memory manifest BEFORE persist succeeds — split-brain on storaged failure (in-memory advances, disk holds old, restart silently rolls back) | Opus | **Fixed** — build candidate copy of prior, persist candidate, swap into `r.byKey` only on persist success |
|
||||
| S2 | WARN | `cmd/catalogd` detects empty-input via `strings.Contains(err.Error(), "empty name")` — brittle, anyone reformatting the registry error silently demotes a 400 to a 500 | Opus | **Fixed** — exported `ErrEmptyName` / `ErrEmptyFingerprint` sentinels, HTTP layer uses `errors.Is` |
|
||||
| S3 | WARN | `Get/List` `cp := *m` aliases the `Objects` slice + `RowCount` pointer — caller mutation corrupts registry state under the lock-free read | Opus | **Fixed** — `cloneManifest` deep-copies Objects + dereferences RowCount into a fresh `*int64` |
|
||||
| S4 | WARN | error paths in `store_client` preview body but don't drain before close → HTTP/1.1 keep-alive pool reuse breaks → slow socket leak | Qwen | **Fixed** — `drainAndClose` helper reads up to 64 KiB before close on every defer |
|
||||
| S5 | INFO×3 | name-validation regex / per-call deadline / Snappy compression on manifest writes | Opus | **Deferred** — small, no risk if skipped |
|
||||
| S6 | WARN | `Rehydrate` aborts on first decode error → partial state | Kimi | **Accepted** — fail-loud > silent-partial is the design; cmd/catalogd `os.Exit(1)`s on rehydrate error so a corrupt manifest is operator-visible at startup |
|
||||
| S7 | WARN | `store_client.List` ignores pagination — if storaged gains continuation tokens, the client silently drops everything past page 1 | Opus | **Accepted** — storaged caps lists at MaxListResults=10_000 with a `...truncated...` sentinel; no continuation token in the wire format yet (deferred to D7.5+ alongside multi-bucket federation) |
|
||||
| F1 | BLOCK | `Decode` crashes on empty Parquet (NumRows==0) — `Value(0)` panics | Kimi | **Dismissed** — already handled. `tbl.NumRows() != 1` returns error before any column access; NumRows==0 also fails the subsequent `rr.Next()` check |
|
||||
| F2 | INFO | `safeKey` double-escapes slashes; should use `url.PathEscape(key)` directly | Kimi | **Dismissed** — `url.PathEscape("a/b")` returns `"a%2Fb"`. Splitting on `/` first is **necessary** to preserve the path separator while escaping segment content. The current code is correct; Kimi's suggested fix would break storaged's chi wildcard match |
|
||||
| F3 | INFO | `rb.NewRecord()` error unchecked | Qwen | **Dismissed** — false signature. `RecordBuilder.NewRecord()` returns `arrow.Record` only; no error to check |
|
||||
|
||||
### Cumulative D3 disposition
|
||||
|
||||
- **3-lineage parallel pass: 20 distinct findings**
|
||||
- **Fixed: 6** (2 convergent + 4 single-reviewer)
|
||||
- **Accepted-with-rationale: 5** (3 INFO + 2 WARN with deferred-by-design rationale)
|
||||
- **Dismissed (false positives): 3** (1 Kimi BLOCK on already-handled empty case, 1 Kimi INFO on safeKey, 1 Qwen INFO on Arrow API signature)
|
||||
- Build clean, vet clean, all tests pass, smoke 6/6 PASS after every fix round.
|
||||
|
||||
The C1 triple-confirmation (Opus + Kimi + Qwen all flagging the same
|
||||
list-offset indexing) is the strongest convergence signal across the
|
||||
three days. All three correctly diagnosed the underlying issue
|
||||
(direct `Offsets()` indexing is fragile under array slicing); Opus
|
||||
named the right Arrow API (`ValueOffsets(0)`), Kimi and Qwen named
|
||||
the bounds-check shape. Fix uses Opus's API + the others' bounds
|
||||
discipline together.
|
||||
|
||||
S1 (split-brain on persist failure) was the most *consequential*
|
||||
finding — D3's smoke would have stayed green through every test case
|
||||
because none of them simulate storaged-down mid-register. Opus alone
|
||||
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.
|
||||
|
||||
Reference in New Issue
Block a user