Bound the parent gate's FOR SHARE wait when a request path first calls a remote create
Why
parentRepositoryIsActive (internal/datastore/repository_parent_gate.go) takes
SELECT ... FOR SHARE on the parent repositories row. The lock closes a
mis-parenting window: against a pending tombstone the gate waits for that
transaction's verdict instead of reading past it, and refuses the parent once
the tombstone commits.
That wait has no bound. The service sets no lock_timeout and no
statement_timeout, and neither is reachable: LabKit's postgres client builds
the DSN internally and exposes no after-connect hook for per-connection
settings, and the pooled deployment (PgBouncer in transaction pooling) would not
hold a session setting across statements anyway.
statementBoundStore in cmd/artifact-registry/operator.go records the same
constraint and bounds its calls with a context deadline instead.
Nothing in the HTTP layer supplies one. internal/server/server.go sets only
ReadTimeout, WriteTimeout and IdleTimeout; none of those cancels a request
context, and there is no http.TimeoutHandler and no per-request deadline
middleware.
The gate is not reachable from a request path today, which is why this is a
guard to put in place rather than a defect to fix. Both current callers,
NpmRemoteRepositoryStore.CreateNpmRemoteRepository and
MavenRemoteRepositoryStore.CreateMavenRemoteRepository, have no caller outside
internal/datastore and its tests.
What the caller has to do
When a request path first calls either remote create, that caller must:
- Wrap the call in a
context.WithTimeoutbefore it reaches the store, the waystatementBoundStorebounds each operator store call. The deadline belongs at the caller because only the caller knows the request's budget. - Pass a transaction handle when it binds to a parent an earlier transaction
committed. On a pool handle the gate runs in its own implicit transaction and
releases the lock before the caller's
INSERT, so the caller still races. The gate's doc comment states this contract. - Classify a deadline that fires as a server-side timeout rather than a client
disconnect.
logAndWriteInternalError(internal/managementapi/resolve.go) currently returns without logging and without writing a body for both, so an expired deadline would be recorded as a200.
Why the wait is short today, and what changes that
Every current writer of a repositories row holds its conflicting lock for a
single statement on the pool handle: the three counter writes,
RepositoryStore.Update, and RepositoryStore.Delete. The Maven upload path
holds FOR SHARE across its commit transaction but takes it, in its own words,
"at the last moment and held to commit".
Two changes end that. RepositoryStore.SoftDelete gains a request-path caller
in S20-A's destructive repository DELETE, whose plan already prescribes a
context.WithTimeout. And the buffered counter pipeline that
docs/specs/S22-storage-accounting.md
specifies commits a whole chunk of scopes in one transaction, locking every
repositories row in the chunk FOR NO KEY UPDATE and holding those locks
across Redis round trips. A gate whose parent sits in such a chunk waits for the
chunk, not for a statement.
Observability
While the wait happens it is invisible. No query in internal/datastore is
instrumented, and a statement parked on a lock never ends its span, so it
exports none. Pool saturation is the only indication, and that gauge has no
alert. See #54.
Out of scope
- Instrumenting the statements. That is #54.
- Bounding the holder at the PostgreSQL instance
(
idle_in_transaction_session_timeout,transaction_timeout), which is #63.
Surfaced during the review of !1513 (merged).