Files
PaperClipAI/server/src/services/run-log-store.test.ts
T
Devin FoleyandBender de9414dd8b fix: return empty read instead of past-EOF range when log reader is caught up (#13592)
## Thinking Path

> - Paperclip is the open source app people use to manage AI agents for
work
> - The server streams agent run logs to the UI. It reads the log in
byte ranges from a local file, or from an S3 mirror after a pod restart.
> - The range math in `readS3Range()` clamps the range end up to the
range start. A fully caught-up reader then asks S3 for the range
`bytes=total-total`.
> - S3 rejects a range that starts at the end of the object. It returns
a 416 `InvalidRange` error. The API turns this into a 500 error, and the
log poller repeats it.
> - This pull request removes the clamp. A caught-up or past-EOF reader
now gets an empty read, and the server does not send an invalid range to
S3.
> - The benefit is that log polling after a pod restart does not cause
repeated 500 errors.

## Linked Issues or Issue Description

No public GitHub issue exists for this bug. The description follows the
bug report template.

**What happened?**

The run-log API returns a 500 error when a client polls a run log that
lives on the S3 mirror and the client is fully caught up (`offset ===
total`). The cause is in `server/src/services/run-log-store.ts`. The
function `readS3Range()` computes `end = Math.max(start, Math.min(start
+ limitBytes - 1, total - 1))`. When `offset === total`, the
`Math.max(start, …)` clamp forces `end` up to `start`. The `start > end`
empty-read guard does not operate, and the code sends `Range:
bytes=total-total` to S3. S3 rejects a range that starts at or past the
end of the object with a 416 `InvalidRange` error. The error monitor
records this error many times, only in the staging environment, because
only the S3 fallback path is sensitive to it. The local-file path has
the same math, but Node file streams accept past-EOF reads. The function
`readFileRange()` in
`server/src/services/workspace-operation-log-store.ts` has the same
latent math.

**Expected behavior**

A caught-up reader gets an empty read: `{ content: "", nextOffset:
undefined }`. The server does not send an invalid range request to S3.
The poller sees no contract change.

**Steps to reproduce**

1. Start a run and let it write a run log.
2. Let the log upload to the S3 mirror, and remove the local file (this
occurs when the pod restarts).
3. Poll the run-log read endpoint until the client offset is equal to
the log size.
4. Poll one more time. The server sends `bytes=total-total` to S3, S3
returns 416 `InvalidRange`, and the API returns a 500 error.

**Relevant logs or output**

```
InvalidRange: Invalid range
    at readS3Range (server/src/services/run-log-store.ts)
```

## What Changed

- `server/src/services/run-log-store.ts` — remove the up-clamp in
`readLocalRange()` and `readS3Range()`. A caught-up or past-EOF reader
gets an empty read.
- `server/src/services/workspace-operation-log-store.ts` — apply the
same fix to the shared math in `readFileRange()`.
- `server/src/services/run-log-store.test.ts` — the in-memory S3 mock
now rejects past-EOF ranges with `InvalidRange`, the same as real S3.
Add two regression tests for caught-up readers on the S3 path and on the
local path.

## Verification

- Run `pnpm vitest run server/src/services/run-log-store.test.ts`. All
17 tests pass.
- Revert only the source fix, and the new regression test fails with the
exact caught-up scenario. This shows the test covers the bug.
- Run the suites that use the workspace operation log store
(`workspace-runtime-control-recovery`,
`workspace-operations-reconciliation`). All 15 tests pass.
- Run `tsc --noEmit` on the server package. It reports no errors.

## Risks

- Low risk. The change only affects the empty and caught-up boundary of
range reads. Normal in-range reads give byte-identical results.
- Behavior change: a read with `limitBytes <= 0` now returns an empty
chunk instead of one byte. No caller passes a non-positive limit (the
default is 256000).
- Caught-up local reads keep the `nextOffset: undefined` semantics, so
pollers see no contract change.

## Model Used

- Claude Fable 5 (Anthropic, model ID `claude-fable-5`), with extended
thinking and tool use, run through the Claude Agent SDK.

## Checklist

- [x] I have included a thinking path that traces from project context
to this change
- [x] I have specified the model used (with version and capability
details)
- [x] I have checked ROADMAP.md and confirmed this PR does not duplicate
planned core work
- [x] I have searched GitHub for duplicate or related PRs and linked
them above
- [x] I have either (a) linked existing issues with `Fixes: #` / `Closes
#` / `Refs #` OR (b) described the issue in-PR following the relevant
issue template
- [x] I have not referenced internal/instance-local Paperclip issues or
links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip`
URLs)
- [ ] My branch name describes the change (e.g. `docs/...`, `fix/...`)
and contains no internal Paperclip ticket id or instance-derived details
- [x] I have run tests locally and they pass
- [x] I have added or updated tests where applicable
- [x] I have updated relevant documentation to reflect my changes
- [x] I have considered and documented any risks above
- [ ] All Paperclip CI gates are green
- [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups
- [x] I will address all Greptile and reviewer comments before
requesting merge

Co-authored-by: Bender (Fable) <noreply@paperclip.ing>
2026-09-18 07:21:11 -07:00

380 lines
18 KiB
TypeScript

import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { promises as fs } from "node:fs";
import path from "node:path";
import os from "node:os";
import { Readable } from "node:stream";
import { createDurableRunLogStore } from "./run-log-store.js";
import type { StorageProvider } from "../storage/types.js";
// In-memory StorageProvider stand-in: durable, survives the "pod roll" (local
// dir wipe) the same way Cubbit does. Records calls so we can assert behaviour.
function createMemoryProvider() {
const objects = new Map<string, Buffer>();
const calls = { put: 0, get: 0, head: 0 };
const provider: StorageProvider = {
id: "s3",
async putObject(input) {
calls.put++;
if (Buffer.isBuffer(input.body)) {
objects.set(input.objectKey, Buffer.from(input.body));
return;
}
const chunks: Buffer[] = [];
for await (const chunk of input.body) {
chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk));
}
objects.set(input.objectKey, Buffer.concat(chunks));
},
async getObject(input) {
calls.get++;
const buf = objects.get(input.objectKey);
if (!buf) {
const err = new Error("Object not found") as Error & { name: string };
err.name = "NoSuchKey";
throw err;
}
// Real S3 answers 416 when the range starts at or past EOF — the mock
// must too, or caught-up-reader regressions (PAPERCLIP-BACKEND-9) pass
// silently with an empty slice.
if (input.range && (input.range.start >= buf.length || input.range.start > input.range.end)) {
const err = new Error("Invalid range") as Error & { name: string };
err.name = "InvalidRange";
throw err;
}
const slice = input.range ? buf.subarray(input.range.start, input.range.end + 1) : buf;
return { stream: Readable.from(slice), contentLength: slice.length };
},
async headObject(input) {
calls.head++;
const buf = objects.get(input.objectKey);
return buf ? { exists: true, contentLength: buf.length } : { exists: false };
},
async deleteObject(input) {
objects.delete(input.objectKey);
},
};
return { provider, objects, calls };
}
let baseDir: string;
beforeEach(async () => {
baseDir = await fs.mkdtemp(path.join(os.tmpdir(), "run-log-store-test-"));
});
afterEach(async () => {
await fs.rm(baseDir, { recursive: true, force: true });
});
const begin = { companyId: "co1", agentId: "ag1", runId: "run1" };
describe("createDurableRunLogStore", () => {
it("keeps store id 'local_file' so downstream coupling (feedback, casts) is unchanged", async () => {
const { provider } = createMemoryProvider();
const store = createDurableRunLogStore({ basePath: baseDir, s3: { provider } });
const handle = await store.begin(begin);
expect(handle.store).toBe("local_file");
});
it("appends locally and reads back during a run WITHOUT hitting S3 (fast live tail)", async () => {
const { provider, calls } = createMemoryProvider();
const store = createDurableRunLogStore({ basePath: baseDir, s3: { provider } });
const handle = await store.begin(begin);
await store.append(handle, { stream: "stdout", chunk: "hello", ts: "t1" });
const res = await store.read(handle);
expect(res.content).toContain("hello");
expect(calls.get).toBe(0); // local file present -> no S3 read
});
it("uploads the complete log to S3 on finalize", async () => {
const { provider, objects, calls } = createMemoryProvider();
const store = createDurableRunLogStore({ basePath: baseDir, s3: { provider, keyPrefix: "run-logs" } });
const handle = await store.begin(begin);
await store.append(handle, { stream: "stdout", chunk: "line-A", ts: "t1" });
await store.append(handle, { stream: "stdout", chunk: "line-B", ts: "t2" });
const summary = await store.finalize(handle);
expect(calls.put).toBe(1);
expect(summary.bytes).toBeGreaterThan(0);
// keyed by prefix + the handle's logRef so read can find it later
const key = `run-logs/${handle.logRef}`;
expect(objects.has(key)).toBe(true);
expect(objects.get(key)!.toString("utf8")).toContain("line-A");
expect(objects.get(key)!.toString("utf8")).toContain("line-B");
});
it("falls back to S3 when the local file is gone (the pod-roll case that caused 'Run log not found')", async () => {
const { provider } = createMemoryProvider();
const store = createDurableRunLogStore({ basePath: baseDir, s3: { provider, keyPrefix: "run-logs" } });
const handle = await store.begin(begin);
await store.append(handle, { stream: "stdout", chunk: "persisted-line", ts: "t1" });
await store.finalize(handle);
// Simulate a pod restart wiping the emptyDir.
await fs.rm(baseDir, { recursive: true, force: true });
const res = await store.read(handle);
expect(res.content).toContain("persisted-line");
});
it("S3 fallback honours offset/limitBytes (range read) and reports nextOffset", async () => {
const { provider } = createMemoryProvider();
const store = createDurableRunLogStore({ basePath: baseDir, s3: { provider, keyPrefix: "p" } });
const handle = await store.begin(begin);
// one line; persisted bytes = JSON line + "\n"
await store.append(handle, { stream: "stdout", chunk: "0123456789", ts: "t" });
await store.finalize(handle);
const full = await store.read(handle); // from local, to learn total size
const total = Buffer.byteLength(full.content, "utf8");
await fs.rm(baseDir, { recursive: true, force: true }); // force S3 path
const firstHalf = await store.read(handle, { offset: 0, limitBytes: 5 });
expect(Buffer.byteLength(firstHalf.content, "utf8")).toBe(5);
expect(firstHalf.nextOffset).toBe(5);
const tail = await store.read(handle, { offset: total - 3, limitBytes: 100 });
expect(Buffer.byteLength(tail.content, "utf8")).toBe(3);
expect(tail.nextOffset).toBeUndefined();
});
it("S3 fallback returns an empty read (no InvalidRange) when the reader is fully caught up", async () => {
// Regression test for PAPERCLIP-BACKEND-9: a poller that had consumed the
// whole log kept polling at offset === total after the pod rolled; the S3
// path clamped end up to start and requested `bytes=total-total`, which
// S3 rejects with 416 InvalidRange -> 500.
const { provider } = createMemoryProvider();
const store = createDurableRunLogStore({ basePath: baseDir, s3: { provider, keyPrefix: "p" } });
const handle = await store.begin(begin);
await store.append(handle, { stream: "stdout", chunk: "0123456789", ts: "t" });
await store.finalize(handle);
const full = await store.read(handle);
const total = Buffer.byteLength(full.content, "utf8");
await fs.rm(baseDir, { recursive: true, force: true }); // force S3 path
const caughtUp = await store.read(handle, { offset: total, limitBytes: 100 });
expect(caughtUp.content).toBe("");
expect(caughtUp.nextOffset).toBeUndefined();
// Past-EOF offsets clamp to the same empty read.
const pastEof = await store.read(handle, { offset: total + 50, limitBytes: 100 });
expect(pastEof.content).toBe("");
expect(pastEof.nextOffset).toBeUndefined();
});
it("local read at offset === size returns an empty read with undefined nextOffset", async () => {
const store = createDurableRunLogStore({ basePath: baseDir });
const handle = await store.begin(begin);
await store.append(handle, { stream: "stdout", chunk: "abc", ts: "t" });
const full = await store.read(handle);
const total = Buffer.byteLength(full.content, "utf8");
const caughtUp = await store.read(handle, { offset: total, limitBytes: 100 });
expect(caughtUp.content).toBe("");
expect(caughtUp.nextOffset).toBeUndefined();
});
it("falls back to S3 when the local file vanishes between stat() and open (TOCTOU race)", async () => {
const { provider } = createMemoryProvider();
const store = createDurableRunLogStore({ basePath: baseDir, s3: { provider, keyPrefix: "run-logs" } });
const handle = await store.begin(begin);
await store.append(handle, { stream: "stdout", chunk: "raced-line", ts: "t1" });
await store.finalize(handle);
// Delete the local file DURING stat(), i.e. after it reports the file
// present but before createReadStream opens it -> the open hits ENOENT.
const realStat = fs.stat.bind(fs);
const statSpy = vi.spyOn(fs, "stat").mockImplementation(async (target, ...rest) => {
const result = await realStat(target as Parameters<typeof realStat>[0], ...(rest as []));
if (String(target).endsWith(".ndjson")) {
await fs.rm(target as string, { force: true });
}
return result;
});
try {
const res = await store.read(handle);
expect(res.content).toContain("raced-line");
} finally {
statSpy.mockRestore();
}
});
it("throws notFound when neither local nor S3 has the log (pre-S3 run after a roll)", async () => {
const { provider } = createMemoryProvider();
const store = createDurableRunLogStore({ basePath: baseDir, s3: { provider } });
const handle = await store.begin(begin);
await fs.rm(baseDir, { recursive: true, force: true }); // never finalized -> never uploaded
await expect(store.read(handle)).rejects.toThrow(/not found/i);
});
it("without S3 configured behaves exactly like the local-only store (safe degrade)", async () => {
const store = createDurableRunLogStore({ basePath: baseDir });
const handle = await store.begin(begin);
await store.append(handle, { stream: "stdout", chunk: "local-only", ts: "t1" });
await store.finalize(handle);
const res = await store.read(handle);
expect(res.content).toContain("local-only");
// and a roll loses it (documented limitation; this is the pre-fix behaviour)
await fs.rm(baseDir, { recursive: true, force: true });
await expect(store.read(handle)).rejects.toThrow(/not found/i);
});
});
describe("in-flight mirror", () => {
it("is OFF by default: appends never upload before finalize", async () => {
const { provider, calls } = createMemoryProvider();
const store = createDurableRunLogStore({ basePath: baseDir, s3: { provider } });
const handle = await store.begin(begin);
await store.append(handle, { stream: "stdout", chunk: "tail-1", ts: "t1" });
await store.append(handle, { stream: "stdout", chunk: "tail-2", ts: "t2" });
await new Promise((resolve) => setTimeout(resolve, 50));
expect(calls.put).toBe(0);
// flush is a no-op with the mirror off
await store.flushInflightMirrors?.();
expect(calls.put).toBe(0);
});
it("mirrors the running log after the interval so a wipe without finalize preserves the tail", async () => {
const { provider, objects } = createMemoryProvider();
const store = createDurableRunLogStore({
basePath: baseDir,
s3: { provider, keyPrefix: "run-logs", inflightMirrorMs: 10 },
});
const handle = await store.begin(begin);
await store.append(handle, { stream: "stdout", chunk: "crash-tail", ts: "t1" });
const key = `run-logs/${handle.logRef}`;
await vi.waitFor(() => expect(objects.has(key)).toBe(true), { timeout: 2000 });
// Simulate a crash mid-run: local dir wiped, run never finalized.
await fs.rm(baseDir, { recursive: true, force: true });
const res = await store.read(handle);
expect(res.content).toContain("crash-tail");
});
it("throttles: rapid appends coalesce into one upload, delivered by flush", async () => {
const { provider, objects, calls } = createMemoryProvider();
// Interval far beyond the test's lifetime: nothing may upload until the
// flush, and the flush must carry every appended line in ONE put.
const store = createDurableRunLogStore({
basePath: baseDir,
s3: { provider, keyPrefix: "run-logs", inflightMirrorMs: 60_000 },
});
const handle = await store.begin(begin);
for (let i = 0; i < 5; i++) {
await store.append(handle, { stream: "stdout", chunk: `line-${i}`, ts: `t${i}` });
}
await new Promise((resolve) => setTimeout(resolve, 50));
expect(calls.put).toBe(0); // throttled: no per-append PUT
await store.flushInflightMirrors?.();
expect(calls.put).toBe(1);
const body = objects.get(`run-logs/${handle.logRef}`)!.toString("utf8");
for (let i = 0; i < 5; i++) expect(body).toContain(`line-${i}`);
// Nothing dirty afterwards: flushing again uploads nothing.
await store.flushInflightMirrors?.();
expect(calls.put).toBe(1);
});
it("finalize supersedes the in-flight mirror and retires its timer", async () => {
const { provider, objects, calls } = createMemoryProvider();
const store = createDurableRunLogStore({
basePath: baseDir,
s3: { provider, keyPrefix: "run-logs", inflightMirrorMs: 10 },
});
const handle = await store.begin(begin);
await store.append(handle, { stream: "stdout", chunk: "early-tail", ts: "t1" });
const key = `run-logs/${handle.logRef}`;
await vi.waitFor(() => expect(objects.has(key)).toBe(true), { timeout: 2000 });
await store.append(handle, { stream: "stdout", chunk: "final-line", ts: "t2" });
await store.finalize(handle);
const body = objects.get(key)!.toString("utf8");
expect(body).toContain("early-tail");
expect(body).toContain("final-line");
// No retired timer fires a stale partial upload over the finalized log.
const putsAfterFinalize = calls.put;
await new Promise((resolve) => setTimeout(resolve, 60));
expect(calls.put).toBe(putsAfterFinalize);
expect(objects.get(key)!.toString("utf8")).toContain("final-line");
});
it("shutdown flush re-checks dirtiness after an upload already on the wire completes", async () => {
const { provider, objects, calls } = createMemoryProvider();
let releaseFirstPut: (() => void) | null = null;
const firstPutGate = new Promise<void>((resolve) => {
releaseFirstPut = resolve;
});
let putStarts = 0;
const realPut = provider.putObject.bind(provider);
provider.putObject = async (input) => {
putStarts++;
if (putStarts === 1) await firstPutGate; // hold the first upload on the wire
return realPut(input);
};
const store = createDurableRunLogStore({
basePath: baseDir,
s3: { provider, keyPrefix: "run-logs", inflightMirrorMs: 10 },
});
const handle = await store.begin(begin);
await store.append(handle, { stream: "stdout", chunk: "mirrored-line", ts: "t1" });
await vi.waitFor(() => expect(putStarts).toBe(1), { timeout: 2000 });
// Re-dirty the entry while the first upload is still in flight.
await store.append(handle, { stream: "stdout", chunk: "tail-during-upload", ts: "t2" });
const flush = store.flushInflightMirrors!();
releaseFirstPut!();
await flush;
// A single-pass flush would exit after the first upload and leave the
// tail on an unref'ed timer that never fires once the process exits.
const body = objects.get(`run-logs/${handle.logRef}`)!.toString("utf8");
expect(body).toContain("tail-during-upload");
expect(calls.put).toBeGreaterThanOrEqual(2);
});
it("bounds the in-flight upload to the stat'ed size when appends race the stream", async () => {
const { provider, objects } = createMemoryProvider();
const store = createDurableRunLogStore({
basePath: baseDir,
s3: { provider, keyPrefix: "run-logs", inflightMirrorMs: 60_000 },
});
const handle = await store.begin(begin);
await store.append(handle, { stream: "stdout", chunk: "counted-line", ts: "t1" });
const absPath = path.join(baseDir, handle.logRef);
const sizeAtStat = (await fs.stat(absPath)).size;
// Grow the file between the mirror's stat() and its stream reaching EOF:
// the upload must carry exactly the stat'ed bytes, not the racing tail
// (an unbounded stream would violate the declared contentLength).
const realStat = fs.stat.bind(fs);
const statSpy = vi.spyOn(fs, "stat").mockImplementation(async (target, ...rest) => {
const result = await realStat(target as Parameters<typeof realStat>[0], ...(rest as []));
if (String(target).endsWith(".ndjson")) {
await fs.appendFile(absPath, `${JSON.stringify({ ts: "t2", stream: "stdout", chunk: "raced-append" })}\n`);
}
return result;
});
try {
await store.flushInflightMirrors!();
} finally {
statSpy.mockRestore();
}
const body = objects.get(`run-logs/${handle.logRef}`)!;
expect(body.length).toBe(sizeAtStat);
expect(body.toString("utf8")).toContain("counted-line");
expect(body.toString("utf8")).not.toContain("raced-append");
});
it("in-flight upload failures never break appends and recover on the next flush", async () => {
const { provider, objects, calls } = createMemoryProvider();
let failPuts = true;
const realPut = provider.putObject.bind(provider);
provider.putObject = async (input) => {
if (failPuts) throw new Error("endpoint down");
return realPut(input);
};
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
try {
const store = createDurableRunLogStore({
basePath: baseDir,
s3: { provider, keyPrefix: "run-logs", inflightMirrorMs: 60_000 },
});
const handle = await store.begin(begin);
await store.append(handle, { stream: "stdout", chunk: "survives-outage", ts: "t1" });
await store.flushInflightMirrors?.(); // upload fails, append flow unaffected
expect(warn).toHaveBeenCalled();
await store.append(handle, { stream: "stdout", chunk: "post-outage", ts: "t2" });
failPuts = false;
await store.flushInflightMirrors?.();
const body = objects.get(`run-logs/${handle.logRef}`)!.toString("utf8");
expect(body).toContain("survives-outage");
expect(body).toContain("post-outage");
expect(calls.put).toBe(1); // only the successful upload reached storage
} finally {
warn.mockRestore();
}
});
});