test(web-e2e): align rotation-durability floor helpers with combined store#600
Conversation
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThe rotation durability test now uses the single ChangesRotation floor migration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Release PreviewNo version bumps detected. All changes are in unversioned paths or use exempt commit types. |
Greptile SummaryThis PR fixes a broken e2e test helper that was probing retired IndexedDB store names after Phase 70.1 collapsed two stores (
Confidence Score: 5/5Test-only changes that fix stale helper references; no product code is touched and all helper logic aligns with the canonical IDB store implementation. The IDB store name, key shape, and read-modify-write pattern in both helpers match rotation-idb-store.ts exactly. The concurrent-test assertion relaxation correctly preserves the hard invariants. Timeout bumps are conservative and well-motivated. The expect.poll block in full-workflow.spec.ts (test 6.6.2) has a mild cleanup gap if waitForContentLoaded times out before clickCancel runs. Important Files Changed
Reviews (4): Last reviewed commit: "test: stabilize web-e2e batch-op and sha..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/web-e2e/tests/rotation-durability.spec.ts (1)
66-78: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider aligning
readDurableFloorswith production's fail-closed sanitization.Production
idbGetCombined(Context snippet 3,rotation-idb-store.ts:190-203) sanitizes the raw record viasanitizeRecord/isValidFloorValue, treating non-integer or negative values asundefined. This test helper readsrec?.generationandrec?.seqdirectly without validation. In a controlled E2E environment this is unlikely to matter, but if the store ever contains stale or corrupt data from a prior test run, the test could resolve a bogus floor and produce a false-positive pass.♻️ Optional: add inline validation to mirror production
tx.oncomplete = () => { const rec = getReq.result as { generation?: number; seq?: number } | undefined; - resolve({ generation: rec?.generation, seq: rec?.seq }); + const valid = (v: unknown): v is number => + typeof v === 'number' && Number.isSafeInteger(v) && v >= 0; + resolve({ + generation: rec && valid(rec.generation) ? rec.generation : undefined, + seq: rec && valid(rec.seq) ? rec.seq : undefined, + }); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/web-e2e/tests/rotation-durability.spec.ts` around lines 66 - 78, Update the readDurableFloors helper to validate rotation-floor values before resolving them, matching production idbGetCombined sanitization: only retain integer, non-negative generation and seq values, and convert invalid, negative, or non-integer values to undefined. Apply this validation to the rec fields read in the rotation-floor transaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/web-e2e/tests/rotation-durability.spec.ts`:
- Around line 99-103: Clarify the comment above the rotation-floor helper to
state that the read-modify-write preserves existing generation and
wrappedKeyCheckpoint fields, but directly overwrites seq and does not apply
production’s max-preserving behavior. Update the parenthetical reference
accordingly.
---
Nitpick comments:
In `@tests/web-e2e/tests/rotation-durability.spec.ts`:
- Around line 66-78: Update the readDurableFloors helper to validate
rotation-floor values before resolving them, matching production idbGetCombined
sanitization: only retain integer, non-negative generation and seq values, and
convert invalid, negative, or non-integer values to undefined. Apply this
validation to the rec fields read in the rotation-floor transaction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d5db5b8-bfdb-4620-a8c6-fc6ccfe9cfea
📒 Files selected for processing (1)
tests/web-e2e/tests/rotation-durability.spec.ts
The doc comment claimed the read-modify-write "matches production's max-preserving write", which is misleading: production idbPutFloors max-preserves seq, whereas this staging helper intentionally overwrites seq so a test can pin an exact sequence floor. Only the preservation of the other fields (generation / wrappedKeyCheckpoint) matches production. Comment-only; no behavior change. Addresses CodeRabbit review on #600. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: f73432632360
…ublish Test 21 (D-06) fires two concurrent first-publishes of the same brand-new ipnsName with the SAME CID at sequence=1, and asserted exactly one 200 plus one 409. But same-CID / same-sequence is idempotent by design (#599): when the loser serializes after the winner commits, it re-reads the identical CID and returns an idempotent 200 -- so both calls can legitimately succeed, flaking the strict one-409 assertion (observed red on #600's SDK E2E leg). Accept both correct shapes: at least one winner (never zero) and any loser is a clean 409 (never a 5xx). The follow-up resolve still asserts exactly one row at sequence=1. Test-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: d752b84767ae
…store Phase 70.1 (D-06, #598) collapsed the generation-high-water and seq-high-water IndexedDB stores into a single rotation-floor store keyed by nodeId holding a { generation?, seq?, wrappedKeyCheckpoint? } record. The rotation-durability e2e helpers still probed the retired store names, so readDurableFloors short-circuited and returned generation: undefined, failing the SC#4 setup assertion at line 216 on every main run since #598. Point readDurableFloors and writeDurableSeqFloor at the rotation-floor store; the write does a read-modify-write so it preserves the node's existing generation/wrappedKeyCheckpoint floors. Test-only -- the product write path (rotation-idb-store.ts) is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 545aa91edc15
The doc comment claimed the read-modify-write "matches production's max-preserving write", which is misleading: production idbPutFloors max-preserves seq, whereas this staging helper intentionally overwrites seq so a test can pin an exact sequence floor. Only the preservation of the other fields (generation / wrappedKeyCheckpoint) matches production. Comment-only; no behavior change. Addresses CodeRabbit review on #600. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: f73432632360
…ublish Test 21 (D-06) fires two concurrent first-publishes of the same brand-new ipnsName with the SAME CID at sequence=1, and asserted exactly one 200 plus one 409. But same-CID / same-sequence is idempotent by design (#599): when the loser serializes after the winner commits, it re-reads the identical CID and returns an idempotent 200 -- so both calls can legitimately succeed, flaking the strict one-409 assertion (observed red on #600's SDK E2E leg). Accept both correct shapes: at least one winner (never zero) and any loser is a clean 409 (never a 5xx). The follow-up resolve still asserts exactly one row at sequence=1. Test-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: d752b84767ae
Batch delete/move (4.10/4.11), version restore poll (6.6.2), the serial root-file delete chain (8.2), and recipient-side shared mkdirs each drive IPNS publish round-trips whose serial-calibrated waits were too tight, flaking locally and under parallel-worker CI load. Raise the explicit per-call timeouts (CI already floors waiters to 60s via ciFloor) and make 6.6.2 poll the editor for the restored content instead of reading once. Verified: full-workflow.spec.ts 53/53 green locally under the CI 60s-floor timeout regime. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 1fe70eb98011
84b0c9f to
e375b2a
Compare
Problem
tests/web-e2e/tests/rotation-durability.spec.tshas been red on everymainrun since #598 (Phase 70.1). The SC#4-setup assertion at line 216 fails:Root cause
Phase 70.1 (D-06, #598) collapsed the two IndexedDB stores
generation-high-waterandseq-high-waterinto a singlerotation-floorstore keyed by nodeId, holding a{ generation?, seq?, wrappedKeyCheckpoint? }record (seepackages/sdk/src/state/rotation-idb-store.ts). The v2onupgradeneededmigration deletes the old stores.The e2e helpers
readDurableFloors/writeDurableSeqFloorstill probed the retired store names, soreadDurableFloorsshort-circuited at itscontains(...)guard and returned{ generation: undefined }unconditionally — decoupled from whether the floor was actually persisted.This is a stale test helper, not a durability regression: the product write path (
enforceResolved→idbPutFloors→rotation-floor) is intact, and the v2 migration folds legacy data forward. Fixing the helper does not mask a bug — the helper never read real data.Fix (test-only)
Point both helpers at the
rotation-floorcombined store.writeDurableSeqFloornow does a read-modify-write so it preserves the node's existinggeneration/wrappedKeyCheckpointfloors (matching production's max-preserving write). No product code changed.Verification
npx tsc --noEmitintests/web-e2e— clean.rotation-idb-store.ts(store namerotation-floor, out-of-line key = nodeId,put(value, key)).main-push web-e2e run after merge (web-e2e only runs on main push).Summary by CodeRabbit
Added: sdk-e2e Test 21 (D-06) flake fix
The
SDK E2E Testsleg flaked red on this PR (orthogonal to the diff). Root cause: Test 21 inipns-publish-gate.test.tsfires two concurrent first-publishes of the same new ipnsName with the same CID at sequence=1 and asserted exactly one 200 + one 409. But same-CID / same-sequence is idempotent by design (#599) — a loser that serializes after the winner commits re-reads the identical CID and returns an idempotent 200, so two 200s is also correct. The assertion now accepts both shapes (at least one winner; any loser is a clean 409, never a 5xx), keeping the exactly-one-row-at-seq-1 resolve check. Test-only.