Skip to content

docs(selfhost): verify + document the shared backend's concurrency model#8141

Closed
joaovictor91123 wants to merge 1 commit into
JSONbored:mainfrom
joaovictor91123:docs/backend-concurrency-model-4942
Closed

docs(selfhost): verify + document the shared backend's concurrency model#8141
joaovictor91123 wants to merge 1 commit into
JSONbored:mainfrom
joaovictor91123:docs/backend-concurrency-model-4942

Conversation

@joaovictor91123

Copy link
Copy Markdown
Contributor

Summary

Verifies the SQLite and Postgres SelfHostD1Database adapters' actual concurrency guarantees against the real shared seam (src/selfhost/backend-contracts.ts, #4010) post-#7175's migration off node:sqlite-directly, and documents them explicitly instead of assuming the old two-local-process design still silently holds.

  • src/selfhost/backend-concurrency-model.md — what each backend guarantees (atomic single statements, all-or-nothing batch()) and what it doesn't (non-atomic read-modify-write loses updates on either backend).
  • test/unit/selfhost-d1-concurrency.test.ts — deterministic, in-process SQLite guarantees (matches the backend's real single-process/single-connection topology, per installation-concurrency-admission.ts's own documented posture): N concurrent atomic increments lose no updates, a failing batch() rolls back cleanly, a committed batch applies in order, an interleaved read never observes a rolled-back intermediate state, and the boundary case (non-atomic read-modify-write losing updates) is pinned as expected/documented behavior, not a bug.
  • test/integration/selfhost-pg.test.ts — extended with two PG_TEST_URL-gated cases exercising the same guarantees against a real Postgres pool (batch rollback on its own connection; N concurrent atomic increments across the pool).

No production code changes — verification + documentation only, against the already-migrated (#7175, closed) shared backend, using the existing test-double SQLite setup and the already-established PG_TEST_URL-gated integration pattern.

Test plan

  • npm run typecheck clean (no new errors beyond the pre-existing baseline)
  • npm run docs:drift-check clean
  • git diff --check clean (no whitespace/conflict markers)
  • CI: test:coverage (SQLite suite runs in every CI pass) and the Postgres integration job

Closes #4942

Verifies the SQLite and Postgres SelfHostD1Database adapters' actual
concurrency guarantees against the real seam post-JSONbored#7175, instead of
assuming the old two-local-process design still silently holds, and
pins each claim to a deterministic in-process test (SQLite) or a
PG_TEST_URL-gated live-Postgres test.

Closes JSONbored#4942
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@loopover-orb loopover-orb Bot added the gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. label Jul 23, 2026
@loopover-orb

loopover-orb Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-23 01:42:45 UTC

3 files · 1 AI reviewer · no blockers · CI failing · blocked

🛑 Suggested Action - Fix Blockers

Review summary
This is a test+docs-only PR that traces the actual synchronous SQLite driver (src/selfhost/d1-adapter.ts) and the pooled Postgres adapter (pg-adapter.ts) to justify each concurrency claim, and the reasoning holds up: because Statement.execSync()/batch() contain no `await` before completing, Array.from(...).map(() => d1.prepare(...).run()) and the batch() BEGIN..COMMIT/ROLLBACK sequence really do run to completion synchronously before control returns to the event loop, so the 'N concurrent atomic increments never lose an update' and 'no read observes a rolled-back intermediate state' assertions are correctly grounded in the real driver's synchronicity, not fabricated behavior. The BOUNDARY test's determinism claim (all N reads resolve before the first write lands) also checks out given Array.from synchronously invokes each async callback up to its first await. The new backend-concurrency-model.md doc accurately reflects d1-adapter.ts's batch() semantics and the Postgres pool's per-connection transaction isolation, and the PR explicitly closes #4942 with no production code changes.

Nits — 5 non-blocking
  • test/unit/selfhost-d1-concurrency.test.ts's makeDb() sets `PRAGMA journal_mode = WAL` on a `:memory:` DatabaseSync, but SQLite doesn't support WAL for in-memory databases (it silently falls back), so the comment claiming this 'keeps the seam under test aligned with the deployed configuration' overstates parity with src/server.ts:266's on-disk WAL setup.
  • Confirm the `src/server.ts:266` and `d1-adapter.ts:75-88` line references in backend-concurrency-model.md still point at the cited PRAGMA/batch code after any future refactor, since doc line-anchors silently rot.
  • The `validate`/`validate-tests (1/2/3)` CI checks failed with no detail provided in this diff's context, so the actual cause could not be verified from what's shown here — worth checking those logs before merging even though nothing in the visible diff explains a defect.
  • Consider a short inline comment in makeDb() acknowledging that WAL is a no-op for `:memory:` so a future reader doesn't assume the PRAGMA line is doing real work.
  • Since chore(selfhost): shared contract + tests for queue/adapter/vectorize backend pairs #4010's backend-contracts.ts already documents batch()'s atomicity contract, backend-concurrency-model.md could link back to that JSDoc directly rather than re-deriving it, to keep the two from drifting independently.

CI checks failing

  • validate
  • validate-tests (3)
  • validate-tests (2)
  • validate-tests (1)

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #4942
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 165 registered-repo PR(s), 74 merged, 4 issue(s).
Contributor context ✅ Confirmed Gittensor contributor joaovictor91123; Gittensor profile; 165 PR(s), 4 issue(s).
Improvement ℹ️ Insufficient signal risk: clean · value: insufficient-signal · LLM: moderate
Linked issue satisfaction

Addressed
The PR adds a documented concurrency model (backend-concurrency-model.md) covering what's guaranteed/not for both SQLite and Postgres adapters, plus concurrent load tests exercising real read/write access (increments, batch rollback, interleaved reads) for both backends, directly matching the issue's two deliverables.

Review context
  • Author: joaovictor91123
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, JavaScript, MDX, C++, CSS, Rust, TypeScript
  • Official Gittensor activity: 165 PR(s), 4 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb

loopover-orb Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (CI is failing (validate, validate-tests (3), validate-tests (2), validate-tests (1))). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

@loopover-orb loopover-orb Bot closed this Jul 23, 2026
@joaovictor91123

Copy link
Copy Markdown
Contributor Author

For whoever reviews this closed PR later: the CI failure here isn't a defect in the diff — it's a reproducible gap in `SCOPED_TEST_SELECTION`'s `vitest run --changed=origin/main` (ci.yml, ~line 991/1009).

`git diff --name-only origin/main ` correctly lists all 3 changed files, including the brand-new `test/unit/selfhost-d1-concurrency.test.ts`. But vitest's own `--changed` selection never picked that file up in any of the 3 shards — the run log only ever mentions the one modified file (`test/integration/selfhost-pg.test.ts`, 13/13 tests skipped since no `PG_TEST_URL` in CI), never the new one. Net result: zero tests actually ran, so `coverage/lcov.info` came out empty and `Verify coverage report exists` hard-failed — a false negative, not a real defect (the AI review itself found no blockers).

This exact shape has now closed 5 separate attempts at #4942 across 3 different contributor accounts (#8002, #8003, #8071, #8072, and this one) — every one a docs+test-only PR whose ENTIRE diff is new/modified test files with no accompanying `src/**` logic change for `--changed` to anchor a reverse-dependency walk on. `--changed` appears to reliably pick up a modified test file but miss a brand-new one, so any PR shaped like #4942's own scope (verification/docs work, no production code) is structurally unable to pass CI under scoped selection regardless of code quality.

I don't have permission to open a new issue directly on this repo (read-only), so flagging it here since closed PRs are apparently reviewed later. Happy to resubmit once this is fixed, or if there's a workaround I'm missing for triggering the full (non-scoped) suite on a test-only PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Re-evaluate the local concurrency model for a shared-service context

1 participant