| Author | Andy Green <andy@warmcat.com> 2026-09-20 07:55 UTC | | Committer | Andy Green <andy@warmcat.com> 2026-09-21 07:08 UTC | | Tree | c43cb51e223a1cd027b9143663fb3fc97c50b9a1 Raw Patch | | | builder, web: keep the ns alive across artifact uploads, serve downloads under /sai | builder, web: keep the ns alive across artifact uploads, serve downloads under /sai
On the builder, the artifact upload SS user object is zalloc'd, so an
in-flight upload never knew its ns: its DESTROYING could not account
against ns->count_artifacts, and nothing could find the uploads when the
ns went away first, so the task grace / cancel paths could free the ns
under a still-running upload. Link each upload to its ns at creation
(ap->ns, ns->artifact_owner) and make the teardown paths agree on who
finishes: the last upload's DESTROYING destroys the ns, while
saib_task_destroy() and sai_ns_destroy() destroy any in-flight uploads
first and mark the ns destroying so those DESTROYINGs don't recurse
into a task teardown that is already under way. While uploads hold the
ns alive, replace the 20s grace timer with a 10 minute hard cap so a
stream stuck retrying can't pin the ns (and the builder's idle / power
state) forever.
On the web side, the pages live under the /sai mount, so the artifact
download links resolve to /sai/artifacts/... at sai-web; accept that
alongside /artifacts/ and make the JS links explicit about it.
Also add churn-management guidance for agents to AGENTS.md.
|
diff --git a/AGENTS.md b/AGENTS.md
index ebf8fd7..7f4cc91 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -26,6 +26,21 @@ Please bear in mind what parts of the system are secrets and look after the secu
In particular, all web pieces are made available on the internet with a strict CSP. That means
no inline styles or scripts. You can find the web pieces (JS, HTML, css) in ./assets/
+## Churn management
+
+When you produced fixes, if possible (main branch, target is within 8 patches back,
+no intervening non-sai- tag) it's preferable to --amend apply the fixes directly to
+the patch that originated the problem, essentially editing the history, even if
+it means just doing that and not adding any fix patch or explanation. Similarly, unless
+asked to produce a new patch or the goal is an explicit phased series, if it's on the
+main branch and we are iterating on the same work, and HEAD patch is yours from the
+last iteration, it's preferable to directly use --amend on it to commit.
+
+If the changes are for mixed purposes, if you initiate new core library changes or fixes,
+these should be broken out into their own patch, even if the rest relates to recent
+changes and is squashed in with those.
+
+
## Build testing
Please don't worry about build-testing, just push patches when you are confident they are complete and have considered all affected code (ie, not half-assed) and ready and I will try them and report back with grounded information.
diff --git a/assets/sai.js b/assets/sai.js
index a76f7c9..887ac9f 100644
--- a/assets/sai.js
+++ b/assets/sai.js
@@ -4703,7 +4703,7 @@ function ws_open_sai()
case "com-warmcat-sai-artifact":
console.log(jso);
- sai_arts += "<div class=\"sai_arts\"><img src=\"artifact.svg\"> <a href=\"artifacts/" +
+ sai_arts += "<div class=\"sai_arts\"><img src=\"artifact.svg\"> <a href=\"/sai/artifacts/" +
san(jso.task_uuid) + "/" +
san(jso.artifact_down_nonce) + "/" +
san(jso.blob_filename) + "\">" +
diff --git a/src/builder/b-artifacts.c b/src/builder/b-artifacts.c
index 7636431..483b9d5 100644
--- a/src/builder/b-artifacts.c
+++ b/src/builder/b-artifacts.c
@@ -176,8 +176,14 @@ saib_artifact_state(void *userobj, void *sh, lws_ss_constate_t state,
}
unlink(ap->path);
- ap->ns->count_artifacts--;
- if (!ap->ns->count_artifacts) {
+ /*
+ * Account for us on the ns, and if we were what it was waiting
+ * for, destroy it. If the ns is already destroying us via
+ * saib_task_destroy(), it will complete itself.
+ */
+
+ if (ap->ns && ap->ns->count_artifacts &&
+ !--ap->ns->count_artifacts && !ap->ns->destroying) {
lwsl_notice("%s: last artifact completed, destroying ns now\n", __func__);
saib_task_destroy(ap->ns);
}
diff --git a/src/builder/b-sai.c b/src/builder/b-sai.c
index 53e8039..88320a8 100644
--- a/src/builder/b-sai.c
+++ b/src/builder/b-sai.c
@@ -504,6 +504,24 @@ void sigint_handler(int sig)
void
sai_ns_destroy(struct sai_nspawn *ns)
{
+ /*
+ * In-flight artifact uploads still reference the ns; their SS handles
+ * outlive it until lws_context_destroy() tears them down, which would
+ * then touch freed ns. Destroy them first, and mark the ns as already
+ * destroying so their DESTROYING doesn't try to run the full task
+ * teardown from inside builder shutdown.
+ */
+
+ ns->destroying = 1;
+
+ lws_start_foreach_dll_safe(struct lws_dll2 *, d, d1,
+ ns->artifact_owner.head) {
+ sai_artifact_t *ap = lws_container_of(d, sai_artifact_t, list);
+ struct lws_ss_handle *h = ap->ss;
+
+ lws_ss_destroy(&h);
+ } lws_end_foreach_dll_safe(d, d1);
+
lws_dll2_remove(&ns->list);
free(ns);
}
diff --git a/src/builder/b-task.c b/src/builder/b-task.c
index 749cd84..c553241 100644
--- a/src/builder/b-task.c
+++ b/src/builder/b-task.c
@@ -351,7 +351,18 @@ saib_task_destroy(struct sai_nspawn *ns)
{
int n;
- lwsl_notice("====== saib_task_destroy START (ns=%p, uuid=%s, spm=%p) ======\n",
+ /*
+ * The last in-flight artifact upload calls this from its own
+ * DESTROYING; if we got here first via the cleaner or cancel paths, the
+ * outstanding artifact destroys below would otherwise call us back
+ * reentrantly.
+ */
+
+ if (ns->destroying)
+ return;
+ ns->destroying = 1;
+
+ lwsl_notice("====== saib_task_destroy START (ns=%p, uuid=%s, spm=%p) ======\n",
(void*)ns, ns->task ? ns->task->uuid : "null", (void*)ns->spm);
lwsl_notice("%s: destroying task %s\n", __func__,
@@ -361,6 +372,24 @@ saib_task_destroy(struct sai_nspawn *ns)
lws_sul_cancel(&ns->sul_task_cancel);
/*
+ * Any artifact uploads still referencing us must go first... their
+ * DESTROYING unlinks their temp file and accounts against
+ * ns->count_artifacts, but skips the recursive destroy since we are
+ * already destroying.
+ */
+
+ lws_start_foreach_dll_safe(struct lws_dll2 *, d, d1,
+ ns->artifact_owner.head) {
+ sai_artifact_t *ap = lws_container_of(d, sai_artifact_t, list);
+ struct lws_ss_handle *h = ap->ss;
+
+ lwsl_notice("%s: destroying in-flight artifact %s\n", __func__,
+ ap->path);
+
+ lws_ss_destroy(&h);
+ } lws_end_foreach_dll_safe(d, d1);
+
+ /*
* If able, builder should reintroduce himself to get
* another task
*/
@@ -617,6 +646,17 @@ artifact_glob_cb(void *data, const char *path)
/* take a copy so we can unlink the path later */
lws_strncpy(ap->path, upp, sizeof(ap->path));
+ /*
+ * The upload outlives the task's own steps and must know its ns, so
+ * DESTROYING can account for it and destroy the ns when the last one
+ * finishes; and saib_task_destroy() can find and kill in-flight uploads
+ * if it goes first. The SS user object is zalloc'd, so without this
+ * ap->ns is NULL.
+ */
+
+ ap->ns = ns;
+ lws_dll2_add_tail(&ap->list, &ns->artifact_owner);
+
lwsl_notice("%s: artifact ss created '%s'\n", __func__, ap->path);
ns->count_artifacts++;
@@ -646,6 +686,9 @@ artifact_glob_cb(void *data, const char *path)
* of the task and reset the nspawn.
*/
+/* cap on how long we let artifact uploads hold the ns alive */
+#define SAIB_ARTIFACT_UPLOAD_MAX_US (10 * 60 * LWS_USEC_PER_SEC)
+
static void
saib_start_artifact_upload(struct sai_nspawn *ns)
{
@@ -776,9 +819,21 @@ scan:
/* no artifacts to hang around for... nuke the ns now */
lws_sul_cancel(&ns->sul_cleaner);
saib_task_destroy(ns);
- } else
+ } else {
lwsl_notice("%s: created / waiting on %d artifact uploads\n",
__func__, ns->count_artifacts);
+
+ /*
+ * The ns now lives until the last upload destroys it, not the
+ * 20s task grace timer... but keep a much longer hard cap so
+ * a stream stuck retrying can't pin the ns (and the builder's
+ * idle / power state) forever.
+ */
+
+ lws_sul_schedule(builder.context, 0, &ns->sul_cleaner,
+ saib_sub_cleaner_cb,
+ SAIB_ARTIFACT_UPLOAD_MAX_US);
+ }
}
void
diff --git a/src/web/w-comms.c b/src/web/w-comms.c
index 0280d50..55a3ef1 100644
--- a/src/web/w-comms.c
+++ b/src/web/w-comms.c
@@ -52,6 +52,7 @@ typedef enum {
SHMUT_BROWSE,
SHMUT_STATUS,
SHMUT_ARTIFACTS,
+ SHMUT_ARTIFACTS_SAI,
SHMUT_LOGIN
} sai_http_murl_t;
@@ -60,6 +61,7 @@ static const char * const well_known[] = {
"/sai/browse",
"/status",
"/artifacts/", /* HTTP api for accessing build artifacts */
+ "/sai/artifacts/", /* same, via the /sai mount the pages live under */
"/login"
};
@@ -451,14 +453,17 @@ w_callback_ws(struct lws *wsi, enum lws_callback_reasons reason, void *user,
goto passthru;
case SHMUT_ARTIFACTS:
+ case SHMUT_ARTIFACTS_SAI:
/*
* HTTP Bulk GET interface for artifact download
*
- * /artifacts/<taskhash>/<down_nonce>/filename
+ * [/sai]/artifacts/<taskhash>/<down_nonce>/filename
*/
lwsl_notice("%s: SHMUT_ARTIFACTS\n", __func__);
pss->artifact_offset = 0;
- if (saiw_get_blob(vhd, (const char *)in + 11,
+ if (saiw_get_blob(vhd,
+ (const char *)in +
+ (mu == SHMUT_ARTIFACTS ? 11 : 15),
&pss->pdb_artifact,
&pss->blob_artifact,
&pss->artifact_length)) {
|