Repository navigation
Review: 9.9.0 patch set (patch/9.9.0-backpressure) - #22
Open
chenrui333 wants to merge 17 commits into
Open
chenrui333 wants to merge 17 commits into
chenrui333 wants to merge 17 commits into
Conversation
A downstream build released `$7_cache_entries_lookup_indexes` and `$8_upload_capability_hash` on top of the shared `$0`-`$6`. The migrator refuses such a database: the executed migrations are missing from this release. Before migrating, drop those two ledger rows and keep their schema: the `uploads.capabilityHash` column on every driver and, on SQLite, the composite lookup index, which are ensured after migrating so fresh and upgraded databases end up identical. The ledger then lists only this release's migrations, so later releases start on it unchanged. The NOCASE prefix index is dropped: with `case_sensitive_like = on` the binary composite index serves prefix matches. On SQLite, migrate while holding the write lock. The `node-cluster` primary and its worker both migrate on start and Kysely's SQLite migration lock is a no-op, so the loser failed with `duplicate column name: reservationHash`; under the lock it waits and finds nothing left to apply, and an interrupted migration leaves no partial schema. Signed-off-by: Rui Chen <rui@chenrui.dev>
…very storage driver (falcondev-oss#291) Co-authored-by: Peter Svensson <peter@sparetimecoders.com> Signed-off-by: Rui Chen <rui@chenrui.dev>
The first download of an unmerged entry tees its parts into the client response and the background merge upload. A destroyed stream returns false from write() and never emits 'drain' (nor 'error' when destroyed without one), so the pump parks forever when: - the merge upload fails: `download()` destroys the merger stream, and the client's download stalls mid-body until the client times out; - the client hangs up while the pump waits on the merger: the next write to the destroyed response stream parks the pump, the merge never completes, and its lease keeps renewing. Either way the current part's backend read stays open (one S3 socket per occurrence), so enough of them fill the S3 socket pool. Wait for 'drain' or 'close', skip destroyed sinks, keep feeding the one that is still open, and stop reading once both are gone so `streamParts` destroys the current part stream. The regression tests fail on the previous pump with the filesystem and S3 drivers: the merge never settles after the client leaves, the client never finishes after the merge upload fails, and the part stream is never destroyed when both are gone. Signed-off-by: Rui Chen <rui@chenrui.dev>
`streamParts()` awaited `globalThis.gc()` after every part, and the image starts Node with `--expose-gc`, so every part of every unmerged restore and streaming merge ran a synchronous full mark-compact on the event loop. With ~1 MiB parts (BuildKit) that caps all concurrent first restores combined at roughly one part per GC pause, and the pause grows with the heap, so more concurrent restores make every one of them slower. Memory no longer depends on it: the upload route lets storage pace the body (falcondev-oss#275) and the download route pipes with backpressure (falcondev-oss#282), so the bytes in flight are bounded by stream high-water marks and become ordinary garbage that V8 collects incrementally. Remove both calls, the `--expose-gc` flag and the startup warning about it. Signed-off-by: Rui Chen <rui@chenrui.dev>
…endpoints The S3 client's agent pool was hard-coded at 50 sockets, and the agent was passed only as `httpsAgent`. NodeHttpHandler picks the agent by endpoint scheme, so an http:// endpoint (MinIO, RustFS, an in-cluster gateway) silently fell back to the SDK's default agent and ignored the configured pool. Local tests and benchmarks against RustFS therefore never exercised the pool an https deployment uses. Add STORAGE_S3_MAX_SOCKETS (default 50, the previous value) and build both the http and the https agent from the same options. tests/s3-socket-pool.test.ts holds one GetObject body unread with a pool of one socket and checks that a second GetObject waits for it; against an http:// endpoint it did not wait before this change. Signed-off-by: Rui Chen <rui@chenrui.dev>
All S3 traffic shared one client and one agent pool. A streaming Merge (the first restore of an unmerged entry, or a streaming Eager Merge) holds a part GetObject open, paused by backpressure, while lib-storage waits in that same pool for a socket to upload the merged object. Once concurrent Merges reach maxSockets, every socket is a paused GET waiting for a socket that never frees: the pool deadlocks, Merge Leases renew forever, no Merge or restore completes, and every later S3 call queues behind them. That includes the HeadObject each cache lookup issues, which also waits behind every bulk transfer in the agent's FIFO queue even without a deadlock. Use one S3Client per kind of work: - reads: GetObject (and presigning) (STORAGE_S3_MAX_SOCKETS) - writes: PutObject, multipart uploads, UploadPartCopy compose (STORAGE_S3_MAX_SOCKETS) - control: HeadObject, HeadBucket, ListObjectsV2, DeleteObjects (STORAGE_S3_CONTROL_MAX_SOCKETS, default 16) Write-socket holders never wait on reads, and control requests hold a socket only briefly, so no wait cycle spans pools. The control client also sets connectionTimeout (STORAGE_S3_CONTROL_QUEUE_TIMEOUT_MS, default 5000), which bounds the wait for a socket plus the connect, so a starved lookup fails instead of hanging. tests/merge-socket-pool.test.ts runs as many concurrent first restores as pool sockets (4) against RustFS; with the shared pool it never finishes. tests/s3-socket-pool.test.ts checks that a HeadObject is answered while every data socket is held by an unread GetObject body. Signed-off-by: Rui Chen <rui@chenrui.dev>
Every first restore of an unmerged entry and, with EAGER_MERGE, every finalize started a Merge with no global limit. Each Merge holds storage connections, a Merge Lease and stream buffers for its whole duration, and a lazy Merge is paced by the restoring client's read speed. A burst of first restores or of finalizes therefore grew Merges, sockets and memory without bound. Start a Merge only while fewer than MAX_CONCURRENT_MERGES (default 16) run in this process. Beyond that, startMerge returns false: a restore streams the parts directly, exactly as when another worker holds the Merge Lease, an Eager Merge is left to the first download, and a later download merges the entry. The slot is reserved before the first await, so concurrent callers cannot overshoot it. tests/merge-concurrency.test.ts covers a first restore and an Eager Merge beyond the limit. Signed-off-by: Rui Chen <rui@chenrui.dev>
Node's HTTP server has no idle timeout for a response in progress and the download route set none. A client that stays connected but stops reading parks the pipe forever: the backend read keeps its storage socket, the Storage Reader Lease keeps renewing, and on the first restore of an unmerged entry the Merge it paces never finishes and keeps its Merge Lease. Each such client permanently removes one socket from the storage pool. Destroy the download when no bytes have moved for DOWNLOAD_IDLE_TIMEOUT_MS (default 300000, 0 disables). The timer is refreshed on every chunk, so slow but progressing clients are unaffected. It covers whole and Range responses and the first restore of an unmerged entry, whose Merge keeps going after the client's side of the tee is closed. tests/proxy-backpressure.test.ts parks a client after the first chunk of a 200, a 206 and an unmerged first restore, and checks that the storage read is released, the response closed, and the Merge completed. Signed-off-by: Rui Chen <rui@chenrui.dev>
… storage answers A client can give up on /download while its storage read is still waiting, for example a GetObject queued behind a saturated socket pool. The body then arrives for a response that is already closed. `pipeline` rejects a closed destination with ERR_STREAM_UNABLE_TO_PIPE before it takes ownership of the source, and the route rethrew without destroying it, so the body pinned its backend socket and the Storage Reader Lease for the life of the process. Enough of them fill the pool and every storage call queues. Check the response before piping and destroy the body if the client is gone, and destroy it on every error exit of the pipe as well. falcondev-oss#282 and falcondev-oss#291 already cover a client leaving mid-body. tests/download-client-gone.test.ts holds the storage read until the client has hung up, then checks that the backend socket and the reader lease are released; on Node 26.12.1 the socket stayed held before this change. Signed-off-by: Rui Chen <rui@chenrui.dev>
…metrics
A starved S3 socket pool (paused GetObject sockets, a deep agent queue,
HeadObject lookups timing out, Merge Leases never finishing) was invisible on
/metrics. Add low-cardinality series that separate S3 latency, socket-pool
queueing, Merge contention and client behaviour:
- s3_http_sockets{pool,state}, s3_http_queued_requests{pool},
s3_http_max_sockets{pool}: agent pool state at scrape time, per pool
(reads, writes, control).
- s3_socket_wait_seconds{pool}: time a request waited for an agent socket.
- s3_requests_total{command,outcome}, s3_request_duration_seconds{command},
s3_request_retries_total{command}: per-command outcome and latency from an
SDK initialize-step middleware (includes retries and socket wait).
- cache_merges_total{kind,result}, cache_merge_duration_seconds{kind,result},
cache_merges_in_progress, cache_merge_oldest_age_seconds,
cache_merges_skipped_total (MAX_CONCURRENT_MERGES reached).
- cache_leases{type,state}: Merge and Storage Reader Lease rows, active vs
expired.
- cache_downloads_total{result}: completed, client_gone, idle_timeout,
not_found, error.
- cache_lookup_duration_seconds{result}: download-URL lookup latency.
Nothing is recorded per chunk; pool and lease gauges are computed at scrape.
The registry stays per process (ADR-0007).
Signed-off-by: Rui Chen <rui@chenrui.dev>
Add a workflow that runs only for `patch-<version>` tags and builds the multi-arch image (linux/amd64, linux/arm64) into an OCI archive uploaded as a workflow artifact (`image-<version>`, kept 7 days). It logs in nowhere and pushes nothing; the archive is published by hand. ci-cd.yml (branch pushes, dev deploys) and release.yml (`v*` tags) are unchanged and do not match these tags. Signed-off-by: Rui Chen <rui@chenrui.dev>
getDatabase checks a client out of the pool to fail fast on bad connection settings and never released it. That client stays checked out for the life of the process, leaving the pool one connection short, and Kysely's destroy() (pool.end()) would wait for it forever. Signed-off-by: Rui Chen <rui@chenrui.dev>
`DELETE /management-api/cache-entries` accepted a `repoId` filter and documented it, but never applied it, so a delete filtered only by repository removed every cache entry of every repository. Apply it like the other filters. The Rust rewrite in falcondev-oss#288 already fixes this; this is the same fix for the current TypeScript server. Signed-off-by: Rui Chen <rui@chenrui.dev> (cherry picked from commit 62e763e)
A direct-download URL is a bearer credential for the stored object, and the only gate in front of it is the token's `repository_id` and `ac` claims. With SKIP_TOKEN_VALIDATION those claims are never verified, so the server now refuses to start with ENABLE_DIRECT_DOWNLOADS and SKIP_TOKEN_VALIDATION together, unless UNSAFE_ALLOW_DIRECT_DOWNLOADS_WITHOUT_TOKEN_VALIDATION is set for local testing. - DIRECT_DOWNLOAD_URL_TTL_SECONDS (default 300, 1-3600) sets the signed URL lifetime. The reader lease outlives the URL by a fixed margin, since storage authenticates a GET only when it starts. - Token verification requires `exp`; ACTIONS_TOKEN_AUDIENCE optionally pins `aud`, and a value that lists no audience stops startup. - Token rejections report the jose error code and claim name instead of attaching the jose error, whose decoded payload was logged with it. - `ac` scopes are schema-checked and treated as bit flags: lookups use only readable scopes, uploads and finalizes only writable ones. The tests run a second server against a local OIDC issuer (discovery and JWKS), covering key rotation, a JWKS outage, fork pull request scopes and the signed URL lifetime on S3, GCS and Azure Blob. Signed-off-by: Rui Chen <rui@chenrui.dev>
The error hook, the debug request/response hooks and the Results Passthrough logged `event.path` with its query string. Upload URLs are used by Azure Blob-style clients that put SAS-style parameters in the query, so those logs could record credentials. Log the path without its query string. Storage errors in the upload route are logged by the route and rethrown as a handled error, because Nitro logs unhandled errors with the full request URL. The request-logging hooks move from `plugins/setup.ts` into their own plugin so the test can drive them against the real upload route. Signed-off-by: Rui Chen <rui@chenrui.dev>
Upload URLs are used by Azure Blob-style clients that send no Actions token, and until now they carried only the numeric upload id. Issue a random 256-bit capability with every upload, return it in the `signed_upload_url` query (`?sig=`), and require it on single-shot, block and blocklist requests. Missing or wrong capabilities, unknown uploads and uploads created without a capability all get the same 403. An authorized part whose upload is deleted while it streams still gets 404. Only the SHA-256 of the capability is stored, in `uploads.capabilityHash` (the column the migration bridge ensures), and compared in constant time. URL shape, parameter and hash are the ones the previous release issued, so uploads begun before an upgrade can be completed after it. Upload URLs requested with a method other than PUT now get 405 instead of falling through to the Results Passthrough, which forwarded the URL, capability included, to the upstream Results service. Clients already keep the signed URL's query and append their own parameters (as with an Azure SAS), so no client change is needed. See ADR 0013. Signed-off-by: Rui Chen <rui@chenrui.dev>
`lib/storage.ts` imported `@azure/storage-blob`, `@azure/identity` and `@google-cloud/storage` at the top level, so every server loaded all three SDKs whatever `STORAGE_DRIVER` said. Since the Azure Blob driver (falcondev-oss#280) that adds about 40 MB of live heap to an idle S3 or filesystem server, and every GC cycle has to walk it. Import each SDK inside its adapter's `fromEnv`; only types stay as static imports. Signed-off-by: Rui Chen <rui@chenrui.dev> (cherry picked from commit 00a7d6c)
Owner
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review copy of the full 9.9.0 patch set (
patch/9.9.0-backpressure@110269c) against upstreamv9.9.0. It's for review and CI only; nothing gets merged here.Commits:
9bab33e). This is the only commit not already reviewed as a separate PR.12b6ac4, fork-only), Postgres probe (fix(db): release the Postgres startup probe client #20), deleteMany repoId (fix(management-api): honor repoId when deleting cache entries #10), token safeguards (fix(auth): token validation and direct-download safeguards #11), log redaction (fix(logging): keep query strings out of request logs #12), upload capability (feat(upload): signed upload URLs #13), lazy SDK load (perf(storage): load the Azure and GCS SDKs only when selected #21).