← back to Cli Printing Press
fix(cli): isolate typed-table upsert failures with per-item SAVEPOINT (#1402)
89f38c4ca977c63f5b3cc43ad41627d4f2467ff5 · 2026-05-14 12:22:30 -0700 · Trevin Chow
* fix(cli): isolate typed-table upsert failures with per-item SAVEPOINT
UpsertBatch wrapped the generic-resources insert and the typed-table
dispatch in a single transaction. Any typed-table error (e.g. a typed
column declared NOT NULL but never populated by the generator) short-
circuited the whole batch, so every successful API fetch in that page
was silently rolled back along with the failing typed row.
Wrap the typed dispatch in a per-item SAVEPOINT inside the outer tx so
a typed-upsert error rolls back only the typed projection; the generic
resources row inserted just above it survives. Aggregate failures into
a single stderr warning per batch instead of aborting.
Gated on whether the spec emits any typed tables: CLIs without typed
tables skip the SAVEPOINT/RELEASE overhead per row and keep the prior
fast path.
Closes #1392
* fix(cli): return running stored count on generic-upsert error
Pre-existing edge case made more visible by the savepoint refactor:
when upsertGenericResourceTx fails on the Nth item after N-1 items
already succeeded, the early return discarded the running stored
counter and returned zero. Callers inspecting partial progress on
error saw a misleading zero instead of what landed.
Return the running count so the failure surface matches the
established stored++-after-success semantics.
Files touched
M internal/generator/generator_test.goM internal/generator/templates/store.go.tmplM internal/generator/templates/store_upsert_batch_test.go.tmplM testdata/golden/expected/generate-sync-walker-api/sync-walker-golden/internal/store/store.go
Diff
commit 89f38c4ca977c63f5b3cc43ad41627d4f2467ff5
Author: Trevin Chow <trevin@trevinchow.com>
Date: Thu May 14 12:22:30 2026 -0700
fix(cli): isolate typed-table upsert failures with per-item SAVEPOINT (#1402)
* fix(cli): isolate typed-table upsert failures with per-item SAVEPOINT
UpsertBatch wrapped the generic-resources insert and the typed-table
dispatch in a single transaction. Any typed-table error (e.g. a typed
column declared NOT NULL but never populated by the generator) short-
circuited the whole batch, so every successful API fetch in that page
was silently rolled back along with the failing typed row.
Wrap the typed dispatch in a per-item SAVEPOINT inside the outer tx so
a typed-upsert error rolls back only the typed projection; the generic
resources row inserted just above it survives. Aggregate failures into
a single stderr warning per batch instead of aborting.
Gated on whether the spec emits any typed tables: CLIs without typed
tables skip the SAVEPOINT/RELEASE overhead per row and keep the prior
fast path.
Closes #1392
* fix(cli): return running stored count on generic-upsert error
Pre-existing edge case made more visible by the savepoint refactor:
when upsertGenericResourceTx fails on the Nth item after N-1 items
already succeeded, the early return discarded the running stored
counter and returned zero. Callers inspecting partial progress on
error saw a misleading zero instead of what landed.
Return the running count so the failure surface matches the
established stored++-after-success semantics.
---
internal/generator/generator_test.go | 20 ++++++-
internal/generator/templates/store.go.tmpl | 68 +++++++++++++++++-----
.../templates/store_upsert_batch_test.go.tmpl | 55 +++++++++++++++++
.../sync-walker-golden/internal/store/store.go | 58 ++++++++++++++----
4 files changed, 174 insertions(+), 27 deletions(-)
diff --git a/internal/generator/generator_test.go b/internal/generator/generator_test.go
index becb7d04..11b7325c 100644
--- a/internal/generator/generator_test.go
+++ b/internal/generator/generator_test.go
@@ -3070,9 +3070,23 @@ func TestGenerateStoreUpsertBatchDispatchesToTypedTable(t *testing.T) {
assert.Contains(t, src, "func (s *Store) UpsertCampaigns(", "public typed upsert missing for campaigns")
// UpsertBatch must dispatch to the typed helper inside its switch.
- assert.Regexp(t, `(?s)func \(s \*Store\) UpsertBatch\(.*case "campaigns":\s+if err := s\.upsertCampaignsTx\(`, src,
+ // The dispatch now captures the typed error so a savepoint can roll it
+ // back without unwinding the outer transaction (issue #1392); the
+ // shape went from `if err := s.upsertCampaignsTx(...)` to
+ // `typedErr = s.upsertCampaignsTx(...)`.
+ assert.Regexp(t, `(?s)func \(s \*Store\) UpsertBatch\(.*case "campaigns":\s+typedErr = s\.upsertCampaignsTx\(`, src,
"UpsertBatch must dispatch to upsertCampaignsTx — without this, paginated syncs leave typed tables empty (issue #268)")
+ // SAVEPOINT/RELEASE/ROLLBACK isolates typed-table failures so a NOT NULL
+ // constraint in a generated typed table does not unwind the outer
+ // transaction and strand the generic resources row (issue #1392).
+ assert.Contains(t, src, `tx.Exec("SAVEPOINT " + savepoint)`,
+ "UpsertBatch must open a SAVEPOINT before the typed dispatch (issue #1392)")
+ assert.Contains(t, src, `tx.Exec("ROLLBACK TO SAVEPOINT " + savepoint)`,
+ "UpsertBatch must ROLLBACK TO SAVEPOINT on typed-table failure so the generic row survives (issue #1392)")
+ assert.Contains(t, src, `tx.Exec("RELEASE SAVEPOINT " + savepoint)`,
+ "UpsertBatch must RELEASE the savepoint after either success or rollback so it is cleared from the stack (issue #1392)")
+
runGoCommand(t, outputDir, "mod", "tidy")
runGoCommand(t, outputDir, "test", "./internal/store")
}
@@ -3146,8 +3160,10 @@ func TestUpsertDispatchPreservesMultiWordResourceCasing(t *testing.T) {
// UpsertBatch must dispatch on the runtime resource form (kebab) to
// match defaultSyncResources/syncResourcePath/resourceIDFieldOverrides.
+ // The dispatch captures the typed error into a local so a savepoint
+ // can roll it back without unwinding the outer transaction (#1392).
assert.Regexp(t,
- `(?s)func \(s \*Store\) UpsertBatch\(.*case "audio-isolation":\s+if err := s\.upsertAudioIsolationTx\(`,
+ `(?s)func \(s \*Store\) UpsertBatch\(.*case "audio-isolation":\s+typedErr = s\.upsertAudioIsolationTx\(`,
store,
"UpsertBatch must dispatch on \"audio-isolation\" — the snake case string never matches the kebab runtime resource (issue #1064)")
diff --git a/internal/generator/templates/store.go.tmpl b/internal/generator/templates/store.go.tmpl
index 72e4d0ea..d2dde88a 100644
--- a/internal/generator/templates/store.go.tmpl
+++ b/internal/generator/templates/store.go.tmpl
@@ -888,18 +888,30 @@ var resourceIDFieldOverrides = map[string]string{
var genericIDFieldFallbacks = []string{"id", "ID", "name", "uuid", "slug", "key", "code", "uid"}
// UpsertBatch inserts or replaces multiple records in a single transaction
-// and returns (stored, extractFailures, err). stored counts rows actually
-// landed; extractFailures counts items that survived JSON unmarshal but had
-// no extractable primary key (templated IDField AND generic fallback both
-// missed). callers (sync.go.tmpl) compare these against len(items) to emit
-// the per-item primary_key_unresolved warning and the F4b
-// stored_count_zero_after_extraction probe.
+// and returns (stored, extractFailures, err). stored counts rows landed in
+// the generic resources table; extractFailures counts items that survived
+// JSON unmarshal but had no extractable primary key (templated IDField AND
+// generic fallback both missed). callers (sync.go.tmpl) compare these
+// against len(items) to emit the per-item primary_key_unresolved warning
+// and the F4b stored_count_zero_after_extraction probe.
//
// For resource types that have a domain-specific typed table, the per-item
// generic insert is followed by a dispatch to the matching upsert<Pascal>Tx
// inside the same transaction. Without that dispatch, paginated syncs would
// only populate the generic resources table — typed tables (and indexed
// columns like parent_id added by dependent-resource sync) would stay empty.
+//
+// Each typed-table dispatch runs inside a per-item SAVEPOINT so a constraint
+// failure in the typed insert (e.g. NOT NULL parent FK when the generator
+// didn't populate the parent path placeholder) rolls back only that typed
+// upsert. The generic resources row inserted just above it survives the
+// rollback, so successful API fetches never strand in memory because one
+// downstream typed table is misconfigured. Failures are surfaced via a
+// trailing stderr warning rather than aborting the batch.
+{{- $hasTyped := false }}
+{{- range .Tables}}
+{{- if hasTypedTable .}}{{$hasTyped = true}}{{end}}
+{{- end}}
func (s *Store) UpsertBatch(resourceType string, items []json.RawMessage) (int, int, error) {
s.writeMu.Lock()
defer s.writeMu.Unlock()
@@ -909,8 +921,8 @@ func (s *Store) UpsertBatch(resourceType string, items []json.RawMessage) (int,
}
defer tx.Rollback()
- var stored, skippedCount, extractFailures int
- for _, item := range items {
+ var stored, skippedCount, extractFailures{{if $hasTyped}}, typedFailures{{end}} int
+ for {{if $hasTyped}}i, {{else}}_, {{end}}item := range items {
var obj map[string]any
if err := json.Unmarshal(item, &obj); err != nil {
skippedCount++
@@ -947,20 +959,43 @@ func (s *Store) UpsertBatch(resourceType string, items []json.RawMessage) (int,
}
if err := s.upsertGenericResourceTx(tx, resourceType, id, item); err != nil {
- return 0, extractFailures, fmt.Errorf("upserting %s/%s: %w", resourceType, id, err)
+ // Return the running stored count rather than zero so callers
+ // inspecting partial progress on failure see what already
+ // landed in earlier loop iterations.
+ return stored, extractFailures, fmt.Errorf("upserting %s/%s: %w", resourceType, id, err)
}
+ stored++
+{{- if $hasTyped}}
+ savepoint := fmt.Sprintf("pp_typed_%d", i)
+ if _, err := tx.Exec("SAVEPOINT " + savepoint); err != nil {
+ return stored, extractFailures, fmt.Errorf("savepoint begin for %s/%s: %w", resourceType, id, err)
+ }
+
+ var typedErr error
switch resourceType {
{{- range .Tables}}
{{- if hasTypedTable .}}
case "{{.Resource}}":
- if err := s.upsert{{pascal .Name}}Tx(tx, id, obj, item); err != nil {
- return 0, extractFailures, fmt.Errorf("typed upsert for %s/%s: %w", resourceType, id, err)
- }
+ typedErr = s.upsert{{pascal .Name}}Tx(tx, id, obj, item)
{{- end}}
{{- end}}
}
- stored++
+
+ if typedErr != nil {
+ if _, rbErr := tx.Exec("ROLLBACK TO SAVEPOINT " + savepoint); rbErr != nil {
+ return stored, extractFailures, fmt.Errorf("rollback to savepoint for %s/%s (typed err: %v): %w", resourceType, id, typedErr, rbErr)
+ }
+ if _, relErr := tx.Exec("RELEASE SAVEPOINT " + savepoint); relErr != nil {
+ return stored, extractFailures, fmt.Errorf("release savepoint after rollback for %s/%s: %w", resourceType, id, relErr)
+ }
+ typedFailures++
+ continue
+ }
+ if _, err := tx.Exec("RELEASE SAVEPOINT " + savepoint); err != nil {
+ return stored, extractFailures, fmt.Errorf("release savepoint for %s/%s: %w", resourceType, id, err)
+ }
+{{- end}}
}
// Warn when most items in a batch lack an extractable ID — this likely
@@ -968,6 +1003,13 @@ func (s *Store) UpsertBatch(resourceType string, items []json.RawMessage) (int,
if skippedCount > 0 && len(items) > 0 && skippedCount*2 > len(items) {
fmt.Fprintf(os.Stderr, "warning: %d/%d %s items skipped (no extractable ID field found)\n", skippedCount, len(items), resourceType)
}
+{{- if $hasTyped}}
+ // Surface typed-table failures without aborting the batch. Generic rows
+ // already committed; only the typed projection failed.
+ if typedFailures > 0 {
+ fmt.Fprintf(os.Stderr, "warning: %d/%d %s items: typed-table upsert failed; generic resources rows preserved\n", typedFailures, len(items), resourceType)
+ }
+{{- end}}
if err := tx.Commit(); err != nil {
return 0, extractFailures, err
diff --git a/internal/generator/templates/store_upsert_batch_test.go.tmpl b/internal/generator/templates/store_upsert_batch_test.go.tmpl
index 919c4c60..a0c7dae8 100644
--- a/internal/generator/templates/store_upsert_batch_test.go.tmpl
+++ b/internal/generator/templates/store_upsert_batch_test.go.tmpl
@@ -321,6 +321,61 @@ func TestUpsertBatch_Populates{{pascal .Name}}Table(t *testing.T) {
t.Fatalf("{{.Name}} count = %d, want %d (typed table not populated by UpsertBatch)", typed, len(items))
}
}
+{{- if $parentFKCol }}
+
+// TestUpsertBatch_TypedFailureDoesNotStrand{{pascal .Name}}Generic exercises
+// the savepoint isolation around the typed-table dispatch. The fixture omits
+// the NOT NULL parent FK column so the typed insert fails; the savepoint
+// rolls back only the typed projection. The generic resources row inserted
+// just before must survive. Regression for issue #1392, where a single
+// outer transaction caused typed-table failures to cascade and silently
+// discard every successfully fetched API row.
+func TestUpsertBatch_TypedFailureDoesNotStrand{{pascal .Name}}Generic(t *testing.T) {
+ dbPath := filepath.Join(t.TempDir(), "data.db")
+ s, err := Open(dbPath)
+ if err != nil {
+ t.Fatalf("open: %v", err)
+ }
+ defer s.Close()
+
+ // Items deliberately omit "{{$parentFKCol}}" so the typed NOT NULL
+ // constraint fires.
+ items := []json.RawMessage{
+ json.RawMessage(`{"id": "orphan-001"}`),
+ json.RawMessage(`{"id": "orphan-002"}`),
+ json.RawMessage(`{"id": "orphan-003"}`),
+ }
+ stored, extractFailures, err := s.UpsertBatch("{{.Resource}}", items)
+ if err != nil {
+ t.Fatalf("UpsertBatch: %v (typed-table failure must not propagate)", err)
+ }
+ if stored != len(items) {
+ t.Fatalf("stored = %d, want %d (generic resources rows must land even when typed table fails)", stored, len(items))
+ }
+ if extractFailures != 0 {
+ t.Fatalf("extractFailures = %d, want 0", extractFailures)
+ }
+
+ db := s.DB()
+
+ var generic int
+ if err := db.QueryRow(`SELECT COUNT(*) FROM resources WHERE resource_type = ?`, "{{.Resource}}").Scan(&generic); err != nil {
+ t.Fatalf("count resources: %v", err)
+ }
+ if generic != len(items) {
+ t.Fatalf("resources count = %d, want %d (savepoint rollback must not undo generic insert)", generic, len(items))
+ }
+
+ var typed int
+ typedQuery := fmt.Sprintf(`SELECT COUNT(*) FROM "%s"`, "{{.Name}}")
+ if err := db.QueryRow(typedQuery).Scan(&typed); err != nil {
+ t.Fatalf("count {{.Name}}: %v", err)
+ }
+ if typed != 0 {
+ t.Fatalf("{{.Name}} count = %d, want 0 (typed insert violated NOT NULL on %q)", typed, "{{$parentFKCol}}")
+ }
+}
+{{- end}}
{{- if $hasParentID }}
// TestUpsertBatch_Sets{{pascal .Name}}ParentID verifies that dependent-resource
diff --git a/testdata/golden/expected/generate-sync-walker-api/sync-walker-golden/internal/store/store.go b/testdata/golden/expected/generate-sync-walker-api/sync-walker-golden/internal/store/store.go
index 993e94a6..69d7bf22 100644
--- a/testdata/golden/expected/generate-sync-walker-api/sync-walker-golden/internal/store/store.go
+++ b/testdata/golden/expected/generate-sync-walker-api/sync-walker-golden/internal/store/store.go
@@ -813,18 +813,26 @@ var resourceIDFieldOverrides = map[string]string{
var genericIDFieldFallbacks = []string{"id", "ID", "name", "uuid", "slug", "key", "code", "uid"}
// UpsertBatch inserts or replaces multiple records in a single transaction
-// and returns (stored, extractFailures, err). stored counts rows actually
-// landed; extractFailures counts items that survived JSON unmarshal but had
-// no extractable primary key (templated IDField AND generic fallback both
-// missed). callers (sync.go.tmpl) compare these against len(items) to emit
-// the per-item primary_key_unresolved warning and the F4b
-// stored_count_zero_after_extraction probe.
+// and returns (stored, extractFailures, err). stored counts rows landed in
+// the generic resources table; extractFailures counts items that survived
+// JSON unmarshal but had no extractable primary key (templated IDField AND
+// generic fallback both missed). callers (sync.go.tmpl) compare these
+// against len(items) to emit the per-item primary_key_unresolved warning
+// and the F4b stored_count_zero_after_extraction probe.
//
// For resource types that have a domain-specific typed table, the per-item
// generic insert is followed by a dispatch to the matching upsert<Pascal>Tx
// inside the same transaction. Without that dispatch, paginated syncs would
// only populate the generic resources table — typed tables (and indexed
// columns like parent_id added by dependent-resource sync) would stay empty.
+//
+// Each typed-table dispatch runs inside a per-item SAVEPOINT so a constraint
+// failure in the typed insert (e.g. NOT NULL parent FK when the generator
+// didn't populate the parent path placeholder) rolls back only that typed
+// upsert. The generic resources row inserted just above it survives the
+// rollback, so successful API fetches never strand in memory because one
+// downstream typed table is misconfigured. Failures are surfaced via a
+// trailing stderr warning rather than aborting the batch.
func (s *Store) UpsertBatch(resourceType string, items []json.RawMessage) (int, int, error) {
s.writeMu.Lock()
defer s.writeMu.Unlock()
@@ -834,8 +842,8 @@ func (s *Store) UpsertBatch(resourceType string, items []json.RawMessage) (int,
}
defer tx.Rollback()
- var stored, skippedCount, extractFailures int
- for _, item := range items {
+ var stored, skippedCount, extractFailures, typedFailures int
+ for i, item := range items {
var obj map[string]any
if err := json.Unmarshal(item, &obj); err != nil {
skippedCount++
@@ -872,16 +880,37 @@ func (s *Store) UpsertBatch(resourceType string, items []json.RawMessage) (int,
}
if err := s.upsertGenericResourceTx(tx, resourceType, id, item); err != nil {
- return 0, extractFailures, fmt.Errorf("upserting %s/%s: %w", resourceType, id, err)
+ // Return the running stored count rather than zero so callers
+ // inspecting partial progress on failure see what already
+ // landed in earlier loop iterations.
+ return stored, extractFailures, fmt.Errorf("upserting %s/%s: %w", resourceType, id, err)
+ }
+ stored++
+
+ savepoint := fmt.Sprintf("pp_typed_%d", i)
+ if _, err := tx.Exec("SAVEPOINT " + savepoint); err != nil {
+ return stored, extractFailures, fmt.Errorf("savepoint begin for %s/%s: %w", resourceType, id, err)
}
+ var typedErr error
switch resourceType {
case "leagues":
- if err := s.upsertLeaguesTx(tx, id, obj, item); err != nil {
- return 0, extractFailures, fmt.Errorf("typed upsert for %s/%s: %w", resourceType, id, err)
+ typedErr = s.upsertLeaguesTx(tx, id, obj, item)
+ }
+
+ if typedErr != nil {
+ if _, rbErr := tx.Exec("ROLLBACK TO SAVEPOINT " + savepoint); rbErr != nil {
+ return stored, extractFailures, fmt.Errorf("rollback to savepoint for %s/%s (typed err: %v): %w", resourceType, id, typedErr, rbErr)
}
+ if _, relErr := tx.Exec("RELEASE SAVEPOINT " + savepoint); relErr != nil {
+ return stored, extractFailures, fmt.Errorf("release savepoint after rollback for %s/%s: %w", resourceType, id, relErr)
+ }
+ typedFailures++
+ continue
+ }
+ if _, err := tx.Exec("RELEASE SAVEPOINT " + savepoint); err != nil {
+ return stored, extractFailures, fmt.Errorf("release savepoint for %s/%s: %w", resourceType, id, err)
}
- stored++
}
// Warn when most items in a batch lack an extractable ID — this likely
@@ -889,6 +918,11 @@ func (s *Store) UpsertBatch(resourceType string, items []json.RawMessage) (int,
if skippedCount > 0 && len(items) > 0 && skippedCount*2 > len(items) {
fmt.Fprintf(os.Stderr, "warning: %d/%d %s items skipped (no extractable ID field found)\n", skippedCount, len(items), resourceType)
}
+ // Surface typed-table failures without aborting the batch. Generic rows
+ // already committed; only the typed projection failed.
+ if typedFailures > 0 {
+ fmt.Fprintf(os.Stderr, "warning: %d/%d %s items: typed-table upsert failed; generic resources rows preserved\n", typedFailures, len(items), resourceType)
+ }
if err := tx.Commit(); err != nil {
return 0, extractFailures, err
← 2698690e fix(skills): emit PRINTING_PRESS_BIN marker so later phases
·
back to Cli Printing Press
·
fix(cli): make cookie-auth login --chrome work on Windows (# fdca9f7a →