fix(import): chunk SIE account creation and close the import row on every exit (#2451)
* fix(import): chunk SIE account creation and close the import row on every exit
A full-BAS Bokio SIE file creates 1 200+ chart_of_accounts rows in one
INSERT. PostgREST runs it under the authenticated role's 8 s
statement_timeout, and with four row-level triggers plus the RLS WITH
CHECK that single statement measured 6.5 s to 8.2 s on prod: it was
cancelled for one company and passed for the next (2026-09-09).
- syncMappedAccounts inserts in chunks of 100 rows (INSERT_CHUNK_SIZE),
so every statement stays an order of magnitude inside the limit. A
failed chunk leaves the earlier ones committed; the next attempt reads
the chart again and inserts only what is still missing.
- executeSIEImport closes its pending sie_imports row in a finally
block. Every early `return result` after createPendingImportRecord
(account sync failure, missing fiscal year, overlapping import,
vouchers outside the year) used to leave the row 'pending'. That row
holds the (company_id, file_hash) slot in the partial unique index,
so a retry inside the five-minute cleanup gate failed on the index
instead of on the real error.
- The slot-held message no longer names "gnubok", an "Ersätt import"
button the import history has never had, or Fortnox; it says the
same file is being imported or was interrupted moments ago and to
retry in a few minutes. The thrown-error prefix is Swedish
("Importen misslyckades:") and the two undo hints name
accounted_undo_sie_import.
Tests: chunk sizes and first-failing-chunk behaviour in
account-sync.test.ts; failed-row finalize on two early exits and the
new slot message in sie-import.account-names.test.ts.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WBTRraVXkCnx93Pa9wxJ6G
* fix(import): write account chunks as ignore-duplicates upserts with an exact count
Review follow-up on #2451. A plain INSERT per chunk still had the old
race: a concurrent import (or the replace flow) creating one account
between our read and write raised a duplicate-key error that the code
swallowed as success, while PostgREST had rolled back the whole chunk,
so every other account in it was silently missing.
Each chunk is now an upsert with onConflict (company_id, account_number)
and ignoreDuplicates, selecting the landed rows: the race becomes a
skipped row, `created` counts exactly what was written (also across a
mid-loop failure, which the compliance review flagged), and the
"duplicate" string special-case is gone.
Tests: conflict-skipped row not counted; a race yields no error; a
failing later chunk reports the rows the earlier chunks committed.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WBTRraVXkCnx93Pa9wxJ6G
---------
Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
co-authored by
Claude Fable 5.1
Jakob Wennberg
Claude Fable 5.1
Jakob Wennberg
parent
fc2d78a7c4
commit
bb28968151
@@ -1,5 +1,5 @@
|
||||
import { describe, it, expect, vi } from 'vitest'
|
||||
import { syncMappedAccounts } from '../account-sync'
|
||||
import { syncMappedAccounts, INSERT_CHUNK_SIZE } from '../account-sync'
|
||||
import { getBASReference } from '@/lib/bookkeeping/bas-reference'
|
||||
import type { AccountMapping } from '../types'
|
||||
import type { SupabaseClient } from '@supabase/supabase-js'
|
||||
@@ -30,11 +30,21 @@ function mapping(
|
||||
function buildCapturingSupabase(opts?: {
|
||||
existingAccounts?: Array<{ account_number: string; account_name: string }>
|
||||
insertError?: { message: string } | null
|
||||
/** Zero-based index of the insert statement that should fail (default: all). */
|
||||
insertErrorOnBatch?: number
|
||||
/**
|
||||
* Account numbers that already exist at write time although the read pass
|
||||
* did not see them (a concurrent import): the upsert's ON CONFLICT DO
|
||||
* NOTHING skips them, and PostgREST does not return skipped rows.
|
||||
*/
|
||||
conflictAccounts?: string[]
|
||||
updateError?: { message: string } | null
|
||||
selectError?: { message: string } | null
|
||||
}) {
|
||||
const existing = opts?.existingAccounts ?? []
|
||||
const inserts: Array<Record<string, unknown>> = []
|
||||
/** Row count of every insert statement, in call order. */
|
||||
const insertBatches: number[] = []
|
||||
const updates: Array<{
|
||||
payload: Record<string, unknown>
|
||||
filters: Record<string, string>
|
||||
@@ -66,9 +76,35 @@ function buildCapturingSupabase(opts?: {
|
||||
}),
|
||||
}),
|
||||
}),
|
||||
insert: (rows: Array<Record<string, unknown>>) => {
|
||||
inserts.push(...rows)
|
||||
return Promise.resolve({ error: opts?.insertError ?? null })
|
||||
upsert: (
|
||||
rows: Array<Record<string, unknown>>,
|
||||
upsertOpts: { onConflict?: string; ignoreDuplicates?: boolean }
|
||||
) => {
|
||||
if (
|
||||
upsertOpts?.onConflict !== 'company_id,account_number' ||
|
||||
upsertOpts?.ignoreDuplicates !== true
|
||||
) {
|
||||
throw new Error(`Unexpected upsert options: ${JSON.stringify(upsertOpts)}`)
|
||||
}
|
||||
const batchIndex = insertBatches.length
|
||||
insertBatches.push(rows.length)
|
||||
const failsThisBatch =
|
||||
opts?.insertError != null &&
|
||||
(opts.insertErrorOnBatch === undefined || opts.insertErrorOnBatch === batchIndex)
|
||||
const conflicts = new Set(opts?.conflictAccounts ?? [])
|
||||
const landed = rows.filter((r) => !conflicts.has(String(r.account_number)))
|
||||
return {
|
||||
select: () => {
|
||||
if (failsThisBatch) {
|
||||
return Promise.resolve({ data: null, error: opts?.insertError ?? null })
|
||||
}
|
||||
inserts.push(...landed)
|
||||
return Promise.resolve({
|
||||
data: landed.map((r) => ({ account_number: r.account_number })),
|
||||
error: null,
|
||||
})
|
||||
},
|
||||
}
|
||||
},
|
||||
update: (payload: Record<string, unknown>) => {
|
||||
const filters: Record<string, string> = {}
|
||||
@@ -88,7 +124,7 @@ function buildCapturingSupabase(opts?: {
|
||||
}),
|
||||
}
|
||||
|
||||
return { supabase: supabase as unknown as SupabaseClient, inserts, updates }
|
||||
return { supabase: supabase as unknown as SupabaseClient, inserts, updates, insertBatches }
|
||||
}
|
||||
|
||||
function run(
|
||||
@@ -280,9 +316,13 @@ describe('syncMappedAccounts: create pass', () => {
|
||||
expect(inserts[0].account_name).toBe('Konto 1932')
|
||||
})
|
||||
|
||||
it('swallows duplicate-key insert errors (concurrent import race)', async () => {
|
||||
const { supabase } = buildCapturingSupabase({
|
||||
insertError: { message: 'duplicate key value violates unique constraint' },
|
||||
it('absorbs a concurrent import race through ON CONFLICT DO NOTHING', async () => {
|
||||
// A plain INSERT raised a duplicate-key error here, which the old code
|
||||
// swallowed while the whole statement had rolled back. The write is now an
|
||||
// ignore-duplicates upsert (the mock refuses any other shape), so the
|
||||
// race surfaces as a skipped row, never as an error.
|
||||
const { supabase, insertBatches } = buildCapturingSupabase({
|
||||
conflictAccounts: ['1930'],
|
||||
})
|
||||
|
||||
const result = await run(supabase, [
|
||||
@@ -290,6 +330,8 @@ describe('syncMappedAccounts: create pass', () => {
|
||||
])
|
||||
|
||||
expect(result.error).toBeNull()
|
||||
expect(result.created).toBe(0)
|
||||
expect(insertBatches).toEqual([1])
|
||||
})
|
||||
|
||||
it('returns a fatal error for non-duplicate insert failures', async () => {
|
||||
@@ -511,3 +553,73 @@ describe('syncMappedAccounts: rename pass', () => {
|
||||
expect(result.renamed).toBe(1)
|
||||
})
|
||||
})
|
||||
|
||||
// A full-BAS SIE import creates 1 200+ accounts. PostgREST runs each request
|
||||
// under the authenticated role's 8 s statement_timeout, and one 1 200-row
|
||||
// INSERT measured 6.5 s to 8.2 s on prod (2026-09-09: cancelled for one
|
||||
// company, passed for the next). The create pass must therefore never send
|
||||
// the whole chart as one statement.
|
||||
describe('syncMappedAccounts: chunked create pass', () => {
|
||||
function fullChart(count: number): AccountMapping[] {
|
||||
// 1000..1000+count: every number is a valid class-1 account, BAS or not.
|
||||
return Array.from({ length: count }, (_, i) => {
|
||||
const num = String(1000 + i)
|
||||
return mapping({ sourceAccount: num, targetAccount: num, sourceName: `Konto ${num}` })
|
||||
})
|
||||
}
|
||||
|
||||
it('inserts missing accounts in statements of at most INSERT_CHUNK_SIZE rows', async () => {
|
||||
const { supabase, inserts, insertBatches } = buildCapturingSupabase()
|
||||
const total = INSERT_CHUNK_SIZE * 2 + 42
|
||||
|
||||
const result = await run(supabase, fullChart(total))
|
||||
|
||||
expect(result.error).toBeNull()
|
||||
expect(result.created).toBe(total)
|
||||
expect(inserts).toHaveLength(total)
|
||||
expect(insertBatches).toEqual([INSERT_CHUNK_SIZE, INSERT_CHUNK_SIZE, 42])
|
||||
expect(new Set(inserts.map((r) => r.account_number)).size).toBe(total)
|
||||
})
|
||||
|
||||
it('keeps a small chart in a single statement', async () => {
|
||||
const { supabase, insertBatches } = buildCapturingSupabase()
|
||||
|
||||
await run(supabase, fullChart(3))
|
||||
|
||||
expect(insertBatches).toEqual([3])
|
||||
})
|
||||
|
||||
it('counts only the rows the conflict clause let through', async () => {
|
||||
// The read pass saw neither account, but 1930 was created by a concurrent
|
||||
// import before our write: ON CONFLICT DO NOTHING skips it and the count
|
||||
// must not claim it.
|
||||
const { supabase, inserts, insertBatches } = buildCapturingSupabase({
|
||||
conflictAccounts: ['1930'],
|
||||
})
|
||||
|
||||
const result = await run(supabase, [
|
||||
mapping({ sourceAccount: '1930', targetAccount: '1930', sourceName: 'Företagskonto' }),
|
||||
mapping({ sourceAccount: '6110', targetAccount: '6110', sourceName: 'Kontorsmateriel' }),
|
||||
])
|
||||
|
||||
expect(result.error).toBeNull()
|
||||
expect(insertBatches).toEqual([2])
|
||||
expect(result.created).toBe(1)
|
||||
expect(inserts.map((r) => r.account_number)).toEqual(['6110'])
|
||||
})
|
||||
|
||||
it('stops at the first failing statement and reports its error', async () => {
|
||||
const { supabase, insertBatches } = buildCapturingSupabase({
|
||||
insertError: { message: 'canceling statement due to statement timeout' },
|
||||
insertErrorOnBatch: 1,
|
||||
})
|
||||
|
||||
const result = await run(supabase, fullChart(INSERT_CHUNK_SIZE * 3))
|
||||
|
||||
expect(result.error).toBe('canceling statement due to statement timeout')
|
||||
// First chunk committed, second failed, third never sent: the count
|
||||
// reports what actually landed in chart_of_accounts.
|
||||
expect(result.created).toBe(INSERT_CHUNK_SIZE)
|
||||
expect(insertBatches).toEqual([INSERT_CHUNK_SIZE, INSERT_CHUNK_SIZE])
|
||||
})
|
||||
})
|
||||
|
||||
@@ -248,3 +248,104 @@ describe('executeSIEImport: account name sync wiring', () => {
|
||||
expect(result.accountsCreated).toBeUndefined()
|
||||
})
|
||||
})
|
||||
|
||||
// Regression for 2026-09-09: the account insert timed out, executeSIEImport
|
||||
// returned through the early `return result` after syncMappedAccounts, and
|
||||
// the sie_imports row it had just created stayed 'pending'. That row holds
|
||||
// the (company_id, file_hash) slot in the partial unique index, so the user's
|
||||
// retry 40 s later failed on the index with a message that named the retired
|
||||
// brand and a button that does not exist.
|
||||
describe('executeSIEImport: pending import record on early exit', () => {
|
||||
function importWith(mock: ReturnType<typeof createQueuedMockSupabase>) {
|
||||
return executeSIEImport(
|
||||
mock.supabase as unknown as SupabaseClient,
|
||||
'company-1',
|
||||
'user-1',
|
||||
makeParsedFile(),
|
||||
makeMappings(),
|
||||
{
|
||||
filename: 'bokio.se',
|
||||
fileContent: '#dummy',
|
||||
createFiscalPeriod: false,
|
||||
importOpeningBalances: false,
|
||||
importTransactions: true,
|
||||
}
|
||||
)
|
||||
}
|
||||
|
||||
it('closes the pending record as failed when the create pass aborts', async () => {
|
||||
mockSync.mockResolvedValue({
|
||||
created: 0,
|
||||
renamed: 0,
|
||||
renamedAccounts: [],
|
||||
renameFailed: 0,
|
||||
error: 'canceling statement due to statement timeout',
|
||||
})
|
||||
const mock = createQueuedMockSupabase()
|
||||
mock.enqueueMany([
|
||||
{ data: null }, // checkDuplicateImport: no prior import
|
||||
{ data: null }, // cleanupStaleImportRecords
|
||||
{ data: { id: 'imp-1' } }, // createPendingImportRecord insert
|
||||
{ data: null }, // finalizeImportRecord update
|
||||
])
|
||||
|
||||
const result = await importWith(mock)
|
||||
|
||||
expect(result.success).toBe(false)
|
||||
expect(result.importId).toBe('imp-1')
|
||||
const updates = mock.findCalls('sie_imports', 'update')
|
||||
expect(updates).toHaveLength(1)
|
||||
expect(updates[0][0]).toMatchObject({
|
||||
status: 'failed',
|
||||
imported_at: null,
|
||||
transactions_count: 0,
|
||||
error_message: expect.stringContaining('statement timeout'),
|
||||
})
|
||||
})
|
||||
|
||||
it('closes the pending record as failed when the file has no fiscal year', async () => {
|
||||
const mock = createQueuedMockSupabase()
|
||||
mock.enqueueMany([
|
||||
{ data: null },
|
||||
{ data: null },
|
||||
{ data: { id: 'imp-2' } },
|
||||
{ data: null },
|
||||
])
|
||||
|
||||
const result = await importWith(mock)
|
||||
|
||||
expect(result.errors).toContain('No fiscal year defined in the SIE file')
|
||||
const updates = mock.findCalls('sie_imports', 'update')
|
||||
expect(updates).toHaveLength(1)
|
||||
expect(updates[0][0]).toMatchObject({ status: 'failed' })
|
||||
})
|
||||
|
||||
it('explains a held file-hash slot in Swedish, without the retired brand or a missing button', async () => {
|
||||
const mock = createQueuedMockSupabase()
|
||||
mock.enqueueMany([
|
||||
{ data: null }, // checkDuplicateImport
|
||||
{ data: null }, // cleanupStaleImportRecords
|
||||
{
|
||||
data: null,
|
||||
error: {
|
||||
code: '23505',
|
||||
message:
|
||||
'duplicate key value violates unique constraint "sie_imports_company_id_file_hash_active_idx"',
|
||||
},
|
||||
},
|
||||
])
|
||||
|
||||
const result = await importWith(mock)
|
||||
|
||||
expect(result.success).toBe(false)
|
||||
expect(result.importId).toBeNull()
|
||||
expect(result.errors).toHaveLength(1)
|
||||
expect(result.errors[0]).toMatch(
|
||||
/^Importen misslyckades: Samma SIE-fil håller redan på att importeras/
|
||||
)
|
||||
expect(result.errors[0]).toMatch(/Vänta några minuter och försök igen/)
|
||||
expect(result.errors[0]).not.toMatch(/gnubok|Ersätt import|Fortnox/)
|
||||
// No row was created, so there is nothing to close.
|
||||
expect(mock.findCalls('sie_imports', 'update')).toHaveLength(0)
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user