mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-02 02:07:25 +08:00
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>
This commit is contained in:
@@ -33,6 +33,14 @@ function createMemoryProvider() {
|
||||
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 };
|
||||
},
|
||||
@@ -122,6 +130,39 @@ describe("createDurableRunLogStore", () => {
|
||||
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" } });
|
||||
|
||||
@@ -209,8 +209,11 @@ export function createDurableRunLogStore(options: DurableRunLogStoreOptions): Ru
|
||||
const stat = await fs.stat(filePath).catch(() => null);
|
||||
if (!stat) return null;
|
||||
const start = Math.max(0, Math.min(offset, stat.size));
|
||||
const end = Math.max(start, Math.min(start + limitBytes - 1, stat.size - 1));
|
||||
if (start > end) return { content: "", nextOffset: start };
|
||||
// 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[] = [];
|
||||
try {
|
||||
@@ -243,8 +246,12 @@ export function createDurableRunLogStore(options: DurableRunLogStoreOptions): Ru
|
||||
if (!head.exists) throw notFound("Run log not found");
|
||||
const total = head.contentLength ?? 0;
|
||||
const start = Math.max(0, Math.min(offset, total));
|
||||
const end = Math.max(start, Math.min(start + limitBytes - 1, total - 1));
|
||||
if (start > end || total === 0) return { content: "", nextOffset: start < total ? start : undefined };
|
||||
// Unlike local file streams, S3 rejects a range that starts at or past
|
||||
// EOF with 416 InvalidRange, so a caught-up reader (offset === total)
|
||||
// must short-circuit to an empty read instead of clamping end up to
|
||||
// start and requesting `bytes=total-total`.
|
||||
const end = Math.min(start + limitBytes - 1, total - 1);
|
||||
if (total === 0 || start > end) return { content: "", nextOffset: start < total ? start : undefined };
|
||||
|
||||
const result = await s3.provider.getObject({ objectKey: key, range: { start, end } });
|
||||
const chunks: Buffer[] = [];
|
||||
|
||||
@@ -61,10 +61,13 @@ function createLocalFileWorkspaceOperationLogStore(basePath: string): WorkspaceO
|
||||
if (!stat) throw notFound("Workspace operation log not found");
|
||||
|
||||
const start = Math.max(0, Math.min(offset, stat.size));
|
||||
const end = Math.max(start, Math.min(start + limitBytes - 1, stat.size - 1));
|
||||
// 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 };
|
||||
return { content: "", nextOffset: start < stat.size ? start : undefined };
|
||||
}
|
||||
|
||||
const chunks: Buffer[] = [];
|
||||
|
||||
Reference in New Issue
Block a user