Skip to content

Conversation

@dwisiswant0
Copy link
Member

@dwisiswant0 dwisiswant0 commented Sep 23, 2025

Proposed changes

add missing go keyword to anonymous funcs that
were intended to run as goroutines but were
executing synchronously instead.

Fixes #6492

Proof

this PR

$ go test -v ./pkg/protocols/file/ -run "TestFileProtocolConcurrentExecution"
=== RUN   TestFileProtocolConcurrentExecution
    request_test.go:201: Total execution time: 10.474836ms
    request_test.go:202: Files processed: 5
    request_test.go:203: Results returned: 5
--- PASS: TestFileProtocolConcurrentExecution (0.22s)
PASS
ok  	github.com/projectdiscovery/nuclei/v3/pkg/protocols/file	(cached)

dev

Note

Apply bcf7a90 patch first.

$ go test -v ./pkg/protocols/file/ -run "TestFileProtocolConcurrentExecution"
=== RUN   TestFileProtocolConcurrentExecution
    request_test.go:201: Total execution time: 51.871803ms
    request_test.go:202: Files processed: 5
    request_test.go:203: Results returned: 5
--- PASS: TestFileProtocolConcurrentExecution (0.30s)
PASS
ok  	github.com/projectdiscovery/nuclei/v3/pkg/protocols/file	1.340s

Checklist

  • Pull request is created against the dev branch
  • All checks passed (lint, unit/integration/regression tests etc.) with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Summary by CodeRabbit

  • Refactor
    • Workflow execution changed to run templates sequentially (per-template context preserved).
    • Per-file processing was converted to run concurrently for faster input handling.
  • Tests
    • Added a test to verify concurrent file-processing behavior.
  • Chores
    • Improved debug-mode detection and adjusted startup/shutdown sequencing.

@auto-assign auto-assign bot requested a review from Mzack9999 September 23, 2025 12:38
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 23, 2025

Walkthrough

Workflow template execution was changed from concurrent to sequential (removed goroutine per template). File protocol per-path processing was changed from sequential to concurrent (each file now processed in its own goroutine). A new concurrent test and small CLI/debug helper were added.

Changes

Cohort / File(s) Summary
Workflow execution (serialized)
pkg/core/workflow_execute.go
Removed goroutine wrapper and WaitGroup concurrency scaffolding; loop now calls runWorkflowStep synchronously with a fresh context per template; per-template error logging preserved.
File protocol (parallelized)
pkg/protocols/file/request.go
Wrapped per-file processing closure with go func(...) { ... }(...), keeping wg.Add()/defer wg.Done(), error handling and progress tracking; enables parallel file handling.
Tests — concurrent file protocol
pkg/protocols/file/request_test.go
Added TestFileProtocolConcurrentExecution with sync/atomic usage to exercise concurrent file processing; includes artificial delay and assertions (test contains a loop index bug to fix).
CLI debug helper & lifecycle tweak
cmd/integration-test/integration-test.go
Introduced isDebugMode() reading multiple env vars (uses slices), adjusted fuzzplayground cleanup sequencing (defer placement) and minor formatting.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Executor
  participant WG as WaitGroup
  participant Template

  Note over Executor: New (current) workflow behavior — sequential
  Executor->>Template: for each template: create context
  Executor->>Template: call runWorkflowStep(...)
  Template-->>Executor: returns (error logged if any)
  Executor->>Executor: next template (no concurrent goroutine)
Loading
sequenceDiagram
  autonumber
  participant Caller as File Request
  participant WG as WaitGroup
  participant File

  Note over Caller: File processing — now concurrent per file
  Caller->>WG: Add(N)
  par For each file
    Caller--)File: go processFile(filePath)
    File-->>WG: Done (defer)
  end
  Caller->>WG: Wait()
  WG-->>Caller: All complete
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Areas to inspect closely:

  • pkg/core/workflow_execute.go — ensure removal of goroutine was intentional; verify performance and correctness implications and absence of data races when switching to sequential execution.
  • pkg/protocols/file/request.go — confirm safe concurrency (shared state, progress updates, error handling) and correct resource cleanup (file closes).
  • pkg/protocols/file/request_test.go — fix the invalid loop (for i := range numFiles), verify test determinism and cleanup of temp files.
  • cmd/integration-test/integration-test.go — validate isDebugMode() semantics and any import changes (slices) compile cleanly.

Poem

I twitch my nose and skip a beat,
Some files now race with nimble feet.
One workflow slowed, one thread took flight,
I nibble code by lamp‑light.
Hop, patch, and thump — the burrow’s bright! 🐇

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Linked Issues Check ⚠️ Warning According to the linked issue #6492, the fix should restore parallel processing by adding the missing go keyword to anonymous functions in both pkg/core/workflow_execute.go and pkg/protocols/file/request.go. The raw summary indicates that pkg/protocols/file/request.go correctly wraps the function with go func(), aligning with the objective. However, the summary for pkg/core/workflow_execute.go indicates the opposite change—transitioning from concurrent to sequential execution by removing goroutine scaffolding—which directly contradicts the stated objective. Additionally, the test file pkg/protocols/file/request_test.go contains a compilation error in the loop statement (for i := range numFiles is invalid for an int type), which would prevent the test from running and fail to provide the promised proof of concurrent execution. Verify the changes to workflow_execute.go to ensure the go keyword was added to restore parallelism as intended by issue #6492, not removed. Fix the compilation error in the test file by correcting the loop to iterate properly over the integer (e.g., for i := 0; i < numFiles; i++). Ensure both files are corrected according to the linked issue's requirements before merging.
Out of Scope Changes Check ⚠️ Warning The PR includes changes to cmd/integration-test/integration-test.go that introduce debug mode detection via a new isDebugMode() function, adjust fuzzplayground cleanup sequencing, and add minor formatting adjustments. These changes are not mentioned in the linked issue #6492 and do not directly relate to the stated objective of restoring parallel processing in workflow execution or file protocol handling. These modifications appear to be scope creep or unrelated maintenance work that should either be justified by a separate issue or removed from this PR. Either provide a separate linked issue that explains the debug mode and fuzzplayground cleanup changes, or remove these changes from the PR to keep it focused on the stated objective of restoring parallel processing. The PR should maintain a clear scope aligned with issue #6492.
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The title "fix: restore parallel processing in file proto" is specific and refers to the file protocol component. While the title accurately describes the intended fix for the file protocol (adding the missing go keyword), the PR includes changes to pkg/core/workflow_execute.go which, according to the raw summary, appears to do the opposite—replacing concurrent execution with sequential execution by removing goroutine scaffolding. This mismatch between the title's scope (file proto only) and the broader PR scope (which includes workflow execution changes that contradict the objectives) creates potential confusion about what the PR actually accomplishes. Clarify whether the raw summary correctly describes the changes to workflow_execute.go. If the summary is accurate and workflow execution is being changed to sequential (contrary to the issue objectives), the PR title should be updated to reflect this scope, or the workflow changes should be reverted to align with the stated objectives. If the summary is incorrect and the go keyword was actually added to both files, the assessment would change to pass.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dwisiswant0/fix/restore-parallel-processing-in-workflow-file-proto

Comment @coderabbitai help to get the list of available commands and usage tips.

@dwisiswant0 dwisiswant0 changed the title fix: restore parallel processing in workflow & file proto [WIP] fix: restore parallel processing in workflow & file proto Sep 23, 2025
@dwisiswant0 dwisiswant0 marked this pull request as draft September 23, 2025 14:42
Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

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

implementation: lgtm!

Fix: failing tests

@Mzack9999 Mzack9999 self-requested a review October 30, 2025 18:43
dwisiswant0 and others added 8 commits November 2, 2025 21:24
add missing `go` keyword to anonymous funcs that
were intended to run as goroutines but were
executing synchronously instead.

Fixes #6492

Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
* replace hardcoded `DEBUG` env var check with
  extensible helper func.
* add support for GitHub Actions Runner env var.
* accept multiple truthy value variants.

Signed-off-by: Dwi Siswanto <[email protected]>
caused by shared context callbacks.

it was exposed after adding concurrent exec to
workflow processing and occurred when multiple
goroutines attempted to write to the same
`ctx.OnResult` callback field simultaneously,
causing data races during workflow template exec.

Signed-off-by: Dwi Siswanto <[email protected]>
@dwisiswant0 dwisiswant0 force-pushed the dwisiswant0/fix/restore-parallel-processing-in-workflow-file-proto branch from 52d9b58 to 69c831b Compare November 2, 2025 14:35
@dwisiswant0 dwisiswant0 changed the title [WIP] fix: restore parallel processing in workflow & file proto [WIP] fix: restore parallel processing in file proto Nov 2, 2025
Signed-off-by: Dwi Siswanto <[email protected]>
@dwisiswant0 dwisiswant0 changed the title [WIP] fix: restore parallel processing in file proto fix: restore parallel processing in file proto Nov 2, 2025
@dwisiswant0 dwisiswant0 marked this pull request as ready for review November 2, 2025 14:53
@auto-assign auto-assign bot requested a review from dogancanbakir November 2, 2025 14:53
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f249a24 and fb93cbe.

📒 Files selected for processing (4)
  • cmd/integration-test/integration-test.go (5 hunks)
  • pkg/core/workflow_execute.go (1 hunks)
  • pkg/protocols/file/request.go (1 hunks)
  • pkg/protocols/file/request_test.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.go: Format Go code using go fmt
Run static analysis with go vet

Files:

  • pkg/protocols/file/request.go
  • pkg/core/workflow_execute.go
  • pkg/protocols/file/request_test.go
  • cmd/integration-test/integration-test.go
pkg/protocols/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

Each protocol implementation must provide a Request interface with methods Compile(), ExecuteWithResults(), Match(), and Extract()

Files:

  • pkg/protocols/file/request.go
  • pkg/protocols/file/request_test.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: hdm
Repo: projectdiscovery/nuclei PR: 6322
File: pkg/templates/compile.go:79-81
Timestamp: 2025-07-16T21:27:14.937Z
Learning: To make the template caching mechanism in pkg/templates/compile.go production-ready, DSLs need to be updated to use runtime options instead of cached variables, rather than restoring the Compile() calls on each request.
Learnt from: hdm
Repo: projectdiscovery/nuclei PR: 6322
File: pkg/templates/compile.go:79-81
Timestamp: 2025-07-16T21:27:14.937Z
Learning: In pkg/templates/compile.go, the template caching mechanism intentionally skips calling Compile() on copied requests to achieve performance benefits. This is the intended design, not a bug. The current implementation isn't production-ready but represents the desired direction.
📚 Learning: 2025-09-05T14:23:55.257Z
Learnt from: CR
Repo: projectdiscovery/nuclei PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T14:23:55.257Z
Learning: Applies to pkg/protocols/**/*.go : Each protocol implementation must provide a Request interface with methods Compile(), ExecuteWithResults(), Match(), and Extract()

Applied to files:

  • pkg/protocols/file/request.go
  • pkg/protocols/file/request_test.go
🧬 Code graph analysis (3)
pkg/core/workflow_execute.go (2)
pkg/scan/scan_context.go (1)
  • NewScanContext (47-49)
pkg/protocols/common/contextargs/contextargs.go (1)
  • Context (22-33)
pkg/protocols/file/request_test.go (7)
pkg/testutils/testutils.go (2)
  • NewMockExecuterOptions (94-116)
  • TemplateInfo (87-91)
pkg/model/types/severity/severity.go (3)
  • Holder (70-72)
  • Severity (11-11)
  • Low (20-20)
pkg/protocols/protocols.go (1)
  • Request (308-333)
pkg/operators/operators.go (1)
  • Operators (19-45)
pkg/operators/matchers/matchers_types.go (3)
  • MatcherTypeHolder (77-79)
  • MatcherType (12-12)
  • WordsMatcher (17-17)
pkg/protocols/common/contextargs/contextargs.go (1)
  • NewWithInput (48-64)
pkg/output/output.go (2)
  • InternalWrappedEvent (100-113)
  • InternalEvent (93-93)
cmd/integration-test/integration-test.go (1)
pkg/testutils/fuzzplayground/db.go (1)
  • Cleanup (35-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Tests (windows-latest)
  • GitHub Check: Tests (ubuntu-latest)

@dwisiswant0
Copy link
Member Author

@coderabbitai resolve

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 3, 2025

✅ Actions performed

Comments resolved.

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.

[BUG] Parallel processing degraded to sync execution

3 participants