[YTDB-382] Transactional schema operations and per-class schema records - #1150
Merged
Conversation
Andrii Lomakin (andrii0lomakin)
added a commit
that referenced
this pull request
Jun 16, 2026
…pened The handoff bridged the paused readability-pass session. Pass-2 (Mutation 4) is committed and the draft PR is open, so the handoff's purpose is fulfilled. Removing it lets the next /create-plan session auto-resume Step 4b (plan derivation) cleanly: design.md is committed and clean with no implementation-plan.md, which is exactly the Step-1c auto-resume condition.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of workflow documentation, adversarial analysis logs, design mutations, and handoff planning files for the transactional schema operations feature. The reviewer's feedback highlights several formatting and documentation standard issues, including the need to explicitly define the target audience and prerequisite knowledge in the first paragraph of the design overview, and to consistently enclose property names, tool names, file names, and parameter references in backticks across the Markdown files.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
I am having trouble creating individual review comments. Click here to see my feedback.
docs/adr/transactional-schema/_workflow/design.md (6-12)
According to the general rules, design documents must establish the intended audience and prerequisite knowledge through prose framing in the Overview's first paragraph. Standalone metadata blocks are forbidden. Please update the first paragraph to explicitly frame the target audience (e.g., YouTrackDB core developers and maintainers) and the required prerequisite knowledge (e.g., familiarity with YouTrackDB's storage, transaction, and schema architecture).
References
- Design documents must establish the intended audience and prerequisite knowledge through prose framing in the Overview's first paragraph; standalone metadata blocks are forbidden.
docs/adr/transactional-schema/_workflow/handoff-planning.md (31)
According to the general rules, property names, tool names, and parameter references in Markdown documentation should be enclosed in backticks for consistency. Please enclose mcp-steroid, CLAUDE.md, steroid_list_projects, transactional-schema, PSI, grep, and Read in backticks.
References
- Property names, tool names, and parameter references in Markdown documentation should be enclosed in backticks for consistency.
docs/adr/transactional-schema/_workflow/handoff-planning.md (35)
Please enclose the tool names mcp-steroid and PSI in backticks to maintain consistency with the repository rules.
References
- Property names, tool names, and parameter references in Markdown documentation should be enclosed in backticks for consistency.
docs/adr/transactional-schema/_workflow/handoff-planning.md (37)
Please enclose the file names design-mutations.md and design.md in backticks for consistency.
References
- Property names, tool names, and parameter references in Markdown documentation should be enclosed in backticks for consistency.
docs/adr/transactional-schema/_workflow/handoff-planning.md (42)
For consistency and adherence to the general rules, file names like house-style.md and design.md should be enclosed in backticks.
References
- Property names, tool names, and parameter references in Markdown documentation should be enclosed in backticks for consistency.
docs/adr/transactional-schema/_workflow/reviews/readability-feedback-design-pass2.md (14)
According to the general rules, property names, tool names, and parameter references in Markdown documentation should be enclosed in backticks. Please enclose ScalableRWLock, OperationsFreezer, cutWaitingList, unpark, freezeRequests, and EXPORTER_VERSION in backticks.
References
- Property names, tool names, and parameter references in Markdown documentation should be enclosed in backticks for consistency.
docs/adr/transactional-schema/_workflow/reviews/readability-feedback-design-pass2-round2.md (14)
According to the general rules, property names, tool names, and parameter references in Markdown documentation should be enclosed in backticks. Please enclose ScalableRWLock, OperationsFreezer, cutWaitingList, unpark, freezeRequests, and EXPORTER_VERSION in backticks.
References
- Property names, tool names, and parameter references in Markdown documentation should be enclosed in backticks for consistency.
Andrii Lomakin (andrii0lomakin)
added a commit
that referenced
this pull request
Jul 14, 2026
…pened The handoff bridged the paused readability-pass session. Pass-2 (Mutation 4) is committed and the draft PR is open, so the handoff's purpose is fulfilled. Removing it lets the next /create-plan session auto-resume Step 4b (plan derivation) cleanly: design.md is committed and clean with no implementation-plan.md, which is exactly the Step-1c auto-resume condition.
Andrii Lomakin (andrii0lomakin)
force-pushed
the
transactional-schema
branch
from
July 14, 2026 08:07
3ccaec1 to
6e7cbb3
Compare
Andrii Lomakin (andrii0lomakin)
added a commit
that referenced
this pull request
Jul 20, 2026
…pened The handoff bridged the paused readability-pass session. Pass-2 (Mutation 4) is committed and the draft PR is open, so the handoff's purpose is fulfilled. Removing it lets the next /create-plan session auto-resume Step 4b (plan derivation) cleanly: design.md is committed and clean with no implementation-plan.md, which is exactly the Step-1c auto-resume condition.
Andrii Lomakin (andrii0lomakin)
force-pushed
the
transactional-schema
branch
from
July 20, 2026 06:46
e3b1e99 to
25634dd
Compare
Andrii Lomakin (andrii0lomakin)
added a commit
that referenced
this pull request
Jul 27, 2026
…pened The handoff bridged the paused readability-pass session. Pass-2 (Mutation 4) is committed and the draft PR is open, so the handoff's purpose is fulfilled. Removing it lets the next /create-plan session auto-resume Step 4b (plan derivation) cleanly: design.md is committed and clean with no implementation-plan.md, which is exactly the Step-1c auto-resume condition.
Andrii Lomakin (andrii0lomakin)
force-pushed
the
transactional-schema
branch
from
July 27, 2026 06:24
1ee08fe to
2cc6ace
Compare
Andrii Lomakin (andrii0lomakin)
changed the title
Andrii Lomakin (andrii0lomakin)
marked this pull request as ready for review
July 27, 2026 14:14
The YTDB-382 Phase-0 research lived in a bespoke decision-log layout that predates the current research-log convention. Reshape it into the canonical _workflow/research-log.md form that /create-plan Phase 1 reads, so the branch can enter planning without a format gap. Rename the file and map the four working sections to canonical headings (Initial request, Surprises & Discoveries, Decision Log, Open Questions), demote the adversarial-findings block to a subsection, and append an Adversarial gate record with one dated verdict heading per hardening pass (1-12). Findings, decision records, and open questions are preserved verbatim with their cross-references; D-records keep their prose form rather than the Why/Alternatives template. Baseline and re-validation is omitted (not a workflow-modifying branch); F107's stale section references are repointed to the renamed subsection.
F108 claimed per-unpark re-evaluation covered the layered freeze case, but the freezer only unparks the waiting list when freezeRequests hits zero (OperationsFreezer:97), and freezeOperations never unparks. A schema commit parked behind a transient quiesce (DiskStorage:356) while an operator freeze layers on (1->2) and the transient then releases (2->1) is therefore never woken for the operator freeze's whole duration, holding the four-lock window and reopening the F86 DDL outage the gate exists to prevent. Accept shape (a): the operator-kind arm of freezeOperations cuts and unparks the waiting list after its increment, so the woken schema-commit entrant re-evaluates the kind-aware gate and throws while data entrants re-park. The freezer's existing Dekker discipline bounds the race and cutWaitingList stays cut-safe under the now-two unpark sites. Shape (b) (timed park) was rejected: its parkNanos backstop leaves a bounded residual outage and demotes the in-window gate to polling. Folds the layered-case wake pin and a fourth acceptance line into D7's freezer bullet, plus correction/extension notes on F108 and F86. PSI-confirmed: sole callers releaseOperations<-AOM:252, cutWaitingList<-OF:105, addThread<-OF:44.
The F104 handshake pinned the holder write order and the engagement record's survival across clear()/close(), but not the session-side record's write position. The foreign teardown's release pass read that record for the ordinal it presents to the session-keyed CAS; a record written after the mark re-check as a plain write has no happens-before edge to the foreign read, so the teardown sees no ordinal, the CAS mismatches and warn-noops, and the permit wedges -- the exact failure F104 set out to kill. Accept shape (b): route the release ordinal by path. The normal release runs on the acquiring thread, which both wrote and reads the session-side record -- same-thread program order, no publication question, captured ordinal for anti-stale. The teardown is the only foreign-thread releaser, and it already loads the volatile holder to identify the session; the holder carries the ordinal (F105 triple), is cross-thread published by the engage's holder write, and survives session wipes on the mutex. The teardown therefore CAS-clears off the holder it reads and never consults the record, so the record's write position no longer gates the foreign heal. Shape (a) (write record before holder) was rejected as the heavier argument: it keeps the foreign path on the record and needs two ordering pins. Zero new fences either way. Folds the path split into D7's abnormal-termination bullet, plus amendment and extension notes on F104 and F96.
F106 named the checkOpenness gate as a second load-bearing exclusion property against a still-running commit-phase zombie, but it reads the plain status field (DatabaseSessionEmbedded:223) with no JMM edge from the foreign teardown's CLOSED write (internalClose:2234), so "never a fresh one" is best-effort, not structural. State it as a best-effort early cap and name F52's whole-commit SchemaShared lock scope as the structural exclusion authority. A late-visible status admits at most one more zombie commit, harmless because that straggler serializes behind F52's lock on a cleared tx (F85). Rejected the volatile-status alternative: D7 already settled plain tx status as a shipped memory mode (the F83-F85 settlement), so making it volatile would reverse a standing decision and tax every checkOpenness call for a property F52's lock already delivers. Folds the demotion into D7's teardown bullet plus an amendment note on F106.
The re-keyed Guard (F38) bullet and the teardown bullet's pre-existing sentence both said a different-session release is "rejected loudly", splitting the mismatch outcome two ways and contradicting the abnormal-termination bullet, which specifies one outcome for all three mismatch arms. A loud throw is wrong at the release site: it runs in the teardown finally, where a throw masks the owner's real exception (the F97 shape). Align both sentences to the mechanism: every release-site mismatch (different session, stale ordinal, holder already null) loses the CAS and warn-noops, permit untouched. The only loud rejection is the engage-side predicate (F105): holder.thread == currentThread && holder.session != engagingSession. Folds both D7 sentences plus a closure note on F107 (the anchor it re-keyed).
The F110 relabel of the legacy exporter's promote class was still not context-exact. Three gaps: a fault between writeFieldName and its value leaves a pending name, so close() emits "name":} -- structurally closed but malformed and parse-rejected at import, not well-formed; the F109 swallow arm was keyed on "record object open" rather than the innermost stranded context, so a swallow mid-embedded-array (which strands at array context and must NOT promote) was miscounted; and the between-sections / array listings read as definitions rather than illustrations. Make "leaves the generator at object context" the sole classifier, demote the listings to illustrations, qualify object-context promotion as structurally closed but possibly malformed (fail-closed at import), and condition the swallow arm on the stranded context. Jackson 2.21.4 confirmed: writeEndObject checks only inObject() with no dangling-name guard. Folds the corrected statement into D20's bounding parenthetical plus a refinement note on F110; F90/F100 inherit via their relabel pointers.
F109's "the primary-exception pin trivializes" and D20's "two mechanism pins deliver those outcomes" overclaimed. The promote-only-on-success and per-record-isolation pins deliver the no-file outcome, but the discard path still performs cleanup I/O (generator and gzip/file close, .tmp delete), so a finally-resident cleanup throw can still replace the in-flight scan failure (correlated disk-full is the live case). The pins narrow the secondary class to cleanup-I/O failures; they do not eliminate it. Reword "trivializes" to "narrows" in the F109 resolved block and D20's outcome clause, and state that the primary-exception outcome stays delivered by the F94 (b) addSuppressed / log-then-rethrow discipline, which stays load-bearing.
F109's per-record write isolation buffered each record whole, but "record sized" is unbounded (embedded recursion, nested collections, base64 4:3), so the buffer was O(rendered record). A too-large record fails its render: best-effort sheds a healthy record as broken, fail-fast aborts -- the next migration cannot export a database the storage handles fine. And OutOfMemoryError is an Error the per-record catch(Exception) at DatabaseExport:221 does not catch, so "discarded whole" is undeliverable for the OOME class. Bound the buffer with a spill-to-temp path: render in memory up to a threshold, spill to a transient temp file beyond it (streamed in on success, discarded on render failure), preserving whole-or-nothing isolation at O(threshold) memory for any record size on this offline tool. Bounding also removes the per-record OOME (a large record spills rather than exhausting the heap); a general OOME stays fail-closed via promote-only-on-success. Rejected the report-oversized / loud-abort alternative: it leaves a valid database un-migratable. Folds the bound into F109's resolved block, D20's isolation pin, and F94's isolation note.
The F113 error-capture liveness control provoked a sentinel "through the logger", but SLF4JLogManager resolves and caches a logger per requester-class name (:48-64), and every DatabaseExport error line travels the ...db.tool.DatabaseExport category (requester this at 152/213/225/281/293/606), which JUL/logback/log4j2 filter and route independently. A sentinel provoked through any other category lands in the capture while the real export error lines route elsewhere, so the control false-passes and an empty capture reads as clean. Pin the sentinel at error level through the export tool's own category (LogManager.error(DatabaseExport.class, ...)), and check that category's effective destination, not the root's. Level direction was already safe. Folds the category pin into D20's review pin and F113's resolved block; F102's two-capture structure is unchanged.
All eight pass-12 findings (F114-F121) are settled, so the gate-state note and the pass-12 entry now record that. The gate stays open pending the pass-13-vs-dry re-attack, after which the formal Phase 0->1 gate runs.
Track 8 Step 6 - the ruled Q-M2/SR2 info-validation matrix at the pre-flight seam plus the WI3 operator migration-procedure page. Before this change the importer read only the exporter version and the best-effort marker from the dump's info section: a dump produced by NEWER binaries (exporter version >= 16) sailed through the v15 strictness arms and imported silently, an absent, malformed, or out-of-range schema-version was skipped unread, a known info field carrying the wrong type was ignored, an unparseable exporter version surfaced as a bare NumberFormatException naming nothing, and a dangling info field name (the mid-write crash shape) desynced the reader past the deferred-preamble boundary before erroring. The matrix (design M2.b-5, ruling R4 + Q-M2, gate WI12a/b), judged in runPreFlightChecks so every rejection is genuinely pre-mutation: - exporter-version dispatch: >= 16 rejects with a redirect naming both versions AHEAD of every v15 arm (making the >= 15-keyed arms effectively == 15 - the Step 5 as-built end-state; also resolves CQ24's hardcoded-v15 wording by ordering); an unparseable version rejects fail-closed naming the raw value (WI12a, same outcome as an undeclared one); declared <= 14 rides the lenient path unchanged. - schema-version: mandatory for v15, must sit in MIN_IMPORTABLE_SCHEMA_VERSION..SchemaShared.CURRENT_VERSION_NUMBER (6..6 today, a one-constant bump next time); missing, malformed, and out-of-range each reject naming declared vs supported. - known optional info fields are type-checked if present (WI12b; violations collected at parse, judged v15-only for R1); unknown extra fields are tolerated and logged (the exporter version is the compatibility contract, not field enumeration). - a dangling/malformed info field name is rejected at parse (FM-M10), pre-mutation, instead of desyncing the reader - no honest dump of any version produces one, so acceptance is unchanged. - the best-effort ack gate stays MARKER-KEYED per the Step 6 ruling (SR3, resolving BG23/CQ26/CS66): no honest legacy exporter writes the marker, and rejecting a hand-edited dump absent an explicit acknowledgment is intended fail-closed behavior. The new docs/operator-migration-procedure.md (WI3, folding CS44 and the SR1 condemn-target doctrine, CN59 genesis-incomplete guidance incl. the OSystem case, CS59/FM-M18 crash residue, and the Step-5-gate exit-0 phrasing: exit 0 = every dump entry was consumed and verified against the manifest) is indexed in docs/README.md and pinned by a content test. Pin M.5 #12's literal DatabaseCompare comparison is id-keyed and structurally unsatisfiable against the renumbering import (the historical DbImportExportTest using it is @disabled for the same reason); the rehearsal pins logical equivalence instead - schema, per-class counts, record contents, link topology, index, and blob bytes - recorded as a blocked-letter deviation for the orchestrator.
Checkbox + commit slot, progress-log line, the Episodes entry (ten red-first signatures, the SR3 marker-keyed ack-gate ruling recorded in the design-drafts rulings section, the WI3 page content record, pin coverage incl. the pre-existing #10 host test), and the Surprises entry for pin M.5 #12's blocked DatabaseCompare letter (id-keyed comparison vs the renumbering import; logical-equivalence rehearsal shipped, letter left to an orchestrator ruling).
…, scalar values Review-fix iteration 1 for Track 8 Step 6 (commit 612340c), merged should-fix set from baseline/crash-safety/docs step-6 reviews: - WI60 (BLOCKER): the runbook documented EXPORT DATABASE / IMPORT DATABASE commands that do not exist anywhere in the product (no grammar token, no console command, no server op, no CLI). The page now documents the REAL surface - the programmatic DatabaseExport / DatabaseImport tools driven from a wrapper JVM via YourTracks.instance(...) -> YouTrackDBImpl.open(...) - with copy-runnable snippets, an honest note on their internal-package status, and the failure contract (the tools signal every failure by throwing). - F1 (BG29=CS76): the best-effort MARKER now parses from the quote-stripped token (parent readBoolean parity) - a hand-edited quoted "true" on a declared-legacy dump arms the SR3 marker-keyed ack gate again (fail-closed) instead of silently disarming it; the v15 WI12b type violation for the quoted form is kept as-is. - F2 (BG30=CS78): the importInfo and importManifest field loops are EOF-bounded (hasNext()) with loud post-loop truncation rejections - a dump truncated mid-info previously spun forever on stale reader state while growing the unknown-field list toward OOM (demonstrated: OutOfMemoryError in 34s), and a dump truncated mid-manifest hung outright. Ungated by version: such dumps could never import (they hung), so acceptance is unchanged - the hang becomes a rejection. - F3 (CS75): info-field VALUES are now rejected at parse when structured ('{'/'['-led) - the reader's until-the-separator scan cannot distinguish a nested closing brace from the info object's own close, so an object-valued unknown field desynced the parse, passed pre-flight on a truncated capture, MUTATED the target, and rejected post-mutation (an SR1-boundary violation); the trailing placement even imported silently. Chosen remedy: the recorded SCALAR-ONLY rule (pre-mutation, loud, naming the field) over a structure-aware skip - no dump shape any exporter has ever written carries structured info values, field-NAME tolerance is untouched, and the version number remains the compatibility contract. - F4 (TQ29): ordering pin - a v16 dump missing its schema-version must get the redirect message, not the schema-version complaint (discrimination proven by temporarily reordering the arms). - F5 (WI61): 'exit status 0' is bound to the real observable (wrapper process; tool returned without throwing). - F6 (WC61): the exit-0 meaning is scoped - legacy (<= 14) dumps carry no manifest and receive no structural verification; stated as an explicit migration caveat. - F7 (WC62): the accept/reject table's schema-version row is scoped to v15 dumps (legacy alien schema-versions stay lenient, as pinned). - F8 (WC63): step 2 no longer claims interrupted exports delete their temp file - only in-process failures clean up; kill/crash leaves residue (cross-referenced to the residue section, FM-M18). - F9 (WI62): 'discard' is bound to the drop surface (manager.drop(...), CN54-exempted for genesis corpses; the OSystem case keeps directory deletion - it has no drop surface). The doc pin test's mandated-content list grew to cover the new obligations (real surface named, without-throwing contract, drop binding, legacy no-structural-verification caveat, v15-scoped row, killed-or-crashed wording) while keeping all previous pins verbatim.
Log the applied findings (WI60 blocker runbook rewrite to the real programmatic surface; F1 quoted-marker gate parity; F2 EOF-bounded info/manifest loops with the live OutOfMemoryError and timeout red signatures; F3 recorded scalar-only info-value rule with both desync shapes; F4 redirect-ordering pin with its discrimination proof; F5-F9 doc corrections with the content-pin kept in sync), the chosen-remedy records, verification numbers, and the deferred suggestion list.
Log the Step 6 review-fix iteration 1 gate verdict (integrity check PASS - all F1-F3 production hunks present despite the mid-work git checkout mishap; WI60 + F1-F9 + pin-test coherence all VERIFIED; 0 RG findings, ledger stands at RG4), fix the gate's cosmetic nit (the runbook snippets' adminPassword placeholder is now declared so both main() examples compile verbatim; content-pin test re-run green), and add the three Step 6 review reports (baseline, crash-safety, docs). Deferred suggestions (CQ29-32, TQ30-32, CS77, WI63/64, WS60/61) and Step-5 residuals unchanged.
…, message order Track 8 track-level (cumulative) review-fix iteration, merged set from the baseline/crash/concurrency cumulative reports: - CN60 (should-fix): AbstractStorage.getCollectionNames and SchemaShared.getBlobCollections now COPY under their guarding locks instead of returning live backing views that escaped the lock. The exporter iterated those views with no lock held while concurrent DDL commits mutate the backing HashMap/IntSet under the write lock - JMM-undefined: a CME at best, silently skipped entries at worst, under-reading maxCollectionId and truncating an exit-0 export whose self-consistent manifest still verifies. Deterministic pins assert the snapshot property that makes the race impossible (the returned set must not reflect DDL performed after the call - a genuine cross-thread interleaving needs a HashMap resize racing an unlocked iterator and is not deterministically buildable). Side benefit: ImmutableSchema snapshots no longer silently track live blob-set changes. - CS80: the hasNext() EOF-bound + loud post-loop truncation rejection (the 46b0446 pattern) is extended from importInfo/importManifest to every remaining reader loop - collections, records, indexes (outer and inner), collectionsToIndex, brokenRids, schema classes, class fields, globalProperties, super-classes, properties, customFields - via a shared truncatedDump helper. Red-first: a dump truncated inside an indexes entry spun forever on stale reader state (silent CPU hang, TestTimedOutException at HEAD); the records family proved protected-by-accident at HEAD for every constructible cut (stale replays die on the rid-map unique key or stale-token parse errors) - its test pins the now-by-construction loudness with a timeout guard. All post-loop checks key on the missing terminator, never on hasNext(), so an honestly-closed structure cannot false-trip (R1-safe; truncated dumps of any version previously hung or desynced, never imported). - CN62: the import constructor captures the physical size from the OPENED descriptor (FileChannel.size, fstat-after-open) instead of a path stat before open - a concurrent re-export promoting between stat and open made the step-(3) size arithmetic falsely condemn a healthy import. Not deterministically testable without filesystem interposition (the property is structural: size and bytes now refer to one inode); the round-trip suites pin the arithmetic. Incidental: a missing dump file now surfaces as FileNotFoundException again (BG21's exception-type drift reverted). - CS79: the exporter's 'Database export completed' message moves after the trailer flush, promote, and completion flag - a transcript can no longer claim completion ahead of the durability point. - CN61 (doc): the runbook's step 1 and the design drafts now record the live-export consistency envelope - records are snapshotted at tx begin but schema/collections/blob-set/index sections are later live reads; an export under concurrent DDL is manifest-verifiable but not cross-section point-in-time consistent; quiesce DDL during migration exports. Pinned by new mandated-content fragments. - CS80 (doc): the runbook's truncation row is scoped honestly (v15 guarantee; lenient path carries no verification guarantee). - CQ33 (record): the version-ungated CS63 latch widening (declared- legacy differing re-declaration now rejected vs silent last-wins) is recorded in the design-drafts rulings as an accepted fail-closed widening, per the SR3 precedent. Deferred per the work order: BG31, CQ34, TQ33.
Log the applied cumulative findings (CN60 snapshot-copy accessors with both red-first signatures; CS80 EOF bounds on all remaining reader loops with the indexes-spin timeout red and the records-family protected-by-accident justification; CN62 fstat-after-open; CS79 message ordering; CN61 consistency-envelope records; CQ33 latch- widening ruling record), correct the Step-5 Surprises bullet's internal-collection-exclusion inaccuracy flagged by both cumulative baseline passes, and record the verification numbers incl. the full IT re-run (first attempt was a surefire fork-start infra flake) and the deferred set (BG31, CQ34, TQ33).
Gate findings from the cumulative-iteration verification: - RG7: the CS80 truncation rejections thrown by the schema-family EOF bounds landed inside importSchema's pre-existing legacy-tolerance catch (Exception) swallow - and the legacy (< 15) path has no post-loop structural check - so a legacy dump truncated inside its schema section completed with exit 0 and only an ERROR log line, contradicting the track's fail-loudly promise. The truncation rejection is now a dedicated type (TruncatedDumpImportException, thrown by every bounded loop via the truncatedDump helper) and importSchema's catch rethrows it, making truncation loud on every path and version. R1 justification, recorded: no honest dump of any version is truncated (the dangling-name-guard precedent), so the rethrow costs no honest acceptance. Swallow audit: importSchema's catch was the only eater - importRecord's Throwable-catch rethrows the type (it only swallows DatabaseException, which the new type is not), importRecords and importDatabase wrap loudly, and the remaining catches (framing detection, close, type checks, reflection) enclose no bounded loop. - RG6: SharedContext's genesis blob-registration comment still claimed getCollectionNames() returns a live view - false since the CN60 copy-under-lock fix; synced (the defensive List.copyOf stays as belt-and-suspenders). - Doc follow-through: with truncation loud everywhere, the runbook's truncation row drops last iteration's v15-only scoping and reads unconditionally: rejected loudly, on every path and for every declared version.
Log the gate verdict (all 8 findings VERIFIED, full-suite side effect corroborating no honest-dump false-trips) and the RG6/RG7 fix addendum with the red-first signature, the swallow audit, the R1 justification, the unconditional runbook truncation row, and verification numbers.
…review Log the RG6/RG7 micro-gate verdict (both VERIFIED, exhaustive catch-site audit clean, no RG8), tick the step-implementation and track-level-code-review progress boxes (track completion pends user review), record the canonical deferred-items ledger in Outcomes & Retrospective (cumulative BG31/CQ34/TQ33; Step-6 suggestion set; Step-5 residuals incl. the justified-deferred CS64; the CN61 future-design note; the out-of-scope follow-up candidates), and add the three track-level cumulative review reports (baseline, crash, concurrency).
User-approved 2026-07-25. Ticks track completion and records the closing status alongside ruling SR4: pin M.5 #12's rehearsal letter is amended from DatabaseCompare-level to LOGICAL equivalence, because the comparator is rid/collection-id-keyed while the import renumbers collections and randomizes blob placement, making the original letter structurally unsatisfiable (the historical DbImportExportTest using that pattern is @disabled for the same reason). The pin is therefore discharged as-built by the logical-equivalence rehearsal and no longer reads as blocked; a rid-mapping-aware DatabaseCompare mode was not commissioned and stays an optional out-of-scope follow-up in the deferred-items ledger.
Post-implementation artifact: design-final.md, the as-built design that supersedes the frozen planning-time design. It reconciles the original document with what execution actually built: the tx-aware schema snapshot, the genesis completion marker, storage-embedded blob collections, engine files keyed by a persisted file-base id behind the storage-format v24 gate, and the version-keyed strict import matrix. Decision codes appear only in section footers and will be restated in full by the companion adr.md; invariants are carried as prose.
Companion to design-final.md: restates decision records D1-D23 in full (including the corrected as-built engine-file identity record and the genesis-marker and blob-collection records added during execution), so the design's section footers have a durable resolution target. Carries the invariants as prose contracts, the accepted risks folded into their owning decision records, the surviving limitations, and the adversarial gate verdict trail that otherwise dies with the workflow scaffolding and the PR-body notes at the ready-for-review flip.
Corrections from the post-commit fidelity reviews of design-final.md and adr.md: - Snapshot-tier consumer figure refreshed from the stale mid-execution 174 to ~190 (re-counted at HEAD: 98 production + 91 test call sites), consistently across both artifacts. - The changed-class marking described as its actual three channels (write choke point, explicit create/drop/rename marking, root-payload diff) instead of a single-site simplification. - Serializer synchronization disambiguated to the schema write lock in both artifacts. - adr.md gate-verdict trail gains the mid-execution adversarial re-review of the concurrency-control design (three rounds, 2026-07-21, blockers resolved, re-approved) — recorded before the workflow archive deletion destroys the source. - Operator migration runbook cited by bare filename in adr.md. - design-final.md polish: genesis lazy-creator edge case no longer reads as contradicting its TL;DR, migration rehearsal cited by test class name, preposition pileup fixed, schema-carry shorthand introduced, all TL;DR blocks trimmed to the 5-line template cap.
The previous fix pass corrected design-final.md but propagated only part of each correction to adr.md, leaving cross-artifact contradictions in permanent records: - D6 and the Component Map now state the changed-class marking as its three actual channels (write choke point on routed writes, explicit marking by whole-schema create/drop/rename, root-payload diff) — matching design-final.md and the code. - D18 no longer claims the functions/sequences libraries stay on the legacy creation path: genesis creates their classes in its schema transaction; only the lazy create-if-absent seam stays legacy. - The Constraints serializer bullet names the schema write lock, like every other serializer-contract site. - The migration-rehearsal citation unified to the test class name in both records, matching the artifacts' own convention. - The unintroduced schema-carry shorthand removed from adr.md prose. Symmetric sweep also caught the reverse gap: design-final.md's migration section now cites the operator runbook by bare filename, as adr.md already did; plus one over-long prose line re-wrapped.
The reconciliation section's footer still glossed D6 with the superseded single-channel phrasing; the body and the ADR record both describe the changed-class signal as three marking channels.
Move design-final.md and adr.md from the working plan directory to docs-internal/adr/transactional-schema/, matching the archive's per-effort-subdirectory convention (adr.md + design-final.md per entry, as in the sibling entries). The two artifacts reference each other by bare filename, so no cross-reference changes are needed; the operator migration runbook stays under docs/ as a user-facing product document.
Delete the docs/adr/transactional-schema/_workflow/ archive (plan, frozen design, track files, research log, adversarial passes, review scaffolding, mutation log — 204 files). Its decision and mechanism content was distilled into the two permanent artifacts now living at docs-internal/adr/transactional-schema/ (design-final.md, adr.md), including the adversarial gate-verdict trail folded into adr.md before this deletion; the artifact-review substance was extracted into the issue-draft archive outside the repo. The now-empty docs/adr/ directory is removed with it.
Andrii Lomakin (andrii0lomakin)
force-pushed
the
transactional-schema
branch
from
July 28, 2026 07:10
239503d to
d409e96
Compare
CI intermittently killed the forked test JVM through the deadlock watchdog: a schema-carrying commit holds the schema write lock and blocks on the index-manager write lock (commitSchemaCarry's documented four-lock order), while an index-manager exclusive region holds the index-manager write lock and blocks on a fresh schema READ lock frames down inside record deserialization - the immutable-snapshot rebuild that runs whenever the shared snapshot cache was concurrently invalidated. ReentrantReadWriteLock.lock() is non-interruptible, so the cycle survived test timeouts and surfaced only as fork death (~2.5% per run locally). The inversion family covers every non-commit exclusive region of IndexManagerEmbedded (load, reload, the legacy eager createIndex/dropIndex, membership add/remove, addIndexInternal), all of which do entity work under the lock; the latent defect predates this branch on develop, but the branch made the schema-write->index- write side common to every schema-carrying commit. Fix: IndexManagerEmbedded.acquireExclusiveLock now takes the committed schema READ lock before the index-manager write lock and holds it for the whole exclusive region (released in reverse order by releaseExclusiveLock). Every call site routes through this single pair, so the whole family conforms to the documented order at once. Holding the read lock (rather than pinning a snapshot) is deliberate: the legacy drop of a non-empty index batch-deletes entries on a COPIED session (IndexAbstract.clearAllEntries), whose separate thread-local snapshot state a pin on the calling session would not cover, while a held read lock makes every nested schema-lock acquisition on the thread - any depth, any session - reentrant and non-blocking, and freezes the committed schema for the region so no staleness window exists. The commit path's acquireExclusiveLockForCommit stays exempt: it already holds the schema write lock, conforming to the same order. Enforcement: SchemaShared now carries an always-on runtime lock-order guard (wired to the index manager by SharedContext.init) that throws IllegalStateException on a fresh schema-lock acquisition while the thread holds the index-manager lock, and on a read-to-write upgrade of the schema lock (which self-deadlocks on RRWL). Always-on rather than assert-only for the same reason as the metadata-write mutex engage-order guard: production JVMs run with -da, and the silent alternative is a non-interruptible parked deadlock. The guard uses the locks' own per-thread accounting (no extra bookkeeping, no allocation) and measures within noise of a bare read-lock acquisition (<0.1 ns on a ~5.3 ns operation). Deterministic regression tests exercise the previously-inverted paths with the snapshot cache force-invalidated, so a regression now fails loudly on first execution instead of hanging one CI run in forty; the existing concurrent race test stays as the stress complement. Post-fix stress: 40/40 clean runs.
Test Count Gate ResultsTolerance: 5% drop allowed per module Overall: ✅ 30200 tests (baseline: 29805, +395)
|
Coverage Gate ResultsThresholds: 85% line, 70% branch Line Coverage: ✅ 88.7% (2764/3116 lines)
Branch Coverage: ✅ 81.2% (1300/1601 branches)
|
Two findings from the baseline review of the ABBA lock-inversion fix (deadlock-fix-baseline-iter1: TQ34-1 should-fix, CQ35-2 suggestion). TQ34-1: the guard tests had no @test(timeout). If the read-to-write upgrade guard ever regresses, schemaReadToWriteUpgradeThrowsInsteadOf- SelfDeadlocking parks forever on the non-interruptible RRWL write acquisition - the test would BECOME the silent forked-JVM death this fix exists to eliminate, leaving no surefire failure record and making the regression invisible. Both guard tests now carry timeout=60_000 (matching the sibling stress test, ~80x observed runtime, so it cannot flake under CI load) plus the watchdog-thread session re-activation a timed body requires. Validated by neutering the upgrade guard: the test now reports a named TestTimedOutException at 60s instead of fork death. The other three deterministic tests deliberately stay without timeouts: they run single-threaded with no contending thread, so no lock acquisition in them can block - their regressions surface as the guard's IllegalStateException, never as a park. CQ35-2: releaseExclusiveLock re-resolved the schema instance through session.getSharedContext() instead of reusing the instance the acquire read-locked. Unreachable today (SharedContext.reInit has no callers), but it was the one structural fragility with a real leak shape: a re-init between acquire and release would unlock a DIFFERENT SchemaShared and leak the original read hold, parking every later schema write forever. The outermost acquireExclusiveLock now captures the locked instance in a field guarded by the index-manager write lock; nested acquires and every release reuse the capture, so the region locks and unlocks one and the same instance end to end. A non-commit entry under the commit-path acquisition (which captures no schema) now fails with a targeted IllegalStateException instead of the generic guard message. Lock acquisition order, release order, and the exception-path release balance are unchanged on every reachable path.
The post-flip ABBA deadlock fix changed the concurrency contract in ways the permanent record must carry: - New decision record D24: the always-on runtime lock-order guard on the schema lock (probe wired at shared-context init), the ordered index-manager exclusive acquire (schema read lock first, held across the region), the exempt commit-path acquire, and why always-on beat assert-only. Cited from design-final.md's lock-order section. - The documented four-lock order is stated honestly as incomplete: the per-index lock inside IndexAbstract sits outside it (rebuild, fill, and standalone delete take it above the ordered tiers without the index-manager lock held) — a pre-existing latent inversion family the guard does not see, tracked in the issue tracker. The storage-state tier remains runtime-unassertable. - The stall-envelope characterization gains the genuinely new edge: index-manager exclusive regions now hold the schema read lock across potentially unbounded region I/O, stalling non-commit schema writers even without an overlapping commit. Both artifacts amended in the same pass and cross-grepped so neither carries a claim the sibling contradicts.
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.
Motivation
Before this change, storage led a schema change: creating or dropping a class or an index mutated storage structure first (the collections and the index engines), then reflected the result into a metadata record. Each such operation self-committed in its own micro-transaction outside the user's transaction, the whole schema lived in one record rewritten on any class change, and a schema change could not be rolled back with the transaction that made it. YTDB-382 targets exactly this: the write amplification of the monolithic schema record and the non-transactionality of schema operations.
Planned changes
Current state (before this branch). Schema and index operations self-committed outside the user transaction; the entire schema was one record; rollback of a schema change was impossible; a class rename rewrote storage files through a rename path the WAL did not journal; entity validation silently skipped constraints created in the same transaction.
What changes. Schema and index DDL is fully transactional: a change mutates only metadata records during the transaction, stays invisible to other sessions until commit, and rolls back for free with the user transaction — a rolled-back or crashed-before-commit transaction leaves storage files byte-for-byte unchanged, and a committed structural change replays from the WAL. Per-class schema records replace the monolithic record, so a one-class change writes one record. A class rename touches zero storage files and keeps every index accelerating under the new name; an index rename is metadata-only. The immutable schema snapshot is transaction-aware, so entity validation and serialization enforce same-transaction classes, property types, and constraint rules instead of silently skipping them. Exactly one schema-changing transaction runs at a time, serialized by blocking (never aborted on contention). Three on-disk formats move: the schema record format (4 to 6, strict equality gate), the storage-configuration format (23 to 24, rejected in both directions), and the export dump format (14 to 15); opening an old-format database is rejected with a redirect to the operator export/import migration, and a genesis completion marker makes a crashed half-created database refuse to open loudly.
How. During a transaction, schema writes route through
SchemaProxyto a per-session copy-on-first-writeSchemaShared, seeded by a read-only re-parse of the committed records;SchemaProxedResourceis the write choke point, andTxSchemaStatecarries the copy, the changed-class set, and theIndexOverlay(index definitions overlaid: committed + tx-created − tx-dropped). Collections created in the transaction carry provisional negative ids resolved to real ids at commit before any record serializes. At commit,AbstractStoragediffs committed against tx-local collection ids (a rename is structurally inert), drops and creates collections and engines inside the commit's own atomic operation, builds a tx-created index eagerly only when its source collection is empty (a populated source is rejected loudly), promotes the tx-local schema into the existing shared instances, and publishes the overlay as replacement objects — all under a fixed four-lock order headed by the newMetadataWriteMutex, with the storage write lock held from commit entry (pure-data commits keep the read-lock fast path).OperationsFreezergains a freeze-kind taxonomy so a schema commit throws loudly with zero locks held against an operator freeze while parking normally for transient quiesces. A late, CI-caught ABBA inversion between that commit sequence and the index manager's exclusive regions — which could reach a fresh schema read lock deep inside record deserialization — was fixed by funnelling every non-commit exclusive region ofIndexManagerEmbeddedthrough one ordered acquire (committed schema read lock first, index-manager write lock second, held across the region), and the lock order is now enforced by an always-on runtime guard inSchemaShared(overhead within noise, deterministic regression tests). Index-engine files are keyed by a persisted per-engine file-base id and collection names by a bare counter — that is what makes renames metadata-only. Genesis is a two-phase bootstrap (one schema transaction, then one data transaction) with the blob collections created by storage itself. Migration ships as hardenedDatabaseExportandDatabaseImport(fail-closed, version-keyed import strictness) plus the operator runbookoperator-migration-procedure.mdin the product docs. The permanent design record — design-final.md and adr.md, the latter restating every decision record in full — lives in docs-internal/adr/transactional-schema.Key decisions. The schema gets a full tx-local copy rather than a merge-on-read overlay (the copy reuses the existing derived-state recomputation; an overlay would re-implement it inside the read path), while indexes get the opposite call — a definition overlay, never a content copy, because an index is a thin handle over a storage-backed engine. Single schema writer by blocking; contention-abort was rejected. The commit-time delta reads the transaction's own change tracking (no separate intent list), with drops detected as a set difference over collection ids. Structural revertibility rides the existing atomic-operation WAL with no deletion pool, resting on a replay fix that materializes a missing file from the create record inside its own atomic unit. Engine files are keyed by a persisted file-base id after the planned registry-slot keying was falsified (slot ids are reused by design after failed-commit cleanup). The tx-aware snapshot replaced both per-field proxy resolution (too slow for the validation hot path) and the committed-only snapshot (silently skipped same-transaction constraints). Format migration is operator-driven export/import; an in-place on-open migrator was rejected so new code never parses the old format and no partial-migration state exists. Lock-order conformance is enforced by an always-on runtime guard rather than review discipline alone — assert-only was rejected because production runs with assertions disabled, and the silent alternative is a non-interruptible parked deadlock.
Out of scope. The populated-source (off-lock, streamed) index build and incremental index-manager link-set serialization (YTDB-1064); the inert index-name rename and an explicit rename statement (YTDB-1066); the residual index-visibility window for concurrent pure-data commits (YTDB-1101); cross-thread reaping of a stranded schema transaction (YTDB-1114). A rid-mapping-aware database comparator was considered and not commissioned — the import renumbers collections, so id-keyed comparison is structurally unsatisfiable and migration verification is logical equivalence; a cross-section point-in-time export pin remains a possible future design. The per-index lock inside
IndexAbstractremains outside the documented lock order (the index rebuild, index fill, and standalone index delete paths take it above the ordered tiers) — a pre-existing latent inversion family the runtime guard does not cover, tracked separately in the issue tracker.Risks & accepted trade-offs
Design review: user-approved — 2026-06-16 (carried over from legacy workflow gates)
Adversarial review: passed, 5 accepted risks — 2026-06-15 (carried over from legacy workflow gates)
No branch artifact enumerates a labeled five-item list — that verdict line was synthesized at a mid-branch workflow migration. The accepted residuals on record at design close were the four issue-backed deferrals listed under Out of scope (YTDB-1064, YTDB-1066, YTDB-1101, YTDB-1114) plus the one genuine trade-off: a schema-carrying commit holds the storage write lock for its whole duration and excludes concurrent data commits, accepted on the premise that the schema-change rate is low — the same premise that bounds the stall envelope together with the empty-source-only in-commit index build.
Two mid-execution adversarial design gates also ran and passed: the engine-file identity decision was revised (two blockers resolved) and re-approved before implementation, and the concurrency-control design (the metadata-write-mutex lifecycle and the freezer gate) was re-reviewed over three rounds on 2026-07-21, all blockers resolved by amendment and the amended design re-approved. The full per-decision accepted-risk register (engine-file stems, mutex lifecycle, freezer gate, including the pre-existing identity-less freeze-counter residuals) is restated in adr.md under docs-internal/adr/transactional-schema.
Standing note for reviewers: the earlier flake-family classification of
SchemaCommitReconciliationTestdisk-profile failures is withdrawn — the intermittent CI kills included a real ABBA lock-order deadlock, fixed on this branch with a runtime guard and deterministic regression tests. Treat any failure there as a genuine signal first.Verification approach
Current state, after the post-flip lock-order fix: Spotless clean; full core and tests unit suites green with zero failures; integration profile green (0 failures, ~3h17m); coverage of changed lines 88.2% line / 79.9% branch versus develop; CI green on all platform legs.