tiennm99/rplace
1
0
Fork 0
mirror of https://github.com/tiennm99/rplace.git synced 2026-09-09 02:17:35 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(canvas): make multi-chunk pixel writes atomic and refund cooldown on failure

Browse Source 
- wrap writePixels in state.storage.transactionSync so a partial multi-chunk
  failure doesn't leave the canvas half-written or out of sync with the WS
  broadcast
- size new chunk buffer against chunkSize(chunkId) instead of the persisted
  blob's length so writes after a canvas-grow no longer silently drop OOB
  bytes in the formerly-last short chunk
- refund the cooldown row when writePixels throws so transient storage
  errors stop soft-DOSing the user (and halving image-uploader throughput)
- bound readAllChunks by chunk_id < CHUNK_COUNT and trim oversized blobs so
  orphan rows from a future shrink no longer crash GET /api/canvas
- require a positive Content-Length on /api/place (411) and reject above the
  pre-parse cap (413); previously a missing or zero header bypassed the cap
- drop String(err) from the 500 response body
- drain the INSERT cursor symmetrically with the UPDATE branch in tryAcquire
- assert CHUNK_BYTES <= 2 MB at module load (DO SQLite per-cell BLOB cap)
- correct the inverted webSocketClose comment and guard the re-close call
- add tests for missing / zero / oversized Content-Length

Plan: plans/260510-0232-fix-do-migration-followups/phase-01-do-storage-atomicity.md
This commit is contained in:
tiennm99
2026-05-10 02:57:30 +07:00
parent a977adc62d
commit 3c1263add6
15 changed files with 1686 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files
plans/260510-0232-fix-do-migration-followups/phase-01-do-storage-atomicity.md
+151
View File
@@ -0,0 +1,151 @@
---
phase: 1
title: "DO storage atomicity & correctness"
status: pending
priority: P1
effort: "4h"
dependencies: []
---
# Phase 1: DO Storage Atomicity & Correctness
## Overview
Fix the four correctness bugs in the Durable Object storage path: non-atomic multi-chunk writes, BLOB-grow data loss after canvas resize, cooldown burned on storage failure, and orphan-row crash on `/api/canvas`. Sanitize the 500 response body. Replace the bypassable `content-length` cap.
## Context Links
- Reports: `plans/reports/code-reviewer-260510-0211-rplace-do-migration.md` (C3, H1, H2, H3, H5)
- Reports: `plans/reports/debugger-260510-0211-rplace-edge-cases.md` (C1, C2, M5)
## Key Insights
- "No `await` between statements" ≠ atomic. Each `sql.exec` auto-commits unless wrapped in `state.storage.transactionSync(...)`.
- `new Uint8Array(typedArray)` copies but preserves the source's length, so growing a short BLOB requires explicit re-allocation against `chunkSize(chunkId)`.
- Cooldown UPDATE must roll back if the subsequent write throws — otherwise transient storage flakes silently halve image-uploader throughput.
## Requirements
**Functional**
- Multi-chunk batch writes atomic: all chunks commit, or none do.
- After `CANVAS_WIDTH`/`CANVAS_HEIGHT` grow + redeploy, new pixels in the formerly-last chunk persist correctly.
- Cooldown row reverted when `writePixels` throws.
- `GET /api/canvas` returns 200 even with orphan `chunk_id ≥ CHUNK_COUNT` rows present (post-shrink).
- `POST /api/place` rejects requests with missing or zero `content-length` (or reads body with hard byte cap).
- 500 responses do not echo raw error message.
**Non-functional**
- No regression in happy-path latency (single-chunk batch).
- Static assertion `CHUNK_BYTES <= 2_000_000` near constants to prevent future BLOB-cell overflow.
## Architecture
```
worker.js (edge)
├─ guard: content-length present AND > 0 AND ≤ MAX_BODY_BYTES (else 413)
└─ forwards to DO
CanvasRoom.placePixels (canvas-room.js)
├─ tryAcquire(userId) → reserves cooldown
├─ TRY:
│ state.storage.transactionSync(() => writePixels(sql, edits))
│ broadcast(edits)
└─ CATCH:
sql.exec('DELETE FROM cooldowns WHERE user_id = ?', userId) // rollback
return Response.json({ error: 'storage_failed' }, { status: 500 })
// no message field; full err logged separately
```
`chunk-storage.writePixels` rewritten so `next` is sized to `chunkSize(chunkId)`, with `buf.subarray(0, min(buf.length, expected))` copied into it.
`chunk-storage.readAllChunks` filters `WHERE chunk_id < ?` bound to `CHUNK_COUNT`.
## Related Code Files
**Modify**
- `src/worker.js` — content-length validator (require > 0 OR switch to bounded `arrayBuffer`)
- `src/durable-objects/canvas-room.js` — wrap write in transactionSync, rollback cooldown on error, drop `message` from 500
- `src/durable-objects/lib/chunk-storage.js` — fix `writePixels` BLOB-grow, bound `readAllChunks`, replace `new Uint8Array(buf)` aliasing comment with `.slice()` or explicit allocation
- `src/lib/constants.js` — add static assertion for `CHUNK_BYTES`
- `src/durable-objects/canvas-room.js:119` — fix the inverted "pre-2026-04-07" comment (M2 in code-review)
**Create** — none
**Delete** — none
## Implementation Steps
1. **`chunk-storage.writePixels` BLOB-grow fix** (review C3)
- In the per-chunk loop, compute `expected = chunkSize(chunkId)`.
- `const next = new Uint8Array(expected); next.set(buf.subarray(0, Math.min(buf.length, expected)));`
- Update inline comment: "size against chunkSize, never trust persisted blob length".
2. **`chunk-storage.writePixels` atomicity** (debugger C2)
- Wrap the entire `for (const [chunkId, edits] of grouped)` block in `state.storage.transactionSync(() => { ... })`.
- Verify `transactionSync` exists at compat date `2025-04-01`; if not, fall back to async `state.storage.transaction(async () => {...})` and propagate rejection.
- Update the comment at L86-87 to describe the transactional invariant, not "no await".
3. **`chunk-storage.readAllChunks` orphan guard** (review H3)
- Change SQL to `SELECT chunk_id, bytes FROM canvas_chunks WHERE chunk_id < ?` bound to `CHUNK_COUNT`.
- Optional secondary defense: skip rows where `chunkId * CHUNK_BYTES + view.length > out.length`.
4. **`canvas-room.placePixels` cooldown rollback** (review H1, debugger C1)
- Add try/catch around the `writePixels` call.
- In catch: `sql.exec('DELETE FROM cooldowns WHERE user_id = ?', userId)`. Rethrow or return 500.
- Add a one-line comment: `// refund cooldown so transient storage errors don't soft-DOS user`.
5. **500 response sanitization** (review H2)
- Drop `message: String(err)` from the response JSON.
- Keep the `console.error` call so debugging info stays in logs.
6. **`worker.js` content-length guard** (review H5)
- Require `content-length` header present AND > 0 AND ≤ `MAX_BODY_BYTES`. Reject with 411 (length required) or 413 (too large).
- Alternative: read raw body via `c.req.arrayBuffer()`, check `byteLength`, then `JSON.parse(decoder.decode(buf))`.
7. **Static `CHUNK_BYTES` cap assertion** (debugger M5)
- At top of `constants.js` (or in a small init): `if (CHUNK_BYTES > 2_000_000) throw new Error('CHUNK_BYTES exceeds DO SQLite cell limit');`
- Document compat-date-versioned cell limit in the comment.
8. **WS-close comment correction** (review M2)
- `canvas-room.js:119` — replace misleading comment with: `// Required because compatibility_date 2025-04-01 predates the 2026-04-07 default-close cutoff. Remove if/when wrangler.json bumps past that date.`
- Add try/catch around the `ws.close(...)` call to handle already-closed sockets (debugger M7).
9. **Compile + smoke**
- `npm run build` — must pass.
- Local `wrangler dev`, hit `POST /api/place` with a 2-pixel batch spanning 2 chunks; verify success.
- Force a write error (e.g. temporarily throw inside `writePixels`); verify cooldown row deleted, 500 returned without raw message.
- Hit `GET /api/canvas` after manually inserting an orphan row; verify 200.
## Todo List
- [ ] Implement BLOB-grow fix in `chunk-storage.writePixels`
- [ ] Wrap multi-chunk write in `transactionSync` (or async fallback)
- [ ] Bound `readAllChunks` query by `chunk_id < CHUNK_COUNT`
- [ ] Add cooldown rollback in `canvas-room.placePixels` catch path
- [ ] Drop `message` from 500 response
- [ ] Replace `content-length` guard with present+nonzero check or bounded body read
- [ ] Add `CHUNK_BYTES <= 2_000_000` static assertion
- [ ] Fix inverted WS-close comment + add try/catch
- [ ] `npm run build` passes
- [ ] Manual smoke: 2-chunk batch success
- [ ] Manual smoke: forced write error → cooldown refunded, no message leak
- [ ] Manual smoke: orphan row → `/api/canvas` 200
## Success Criteria
- [ ] No regressions in existing 94-test suite
- [ ] `transactionSync` (or async transaction) confirmed available at compat date 2025-04-01
- [ ] All bullets in Todo List checked
- [ ] PR includes file:line evidence linking each change to the originating finding
## Risk Assessment
- **Risk:** `transactionSync` not available at our compat date → fix back-compat.
**Mitigation:** Verify via `wrangler` runtime docs first; fall back to `state.storage.transaction(async () => {...})` and ensure `placePixels` is awaited end-to-end.
- **Risk:** Cooldown rollback DELETE race against concurrent insert from same user.
**Mitigation:** Single-DO is single-threaded per request; rollback runs synchronously before response. No lock needed.
- **Risk:** Bounded body read changes Hono request-handling shape; could break the existing tests.
**Mitigation:** Keep `c.req.json()` if guard is sufficient; only refactor to `arrayBuffer()` if guard alone leaves a gap.
## Security Considerations
- 500 sanitization closes a low-severity info-leak.
- content-length guard removes a DOS-amplification vector against the JSON parser.
- Atomicity fix prevents broadcast/persistence divergence that could be probed for state inference.
plans/260510-0232-fix-do-migration-followups/phase-02-cookie-ip-identity.md
+153
View File
@@ -0,0 +1,153 @@
---
phase: 2
title: "Cookie+IP identity & broadcast sequence numbers"
status: pending
priority: P1
effort: "3h"
dependencies: []
---
# Phase 2: Cookie+IP Identity & Broadcast Sequence Numbers
## Overview
Replace IP-only rate-limit identity with cookie-preferred / IP-fallback to unblock NAT/CGNAT/mobile users. Add monotonically-increasing sequence numbers to WebSocket broadcast frames so clients can detect missed pixels and refetch.
## Context Links
- Reports: `plans/reports/debugger-260510-0211-rplace-edge-cases.md` (C3 NAT, H2 dev bucket, H3 hibernation gap)
- Reports: `plans/reports/code-reviewer-260510-0211-rplace-do-migration.md` (M4 dev bucket)
## Key Insights
- Cookie identity isn't a security boundary — it's a usability fix to break NAT collisions. Trivially defeated by clearing cookies; that's acceptable scope.
- Broadcast sequence numbers don't require persistence: a per-DO instance counter works because reconnects after hibernation refetch the canvas anyway.
- `cf-connecting-ip` missing in production is an alarm condition, not a fallback path. Fail-closed in prod, soft-fall in dev.
## Requirements
**Functional**
- First request without `rplace_id` cookie: server issues `Set-Cookie: rplace_id=<uuid>; HttpOnly; Secure; SameSite=Lax; Path=/; Max-Age=31536000`.
- Subsequent requests with cookie: identity = `cookie:<uuid>`. Without cookie but with valid `cf-connecting-ip`: identity = `ip:<ip>`. Neither in prod: 500 with `{ error: "no_identity" }`. Neither in dev: identity = `dev:<random-per-process>`.
- WS broadcast frames carry monotonic `seq` field (uint32, wraps at 2^32).
- Client tracks last-seen `seq`; on gap (`seq != lastSeq + 1`) triggers `refetchCanvas()`.
**Non-functional**
- Cookie issuance adds zero latency (Set-Cookie on existing 200 response).
- No new dependencies.
## Architecture
```
src/lib/get-user-id.js
resolveIdentity(request, env):
if (cookie 'rplace_id' present) → "cookie:" + uuid
else if (cf-connecting-ip present) → "ip:" + sha256(ip).slice(0,16)
else if (env is production) → throw NoIdentityError → caller maps to 500
else → "dev:<isolate-stable-uuid>"
needsCookieIssue(request) → true if no rplace_id cookie
src/worker.js
/api/canvas handler:
if needsCookieIssue → response.headers.append('Set-Cookie', issueCookie(uuid))
src/durable-objects/canvas-room.js
state #seq = 0 (in-memory, resets on hibernation rehydrate)
broadcast(edits):
#seq = (#seq + 1) >>> 0
payload = { type: 'pixels', seq: #seq, edits }
src/client/App.svelte
ws.onmessage:
if (msg.seq != null) {
if (lastSeq != null && msg.seq !== ((lastSeq + 1) >>> 0)) {
refetchCanvas() // gap detected
}
lastSeq = msg.seq
}
```
## Related Code Files
**Modify**
- `src/lib/get-user-id.js` — cookie-first resolution + production fail-closed
- `src/worker.js` — issue cookie on `/api/canvas` when missing; pass identity into DO
- `src/durable-objects/canvas-room.js` — instance `#seq` counter; include in broadcast frames
- `src/client/App.svelte` — track `lastSeq`, refetch on gap
- `src/client/components/CanvasRenderer.svelte` — accept `seq`-aware events if needed (likely not)
**Create**
- `src/lib/cookie.js` — minimal helpers: `parseCookie(header)`, `formatSetCookie(name, value, opts)`. ~30 lines, kebab-case per code standards.
## Implementation Steps
1. **Cookie helpers** (`src/lib/cookie.js`)
- `parseCookie(header) → Map<name, value>` — handle missing header, malformed pairs.
- `formatSetCookie(name, value, { httpOnly, secure, sameSite, path, maxAge })` → string.
- Export both as named exports.
- Add tests in `test/lib/cookie.test.js` (Phase 4).
2. **Identity resolution rewrite** (`get-user-id.js`)
- Import `parseCookie`.
- `resolveIdentity(request, env)` returns `{ id, issueCookie?: { name, value } }`.
- Production path: if no cookie AND no `cf-connecting-ip` → throw `NoIdentityError`.
- Dev path: stable `dev:<uuid>` per-isolate (cache module-level).
- When identity comes from `ip:`, mark `issueCookie = { name: 'rplace_id', value: crypto.randomUUID() }` so the worker can attach Set-Cookie even before the user has one.
3. **Worker integration** (`worker.js`)
- On `/api/canvas` and `/api/place`: call `resolveIdentity(c.req.raw, c.env)`. If it throws `NoIdentityError`, return 500 `{ error: "no_identity" }`.
- Pass `id` into the DO body / header.
- On `/api/canvas` only: if `issueCookie` is set, append `Set-Cookie` header to the response.
- On `/api/place`: don't issue cookies (POST shouldn't mutate cookies in this design).
4. **DO sequence counter** (`canvas-room.js`)
- Add `#seq = 0` private field.
- In `#broadcastPixels(edits)`: `this.#seq = (this.#seq + 1) >>> 0; const message = JSON.stringify({ type: 'pixels', seq: this.#seq, edits });`
- Note: counter resets on DO hibernation rehydrate; that's fine — clients refetch on reconnect.
5. **Client gap detection** (`App.svelte`)
- Track `let lastSeq = null;`.
- In WS message handler: if `msg.seq != null && lastSeq != null && msg.seq !== ((lastSeq + 1) >>> 0)` → call `canvasRenderer.refetchCanvas()`. Always update `lastSeq = msg.seq`.
- On reconnect: reset `lastSeq = null` (it's already a fresh canvas fetch).
6. **Compile + smoke**
- `npm run build` passes.
- Cookie issuance: open `wrangler dev`, GET `/api/canvas`, confirm `Set-Cookie` present. Subsequent request shows cookie.
- Identity: 2 tabs, 1 cookie each → independent rate limits even on same IP.
- Sequence gap: temporarily log `seq` on client; place pixel, confirm seq increments.
## Todo List
- [ ] Create `src/lib/cookie.js` (parseCookie + formatSetCookie)
- [ ] Refactor `src/lib/get-user-id.js` to cookie-first / IP-fallback / fail-closed
- [ ] Worker issues `Set-Cookie` on `/api/canvas` when missing
- [ ] Worker forwards resolved identity to DO
- [ ] DO maintains `#seq` counter; broadcast includes `seq`
- [ ] Client tracks `lastSeq`, refetches on gap
- [ ] Client resets `lastSeq` on reconnect
- [ ] `npm run build` passes
- [ ] Manual smoke: cookie issued, identity changes per cookie
- [ ] Manual smoke: seq gap → client refetches
## Success Criteria
- [ ] Two browsers on the same IP can place pixels independently within their own 1Hz cooldown.
- [ ] In production env (`env.ENVIRONMENT === 'production'`), missing both cookie and `cf-connecting-ip` returns 500 with `no_identity`.
- [ ] Forced WS gap (drop a frame in dev console) triggers `refetchCanvas`.
- [ ] No regression in existing tests.
## Risk Assessment
- **Risk:** Browser cookie blocking (private mode, strict tracker prevention) → falls back to IP, NAT users still collide.
**Mitigation:** Acceptable per Q&A scope; document. Future plan can add HMAC-signed token for stricter envs.
- **Risk:** Cookie collision if two users share a stolen cookie value (e.g., copy-paste curl).
**Mitigation:** Cookies are HttpOnly — not exfiltrable from JS. The opaque UUID is high-entropy.
- **Risk:** Seq counter wraparound after 2^32 broadcasts.
**Mitigation:** ~136 years at 1 broadcast/sec. Wrap is deliberately handled via `>>> 0` so wrap doesn't trigger false gap.
- **Risk:** Production fail-closed behind a misconfigured proxy denies all traffic.
**Mitigation:** Add a `wrangler tail` check before deploy; if 500s spike, hotfix to fall back to a synthetic identity.
## Security Considerations
- Cookie is `HttpOnly`, `Secure`, `SameSite=Lax` — standard hygiene.
- Cookie is opaque (UUID v4) — no PII, no prediction.
- Identity is used only for rate-limiting; no privilege gating, so cookie theft doesn't escalate.
- No CSRF concern — POST `/api/place` doesn't depend on cookie identity for authorization, only for rate-limit bucketing. (Worth confirming in PR review.)
plans/260510-0232-fix-do-migration-followups/phase-03-ws-hardening-client-race.md
+153
View File
@@ -0,0 +1,153 @@
---
phase: 3
title: "WebSocket hardening & client race fix"
status: pending
priority: P1
effort: "3h"
dependencies: []
---
# Phase 3: WebSocket Hardening & Client Race Fix
## Overview
Fix the WebSocket-during-initial-fetch race that silently drops pixels. Add Origin allowlist + per-identity connection cap on WS upgrade. Add a minimal heartbeat so dead connections fire `onclose` promptly.
## Context Links
- Reports: `plans/reports/code-reviewer-260510-0211-rplace-do-migration.md` (C2 race, H4 conn cap, M3 heartbeat)
- Reports: `plans/reports/debugger-260510-0211-rplace-edge-cases.md` (H5 origin/amplification, L6 conn cap, M3 heartbeat — duplicate)
## Key Insights
- The pre-allocated `committedColors` zero array gets overwritten by the post-fetch `new Uint8Array(indices)` — any WS edits between WS-open and fetch-resolve are lost. Fix: buffer WS edits, replay after replacement.
- CF DO `state.acceptWebSocket(ws, [tag1, tag2])` lets `getWebSockets(tag)` filter — perfect for per-identity caps.
- Hibernation API may auto-ping at TCP level, but app-level heartbeat is cheaper insurance and gives clients a way to detect zombies.
## Requirements
**Functional**
- WS messages received during the initial canvas fetch are applied (not dropped) once the fetch resolves.
- WS upgrade rejected (403) if `Origin` header is present and not in allowlist.
- WS upgrade rejected (429) if the requesting identity already has ≥ N (default 5) live sockets.
- Server accepts `ping` text message and responds `pong`. Client sends ping every 30s; if no pong in 60s, closes WS to trigger reconnect.
**Non-functional**
- Allowlist configurable via `wrangler.json` env vars (`ALLOWED_ORIGINS`, comma-separated).
- Conn cap configurable (`MAX_WS_PER_IDENTITY`, default 5).
## Architecture
```
Worker /api/ws
├─ resolve identity (Phase 2 helper)
├─ origin check: if Origin present AND not in env.ALLOWED_ORIGINS → 403
└─ forward to DO with identity in header
CanvasRoom #handleWsUpgrade
├─ existing = state.getWebSockets(identity)
├─ if existing.length >= MAX_WS_PER_IDENTITY → 429
├─ state.acceptWebSocket(server, [identity])
└─ return 101
CanvasRoom webSocketMessage(ws, msg)
├─ if msg === 'ping' → ws.send('pong'); return
└─ else → ws.close(1003, 'unsupported message')
Client (CanvasRenderer.svelte loadCanvas)
let pendingWsEdits = [];
ws.onmessage during fetch → push to pendingWsEdits (don't apply)
fetch resolves:
committedColors = new Uint8Array(indices);
apply pendingWsEdits to committedColors
pendingWsEdits = null
flag "live mode" — onmessage now applies directly
Client (App.svelte)
setInterval(() => ws.send('ping'), 30_000)
trackPongTimer; if no pong in 60s → ws.close()
```
## Related Code Files
**Modify**
- `src/worker.js` — origin check before WS upgrade forwarding
- `src/durable-objects/canvas-room.js` — per-identity conn cap, ping handling, tag-aware acceptWebSocket
- `src/client/components/CanvasRenderer.svelte` — buffer-and-replay during initial fetch (around lines 465–484)
- `src/client/App.svelte` — ping interval, pong watchdog
- `src/lib/constants.js` — add `MAX_WS_PER_IDENTITY = 5`
- `wrangler.json` — add `vars: { ALLOWED_ORIGINS: "https://rplace.miti99.workers.dev" }`
**Create** — none
## Implementation Steps
1. **Origin allowlist in worker** (debugger H5)
- Read `env.ALLOWED_ORIGINS` (comma-separated). Parse to Set at module top.
- In `/api/ws` handler: if `Origin` header present and not in allowlist, return `c.text('forbidden_origin', 403)`. Empty allowlist → allow all (dev default).
- Document in `wrangler.json` comment.
2. **Per-identity WS cap in DO** (review H4, debugger L6)
- In `#handleWsUpgrade(request, identity)`: `const existing = this.state.getWebSockets(identity);`
- If `existing.length >= MAX_WS_PER_IDENTITY` → return `new Response('too_many_sockets', { status: 429 })`.
- Replace `state.acceptWebSocket(server)` with `state.acceptWebSocket(server, [identity])`.
3. **Server-side heartbeat** (review M3, debugger M3)
- In `webSocketMessage(ws, message)`: if `message === 'ping'` → `ws.send('pong'); return;`. Else keep current close behavior.
- Note: this works under hibernation because messages auto-rehydrate the DO.
4. **Client buffer-and-replay** (review C2)
- In `loadCanvas` (`CanvasRenderer.svelte:465-484`):
- Add `let pendingWsEdits = [];` and `let isLive = false;` at top of `loadCanvas`.
- Expose `pushWsEdit(edit)` from the component: if `!isLive` → `pendingWsEdits.push(edit)`; else apply directly.
- After fetch resolves and `committedColors = new Uint8Array(indices)`, replay: for each `edit` in `pendingWsEdits`, write to `committedColors[edit.idx] = edit.color` AND update `imageData`. Then `isLive = true; pendingWsEdits = null;`.
- In parent (`App.svelte`), route ws.onmessage pixel events to `canvasRenderer.pushWsEdit(...)` instead of applying directly when first connect.
5. **Client heartbeat** (review M3)
- In `App.svelte` WS open handler: start `setInterval(() => ws.readyState === 1 && ws.send('ping'), 30_000)`. Track `lastPongAt = Date.now()`.
- On message `'pong'`: `lastPongAt = Date.now()`.
- Watchdog: if `Date.now() - lastPongAt > 60_000` → `ws.close()` to trigger reconnect logic.
- Clear interval/watchdog on `onclose`.
6. **Compile + smoke**
- `npm run build` passes.
- Open dev console, throttle network to "Slow 3G", reload, place pixel from a second tab during fetch, confirm pixel appears in tab 1 once fetch completes.
- Try opening 6 WS connections from same browser → 6th gets 429.
- Try opening WS from a different origin (curl with `Origin: https://evil.example`) → 403.
- Confirm `ping`/`pong` round-trips in dev console.
## Todo List
- [ ] Origin allowlist parsing + worker check
- [ ] Per-identity WS cap (`MAX_WS_PER_IDENTITY`) with `acceptWebSocket(server, [identity])`
- [ ] Server `ping` handler returns `pong`
- [ ] `wrangler.json` `vars.ALLOWED_ORIGINS`
- [ ] Client buffer-and-replay for WS during initial fetch
- [ ] Client 30s ping / 60s pong watchdog
- [ ] `npm run build` passes
- [ ] Manual smoke: race-fix verified by slow network reload + remote pixel placement
- [ ] Manual smoke: 6th WS rejected with 429
- [ ] Manual smoke: foreign-origin WS rejected with 403
- [ ] Manual smoke: ping/pong visible in dev tools
## Success Criteria
- [ ] No pixel placed during the initial-fetch window is dropped (verified via instrumented log).
- [ ] WS upgrade from disallowed origin returns 403 in production.
- [ ] Per-identity cap enforced; logs show `too_many_sockets` when triggered.
- [ ] `onclose` fires within ~60s of network drop (verified via airplane-mode toggle).
- [ ] No regression in 94-test suite.
## Risk Assessment
- **Risk:** Origin allowlist set too tight → legitimate clients (preview deployments, custom domains) get 403.
**Mitigation:** Empty `ALLOWED_ORIGINS` allows all — start with empty in dev/preview, populate before production deploy.
- **Risk:** Cap on identity blocks tab-power-users (5 tabs is normal for some folks).
**Mitigation:** Cap is configurable; bump to 10 if support tickets appear.
- **Risk:** Heartbeat interval too aggressive → battery drain on mobile.
**Mitigation:** 30s ping is well below the typical mobile-radio-wakeup penalty; keeping interval >= 25s avoids extra wakes.
- **Risk:** Buffer-and-replay logic interacts oddly with the existing `imageData` invalidation in CanvasRenderer.
**Mitigation:** Replay loop must call the same path the live message handler does (write to both `committedColors` AND `imageData`); add a unit test in Phase 4.
## Security Considerations
- Origin check is a usability/cost barrier, not a security one — WS protocol allows non-browser clients to spoof Origin. Real defense is the per-identity cap + Worker request budget.
- Per-identity cap prevents broadcast amplification (debugger H5, L6).
- Heartbeat surface is a single text-equality check; no parser exposure.
plans/260510-0232-fix-do-migration-followups/phase-04-do-surface-tests.md
+161
View File
@@ -0,0 +1,161 @@
---
phase: 4
title: "Full DO surface test coverage"
status: pending
priority: P2
effort: "6h"
dependencies: [1, 2, 3]
---
# Phase 4: Full DO Surface Test Coverage
## Overview
Close the test gap: today only `test/worker-validation.test.js` exists. The DO surface (chunk-storage, cooldown-store, WS hub, identity resolution) has zero unit tests. Add a vitest harness that runs against `wrangler unstable_dev` so DO state is real, not mocked.
## Context Links
- Reports: `plans/reports/code-reviewer-260510-0211-rplace-do-migration.md` (positive observations re tests; gap implicit)
- Reports: `plans/reports/debugger-260510-0211-rplace-edge-cases.md` (L7 — explicit DO test gap)
## Key Insights
- Mocked DO unit tests (current style) won't catch the bugs Phase 1–3 fix. Need real DO state.
- `wrangler unstable_dev` lets vitest hit a live local Worker + DO via fetch. Heavier but accurate.
- For pure-function modules (`pixel-buffer`, `chunk-storage` math), keep classic vitest unit tests — fast loop.
- The 200-line file rule applies to test files too; split per concern (one test file per source file).
## Requirements
**Functional**
- Tests cover: write atomicity, BLOB-grow, orphan-row read, cooldown refund, cooldown TTL, WS broadcast frame format with `seq`, WS per-identity cap, ping/pong, identity cookie/IP/dev fallback, gap detection.
- All tests pass on `npm test`.
**Non-functional**
- Total test runtime under 30s on a dev machine.
- No hard-coded sleeps > 100ms; use polling helpers where needed.
- Tests are deterministic — no flakes when run 10× consecutively.
## Architecture
```
test/
├── lib/
│ ├── cookie.test.js # NEW — parseCookie/formatSetCookie
│ ├── get-user-id.test.js # EXTEND — cookie/ip/dev fallback paths
│ ├── pixel-buffer.test.js # EXISTING — keep
│ └── ... (existing image-* tests stay)
├── durable-objects/
│ ├── chunk-storage.test.js # NEW — pure-function unit tests via mocked sql
│ ├── cooldown-store.test.js # NEW — TTL math, GC sample, race
│ └── canvas-room.integration.test.js # NEW — wrangler unstable_dev
├── worker-validation.test.js # EXISTING — keep
└── helpers/
└── do-harness.js # NEW — boot wrangler unstable_dev once per file
```
`do-harness.js` exports `setupDO()` returning `{ worker, fetch, close }`. Suite uses `beforeAll` / `afterAll` to share the harness.
## Related Code Files
**Create**
- `test/helpers/do-harness.js`
- `test/lib/cookie.test.js`
- `test/durable-objects/chunk-storage.test.js`
- `test/durable-objects/cooldown-store.test.js`
- `test/durable-objects/canvas-room.integration.test.js`
**Modify**
- `test/lib/get-user-id.test.js` — extend for new resolution logic
- `vitest.config.js` — add `testTimeout: 30_000`, include `test/durable-objects/**/*`
- `package.json` — no new deps; `wrangler` is already a devDependency
**Delete** — none
## Implementation Steps
### Pure-function tests
1. **`test/lib/cookie.test.js`**
- `parseCookie('a=1; b=2')` → Map `{a:'1', b:'2'}`
- `parseCookie('')` → empty Map
- `parseCookie(undefined)` → empty Map
- Malformed: `parseCookie('a; b=2')` → Map `{b:'2'}`
- `formatSetCookie('rplace_id', 'uuid', { httpOnly: true, secure: true, sameSite: 'Lax', maxAge: 31536000, path: '/' })` → exact string match.
2. **`test/lib/get-user-id.test.js`** (extend)
- Cookie present → `cookie:<uuid>`
- No cookie, IP present → `ip:<hash>`
- Production env, neither → throws `NoIdentityError`
- Dev env, neither → `dev:*`
- Cookie + IP both present → cookie wins
- Cookie issuance flag set when caller should `Set-Cookie`
3. **`test/durable-objects/chunk-storage.test.js`** (mock sql)
- Build a fake `sql` with an in-memory `Map<chunkId, Uint8Array>` backing `INSERT OR REPLACE` and `SELECT`.
- `writePixels` single chunk: write 3 pixels, read back identical bytes.
- `writePixels` 2-chunk batch: pixels at offset 0 and offset 65535+1, both persist.
- `writePixels` BLOB-grow: pre-seed last chunk with a short blob (8KB), grow via fake `chunkSize` returning 64KB, write a pixel at byte 30000, read back exactly 64KB blob with byte 30000 set.
- `readAllChunks` orphan: pre-seed `chunk_id = 999`, `CHUNK_COUNT = 256`, read returns valid `Uint8Array(TOTAL_PIXELS)` without throwing.
- `chunkSize(255)` for current dims = 65536; `chunkSize(0..254)` = 65536.
- Test the bound query: orphan row not selected.
4. **`test/durable-objects/cooldown-store.test.js`** (mock sql)
- `tryAcquire(userId, now)` first call returns `{ ok: true }`.
- Second call within 1s returns `{ ok: false, retryAfter: <ms remaining> }`.
- Second call after 1s returns `{ ok: true }`.
- GC sample with deterministic RNG: pre-seed expired rows, run `tryAcquire` with mocked `Math.random()` returning 0 (always GC) → expired rows deleted.
- INSERT cursor symmetry: ensure both branches drain (after fix in Phase 1's M1).
### Integration tests (real DO)
5. **`test/helpers/do-harness.js`**
- `import { unstable_dev } from 'wrangler'`
- `export async function setupDO() { const worker = await unstable_dev('src/worker.js', { config: 'wrangler.json', experimental: { disableExperimentalWarning: true } }); return { worker, fetch: worker.fetch.bind(worker), close: () => worker.stop() }; }`
- Add helper `placePixel(worker, { x, y, color, cookie? })` returning `{ status, body, setCookie }`.
6. **`test/durable-objects/canvas-room.integration.test.js`**
- **Cookie issuance:** GET `/api/canvas` without cookie → response has `Set-Cookie: rplace_id=...`. Subsequent GET with cookie → no new Set-Cookie.
- **Cooldown isolation:** two cookies, same simulated IP → both place pixels in the same second.
- **Cooldown refund on error:** force a write error path (test-only env flag throws inside `writePixels`) → cooldown row deleted, second attempt succeeds immediately.
- **Multi-chunk atomicity:** force-error mid-batch on second chunk → `GET /api/canvas` shows none of the batch applied, broadcast not fired.
- **Orphan row:** seed orphan via DO debug endpoint (test-only) → `GET /api/canvas` returns 200.
- **content-length=0 rejected:** POST `/api/place` with `Content-Length: 0` → 411 or 413.
- **WS broadcast carries seq:** open WS, place a pixel, receive frame with `{ type:'pixels', seq:1, edits:[...] }`. Place another → `seq:2`.
- **WS per-identity cap:** open 5 WS with same cookie → all accepted. 6th → 429 / connection-close.
- **WS ping/pong:** send `'ping'`, receive `'pong'`.
- **WS gap → client refetch:** simulate dropped frame by manually skipping a `seq`; client-side gap detection is a unit-test concern (covered in `test/lib/seq-gap.test.js` if extracted; otherwise verify protocol shape only).
- **Origin allowlist:** WS upgrade with `Origin: https://evil.example` → 403; with no Origin → allowed.
## Todo List
- [ ] Decide test-only env flag mechanism (e.g., `env.TEST_FORCE_WRITE_ERROR === '1'`) and add gated branches
- [ ] Write `test/helpers/do-harness.js`
- [ ] Write `test/lib/cookie.test.js`
- [ ] Extend `test/lib/get-user-id.test.js`
- [ ] Write `test/durable-objects/chunk-storage.test.js`
- [ ] Write `test/durable-objects/cooldown-store.test.js`
- [ ] Write `test/durable-objects/canvas-room.integration.test.js`
- [ ] Update `vitest.config.js` (timeout + include patterns)
- [ ] `npm test` — all green
- [ ] Run 10× back-to-back; no flakes
## Success Criteria
- [ ] All Phase 1–3 fixes have at least one test that fails on the pre-fix code and passes after.
- [ ] Coverage report shows DO source files at ≥ 80% line coverage.
- [ ] Total test runtime under 30s on dev hardware.
- [ ] CI (if configured) passes; otherwise local 10× green.
## Risk Assessment
- **Risk:** `wrangler unstable_dev` is not stable across versions; tests may break on upgrade.
**Mitigation:** Pin `wrangler` version in `package.json` (already pinned to `^4.14.1`); update tests when intentionally bumping.
- **Risk:** Test-only env flags leak into production.
**Mitigation:** Gate behind `env.NODE_ENV === 'test'` AND require an explicit `env.TEST_HOOKS === 'enabled'` second flag. Document; reject in deploy script.
- **Risk:** Integration tests slow down dev loop, devs skip them.
**Mitigation:** Add `npm run test:unit` (excludes `**/integration.*`) for fast inner loop.
- **Risk:** WS testing in vitest is awkward (`unstable_dev` returns Worker, not Server).
**Mitigation:** Use `worker.fetch` for upgrade and treat the returned `WebSocket` directly. If tooling doesn't allow that, fall back to spawning `wrangler dev` in a child process.
## Security Considerations
- Test-only hooks must never ship to production. Add a build-time assertion (e.g., a fail-fast check at worker startup if `TEST_HOOKS` is enabled in production env).
- Tests must not commit any secrets — use `crypto.randomUUID()` for ephemeral test cookies.
plans/260510-0232-fix-do-migration-followups/phase-05-docs-cleanup.md
+121
View File
@@ -0,0 +1,121 @@
---
phase: 5
title: "Docs cleanup & legacy plan archival"
status: pending
priority: P2
effort: "30m"
dependencies: []
---
# Phase 5: Docs Cleanup & Legacy Plan Archival
## Overview
Strike stale Upstash references from `README.md` and `.env.example`. Verify the `docs/` directory matches current code (the docs-manager and code-reviewer reports disagree — verify file by file). Mark the migration plan as `completed` after the 7-day rollback observation window.
## Context Links
- Reports: `plans/reports/docs-manager-260510-0211-rplace-docs-drift.md` (primary)
- Reports: `plans/reports/code-reviewer-260510-0211-rplace-do-migration.md` (C1 — claims docs/*.md still mention Upstash; conflicts with docs-manager finding)
## Key Insights
- Docs-manager said `docs/system-architecture.md`, `deployment-guide.md`, `code-standards.md` are accurate. Code-reviewer C1 said they still reference Upstash. Verify directly before deciding.
- Migration endpoint already removed from worker; documenting it as "transitional" misleads onboarding.
- `.env.example` placeholders for deleted services cause new-developer confusion.
## Requirements
**Functional**
- README's Project Structure block lists only files that exist in `src/`.
- README does not advertise `/admin/migrate-from-upstash` as transitional.
- `.env.example` either deleted or replaced with a comment explaining no external secrets.
- All `docs/*.md` files verified accurate; any Upstash references that describe current state (not history) removed.
- `plans/260509-2309-canvas-on-do-storage/plan.md` frontmatter `status: completed` (after 2026-05-17, the rollback window cutoff).
**Non-functional**
- Edits target only the lines in question; no unrelated reformatting.
- Git history preserves clear "docs:" commit type per `.claude` convention (drop `docs:` from `.claude/` paths only — not relevant here).
## Architecture
N/A — text edits.
## Related Code Files
**Modify**
- `README.md` — Project Structure tree (lines 79–111), API section (lines 146–151)
- `.env.example` — content
- `docs/system-architecture.md` — only if verification finds Upstash-as-current text
- `docs/deployment-guide.md` — only if verification finds Upstash-as-current text
- `docs/code-standards.md` — only if verification finds Upstash-as-current text
- `plans/260509-2309-canvas-on-do-storage/plan.md` — frontmatter `status` field (after rollback window)
**Create** — none
**Delete** — possibly `.env.example` (Option A in docs-manager report)
## Implementation Steps
1. **Verify docs/ accuracy**
- `grep -ni 'upstash\|redis\|@upstash' docs/*.md`
- For each match: read context, classify as (a) historical mention OK, (b) presented as current — needs edit.
- Decide whether to keep migration narrative as historical or scrub entirely.
2. **README.md Project Structure** (docs-manager STALE finding 1)
- Replace lines 79–111 with the corrected tree from docs-manager report (no `src/admin/`, no legacy lib files; include image pipeline files).
- Use the actual `find src -type f` output as ground truth.
3. **README.md API Section** (docs-manager MISLEADING finding 2)
- Delete the `### POST /admin/migrate-from-upstash (transitional)` block (lines 146–151).
- Verify no other doc references this section.
4. **.env.example** (docs-manager STALE finding 3)
- Choose Option B (keep as documentation): replace contents with a single comment block:
```
# No external secrets required.
# Canvas + cooldown state live inside CanvasRoom Durable Object (SQLite).
# All configuration is in src/lib/constants.js.
```
- Rationale: discoverability. New devs find `.env.example` and learn there's nothing to set.
5. **docs/ targeted edits** (only if step 1 found drift)
- Edit each flagged section. Keep migration mentioned in `deployment-guide.md` "Optional One-Shot Migration from Upstash" as historical — do not delete the narrative if it's already framed as past.
6. **Migration plan archival** (docs-manager finding 4)
- Today is 2026-05-10; rollback window per migration-plan ends ~2026-05-17.
- Add a TODO note in this phase saying "after 2026-05-17, change `plans/260509-2309-canvas-on-do-storage/plan.md` frontmatter `status` to `completed`".
- For now (before 2026-05-17), leave `status: in-progress` BUT add `deployment.cleanupAt: 2026-05-10` (already present per existing frontmatter) and verify it's accurate.
7. **Sanity sweep**
- `grep -rni 'upstash\|@upstash\|redis-client\|rate-limiter\.js\|migrate-from-upstash' README.md docs/ .env.example src/` — should return only intentional historical refs.
- `npm run build` — must still pass (sanity check that a doc edit didn't break a `<script src>` reference or similar).
## Todo List
- [ ] Run grep sweep on `docs/` for upstash/redis terms
- [ ] Classify each match (historical vs current-as-of-today)
- [ ] Edit README.md Project Structure tree
- [ ] Delete README.md `/admin/migrate-from-upstash` API section
- [ ] Rewrite `.env.example` with documentation comment
- [ ] Apply targeted edits in `docs/*.md` if step 1 found current-as-of-today refs
- [ ] Add reminder note (this phase) to flip migration plan status to `completed` after 2026-05-17
- [ ] Final grep sweep — no stale refs remain
- [ ] `npm run build` passes
## Success Criteria
- [ ] `find src -type f` matches the README Project Structure tree
- [ ] No `/admin/migrate-from-upstash` reference in `README.md`
- [ ] `.env.example` is documentation-only or absent
- [ ] `docs/*.md` accurately describes current code (no current-tense Upstash refs)
- [ ] Final grep sweep shows only intentional historical mentions
## Risk Assessment
- **Risk:** Editing docs accidentally drops useful historical context for future operators.
**Mitigation:** Keep "Optional One-Shot Migration from Upstash" sections as past-tense narrative; only scrub anything that says "use this NOW".
- **Risk:** Premature archival of migration plan loses rollback information.
**Mitigation:** Wait until 2026-05-17 (7-day window). Frontmatter already records `migratedAt` and `postCleanupVersionId` — those stay regardless of `status`.
## Security Considerations
N/A — docs only.
## Next Steps
None — this is the cleanup tail.
plans/260510-0232-fix-do-migration-followups/plan.md
+69
View File
@@ -0,0 +1,69 @@
---
title: "Fix critical bugs and security gaps from DO migration code review"
status: in-progress
priority: P1
created: 2026-05-10
phases: 5
source: skill
sourceReports:
- ../reports/code-reviewer-260510-0211-rplace-do-migration.md
- ../reports/debugger-260510-0211-rplace-edge-cases.md
- ../reports/docs-manager-260510-0211-rplace-docs-drift.md
blockedBy: []
blocks: []
---
# Plan: Fix DO Migration Code-Review Follow-ups
## Goal
Close the 3 Critical + 5 High findings from the post-migration triple-review (code-reviewer, debugger, docs-manager). Add full DO surface test coverage. Scrub stale Upstash references from `README.md` and `.env.example`.
## Decisions Locked (from validation Q&A)
- **Identity:** Cookie + IP fallback. Issue opaque per-browser cookie on first `/api/canvas`; rate-limit by cookie when present, fall back to IP otherwise.
- **Daily pixel cap:** Deferred to a follow-up plan (product input needed).
- **Test scope:** Full DO surface coverage (chunk-storage, cooldown-store, WS hub, get-user-id, integration).
## Phases
| # | File | Title | Status | Priority | Blocks |
|---|---|---|---|---|---|
| 1 | [phase-01-do-storage-atomicity.md](phase-01-do-storage-atomicity.md) | DO storage atomicity & correctness | pending | P1 | 4 |
| 2 | [phase-02-cookie-ip-identity.md](phase-02-cookie-ip-identity.md) | Cookie+IP identity & broadcast sequence | pending | P1 | 4 |
| 3 | [phase-03-ws-hardening-client-race.md](phase-03-ws-hardening-client-race.md) | WebSocket hardening & client race fix | pending | P1 | 4 |
| 4 | [phase-04-do-surface-tests.md](phase-04-do-surface-tests.md) | Full DO surface test coverage | pending | P2 | — |
| 5 | [phase-05-docs-cleanup.md](phase-05-docs-cleanup.md) | Docs cleanup & legacy plan archival | pending | P2 | — |
Phases 1, 2, 3 are independent and can ship in any order or in parallel. Phase 4 depends on the API surfaces stabilized in 1–3. Phase 5 is independent — can land first.
## Findings Coverage Map
| Phase | Critical | High | Medium |
|---|---|---|---|
| 1 | C2 (review C3 BLOB-grow), C2 (debugger atomicity), C1 (debugger cooldown burn) | review-H1, H2, H3, H5 | review-M1 |
| 2 | C3 (debugger NAT) | debugger-H2, H3 | review-M4 |
| 3 | C2 (review WS race) | review-H4, debugger-H5 | review-M2, M3, M7 |
| 4 | — | L7 test gap | — |
| 5 | C1 (review stale docs) | — | — |
## Out of Scope
- Per-IP daily pixel quota (deferred per Q&A).
- Edge-cache strategy change (`s-maxage` tuning) — needs product call on live-fresh vs cheap.
- Multi-room sharding (`idFromName('main')` stays single-DO).
- Signed cookie / HMAC identity — opaque cookie is sufficient for this round.
## Success Criteria
- All listed Critical + High findings resolved with file:line citations in PR description.
- `npm test` passes; new DO tests cover write atomicity, BLOB-grow, cooldown refund, WS hub, identity.
- README + `.env.example` purged of Upstash refs; docs/ verified accurate.
- Production deploy + 24h soak shows no new error class in CF logs.
- Migration plan `260509-2309-canvas-on-do-storage/plan.md` marked `status: completed`.
## Related Reports
- `plans/reports/code-reviewer-260510-0211-rplace-do-migration.md`
- `plans/reports/debugger-260510-0211-rplace-edge-cases.md`
- `plans/reports/docs-manager-260510-0211-rplace-docs-drift.md`
plans/reports/code-reviewer-260510-0211-rplace-do-migration.md
+321
View File
@@ -0,0 +1,321 @@
# Code Review — rplace DO migration (Phases 1–4)
**Date:** 2026-05-10
**Scope:** Last 4 commits (c3f7c02 → a977adc) — Upstash → DO SQLite migration + cleanup
**Reviewer:** code-reviewer
## Summary
Migration is functionally correct on the happy path. SQLite-backed canvas + cooldown design is sound and well-commented. Primary issues are (1) **stale documentation referencing deleted code** (real risk: misleads contributors / ops on rollback), (2) a **client-side race** where WebSocket pixel broadcasts received during the initial canvas fetch are silently dropped, (3) a **resize-grow correctness bug** in `writePixels` for canvases whose previous last-chunk was short, and (4) several **minor security / DOS gaps** at the edge.
LOC reviewed: ~700 (server) + ~1200 (client). Tests: 8 files, all green per commit notes (94/94).
---
## Critical
### C1. Stale documentation references deleted code paths
**Files:**
- `README.md:84-97` — describes `src/admin/migrate-from-upstash.js`, `src/lib/canvas-storage.js`, `src/lib/redis-client.js`, `src/lib/rate-limiter.js` as if they exist (all deleted in a977adc)
- `README.md:146-151` — documents `POST /admin/migrate-from-upstash` endpoint as "transitional" but the worker no longer mounts it (returns 404 in production per commit message)
- `docs/system-architecture.md:117-124` — same migration endpoint documented as live
- `docs/deployment-guide.md:34-65,107-111` — full "Optional One-Shot Migration from Upstash" section + troubleshooting entries reference the deleted endpoint and secrets
- `docs/code-standards.md:29-32` — "Functions receive env parameter for Cloudflare bindings (Redis credentials...)", "Use `@upstash/redis/cloudflare`", "Bitfield operations use builder pattern"
**Impact:** A dev reading these docs will try to call a 404 endpoint, set secrets that don't exist, or write code against `@upstash/redis` which is no longer in `package.json`. Ops doing rollback per `docs/deployment-guide.md` will be confused.
**Severity:** Critical (docs claim functionality that's been removed; this is exactly what a "migration cleanup" PR should not leave behind).
**Fix:** Strike the entire migration / Upstash sections. Replace `code-standards.md:27-32` with the DO-binding pattern actually in use. Verify `docs/references.md` Redis links remain only as historical references.
### C2. WebSocket updates dropped during initial canvas fetch
**File:** `src/client/components/CanvasRenderer.svelte:465-484`
The renderer pre-allocates a zero `committedColors` so WS messages arriving before `loadCanvas` resolves don't null-deref (line 16, comment at 14-16). But once the fetch completes, line 473 unconditionally **replaces** the array:
```js
committedColors = new Uint8Array(indices); // replace pre-allocated zero array
```
Any pixel writes that landed in the pre-allocated array between WS connect and fetch resolve are silently overwritten. Same hazard for `imageData` (line 475).
**Repro:** User A opens app. WS connects fast, fetch is slow (16 MB binary). User B places a pixel. User A's WS receives it → writes to `committedColors[idx]`. Fetch resolves → `committedColors = new Uint8Array(indices)` (which was sampled at server BEFORE B's pixel hit, since the GET hits a 10s edge cache). User B's pixel is invisible to A until the next WS update or refresh.
**Severity:** Critical — this is the exact data-loss scenario the architecture comment at `App.svelte:122-126` ("Refetch canvas after a reconnect") tries to prevent, but only triggers on `isReconnect`, not initial connect.
**Fix options:**
1. Buffer WS messages until fetch resolves, replay on completion.
2. After replacing `committedColors`, re-apply the pre-fetch WS edits (track them in a side map).
3. Open WS only after fetch completes (loses real-time updates during load — likely worst option).
### C3. `writePixels` silently drops writes on resize-grow path
**File:** `src/durable-objects/lib/chunk-storage.js:88-101`
When the canvas is grown (CANVAS_WIDTH or CANVAS_HEIGHT bumped), the old last-chunk row's BLOB is stored at the old `chunkSize(lastChunkId)` size, which is shorter than the new `CHUNK_BYTES`. The grow-path read returns that short blob; `new Uint8Array(buf)` (line 92) preserves the short length; `next[byteOffset] = color` (line 94) is a **no-op when byteOffset ≥ next.length** (typed-array OOB writes are silently dropped per spec). The short blob is then INSERT-OR-REPLACE'd unchanged.
**Effect:** New pixels written into the formerly-last chunk after a resize-grow disappear.
This isn't theoretical — `docs/canvas-resize-procedure.md` explicitly recommends growing the canvas as a config-only change. With a 4096×4096 canvas all chunks are full 64KB so the bug is dormant *today*. Bump width to 4097 and the bug activates immediately.
**Severity:** Critical (data loss after a documented operation).
**Fix:** In `writePixels`, allocate `next` to `chunkSize(chunkId)` and copy `buf` into it:
```js
const expected = chunkSize(chunkId); // current expected size for this chunk
const next = new Uint8Array(expected);
next.set(buf.subarray(0, Math.min(buf.length, expected)));
```
Or always allocate `CHUNK_BYTES` for non-last chunks and `chunkSize(lastId)` for the last; never trust the persisted blob's length.
---
## High
### H1. Cooldown is consumed even on storage failure
**File:** `src/durable-objects/canvas-room.js:73-86`
`tryAcquire` runs first (line 73). If `writePixels` then throws (lines 78-83), the user gets a 500 but their cooldown row is already updated. They're locked out for 1s without their pixel placement having succeeded.
**Severity:** High — bad UX on transient errors; effectively turns a storage flake into a 1s soft-DOS of the user.
**Fix:** Rollback the cooldown UPDATE in the catch:
```js
sql.exec('DELETE FROM cooldowns WHERE user_id = ?', userId);
```
Or run write first, then cooldown — but that opens a different race (concurrent placements within DO would all succeed before any cooldown row exists). Safer is the rollback.
### H2. Server response leaks raw error to client
**File:** `src/durable-objects/canvas-room.js:82`
```js
return Response.json({ error: 'storage_failed', message: String(err) }, { status: 500 });
```
`String(err)` may include SQLite error messages, paths, query fragments. Low risk because the DO's SQL is internal but it still violates the "don't leak internals" rule and sets a precedent.
**Severity:** High (security best-practice: never echo `err.toString()` over the wire from a 5xx).
**Fix:** Drop `message` from the response; keep only `error: 'storage_failed'`. Log the full error (already done at line 81).
### H3. Chunk-storage `readAllChunks` crashes on orphaned shrink rows
**File:** `src/durable-objects/lib/chunk-storage.js:46-56`
`SELECT chunk_id, bytes FROM canvas_chunks` returns ALL rows including any with `chunk_id ≥ CHUNK_COUNT` (orphans left after a resize-shrink — explicitly documented as possible in `docs/canvas-resize-procedure.md:32-37`). Then `out.set(view, chunkId * CHUNK_BYTES)` will throw RangeError if `chunkId * CHUNK_BYTES + view.length > out.length`. Since `out` is `TOTAL_PIXELS` long and an orphan has a higher chunk_id, this will throw.
**Severity:** High — `GET /api/canvas` would 500 forever after a shrink, until manual cleanup.
**Fix:** `WHERE chunk_id < ?` bound, or `if (chunkId >= CHUNK_COUNT) continue;` defensive skip. The resize doc's "to reclaim, run DELETE" advice should not be a precondition for the read path.
### H4. WebSocket connections are uncapped per-IP
**File:** `src/durable-objects/canvas-room.js:89-94`
`#handleWsUpgrade` accepts every incoming WS upgrade unconditionally. A single client can open thousands of WSs — CF DO has a soft tens-of-thousands limit per object, but no per-source cap. Each connected WS receives every broadcast (`#broadcastPixels` iterates `getWebSockets()`), so 10K connections × N pixels-per-broadcast = 10K × N message sends.
**Severity:** High at hobby scale (one bad actor can saturate CPU on the singleton DO).
**Fix:** Track WS-per-userId via `state.acceptWebSocket(server, [userId])` tags, and deny upgrade past N existing tagged sockets (`getWebSockets(userId)`). Even a 10-per-IP cap eliminates the trivial DOS.
### H5. `content-length` body cap is bypassable
**File:** `src/worker.js:24-27`
`parseInt(c.req.header('content-length') || '0', 10)` defaults to 0 when missing. A malicious client can send chunked-transfer-encoded body without `content-length` and bypass the 128KB pre-parse cap. The CF runtime caps overall request size to 100 MB; until then, the JSON parser allocates as it reads. Practical exposure is "force the worker to allocate up to 100 MB before failing the per-pixel `batch_too_large` check."
**Severity:** High (DOS amplification, easy to fix).
**Fix:** Read raw body via `c.req.arrayBuffer()` with a hard byte cap, then `JSON.parse(decoder.decode(buf))`. Or stream-validate while reading. Or short-circuit if `content-length === 0` (no header → reject).
---
## Medium
### M1. `retryAfter` is always 1s, never the actual remaining window
**File:** `src/durable-objects/lib/cooldown-store.js:55`
On rate-limit denial, the function returns `retryAfter: REQUEST_COOLDOWN_SEC` (always 1s), but the user may have, e.g., 200ms left. Client (`App.svelte:220-222` and `image-uploader.js:104`) waits a full second when ~200ms would suffice.
**Severity:** Medium (UX, not correctness).
**Fix:** When the INSERT throws, run `SELECT expires_at FROM cooldowns WHERE user_id = ?` and return `Math.ceil((expires - now)/1000)`. Optional micro-opt: structure as `INSERT … ON CONFLICT … RETURNING` to avoid the round-trip.
### M2. Comment about WebSocket compat date is misleading / inverted
**File:** `src/durable-objects/canvas-room.js:119`
```js
// Required pre-2026-04-07 compat date; harmless after.
ws.close(code, reason);
```
The wrangler `compatibility_date` is `2025-04-01` (almost a year before the comment's "pre-2026-04-07" cutoff), so the explicit close IS needed. The comment reads as "you can delete this any day now" but actually says the opposite. Worth fixing before someone deletes the line.
**Severity:** Medium (foot-gun for future maintenance).
**Fix:** "Required because compatibility_date (2025-04-01) is before the 2026-04-07 default-close cutoff. Remove if/when wrangler.json bumps past that date."
### M3. WebSocket protocol is broadcast-only but doesn't ping/keepalive
**File:** `src/durable-objects/canvas-room.js:110-113`
`webSocketMessage` immediately closes any inbound message. That means clients can't ping the server to detect zombies. Browser will only know the connection is dead when the OS / proxy times it out (could be minutes). On the wire there's no heartbeat — `App.svelte:131-135` reconnects on `onclose`, but that won't fire if the network silently drops.
**Severity:** Medium (real-time UX during flaky networks).
**Fix:** Either accept `'ping'`/`'pong'` text messages (add small whitelist), or rely on Hibernation API auto-ping (verify CF behavior; docs are sparse). Lowest-cost option: client sends `WebSocket` protocol-level pings via a heartbeat timer; server must permit them.
### M4. Dev-bucket userId = `anon:dev` collapses all dev traffic into one rate-limit bucket
**File:** `src/lib/get-user-id.js:10-14`
In dev (no `cf-connecting-ip`), every request is bucketed as `anon:dev`. If this fallback ever triggers in prod (proxy misconfig, custom domain misrouted), all users share a single 1 req/s budget — soft-DOS for everyone.
**Severity:** Medium (production blast radius is total but trigger is unlikely).
**Fix:** In production (e.g., `env.ENVIRONMENT === 'production'`), throw or 500 instead of falling back. Or use `request.cf?.colo` as a salt, or fall back to `x-real-ip` / `x-forwarded-for`. Document the assumption clearly.
### M5. `Math.random()`-based GC sample assumes per-call randomness in CF Workers
**File:** `src/durable-objects/lib/cooldown-store.js:36,50`
`Math.random()` in CF Workers historically had unusual semantics around isolate reuse. If it returns the same value across all `tryAcquire` calls in an isolate, GC either fires every time or never. Modern CF runtime is supposed to handle this, but worth verifying with a quick log.
**Severity:** Medium (correctness depends on platform behavior, not visible from the code).
**Fix:** If unsure, swap to `crypto.getRandomValues(new Uint8Array(1))[0] < 256 * GC_SAMPLE_RATE` for guaranteed entropy. Or trigger GC every Nth call via a counter on the DO instance.
---
## Low
### L1. `new Uint8Array(buf)` copy in `writePixels` is slower than `.slice()`
**File:** `src/durable-objects/lib/chunk-storage.js:92`
The iterable-constructor copy walks element-by-element. `buf.slice()` uses memcpy. For 64KB and 1 req/sec, irrelevant — but the comment at 90-91 implies a copy is required for safety, and `.slice()` reads cleaner.
**Severity:** Low.
### L2. Cooldown `retryAfter` not surfaced in seconds with sub-second precision
**File:** `src/durable-objects/lib/cooldown-store.js:55`
If we ever want sub-second cooldowns, the `retryAfter` integer second contract caps us. Worth typing `retryAfterMs` for forward compat.
### L3. `writePixels` per-pixel branch could be vectorized for big batches
**File:** `src/durable-objects/lib/chunk-storage.js:88-101`
For a 2048-pixel batch all in one chunk, the inner write loop runs in JS one byte at a time. CF DO SQLite charges per-row, so cost is dominated by the BLOB write — but if batches grow (e.g., admin imports), the pure-JS loop becomes the bottleneck. Not relevant today.
### L4. `setOverlay`'s Texture.from is called twice on race
**File:** `src/client/components/CanvasRenderer.svelte:233-240,521-528`
If `setOverlay` is called before `initPixi` finishes, `overlayState` holds the data. After init, lines 521-528 materialize the sprite. But if a second `setOverlay` arrives mid-init, the first `overlayState` is overwritten silently (no second materialize-from-overlayState pass), so only the latest survives — actually correct behavior, just non-obvious. Worth a one-line comment.
### L5. Server-side `MAX_BATCH_SIZE` import in `canvas-room.js` is redundant
**File:** `src/durable-objects/canvas-room.js:58-60`
The DO re-validates bounds even though the worker did. Defense-in-depth is the stated rationale, fine. But the worker validates `pixels.length > MAX_BATCH_SIZE` *and* the DO does. If they ever diverge (different `MAX_BATCH_SIZE`), edge would reject what DO accepts. Single source of truth via shared constant — already true here; just a note.
### L6. WebSocket message JSON is rebuilt per broadcast; payload not reused
**File:** `src/durable-objects/canvas-room.js:97`
`JSON.stringify` once per broadcast (line 97) — already correct. Disregard. (Including this as a non-finding to confirm I checked.)
---
## Nit
### N1. `idx_cooldowns_expires` only used by the GC sweep
**File:** `src/durable-objects/lib/schema.js:30-32`
The index is consulted by `gc()` only — `tryAcquire` queries by primary key. Comment at 22-23 says "the index keeps the GC sweep cheap" — accurate but redundant given the index name. Fine as-is.
### N2. Worker comment about MAX_BODY_BYTES math
**File:** `src/worker.js:9-10`
`MAX_BATCH_SIZE * 64` overestimates by ~10× (real `{"x":2047,"y":2047,"color":31}` is 27 bytes plus 2 for `,` and brackets ≈ 30B). The 64B headroom is fine but noting that the actual cap on parsed JSON could be tighter if it ever matters.
### N3. `loadCanvas` overwrites `loadError` to null on retry — but doesn't clear the `loading` text in the error state
**File:** `src/client/components/CanvasRenderer.svelte:466-467,573-577`
When the user clicks Retry, both `loading` and the error banner are visible until the fetch completes. Visual nit.
### N4. `handleWsUpgrade` doesn't validate auth or origin
**File:** `src/durable-objects/canvas-room.js:89-94`
For a public collaborative canvas, no auth is needed. But the DO accepts upgrades from any origin. CF's WAF would handle malicious traffic before it gets here. Fine for the scope; document if any future feature gates per-user state.
---
## Edge Cases Found by Scout
| Path | Edge case | Found in |
|---|---|---|
| Initial render | WS message arrives between fetch start and replace | C2 |
| Resize-grow | Last chunk shorter than CHUNK_BYTES, new write past short length silently dropped | C3 |
| Resize-shrink | Orphan rows past CHUNK_COUNT crash readAllChunks with RangeError | H3 |
| Storage flake | Cooldown consumed but write failed → user soft-DOS'd 1s | H1 |
| 500 error path | Raw error string echoed to client | H2 |
| WebSocket flood | No per-IP / per-userId cap on concurrent sockets | H4 |
| Chunked POST | content-length = 0 bypasses pre-parse cap | H5 |
| Dev-misroute to prod | All anon:dev share 1/s globally | M4 |
| Network silent-drop | No client/server WS heartbeat | M3 |
| Math.random in workers | GC may fire every call or never depending on runtime | M5 |
| 429 retryAfter | Always 1s, never sub-second remaining | M1 |
---
## Positive Observations
- Atomic-by-virtue-of-DO model is the right call for a hobby-scale rplace clone. The "no await between cooldown + write + broadcast" comment at `chunk-storage.js:85-87` is excellent — exactly the kind of invariant that breaks when someone drops in `await` later.
- `tryAcquire`'s UPDATE-then-INSERT race-safe rate-limit is genuinely clever and well-explained at lines 14-19 of `cooldown-store.js`.
- Lazy chunk allocation (zero-fill on read) makes resize-grow trivially correct **on the read side**. Only the write side has the bug (C3).
- Schema is `IF NOT EXISTS`, idempotent — survives DO eviction cleanly.
- Edge validation at the worker is thorough and re-validated at the DO; a `Number.isInteger` check correctly rejects strings (test `worker-validation.test.js:115-119`).
- The WebSocket hibernation pattern is correct (`state.acceptWebSocket`, `webSocketMessage/Close/Error` handlers all present).
- `package-lock.json` cleanly removed 184 packages with the Upstash / testcontainers cleanup — no orphan deps observed in `package.json`.
---
## Recommended Actions
1. **Critical (must fix before next deploy):**
- C1 — purge migration / Redis references from README, system-architecture, deployment-guide, code-standards.
- C2 — buffer WS pixel messages until initial canvas fetch resolves; merge instead of replace.
- C3 — `writePixels` must size `next` against `chunkSize(chunkId)`, not against the persisted blob's length.
2. **High (should fix this week):**
- H1 — rollback cooldown row on `writePixels` failure.
- H2 — strip raw error from 500 response.
- H3 — bound `readAllChunks` by `chunk_id < CHUNK_COUNT` (or skip orphans).
- H4 — per-userId WS connection cap.
- H5 — replace content-length cap with bounded body read.
3. **Medium (next-sprint backlog):**
- M1 — return precise `retryAfterMs` from cooldown denial.
- M2 — fix the misleading WS-close comment.
- M3 — add WS heartbeat (server permits ping or auto-pings).
- M4 — production fail-closed when `cf-connecting-ip` missing.
- M5 — verify `Math.random()` semantics in CF Workers; switch to `crypto` if unclear.
4. **Low / Nit:** L1–L6, N1–N4 at code-review-cycle pace, no urgency.
---
## Metrics
- Files reviewed: 12 server + 4 client (skim) + 4 docs
- LOC reviewed: ~1900
- Issues found: 3 Critical, 5 High, 5 Medium, 6 Low, 4 Nit (total 23)
- Type coverage: N/A (JS, JSDoc-typed)
- Test coverage: 94/94 unit tests pass per commit message; not independently re-run
- Linting issues: not run (no lint script in package.json)
---
## Unresolved Questions
1. **Math.random() in CF Workers** — does it return per-call entropy or per-isolate-fixed values? (Affects M5 GC behavior.) Worth a one-line `wrangler tail` log to confirm.
2. **Hibernation API auto-ping** — does `state.acceptWebSocket` arrange a TCP-level keepalive, or do clients need to explicitly heartbeat? CF docs are unclear; would unblock M3.
3. **Production smoke-test for resize-grow** — is there an environment where we can verify C3 with a non-aligned `CANVAS_WIDTH` (e.g., 4097)? Otherwise the fix is correct-by-construction but unproven.
4. **CF `cf-cache-status: HIT` ratio in production** — README and deployment-guide claim 10s edge-cache will absorb most `/api/canvas` traffic, but no telemetry is wired up. Worth a single-line log + dashboard panel before traffic ramps.
5. **Single-DO singleton failure mode** — when `idFromName('main')` colocates a single DO, what is the user-visible behavior during CF colo failover? (Probably brief 5xx then recover.) Worth documenting in `docs/system-architecture.md`'s "Operational Notes" section.
---
**Status:** DONE_WITH_CONCERNS
**Summary:** Migration is structurally sound; 3 Critical and 5 High issues identified, primarily around stale docs (C1), client-side WS race (C2), resize-grow data-loss bug (C3), error-path hygiene (H1/H2/H3), and edge DOS surface (H4/H5). All have clear, scoped fixes.
**Concerns/Blockers:** C1 (stale docs) is the most embarrassing — fix before any new contributor onboards. C2 and C3 are real correctness bugs that the existing test suite will not catch.
plans/reports/debugger-260510-0211-rplace-edge-cases.md
+223
View File
@@ -0,0 +1,223 @@
# rplace edge-case audit (post-DO-migration)
Static adversarial review. Scope: cooldown + chunk storage + WS hub + edge cache + image-importer DoS + SQLite limits.
Constants used (from `src/lib/constants.js`):
- `CANVAS_WIDTH = CANVAS_HEIGHT = 4096`, `TOTAL_PIXELS = 16_777_216`
- `MAX_COLORS = 256`, `MAX_BATCH_SIZE = 2048`, `REQUEST_COOLDOWN_SEC = 1`
- `CHUNK_BYTES = 65536`, `CHUNK_COUNT = 256`
---
## Punch list (severity-ordered)
### CRITICAL
#### C1. Cooldown consumed even on storage failure → user locked out 1s with no write
- **Scenario:** `tryAcquire` succeeds, then `writePixels` throws (SQLite I/O error, BLOB-too-large, OOM, transient).
- **Trigger:** any unhandled SQL error inside `writePixels` (e.g. row > BLOB cap, see L1).
- **Observable:** client gets `500 storage_failed`, but the cooldown row was already inserted/updated. User must wait 1s before retrying. UX is mildly annoying for humans, fatal for the long-running image-uploader: each failed batch costs both the batch *and* the cooldown slot.
- **Evidence:** `src/durable-objects/canvas-room.js:73` (`tryAcquire` first), `:79` (`writePixels` after) — no compensating "release" path on failure.
- **Severity:** Critical for image upload (silently halves throughput on transient errors); High otherwise.
#### C2. WS broadcast happens before any commit barrier → other clients see pixels server may roll back
- **Scenario:** `writePixels` does N synchronous `INSERT OR REPLACE` calls in a single JS turn. Without an explicit transaction wrapper, each `sql.exec(...)` is its own auto-commit. If chunk K succeeds and chunk K+1 fails (disk pressure, BLOB limit), the partial state is already persisted *and* the broadcast (line 85) has not yet run — but the user sees `500` and may resubmit.
- **Trigger:** any error path on the second (or later) chunk write inside `writePixels`.
- **Observable:** canvas left in half-written state; subsequent `GET /api/canvas` returns it; broadcast never fires so connected WS clients don't see it until they refetch (cache 10s).
- **Evidence:** `src/durable-objects/lib/chunk-storage.js:88-102` — loop has no `transactionSync` despite the comment at L86-87 claiming atomicity by virtue of "no `await`". *No `await` ≠ atomic*; auto-commit per statement still applies.
- **Severity:** Critical. Atomicity claim in the comment is misleading and false in the failure case.
#### C3. NAT / CGNAT collision = group-rate-limit by IP
- **Scenario:** `getUserId` hashes only `cf-connecting-ip`. Mobile carriers, university networks, corporate proxies, and CGNAT all share single egress IPs across thousands of users.
- **Trigger:** any deployment with shared-IP users.
- **Observable:** all users behind the same IP share *one* 1 Hz bucket. First placer "wins" each second; everyone else gets `429`. Effectively unusable on mobile during peak.
- **Evidence:** `src/lib/get-user-id.js:9-23`. No cookie/session/fingerprint augmentation; no per-room or per-IP-group differentiation.
- **Severity:** Critical for product usability (not a security flaw, but a denial-of-service against legitimate users).
---
### HIGH
#### H1. Image-importer DoS / griefing — no per-IP daily quota
- **Scenario:** Per-IP rate limit is 1 batch (= 2048 pixels) / sec ≈ 7.37 M pixels/hour. One IP can repaint 44% of the entire canvas every hour, indefinitely. The image-importer (`src/lib/image-uploader.js:60-122`) is built to do exactly this.
- **Trigger:** anyone running the importer or a custom script with a multi-megapixel image. Multiple users behind the same NAT amplify it (see C3 inverted: NAT *costs* legit users; from the operator side a single bad actor with multiple clients still funnels through the same bucket).
- **Observable:** entire canvas can be overwritten every ~2 hrs by a single attacker. No global cap, no per-day cap, no throttle on "draw image" sessions.
- **Evidence:** no daily/hourly cap anywhere in `src/durable-objects/lib/cooldown-store.js` or `canvas-room.js`. `MAX_BATCH_SIZE = 2048` (constants L12) makes throughput 2048× a naive 1 Hz limit.
- **Severity:** High. Obvious griefing primitive.
#### H2. `cf-connecting-ip` missing → entire dev/preview traffic shares "anon:dev" bucket
- **Scenario:** Wrangler local dev (`wrangler dev`), preview URLs, custom Workers-for-Platforms tunnels, or any non-CF-fronted invocation drops `cf-connecting-ip`.
- **Trigger:** local dev, preview URLs, tests using real DOs.
- **Observable:** all dev traffic shares one cooldown bucket. Manifests as random `429`s when more than one tab is open during dev. Easy to mistake for a real bug.
- **Evidence:** `src/lib/get-user-id.js:11-14`.
- **Severity:** High for DX; not a production bug per se but trips reviewers.
#### H3. WS hibernation: hub state across rehydrate is fine, but no catch-up message protocol
- **Scenario:** Server hibernates DO, drops in-RAM state. Client stays connected (CF keeps the socket). On the next placement, `state.getWebSockets()` returns the rehydrated sockets and broadcast resumes — this part is correct.
- **The bug:** during hibernation gap *or* during reconnect, missed pixels are recovered only by `canvasRenderer.refetchCanvas()` on `onopen` *if* `isReconnect` is true (`App.svelte:122-129`). On the very first connect after a fresh page load, `isReconnect = false` (`:106`), so the initial canvas fetch is the only source of truth. If the canvas fetch happened seconds ago (CDN cache, see H4) and pixels were placed in the gap *between* canvas fetch completion and WS open, those pixels are missed silently until the user causes a refetch or another pixel near them lands.
- **Trigger:** slow page load, two HTTP/1 connection limits, or just unlucky timing.
- **Observable:** persistent stale pixels shown to the user; only fixed by reload or by another nearby placement triggering a render diff.
- **Evidence:** `App.svelte:103-144` does NOT serialize "fetch canvas → open WS"; both happen as separate effects. No version/seq number on broadcast frames to detect gaps.
- **Severity:** High. Common race in r/place clones.
#### H4. Edge cache (`max-age=10, s-maxage=10, stale-while-revalidate=30`) on `/api/canvas` writes a 10-second blind spot
- **Scenario:** Pixel placed at t=0. CF edge has cached canvas from t=−9. New tab loads at t=+0.5: gets the t=−9 snapshot. WS opens at t=+0.6 — but the pixel was already broadcast at t=0, so the new tab will *never* see it via WS, and will see the stale value until either a) cache expiry triggers a refetch or b) the same coord is repainted.
- **Trigger:** any reload during heavy paint activity. Worse with `stale-while-revalidate=30`: total stale window can reach ~40s if the revalidate is delayed.
- **Observable:** users on different tabs/devices see different canvases for tens of seconds. Image uploader's `shouldSkip` predicate (uploader L21-25) reads from this stale view and may "skip" pixels that *aren't* actually the right color, leaving holes.
- **Evidence:** `src/durable-objects/canvas-room.js:38` — `Cache-Control: public, max-age=10, s-maxage=10, stale-while-revalidate=30`.
- **Severity:** High. Visibly degrades multi-client UX; corrupts the importer's resume logic.
#### H5. WS upgrade forwards full request, but DO routes by `url.pathname` after Worker rewrites URL → WS upgrade may miss client IP / origin checks
- **Scenario:** `app.get('/api/ws', ...)` calls `room(c.env).fetch('http://do/ws', c.req.raw)`. The DO's `fetch` switches on `url.pathname` only (canvas-room.js:25). The WS upgrade handler `#handleWsUpgrade()` does *no* origin check, no auth, no IP rate-limit.
- **Trigger:** any client opens a WS to `/api/ws`. `Origin` header is unverified.
- **Observable:** any third-party site can open and hold a WS to the canvas DO. Hibernation lets connections sit cheaply, but every pixel placement broadcasts to all of them, multiplying egress per active user. With N hostile clients, broadcast cost is O(N) per placement.
- **Evidence:** `src/worker.js:67-73`, `src/durable-objects/canvas-room.js:89-94`. No `Origin` allowlist, no max-clients.
- **Severity:** High. Cheap WS-amplification attack on the broadcast hub.
---
### MEDIUM
#### M1. `tryAcquire` `INSERT … VALUES` race — relies on PK conflict throwing, but cursor is not drained on success
- **Scenario:** The SUCCESS branch of the second insert (`cooldown-store.js:45-49`) does not call `.toArray()` on the cursor (compare to the UPDATE branch L34 which *does* drain). On CF DO SQL, statement effects are committed once the cursor is materialized. If the engine deferred the commit until cursor drain and a subsequent `Math.random()` GC sweep fires (L51) and a *second* `tryAcquire` runs concurrently (impossible in a single DO, but possible across DO replays / fast retry) … this is a code-smell, not provably wrong.
- **Trigger:** API-level retry at the millisecond boundary.
- **Observable:** None reproducible from static reading. Drain-symmetry is the safe default.
- **Evidence:** `src/durable-objects/lib/cooldown-store.js:34` (drain) vs `:45-49` (no drain).
- **Severity:** Medium. Flag for fix; low real-world impact since DOs are single-threaded per name.
#### M2. `writePixels` aliasing comment is misleading; `Uint8Array(buf)` does *not* always copy
- **Scenario:** `readChunk` returns either a fresh zero-fill (no row) or wraps the SQL-returned BLOB with `new Uint8Array(blob)` if it's not already a Uint8Array. If the BLOB *is* already Uint8Array, `readChunk` returns it directly (L37-39). Then `writePixels` does `const next = new Uint8Array(buf)`. Per spec, `new Uint8Array(typedArray)` *copies*, so this is fine — but the comment at L91 says "must not alias persisted state, so copy to be safe", suggesting uncertainty.
- **Trigger:** Future refactor that uses `new Uint8Array(arrayBuffer)` instead of `new Uint8Array(typedArray)` would silently introduce aliasing (the latter constructs a view, not a copy, when given an ArrayBuffer).
- **Observable:** would corrupt the SQLite-cached BLOB and produce inconsistent reads.
- **Evidence:** `chunk-storage.js:37-39` (return path), `:92` (consumer).
- **Severity:** Medium (latent footgun, not an active bug).
#### M3. Missing duplicate-coord dedup in batch → user can inflate batch size with redundant pixels
- **Scenario:** Client submits 2048 pixels all at `(0,0)` with random colors. Validation (`worker.js:43-52`) accepts. `writePixels` groups by chunk, so all 2048 hit chunk 0; then iterates `edits` writing 2048 times to `next[0]`. Result: only the last write survives (no functional bug), but you've burned the user's full batch quota on 1 effective pixel.
- **Trigger:** buggy client, or an attacker trying to hide intent in a noisy batch.
- **Observable:** user pays 1 sec cooldown for what looks like 2048 pixels but is 1.
- **Evidence:** `worker.js:36-52` (no Set dedup), `chunk-storage.js:72-80` (last-write-wins per byte).
- **Severity:** Medium UX nuance; the importer's `pixel-buffer.js:23-30` actually *does* dedup client-side, so well-behaved clients are unaffected.
#### M4. Image-importer "progressive skip" reads stale canvas state via `shouldSkip`
- **Scenario:** `shouldSkip` is fed by the client's local canvas view, which is updated from WS broadcasts and the initial fetch. Combined with H4 (10s edge cache), the importer can skip pixels that aren't truly placed yet, leaving holes in uploaded images.
- **Trigger:** Concurrent edits + cached canvas view.
- **Observable:** importer reports "Skipped N already-matching pixels" but the pixels weren't actually placed.
- **Evidence:** `image-uploader.js:64-76`. No server-authoritative readback.
- **Severity:** Medium. Self-inflicted, recoverable by re-running the importer.
#### M5. SQLite per-row BLOB size — DO SQLite cell size limit
- **Scenario:** CF DO SQLite has a per-cell limit (commonly 2 MB hard, often documented around ~2 MB). Each chunk is 64 KB — well under. ✅ Not a bug today. Becomes a problem if `CHUNK_BYTES` is bumped above the cell limit during a "resize redeploy".
- **Trigger:** Future redeploy with `CHUNK_BYTES > 2 MB`.
- **Observable:** writes throw at runtime; canvas frozen.
- **Evidence:** `constants.js:18` — no assertion that `CHUNK_BYTES` ≤ documented cell-size cap.
- **Severity:** Medium (latent; add a static assertion).
#### M6. Per-DO storage cap (CF DO SQLite ~10 GB / instance)
- **Scenario:** Cooldown table grows to ~`active-users` rows; canvas chunks at 256 × 64 KB = 16 MB. Plenty of headroom *unless* users explode (millions of unique IPs/day) and GC at 1% sample rate fails to keep up.
- **Math:** at 1% GC sample rate per `tryAcquire`, expected GC runs/sec = 0.01 × QPS. At low QPS (1-10), GC may run < once/min. Each row ~50-100 bytes; 10 GB cap = ~150 M rows. Not realistic on rPlace traffic, but on a viral spike, possible.
- **Evidence:** `cooldown-store.js:7` (`GC_SAMPLE_RATE = 0.01`), `:36-37, :50-51` (probabilistic).
- **Severity:** Medium (capacity, not correctness).
#### M7. `webSocketClose` re-closes already-closed socket
- **Scenario:** Hibernation API delivers `close` events for *all* terminations including ones the server initiated. The handler at `canvas-room.js:115-121` calls `ws.close(code, reason)` again, on a socket that's already closed.
- **Trigger:** any close event on hibernation-mode WS.
- **Observable:** likely silent (ws.close on closed = no-op or throws caught upstream). Comment says "Required pre-2026-04-07 compat" which today's compat date is `2025-04-01` — so this code path *is* exercised. Confirmed no try/catch around it.
- **Evidence:** `canvas-room.js:120` and `wrangler.json:4` (`compatibility_date: "2025-04-01"`).
- **Severity:** Medium. Add try/catch defensively.
---
### LOW
#### L1. Chunk-boundary math is correct for current dims, but no off-by-one guard if `TOTAL_PIXELS % CHUNK_BYTES != 0`
- **Math:** `4096 × 4096 = 16_777_216`; `16_777_216 / 65536 = 256` exactly. So `chunkSize(255) = min(65536, 16_777_216 - 255*65536) = 65536`. ✅ Today.
- **Risk:** if dims become non-multiples (e.g. `CANVAS_WIDTH = 4097`), `TOTAL_PIXELS = 16_785_409`, `CHUNK_COUNT = ceil(16_785_409 / 65536) = 257`. `chunkSize(256) = 16_785_409 - 256*65536 = 8193`. `readChunk` returns a `Uint8Array(8193)` for the missing row, but `readAllChunks` does `out.set(view, chunkId * CHUNK_BYTES)` where `out` is sized `TOTAL_PIXELS` — write at offset `256*65536 = 16_777_216` of length `8193` ends at `16_785_409` ✅.
- **Where it breaks:** `pixelToChunk` returns `byteOffset = offset % CHUNK_BYTES` — for the last chunk this is fine because writes are bounded by valid (x,y). But if a stored row's BLOB is larger than `chunkSize(chunkId)` (e.g. legacy rows after a *shrink*), `out.set` would overrun. No length-check on the read path.
- **Evidence:** `chunk-storage.js:46-56` — no `subarray(0, chunkSize(chunkId))` clamp.
- **Severity:** Low (current dims safe; flag if shrinking).
#### L2. Validator double-work: edge validates, then DO re-validates
- Edge in `worker.js:36-52`, DO in `canvas-room.js:51-70`. Defensive, not a bug, but increases JSON parse cost twice for every batch.
- **Severity:** Low (perf; acceptable defense-in-depth).
#### L3. `Number.isInteger(p?.x)` accepts `-0` and the same coord as `0`
- Per ES spec `Number.isInteger(-0) === true`, and `-0 < 0 === false`, so `-0` passes through. Harmless (writes byte 0 of chunk 0), but worth noting if anyone ever uses x as a JSON Map key.
- **Severity:** Low.
#### L4. `MAX_BODY_BYTES = MAX_BATCH_SIZE * 64` is a heuristic; large palette indices aren't longer
- 64 bytes/pixel is generous (`{"x":4095,"y":4095,"color":255}` is ~32 chars). Comment says "~64 bytes is generous" — fine. But malicious whitespace-padded JSON (`" x ": 0`) can blow past this *length* before parsing. `c.req.header('content-length')` is client-supplied and may lie.
- **Trigger:** crafted body with bogus `Content-Length`. Hono / `c.req.json()` will read the actual body; if it's larger than declared, behavior depends on the Hono runtime (usually reads what's there). Not a memory exhaustion attack on Workers (req body capped at 100 MB), but it bypasses the early reject.
- **Severity:** Low.
#### L5. WS broadcast: `JSON.stringify` once, send to all — but no backpressure detection
- `canvas-room.js:97-105`: `ws.send(message)` in a loop. No queue length check. If a client is slow / hibernated incorrectly / on flaky cell, sends pile up. CF docs suggest checking `getReadyState` or buffered amount; here we just rely on the catch.
- **Severity:** Low (CF runtime likely drops or queues internally).
#### L6. No cap on number of WebSocket clients per DO
- Combined with H5 (no Origin check), a malicious party can open 10K hibernation sockets cheaply. Each pixel broadcast iterates all of them via `getWebSockets()` — O(N) per place.
- **Severity:** Low standalone, High when combined with H5.
#### L7. Test coverage gaps
- No tests for the DO itself (storage, cooldown, broadcast, hibernation).
- No tests for `chunk-storage.writePixels` (atomicity, multi-chunk batches, boundaries).
- No tests for `cooldown-store.tryAcquire` (race, GC, expiry).
- No tests for `getUserId` (header presence/absence, hashing).
- No integration / WS reconnect tests.
- The single test file (`worker-validation.test.js`) only exercises edge JSON validation — exactly what TypeScript types would catch.
- **Severity:** Low (process), High in aggregate (quality risk).
---
## Test file gaps (worker-validation.test.js)
- Mocks the DO as an empty class; never tests forwarding behavior under failure (DO 5xx, network error from the DO stub).
- No assertion that `getUserId` is called and forwarded in the body.
- No test for `MAX_BODY_BYTES` early reject (`content-length > MAX_BODY_BYTES`).
- No test that the DO response is passed through verbatim — currently only the body is checked, not headers, not status text.
- No test that `pixels` non-object element (e.g. `[null, 1, "x"]`) is rejected with `invalid_pixel`. (Code uses `p?.x` so null/non-object yields `undefined`, fails `Number.isInteger`, returns 400. Good — but untested.)
- No `/api/canvas` test at all.
- Boundary test (L137) only tests upper bound, not `(0, 0, 0)`.
---
## Cross-cutting observations
1. **Cooldown identity is the weakest link.** IP-based hashing collides under NAT (C3) and is shared in dev (H2). Combined with H1 (no daily cap), one IP can either lock out a building or repaint the whole canvas — both bad outcomes from the same input.
2. **The atomicity story is broken.** `writePixels` comment claims atomicity from "no `await`" (chunk-storage.js:86-87) but each `sql.exec` is auto-commit. Wrap the loop in `state.storage.transactionSync(() => { ... })` to actually deliver on the comment. C1+C2 both go away.
3. **Edge cache and WS race (H3+H4) is the canonical r/place clone bug.** Mitigation: serve canvas via the WS first message (snapshot frame) instead of via cached HTTP, *or* attach a `version` (monotonic counter) to broadcast frames and let the client refetch when it detects a gap.
4. **Hibernation API is used correctly.** `acceptWebSocket` (canvas-room.js:92) and `getWebSockets` (`:98`) are right. The `webSocketClose` re-close (M7) is the only smell; everything else aligns with CF docs.
5. **SQLite usage is conservative.** 64 KB BLOBs, single-PK rows, sparse rows. No schema landmines today. Future-proofing: assert `CHUNK_BYTES <= 2*1024*1024` in init.
---
## Suggested priority fixes (not implementing — read-only audit)
1. Wrap `writePixels` in `state.storage.transactionSync` (fixes C2, partially C1).
2. Move `tryAcquire` *after* `writePixels` succeeds, OR add a "release" path on storage failure (fixes C1).
3. Add `Origin` allowlist + max-clients cap on `#handleWsUpgrade` (fixes H5/L6).
4. Drop `s-maxage` to 1-2s (or remove edge cache and serve from DO live), and emit a sequence number on every broadcast frame to detect gaps (fixes H4 and partially H3).
5. Add per-IP daily quota in addition to 1Hz cooldown (fixes H1).
6. Augment `getUserId` with a stable client-side cookie (signed) to break NAT collisions (fixes C3).
7. Add static assertion `CHUNK_BYTES <= 2_000_000` (M5).
8. Add try/catch around `webSocketClose`'s re-close (M7).
9. Drain INSERT cursor symmetrically in `tryAcquire` (M1).
10. Add DO unit tests covering the gaps in L7.
---
## Status: DONE_WITH_CONCERNS
**Summary:** 3 critical, 5 high, 7 medium, 7 low findings across cooldown identity (IP/NAT), atomicity claims that don't match implementation, WS+cache race window, and image-importer DoS surface. Test coverage is limited to edge-validation; DO logic is untested.
**Concerns:**
- The "no await = atomic" comment in `chunk-storage.js:86-87` is wrong; needs `transactionSync` for true atomicity.
- IP-only identity is both a usability bug (NAT) and an abuse vector (no daily cap). These are the same fix from different angles.
- WS broadcast lacks an Origin gate — any site can hold sockets to the canvas DO.
**Unresolved questions:**
1. What is the documented per-cell BLOB limit on CF DO SQLite as of compat date 2025-04-01? (Affects M5 severity if user later resizes chunks.)
2. Is `state.storage.transactionSync` available on the Workers runtime version pinned by `compatibility_date: 2025-04-01`? If not, the atomicity fix needs `transaction(async () => {...})` instead.
3. Is there a planned per-room model (so `idFromName('main')` becomes one of many)? Per-room would naturally lift IP collision pain *if* combined with an account/cookie identity. Without that, sharding doesn't help.
4. Is there a CDN-layer mitigation for H4 (e.g. Cache API key on a freshness query string set by the DO)? The current `Cache-Control` headers will be honored by CF colos; product needs to decide live-fresh vs. cheap.
5. What's the operational policy on importer abuse? A 7M pixel/hr/IP cap is the canvas-rewrite budget; this is a product decision, not just engineering.
plans/reports/docs-manager-260510-0211-rplace-docs-drift.md
+228
View File
@@ -0,0 +1,228 @@
# rplace Documentation Drift Audit
**Date:** 2026-05-10
**Scope:** Verify docs alignment with recent Upstash → DO migration (commits a977adc through c3f7c02)
**Methodology:** Cross-check README, docs/, package.json, wrangler.json, .env.example, and active plan against actual src/ tree
---
## Summary
**Migration Status:** Phases 1–4 complete (files deleted, dependencies removed). Phase 5 partial (deploy done, 7-day observation window).
**Drift Found:** 4 stale/misleading items in README.md and .env.example. Docs in `docs/` directory are **accurate and current**. Migration plan ready for archival.
---
## Drift Findings
### STALE — README.md Project Structure (Lines 79–111)
**Severity:** Stale (factually wrong)
**Current State:**
```md
src/
├── worker.js # ✓ exists
├── admin/
│ └── migrate-from-upstash.js # ✗ DOES NOT EXIST (deleted in Phase 4)
├── durable-objects/ # ✓ exists
│ ├── canvas-room.js # ✓ exists
│ └── lib/
│ ├── schema.js # ✓ exists
│ ├── chunk-storage.js # ✓ exists
│ └── cooldown-store.js # ✓ exists
├── lib/
│ ├── constants.js # ✓ exists
│ ├── canvas-decoder.js # ✓ exists
│ ├── canvas-storage.js # ✗ DOES NOT EXIST (deleted in Phase 4)
│ ├── redis-client.js # ✗ DOES NOT EXIST (deleted in Phase 4)
│ ├── rate-limiter.js # ✗ DOES NOT EXIST (deleted in Phase 4)
│ ├── image-uploader.js # ✓ exists
│ └── get-user-id.js # ✓ exists
```
**Proposed Fix:**
Remove the entire `src/admin/` block and the three legacy lib files from the tree display:
```markdown
## Project Structure
```
src/
├── worker.js # Hono entry — thin proxy + edge validation
├── durable-objects/
│ ├── canvas-room.js # DO: storage + cooldown + WS hub
│ └── lib/
│ ├── schema.js # Idempotent CREATE TABLE
│ ├── chunk-storage.js # BLOB chunk read/write
│ └── cooldown-store.js # Rate-limit acquire + lazy GC
├── lib/
│ ├── constants.js # CANVAS_WIDTH/HEIGHT, CHUNK_BYTES, palette
│ ├── canvas-decoder.js # Raw bytes → RGBA (client-side)
│ ├── image-uploader.js # Browser-side batched uploader
│ ├── get-user-id.js # IP-based identity
│ ├── dither-kernels.js # Dithering algorithms
│ ├── image-color-correction.js # Color-space transform
│ ├── image-to-palette.js # Quantization
│ ├── image-transform.js # Scaling + rotation
│ ├── image-resize.js # Image dimensions
│ ├── image-pipeline.js # Multi-step image processing
│ ├── image-pipeline-client.js # Client-side queue
│ ├── image-pipeline-worker.js # Worker-side handler
│ ├── image-job-storage.js # Job persistence
│ └── pixel-buffer.js # Batch accumulator
├── client/
│ ├── main.js # Svelte mount
│ ├── App.svelte # Root + WebSocket
│ ├── app.css # Global styles
│ └── components/
│ ├── CanvasRenderer.svelte # Canvas + zoom/pan + touch
│ ├── ColorPicker.svelte # Favorites + 256-color grid
│ ├── CanvasControls.svelte # Zoom buttons + coordinates
│ ├── DrawToolbar.svelte # Paint / submit / undo / redo
│ └── ImageImporter.svelte # Image-to-canvas uploader
└── index.html # Vite entry
```
```
**Reason:** Upstash files deleted in commit a977adc. Showing orphaned files confuses developers and suggests the migration is incomplete. Current tree is incomplete (missing image pipeline files); use actual tree from bash scan.
---
### MISLEADING — README.md API Section (Lines 146–151)
**Severity:** Misleading (technically exists but endpoint removed)
**Current Text:**
```markdown
### `POST /admin/migrate-from-upstash` (transitional)
Token-gated one-shot endpoint that pulls the canvas from a legacy Upstash
Redis instance and imports it into the Durable Object. Slated for removal
after the production migration completes (Phase 4 of
[`plans/260509-2309-canvas-on-do-storage`](plans/260509-2309-canvas-on-do-storage)).
```
**Proposed Fix:**
Delete this section entirely. The endpoint was removed in commit a977adc (Phase 4 cleanup). No need to document historical endpoints.
**Reason:** Endpoint no longer exists in worker.js. Documenting it as "slated for removal" when it's already removed is confusing. Developers might spend time looking for it.
---
### STALE — .env.example
**Severity:** Stale (now incorrect for setup)
**Current Content:**
```
UPSTASH_REDIS_REST_URL=
UPSTASH_REDIS_REST_TOKEN=
```
**Proposed Fix:**
Delete file entirely or replace with a comment explaining that no external environment variables are required:
**Option A (Delete):** Remove `.env.example` — the repo has no external secrets now.
**Option B (Keep as placeholder):**
```
# No external secrets required.
# Canvas + cooldown state live inside CanvasRoom Durable Object (SQLite).
# All configuration is in src/lib/constants.js.
```
**Reason:** Current file references Upstash credentials that are no longer needed. New developers will be confused by empty placeholders for deleted services. Phase 4 success criteria explicitly calls for clean `.env.example`.
---
### ACCURATE — docs/ Directory
All docs files checked and found **accurate**:
- **canvas-resize-procedure.md** — correctly describes lazy-init, CHUNK_COUNT derivation, DO storage caps. No Upstash references.
- **deployment-guide.md** — migration section accurately marked `(Optional) One-Shot Migration from Upstash` with clear date context. No Upstash in main deploy flow.
- **system-architecture.md** — correctly describes CanvasRoom DO, SQLite schema, no Upstash. Migration endpoint marked "transitional" and "Removed in Phase 4".
- **code-standards.md** — does not reference Upstash or legacy code. Reflects current arch.
- **references.md** — informational only, no implementation details to drift.
---
## Cross-Check Results
### File Existence Verification
| File Reference | Status | Location |
|---|---|---|
| `src/worker.js` | ✓ Exists | Confirmed, 75 lines |
| `src/durable-objects/canvas-room.js` | ✓ Exists | Confirmed |
| `src/durable-objects/lib/schema.js` | ✓ Exists | Confirmed |
| `src/durable-objects/lib/chunk-storage.js` | ✓ Exists | Confirmed |
| `src/durable-objects/lib/cooldown-store.js` | ✓ Exists | Confirmed |
| `src/lib/constants.js` | ✓ Exists | Confirmed |
| `src/lib/canvas-decoder.js` | ✓ Exists | Confirmed |
| `src/lib/image-uploader.js` | ✓ Exists | Confirmed |
| `src/lib/get-user-id.js` | ✓ Exists | Confirmed |
| `src/admin/migrate-from-upstash.js` | ✗ Deleted | Removed in Phase 4 (a977adc) |
| `src/lib/canvas-storage.js` | ✗ Deleted | Removed in Phase 4 (a977adc) |
| `src/lib/redis-client.js` | ✗ Deleted | Removed in Phase 4 (a977adc) |
| `src/lib/rate-limiter.js` | ✗ Deleted | Removed in Phase 4 (a977adc) |
### Dependencies Verification
| Package | Current | Status |
|---|---|---|
| `@upstash/redis` | Not in package.json | ✓ Removed |
| `ioredis` | Not in package.json | ✓ Removed |
| `hono` | ^4.7.6 | ✓ Present, correct |
| `svelte` | ^5.28.2 | ✓ Present, correct |
### Configuration Verification
| Config Item | File | Status |
|---|---|---|
| DO binding name `CANVAS_ROOM` | wrangler.json line 11 | ✓ Matches docs reference |
| DO class `CanvasRoom` | wrangler.json line 12 | ✓ Matches src/durable-objects/canvas-room.js |
| SQLite migration tag `v1` | wrangler.json line 18 | ✓ Registered for CanvasRoom |
---
## Docs Directory Size Assessment
| File | Lines | Status |
|---|---|---|
| canvas-resize-procedure.md | 57 | ✓ Under 800 LOC limit |
| deployment-guide.md | 115 | ✓ Under 800 LOC limit |
| system-architecture.md | 165 | ✓ Under 800 LOC limit |
| code-standards.md | 51 | ✓ Under 800 LOC limit |
| references.md | 21 | ✓ Under 800 LOC limit |
---
## Plan Archive Status
`plans/260509-2309-canvas-on-do-storage/plan.md` marked `status: in-progress` but phases 1–4 complete and deployed to production.
**Proposed Action:** Update plan.md line 3 to `status: completed` (observing 7-day rollback window before archival).
---
## Unresolved Questions
1. **Image pipeline files** — README was outdated before audit (missing `image-pipeline.js`, `image-pipeline-client.js`, etc.). Was the tree intentionally simplified, or is it an ongoing drift issue unrelated to migration?
2. **.env.example strategy** — Are empty placeholder secrets (Option B) preferable to deletion (Option A) for discoverability?
---
## Recommended Actions (Priority Order)
1. **README.md line 85–97** — Remove `src/admin/` and three legacy lib files from project structure tree.
2. **README.md line 146–151** — Delete `/admin/migrate-from-upstash` API section.
3. **.env.example** — Delete or replace with placeholder comment.
4. **plans/260509-2309-canvas-on-do-storage/plan.md line 3** — Change `status: in-progress` → `status: completed` after 7-day window (ca. May 17, 2026).
---
**Status:** DONE
**Summary:** 4 stale refs flagged (README tree + API section, .env.example, plan status). Docs directory accurate. No broken links or config mismatches. Ready for targeted edits.
src/durable-objects/canvas-room.js
+17 -5
View File
@@ -1,7 +1,7 @@
import { CANVAS_WIDTH, CANVAS_HEIGHT, MAX_COLORS, MAX_BATCH_SIZE } from '../lib/constants.js';
import { init as initSchema } from './lib/schema.js';
import { readAllChunks, writePixels } from './lib/chunk-storage.js';
import { tryAcquire } from './lib/cooldown-store.js';
import { tryAcquire, release } from './lib/cooldown-store.js';
/**
* CanvasRoom: single Durable Object that owns
@@ -76,10 +76,20 @@ export class CanvasRoom {
}
try {
writePixels(this.sql, pixels);
// transactionSync makes the multi-chunk batch all-or-nothing. Without
// it, each sql.exec auto-commits and a partial failure leaves the
// canvas in a half-written state.
this.state.storage.transactionSync(() => {
writePixels(this.sql, pixels);
});
} catch (err) {
console.error('writePixels failed:', err);
return Response.json({ error: 'storage_failed', message: String(err) }, { status: 500 });
// Refund the cooldown so a transient storage error doesn't lock the
// user out for 1s (and the image-uploader doesn't lose throughput).
try { release(this.sql, userId); } catch (releaseErr) {
console.warn('cooldown release failed:', releaseErr?.message || releaseErr);
}
return Response.json({ error: 'storage_failed' }, { status: 500 });
}
this.#broadcastPixels(pixels);
@@ -116,8 +126,10 @@ export class CanvasRoom {
if (!wasClean) {
console.warn(`WS unclean close: code=${code} reason=${reason || '<none>'}`);
}
// Required pre-2026-04-07 compat date; harmless after.
ws.close(code, reason);
// Required because compatibility_date 2025-04-01 predates the
// 2026-04-07 default-close cutoff. Remove if/when wrangler.json
// bumps past that date. The try/catch handles already-closed sockets.
try { ws.close(code, reason); } catch { /* already closed */ }
}
webSocketError(ws, error) {
src/durable-objects/lib/chunk-storage.js
+23 -14
View File
@@ -1,4 +1,4 @@
import { CANVAS_WIDTH, TOTAL_PIXELS, CHUNK_BYTES } from '../../lib/constants.js';
import { CANVAS_WIDTH, TOTAL_PIXELS, CHUNK_BYTES, CHUNK_COUNT } from '../../lib/constants.js';
/**
* Canvas pixel storage as fixed-size BLOB chunks in DO SQLite.
@@ -42,23 +42,36 @@ export function readChunk(sql, chunkId) {
/**
* Read all chunks concatenated into a single TOTAL_PIXELS-sized buffer.
* Used to serve GET /api/canvas.
*
* Bounded by chunk_id < CHUNK_COUNT so orphan rows left after a canvas-shrink
* don't trip a RangeError on out.set(...).
*/
export function readAllChunks(sql) {
const out = new Uint8Array(TOTAL_PIXELS);
const cursor = sql.exec('SELECT chunk_id, bytes FROM canvas_chunks');
const cursor = sql.exec(
'SELECT chunk_id, bytes FROM canvas_chunks WHERE chunk_id < ?',
CHUNK_COUNT,
);
for (const row of cursor) {
const chunkId = row.chunk_id;
const blob = row.bytes;
const view = blob instanceof Uint8Array ? blob : new Uint8Array(blob);
out.set(view, chunkId * CHUNK_BYTES);
// Defensive clamp: if a row's persisted blob is longer than this chunk's
// expected size (e.g. legacy data), trim before copying.
const expected = chunkSize(chunkId);
const trimmed = view.length > expected ? view.subarray(0, expected) : view;
out.set(trimmed, chunkId * CHUNK_BYTES);
}
return out;
}
/**
* Apply a batch of pixel writes. Groups by chunk so each touched chunk
* incurs at most one read and one write. Single transaction so the batch
* is atomic across all touched chunks.
* incurs at most one read and one write.
*
* Atomicity: each sql.exec auto-commits, so this function is NOT atomic on
* its own. The caller must wrap the call in state.storage.transactionSync
* (or transaction) to make a multi-chunk batch all-or-nothing.
*
* @param {SqlStorage} sql
* @param {Array<{x:number, y:number, color:number}>} pixels
@@ -79,17 +92,13 @@ export function writePixels(sql, pixels) {
bucket.push({ byteOffset, color: p.color });
}
// For each touched chunk: read current bytes (or zero-fill), apply edits,
// write back. INSERT OR REPLACE upserts the row.
// Atomicity: this loop is fully synchronous (no `await`) and the DO is
// single-threaded, so the chunk updates are atomic with respect to other
// requests. If anyone adds an `await` inside this loop, wrap the whole
// block in `state.storage.transactionSync(() => { ... })` to preserve it.
for (const [chunkId, edits] of groups) {
const buf = readChunk(sql, chunkId);
// readChunk returns a fresh Uint8Array (or wraps a buffer); writes must
// not alias persisted state, so copy to be safe.
const next = new Uint8Array(buf);
// Allocate against chunkSize, never the persisted blob's length: after a
// canvas grow, the old short blob would silently drop OOB writes.
const expected = chunkSize(chunkId);
const next = new Uint8Array(expected);
next.set(buf.subarray(0, Math.min(buf.length, expected)));
for (const { byteOffset, color } of edits) {
next[byteOffset] = color;
}
src/durable-objects/lib/cooldown-store.js
+11 -1
View File
@@ -42,11 +42,13 @@ export function tryAcquire(sql, userId, now = Date.now()) {
// No expired row to update. Either the user has never been seen (insert
// succeeds) or they hold an active claim (insert fails on PK conflict).
try {
sql.exec(
const insertCursor = sql.exec(
'INSERT INTO cooldowns (user_id, expires_at) VALUES (?, ?)',
userId,
expiresAt,
);
// Drain symmetrically with the UPDATE branch so statement effects commit.
insertCursor.toArray();
if (Math.random() < GC_SAMPLE_RATE) {
try { gc(sql, now); } catch { /* GC is best-effort */ }
}
@@ -56,6 +58,14 @@ export function tryAcquire(sql, userId, now = Date.now()) {
}
}
/**
* Refund a cooldown row so a transient write failure doesn't soft-DOS the
* user for 1s. Called from the catch path in placePixels.
*/
export function release(sql, userId) {
sql.exec('DELETE FROM cooldowns WHERE user_id = ?', userId).toArray();
}
/** Delete all expired cooldown rows. Cheap with the expires_at index. */
export function gc(sql, now = Date.now()) {
sql.exec('DELETE FROM cooldowns WHERE expires_at <= ?', now).toArray();
src/lib/constants.js
+6
View File
@@ -18,6 +18,12 @@ export const MAX_BATCH_SIZE = 2048;
export const CHUNK_BYTES = 65536;
export const CHUNK_COUNT = Math.ceil(TOTAL_PIXELS / CHUNK_BYTES);
// CF DO SQLite has a per-cell BLOB cap (~2 MB at compat date 2025-04-01).
// Fail-fast at module load if a future bump would overflow.
if (CHUNK_BYTES > 2_000_000) {
throw new Error(`CHUNK_BYTES (${CHUNK_BYTES}) exceeds DO SQLite per-cell BLOB cap (~2 MB)`);
}
/**
* Build the 256-color palette deterministically:
src/worker.js
+7 -1
View File
@@ -21,7 +21,13 @@ app.get('/api/canvas', async (c) => {
/** POST /api/place — validate at the edge, forward to the DO. */
app.post('/api/place', async (c) => {
const contentLength = parseInt(c.req.header('content-length') || '0', 10);
// Require a positive Content-Length. Missing or zero would otherwise let a
// chunked-transfer-encoded body bypass the MAX_BODY_BYTES pre-parse cap.
const contentLengthRaw = c.req.header('content-length');
const contentLength = parseInt(contentLengthRaw ?? '', 10);
if (!Number.isFinite(contentLength) || contentLength <= 0) {
return c.json({ error: 'content_length_required' }, 411);
}
if (contentLength > MAX_BODY_BYTES) {
return c.json({ error: 'body_too_large', max: MAX_BODY_BYTES }, 413);
}
test/worker-validation.test.js
+42 -3
View File
@@ -10,12 +10,18 @@ vi.mock('../src/durable-objects/canvas-room.js', () => ({
import app from '../src/worker.js';
/** Helper to create POST request */
/** Helper to create POST request. Computes Content-Length explicitly because
* the synthetic Request constructor in this environment doesn't auto-set it. */
function postPlace(body) {
const bodyStr = typeof body === 'string' ? body : JSON.stringify(body);
const bodyBytes = new TextEncoder().encode(bodyStr).byteLength;
return new Request('http://localhost/api/place', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: typeof body === 'string' ? body : JSON.stringify(body),
headers: {
'Content-Type': 'application/json',
'Content-Length': String(bodyBytes),
},
body: bodyStr,
});
}
@@ -35,6 +41,7 @@ describe('POST /api/place validation', () => {
it('rejects invalid JSON', async () => {
const req = new Request('http://localhost/api/place', {
method: 'POST',
headers: { 'Content-Length': '8' },
body: 'not json',
});
const res = await app.fetch(req, env);
@@ -43,6 +50,38 @@ describe('POST /api/place validation', () => {
expect(data.error).toBe('invalid_json');
});
it('rejects POST without Content-Length', async () => {
const req = new Request('http://localhost/api/place', {
method: 'POST',
body: '{"pixels":[]}',
});
const res = await app.fetch(req, env);
expect(res.status).toBe(411);
expect((await res.json()).error).toBe('content_length_required');
});
it('rejects POST with zero Content-Length', async () => {
const req = new Request('http://localhost/api/place', {
method: 'POST',
headers: { 'Content-Length': '0' },
body: '',
});
const res = await app.fetch(req, env);
expect(res.status).toBe(411);
expect((await res.json()).error).toBe('content_length_required');
});
it('rejects POST with Content-Length above the cap', async () => {
const req = new Request('http://localhost/api/place', {
method: 'POST',
headers: { 'Content-Length': String(MAX_BATCH_SIZE * 64 + 1) },
body: '{}',
});
const res = await app.fetch(req, env);
expect(res.status).toBe(413);
expect((await res.json()).error).toBe('body_too_large');
});
it('rejects missing pixels array', async () => {
const res = await app.fetch(postPlace({}), env);
expect(res.status).toBe(400);