fix(package): Only mount stream_output_dir when stream storage type is FS.#1129
Merged
Conversation
Contributor
WalkthroughThe update modifies the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
junhaoliao
requested changes
Aug 5, 2025
junhaoliao
left a comment
Member
There was a problem hiding this comment.
the current changes in start_clp.py lgtm
as @haiqi96 brought up offline, a warning message about the stream file folder being non-existent is printed when the stream files should be served on s3. let's wrap
clp/components/webui/server/src/routes/static.ts
Lines 20 to 28 in ac6f359
settings.
as for what to check from the settings, i believe checking for null of StreamFilesDir might be good in terms of semantics, which will require us to supply it conditionally in start_clp.py . what do you think?
storage_directory when stream storage type is S3.
storage_directory when stream storage type is S3.stream_output_dir when stream storage type is S3.
stream_output_dir when stream storage type is S3.stream_output_dir when stream storage type is FS.
junhaoliao
previously approved these changes
Aug 6, 2025
junhaoliao
left a comment
Member
There was a problem hiding this comment.
lgtm. adding @davemarco & @hoophalab for awareness
Co-authored-by: Junhao Liao <junhao@junhao.ca>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/server/src/routes/static.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/server/src/routes/static.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: haiqi96
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-07-29T14:04:13.769Z
Learning: User haiqi96 requested creating a GitHub issue to document a bug fix from PR #1136, which addressed MySQL compatibility issues with invalid SQL CAST operations in the WebUI component.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: gibber9809
PR: y-scope/clp#504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.
Learnt from: haiqi96
PR: y-scope/clp#673
File: components/log-viewer-webui/server/src/routes/query.js:9-10
Timestamp: 2025-01-17T22:33:23.547Z
Learning: When initializing S3Manager in log-viewer-webui, both StreamFilesS3Region and StreamFilesS3PathPrefix settings must be non-null for S3 functionality to work correctly.
Learnt from: davemarco
PR: y-scope/clp#1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
📚 Learning: in components/log-viewer-webui/server/src/routes/static.ts, when decoratereply is set to true in fas...
Learnt from: davemarco
PR: y-scope/clp#1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
Applied to files:
components/webui/server/src/routes/static.ts
Comment on lines
+27
to
+35
| if (null !== streamFilesDir) { | ||
| if (false === path.isAbsolute(streamFilesDir)) { | ||
| streamFilesDir = path.resolve(rootDirname, streamFilesDir); | ||
| } | ||
| await fastify.register(fastifyStatic, { | ||
| prefix: "/streams", | ||
| root: streamFilesDir, | ||
| decorateReply: false, | ||
| }); |
Contributor
There was a problem hiding this comment.
Harden the null-check: undefined will still slip through and crash
path.isAbsolute(undefined) throws a TypeError.
If StreamFilesDir is omitted from the JSON (→ undefined) the current condition passes (undefined !== null), and the server will crash on the very next line.
-// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
-if (null !== streamFilesDir) {
+// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
+if ("string" === typeof streamFilesDir && 0 < streamFilesDir.length) {This guards against both null and undefined and ensures we only proceed when we have a non-empty string path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (null !== streamFilesDir) { | |
| if (false === path.isAbsolute(streamFilesDir)) { | |
| streamFilesDir = path.resolve(rootDirname, streamFilesDir); | |
| } | |
| await fastify.register(fastifyStatic, { | |
| prefix: "/streams", | |
| root: streamFilesDir, | |
| decorateReply: false, | |
| }); | |
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | |
| if ("string" === typeof streamFilesDir && 0 < streamFilesDir.length) { | |
| if (false === path.isAbsolute(streamFilesDir)) { | |
| streamFilesDir = path.resolve(rootDirname, streamFilesDir); | |
| } | |
| await fastify.register(fastifyStatic, { | |
| prefix: "/streams", | |
| root: streamFilesDir, | |
| decorateReply: false, | |
| }); |
🤖 Prompt for AI Agents
In components/webui/server/src/routes/static.ts around lines 27 to 35, the
null-check for streamFilesDir only excludes null but allows undefined, which
causes path.isAbsolute to throw a TypeError. Update the condition to check that
streamFilesDir is a non-empty string by verifying it is truthy and of type
string before calling path.isAbsolute and proceeding with registration.
junhaoliao
approved these changes
Aug 6, 2025
stream_output_dir when stream storage type is FS.stream_output_dir when stream storage type is FS.
junhaoliao
added a commit
to junhaoliao/clp
that referenced
this pull request
May 17, 2026
… is `FS`. (y-scope#1129) Co-authored-by: Junhao Liao <junhao@junhao.ca>
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.
Description
This PR let
start_clp.pyonly mountstream_output_dirwhen storage type is filesystem. This prevent the webui from mounting the stream_storage_dir when storing stream on s3.A potential error could be caused by the existing code. If webui runs on another a different node from extraction worker, so the staged-stream dir wouldn't be created on webui's node. When testing locally, docker seems to create the mount directory on host if it doesn't exist. However, on certain machine, for example ec2's arm instance. docker doesn't allow binding to a non-existing directory.
This PR also updates the webui, so it only mount the stream output if the storage type is FS.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Summary by CodeRabbit