Batch import/update apply partial changes when one record fails

[oritwoen]

What happened?

Noticed this while validating batch workflows for automation: both batch paths stop on the first failing record, but earlier records are already committed.

• observation.addMany inserts earlier observations before throwing on a later invalid record (src/core/observation.ts:94)
• observation.updateMany applies earlier updates before throwing on a later missing/invalid id (src/core/observation.ts:318)

That means a failed batch operation leaves the database partially changed, which is surprising for import/update pipelines that expect all-or-nothing behavior.

How to reproduce

I reproduced this with the published API from dist/index.mjs.

addMany partial commit:

node --input-type=module -e "import { mkdtempSync, rmSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { createObsxa } from './dist/index.mjs'; const dir = mkdtempSync(join(tmpdir(), 'obsxa-audit-')); const db = join(dir, 'audit.db'); const obsxa = await createObsxa({ db }); try { await obsxa.project.add({ id: 'p1', name: 'P1' }); try { await obsxa.observation.addMany([{ projectId: 'p1', title: 'ok1', source: 's1' }, { projectId: 'missing', title: 'bad', source: 's2' }, { projectId: 'p1', title: 'ok2', source: 's3' }]); } catch (e) { console.log('addMany error:', e.message); } const rows = await obsxa.observation.list('p1'); console.log('rows_in_p1', rows.length); console.log('titles', rows.map(r => r.title).join(',')); } finally { await obsxa.close(); rmSync(dir, { recursive: true, force: true }); }"

Observed output:

• addMany error: Project \"missing\" not found
• rows_in_p1 1
• titles ok1

updateMany partial commit:

node --input-type=module -e "import { mkdtempSync, rmSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { createObsxa } from './dist/index.mjs'; const dir = mkdtempSync(join(tmpdir(), 'obsxa-audit-')); const db = join(dir, 'audit.db'); const obsxa = await createObsxa({ db }); try { await obsxa.project.add({ id: 'p1', name: 'P1' }); const a = await obsxa.observation.add({ projectId: 'p1', title: 'a', source: 's1' }); const b = await obsxa.observation.add({ projectId: 'p1', title: 'b', source: 's2' }); try { await obsxa.observation.updateMany([{ id: a.id, title: 'a-updated' }, { id: 999999, title: 'missing' }, { id: b.id, title: 'b-updated' }]); } catch (e) { console.log('updateMany error:', e.message); } const rows = await obsxa.observation.list('p1'); console.log('titles_after', rows.map(r => r.id + ':' + r.title).join(',')); } finally { await obsxa.close(); rmSync(dir, { recursive: true, force: true }); }"

Observed output:

• updateMany error: Observation #999999 not found
• titles_after 1:a-updated,2:b

Anything else?

This looks fixable by wrapping each batch method in a single transaction, so either all records are applied or none are.

Related tests that currently cover happy path only: test/index.test.ts:520.

[coderabbitai]

🔗 Related PRs

#11 - refactor(db): migrate to async libsql backend [merged]
#13 - fix(cli): validate observation percentage inputs [merged]
#15 - fix(search): avoid silent fallback on non-recoverable FTS failures [closed]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

✅ Successfully generated implementation plan.

💡 Use @coderabbitai <feedback> to replan

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[coderabbitai]

Coding Plan

Summary

• Extract tx-accepting core helpers (_addCore, _updateCore, _dismissCore, _archiveCore, _promoteCore) from existing methods
• Public methods continue working unchanged by wrapping their core helper in db.transaction() (or calling directly if no tx needed)
• Batch methods wrap their loop in a single db.transaction() and call core helpers with tx, making the entire batch atomic
• Add tests that verify rollback: on failure, previously processed records are not committed

Design Choices

Design Choice 1: How should we handle methods that currently open their own transactions being called from within a batch transaction?

Options Considered:

1. Extract tx-accepting core helpers (e.g., _addCore(q, input)) that both public methods and batch methods can use
2. Inline all logic in batch methods, duplicating code like merge() does with archive
3. Use libSQL savepoints via tx.transaction() for nesting

Chosen Option: 1

Rationale: Option 1 - extracting helpers avoids duplication, creates a clean pattern, and doesn't require savepoint complexity. Public methods call helpers with db, batch methods wrap in db.transaction() and call helpers with tx.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Make Batch Operations Atomic

Refactor addMany and updateMany to run all their operations within a single transaction. This requires extracting the core logic from individual methods into helpers that accept a transaction handle.

Task 1: Extract Core Helpers for Add and Status Transitions

Create internal helpers that accept a queryable handle (db or tx) so they can participate in an outer transaction.

• Add _addCore(q, input) in src/core/observation.ts containing the project validation SELECT and observation INSERT, using the passed queryable instead of db
• Refactor add() to call _addCore(db, input) - behavior unchanged, just delegates
• Add _dismissCore(tx, id, transition) containing the dismiss logic (validation, UPDATE observations, INSERT status event)
• Add _archiveCore(tx, id, transition) and _promoteCore(tx, id, promotedTo) following the same pattern
• Refactor dismiss(), archive(), promote() to wrap db.transaction() and call their respective core helpers

Task 2: Wrap addMany in a Single Transaction

Update addMany to use the core helpers inside one outer transaction.

• Wrap the existing for...of loop body inside db.transaction(async (tx) => {...})
• Replace this.add(record) with _addCore(tx, record)
• Replace this.dismiss(...) / this.archive(...) / this.promote(...) with their core equivalents passing tx
• Return the results array from within the transaction callback

Task 3: Extract Core Helper for Update

Create an internal helper for update that accepts a transaction handle.

• Add _updateCore(tx, id, fields) in src/core/observation.ts containing the current row fetch, diff computation, UPDATE observations, and INSERT observationEdits logic
• Refactor update() to wrap db.transaction() and call _updateCore(tx, id, fields) - behavior unchanged

Task 4: Wrap updateMany in a Single Transaction

Update updateMany to use the core helper inside one outer transaction.

• Wrap the existing for...of loop body inside db.transaction(async (tx) => {...})
• Replace this.update(id, fields) with _updateCore(tx, record.id, fields)
• Return the updated array from within the transaction callback

🤖 Prompt for AI agents

Refactor `src/core/observation.ts` to extract tx-accepting core helpers so
`addMany` and `updateMany` can wrap their entire loop in a single
`db.transaction()` - this is what makes the batch operations atomic.

**Extract core helpers:**
- Add `_addCore(q, input)`: pull the project validation SELECT and observation
INSERT out of `add()`, replace `db` references with the passed queryable `q`
- Add `_dismissCore(tx, id, transition)`: pull dismiss logic (validation, UPDATE
observations, INSERT status event) out of `dismiss()`
- Add `_archiveCore(tx, id, transition)` and `_promoteCore(tx, id, promotedTo)`
following the same pattern from `archive()` and `promote()`
- Add `_updateCore(tx, id, fields)`: pull current row fetch, diff computation,
UPDATE observations, and INSERT observationEdits out of `update()`

**Refactor public methods to delegate:**
- `add()` calls `_addCore(db, input)` - no behavior change
- `dismiss()`, `archive()`, `promote()` each wrap `db.transaction()` and call
their respective core helper with `tx` - no behavior change
- `update()` wraps `db.transaction()` and calls `_updateCore(tx, id, fields)` -
no behavior change

**Refactor batch methods to use a single outer transaction:**
- `addMany`: wrap the full `for...of` loop body in `db.transaction(async (tx) =>
{...})`, replace `this.add(record)` with `_addCore(tx, record)`, replace
`this.dismiss/archive/promote(...)` with their core equivalents passing `tx`,
return the results array from inside the callback
- `updateMany`: wrap the full `for...of` loop body in `db.transaction(async (tx)
=> {...})`, replace `this.update(id, fields)` with `_updateCore(tx, record.id,
fields)`, return the updated array from inside the callback

Do NOT call `db.transaction()` inside another `db.transaction()` - that's
exactly the bug we're fixing. Core helpers must only use the passed handle,
never `db` directly.

Phase 2: Add Rollback Behavior Tests

Verify that failures mid-batch leave the database unchanged.

Task 1: Add Rollback Test for addMany

Test that a failure on record N rolls back records 1 through N-1.

• Add test in test/index.test.ts near the existing addMany test (around line 520)
• Setup: create project, then call addMany with [valid record, invalid record (bad projectId), valid record]
• Assert: call rejects with expected error
• Assert: observation.list() returns empty - the first record should NOT be committed

Task 2: Add Rollback Test for updateMany

Test that a failure on record N rolls back records 1 through N-1.

• Add test in test/index.test.ts near the existing updateMany test
• Setup: create project, add two observations, then call updateMany with [valid update for obs1, invalid update (bad id 999999), valid update for obs2]
• Assert: call rejects with expected error
• Assert: both observations retain their original titles - the first update should NOT be committed

🤖 Prompt for AI agents

Add rollback tests in `test/index.test.ts` to prove that a mid-batch failure
leaves the DB untouched - these tests should fail before the Phase 1 refactor
and pass after.

**addMany rollback test** (add near line 520, next to the existing `addMany`
test):
- Setup: create a project
- Call `addMany` with 3 records: [valid record, record with a bad/nonexistent
projectId, valid record]
- Assert the call rejects with the expected error
- Assert `observation.list()` returns empty - the first valid record must NOT
have been committed

**updateMany rollback test** (add near the existing `updateMany` test):
- Setup: create a project, add two observations
- Call `updateMany` with 3 updates: [valid update for obs1, update with a bad id
like 999999, valid update for obs2]
- Assert the call rejects with the expected error
- Assert both observations still have their original titles - the first update
must NOT have been committed

Research

The observation.ts module uses db.transaction(async (tx) => {...}) for single-record methods that touch multiple tables (update, dismiss, archive, promote), but addMany and updateMany just loop over records calling those methods individually. Each inner method opens its own transaction, so there's no outer atomicity. The codebase has no tx-passing helpers - when dedup.merge() needed to archive inside its transaction, it inlined the SQL rather than calling archive(). Per libSQL/drizzle docs, calling db.transaction() inside another db.transaction() fails (nested top-level transactions aren't supported), so we can't just wrap the existing method calls.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Refactor `src/core/observation.ts` to extract tx-accepting core helpers so
`addMany` and `updateMany` can wrap their entire loop in a single
`db.transaction()` - this is what makes the batch operations atomic.

**Extract core helpers:**
- Add `_addCore(q, input)`: pull the project validation SELECT and observation
INSERT out of `add()`, replace `db` references with the passed queryable `q`
- Add `_dismissCore(tx, id, transition)`: pull dismiss logic (validation, UPDATE
observations, INSERT status event) out of `dismiss()`
- Add `_archiveCore(tx, id, transition)` and `_promoteCore(tx, id, promotedTo)`
following the same pattern from `archive()` and `promote()`
- Add `_updateCore(tx, id, fields)`: pull current row fetch, diff computation,
UPDATE observations, and INSERT observationEdits out of `update()`

**Refactor public methods to delegate:**
- `add()` calls `_addCore(db, input)` - no behavior change
- `dismiss()`, `archive()`, `promote()` each wrap `db.transaction()` and call
their respective core helper with `tx` - no behavior change
- `update()` wraps `db.transaction()` and calls `_updateCore(tx, id, fields)` -
no behavior change

**Refactor batch methods to use a single outer transaction:**
- `addMany`: wrap the full `for...of` loop body in `db.transaction(async (tx) =>
{...})`, replace `this.add(record)` with `_addCore(tx, record)`, replace
`this.dismiss/archive/promote(...)` with their core equivalents passing `tx`,
return the results array from inside the callback
- `updateMany`: wrap the full `for...of` loop body in `db.transaction(async (tx)
=> {...})`, replace `this.update(id, fields)` with `_updateCore(tx, record.id,
fields)`, return the updated array from inside the callback

Do NOT call `db.transaction()` inside another `db.transaction()` - that's
exactly the bug we're fixing. Core helpers must only use the passed handle,
never `db` directly.
===============================================================================

Task: 2

Add rollback tests in `test/index.test.ts` to prove that a mid-batch failure
leaves the DB untouched - these tests should fail before the Phase 1 refactor
and pass after.

**addMany rollback test** (add near line 520, next to the existing `addMany`
test):
- Setup: create a project
- Call `addMany` with 3 records: [valid record, record with a bad/nonexistent
projectId, valid record]
- Assert the call rejects with the expected error
- Assert `observation.list()` returns empty - the first valid record must NOT
have been committed

**updateMany rollback test** (add near the existing `updateMany` test):
- Setup: create a project, add two observations
- Call `updateMany` with 3 updates: [valid update for obs1, update with a bad id
like 999999, valid update for obs2]
- Assert the call rejects with the expected error
- Assert both observations still have their original titles - the first update
must NOT have been committed

💡 Iterate on the plan with: @coderabbitai <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!