-
Notifications
You must be signed in to change notification settings - Fork 454
fix: enforce 48h data retention on AIC usage cache entries #39084
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -20,6 +20,9 @@ const { getErrorMessage } = require("./error_helpers.cjs"); | |
| /** Path where the restored (and updated) usage cache lives on the runner. */ | ||
| const CACHE_FILE_PATH = "/tmp/gh-aw/agentic-workflow-usage-cache.jsonl"; | ||
|
|
||
| /** Entries older than this threshold (in ms) are pruned when rewriting the cache. */ | ||
| const CACHE_RETENTION_MS = 48 * 60 * 60 * 1000; | ||
|
|
||
| /** | ||
| * Directory prepared by the "Collect usage artifact files" step in the conclusion job. | ||
| * Contains agent_usage.jsonl and agent/token_usage.jsonl which mirror the contents of | ||
|
|
@@ -47,12 +50,18 @@ function logCache(message, details) { | |
| } | ||
|
|
||
| /** | ||
| * Appends a `{run_id, aic}` JSONL entry to the cache file, preserving any existing entries | ||
| * that were restored from the previous cache snapshot. | ||
| * Appends a `{run_id, aic, timestamp}` JSONL entry to the cache file, preserving any existing | ||
| * entries that were restored from the previous cache snapshot and are within the 48-hour | ||
| * retention window. Entries older than {@link CACHE_RETENTION_MS} are pruned to keep the | ||
| * cache file bounded. | ||
| * | ||
| * @param {string} [cacheFilePath] Override the cache file path (defaults to {@link CACHE_FILE_PATH}; useful in tests). | ||
| * @param {string} [usageDir] Override the usage directory (defaults to {@link USAGE_DIR}; useful in tests). | ||
| * @returns {Promise<void>} | ||
| */ | ||
| async function main() { | ||
| async function mainWithPaths(cacheFilePath, usageDir) { | ||
| const cachePath = cacheFilePath || CACHE_FILE_PATH; | ||
| const usageDirPath = usageDir || USAGE_DIR; | ||
| try { | ||
| const runId = Number(process.env.GITHUB_RUN_ID || 0); | ||
| if (!runId) { | ||
|
|
@@ -61,8 +70,8 @@ async function main() { | |
| } | ||
|
|
||
| // Compute AIC from the usage JSONL files prepared by buildUsageArtifactUploadSteps. | ||
| const usageFiles = findJSONLFiles(USAGE_DIR); | ||
| logCache("Scanning usage JSONL files", { dir: USAGE_DIR, count: usageFiles.length, files: usageFiles }); | ||
| const usageFiles = findJSONLFiles(usageDirPath); | ||
| logCache("Scanning usage JSONL files", { dir: usageDirPath, count: usageFiles.length, files: usageFiles }); | ||
| const aic = sumAICFromUsageJSONLFiles(usageFiles); | ||
| logCache("Computed AIC for current run", { runId, aic }); | ||
|
|
||
|
|
@@ -76,32 +85,67 @@ async function main() { | |
| } | ||
|
|
||
| // Read existing cache content (restored from the previous run's cache snapshot, if any). | ||
| let existingLines = ""; | ||
| // Entries with a `timestamp` older than CACHE_RETENTION_MS are pruned to keep the file | ||
| // bounded. Entries without a `timestamp` (written by an older version of this script) | ||
|
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. [/zoom-out] No-timestamp entries are preserved forever — they are never pruned by the write path, and never skipped by the read path. This is intentional for migration safety, but it means caches that existed before this PR deployed will carry unbounded legacy entries until GitHub Actions expires the cache key naturally (typically 7 days). 💡 SuggestionConsider documenting the expected convergence timeline in the comment, e.g.: // Entries without a `timestamp` (written by an older version of this script before YYYY-MM-DD)
// are preserved indefinitely until the GitHub Actions cache key expires (~7 days).
// After all active cache files have cycled through at least one write with the new code,
// every entry will have a timestamp and pruning will be fully enforced.This helps future maintainers understand when the backward-compat clause can safely be removed. |
||
| // are preserved for backward compatibility. | ||
| /** @type {string[]} */ | ||
| let keptLines = []; | ||
| try { | ||
| if (fs.existsSync(CACHE_FILE_PATH)) { | ||
| existingLines = fs.readFileSync(CACHE_FILE_PATH, "utf8").trimEnd(); | ||
| const lineCount = existingLines ? existingLines.split("\n").length : 0; | ||
| logCache("Loaded existing cache entries", { path: CACHE_FILE_PATH, lineCount }); | ||
| if (fs.existsSync(cachePath)) { | ||
| const raw = fs.readFileSync(cachePath, "utf8").trimEnd(); | ||
| const now = Date.now(); | ||
| const cutoff = now - CACHE_RETENTION_MS; | ||
| let total = 0; | ||
| let pruned = 0; | ||
| for (const rawLine of raw.split("\n")) { | ||
| const line = rawLine.trim(); | ||
| if (!line) continue; | ||
| total++; | ||
| try { | ||
| const entry = JSON.parse(line); | ||
| if (typeof entry?.timestamp === "string") { | ||
| const ts = Date.parse(entry.timestamp); | ||
| if (Number.isFinite(ts) && ts < cutoff) { | ||
| pruned++; | ||
| continue; | ||
| } | ||
| } | ||
| keptLines.push(line); | ||
| } catch { | ||
| // Preserve lines that cannot be parsed (defensive: avoids data loss). | ||
| keptLines.push(line); | ||
| } | ||
| } | ||
| logCache("Loaded existing cache entries", { path: cachePath, total, kept: keptLines.length, pruned }); | ||
| } else { | ||
| logCache("No existing cache file found; starting fresh", { path: CACHE_FILE_PATH }); | ||
| logCache("No existing cache file found; starting fresh", { path: cachePath }); | ||
| } | ||
| } catch (readErr) { | ||
| core.warning(`[daily-aic-cache] Could not read existing cache file: ${getErrorMessage(readErr)}`); | ||
| } | ||
|
|
||
| // Build the updated JSONL content. | ||
| const newEntry = JSON.stringify({ run_id: runId, aic }); | ||
| const updatedContent = existingLines ? `${existingLines}\n${newEntry}\n` : `${newEntry}\n`; | ||
| const newEntry = JSON.stringify({ run_id: runId, aic, timestamp: new Date().toISOString() }); | ||
| const updatedContent = keptLines.length > 0 ? `${keptLines.join("\n")}\n${newEntry}\n` : `${newEntry}\n`; | ||
|
|
||
| // Ensure the directory exists and write the updated file. | ||
| const dir = path.dirname(CACHE_FILE_PATH); | ||
| const dir = path.dirname(cachePath); | ||
| fs.mkdirSync(dir, { recursive: true }); | ||
| fs.writeFileSync(CACHE_FILE_PATH, updatedContent, "utf8"); | ||
| logCache("Wrote cache entry", { runId, aic, path: CACHE_FILE_PATH }); | ||
| fs.writeFileSync(cachePath, updatedContent, "utf8"); | ||
| logCache("Wrote cache entry", { runId, aic, path: cachePath }); | ||
| } catch (error) { | ||
| // Non-fatal: a cache write failure should never block the conclusion job. | ||
| core.warning(`[daily-aic-cache] Failed to write usage cache: ${getErrorMessage(error)}`); | ||
| } | ||
| } | ||
|
|
||
| module.exports = { main }; | ||
| /** | ||
| * Entry point called from the GitHub Actions step. | ||
| * | ||
| * @returns {Promise<void>} | ||
| */ | ||
| async function main() { | ||
| return mainWithPaths(); | ||
| } | ||
|
|
||
| module.exports = { main, mainWithPaths }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| // @ts-check | ||
| import fs from "fs"; | ||
| import os from "os"; | ||
| import path from "path"; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | ||
|
|
||
| let exports; | ||
|
|
||
| describe("write_daily_aic_usage_cache", () => { | ||
| let tmpDir; | ||
| let cacheFile; | ||
| let usageDir; | ||
|
|
||
| beforeEach(async () => { | ||
| vi.resetModules(); | ||
| tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "write-aic-cache-test-")); | ||
| cacheFile = path.join(tmpDir, "agentic-workflow-usage-cache.jsonl"); | ||
| usageDir = path.join(tmpDir, "usage"); | ||
| fs.mkdirSync(usageDir, { recursive: true }); | ||
|
|
||
| global.core = { info: vi.fn(), warning: vi.fn(), error: vi.fn(), setFailed: vi.fn() }; | ||
| process.env.GITHUB_RUN_ID = "12345"; | ||
|
|
||
| const mod = await import("./write_daily_aic_usage_cache.cjs"); | ||
| exports = mod.default || mod; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| fs.rmSync(tmpDir, { recursive: true, force: true }); | ||
| delete global.core; | ||
| delete process.env.GITHUB_RUN_ID; | ||
| }); | ||
|
|
||
| /** | ||
| * Writes a usage JSONL file in the temp usage directory so that sumAICFromUsageJSONLFiles | ||
| * can pick it up. The entry uses the `ai_credits` field for simplicity. | ||
| * | ||
| * @param {number} aiCredits | ||
| */ | ||
| function writeUsageFile(aiCredits) { | ||
| fs.writeFileSync(path.join(usageDir, "agent_usage.jsonl"), JSON.stringify({ ai_credits: aiCredits }) + "\n", "utf8"); | ||
| } | ||
|
|
||
| /** | ||
| * Invoke the main() function after patching the module-level paths to point to the | ||
| * temp directory. We call the function directly on the in-process module instance. | ||
| * | ||
| * @returns {Promise<void>} | ||
| */ | ||
| async function runMain() { | ||
| // Patch the module-level path constants by calling main() on the already-imported | ||
| // module; to make the paths configurable we call the exported helper that accepts | ||
| // explicit path arguments (defined below), falling back to swapping env vars. | ||
|
Comment on lines
+51
to
+53
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. [/tdd] The comment "falling back to swapping env vars" describes a strategy that does not exist in this implementation. Suggest simplifying the comment: async function runMain() {
await exports.mainWithPaths(cacheFile, usageDir);
} |
||
| await exports.mainWithPaths(cacheFile, usageDir); | ||
| } | ||
|
|
||
| it("writes a new entry with run_id, aic, and a timestamp when no cache file exists", async () => { | ||
| writeUsageFile(7.5); | ||
| await runMain(); | ||
|
|
||
| const content = fs.readFileSync(cacheFile, "utf8").trim(); | ||
| const entry = JSON.parse(content); | ||
| expect(entry.run_id).toBe(12345); | ||
| expect(entry.aic).toBe(7.5); | ||
| expect(typeof entry.timestamp).toBe("string"); | ||
| // Timestamp should be a valid ISO 8601 date within the last minute. | ||
| const ts = Date.parse(entry.timestamp); | ||
| expect(Number.isFinite(ts)).toBe(true); | ||
| expect(ts).toBeLessThanOrEqual(Date.now()); | ||
| expect(ts).toBeGreaterThan(Date.now() - 60_000); | ||
| }); | ||
|
Comment on lines
+57
to
+71
|
||
|
|
||
| it("appends to an existing cache file and preserves entries within 48 h", async () => { | ||
| const recentTs = new Date(Date.now() - 2 * 60 * 60 * 1000).toISOString(); // 2 hours ago | ||
| fs.writeFileSync(cacheFile, JSON.stringify({ run_id: 9999, aic: 3.0, timestamp: recentTs }) + "\n", "utf8"); | ||
|
|
||
| writeUsageFile(5.0); | ||
| await runMain(); | ||
|
|
||
| const lines = fs.readFileSync(cacheFile, "utf8").trim().split("\n"); | ||
| expect(lines).toHaveLength(2); | ||
| const first = JSON.parse(lines[0]); | ||
| expect(first.run_id).toBe(9999); | ||
| const second = JSON.parse(lines[1]); | ||
| expect(second.run_id).toBe(12345); | ||
| expect(second.aic).toBe(5.0); | ||
| }); | ||
|
|
||
| it("prunes existing entries whose timestamp is older than 48 h", async () => { | ||
| const staleTs = new Date(Date.now() - 50 * 60 * 60 * 1000).toISOString(); // 50 hours ago | ||
| const recentTs = new Date(Date.now() - 1 * 60 * 60 * 1000).toISOString(); // 1 hour ago | ||
| fs.writeFileSync(cacheFile, [JSON.stringify({ run_id: 1001, aic: 1.0, timestamp: staleTs }), JSON.stringify({ run_id: 1002, aic: 2.0, timestamp: recentTs })].join("\n") + "\n", "utf8"); | ||
|
|
||
| writeUsageFile(4.0); | ||
| await runMain(); | ||
|
|
||
| const lines = fs.readFileSync(cacheFile, "utf8").trim().split("\n"); | ||
| const runIds = lines.map(line => JSON.parse(line).run_id); | ||
| // Stale entry 1001 must be pruned; recent 1002 and new 12345 kept. | ||
| expect(runIds).not.toContain(1001); | ||
| expect(runIds).toContain(1002); | ||
| expect(runIds).toContain(12345); | ||
| }); | ||
|
|
||
| it("preserves entries without a timestamp (backward compatibility)", async () => { | ||
| fs.writeFileSync(cacheFile, JSON.stringify({ run_id: 2001, aic: 8.0 }) + "\n", "utf8"); | ||
|
|
||
| writeUsageFile(1.0); | ||
| await runMain(); | ||
|
|
||
| const lines = fs.readFileSync(cacheFile, "utf8").trim().split("\n"); | ||
| const runIds = lines.map(line => JSON.parse(line).run_id); | ||
| expect(runIds).toContain(2001); | ||
| expect(runIds).toContain(12345); | ||
| }); | ||
|
|
||
| it("skips writing when GITHUB_RUN_ID is not set", async () => { | ||
| delete process.env.GITHUB_RUN_ID; | ||
| writeUsageFile(5.0); | ||
| await runMain(); | ||
| expect(fs.existsSync(cacheFile)).toBe(false); | ||
|
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. [/tdd] The test verifies the cache file is not created, but does not assert that 💡 Suggested additionit("skips writing when GITHUB_RUN_ID is not set", async () => {
delete process.env.GITHUB_RUN_ID;
writeUsageFile(5.0);
await runMain();
expect(fs.existsSync(cacheFile)).toBe(false);
expect(global.core.warning).toHaveBeenCalledWith(
expect.stringContaining("GITHUB_RUN_ID not set")
);
}); |
||
| }); | ||
|
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. [/tdd] Two edge cases are missing from the test suite:
💡 Suggested testsit("keeps entries with an unparseable timestamp (treats as no-timestamp)", async () => {
fs.writeFileSync(cacheFile, JSON.stringify({ run_id: 9001, aic: 1.0, timestamp: "not-a-date" }) + "\n", "utf8");
writeUsageFile(0);
await runMain();
const lines = fs.readFileSync(cacheFile, "utf8").trim().split("\n");
expect(lines.map(l => JSON.parse(l).run_id)).toContain(9001);
});
it("keeps an entry at exactly the 48 h cutoff boundary", async () => {
const boundaryTs = new Date(Date.now() - 48 * 60 * 60 * 1000).toISOString();
fs.writeFileSync(cacheFile, JSON.stringify({ run_id: 9002, aic: 2.0, timestamp: boundaryTs }) + "\n", "utf8");
writeUsageFile(0);
await runMain();
const lines = fs.readFileSync(cacheFile, "utf8").trim().split("\n");
expect(lines.map(l => JSON.parse(l).run_id)).toContain(9002);
}); |
||
| }); | ||
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.
[/diagnose]
CACHE_RETENTION_MSis defined identically in bothcheck_daily_aic_workflow_guardrail.cjsandwrite_daily_aic_usage_cache.cjs. If the retention window changes, both files must be updated atomically — easy to miss.💡 Suggestion
Extract the constant into a shared module (e.g.
daily_aic_constants.cjs) andrequireit in both files:This also makes it clear that both the write and read paths are intentionally coupled to the same window.