From 1578c75709312931703ff9f990b2c91dae082de2 Mon Sep 17 00:00:00 2001 From: Michael Davidson Date: Tue, 1 Sep 2026 09:21:24 +1000 Subject: [PATCH 1/3] fix(edge-worker): stop reading activities back through an unmemoized SDK getter `AgentActivityPayload.agentActivity` is a getter with no memoisation: each access constructs a fresh `AgentActivityQuery` and issues another round trip. Both activity-posting paths touched it twice per post -- once for truthiness in the `if`, once for the awaited assignment -- while consuming only `.id`. Every agent activity therefore cost three Linear requests instead of one. The promise produced by the `if`-condition access is never awaited, so a failed read-back also leaves an unhandled rejection. `ActivityPoster`'s try/catch does not cover it: the catch block never awaits that promise. `agentActivityId` returns the same id from the mutation response the SDK is already holding, at no request cost. Failure semantics are unchanged: a mutation that did not succeed takes exactly the same branch as before. Adds a request-counting harness that reproduces the SDK getter, because the existing mocks model `agentActivity` as a plain resolved promise property and cannot observe the cost of touching it twice. --- CHANGELOG.md | 1 + packages/edge-worker/src/ActivityPoster.ts | 13 +- .../src/sinks/LinearActivitySink.ts | 12 +- .../test/ActivityPoster.request-count.test.ts | 125 ++++++++++++++++++ .../LinearActivitySink.request-count.test.ts | 122 +++++++++++++++++ .../test/LinearActivitySink.test.ts | 18 +++ .../test/agent-activity-payload-double.ts | 74 +++++++++++ 7 files changed, 358 insertions(+), 7 deletions(-) create mode 100644 packages/edge-worker/test/ActivityPoster.request-count.test.ts create mode 100644 packages/edge-worker/test/LinearActivitySink.request-count.test.ts create mode 100644 packages/edge-worker/test/agent-activity-payload-double.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f772c488..9cff3320b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ All notable changes to this project will be documented in this file. ## [Unreleased] ### Fixed +- Posting an agent activity to Linear now costs one API request instead of three. Both activity-posting paths read the newly created activity back through `AgentActivityPayload.agentActivity`, an unmemoized Linear SDK getter that issues a fresh query on every property access, and both touched it twice while consuming only its `id`. The two extra requests per activity tripled Cyrus's consumption of the Linear API budget and drove rate limiting on busy sessions; a read-back that failed also left an unhandled promise rejection behind. Activity ids now come from `agentActivityId`, which the mutation response already carries. - EdgeWorker state saves are now atomic, preventing a process interrupted during a save from leaving a truncated state file that strands in-flight sessions; empty and legacy-truncated state files also recover cleanly. Thanks @connor-tembo for the contribution. ([CYPACK-1486](https://linear.app/ceedar/issue/CYPACK-1486/can-you-add-a-changelog-entry-for-this), [#1444](https://github.com/cyrusagents/cyrus/pull/1444)) ### Changed diff --git a/packages/edge-worker/src/ActivityPoster.ts b/packages/edge-worker/src/ActivityPoster.ts index 689849c7d..32a740a1b 100644 --- a/packages/edge-worker/src/ActivityPoster.ts +++ b/packages/edge-worker/src/ActivityPoster.ts @@ -29,10 +29,15 @@ export class ActivityPoster { try { const result = await issueTracker.createAgentActivity(input); if (result.success) { - if (result.agentActivity) { - const activity = await result.agentActivity; - this.logger.debug(`Created ${label} activity ${activity.id}`); - return activity.id; + // `result.agentActivity` is an unmemoized getter: every access + // constructs a fresh AgentActivityQuery and issues another round trip + // to read back the activity we just created. Only the id was ever + // consumed, and `result.agentActivityId` returns it from the mutation + // response the SDK is already holding, at no request cost. + const activityId = result.agentActivityId; + if (activityId) { + this.logger.debug(`Created ${label} activity ${activityId}`); + return activityId; } this.logger.debug(`Created ${label}`); return null; diff --git a/packages/edge-worker/src/sinks/LinearActivitySink.ts b/packages/edge-worker/src/sinks/LinearActivitySink.ts index 27244edc4..faeef5ee8 100644 --- a/packages/edge-worker/src/sinks/LinearActivitySink.ts +++ b/packages/edge-worker/src/sinks/LinearActivitySink.ts @@ -97,9 +97,15 @@ export class LinearActivitySink implements IActivitySink { }), }); - if (result.success && result.agentActivity) { - const agentActivity = await result.agentActivity; - return { activityId: agentActivity.id }; + // `result.agentActivity` is an unmemoized getter: every access constructs + // a fresh AgentActivityQuery and issues another round trip to read back + // the activity we just created. Only the id was ever consumed, and + // `result.agentActivityId` returns it from the mutation response the SDK + // is already holding, at no request cost. + const activityId = result.agentActivityId; + + if (result.success && activityId) { + return { activityId }; } return {}; diff --git a/packages/edge-worker/test/ActivityPoster.request-count.test.ts b/packages/edge-worker/test/ActivityPoster.request-count.test.ts new file mode 100644 index 000000000..d8a3f39cd --- /dev/null +++ b/packages/edge-worker/test/ActivityPoster.request-count.test.ts @@ -0,0 +1,125 @@ +/** + * Regression tests for the number of Linear requests that one + * ActivityPoster.postActivityDirect() call issues. + * + * postActivityDirect is the second path that posts an agent activity, and it + * read the created activity back through the same unmemoized SDK getter as + * LinearActivitySink did. Its try/catch does not make the orphaned read-back + * safe: the promise produced by the `if` condition is never awaited, so its + * rejection is not the catch block's to handle. + */ + +import type { IIssueTrackerService, ILogger } from "cyrus-core"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { ActivityPoster } from "../src/ActivityPoster.js"; +import { + countingCreateAgentActivity, + createAgentActivityPayload, + createRequestLog, + type RequestLog, +} from "./agent-activity-payload-double.js"; + +describe("ActivityPoster request count", () => { + let poster: ActivityPoster; + let issueTracker: IIssueTrackerService; + let createAgentActivity: ReturnType; + let logger: ILogger; + let log: RequestLog; + + const input = { + agentSessionId: "session-1", + content: { type: "thought" as const, body: "Analyzing..." }, + }; + + const respondWith = (payload: object) => { + createAgentActivity.mockImplementation( + countingCreateAgentActivity(log, payload), + ); + }; + + beforeEach(() => { + log = createRequestLog(); + createAgentActivity = vi.fn(); + issueTracker = { createAgentActivity } as unknown as IIssueTrackerService; + logger = { + debug: vi.fn(), + error: vi.fn(), + warn: vi.fn(), + info: vi.fn(), + } as unknown as ILogger; + + poster = new ActivityPoster( + new Map([["workspace-1", issueTracker]]), + new Map(), + logger, + ); + }); + + it("should issue exactly one Linear request per posted activity", async () => { + respondWith(createAgentActivityPayload(log, "activity-1")); + + const activityId = await poster.postActivityDirect( + issueTracker, + input, + "thought", + ); + + expect(activityId).toBe("activity-1"); + expect(log.operations).toEqual(["mutation:agentActivityCreate"]); + }); + + it("should not read the activity back after creating it", async () => { + respondWith(createAgentActivityPayload(log, "activity-2")); + + await poster.postActivityDirect(issueTracker, input, "thought"); + + expect(log.operations).not.toContain("query:agentActivity"); + }); + + it("should not orphan a rejected read-back promise", async () => { + respondWith( + createAgentActivityPayload(log, "activity-3", { readBackRejects: true }), + ); + + const unhandled: unknown[] = []; + const onUnhandledRejection = (reason: unknown) => unhandled.push(reason); + process.on("unhandledRejection", onUnhandledRejection); + + try { + await poster.postActivityDirect(issueTracker, input, "thought"); + // Yield a macrotask so Node can report any rejection left unhandled. + await new Promise((resolve) => setTimeout(resolve, 10)); + } finally { + process.off("unhandledRejection", onUnhandledRejection); + } + + expect(unhandled).toEqual([]); + }); + + it("should still return null and log an error when the mutation did not succeed", async () => { + respondWith({ success: false, lastSyncId: 1 }); + + const activityId = await poster.postActivityDirect( + issueTracker, + input, + "thought", + ); + + expect(activityId).toBeNull(); + expect(logger.error).toHaveBeenCalled(); + expect(log.operations).toEqual(["mutation:agentActivityCreate"]); + }); + + it("should still return null when the payload carries no activity id", async () => { + respondWith({ success: true, lastSyncId: 1, agentActivityId: undefined }); + + const activityId = await poster.postActivityDirect( + issueTracker, + input, + "thought", + ); + + expect(activityId).toBeNull(); + expect(logger.error).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/edge-worker/test/LinearActivitySink.request-count.test.ts b/packages/edge-worker/test/LinearActivitySink.request-count.test.ts new file mode 100644 index 000000000..78316c19b --- /dev/null +++ b/packages/edge-worker/test/LinearActivitySink.request-count.test.ts @@ -0,0 +1,122 @@ +/** + * Regression tests for the number of Linear requests that one + * LinearActivitySink.postActivity() call issues. + * + * These live in their own file because they need a different test double from + * the one in LinearActivitySink.test.ts: the payload has to reproduce the SDK's + * unmemoized `agentActivity` getter before the cost of touching it twice is + * observable at all. See ./agent-activity-payload-double.ts. + */ + +import type { AgentActivityContent, IIssueTrackerService } from "cyrus-core"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { LinearActivitySink } from "../src/sinks/LinearActivitySink.js"; +import { + countingCreateAgentActivity, + createAgentActivityPayload, + createRequestLog, + type RequestLog, +} from "./agent-activity-payload-double.js"; + +describe("LinearActivitySink request count", () => { + let sink: LinearActivitySink; + let mockIssueTracker: IIssueTrackerService; + let log: RequestLog; + + const mockWorkspaceId = "workspace-123"; + const mockSessionId = "session-456"; + + const activity: AgentActivityContent = { + type: "thought", + body: "Analyzing the codebase...", + }; + + const respondWith = (payload: object) => { + vi.mocked(mockIssueTracker.createAgentActivity).mockImplementation( + countingCreateAgentActivity(log, payload), + ); + }; + + beforeEach(() => { + log = createRequestLog(); + mockIssueTracker = { + createAgentActivity: vi.fn(), + createAgentSessionOnIssue: vi.fn(), + } as unknown as IIssueTrackerService; + + sink = new LinearActivitySink(mockIssueTracker, mockWorkspaceId); + }); + + it("should issue exactly one Linear request per posted activity", async () => { + respondWith(createAgentActivityPayload(log, "activity-1")); + + const result = await sink.postActivity(mockSessionId, activity); + + expect(result).toEqual({ activityId: "activity-1" }); + expect(log.operations).toEqual(["mutation:agentActivityCreate"]); + expect(log.operations).toHaveLength(1); + }); + + it("should not read the activity back after creating it", async () => { + respondWith(createAgentActivityPayload(log, "activity-2")); + + await sink.postActivity(mockSessionId, activity); + + expect(log.operations).not.toContain("query:agentActivity"); + }); + + it("should issue one request per activity across a burst of posts", async () => { + respondWith(createAgentActivityPayload(log, "activity-3")); + + await sink.postActivity(mockSessionId, activity); + await sink.postActivity(mockSessionId, activity); + await sink.postActivity(mockSessionId, activity); + + expect(log.operations).toHaveLength(3); + }); + + it("should not orphan a rejected read-back promise", async () => { + // The getter access in the `if` condition produced a promise that nobody + // awaited. When that read-back failed -- which is exactly what happens + // once the extra requests have exhausted the rate-limit budget -- its + // rejection was unhandled. + respondWith( + createAgentActivityPayload(log, "activity-4", { readBackRejects: true }), + ); + + const unhandled: unknown[] = []; + const onUnhandledRejection = (reason: unknown) => unhandled.push(reason); + process.on("unhandledRejection", onUnhandledRejection); + + try { + await sink.postActivity(mockSessionId, activity).catch(() => undefined); + // Yield a macrotask so Node can report any rejection left unhandled. + await new Promise((resolve) => setTimeout(resolve, 10)); + } finally { + process.off("unhandledRejection", onUnhandledRejection); + } + + expect(unhandled).toEqual([]); + }); + + it("should still return an empty result when the mutation did not succeed", async () => { + respondWith({ success: false, lastSyncId: 1 }); + + const result = await sink.postActivity(mockSessionId, activity); + + expect(result).toEqual({}); + expect(log.operations).toEqual(["mutation:agentActivityCreate"]); + }); + + it("should still return an empty result when the payload carries no activity id", async () => { + respondWith({ + success: true, + lastSyncId: 1, + agentActivityId: undefined, + }); + + const result = await sink.postActivity(mockSessionId, activity); + + expect(result).toEqual({}); + }); +}); diff --git a/packages/edge-worker/test/LinearActivitySink.test.ts b/packages/edge-worker/test/LinearActivitySink.test.ts index 78289dc40..f830294e9 100644 --- a/packages/edge-worker/test/LinearActivitySink.test.ts +++ b/packages/edge-worker/test/LinearActivitySink.test.ts @@ -50,6 +50,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-1" }), + agentActivityId: "activity-1", } as any); const result = await sink.postActivity(mockSessionId, activity); @@ -72,6 +73,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-2" }), + agentActivityId: "activity-2", } as any); const result = await sink.postActivity(mockSessionId, activity); @@ -92,6 +94,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-3" }), + agentActivityId: "activity-3", } as any); const result = await sink.postActivity(mockSessionId, activity); @@ -108,6 +111,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-4" }), + agentActivityId: "activity-4", } as any); const result = await sink.postActivity(mockSessionId, activity); @@ -124,6 +128,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-5" }), + agentActivityId: "activity-5", } as any); const result = await sink.postActivity(mockSessionId, activity); @@ -185,6 +190,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-1" }), + agentActivityId: "activity-1", } as any); await sink.postActivity(mockSessionId, activity); @@ -202,6 +208,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-eph" }), + agentActivityId: "activity-eph", } as any); await sink.postActivity(mockSessionId, activity, options); @@ -226,6 +233,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-sig" }), + agentActivityId: "activity-sig", } as any); await sink.postActivity(mockSessionId, activity, options); @@ -247,6 +255,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-sel" }), + agentActivityId: "activity-sel", } as any); await sink.postActivity(mockSessionId, activity, { signal: "select" }); @@ -267,6 +276,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-stop" }), + agentActivityId: "activity-stop", } as any); await sink.postActivity(mockSessionId, activity, { signal: "stop" }); @@ -287,6 +297,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-cont" }), + agentActivityId: "activity-cont", } as any); await sink.postActivity(mockSessionId, activity, { @@ -310,6 +321,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-meta" }), + agentActivityId: "activity-meta", } as any); await sink.postActivity(mockSessionId, activity, { @@ -334,6 +346,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-no-eph" }), + agentActivityId: "activity-no-eph", } as any); await sink.postActivity(mockSessionId, activity, {}); @@ -416,6 +429,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-1" }), + agentActivityId: "activity-1", } as any); await sink.postActivity(mockSessionId, { @@ -475,6 +489,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-1" }), + agentActivityId: "activity-1", } as any); await sink.postActivity(mockSessionId, { @@ -496,6 +511,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-1" }), + agentActivityId: "activity-1", } as any); await sink.postActivity(mockSessionId, activity); @@ -515,6 +531,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-1" }), + agentActivityId: "activity-1", } as any); await sink.postActivity(mockSessionId, activity); @@ -535,6 +552,7 @@ describe("LinearActivitySink", () => { vi.mocked(mockIssueTracker.createAgentActivity).mockResolvedValue({ success: true, agentActivity: Promise.resolve({ id: "activity-1" }), + agentActivityId: "activity-1", } as any); await sink.postActivity(mockSessionId, activity); diff --git a/packages/edge-worker/test/agent-activity-payload-double.ts b/packages/edge-worker/test/agent-activity-payload-double.ts new file mode 100644 index 000000000..4c8a65669 --- /dev/null +++ b/packages/edge-worker/test/agent-activity-payload-double.ts @@ -0,0 +1,74 @@ +/** + * A test double for @linear/sdk's AgentActivityPayload that preserves the + * property shapes the edge worker actually reads, and a request counter to + * observe what those reads cost. + * + * The ordinary mocks elsewhere in this suite model + * `AgentActivityPayload.agentActivity` as a plain, already-resolved promise + * property. The real payload models it as an *unmemoized getter* (generated SDK + * v64): + * + * get agentActivity(): LinearFetch | undefined { + * return new AgentActivityQuery(this._request).fetch(this._agentActivity.id); + * } + * get agentActivityId(): string | undefined { + * return this._agentActivity?.id; + * } + * + * Every access of `agentActivity` constructs a fresh query and issues another + * round trip; `agentActivityId` reads an id the mutation response already + * carries, at no request cost. A property mock cannot tell the two apart, so + * the getter is reproduced here. + */ + +/** Records every Linear round trip issued during a call under test. */ +export interface RequestLog { + operations: string[]; +} + +export const createRequestLog = (): RequestLog => ({ operations: [] }); + +export interface AgentActivityPayloadDoubleOptions { + /** Make the activity read-back fail, as it does once the budget is spent. */ + readBackRejects?: boolean; +} + +/** + * Build a payload double that counts each `agentActivity` access as one query + * and charges nothing for `agentActivityId`. + */ +export function createAgentActivityPayload( + log: RequestLog, + activityId: string, + options: AgentActivityPayloadDoubleOptions = {}, +) { + return { + success: true, + lastSyncId: 1, + get agentActivity(): Promise<{ id: string }> { + log.operations.push("query:agentActivity"); + return options.readBackRejects + ? Promise.reject(new Error("Ratelimit exceeded")) + : Promise.resolve({ id: activityId }); + }, + get agentActivityId(): string | undefined { + return activityId; + }, + }; +} + +/** + * A `createAgentActivity` implementation that counts the mutation itself, so + * the request count for a call under test is `log.operations.length`. + * + * Measurement method: the counter sits on the transport, not on the caller. + * Each operation that would reach Linear appends exactly one entry -- the + * agentActivityCreate mutation here, and one agentActivity read-back query per + * access of the unmemoized getter above. + */ +export function countingCreateAgentActivity(log: RequestLog, payload: object) { + return async () => { + log.operations.push("mutation:agentActivityCreate"); + return payload as any; + }; +} From 1df68eff9a88dd24f39031cbd476c7febcebea97 Mon Sep 17 00:00:00 2001 From: Michael Davidson Date: Tue, 1 Sep 2026 09:22:49 +1000 Subject: [PATCH 2/3] docs: link PR #1448 in the changelog entry --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cff3320b..2ca0120f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ All notable changes to this project will be documented in this file. ## [Unreleased] ### Fixed -- Posting an agent activity to Linear now costs one API request instead of three. Both activity-posting paths read the newly created activity back through `AgentActivityPayload.agentActivity`, an unmemoized Linear SDK getter that issues a fresh query on every property access, and both touched it twice while consuming only its `id`. The two extra requests per activity tripled Cyrus's consumption of the Linear API budget and drove rate limiting on busy sessions; a read-back that failed also left an unhandled promise rejection behind. Activity ids now come from `agentActivityId`, which the mutation response already carries. +- Posting an agent activity to Linear now costs one API request instead of three. Both activity-posting paths read the newly created activity back through `AgentActivityPayload.agentActivity`, an unmemoized Linear SDK getter that issues a fresh query on every property access, and both touched it twice while consuming only its `id`. The two extra requests per activity tripled Cyrus's consumption of the Linear API budget and drove rate limiting on busy sessions; a read-back that failed also left an unhandled promise rejection behind. Activity ids now come from `agentActivityId`, which the mutation response already carries. ([#1448](https://github.com/cyrusagents/cyrus/pull/1448)) - EdgeWorker state saves are now atomic, preventing a process interrupted during a save from leaving a truncated state file that strands in-flight sessions; empty and legacy-truncated state files also recover cleanly. Thanks @connor-tembo for the contribution. ([CYPACK-1486](https://linear.app/ceedar/issue/CYPACK-1486/can-you-add-a-changelog-entry-for-this), [#1444](https://github.com/cyrusagents/cyrus/pull/1444)) ### Changed From 1d2be147ed9cba22e83fcf3379b87e6cc1b37de7 Mon Sep 17 00:00:00 2001 From: Michael Davidson Date: Tue, 1 Sep 2026 15:01:40 +1000 Subject: [PATCH 3/3] fix(edge-worker): keep the activity id on the CLI issue-tracker adapter Reading `agentActivityId` alone is correct for the Linear SDK payload but not for `CLIIssueTrackerService.createAgentActivity`, which returns { agentActivity: Promise.resolve({ id: activityId }), success: true, lastSyncId: Date.now(), } as AgentActivityPayload -- the id is carried only on `agentActivity`, as a plain already-resolved promise property rather than the SDK's lazy getter, and `agentActivityId` is absent. On that adapter `postActivityDirect` returned null and `LinearActivitySink.postActivity` returned {}, where both previously returned the id. The `as` cast is why the compiler did not catch it. Both paths now read `result.agentActivityId ?? (await result.agentActivity)?.id`. The fallback costs nothing on the Linear path: `agentActivityId` is present on every successful mutation response, `??` short-circuits before the right-hand side is evaluated, and the getter is never touched -- so the one-request-per- activity property holds. On the CLI adapter the fallback awaits a promise that is already settled, which is not a round trip. The getter stays out of the `if` condition so a rejected read-back is never orphaned, and in LinearActivitySink it is reached only once `success` holds, keeping failed mutations free. Adds a CLI-adapter-shaped payload double alongside the SDK-shaped one, and a guard that pins the id to a falsy-but-present value -- the only case that tells `??` from `||`, since both short-circuit on a populated id. Failure branches are unchanged. --- CHANGELOG.md | 2 +- packages/edge-worker/src/ActivityPoster.ts | 22 ++++++++---- .../src/sinks/LinearActivitySink.ts | 27 ++++++++++----- .../test/ActivityPoster.request-count.test.ts | 34 +++++++++++++++++++ .../LinearActivitySink.request-count.test.ts | 26 ++++++++++++++ .../test/agent-activity-payload-double.ts | 25 ++++++++++++++ 6 files changed, 121 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ca0120f9..73c59d834 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ All notable changes to this project will be documented in this file. ## [Unreleased] ### Fixed -- Posting an agent activity to Linear now costs one API request instead of three. Both activity-posting paths read the newly created activity back through `AgentActivityPayload.agentActivity`, an unmemoized Linear SDK getter that issues a fresh query on every property access, and both touched it twice while consuming only its `id`. The two extra requests per activity tripled Cyrus's consumption of the Linear API budget and drove rate limiting on busy sessions; a read-back that failed also left an unhandled promise rejection behind. Activity ids now come from `agentActivityId`, which the mutation response already carries. ([#1448](https://github.com/cyrusagents/cyrus/pull/1448)) +- Posting an agent activity to Linear now costs one API request instead of three. Both activity-posting paths read the newly created activity back through `AgentActivityPayload.agentActivity`, an unmemoized Linear SDK getter that issues a fresh query on every property access, and both touched it twice while consuming only its `id`. The two extra requests per activity tripled Cyrus's consumption of the Linear API budget and drove rate limiting on busy sessions; a read-back that failed also left an unhandled promise rejection behind. Activity ids now come from `agentActivityId`, which the mutation response already carries, falling back to `agentActivity` for the CLI issue-tracker adapter, whose payload holds the id there as an already-resolved promise and carries no `agentActivityId` at all. The fallback costs nothing on the Linear path, where `agentActivityId` is always present and short-circuits before the getter is reached. ([#1448](https://github.com/cyrusagents/cyrus/pull/1448)) - EdgeWorker state saves are now atomic, preventing a process interrupted during a save from leaving a truncated state file that strands in-flight sessions; empty and legacy-truncated state files also recover cleanly. Thanks @connor-tembo for the contribution. ([CYPACK-1486](https://linear.app/ceedar/issue/CYPACK-1486/can-you-add-a-changelog-entry-for-this), [#1444](https://github.com/cyrusagents/cyrus/pull/1444)) ### Changed diff --git a/packages/edge-worker/src/ActivityPoster.ts b/packages/edge-worker/src/ActivityPoster.ts index 32a740a1b..2bbd024a3 100644 --- a/packages/edge-worker/src/ActivityPoster.ts +++ b/packages/edge-worker/src/ActivityPoster.ts @@ -29,12 +29,22 @@ export class ActivityPoster { try { const result = await issueTracker.createAgentActivity(input); if (result.success) { - // `result.agentActivity` is an unmemoized getter: every access - // constructs a fresh AgentActivityQuery and issues another round trip - // to read back the activity we just created. Only the id was ever - // consumed, and `result.agentActivityId` returns it from the mutation - // response the SDK is already holding, at no request cost. - const activityId = result.agentActivityId; + // `result.agentActivity` is an unmemoized getter on the Linear SDK + // payload: every access constructs a fresh AgentActivityQuery and + // issues another round trip to read back the activity we just + // created. Only the id was ever consumed, and `result.agentActivityId` + // returns it from the mutation response the SDK is already holding, at + // no request cost. + // + // The fallback is for CLIIssueTrackerService, whose payload carries the + // id only on `agentActivity`, as an already-resolved promise property, + // and omits `agentActivityId` entirely. `??` short-circuits on the SDK + // path, so the getter is never touched there; on the CLI adapter it + // awaits a settled promise, which is not a request. Keeping the getter + // out of the `if` is what stops an unawaited read-back from being + // orphaned when it rejects. + const activityId = + result.agentActivityId ?? (await result.agentActivity)?.id; if (activityId) { this.logger.debug(`Created ${label} activity ${activityId}`); return activityId; diff --git a/packages/edge-worker/src/sinks/LinearActivitySink.ts b/packages/edge-worker/src/sinks/LinearActivitySink.ts index faeef5ee8..477a47c54 100644 --- a/packages/edge-worker/src/sinks/LinearActivitySink.ts +++ b/packages/edge-worker/src/sinks/LinearActivitySink.ts @@ -97,15 +97,26 @@ export class LinearActivitySink implements IActivitySink { }), }); - // `result.agentActivity` is an unmemoized getter: every access constructs - // a fresh AgentActivityQuery and issues another round trip to read back - // the activity we just created. Only the id was ever consumed, and - // `result.agentActivityId` returns it from the mutation response the SDK - // is already holding, at no request cost. - const activityId = result.agentActivityId; + // `result.agentActivity` is an unmemoized getter on the Linear SDK + // payload: every access constructs a fresh AgentActivityQuery and issues + // another round trip to read back the activity we just created. Only the + // id was ever consumed, and `result.agentActivityId` returns it from the + // mutation response the SDK is already holding, at no request cost. + // + // The fallback is for CLIIssueTrackerService, whose payload carries the id + // only on `agentActivity`, as an already-resolved promise property, and + // omits `agentActivityId` entirely. `??` short-circuits on the SDK path, so + // the getter is never touched there; on the CLI adapter it awaits a settled + // promise, which is not a request. Keeping the getter out of the `if` is + // what stops an unawaited read-back from being orphaned when it rejects, + // and reading it only once `success` holds keeps failed mutations free. + if (result.success) { + const activityId = + result.agentActivityId ?? (await result.agentActivity)?.id; - if (result.success && activityId) { - return { activityId }; + if (activityId) { + return { activityId }; + } } return {}; diff --git a/packages/edge-worker/test/ActivityPoster.request-count.test.ts b/packages/edge-worker/test/ActivityPoster.request-count.test.ts index d8a3f39cd..bfaaa1ffe 100644 --- a/packages/edge-worker/test/ActivityPoster.request-count.test.ts +++ b/packages/edge-worker/test/ActivityPoster.request-count.test.ts @@ -15,6 +15,7 @@ import { ActivityPoster } from "../src/ActivityPoster.js"; import { countingCreateAgentActivity, createAgentActivityPayload, + createCLIAdapterAgentActivityPayload, createRequestLog, type RequestLog, } from "./agent-activity-payload-double.js"; @@ -110,6 +111,39 @@ describe("ActivityPoster request count", () => { expect(log.operations).toEqual(["mutation:agentActivityCreate"]); }); + it("should not touch the read-back getter when the id the payload carries is empty", async () => { + // Distinguishes `??` from `||`. Both short-circuit on a populated id, so + // only a falsy-but-present id tells them apart: `??` keeps it and skips + // the getter, `||` falls through and pays for a read-back. + respondWith(createAgentActivityPayload(log, "")); + + const activityId = await poster.postActivityDirect( + issueTracker, + input, + "thought", + ); + + expect(activityId).toBeNull(); + expect(log.operations).toEqual(["mutation:agentActivityCreate"]); + }); + + it("should return the id when the payload comes from the CLI issue-tracker adapter", async () => { + // CLIIssueTrackerService carries the id only on `agentActivity`, as an + // already-resolved promise property, and omits `agentActivityId`. Reading + // `agentActivityId` alone drops the id on that adapter. + respondWith(createCLIAdapterAgentActivityPayload("activity-cli")); + + const activityId = await poster.postActivityDirect( + issueTracker, + input, + "thought", + ); + + expect(activityId).toBe("activity-cli"); + // Awaiting an already-resolved promise is not a round trip. + expect(log.operations).toEqual(["mutation:agentActivityCreate"]); + }); + it("should still return null when the payload carries no activity id", async () => { respondWith({ success: true, lastSyncId: 1, agentActivityId: undefined }); diff --git a/packages/edge-worker/test/LinearActivitySink.request-count.test.ts b/packages/edge-worker/test/LinearActivitySink.request-count.test.ts index 78316c19b..83d446fd3 100644 --- a/packages/edge-worker/test/LinearActivitySink.request-count.test.ts +++ b/packages/edge-worker/test/LinearActivitySink.request-count.test.ts @@ -14,6 +14,7 @@ import { LinearActivitySink } from "../src/sinks/LinearActivitySink.js"; import { countingCreateAgentActivity, createAgentActivityPayload, + createCLIAdapterAgentActivityPayload, createRequestLog, type RequestLog, } from "./agent-activity-payload-double.js"; @@ -108,6 +109,31 @@ describe("LinearActivitySink request count", () => { expect(log.operations).toEqual(["mutation:agentActivityCreate"]); }); + it("should not touch the read-back getter when the id the payload carries is empty", async () => { + // Distinguishes `??` from `||`. Both short-circuit on a populated id, so + // only a falsy-but-present id tells them apart: `??` keeps it and skips + // the getter, `||` falls through and pays for a read-back. + respondWith(createAgentActivityPayload(log, "")); + + const result = await sink.postActivity(mockSessionId, activity); + + expect(result).toEqual({}); + expect(log.operations).toEqual(["mutation:agentActivityCreate"]); + }); + + it("should return the id when the payload comes from the CLI issue-tracker adapter", async () => { + // CLIIssueTrackerService carries the id only on `agentActivity`, as an + // already-resolved promise property, and omits `agentActivityId`. Reading + // `agentActivityId` alone drops the id on that adapter. + respondWith(createCLIAdapterAgentActivityPayload("activity-cli")); + + const result = await sink.postActivity(mockSessionId, activity); + + expect(result).toEqual({ activityId: "activity-cli" }); + // Awaiting an already-resolved promise is not a round trip. + expect(log.operations).toEqual(["mutation:agentActivityCreate"]); + }); + it("should still return an empty result when the payload carries no activity id", async () => { respondWith({ success: true, diff --git a/packages/edge-worker/test/agent-activity-payload-double.ts b/packages/edge-worker/test/agent-activity-payload-double.ts index 4c8a65669..e6821761f 100644 --- a/packages/edge-worker/test/agent-activity-payload-double.ts +++ b/packages/edge-worker/test/agent-activity-payload-double.ts @@ -57,6 +57,31 @@ export function createAgentActivityPayload( }; } +/** + * Build a payload double shaped like the one CLIIssueTrackerService returns: + * + * return { + * agentActivity: Promise.resolve({ id: activityId }), + * success: true, + * lastSyncId: Date.now(), + * } as AgentActivityPayload; + * + * `agentActivity` is a plain, already-resolved promise *property* -- not the + * SDK's lazy getter -- and `agentActivityId` is absent entirely. The `as` cast + * in the adapter is why the compiler accepts the omission, so only a test can + * catch a caller that reads `agentActivityId` alone. + * + * Nothing is logged for the read here: awaiting a promise that is already + * resolved is not a round trip. The mutation remains the only operation. + */ +export function createCLIAdapterAgentActivityPayload(activityId: string) { + return { + agentActivity: Promise.resolve({ id: activityId }), + success: true, + lastSyncId: 1, + }; +} + /** * A `createAgentActivity` implementation that counts the mutation itself, so * the request count for a call under test is `log.operations.length`.