Skip to content

Commit 5ad8ca2

Browse files
committed
fix(survey): claim the cadence window before rendering the invite
Nothing locks next_ask, so two CLI processes that read it in the same instant can both see the gate open and both serve the invite. The write used to land after getRecipe and a getTelemetry await; it now lands the moment surveyGateIsOpen returns true, so the race is the width of one read-then-write rather than a render and a client init. Accepted tradeoff: the worst case is one duplicate invite and one duplicate survey shown, which the funnel over-counts by design. A lock file was rejected because it would need a TTL to survive a crashed process. The new test has the mocked getTelemetry read the cadence file when called and asserts it already holds the advanced value.
1 parent 28c2f6f commit 5ad8ca2

2 files changed

Lines changed: 33 additions & 3 deletions

File tree

‎packages/cli/src/survey/invite.ts‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,17 @@ export async function surveyGateIsOpen(
6060
* here too, at show time, so an invite the agent never surfaces still holds
6161
* the next one off: silence earns the short gap, an answer the long one.
6262
*
63+
* The cadence is written the moment the gate opens, before the fragment is
64+
* rendered and before the telemetry client is initialised. Nothing locks the
65+
* file, so two CLI processes that read `next_ask` in the same instant can both
66+
* see the gate open and both serve the invite. Writing first makes that window
67+
* the width of one read-then-write rather than a recipe render and a telemetry
68+
* init. The worst case is one duplicate invite and one duplicate `survey
69+
* shown`, and the funnel already over-counts shown by design, so this is an
70+
* accepted tradeoff. A lock file was considered and rejected: it would need a
71+
* TTL to survive a crashed process, which is more mechanism than one duplicate
72+
* invite earns.
73+
*
6374
* Appended after the recipe's last section rather than parsed into it. Agents
6475
* attend to the start and end of a response, and the end puts the ask after
6576
* the task rather than in front of it. The `prompts` export never sees this:
@@ -72,18 +83,22 @@ export async function withSurveyInvite(
7283
const recipe = context.recipe.trimEnd();
7384
if (!(await surveyGateIsOpen(context))) return recipe;
7485

86+
// Claim the window first; see the note above on the read-then-write race.
87+
const now = (context.now ?? Date.now)();
88+
await writeNextAsk(SURVEY_ID, now + SHOWN_INTERVAL_MS);
89+
7590
const invite = getRecipe(INVITE_TOPIC, {
7691
invocation: context.invocation,
7792
header: false,
7893
});
7994
// The fragment is embedded at build time; its absence is a build defect,
80-
// and serving the recipe without it is the right failure.
95+
// and serving the recipe without it is the right failure. The cadence has
96+
// already been advanced by then, which is fine: a missing fragment is not
97+
// a reason to ask again sooner.
8198
if (invite === undefined) return recipe;
8299

83100
const telemetry = await getTelemetry(context.cwd);
84101
telemetry.capture("survey shown", { $survey_id: SURVEY_ID });
85-
const now = (context.now ?? Date.now)();
86-
await writeNextAsk(SURVEY_ID, now + SHOWN_INTERVAL_MS);
87102

88103
return `${recipe}\n\n${invite.trimEnd()}`;
89104
}

‎packages/cli/test/survey-invite.test.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
77
import { getRecipe } from "../src/prompts/recipes";
88
import { nextAskPath, readNextAsk, writeNextAsk } from "../src/survey/cadence";
99
import { SHOWN_INTERVAL_MS, SURVEY_ID } from "../src/survey/constants";
10+
import { getTelemetry } from "../src/telemetry";
1011

1112
// Spy on telemetry by mocking the module the gate imports, the same way
1213
// agent-telemetry.test.ts does. `enabled` flips per test so the opt-out branch
@@ -92,6 +93,20 @@ describe("the survey gate", () => {
9293
expect(await readNextAsk(SURVEY_ID)).toBe(NOW + SHOWN_INTERVAL_MS);
9394
});
9495

96+
it("claims the cadence window before the telemetry client is initialised", async () => {
97+
// The mocked client reads the cadence file at the moment the gate asks
98+
// for it, so the assertion is about ordering, not the final state.
99+
let seenAtTelemetryInit: number | undefined;
100+
vi.mocked(getTelemetry).mockImplementationOnce(async () => {
101+
seenAtTelemetryInit = await readNextAsk(SURVEY_ID);
102+
return { capture, shutdown: () => Promise.resolve() };
103+
});
104+
105+
await serve("create-sg-rule");
106+
expect(capture).toHaveBeenCalledTimes(1);
107+
expect(seenAtTelemetryInit).toBe(NOW + SHOWN_INTERVAL_MS);
108+
});
109+
95110
it.each([
96111
"onboard",
97112
"create-sg-rule",

0 commit comments

Comments
 (0)