Repository navigation
fix: unpack executable assets inside SASJS_ROOT, and remove them - #536
Conversation
15191eb to
97a7443
Compare
![sasjs-dev[bot]](https://avatars.githubusercontent.com/u/67858352?s=60&v=4){kind=small}
There was a problem hiding this comment.
REVIEWED ON 2026-11-04 13:58 UTC
Reviewed the diff against yao-pkg/pkg's prelude (pkgCacheDir, deflateSync, patchDlopen) and the executable smoke harness; three findings.
-
api/src/utils/pkgCache.ts:117 - naming the cache folder
pkg-cache-<pid>assumes a pid identifies a process across restarts and instances. Under Docker (README.md:470 documents--stop-timeoutfor exactly this deployment) the server is pid 1 in every container, so apkg-cache-1left by an OOM-killed or SIGKILLed instance looks alive to the sweep at line 119 (pidIsAlivereturns true for its own pid), is reused by themkdirSyncat line 121, and gains anotherpkg/tmp-XXXXXXon every start - a crash-loop accumulates the residue this change exists to remove. Two instances sharing one SASJS_ROOT collide the other way: both name the folderpkg-cache-1, and the first to exit deletes the other's live assets. Make the folder unique (${cacheFolderPrefix}${process.pid}-${randomUUID()}) and sweep on age rather than onprocess.kill(pid, 0), which cannot tell pids apart across namespaces. -
api/src/utils/specs/pkgCache.spec.ts:1 - the packaged path is never exercised: each spec injects
packaged: trueinto an ordinary node process, and .github/scripts/smoke-executable.mjs boots the real binary but asserts nothing about where pkg unpacked. The assumption the change rests on - that setting PKG_NATIVE_CACHE_PATH duringsetProcessVariables(api/src/app.ts:44) still redirects extraction - holds only until pkg's first extraction, because prelude/bootstrap.js pins itstmpFolderon first use (if (!tmpFolder) { ... }) and reads the variable once. Any snapshot file read during module import, before that await resolves, pins~/.cache/pkgfor the life of the process. Assert it in the smoke script, which already sets SASJS_ROOT: atmp/pkg-cache-*folder appears there and~/.cache/pkgdoes not. -
README.md:78 - "removed when the process exits" is stated unconditionally, while the code's own note says pkg's removal never runs for
process.exit()and a killed process leaves the folder to the sweep, and that~/.cache/pkgcan still appear for a native addon. One sentence for each caveat keeps the operator's expectation honest.
3 findings above for review.
97a7443 to
f12c4ee
Compare
|
All three actioned in 1. pid is not an identity across namespaces (pkgCache.ts). Correct, and it invalidated the sweep rather than just the name. The folder is now 2. the packaged path is untested. Added to and it bites - the same binary with 3. README overclaimed. It now says the folder is removed on exit, that one left by a process killed outright is removed by a later start once it is a day old, that Also re-measured on the rebuilt binary: 192 files under |
A packaged executable reads its bundled files from a snapshot, and pkg externalises each one it is asked for into `pkgCacheDir()` - `~/.cache/pkg` unless `PKG_NATIVE_CACHE_PATH` says otherwise - under a random `tmp-XXXXXX` name. It registers that folder's removal on `beforeExit`, which is not emitted for `process.exit()` (yao-pkg/pkg#52), and this server's shutdown path ends with exactly that call. Every start that refreshed the drive therefore left a folder holding the 192 macro files it copied in the service account's home, where nothing looks and nothing cleans (#523). `configurePkgCache` now directs the cache into `<SASJS_ROOT>/tmp/pkg-cache-<pid>-<uuid>` and removes it from an `exit` handler, which runs on `process.exit()` as well as on a drained event loop, and whose synchronous removal completes before the process goes. A `PKG_NATIVE_CACHE_PATH` the operator has set is left alone. Three decisions behind that shape: - `exit`, not `beforeExit`: `exit` runs on `process.exit()`, and the removal is synchronous, which is the route the upstream issue names as working. - The name carries a random id as well as the pid, so two instances never share a folder - including two containers, where this server is pid 1 in each, and where a reused name would also have made a dead instance's folder look alive. - Liveness for the sweep is a timestamp the instance refreshes hourly, not a pid probe: a pid means nothing outside its own namespace, so the sweep removes by age instead. A folder is only swept once nothing has refreshed it for a day. The packaged path itself is now asserted by `.github/scripts/smoke-executable.mjs`, which boots the real binary on three platforms: exactly one `pkg-cache-*` folder under the run's SASJS_ROOT, holding extracted files, and nothing new in `~/.cache/pkg`. That assertion is the only thing that can prove the redirect lands before pkg's first extraction - a source checkout and the npm package run no pkg prelude at all. Scope: pkg resolves the directory it extracts NATIVE ADDONS into when its prelude loads, before any application code runs, so `~/.cache/pkg` can still appear if a native addon is ever added. This server ships none - the assets here are the ones the application reads, and the README says so. Measured on a built linux executable at this commit, desktop mode, fresh SASJS_ROOT: - before: `~/.cache/pkg/tmp-d8roSV` appears during the start, holds 192 files, and is still there after SIGTERM; - after: nothing is created in `~/.cache/pkg`, the 192 files sit in `<SASJS_ROOT>/tmp/pkg-cache-<pid>-<uuid>` during the run, and the folder is gone after SIGTERM; - after a SIGKILL the folder is still there, and once it is a day old the next start removes it while creating its own.
f12c4ee to
ee95f31
Compare
Removed unnecessary line breaks and cleaned up text formatting in the README.
|
🎉 This PR is included in version 3.14.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Closes #523.
What happens today
@yao-pkg/pkgexternalises the snapshot files an executable asks for intopkgCacheDir()-~/.cache/pkgunlessPKG_NATIVE_CACHE_PATHsays otherwise - astmp-XXXXXX, and registers the removal onbeforeExit. This server's shutdown path ends withprocess.exit(), which does not emitbeforeExit, so the folder survives (upstream: yao-pkg/pkg#52, open since 2024-04-09).Measured on a linux executable built at this commit, desktop mode, fresh
SASJS_ROOT:~/.cache/pkg/tmp-d8roSVappears during the start, holds 192 files (the macros copied to the drive), and is still there after SIGTERM.The fix
configurePkgCache(api/src/utils/pkgCache.ts, called fromsetProcessVariablesonceSASJS_ROOTis resolved) directs pkg's cache into<SASJS_ROOT>/tmp/pkg-cache-<pid>-<uuid>and removes it from anexithandler.Four decisions behind that shape:
exit, notbeforeExit.exitruns onprocess.exit()as well as on a drained event loop, and the removal is synchronous, so it completes before the process goes. This is the same route the upstream issue names as working for them.SASJS_ROOT, notos.tmpdir(). The data root is the directory the operator already watches and backs up, so a folder that does survive a kill is visible where someone looks. Pointing it back at/tmpwould recreate Housekeeping of PKG_XXXX files #208's invisible files.SASJS_ROOTwould otherwise sharepkg-cache-1- one removing the other's live assets on exit, or inheriting a dead instance's folder on start.process.kill(pid, 0)cannot tell live from dead across namespaces, so the sweep works by age: a running instance refreshes its folder's timestamp hourly (an unref'd timer), and a start removes apkg-cache-*folder that nothing has refreshed for a day (staleAfterMs). A folder is never swept on the strength of its pid.A
PKG_NATIVE_CACHE_PATHthe operator has set is left untouched - on a host whose data root is small or read-only that is their call. The README documents the variable, its source (pkg's), and the two caveats: a killed process's folder stays until a later start sweeps it, and a native addon's extraction happens in pkg's prelude, before this code runs.Scope of the fix: no native addon ships today, so
~/.cache/pkgstays empty for the assets this issue is about; if one is ever bundled, pkg will use its default for that one and the README says so.Evidence
Built from this commit's tree with
npx pkg . --targets node24-linux-x64, desktop mode (MODE=desktop RUN_TIMES=js), a freshSASJS_ROOTeach run:Without this change, on the same tree,
~/.cache/pkg/tmp-d8roSVis created during the start and is still there after SIGTERM.The packaged path is asserted by the smoke test now, on three platforms:
and it bites - run with the cache forced elsewhere, the same binary fails the job:
Tests and gates
api/src/utils/specs/pkgCache.spec.ts- eight cases: a plain node process is left alone; an executable gets a folder whose name carries the pid and a uuid; two instances get different folders; an operator-setPKG_NATIVE_CACHE_PATHwins; theexithandler removes the folder; the heartbeat moves the folder's own mtime; the sweep removes a folder nothing has refreshed for a day and keeps a recent one; the sweep touches nothing it did not name. Proven to bite: dropping the heartbeat'sutimesSyncfails the heartbeat case.npx jestwith the CI env: 64 suites / 747 tests green.npx tsc --noEmit -p .clean;prettier@3.9.9 --checkclean overapi/src.Docs
README.md- theSASJS_ROOTblock and the standalone-executable section say where the unpacked assets live, that a killed process's folder is swept once a day old, thatPKG_NATIVE_CACHE_PATH(pkg's own variable) moves it, and that native addons are unpacked before this code runs.server.sasjs.io:
SASJS_ROOTindocs/settings.md, in sasjs/server.sasjs.io#32.