From 2913b786de2c3d8ab1a213974e17555ec1446513 Mon Sep 17 00:00:00 2001 From: Andrew <6360780@gmail.com> Date: Tue, 11 Nov 2025 21:45:12 -0500 Subject: [PATCH 1/6] Fix Actor configuration override for custom upgrade paths --- packages/core/src/actor-configuration.test.ts | 66 +++++++++++++++++++ packages/core/src/index.ts | 13 ++-- 2 files changed, 74 insertions(+), 5 deletions(-) create mode 100644 packages/core/src/actor-configuration.test.ts diff --git a/packages/core/src/actor-configuration.test.ts b/packages/core/src/actor-configuration.test.ts new file mode 100644 index 0000000..5b10a97 --- /dev/null +++ b/packages/core/src/actor-configuration.test.ts @@ -0,0 +1,66 @@ +import { describe, expect, it, vi } from "vitest"; + +vi.mock("cloudflare:workers", () => { + class DurableObject { + constructor(_state?: unknown, _env?: unknown) {} + } + + class WorkerEntrypoint {} + + return { + DurableObject, + WorkerEntrypoint, + env: {}, + }; +}); + +import { Actor, type ActorConfiguration } from "./index"; + +describe("Actor configuration overrides", () => { + it("respects custom upgrade paths defined by subclasses", async () => { + let upgradeCalls = 0; + + class CustomPathActor extends Actor> { + static override configuration(): ActorConfiguration { + return { + sockets: { + upgradePath: "/custom", + }, + }; + } + + protected override async shouldUpgradeWebSocket( + request: Request, + ): Promise { + return request.headers.get("Upgrade")?.toLowerCase() === "websocket"; + } + + protected override onWebSocketUpgrade(_request: Request): Response { + upgradeCalls += 1; + return new Response("upgraded", { status: 200 }); + } + + protected override onRequest(): Promise { + return Promise.resolve(new Response("fallback", { status: 418 })); + } + } + + const actor = new CustomPathActor(undefined, undefined); + (actor as Record)["_setNameCalled"] = true; + + const upgradeResponse = await actor.fetch( + new Request("https://example.com/custom/game", { + headers: { Upgrade: "websocket" }, + }), + ); + // Node/undici Response objects cannot emit 101, so we just ensure the response we returned flows through. + expect(upgradeResponse.status).toBe(200); + expect(upgradeCalls).toBe(1); + + const fallbackResponse = await actor.fetch( + new Request("https://example.com/ws/game"), + ); + expect(fallbackResponse.status).toBe(418); + expect(upgradeCalls).toBe(1); + }); +}); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 69bec8b..16b7010 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -107,10 +107,11 @@ export abstract class Actor extends DurableObject { /** * Static method to configure the actor. - * @param options - * @returns + * Subclasses can override this to customize upgrade paths or other options. + * @param request - Incoming request (optional for subclasses that need context) + * @returns Actor configuration values */ - static configuration = (request: Request): ActorConfiguration => { + static configuration(request?: Request): ActorConfiguration { return { locationHint: undefined, sockets: { @@ -237,7 +238,8 @@ export abstract class Actor extends DurableObject { async fetch(request: Request): Promise { // If the request route is `/ws` then we should upgrade the connection to a WebSocket // Get configuration from the static property - const config = (this.constructor as typeof Actor).configuration(request); + const ActorClass = this.constructor as typeof Actor; + const config = ActorClass.configuration(request); // Parse the URL to check if the path component matches the upgradePath const url = new URL(request.url); @@ -562,7 +564,8 @@ export function getActor>( ): DurableObjectStub { const className = ActorClass.name; const envObj = env as unknown as Record; - const locationHint = (ActorClass as any).configuration().locationHint; + const actorConfig = (ActorClass as unknown as typeof Actor).configuration(); + const locationHint = actorConfig.locationHint; const bindingName = Object.keys(envObj).find(key => { const binding = (env as any).__DURABLE_OBJECT_BINDINGS?.[key]; From 501e8362d1cf3cec7511337e486f53c267e72c5e Mon Sep 17 00:00:00 2001 From: Andrew <6360780@gmail.com> Date: Wed, 26 Nov 2025 00:50:13 -0500 Subject: [PATCH 2/6] Add regression test for null property auto-vivification in @Persist proxy --- packages/core/src/persist.test.ts | 128 ++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 packages/core/src/persist.test.ts diff --git a/packages/core/src/persist.test.ts b/packages/core/src/persist.test.ts new file mode 100644 index 0000000..f87bc6e --- /dev/null +++ b/packages/core/src/persist.test.ts @@ -0,0 +1,128 @@ +import { describe, expect, it } from "vitest"; + +// Symbol to mark an object as proxied +const IS_PROXIED = Symbol("IS_PROXIED"); + +/** + * Minimal reproduction of the createDeepProxy function from persist.ts + * to test the null auto-vivification bug in isolation. + * + * This replicates the buggy behavior from lines 83-90 of persist.ts + */ +function createBuggyProxy(value: T): T { + const proxy = new Proxy(value as object, { + get(target: object, key: string | symbol): unknown { + if (key === IS_PROXIED) return true; + + if (typeof key === "symbol") { + return Reflect.get(target, key); + } + + const prop = Reflect.get(target, key); + + // BUG: This auto-vivifies null to {} on READ operations + // The proxy MUTATES the underlying object when reading a null property + // This is the exact logic from persist.ts lines 83-90 + if ( + (prop === null || prop === undefined) && + typeof key === "string" && + !key.startsWith("_") && + key !== "length" + ) { + const newObj = {}; + Reflect.set(target, key, newObj); // <-- MUTATES the underlying object! + return newObj; + } + + return prop; + }, + }); + + return proxy as T; +} + +describe("persist proxy - null auto-vivification bug", () => { + it("should NOT mutate null properties to {} when reading them", () => { + // This is the bug: reading a null property through the proxy + // should return null, not auto-vivify it to {} + const original = { + ownerId: null as string | null, + name: "test", + }; + + const proxied = createBuggyProxy(original); + + // Read the null property + const ownerId = proxied.ownerId; + + // BUG: Currently ownerId is {} instead of null + // and original.ownerId has been mutated to {} + expect(ownerId).toBe(null); // FAILS - gets {} + expect(original.ownerId).toBe(null); // FAILS - mutated to {} + }); + + it("should preserve null values in object spread", () => { + const original = { + id: "match_123", + ownerId: null as string | null, + firstMoveMadeAt: null as number | null, + completedAt: null as number | null, + }; + + const proxied = createBuggyProxy(original); + + // Spread the proxied object (this is what our code does) + const copy = { ...proxied }; + + // BUG: Spread triggers get() for each property + // null properties get auto-vivified to {} + expect(copy.ownerId).toBe(null); // FAILS - gets {} + expect(copy.firstMoveMadeAt).toBe(null); // FAILS - gets {} + expect(copy.completedAt).toBe(null); // FAILS - gets {} + + // The original should NOT be mutated by a read operation + expect(original.ownerId).toBe(null); // FAILS - mutated to {} + }); + + it("should NOT mutate the underlying object on property read", () => { + const original = { nullProp: null as null }; + const proxied = createBuggyProxy(original); + + // Just reading should not mutate + void proxied.nullProp; + + // BUG: The underlying object has been mutated + expect(original.nullProp).toBe(null); // FAILS - mutated to {} + }); + + it("real-world: D1 journaling with nullable fields", () => { + // Real-world scenario: match state with nullable fields + // When serialized to D1, {} becomes "[object Object]" instead of NULL + interface MatchState { + id: string; + ownerId: string | null; + firstMoveMadeAt: number | null; + completedAt: number | null; + } + + const matchState: MatchState = { + id: "match_123", + ownerId: null, + firstMoveMadeAt: null, + completedAt: null, + }; + + const proxied = createBuggyProxy(matchState); + + // When we journal to D1, we need these null values + // BUG: They get auto-vivified to {} which serializes as "[object Object]" + expect(proxied.ownerId).toBe(null); + expect(proxied.firstMoveMadeAt).toBe(null); + expect(proxied.completedAt).toBe(null); + + // Verify the original wasn't corrupted + expect(matchState.ownerId).toBe(null); + expect(matchState.firstMoveMadeAt).toBe(null); + expect(matchState.completedAt).toBe(null); + }); +}); From 9c69679870692b7fcb9b29ceac95b4eb6128600f Mon Sep 17 00:00:00 2001 From: Andrew <6360780@gmail.com> Date: Wed, 26 Nov 2025 00:50:18 -0500 Subject: [PATCH 3/6] fix(@Persist): prevent null auto-vivification on read operations The proxy's get() handler was mutating null/undefined properties to {} when reading them. This corrupted domain values and caused [object Object] serialization in D1 journaling. Fix: Return null/undefined as-is instead of auto-vivifying them. Also updates test to use actual createDeepProxy via __test export. --- packages/core/src/persist.test.ts | 80 ++++++++++--------------------- packages/core/src/persist.ts | 21 ++++---- 2 files changed, 36 insertions(+), 65 deletions(-) diff --git a/packages/core/src/persist.test.ts b/packages/core/src/persist.test.ts index f87bc6e..90787d6 100644 --- a/packages/core/src/persist.test.ts +++ b/packages/core/src/persist.test.ts @@ -1,44 +1,17 @@ import { describe, expect, it } from "vitest"; +import { __test } from "./persist"; -// Symbol to mark an object as proxied -const IS_PROXIED = Symbol("IS_PROXIED"); +const { createDeepProxy } = __test; + +// No-op trigger for testing (we don't need persistence in these tests) +const noopTrigger = () => {}; /** - * Minimal reproduction of the createDeepProxy function from persist.ts - * to test the null auto-vivification bug in isolation. - * - * This replicates the buggy behavior from lines 83-90 of persist.ts + * Helper to create a proxy using the real createDeepProxy function. + * Uses minimal mock instance and property key for testing purposes. */ -function createBuggyProxy(value: T): T { - const proxy = new Proxy(value as object, { - get(target: object, key: string | symbol): unknown { - if (key === IS_PROXIED) return true; - - if (typeof key === "symbol") { - return Reflect.get(target, key); - } - - const prop = Reflect.get(target, key); - - // BUG: This auto-vivifies null to {} on READ operations - // The proxy MUTATES the underlying object when reading a null property - // This is the exact logic from persist.ts lines 83-90 - if ( - (prop === null || prop === undefined) && - typeof key === "string" && - !key.startsWith("_") && - key !== "length" - ) { - const newObj = {}; - Reflect.set(target, key, newObj); // <-- MUTATES the underlying object! - return newObj; - } - - return prop; - }, - }); - - return proxy as T; +function createTestProxy(value: T): T { + return createDeepProxy(value, {}, "testProp", noopTrigger); } describe("persist proxy - null auto-vivification bug", () => { @@ -50,15 +23,14 @@ describe("persist proxy - null auto-vivification bug", () => { name: "test", }; - const proxied = createBuggyProxy(original); + const proxied = createTestProxy(original); // Read the null property const ownerId = proxied.ownerId; - // BUG: Currently ownerId is {} instead of null - // and original.ownerId has been mutated to {} - expect(ownerId).toBe(null); // FAILS - gets {} - expect(original.ownerId).toBe(null); // FAILS - mutated to {} + // Should return null as-is without mutation + expect(ownerId).toBe(null); + expect(original.ownerId).toBe(null); }); it("should preserve null values in object spread", () => { @@ -69,35 +41,34 @@ describe("persist proxy - null auto-vivification bug", () => { completedAt: null as number | null, }; - const proxied = createBuggyProxy(original); + const proxied = createTestProxy(original); // Spread the proxied object (this is what our code does) const copy = { ...proxied }; - // BUG: Spread triggers get() for each property - // null properties get auto-vivified to {} - expect(copy.ownerId).toBe(null); // FAILS - gets {} - expect(copy.firstMoveMadeAt).toBe(null); // FAILS - gets {} - expect(copy.completedAt).toBe(null); // FAILS - gets {} + // Spread should preserve null values + expect(copy.ownerId).toBe(null); + expect(copy.firstMoveMadeAt).toBe(null); + expect(copy.completedAt).toBe(null); // The original should NOT be mutated by a read operation - expect(original.ownerId).toBe(null); // FAILS - mutated to {} + expect(original.ownerId).toBe(null); }); it("should NOT mutate the underlying object on property read", () => { const original = { nullProp: null as null }; - const proxied = createBuggyProxy(original); + const proxied = createTestProxy(original); // Just reading should not mutate void proxied.nullProp; - // BUG: The underlying object has been mutated - expect(original.nullProp).toBe(null); // FAILS - mutated to {} + // The underlying object should remain unchanged + expect(original.nullProp).toBe(null); }); it("real-world: D1 journaling with nullable fields", () => { // Real-world scenario: match state with nullable fields - // When serialized to D1, {} becomes "[object Object]" instead of NULL + // When serialized to D1, null values must be preserved interface MatchState { id: string; ownerId: string | null; @@ -112,10 +83,9 @@ describe("persist proxy - null auto-vivification bug", () => { completedAt: null, }; - const proxied = createBuggyProxy(matchState); + const proxied = createTestProxy(matchState); - // When we journal to D1, we need these null values - // BUG: They get auto-vivified to {} which serializes as "[object Object]" + // When we journal to D1, null values should be preserved expect(proxied.ownerId).toBe(null); expect(proxied.firstMoveMadeAt).toBe(null); expect(proxied.completedAt).toBe(null); diff --git a/packages/core/src/persist.ts b/packages/core/src/persist.ts index dc78742..631cd3f 100644 --- a/packages/core/src/persist.ts +++ b/packages/core/src/persist.ts @@ -77,16 +77,12 @@ function createDeepProxy(value: any, instance: any, propertyKey: string, trigger } const prop = Reflect.get(target, key); - - // If the property is null or undefined but is being accessed as an object, - // automatically convert it to an object - if ((prop === null || prop === undefined) && - typeof key === 'string' && - !key.startsWith('_') && - key !== 'length') { - const newObj = {}; - Reflect.set(target, key, newObj); - return createDeepProxy(newObj, instance, propertyKey, triggerPersist); + + // Return null/undefined as-is - these are intentional values + // Do NOT auto-vivify on read operations as this mutates the + // underlying object and corrupts domain values + if (prop === null || prop === undefined) { + return prop; } // Special handling for array methods that modify the array @@ -515,6 +511,11 @@ function safeParse(json: string): any { }); } +// Test exports - expose internal functions for unit testing +export const __test = { + createDeepProxy, +}; + /** * Helper function to persist a property value to storage. */ From 9e22e1c38b249af34ac0db73156cd4c901f18e2d Mon Sep 17 00:00:00 2001 From: Andrew <6360780@gmail.com> Date: Wed, 26 Nov 2025 00:50:24 -0500 Subject: [PATCH 4/6] fix(@Persist): add test coverage for undefined and falsy values - Added test for undefined property values (same behavior as null) - Added test for falsy but valid values (0, false, "") --- packages/core/src/persist.test.ts | 37 +++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/packages/core/src/persist.test.ts b/packages/core/src/persist.test.ts index 90787d6..cb09782 100644 --- a/packages/core/src/persist.test.ts +++ b/packages/core/src/persist.test.ts @@ -95,4 +95,41 @@ describe("persist proxy - null auto-vivification bug", () => { expect(matchState.firstMoveMadeAt).toBe(null); expect(matchState.completedAt).toBe(null); }); + + it("should preserve undefined values without mutation", () => { + const original = { + definedProp: undefined as string | undefined, + anotherProp: "value", + }; + + const proxied = createTestProxy(original); + + // Read the undefined property + const value = proxied.definedProp; + + // Should return undefined as-is without mutation + expect(value).toBe(undefined); + expect(original.definedProp).toBe(undefined); + }); + + it("should preserve falsy but valid values (0, false, empty string)", () => { + const original = { + zero: 0, + falseValue: false, + emptyString: "", + }; + + const proxied = createTestProxy(original); + + // All falsy values should pass through unchanged + expect(proxied.zero).toBe(0); + expect(proxied.falseValue).toBe(false); + expect(proxied.emptyString).toBe(""); + + // Original should remain unchanged + expect(original.zero).toBe(0); + expect(original.falseValue).toBe(false); + expect(original.emptyString).toBe(""); + }); + }); From fbaab1e0e75ee2c9b0b9789f49a6b54806b3208a Mon Sep 17 00:00:00 2001 From: Andrew <6360780@gmail.com> Date: Wed, 26 Nov 2025 00:52:57 -0500 Subject: [PATCH 5/6] docs(example): demonstrate nullable field persistence Add optionalOwner nullable field to persist example showing that null values are correctly preserved through the @Persist proxy. --- examples/persist/src/index.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/examples/persist/src/index.ts b/examples/persist/src/index.ts index f0ead45..2db080d 100644 --- a/examples/persist/src/index.ts +++ b/examples/persist/src/index.ts @@ -6,7 +6,7 @@ import { Actor, handler, Persist } from '../../../packages/core/src' export class MyPersistActor extends Actor { @Persist public myCustomNumber: number = 0; - + @Persist public myCustomObject: Record = { customKey: { @@ -14,6 +14,10 @@ export class MyPersistActor extends Actor { } }; + // Nullable fields are preserved correctly - reading them returns null, not {} + @Persist + public optionalOwner: string | null = null; + static async nameFromRequest(request: Request): Promise { // Pause for 500 milliseconds to mimic async operation await new Promise(resolve => setTimeout(resolve, 500)); @@ -26,6 +30,7 @@ export class MyPersistActor extends Actor { // you could use this as a replacement to `constructor`. console.log('Previous value: ', this.myCustomNumber); console.log('Previous object: ', JSON.stringify(this.myCustomObject)); + console.log('Optional owner: ', this.optionalOwner); // null is preserved, not converted to {} } async fetch(request: Request): Promise { From 438b5d0ccde43e76430eb1a914b8a18c781bb482 Mon Sep 17 00:00:00 2001 From: Andrew <6360780@gmail.com> Date: Wed, 26 Nov 2025 00:59:35 -0500 Subject: [PATCH 6/6] fix(`@Persist`): prevent heap overflow in error handler The error handler was creating {} and returning a new proxy on ANY error, which caused: 1. Silent data corruption (original value replaced with {}) 2. Infinite proxy recursion leading to heap overflow Fix: Return undefined on error instead of auto-vivifying. --- packages/core/src/persist.test.ts | 55 +++++++++++++++++++++++++++++++ packages/core/src/persist.ts | 13 +++++--- 2 files changed, 64 insertions(+), 4 deletions(-) create mode 100644 packages/core/src/persist.test.ts diff --git a/packages/core/src/persist.test.ts b/packages/core/src/persist.test.ts new file mode 100644 index 0000000..534302d --- /dev/null +++ b/packages/core/src/persist.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, it } from "vitest"; +import { __test } from "./persist"; + +const { createDeepProxy } = __test; + +// No-op trigger for testing (we don't need persistence in these tests) +const noopTrigger = () => {}; + +/** + * Helper to create a proxy using the real createDeepProxy function. + * Uses minimal mock instance and property key for testing purposes. + */ +function createTestProxy(value: T): T { + return createDeepProxy(value, {}, "testProp", noopTrigger); +} + +describe("persist proxy - error handler heap overflow bug", () => { + it("should return undefined on error instead of creating infinite proxy chain", () => { + // Create an object with a throwing getter + const throwingObj = { + get badProp(): never { + throw new Error("This getter throws"); + }, + normalProp: "hello", + }; + + const proxied = createTestProxy(throwingObj); + + // Accessing the throwing getter should return undefined, not create {} and recurse + // Before the fix, this would cause a heap overflow from infinite proxy recursion + const result = proxied.badProp; + + expect(result).toBe(undefined); + // The normal prop should still work + expect(proxied.normalProp).toBe("hello"); + }); + + it("should not corrupt the original object on error", () => { + const original: Record = { + get explosive(): never { + throw new Error("boom"); + }, + }; + + const proxied = createTestProxy(original); + + // Access the throwing getter + void proxied.explosive; + + // The original should not be mutated with {} + // Check that 'explosive' is still a getter, not {} + const descriptor = Object.getOwnPropertyDescriptor(original, "explosive"); + expect(descriptor?.get).toBeDefined(); + }); +}); diff --git a/packages/core/src/persist.ts b/packages/core/src/persist.ts index dc78742..bf84a15 100644 --- a/packages/core/src/persist.ts +++ b/packages/core/src/persist.ts @@ -123,10 +123,10 @@ function createDeepProxy(value: any, instance: any, propertyKey: string, trigger return prop; } catch (e) { console.error(`Error accessing property ${String(key)}:`, e); - // Return an empty object proxy for error recovery - const newObj = {}; - Reflect.set(target, key, newObj); - return createDeepProxy(newObj, instance, propertyKey, triggerPersist); + // Return undefined on error - don't auto-vivify as it causes: + // 1. Silent data corruption (replaces original value with {}) + // 2. Infinite proxy recursion leading to heap overflow + return undefined; } }, set(target, key, newValue) { @@ -515,6 +515,11 @@ function safeParse(json: string): any { }); } +// Test exports - expose internal functions for unit testing +export const __test = { + createDeepProxy, +}; + /** * Helper function to persist a property value to storage. */