mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-22 00:30:19 +02:00
## 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>
160 lines
5.4 KiB
TypeScript
160 lines
5.4 KiB
TypeScript
import { createReadStream, promises as fs } from "node:fs";
|
|
import path from "node:path";
|
|
import { createHash } from "node:crypto";
|
|
import { notFound } from "../errors.js";
|
|
import { resolvePaperclipInstanceRoot } from "../home-paths.js";
|
|
|
|
export type WorkspaceOperationLogStoreType = "local_file";
|
|
|
|
export interface WorkspaceOperationLogHandle {
|
|
store: WorkspaceOperationLogStoreType;
|
|
logRef: string;
|
|
}
|
|
|
|
export interface WorkspaceOperationLogReadOptions {
|
|
offset?: number;
|
|
limitBytes?: number;
|
|
}
|
|
|
|
export interface WorkspaceOperationLogReadResult {
|
|
content: string;
|
|
nextOffset?: number;
|
|
}
|
|
|
|
export interface WorkspaceOperationLogFinalizeSummary {
|
|
bytes: number;
|
|
sha256?: string;
|
|
compressed: boolean;
|
|
}
|
|
|
|
export interface WorkspaceOperationLogStore {
|
|
begin(input: { companyId: string; operationId: string }): Promise<WorkspaceOperationLogHandle>;
|
|
append(
|
|
handle: WorkspaceOperationLogHandle,
|
|
event: { stream: "stdout" | "stderr" | "system"; chunk: string; ts: string },
|
|
): Promise<void>;
|
|
finalize(handle: WorkspaceOperationLogHandle): Promise<WorkspaceOperationLogFinalizeSummary>;
|
|
read(handle: WorkspaceOperationLogHandle, opts?: WorkspaceOperationLogReadOptions): Promise<WorkspaceOperationLogReadResult>;
|
|
}
|
|
|
|
function safeSegments(...segments: string[]) {
|
|
return segments.map((segment) => segment.replace(/[^a-zA-Z0-9._-]/g, "_"));
|
|
}
|
|
|
|
function resolveWithin(basePath: string, relativePath: string) {
|
|
const resolved = path.resolve(basePath, relativePath);
|
|
const base = path.resolve(basePath) + path.sep;
|
|
if (!resolved.startsWith(base) && resolved !== path.resolve(basePath)) {
|
|
throw new Error("Invalid log path");
|
|
}
|
|
return resolved;
|
|
}
|
|
|
|
function createLocalFileWorkspaceOperationLogStore(basePath: string): WorkspaceOperationLogStore {
|
|
async function ensureDir(relativeDir: string) {
|
|
const dir = resolveWithin(basePath, relativeDir);
|
|
await fs.mkdir(dir, { recursive: true });
|
|
}
|
|
|
|
async function readFileRange(filePath: string, offset: number, limitBytes: number): Promise<WorkspaceOperationLogReadResult> {
|
|
const stat = await fs.stat(filePath).catch(() => null);
|
|
if (!stat) throw notFound("Workspace operation log not found");
|
|
|
|
const start = Math.max(0, Math.min(offset, stat.size));
|
|
// No lower clamp to `start`: when the reader is fully caught up
|
|
// (offset === size) that clamp made end === start and produced a
|
|
// 1-byte-past-EOF range instead of an empty read.
|
|
const end = Math.min(start + limitBytes - 1, stat.size - 1);
|
|
|
|
if (start > end) {
|
|
return { content: "", nextOffset: start < stat.size ? start : undefined };
|
|
}
|
|
|
|
const chunks: Buffer[] = [];
|
|
await new Promise<void>((resolve, reject) => {
|
|
const stream = createReadStream(filePath, { start, end });
|
|
stream.on("data", (chunk) => {
|
|
chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk));
|
|
});
|
|
stream.on("error", reject);
|
|
stream.on("end", () => resolve());
|
|
});
|
|
|
|
const content = Buffer.concat(chunks).toString("utf8");
|
|
const nextOffset = end + 1 < stat.size ? end + 1 : undefined;
|
|
return { content, nextOffset };
|
|
}
|
|
|
|
async function sha256File(filePath: string): Promise<string> {
|
|
return new Promise<string>((resolve, reject) => {
|
|
const hash = createHash("sha256");
|
|
const stream = createReadStream(filePath);
|
|
stream.on("data", (chunk) => hash.update(chunk));
|
|
stream.on("error", reject);
|
|
stream.on("end", () => resolve(hash.digest("hex")));
|
|
});
|
|
}
|
|
|
|
return {
|
|
async begin(input) {
|
|
const [companyId] = safeSegments(input.companyId);
|
|
const operationId = safeSegments(input.operationId)[0]!;
|
|
const relDir = companyId;
|
|
const relPath = path.join(relDir, `${operationId}.ndjson`);
|
|
await ensureDir(relDir);
|
|
|
|
const absPath = resolveWithin(basePath, relPath);
|
|
await fs.writeFile(absPath, "", "utf8");
|
|
|
|
return { store: "local_file", logRef: relPath };
|
|
},
|
|
|
|
async append(handle, event) {
|
|
if (handle.store !== "local_file") return;
|
|
const absPath = resolveWithin(basePath, handle.logRef);
|
|
const line = JSON.stringify({
|
|
ts: event.ts,
|
|
stream: event.stream,
|
|
chunk: event.chunk,
|
|
});
|
|
await fs.appendFile(absPath, `${line}\n`, "utf8");
|
|
},
|
|
|
|
async finalize(handle) {
|
|
if (handle.store !== "local_file") {
|
|
return { bytes: 0, compressed: false };
|
|
}
|
|
const absPath = resolveWithin(basePath, handle.logRef);
|
|
const stat = await fs.stat(absPath).catch(() => null);
|
|
if (!stat) throw notFound("Workspace operation log not found");
|
|
|
|
const hash = await sha256File(absPath);
|
|
return {
|
|
bytes: stat.size,
|
|
sha256: hash,
|
|
compressed: false,
|
|
};
|
|
},
|
|
|
|
async read(handle, opts) {
|
|
if (handle.store !== "local_file") {
|
|
throw notFound("Workspace operation log not found");
|
|
}
|
|
const absPath = resolveWithin(basePath, handle.logRef);
|
|
const offset = opts?.offset ?? 0;
|
|
const limitBytes = opts?.limitBytes ?? 256_000;
|
|
return readFileRange(absPath, offset, limitBytes);
|
|
},
|
|
};
|
|
}
|
|
|
|
let cachedStore: WorkspaceOperationLogStore | null = null;
|
|
|
|
export function getWorkspaceOperationLogStore() {
|
|
if (cachedStore) return cachedStore;
|
|
const basePath = process.env.WORKSPACE_OPERATION_LOG_BASE_PATH
|
|
?? path.resolve(resolvePaperclipInstanceRoot(), "data", "workspace-operation-logs");
|
|
cachedStore = createLocalFileWorkspaceOperationLogStore(basePath);
|
|
return cachedStore;
|
|
}
|