feat(scripts): concurrent runs, results, progress and cancel (#1843) - #1857
Merged
Merged
Conversation
Eight changes on the managed-script run path: run_script, manage_script (help, validate, get_run, cancel_run), platform.query, platform.call, trino_export / api_export / graphql_export inside a run, the run worker, and the portal run routes. 1. #1843: the run worker executes runs concurrently. Admission is adaptive by default: a replica claims another run while its memory is under 70% of the container limit (or GOMEMLIMIT) and its CPU under 75% of its quota, always at least min_concurrency and never more than max_concurrency (16); past 90% memory it stops and requeues its newest run on the platform's retry budget. A fixed scripts.worker.concurrency, including 1, still works. run_timeout (default 15m, was a fixed 10m), max_steps, max_query_rows and result_max_bytes are configurable and reported by manage_script help; the claim lease is the timeout plus five minutes. New metrics: script_run_admission_refusals_total{reason} and script_run_queue_wait_seconds. 2. #1845: platform.result(value) hands one JSON value back (1 MiB cap, set once; an oversized or non-JSON value fails the run). run_script, get_run and drafts carry it as result. POST /api/v1/portal/scripts/{id}/runs?wait=N answers 200 with the finished run, or 202 with its id as before. 3. #1847: platform.progress(message, done, total) and the log printed so far are written to a running run every two seconds. manage_script cancel_run and POST /api/v1/portal/scripts/{id}/runs/{runID}/cancel stop a run: a queued run never starts, a running one ends canceled within seconds keeping its outputs, and a finished one is left alone. canceled is a new terminal status (migration 000153). The portal run history shows progress, the live log, a Cancel/Stop control and the result. 4. #1852: platform.query and platform.call of a query tool build each row in the SELECT's column order, so rows exported as they came keep their columns in that order. 5. #1853: validate's Python-ism checks read the code with string contents and comments blanked, so a string literal "f" or a SQL column named datetime is no longer reported. The credential scan still reads the raw source. 6. #1854: a file trino_export, api_export or graphql_export writes inside a run is listed on the run's outputs, marked with the tool. A named export inside a run versions the script's asset for that name across runs, the identity platform.export uses (script.OutputIdentityKey, toolkit.PersistRunAsset); resource= and an explicit idempotency_key keep their meaning. 7. #1841: dev/start.sh's probe_bind sets SO_REUSEADDR, so make dev right after make dev-stop no longer fails on ports that only have TIME_WAIT sockets. 8. The runs listing's run_status filter now admits skipped_overlap, which it previously could not filter on, and canceled. New packages, split out for the package size and cohesion budgets: internal/procload, internal/platform/scriptadmit, scriptlex, scriptlive, runcontrol, and internal/httpserver/scripthttp/transferwords. The import allowlist gains the edges those packages and their callers add. Closes #1841 Closes #1843 Closes #1845 Closes #1847 Closes #1852 Closes #1853 Closes #1854
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1857 +/- ##
==========================================
+ Coverage 91.87% 91.92% +0.04%
==========================================
Files 941 953 +12
Lines 90680 91547 +867
==========================================
+ Hits 83314 84150 +836
- Misses 4815 4834 +19
- Partials 2551 2563 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Seven tickets and one filter fix on the managed-script run path, touching
run_script,manage_script(help,validate,get_run, and the newcancel_run),platform.query,platform.call,trino_export/api_export/graphql_exportinside a run, the run worker,POST /api/v1/portal/scripts/{id}/runs, the newPOST /api/v1/portal/scripts/{id}/runs/{runID}/cancel, anddev/start.sh. Eight changes:platform.resulthands a value back, and the portal run route can wait for it.platform.progress, the log so far on a running run, and cancel.validate's Python-ism checks read code, not string contents or comments.probe_bindsetsSO_REUSEADDR.runslisting'srun_statusfilter admitsskipped_overlap(previously unfilterable) andcanceled.1. Concurrent runs (#1843)
A replica executed one run at a time; its capacity was its replica count. The worker now launches each claimed run on its own goroutine under its own context, and decides before every claim whether to take another (
internal/platform/scriptadmit,internal/platform/scriptexec/worker.go).min_concurrency(1) are executing, or while fewer thanmax_concurrency(16) are executing and the replica has headroom.max_memory_percent(70) of the container's cgroup limit orGOMEMLIMIT, and CPU undermax_cpu_percent(75) of the cgroup quota orGOMAXPROCS.runtime/metrics; CPU isgetrusageover a one-second window (internal/procload). The runtime's/cpu/classesmetrics are not used: they update only at GC.SKIP LOCKED).shed_memory_percent(90), the worker stops its most recently started run and requeues it with a memory-pressure reason. That spends the platform's retry budget, not the script's. The last run is never shed.concurrency: Nclaims N at once;1reproduces the previous worker. A value that is neitheradaptivenor a whole number fails config validation at startup.run_timeoutdefaults to 15m (was a fixed 10m);max_steps,max_query_rowsandresult_max_bytesare configurable too.manage_script helpreports them underlimits.scriptexec.LeaseFor, timeout plus 5m), so a long run is not claimed twice.script_run_admission_refusals_total{reason=ceiling|memory|cpu}(counted when the queue held work) andscript_run_queue_wait_seconds, beside the existingscript_runs_running.run_scriptandrun_draftstay refused from inside a run. The stated reason changes from "one run at a time" to what still holds: a run waiting on a run it queued holds a worker slot, and waiters can hold every slot a replica admits.2.
platform.resultand waiting over HTTP (#1845)platform.result(value)sets one JSON value on the run: capped atresult_max_bytes(1 MiB), set once, stored inscript_runs.result.result, notreturn:returnis a Starlark keyword andplatform.return(...)does not parse.run_script,get_runand drafts carry it asresult.POST /api/v1/portal/scripts/{id}/runs?wait=Nholds the request up to N seconds (capped at 300, the same cap asrun_script'swait_seconds).200with the run detail. Otherwise, and withoutwait, the route answers202with the run id as before.run_scriptand the route share one wait,runcontrol.AwaitRun.3. Progress, live log, cancel (#1847)
platform.progress(message, done=None, total=None)records the latest report in memory (internal/platform/scriptlive).RunStore.RecordProgress; an unchanged snapshot leaves the row alone.manage_script command=cancel_runandPOST /api/v1/portal/scripts/{id}/runs/{runID}/cancelare allowed to whoever may read the run: owner, administrator, requester.RunStore.CancelRunlocks the row, finishes a pending run ascanceledin the same statement (never claimed), or stampscancel_requested_at/_byon a running one.runcontrol.CancelMessage.canceledwithin seconds, keeping the outputs it wrote.canceledwithout executing.canceledis terminal: not retried, not mailed, swept by retention.cancel_runis classified as a write for the draft write barrier (internal/toolwrite) and refused inside a run.000153_script_run_liveaddsresult,progress_message/done/total/atandcancel_requested_at/by, and extends the status check withcanceled. The down migration recordscanceledrows asfailed.4. SELECT column order (#1852)
Rows arrived as JSON objects and were converted with sorted keys, so exporting query rows alphabetized the CSV columns.
starlarkconv.RowsToStarlark/ResultToStarlarkbuild each row in the order the result'scolumnsnames them, with any key the columns do not name following in sorted order.platform.queryandplatform.callof a query tool both use it.5.
validatereads code (#1853)The lexical checks moved to
internal/platform/scriptlexand now run overCodeOnly(source). That is the source with string contents and comments blanked, keeping every offset so line numbers stay exact. A string literal"f",r["f"],SELECT datetime ...or a comment mentioningopen(are no longer reported. The credential scan still reads the raw source, because a pasted key sits inside a string.6. Export tools inside a run (#1854)
trino_export,api_exportorgraphql_exportwrites inside a run is recorded on the run's outputs through the output writer's lease-fencedRecordToolOutput. The record carries name, asset and version (or managed resource), format, rows, bytes, and a newtoolfield. It does not claim the name forplatform.export.platform.export's identity: the first run creates the script's asset for that name, and each later run writes its next version.ExportUserContext.RunOutputKey(producedby.RunOutputKey, nil outside a run). One helper,toolkit.PersistRunAsset, does the lookup, insert, version and first-write race for all three tools.script.OutputIdentityKey, and the platform export writer uses it too.resource=still lands in a managed file, an explicitidempotency_keykeeps its meaning, and every call outside a run still creates a new asset.asset_version.7.
probe_bind(#1841)The probe bound without
SO_REUSEADDR, which every listener the stack starts sets. So a port whose only sockets were in TIME_WAIT aftermake dev-stopread as busy for about thirty seconds.Checked on darwin, for four cases (TIME_WAIT only, a live same-user listener, a Docker listener on a specific address, a Docker listener on the wildcard), against
net.Listen: the probe withSO_REUSEADDRanswers the same as the Go server's own bind; the plain bind was stricter in three.Portal
The run history shows a running run's progress under its status (
120 of 500 · entities) and "Stopping" once a cancel is requested. An open run re-reads itself every 3 s while in flight, with Cancel run / Stop run, the log so far, and a Result block.canceledis counted in the summary line. Outputs written by an export tool sayvia <tool>.Readers of this state
The portal JSON contract changed on
portalRun(progress,cancel_requested),portalRunDetail(result,cancel_requested_by), the run route (a200carryingportalRunDetail), the newcancelResponse, andscript.RunOutput.tool. Readers checked:ui/src/api/portal/hooks/scripts.ts:ScriptRun,ScriptRunDetail,ScriptRunOutput,useScriptRun(live refetch),useCancelScriptRun.ui/src/pages/scripts/ScriptRunHistory.tsx: progress line, run control, result.ui/src/pages/scripts/runFormat.ts:progressText, thecanceledcount,outputLinknaming the tool.ui/src/mocks/handlers/scripts.tsandui/src/mocks/data/scripts.ts: the cancel route and a run result, held to the swagger routes byroute-conformance.test.ts.internal/apidocsis regenerated.Decomposition and the import allowlist
The package budgets required splitting the new code into packages:
internal/procload: process memory and CPU against the container's limits.internal/platform/scriptadmit: the admission policy and thescripts.workercapacity section, inlined intopkg/platform.ScriptsWorkerConfig.internal/platform/scriptlex: the lexical checks, moved out ofscriptrun.internal/platform/scriptlive: the run's live log, progress and result.internal/platform/runcontrol: waiting on and canceling a run.internal/httpserver/scripthttp/transferwords: the owner-transfer wording, moved out ofscripthttp.pkg/scriptstays under its exported-surface budget:CancelRunreturns the prior status rather than a new outcome type.The 16 new allowlist edges are these packages and their callers, plus
pkg/platform -> internal/producedbyandinternal/producedby -> pkg/scriptfor the run output key:Behavior that changed for existing tests
TestIntegration_TwoRunsReadingOneRevisionCannotBothWriteasserted the worker executes two queued runs in order. They now run at once, so the test asserts the invariant instead: exactly one writes the state revision and the other fails naming it.TestValidate_WarningsDoNotBlockrelied ondatetimeinside a comment producing a warning, which is the false positive manage_script validate reports an f-string for a string literal "f" #1853 removes.Evidence
test/acceptance/issue_{1843,1845,1847,1852,1853,1854}_test.go, recorded atbuild/<n>/acceptance.jsonl.make devcame up immediately aftermake dev-stopwith 67 sockets in TIME_WAIT on the stack's ports.test/gates/probe_bind_test.gofails against the old probe.RecordProgress,CancelRunfor each prior status, the result and the progress columns (internal/platform/scriptstore/runlive_realdb_integration_test.go).make verifypassed.Closes #1841
Closes #1843
Closes #1845
Closes #1847
Closes #1852
Closes #1853
Closes #1854