blurts-server icon indicating copy to clipboard operation
blurts-server copied to clipboard

Don't inspect request headers in cron jobs

Open Vinnl opened this issue 2 months ago • 4 comments

References:

Jira: MNTOR-5071 Figma:

Description

I could not reproduce the exact issue that we're seeing in our logs, but I could trigger the code path that imports next/headers locally, and that shouldn't be possible.

Possibly the error in the logs are because next isn't installed for some reason, or we're using a version of Node that doesn't support importing from next/headers rather than just next?

How to test

Follow the readme instructions on pubsub, but for the curl request, find a user's primary_sha1, take the first six characters and use that as the hashPrefix, and the rest as a hashSuffix.

Also, after you do this once, be sure to DELETE FROM "email_notifications"; if you want to try again.

Checklist (Definition of Done)

  • [x] Localization strings (if needed) have been added.
  • [x] Commits in this PR are minimal and have descriptive commit messages.
  • [x] I've added or updated the relevant sections in readme and/or code comments
  • [ ] I've added a unit test to test for potential regressions of this bug.
  • [x] If this PR implements a feature flag or experimentation, I've checked that it still works with the flag both on, and with the flag off.
  • [x] If this PR implements a feature flag or experimentation, the Ship Behind Feature Flag status in Jira has been set
  • [ ] Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
  • [ ] All acceptance criteria are met.
  • [ ] Jira ticket has been updated (if needed) to match changes made during the development process.
  • [ ] Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

Vinnl avatar Oct 16 '25 12:10 Vinnl

You correctly identified here that the default value of E2E_TEST_ENV=local was leaking into prod (it wasn't overridden by prod config). I'm concerned about how much the runtime and config is entangled with the business logic, and I know it's caused incidents in the past. I think rather than adding more, we should try to address the issue of unclear domain separation/boundaries and hidden dependencies.

kschelonka avatar Nov 03 '25 20:11 kschelonka

@kschelonka As you know, I agree :) That said, I think with that in mind too it can be helpful to merge this - when we get around to untangling it, the checks for NEXT_RUNTIME will help us identify which code paths go into the Next.js codebase and can be left out of the cron job/PubSub codebase. WDYT?

Vinnl avatar Nov 04 '25 08:11 Vinnl

Reading up on this so I can give a more educated reply

kschelonka avatar Nov 06 '25 21:11 kschelonka

Ok, I've done some more reading and thinking. I'm cautious with "magical" stuff like next because I want to make sure I understand what's going on under the hood...

getExperiments as written before had a hidden dependency on the next environment due to importing and invoking loadNextHeaders. I'm resistant to adding more layers of state-dependent logic, when the issue could be solved more simply by accepting actual headers in getExperiments function args (or using DI similarly)

kschelonka avatar Nov 06 '25 22:11 kschelonka

I think I'd rather just not have different code paths executing depending on its call site (that reeks of the wrong abstraction), but figured until we get to that point, it's better to be safe than sorry. But since we're not running into specific issues at this time, it's probably not worth spending too much time on, so I'll close this for now.

Vinnl avatar Dec 16 '25 10:12 Vinnl