fix(claude-usage): review findings of the #52/#51/#53 stack #56

Merged
maximus merged 4 commits from issue-54-claude-usage-review-findings into main 2026-10-08 00:54:24 +00:00
4 changed files with 54 additions and 3 deletions
Showing only changes of commit e2a24d768b - Show all commits

View file

@ -736,6 +736,23 @@ describe("POST /claude-usage — payload", () => {
expect(JSON.parse(fs.readFileSync(usagePath, "utf-8")).claudeCodeVersion).toBe("2.1.284"); expect(JSON.parse(fs.readFileSync(usagePath, "utf-8")).claudeCodeVersion).toBe("2.1.284");
}); });
test("500 when the rename fails after the temp file is written, and the temp file is removed", async () => {
// The target is a non-empty directory: mkdir and the temp write succeed,
// renameSync fails. Only the cleanup in writeClaudeUsage() removes the
// temp file (#54: the mkdir case below never reaches it).
fs.mkdirSync(path.join(usagePath, "occupied"), { recursive: true });
const error = vi.spyOn(console, "error").mockImplementation(() => {});
try {
const res = await push();
expect(res.status).toBe(500);
expect(res.body.error).toBe("Internal error");
} finally {
error.mockRestore();
}
expect(fs.readdirSync(path.dirname(usagePath))).toEqual(["claude-usage.json"]);
expect(fs.readdirSync(usagePath)).toEqual(["occupied"]);
});
test("500 when the snapshot cannot be written, and no temp file is left", async () => { test("500 when the snapshot cannot be written, and no temp file is left", async () => {
// The parent of the file is a regular file: mkdir fails, nothing lands. // The parent of the file is a regular file: mkdir fails, nothing lands.
const blocker = path.join(tmpDir, "blocker"); const blocker = path.join(tmpDir, "blocker");
@ -842,6 +859,11 @@ describe("GET /claude-usage", () => {
["another schemaVersion", JSON.stringify({ ...validUsage(), schemaVersion: 2, receivedAt: "2026-09-27T19:00:00.000Z" })], ["another schemaVersion", JSON.stringify({ ...validUsage(), schemaVersion: 2, receivedAt: "2026-09-27T19:00:00.000Z" })],
["no receivedAt", JSON.stringify(validUsage())], ["no receivedAt", JSON.stringify(validUsage())],
["an unusable receivedAt", JSON.stringify({ ...validUsage(), receivedAt: "never" })], ["an unusable receivedAt", JSON.stringify({ ...validUsage(), receivedAt: "never" })],
// new Date() reads all three; the server only ever stamps a full ISO
// timestamp with an offset, so anything else was not written by it (#54).
["a receivedAt of \"1\"", JSON.stringify({ ...validUsage(), receivedAt: "1" })],
["a date-only receivedAt", JSON.stringify({ ...validUsage(), receivedAt: "2026-10-07" })],
["a receivedAt without an offset", JSON.stringify({ ...validUsage(), receivedAt: "2026-10-07T12:00:00" })],
])("on %s", async (_label, raw) => { ])("on %s", async (_label, raw) => {
writeStored(raw); writeStored(raw);
const res = await read(); const res = await read();

View file

@ -869,6 +869,19 @@ describe("POST /hosts/<id> — what actually lands on disk", () => {
expect(status).toBe(500); expect(status).toBe(500);
expect(body.error).toBe("Internal error"); expect(body.error).toBe("Internal error");
}); });
test("500 when the rename fails after the temp file is written, and the temp file is removed", async () => {
// The target is a non-empty directory: mkdir and the temp write succeed,
// renameSync fails. Only the cleanup in writeHostSnapshot() removes the
// temp file (#54: the case above fails at mkdir, before it exists).
fs.mkdirSync(path.join(hostsDir, "thinkpad.json", "occupied"), { recursive: true });
const { status, body } = await ingest("/hosts/thinkpad");
expect(status).toBe(500);
expect(body.error).toBe("Internal error");
expect(fs.readdirSync(hostsDir)).toEqual(["thinkpad.json"]);
expect(fs.readdirSync(path.join(hostsDir, "thinkpad.json"))).toEqual(["occupied"]);
});
}); });
describe("GET /hosts — freshness boundary", () => { describe("GET /hosts — freshness boundary", () => {
@ -1010,6 +1023,14 @@ describe("GET /hosts — listing semantics", () => {
"a receivedAt that is not a date", "a receivedAt that is not a date",
JSON.stringify({ id: "thinkpad", ...validSnapshot(), receivedAt: "yesterday" }), JSON.stringify({ id: "thinkpad", ...validSnapshot(), receivedAt: "yesterday" }),
], ],
// new Date() reads the next three; the server only ever stamps a full ISO
// timestamp with an offset, so anything else was not written by it (#54).
['a receivedAt of "1"', JSON.stringify({ id: "thinkpad", ...validSnapshot(), receivedAt: "1" })],
["a date-only receivedAt", JSON.stringify({ id: "thinkpad", ...validSnapshot(), receivedAt: "2026-10-07" })],
[
"a receivedAt without an offset",
JSON.stringify({ id: "thinkpad", ...validSnapshot(), receivedAt: "2026-10-07T12:00:00" }),
],
[ [
"a receivedAt that is not a string", "a receivedAt that is not a string",
JSON.stringify({ id: "thinkpad", ...validSnapshot(), receivedAt: 1700000000000 }), JSON.stringify({ id: "thinkpad", ...validSnapshot(), receivedAt: 1700000000000 }),

View file

@ -68,6 +68,13 @@ function cleanTimestamp(value) {
return value; return value;
} }
// Also used by index.js on the stored receivedAt of both snapshot files: the
// server only ever stamps Date#toISOString(), and new Date() alone would
// accept "1" or a bare date (#54).
function isIsoTimestamp(value) {
return cleanTimestamp(value) !== undefined;
}
// The calendar check catches what the regex lets through (2026-02-31). // The calendar check catches what the regex lets through (2026-02-31).
function cleanDayDate(value) { function cleanDayDate(value) {
if (typeof value !== "string" || !DAY_DATE_RE.test(value)) return undefined; if (typeof value !== "string" || !DAY_DATE_RE.test(value)) return undefined;
@ -176,6 +183,7 @@ function sanitizeClaudeUsage(payload) {
module.exports = { module.exports = {
sanitizeClaudeUsage, sanitizeClaudeUsage,
isIsoTimestamp,
CLAUDE_USAGE_MAX_BODY_BYTES, CLAUDE_USAGE_MAX_BODY_BYTES,
SCHEMA_VERSION, SCHEMA_VERSION,
MAX_DAYS, MAX_DAYS,

View file

@ -13,7 +13,7 @@ const {
const path = require("node:path"); const path = require("node:path");
const { createHash, timingSafeEqual } = require("node:crypto"); const { createHash, timingSafeEqual } = require("node:crypto");
const { collectMetrics } = require("./metrics.js"); const { collectMetrics } = require("./metrics.js");
const { sanitizeClaudeUsage, CLAUDE_USAGE_MAX_BODY_BYTES } = require("./claude-usage.js"); const { sanitizeClaudeUsage, isIsoTimestamp, CLAUDE_USAGE_MAX_BODY_BYTES } = require("./claude-usage.js");
const PORT = parseInt(process.env.PORT || "3001", 10); const PORT = parseInt(process.env.PORT || "3001", 10);
const TOKEN = process.env.HEALTH_TOKEN; const TOKEN = process.env.HEALTH_TOKEN;
@ -500,7 +500,7 @@ function readHostSnapshot(id) {
} }
const receivedAt = typeof parsed.receivedAt === "string" ? parsed.receivedAt : null; const receivedAt = typeof parsed.receivedAt === "string" ? parsed.receivedAt : null;
if (!receivedAt || Number.isNaN(new Date(receivedAt).getTime())) { if (!isIsoTimestamp(receivedAt)) {
console.error(`[hosts] snapshot for ${id} has no usable receivedAt`); console.error(`[hosts] snapshot for ${id} has no usable receivedAt`);
return null; return null;
} }
@ -554,7 +554,7 @@ function readClaudeUsage() {
} }
const receivedAt = typeof parsed.receivedAt === "string" ? parsed.receivedAt : null; const receivedAt = typeof parsed.receivedAt === "string" ? parsed.receivedAt : null;
if (!receivedAt || Number.isNaN(new Date(receivedAt).getTime())) { if (!isIsoTimestamp(receivedAt)) {
console.error("[claude-usage] snapshot has no usable receivedAt"); console.error("[claude-usage] snapshot has no usable receivedAt");
return null; return null;
} }