Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntegrates Sentry scopes and structured logging across Riven, adds ECS-formatted logger pipeline and new logShowStackTraces setting, updates message-queue processors/workers to accept and propagate Sentry scopes, wraps VFS ops in a VFS Sentry scope, and adds a local Elasticsearch/Kibana/Filebeat development stack. Changes
Sequence Diagram(s)sequenceDiagram
participant Producer as Job Producer
participant Worker as BullMQ Worker
participant Proc as Processor
participant Sentry as Sentry
participant DB as Database/ORM
Producer->>Worker: enqueue job
Worker->>Proc: invoke processor({ job })
Proc->>Sentry: Sentry.withScope (set tags: flow, queue, job id)
Proc->>Proc: processor logic uses provided scope (e.g., scope.setTag)
Proc->>DB: DB logger invoked (wrapped with Sentry.withScope, tag riven.log.source: "database")
alt success
Proc->>Worker: emit success event
else error
Proc->>Sentry: Sentry.captureException(err)
Proc->>Worker: emit failure event (structured { err })
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/riven/lib/index.ts`:
- Around line 17-19: The current uncaughtException handler
(process.on("uncaughtException", ...)) only logs via logger.error and lets the
process continue in an unsafe state; update the handler to either (A) log the
error with logger.error("Uncaught exception", { err: error }) and then terminate
the process with process.exit(1), or (B) replace the listener with
process.on("uncaughtExceptionMonitor", ...) to keep Node's default crash
behavior while still logging; modify the handler in apps/riven/lib/index.ts
accordingly (target the process.on("uncaughtException", ...) listener and the
logger.error call).
- Around line 40-46: The shutdown path currently calls process.exit(1) when
value === "Errored" and process.exit(0) after logger.info("Riven has shut
down"), which can terminate before async sinks (logger/Sentry) finish; update
both shutdown branches in apps/riven/lib/index.ts to await
Sentry.flush(timeoutMs) or Sentry.close(timeoutMs) (or set process.exitCode and
return to let the event loop drain) before exiting so that the final logger.info
and any pending Sentry events are transmitted; target the value === "Errored"
branch and the code immediately after logger.info("Riven has shut down") when
making this change.
In
`@apps/riven/lib/message-queue/flows/download-item/steps/find-valid-torrent/find-valid-torrent.processor.ts`:
- Around line 68-71: The scope tag for "riven.downloader-provider" is only set
when provider is truthy, causing the previous provider value to leak into
iterations where provider is null; update the loop in
find-valid-torrent.processor.ts to always set the tag for each iteration (use
scope.setTag("riven.downloader-provider", provider ?? "null") or
scope.setTag("riven.downloader-provider", String(provider)) so the tag
explicitly reflects the current provider state even when null) — modify the code
around providerList / provider and the existing scope.setTag call to remove the
truthy conditional and set the tag unconditionally.
In `@apps/riven/lib/riven-settings.schema.ts`:
- Around line 49-52: The default for the schema field logShowStackTraces
currently uses .default(true), which risks leaking sensitive stack traces in
production; change the default to false by replacing .default(true) with
.default(false) on the logShowStackTraces Zod entry (refer to the
logShowStackTraces symbol in riven-settings.schema.ts), and run/update any tests
or docs that assert the previous default to reflect the new secure-by-default
behavior.
In `@apps/riven/lib/state-machines/bootstrap/index.ts`:
- Around line 375-381: The VFS onError handler currently only raises the error
and drops detailed logging; update the onError block that targets
"#Bootstrap.Errored" to also log the specific VFS error before raising it—add an
actions entry that calls the same structured logger used in the DB and GraphQL
handlers (e.g., the logging action used around lines handling database/GraphQL
errors) to record a descriptive message and error details, then keep the
existing raiseError params ({ event }) => event.error as Error so the machine
still transitions to Errored with the original error.
In `@apps/riven/lib/utilities/logger/formatters/console.format.ts`:
- Around line 48-52: The timestamp styling uses chalk.dim.black which is
unreadable on dark terminals; update the formatter that builds
formattedTimestamp/return string (the expression using
chalk.dim.black(formattedTimestamp)) to use a subdued but visible style such as
chalk.dim(formattedTimestamp) or chalk.gray(formattedTimestamp) instead, keeping
the rest of the return string intact (the parts referencing
meta["riven.log.source"], level, and maybeColouredMessage).
In `@apps/riven/lib/utilities/logger/types/logform.ts`:
- Around line 3-8: The CustomLogMeta interface currently requires
"riven.error.validation-message", forcing every TransformableInfo to include it;
change the property to be optional (e.g., "riven.error.validation-message"?:
string) on CustomLogMeta so TransformableInfo does not mandate a validation
message for all logs and only validation-related logs supply it.
In `@apps/riven/lib/vfs/operations/open.ts`:
- Around line 191-195: The catch block in open.ts currently logs the error and
always calls process.nextTick(callback, Fuse.EIO), losing specific FuseError
codes; update the handler to detect FuseError using isFuseError (import {
FuseError, isFuseError } from "../errors/fuse-error.ts") and, if it is a
FuseError, pass its specific errno to process.nextTick(callback, fuseErr.errno)
(otherwise keep Fuse.EIO), while still logging the error via logger.error;
locate the catch around the VFS open path that currently uses logger.error("VFS
open error", { err: error }) and replace the unconditional Fuse.EIO return with
the conditional isFuseError check.
In `@apps/riven/lib/vfs/operations/readdir.ts`:
- Around line 47-49: The fallback for unexpected errors in the VFS readdir
implementation is using Fuse.ENOENT but should use Fuse.EIO to match other ops;
update the error-handling path in readdir (the spot that currently calls
process.nextTick(callback, Fuse.ENOENT) right after logger.error("Unexpected VFS
readdir error", { err: error })) to call process.nextTick(callback, Fuse.EIO)
instead so generic internal failures return the standard I/O error like read.ts,
release.ts, getattr.ts and open.ts.
In `@elastic-local/.env`:
- Line 21: Remove the stray blank line in the elastic-local/.env file (the empty
line around line 21) so there are no extra empty lines or trailing blank lines;
edit the .env to collapse that single empty line and ensure dotenv-linter
passes.
- Around line 1-23: The committed .env contains secrets like ES_LOCAL_PASSWORD,
ES_LOCAL_API_KEY and KIBANA_ENCRYPTION_KEY; either stop tracking it and purge
history or replace it with a template and docs: add the filename pattern to
.gitignore, run git rm --cached on the tracked file and commit, and (if needed)
remove past commits using git filter-repo/BFG; alternatively create
elastic-local/.env.example with placeholder values for START_LOCAL_VERSION,
ES_LOCAL_PORT, ES_LOCAL_URL, ES_LOCAL_JAVA_OPTS, FILEBEAT_LOCAL_CONTAINER_NAME,
etc., add a short README entry explaining how to create the real .env from the
example, and ensure the real .env is removed from the repo and not committed
going forward.
In `@elastic-local/.gitignore`:
- Line 1: Remove the negation rule "!.env" from elastic-local/.gitignore (do not
force-track .env); instead add an elastic-local/.env.example file containing
placeholder keys for all required local config vars so developers can copy it to
.env locally; ensure .env remains ignored and update any README/dev setup notes
to reference copying .env.example to .env.
In `@elastic-local/config/filebeat.yaml`:
- Around line 11-14: The ndjson parser options are currently siblings of the
`ndjson` key instead of nested under it, so Filebeat never applies them; update
the YAML so `overwrite_keys`, `add_error_key`, and `expand_keys` are indented
one level deeper under the `ndjson` mapping (i.e., make them children of the
`ndjson` key) in the `filebeat.yaml` configuration so the `ndjson` parser
configuration is recognized.
In `@elastic-local/docker-compose.yml`:
- Line 17: Replace the hard-coded license value so the persisted
ES_LOCAL_LICENSE is honored: change the service environment entry setting
xpack.license.self_generated.type to use Docker Compose interpolation, e.g.
xpack.license.self_generated.type=${ES_LOCAL_LICENSE:-trial}, so the value
written by start.sh (ES_LOCAL_LICENSE=basic) in .env is consumed on startup.
In `@packages/plugin-stremthru/lib/datasource/stremthru.datasource.ts`:
- Around line 245-249: The catch block in the Stremthru datasource's scrape flow
logs errors without item context; update the this.logger.error call inside the
catch in scrape (or the method handling the per-item scrape) to include
item.fullTitle and item.imdbId (falling back to undefined/null if missing) in
the structured log payload so the error reads like this.logger.error("Stremthru
scrape error", { err: error, fullTitle: item?.fullTitle, imdbId: item?.imdbId
});—use the existing this.logger.error symbol and the local item variable to add
these fields for better debugging and correlation.
In `@packages/plugin-torrentio/lib/datasource/torrentio.datasource.ts`:
- Around line 105-109: In the catch block of the Torrentio datasource scrape
flow (where this.logger.error is called), include the item context in the log
message similar to CometAPI.scrape(): log the item's identifying fields (e.g.,
item.fullTitle and item.imdbId) alongside the error and keep the structured
payload (e.g., pass err: error and item or imdbId). Update the this.logger.error
call (in the scrape method / catch) to a descriptive message like "Failed to
scrape ${item.fullTitle} (IMDB: ${item.imdbId})" and include the error and item
data in the metadata before returning {}.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 53d1d8ad-14aa-4d9a-aa2a-767a17ebe9d3
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (58)
apps/riven/docs/settings.mdapps/riven/lib/database/config.tsapps/riven/lib/graphql/build-context.tsapps/riven/lib/index.tsapps/riven/lib/message-queue/flows/download-item/download-item.processor.spec.tsapps/riven/lib/message-queue/flows/download-item/download-item.processor.tsapps/riven/lib/message-queue/flows/download-item/steps/find-valid-torrent/find-valid-torrent.processor.tsapps/riven/lib/message-queue/flows/download-item/steps/rank-streams/rank-streams.processor.spec.tsapps/riven/lib/message-queue/flows/download-item/steps/rank-streams/rank-streams.processor.tsapps/riven/lib/message-queue/flows/scrape-item/scrape-item.processor.spec.tsapps/riven/lib/message-queue/flows/scrape-item/steps/parse-scrape-results/parse-scrape-results.processor.spec.tsapps/riven/lib/message-queue/flows/scrape-item/steps/parse-scrape-results/parse-scrape-results.processor.tsapps/riven/lib/message-queue/utilities/create-flow-schema.tsapps/riven/lib/message-queue/utilities/create-flow-worker.tsapps/riven/lib/message-queue/utilities/create-plugin-worker.tsapps/riven/lib/riven-settings.schema.tsapps/riven/lib/state-machines/bootstrap/actors/initialise-vfs.actor.tsapps/riven/lib/state-machines/bootstrap/actors/start-gql-server.actor.tsapps/riven/lib/state-machines/bootstrap/index.tsapps/riven/lib/state-machines/main-runner/actors/retry-library.actor.tsapps/riven/lib/state-machines/main-runner/index.tsapps/riven/lib/state-machines/plugin-registrar/actors/collect-plugins-for-registration.actor.tsapps/riven/lib/state-machines/plugin-registrar/actors/register-plugin-hook-workers.actor.tsapps/riven/lib/state-machines/plugin-registrar/index.tsapps/riven/lib/state-machines/program/index.tsapps/riven/lib/state-machines/utilities/with-log-action.tsapps/riven/lib/utilities/logger/formatters/console.format.tsapps/riven/lib/utilities/logger/formatters/ecs-file.format.tsapps/riven/lib/utilities/logger/formatters/file.format.tsapps/riven/lib/utilities/logger/formatters/sentry-meta.format.tsapps/riven/lib/utilities/logger/formatters/validation-error-meta.format.tsapps/riven/lib/utilities/logger/logger.tsapps/riven/lib/utilities/logger/schemas/error-splat.schema.tsapps/riven/lib/utilities/logger/types/logform.tsapps/riven/lib/utilities/redis-cache.tsapps/riven/lib/vfs/operations/getattr.tsapps/riven/lib/vfs/operations/open.tsapps/riven/lib/vfs/operations/read.tsapps/riven/lib/vfs/operations/readdir.tsapps/riven/lib/vfs/operations/release.tsapps/riven/lib/vfs/utilities/with-vfs-scope.tsapps/riven/package.jsonelastic-local/.envelastic-local/.gitignoreelastic-local/config/filebeat.yamlelastic-local/config/telemetry.ymlelastic-local/docker-compose.ymlelastic-local/start.shelastic-local/stop.shpackages/plugin-comet/lib/datasource/__tests__/validate.spec.tspackages/plugin-comet/lib/datasource/comet.datasource.tspackages/plugin-notifications/lib/index.tspackages/plugin-seerr/lib/datasource/seerr.datasource.tspackages/plugin-stremthru/lib/datasource/stremthru.datasource.tspackages/plugin-torrentio/lib/datasource/torrentio.datasource.tspackages/util-plugin-sdk/lib/datasource/index.tspackages/util-plugin-sdk/lib/helpers/register-mq-listeners.tspackages/util-rank-torrent-name/lib/parser/parse.ts
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
apps/riven/lib/message-queue/flows/download-item/steps/find-valid-torrent/find-valid-torrent.processor.ts (1)
51-69:⚠️ Potential issue | 🟡 MinorReset
riven.downloader-providerbefore provider discovery.This fixes the
provider === nulliteration, but Line 55 can throw and Line 58 cancontinuebefore Line 69 runs. On those paths, the scope keeps the previous provider value, so the "no providers configured" log and any provider-list lookup failure are attributed to the wrong provider.Suggested fix
for (const plugin of availableDownloaders) { scope.setTag("riven.downloader-plugin", plugin.pluginName); + scope.setTag("riven.downloader-provider", "none"); try { const providers = plugin.hasProviderListHook ? await getPluginProviderList(plugin.pluginName, jobParentOptions) : [];#!/bin/bash set -euo pipefail echo "== provider-tag assignment order ==" nl -ba apps/riven/lib/message-queue/flows/download-item/steps/find-valid-torrent/find-valid-torrent.processor.ts | sed -n '50,75p' echo echo "== worker scope / exception capture ==" nl -ba apps/riven/lib/message-queue/utilities/create-flow-worker.ts | rg -n -C2 'withScope|captureException|scope'Expected: the first snippet shows
getPluginProviderList()and the empty-providercontinueboth happen before the provider tag is written inside the provider loop; the second should show failures are captured on the same per-job scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/riven/lib/message-queue/flows/download-item/steps/find-valid-torrent/find-valid-torrent.processor.ts` around lines 51 - 69, The scope's "riven.downloader-provider" tag can retain a previous provider when getPluginProviderList throws or when the early continue runs before the provider loop sets the tag; to fix, clear/reset the provider tag immediately after tagging the plugin (i.e., after scope.setTag("riven.downloader-plugin", plugin.pluginName)) — e.g., call scope.setTag("riven.downloader-provider", null or "none") before calling getPluginProviderList(plugin.pluginName, jobParentOptions) and then keep the existing scope.setTag("riven.downloader-provider", provider) inside the for (const provider of providerList) loop so each provider iteration overrides the cleared value; reference symbols: scope.setTag, plugin.pluginName, getPluginProviderList, providerList, and the for (const provider...) loop in find-valid-torrent.processor.ts.apps/riven/lib/index.ts (1)
17-20:⚠️ Potential issue | 🟠 MajorDrain Sentry before the fatal exits.
Sentry.close(timeout)/flush(timeout)are the shutdown APIs for draining pending events, andprocess.exit()terminates synchronously. The remaining fatalprocess.exit(1)paths still bypass that drain, so the last crash event can be lost. (docs.sentry.io)Suggested change
+ const closeAndExit = async (code: number) => { + try { + await Sentry.close(2000); + } finally { + process.exit(code); + } + }; + process.on("uncaughtException", (error) => { logger.error("Uncaught exception", { err: error }); - - process.exit(1); + void closeAndExit(1); }); @@ if (value === "Errored") { - process.exit(1); + await closeAndExit(1); } @@ - await Sentry.close(); - - process.exit(0); + await closeAndExit(0);Also applies to: 46-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/riven/lib/index.ts` around lines 17 - 20, The uncaughtException and other fatal exit handlers (the process.on("uncaughtException", ...) handler and the similar process.on("unhandledRejection", ...) block) currently call process.exit(1) immediately and can drop Sentry events; update those handlers to call Sentry.close(timeout) or Sentry.flush(timeout) and await the promise before calling process.exit(1), passing a reasonable timeout (e.g., 2000 ms), and add a fallback that forces exit if the drain promise rejects or times out to avoid hanging; ensure you import/resolve the Sentry instance used by these handlers and call the drain API in both uncaughtException and unhandledRejection paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/riven/lib/index.ts`:
- Around line 23-25: The unhandledRejection handler currently only logs the
error and doesn't exit like the uncaughtException handler; update the
process.on("unhandledRejection", (error) => { ... }) handler in
apps/riven/lib/index.ts to perform a fatal shutdown after logging (e.g., call
process.exit(1) immediately after logger.error("Uncaught rejection", { err:
error }) or rethrow the error) so behavior matches the uncaughtException handler
and the process does not continue after a fatal promise rejection.
- Around line 17-25: The logger calls in the process event handlers currently
pass the Error as a second parameter (logger.error("Uncaught exception", { err:
error })) which your formatters ignore; change both handlers to inline the error
details into the log message itself (use the handlers registered via process.on
for "uncaughtException" and "unhandledRejection") by including error.message
and/or error.stack (or String(reason) for rejections) in the first argument so
the actual error text appears in logs (e.g., build a single descriptive message
like "Uncaught exception: <message/stack>" and "Unhandled rejection:
<reason/message/stack>" when calling logger.error).
In `@apps/riven/lib/vfs/operations/open.ts`:
- Around line 191-200: The logger calls in the open error handler are passing
the error object as meta (logger.error("VFS open FuseError", { err: error }) and
logger.error("VFS open error", { err: error })), which causes the error details
to be discarded by the formatter; change both calls to embed the error details
in the message string (e.g., include error.message and any identifying
properties such as error.errorCode or stack) when calling logger.error so the
information is preserved; keep the isFuseError branch and the
process.nextTick(callback, error.errorCode) behavior unchanged, only modify the
logger.error invocations in the catch block to compose the message with the
error details.
In `@apps/riven/lib/vfs/operations/readdir.ts`:
- Around line 38-47: In readdir's catch block (in the function handling VFS
readdir), change the two logger.error invocations to embed the error details
directly into the message string (use a template literal or string
interpolation) instead of passing the error as metadata; specifically update the
logger.error("VFS readdir FuseError", { err: error }) and
logger.error("Unexpected VFS readdir error", { err: error }) calls so the
formatted message includes error (e.g. `VFS readdir FuseError: ${error}`) to
match the project's Winston formatting convention while leaving the existing
process.nextTick(callback, error.errorCode) behavior intact.
---
Duplicate comments:
In `@apps/riven/lib/index.ts`:
- Around line 17-20: The uncaughtException and other fatal exit handlers (the
process.on("uncaughtException", ...) handler and the similar
process.on("unhandledRejection", ...) block) currently call process.exit(1)
immediately and can drop Sentry events; update those handlers to call
Sentry.close(timeout) or Sentry.flush(timeout) and await the promise before
calling process.exit(1), passing a reasonable timeout (e.g., 2000 ms), and add a
fallback that forces exit if the drain promise rejects or times out to avoid
hanging; ensure you import/resolve the Sentry instance used by these handlers
and call the drain API in both uncaughtException and unhandledRejection paths.
In
`@apps/riven/lib/message-queue/flows/download-item/steps/find-valid-torrent/find-valid-torrent.processor.ts`:
- Around line 51-69: The scope's "riven.downloader-provider" tag can retain a
previous provider when getPluginProviderList throws or when the early continue
runs before the provider loop sets the tag; to fix, clear/reset the provider tag
immediately after tagging the plugin (i.e., after
scope.setTag("riven.downloader-plugin", plugin.pluginName)) — e.g., call
scope.setTag("riven.downloader-provider", null or "none") before calling
getPluginProviderList(plugin.pluginName, jobParentOptions) and then keep the
existing scope.setTag("riven.downloader-provider", provider) inside the for
(const provider of providerList) loop so each provider iteration overrides the
cleared value; reference symbols: scope.setTag, plugin.pluginName,
getPluginProviderList, providerList, and the for (const provider...) loop in
find-valid-torrent.processor.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f4a7c5f5-1e2a-4697-b488-9f42e198bb65
📒 Files selected for processing (7)
apps/riven/lib/index.tsapps/riven/lib/message-queue/flows/download-item/steps/find-valid-torrent/find-valid-torrent.processor.tsapps/riven/lib/vfs/operations/open.tsapps/riven/lib/vfs/operations/readdir.tselastic-local/config/filebeat.yamlpackages/plugin-stremthru/lib/datasource/stremthru.datasource.tspackages/plugin-torrentio/lib/datasource/torrentio.datasource.ts
Summary by CodeRabbit
New Features
Improvements