Benjamin commit on credits ran out

CHANGELOG.md

@@ -1,6 +1,39 @@
 Changelog
 =============
 
+0.1.1 (unreleased, branch code-review-fixes)
+-----
+Independent code review against live Couchbase 8.0.1 CE (see
+SIMPLY_BASED_REVIEW.md). Fixes:
+
+- Embedded documents: fixed SystemStackError (infinite mutual recursion) when
+  accessing embedded collections loaded from the database; rebuilt
+  EmbeddedClass.all/.count (they referenced silently-dropped raw JS views —
+  .all returned [] and .count raised NoMethodError); the spec exclusion for
+  embedding was broken (filtered a tag no example carried) — basic embedded
+  flows now run green, only reverse lookups stay excluded, honestly tagged.
+- Store#view raises SimplyBased::UnsupportedView for view declarations the
+  N1QL translation cannot honour (:conditions / :emit_value / list functions)
+  instead of silently returning unfiltered results.
+- N1QL identifier quoting hardened (quote_ident rejects backtick smuggling);
+  index predicates quote class names as proper string literals.
+- ensure_index warns when index creation is swallowed (was fully silent while
+  every subsequent query fell back to a primary scan); bookkeeping is
+  mutex-guarded.
+- bulk_load / bulk_destroy now use batched KV multi-operations (get_multi /
+  remove_multi) — previously one KV round trip per document despite the
+  "batched KV" claim.
+- Counter#increment follows Redis INCR/INCRBY semantics (first increment of a
+  missing key returns the delta; it used to return 0 and undercount by one).
+  Decrement's unsigned clamp-at-zero divergence is documented and tested.
+- Lock#release requires the CAS handle (nil used to delete any lock
+  unconditionally); force_release! added; acquire validates a positive ttl.
+- Deterministic tree ordering: tree queries tie-break equal positions by
+  created_at and document id (was the main source of the "flaky ancestry"
+  failures).
+- The adapter-contract shared examples are now actually included by a spec;
+  added multi-threaded concurrency specs (counters, locks, CAS conflicts).
+
 0.1.0
 -----
 - Initial release: a Couchbase-native ActiveModel ORM, API-compatible with

README.md

@@ -70,11 +70,16 @@ SimplyBased::Lock.new.with_lock("rebuild", ttl: 30) do
 end
 ```
 
-- **Counter** — Couchbase binary increment/decrement (true atomic counters).
+- **Counter** — Couchbase binary increment/decrement (true atomic counters),
+  with Redis INCR/INCRBY semantics: the first increment of a missing key
+  returns the delta. One divergence: Couchbase counters are unsigned, so
+  decrement clamps at 0 instead of going negative.
 - **Cache** — an `ActiveSupport::Cache::Store` over KV + native TTL; drop-in for
-  `Rails.cache` and therefore ActiveJob.
+  `Rails.cache` and therefore ActiveJob. (`clear` is a no-op — entries expire
-- **Lock** — distributed lock: insert-if-absent + TTL auto-expiry + CAS-checked
+  via TTL.)
-  release so only the holder can unlock.
+- **Lock** — distributed lock: insert-if-absent + mandatory TTL auto-expiry +
+  CAS-checked release, so only the holder can unlock (`force_release!` is the
+  explicit override) and a crashed holder can never leave an immortal lock.
 
 ## The one recommended pattern shift
 
@@ -85,22 +90,41 @@ Couchbase **Collection** — tighter GSIs, cleaner queries, idiomatic Couchbase.
 
 ## Status & test coverage
 
-SimplyBased runs the **simply_couch test suite** against a live Couchbase.
+SimplyBased runs the **simply_couch test suite** (plus its own additions)
+against a live Couchbase. See `SIMPLY_BASED_REVIEW.md` for the full
+independent review this status section is based on.
 
-- **403 / 403 applicable examples green** on a clean run (one ancestry reparent
-  edge in the namespaced cascade is intermittently flaky).
 - **Green:** CRUD, finders (`find`, `all`, `first`, `find_by_*`, `count_by_*`),
   `belongs_to` / `has_many` / `has_one` / `has_and_belongs_to_many`, validations,
   callbacks, dirty tracking, soft-delete (incl. soft-delete-aware finders),
-  pagination, bulk, eager-loading, **ancestry / trees**, and the Redis primitives.
+  pagination, bulk (true batched KV multi-ops), eager-loading, embedded documents
+  (`has_many_embedded` / `is_embedded_in`, minus reverse lookups),
+  **ancestry / trees**, the Redis primitives, adapter-contract shared examples,
+  and multi-threaded concurrency examples (CAS conflicts, atomic counters,
+  lock mutual exclusion).
 - **Ancestry** is a materialized path (`path_ids`, root→self): children =
   `path_ids[len-2] = $id`, subtree = `$id IN path_ids`, roots =
   `ARRAY_LENGTH(path_ids) = 1` — plain N1QL, no recursive view build.
+- **Views that cannot translate to N1QL raise `SimplyBased::UnsupportedView`**
+  at query time (`:conditions` JS fragments, `:emit_value`, list functions)
+  instead of silently returning unfiltered results.
 - **Excluded by design:**
   - `:couchdb_only` specs — native inline attachments, JS-view token-safety,
     design-document maintenance, raw custom JS views: no Couchbase equivalent
     (use `has_s3_attached` / `has_local_attached`; N1QL uses bound parameters).
-  - `embedded_in` reverse-lookups — out of scope for this gem.
+  - `embedded_in` reverse-lookups (`:embedded_in_reverse_lookup`) — querying a
+    foreign key stored *inside* an embedded array element would need an
+    UNNEST-based query path; not implemented.
+
+### Known gaps (unchanged, deliberate)
+
+- **No multi-document transactions** — single-document atomicity + CAS only.
+  Couchbase's SDK has transactions; this ORM does not surface them.
+- **No index-migration framework** — GSIs are created lazily per association;
+  ad-hoc queries need deliberately designed indexes or they fall back to a
+  primary scan (a warning is emitted when lazy index creation fails).
+- `Cache#clear` is a no-op (namespaced clear would need a key scan); entries
+  expire via their TTLs.
 
 ```bash
 docker run -d --name cb -p 8091-8096:8091-8096 -p 11210:11210 couchbase:community

SIMPLY_BASED_REVIEW.md

@@ -0,0 +1,137 @@
+# simply_based — Independent Code Review
+
+**Reviewer:** Claude (Claude Code session, 2026-07-06), for Benjamin ter Kuile
+**Scope:** the gem as an artifact — correctness, test honesty, safety, production readiness. Whether mozo should sit on Couchbase at all is a separate, already-settled question and not revisited here.
+**Verified against:** live Couchbase Server 8.0.1 Community (aarch64), full suite runs before and after fixes. All changes on branch `code-review-fixes`; `main` untouched.
+
+> Verification environment note: this machine runs a 16K-page Asahi kernel, which Couchbase's jemalloc build refuses (native and qemu-user both fail). The server was run from the official image's extracted rootfs inside a `muvm` 4K-page microVM via bubblewrap, and the suite executed inside the same guest. Full recipe at the bottom.
+
+---
+
+## Verdict
+
+**Good bones, not yet production-grade — and closer after this pass.** The architecture is genuinely sound: the Store seam is clean, N1QL generation uses bound parameters, CAS-as-`_rev` optimistic concurrency is implemented correctly (no lost updates found, including under threads), and the Redis-replacement primitives are built on the right server primitives. But the review found one hard crash bug in shipped code (embedded documents), one silent-wrong-results footgun (`:conditions` views), several smaller correctness/semantic issues, and a headline test claim that does not reproduce as stated. For a gem written in one session, the hit rate is low; but every one of these would have burned a production user.
+
+What it is after this branch: a credible 0.1.x you could pilot on a low-stakes document-shaped workload, provided you accept the two structural gaps (no multi-document transactions, no index migrations). What it is not: something to put under money- or inventory-critical flows, because the transaction gap is architectural, not a bug.
+
+---
+
+## Does the "403/403 applicable examples green" claim hold?
+
+**Not as the repo stands.** Clean checkout of `main`, live Couchbase, default `bundle exec rspec`:
+
+```
+423 examples, 17 failures, 1 pending
+```
+
+All 17 failures are in `embedding_spec.rb`. The session report says embedded specs were "excluded by design" — but the exclusion line (`filter_run_excluding(:embedding)`) filters a tag **no example carries**, so the whole file ran and failed. Worse, the failures are not "unimplemented feature politely declining": basic `has_many_embedded` access on a database-loaded record died with `SystemStackError` (see finding 1), which means embedded documents were not merely out of scope — they were broken, and the broken state was invisible because the suite was believed to exclude them.
+
+The nuance in the gem's favour: the ~405 examples *outside* the embedding area do pass, which roughly matches the 403 figure — so the claim was most likely true of whatever tree state the original session measured, and rotted before commit. The report's other numbers (Redis primitives 5/5, `:couchdb_only` exclusions) check out. Its characterization of the flaky ancestry test as a save/race issue turned out to be wrong (finding 4).
+
+After the fixes on this branch: **442 examples, 0 failures, 1 pending** (see "Final verification" below), with exclusions that are real: `:couchdb_only` and `:embedded_in_reverse_lookup`, both tags that examples actually carry.
+
+---
+
+## Findings, by severity
+
+### 1. Embedded documents: `SystemStackError` on any DB-loaded access — **confirmed crash, fixed**
+
+`HasManyEmbedded::Property#build` wired `parent_object` onto each embedded record *before* installing the collection ivar on the parent. `parent_object=` reaches back into the parent's collection getter (via the `embedded_in` writer's `include?` check); with the ivar unset, that getter re-enters `build` — unbounded mutual recursion. Every one of `Post.find(id).embedded_comments`-style accesses crashed. Fixed by installing the ivar before wiring (`lib/simply_based/model/has_many_embedded.rb`).
+
+Two adjacent breaks in the same area, also fixed:
+- `EmbeddedComment.all` returned `[]` and `.count` raised `NoMethodError` — both were built on raw JS views that the N1QL port silently drops (`ClassMethods::Base#view` no-ops any `:map_function`/`:custom`/`:raw` registration). Rebuilt by enumerating parent documents (same work as the old CouchDB view scan, client-side; an UNNEST query is the future optimization).
+- The spec exclusion was fixed to be honest: basic embedded flows now run and pass; only reverse lookups (querying a foreign key held *inside* an embedded array element) remain unimplemented, tagged `:embedded_in_reverse_lookup` and excluded with an explanatory comment.
+
+`embedding_spec.rb`: 16 examples, 0 failures against live Couchbase.
+
+### 2. `:conditions` views silently return unfiltered results — **confirmed silent-wrong-results, fixed**
+
+simply_couch allows `view :completed, key: :name, conditions: 'doc.completed === true'`. The Store's N1QL translation read only `:key` and ignored `:conditions` entirely — the query ran *without the condition* and returned every document of the class. No error, no warning, plausible-looking results. This is the worst failure mode a storage layer can have. Same (lower-stakes) story for `:emit_value` and CouchDB list functions.
+
+Fixed: `Store#view` now raises `SimplyBased::UnsupportedView` with a pointed message when a queried view declares any untranslatable option. Declaring such a view stays legal (models port unchanged); querying it fails loudly. Covered by `spec/unsupported_view_spec.rb`.
+
+### 3. The "flaky ancestry race" is not a race — **root-caused, fixed**
+
+The session report: *"one ancestry reparent edge in the namespaced (by_property) cascade is intermittently flaky (~50%) — it does several dependent saves and occasionally races."* Reproduced at ~50% over repeated runs, captured the failure, and the diagnosis is wrong: the failing example is `NamespacedDirectory.roots` returning `[dir4, dir1]` instead of `[dir1, dir4]` — **an ORDER BY tie**, not a save race. Fixture records all share `position: 0`; `run_tree` ordered by `position` alone; N1QL tie order is whatever the index scan produces. `run_docs` already tie-broke by `created_at, META(c).id`; `run_tree` didn't.
+
+Fixed: `run_tree` now orders `position, created_at, META(c).id`; the two client-side tree sorts (`descendants`, `build_tree`) got the same tie-break because Ruby's `sort_by` is unstable. Ancestry spec then ran repeatedly without failure.
+
+The actual multi-save cascade in `children=` / `parent=` (many dependent saves, no transaction) is real and ugly — see finding 8 — but it was not what made the test flaky, and CAS conflicts in it would raise, not reorder.
+
+### 4. `Counter#increment` undercounts by one — **semantic bug for its stated purpose, fixed**
+
+Couchbase's binary increment seeds a missing key at `initial` (default 0) and *ignores the delta on creation*: the first `INCR` returned 0 where Redis returns 1. For the advertised "drop-in for Redis INCR" (rate limiters, job counters), that's a permanent off-by-one that nobody ever notices. The old spec even asserted the wrong value, codifying it. Fixed: `increment` seeds at the delta (Redis INCRBY parity), explicit `initial:` still overrides; `decrement`'s clamp-at-zero divergence (Couchbase counters are unsigned) is now documented and tested rather than implicit.
+
+### 5. `Lock#release(key, cas = nil)` unlocked other holders' locks — **safety footgun, fixed**
+
+With `cas` defaulting to nil, `release(key)` deleted the lock document *unconditionally* — any caller could unlock a lock it never held, which defeats the point of the CAS-checked release the README advertises. Fixed: the handle is required, `force_release!` is the explicit override, and `acquire` rejects nil/zero TTLs (a crashed holder must never leave an immortal lock). The TTL-expiry path (stale handle must not unlock the new holder) was already correct — now there's a test proving it.
+
+### 6. "Batched KV" bulk ops were one round trip per document — **claim/code mismatch, fixed**
+
+`bulk_load` looped `collection.get` per id; `bulk_destroy` looped `remove`. The SDK has `get_multi`/`remove_multi`. Both now use them — one KV round trip per batch, which is the behaviour the README and session report already claimed.
+
+### 7. Test-suite honesty issues — **fixed**
+
+- The adapter-contract shared examples (`spec/support/adapter_contract.rb`, "a simply_couch adapter") were shipped but **never included by any spec** — the parity proof they document never executed. Now included (`spec/store_contract_spec.rb`).
+- Zero multi-threaded examples existed despite CAS concurrency and Redis-replacement being the gem's core claims. Added `spec/concurrency_spec.rb`: concurrent counter increments (no lost increments), lock granted to exactly one of many contenders, `with_lock` mutual exclusion, stale-handle release after expiry, same-attribute CAS conflict (loser raises, winner's data intact), different-attribute auto-merge, and a disjoint-attribute write storm. All pass against the live cluster.
+
+### 8. Flagged but not fixed (with reasons)
+
+- **No multi-document transactions.** Real and architectural. Every multi-entity operation in the model layer — `children=`'s reparent cascade, `dependent: :destroy` fan-out, HABTM's write-to-the-other-side — is a sequence of independent CAS writes; a crash mid-sequence leaves half-applied state. Couchbase's SDK does offer transactions; surfacing them means designing an API (`SimplyBased.transaction do ... end`), deciding lambda-retry semantics, and touching every association mutator. That is a feature project, not a review fix. Until then this gem should not sit under flows where partially-applied writes are unacceptable.
+- **No index-migration framework.** GSIs are created lazily per association at first query, which is fine for associations but means ad-hoc `find_by_*` queries ride the primary index until someone notices. A `rake simply_based:indexes` that walks the models and declares expected GSIs would be the next increment. Mitigated in this pass: swallowed index-creation failures now warn instead of vanishing.
+- **`create` with a client-supplied duplicate id can silently merge into the existing document.** `save_document` maps `DocumentExists` to `Conflict`, and `auto_conflict_resolution_on_save` (default on) then loads the existing doc, adopts its `_rev`, and — if no attribute conflicts — retries as an *update*. Inherited simply_couch behaviour, arguably intended, definitely surprising. Left as-is; documented here.
+- **Model equality includes `_rev`.** Two loads of the same document at different revisions compare unequal (ActiveRecord compares by id). Associations and specs rely on the current semantics, so changing it is a breaking decision for the owner, not a review fix. Be aware of it when putting models in Sets or comparing across saves.
+- **`Cache#clear` is a no-op** (would need a key scan or a namespace epoch). Now documented in the README. An epoch-based namespace version (bump one counter, prefix keys) is the clean future fix.
+- **`is_embedded_in` remains legacy-shaped.** It still registers dead JS views (harmlessly dropped) and its `belongs_to` override — the reverse-lookup path — is unimplemented on N1QL (excluded, tagged). An UNNEST-based query path would complete it.
+- **Ancestry's `children=` mutates and saves eagerly** (assignment triggers N saves, old descendants get re-rooted and saved immediately). Correctness held up under test, but it's an API where assignment has heavyweight side effects. Design smell inherited from simply_couch; noted, not changed.
+
+### 9. Things checked that came back clean
+
+- **N1QL injection:** all *values* go through named parameters (`$rc`, `$v`, `$k0`…); `LIMIT`/`OFFSET` coerce through `Integer()`. Identifiers (property/column names) were interpolated with backtick wrapping — developer-controlled, so not an injection vector in practice, but now hardened anyway (`quote_ident` rejects backticks/backslashes; class names in index predicates use proper string-literal quoting).
+- **CAS discipline:** `persist` always sends the stored CAS on replace; a nil `_rev` implies `new?` and takes the insert path, so there is no replace-without-CAS route. `CasMismatch`/`DocumentExists` map to `SimplyBased::Conflict`; `retry_on_conflict` retries at most twice and only when auto-merge succeeds; merge conflicts surface with attribute names in the exception. Confirmed under threads: the same-attribute loser always raises, the winner's write is never stomped.
+- **Cache TTL >30 days:** Couchbase interprets big integer expiries as epoch timestamps — a classic footgun — but the Ruby SDK (`Couchbase::Utils::Time.extract_expiry_time`) converts correctly, so `expires_in: 60.days` is safe.
+- **Soft-delete-aware finders:** the `[key, nil]` element-wise comparison correctly matches `MISSING OR NULL` for non-deleted docs; live-verified.
+- **Pagination determinism:** microsecond `created_at` plus `META(c).id` tie-break in `run_docs` holds up; ascending/descending are exact reverses.
+- **Store thread-sharing:** the SDK cluster object is thread-safe; the one mutable Store structure (`@ensured_indexes`) is now mutex-guarded.
+
+---
+
+## What was fixed on this branch (commit story)
+
+1. `Fix embedded documents: recursion crash, .all/.count, honest spec exclusion`
+2. `Store: fail loudly on untranslatable views; quoting hardening; true batched bulk KV`
+3. `Counter/Lock: Redis INCR parity and safe lock release`
+4. `Add real concurrency coverage and actually run the adapter contract`
+5. `Deterministic tree ordering (the "flaky ancestry" fix)` + README/CHANGELOG truthing + this review
+
+## Final verification
+
+Full suite, clean run, live Couchbase 8.0.1 CE, after all fixes:
+
+```
+Run options: exclude {embedded_in_reverse_lookup: true, couchdb_only: true}
+442 examples, 0 failures, 1 pending
+```
+
+- The 1 pending example is `s3_spec.rb`'s `xit` (attachment-on-validation-failure), pre-existing.
+- Exclusions are tag-real: `:couchdb_only` (CouchDB mechanics with no Couchbase equivalent) and `:embedded_in_reverse_lookup` (documented gap).
+- `ancestry_spec.rb` re-run repeatedly post-fix with no failures (previously ~50% flaky).
+
+## Reproducing the environment (Asahi/16K-page hosts)
+
+Couchbase's jemalloc requires 4K pages; on an Asahi kernel neither the native arm64 image nor qemu-user x86_64 emulation runs. Working recipe:
+
+```bash
+# extract the official image's rootfs (arm64)
+podman pull --platform linux/arm64 docker.io/library/couchbase:community
+cid=$(podman create couchbase:community); podman export "$cid" | tar -C /var/tmp/cb-root -xf -; podman rm "$cid"
+
+# run server + suite inside a 4K-page muvm guest (shares the host FS)
+muvm --mem 4608 -- /var/tmp/cb-suite.sh
+# where cb-suite.sh: bwrap --bind /var/tmp/cb-root / ... couchbase-server &
+#   curl :8091/clusterInit (services=kv,n1ql,index, CE storage mode)
+#   create couchstore bucket simply_based_test, wait for :8093 query service
+#   COUCHBASE_URL=couchbase://Administrator:password@localhost/simply_based_test bundle exec rspec
+```
+
+Note muvm's inbound port forwarding did not deliver data on this passt version, hence running the suite inside the same guest rather than from the host.