Skip to content

Benchmark: sentry PR 93824 - #8

Open
celmis-codereviewer wants to merge 24 commits into
cr-base-93824from
cr-pr-93824
Open

Benchmark: sentry PR 93824#8
celmis-codereviewer wants to merge 24 commits into
cr-base-93824from
cr-pr-93824

Conversation

@celmis-codereviewer

Copy link
Copy Markdown

Benchmark reproduction of getsentry#93824

@celmis-codereviewer celmis-codereviewer left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHANGES REQUESTED — blocking findings

Full findings and scope are in the review summary comment on this pull request — one persistent comment, updated in place on every run.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

@celmis-codereviewer celmis-codereviewer left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 COMMENT — findings to consider

Full findings and scope are in the review summary comment on this pull request — one persistent comment, updated in place on every run.

"click_options": multiprocessing_options(default_max_batch_size=100),
"click_options": [
*multiprocessing_options(default_max_batch_size=100),
click.Option(

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why: When ingest_transactions_options() runs on line 432, click.Option receives 'flusher_processes' in param_decls, raising ValueError at startup because option declarations in Click must start with a dash.

🟠 Invalid Click option declaration causes ValueError on startup

click.Option expects option flags starting with dashes (e.g., ['--flusher-processes']). Passing 'flusher_processes' as a second item in param_decls causes Click's parameter parser to raise ValueError: Option declarations must start with '-' or '/' ('flusher_processes') when options are loaded.

Suggested change
click.Option(
click.Option(
["--flusher-processes"],
"flusher_processes",
default=1,
type=int,
help="Maximum number of processes for the span flusher. Defaults to 1.",
),

agent: defect · rule: defect.invalid-api-call · confidence: 0.95

self.process_restarts += 1
self._create_process()
try:
if isinstance(process, multiprocessing.Process):

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why: When process is created via self.mp_context.Process on line 104, it is a SpawnProcess which does not inherit from multiprocessing.Process, so isinstance(process, multiprocessing.Process) on line 254 evaluates to False and hung flusher processes are never killed.

🟠 isinstance check fails for SpawnProcess, preventing hung flusher processes from being killed

self.mp_context is created with multiprocessing.get_context("spawn"), so self.mp_context.Process returns a SpawnProcess instance. SpawnProcess inherits from multiprocessing.process.BaseProcess, not multiprocessing.Process (multiprocessing.context.Process), causing isinstance(process, multiprocessing.Process) to evaluate to False. As a result, process.kill() is skipped when a process hangs.

Also at line 346.

Suggested change
if isinstance(process, multiprocessing.Process):
if isinstance(process, multiprocessing.process.BaseProcess):
process.kill()

agent: defect · rule: defect.type-check · confidence: 0.90

@celmis-codereviewer

Copy link
Copy Markdown
Author

🤖 Code Review for PR #8

💬 COMMENT — findings to consider

Findings

  • 🟠 Error: 2

Scope

  • Files changed: 6
  • Lines: +199 / -50

Performance

  • Analysis time: 482.4s · agents: structural, cve, security, contract, defect · tokens: 33,526/33,152

Powered by Code Analyzer · context: tree-sitter graph + structural, cve, security, contract, defect

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants