fix(revisions): leave revision state unchanged when a write fails

thklein commented

2026-10-02 19:26:59 +00:00

Owner

Part of basepeak-ai/bpai#1220. Follows #27.

With revisions on, a write bumped the revision counter and saved the old content as a revision before reading the new body. If the body then failed part-way, nothing was stored but the counter had moved. A retry with the old latestRevisionID got a spurious conflict, and history held a duplicate. Streaming makes a failure part-way the normal way for a write to fail.

Design

Per-file lock (in-process). The key is provider + workspace + file name. For the directory provider the name is normalised, so F.txt, /F.txt and ./F.txt share one lock.

WriteFile, DeleteFile and DeleteRevision take it.
A waiter gives up when its request ends.
The directory provider stages the body before taking the lock.

A revisioned write of revision N = CurrentID+1, under the lock:

Promote a leftover pending revision (PendingID == CurrentID+1 → CurrentID = PendingID). Then run the conflict check.
Snapshot the current content into file N. It always overwrites a stray, or deletes the stray when there is no content.
Write the info with PendingID = N; CurrentID stays unchanged.
Store the new content.
Write the info with CurrentID = N and PendingID cleared. This is retried; if it still fails, RevisionNotRecordedError names the new latest revision to use as the next base.

What happens when something fails:

Failure	Result
conflict	`ConflictError`, nothing touched (the regex go-gptscript parses still matches)
body never reached EOF, or the directory rename failed	pending marker cleared, own snapshot deleted
anything after EOF (lost response, cancel, an SDK retry's 4xx, 5xx)	pending left in place; the next revisioned write or `DeleteRevision` records the predecessor, and a writer with the old base gets a conflict
the pending-marker write itself	detached restore of the info to the write's base
promotion finds the file was never written	marker dropped, no phantom revision 0
a revision delete during `DeleteFile`	error returned, info kept so numbering continues above the leftover; a retry completes the delete, on Azure too

Other rules:

Writes with CreateRevision=false never touch the revision store, as on main. Every knowledge connector writes that way.
Revision IDs must be canonical non-negative integers. GetRevision returns NotFound above CurrentID; DeleteRevision is a no-op there, as main is for missing revisions.
DeleteFile deletes revisions 0 through CurrentID+1.
Reads take the revision info before the content.
S3 StatFile skips the ranged mimetype read only for an object known to be empty.

Limits are documented in filelock.go:

Only writers within one process are serialised.
S3 and Azure hold the lock while the body streams.
A request the client gave up on can still commit on the server after the lock is released; only conditional writes in the store could prevent that.

Review

Three fresh-context review rounds, each with a correctness reviewer and a silent-failure hunter. Every reported data-loss sequence was reproduced and is now a regression test:

a lost response followed by a 4xx deleting the predecessor;
a stale hidden snapshot adopted after an unrevisioned write;
a revision file left behind by a failed DeleteFile becoming a wrong revision;
a second same-base writer silently winning;
"json" revision IDs deleting the index;
F.0 surviving DeleteFile;
two lock keys for one file;
a stalled body blocking a file;
a read pairing old content with the new revision ID;
a 416 on an empty object;
unrevisioned writes failing on a corrupt index;
an Azure DeleteFile retry not finishing the cleanup;
a phantom revision 0;
a failed pending-marker write.

An 8-goroutine × 15-round conditional-write stress test showed no base winning twice. Each fix was mutation-checked: reverting it fails at least one test.

Checks

go vet, golangci-lint (0 issues), go mod tidy (no diff).
go test -count=1 -race ./...: 92 passed, 59 credential-gated skipped.
The live suite against Garage v2.1.0 (path-style, the SaaS backend) and Azurite: 291 passed, 0 skipped, no objects left behind. That was on 976fb9a; the three commits since change only an error message, a doc comment and tests.

Requests per write on S3

write	requests
unrevisioned	1 PUT, as on main
revisioned, existing file	2 GET + 4 PUT (main: 5)

Follow-ups, out of scope (all pre-existing)

Workspace copy writes revisioned.
IfNotExists is ignored when CreateRevision=false.
Rm leaves revision files behind.

Part of basepeak-ai/bpai#1220. Follows #27. With revisions on, a write bumped the revision counter and saved the old content as a revision **before** reading the new body. If the body then failed part-way, nothing was stored but the counter had moved. A retry with the old `latestRevisionID` got a spurious conflict, and history held a duplicate. Streaming makes a failure part-way the normal way for a write to fail. ## Design **Per-file lock (in-process).** The key is provider + workspace + file name. For the directory provider the name is normalised, so `F.txt`, `/F.txt` and `./F.txt` share one lock. - `WriteFile`, `DeleteFile` and `DeleteRevision` take it. - A waiter gives up when its request ends. - The directory provider stages the body before taking the lock. **A revisioned write of revision N = CurrentID+1, under the lock:** 1. Promote a leftover pending revision (`PendingID == CurrentID+1` → `CurrentID = PendingID`). Then run the conflict check. 2. Snapshot the current content into file N. It always overwrites a stray, or deletes the stray when there is no content. 3. Write the info with `PendingID = N`; `CurrentID` stays unchanged. 4. Store the new content. 5. Write the info with `CurrentID = N` and `PendingID` cleared. This is retried; if it still fails, `RevisionNotRecordedError` names the new latest revision to use as the next base. **What happens when something fails:** | Failure | Result | |---|---| | conflict | `ConflictError`, nothing touched (the regex go-gptscript parses still matches) | | body never reached EOF, or the directory rename failed | pending marker cleared, own snapshot deleted | | anything after EOF (lost response, cancel, an SDK retry's 4xx, 5xx) | pending left in place; the next revisioned write or `DeleteRevision` records the predecessor, and a writer with the old base gets a conflict | | the pending-marker write itself | detached restore of the info to the write's base | | promotion finds the file was never written | marker dropped, no phantom revision 0 | | a revision delete during `DeleteFile` | error returned, info kept so numbering continues above the leftover; a retry completes the delete, on Azure too | **Other rules:** - Writes with `CreateRevision=false` never touch the revision store, as on main. Every knowledge connector writes that way. - Revision IDs must be canonical non-negative integers. `GetRevision` returns NotFound above CurrentID; `DeleteRevision` is a no-op there, as main is for missing revisions. - `DeleteFile` deletes revisions 0 through CurrentID+1. - Reads take the revision info before the content. - S3 `StatFile` skips the ranged mimetype read only for an object known to be empty. Limits are documented in `filelock.go`: - Only writers within one process are serialised. - S3 and Azure hold the lock while the body streams. - A request the client gave up on can still commit on the server after the lock is released; only conditional writes in the store could prevent that. ## Review Three fresh-context review rounds, each with a correctness reviewer and a silent-failure hunter. Every reported data-loss sequence was reproduced and is now a regression test: - a lost response followed by a 4xx deleting the predecessor; - a stale hidden snapshot adopted after an unrevisioned write; - a revision file left behind by a failed `DeleteFile` becoming a wrong revision; - a second same-base writer silently winning; - `"json"` revision IDs deleting the index; - `F.0` surviving `DeleteFile`; - two lock keys for one file; - a stalled body blocking a file; - a read pairing old content with the new revision ID; - a 416 on an empty object; - unrevisioned writes failing on a corrupt index; - an Azure `DeleteFile` retry not finishing the cleanup; - a phantom revision 0; - a failed pending-marker write. An 8-goroutine × 15-round conditional-write stress test showed no base winning twice. Each fix was mutation-checked: reverting it fails at least one test. ## Checks - `go vet`, `golangci-lint` (0 issues), `go mod tidy` (no diff). - `go test -count=1 -race ./...`: 92 passed, 59 credential-gated skipped. - **The live suite against Garage v2.1.0 (path-style, the SaaS backend) and Azurite: 291 passed, 0 skipped**, no objects left behind. That was on `976fb9a`; the three commits since change only an error message, a doc comment and tests. ## Requests per write on S3 | write | requests | |---|---| | unrevisioned | 1 PUT, as on main | | revisioned, existing file | 2 GET + 4 PUT (main: 5) | ## Follow-ups, out of scope (all pre-existing) - Workspace copy writes revisioned. - IfNotExists is ignored when `CreateRevision=false`. - `Rm` leaves revision files behind.

thklein added 3 commits

2026-10-02 19:26:59 +00:00

fix(client): serialise writes to a file within the daemon b51f7b6c69

Take an in-process lock per (workspace, file) in WriteFile, DeleteFile
and DeleteRevision, so two writers based on the same latestRevisionID
cannot both pass the conflict check while the other is still writing.
A waiter gives up when its request context ends.

This only serialises writers within one daemon process; writers in
different processes keep the small window between the conflict check
and the revision bookkeeping.

Refs basepeak-ai/bpai#1220

fix(revisions): hide revision files beyond CurrentID 1a82b74240

GetRevision now answers NotFound for a revision number above
CurrentID, DeleteRevision treats one as already gone (a no-op, as for
any revision that does not exist), and DeleteFile also removes
CurrentID+1. A write is about to snapshot the previous content there
before it records the revision, so such a file is not a revision yet:
it must neither be readable, nor deletable as a revision, nor outlive
the file.

Refs basepeak-ai/bpai#1220

fix(revisions): leave revision state unchanged when a write fails

test / test (pull_request) Successful in 17m41s

Details

b7949253b8

WriteFile bumped and stored the revision info before reading the new
content. A body that failed part-way stored nothing, but CurrentID had
moved, so a retry with the same latestRevisionID got a spurious
conflict (and IfNotExists failed for a file that was never written).

Under the per-file lock: check for a conflict, snapshot the current
content into hidden revision N+1, store the new content, then write
the revision info. On top of that:

- an existing hidden N+1 is kept, not overwritten: it was taken
  against the same base, so it is the true predecessor even if an
  earlier attempt already stored its content
- the snapshot is deleted only when the failure proves nothing was
  stored (body not read to EOF, or a 4xx); after a cancellation or a
  cut connection it stays, since the store may have committed
- the revision info write is retried briefly; if it still fails the
  write returns RevisionNotRecordedError (content stored, revision
  not recorded), and a retry with the same base records it correctly
- detached cleanup contexts are bounded to 30s

The directory provider stages its temp file before the snapshot, so a
failed body there touches no revision state at all.

Refs basepeak-ai/bpai#1220

thklein force-pushed fix/revisions-on-failure from b7949253b8

test / test (pull_request) Successful in 17m41s

Details

to 4e895057ca

test / test (pull_request) Successful in 9m41s

Details

2026-10-02 20:04:10 +00:00

Compare

thklein added 7 commits

2026-10-02 20:26:27 +00:00

fix(revisions): leave the revision store alone for unrevisioned writes 47b10bf065

A write with CreateRevision=false read the revision index (and
promoted a pending revision) before writing. That doubled the request
count of every connector writing this way, and an empty or corrupt
index, or an unreachable revision store, made the file unwritable.
Such writes now never touch the revision store; a pending revision
stays pending for the next revisioned write or DeleteRevision.

Refs basepeak-ai/bpai#1220

fix(azure): clean up revisions when the blob is already gone 514471a30f

DeleteFile returned early on a 404 for the blob, before deleting the
revisions. A DeleteFile retried after a partial failure (blob deleted,
a revision delete failed) therefore never finished. Fall through to
the revision cleanup, as the S3 and directory providers do.

Refs basepeak-ai/bpai#1220

fix(revisions): drop a pending revision whose file was never written 53ed869b8d

A first write rejected after its body was read (a 403, a cancel before
PutObject) is ambiguous and leaves revision 0 pending. The next
operation promoted it to CurrentID 0, so StatFile said the file did
not exist while an IfNotExists write of it conflicted. When promoting,
check (HEAD / stat) that the file has content; if not, drop the marker
without advancing CurrentID.

Refs basepeak-ai/bpai#1220

fix(revisions): undo a pending marker whose write reported failure ce8f1f5aab

A pending-marker write can land although it reports an error (a lost
response, then a rejected retry). The write then failed with nothing
stored, but the marker stayed: the next operation promoted a duplicate
and a retry with the same base conflicted. Restore the revision info
to the write's base (detached, 30s), as abort does; if that fails too,
the duplicate it leaves is still safe.

Refs basepeak-ai/bpai#1220

fix(revisions): name the revision index in decode errors 69d237dd81

A corrupt or empty <file>.json failed revisioned writes, reads with
the latest revision and DeleteRevision with a bare "EOF" or JSON
syntax error. Say which index could not be read.

Refs basepeak-ai/bpai#1220

fix(s3): only skip the mimetype read for a known empty object acc6a46164

A HeadObject response without a Content-Length was treated as an empty
object. Skip the ranged read only when the length is known to be 0.

Refs basepeak-ai/bpai#1220

docs(client): describe what the file lock does and does not cover

test / test (pull_request) Has been cancelled

Details

976fb9afaf

The comment claimed writers in different processes keep only a small
window between the conflict check and the revision commit. With the
revision recorded after the upload, that window spans the upload. Also
note that S3 and Azure hold the lock while the body streams, and that a
request the client gave up on can still commit after the lock is
released.

Refs basepeak-ai/bpai#1220

thklein added 3 commits

2026-10-02 20:33:47 +00:00

fix(revisions): name the new latest revision in RevisionNotRecordedError 86337b298b

The error told the caller to re-read the latest revision. Reads never
record a pending revision, so a re-read still reports the old ID and
the next write is a guaranteed conflict. Say that the latest revision
is now the one the write created, and to use it as the next base.

Refs basepeak-ai/bpai#1220

docs(client): cover cross-process writers without a base b900eede9c

A writer in another process that has no latestRevisionID promotes a
pending revision instead of conflicting, so its commit races the
pending writer's. Say so next to the conflict case.

Refs basepeak-ai/bpai#1220

test(revisions): cover the promotion's existence check failing

test / test (pull_request) Successful in 9m9s

Details

6713b6fabd

A 5xx or a cancellation during the check must fail the write and keep
the pending revision for the next operation. Also cover the drop path
on the directory provider: a pending revision of a file that was never
written is dropped, with no phantom revision 0.

Refs basepeak-ai/bpai#1220

thklein merged commit d4cbb9c838 into main

2026-10-02 21:54:39 +00:00

thklein deleted branch fix/revisions-on-failure

2026-10-02 21:54:39 +00:00

thklein referenced this pull request from a commit

2026-10-02 21:54:41 +00:00

fix(revisions): leave revision state unchanged when a write fails (#28)

thklein referenced this pull request

2026-10-03 05:04:30 +00:00

Workspace copy writes every file as a revision, so revision IDs disagree across workspaces #29

thklein referenced this pull request

2026-10-03 05:04:30 +00:00