Forward-integrate v1.6.3 hotfix from master into develop - #811
Merged
Conversation
…Phase A) THOTH-CHAPTER-01 / #803 - HIGH-RISK HOTFIX, Phase A only (audit + design). Add a read-only existing-data audit for chapter Works with more than one parent Work, validated against controlled data using the DB-backed test harness. The qualifying predicate targets work_relation rows with relation_type = 'is-child-of' (a chapter that is-child-of more than one DISTINCT parent). Tests cover 0/1/>1 parents in both relation orientations, exact-duplicate rejection, unrelated relation types, is-part-of, and opposite-orientation (parent-book) rows. Also validates, ephemerally and inside a rolled-back transaction, that the candidate Phase B enforcement - a partial unique index on work_relation - is representable, rejects a second distinct parent, leaves has-child and is-part-of relations unaffected, and fails to build over corrupt data. No production write behaviour change, no migration, no enforcement activated. Refs #803.
…esign Independent review correction. The approved #803 invariant is about chapter Works (work_type = 'book-chapter'), and the issue states "do not broaden this into general WorkRelation semantic validation". The prior revision scoped the audit and proposed enforcement to any is-child-of relator (a global partial unique index), which was broader than approved. Corrections: - Authoritative blocking audit now joins work and filters work_type = 'book-chapter'; only book-chapter Works with >1 DISTINCT is-child-of parent are #803 blocking evidence. - Add a SEPARATE non-BookChapter is-child-of diagnostic, explicitly NOT #803 blocking evidence. - Replace the global partial-unique-index design validation with a BookChapter-scoped trigger (serialises on the chapter work row via FOR NO KEY UPDATE; RAISE ... USING CONSTRAINT for deterministic client error mapping), validated to reject a second distinct parent, to leave Monographs / is-part-of / has-child unaffected, and to protect the work_type -> book-chapter transition. Still Phase A only: no migration, no enforcement activated, no production write behaviour change. Refs #803.
…nce (#803) Second independent review corrections (design/tests only; still Phase A, non-enforcing). 1. Pre-lock work_type read race: the candidate relation trigger now acquires the per-Work serialization lock (SELECT ... FROM work FOR NO KEY UPDATE on the relator's work row) BEFORE reading work_type, for ANY is-child-of mutation. The work_type-transition trigger takes the SAME lock before counting parents. This closes the race where a Monograph is-child-of insert skipped locking while a concurrent Monograph->BookChapter transition could not see the uncommitted second parent. 2. Deterministic two-connection concurrency evidence: two tests exercise both lock orderings (transition-first and relation-first) using independent connections, channels and a bounded pg_stat_activity wait-for-lock gate. Each asserts the loser actually blocked and that the final persisted state is never book-chapter with >1 parent. 3. Production write-path test: WorkRelation::create rejects a second distinct parent and rolls back the paired inverse row. 4. Single-parent replacement (A -> B on the same relation) remains allowed. Design docs (PR/#803) also corrected: the Phase B migration now validates and installs enforcement inside one write-excluded transaction (LOCK TABLE work, work_relation IN SHARE ROW EXCLUSIVE MODE) to close the audit->activation TOCTOU window; CREATE TRIGGER lock mode documented as SHARE ROW EXCLUSIVE (not ACCESS EXCLUSIVE); production lock-wait/duration marked unverified. No migration, no enforcement activated, no production write behaviour change. Refs #803.
… Phase B) THOTH-CHAPTER-01 / #803 — HIGH-RISK HOTFIX, Phase B (production enforcement), continued on the existing branch/PR #806 per the recorded CTO decision. A Work with work_type = 'book-chapter' may have at most one DISTINCT parent (a work_relation of type 'is-child-of'). Zero-parent chapters remain valid; non-book-chapter works are unaffected. Migration 20260813_v1.6.3 (Diesel runs each migration in one transaction): - LOCK TABLE work, work_relation IN SHARE ROW EXCLUSIVE MODE, then a corrupt-data guard that aborts if any book-chapter already has >1 distinct is-child-of parent, then two triggers — all in the single migration transaction, so there is no audit->install TOCTOU window. - work_relation trigger (BEFORE INSERT OR UPDATE): for any is-child-of row, SELECT work_type FROM work ... FOR NO KEY UPDATE (lock + read in one query, before deciding the rule applies) and reject a second distinct parent for a book-chapter relator, excluding the row being updated. - work trigger (BEFORE UPDATE OF work_type, only for a transition into book-chapter): same FOR NO KEY UPDATE lock, then reject if the work already has >1 distinct is-child-of parent. Both raise a unique_violation with constraint work_relation_single_book_chapter_parent, mapped to a deterministic client message via DATABASE_CONSTRAINT_ERRORS. down.sql drops the triggers/functions only; no data is deleted, reparented, or rewritten. Tests: production WorkRelation::create rejection (both orientations, inverse rolled back); relation-update semantics (replace single parent allowed; second parent by related/relator/type change rejected; away-from-is-child-of allowed; exact duplicate unchanged); work_type transition guard; non-book chapter regressions; deterministic two-connection concurrency (parent-vs- parent, relation-vs-transition both orderings) asserting the loser blocks on the shared lock and verifying final state; migration matrix on isolated databases (empty/valid/invalid-abort/down/up-down-up) and an activation-race test; a read-only audit-detection test; and a GraphQL create_work_relation boundary + authorization test. Changelog updated under Unreleased. Refs #803.
…race test (#803) The activation-race test's concurrent writer attempted its insert without waiting for the migration to hold the SHARE ROW EXCLUSIVE table lock. Under CI timing the writer could run first (enforcement reverted, no contention), so it neither blocked nor was rejected, failing the `blocked` assertion. Add a go-signal so the writer only issues its insert after the migration is known to hold the lock, matching the other two-connection concurrency tests. Verified stable locally (activation-race 5x, full audit module 3x under default parallelism). Refs #803.
…nt-audit THOTH-CHAPTER-01 (#803): enforce single parent for BookChapter works [HIGH-RISK HOTFIX]
Forward-integrate THOTH-CHAPTER-01 (#803) after the v1.6.3 release. Preserve all unreleased develop work while carrying the released single-parent BookChapter enforcement forward. Conflicts resolved (union of develop content + hotfix addition): - CHANGELOG.md: keep all develop Unreleased entries; add the released #803/#806 entry once under develop's existing "### Changed". - thoth-api/src/graphql/tests.rs: keep all develop tests and the newer async test infrastructure; add the #803 GraphQL regression test adapted to develop's block_on_graphql(juniper::execute(...)) convention, with unchanged acceptance semantics. No production implementation, migration behaviour, or error semantics were changed during conflict resolution. Direction: master -> develop only. Refs #803.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Member
Author
CI failure at head
|
| directory | origin | status |
|---|---|---|
20260813_v1.6.3 |
#803 Phase B (2590b4ce) |
released in v1.6.3 (tag on master) |
20260813_v1.7.0 |
BE-03 (3facc3f9, #809) |
develop-only, unreleased |
Diesel keys migrations by the numeric prefix before the first _, so both are version 20260813. Only one can ever run; the other is silently skipped.
Evidence on a fresh database with all migrations applied via the repo runner (thoth migrate):
__diesel_schema_migrations versions: 20250000,20260417,20260429,20260504,20260805,20260812,20260813
rows with version 20260813 : 1 <- two directories, one recorded version
BE-03 table publisher_service_configuration_history : present <- 20260813_v1.7.0 ran
#803 triggers (%single_book_chapter%) : NONE <- 20260813_v1.6.3 SILENTLY SKIPPED
Re-running thoth migrate does not repair it (STILL NONE) — once 20260813 is recorded, the other directory is never applied.
Why this matters in both directions
- On
develop(observed): the released THOTH-CHAPTER-01: Enforce a single parent for chapter works #803 BookChapter single-parent enforcement silently does not exist. - On production when
developis later released (mirror case, by the same mechanism): production already recorded20260813from the v1.6.3 hotfix, so BE-03's migration would be silently skipped — its column/enum/history table would never be created while BE-03 code expects them.
Why I did not just renumber it. Renumbering the released 20260813_v1.6.3 is unsafe: production has already recorded 20260813, so a new version would make it re-run, and its DDL is intentionally non-idempotent (plain CREATE FUNCTION / CREATE TRIGGER, up.sql lines 46/78/87/107) → the deploy would fail with "already exists". It would also rewrite released history. The safe technical fix is to renumber the unreleased BE-03 migration (e.g. 20260813_v1.7.0 → 20260814_v1.7.0), but that is unrelated develop work owned by #809/BE-03, and any dev/staging database that already applied BE-03 would need reconciliation. That is a programme decision, not a CI-only correction.
Scope confirmation: no files were changed; no #803 enforcement, migration SQL, trigger logic, concurrency behaviour, error mapping, GraphQL semantics, authorization, or conflict resolution was modified. Working tree clean; nothing pushed; PR not merged.
Status: BLOCKED pending a decision on which migration version is renumbered.
This was referenced Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Forward-integrate v1.6.3 hotfix from
masterintodevelopForward-integration of the released THOTH-CHAPTER-01 (#803) single-parent BookChapter enforcement (v1.6.3) into
develop. Not new development — this PR only resolves the two merge conflicts and preserves everything else.MERGE_HEAD):d6c7effbaf63fa4a591a3859e61cb7714984f27fmaster → developonly (neverdevelop → master).1a896ac40c0eb5cd640d8b928b1bc126aacb88dd(first parent = develop lineage3b6b3a31…; second parent = masterd6c7eff…).Conflicts resolved (only these two files were conflicted)
CHANGELOG.md— kept all develop## [Unreleased]content (Added/Changed/Removed/Fixed, incl.BE-03-SPEC, PRs 800/797/778/…) and merged the released THOTH-CHAPTER-01: Enforce a single parent for chapter works #803/THOTH-CHAPTER-01 (#803): enforce single parent for BookChapter works [HIGH-RISK HOTFIX] #806 entry as the first item under develop's existing### Changed. The THOTH-CHAPTER-01: Enforce a single parent for chapter works #803 entry retains its narrow meaning (a BookChapter Work may have at most one DISTINCT parent) and does not claim to solve the historical wrong-parent incident. Present exactly once.thoth-api/src/graphql/tests.rs— kept all develop tests and its newer async test infrastructure (block_on_graphql(juniper::execute(…))), and added master's regression testcreate_work_relation_rejects_second_book_chapter_parent_through_graphql, adapted minimally to develop's async convention (its twojuniper::execute_sync(…)calls becameblock_on_graphql(juniper::execute(…))). The test's acceptance semantics are unchanged: authorized first parent succeeds; authorized second distinct parent is rejected with the client message "A book chapter may belong to only one parent work"; the unauthorized caller is still rejected by authorization.Already-merged hotfix files (auto-merged cleanly; not modified during resolution)
thoth-api/migrations/20260813_v1.6.3/{up,down}.sql,thoth-api/src/model/work_relation/tests.rs,thoth-errors/src/database_errors.rs. The released #803 enforcement (triggers, guard, error mapping) is carried forward unchanged. My migration20260813_v1.6.3sorts after develop's newest (20260812_v1.7.0) — correct ordering; it applies cleanly on develop's schema.Net effect
git diff --cached --numstatshows 1664 insertions, 0 deletions across 6 files — the merge only adds the #803 hotfix; no develop work is removed or altered.No production implementation changes
Conflict resolution changed only test/CHANGELOG text. The approved #803 enforcement architecture, migration behaviour, and error semantics are unchanged.
Verification (on the merged tree)
git diff --check/git diff --cached --check: clean ·cargo fmt --all -- --check: clean.cargo clippy -p thoth-api --all-targets --all-features -- -D warnings: clean.block_on_graphql_*,generated_schema_exposes_no_package_or_capability_surface,ordinary_publisher_mutations_leave_package_unchanged) — all pass (25/25).make check-all: passed (rc=0) on the merged tree.Requires independent review before merge. Do not merge into
developwithout review. Direction ismaster → developonly.Refs #803.