simply_based/SIMPLY_BASED_REVIEW.md

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:

# 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.