fix(build): remove the build container when a build times out (#143) - #188
Conversation
📝 WalkthroughWalkthroughThe build executor now stops persistent Docker containers after build timeouts. ZIP exports now archive the output directory without explicitly filtering symbolic links. ChangesBuild timeout cleanup
ZIP export behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/services/buildExecutor.ts`:
- Line 647: Serialize per-session container teardown by storing the promise
returned from _stopPersistentContainer instead of discarding it, and await that
promise in _ensureContainerRunning before recreating the container. Preserve the
shutdown promise until cleanup succeeds, retaining failed cleanup for retry
rather than clearing the session’s tracking state on rejection.
- Line 1738: Update the archive creation flow around
archive.directory(outputPath, false) to exclude symlink entries before they are
added to the ZIP, preventing links under outputPath from being recreated on
extraction. Preserve archiving of regular files and directories, and add
coverage for a symlink pointing outside outputPath.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ee640e38-760f-4c0d-9fa6-248626e7c022
📒 Files selected for processing (1)
backend/src/services/buildExecutor.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test
🧰 Additional context used
🪛 ast-grep (0.45.0)
backend/src/services/buildExecutor.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { exec, execFile, spawn } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { exec, execFile, spawn } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
| // rejection below must not wait on Docker either way. | ||
| proc.kill('SIGKILL'); | ||
|
|
||
| void this._stopPersistentContainer(sessionId).catch(() => {}); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Serialize container teardown before permitting a rebuild.
Line 647 starts removal after _stopPersistentContainer immediately drops the container mapping, while timeout rejection lets build() clear the in-progress marker. A new build can recreate the same container before the prior docker rm -f settles; the older removal can then delete the replacement or cause a name-conflict failure. Track a per-session shutdown promise, await it from _ensureContainerRunning, and retain/retry failed cleanup rather than silently losing tracking.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { exec, execFile, spawn } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/services/buildExecutor.ts` at line 647, Serialize per-session
container teardown by storing the promise returned from _stopPersistentContainer
instead of discarding it, and await that promise in _ensureContainerRunning
before recreating the container. Preserve the shutdown promise until cleanup
succeeds, retaining failed cleanup for retry rather than clearing the session’s
tracking state on rejection.
| entry?.stats?.isSymbolicLink?.() ? false : entry | ||
| )); | ||
|
|
||
| archive.directory(outputPath, false); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1680,1765p' backend/src/services/buildExecutor.tsRepository: harsharajkumar-273/Proofdesk
Length of output: 2852
🏁 Script executed:
rg -n "archiver|symlink|directory\\(outputPath|exportZip|zip" backend package.jsonRepository: harsharajkumar-273/Proofdesk
Length of output: 10595
🌐 Web query:
archiver directory symlink behavior zip symlink entries follows links documentation
💡 Result:
The ZIP file format does not natively standardize the storage of symbolic links [1][2][3]. Because of this, behavior regarding symlinks varies significantly depending on the archiver implementation, operating system, and specific options used [1][2]. Standard ZIP Behavior: Many basic ZIP implementations, including some libraries and default system utilities, treat symlinks as regular files [1][4]. When they encounter a symlink, they follow the link to its target, read the target's content, and store that content as a standard file within the ZIP archive [4][5]. This effectively dereferences the link, preventing the preservation of the original link structure [5]. Info-ZIP and Platform-Specific Extensions: The popular Info-ZIP utility (often used on Unix-like systems) provides an extension to store symlinks as such using the --symlinks (or -y) flag [6][7][8]. When this option is enabled, the utility stores the symlink path as an entry in the archive rather than the content of the target file [9][6]. This is accomplished by storing the file type attribute (e.g., using bits in the external_attr field) and the link target path within the archive member [9][1]. However, this is a non-standard extension and may not be correctly interpreted by all decompression software [1][2]. Library-Specific Behavior: Archiving libraries frequently provide explicit configuration options to control this behavior: - Node.js Archiver: By default, many configurations may not preserve symlinks, or they may require specific handling [10][4]. Newer updates and configurations (such as followSymlinks options) allow users to explicitly choose whether to dereference (follow) symlinks or preserve them as links [11][5]. - Python's zipfile module: This module does not provide native, high-level support for creating symlink entries in the ZIP format. Creating archives with preserved symlinks using this library generally requires manually setting external attributes (such as the UNIX extra field) to mimic the behavior of Info-ZIP [1][2]. In summary, if you are using an archiver that does not explicitly support storing symlinks, it will typically follow them and archive the target file instead [4][5]. To preserve symlinks, you must use a tool or library configuration that explicitly supports the storage of symlink entries, usually via non-standard ZIP format extensions [6][1][5].
Citations:
- 1: https://stackoverflow.com/questions/35782941/archiving-symlinks-with-python-zipfile
- 2: zipfile: symlinks etc. python/cpython#62795
- 3: Can't create archive with broken symlinks mholt/archiver#392
- 4: symbolic links are not stored when zipping files archiverjs/node-archiver#158
- 5: https://github.com/mholt/archiver/blob/master/archiver.go
- 6: https://man.archlinux.org/man/zip.1
- 7: https://man.archlinux.org/man/zip.1.txt
- 8: https://manpages.debian.org/bullseye/zip/zip.1.en.html
- 9: https://discuss.python.org/t/how-info-zip-represents-symlinks/4104
- 10: archiver.directory doesn't follow symbolic link archiverjs/node-archiver#402
- 11: Add an option to allow following the symlinks archiverjs/node-archiver#810
🌐 Web query:
Archiver 7 directory followSymlinks default symlink zip behavior
💡 Result:
The behavior of symbolic links in archiving depends on the specific software being used, as "Archiver" can refer to multiple distinct tools. Node.js 'archiver' library: For the widely used Node.js 'archiver' package (archiverjs/node-archiver), which is frequently used for creating ZIP archives, symbolic link handling is configurable [1]. By default, the library historically did not follow symbolic links, often ignoring them or storing them in ways that required specific configuration to resolve [2][1][3]. To include the target files of symbolic links rather than the links themselves, you must enable the 'followSymlinks' option (often set via the options object when initializing or calling directory methods) [1]. Version 7 of the library supports this at runtime [4]. 7-Zip: If you are referring to 7-Zip, it is a separate file archiver utility [5][6]. While 7-Zip can archive and restore symbolic links on Unix-like systems, this is not always the default behavior for all operations [7]. To ensure symbolic links are handled correctly (stored as links), you may need to use specific command-line flags, such as -snl (store symlinks) [7]. macOS 'Archiver' application: There is also a macOS utility named 'Archiver' (archiverapp.com) [8]. This is a graphical application for macOS, and its specific handling of symlinks during compression would be governed by its internal GUI-based settings or system-level integration rather than a 'followSymlinks' code configuration [8]. If you are using the Node.js 'archiver' library, ensure you are passing the 'followSymlinks: true' option in your configuration to dereference symlinks and include the linked files in your ZIP archive [1].
Citations:
- 1: https://github.com/archiverjs/node-archiver/pull/810/files/9004933e1e71599b1d9cb2280becd6df520d6536
- 2: archiver.directory doesn't follow symbolic link archiverjs/node-archiver#402
- 3: symbolic links are not stored when zipping files archiverjs/node-archiver#158
- 4: Automattic/studio@05d985f
- 5: https://www.7-zip.org/
- 6: https://en.wikipedia.org/wiki/7-Zip
- 7: https://unix.stackexchange.com/questions/600282/preserve-file-permissions-and-symlinks-in-archive-with-7-zip
- 8: https://archiverapp.com/?locale=en
🏁 Script executed:
rg -n "exportZip|archive\\.directory|followSymlinks|symlink|outputPath" backend/tests backend/src/services/buildExecutor.ts backend/src -g '!**/assets/**'Repository: harsharajkumar-273/Proofdesk
Length of output: 18345
🌐 Web query:
site:github.com archiverjs node-archiver followSymlinks default directory symlink
💡 Result:
In node-archiver, the default behavior when using the directory method is not to follow symbolic links [1][2]. Instead, symbolic links are typically either ignored or archived as literal symlink entries rather than dereferencing them to include the target file contents [3][2][4]. Recent development efforts have introduced a followSymlinks option (e.g., in pull request #810) to allow users to explicitly enable this behavior when creating an Archiver instance [5]: const archive = new ZipArchive({ followSymlinks: true, zlib: { level: 9 }, }); When using the glob method, you can also manually control this behavior by adding the follow: true property to the glob options object [6]. If you use glob with following enabled, it is often necessary to set nodir: true to ensure the library correctly dereferences the symlinked folder contents rather than simply creating an empty symlink entry in the archive [6]. For older versions or scenarios where the built-in options do not suffice, a common workaround involves manually walking the directory structure, using fs.stat to detect symlinks, and appending the target files individually using archive.append [1][3].
Citations:
- 1: archiver.directory doesn't follow symbolic link archiverjs/node-archiver#402
- 2: withCopyFilesToContainer should follow symlinks by default testcontainers/testcontainers-node#713
- 3: symbolic links are not stored when zipping files archiverjs/node-archiver#158
- 4: Zip of macOS .app prevents it from running archiverjs/node-archiver#225
- 5: Add an option to allow following the symlinks archiverjs/node-archiver#810
- 6: npm link packages in node_modules folder are not zipped using glob archiverjs/node-archiver#311
🏁 Script executed:
sed -n '840,930p' backend/src/services/buildExecutor.tsRepository: harsharajkumar-273/Proofdesk
Length of output: 3057
🌐 Web query:
archiverjs node-archiver directory method symlink handling default source code
💡 Result:
In the node-archiver library, the directory method behaves such that symbolic links are generally not followed by default when recursing through directories [1][2]. Instead, the symlinks are often ignored or treated in a way that does not archive the target file's content [2]. Regarding technical handling and configuration: 1. Configuration Options: Recent development has introduced the followSymlinks option (exposed via the Archiver constructor) to allow users to toggle whether symbolic links are followed [3][4]. When enabled, this option instructs the underlying globbing mechanism (readdir-glob) to follow symlinks, thereby archiving the target files and folders instead of the link itself [3]. Example usage: const archive = new ZipArchive({ followSymLinks: true, zlib: { level: 9 }, }); 2. Programmatic Symlinks: The library provides a separate symlink method for creating symlinks within an archive programmatically [5][6]. This method does not interact with the filesystem; it is specifically designed to create symlink entries inside the archive file itself, independent of the source directory structure [5]. 3. Historical Context: Historically, node-archiver has been noted for not natively preserving or following symlinks during recursive directory archiving, which has prompted users to manually walk directories and use the append method to include linked files as regular files [1][2]. If you require specific behavior (such as preserving the link itself or dereferencing it), you should check your version of archiver to ensure it supports the followSymlinks option, or consider implementing a manual walk of the directory structure using fs.stat and archive.append for fine-grained control [1].
Citations:
- 1: archiver.directory doesn't follow symbolic link archiverjs/node-archiver#402
- 2: symbolic links are not stored when zipping files archiverjs/node-archiver#158
- 3: Add an option to allow following the symlinks archiverjs/node-archiver#810
- 4: https://github.com/archiverjs/node-archiver/pull/810/files/9004933e1e71599b1d9cb2280becd6df520d6536
- 5: https://github.com/archiverjs/node-archiver/blob/master/website/docs/archiver_api.md
- 6: https://www.archiverjs.com/docs/archiver/
🌐 Web query:
archiver directory method symlink docs followSymlinks false ignored preserved
💡 Result:
In archiving contexts, the behavior of followSymlinks and the handling of symbolic links depend on the specific library and implementation being used, as different tools treat them as either "ignored," "preserved" (as links), or "dereferenced" (replaced by target content). For node-archiver, the primary methods like archive.directory() have historically had limited native support for following symlinks [1][2]. When using archive.glob() or archive.directory(), symlinks might be ignored or skipped by default [3]. To ensure symlinks are followed—meaning the actual content they point to is archived instead of the link entry itself—developers often need to enable a follow option (often specific to the globbing configuration) [3]. Some implementations suggest manual directory walking and append() calls for full control over whether to preserve or dereference links [1]. For other libraries like mholt/archiver (Go) or dir-archiver, followSymlinks is a configuration option that dictates this behavior [4][5][6]: - When followSymlinks: true: The archiver dereferences the symlink, effectively copying the target file or directory content into the archive at the symlink's location [6]. - When followSymlinks: false: The archiver typically attempts to preserve the symlink entry itself (storing it as a link). If the library does not support storing symlinks in the specific archive format (e.g., standard ZIP files often lack native support for storing arbitrary symlinks), these links may be ignored or cause errors (e.g., "no such file or directory" for broken links) [7][6]. In summary, if followSymlinks is set to false, the expected behavior is that the archiver will attempt to preserve the symlink as an entry rather than following it [6]. If this fails or results in ignored entries, it is often due to the limitations of the destination archive format or the specific library's inability to write symlink metadata [7]. To guarantee specific behavior, developers must verify if their library supports symlink persistence in their chosen format or implement custom logic to manually resolve and append the linked files [1][6].
Citations:
- 1: archiver.directory doesn't follow symbolic link archiverjs/node-archiver#402
- 2: symbolic links are not stored when zipping files archiverjs/node-archiver#158
- 3: npm link packages in node_modules folder are not zipped using glob archiverjs/node-archiver#311
- 4: https://cdn.jsdelivr.net/npm/dir-archiver@3.0.2/docs/reference/options.md
- 5: https://github.com/Ismail-elkorchi/dir-archiver/blob/master/README.md
- 6: https://github.com/mholt/archiver/blob/master/archiver.go
- 7: Can't create archive with broken symlinks mholt/archiver#392
🏁 Script executed:
rg -n "isSymbolicLink|lstat\\(|followSymlinks|symlink" backend/src backend/tests -g '!**/assets/**'Repository: harsharajkumar-273/Proofdesk
Length of output: 2909
🏁 Script executed:
sed -n '1,180p' backend/src/utils/pathContainment.tsRepository: harsharajkumar-273/Proofdesk
Length of output: 5025
Exclude symlink entries from the ZIP. archive.directory() can carry links through into the archive, so a build-controlled symlink under outputPath may be recreated on extraction. Filter symlinks here and add coverage for a link that points outside outputPath.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { exec, execFile, spawn } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/services/buildExecutor.ts` at line 1738, Update the archive
creation flow around archive.directory(outputPath, false) to exclude symlink
entries before they are added to the ZIP, preventing links under outputPath from
being recreated on extraction. Preserve archiving of regular files and
directories, and add coverage for a symlink pointing outside outputPath.
harsharajkumar-273
added
ELUSOC
cbcfe13
into
harsharajkumar-273:main
2 participants
Refs #143
Problem
The build is dispatched with
docker exec, notdocker run:so the child process is the local
docker execclient, and the compile itself runs inside the persistent container. On timeout:SIGKILLkills the client. The build keeps running in the container, now with nothing waiting on it — which for a PreTeXt build can mean apdflatexloop consuming CPU indefinitely. This is exactly the outcome #143 describes, reached by a different route than the one it names.The existing
docker rm -fcalls don't cover it: one runs before starting a fresh container, the other on explicit session teardown. Neither fires when a build times out.Change
The timeout handler now tears the container down as well:
_stopPersistentContaineralready exists and does the right thing — removes it frompersistentContainers, runsdocker rm -fwith a 15s timeout, and swallows its own failures.It's fired without awaiting because a
setTimeoutcallback can't await, and the rejection shouldn't be delayed behind a Docker call. The extra.catch()is belt-and-braces: the method already handles its own errors, but an unhandled rejection escaping a timer would be worse than a silent cleanup failure.sessionIdis already a parameter of_spawnDockerWithLogs, and both enclosing callbacks are arrow functions, sothisbinds correctly — no signature change needed.Verification
npx tsc --noEmitinbackend/clean.I couldn't exercise a real timeout — it needs a build that outruns
PROOFDESK_BUILD_TIMEOUT_MS(4 hours by default) inside a live Docker container. The change reuses the teardown path that session cleanup already exercises.Ordering
Touches
buildExecutor.tsaround line 635. My open #142 branch touches lines 40-104 of the same file; they should merge cleanly, but worth landing one and re-checking the other.Summary by CodeRabbit