Skip to content

fix(docroom): guard lastsync writes against DO 128 KiB value cap#174

Merged
kptdobe merged 1 commit into
mainfrom
lastsync
Jun 18, 2026
Merged

fix(docroom): guard lastsync writes against DO 128 KiB value cap#174
kptdobe merged 1 commit into
mainfrom
lastsync

Conversation

@kptdobe

@kptdobe kptdobe commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Durable Object storage rejects values > 131072 bytes with a RangeError. Both lastsync writes in src/shareddoc.jspersistence.update after a successful da-admin PUT, and persistence.bindState after the da-admin restore fetch — used to call storage.put('lastsync', …) unconditionally and surface the platform-deterministic throw as [docroom] Failed to write lastsync … console.error noise (sustained 2 events / 24h on the da-collab error stream, two distinct payload sizes per day).

Introduce a small safePutLastsync(storage, value, docName, context) helper that:

  • Skips the put entirely when value.length > MAX_STORAGE_VALUE_SIZE, logging the skip at console.log level so it stays out of the error channel.
  • Routes any other exception class through logError(...), so the existing demotion path for isExpectedPlatformEvent (DO live migration / worker upgrade events) still applies.

Both call sites delegate to the helper, so any future lastsync write inherits the guard for free.

Why

The 128 KiB cap is platform-deterministic in the same way that the live-migration error is — there is no point landing it in console.error. The pre-existing try/catch comment promised "non-fatal: worst case the restore falls back to da-admin"; in practice that fallback was already taking effect (the marker was skipped after the throw), but the throw was producing log noise on the error channel for every oversize save. Skipping is safe because the absence of the lastsync key is already the documented trigger for the da-admin restore on DO restart.

The pattern matches the demotions shipped in #135 / #155 / #157 for the live-migration / upgrade events.

Test plan

  • Failing-test-first: new mocha cases in test/shareddoc.test.js mock a storage stub that throws the production RangeError shape (Values cannot be larger than 131072 bytes. A value of size <N> was provided.). Both persistence.update and persistence.bindState integration cases for oversize content were verified to fail against an inline-revert of the call-site updates before the guard was applied.
  • After the fix: npx mocha --reporter spec --import=./test/stubs/register.mjs --exit test/*.test.js165 passing, 0 regressions.
  • Direct helper tests cover oversize, in-cap, and no-storage cases.
  • npm run lint — clean.
  • Post-deploy: confirm m.contains('Values cannot be larger than') errors at $d.ScriptName == 'da-collab', l.Level == 'error' return to 0 / 24h × 3 consecutive days.

Scope notes

  • No change in lastsync semantics for ≤ 128 KiB content — the existing "preserve pending Yjs changes across DO evictions" guarantee is preserved (verified by the existing bindState writes lastsync … test, which still passes unchanged).
  • Not addressing whether oversize documents should chunk the marker — current behavior (rely on da-admin restore fallback) is unchanged.

Durable Object storage rejects values > 131072 bytes with a RangeError.
Both lastsync writes — `persistence.update` after a successful da-admin
PUT and `persistence.bindState` after the da-admin restore fetch — used
to call `storage.put('lastsync', …)` unconditionally and surface the
platform-deterministic throw as `[docroom] Failed to write lastsync …`
`console.error` noise.

Introduce a small `safePutLastsync(storage, value, docName, context)`
helper that skips the put when the value exceeds MAX_STORAGE_VALUE_SIZE
(logging the skip at `console.log` level) and routes any other
exception class through `logError(...)` so the existing demotion path
for `isExpectedPlatformEvent` still applies. Both call sites delegate
to the helper, so any future lastsync write picks up the guard.

The "fall back to da-admin on restore" guarantee is preserved because
the absence of the `lastsync` key is already the documented trigger for
the da-admin restore path.

Verification:

- Tests in `test/shareddoc.test.js` mock a storage stub that throws the
  same `RangeError` shape the production logs show. New cases cover the
  helper directly (oversize, in-cap, no-storage) and both call sites at
  the integration level for oversize and in-cap content.
- Integration tests verified failing-first against an inline-revert of
  the call-site updates (both `persistence.update` and `bindState`
  oversize cases fail before the guard is applied).
- `npx mocha --reporter spec --import=./test/stubs/register.mjs --exit
  test/*.test.js`: 165 passing, 0 regressions.
- `npm run lint`: clean.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@kptdobe kptdobe requested a review from bosschaert June 18, 2026 08:57
@kptdobe kptdobe merged commit 0cf15e3 into main Jun 18, 2026
5 checks passed
@kptdobe kptdobe deleted the lastsync branch June 18, 2026 11:47
adobe-bot pushed a commit that referenced this pull request Jun 18, 2026
## [1.7.3](v1.7.2...v1.7.3) (2026-06-18)

### Bug Fixes

* **docroom:** demote 401/403 from da-admin to non-error logs ([#175](#175)) ([780f36d](780f36d))
* **docroom:** guard lastsync writes against DO 128 KiB value cap ([#174](#174)) ([0cf15e3](0cf15e3))
@adobe-bot

Copy link
Copy Markdown
Collaborator

🎉 This PR is included in version 1.7.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants