Readonly state

[threepointone]

fixes #255

[changeset-bot]

🦋 Changeset detected

Latest commit: 66d1916

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
agents	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/agents@610

commit: 66d1916

[claude]

Claude Code Review

PR #610: Readonly State

This PR implements readonly connections, allowing WebSocket clients to receive state updates and call RPC methods without permission to modify state. The implementation is solid with comprehensive test coverage.

Issues Found

1. SQL Query Performance Issue (packages/agents/src/index.ts:775-795)

isConnectionReadonly() queries the database on every check when the connection is not in the in-memory cache. This could cause performance problems during high-frequency state updates.

isConnectionReadonly(connection: Connection): boolean {
  if (this._readonlyConnections.has(connection.id)) {
    return true;
  }
  
  // This SQL query runs every time for non-readonly connections
  const result = this.sql<{ connection_id: string }>`
    SELECT connection_id FROM cf_agents_readonly_connections
    WHERE connection_id = ${connection.id}
  `;
  // ...
}

Recommendation: Cache negative results (non-readonly connections) as well, or at minimum document the performance characteristics. Consider:

• Track non-readonly connections in a separate Set
• Add a flag to track if DB has been checked for a given connection
• Document that this is optimized for the readonly case

2. Race Condition on Cleanup (packages/agents/src/index.ts:648-675)

The onClose wrapper performs SQL DELETE without waiting for completion. If the agent hibernates immediately after onClose returns, the deletion might not persist.

this.onClose = (connection: Connection) => {
  this._readonlyConnections.delete(connection.id);
  this.sql`DELETE FROM cf_agents_readonly_connections WHERE connection_id = ${connection.id}`;
  // No await - might not complete before hibernation
};

Impact: Low probability but could leave orphaned entries in the database that accumulate over time.

Recommendation: If SQL operations should complete before hibernation, consider awaiting or document this tradeoff.

Architecture & Testing

✅ Proper separation of concerns with server-side and client-side APIs
✅ Comprehensive test coverage (425 lines covering all major scenarios)
✅ Hibernation persistence correctly implemented with dual in-memory/SQL storage
✅ Client error handling with onStateUpdateError callback
✅ Documentation is thorough with practical examples

Minor Observations

• Test at line 263: Comment says "reconnect with same connection" but creates a new connection with same room name (expected behavior, just misleading comment)
• The TestReadonlyAgent.stateUpdateAttempts array tracks attempts but is never cleared - acceptable for test agent

---

Overall: Good implementation. Address the SQL performance pattern before merging.

[threepointone]

moving to draft, might have a better implementation here