mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-12 22:56:58 +01:00
fix(workspaces): preserve import verification across process restarts (#20311)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Online import preserves old local files before the web server can use a disposable filesystem. > - A parent import must authenticate saved child archives before it can omit their directories. > - The operation journal preserves import identity, but completed child checks existed only in process memory. > - A container restart therefore repeated downloads, decryption and restoration that had already succeeded. > - This pull request saves a versioned verification receipt on each immutable child snapshot. > - A replacement process can reuse those checks while it reacquires locks and verifies current local files. ## Linked Issues or Issue Description Refs #19854 for verification reuse within a live source fence. Refs #19873 for cohort import. Refs #20298 for the separate remote-hold admission fix. Searched related workspace verification and restart PRs; none provides durable child verification. **What happened?** Replacing a server container interrupts a long online import. The original operation and snapshot survive, but a later invocation downloads, decrypts and restores completed child archives again. The current cache ends with the source-lock owner. **Expected behavior** Completed immutable checks survive process and scratch loss. Resume keeps the same operation and snapshot, reuses only correctly bound verification, and publishes once. Current local files and Git state must still be checked under fresh source locks. **Steps to reproduce** 1. Import two nested child workspaces, then start the parent import. 2. Kill the importer after a child verification finishes. Remove its scratch directory. 3. Resume in a new process. Before this fix, the completed child is downloaded and restored again. **Paperclip version or commit** The regression fails on the previous migration verifier. The final branch includes master at `189fefb99183953be37f0d1084275fbb0f35d6e2`. **Deployment mode** Managed Linux server with PostgreSQL and explicit S3 storage configuration. ## What Changed - Add optional opaque storage namespace identity. S3 binds it to the explicit endpoint, region, bucket, normalized prefix and addressing style. Implicit endpoints retain full verification. - Save a versioned child-snapshot receipt only after canonical crypto, manifest and file restoration succeed and a second HEAD confirms the same strong ETag and size. - Bind reuse to the complete snapshot reference, key material, child lineage, source operation and storage namespace. Require a fresh strong HEAD and canonical validation that the current secret provider can unwrap the saved snapshot key. - Keep current directory/Git capture and source-owner checks on every invocation. Read-only verifier callers do not write checkpoints. - Merge checkpoint metadata under a short row lock without changing parent journal hashes, snapshot retention or update timestamps. Bound reads and writes. Lock timeouts, statement timeouts, deadlocks and serialization conflicts are safe cache misses. Other errors remain fatal. - Extend real PostgreSQL/Git/archive tests with process death, scratch removal, resume, changed bindings and single publication. ## Verification - Baseline RED: two import invocations read each child twice. With durable receipts, each child is read once across both invocations. - Real process regression passed on the final rebased head: SIGKILL the owned importer after one checkpoint, delete all scratch, and resume in a new Node process. It retains the same operation and snapshot, reads each child once across both processes, and records one publication. - 53 focused cases passed in 104.86 seconds on the final production bytes before rebase. They cover existing nested/cohort behavior, namespace identity, changed source/reference/key/lineage/ETag, unknown proof versions, weak validators and bounded row contention. - After clean rebase and stronger scratch/snapshot assertions, the real process case passed again in 18.54 seconds. Server TypeScript, module boundaries and diff checks passed on final head `9da68ee7ce4a778201ef85e21a11c49eeea7bc16`. Required CI supplies final-head broad coverage. - The initial restricted-network dependency setup failed; unchanged preflight passed with normal network access. Initial TypeScript failures in test pipe assertions were corrected. No failed attempt is counted as a pass. - Review found a missing current-master-key check. A real restart regression with all child checkpoints complete reproduced unsafe publication after switching to a different valid master key. Follow-up `7ea92c2abd9ffdb2e7dd81cecf62c8fc7a6e74a5` delegates to the canonical snapshot-key owner before both durable and in-memory cache hits. - The fixed process regressions passed 2/2. Ten key-authority and existing identity/source guard cases passed. The wrong-key process now refuses with zero additional child payload GETs, no publication, and unchanged original journal/snapshot rows. Server typecheck, module boundaries and diff checks passed on the follow-up. - Review also found that transient database failures could make the optional checkpoint cache stop a valid import. Read and save now fall back only for SQLSTATE `55P03`, `57014`, `40P01`, and `40001`. Connection loss, identity changes, crypto failures, programming errors and cancellation remain fatal. Reads use a read-only transaction with one-second lock and eight-second statement limits. - The checkpoint database-fault regression failed two cases before the fix. All four recoverable import cases then passed. The final selected run passed 21 cases, including fatal-error and cancellation controls, real row contention, both process-restart cases and key/identity guards. Server typecheck and boundaries passed. Fault injection is limited to the checkpoint database boundary; the importer, PostgreSQL, Git, archives, storage and publication remain real. - Latest master was merged without rewriting published history. Six real import/process-restart cases passed again on final head `8c8e4213a680ebb5c8d6864416bc3a4f271e1e76` in 38.03 seconds. [Required CI](https://github.com/paperclipai/paperclip/actions/runs/42658248222) finished with 54 successful checks and four intentional skips; Snyk passed. [Fresh review](https://github.com/paperclipai/paperclip/pull/20311#issuecomment-6745354381) is 5/5 with all four threads addressed and resolved. Hosted verification after normal release delivery remains pending. ## Risks - A stale receipt could skip authentication of a different object. Complete reference/lineage/namespace binding, fresh strong HEAD and row-locked save validation prevent that reuse. Missing or unsupported hints trigger full verification. - Mutable local files can change between attempts. Their source locks and fresh captures remain required; a checkpoint never certifies current local bytes. - Checkpoint persistence can contend with other snapshot owners. It has one-second lock and eight-second statement limits; the four classified transient SQL errors retain the normal fully verified path. Other database errors stop the import so lost source ownership cannot be hidden. - No schema, archive format, public API or saved-run identity change. Older snapshots without receipts remain readable and get full verification. Existing data and recovery obligations stay intact. ## Model Used OpenAI Codex, GPT-6.1 Sol (`gpt-6.1-sol`), with reasoning, repository tools and code execution. Context-window size is not exposed by the runtime. ## 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) - [x] 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 - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge
This commit is contained in:
1 parent
0cf70c9d0c
commit
1b7fe33432
10 files changed
+416
-7
No files matched your search
@@ -9,6 +9,7 @@ import {
|
||||
} from "@aws-sdk/client-s3";
|
||||
import { assertSourceFileSize, fileRangeMd5, putS3Multipart, putS3MultipartFile } from "./s3-multipart.js";
|
||||
import { createReadStream } from "node:fs";
|
||||
import { createHash } from "node:crypto";
|
||||
import { recordS3BodyFailure, withS3FailureDiagnostics } from "./s3-failure.js";
|
||||
import { addAbortSignal, Readable } from "node:stream";
|
||||
import { retryS3RequestTimeouts } from "./s3-request-timeout-retry.js";
|
||||
@@ -121,6 +122,9 @@ export function createS3StorageProvider(config: S3ProviderConfig): StorageProvid
|
||||
|
||||
return {
|
||||
id: "s3",
|
||||
immutableObjectNamespace: config.endpoint ? createHash("sha256").update(JSON.stringify(["s3",
|
||||
new URL(config.endpoint).href, region, bucket, prefix, Boolean(config.forcePathStyle),
|
||||
])).digest("hex") : undefined,
|
||||
|
||||
async putObjectConditional(input) {
|
||||
const output = await client.send(new PutObjectCommand({
|
||||
|
||||
@@ -107,6 +107,7 @@ function assertPutFileInput(input: PutFileInput): void {
|
||||
export function createStorageService(provider: StorageProvider): StorageService {
|
||||
return {
|
||||
provider: provider.id,
|
||||
...(provider.immutableObjectNamespace ? { immutableObjectNamespace: provider.immutableObjectNamespace } : {}),
|
||||
|
||||
async putFile(input: PutFileInput): Promise<PutFileResult> {
|
||||
assertPutFileInput(input);
|
||||
|
||||
@@ -61,6 +61,7 @@ export interface HeadObjectResult {
|
||||
|
||||
export interface StorageProvider {
|
||||
id: StorageProviderId;
|
||||
readonly immutableObjectNamespace?: string;
|
||||
putObject(input: PutObjectInput): Promise<void | PutObjectReceipt>;
|
||||
getObject(input: GetObjectInput): Promise<GetObjectResult>;
|
||||
headObject(input: GetObjectInput): Promise<HeadObjectResult>;
|
||||
@@ -100,6 +101,7 @@ export interface PutFileResult {
|
||||
|
||||
export interface StorageService {
|
||||
provider: StorageProviderId;
|
||||
readonly immutableObjectNamespace?: string;
|
||||
putFile(input: PutFileInput): Promise<PutFileResult>;
|
||||
getObject(companyId: string, objectKey: string, options?: Pick<GetObjectInput, "range" | "signal">): Promise<GetObjectResult>;
|
||||
headObject(companyId: string, objectKey: string, options?: Pick<GetObjectInput, "signal">): Promise<HeadObjectResult>;
|
||||
|
||||
Reference in new issue
Block a user