Skip to content

Conversation

fabiovincenzi
Copy link
Contributor

Fixes #1196
Refactor of teeAndValidate middleware to extractRawBody that only extracts request body, centralized all validation logic in proxyFilter function, and eliminated duplicate executeChain() calls for POST git-pack requests

Copy link

netlify bot commented Sep 19, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit afece3a
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/68d404f72909d7000854d3f5

Copy link

codecov bot commented Sep 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.88%. Comparing base (cbc7ad6) to head (afece3a).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1209      +/-   ##
==========================================
+ Coverage   83.87%   83.88%   +0.01%     
==========================================
  Files          68       68              
  Lines        2908     2904       -4     
  Branches      367      364       -3     
==========================================
- Hits         2439     2436       -3     
  Misses        409      409              
+ Partials       60       59       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

LGTM - have you tested against any large pushes?

@fabiovincenzi
Copy link
Contributor Author

fabiovincenzi commented Sep 24, 2025

LGTM - have you tested against any large pushes?

@kriswest I'm actually doing some manual testing here, the fix successfully eliminates the duplicate executeChain calls - I can see in the logs, which should improve performance.

However, I'm investigating an issue where large pushes seem to hang during the getRawBody() buffering step, both before and after my changes.

@kriswest
Copy link
Contributor

@fabiovincenzi if you end up needing an alternative to raw-body you could try something simple like:

app.use(function(req, res, next) {
  req.rawBody = '';
  req.setEncoding('utf8');

  req.on('data', function(chunk) { 
    req.rawBody += chunk;
  });

  req.on('end', function() {
    next();
  });
});

I can't think what else might be stalling things...

@fabiovincenzi
Copy link
Contributor Author

@kriswest
The stalling wasn't raw-body itself but the PassThrough stream highWaterMark being too small (16KB default).
Fixed by increasing it to 4MB.
Now handles big pushes as well.

@kriswest
Copy link
Contributor

Aha, good catch then - I think this is possibly affecting us and would explain some weird performance issues

Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

LGTM

@kriswest kriswest enabled auto-merge September 24, 2025 14:53
@kriswest kriswest merged commit dcf154f into finos:main Sep 24, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate duplication of process steps in push approval flow
2 participants