compiler: artifact server mode, atomic uploads, and a compilation demand count - #6790
Conversation
mythical-fred
left a comment
There was a problem hiding this comment.
High-level pass on the draft — I'll wait for ready-for-review before nitpicking.
Architecture
Splitting artifact_server_main out from compiler_main by sharing spawn_compiler_http_server and running only the janitor is the right shape: one durable binary store, N ephemeral compiler workers, and the two share exactly the HTTP surface and nothing else. The seam is clean.
The upload path (save_file → temp file + sync_all → rename → fsync_parent_dir) is now correctly durable. The old code left partial or unfsynced files behind on any failure; this doesn't. Nice.
count_pipelines_needing_compilation gets the invariant right by construction — a single WHERE that is intentionally the union across shards and platform versions of the four worker queries — and you added mirror comments at all four picker sites. That is the correct discipline. Two thoughts:
-
The predicate hardcodes
program_statusas string literals('pending', 'compiling_sql', 'sql_compiled', 'compiling_rust'). If a new intermediate status is ever added, the four worker WHEREs will be updated (compilation would break otherwise), but this count could silently drift and only surface as a scaling bug. A shared constant list or a typed enum feeding all five call sites would collapse the whole invariant into one place. Not blocking, but the mirror comments only help if someone actually reads them. -
The public wrapper on
StoragePostgres(impl StoragePostgres { pub async fn count_pipelines_needing_compilation ... }) is called out as needed because the enterprise runner cannot name the crate-privateStoragetrait. That is worth apub(crate)visibility check — is the trait actually the wrong visibility, or is the wrapper the right long-term shape? Either answer is fine; just make sure it's the intended one.
Health check split
?deep=true gating storage pressure only when the cluster monitor asks is exactly the right call — Kubernetes liveness/readiness must not restart a compiler pod because the disk filled, since the pod is still serving already-compiled binaries. The 95% threshold and the message calling out "operator must grow the volume" both read well.
ENOSPC → 507 + terminal SystemError
is_out_of_storage_error matching on ErrorKind::StorageFull is right (stabilized 1.83, MSRV 1.93 fine). Worth double-checking that all four failure sites in the upload path — chunk write, flush, sync_all, rename, and fsync_parent_dir — surface as StorageFull when the volume is genuinely out of space (in practice ENOSPC on Linux hits all of them, but the mapping in tokio's fs wrappers is worth a spot-check). If one of those bubbles as a different ErrorKind, workers will chew through their retry budget instead of failing fast.
Ephemeral dir GC (1 h)
ORPHANED_EPHEMERAL_DIR_MAX_AGE = 1 hour for validation directories is very safe (validations finish in seconds) but implicitly assumes no validation ever takes longer than that. If someone ever validates a huge SQL program with a cold SQL compiler jar warmup, it would be unpleasant to have their working directory yanked mid-run. Worth an assertion elsewhere that validation has a hard timeout well below 1 hour, or bump the constant to something obviously safe (24 h). The GC pass runs every CLEANUP_INTERVAL, so the cost of a longer window is essentially zero.
JAR cache refactor
Extracting decide_stale_jar + jar_cache_dir and making both usable from the artifact-server janitor is a nice tidy. The 7-day retention constant now has a name (JAR_CACHE_RETENTION), and the two janitors share the exact same policy — good.
Minor
save_file's temp filename usesUuid::now_v7()— deterministic ordering, good for debugging orphans.remove_temp_upload_filelogs and swallows on non-NotFound— right call for a best-effort cleanup.- The doc comment on
count_pipelines_needing_compilationis doing real work; keep it. If you ever add a new intermediate status, the CI story would ideally be a test that fuzzes program_status values and asserts count > 0 iff any of the four pickers would return a row.
Overall: solid piece of infrastructure work, invariant discipline is exactly what I want to see on things that drive autoscaling. Waiting for ready-for-review.
mythical-fred
left a comment
There was a problem hiding this comment.
LGTM. Atomic upload path (temp file in same dir, sync_all, rename, parent-dir fsync, cleanup on every error path) reads right, the ENOSPC 507 fast-fail is wired end-to-end, and the retry classifier is nicely tested. Two very small non-blocking notes inline.
| _ = http_server => "Compiler HTTP(S) server task ended prematurely", | ||
| /// Age above which an ephemeral validation directory is an orphan of a crashed | ||
| /// validation; live validations finish within seconds. | ||
| const ORPHANED_EPHEMERAL_DIR_MAX_AGE: Duration = Duration::from_secs(3600); |
There was a problem hiding this comment.
Nit (non-blocking): 1h implicitly assumes validation never exceeds 1h. If a future change bumps the validation timeout past 1h without touching this constant, an in-flight validation directory could be nuked. Either tie this to the validation timeout in code (e.g. 2 * VALIDATION_TIMEOUT), or bump to 24h — GC cost is negligible and the safety margin is generous.
| "SELECT COUNT(*) | ||
| FROM pipeline AS p | ||
| WHERE p.deployment_resources_status = 'stopped' | ||
| AND p.program_status IN ('pending', 'compiling_sql', 'sql_compiled', 'compiling_rust') |
There was a problem hiding this comment.
Nit (non-blocking): the four worker queries above and this count are kept in sync by four separate // Predicate changes must be mirrored... comments — which works, but a shared const COMPILATION_ACTIVE_PROGRAM_STATUSES: &[&str] = &["pending", "compiling_sql", "sql_compiled", "compiling_rust"]; (or a small helper returning the predicate fragment) would collapse the invariant into a single source of truth. Fine to defer.
Counts stopped pipelines whose program status is pending, compiling_sql, sql_compiled or compiling_rust: exactly the union of the four compiler worker queries across shards and platform versions, so the count is nonzero if and only if some compiler worker would act. The enterprise runner uses it to derive compiler autoscaling demand. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
…ures artifact_server_main runs the compiler HTTP surface plus a janitor without the SQL and Rust compile tasks. It backs deployments where compiler workers scale to zero and a small always-on pod stores and serves binaries (enterprise compiler autoscaling). The janitor garbage-collects pipeline binaries of deleted or recompiled pipelines, ephemeral validation directories orphaned by crashed validations, and stale SQL compiler jars. Binary and program info uploads stream to a temp file and rename onto the final path after checksum verification and fsync, so an interrupted upload can no longer leave a truncated file under a valid name. Cleanup removes orphaned temp files after an hour. Upload failures no longer park programs in SystemError: transport errors, 5xx, 408 and 429 leave the row in CompilingRust so the regular reset retries the compile and upload once the endpoint recovers; other 4xx responses are permanent rejections and still surface as SystemError. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
Covers the 0-or-N model, the always-on artifact server, configuration defaults, cold start latency expectations, the enable and disable procedures for existing installations including binary store seeding, and troubleshooting. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
…e pressure Persistent binary upload failures (a full binary store, a long-dead endpoint) need an operator, so after the in-cycle retry budget the program parks in SystemError naming the cause instead of silently recompiling forever, which pinned the autoscaled fleet at N with no user-visible error when the artifact store filled up in a dev-cluster incident. Permanent 4xx rejections keep failing immediately; transient blips are still absorbed by the exponential retry backoff. The compiler /healthz gains a deep variant (?deep=1) that fails at 95% usage of the working-directory filesystem; the cluster monitor polls it so /v0/cluster_healthz warns before uploads start failing. Kubernetes probes keep the shallow variant: a full disk must not restart a pod that still serves compiled binaries. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
The artifact server returns 507 Insufficient Storage when a binary or program info write fails with ENOSPC, and workers classify 507 as a permanent rejection: the compilation parks in SystemError on the first attempt instead of retrying for half an hour against a volume that only an operator can grow. Other 5xx responses keep the retry budget for genuinely transient failures. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
Typed deep-healthz query instead of substring matching, StorageFull kind instead of errno 28, one shared jar-cache retention constant, the demand-count invariant referenced from the four worker queries it mirrors, a shared upload-failure classifier, and docs corrections (50Gi artifact store, safe disabling order, upload failure classes). Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
The pre-upgrade hook recreates the compiler StatefulSet on upgrades that flip the immutable podManagementPolicy, so toggling compilerAutoscaling no longer needs manual kubectl steps. Documents the no-seed enable path and restructures disabling into with and without binary preservation. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
…itor The janitor's age sweeps duplicated the directory-walking machinery in util.rs and the jar staleness decision in cleanup_sql_compilation. cleanup_specific_directories gains the same metadata support as the files variant, the jar decision moves into decide_stale_jar shared by the worker cleanup and the janitor (unifying on access time), and the bespoke remove_stale_entries is gone. Also simplifies the autoscaling docs' demand description. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
Adds the typical 1 to 3 minute cold-start latency, concrete air-gap mirroring for the hook kubectl image, and an explicit binary-restore pod manifest with source and target paths; drops reassurance filler, the unwedge and rollback caveats, and the retry-tuning aside. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
Rename the /healthz storage probe from '?deep=true' to '?check_storage=true', which says what it checks instead of naming a variant. Treat EROFS like ENOSPC: both mean the binary store cannot accept writes and only an operator can fix it, so uploads fail fast with 507 rather than burning the retry budget. 'unwritable_store_cause' now returns the cause plus the resolving action, and the 507 message names both and links the operator documentation. Extend the Out-of-storage Errors guide with a compiler binary store section, since the existing text covered only the per-pipeline volume. Build the temp upload name with 'with_added_extension', and assert the errno-to-ErrorKind mapping with 'libc::ENOSPC'/'libc::EROFS' instead of a hardcoded 28. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
Enables let chains, which collapse 33 nested 'if' blocks across the crate and the storage check in the compiler health handler. Two changes are more than mechanical: - 'build_app' returns 'impl Trait' that edition 2024 would make capture its argument lifetimes, forcing every caller's config to live for 'static. 'use<>' keeps the returned app capturing nothing, which holds because it owns everything it needs. - 'emit_logs' set the environment it had already been handed: 'run_child' passes RUST_LOG, NO_COLOR and the log format to the child, and 'use_json_log_format' reads FELDERA_LOG_JSON=0 as false. Dropping the dead 'set_var' calls avoids the edition 2024 unsafety rather than wrapping it, which matters because 'set_var' races in a threaded test harness. The rest is rustfmt style edition 2024 reordering imports, plus two clippy fixes in test code. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
6d5f7c4 to
1f2e052
Compare
mythical-fred
left a comment
There was a problem hiding this comment.
Re-approve. Two new commits since my prior approval on 6d5f7c4e, both good.
9319daf8 — review-response tightening on upload/health:
?deep=true→?check_storage=trueis the right rename; the old name told you nothing about what "deep" meant. Docstring now separately explains why Kubernetes probes omit the param and why the cluster monitor passes it, which is exactly the confusion that made "deep" fragile.- Extending the fail-fast path from ENOSPC to also cover EROFS (
ReadOnlyFilesystem) is a real bug catch: a kernel-remounted-read-only volume was previously a retryable error even though only a human can fix it.unwritable_store_causereturningOption<&'static str>with the cause + resolution baked into the string keeps the caller straightforward and the 507 payload actionable. - New
Compiler binary storesection inoperations/guide.mdgives operators the exact runbook (grow volume vs. repair disk, 200–300 MB per program version budget, GC on delete). Good. - Small
.with_added_extension(...)cleanup — nicer than thefile_name()+to_string_lossy()dance the old code had. Requires nightly / recent stable; presumably already available given the toolchain in use.
1f2e0529 — pipeline-manager to edition 2024:
- 54-file mechanical migration confined to
pipeline-manager; the substantive win is collapsing 33 nestedifblocks via let-chains. Skimmed a handful (api/main.rs,auth.rs,db/error.rs,runner/pipeline_automata.rs) and it's the expected rustfmt-style-edition import reordering plus let-chain simplifications, no semantic drift. use<>onbuild_app's return type is the sort of edition-2024 opaque-type-capture nit that's easy to get wrong; worth a second look during merge that it doesn't accidentally widen or narrow captures.
No new blockers on the delta. Approving on 1f2e0529.
Compiler-service infrastructure for dynamically scaled compiler server deployments; inert in the single-process OSS deployment.
artifact_server_main: runs the compiler HTTP surface plus a janitor without the SQL and Rust compile tasks.SystemErrorinstead of retrying.count_pipelines_needing_compilation: counts pipelines with outstanding compilation work.?check_storage=truevariant that reports storage pressure of the working directory.pipeline-managermoves to edition 2024 (separate commit), which enables let chains and collapses 33 nestedifblocks. Mostly mechanical: rustfmt style-edition import reordering, oneuse<>onbuild_app's return type, and deadenv::set_varcalls dropped from a test.Describe Manual Test Plan
Unit tests cover the upload paths, cleanup decisions, health check thresholds, and the new count query (deterministic plus model-based against embedded postgres). Exercised end to end on an internal Kubernetes deployment, including binary-store exhaustion and recovery.
Checklist
Breaking Changes?
Mark if you think the answer is yes for any of these components:
Describe Incompatible Changes
None.