From f82237da365a58243be5265690e016e155a8d20d Mon Sep 17 00:00:00 2001 From: librelad Date: Mon, 24 Aug 2026 02:51:18 +0100 Subject: [PATCH] fix(instance): make an instance's own functions reachable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four defects that all reduce to "a function an instance defines is invisible to the code that dispatches it". Reported as `mattermost_teest has no upgrade verifier`, for an app whose verifier was on disk the whole time. * generate_function_manifest.sh shipped 0664. lpRegenArrays invokes it as an executable, so it died rc=126 on every call and `|| true` swallowed it — the manifest was never rebuilt on any live system, only laid down at deploy. Its sibling generate_arrays.sh is 0775, which is why the files_*.sh arrays looked current while the manifest was byte-identical to the shipped copy. * lpRegenArrays now runs both generators through bash rather than depending on the exec bit, reports a manifest failure instead of hiding it, and treats a new containers/ dir as stale — the one event on a live box that adds functions was the one the scripts/-only mtime check could not see. * updaterHasVerifier consults the disk before answering no. The CLI runs LP_LAZY=1, where the container scan is skipped and every function must come from the build-time manifest, so an app created after the build reads as having no verifier. GATE 1 then refuses an upgrade that is fully verifiable, and updaterUpgradeAuto's `|| continue` drops the app in silence for good. Self-healing regardless of manifest staleness, which matters because a self-update restores the shipped manifest and drops instance entries again. * _instanceRewriteTools gains three renames. authAdapter__() was caught by neither the prefix rule (no word boundary before _) nor the suffix rule (needs () right after the type), so the clone defined the base app's adapter name while pointing at its own container — every instance user tool answered "does not implement", and which definition survived came down to find(1) order. Bare-app arguments to authAdapterCall/authPersistCfg went unrewritten too, so an instance's password reset wrote the credential into the base app's config. And dockerAppRunTool wants app, which no rule produced, so every tool on every instance was unreachable. The infix rename runs before the suffix rename: the reverse order appends the id half twice (appSetupComposeTags_nextcloud_work_work). Verified on a live install: the upgrade ladder now plans mattermost_teest 11.9 -> 11.10, and `regen arrays --force` indexes the instance hooks. Also carries in-flight instance-removal regen work from a concurrent session on the same worktree (_lpRegenOrphanedApp, instanceRemove's WebUI refresh). Co-Authored-By: Claude Opus 5 --- .../commands/updater/cli_updater_verify.sh | 21 +++++ scripts/instance/instance_create.sh | 80 +++++++++++++++++++ .../files/generate_function_manifest.sh | 0 scripts/webui/webui_regen.sh | 61 +++++++++++--- 4 files changed, 152 insertions(+), 10 deletions(-) mode change 100644 => 100755 scripts/source/files/generate_function_manifest.sh diff --git a/scripts/cli/commands/updater/cli_updater_verify.sh b/scripts/cli/commands/updater/cli_updater_verify.sh index 4c71dce..dd7f6de 100644 --- a/scripts/cli/commands/updater/cli_updater_verify.sh +++ b/scripts/cli/commands/updater/cli_updater_verify.sh @@ -89,8 +89,29 @@ updaterVerifyGeneric() { } # Does this app ship a real verifier? The stepped engine requires one. +# +# "Not currently defined" is NOT the same as "not shipped", and conflating the +# two is the most expensive mistake this file can make. The CLI runs lazy +# (LP_LAZY=1), where the container scan is skipped entirely and every function +# has to arrive through function_manifest.sh — a BUILD-time artifact. Any app +# that came into existence after that build has no stub, so its verifier reads +# as absent while sitting on disk two directories away. That is exactly every +# multi-instance clone, and the consequences both point the wrong way: GATE 1 +# refuses an upgrade for an app that is fully verifiable, and the auto-ladder's +# `updaterHasVerifier || continue` drops it in silence, for good. +# +# So consult the disk before answering no, and source what is actually there — +# the same file the eager loader would have picked up. Cheap (one stat for the +# common case) and self-healing regardless of how stale the manifest is, which +# matters because a LibrePortal self-update overwrites the manifest with the +# shipped copy and drops every instance entry again. updaterHasVerifier() { local app="$1" + declare -F "${app}_upgrade_verify" >/dev/null 2>&1 && return 0 + + local hooks="${install_containers_dir%/}/$app/scripts/${app}_upgrade_hooks.sh" + [ -f "$hooks" ] || return 1 + source "$hooks" 2>/dev/null || return 1 declare -F "${app}_upgrade_verify" >/dev/null 2>&1 } diff --git a/scripts/instance/instance_create.sh b/scripts/instance/instance_create.sh index f9a860c..5ec10b4 100644 --- a/scripts/instance/instance_create.sh +++ b/scripts/instance/instance_create.sh @@ -363,6 +363,27 @@ _instanceRewriteTools() { # names and prose. sed -i -E "s/(\b(container|container_name)=\")${type}(\")/\1${slug}\3/g" "$f" + # Function names carrying the app as an INFIX. Neither the prefix rule + # (needs at a word boundary — `_mattermost` has none, `_` is a + # word character) nor the suffix rule below (needs the () immediately + # after the type) can see authAdapter__(), so the clone + # defined authAdapter_mattermost_listUsers while auth_adapter.sh + # dispatches authAdapter_${app}_${method} — authAdapter_mattermost_teest_… + # Every user-management action on an instance failed with "does not + # implement", and the clone's definitions (bodies already rewritten to + # exec against the INSTANCE's container) collided with the base app's + # under its own name. Which of the two survived came down to the order + # `find` happened to return the files in, so on an unlucky filesystem + # the base app's user tools would have administered the instance. + # + # MUST run before the suffix rule, not after: the suffix rule turns + # appSetupComposeTags_() into appSetupComposeTags_(), which + # then reads as an infix match (__ followed by the id half of the + # slug) and gets the suffix appended a second time — + # appSetupComposeTags_nextcloud_work_work(). In this order the infix + # rule sees `_(` with no trailing underscore and passes it over. + sed -i -E "s/\b([A-Za-z_][A-Za-z0-9_]*)_${type}_([A-Za-z0-9_]+)(\(\))/\1_${slug}_\2\3/g" "$f" + # Function names carrying the app as a SUFFIX. The prefix rule above # only matches _, so appSetupComposeTags_vaultwarden survived # untouched — and docker_config_setup_data.sh dispatches that hook as @@ -374,6 +395,33 @@ _instanceRewriteTools() { # names are touched. Affects 8 apps that ship this hook shape. sed -i -E "s/\b([A-Za-z_][A-Za-z0-9_]*)_${type}(\(\))/\1_${slug}\2/g" "$f" + # …and the call side of the same dispatch. The app is passed as a BARE + # word (`authAdapterCall mattermost listUsers`), which no rename above + # touches: not the prefix rule (no trailing _), not the container rules + # (not a docker verb, not container="…"). The instance's own tools + # therefore asked for the BASE app's adapter by name and operated on + # the base app's container — the failure mode this whole function + # exists to prevent, reached through the one argument nobody rewrote. + # + # authPersistCfg is the same shape and worse consequence: it writes the + # admin credential the tool just set into CFG__ADMIN_*, so an + # instance resetting its own admin password was overwriting the BASE + # app's stored credential with a password that does not open it. + # + # Anchored on the two helper names that take a bare app as $1 — a + # blanket bare- rewrite is not an option, mattermost_auth.sh has + # a comment about the deprecated `mattermost` binary that must survive. + sed -i -E "s/(\b(authAdapterCall|authAdapterCanDo|authPersistCfg)[[:space:]]+)${type}\b/\1${slug}/g" "$f" + + # Tool entry points. dockerAppRunTool derives the function name from the + # slug as app with NO case-insensitive fallback, + # so an instance's tools must be appMattermost_teestListUsers. The clone + # kept appMattermostListUsers — same collision as the adapters, and + # every tool on every instance answered "App '' has no tool ''". + # ${type^} / ${slug^} reproduce dockerAppRunTool's own ucfirst exactly; + # anything cleverer would stop matching the name it has to produce. + sed -i -E "s/\bapp${type^}([A-Za-z0-9_]*)(\(\))/app${slug^}\1\2/g" "$f" + # The uppercase tag namespace, mirroring rule 4 of the compose rewrite. # These hooks pass tag NAMES as strings ("VAULTWARDEN_ADMIN_TOKEN_1_TAG"), # which the lowercase renames above cannot see. The cloned compose has @@ -581,5 +629,37 @@ instanceRemove() { dockerUninstallApp "$slug" "false" "false" fi rm -rf "${install_containers_dir%/}/$slug" + + # Mirror of the create path: the file arrays + function manifest are keyed on + # the app dirs, and one just disappeared. + if declare -F lpRegenArrays >/dev/null 2>&1; then + lpRegenArrays force >/dev/null 2>&1 || true + fi + + # Every WebUI app artifact is derived from the containers/ dirs, so + # dropping one has to be followed by a regen of the three that enumerate + # them. dockerUninstallApp already refreshed them — but that ran while the + # dir still existed, and its patch path can only flip an app to "not + # installed", never delete it. Without this the removed instance stayed in + # apps.json (installed false, INSTANCE_OF intact), so the app-detail + # Instances bar kept rendering a pill for it and the Apps grid kept counting + # it — across reloads, indefinitely. + # + # Called directly rather than via lpRegenWebui: that routes through the WebUI + # updater, which no-ops while another update holds the lock. A skipped run + # here is not self-correcting — the staleness check that would catch it + # compares mtimes of files that no longer exist. + isNotice "Refreshing WebUI app data after instance removal..." + local _gen _gen_rc=0 + for _gen in webuiGenerateLibrePortalConfig webuiGenerateAppsServicesConfig webuiGenerateAppsToolsConfig; do + declare -F "$_gen" >/dev/null 2>&1 || continue + "$_gen" >/dev/null || _gen_rc=1 + done + if [[ "$_gen_rc" -eq 0 ]]; then + isSuccessful "Refreshed WebUI app data." + else + isNotice "WebUI app data refresh reported an error — if '$slug' still shows in the Instances list, run 'libreportal regen webui --force'." + fi + isSuccessful "Removed instance '$slug'." } diff --git a/scripts/source/files/generate_function_manifest.sh b/scripts/source/files/generate_function_manifest.sh old mode 100644 new mode 100755 diff --git a/scripts/webui/webui_regen.sh b/scripts/webui/webui_regen.sh index f507171..4ba3851 100644 --- a/scripts/webui/webui_regen.sh +++ b/scripts/webui/webui_regen.sh @@ -28,6 +28,25 @@ _lpRegenStale() { find "$@" -newer "$artifact" -print -quit 2>/dev/null | grep -q . } +# Does apps.json still list an app whose containers/ dir is gone? +# +# Deletions are invisible to the mtime check above: removing an app dir leaves +# no source file newer than the artifact, so a ghost entry (an uninstalled +# instance, a hand-deleted app folder) would sit in apps.json forever — still +# drawn as a pill in the app-detail Instances bar and still counted on the Apps +# grid. Compare the two sets instead of their timestamps. Returns 0 when a ghost +# is present. No jq → skip the check rather than guess. +_lpRegenOrphanedApp() { + local apps_json="$1" slug + [[ -f "$apps_json" ]] || return 1 + command -v jq >/dev/null 2>&1 || return 1 + while IFS= read -r slug; do + [[ -n "$slug" ]] || continue + [[ -d "${install_containers_dir%/}/$slug" ]] || return 0 + done < <(jq -r '.apps[]? | (.command // "") | split(" ") | last' "$apps_json" 2>/dev/null) + return 1 +} + lpRegenWebui() { local force="$1" local gen="${containers_dir}libreportal/frontend/data/apps/generated" @@ -36,10 +55,11 @@ lpRegenWebui() { if [[ "$force" == "force" ]] \ || _lpRegenStale "$apps_json" "$install_containers_dir" -maxdepth 2 -name '*.config' \ - || _lpRegenStale "$tools_json" "$install_containers_dir" -maxdepth 3 -path '*/tools/*.tools.json'; then - # Sources changed (e.g. an app folder was dropped in) — do the full, - # debounced refresh so the new app appears everywhere. Force past the - # updater's own debounce: we have already established there is real work. + || _lpRegenStale "$tools_json" "$install_containers_dir" -maxdepth 3 -path '*/tools/*.tools.json' \ + || _lpRegenOrphanedApp "$apps_json"; then + # Sources changed (e.g. an app folder was dropped in or removed) — do the + # full, debounced refresh so the app appears/disappears everywhere. Force + # past the updater's own debounce: there is established real work. WEBUI_UPDATER_FORCE=1 webuiLibrePortalUpdate return $? fi @@ -54,15 +74,36 @@ lpRegenArrays() { local newest_array newest_array="$(ls -t "$arrays_dir"/files_*.sh 2>/dev/null | head -1)" + # Staleness covers BOTH roots the manifest indexes. generate_function_manifest + # scans scripts/ AND containers/, so watching only scripts/ meant the one event + # that actually adds functions on a live box — a new containers/ dir, i.e. + # every instance clone — never registered as stale. Nothing but an explicit + # --force would rebuild, so an instance's hooks stayed invisible to lazy mode. if [[ "$force" == "force" ]] || [[ -z "$newest_array" ]] \ - || find "$install_scripts_dir" -name '*.sh' -newer "$newest_array" -print -quit 2>/dev/null | grep -q .; then + || find "$install_scripts_dir" -name '*.sh' -newer "$newest_array" -print -quit 2>/dev/null | grep -q . \ + || find "$install_containers_dir" -maxdepth 3 -name '*.sh' -newer "$newest_array" -print -quit 2>/dev/null | grep -q .; then local rc=0 - [[ -f "$gen_script" ]] && "$gen_script" run || rc=$? + # Both generators run THROUGH bash rather than being executed, so a missing + # exec bit cannot silently disable regeneration. Not hypothetical: + # generate_function_manifest.sh shipped 0644, so every invocation died with + # rc=126 and the `|| true` below swallowed it. The manifest was therefore + # never rebuilt on any live system — it stayed byte-identical to the copy + # laid down at deploy — and lazy mode (the CLI default) could not see a + # single function belonging to an app created after that deploy. + if [[ -f "$gen_script" ]]; then + bash "$gen_script" run || rc=$? + else + rc=1 + fi # Function manifest tracks the same source set — keep them in sync. - # Failures here don't abort: lazy-load is opt-in via LP_LAZY=1, so a - # stale manifest just means lazy mode might miss a recently-added - # function. Eager mode (the default) is unaffected. - [[ -f "$manifest_script" ]] && "$manifest_script" run >/dev/null 2>&1 || true + # A failure still doesn't abort (eager mode is unaffected by a stale + # manifest) but it is no longer silent: lazy mode is what the CLI runs, so + # "the manifest is stale" is the difference between an app's hooks existing + # and not existing, which surfaces as absurd errors elsewhere ("app has no + # upgrade verifier" for an app whose verifier is sitting on disk). + if [[ -f "$manifest_script" ]] && ! bash "$manifest_script" run >/dev/null 2>&1; then + isNotice "Function manifest regeneration failed — lazy mode may not see recently added functions. Retry with: libreportal regen arrays --force" + fi return $rc fi return 0