DongHyeonkaandClaude Opus 5
5f10b791d3
Snapshot of the in-flight state that already existed, identically, in both this worktree and the main checkout before this session began: the initial HTTP Client platform implementation (previously untracked), the redis-lab removal, and the JPA / object-storage / notification integration work. Kept separate from this session's HTTP Client review response, which lands in the following commit, so the two bodies of work stay reviewable apart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
214 lines
12 KiB
Markdown
214 lines
12 KiB
Markdown
# Fileserver — deviations from the design
|
|
|
|
The design specification and the implementation plan are frozen documents. Where implementation
|
|
found them under-specified or self-contradicting, the resolution is recorded here rather than by
|
|
editing the specification, and every entry names the test that pins the decision.
|
|
|
|
## Resolved inconsistencies in the state machine and contracts
|
|
|
|
### 1. `CREATED → FAILED` has no edge in the transition table
|
|
|
|
A create that fails after the record exists must end in `FAILED`, but the table has no direct edge.
|
|
The record therefore walks `CREATED → UPLOADING → FAILED`, which is also the honest reading: the
|
|
upload had been admitted before it failed.
|
|
|
|
Pinned by `UploadApplicationServiceTest` (application-core).
|
|
|
|
### 2. `ContentKey`'s alphabet admits a leading separator
|
|
|
|
The design's key pattern `[a-z0-9/_-]{16,200}` matches `/etc/passwd/...`. Rejecting an absolute path
|
|
at the value type would change a design-fixed contract, so the stricter shape check lives in
|
|
`PhysicalPathResolver`, per §12.2 rule 1 — the only place that turns an identifier into a path.
|
|
|
|
Pinned by `PhysicalPathResolverTest`.
|
|
|
|
### 3. `VERIFYING → DELETING` has no edge
|
|
|
|
Deleting a file that is mid-verification has no legal transition. The lifecycle service refuses it
|
|
with `409 FILE_NOT_READY` rather than inventing an edge, which matches the allowed-state list the
|
|
JPA `markDeleting` statement already enforced.
|
|
|
|
Pinned by `FileLifecycleServiceTest`.
|
|
|
|
### 4. `If-Match` is specified as an ETag but the lifecycle was designed around the row version
|
|
|
|
The HTTP contract sends an entity tag; the metadata store guards on a numeric version. The service
|
|
takes `Optional<String> expectedEtag` and compares against the record's strong validator, so the
|
|
precondition a client sends is the precondition that is checked.
|
|
|
|
Pinned by `FileLifecycleServiceTest`.
|
|
|
|
### 5. The filename policy left `:` intact
|
|
|
|
`C:\Windows\system.ini` sanitized to `C:Windowssystem.ini` — a drive-qualified name surviving into
|
|
display text and headers. `:` joined the structural strip set.
|
|
|
|
Pinned by `FileserverHardeningContractTest` and `AmbiguousFilesystemOperationDetectorTest`.
|
|
|
|
## Additions the design implies but does not specify
|
|
|
|
### 6. `fs_recovery_item`
|
|
|
|
§10.2 lists five core tables and none of them can hold the recovery queue, yet §29.3 requires one:
|
|
reconciliation reports files whose bytes and metadata disagree, and holding that list in memory
|
|
would lose exactly the cases a restart interrupted. Added in
|
|
`V2__fileserver_recovery_and_staging_cleanup.sql` with one open item per file, so repeated sweeps
|
|
update a worklist rather than accumulating a log.
|
|
|
|
Pinned by `PostgreSqlFileserverReclamationIntegrationTest`.
|
|
|
|
### 7. `fs_cleanup_item.upload_id`
|
|
|
|
A staging object is addressed by upload, not by file. Without this column a queued staging cleanup
|
|
could name only already-published content, so a cancelled or expired upload left bytes nothing could
|
|
find. Added in the same migration, with a check constraint that an item names exactly one target.
|
|
|
|
Pinned by `PostgreSqlFileserverReclamationIntegrationTest`.
|
|
|
|
### 8. `ContentReferenceLedger` and `StagingUploadLocator`
|
|
|
|
The orphan scan must ask whether a record still claims a physical object, and reconciliation must
|
|
map a file back to the upload that last staged it. Neither question is answerable through the
|
|
design's `FileMetadataStore` or `UploadSessionStore` as written. Rather than widen those
|
|
design-fixed interfaces, both are narrow single-method ports.
|
|
|
|
Pinned by `PostgreSqlFileserverReclamationIntegrationTest` and `LocalOrphanScanAdapterTest`.
|
|
|
|
## Interpretations
|
|
|
|
### 9. Quota settlement is FIFO within a scope
|
|
|
|
Nothing links a reservation row to the upload that took it, and the design deliberately reclaims
|
|
stragglers by TTL and the `STALE_QUOTA_RESERVATION` cleanup type rather than threading a reservation
|
|
id through the upload session. `QuotaCommitGateway` therefore settles the oldest live reservation in
|
|
the file's namespace.
|
|
|
|
Which row closes does not change any quota decision: enforcement sums reserved and committed bytes
|
|
per scope and never reads an individual row. Concurrent uploads of different sizes can leave the
|
|
reserved total transiently high or low, and it converges as each settles. Durable usage with no live
|
|
reservation behind it — an upload that outlived its TTL — is still recorded, because a ledger that
|
|
silently under-counts is worse than one that is briefly imprecise.
|
|
|
|
Pinned by `PostgreSqlFileserverReclamationIntegrationTest`.
|
|
|
|
### 10. Zero copy is a channel transfer, not a file handoff
|
|
|
|
Task 22 asks for zero copy on local files; §19 forbids a `Path` leaving the storage adapter, and §5
|
|
of the plan forbids adding `Path` to the content store. WebFlux's zero-copy API takes a `Path`, so
|
|
that route is closed.
|
|
|
|
The servlet path takes the other one: `ZeroCopyDownloadGateway` receives a `WritableByteChannel` from
|
|
the transport and the storage adapter performs `FileChannel.transferTo` into it. That is a genuine
|
|
kernel-level transfer with no filesystem concept leaving storage. The reactive path continues to
|
|
stream with bounded demand.
|
|
|
|
Zero copy is an optimization with no observable difference: when storage declines, the response is
|
|
streamed and is byte-identical.
|
|
|
|
Pinned by `LocalStorageGatewayContractTest` and `ZeroCopyEligibilityTest`.
|
|
|
|
### 11. The Fileserver JPA stores are gated on the capability switch
|
|
|
|
The metadata store, session store, quota service, queues, ledger, and staging locator carry
|
|
`@ConditionalOnProperty(app.fileserver-platform.enabled)` even though the rest of
|
|
`adapter:outbound:persistence-jpa` is unconditional.
|
|
|
|
Without the gate, every composition root that includes the persistence module built these beans —
|
|
including `sample-portfolio`, which has no Fileserver — and each of them needs collaborators only
|
|
the Fileserver configuration provides. That is the same rule the design states for the transport
|
|
surface, applied to persistence: no surface appears merely because the dependency is present.
|
|
|
|
`FileStateMachine` is bound alongside them, in `FileserverStorageConfiguration`. It had no
|
|
production binding at all before, which made the metadata store unconstructible in any
|
|
component-scanned context.
|
|
|
|
Pinned by `SampleApplicationContextTest` (the capability off) and
|
|
`FileserverRuntimeAssemblyTest` (the capability on).
|
|
|
|
### 12. Transaction boundaries are owned by the application services, and are deliberately narrow
|
|
|
|
The design does not say where a transaction begins. The repository does:
|
|
`adapter:outbound:persistence-jpa` forbids a repository adapter from owning a `@Transactional`
|
|
boundary, and `application-core` owns them through `TransactionPort`. The Fileserver follows that
|
|
rule — every `Jpa*` store here declares no `@Transactional` of its own.
|
|
|
|
What is specific to this capability is how narrow the boundaries are. A boundary covers a contiguous
|
|
run of metadata writes and **stops before every storage call**, because a filesystem operation
|
|
inside a database transaction would hold a connection for the length of a byte transfer. The upload
|
|
path therefore has three boundaries, not one: acquire the lease, transfer the bytes, commit the
|
|
offset.
|
|
|
|
Where several stores must agree, they share one boundary:
|
|
|
|
| Unit | Why it is one boundary |
|
|
| --- | --- |
|
|
| reserve quota + insert record + create session | a reservation that outlived a failed insert holds capacity for a file that never existed |
|
|
| READY transition + quota commit | a finished file whose reservation was never converted holds capacity until the reservation expires |
|
|
| `markDeleting` + enqueue cleanup | a file that stopped being reachable with nothing queued to reclaim it is never collected |
|
|
| content delete settlement: reclaim + retire record + close queue item | half of it leaves the item to be retried against content that no longer exists |
|
|
|
|
What this cannot make atomic is the storage/metadata seam itself — no database boundary could. That
|
|
seam is exactly what the ambiguous-completion path and the reconciler exist for, and the one
|
|
hand-written compensation that remains (staging creation failing after the records committed) is
|
|
there for the same reason.
|
|
|
|
Pinned by `FileserverRoundTripContractTest` against real PostgreSQL; the application tests use
|
|
`DirectTransactions`, which runs a boundary inline and counts it.
|
|
|
|
## Not implemented
|
|
|
|
### `AsyncContentStore`, `CapacityAwareContentStore`, `CopyCapableContentStore`, `DelegatedDownloadStore`
|
|
|
|
Four optional content-store SPIs are declared in `application-core` with no implementation. Each is
|
|
an extension point for a backend this template does not ship:
|
|
|
|
- `AsyncContentStore` — for a backend whose native client is non-blocking. The local platform is
|
|
blocking, and the reactive transport bridges to it on a dedicated I/O scheduler.
|
|
- `CapacityAwareContentStore` — capacity is reported through `StorageHealthPort` and
|
|
`StorageUsageProbe`, which the local platform implements.
|
|
- `CopyCapableContentStore` — server-side copy is delivered by `CopyContentGateway`; the local
|
|
platform has no cheaper primitive than a streamed copy.
|
|
- `DelegatedDownloadStore` — delegation is delivered at the transport boundary by the nginx
|
|
`X-Accel-Redirect` strategy, which needs no store participation.
|
|
|
|
`ContentStoreCapabilities` reports what the running store actually supports, so no unimplemented SPI
|
|
is advertised as available.
|
|
|
|
## Known deviation from the repository's application-layer contract
|
|
|
|
### 5. Fileserver application services are not `CommandUseCase` / `QueryUseCase`
|
|
|
|
`src/application-core/CLAUDE.md` requires every inbound port implementation to extend
|
|
`CommandUseCase` or `QueryUseCase` and to carry `@UseCaseCapability`, which declares its transaction
|
|
mode, idempotency and repository access. The Fileserver instead exposes multi-method services —
|
|
`UploadApplicationService`, `DownloadApplicationService`, `FileLifecycleService`,
|
|
`FileserverAdminService` and their `Default*` implementations.
|
|
|
|
This is a real deviation, not an oversight, and it is unenforced: the ArchUnit rules
|
|
`inbound_port_implementations_end_with_use_case` and
|
|
`inbound_port_implementations_declare_capability` only match types that implement `UseCase`, so a
|
|
service that never does is silently exempt. The capability contract that every other feature in
|
|
this repository declares is therefore absent here.
|
|
|
|
Two things follow from it. The transaction mode of each operation is expressed only by which
|
|
`TransactionPort` method the body happens to call, rather than declared and checked. And the
|
|
application layer holds transport policy it would not hold if each operation were a use case with
|
|
its own command: HTTP status codes on `FileserverErrorCode`, `Range` and conditional-request
|
|
parsing in `api.transfer`, and `Content-Disposition` construction.
|
|
|
|
The status mapping in particular is a deliberate trade rather than an accident. It lives in
|
|
`application-core` so the servlet transport, the reactive transport and the Nginx delegation path
|
|
cannot answer the same failure with three different statuses. Moving it to the transport layer
|
|
resolves the layering complaint and reintroduces exactly that drift, which is why this is an
|
|
architecture decision rather than a cleanup.
|
|
|
|
**Status: open, deliberately unresolved in this change set.** Closing it means roughly thirty
|
|
command/query use cases, a decision about where the shared status vocabulary lives, and a change to
|
|
the ArchUnit rules so a service that bypasses the contract fails the build instead of being exempt
|
|
from it. That belongs in its own ADR with its own review, and doing it inside a correctness patch
|
|
would mix a large mechanical refactor into changes that need to be readable.
|
|
|
|
Nothing here is pinned by a test, because the deviation is the absence of a constraint. The next
|
|
step is the ADR, not another test.
|
|
|