Skip to content

Add reproduction for platform-browser/BrowserKeyValueStore issue - #6857

Merged
tim-smart merged 3 commits into
mainfrom
audit/repro-platform-browser-key-value-store-transaction
Aug 2, 2026
Merged

Add reproduction for platform-browser/BrowserKeyValueStore issue#6857
tim-smart merged 3 commits into
mainfrom
audit/repro-platform-browser-key-value-store-transaction

Conversation

@fubhy

@fubhy fubhy commented Aug 1, 2026

Copy link
Copy Markdown
Member

Summary

  • wait for IndexedDB write transactions to commit before reporting KeyValueStore mutations as successful
  • surface transaction errors and aborts as KeyValueStoreError
  • add a regression test for an abort after the individual request succeeds

Testing

pnpm test --run packages/platform-browser/test/BrowserKeyValueStore.test.ts
pnpm check

Closes EFF-299

@github-project-automation github-project-automation Bot moved this to Discussion Ongoing in PR Backlog Aug 1, 2026
@changeset-bot

changeset-bot Bot commented Aug 1, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 5b02317

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages
Name Type
@effect/platform-browser Patch
effect Patch
@effect/opentelemetry Patch
@effect/platform-bun Patch
@effect/platform-deno Patch
@effect/platform-node-shared Patch
@effect/platform-node Patch
@effect/vitest Patch
@effect/ai-anthropic Patch
@effect/ai-openai-compat Patch
@effect/ai-openai Patch
@effect/ai-openrouter Patch
@effect/atom-react Patch
@effect/atom-solid Patch
@effect/atom-vue Patch
@effect/sql-clickhouse Patch
@effect/sql-d1 Patch
@effect/sql-libsql Patch
@effect/sql-mssql Patch
@effect/sql-mysql2 Patch
@effect/sql-pg Patch
@effect/sql-pglite Patch
@effect/sql-sqlite-bun Patch
@effect/sql-sqlite-do Patch
@effect/sql-sqlite-node Patch
@effect/sql-sqlite-react-native Patch
@effect/sql-sqlite-wasm Patch
@effect/docgen Patch
@effect/doctest Patch
@effect/openapi-generator Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@effect-slopcop effect-slopcop Bot added the 4.0 label Aug 1, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — a single-file reproduction test that mocks IndexedDB to surface the transaction-commit bug.

  • BrowserKeyValueStoreTransaction.test.ts — mocks indexedDB, IDBTransaction, and IDBObjectStore to fire request.onsuccess before transaction.onabort in the same microtask, then asserts Effect.result(store.set(...)) is a failure.

The test correctly reproduces the root cause: idbRequest in BrowserKeyValueStore.ts:153 resolves the effect on request.onsuccess without tracking the transaction lifecycle. Until that is fixed, a readwrite transaction that aborts after individual request completion will report a false success.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) | 𝕏

@effect-slopcop effect-slopcop Bot added the bug Something isn't working label Aug 2, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — a fix that adds idbWriteRequest to properly track IndexedDB transaction lifecycle for write operations.

  • idbWriteRequest in BrowserKeyValueStore.ts — new callback-based effect that creates its own readwrite transaction and resolves only on transaction.oncomplete, mapping transaction.onabort and transaction.onerror to KeyValueStoreError failures.
  • clear, set, remove now use idbWriteRequest — replaced direct idbRequest + getKvsEntriesStore calls so that write operations no longer report success before the transaction commits.

The original idbRequest remains in place for read-only operations (get, getUint8Array, size), which is appropriate since readonly transactions have no silent-abort problem.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) | 𝕏

@tim-smart
tim-smart enabled auto-merge (squash) August 2, 2026 22:46
@@ -0,0 +1,73 @@
import * as BrowserKeyValueStore from "@effect/platform-browser/BrowserKeyValueStore"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please remove this test file and merge it into the main one?

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — merged the standalone transaction test into the main test file and added a changeset.

  • Moved regression test into BrowserKeyValueStore.test.ts — the BrowserKeyValueStoreTransaction.test.ts file was deleted and its test was folded into the existing describe("KeyValueStore / layerIndexedDb", …) block, using a separate database name for isolation from the shared testLayer.
  • Renamed indexedDB to failingIndexedDb — avoids shadowing the top-level import from fake-indexeddb.
  • Added changeset@effect/platform-browser patch for the transaction-commit fix.

All prior review feedback is addressed. No source implementation changes since the last review.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) | 𝕏

@tim-smart
tim-smart merged commit 79db8e5 into main Aug 2, 2026
17 checks passed
@tim-smart
tim-smart deleted the audit/repro-platform-browser-key-value-store-transaction branch August 2, 2026 23:58
@github-project-automation github-project-automation Bot moved this from Discussion Ongoing to Done in PR Backlog Aug 2, 2026
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Bundle Size Analysis

Generated from PR build output; treat the content below as untrusted.

File Name Current Size Previous Size Difference
basic.ts 6.65 KB 6.65 KB 0.00 KB (0.00%)
batching.ts 9.44 KB 9.44 KB 0.00 KB (0.00%)
brand.ts 6.33 KB 6.33 KB 0.00 KB (0.00%)
cache.ts 10.20 KB 10.20 KB 0.00 KB (0.00%)
config.ts 20.34 KB 20.34 KB 0.00 KB (0.00%)
differ.ts 19.95 KB 19.95 KB 0.00 KB (0.00%)
http-client.ts 21.04 KB 21.04 KB 0.00 KB (0.00%)
logger.ts 10.35 KB 10.35 KB 0.00 KB (0.00%)
metric.ts 8.58 KB 8.58 KB 0.00 KB (0.00%)
optic.ts 7.34 KB 7.34 KB 0.00 KB (0.00%)
pubsub.ts 14.49 KB 14.49 KB 0.00 KB (0.00%)
queue.ts 11.15 KB 11.15 KB 0.00 KB (0.00%)
schedule.ts 10.33 KB 10.33 KB 0.00 KB (0.00%)
schema-class.ts 18.88 KB 18.88 KB 0.00 KB (0.00%)
schema-fromJsonSchemaDocument.ts 28.69 KB 28.69 KB 0.00 KB (0.00%)
schema-representation-roundtrip.ts 25.01 KB 25.01 KB 0.00 KB (0.00%)
schema-string-transformation.ts 13.01 KB 13.01 KB 0.00 KB (0.00%)
schema-string.ts 10.66 KB 10.66 KB 0.00 KB (0.00%)
schema-template-literal.ts 14.87 KB 14.87 KB 0.00 KB (0.00%)
schema-toArbitraryLazy.ts 21.67 KB 21.67 KB 0.00 KB (0.00%)
schema-toCodeDocument.ts 24.10 KB 24.10 KB 0.00 KB (0.00%)
schema-toCodecJson.ts 18.93 KB 18.93 KB 0.00 KB (0.00%)
schema-toEquivalence.ts 18.74 KB 18.74 KB 0.00 KB (0.00%)
schema-toFormatter.ts 18.61 KB 18.61 KB 0.00 KB (0.00%)
schema-toJsonSchemaDocument.ts 22.36 KB 22.36 KB 0.00 KB (0.00%)
schema-toRepresentation.ts 19.28 KB 19.28 KB 0.00 KB (0.00%)
schema.ts 18.14 KB 18.14 KB 0.00 KB (0.00%)
stm.ts 12.13 KB 12.13 KB 0.00 KB (0.00%)
stream.ts 9.38 KB 9.38 KB 0.00 KB (0.00%)

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

Labels

4.0 bug Something isn't working

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants