diff --git a/.github/workflows/canary-sunset.yml b/.github/workflows/canary-sunset.yml index a8ff59c55..5883f997b 100644 --- a/.github/workflows/canary-sunset.yml +++ b/.github/workflows/canary-sunset.yml @@ -2,9 +2,12 @@ # # Deliberately NOT a PR gate. The check reads the current date, so as a PR gate # it would fail builds for authors who touched nothing related, on a calendar -# date, with no fix available to them. On a schedule the failure lands on the -# rollout's owner instead, which is who can actually ramp it to 100 and delete -# the guard. +# date, with no fix available to them. On a schedule the failure stands on its +# own instead of blocking an unrelated author. +# +# The job names the overdue canary and its owner in the run log; it does not +# notify anyone. Routing that to the owner automatically (an issue, a ping) +# is worth doing and is not done here. name: Canary sunset permissions: diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index d1cb779da..cbb0408ad 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -56,7 +56,8 @@ call site. **4. Delete it** once it is at 100 and holding — both the registry entry and the branch it guarded. `sunsetAfter` exists to force this: the scheduled **Canary sunset** workflow runs weekly and fails once the date passes, naming -the rollout and its owner. +the overdue rollout and its owner in the run log. It does not notify anyone — +watch the workflow if you own a canary. It is a scheduled job rather than a PR check on purpose. A current-date assertion in the unit suite would redden builds for authors who changed diff --git a/packages/cli/src/server/studioServer.ts b/packages/cli/src/server/studioServer.ts index fced4070f..9acf70ae8 100644 --- a/packages/cli/src/server/studioServer.ts +++ b/packages/cli/src/server/studioServer.ts @@ -387,6 +387,10 @@ export function createStudioServer(options: StudioServerOptions): StudioServer { rendersDir: () => join(projectDir, "renders"), startRender(opts): RenderJobState { + // The render POST is a request boundary like any other. Without this an + // already-open Studio tab keeps rendering under the posture cached when + // the server booted. + refreshTelemetryPosture(); const abortController = new AbortController(); const state: RenderJobState = { id: opts.jobId, @@ -466,6 +470,12 @@ export function createStudioServer(options: StudioServerOptions): StudioServer { metaPath, JSON.stringify({ status: "complete", durationMs: Date.now() - startTime }), ); + // Refreshed HERE, not just at render start: a render can run for + // minutes, and `hyperframes telemetry disable` during one must be + // honoured by the event that reports it. Studio never polls + // /api/telemetry-identity, so this process would otherwise keep its + // startup-cached posture for the life of the preview server. + refreshTelemetryPosture(); emitStudioRenderComplete(opts, Date.now() - startTime, job.perfSummary); } catch (err) { if (abortController.signal.aborted) { @@ -475,6 +485,7 @@ export function createStudioServer(options: StudioServerOptions): StudioServer { state.status = "failed"; state.error = err instanceof Error ? err.message : String(err); // fallow-ignore-next-line code-duplication + refreshTelemetryPosture(); emitStudioRenderError(opts, Date.now() - startTime, state.stage, err, renderJob); try { const metaPath = opts.outputPath.replace(/\.(mp4|webm|mov)$/, ".meta.json"); diff --git a/packages/cli/src/server/telemetryIdentity.test.ts b/packages/cli/src/server/telemetryIdentity.test.ts index 7e0d126d9..5fb1b432d 100644 --- a/packages/cli/src/server/telemetryIdentity.test.ts +++ b/packages/cli/src/server/telemetryIdentity.test.ts @@ -11,11 +11,20 @@ const readConfig = vi.fn(); // don't move every time a canary is added, ramped, or retired. const canaryDecisions = vi.fn<() => Record>(); +// Every export the module under test imports must be mocked. Omitting +// `resetTelemetryPostureCache` / `readConfigFresh` made `refreshTelemetryPosture` +// throw a missing-export error that its own catch swallowed, so every +// assertion below ran against a refresh that silently did nothing. +const resetPostureCache = vi.fn(); +const readConfigFresh = vi.fn(); + vi.mock("../telemetry/client.js", () => ({ shouldTrack: (...args: unknown[]) => shouldTrack(...args), + resetTelemetryPostureCache: () => resetPostureCache(), })); vi.mock("../telemetry/config.js", () => ({ readConfig: (...args: unknown[]) => readConfig(...args), + readConfigFresh: () => readConfigFresh(), })); vi.mock("../telemetry/canary.js", () => ({ canaryDecisionsForStudio: () => canaryDecisions(), @@ -27,6 +36,7 @@ const { buildStudioHeadScripts, isLoopbackHost, buildStudioHeadScriptsForHost, + refreshTelemetryPosture, identityAllowed, } = await import("./telemetryIdentity.js"); @@ -344,3 +354,42 @@ describe("identityAllowed — loopback-bound vs explicitly LAN-bound", () => { }); }); }); + +// A long-lived preview server: the posture it cached at boot must not outlive +// an opt-out run in another terminal. Studio has no poller for +// /api/telemetry-identity, so the refresh has to happen on the paths that +// actually run — the SPA document and the render boundary. +describe("cross-process opt-out refresh", () => { + beforeEach(() => { + resetPostureCache.mockClear(); + readConfigFresh.mockClear(); + }); + + it("actually invalidates both caches — the mocks used to swallow this", () => { + refreshTelemetryPosture(); + expect(readConfigFresh).toHaveBeenCalledTimes(1); + expect(resetPostureCache).toHaveBeenCalledTimes(1); + }); + + it("refreshes before building a head script", () => { + shouldTrack.mockReturnValue(true); + readConfig.mockReturnValue({ anonymousId: "id-1", bucketSeed: "seed-1" }); + canaryDecisions.mockReturnValue({}); + buildStudioHeadScriptsForHost("", "localhost:3000"); + expect(resetPostureCache).toHaveBeenCalled(); + }); + + it("stops publishing identity once another process disables telemetry", () => { + canaryDecisions.mockReturnValue({}); + readConfig.mockReturnValue({ anonymousId: "id-1", bucketSeed: "seed-1" }); + + shouldTrack.mockReturnValue(true); + expect(buildStudioHeadScriptsForHost("", "localhost:3000")).toContain("__HF_CLI_DISTINCT_ID"); + + // `hyperframes telemetry disable` in another terminal. + shouldTrack.mockReturnValue(false); + const after = buildStudioHeadScriptsForHost("", "localhost:3000"); + expect(after).not.toContain("__HF_CLI_DISTINCT_ID"); + expect(after).not.toContain("__HF_CLI_BUCKET_SEED"); + }); +}); diff --git a/packages/cli/src/telemetry/client.postureRefresh.test.ts b/packages/cli/src/telemetry/client.postureRefresh.test.ts new file mode 100644 index 000000000..25f86c10c --- /dev/null +++ b/packages/cli/src/telemetry/client.postureRefresh.test.ts @@ -0,0 +1,72 @@ +import { describe, expect, it, vi, beforeEach } from "vitest"; + +// The suppression half of the cross-process opt-out. `hyperframes preview` is +// long-lived, and Studio has no poller for /api/telemetry-identity, so a +// render that finishes AFTER `hyperframes telemetry disable` ran in another +// terminal used to report its outcome anyway: shouldTrack() had cached `true` +// at boot and nothing ever asked again. +// +// This pins the mechanism the render-outcome path depends on — refresh, then +// emit — at the layer where the event is actually dropped. + +vi.stubEnv("HYPERFRAMES_NO_TELEMETRY", ""); +vi.stubEnv("DO_NOT_TRACK", ""); + +const configState = { telemetryEnabled: true }; +vi.mock("./config.js", () => ({ + readConfig: () => ({ anonymousId: "anon-1", telemetryEnabled: configState.telemetryEnabled }), + writeConfig: () => {}, +})); +vi.mock("../utils/env.js", () => ({ isDevMode: () => false })); +vi.mock("./canary.js", () => ({ canaryEventProperties: () => ({}) })); + +const enqueue = vi.fn(); +vi.mock("./transport.js", () => ({ + enqueue: (...args: unknown[]) => enqueue(...args), + flush: vi.fn(), + flushSync: vi.fn(), +})); + +const { trackEvent, resetTelemetryPostureCache, shouldTrack } = await import("./client.js"); + +beforeEach(() => { + configState.telemetryEnabled = true; + enqueue.mockClear(); + resetTelemetryPostureCache(); +}); + +describe("telemetry posture refresh", () => { + it("stops emitting once another process disables telemetry", () => { + trackEvent("render_complete", {}); + expect(enqueue).toHaveBeenCalledTimes(1); + + // `hyperframes telemetry disable` elsewhere, mid-render. + configState.telemetryEnabled = false; + resetTelemetryPostureCache(); + + trackEvent("render_complete", {}); + expect(enqueue, "outcome emitted after the user opted out").toHaveBeenCalledTimes(1); + }); + + // The memo is load-bearing for a CLI command — one process, one answer, asked + // once per event. Dropping it entirely would be a per-event config read. + it("still caches within a posture, so it is not a per-event disk read", () => { + expect(shouldTrack()).toBe(true); + configState.telemetryEnabled = false; + expect(shouldTrack(), "changed without an explicit refresh").toBe(true); + resetTelemetryPostureCache(); + expect(shouldTrack()).toBe(false); + }); + + it("re-enables after the user opts back in", () => { + configState.telemetryEnabled = false; + resetTelemetryPostureCache(); + trackEvent("render_complete", {}); + expect(enqueue).not.toHaveBeenCalled(); + + configState.telemetryEnabled = true; + resetTelemetryPostureCache(); + trackEvent("render_complete", {}); + expect(enqueue).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/cli/src/telemetry/config.test.ts b/packages/cli/src/telemetry/config.test.ts index a5e767440..d714d5ffc 100644 --- a/packages/cli/src/telemetry/config.test.ts +++ b/packages/cli/src/telemetry/config.test.ts @@ -276,6 +276,27 @@ describe("install-state rollover (breaker survives a config re-mint)", () => { expect(readConfigFresh().predecessorFound).toBeUndefined(); }); + // A full reset in a LONG-LIVED process. The memo that says "this process + // already mirrored the state file" stayed set after the file was deleted, so + // the mirror was never recreated: the freshly minted seed lived in + // config.json alone, and the NEXT config-only re-mint rolled a THIRD seed + // instead of inheriting the second. The old test stopped at the second mint + // and so missed the durability half entirely. + it("recreates install-state after a full wipe, so the new seed's lineage is durable", () => { + const first = readConfig().bucketSeed; + fsState.files.delete(CONFIG_PATH); + fsState.files.delete(STATE_PATH); + + const second = readConfigFresh().bucketSeed; + expect(second, "a full reset must mint a new cohort").not.toBe(first); + expect(fsState.files.has(STATE_PATH), "state file must be recreated").toBe(true); + expect(stateFile()["bucketSeed"]).toBe(second); + + // Config-only re-mint: the seed must now be inherited, not rolled again. + fsState.files.delete(CONFIG_PATH); + expect(readConfigFresh().bucketSeed, "lineage lost after reset").toBe(second); + }); + // The move: state used to live in ~/.local/state/hyperframes/ so it would // survive `rm -rf ~/.hyperframes`. Review rejected persisting state outside // the config dir to defeat the user's reset, so it now shares CONFIG_DIR. diff --git a/packages/cli/src/telemetry/config.ts b/packages/cli/src/telemetry/config.ts index 0b23eef9c..6048b6798 100644 --- a/packages/cli/src/telemetry/config.ts +++ b/packages/cli/src/telemetry/config.ts @@ -317,6 +317,16 @@ function applyInstallState(config: HyperframesConfig, wantFired: boolean): void function syncInstallState(config: HyperframesConfig): boolean { const wantFired = config.deParallelRouterTrialFired === true; + // The memo says "this process already wrote the state file". That is only + // true while the file is still there. `rm -rf ~/.hyperframes` under a + // long-lived preview left the memo set, so the mirror was never recreated: + // the freshly minted seed lived in config.json alone, and the NEXT + // config-only re-mint rolled a third seed instead of inheriting the second. + // One existsSync on a path we are about to write anyway. + if (stateMarkerSynced && !existsSync(STATE_FILE)) { + stateMarkerSynced = false; + stateFiredSynced = false; + } if (stateMarkerSynced && (stateFiredSynced || !wantFired)) return true; try { applyInstallState(config, wantFired); diff --git a/packages/core/src/canaryRegistry.ts b/packages/core/src/canaryRegistry.ts index f0ad7936c..a5a28fdac 100644 --- a/packages/core/src/canaryRegistry.ts +++ b/packages/core/src/canaryRegistry.ts @@ -38,8 +38,9 @@ export interface CanaryDefinition { /** * ISO date after which this canary is overdue for removal. A canary that * outlives its rollout is a permanent fork of the product with none of the - * review a permanent fork would have received. `assertNoOverdueCanaries` - * turns the date into a failing test rather than a good intention. + * review a permanent fork would have received. The scheduled `Canary sunset` + * workflow runs `scripts/check-canary-sunset.ts` weekly and fails once the + * date passes, so this is an enforced deadline rather than a good intention. */ sunsetAfter: string; }