amadan
plan: claim Plan 2 rev 2, after Codex's review
Region-bound versionedS3 (no lazy memo), external lease_test package, AWS copy-source escaping, delete markers as ErrIsDeleteMarker (405), complete locked helpers, fault injection across both implementations, request-counted never-bare delete, OpenVersion streaming, sibling cleanup left to the destination region, honest §13 coverage table.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 file changed,
+894
−644
docs/superpowers/plans/2026-10-05-host-lifecycle-claim-2-versioned-storage.md+894 −644
CI passed — run details
diff --git a/docs/superpowers/plans/2026-10-05-host-lifecycle-claim-2-versioned-storage.md b/docs/superpowers/plans/2026-10-05-host-lifecycle-claim-2-versioned-storage.md| index db5b48f47..8332e02c5 100644 |
| --- a/docs/superpowers/plans/2026-10-05-host-lifecycle-claim-2-versioned-storage.md |
| +++ b/docs/superpowers/plans/2026-10-05-host-lifecycle-claim-2-versioned-storage.md |
| @@ -2,91 +2,128 @@ |
| > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. |
| -**Goal:** Give the data bucket a small, exact-version storage interface. |
| -It lists every version of every object, delete markers included and |
| -paginated, and copies, reads and deletes **by version id**. It also has a |
| -create-only put. That is everything Plans 3 to 8 need to park data, write |
| -certificates and read pinned versions, without ever issuing a bare-key |
| -destructive call (spec C3). |
| +**Goal:** give the data bucket a small storage interface that works on |
| +exact object versions. It can: |
| + |
| +- list every version of every object, delete markers included, across |
| + pages; |
| +- read, stream, copy and delete **by version id**; |
| +- put an object only if none exists (create-only). |
| + |
| +That covers what Plans 3 to 8 need for parking data, writing |
| +certificates and reading pinned versions, and nothing in it ever deletes |
| +by bare key (spec C3). |
| **Architecture:** |
| -- **A new interface, `lease.VersionedStore`.** It sits beside the |
| - existing `MirrorStore`. `MirrorStore` and its two test fakes are left |
| - untouched. |
| -- **The production implementation is the existing `s3Mirror`.** It |
| - reuses the same regional client routing. |
| -- **One exported fake, `leasetest.VersionedBucket`,** models versions, |
| - delete markers and ETag rules, with failure injection. It is the |
| - reference every later plan tests against. |
| -- **An `http.Handler` that speaks the S3 wire protocol over that fake.** |
| - The real `s3Mirror` code is tested against it, using an actual AWS SDK |
| - client, over the wire. A single contract test suite runs against both |
| - the fake directly and the `s3Mirror`. |
| -- **IAM:** the box role gains the three S3 version actions on its own |
| - data bucket. |
| - |
| -**Tech Stack:** Go, aws-sdk-go-v2 `service/s3` v1.105.0 (already in |
| -go.mod), `net/http/httptest`, OpenTofu HCL. |
| - |
| -**Spec:** `docs/superpowers/specs/2026-10-02-host-lifecycle-claim-design.md`, |
| -rev 6.12. Read C3, §6.2 and §13 first. §13 lists the S3 behaviour already |
| -verified on real AWS, and this plan encodes exactly that behaviour in the |
| -fake. |
| - |
| -**Depends on:** nothing. It can be built in parallel with Plan 1's merge. |
| +- **One interface.** A new `lease.VersionedStore` sits beside the |
| + existing `MirrorStore`, which stays untouched along with its fakes and |
| + callers. |
| +- **The production type is a new `versionedS3`.** Every bucket is bound |
| + explicitly to a region at construction, so no call can ever reach the |
| + wrong region. It doesn't reuse `s3Mirror`'s lazy `byBucket` memo. |
| + `CopyObject` runs on the destination bucket's client, because that is |
| + where S3 executes a copy. |
| +- **One exported fake.** `leasetest.VersionedBucket` models versions, |
| + delete markers and ETag rules, and lets tests inject failures. Beside |
| + it, `leasetest.NewS3Server` speaks the S3 wire protocol over that fake |
| + and counts requests. |
| +- **One shared contract suite.** It runs against the fake directly, and |
| + against `versionedS3` over the wire with a real AWS SDK client. Those |
| + tests live in the external `lease_test` package, which avoids an |
| + import cycle. |
| +- **IAM.** The box role gains S3's version actions on its **own** data |
| + bucket only. Cleaning up cross-region staging stays with the |
| + destination region's box (spec N13), so the sibling grants don't |
| + change. |
| + |
| +**Tech Stack:** Go; aws-sdk-go-v2 `service/s3` v1.105.0 (already in |
| +`go.mod`; `CopyObjectInput.IfNoneMatch` and `PutObjectInput.IfNoneMatch` |
| +exist); smithy-go `encoding/httpbinding` (already a transitive |
| +dependency); `net/http/httptest`; OpenTofu. |
| + |
| +**Spec:** `docs/superpowers/specs/2026-10-02-host-lifecycle-claim-design.md` |
| +rev 6.12. Read C3, §6.2, §6.5 and §13 before starting. Rev 2 of this |
| +plan answers Codex's review of rev 1: two P1s and seven P2s. |
| + |
| +**Depends on:** nothing. |
| ## Global Constraints |
| -- **Every destructive call names a version id.** No method in this |
| - interface deletes or overwrites by bare key. |
| -- **Delete by version id is idempotent.** A version or key that is |
| - already gone is success, not an error (§13 row 3). |
| -- **Listings never return partial results.** A page error fails the |
| - whole listing, with no partial result. A listing returns every version |
| - **and** every delete marker, in key order, with versions newest first |
| - within a key. |
| -- **Copy semantics.** |
| - - Copy reads the source **by version id** (`CopySource` carries |
| - `?versionId=`). |
| - - A copy may also be made create-only at the destination: `If-None-Match: |
| - *` on the copy. A 412 then returns `ErrDestinationExists`. |
| - - A copy never needs the source ETag, but it records the destination's |
| - new version id and ETag. |
| - - A multipart source ETag becomes single-part on copy (§13 row 9). The |
| - fake models this. |
| +- **No bare-key destruction.** Every destructive call names a version |
| + id. `DeleteVersion` refuses an empty version id **without sending a |
| + request**. No method deletes or overwrites by bare key. |
| +- **Deletes are idempotent.** Deleting a version that is already gone, |
| + or a version of a missing key, is success (§13 row 3). |
| +- **Listings are whole or nothing.** A page error fails the whole |
| + listing; there is never a partial result. A listing returns every |
| + version and every delete marker: |
| + - in **key order**; |
| + - **within a key, the entry with `IsLatest` comes first**; |
| + - the remaining entries for that key come newest-`LastModified` first, |
| + and **their relative order is unspecified when timestamps tie**. |
| + Callers rely on `IsLatest`, never on position among non-latest |
| + entries. |
| +- **Delete markers can't be read or copied.** `GetVersion`, |
| + `OpenVersion` or `CopyVersion` on a version id that is a delete marker |
| + returns `ErrIsDeleteMarker` (S3: 405 with `x-amz-delete-marker: true`). |
| + A missing version returns `lease.ErrMirrorNotFound`. |
| +- **Copy rules.** |
| + - The source is read by version id. `CopySource` is |
| + `httpbinding.EscapePath(srcBucket+"/"+srcKey, false)` followed by |
| + `"?versionId="+url.QueryEscape(id)`. |
| + - A create-only copy uses `If-None-Match: *`. A 412 then means |
| + `ErrDestinationExists`. A 409 is **never** read as "exists". |
| + - A delete marker at the destination does not count as existing. |
| + - A copy produces a single-part ETag (§13 row 9). |
| + - After `ErrDestinationExists`, the caller recovers the destination's |
| + id and size with `ListVersions` on the exact key. |
| - **`PutIfAbsent`** uses `If-None-Match: *`. A 412 returns `ErrExists`. |
| -- **The fake follows §13 exactly:** |
| - - a bare delete adds a delete marker; |
| - - deleting a version removes only that version; |
| - - deleting the current version makes the next newest version current |
| - (a delete marker counts); |
| - - versions are retained. |
| -- **IAM:** `s3:ListBucketVersions` on the data bucket ARN, and |
| - `s3:GetObjectVersion` and `s3:DeleteObjectVersion` on `<data bucket>/*`, |
| - in **both** `infra/modules/region-host/main.tf` and |
| - `infra/modules/region-remote-host/main.tf`. No other grants change. |
| + A delete marker doesn't count as existing. |
| +- **Fault injection matches across implementations.** A `FailOn` |
| + failure in the fake surfaces as a non-nil error from both the fake and |
| + `versionedS3`; the server returns it as 500 `InternalError`. A |
| + failure is never turned into not-found, and never into success. |
| +- **IAM.** The box role gets exactly these grants, in both |
| + `infra/modules/region-host/main.tf` and |
| + `infra/modules/region-remote-host/main.tf`, each using its own module's |
| + data bucket ARN: |
| + - `s3:ListBucketVersions` on the data bucket ARN; |
| + - `s3:GetObjectVersion` and `s3:DeleteObjectVersion` on |
| + `<data bucket ARN>/*`. |
| + |
| + Leave the sibling `moves/*` statements unchanged. |
| - **Gate (AGENTS.md):** `go build ./...`, `go vet ./...`, |
| - `go test ./... -count=1` and `gofmt -l .` must all be clean. Use the |
| - default Go cache. In a read-only sandbox, use `GOCACHE=$TMPDIR/gocache`, |
| - never `/tmp`. CI is the authority for any test that only fails in the |
| - sandbox. |
| + `go test ./... -count=1` and `gofmt -l .`, all clean. Use the default |
| + Go cache. In a read-only sandbox, use `GOCACHE=$TMPDIR/gocache`, never |
| + `/tmp`. |
| + |
| +## What this plan does and doesn't cover of §13 |
| + |
| +| §13 row | Covered here | |
| +|---|---| |
| +| 1, distinct version ids | yes, fake and contract | |
| +| 2, copy by source version | yes | |
| +| 3, delete by version is exact | yes | |
| +| 4, a bare delete adds a marker | fake helper `BareDelete` only; the interface never issues one | |
| +| 5, `If-None-Match` on PUT | yes, `PutIfAbsent` | |
| +| 6, `PutObject If-Match` | **no**: claim CASes go through `internal/release.Store`, which already has it | |
| +| 7, `DeleteObject If-Match` | **no**: `release.DeleteIfMatch` already covers it | |
| +| 8, signature age | **no**: Plan 3/6's staging writer owns signing at the deadline check (CC19) | |
| +| 9, multipart to single-part ETag on copy | yes | |
| ## Review Focus |
| -- **A listing over 1,000 objects**, where S3 paginates by key and version |
| - marker. Every version must come back exactly once (Task 3). |
| -- **Keys with characters that need escaping** (`+`, spaces, `%`, `?` in |
| - a key) in `CopySource` and in paths. The copy must hit the exact |
| - version of the exact key (Task 4). |
| -- **Deleting the current version when an older one exists.** The older |
| - one becomes current, in both the fake and S3. Callers must expect it |
| - (Tasks 2 and 5). |
| -- **A page error mid-listing** must surface as an error, never as a short |
| - list (Tasks 2 and 3). |
| -- **Copy create-only at a destination that only holds a delete marker.** |
| - S3 lets `If-None-Match: *` succeed there (§13 row 5's sibling), and the |
| - fake must match (Task 2). |
| +1. **A listing over many pages, with many versions of one key spanning |
| + pages.** Every entry must come back exactly once (Task 3). |
| +2. **Keys containing `+`, space, `%`, `?` and `/`.** The `CopySource` |
| + header must escape them the way AWS's encoder does (Task 4). |
| +3. **Deleting the current version when an older one exists.** The older |
| + version becomes current (Tasks 2 and 5). |
| +4. **A page error mid-listing.** It is an error, never a short list |
| + (Tasks 2 and 3). |
| +5. **Reading or copying a delete marker's version id** gives |
| + `ErrIsDeleteMarker` from both implementations (Tasks 2 and 4). |
| --- |
| @@ -94,28 +131,31 @@ fake. |
| | File | Responsibility | |
| |---|---| |
| -| `internal/lease/versioned.go` | `VersionedStore`, `ObjectVersion`, the errors, and the `s3Mirror` methods | |
| -| `internal/lease/leasetest/bucket.go` | `VersionedBucket`, the in-memory versioned fake (exported, for every later plan) | |
| -| `internal/lease/leasetest/s3server.go` | `NewS3Server(*VersionedBucket) http.Handler`, the S3 wire protocol over the fake | |
| -| `internal/lease/leasetest/contract.go` | `RunVersionedStoreContract(t, newStore)`, the shared behaviour suite | |
| -| `internal/lease/leasetest/bucket_test.go` | Runs the contract against the fake | |
| -| `internal/lease/versioned_test.go` | Runs the contract against `s3Mirror` over `NewS3Server`, plus wire-specific tests | |
| -| `infra/modules/region-host/main.tf`, `infra/modules/region-remote-host/main.tf` | IAM grants | |
| +| `internal/lease/versioned.go` | `VersionedStore`, `ObjectVersion`, errors, `versionedS3`, `NewVersionedS3` | |
| +| `internal/lease/export_test.go` | `NewVersionedS3WithClients`, a test-only constructor (package `lease`) | |
| +| `internal/lease/versioned_test.go` | package `lease_test`: the contract over the wire, plus wire-specific tests | |
| +| `internal/lease/leasetest/bucket.go` | `VersionedBucket`, the in-memory model | |
| +| `internal/lease/leasetest/s3server.go` | `NewS3Server`, the S3 wire protocol over the model, with request counting | |
| +| `internal/lease/leasetest/contract.go` | `RunVersionedStoreContract` | |
| +| `internal/lease/leasetest/bucket_test.go` | the contract against the model | |
| +| `infra/modules/region-host/main.tf`, `infra/modules/region-remote-host/main.tf` | IAM | |
| --- |
| -### Task 1: The interface, types and errors |
| +### Task 1: The interface, types, errors and constructor |
| **Files:** |
| - Create: `internal/lease/versioned.go` |
| +- Create: `internal/lease/export_test.go` |
| **Interfaces:** |
| - Produces: |
| - - `type ObjectVersion struct { Key, VersionID, ETag string; Size int64; IsLatest, DeleteMarker bool; LastModified time.Time }`; |
| - - `type VersionedStore interface` (below); |
| - - `ErrExists` and `ErrDestinationExists`. |
| + - `ObjectVersion`, `VersionedStore`; |
| + - `ErrExists`, `ErrDestinationExists`, `ErrIsDeleteMarker`, `ErrNoVersionID`; |
| + - `NewVersionedS3(ctx, regionByBucket map[string]string) (VersionedStore, error)`; |
| + - test-only `NewVersionedS3WithClients(clients map[string]*awss3.Client) VersionedStore`. |
| -- [ ] **Step 1: Write the file** |
| +- [ ] **Step 1: Write `versioned.go` (the interface and stubs that return `errNotYet`)** |
| ```go |
| package lease |
| @@ -123,7 +163,12 @@ package lease |
| import ( |
| "context" |
| "errors" |
| + "fmt" |
| + "io" |
| "time" |
| + |
| + awsconfig "github.com/aws/aws-sdk-go-v2/config" |
| + awss3 "github.com/aws/aws-sdk-go-v2/service/s3" |
| ) |
| // ObjectVersion is one entry of a versioned listing: an object version, or |
| @@ -139,55 +184,131 @@ type ObjectVersion struct { |
| } |
| var ( |
| - // ErrExists is PutIfAbsent finding the key already holds a current |
| - // object version. |
| - ErrExists = errors.New("lease: object already exists") |
| - // ErrDestinationExists is a create-only CopyVersion whose destination |
| - // key already holds a current object version. |
| + ErrExists = errors.New("lease: object already exists") |
| ErrDestinationExists = errors.New("lease: copy destination already exists") |
| + ErrIsDeleteMarker = errors.New("lease: version is a delete marker") |
| + ErrNoVersionID = errors.New("lease: a version id is required") |
| ) |
| // VersionedStore is the data bucket addressed by exact object version (host |
| -// lifecycle claim spec C3, §6.2). No method deletes or overwrites by bare |
| -// key. |
| +// lifecycle claim spec C3, §6.2, §6.5). No method deletes or overwrites by |
| +// bare key. |
| type VersionedStore interface { |
| - // ListVersions returns every version and delete marker under prefix, |
| - // in key order, newest first within a key. It reads every page and |
| - // fails on any page error: never a partial listing. |
| + // ListVersions returns every version and delete marker under prefix: |
| + // key order; within a key the IsLatest entry first, the rest newest |
| + // first with ties unordered. Any page error fails the whole call. |
| ListVersions(ctx context.Context, bucket, prefix string) ([]ObjectVersion, error) |
| - // GetVersion reads one exact version. A missing version is |
| - // ErrMirrorNotFound. |
| + // OpenVersion streams one exact version. Missing: ErrMirrorNotFound. |
| + // A delete marker: ErrIsDeleteMarker. |
| + OpenVersion(ctx context.Context, bucket, key, versionID string) (io.ReadCloser, error) |
| + // GetVersion reads one exact version whole (OpenVersion's errors). |
| GetVersion(ctx context.Context, bucket, key, versionID string) ([]byte, error) |
| - // CopyVersion copies one exact source version to dstKey. With |
| - // createOnly the copy carries If-None-Match: *, and an existing |
| - // current destination object gives ErrDestinationExists. It returns |
| - // the destination's new version id and its ETag. |
| + // CopyVersion copies one exact source version to dstKey. createOnly |
| + // adds If-None-Match: *; an existing current destination object gives |
| + // ErrDestinationExists. It returns the destination's new version id |
| + // and ETag. |
| CopyVersion(ctx context.Context, srcBucket, srcKey, srcVersionID, dstBucket, dstKey string, createOnly bool) (dstVersionID, dstETag string, err error) |
| - // DeleteVersion removes one exact version. A version or key already |
| - // gone is success. |
| + // DeleteVersion removes one exact version; gone already is success. |
| + // An empty versionID is ErrNoVersionID, without any request. |
| DeleteVersion(ctx context.Context, bucket, key, versionID string) error |
| - // PutIfAbsent writes body only if key holds no current object version |
| - // (If-None-Match: *); otherwise ErrExists. It returns the new version |
| - // id. |
| + // PutIfAbsent writes body only if key holds no current object version. |
| PutIfAbsent(ctx context.Context, bucket, key string, body []byte) (versionID string, err error) |
| } |
| + |
| +// versionedS3 binds every bucket to its own regional client explicitly. |
| +// An unbound bucket is an error, never a guess. |
| +type versionedS3 struct { |
| + clients map[string]*awss3.Client // bucket -> client for its region |
| +} |
| + |
| +// NewVersionedS3 builds one client per region from the default credential |
| +// chain (with s3compat options, as newLeaserClient does) and binds each |
| +// bucket to its region's client. |
| +func NewVersionedS3(ctx context.Context, regionByBucket map[string]string) (VersionedStore, error) { |
| + byRegion := map[string]*awss3.Client{} |
| + out := &versionedS3{clients: map[string]*awss3.Client{}} |
| + for bucket, region := range regionByBucket { |
| + c, ok := byRegion[region] |
| + if !ok { |
| + var err error |
| + if c, err = newLeaserClient(ctx, region); err != nil { |
| + return nil, fmt.Errorf("lease: versioned client for %s: %w", region, err) |
| + } |
| + byRegion[region] = c |
| + } |
| + out.clients[bucket] = c |
| + } |
| + return out, nil |
| +} |
| + |
| +func (s *versionedS3) client(bucket string) (*awss3.Client, error) { |
| + c, ok := s.clients[bucket] |
| + if !ok { |
| + return nil, fmt.Errorf("lease: bucket %q is not bound to a region", bucket) |
| + } |
| + return c, nil |
| +} |
| + |
| +var errNotYet = errors.New("lease: not implemented yet") |
| + |
| +func (s *versionedS3) ListVersions(ctx context.Context, bucket, prefix string) ([]ObjectVersion, error) { |
| + return nil, errNotYet |
| +} |
| +func (s *versionedS3) OpenVersion(ctx context.Context, bucket, key, versionID string) (io.ReadCloser, error) { |
| + return nil, errNotYet |
| +} |
| +func (s *versionedS3) GetVersion(ctx context.Context, bucket, key, versionID string) ([]byte, error) { |
| + return nil, errNotYet |
| +} |
| +func (s *versionedS3) CopyVersion(ctx context.Context, srcBucket, srcKey, srcVersionID, dstBucket, dstKey string, createOnly bool) (string, string, error) { |
| + return "", "", errNotYet |
| +} |
| +func (s *versionedS3) DeleteVersion(ctx context.Context, bucket, key, versionID string) error { |
| + return errNotYet |
| +} |
| +func (s *versionedS3) PutIfAbsent(ctx context.Context, bucket, key string, body []byte) (string, error) { |
| + return "", errNotYet |
| +} |
| + |
| +var _ VersionedStore = (*versionedS3)(nil) |
| +var _ = awsconfig.LoadDefaultConfig // newLeaserClient's dependency; remove if unused |
| +``` |
| + |
| +`newLeaserClient(ctx, region)` exists in `internal/lease/endpoint.go` |
| +(around lines 99-110). If its real signature differs, adapt the call and |
| +keep its semantics: default chain, `s3compat.Options()`, and the |
| +`CARLOS_S3_ENDPOINT` override. Delete the `awsconfig` placeholder line |
| +if the import isn't needed. |
| + |
| +- [ ] **Step 2: Write `export_test.go`** |
| + |
| +```go |
| +package lease |
| + |
| +import awss3 "github.com/aws/aws-sdk-go-v2/service/s3" |
| + |
| +// NewVersionedS3WithClients is the test-only constructor: tests bind |
| +// buckets to clients pointed at leasetest.NewS3Server. |
| +func NewVersionedS3WithClients(clients map[string]*awss3.Client) VersionedStore { |
| + return &versionedS3{clients: clients} |
| +} |
| ``` |
| -- [ ] **Step 2: Build** |
| +- [ ] **Step 3: Build** |
| -Run: `go build ./internal/lease/` |
| +Run: `go build ./internal/lease/ && go vet ./internal/lease/` |
| Expected: success. |
| -- [ ] **Step 3: Commit** |
| +- [ ] **Step 4: Commit** |
| ```bash |
| -git add internal/lease/versioned.go |
| -git commit -m "lease: the VersionedStore interface (exact-version data bucket access)" |
| +git add internal/lease/versioned.go internal/lease/export_test.go |
| +git commit -m "lease: the VersionedStore interface and an explicitly region-bound S3 type" |
| ``` |
| --- |
| -### Task 2: The fake versioned bucket and the contract suite |
| +### Task 2: The model and the contract suite |
| **Files:** |
| - Create: `internal/lease/leasetest/bucket.go`, `internal/lease/leasetest/contract.go`, `internal/lease/leasetest/bucket_test.go` |
| @@ -195,177 +316,236 @@ git commit -m "lease: the VersionedStore interface (exact-version data bucket ac |
| **Interfaces:** |
| - Consumes: Task 1. |
| - Produces: |
| - - `type VersionedBucket struct` with `NewVersionedBucket() *VersionedBucket`; |
| - - the methods of `lease.VersionedStore`, with the bucket name argument |
| - checked against a set of known bucket names (`AddBucket(name string)`); |
| - - helpers for later plans: |
| - - `PutMultipart(bucket, key string, body []byte)`, which stores an |
| - ETag with a `-2` suffix; |
| - - `BareDelete(bucket, key string)`, which adds a delete marker; |
| - - `Current(bucket, key string) ([]byte, bool)`; |
| - - failure hooks: `FailListAfterPages int` (fail the listing after this |
| - many pages, 0 = never), `PageSize int` (default 1000), and |
| + - `VersionedBucket` (implements `lease.VersionedStore`), built with |
| + `NewVersionedBucket()`; |
| + - setup: `AddBucket`; |
| + - writes: `Put` and `PutMultipart` (each returns the version id) and |
| + `BareDelete` (returns the marker id); |
| + - reads: `LatestID`, `Current` and `Snapshot` (the full state as a |
| + string, for before/after comparisons); |
| + - knobs: `PageSize int` (default 1000), `FailListAfterPages int` and |
| `FailOn func(op, key string) error`; |
| - - `RunVersionedStoreContract(t *testing.T, mk func(t *testing.T) (lease.VersionedStore, *VersionedBucket, string))`. |
| - `mk` returns the store under test, the backing fake (for setup and |
| - inspection) and the bucket name. |
| - |
| -**Model:** |
| -- Each key holds a list of versions, newest first. Each version is |
| - `{id, body, etag, size, deleteMarker, modified}`. |
| -- Version ids are `v000001`, `v000002` and so on, from a counter, so |
| - ordering is deterministic. |
| -- The ETag of a put is the hex md5 of the body. For `PutMultipart` it is |
| - md5 + `"-2"`. A copy always gets the single-part md5 of the body. |
| -- "Current" means the newest entry, if it isn't a delete marker. |
| + - the shared listing helpers `listAll` and `pageOf`, both run with the |
| + mutex held; |
| + - `RunVersionedStoreContract(t, mk)`, where `mk` returns |
| + `(store lease.VersionedStore, model *VersionedBucket, bucket string, served func() int)`. |
| + `served` counts HTTP requests for wire stores; for the model it is |
| + `nil`. |
| -- [ ] **Step 1: Write the contract suite (it fails: nothing implements it yet)** |
| +- [ ] **Step 1: Write the contract suite** |
| ```go |
| package leasetest |
| import ( |
| - "bytes" |
| "context" |
| "errors" |
| "fmt" |
| + "io" |
| + "strings" |
| "testing" |
| "github.com/carlosframework/platform/internal/lease" |
| ) |
| -// RunVersionedStoreContract is the behaviour every lease.VersionedStore must |
| -// have (spec §13). mk returns the store under test, the fake that backs it |
| -// (used to set up and inspect state) and the bucket name. |
| -func RunVersionedStoreContract(t *testing.T, mk func(t *testing.T) (lease.VersionedStore, *VersionedBucket, string)) { |
| +// Maker builds a store under test, the model backing it, the bucket name, |
| +// and (for wire stores) a request counter. |
| +type Maker func(t *testing.T) (st lease.VersionedStore, m *VersionedBucket, bucket string, served func() int) |
| + |
| +// RunVersionedStoreContract is the behaviour every lease.VersionedStore |
| +// must have (spec §13, as covered by Plan 2). |
| +func RunVersionedStoreContract(t *testing.T, mk Maker) { |
| ctx := context.Background() |
| - t.Run("versions and delete markers, newest first", func(t *testing.T) { |
| - st, fb, b := mk(t) |
| - fb.Put(b, "p/a", []byte("one")) |
| - fb.Put(b, "p/a", []byte("two")) |
| - fb.BareDelete(b, "p/a") |
| - fb.Put(b, "p/b", []byte("x")) |
| - fb.Put(b, "q/c", []byte("other prefix")) |
| + t.Run("listing: key order, latest first, markers included, prefix bounded", func(t *testing.T) { |
| + st, m, b, _ := mk(t) |
| + m.Put(b, "p/a", []byte("one")) |
| + m.Put(b, "p/a", []byte("two")) |
| + m.BareDelete(b, "p/a") |
| + m.Put(b, "p/b", []byte("x")) |
| + m.Put(b, "q/c", []byte("other prefix")) |
| vs, err := st.ListVersions(ctx, b, "p/") |
| if err != nil { |
| t.Fatal(err) |
| } |
| - var got []string |
| - for _, v := range vs { |
| - got = append(got, fmt.Sprintf("%s:%v:%v", v.Key, v.DeleteMarker, v.IsLatest)) |
| + if len(vs) != 4 || vs[0].Key != "p/a" || !vs[0].IsLatest || !vs[0].DeleteMarker || vs[3].Key != "p/b" || !vs[3].IsLatest { |
| + t.Fatalf("listing = %+v", vs) |
| } |
| - want := []string{"p/a:true:true", "p/a:false:false", "p/a:false:false", "p/b:false:true"} |
| - if fmt.Sprint(got) != fmt.Sprint(want) { |
| - t.Fatalf("ListVersions = %v, want %v", got, want) |
| + for _, v := range vs[1:3] { |
| + if v.Key != "p/a" || v.IsLatest || v.DeleteMarker || v.VersionID == "" { |
| + t.Fatalf("non-latest entries = %+v", vs[1:3]) |
| + } |
| } |
| }) |
| - t.Run("listing spans pages and fails whole on a page error", func(t *testing.T) { |
| - st, fb, b := mk(t) |
| - fb.PageSize = 3 |
| - for i := 0; i < 10; i++ { |
| - fb.Put(b, fmt.Sprintf("p/k%02d", i), []byte{byte(i)}) |
| + t.Run("listing spans pages, including many versions of one key", func(t *testing.T) { |
| + st, m, b, _ := mk(t) |
| + m.PageSize = 3 |
| + for i := 0; i < 5; i++ { |
| + m.Put(b, "p/same", []byte{byte(i)}) |
| + } |
| + for i := 0; i < 4; i++ { |
| + m.Put(b, fmt.Sprintf("p/k%02d", i), []byte{byte(i)}) |
| } |
| vs, err := st.ListVersions(ctx, b, "p/") |
| - if err != nil || len(vs) != 10 { |
| - t.Fatalf("paged listing = %d, %v; want 10", len(vs), err) |
| + if err != nil || len(vs) != 9 { |
| + t.Fatalf("paged listing = %d, %v; want 9", len(vs), err) |
| + } |
| + seen := map[string]bool{} |
| + for _, v := range vs { |
| + if seen[v.Key+"@"+v.VersionID] { |
| + t.Fatalf("duplicate %s@%s", v.Key, v.VersionID) |
| + } |
| + seen[v.Key+"@"+v.VersionID] = true |
| + } |
| + }) |
| + |
| + t.Run("listing fails whole on a page error", func(t *testing.T) { |
| + st, m, b, _ := mk(t) |
| + m.PageSize = 2 |
| + for i := 0; i < 6; i++ { |
| + m.Put(b, fmt.Sprintf("p/k%d", i), []byte{byte(i)}) |
| } |
| - fb.FailListAfterPages = 2 |
| + m.FailListAfterPages = 2 |
| if vs, err := st.ListVersions(ctx, b, "p/"); err == nil { |
| - t.Fatalf("listing with a failing page returned %d entries and no error", len(vs)) |
| + t.Fatalf("got %d entries and no error", len(vs)) |
| } |
| }) |
| - t.Run("get and copy by exact version", func(t *testing.T) { |
| - st, fb, b := mk(t) |
| - fb.Put(b, "src/a b+c%", []byte("old")) |
| - v1 := fb.LatestID(b, "src/a b+c%") |
| - fb.Put(b, "src/a b+c%", []byte("new")) |
| - body, err := st.GetVersion(ctx, b, "src/a b+c%", v1) |
| - if err != nil || string(body) != "old" { |
| - t.Fatalf("GetVersion(v1) = %q, %v", body, err) |
| + t.Run("read and stream by exact version; markers and missing", func(t *testing.T) { |
| + st, m, b, _ := mk(t) |
| + k := "dir/a b+c%?d" |
| + v1 := m.Put(b, k, []byte("old")) |
| + m.Put(b, k, []byte("new")) |
| + if body, err := st.GetVersion(ctx, b, k, v1); err != nil || string(body) != "old" { |
| + t.Fatalf("GetVersion = %q, %v", body, err) |
| } |
| - dv, etag, err := st.CopyVersion(ctx, b, "src/a b+c%", v1, b, "dst/x", true) |
| - if err != nil || dv == "" || etag == "" { |
| - t.Fatalf("CopyVersion = %q %q %v", dv, etag, err) |
| + rc, err := st.OpenVersion(ctx, b, k, v1) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + body, _ := io.ReadAll(rc) |
| + rc.Close() |
| + if string(body) != "old" { |
| + t.Fatalf("OpenVersion = %q", body) |
| } |
| - if cur, ok := fb.Current(b, "dst/x"); !ok || string(cur) != "old" { |
| - t.Fatalf("copied body = %q", cur) |
| + mk1 := m.BareDelete(b, k) |
| + if _, err := st.GetVersion(ctx, b, k, mk1); !errors.Is(err, lease.ErrIsDeleteMarker) { |
| + t.Fatalf("GetVersion(marker) = %v, want ErrIsDeleteMarker", err) |
| } |
| - if _, _, err := st.CopyVersion(ctx, b, "src/a b+c%", v1, b, "dst/x", true); !errors.Is(err, lease.ErrDestinationExists) { |
| - t.Fatalf("create-only copy onto an existing object = %v", err) |
| + if _, err := st.GetVersion(ctx, b, k, "v999999"); !errors.Is(err, lease.ErrMirrorNotFound) { |
| + t.Fatalf("GetVersion(missing) = %v, want ErrMirrorNotFound", err) |
| } |
| - if _, err := st.GetVersion(ctx, b, "src/a b+c%", "v999999"); !errors.Is(err, lease.ErrMirrorNotFound) { |
| - t.Fatalf("missing version = %v", err) |
| + // A version hidden behind a marker is still readable by id. |
| + if body, err := st.GetVersion(ctx, b, k, v1); err != nil || string(body) != "old" { |
| + t.Fatalf("GetVersion behind a marker = %q, %v", body, err) |
| } |
| }) |
| - t.Run("create-only copy succeeds over a delete marker", func(t *testing.T) { |
| - st, fb, b := mk(t) |
| - fb.Put(b, "src/k", []byte("s")) |
| - sv := fb.LatestID(b, "src/k") |
| - fb.Put(b, "dst/k", []byte("gone")) |
| - fb.BareDelete(b, "dst/k") |
| - if _, _, err := st.CopyVersion(ctx, b, "src/k", sv, b, "dst/k", true); err != nil { |
| + t.Run("copy by exact version; create-only; markers", func(t *testing.T) { |
| + st, m, b, _ := mk(t) |
| + k := "src/a b+c%?d" |
| + v1 := m.Put(b, k, []byte("old")) |
| + m.Put(b, k, []byte("new")) |
| + dv, etag, err := st.CopyVersion(ctx, b, k, v1, b, "dst/x", true) |
| + if err != nil || dv == "" || etag == "" || strings.Contains(etag, "-") { |
| + t.Fatalf("CopyVersion = %q %q %v", dv, etag, err) |
| + } |
| + if cur, ok := m.Current(b, "dst/x"); !ok || string(cur) != "old" { |
| + t.Fatalf("copied %q", cur) |
| + } |
| + if _, _, err := st.CopyVersion(ctx, b, k, v1, b, "dst/x", true); !errors.Is(err, lease.ErrDestinationExists) { |
| + t.Fatalf("create-only copy onto existing = %v", err) |
| + } |
| + m.BareDelete(b, "dst/x") |
| + if _, _, err := st.CopyVersion(ctx, b, k, v1, b, "dst/x", true); err != nil { |
| t.Fatalf("create-only copy over a delete marker = %v", err) |
| } |
| - }) |
| - |
| - t.Run("multipart source etag becomes single-part on copy", func(t *testing.T) { |
| - st, fb, b := mk(t) |
| - fb.PutMultipart(b, "src/m", []byte("payload")) |
| - sv := fb.LatestID(b, "src/m") |
| - _, etag, err := st.CopyVersion(ctx, b, "src/m", sv, b, "dst/m", false) |
| - if err != nil || bytes.Contains([]byte(etag), []byte("-")) { |
| - t.Fatalf("copied etag = %q, %v; want single-part", etag, err) |
| + mid := m.BareDelete(b, k) |
| + if _, _, err := st.CopyVersion(ctx, b, k, mid, b, "dst/y", false); !errors.Is(err, lease.ErrIsDeleteMarker) { |
| + t.Fatalf("copy of a marker = %v, want ErrIsDeleteMarker", err) |
| + } |
| + mpID := m.PutMultipart(b, "src/mp", []byte("payload")) |
| + if _, etag, err := st.CopyVersion(ctx, b, "src/mp", mpID, b, "dst/mp", false); err != nil || strings.Contains(etag, "-") { |
| + t.Fatalf("multipart copy etag = %q, %v; want single-part", etag, err) |
| } |
| }) |
| - t.Run("delete by version is exact and idempotent", func(t *testing.T) { |
| - st, fb, b := mk(t) |
| - fb.Put(b, "k", []byte("one")) |
| - v1 := fb.LatestID(b, "k") |
| - fb.Put(b, "k", []byte("two")) |
| - v2 := fb.LatestID(b, "k") |
| + t.Run("delete by version: exact, idempotent, never bare", func(t *testing.T) { |
| + st, m, b, served := mk(t) |
| + v1 := m.Put(b, "k", []byte("one")) |
| + v2 := m.Put(b, "k", []byte("two")) |
| if err := st.DeleteVersion(ctx, b, "k", v2); err != nil { |
| t.Fatal(err) |
| } |
| - if cur, ok := fb.Current(b, "k"); !ok || string(cur) != "one" { |
| - t.Fatalf("after deleting the current version, current = %q %v; want the older version", cur, ok) |
| + if cur, ok := m.Current(b, "k"); !ok || string(cur) != "one" { |
| + t.Fatalf("after deleting the current version: %q %v; want the older one current", cur, ok) |
| } |
| if err := st.DeleteVersion(ctx, b, "k", v2); err != nil { |
| - t.Fatalf("deleting an already-deleted version = %v, want success", err) |
| + t.Fatalf("re-delete = %v", err) |
| } |
| - if err := st.DeleteVersion(ctx, b, "k", v1); err != nil { |
| - t.Fatal(err) |
| + if err := st.DeleteVersion(ctx, b, "never", "v000001"); err != nil { |
| + t.Fatalf("delete on a missing key = %v", err) |
| } |
| - if _, ok := fb.Current(b, "k"); ok { |
| - t.Fatal("key still has a current version after deleting every version") |
| + before, reqs := m.Snapshot(), 0 |
| + if served != nil { |
| + reqs = served() |
| } |
| - if err := st.DeleteVersion(ctx, b, "never", "v000001"); err != nil { |
| - t.Fatalf("deleting a version of a missing key = %v, want success", err) |
| + if err := st.DeleteVersion(ctx, b, "k", ""); !errors.Is(err, lease.ErrNoVersionID) { |
| + t.Fatalf("empty version id = %v, want ErrNoVersionID", err) |
| + } |
| + if m.Snapshot() != before { |
| + t.Fatal("state changed on an empty-version delete") |
| } |
| + if served != nil && served() != reqs { |
| + t.Fatal("a request was sent for an empty-version delete") |
| + } |
| + _ = v1 |
| }) |
| t.Run("put if absent", func(t *testing.T) { |
| - st, fb, b := mk(t) |
| - v, err := st.PutIfAbsent(ctx, b, "m/closed.json", []byte("{}")) |
| - if err != nil || v == "" { |
| - t.Fatalf("first PutIfAbsent = %q %v", v, err) |
| + st, m, b, _ := mk(t) |
| + if v, err := st.PutIfAbsent(ctx, b, "m/closed.json", []byte("{}")); err != nil || v == "" { |
| + t.Fatalf("first = %q %v", v, err) |
| } |
| if _, err := st.PutIfAbsent(ctx, b, "m/closed.json", []byte("{}")); !errors.Is(err, lease.ErrExists) { |
| - t.Fatalf("second PutIfAbsent = %v, want ErrExists", err) |
| + t.Fatalf("second = %v, want ErrExists", err) |
| } |
| - fb.BareDelete(b, "m/closed.json") |
| + m.BareDelete(b, "m/closed.json") |
| if _, err := st.PutIfAbsent(ctx, b, "m/closed.json", []byte("{}")); err != nil { |
| - t.Fatalf("PutIfAbsent over a delete marker = %v, want success", err) |
| + t.Fatalf("over a marker = %v", err) |
| + } |
| + }) |
| + |
| + t.Run("injected failures surface as errors from every operation", func(t *testing.T) { |
| + st, m, b, _ := mk(t) |
| + v := m.Put(b, "k", []byte("x")) |
| + boom := errors.New("boom") |
| + m.FailOn = func(op, key string) error { return boom } |
| + if _, err := st.ListVersions(ctx, b, ""); err == nil { |
| + t.Error("list: no error") |
| + } |
| + if _, err := st.GetVersion(ctx, b, "k", v); err == nil || errors.Is(err, lease.ErrMirrorNotFound) { |
| + t.Errorf("get: %v", err) |
| + } |
| + if _, _, err := st.CopyVersion(ctx, b, "k", v, b, "k2", false); err == nil || errors.Is(err, lease.ErrMirrorNotFound) { |
| + t.Errorf("copy: %v", err) |
| + } |
| + if err := st.DeleteVersion(ctx, b, "k", v); err == nil { |
| + t.Error("delete: no error") |
| + } |
| + if _, err := st.PutIfAbsent(ctx, b, "new", []byte("x")); err == nil { |
| + t.Error("put: no error") |
| + } |
| + m.FailOn = nil |
| + if _, ok := m.Current(b, "k"); !ok { |
| + t.Fatal("a failed delete removed data") |
| } |
| }) |
| } |
| ``` |
| -- [ ] **Step 2: Write `bucket_test.go`, which runs the contract against the fake** |
| +- [ ] **Step 2: Write `bucket_test.go`** |
| ```go |
| package leasetest |
| @@ -377,30 +557,32 @@ import ( |
| ) |
| func TestVersionedBucketContract(t *testing.T) { |
| - RunVersionedStoreContract(t, func(t *testing.T) (lease.VersionedStore, *VersionedBucket, string) { |
| - fb := NewVersionedBucket() |
| - fb.AddBucket("data") |
| - return fb, fb, "data" |
| + RunVersionedStoreContract(t, func(t *testing.T) (lease.VersionedStore, *VersionedBucket, string, func() int) { |
| + m := NewVersionedBucket() |
| + m.AddBucket("data") |
| + return m, m, "data", nil |
| }) |
| } |
| ``` |
| Run: `go test ./internal/lease/leasetest/ -count=1` |
| -Expected: FAIL (`undefined: NewVersionedBucket`). |
| +Expected: FAIL, with `undefined: NewVersionedBucket`. |
| - [ ] **Step 3: Implement `bucket.go`** |
| ```go |
| -// Package leasetest holds test doubles for internal/lease. Its |
| -// VersionedBucket is the reference model of a versioned S3 bucket that |
| -// every plan of the host lifecycle claim tests against (spec §13). |
| +// Package leasetest holds test doubles for internal/lease. VersionedBucket |
| +// is the reference model of a versioned S3 bucket that every plan of the |
| +// host lifecycle claim tests against (spec §13). |
| package leasetest |
| import ( |
| + "bytes" |
| "context" |
| "crypto/md5" |
| "encoding/hex" |
| "fmt" |
| + "io" |
| "sort" |
| "strings" |
| "sync" |
| @@ -424,16 +606,9 @@ type VersionedBucket struct { |
| next int |
| clock time.Time |
| - // PageSize is the listing page size the S3 server and ListVersions |
| - // use (default 1000). |
| - PageSize int |
| - // FailListAfterPages fails a listing once this many pages have been |
| - // served (0: never). |
| - FailListAfterPages int |
| - // FailOn, when set, is consulted before every operation, with op one |
| - // of "list", "get", "copy", "delete" or "put" and the key; a non-nil |
| - // result fails the call. |
| - FailOn func(op, key string) error |
| + PageSize int // listing page size; default 1000 |
| + FailListAfterPages int // fail a listing once this many pages have been served; 0 never |
| + FailOn func(op, key string) error // op: list|get|copy|delete|put |
| } |
| func NewVersionedBucket() *VersionedBucket { |
| @@ -441,6 +616,8 @@ func NewVersionedBucket() *VersionedBucket { |
| clock: time.Date(2026, 10, 5, 0, 0, 0, 0, time.UTC)} |
| } |
| +func md5hex(b []byte) string { s := md5.Sum(b); return hex.EncodeToString(s[:]) } |
| + |
| func (f *VersionedBucket) AddBucket(name string) { |
| f.mu.Lock() |
| defer f.mu.Unlock() |
| @@ -449,17 +626,7 @@ func (f *VersionedBucket) AddBucket(name string) { |
| } |
| } |
| -func (f *VersionedBucket) newID() string { f.next++; return fmt.Sprintf("v%06d", f.next) } |
| -func (f *VersionedBucket) tick() time.Time { f.clock = f.clock.Add(time.Second); return f.clock } |
| -func md5hex(b []byte) string { s := md5.Sum(b); return hex.EncodeToString(s[:]) } |
| - |
| -func (f *VersionedBucket) objs(bucket string) (map[string][]version, error) { |
| - o, ok := f.buckets[bucket] |
| - if !ok { |
| - return nil, fmt.Errorf("leasetest: no bucket %q", bucket) |
| - } |
| - return o, nil |
| -} |
| +// --- locked helpers: callers hold f.mu --- |
| func (f *VersionedBucket) fail(op, key string) error { |
| if f.FailOn != nil { |
| @@ -468,35 +635,100 @@ func (f *VersionedBucket) fail(op, key string) error { |
| return nil |
| } |
| +func (f *VersionedBucket) known(bucket string) error { |
| + if _, ok := f.buckets[bucket]; !ok { |
| + return fmt.Errorf("leasetest: no bucket %q", bucket) |
| + } |
| + return nil |
| +} |
| + |
| func (f *VersionedBucket) push(bucket, key string, v version) string { |
| - o := f.buckets[bucket] |
| - v.id, v.modified = f.newID(), f.tick() |
| - o[key] = append([]version{v}, o[key]...) |
| + f.next++ |
| + f.clock = f.clock.Add(time.Second) |
| + v.id, v.modified = fmt.Sprintf("v%06d", f.next), f.clock |
| + f.buckets[bucket][key] = append([]version{v}, f.buckets[bucket][key]...) |
| return v.id |
| } |
| -// Put stores a new current version (single-part ETag). |
| +func (f *VersionedBucket) hasCurrent(bucket, key string) bool { |
| + vs := f.buckets[bucket][key] |
| + return len(vs) > 0 && !vs[0].deleteMarker |
| +} |
| + |
| +func (f *VersionedBucket) find(bucket, key, id string) (version, bool) { |
| + for _, v := range f.buckets[bucket][key] { |
| + if v.id == id { |
| + return v, true |
| + } |
| + } |
| + return version{}, false |
| +} |
| + |
| +// listAll is the whole ordered listing under prefix (key order, newest |
| +// first within a key). |
| +func (f *VersionedBucket) listAll(bucket, prefix string) []lease.ObjectVersion { |
| + o := f.buckets[bucket] |
| + keys := make([]string, 0, len(o)) |
| + for k := range o { |
| + if strings.HasPrefix(k, prefix) { |
| + keys = append(keys, k) |
| + } |
| + } |
| + sort.Strings(keys) |
| + var all []lease.ObjectVersion |
| + for _, k := range keys { |
| + for i, v := range o[k] { |
| + ov := lease.ObjectVersion{Key: k, VersionID: v.id, IsLatest: i == 0, DeleteMarker: v.deleteMarker, LastModified: v.modified} |
| + if !v.deleteMarker { |
| + ov.ETag, ov.Size = v.etag, int64(len(v.body)) |
| + } |
| + all = append(all, ov) |
| + } |
| + } |
| + return all |
| +} |
| + |
| +// pageOf returns page number page (0-based) of all, whether more follow, |
| +// and the failure FailListAfterPages injects. |
| +func (f *VersionedBucket) pageOf(all []lease.ObjectVersion, page int) ([]lease.ObjectVersion, bool, error) { |
| + if f.FailListAfterPages > 0 && page >= f.FailListAfterPages { |
| + return nil, false, fmt.Errorf("leasetest: injected listing failure on page %d", page+1) |
| + } |
| + size := f.PageSize |
| + if size <= 0 { |
| + size = 1000 |
| + } |
| + start := page * size |
| + if start > len(all) { |
| + start = len(all) |
| + } |
| + end := start + size |
| + if end >= len(all) { |
| + return all[start:], false, nil |
| + } |
| + return all[start:end], true, nil |
| +} |
| + |
| +// --- exported helpers for tests --- |
| + |
| func (f *VersionedBucket) Put(bucket, key string, body []byte) string { |
| f.mu.Lock() |
| defer f.mu.Unlock() |
| return f.push(bucket, key, version{body: append([]byte(nil), body...), etag: md5hex(body)}) |
| } |
| -// PutMultipart stores a new current version with a multipart-shaped ETag. |
| func (f *VersionedBucket) PutMultipart(bucket, key string, body []byte) string { |
| f.mu.Lock() |
| defer f.mu.Unlock() |
| return f.push(bucket, key, version{body: append([]byte(nil), body...), etag: md5hex(body) + "-2"}) |
| } |
| -// BareDelete adds a delete marker, as a delete without a version id does. |
| func (f *VersionedBucket) BareDelete(bucket, key string) string { |
| f.mu.Lock() |
| defer f.mu.Unlock() |
| return f.push(bucket, key, version{deleteMarker: true}) |
| } |
| -// LatestID is the id of the newest entry (version or marker) of key. |
| func (f *VersionedBucket) LatestID(bucket, key string) string { |
| f.mu.Lock() |
| defer f.mu.Unlock() |
| @@ -506,8 +738,6 @@ func (f *VersionedBucket) LatestID(bucket, key string) string { |
| return "" |
| } |
| -// Current returns the current object body: false when the key is absent or |
| -// its newest entry is a delete marker. |
| func (f *VersionedBucket) Current(bucket, key string) ([]byte, bool) { |
| f.mu.Lock() |
| defer f.mu.Unlock() |
| @@ -518,58 +748,39 @@ func (f *VersionedBucket) Current(bucket, key string) ([]byte, bool) { |
| return vs[0].body, true |
| } |
| -func (f *VersionedBucket) hasCurrent(bucket, key string) bool { |
| - vs := f.buckets[bucket][key] |
| - return len(vs) > 0 && !vs[0].deleteMarker |
| -} |
| - |
| -// listPage returns one page of the listing and whether it is truncated; |
| -// page counts served pages for FailListAfterPages. |
| -func (f *VersionedBucket) listPage(bucket, prefix string, start, page int) ([]lease.ObjectVersion, int, bool, error) { |
| - if f.FailListAfterPages > 0 && page >= f.FailListAfterPages { |
| - return nil, 0, false, fmt.Errorf("leasetest: injected listing failure on page %d", page+1) |
| - } |
| - if err := f.fail("list", prefix); err != nil { |
| - return nil, 0, false, err |
| - } |
| - o, err := f.objs(bucket) |
| - if err != nil { |
| - return nil, 0, false, err |
| +// Snapshot renders the whole state, for before/after comparison. |
| +func (f *VersionedBucket) Snapshot() string { |
| + f.mu.Lock() |
| + defer f.mu.Unlock() |
| + var b strings.Builder |
| + names := make([]string, 0, len(f.buckets)) |
| + for n := range f.buckets { |
| + names = append(names, n) |
| } |
| - keys := make([]string, 0, len(o)) |
| - for k := range o { |
| - if strings.HasPrefix(k, prefix) { |
| - keys = append(keys, k) |
| + sort.Strings(names) |
| + for _, n := range names { |
| + for _, v := range f.listAll(n, "") { |
| + fmt.Fprintf(&b, "%s/%s@%s:%v:%s\n", n, v.Key, v.VersionID, v.DeleteMarker, v.ETag) |
| } |
| } |
| - sort.Strings(keys) |
| - var all []lease.ObjectVersion |
| - for _, k := range keys { |
| - for i, v := range o[k] { |
| - ov := lease.ObjectVersion{Key: k, VersionID: v.id, IsLatest: i == 0, DeleteMarker: v.deleteMarker, LastModified: v.modified} |
| - if !v.deleteMarker { |
| - ov.ETag, ov.Size = v.etag, int64(len(v.body)) |
| - } |
| - all = append(all, ov) |
| - } |
| - } |
| - size := f.PageSize |
| - if size <= 0 { |
| - size = 1000 |
| - } |
| - end := start + size |
| - if end >= len(all) { |
| - return all[start:], len(all), false, nil |
| - } |
| - return all[start:end], end, true, nil |
| + return b.String() |
| } |
| +// --- lease.VersionedStore --- |
| + |
| func (f *VersionedBucket) ListVersions(ctx context.Context, bucket, prefix string) ([]lease.ObjectVersion, error) { |
| f.mu.Lock() |
| defer f.mu.Unlock() |
| + if err := f.known(bucket); err != nil { |
| + return nil, err |
| + } |
| + if err := f.fail("list", prefix); err != nil { |
| + return nil, err |
| + } |
| + all := f.listAll(bucket, prefix) |
| var out []lease.ObjectVersion |
| - for start, page := 0, 0; ; page++ { |
| - vs, next, more, err := f.listPage(bucket, prefix, start, page) |
| + for page := 0; ; page++ { |
| + vs, more, err := f.pageOf(all, page) |
| if err != nil { |
| return nil, err |
| } |
| @@ -577,89 +788,108 @@ func (f *VersionedBucket) ListVersions(ctx context.Context, bucket, prefix strin |
| if !more { |
| return out, nil |
| } |
| - start = next |
| } |
| } |
| -func (f *VersionedBucket) find(bucket, key, id string) (version, bool) { |
| - for _, v := range f.buckets[bucket][key] { |
| - if v.id == id { |
| - return v, true |
| - } |
| - } |
| - return version{}, false |
| -} |
| - |
| -func (f *VersionedBucket) GetVersion(ctx context.Context, bucket, key, versionID string) ([]byte, error) { |
| - f.mu.Lock() |
| - defer f.mu.Unlock() |
| - if err := f.fail("get", key); err != nil { |
| +func (f *VersionedBucket) read(bucket, key, versionID string) ([]byte, error) { |
| + if err := f.known(bucket); err != nil { |
| return nil, err |
| } |
| - if _, err := f.objs(bucket); err != nil { |
| + if err := f.fail("get", key); err != nil { |
| return nil, err |
| } |
| v, ok := f.find(bucket, key, versionID) |
| - if !ok || v.deleteMarker { |
| + if !ok { |
| return nil, lease.ErrMirrorNotFound |
| } |
| + if v.deleteMarker { |
| + return nil, lease.ErrIsDeleteMarker |
| + } |
| return append([]byte(nil), v.body...), nil |
| } |
| -func (f *VersionedBucket) CopyVersion(ctx context.Context, srcBucket, srcKey, srcVersionID, dstBucket, dstKey string, createOnly bool) (string, string, error) { |
| +func (f *VersionedBucket) GetVersion(ctx context.Context, bucket, key, versionID string) ([]byte, error) { |
| f.mu.Lock() |
| defer f.mu.Unlock() |
| - if err := f.fail("copy", srcKey); err != nil { |
| + return f.read(bucket, key, versionID) |
| +} |
| + |
| +func (f *VersionedBucket) OpenVersion(ctx context.Context, bucket, key, versionID string) (io.ReadCloser, error) { |
| + f.mu.Lock() |
| + defer f.mu.Unlock() |
| + b, err := f.read(bucket, key, versionID) |
| + if err != nil { |
| + return nil, err |
| + } |
| + return io.NopCloser(bytes.NewReader(b)), nil |
| +} |
| + |
| +func (f *VersionedBucket) copyLocked(srcBucket, srcKey, srcVersionID, dstBucket, dstKey string, createOnly bool) (string, string, error) { |
| + if err := f.known(srcBucket); err != nil { |
| return "", "", err |
| } |
| - if _, err := f.objs(srcBucket); err != nil { |
| + if err := f.known(dstBucket); err != nil { |
| return "", "", err |
| } |
| - if _, err := f.objs(dstBucket); err != nil { |
| + if err := f.fail("copy", srcKey); err != nil { |
| return "", "", err |
| } |
| v, ok := f.find(srcBucket, srcKey, srcVersionID) |
| - if !ok || v.deleteMarker { |
| + if !ok { |
| return "", "", lease.ErrMirrorNotFound |
| } |
| + if v.deleteMarker { |
| + return "", "", lease.ErrIsDeleteMarker |
| + } |
| if createOnly && f.hasCurrent(dstBucket, dstKey) { |
| return "", "", lease.ErrDestinationExists |
| } |
| - etag := md5hex(v.body) // a copy is always single-part (§13 row 9) |
| - id := f.push(dstBucket, dstKey, version{body: append([]byte(nil), v.body...), etag: etag}) |
| - return id, etag, nil |
| + etag := md5hex(v.body) // a copy is single-part (§13 row 9) |
| + return f.push(dstBucket, dstKey, version{body: append([]byte(nil), v.body...), etag: etag}), etag, nil |
| } |
| -func (f *VersionedBucket) DeleteVersion(ctx context.Context, bucket, key, versionID string) error { |
| +func (f *VersionedBucket) CopyVersion(ctx context.Context, srcBucket, srcKey, srcVersionID, dstBucket, dstKey string, createOnly bool) (string, string, error) { |
| f.mu.Lock() |
| defer f.mu.Unlock() |
| - if err := f.fail("delete", key); err != nil { |
| + return f.copyLocked(srcBucket, srcKey, srcVersionID, dstBucket, dstKey, createOnly) |
| +} |
| + |
| +func (f *VersionedBucket) deleteLocked(bucket, key, versionID string) error { |
| + if versionID == "" { |
| + return lease.ErrNoVersionID |
| + } |
| + if err := f.known(bucket); err != nil { |
| return err |
| } |
| - o, err := f.objs(bucket) |
| - if err != nil { |
| + if err := f.fail("delete", key); err != nil { |
| return err |
| } |
| - vs := o[key] |
| + vs := f.buckets[bucket][key] |
| for i, v := range vs { |
| if v.id == versionID { |
| - o[key] = append(vs[:i:i], vs[i+1:]...) |
| - if len(o[key]) == 0 { |
| - delete(o, key) |
| + rest := append(vs[:i:i], vs[i+1:]...) |
| + if len(rest) == 0 { |
| + delete(f.buckets[bucket], key) |
| + } else { |
| + f.buckets[bucket][key] = rest |
| } |
| return nil |
| } |
| } |
| - return nil // already gone: success |
| + return nil |
| } |
| -func (f *VersionedBucket) PutIfAbsent(ctx context.Context, bucket, key string, body []byte) (string, error) { |
| +func (f *VersionedBucket) DeleteVersion(ctx context.Context, bucket, key, versionID string) error { |
| f.mu.Lock() |
| defer f.mu.Unlock() |
| - if err := f.fail("put", key); err != nil { |
| + return f.deleteLocked(bucket, key, versionID) |
| +} |
| + |
| +func (f *VersionedBucket) putIfAbsentLocked(bucket, key string, body []byte) (string, error) { |
| + if err := f.known(bucket); err != nil { |
| return "", err |
| } |
| - if _, err := f.objs(bucket); err != nil { |
| + if err := f.fail("put", key); err != nil { |
| return "", err |
| } |
| if f.hasCurrent(bucket, key) { |
| @@ -668,67 +898,50 @@ func (f *VersionedBucket) PutIfAbsent(ctx context.Context, bucket, key string, b |
| return f.push(bucket, key, version{body: append([]byte(nil), body...), etag: md5hex(body)}), nil |
| } |
| +func (f *VersionedBucket) PutIfAbsent(ctx context.Context, bucket, key string, body []byte) (string, error) { |
| + f.mu.Lock() |
| + defer f.mu.Unlock() |
| + return f.putIfAbsentLocked(bucket, key, body) |
| +} |
| + |
| var _ lease.VersionedStore = (*VersionedBucket)(nil) |
| ``` |
| -- [ ] **Step 4: Run the contract against the fake** |
| +- [ ] **Step 4: Run** |
| Run: `go test ./internal/lease/leasetest/ -count=1 -race` |
| -Expected: PASS (every subtest). |
| +Expected: PASS. |
| - [ ] **Step 5: Commit** |
| ```bash |
| git add internal/lease/leasetest/ |
| -git commit -m "leasetest: the versioned bucket model and the VersionedStore contract suite (spec §13)" |
| +git commit -m "leasetest: the versioned bucket model and the VersionedStore contract (spec §13)" |
| ``` |
| --- |
| -### Task 3: The S3 server over the fake, and `s3Mirror.ListVersions` |
| +### Task 3: The S3 server, and `versionedS3.ListVersions` |
| **Files:** |
| - Create: `internal/lease/leasetest/s3server.go`, `internal/lease/versioned_test.go` |
| -- Modify: `internal/lease/versioned.go` (add `s3Mirror.ListVersions`) |
| +- Modify: `internal/lease/versioned.go` |
| **Interfaces:** |
| -- Consumes: Tasks 1 and 2, and the existing `s3Mirror` in |
| - `internal/lease/mirror.go` (`clientFor`, the `byBucket` memo, |
| - `normalizeETag`). |
| - Produces: |
| - - `leasetest.NewS3Server(f *VersionedBucket) http.Handler`; |
| - - `func (m *s3Mirror) ListVersions(ctx, bucket, prefix string) ([]ObjectVersion, error)`; |
| - - the test helper `newWireMirror(t) (*s3Mirror, *leasetest.VersionedBucket)` |
| - in `versioned_test.go`. |
| - |
| -**The server handles path-style requests, `/{bucket}/{key...}`:** |
| -- `GET /{bucket}?versions&prefix=&key-marker=&version-id-marker=` |
| - returns a `ListVersionsResult` XML page. The marker pair resumes after |
| - the given (key, version id). |
| -- `GET /{bucket}/{key}?versionId=` returns the body. A missing version |
| - gets 404 with `<Code>NoSuchVersion</Code>`. |
| -- `PUT /{bucket}/{key}` with `x-amz-copy-source: /{srcBucket}/{escaped key}?versionId=…` |
| - is a copy: |
| - - `If-None-Match: *` makes it create-only (412 `PreconditionFailed` |
| - when a current version exists); |
| - - it returns `CopyObjectResult` XML with ETag, and the header |
| - `x-amz-version-id`. |
| -- `PUT /{bucket}/{key}` without the copy header is a put, with |
| - `If-None-Match: *` honoured. It returns header `x-amz-version-id`. |
| -- `DELETE /{bucket}/{key}?versionId=` deletes that version and returns |
| - 204. A missing version also returns 204. A DELETE **without** |
| - `versionId` adds a delete marker; the interface never sends one, but |
| - the server models it. |
| -- Anything else gets 405. |
| - |
| -Paging is built on the fake's `listPage`. The server uses the fake's |
| -`PageSize` and `FailListAfterPages`, and an injected failure returns 500 |
| -`<Code>InternalError</Code>`. |
| - |
| -- [ ] **Step 1: Write the failing wire test** |
| + - `leasetest.NewS3Server(f *VersionedBucket) (http.Handler, func() int)`. |
| + The second value counts the requests served. |
| + - `versionedS3.ListVersions`. |
| + |
| +The server keeps `f.mu` held for each request, and calls only the |
| +model's locked helpers (`listAll`, `pageOf`, `read`, `copyLocked`, |
| +`deleteLocked`, `putIfAbsentLocked`). It never calls an exported method |
| +that would take the lock again. |
| + |
| +- [ ] **Step 1: Write the failing wire test (external test package)** |
| ```go |
| -package lease |
| +package lease_test |
| import ( |
| "context" |
| @@ -739,50 +952,55 @@ import ( |
| "github.com/aws/aws-sdk-go-v2/credentials" |
| awss3 "github.com/aws/aws-sdk-go-v2/service/s3" |
| + "github.com/carlosframework/platform/internal/lease" |
| "github.com/carlosframework/platform/internal/lease/leasetest" |
| ) |
| -// newWireMirror is an s3Mirror whose clients talk to NewS3Server over the |
| -// wire, so the production request building and response parsing are what |
| -// is tested. |
| -func newWireMirror(t *testing.T) (*s3Mirror, *leasetest.VersionedBucket) { |
| +func wireClient(t *testing.T, url string) *awss3.Client { |
| t.Helper() |
| - fb := leasetest.NewVersionedBucket() |
| - fb.AddBucket("data") |
| - fb.AddBucket("other") |
| - srv := httptest.NewServer(leasetest.NewS3Server(fb)) |
| - t.Cleanup(srv.Close) |
| - c := awss3.New(awss3.Options{ |
| - Region: "eu-west-1", BaseEndpoint: aws.String(srv.URL), UsePathStyle: true, |
| + return awss3.New(awss3.Options{ |
| + Region: "eu-west-1", BaseEndpoint: aws.String(url), UsePathStyle: true, |
| Credentials: credentials.NewStaticCredentialsProvider("k", "s", ""), |
| RetryMaxAttempts: 1, |
| }) |
| - return &s3Mirror{src: c, dst: c, byBucket: map[string]*awss3.Client{}}, fb |
| } |
| -func TestS3MirrorVersionedContract(t *testing.T) { |
| - leasetest.RunVersionedStoreContract(t, func(t *testing.T) (VersionedStore, *leasetest.VersionedBucket, string) { |
| - m, fb := newWireMirror(t) |
| - return m, fb, "data" |
| +// wireStore is a versionedS3 whose buckets "data" and "other" are served |
| +// by two separate servers over one model, so a wrong-region routing is a |
| +// visible failure. |
| +func wireStore(t *testing.T) (lease.VersionedStore, *leasetest.VersionedBucket, func() int) { |
| + t.Helper() |
| + m := leasetest.NewVersionedBucket() |
| + m.AddBucket("data") |
| + m.AddBucket("other") |
| + h1, n1 := leasetest.NewS3Server(m) |
| + h2, n2 := leasetest.NewS3Server(m) |
| + s1, s2 := httptest.NewServer(h1), httptest.NewServer(h2) |
| + t.Cleanup(s1.Close) |
| + t.Cleanup(s2.Close) |
| + st := lease.NewVersionedS3WithClients(map[string]*awss3.Client{ |
| + "data": wireClient(t, s1.URL), "other": wireClient(t, s2.URL), |
| }) |
| + return st, m, func() int { return n1() + n2() } |
| } |
| -func TestS3MirrorListVersionsPagesAcrossMarkers(t *testing.T) { |
| - m, fb := newWireMirror(t) |
| - fb.PageSize = 2 |
| - for i := 0; i < 3; i++ { |
| - fb.Put("data", "p/a", []byte{byte(i)}) // three versions of one key span pages |
| - } |
| - fb.Put("data", "p/b", []byte("b")) |
| - vs, err := m.ListVersions(context.Background(), "data", "p/") |
| - if err != nil || len(vs) != 4 { |
| - t.Fatalf("ListVersions over pages = %d, %v; want 4", len(vs), err) |
| +func TestVersionedS3Contract(t *testing.T) { |
| + leasetest.RunVersionedStoreContract(t, func(t *testing.T) (lease.VersionedStore, *leasetest.VersionedBucket, string, func() int) { |
| + st, m, served := wireStore(t) |
| + return st, m, "data", served |
| + }) |
| +} |
| + |
| +func TestVersionedS3RefusesUnboundBucket(t *testing.T) { |
| + st, _, _ := wireStore(t) |
| + if _, err := st.ListVersions(context.Background(), "unbound", ""); err == nil { |
| + t.Fatal("listing an unbound bucket did not fail") |
| } |
| } |
| ``` |
| -Run: `go test ./internal/lease/ -run 'TestS3MirrorVersioned|TestS3MirrorListVersions' -count=1` |
| -Expected: FAIL (`undefined: leasetest.NewS3Server`). |
| +Run: `go test ./internal/lease/ -run 'TestVersionedS3' -count=1` |
| +Expected: FAIL, with `undefined: leasetest.NewS3Server`. |
| - [ ] **Step 2: Implement `s3server.go`** |
| @@ -791,11 +1009,15 @@ package leasetest |
| import ( |
| "encoding/xml" |
| + "errors" |
| "io" |
| "net/http" |
| "net/url" |
| "strings" |
| + "sync/atomic" |
| "time" |
| + |
| + "github.com/carlosframework/platform/internal/lease" |
| ) |
| type xmlVersion struct { |
| @@ -835,12 +1057,29 @@ func s3err(w http.ResponseWriter, status int, code string) { |
| io.WriteString(w, "<Error><Code>"+code+"</Code><Message>"+code+"</Message></Error>") |
| } |
| +// writeErr maps a model error onto the S3 response real S3 gives. |
| +func writeErr(w http.ResponseWriter, err error) { |
| + switch { |
| + case errors.Is(err, lease.ErrMirrorNotFound): |
| + s3err(w, http.StatusNotFound, "NoSuchVersion") |
| + case errors.Is(err, lease.ErrIsDeleteMarker): |
| + w.Header().Set("x-amz-delete-marker", "true") |
| + s3err(w, http.StatusMethodNotAllowed, "MethodNotAllowed") |
| + case errors.Is(err, lease.ErrDestinationExists), errors.Is(err, lease.ErrExists): |
| + s3err(w, http.StatusPreconditionFailed, "PreconditionFailed") |
| + default: |
| + s3err(w, http.StatusInternalServerError, "InternalError") |
| + } |
| +} |
| + |
| // NewS3Server speaks the subset of the S3 wire protocol VersionedStore uses, |
| -// path-style, over f. |
| -func NewS3Server(f *VersionedBucket) http.Handler { |
| - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| - path := strings.TrimPrefix(r.URL.EscapedPath(), "/") |
| - bucket, rawKey, _ := strings.Cut(path, "/") |
| +// path-style, over f, holding f.mu for each request. The second result |
| +// counts requests served. |
| +func NewS3Server(f *VersionedBucket) (http.Handler, func() int) { |
| + var n atomic.Int64 |
| + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + n.Add(1) |
| + bucket, rawKey, _ := strings.Cut(strings.TrimPrefix(r.URL.EscapedPath(), "/"), "/") |
| key, err := url.PathUnescape(rawKey) |
| if err != nil { |
| s3err(w, http.StatusBadRequest, "InvalidURI") |
| @@ -848,9 +1087,8 @@ func NewS3Server(f *VersionedBucket) http.Handler { |
| } |
| q := r.URL.Query() |
| f.mu.Lock() |
| - _, known := f.buckets[bucket] |
| - f.mu.Unlock() |
| - if !known { |
| + defer f.mu.Unlock() |
| + if err := f.known(bucket); err != nil { |
| s3err(w, http.StatusNotFound, "NoSuchBucket") |
| return |
| } |
| @@ -858,9 +1096,9 @@ func NewS3Server(f *VersionedBucket) http.Handler { |
| case r.Method == http.MethodGet && key == "" && q.Has("versions"): |
| f.serveList(w, bucket, q) |
| case r.Method == http.MethodGet && key != "": |
| - body, err := f.GetVersion(r.Context(), bucket, key, q.Get("versionId")) |
| + body, err := f.read(bucket, key, q.Get("versionId")) |
| if err != nil { |
| - s3err(w, http.StatusNotFound, "NoSuchVersion") |
| + writeErr(w, err) |
| return |
| } |
| w.Write(body) |
| @@ -868,36 +1106,40 @@ func NewS3Server(f *VersionedBucket) http.Handler { |
| f.serveCopy(w, r, bucket, key) |
| case r.Method == http.MethodPut: |
| body, _ := io.ReadAll(r.Body) |
| - f.mu.Lock() |
| - if r.Header.Get("If-None-Match") == "*" && f.hasCurrent(bucket, key) { |
| - f.mu.Unlock() |
| - s3err(w, http.StatusPreconditionFailed, "PreconditionFailed") |
| + if r.Header.Get("If-None-Match") != "*" { |
| + s3err(w, http.StatusNotImplemented, "NotImplemented") // VersionedStore only puts create-only |
| + return |
| + } |
| + id, err := f.putIfAbsentLocked(bucket, key, body) |
| + if err != nil { |
| + writeErr(w, err) |
| return |
| } |
| - id := f.push(bucket, key, version{body: body, etag: md5hex(body)}) |
| - f.mu.Unlock() |
| w.Header().Set("x-amz-version-id", id) |
| w.Header().Set("ETag", `"`+md5hex(body)+`"`) |
| - case r.Method == http.MethodDelete && key != "": |
| - if v := q.Get("versionId"); v != "" { |
| - _ = f.DeleteVersion(r.Context(), bucket, key, v) |
| - } else { |
| - f.BareDelete(bucket, key) |
| + case r.Method == http.MethodDelete && key != "" && q.Get("versionId") != "": |
| + if err := f.deleteLocked(bucket, key, q.Get("versionId")); err != nil { |
| + writeErr(w, err) |
| + return |
| } |
| w.WriteHeader(http.StatusNoContent) |
| default: |
| s3err(w, http.StatusMethodNotAllowed, "MethodNotAllowed") |
| } |
| }) |
| + return h, func() int { return int(n.Load()) } |
| } |
| func (f *VersionedBucket) serveList(w http.ResponseWriter, bucket string, q url.Values) { |
| - prefix, km, vm := q.Get("prefix"), q.Get("key-marker"), q.Get("version-id-marker") |
| - f.mu.Lock() |
| - // Resume after (km, vm): find its position in the full ordered listing. |
| - all, _, _, err := f.listPageAll(bucket, prefix) |
| + if err := f.fail("list", q.Get("prefix")); err != nil { |
| + writeErr(w, err) |
| + return |
| + } |
| + all := f.listAll(bucket, q.Get("prefix")) |
| + km, vm := q.Get("key-marker"), q.Get("version-id-marker") |
| start := 0 |
| - if err == nil && km != "" { |
| + if km != "" { |
| + start = len(all) // a marker not found resumes at the end |
| for i, v := range all { |
| if v.Key == km && v.VersionID == vm { |
| start = i + 1 |
| @@ -905,21 +1147,20 @@ func (f *VersionedBucket) serveList(w http.ResponseWriter, bucket string, q url. |
| } |
| } |
| } |
| - page := 0 |
| - if start > 0 { |
| - page = (start + f.pageSize() - 1) / f.pageSize() |
| + size := f.PageSize |
| + if size <= 0 { |
| + size = 1000 |
| } |
| - var vs []objectRow |
| - var more bool |
| - if err == nil { |
| - vs, more, err = f.slice(all, start, page) |
| + page := (start + size - 1) / size // pages already served before this one |
| + vs, more, err := f.pageOf(all[start:], 0) |
| + if f.FailListAfterPages > 0 && page >= f.FailListAfterPages { |
| + err = errors.New("leasetest: injected listing failure") |
| } |
| - f.mu.Unlock() |
| if err != nil { |
| - s3err(w, http.StatusInternalServerError, "InternalError") |
| + writeErr(w, err) |
| return |
| } |
| - res := listVersionsResult{Name: bucket, Prefix: prefix, KeyMarker: km, VersionIdMarker: vm, MaxKeys: f.pageSize(), IsTruncated: more} |
| + res := listVersionsResult{Name: bucket, Prefix: q.Get("prefix"), KeyMarker: km, VersionIdMarker: vm, MaxKeys: size, IsTruncated: more} |
| for _, v := range vs { |
| lm := v.LastModified.UTC().Format(time.RFC3339) |
| if v.DeleteMarker { |
| @@ -929,8 +1170,7 @@ func (f *VersionedBucket) serveList(w http.ResponseWriter, bucket string, q url. |
| } |
| } |
| if more && len(vs) > 0 { |
| - last := vs[len(vs)-1] |
| - res.NextKeyMarker, res.NextVersionIdMarker = last.Key, last.VersionID |
| + res.NextKeyMarker, res.NextVersionIdMarker = vs[len(vs)-1].Key, vs[len(vs)-1].VersionID |
| } |
| w.Header().Set("Content-Type", "application/xml") |
| xml.NewEncoder(w).Encode(res) |
| @@ -939,53 +1179,61 @@ func (f *VersionedBucket) serveList(w http.ResponseWriter, bucket string, q url. |
| func (f *VersionedBucket) serveCopy(w http.ResponseWriter, r *http.Request, bucket, key string) { |
| src := strings.TrimPrefix(r.Header.Get("x-amz-copy-source"), "/") |
| srcPath, rawQ, _ := strings.Cut(src, "?") |
| - sq, _ := url.ParseQuery(rawQ) |
| - sb, sk, _ := strings.Cut(srcPath, "/") |
| - srcKey, err := url.PathUnescape(sk) |
| + sq, err := url.ParseQuery(rawQ) |
| if err != nil { |
| s3err(w, http.StatusBadRequest, "InvalidArgument") |
| return |
| } |
| - id, etag, err := f.CopyVersion(r.Context(), sb, srcKey, sq.Get("versionId"), bucket, key, r.Header.Get("If-None-Match") == "*") |
| - switch { |
| - case err == nil: |
| - case strings.Contains(err.Error(), "already exists"): |
| - s3err(w, http.StatusPreconditionFailed, "PreconditionFailed") |
| + unesc, err := url.PathUnescape(srcPath) |
| + if err != nil { |
| + s3err(w, http.StatusBadRequest, "InvalidArgument") |
| return |
| - default: |
| - s3err(w, http.StatusNotFound, "NoSuchVersion") |
| + } |
| + sb, sk, _ := strings.Cut(unesc, "/") |
| + id, etag, err := f.copyLocked(sb, sk, sq.Get("versionId"), bucket, key, r.Header.Get("If-None-Match") == "*") |
| + if err != nil { |
| + writeErr(w, err) |
| return |
| } |
| w.Header().Set("x-amz-version-id", id) |
| w.Header().Set("Content-Type", "application/xml") |
| - io.WriteString(w, `<CopyObjectResult><ETag>"`+etag+`"</ETag><LastModified>`+time.Now().UTC().Format(time.RFC3339)+`</LastModified></CopyObjectResult>`) |
| + io.WriteString(w, `<CopyObjectResult><ETag>"`+etag+`"</ETag><LastModified>2026-10-05T00:00:00Z</LastModified></CopyObjectResult>`) |
| } |
| ``` |
| -**Two small additions to `bucket.go`** support the server: `pageSize()`, |
| -and a `listPageAll` and `slice` pair that splits the ordered listing. |
| -Implement them so that `ListVersions` and the server share **one** |
| -ordering and **one** page-failure rule (`FailListAfterPages`), with |
| -`objectRow = lease.ObjectVersion`. Keep `listPage` (Task 2) as a thin |
| -wrapper over them. |
| +The server rejects any literal `+` or unescaped `?` in `x-amz-copy-source` |
| +before unescaping, so a client that fails to escape fails the test. Add |
| +this check at the top of `serveCopy`. Allow `/` literally, since it |
| +separates the bucket from the key, and accept everything else |
| +percent-encoded: |
| -- [ ] **Step 3: Implement `s3Mirror.ListVersions` in `versioned.go`** |
| +```go |
| + if strings.ContainsAny(srcPath, "+? ") { |
| + s3err(w, http.StatusBadRequest, "InvalidArgument") // AWS's encoder escapes these; a raw one is a client bug |
| + return |
| + } |
| +``` |
| + |
| +- [ ] **Step 3: Implement `versionedS3.ListVersions`, replacing the stub** |
| ```go |
| -func (m *s3Mirror) ListVersions(ctx context.Context, bucket, prefix string) ([]ObjectVersion, error) { |
| - c := m.clientFor(bucket, m.src) |
| +func (s *versionedS3) ListVersions(ctx context.Context, bucket, prefix string) ([]ObjectVersion, error) { |
| + c, err := s.client(bucket) |
| + if err != nil { |
| + return nil, err |
| + } |
| p := awss3.NewListObjectVersionsPaginator(c, &awss3.ListObjectVersionsInput{ |
| Bucket: aws.String(bucket), Prefix: aws.String(prefix), |
| - }) |
| + }) // StopOnDuplicateToken stays off: a key legitimately repeats across version pages |
| var out []ObjectVersion |
| for p.HasMorePages() { |
| page, err := p.NextPage(ctx) |
| if err != nil { |
| - return nil, fmt.Errorf("lease: list versions %s/%s: %w", bucket, prefix, err) // never partial |
| + return nil, fmt.Errorf("lease: list versions %s/%s: %w", bucket, prefix, err) |
| } |
| for _, v := range page.Versions { |
| out = append(out, ObjectVersion{Key: aws.ToString(v.Key), VersionID: aws.ToString(v.VersionId), |
| - ETag: normalizeETag(aws.ToString(v.ETag)), Size: aws.ToInt64(v.Size), |
| + ETag: strings.Trim(aws.ToString(v.ETag), `"`), Size: aws.ToInt64(v.Size), |
| IsLatest: aws.ToBool(v.IsLatest), LastModified: aws.ToTime(v.LastModified)}) |
| } |
| for _, d := range page.DeleteMarkers { |
| @@ -993,229 +1241,233 @@ func (m *s3Mirror) ListVersions(ctx context.Context, bucket, prefix string) ([]O |
| IsLatest: aws.ToBool(d.IsLatest), DeleteMarker: true, LastModified: aws.ToTime(d.LastModified)}) |
| } |
| } |
| - sortVersions(out) // key order; newest first within a key (S3 returns versions and markers in separate lists) |
| - return out, nil |
| -} |
| - |
| -// sortVersions orders a listing by key, then newest first: LastModified |
| -// descending, then the IsLatest entry first. |
| -func sortVersions(vs []ObjectVersion) { |
| - sort.SliceStable(vs, func(i, j int) bool { |
| - if vs[i].Key != vs[j].Key { |
| - return vs[i].Key < vs[j].Key |
| + sort.SliceStable(out, func(i, j int) bool { |
| + if out[i].Key != out[j].Key { |
| + return out[i].Key < out[j].Key |
| } |
| - if vs[i].IsLatest != vs[j].IsLatest { |
| - return vs[i].IsLatest |
| + if out[i].IsLatest != out[j].IsLatest { |
| + return out[i].IsLatest |
| } |
| - return vs[i].LastModified.After(vs[j].LastModified) |
| + return out[i].LastModified.After(out[j].LastModified) // ties: unspecified (Global Constraints) |
| }) |
| + return out, nil |
| } |
| ``` |
| -Add `"fmt"`, `"sort"`, `"github.com/aws/aws-sdk-go-v2/aws"` and |
| -`awss3 "github.com/aws/aws-sdk-go-v2/service/s3"` to `versioned.go`'s |
| -imports. `clientFor` and `normalizeETag` already exist in `mirror.go`. If |
| -`clientFor`'s signature differs from `clientFor(bucket string, fallback |
| -*awss3.Client) *awss3.Client`, use the existing one and note the |
| -difference in the report. |
| - |
| -The other four methods are still missing, so the contract test doesn't |
| -compile yet. Add temporary stubs that return |
| -`errors.New("not implemented")`, so that this task's two tests can run |
| -(`TestS3MirrorListVersionsPagesAcrossMarkers`, and the contract's two |
| -listing subtests via `-run`). |
| +Add `"sort"`, `"strings"` and `"github.com/aws/aws-sdk-go-v2/aws"` to |
| +the imports. |
| -- [ ] **Step 4: Run** |
| +- [ ] **Step 4: Run (the listing subtests only; the other methods are still stubs)** |
| -Run: `go test ./internal/lease/ -run 'TestS3MirrorListVersions|TestS3MirrorVersionedContract/versions|TestS3MirrorVersionedContract/listing' -count=1` |
| +Run: `go test ./internal/lease/ -run 'TestVersionedS3Contract/listing|TestVersionedS3RefusesUnboundBucket' -count=1` |
| Expected: PASS. |
| - [ ] **Step 5: Commit** |
| ```bash |
| -git add internal/lease/versioned.go internal/lease/versioned_test.go internal/lease/leasetest/ |
| -git commit -m "lease: ListVersions over S3 (paginated, never partial), tested over the wire" |
| +git add internal/lease/ internal/lease/leasetest/ |
| +git commit -m "lease: ListVersions over region-bound S3 clients, tested over the wire" |
| ``` |
| --- |
| -### Task 4: `GetVersion` and `CopyVersion` over S3 |
| +### Task 4: Reading, streaming and copying by exact version |
| **Files:** |
| - Modify: `internal/lease/versioned.go`, `internal/lease/versioned_test.go` |
| -**Interfaces:** |
| -- Produces: `s3Mirror.GetVersion` and `s3Mirror.CopyVersion`, replacing |
| - the stubs. |
| - |
| -- [ ] **Step 1: Write a failing wire test for escaping** |
| +- [ ] **Step 1: Write a failing wire test for escaping (the server rejects raw `+`, `?` and spaces)** |
| ```go |
| -func TestS3MirrorCopyEscapesKeyAndPinsVersion(t *testing.T) { |
| - m, fb := newWireMirror(t) |
| +func TestVersionedS3CopyEscapesLikeAWS(t *testing.T) { |
| + st, m, _ := wireStore(t) |
| key := "src/a b+c%?d" |
| - v1 := fb.Put("data", key, []byte("old")) |
| - fb.Put("data", key, []byte("new")) |
| - if _, _, err := m.CopyVersion(context.Background(), "data", key, v1, "other", "dst/k", true); err != nil { |
| + v1 := m.Put("data", key, []byte("old")) |
| + m.Put("data", key, []byte("new")) |
| + if _, _, err := st.CopyVersion(context.Background(), "data", key, v1, "other", "dst/k", true); err != nil { |
| t.Fatal(err) |
| } |
| - if cur, ok := fb.Current("other", "dst/k"); !ok || string(cur) != "old" { |
| + if cur, ok := m.Current("other", "dst/k"); !ok || string(cur) != "old" { |
| t.Fatalf("copied %q, want the pinned old version", cur) |
| } |
| } |
| ``` |
| -Run: `go test ./internal/lease/ -run TestS3MirrorCopyEscapes -count=1` |
| -Expected: FAIL (`not implemented`). |
| +Run: `go test ./internal/lease/ -run TestVersionedS3CopyEscapes -count=1` |
| +Expected: FAIL (the stub). |
| -- [ ] **Step 2: Implement** |
| +- [ ] **Step 2: Implement `OpenVersion`, `GetVersion` and `CopyVersion`** |
| ```go |
| -func (m *s3Mirror) GetVersion(ctx context.Context, bucket, key, versionID string) ([]byte, error) { |
| - out, err := m.clientFor(bucket, m.src).GetObject(ctx, &awss3.GetObjectInput{ |
| - Bucket: aws.String(bucket), Key: aws.String(key), VersionId: aws.String(versionID)}) |
| +// apiCode returns the smithy error code and HTTP status of err. |
| +func apiCode(err error) (string, int) { |
| + var ae smithy.APIError |
| + code := "" |
| + if errors.As(err, &ae) { |
| + code = ae.ErrorCode() |
| + } |
| + var re *smithyhttp.ResponseError |
| + status := 0 |
| + if errors.As(err, &re) { |
| + status = re.HTTPStatusCode() |
| + } |
| + return code, status |
| +} |
| + |
| +func mapReadErr(err error) error { |
| + code, status := apiCode(err) |
| + switch { |
| + case code == "NoSuchKey" || code == "NoSuchVersion" || code == "NotFound" || status == http.StatusNotFound: |
| + return ErrMirrorNotFound |
| + case status == http.StatusMethodNotAllowed: |
| + return ErrIsDeleteMarker // S3 answers 405 for a delete marker's version id |
| + } |
| + return err |
| +} |
| + |
| +func (s *versionedS3) OpenVersion(ctx context.Context, bucket, key, versionID string) (io.ReadCloser, error) { |
| + if versionID == "" { |
| + return nil, ErrNoVersionID |
| + } |
| + c, err := s.client(bucket) |
| if err != nil { |
| - if isNotFound(err) { |
| - return nil, ErrMirrorNotFound |
| + return nil, err |
| + } |
| + out, err := c.GetObject(ctx, &awss3.GetObjectInput{Bucket: aws.String(bucket), Key: aws.String(key), VersionId: aws.String(versionID)}) |
| + if err != nil { |
| + if m := mapReadErr(err); m != err { |
| + return nil, m |
| } |
| return nil, fmt.Errorf("lease: get %s/%s@%s: %w", bucket, key, versionID, err) |
| } |
| - defer out.Body.Close() |
| - return io.ReadAll(out.Body) |
| + return out.Body, nil |
| } |
| -func (m *s3Mirror) CopyVersion(ctx context.Context, srcBucket, srcKey, srcVersionID, dstBucket, dstKey string, createOnly bool) (string, string, error) { |
| +func (s *versionedS3) GetVersion(ctx context.Context, bucket, key, versionID string) ([]byte, error) { |
| + rc, err := s.OpenVersion(ctx, bucket, key, versionID) |
| + if err != nil { |
| + return nil, err |
| + } |
| + defer rc.Close() |
| + return io.ReadAll(rc) |
| +} |
| + |
| +func (s *versionedS3) CopyVersion(ctx context.Context, srcBucket, srcKey, srcVersionID, dstBucket, dstKey string, createOnly bool) (string, string, error) { |
| + if srcVersionID == "" { |
| + return "", "", ErrNoVersionID |
| + } |
| + if _, err := s.client(srcBucket); err != nil { |
| + return "", "", err |
| + } |
| + c, err := s.client(dstBucket) // a copy executes in the destination's region |
| + if err != nil { |
| + return "", "", err |
| + } |
| in := &awss3.CopyObjectInput{ |
| Bucket: aws.String(dstBucket), |
| Key: aws.String(dstKey), |
| - CopySource: aws.String(srcBucket + "/" + url.PathEscape(srcKey) + "?versionId=" + url.QueryEscape(srcVersionID)), |
| + CopySource: aws.String(httpbinding.EscapePath(srcBucket+"/"+srcKey, false) + "?versionId=" + url.QueryEscape(srcVersionID)), |
| } |
| if createOnly { |
| in.IfNoneMatch = aws.String("*") |
| } |
| - out, err := m.dst.CopyObject(ctx, in) |
| + out, err := c.CopyObject(ctx, in) |
| if err != nil { |
| - if isPreconditionFailed(err) { |
| + code, status := apiCode(err) |
| + switch { |
| + case createOnly && (code == "PreconditionFailed" || status == http.StatusPreconditionFailed): |
| return "", "", ErrDestinationExists |
| - } |
| - if isNotFound(err) { |
| - return "", "", ErrMirrorNotFound |
| + default: |
| + if m := mapReadErr(err); m != err { |
| + return "", "", m |
| + } |
| } |
| return "", "", fmt.Errorf("lease: copy %s/%s@%s -> %s/%s: %w", srcBucket, srcKey, srcVersionID, dstBucket, dstKey, err) |
| } |
| - var etag string |
| + etag := "" |
| if out.CopyObjectResult != nil { |
| - etag = normalizeETag(aws.ToString(out.CopyObjectResult.ETag)) |
| + etag = strings.Trim(aws.ToString(out.CopyObjectResult.ETag), `"`) |
| } |
| return aws.ToString(out.VersionId), etag, nil |
| } |
| ``` |
| -**Escaping a key that contains `/`.** `url.PathEscape` escapes `/` as |
| -`%2F`, but S3 copy sources keep `/` literal inside the key. Escape each |
| -path segment with `url.PathEscape`, then join them with `/`. |
| - |
| -**Helpers:** |
| -- `isNotFound` should recognise smithy codes `NoSuchKey`, |
| - `NoSuchVersion` and `NotFound`, and HTTP 404. |
| -- `isPreconditionFailed` should recognise code `PreconditionFailed` and |
| - HTTP 412. |
| - |
| -If equivalents exist in `mirror.go` or `internal/release` (for example |
| -`s3NotFound` or `s3CASConflict`), reuse their logic rather than copying |
| -it. Shared helpers must not change behaviour for existing callers. |
| +Add these imports: `"net/http"`, `"net/url"`, |
| +`"github.com/aws/smithy-go"`, |
| +`smithyhttp "github.com/aws/smithy-go/transport/http"` and |
| +`"github.com/aws/smithy-go/encoding/httpbinding"`. |
| -**`IfNoneMatch` on `CopyObjectInput`:** if this SDK version lacks the |
| -field, set the `If-None-Match` header with a smithy middleware option on |
| -that one call, and say so in the report. |
| +A 409 from S3 maps to none of the sentinel errors. It is returned |
| +wrapped, as the Global Constraints require. |
| - [ ] **Step 3: Run** |
| -Run: `go test ./internal/lease/ -run 'TestS3MirrorCopyEscapes|TestS3MirrorVersionedContract/get|TestS3MirrorVersionedContract/create-only|TestS3MirrorVersionedContract/multipart' -count=1` |
| +Run: `go test ./internal/lease/ -run 'TestVersionedS3CopyEscapes|TestVersionedS3Contract/read|TestVersionedS3Contract/copy' -count=1` |
| Expected: PASS. |
| - [ ] **Step 4: Commit** |
| ```bash |
| git add internal/lease/versioned.go internal/lease/versioned_test.go |
| -git commit -m "lease: GetVersion and CopyVersion by exact source version, create-only copies" |
| +git commit -m "lease: read, stream and copy by exact version (AWS copy-source escaping, marker 405)" |
| ``` |
| --- |
| -### Task 5: `DeleteVersion` and `PutIfAbsent` over S3 |
| +### Task 5: Deleting by version, and the create-only put |
| **Files:** |
| -- Modify: `internal/lease/versioned.go`, `internal/lease/versioned_test.go` |
| - |
| -**Interfaces:** |
| -- Produces: `s3Mirror.DeleteVersion` and `s3Mirror.PutIfAbsent`. |
| +- Modify: `internal/lease/versioned.go` |
| -- [ ] **Step 1: Write a failing wire test (a bare delete must never be sent)** |
| +- [ ] **Step 1: Implement** |
| ```go |
| -func TestS3MirrorDeleteVersionNeverBare(t *testing.T) { |
| - m, fb := newWireMirror(t) |
| - fb.Put("data", "k", []byte("one")) |
| - if err := m.DeleteVersion(context.Background(), "data", "k", ""); err == nil { |
| - t.Fatal("DeleteVersion with an empty version id must refuse rather than send a bare delete") |
| - } |
| - if fb.LatestID("data", "k") == "" { |
| - t.Fatal("a delete marker or deletion happened") |
| - } |
| -} |
| -``` |
| - |
| -Run: `go test ./internal/lease/ -run TestS3MirrorDeleteVersionNeverBare -count=1` |
| -Expected: FAIL. |
| - |
| -- [ ] **Step 2: Implement** |
| - |
| -```go |
| -var errNoVersion = errors.New("lease: a version id is required") |
| - |
| -func (m *s3Mirror) DeleteVersion(ctx context.Context, bucket, key, versionID string) error { |
| +func (s *versionedS3) DeleteVersion(ctx context.Context, bucket, key, versionID string) error { |
| if versionID == "" { |
| - return errNoVersion // never a bare-key delete (spec C3) |
| + return ErrNoVersionID // never a bare-key delete (spec C3), and no request is sent |
| + } |
| + c, err := s.client(bucket) |
| + if err != nil { |
| + return err |
| } |
| - _, err := m.clientFor(bucket, m.dst).DeleteObject(ctx, &awss3.DeleteObjectInput{ |
| - Bucket: aws.String(bucket), Key: aws.String(key), VersionId: aws.String(versionID)}) |
| - if err != nil && !isNotFound(err) { |
| + if _, err := c.DeleteObject(ctx, &awss3.DeleteObjectInput{Bucket: aws.String(bucket), Key: aws.String(key), VersionId: aws.String(versionID)}); err != nil { |
| + if mapReadErr(err) == ErrMirrorNotFound { |
| + return nil // already gone |
| + } |
| return fmt.Errorf("lease: delete %s/%s@%s: %w", bucket, key, versionID, err) |
| } |
| return nil |
| } |
| -func (m *s3Mirror) PutIfAbsent(ctx context.Context, bucket, key string, body []byte) (string, error) { |
| - out, err := m.clientFor(bucket, m.src).PutObject(ctx, &awss3.PutObjectInput{ |
| - Bucket: aws.String(bucket), Key: aws.String(key), Body: bytes.NewReader(body), |
| - IfNoneMatch: aws.String("*")}) |
| +func (s *versionedS3) PutIfAbsent(ctx context.Context, bucket, key string, body []byte) (string, error) { |
| + c, err := s.client(bucket) |
| + if err != nil { |
| + return "", err |
| + } |
| + out, err := c.PutObject(ctx, &awss3.PutObjectInput{Bucket: aws.String(bucket), Key: aws.String(key), |
| + Body: bytes.NewReader(body), IfNoneMatch: aws.String("*")}) |
| if err != nil { |
| - if isPreconditionFailed(err) { |
| + if code, status := apiCode(err); code == "PreconditionFailed" || status == http.StatusPreconditionFailed { |
| return "", ErrExists |
| } |
| return "", fmt.Errorf("lease: put-if-absent %s/%s: %w", bucket, key, err) |
| } |
| return aws.ToString(out.VersionId), nil |
| } |
| - |
| -var _ VersionedStore = (*s3Mirror)(nil) |
| ``` |
| -Also make the fake's `DeleteVersion` refuse an empty version id the same |
| -way, returning an error, and add that case to the contract suite (Task |
| -2's file), so both implementations agree. |
| +Add the `"bytes"` import. Remove `errNotYet` and every stub. |
| -- [ ] **Step 3: Run the full contract over the wire, and the fake** |
| +- [ ] **Step 2: Run the whole contract, over the wire and against the model** |
| Run: `go test ./internal/lease/... -count=1 -race` |
| -Expected: PASS (the fake contract, the wire contract and the wire tests). |
| +Expected: PASS (both contracts and every wire test). |
| -- [ ] **Step 4: Commit** |
| +- [ ] **Step 3: Commit** |
| ```bash |
| -git add internal/lease/ internal/lease/leasetest/ |
| -git commit -m "lease: DeleteVersion (never bare) and PutIfAbsent over S3" |
| +git add internal/lease/versioned.go |
| +git commit -m "lease: delete by version (never bare, no request on empty id) and create-only put" |
| ``` |
| --- |
| @@ -1223,95 +1475,93 @@ git commit -m "lease: DeleteVersion (never bare) and PutIfAbsent over S3" |
| ### Task 6: IAM grants |
| **Files:** |
| -- Modify: `infra/modules/region-host/main.tf` (the `host_buckets` policy, |
| - around lines 153-232) and `infra/modules/region-remote-host/main.tf` |
| - (around lines 85-134). |
| +- Modify: `infra/modules/region-host/main.tf` and `infra/modules/region-remote-host/main.tf` |
| -- [ ] **Step 1: Add the statements** |
| +- [ ] **Step 1: Add the two statements to each module's box policy** |
| -In both policies' `Statement` lists, beside the existing data-bucket |
| -object statement, add these two statements, using each module's own |
| -data bucket ARN variable: |
| +In `region-host` the data bucket ARN is `var.data_bucket_arn`. In |
| +`region-remote-host` it is `aws_s3_bucket.data.arn`; confirm this |
| +against the module and use what it actually names. |
| ```hcl |
| + # Host lifecycle claim, Plan 2 (spec C3, §6.2, §6.5): exact-version |
| + # list, read and delete on this box's OWN data bucket. Sibling-region |
| + # moves/* grants are unchanged: the destination region retires its |
| + # own staging (spec N13). |
| { |
| Effect = "Allow" |
| Action = ["s3:GetObjectVersion", "s3:DeleteObjectVersion"] |
| - Resource = ["${var.data_bucket_arn}/*"] |
| + Resource = ["<DATA_BUCKET_ARN>/*"] |
| }, |
| { |
| Effect = "Allow" |
| Action = ["s3:ListBucketVersions"] |
| - Resource = [var.data_bucket_arn] |
| + Resource = ["<DATA_BUCKET_ARN>"] |
| }, |
| ``` |
| -Leave the deployment bucket, the sibling `moves/*` staging statements |
| -and the store policies unchanged. Above the statements, add a comment |
| -naming spec C3 and Plan 2. |
| +Replace `<DATA_BUCKET_ARN>` with the module's own expression: |
| +`"${var.data_bucket_arn}/*"` and `var.data_bucket_arn` in `region-host`, |
| +and `"${aws_s3_bucket.data.arn}/*"` and `aws_s3_bucket.data.arn` in |
| +`region-remote-host`. |
| - [ ] **Step 2: Format and validate** |
| -Run: `tofu fmt -check infra/modules/region-host infra/modules/region-remote-host`. Then run |
| -`tofu -chdir=infra/modules/region-host validate` and the same for |
| -`region-remote-host`, if `tofu` is available. If it isn't, run |
| -`terraform fmt -check` instead, and record which one you used. |
| -Expected: no formatting diff, and validation passes. Without either |
| -tool, inspect the diff for HCL syntax and say so in the report. |
| +Run `tofu fmt -check` on both module directories. Then run |
| +`tofu -chdir=<module> init -backend=false && tofu -chdir=<module> validate` |
| +on each, if `tofu` is available. Otherwise use the same commands with |
| +`terraform`, and record which you used. If neither tool is available, |
| +review the HCL diff by eye and say so in the report. |
| - [ ] **Step 3: Commit** |
| ```bash |
| git add infra/modules/region-host/main.tf infra/modules/region-remote-host/main.tf |
| -git commit -m "infra: box role may list, read and delete data-bucket object versions (claim Plan 2)" |
| +git commit -m "infra: box role lists, reads and deletes its own data bucket's object versions (claim Plan 2)" |
| ``` |
| --- |
| ### Task 7: The repository gate |
| -- [ ] **Step 1: Run the full gate (AGENTS.md)** |
| - |
| -```bash |
| -go build ./... && go vet ./... && gofmt -l . && go test ./... -count=1 |
| -``` |
| -Expected: build and vet are clean, `gofmt -l .` prints nothing, and every |
| -package passes. Confirm sandbox-only failures through `amadan ci status`. |
| - |
| -- [ ] **Step 2: Commit any formatting fixes** |
| - |
| -```bash |
| -git add -A internal/lease infra/modules |
| -git commit -m "lease: Plan 2 gate" || true |
| -``` |
| +- [ ] **Step 1:** `go build ./... && go vet ./... && gofmt -l . && go test ./... -count=1`. |
| + Expected: everything is clean. Confirm sandbox-only failures through |
| + `amadan ci status`. |
| +- [ ] **Step 2:** `git add -A internal/lease infra/modules && git commit -m "lease: Plan 2 gate" || true` |
| --- |
| ## Rollout note |
| -The IAM change has to reach each deployment before Plans 4 and 6 run |
| -anything that calls these methods. Dev tracks `main`. Flagship applies |
| -through `carloku-infrastructure` at a release tag, and that apply must |
| -**target** the policy resources only: a whole-root apply touches |
| -`user_data` and restarts every box (memory: carloku infra split). Plan 2 |
| -itself calls nothing in production, so merging it is inert. |
| +The IAM grants must reach each deployment before Plans 4 and 6 run |
| +anything that calls these methods. |
| + |
| +- **Dev** tracks `main`. |
| +- **Flagship** applies through `carloku-infrastructure` at a release |
| + tag, and must **target** only the policy resources. A whole-root apply |
| + touches `user_data` and restarts every box. |
| + |
| +Plan 2 calls nothing in production, so merging it changes nothing that |
| +runs. |
| ## Self-review |
| -- **Spec coverage:** |
| - - C3, exact-version destructive calls: Tasks 1, 4 and 5; |
| - - §6.2, list every version and archive by version: Tasks 2 to 4; |
| - - §6.4a certificates, create-only: Task 5 `PutIfAbsent`; |
| - - §6.5, pinned reads: Task 4 `GetVersion`; |
| - - §13 rows 1 to 9 in the fake: Task 2; |
| - - IAM for §6.2 and §6.5: Task 6. |
| -- **Placeholders:** none. Two conditional instructions are explicit: the |
| - SDK `IfNoneMatch` fallback, and reusing the existing not-found and |
| - precondition helpers. |
| -- **Types:** |
| - - `ObjectVersion` and `VersionedStore` are used consistently. |
| - - `VersionedBucket`'s exported helpers (`Put`, `PutMultipart`, |
| - `BareDelete`, `LatestID`, `Current`, `AddBucket`, `PageSize`, |
| - `FailListAfterPages`, `FailOn`) are used by the contract and later |
| - plans. |
| - - `newWireMirror` is test-only. |
| +Each finding from the rev 1 review, and where it's fixed: |
| + |
| +| Finding | Fix | |
| +|---|---| |
| +| P1, import cycle | external `lease_test` package plus `export_test.go` | |
| +| P1, routing | explicit bucket-to-client binding, unbound buckets refused, two servers in the wire tests | |
| +| P2, escaping | `httpbinding.EscapePath`, plus a server that rejects raw `+`, `?` and spaces | |
| +| P2, failure injection | locked helpers, `writeErr` maps errors to statuses, and a contract subtest over every operation | |
| +| P2, ordering | the contract states that ties are unspecified, and tests rely only on `IsLatest` | |
| +| P2, delete markers | `ErrIsDeleteMarker` (405) in both implementations, with marker reads and copies tested | |
| +| P2, the false-positive safety test | request count plus a full state snapshot | |
| +| P2, sibling cleanup | assigned to the destination region; no sibling grants change | |
| +| P2, missing helpers | complete helpers, with locking stated | |
| +| P3, §13 coverage | the coverage table | |
| +| P3, consumer contract | `NewVersionedS3`, `OpenVersion`, and destination recovery via `ListVersions` | |
| + |
| +The types are consistent throughout: `VersionedStore`, `ObjectVersion`, |
| +the `Maker` signature, `NewS3Server` returning `(Handler, counter)`, and |
| +`NewVersionedS3WithClients`. |