Skip to content

feat(api): keep the resource-state read model current on writes - #332

Open
AlexAxthelm wants to merge 2 commits into
feat/stit-766-rebuildfrom
feat/stit-766-writes
Open

AlexAxthelm wants to merge 2 commits into
feat/stit-766-rebuildfrom
feat/stit-766-writes

Conversation

@AlexAxthelm

@AlexAxthelm AlexAxthelm commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Third PR of splitting #324 into smaller PRs for ease of review. Builds on #331: the various write actions now keep the read-model table current, so once it has been rebuilt it stays correct as resources are operated on (merge, attach, etc.). This is where the read model starts touching existing behavior: every write that can change a list answer now takes a row lock and does a bit more work.

Note that this imposes a small performance penalty over main baseline at this point, since we're adding some small complexity to the write actions, but not using any of the read paths yet.


Summary

Third PR in the STIT-766 read-model stack (builds on #331). Every API write that can change a list answer now recomputes the affected resources' read-model rows in the same transaction, so the table and the data commit together.

Which writes

  • Attaching or creating sources: create with sources, attach, and create-and-attach. All three add memberships through one helper (_attach_source_models), which now refreshes the resource.
  • Re-ordering a field's sources (set_field_source_priority).
  • Merging (apply_resource_merge): the original resources lose their rows (they're no longer listable) and the new resource gets rows built from their combined sources.

Locking first (the behavior change reviewers should look at)

  • Each write locks its resources before it reads or writes anything (lock_resources: SELECT ... FOR UPDATE, in ascending id order).
  • Why first and not at the refresh: a write's inserts take a weaker lock on the resource row through their foreign keys. Two writers that each hold that and then both try to upgrade it for the refresh deadlock. Locked first, the second writer simply waits.
  • The waiting writer then re-reads the resource and recomputes from the first writer's committed changes, so neither overwrites the other with a stale result.
  • Waiting can reveal a merge:
    • an attach that waited on a merge now sees the resource was merged and refuses, as it would have if it had arrived later;
    • two merges sharing a resource in opposite order now queue instead of deadlocking.
  • SQLite has no row locks (it already serializes writers), so this is a no-op there.

Cost

The refresh adds ~85 ms per write at 50K resources (labeled read_model.refresh in query timings). Almost all of it is finding the resource's memberships, because og_field_memberships has no index on resource_id. The (resource_id, source_pk) constraint in #322 provides one, which brings it to ~16 ms per resource (~24 ms for a three-resource merge). Until #4 lands that cost buys nothing, so #3 and #4 should merge close together.

Docs

deployments/api/READ_MODEL.md gains:

  • a "Staying current" section;
  • the "new write path" rule: lock first, refresh in the same transaction, add a mutation test;
  • a "Known limitations" note on write overhead.

The stack

  1. Schema (feat(api): add the resource-state read model tables #330)
  2. Build and rebuild (feat(api): build/rebuild the resource-state read model tables #331)
  3. Writes keep the table current (this PR)
  4. Read path: list and filter-options read from the table when it's ready, plus a per-process readiness cache, an operator switch-off, and its migration
  5. CI: rebuild after each deploy, and wait for the old API revision to stop serving first

Related issues

Part of: STIT-766
Related: #322 (index that cuts the per-write cost)
Replaces: #324 (split for review)

Testing

  • New tests:
    • tests/db/read_model/test_mutations.py: each write path (create, attach, create-and-attach, re-order, merge, merge-candidate approval, an in-place source value edit) leaves the table equal to a from-scratch recompute, and each recompute takes the lock before reading anything.
    • tests/db/read_model/test_concurrency_postgres.py (Postgres only):
      • concurrent writes to one resource serialize, and collide without the lock;
      • two writers that both insert before refreshing don't deadlock;
      • attach plus re-order, and two re-orders, together don't deadlock;
      • overlapping merges in opposite order serialize;
      • an attach waiting on a merge sees the merge;
      • writes landing midway through a full rebuild (on SQLite too).
    • tests/observability/test_query_name_actions.py: write actions also emit read_model.refresh, and the refresh's label doesn't leak into the write's own queries.
    • tests/db/read_model/test_state.py: writes to a resource holding a value today's model rejects still succeed.
  • Results: the API suite passes on SQLite and against Postgres; make check passes.
  • Deliberate breakage check: I removed each lock and each refresh in the three write paths, plus the lock inside the refresh itself, one at a time. Every one makes a test fail, several as real Postgres deadlocks.

Checklist

  • PR is focused on a single concern
  • Tests pass locally and in CI
  • Docs updated for user-visible changes
  • AI-assisted portions declared (see CONTRIBUTING.md)

AI use: Claude Code split this out of #324 and wrote most of the code, tests and docs. I reviewed it before committing. Verified by the tests and the deliberate breakage check above.

🤖 Generated with Claude Code

…-766)

Attaching or creating sources, re-ordering a field's sources, and
merging now recompute the affected resources' read-model rows in the
same transaction. Each write locks its resources first (in id order for
merges) and re-reads them after locking: locking later lets two writers
deadlock upgrading the foreign-key locks their inserts take, and a
write that waited must see a merge that committed meanwhile.

Nothing reads the table yet, so answers are unchanged; each write pays
~85 ms at 50K resources for the refresh (~16 ms with #322's index).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new merge locks can deadlock with concurrent merge-candidate creation because resource lock ordering is inconsistent.

1 open finding
What changed in this PR

Keeps the resource-state read model current through API writes, building on #331 without enabling read paths yet.

Changes:

  • Locks affected resources before writes and refreshes state in the same transaction.
  • Adds mutation, concurrency, rebuild, and query-label coverage.
  • Documents write-path requirements and refresh overhead.
File Description
deployments/​api/​tests/​observability/​test_query_name_actions.py Checks refresh labels and label restoration.
deployments/​api/​tests/​db/​read_model/​test_state.py Tests legacy values and rebuild-time writes.
deployments/​api/​tests/​db/​read_model/​test_mutations.py Checks mutation consistency, rollback, and lock ordering.
deployments/​api/​tests/​db/​read_model/​test_concurrency_postgres.py Tests concurrent PostgreSQL writes.
deployments/​api/​tests/​db/​read_model/​scenarios.py Adds a shared rebuild-with-writes scenario.
deployments/​api/​tests/​db/​read_model/​dataset.py Clears write-generated state during fixture setup.
deployments/​api/​src/​stitch/​api/​db/​read_model/​state.py Documents write-first locking.
deployments/​api/​src/​stitch/​api/​db/​og_field_source_actions.py Locks attachment targets and refreshes state.
deployments/​api/​src/​stitch/​api/​db/​og_field_resource_actions.py Adds locking and refreshes for reorder and merge.
deployments/​api/​READ_MODEL.md Documents maintenance rules and write overhead.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread deployments/api/src/stitch/api/db/og_field_resource_actions.py
FOR UPDATE conflicts with the key-share lock that inserting any row
referencing a resource takes. Creating a merge candidate for [B, A]
holds key-share on B, then waits on A, while a merge of [A, B] holds A
and waits on B: Postgres aborts one. FOR NO KEY UPDATE (the lock an
update of repointed_id already takes) still makes writers queue but
never conflicts with key-share, so no insert path needs to lock first
to stay safe.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch was successfully deployed

1 active (outdated) deployment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants