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 { 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]; diff --git a/packages/core/src/persist.test.ts b/packages/core/src/persist.test.ts new file mode 100644 index 0000000..a65f2e7 --- /dev/null +++ b/packages/core/src/persist.test.ts @@ -0,0 +1,169 @@ +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 - 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 = createTestProxy(original); + + // Read the null property + const ownerId = proxied.ownerId; + + // Should return null as-is without mutation + expect(ownerId).toBe(null); + expect(original.ownerId).toBe(null); + }); + + 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, +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); + + // Spread the proxied object (this is what our code does) + const copy = { ...proxied }; + + // 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); + }); + + it("should NOT mutate the underlying object on property read", () => { + const original = { nullProp: null as null }; + const proxied = createTestProxy(original); + + // Just reading should not mutate + void proxied.nullProp; + + // 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, null values must be preserved + 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 = createTestProxy(matchState); + + // 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); + + // Verify the original wasn't corrupted + expect(matchState.ownerId).toBe(null); + 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(""); + }); + + // 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..9bd0e1e 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 @@ -123,10 +119,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 +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. */