Skip to content

Adopt multi-repo transactions: atomic observation writes, atomic SPAC history, and retire the two hand-rolled raw-SQL transaction paths #247

Description

@sroussey

Depends on workglow-dev/libs#708 (connection-scoped multi-repo transactions), umbrella workglow-dev/libs#707.

sec is the heaviest multi-table writer in the workspace, and none of its cross-repo write sequences have an atomicity boundary today, because ITabularStorage.withTransaction is scoped to a single storage instance and a sibling repo touching the same connection throws ConnectionReentryError. This issue tracks sec's side once libs lands the multi-participant API.

1. The observation tier writes 6 rows across 5 repos with no boundary

src/resolver/EntityObserver.ts observePerson (:166-240) does, in order:

  1. removePriorPersonJunctions (junction repos)
  2. personObservationRepo.upsertByNaturalKey (:179)
  3. personObservationTitleRepo.replaceForObservation (:205) — itself a delete-then-insert
  4. personResolver.resolve — mints a canonical row on first sight
  5. personIdentityLinkRepo.upsert (:214)
  6. recordPersonRoles (:220) — person_role tenure open/close/tighten
  7. address + phone junction recordObservation

observeCompany mirrors it. A failure anywhere past step 2 leaves an observation with no identity link — invisible to every query that joins through the link table, and not repaired by a re-run unless the extractor version is bumped, since extractor_runs may already record the filing.

The junction removal at step 1 is the sharpest edge: it exists specifically so replays net out to the same co-occurrence counts, so a crash between step 1 and step 7 decrements counts with nothing added back.

  • Enlist the observation, title, identity-link, person_role, and junction repos in one transaction per observePerson / observeCompany call.
  • Decide the boundary granularity: per-claim, or per-filing across the whole observePerson loop. Per-filing is the stronger invariant (a filing's roster is all-or-nothing, which matters for the completeness-gated closeUnassertedPersonRoles path) but holds the connection for the length of a form's entire person loop. Per-filing is probably right for the roster-closure forms specifically.
  • closeUnassertedPersonRoles must be inside whatever boundary the roster uses — a closure that commits while its asserting roster rolls back is worse than no closure.

2. SPAC history can lose its open row

src/storage/spac/SpacReportWriter.ts recordHistory (:495-555): closes the currently-open history row via saveHistory({...open, valid_to}), inserts the new open row, then writes N ChangeLog rows through a different repo. A crash between the close and the insert leaves the SPAC with no open history row — a broken temporal chain, and precisely the invariant the surrounding monotonicity logic works hard to maintain.

  • Wrap close + insert + change-log fan-out in one transaction.
  • Same for recordDeSpacLinkage and the spac row + spac_deal recompute, which currently only has atomicity on the deal half (see 3).

3. Retire the two hand-rolled raw-SQL transaction paths

src/storage/form-8k-event/Form8KEventReplace.ts and src/storage/spac/SpacDealReplace.ts each carry three implementations of the same delete-then-insert — better-sqlite3 db.transaction, an explicit Postgres BEGIN/COMMIT on a checked-out client, and a repository fallback — dispatched through resolveSqlBackend("write", repo). They exist purely to get a transaction the storage layer couldn't provide, and they bypass ITabularStorage entirely, which is why each one needs the resolveSqlBackend comment explaining why the repo must be passed to avoid routing test writes into a real backend.

  • Once the storage-level transaction covers the case, collapse both into ordinary repo calls inside withTransaction. This removes two dual-dialect SQL bodies, two dispatch sites, and the dry-run hazard that motivated resolveSqlBackend's access: "write" guard.
  • Note the ordering: recomputeSpacDeals is called from inside SpacReportWriter, so it must become an enlisted participant of the caller's transaction rather than opening its own — the natural first consumer of the multi-participant API.

4. ReadOnlyTabularStorage must participate

src/storage/ReadOnlyTabularStorage.ts:128 implements withTransaction as fn(this) so writes keep no-opping under --dry-run. Whatever multi-participant signature libs lands, this wrapper needs the matching one, and enlisting a mix of read-only-wrapped and raw storages should be a loud error rather than a half-real transaction.

5. Per-filing store atomicity (stretch)

ProcessAccessionDocFormTask's store stage dead-letters STORE_ERROR on a throw, and CLAUDE.md's documented worst-case recovery is "re-process the whole CIK's filings." A per-filing transaction around the store handler would make a failed store leave nothing behind, turning STORE_ERROR into a clean retry rather than a partial write to reason about. Worth evaluating after 1-3; the clear-then-insert tiers (xbrl_fact, executive_compensation, risk_factor, spac_unit_terms) are the ones that benefit most.

Verification

  • Atomicity tests for observePerson / observeCompany that fail mid-sequence and assert no observation exists without its identity link, and that junction counts are unchanged.
  • A recordHistory failure test asserting the open history row survives.
  • Existing suites green after the raw-SQL collapse: Form8KEventRepo, SpacDealReplace, SpacReportWriter, plus the resolveSqlBackend coverage.
  • --dry-run still writes nothing with the new enlistment path.

Refs: workglow-dev/libs#708 (blocker), workglow-dev/libs#707 (umbrella).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions