Fixed bug with calling next Express middleware after sending response #3707

Ryszard-Trojnacki · 2025-03-06T12:00:15Z

Description

There were few places in code when response was send (res.send) and then called next() middleware from ExpressJS.
This caused error in response, because 404 handler was executed and tried to send response (status header) once again.

The exception:

VM560:1 Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
    at ServerResponse.setHeader (node:_http_outgoing:659:11)
    at ServerResponse.header (/home/fruitapp/app/LiteFarm/packages/api/node_modules/express/lib/response.js:794:10)
    at ServerResponse.send (/home/fruitapp/app/LiteFarm/packages/api/node_modules/express/lib/response.js:174:12)
    at ServerResponse.json (/home/fruitapp/app/LiteFarm/packages/api/node_modules/express/lib/response.js:278:15)
    at errorHandler (file:///home/fruitapp/app/LiteFarm/packages/api/dist/api/src/server.js:326:9)
    at Layer.handle_error (/home/fruitapp/app/LiteFarm/packages/api/node_modules/express/lib/router/layer.js:71:5)
    at trim_prefix (/home/fruitapp/app/LiteFarm/packages/api/node_modules/express/lib/router/index.js:326:13)
    at /home/fruitapp/app/LiteFarm/packages/api/node_modules/express/lib/router/index.js:286:9
    at Function.process_params (/home/fruitapp/app/LiteFarm/packages/api/node_modules/express/lib/router/index.js:346:12)
    at next (/home/fruitapp/app/LiteFarm/packages/api/node_modules/express/lib/router/index.js:280:10)
    at file:///home/fruitapp/app/LiteFarm/packages/api/dist/api/src/server.js:336:5
    at Layer.handle [as handle_request] (/home/fruitapp/app/LiteFarm/packages/api/node_modules/express/lib/router/layer.js:95:5)
    at trim_prefix (/home/fruitapp/app/LiteFarm/packages/api/node_modules/express/lib/router/index.js:328:13)
    at /home/fruitapp/app/LiteFarm/packages/api/node_modules/express/lib/router/index.js:286:9
    at Function.process_params (/home/fruitapp/app/LiteFarm/packages/api/node_modules/express/lib/router/index.js:346:12)
    at next (/home/fruitapp/app/LiteFarm/packages/api/node_modules/express/lib/router/index.js:280:10)
    at /home/fruitapp/app/LiteFarm/packages/api/node_modules/express/lib/router/index.js:646:15
    at next (/home/fruitapp/app/LiteFarm/packages/api/node_modules/express/lib/router/index.js:265:14)
    at next (/home/fruitapp/app/LiteFarm/packages/api/node_modules/express/lib/router/route.js:141:14)
    at file:///home/fruitapp/app/LiteFarm/packages/api/dist/api/src/controllers/farmController.js:55:24
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Code causing error:

// server.ts - error handler
// handle errors
const errorHandler: ErrorRequestHandler = (error, _req, res, _next) => {
  res.status(error.status || 500);
  res.json({
    error: {
      message: error.message,
    },
  });
};

app
  .use((_req, _res, next) => {
    const error: Error & { status?: number } = new Error('Not found');
    error.status = 404;
    next(error);
  })
  .use(errorHandler);

// farmControler.js
          res.status(400).send('No country selected');
          return next();

Status is set res.status(400) then next() is called which exececutes error handler, which trying once again status res.status(error.status || 500);

Jira link: N/A

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Tested on self hosted server before there was an error in console now this error not happens.

Duncan-Brain

@Ryszard-Trojnacki

Sorry for the delayed review. To me this looks ok for farm controller. But field controller actually has a next function in the express routes.

I am not sure the purpose, or if its still working, but there is a weather station id check that looks as though it is supposed to be run after sending a response.

antsgar · 2025-04-01T13:41:33Z

@Ryszard-Trojnacki I haven't reviewed this one yet because although the change seems reasonable, I haven't had the time to dig into why the API tests are failing. Do you have a clue as to why that is?

Duncan-Brain · 2025-04-01T13:51:17Z

@antsgar It seems like the tests failing is on our end - for some reason it seems like despite approving the workflows to run on this external repo branch the environment variables did not get passed in as expected.

Ryszard-Trojnacki · 2025-04-01T13:55:16Z

@antsgar I have seen this API test error, but this isn't related to this PR. It is in all Pull requests that are remote (from forks). For example:

PR by dependabot: Bump tar-fs and puppeteer in /packages/api #3727,
PR by me for translations: Translation tools #3227,
PR by someone else: Update jest version to 29.7 #3528.

This is related to GitHub behaviour and secrects: https://docs.github.com/en/actions/security-for-github-actions/security-guides/using-secrets-in-github-actions#using-secrets-in-a-workflow

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

This makes a lot of sense, because I could make a PR with printing those secret variables.

antsgar · 2025-04-08T00:22:19Z

@Ryszard-Trojnacki excellent, I hadn't had the time to dig into it, makes sense to me too.

Did you get a chance to see @Duncan-Brain's comment re the field route? It does seem like there's another middleware running after addField and from a quick glance it doesn't seem like it attempts to write to the response stream, so that one should be okay?

Ryszard-Trojnacki · 2025-04-08T06:23:00Z

OK, I have checked this and @Duncan-Brain was right.

Changes were made in two files: farmController.js and fieldController.js.

fieldController.js in route has this:

router.post(
  '/',
  hasFarmAccess({ body: 'farm_id' }),
  checkScope(['add:fields']),
  fieldController.addField(),
  fieldController.mapFieldToStation,
);

handler for fieldController.mapFieldToStation and without next() it will be not called.
I have fixed this and I'm now calling directly this handler in changed method addField:

          mapFieldsToStationId([field]);

I have checked this and mapFieldToStation is used only in this one route.

The second file farmController.js also has changed one method addFarm but here in route configuration I don't see any extra handlers:

router.post('/', farmController.addFarm());

and therefore it doesn't require additional change.

There are also global handlers added in server.ts but those are:

path routes like .use('/fertilizer', fertilizerRoutes) and this will be not called because of path prefix,
error handler but this also doesn't matter in this case.

I hope that it is all right this time.

Duncan-Brain · 2025-04-11T16:05:54Z

packages/api/src/controllers/fieldController.js

          await trx.commit();
-          req.field = { fieldId: result.field_id, point: result.grid_points[0] };
+          const field = { fieldId: result.field_id, point: result.grid_points[0] };
+          mapFieldsToStationId([field]);


Thanks for looking at this, now what I am seeing is that we are changing the order of execution, and the function is also now covered by the try catch block.

So should this function fail, the response will not be sent. I still don't know anything about this function but the programmer who did it felt that it should not block a successful response from the other code.

As I understand this mapFieldsToStationId is a RxJS function and it is not blocking and will not throw exception.
It will create only the RxJS pipeline and return (not throwing exception not doint anything). Then, when request is handled it will be executed by pipeline steps which are async.
There are two places when an exception can be thrown:

getStationIdFromField but it is catched with catchError and logged to console.

await insertStationToField(fieldData) and this is unhandled, but this will not throw and error in this call context.

As I understand RxJS (never used it) mapFieldsToStationId will never throw exception in this call context.

@Ryszard-Trojnacki Yeah, I have no clue about how this RxJS works -- RxJS really seems overkill and unnecessary here. And I really don't understand the call context with RxJS.

Since this bug is not super high on our bug radar, and it is on such an important endpoint (AddField) , I don't quite feel comfortable not putting some time into learning RxJS or guaranteeing handling the error. So I cheated and asked an AI if there could be some unhandled errors and it seemed to believe that there are some chances:

If fieldsWithNoStationId() rejects (e.g., DB failure), the promise is passed to from(fields), which won’t catch rejection unless fieldsWithNoStationId is explicitly awaited and wrapped in a try/catch.

catchError should return something (e.g., an observable) to recover. Otherwise, it terminates the stream.

.subscribe() does not catch async/await errors unless you use a try/catch inside the callback.

My thoughts are:

Does this one specific endpoint actually cause the 404 error since it uses next() correctly? The others had no next middleware just the error handler, this one actually has next middleware. I think this should not cause a 404.

If it does actually cause the error - can we leave it in for now -- I think we could make a tech-debt ticket to remove the station_id, weather-station stuff altogether -- I cannot find where it is used at all -- and if we go that route then @antsgar could set it's priority.

Follow AI guidelines - least favourite option - inside the function top level try/catch, handle catchError() correctly with return, try/catch and error in subscribe()

First of all fieldsWithNoStationId() is never called in this scenario. This method mapFieldsToStationId is always called with parameter in this context:

mapFieldsToStationId([field]);

then:

const fields = fieldsToSet ? Promise.resolve(fieldsToSet) : fieldsWithNoStationId();

fieldsToSet is set to [field] and Promise.resolve(fieldsToSet) is returned, not fieldsWithNoStationId().

Next what is called is:

concatMap((field) => from(getStationIdFromField(field)).pipe(delay(1000))),

and getStationIdFromField is a Promise then it cannot throw exception in mapFieldsToStationId call.

And last I can just wrap this call in try catch, but still I think if there is throw an error, then it should be handled/fixed, not ignored like now. Now response is send to user and if an exception happens it is ignored.

Good point about the first line. I do still wonder if a 404 is possible here since next() points to this function correctly unlike the other endpoints.

I think the deeper issue for me is it is possibly just unused code so I might prefer to address this one in another PR and accept the other good fixes you have here

Duncan-Brain · 2025-04-11T16:06:39Z

packages/api/src/controllers/fieldController.js


  // eslint-disable-next-line no-unused-vars
-  mapFieldToStation(req, res) {
+  mapFieldToStation(req, _res) {


Depending on the solution, is this function necessary?

No, I don't think so. I didn't want to remove code that maybe someone would need to use in future.

… in `farmController.js` and `fieldController.js`.

Ryszard-Trojnacki requested review from a team as code owners March 6, 2025 12:00

Ryszard-Trojnacki requested review from Duncan-Brain and kathyavini and removed request for a team March 6, 2025 12:00

kathyavini requested a review from antsgar March 7, 2025 23:47

Duncan-Brain reviewed Mar 17, 2025

View reviewed changes

Duncan-Brain reviewed Apr 11, 2025

View reviewed changes

antsgar removed their request for review April 16, 2025 14:40

Ryszard-Trojnacki added 2 commits September 16, 2025 13:41

Fixed bug with calling next Express middleware after sending response…

f4f610d

… in `farmController.js` and `fieldController.js`.

Fix for fieldRoute addField additional handler call.

5d8e44a

Ryszard-Trojnacki force-pushed the bugfix/ILOT-50-invalid-express-next-call branch from 9be9daf to 5d8e44a Compare September 16, 2025 11:41

Fixed bug with calling next Express middleware after sending response #3707

Are you sure you want to change the base?

Fixed bug with calling next Express middleware after sending response #3707

Uh oh!

Conversation

Ryszard-Trojnacki commented Mar 6, 2025

Uh oh!

Duncan-Brain left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antsgar commented Apr 1, 2025

Uh oh!

Duncan-Brain commented Apr 1, 2025

Uh oh!

Ryszard-Trojnacki commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antsgar commented Apr 8, 2025

Uh oh!

Ryszard-Trojnacki commented Apr 8, 2025

Uh oh!

Duncan-Brain Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

Ryszard-Trojnacki Apr 12, 2025

Choose a reason for hiding this comment

Uh oh!

Duncan-Brain Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ryszard-Trojnacki Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

Duncan-Brain Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

Duncan-Brain Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

Ryszard-Trojnacki Apr 12, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Duncan-Brain left a comment •

edited

Loading

Ryszard-Trojnacki commented Apr 1, 2025 •

edited

Loading

Duncan-Brain Apr 16, 2025 •

edited

Loading