mirror of
https://github.com/mentalfaculty/LLVS.git
synced 2026-09-26 10:01:28 +02:00
Drew McCormackandClaude Opus 5
f168aa90f9
* Add design for the projection layer LLVS is a log, which is good at history and merging and bad at answering "show me today's notes". The three shortcomings raised (no querying except by ID, cloud data too fine-grained, no coalescing) are one problem: a log needs pairing with something that is not a log. The design keeps LLVS as truth and projects version diffs into a SQLite current-values view, which is what gets queried. Map diffing already supports this: differences() is a set difference between two arbitrary versions, so a merge commit or a move backwards onto a branch needs no replay and no reverse diff. Records the two decisions with consequences downstream: the projected columns are an index rather than a copy, so adding a queryable field is a rebuild and never a migration; and a blob that will not decode is skipped and reported, so one unreadable value neither freezes sync nor disappears unannounced. Map bucketing must be fixed first. LLVSModel IDs put every note in one bucket, which is O(N) per write and fatal at the target size. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Add implementation plan for the projection layer Seven tasks, each with its own test cycle: flip model value IDs off the crowded Map bucket, add transactions to SQLiteDatabase, expose a correct diff between two arbitrary versions, then build LLVSProjection on top of those three. Also corrects the spec. Its claim that the DAG case was already served by existing API was wrong: Map and Map.Diff are internal, and the one public route passes the first version as its own common ancestor and calls fatalError on the two-branch forks, so it traps on exactly the case the projector needs. Resolving the ancestor and folding those forks is now a task rather than an assumption. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Put the instance identifier first in model value IDs The Map buckets values by the first two characters of their ID, and LLVSModel built IDs as "Type/instance", so every instance of a type landed in one bucket and each save rewrote a node listing all of them. That is O(N) per write. Audit item 7. The instance identifier now leads. The helpers split on the last slash rather than the first, so an app-supplied instance identifier may contain slashes and the type name still survives. fetchAllModels matched an ID prefix, which the flip breaks. It now compares the type identifier through the same helper, so the ID layout has one owner. The cost is that it scans every reference at the version, there being no prefix left to narrow with. Nothing else relied on the layout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Add transactions to SQLiteDatabase A projection must apply its rows and record the version they came from together, or a crash between the two leaves no way to tell what had been applied. SQLiteDatabase had no transaction support, so there was no way to express that. The rollback is deliberately try?: the caller's error is the one worth reporting, and a rollback failure would mask it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Add a public diff between two arbitrary versions Keeping a derived view in step with a store means asking what differs between the version the view holds and the version it must reach. After a sync and merge those two are sideways from each other, not in a line. Map.differences answers that correctly given a true common ancestor, but Map is internal. The one public route, valueChanges(madeBetween:and:), passes the first version as its own common ancestor, which holds only when it is an ancestor of the second, and calls fatalError on the forks that arise when it is not. It traps on exactly the case a derived view needs. The new call resolves the real ancestor and then decides each change from whether the value exists at each end rather than from the fork label, so the two-branch forks need no special handling. The existing call is left alone; it is public API with callers. Resolving the ancestor walks the graph, so cost grows with history length rather than with the size of the difference. Every merge already pays this. Worth measuring before it matters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Add the LLVSProjection target and its schema description type ProjectedType is the one thing an app must write: which table a stored model type becomes, which columns it carries, and how to get those column values out of a value's data. The columns are an index rather than a copy. Declaring only what is queried or sorted on keeps writes cheap, and leaves the store as the only place the whole object lives, which is what makes adding a field later a rebuild rather than a migration. extract throws to mark a value unreadable, for when another device wrote a model this build cannot decode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Project version diffs into SQLite in one transaction Each pass asks the store what differs between the version the database holds and the version it must reach, then writes those rows and the new version marker together. A failure part-way rolls both back, so the database is either current or behind, never half-updated, and behind is repaired by running again. Because the diff is a set difference rather than a replay, a merge commit or a move backwards onto a branch needs no special handling. An unreadable value is skipped and its ID returned. Failing the pass would let one value another device wrote freeze the whole projection; dropping it silently would lose it from the UI with nothing to show for it. schemaVersion is the app's own number for the shape of its tables. Raising it rebuilds rather than diffing onto tables built for different columns, which is why there are no migrations here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Test that a failed projection pass rolls back whole The single-transaction claim was asserted but never checked. These tests fail a pass part-way and require that nothing it wrote survives and that the version marker did not move. Getting this to test anything took two attempts. Failing the pass by dropping the table failed its very first write, so there was nothing written to roll back and the tests passed with transactions removed. They now fail on a CHECK constraint that rejects one title, after earlier rows have already been written. Verified by removing BEGIN and COMMIT and watching two of them fail, then restoring. Caching hid that on the first attempt: swift test --filter reported a pass against a build that did not contain the mutation, which is worth knowing the next time a mutation looks survivable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Follow the coordinator's version, and document LLVSProjection ProjectionFollower keeps a projection in step with a coordinator, either by being awaited after each save or by draining the version stream. It is an actor owning both the projector and the SQLite connection, which the first attempt was not. Following from a background task while the test saved on another crashed the suite: Store is @unchecked Sendable but only its history is behind a mutex, so the value map is read and written concurrently. That is audit item 9, and it is the library reporting a real race rather than a test problem. Neither object is thread-safe, so neither is now reachable from outside the actor: the database is opened from a URL rather than handed in, and queries go through query(). The README example is also compiled, as ReadmeExampleCompileCheck. The samples went stale once before and had to be rewritten wholesale (audit item 22); one that is built cannot drift unnoticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Record the concurrency constraint in the spec, plan and README The design said nothing about threads, which turned out to be the hardest constraint in the work: Store's value map is unguarded, so a pass reading it while the app saves crashed the suite with a signal. The spec now carries that, and the plan's Task 7 is marked superseded rather than left describing an API that cannot compile. The README says the part an adopter needs: await projectCurrentVersion() after a save, because the actor cannot stop an app saving on another thread while a pass runs. Closing that properly is audit item 9. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Make the projection rollback tests catch a lost transaction Code review found the rollback tests were not reliable guards: with BEGIN and COMMIT removed they still passed 2 of 12 and 4 of 12 runs respectively. The cause is ordering. Map.differences builds its result from Sets of keys and IDs, and Swift seeds its hasher per process, so the order of changes in a pass varies from run to run. Failing the pass by rejecting one row's content therefore hit first on some runs and last on others, and on the runs where it went first nothing had been written and there was nothing to roll back. They now fail on a trigger counting inserts, which holds whatever the order, and catch a removed transaction 12 of 12. Also recorded which two tests are the atomicity guards. The other two catch nothing when the transaction goes, correctly: they cover recovery, and without a transaction the work still completes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Correct why the projection follower is an actor The crash that drove this design was recorded as a Store data race, on the grounds that Store is @unchecked Sendable with only its history behind a Mutex. Code review checked that under Thread Sanitizer, 300 concurrent saves against 300 concurrent passes, and found no race: Map holds only a let zone and a Mutex-protected Cache, and FileZone is the same. The real cause is SQLiteDatabase, which documents itself as not thread-safe and serialises nothing. The crashing design had the test reading the projected database while the follower's task wrote it. Four threads sharing one SQLiteDatabase, with no LLVS store involved, reproduces the same signal on its own. The actor stands, and for a better reason than the one given: it serialises the database, which genuinely needs it. Store's own serialisation stays open as audit item 9, and this design does not depend on it. The README caveat went too: it warned against saving during a pass on a premise that turned out to be false. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Skip diff entries both versions already share Code review noted that a value updated to the same content on both branches still came back as an update. The diff can also mention a value neither end touched, and that was reported too. Both cases are now skipped by comparing stored version ids: two references to the same stored version are the same bytes. That costs a map lookup on the from side, which the code already did to test existence, and saves reading the value and writing it back unchanged. Comparing the data itself would also work and catch two separate copies of identical bytes, but it means reading both values, which is the expensive half. Identity of the stored version is the part that is free. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Test that saving during a projection pass stays safe Every other test here awaits its pass after the save, so nothing covered the case the follower exists to handle. These run saves against passes, drive the same thing through the version stream, and read the projection while it is being written. They pass, and pass under Thread Sanitizer, which matches what code review measured independently. The property is asserted rather than described because it is accidental: Map's fields are a let zone and a Mutex-backed Cache, and FileZone is the same, so concurrent readers and writers meet only in file I/O and a lock. Store promises none of that. A comment saying so would go stale in silence the day Map gains mutable state, which is the day this matters; a test starts failing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Guard Store's concurrent-read property, and check it in CI The concurrency guard was in the projection tests, where it reached Map's immutability through two intermediary layers: slow, indirect, and liable to be blamed on the projection when it failed. Code review pointed out it belongs against Store, which is where audit item 9 lives and where someone changing Map would look. It is written to fail without a sanitizer, on a crash or a wrong answer, because CI runs none by default. A second CI job now runs the concurrency suites under Thread Sanitizer, so the race is caught as a race rather than waited on. Both pass today, which matches what code review measured. Projector+Coordinator.swift keeps one line about it: the constraint, that Store promises no serialisation, rather than the mechanism, which would go stale silently the day Map changes. Also from review: ProjectedType now rejects a table or column name that is not a plain ASCII SQL identifier. SQLite has no binding for identifiers, so they are interpolated; a typo becomes a clear failure at construction rather than a syntax error at the first write. And query's doc comment says the compiler prevents the database escaping, which it does, rather than asking the caller not to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Take the history walk off the projection write path The spec called the common-ancestor walk "not new risk, since every merge already pays it". Code review pushed back, and was right: a merge is occasional, a projection pass runs on every save, so the design had moved an O(history) operation onto the write path. Measured: one diff cost 0.68 ms at 100 versions and 17.19 ms at 3000. Linear, per save. Two consecutive versions are almost always on one line, and then the source version is the common ancestor. isAncestor searches back from the target alone and stops on a hit, so the full search runs only when that cannot settle it. The same measurements are now 0.13 ms and 0.36 ms. A false from isAncestor means "not found within the limit", not "not an ancestor", so the caller must treat it as inconclusive and fall back. That is what the limit buys and it is tested. What remains is not the walk. Over fifteen times the history a diff still costs about 2.4x; probing the parts shows ancestry flat at 0.002 ms and map lookups flat, so the rest is Map bucket size — audit item 7, from the read side. With IDs sharing a prefix it is about 11x. The new performance test bounds that and fails at 11.7x if the shortcut is removed, which I checked in a throwaway worktree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Cap the unreadable IDs a projection pass reports A version-skew event, where a newer device writes a model shape this build cannot decode, can make every value in a rebuild unreadable. Handing the app one ID per value then costs memory proportional to the store and tells it nothing the count does not. The list stops at 100 and unreadableCount carries the true number. Also fixes a README line still describing the old "Type/instance" value ID layout, which the ID flip changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Add design for local-first SQLite over LLVS The projection layer is one-way, so an app still writes through StoreCoordinator.save and a write to the SQLite is destroyed by the next rebuild. What an app wants is to use SQLite the way any SQLite app does, with syncing following from that. SQLite becomes a second write path into LLVS rather than a second truth: triggers capture row and column changes, a drain step turns them into a version, and incoming versions apply back under a suppression flag. All four mechanisms were checked against SQLite before writing this down, including that a multi-row UPDATE fires per row and that a no-op update records nothing. The merge needs no new code. A row becomes a value, values merge in LLVS as they always have, and because MergeableArbiter already merges property by property, a column is a property and column-level merge falls out. Same-column collisions go to the MergeArbiter, which is what an app's arbiter is already for. Scalars get real indexable columns rather than JSON with duplicate index columns; only nested properties become JSON, and only that property rather than the whole type. The macro already walks the stored properties and has the type annotation in reach. Three questions are named and left open rather than guessed: owned-table migration, delete and tombstone rules, and whether an app's own transaction should bound a version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Specify typed reads for owned SQLite tables An owned table is generated from a Swift model, so handing its rows back as columns throws away what the declaration already knows. Reads now hydrate into the model and carry the llvs_id and the version they were read at, so a row knows where it came from and a later typed write has what it needs. Writes stay SQL on purpose. An UPDATE that sets one column says only that column changed, and the per-column trigger records that. A save(model) writing every column would claim they all changed and destroy the granularity that lets concurrent edits to different columns merge. A typed write has to diff against the stored row first, which is worth building deliberately rather than getting for free by writing whole rows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Settle the three open questions in the SQLite design A plan cannot carry a TBD, so each is decided with its reasoning. Migration is drain-then-rebuild, and the order is forced: rebuilding first would discard local edits that never reached the truth. A failed drain cancels the migration, because running on the old schema beats losing unsynced work. A delete is a removal, and an edit on another device brings the row back, which is what the default arbiter already does with removedAndUpdated. A returning row is visible and fixable; a silently discarded edit is neither. Stating the rule is the point. Transaction boundaries need no mechanism. SQLite fires triggers inside the app's transaction, so a committed transaction yields one version and a rolled-back one yields nothing. Verified: an update inside a rolled-back transaction leaves no changelog row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Add implementation plan for local-first SQLite Seven tasks. The macro learns each property's column type; an owned table gets its capture triggers; captured changes drain into a version; incoming versions apply back under suppression; a round trip proves two devices editing different columns both keep their edit; reads come back typed; and a rebuild drains first. The mechanism the plan rests on was checked rather than assumed: binding.typeAnnotation?.type.trimmedDescription compiles against the SwiftSyntax already linked, so the macro can see a declared type. It cannot see an inferred one, so var x = 0 gets no column and is reported instead of guessed at. Typed writes, generating the table from the type alone, wiring an owned table into ProjectionFollower, and measuring a drain are all named as deferred with the reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Generate a SQLite schema from the model declaration @MergeableModel already walked every stored property and skipped let, static and computed ones correctly. It kept only the name, while binding.typeAnnotation sat unused beside it. It now also emits sqliteSchema: a real indexable column for each scalar property, and a JSON TEXT column for a type with no column shape. Only the awkward property becomes JSON, so an array of tags does not stop the title being a plain TEXT column. A property whose type is inferred gets no column. A macro sees only syntax, so var count = 0 has no visible type, and guessing Int would be wrong for var count = someExpression. Such properties are listed in propertiesWithoutColumns instead, which the existing MultipleBindingModel exercises. Column names are snake_cased, and one colliding with a SQLite keyword takes a trailing underscore rather than quoting, so it stays a plain identifier everywhere it appears. "when" was in the test set for exactly that reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Capture SQL writes to an owned table Three triggers record what an app's ordinary SQL changes: per row for an insert or delete, and per column for an update. The per column part is what the merge needs, so that two devices editing different columns of one row do not overwrite each other. Two behaviours come free from SQLite and are tested rather than assumed. A guard of OLD.col IS NOT NEW.col means a write that changes nothing records nothing, NULL included. And because triggers fire inside the app's transaction, a rollback takes the changelog rows with it, so an app's own BEGIN/COMMIT bounds a version without any mechanism. Suppression is a one-row table rather than a Swift property, because a trigger's WHEN clause is SQL and can only consult the database. It lifts even when the block throws, or capture would stay off for good. Clearing takes a sequence, because an app may write between a drain reading the changelog and finishing with it; those writes have to survive for the next drain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Turn captured SQL writes into LLVS versions A drain reads the changelog, assembles each touched row, and makes one version. Several edits to a row collapse into one change: the changelog says which rows and columns moved, the row says what they now hold, and the per-column detail is what the merge uses later. Three cases that would otherwise be wrong. A row inserted and deleted before the drain reached it produces nothing, rather than a removal of something never stored. A remove for a value the store does not hold is dropped for the same reason. And the changelog is cleared only through the sequence that was read, so a write landing while the version is being made is kept for next time. The stored JSON is keyed by property, not column, because that is what the model decodes from. A null column is omitted rather than written as JSON null, so an optional property decodes as nil and a non-optional one fails loudly rather than silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Apply incoming versions into an owned table Closes the loop. A version from another device, or one a merge produced, lands in the table under suppression, so it is not recaptured as a local edit and traded back and forth forever. A value that is not a JSON object is skipped rather than failing the batch, which is the rule the read-only projection already follows: one value this build cannot read must not stop the rest. A property the value does not carry binds as NULL rather than failing, so a model that has gained a property still applies and the new column simply reads back empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Test the owned table round trip and per-column merge Two devices, each with its own store and its own SQLite, both edit one row from a shared base: one changes the title, the other the body. After the merge both edits are present. That is the claim the whole design rests on, and until now it was only reasoned about. Checked that it tests what it claims: with the type unregistered, so the property-wise merge does not run, the test fails and one device's edit is lost. It passes only because a column is a property and MergeableArbiter merges properties independently. Also covers the stated delete rule, where an edit beats a delete and the row returns carrying it, and a same-column conflict, which resolves through the arbiter with the untouched column unharmed either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Read owned table rows as models, and drain before rebuilding Reads hydrate into the model and carry the value ID and the version they were read at, so a row knows where it came from and a typed write would later have what it needs. A row that cannot be decoded is left out rather than failing the query, so one bad row does not blank a list. Writes stay SQL, which is the deliberate half. An UPDATE naming one column says only that column changed; a whole-row typed write would claim they all did and cost the per-column merge that the round trip test proves. Rebuilding discards a table, so registerOwnedTable and drainOwnedTables get pending local edits into the store first, and a failed drain stops before anything is thrown away. The README's example is compiled as ReadmeOwnedTableCompileCheck, the same guard the projection example has, because these samples went stale once before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Store a Date column as Unix seconds Codable encodes a Date as seconds since 2001. SQLite's strftime, a database browser, and every other tool mean seconds since 1970. Storing Codable's number made the round trip work, because both ends used the same convention, while making the promise the design rests on false: an app writing strftime('%s', 'now') and reading it back got a date 31 years out, with no error. Found by checking the encoding rather than reading the code. The round trip test passed throughout, which is why it needed a test written from the app's side instead: write a Unix timestamp in plain SQL, read the date it meant. A Date column is now its own storage case, holding Unix seconds, with the conversion at the boundary. The macro comment claiming "seconds since 1970" was wrong and is fixed too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Key column conversion on the semantic type, not the SQLite one Code review found that a Bool did not round-trip in either direction. The drain wrote 1 into the JSON, which Codable refuses, so the value in the store never decoded; and fetch, which skips a row it cannot decode, returned an empty list with no error. Any model with a starred or isDone flag was silently unusable, and the failure shape was the worst kind. This is the Date bug again. Int, Bool and Date are all INTEGER, and the conversion was switching on the declaration string, which cannot tell them apart. Adding a .bool case would have fixed the instance and left the third one waiting. ColumnStorage now names the semantic type for every case, the declaration derives from it so the two cannot disagree, and both conversions switch exhaustively. A type that needs new handling is a compile error rather than a wrong answer. Also adds the test review asked for: one model carrying every supported type, driven both ways, asserting equality. Reverting the Bool conversion fails three of its tests, so it guards what it claims. That test would have caught both bugs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Apply only a table's own type, and allow a table with no columns Two findings from code review, both reproduced before fixing. apply wrote every change it was handed. A change set from valueChanges(updatingFrom:to:) carries every type in the store, so with two owned tables each ended up holding every value: rows with null columns where the properties did not exist, and real but wrong data where names collided. It now takes only values whose ID names its own type, which is the check Projector already made. A table with no columns could not be created at all. SQLite rejects a trigger with an empty body, and with nothing to capture the update trigger was exactly that. It is now left out. Reachable from a marker or tombstone type, or one whose properties all have inferred types. Also from review: fetch logs the rows it skipped rather than silently returning fewer, since silence is what let the Bool bug look like an empty table; failing to lift capture suppression is logged rather than swallowed, because leaving it set discards every later local write; UInt maps to INTEGER instead of falling through to JSON; and rowid is escaped, as it shadows SQLite's implicit alias. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Say plainly that fetch's version is a label, not a check Code review pointed out that atVersion is stamped onto every row without verification, which is a trap for something whose stated purpose is future optimistic concurrency. fetch genuinely cannot check it: an owned table records no version of its own, and the one that matters lives in Projector.projectedVersion(). So the documentation now says it is a label the caller attaches, and to pass the projected version or nothing, rather than implying a guarantee that is not there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Close the window where a local write is swallowed by suppression Code review found data loss with no signal, and reproducing it was the convincing part: a write landing while apply holds the suppression flag changes the row but records nothing, so the UI shows the user's edit and it never syncs. apply now takes the write lock before suppressing rather than after, so a writer on another connection waits instead of falling into that window. Nothing can protect a second thread sharing the same connection, so the requirement to drive one table from one place is now stated on the type, on the suppression call, and as the first of the README's rules — it is the one that loses data. Also from review: a typealias silently became a JSON column instead of an indexable one. A macro cannot resolve an alias, since that needs type checking, so it is reported in propertiesStoredAsJSON with the type as written rather than left as a silent demotion. And the drain's store check, which review noted was load-bearing but not obvious: a row deleted and reinserted between drains has insert as its last operation while still existing in the store, so it must go out as an update. Now commented, and covered by tests for all three sequences review ran by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Measure the drain against table size, and fix a load-sensitive test The drain reads one row per touched value, which should make its cost follow what changed rather than what the table holds. That was the right shape on paper and unmeasured, and the previous review's scaling predictions were sharper than my own, so it is now measured: a one-row drain takes 1.85 ms against 100 rows and 2.50 ms against 5000. Flat. The test asserts the shape rather than those numbers, so it survives a slower machine. Adding it surfaced a test that was already fragile. followUpdatesProjectsTheCurrentVersionFirst slept a fixed 200 ms and hoped, which is long enough on an idle machine and not on a busy one: it failed once in a full run while passing ten times on its own. It now waits for the row it expects. Six suite runs clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Stop a scalar in a JSON column killing the process Code review found an uncatchable crash on a path the previous commit had just started advertising. JSONSerialization raises an Objective-C exception, not a Swift error, for a top-level value that is not a collection: try? cannot catch it and the process dies. A bare string reaches a JSON column whenever a property's type could not be resolved — a typealias, or a raw-value enum — so propertiesStoredAsJSON told a developer their alias was a JSON column, and the first value through it killed the app. Worse, the value can arrive from another device, making it a sync-time crash on a device that did nothing wrong. Both directions now handle fragments: the write checks isValidJSONObject and falls back to .fragmentsAllowed, and the read says explicitly that it allows them rather than relying on the default. Removing the guard kills the test process with signal 6, so the test guards what it claims. Also records the scaling question review raised, which is sharper than the one I measured: the reads follow what changed, but one drain is one version, so a bulk UPDATE over 50,000 rows makes a single version carrying 50,000 values. Batching changes drain's return type, so the decision is cheaper now than later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg * Let an owned table record which version it is a copy of The design says SQLite is the working copy at a version, and then nothing recorded which one: the caller passed basedOn: and had to remember it. A checkout that does not know its own revision is not a checkout. Reproducing the consequence is what settled it. A drain based on nothing, when the store already holds versions, makes a version with no predecessor — a second root. One device that never synced with anything ends up with a forked history and two heads, silently. The table now keeps a state row, written in the same transaction as the changelog clearing, the way Projector already does for its own version. drain defaults to it, apply takes an optional atVersion so a sync can record what it brought, and basedOn: stays for deliberately building on something else. Found by code review, which noticed the gap between the framing and the code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kU8zpV5HdgtcqyTbDBkQg --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
46 lines
1.3 KiB
YAML
46 lines
1.3 KiB
YAML
name: CI
|
|
|
|
on:
|
|
push:
|
|
branches: [main]
|
|
pull_request:
|
|
|
|
permissions:
|
|
contents: read
|
|
|
|
concurrency:
|
|
group: ci-${{ github.ref }}
|
|
cancel-in-progress: true
|
|
|
|
jobs:
|
|
test:
|
|
name: Build and test
|
|
runs-on: macos-15
|
|
timeout-minutes: 30
|
|
steps:
|
|
- uses: actions/checkout@v4
|
|
- name: Select Xcode
|
|
run: sudo xcode-select -s /Applications/Xcode_16.4.app
|
|
- name: Swift version
|
|
run: swift --version
|
|
- name: Test (default traits)
|
|
run: swift test
|
|
- name: Build with all traits (Box and pCloud backends)
|
|
run: swift build --enable-all-traits
|
|
|
|
thread-sanitizer:
|
|
name: Thread Sanitizer
|
|
runs-on: macos-15
|
|
timeout-minutes: 30
|
|
steps:
|
|
- uses: actions/checkout@v4
|
|
- name: Select Xcode
|
|
run: sudo xcode-select -s /Applications/Xcode_16.4.app
|
|
# The concurrency suites assert that reading a Store while another thread writes it is
|
|
# safe. That holds today only because Map and FileZone keep no unguarded mutable state,
|
|
# which Store does not promise (AUDIT.md item 9). They fail without a sanitizer too, on
|
|
# a crash or a wrong answer, but under TSan they fail on the race itself rather than
|
|
# waiting for it to show.
|
|
- name: Test concurrency suites under Thread Sanitizer
|
|
run: swift test --sanitize=thread --filter 'ConcurrencyTests'
|