fix(storage,ai,gemini,llamacpp): shared-connection safety + bulk BigInt + Gemini NOT_FOUND + checkpoint scoping - #657
Merged
Conversation
…andle Two storage instances that were handed the same SQLite `Database` or PGlite session share every write path but never saw each other's per-instance mutex, so an unrelated caller could slip a statement between another instance's `BEGIN` and `COMMIT` on the shared connection. Extract a `ConnectionMutex` keyed on the connection handle so every instance that binds the same handle queues on one chain, and use `AsyncLocalStorage` to detect same-instance re-entry (inline, no chain wait) versus cross-instance re-entry (`ConnectionReentryError`, no hang). Co-Authored-By: Claude <noreply@anthropic.com>
`JSON.stringify` silently drops `bigint` values and stringifies `Uint8Array` as its index-keyed object form, so two rows with a distinct BigInt or byte-string primary key produced identical fingerprints in `runBulkPut`'s dedupe map and Supabase's response alignment. Replace the direct `JSON.stringify` with a `pkFingerprint` helper that tags `bigint` (via `toString()`) and `Uint8Array` (via hex) so those primary keys stay distinct while every other input shape serializes byte-identically to today. Co-Authored-By: Claude <noreply@anthropic.com>
…k file The Gemini NOT_FOUND matcher two-tier work targets `providers/google-gemini/src/ai/common/Gemini_CachedContentFallback.ts`, which does not yet live on `main` — the cache-checkpoint feature branch carries it. Land a skipped test module now so the intended assertions are queued next to the file that will host them, and re-enable once the fallback file merges into `main`. Co-Authored-By: Claude <noreply@anthropic.com>
Add a stable `id` on `ResourceScope` so downstream tenant-scoped resource caches can gate re-use by the mint-site scope's identity, and a sessionId-keyed `withSessionLock` helper on the node-llama-cpp runtime so two concurrent keep-parent checkpoint consumers of one cached chat session can serialize their `session.prompt` / `LlamaChat.generateResponse` calls (and rewind KV between them) without overlapping on the sequence. The lock is session-scoped so unrelated sessions still run in parallel, and releases in `finally` so a thrown body does not strand the map entry. Co-Authored-By: Claude <noreply@anthropic.com>
Coverage Report
File CoverageNo changed files found. |
…ctionReentryError Two follow-ups on the ConnectionMutex introduced in the previous commit. F1 (browser ALS shim cannot detect cross-instance re-entry across await): the ALS-first check missed a legitimate cross-instance sibling call whenever the shim reset its `current` at the first `await`, so a sibling call on a different storage instance could sneak into another's BEGIN/COMMIT window on the browser bundle. Move the `state.txOwner` check ahead of `ensureAls` so the throw path is synchronous and ALS-independent; ALS survives only as an inline optimization on Node (browser shim falls back to chain-wait for same-instance re-entry, which the sanctioned tx-proxy path avoids anyway). Add a `classifyReentry(state, owner, alsStore)` helper and a `__resetAlsForTesting(useShim?)` export so the test suite can exercise both the real ALS and the shim paths. F2 (ConnectionReentryError gave no migration path): thread a `mode: "sibling-op" | "nested-transaction"` discriminator onto the error and rewrite the message to a multi-line block naming both callers, the mode, and the three supported refactors: (a) route the sibling through the `tx` proxy, (b) open a SAVEPOINT on `tx` for a nested rollback boundary, (c) collapse to a single storage instance or give each its own connection. `runOnConnection` throws with `"sibling-op"`, `runInTransactionOnConnection` with `"nested-transaction"`. Tests: new `ConnectionMutex.test.ts` covers F1 across the real-ALS and browser-shim paths (via `__resetAlsForTesting(true)`), asserting the throw path is synchronous on both and same-instance nested inlines under real ALS. Sqlite and Postgres integration tests replace the earlier "sibling blocks" contract with the new "sibling throws" contract and assert the F2 mode/message shape for both sibling-op and nested-transaction reach-throughs; the same-instance tx-proxy regression check is preserved (and added to the Postgres suite for parity). Co-Authored-By: Claude <noreply@anthropic.com>
…3e56is fix(storage): browser-safe cross-instance re-entry + actionable ConnectionReentryError
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
2 participants
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.
Summary
ConnectionMutex(packages/storage/src/tabular/ConnectionMutex.ts) keyed on the connection handle, backed by a lazy-loadednode:async_hooksAsyncLocalStorage(browser bundle degrades to a synchronous shim).BaseSqlTabularStoragegains aconnectionHandle()hook;SqliteTabularStorage,SqliteAiVectorStorage, andPostgresTabularStorage(PGlite / PGLitePool branch only) opt in so two storages that share aDatabase/ PGlite session queue on one chain, same-instance re-entry runs inline via ALS, and cross-instance re-entry throwsConnectionReentryErrorinstead of hanging. Also fixes an existing latent bug whereSqlite.Database'sinTransactioncheck missed the wrapper — the code now consults our ownthis.inTransactionflag as well, unblockingputBulkvia thetxproxy.pkFingerprint()(packages/storage/src/tabular/pkFingerprint.ts) tagsbigintandUint8Arraywith{ __t: "n" | "u8", v: … }while every other input shape stays byte-identical to the previousJSON.stringifyoutput.BaseSqlTabularStorage.bulkKeyOfandSupabaseTabularStorage.primaryKeyFingerprintroute through it so BigInt / byte-key primary keys no longer collide in therunBulkPutdedup map or Supabase's response alignment.providers/google-gemini/src/ai/common/Gemini_CachedContentFallback.tsdoes not yet live onmain— it is on theai-provider-cache-checkpointsfeature branch — so the matcher rewrite has no target file. Landed adescribe.skip("TODO — Plan C: …")module (packages/test/src/test/ai-provider/Gemini_CachedContentFallback.test.ts) enumerating the five assertions to enable once the fallback file merges tomain.readonly id: string = uuid4()toResourceScopewith tenant-scope tsdoc. Added a sessionId-keyed promise-chainwithSessionLockhelper onproviders/node-llama-cpp/src/ai/common/LlamaCpp_Runtime.ts(map entry cleaned up infinallyso a thrown body does not strand it). Deferred the rest of Plan D —CheckpointEntry.scopeToken,validateParentCheckpoint/resolveCheckpointSession/finalizeEmittedCheckpoint/ResolvedCheckpointsignature changes, theCacheCheckpointTask/TextGenerationTask/ToolCallingTask/AiChatTaskcaller threading, and the LlamaCpp keep-parent / supersede branches — becausepackages/ai/src/provider/CheckpointRegistry.ts,packages/ai/src/task/base/CheckpointPorts.ts,CacheCheckpointTask.ts, and the checkpoint-consumption blocks inLlamaCpp_TextGeneration.ts/LlamaCpp_ToolCalling.tsall live on theai-provider-cache-checkpointsfeature branch and are not present onmain. Re-open once that branch merges.Test plan
bunx vitest run packages/test/src/test/storage-tabular/— 1345 passed / 14 skipped (Plan A + Plan B)bunx vitest run packages/test/src/test/storage-tabular/SqliteTabularStorage.integration.test.ts— new: sibling single-op blocks across a tx on the shared handle, cross-instance nestedwithTransactionthrowsConnectionReentryErrorunder 500 ms, same-instance tx-proxy nested writes still work, BigInt PK alignment + last-wins dedupebunx vitest run packages/test/src/test/storage-tabular/PostgresTabularStorage.integration.test.ts— new: shared-connection safety on the PGlite path (sibling block + cross-instance ConnectionReentryError)bunx vitest run packages/test/src/test/storage-tabular/pkFingerprint.test.ts— unit-level BigInt / Uint8Array / plain-value fingerprint distinctness (5 tests)bunx vitest run packages/test/src/test/resource/ResourceScope.test.ts— new: id is minted, distinct per instance, stable across dispose (10 tests total)bunx vitest run packages/test/src/test/ai-provider-nodellama/LlamaCpp_Runtime.test.ts— new:withSessionLockserializes same-session callers, unrelated sessions run in parallel, lock releases even when the body throws (29 tests total)bunx vitest run packages/test/src/test/ai-provider/Gemini_CachedContentFallback.test.ts— skip block loads cleanlybun run build:types(40 packages, all cache-miss on this branch) — cleanDeferrals
ai-provider-cache-checkpointsfeature branch. See the summary bullets for the exact seam list. Re-open once that branch merges intomain; the skipped Gemini test module holds the assertion checklist.Generated by Claude Code