Add e2e tests for DO-to-DO sync#46
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
cloudstash | c5d1111 | Apr 17 2026, 07:43 PM |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 44 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new end-to-end test suite for the LinkProcessor Cloudflare Durable Object and small test setup/config tweaks: an E2E test file covering ingest/fetch/queue flows, a migration pre-check in test setup, and a Vitest pool worker option change. Changes
Sequence Diagram(s)(silently skipped) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cf-worker/__tests__/e2e/do-sync.test.ts (1)
29-61:createMockBatchomitsackAll/retryAllfrom the mockedMessageBatch.The cast
as unknown as MessageBatch<LinkQueueMessage>bypasses the type system. CurrenthandleQueueBatchonly uses per-messageack()/retry(), so these tests pass, but if the handler is ever extended to callbatch.ackAll()orbatch.retryAll()(a common pattern for terminal errors), the tests will throwTypeError: batch.ackAll is not a functionat runtime with no compile-time hint. Cheap to stub defensively.♻️ Add no-op `ackAll`/`retryAll` stubs
const batch = { messages: mockMessages, queue: "cloudstash-link-queue", + ackAll() {}, + retryAll() {}, } as unknown as MessageBatch<LinkQueueMessage>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cf-worker/__tests__/e2e/do-sync.test.ts` around lines 29 - 61, createMockBatch currently builds a MessageBatch stub but casts via "as unknown as MessageBatch<LinkQueueMessage>" without implementing ackAll/retryAll, so if handleQueueBatch later calls batch.ackAll() or batch.retryAll() tests will throw at runtime; update createMockBatch to add no-op ackAll() and retryAll() methods on the returned batch object (and ensure the mockMessages array remains accessible) so the stub fully satisfies MessageBatch<LinkQueueMessage> at runtime while keeping per-message ack()/retry() behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cf-worker/__tests__/e2e/do-sync.test.ts`:
- Around line 109-133: The concurrent-ingest test doesn't exercise the
cold-start race because the shared stub is already initialized; add a new test
variant that uses signupUser() to create a fresh orgId (ensuring a new DO
instance) and then immediately fires Promise.all(urls.map(u =>
stub.ingestAndProcess(makeQueueMessage(u, orgId)))) against a newly obtained
stub from getLinkProcessorStub(orgId) so all ingestAndProcess calls run before
any prior initialization completes; this will trigger the createStoreDoPromise /
storeCreationPromise path in getStore() and verify that concurrent store
creation is properly deduplicated and serialized.
---
Nitpick comments:
In `@src/cf-worker/__tests__/e2e/do-sync.test.ts`:
- Around line 29-61: createMockBatch currently builds a MessageBatch stub but
casts via "as unknown as MessageBatch<LinkQueueMessage>" without implementing
ackAll/retryAll, so if handleQueueBatch later calls batch.ackAll() or
batch.retryAll() tests will throw at runtime; update createMockBatch to add
no-op ackAll() and retryAll() methods on the returned batch object (and ensure
the mockMessages array remains accessible) so the stub fully satisfies
MessageBatch<LinkQueueMessage> at runtime while keeping per-message
ack()/retry() behavior intact.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfae712a-7f29-48c7-8824-10eade3ea7c5
📒 Files selected for processing (1)
src/cf-worker/__tests__/e2e/do-sync.test.ts
| describe("concurrent ingests", () => { | ||
| it("ingests multiple links simultaneously", async () => { | ||
| const stub = getLinkProcessorStub(user.orgId); | ||
| const urls = [ | ||
| "https://example.com/concurrent-1", | ||
| "https://example.com/concurrent-2", | ||
| "https://example.com/concurrent-3", | ||
| ]; | ||
|
|
||
| const results = await Promise.all( | ||
| urls.map((url) => | ||
| stub.ingestAndProcess(makeQueueMessage(url, user.orgId)) | ||
| ) | ||
| ); | ||
|
|
||
| for (const result of results) { | ||
| expect(result.status).toBe("ingested"); | ||
| expect(result.linkId).toBeDefined(); | ||
| } | ||
|
|
||
| const linkIds = results.map((r) => r.linkId); | ||
| const uniqueIds = new Set(linkIds); | ||
| expect(uniqueIds.size).toBe(urls.length); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate ingestAndProcess / createStoreDoPromise and inspect serialization/guarding logic
fd -e ts -e tsx | xargs rg -nP -C4 '\b(ingestAndProcess|createStoreDoPromise)\b'Repository: bohdanbirdie/cloudstash
Length of output: 19148
🏁 Script executed:
sed -n '500,545p' src/cf-worker/link-processor/durable-object.tsRepository: bohdanbirdie/cloudstash
Length of output: 1251
🏁 Script executed:
sed -n '130,160p' src/cf-worker/link-processor/durable-object.tsRepository: bohdanbirdie/cloudstash
Length of output: 911
🏁 Script executed:
rg -n 'async getStore|getStore\(' src/cf-worker/link-processor/durable-object.ts | head -20Repository: bohdanbirdie/cloudstash
Length of output: 219
🏁 Script executed:
sed -n '82,95p' src/cf-worker/link-processor/durable-object.tsRepository: bohdanbirdie/cloudstash
Length of output: 410
🏁 Script executed:
sed -n '82,100p' src/cf-worker/link-processor/durable-object.tsRepository: bohdanbirdie/cloudstash
Length of output: 516
🏁 Script executed:
cat -n src/cf-worker/__tests__/e2e/do-sync.test.ts | sed -n '60,135p'Repository: bohdanbirdie/cloudstash
Length of output: 2950
Add a test variant with fresh DO to exercise true cold-boot race condition.
The current concurrent-ingest test does not exercise the initialization race it appears to test. By the time this it runs, the shared stub has already been initialized by preceding tests ("cold boot", "warm DO", "duplicate detection"), so all three concurrent ingestAndProcess calls hit an already-cached store. The actual hazard from the learning—concurrent createStoreDoPromise calls corrupting the eventlog—only occurs during initialization.
Add a variant that:
- Calls
signupUser()to create a freshorgId(new DO instance) - Fires
Promise.all(urls.map(...ingestAndProcess...))before any prior call runs
This will verify that concurrent RPCs during cold-boot properly serialize via the deduplication in getStore() (which reuses this.storeCreationPromise).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cf-worker/__tests__/e2e/do-sync.test.ts` around lines 109 - 133, The
concurrent-ingest test doesn't exercise the cold-start race because the shared
stub is already initialized; add a new test variant that uses signupUser() to
create a fresh orgId (ensuring a new DO instance) and then immediately fires
Promise.all(urls.map(u => stub.ingestAndProcess(makeQueueMessage(u, orgId))))
against a newly obtained stub from getLinkProcessorStub(orgId) so all
ingestAndProcess calls run before any prior initialization completes; this will
trigger the createStoreDoPromise / storeCreationPromise path in getStore() and
verify that concurrent store creation is properly deduplicated and serialized.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cf-worker/__tests__/e2e/setup.ts (1)
9-16: Migration skip check may stale out when new migrations are added.The guard only checks for the
usertable (created by the first migration). WithisolatedStorage: false, a D1 database populated by an earlier test run will cause all subsequent migrations — including any newly added ones — to be silently skipped on later runs until the persistent storage is wiped. Consider keying off the full migration set (e.g., adrizzle_migrationstracking table, or checking the last migration's tag/table) so newly added migrations get applied against an already-initialized DB.As per coding guidelines: "Avoid code comments unless absolutely necessary for complex logic" — the comment on lines 7-8 is arguably necessary here, so no action needed on that front.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cf-worker/__tests__/e2e/setup.ts` around lines 9 - 16, The migration guard currently detects initialization by querying for the 'user' table via db.prepare(...).first(), which will incorrectly skip newly added migrations if the DB was created by an earlier run; change the guard to verify the actual migration tracking state instead (for example query for a drizzle_migrations or migration tracking table, or check for the last migration's tag/table used by your migration runner) so that presence of the initial 'user' table no longer short-circuits applying any newer migrations; update the check that uses db.prepare and the SQL string to look up the migrations table or last-applied migration marker (e.g., check for 'drizzle_migrations' or the latest migration identifier) and return only when that tracking entry indicates all migrations have been applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cf-worker/__tests__/e2e/setup.ts`:
- Around line 9-16: The migration guard currently detects initialization by
querying for the 'user' table via db.prepare(...).first(), which will
incorrectly skip newly added migrations if the DB was created by an earlier run;
change the guard to verify the actual migration tracking state instead (for
example query for a drizzle_migrations or migration tracking table, or check for
the last migration's tag/table used by your migration runner) so that presence
of the initial 'user' table no longer short-circuits applying any newer
migrations; update the check that uses db.prepare and the SQL string to look up
the migrations table or last-applied migration marker (e.g., check for
'drizzle_migrations' or the latest migration identifier) and return only when
that tracking entry indicates all migrations have been applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fcdbfb5f-c70c-42b1-838d-9025c68a96eb
📒 Files selected for processing (2)
src/cf-worker/__tests__/e2e/setup.tsvitest.e2e.config.ts
✅ Files skipped from review due to trivial changes (1)
- vitest.e2e.config.ts
Tests the LinkProcessorDO ↔ SyncBackendDO sync path end-to-end: store creation, link ingestion, duplicate detection, concurrent ingests, cross-org isolation, fetch trigger path, queue batch handler with real DOs, and mixed-org batch routing.
bohdanbirdie
force-pushed
the
test/e2e-do-sync
branch
from
4883d99 to
c5d1111
Compare
Blocker: CI hangs on DO storage cleanupTests pass locally on macOS but CI on Linux hangs indefinitely after throwing: Root cause
It passes locally because macOS SQLite checkpoints WAL more eagerly than Linux, so the sidecars often don't exist at pop time. Same code, different filesystem behavior → environmental flake. Upgrade path is blocked
Alternatives considered
PlanLeave PR open until Full diagnosis in issue #11031 and related. |
1 participant
Summary
Summary by CodeRabbit