-
Notifications
You must be signed in to change notification settings - Fork 631
Schedule fixes #653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Schedule fixes #653
Changes from all commits
5c0c76c
7977d49
c2b5ce2
9e279fe
8dffadb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "agents": patch | ||
| --- | ||
|
|
||
| Allow `this.destroy` inside a schedule by including a `destroyed` flag and yielding `ctx.abort` instead of calling it directly | ||
| Fix issue where schedules would not be able to run for more 30 seconds due to `blockConccurencyWhile`. `alarm()` isn't manually called anymore, getting rid of the bCW. | ||
| Fix an issue where immediate schedules (e.g. `this.schedule(0, "foo"))`) would not get immediately scheduled. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import { env } from "cloudflare:test"; | ||
| import { describe, expect, it } from "vitest"; | ||
| import type { Env } from "./worker"; | ||
| import { getAgentByName } from ".."; | ||
|
|
||
| declare module "cloudflare:test" { | ||
| interface ProvidedEnv extends Env {} | ||
| } | ||
|
|
||
| describe("scheduled destroys", () => { | ||
| it("should not throw when a scheduled callback nukes storage", async () => { | ||
| let agentStub = await getAgentByName( | ||
| env.TestDestroyScheduleAgent, | ||
| "alarm-destroy-repro" | ||
| ); | ||
|
|
||
| // Alarm should fire immediately | ||
| await agentStub.scheduleSelfDestructingAlarm(); | ||
| await expect(agentStub.getStatus()).resolves.toBe("scheduled"); | ||
|
|
||
| // Let the alarm run | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); | ||
|
|
||
| agentStub = await getAgentByName( | ||
| env.TestDestroyScheduleAgent, | ||
| "alarm-destroy-repro" | ||
| ); | ||
|
|
||
| await expect(agentStub.getStatus()).resolves.toBe("unscheduled"); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ export type Env = { | |
| TestChatAgent: DurableObjectNamespace<TestChatAgent>; | ||
| TestOAuthAgent: DurableObjectNamespace<TestOAuthAgent>; | ||
| TEST_MCP_JURISDICTION: DurableObjectNamespace<TestMcpJurisdiction>; | ||
| TestDestroyScheduleAgent: DurableObjectNamespace<TestDestroyScheduleAgent>; | ||
| }; | ||
|
|
||
| type State = unknown; | ||
|
|
@@ -39,6 +40,7 @@ type Props = { | |
| }; | ||
|
|
||
| export class TestMcpAgent extends McpAgent<Env, State, Props> { | ||
| observability = undefined; | ||
| private tempToolHandle?: { remove: () => void }; | ||
|
|
||
| server = new McpServer( | ||
|
|
@@ -130,6 +132,7 @@ export class TestMcpAgent extends McpAgent<Env, State, Props> { | |
|
|
||
| // Test email agents | ||
| export class TestEmailAgent extends Agent<Env> { | ||
| observability = undefined; | ||
| emailsReceived: AgentEmail[] = []; | ||
|
|
||
| async onEmail(email: AgentEmail) { | ||
|
|
@@ -144,6 +147,7 @@ export class TestEmailAgent extends Agent<Env> { | |
| } | ||
|
|
||
| export class TestCaseSensitiveAgent extends Agent<Env> { | ||
| observability = undefined; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why removing observability on all these agents?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They flood stdout with logs about schedules, connections etc. I've never found them particularly useful (at least in the test runs). There's still a lot of workerd internal logs but I appreciate less noise unless others find them useful
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fair. tbh I'm very unhappy with our observability stuff anyway, I want to rip it all out. |
||
| emailsReceived: AgentEmail[] = []; | ||
|
|
||
| async onEmail(email: AgentEmail) { | ||
|
|
@@ -156,6 +160,7 @@ export class TestCaseSensitiveAgent extends Agent<Env> { | |
| } | ||
|
|
||
| export class TestUserNotificationAgent extends Agent<Env> { | ||
| observability = undefined; | ||
| emailsReceived: AgentEmail[] = []; | ||
|
|
||
| async onEmail(email: AgentEmail) { | ||
|
|
@@ -167,12 +172,30 @@ export class TestUserNotificationAgent extends Agent<Env> { | |
| } | ||
| } | ||
|
|
||
| export class TestDestroyScheduleAgent extends Agent<Env, { status: string }> { | ||
| observability = undefined; | ||
| initialState = { | ||
| status: "unscheduled" | ||
| }; | ||
|
|
||
| async scheduleSelfDestructingAlarm() { | ||
| this.setState({ status: "scheduled" }); | ||
| await this.schedule(0, "destroy"); | ||
| } | ||
|
|
||
| getStatus() { | ||
| return this.state.status; | ||
| } | ||
| } | ||
|
|
||
| // An Agent that tags connections in onConnect, | ||
| // then echoes whether the tag was observed in onMessage | ||
| export class TestRaceAgent extends Agent<Env> { | ||
| initialState = { hello: "world" }; | ||
| static options = { hibernate: true }; | ||
|
|
||
| observability = undefined; | ||
|
|
||
| async onConnect(conn: Connection<{ tagged: boolean }>) { | ||
| // Simulate real async setup to widen the window a bit | ||
| conn.setState({ tagged: true }); | ||
|
|
@@ -187,6 +210,8 @@ export class TestRaceAgent extends Agent<Env> { | |
|
|
||
| // Test Agent for OAuth client side flows | ||
| export class TestOAuthAgent extends Agent<Env> { | ||
| observability = undefined; | ||
|
|
||
| async onRequest(_request: Request): Promise<Response> { | ||
| return new Response("Test OAuth Agent"); | ||
| } | ||
|
|
@@ -311,6 +336,8 @@ export class TestOAuthAgent extends Agent<Env> { | |
| } | ||
|
|
||
| export class TestChatAgent extends AIChatAgent<Env> { | ||
| observability = undefined; | ||
|
|
||
| async onChatMessage() { | ||
| // Simple echo response for testing | ||
| return new Response("Hello from chat agent!", { | ||
|
|
@@ -373,6 +400,8 @@ export class TestChatAgent extends AIChatAgent<Env> { | |
|
|
||
| // Test MCP Agent for jurisdiction feature | ||
| export class TestMcpJurisdiction extends McpAgent<Env> { | ||
| observability = undefined; | ||
|
|
||
| server = new McpServer( | ||
| { name: "test-jurisdiction-server", version: "1.0.0" }, | ||
| { capabilities: { tools: {} } } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really wish I could remember why we did this. but I'll keep an.eye out for it.