Skip to content

fix: remove If-Match header and stop clearing storage on 412#139

Closed
kptdobe wants to merge 1 commit into
mainfrom
fix/no-ifmatch-412
Closed

fix: remove If-Match header and stop clearing storage on 412#139
kptdobe wants to merge 1 commit into
mainfrom
fix/no-ifmatch-412

Conversation

@kptdobe

@kptdobe kptdobe commented May 8, 2026

Copy link
Copy Markdown
Contributor

Problem

persistence.put sent If-Match: * on every PUT to da-admin. This header tells da-admin "only apply this write if the resource already exists." When the document didn't exist yet (new document, DO cold-start before the first successful write), da-admin returned 412 Precondition Failed.

The 412 handler then:

  1. Called ydoc.storage.deleteAll() — destroying the entire Yjs state, including images not yet flushed to da-admin
  2. Closed all WebSocket connections — forcing all clients to reconnect and reload from a now-empty da-admin

This was the chain that caused inserted images to permanently disappear.

Root cause

If-Match: * has no useful purpose in the collab context:

  • Collab is the authoritative writer for a document once a session is open
  • Whether the resource already exists in da-admin or not, the PUT should always proceed
  • The syncadmin/deleteadmin APIs are the correct mechanism for coordinating external changes

Solution

  • Remove If-Match: * from persistence.put — da-admin now always accepts PUT unconditionally
  • Remove the 412 special-case handler — a 412 (or any other non-2xx) is now treated like any other PUT failure: log the error, surface it via the ydoc error map, and return the current content so the debounced save can retry on the next edit
  • Only 401/403 still trigger session termination (genuine auth failures)

Tests

  • Two new tests assert the correct behavior: If-Match header is absent on PUT; 412 does NOT clear storage or close connections
  • Test persistence update shows error but preserves state on 412 replaces the old "closes all and cleans storage" test
  • Four existing put tests updated: If-Match: * assertions replaced with null assertions

🤖 Generated with Claude Code

@bosschaert

Copy link
Copy Markdown
Contributor

I think the If-Match header was added to fix a specific issue: b33ae65

@karlpauls could you please review?

The If-Match: * header on PUT requests caused da-admin to return 412
whenever the document did not yet exist (new document, or first write
after DO cold-start). The 412 handler then called storage.deleteAll()
and closed all WebSocket connections, destroying the entire Yjs state
including images that had not yet been flushed to da-admin.

Remove If-Match: * entirely — da-admin should always accept a PUT from
collab whether the resource exists or not. On any non-2xx response, show
an error on the ydoc but do not touch storage or close connections;
only 401/403 still trigger session termination.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kptdobe
kptdobe force-pushed the fix/no-ifmatch-412 branch from e6e9fe6 to c728a3b Compare May 11, 2026 07:55
@kptdobe

kptdobe commented May 11, 2026

Copy link
Copy Markdown
Contributor Author

Closing — root cause is in da-admin, not da-collab

After deeper analysis, the root cause of the image loss is a bug in da-admin's If-Match: * handling, not in da-collab's use of the header. Removing If-Match: * from da-collab would regress the intentional no-implicit-creation capability introduced by #105.

What actually happens

When da-collab sends PUT with If-Match: *, da-admin correctly validates that the document exists (returning 412 only when current.status === 404). However, it then uses the document's current ETag for the actual R2 write (If-Match: <etag>). If a concurrent write (e.g. during a DO live migration) changes the ETag between the existence check and the write, R2 rejects the PUT with 412.

At that point da-admin has two paths:

  • No client conditional → retry transparently (fetches fresh ETag and tries again)
  • Client sent If-Match → return 412 immediately without retry

The second path conflates If-Match: * (wildcard — "document must exist") with If-Match: "<specific-etag>" (version lock — "document must have this exact version"). A wildcard should behave like the no-conditional path on ETag mismatch because the document still exists; only a specific ETag warrants a non-retriable 412.

The fix

da-admin PR to follow — putObjectWithVersion retries on R2 ETag mismatch when If-Match: * was sent, exactly as it does for internal (no-conditional) writes. If-Match: "<specific-etag>" still returns 412 without retry.

da-collab: no changes needed.

@kptdobe kptdobe closed this May 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants