fix(build): dlopen OpenTUI native lib from a stable cache path#577
Open
benvinegar wants to merge 4 commits into
Open
fix(build): dlopen OpenTUI native lib from a stable cache path#577benvinegar wants to merge 4 commits into
benvinegar wants to merge 4 commits into
Conversation
Bun single-file executables extract the embedded OpenTUI native library to the OS temp directory under a fresh random name on every launch and never reuse or remove it (oven-sh/bun#30962). Repeated invocations — hunk as a git pager, agents polling `hunk session` — leak one ~4-14 MB copy per run and can fill a tmpfs (#556). The compiled build now substitutes a stable content-addressed path for the platform package's default export: a Bun build plugin intercepts the @opentui/core-<platform> imports and routes them through a shim that writes the embedded bytes once to <user-cache>/hunk/native and reuses that path across runs. Reading embedded bytes never extracts; only dlopen does, and dlopen now targets the cache path. Failures fall back to the embedded path, preserving the previous leaking-but-working behavior. Removable once Bun's extraction dedupe (oven-sh/bun#29587) ships.
Contributor
Greptile SummaryThis PR fixes the Bun single-file executable native library leak (#556) by intercepting OpenTUI platform package imports at build time and routing
Confidence Score: 4/5Safe to merge; all failure modes fall back to the previous leaking-but-working behavior and the binary never fails to start. The materialization logic has a concrete gap where src/core/nativeLibMaterialize.ts — the prune call placement and the tmp-file cleanup on rename failure deserve a second look. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Build as build-bin.ts
participant Plugin as opentuiStableNativeLibPlugin
participant Shim as Generated Shim Module
participant Mat as nativeLibMaterialize.ts
participant Cache as ~/.cache/hunk/native/
Build->>Plugin: "Bun.build({ plugins: [plugin] })"
Plugin->>Plugin: "onResolve @opentui/core-* to SHIM_NAMESPACE"
Plugin->>Plugin: onLoad - generate shim contents
Plugin-->>Build: shim TS injected into bundle
Note over Build: Binary compiled with shim
rect rgb(230, 240, 255)
Note over Shim,Cache: At binary startup (module load)
Shim->>Mat: materializeStableNativeLibPath(embeddedPath, libFileName)
Mat->>Mat: readEmbedded(embeddedPath) - bytes
Mat->>Mat: SHA-256(bytes).slice(0,16) - hash
Mat->>Cache: existsSync(dest)?
alt file missing
Mat->>Cache: mkdirSync(dir)
Mat->>Cache: writeFileSync(tmpPath, bytes)
Mat->>Cache: renameSync(tmpPath, dest)
end
Mat->>Cache: pruneStaleVersions (files older than 1h)
Mat-->>Shim: return dest (stable cache path)
end
Shim-->>Build: "export default = dest"
Note over Build: @opentui/core dlopens dest instead of Bun tmp path
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Build as build-bin.ts
participant Plugin as opentuiStableNativeLibPlugin
participant Shim as Generated Shim Module
participant Mat as nativeLibMaterialize.ts
participant Cache as ~/.cache/hunk/native/
Build->>Plugin: "Bun.build({ plugins: [plugin] })"
Plugin->>Plugin: "onResolve @opentui/core-* to SHIM_NAMESPACE"
Plugin->>Plugin: onLoad - generate shim contents
Plugin-->>Build: shim TS injected into bundle
Note over Build: Binary compiled with shim
rect rgb(230, 240, 255)
Note over Shim,Cache: At binary startup (module load)
Shim->>Mat: materializeStableNativeLibPath(embeddedPath, libFileName)
Mat->>Mat: readEmbedded(embeddedPath) - bytes
Mat->>Mat: SHA-256(bytes).slice(0,16) - hash
Mat->>Cache: existsSync(dest)?
alt file missing
Mat->>Cache: mkdirSync(dir)
Mat->>Cache: writeFileSync(tmpPath, bytes)
Mat->>Cache: renameSync(tmpPath, dest)
end
Mat->>Cache: pruneStaleVersions (files older than 1h)
Mat-->>Shim: return dest (stable cache path)
end
Shim-->>Build: "export default = dest"
Note over Build: @opentui/core dlopens dest instead of Bun tmp path
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/core/nativeLibMaterialize.ts:136-140
**Prune failure silently discards a successfully materialized cache path**
`pruneStaleVersions` sits inside the same `try` block and is the last statement before `return dest`. If `readdirSync(dir)` inside it throws — the cache dir could be removed by a concurrent process or a system sweep between the `mkdirSync`/`renameSync` and the prune — the outer `catch` discards the already-correct `dest` and returns `embeddedPath`, causing the file leak we're trying to prevent. The pruning is explicitly best-effort; wrapping it in its own `try/catch` or moving the `return dest` before the prune call ensures the stable path is returned even when cleanup fails.
### Issue 2 of 3
src/core/nativeLibMaterialize.ts:126
**Existence check doesn't cover hash-but-same-size corruption**
The reuse guard checks `statSync(dest).size !== bytes.byteLength`. Because the filename already encodes the SHA-256 prefix, an intentional collision is negligible — but a truncated-then-padded or partially-overwritten cache file with the right byte count passes the check and is returned as a valid path. Additionally, between `existsSync` and `statSync` the file could be concurrently removed, throwing from inside the `try` and falling back unnecessarily. Dropping the size check and trusting the content-addressed name alone would be more robust.
```suggestion
if (!existsSync(dest)) {
```
### Issue 3 of 3
src/core/nativeLibMaterialize.ts:132-133
**Orphaned `.tmp` files on `renameSync` failure (Windows in particular)**
If `writeFileSync(tmpPath, …)` succeeds but `renameSync(tmpPath, dest)` throws (on Windows this happens when another concurrent process has already placed `dest`), the outer `catch` fires and the `.tmp` file is left in the cache directory forever — `pruneStaleVersions` skips it because it starts with `.`. Consider adding an explicit `unlinkSync(tmpPath)` in a `catch` around the rename.
```suggestion
writeFileSync(tmpPath, bytes, { mode: 0o755 });
try {
renameSync(tmpPath, dest);
} catch (renameErr) {
try { unlinkSync(tmpPath); } catch { /* ignore */ }
throw renameErr;
}
```
Reviews (1): Last reviewed commit: "fix(build): dlopen OpenTUI native lib fr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.mcszy.xyz/in/867647?s=60&v=4){kind=small}
Comment on lines
+136
to
+140
| pruneStaleVersions(dir, base, ext, destName, now()); | ||
| return dest; | ||
| } catch { | ||
| // Leaking via Bun's extraction is strictly better than failing to start. | ||
| return embeddedPath; |
Contributor
There was a problem hiding this comment.
pruneStaleVersions sits inside the same try block and is the last statement before return dest. If readdirSync(dir) inside it throws — the cache dir could be removed by a concurrent process or a system sweep between the mkdirSync/renameSync and the prune — the outer catch discards the already-correct dest and returns embeddedPath, causing the file leak we're trying to prevent. The pruning is explicitly best-effort; wrapping it in its own try/catch or moving the return dest before the prune call ensures the stable path is returned even when cleanup fails.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/nativeLibMaterialize.ts
Line: 136-140
Comment:
**Prune failure silently discards a successfully materialized cache path**
`pruneStaleVersions` sits inside the same `try` block and is the last statement before `return dest`. If `readdirSync(dir)` inside it throws — the cache dir could be removed by a concurrent process or a system sweep between the `mkdirSync`/`renameSync` and the prune — the outer `catch` discards the already-correct `dest` and returns `embeddedPath`, causing the file leak we're trying to prevent. The pruning is explicitly best-effort; wrapping it in its own `try/catch` or moving the `return dest` before the prune call ensures the stable path is returned even when cleanup fails.
How can I resolve this? If you propose a fix, please make it concise.| const destName = `${base}-${hash}${ext}`; | ||
| const dest = join(dir, destName); | ||
|
|
||
| if (!existsSync(dest) || statSync(dest).size !== bytes.byteLength) { |
Contributor
There was a problem hiding this comment.
The reuse guard checks statSync(dest).size !== bytes.byteLength. Because the filename already encodes the SHA-256 prefix, an intentional collision is negligible — but a truncated-then-padded or partially-overwritten cache file with the right byte count passes the check and is returned as a valid path. Additionally, between existsSync and statSync the file could be concurrently removed, throwing from inside the try and falling back unnecessarily. Dropping the size check and trusting the content-addressed name alone would be more robust.
Suggested change
| if (!existsSync(dest) || statSync(dest).size !== bytes.byteLength) { | |
| if (!existsSync(dest)) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/nativeLibMaterialize.ts
Line: 126
Comment:
**Existence check doesn't cover hash-but-same-size corruption**
The reuse guard checks `statSync(dest).size !== bytes.byteLength`. Because the filename already encodes the SHA-256 prefix, an intentional collision is negligible — but a truncated-then-padded or partially-overwritten cache file with the right byte count passes the check and is returned as a valid path. Additionally, between `existsSync` and `statSync` the file could be concurrently removed, throwing from inside the `try` and falling back unnecessarily. Dropping the size check and trusting the content-addressed name alone would be more robust.
```suggestion
if (!existsSync(dest)) {
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+132
to
+133
| writeFileSync(tmpPath, bytes, { mode: 0o755 }); | ||
| renameSync(tmpPath, dest); |
Contributor
There was a problem hiding this comment.
.tmp files on renameSync failure (Windows in particular)
If writeFileSync(tmpPath, …) succeeds but renameSync(tmpPath, dest) throws (on Windows this happens when another concurrent process has already placed dest), the outer catch fires and the .tmp file is left in the cache directory forever — pruneStaleVersions skips it because it starts with .. Consider adding an explicit unlinkSync(tmpPath) in a catch around the rename.
Suggested change
| writeFileSync(tmpPath, bytes, { mode: 0o755 }); | |
| renameSync(tmpPath, dest); | |
| writeFileSync(tmpPath, bytes, { mode: 0o755 }); | |
| try { | |
| renameSync(tmpPath, dest); | |
| } catch (renameErr) { | |
| try { unlinkSync(tmpPath); } catch { /* ignore */ } | |
| throw renameErr; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/nativeLibMaterialize.ts
Line: 132-133
Comment:
**Orphaned `.tmp` files on `renameSync` failure (Windows in particular)**
If `writeFileSync(tmpPath, …)` succeeds but `renameSync(tmpPath, dest)` throws (on Windows this happens when another concurrent process has already placed `dest`), the outer `catch` fires and the `.tmp` file is left in the cache directory forever — `pruneStaleVersions` skips it because it starts with `.`. Consider adding an explicit `unlinkSync(tmpPath)` in a `catch` around the rename.
```suggestion
writeFileSync(tmpPath, bytes, { mode: 0o755 });
try {
renameSync(tmpPath, dest);
} catch (renameErr) {
try { unlinkSync(tmpPath); } catch { /* ignore */ }
throw renameErr;
}
```
How can I resolve this? If you propose a fix, please make it concise.Cache-root expectations must be built with path.join since the implementation joins segments (CI runs them on Windows), and the exec bit assertion is POSIX-only.
Two hardening fixes against silent regressions of the tmp-leak fix: - The plugin now requires exactly one native library per OpenTUI platform package instead of picking the first match, so upstream restructuring fails the build instead of dlopening the wrong file. - CI asserts the compiled binary leaves TMPDIR empty across runs, so if the platform-package import ever stops matching the plugin filter the leak resurfaces as a red job rather than silently returning.
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
2 participants
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.
What
Compiled hunk binaries leak one copy of the embedded OpenTUI native library into the OS temp dir on every launch (Bun bug oven-sh/bun#30962; #556 hit 190 GB in three weeks). This stops the leak at the source: the library is written once to a stable cache path and reused across runs.
How
@opentui/core-<platform>default-exports a path string that@opentui/coredlopens. In compiled binaries that path comes from Bun's per-launch tmp extraction — but reading the embedded bytes never extracts; onlydlopendoes. So:scripts/opentuiStableNativeLibPlugin.ts).src/core/nativeLibMaterialize.tswrites the bytes once to<user-cache>/hunk/native/<name>-<hash>.<ext>(atomic rename, reused across runs, old versions pruned).Net effect: one file per OpenTUI version instead of one per launch. Removable once Bun's dedupe fix (oven-sh/bun#29587) ships.
vs. #568
Complementary: this stops future leaks; #568's sweep cleans up past ones. If both land, the sweep becomes a deletable migration cleaner.
Verification
typecheck/lint/format:check/test:tty-smokepass. Two PTY failures reproduce on clean HEAD — pre-existing, unrelated.--help+ pager runs (baseline: one per run), single reused~/.cache/hunk/native/libopentui-<hash>.so.Refs #556.