Every fetch of an artifact wrote one path, so fetches with different URLs
or digests, which do not share a coalescing key, could overwrite each
other's bytes, serve the wrong ones, or delete them on a digest mismatch
while the other was about to open them.
storeArtifact now writes {ecosystem}/{name}/{version}/{fetch id}/{filename},
with a random id per fetch. Most ecosystems learn the digest only after
the fetch, and storage has no rename, so the id is the one rule that fits
all of them. Existing records keep their paths and stay readable. On a
file:// bucket, Delete now removes an emptied fetch directory, which
fileblob leaves behind, and no other directory, since another fetch may
be creating its own inside it.
A path a record stops pointing at is queued in pending_deletes rather
than deleted, since a request that read the record may still open it. A
loop deletes queued paths after max(1h, direct_serve_ttl), whether or not
max_size is set. UpsertArtifact and clears apply only if the record still
holds the path the caller read, so racing commits each queue the path
they replaced and a clear never orphans a newer commit. Eviction still
deletes inline, and counts space as freed only when the record still
pointed at what it deleted.
The denylist tests call NewDebianHandler with three arguments, but the
additional APT archives change gave it a fourth. Each passed CI on its
own branch, and together they leave internal/handler without a test
binary that compiles, so go vet and go test fail on main.
A failed delete leaves the record eligible, so the same batch came back
on the next iteration while the recorded size never dropped. Against a
backend refusing deletes the pass retried forever, burning CPU and
flooding the log. A canceled context made it faster: every delete fails
at once, so the loop spun as quickly as the database could answer.
The pass now ends when a batch clears no record, and returns as soon as
the context is canceled. Progress counts records rather than bytes,
because a cached record can have no size recorded. A pass can therefore
end with the cache still above its limit, leaving the records that
failed for the next pass to retry.
A request to /debian/{name}/ for a name that is not configured fell through
to the main archive, so a typo surfaced as the upstream's own HTML 404 with
no mention of the repository. The reply now names the unknown repository and
lists the configured ones.
Scoped to {name}/dists/ and {name}/pool/. The main archive serves README,
indices/ and project/ at its root, so an unknown first segment stays a
main-archive path everywhere else.
apt prints only the status line, so this is visible via curl or the proxy
log rather than in apt output.
The motivation for separate archives was repeated across six files and the
reserved-name rationale across five. Keep each in one place -- the reference
docs -- and leave the code comments to what an informed reader cannot get
from the code: why an unknown first segment is not a 404 as it is for APK,
and why the main archive keeps its legacy cache identities.
Comments and prose only; no behaviour change.
go install does not apply goreleaser's -ldflags -X, so binaries
installed that way reported 'dev'. Read debug.BuildInfo.Main.Version
when the ldflag is unset.
The 250ms client is meant for the readiness poll, where a timeout is
retried. Reusing it for the OCI manifest request makes the test flake
under -race on Windows CI when fetch and cache I/O take longer, as
seen on #328. The request checks upstream routing, not latency.
The digest-aware cache check discarded a stale entry before its caller
took the key. A slow caller could delete an entry another caller's
fetch had just committed, and everyone sharing that fetch then failed
to open it.
The check now only reports the miss. The caller running the shared
fetch discards the entry under the key, after the recheck, so one a
previous fetch refreshed is served, not deleted. The mismatch warning
fires once per refresh instead of once per request.
Swift HEAD no longer discards either, having no fetch to do it under.
It probes upstream as before, and the next GET replaces the entry.
Different digests or URLs, or no digest, use different keys and can
still collide on the storage path. That is the storage layout follow-up.
createTestPostgresDB dropped artifacts, versions, packages and
schema_info before calling CreateSchema, but not the migrations table.
On a database that has seen one test the migration records survive, so
the next CreateSchema fails while recording 001_add_packages_enrichment_
columns with a duplicate key on migrations_pkey. Running the package
against one Postgres therefore failed from the second Postgres-backed
test on.
Drop vulnerabilities, metadata_cache and migrations as well, so the
fixture clears every table CreateSchema creates. The package now passes
repeatedly against the same database.
* fix(database): set connection pool limits for Postgres
OpenPostgres returned sqlx.Open's handle with database/sql's defaults:
no cap on open connections and two idle ones. Under load nearly every
request opened a new Postgres session, ran its few statements and closed
it again, paying a backend fork and SCRAM authentication each time.
Set the pool limits the issue suggests: 32 open and 32 idle connections,
idle connections closed after 5 minutes and every connection recycled
after 30 minutes. The values live in named constants because the mnd
linter rejects the literals inline.
The test (skipped without PROXY_DATABASE_URL, like the other Postgres
tests) takes 16 connections from the pool, releases them and checks that
all 16 stay idle; with the default pool only two survive.
Fixes#323
* test(database): pin the pool properties instead of the wiring
Review pointed out that the pool test compared MaxOpenConnections to the
constant it was set from, so the assertion followed the constant and
would have accepted postgresMaxOpenConns = 0 (unlimited), and that its
burst of 16 only proved MaxIdleConns >= 16.
The burst now takes postgresMaxIdleConns connections, so every
configured idle slot has to survive the release, and the open cap is
checked to be finite and large enough for that burst before any
connection is taken, so a cap below the idle count fails fast instead
of blocking in db.Conn. The doc comment now says which settings the
test covers; the idle-time and lifetime settings only show up in
DBStats.MaxIdleTimeClosed and MaxLifetimeClosed after minutes of
wall-clock time and stay unexercised.
* test(database): guard the pool test against a vacuous burst
Review showed that with the burst tied to postgresMaxIdleConns the test
also passed for a constant of 2, database/sql's default, or of 0, where
it took no connections at all. It now fails outright unless the
configured idle count exceeds the default, and every connection it
takes is released in a cleanup, so an assertion failure mid-burst no
longer leaves sessions open for the rest of the test binary.
* fix(server): tune the shared upstream transport defaults
server.serve builds the shared client with safehttp.New, which clones
Go's default transport: MaxIdleConnsPerHost stays 0 (an effective limit
of two idle connections per host) and ResponseHeaderTimeout stays 0.
Handing that client to fetch.NewFetcher via fetch.WithHTTPClient
replaces the fetcher's own defaults of 10 idle connections per host and
a 60-second response-header timeout.
Set both on the shared transport, matching the fetcher defaults: a
second burst of concurrent cache misses to one registry now reuses its
connections instead of re-dialling most of them, and an upstream that
accepts a request but stalls before sending headers is cut off after 60
seconds rather than only by the client's overall timeout.
Tests measure connection reuse across two concurrent bursts against a
TLS upstream that counts accepted connections (Go default: at most two
reused; tuned: all eight) and assert that a stall before headers fails
with the response-header timeout.
Fixes#327
* fix(server): reconcile http_timeout docs and tighten the transport tests
The http_timeout documentation and the config comment said "0" disables
the upstream timeout entirely. With a fixed 60-second
ResponseHeaderTimeout on the shared transport that is no longer the
whole story, so both now say that waiting for response headers stays
bounded independently of the setting.
Test cleanup from review: the 50ms settle between bursts was dead time
(the transport returns a connection to the idle pool before the body's
final Read returns, so burst returning already means the pool is
settled); the maxNewInBurst sentinel became explicit min/max bounds per
case; the stall test dropped the client.Timeout override and the
elapsed-time assertion, which was redundant with the error-text check
in any realistic run and whose failure message misattributed the cause,
and its comment now says plainly that the field assertions pin
production while the behavioural half runs at a lowered timeout.
* test(server): hold each burst at the upstream instead of sleeping
The reuse test kept a burst in flight with a 100ms handler sleep, so a
process stall longer than that between spawning the goroutines and
their dials let a request finish early and hand its connection to a
sibling. Review reproduced this with forced stalls: the default-transport
case then dialled 5 instead of 6 connections.
The handler now answers only once burstSize requests are waiting at the
same time. With HTTP/1.1 pinned that forces every burst onto burstSize
distinct connections regardless of scheduling, and the measured counts
stay exactly 6 new for Go's default and 0 for the tuned transport, also
under the same forced stalls.
A Debian release is served by more than one archive. Security updates live on
a different host than the main archive, so a single upstream.debian URL cannot
serve a complete suite set and -security suites are unreachable.
Add upstream.debian_repositories, a name-to-URL map served at /debian/{name}/,
modelled on upstream.apk. The field is additive: upstream.debian keeps serving
/debian/pool/... and /debian/dists/... with unchanged cache identities, so
existing deployments and their warm caches are unaffected, and the scalar
field and PROXY_UPSTREAM_DEBIAN are untouched.
Named repositories scope both caches by name, since the same filename can hold
different bytes in different archives. Metadata keys are hashed over name,
upstream URL, and path, as APKHandler does. Names are validated through
validateNamedUpstreams, and "pool" and "dists" are refused because they would
shadow the main archive's own paths.
An unconfigured first path segment stays a main-archive path rather than
returning 404 as the APK handler does: the main archive is unnamed and serves
paths of its own at the root.
* Stop writing fileblob's .attrs sidecar
fileblob stores blob metadata in an ".attrs" file per object and rewrites
it with os.Create, truncating in place outside the atomic rename that
protects the blob. A read overlapping a write decodes a partial file and
fails with "opening reader: EOF", served as a 502. One writer against
four readers on a single key failed 408 of 2000 reads. cacheMetadataBlob
is most exposed to it, rewriting a key on every refresh while readers
are served from it.
Nothing in the proxy reads what the sidecar holds. gocloud.dev/blob is
imported only by internal/storage, Store sets no ContentType, and
Attributes is used only for Size, which comes from os.Stat. A missing
sidecar already defaults cleanly, so "metadata=skip" removes the hazard
rather than locking around it, and saves a write per store.
* Clear .attrs sidecars left by earlier versions
metadata=skip stops fileblob rewriting sidecars but does not delete ones
already on disk, so a sidecar left partial by an interrupted write now
fails every read of its key for good. Before, a later store repaired it
by rewriting.
Store therefore removes the sidecar for the key it writes. Removal is
atomic where the rewrite was not, so a concurrent reader gets the whole
old file or nothing. Delete already removes sidecars, so the two paths
drain a cache between them.
Deriving that path is necessary because fileblob's key escaping is
unexported. It is the identity for a plain key and parts from one only
for keys that are not valid local paths, which is what filepath.Localize
rejects. That also keeps the removal inside the cache directory: without
it a key holding ".." resolves outside.
The clearing test runs one key per storage path the proxy builds, seeded
through a bucket that still writes sidecars so the path under test is
fileblob's own.
* Explain why failed sidecar cleanup does not fail the store
Move the removal into clearLegacySidecar and say why its error is
dropped rather than returned. A failed removal leaves exactly the state
this change inherited, while failing the write would turn a cleanup miss
into a failed request.
Windows makes that concrete: Go opens files with FILE_SHARE_READ and
FILE_SHARE_WRITE but not FILE_SHARE_DELETE, so a reader holding the
sidecar open blocks deletion, and that reader is the workload this
change exists to protect. Propagating would fail stores during exactly
the overlap being fixed. The next store of the key retries.
A test pins it, using a non-empty directory at the sidecar path to make
os.Remove fail with something other than not-exist on any platform.
* Fail the concurrency test if its writer stops
The writer returned silently when Store failed, so the test could pass
with no concurrent writes at all. Its error is now reported, and the
test also checks that at least one write completed.
Reporting it showed the writer had been dying on Windows at its first
collision: Go opens files without FILE_SHARE_DELETE, so a reader holding
the file open makes the writer's rename fail with access denied. The
test now skips there, since it cannot contend a writer with readers on
that platform.
* Clear legacy sidecars for keys fileblob escapes
legacySidecarPath declined any key filepath.Localize rejects, which on
Windows is every key with a colon: OCI digests and Debian epochs. Their
sidecars were never cleared there, and a truncated one kept failing
reads, since fileblob still reads a sidecar it finds under metadata=skip.
fileblob hex-escapes such characters on the way to disk. The path is now
derived the same way, so the sidecar is looked for where fileblob wrote
it. Localize still validates the escaped form, which keeps the removal
inside the cache directory.
* Drop stale comment about declining colon keys on Windows
623ff3e made legacySidecarPath escape keys the way fileblob does, so
colon-bearing keys are now cleared on Windows and the OCI and Debian
rows in TestStoreClearsLegacyAttrsSidecar prove it. The comment
described the behaviour before that commit.
---------
Co-authored-by: Andrew Nesbitt <andrewnez@gmail.com>
