Skip to content

Conversation

@ricellis
Copy link
Member

Checklist

  • Added tests for code changes or test/build only changes
  • Updated the change log file (CHANGES.md) or test/build only changes
  • Completed the PR template below:

Description

Resolve issues and add Node.js 24 LTS to supported engines.

Fixes i508

Approach

  1. Repeat parts of Add Node.js 24 LTS to supported engines #905 that were reverted in Revert Node.js 24 parts of #905 #912:
  • feat: add Node.js 24 LTS to engines
  • fix: don't call pause/resume after close
    • On Node.js 24 the ordering sometimes changed which caused some reads on the Liner to happen after the readlineInterface was already closed. The pause/resume behaviour would then fail because the stream was already closed. Changed the readlineInterface close event to a once and added a flag to mark when it closed and avoid calling pause/resume after that.
    • readlineInterface does have a closed property, but it is undocumented, so I decided to add our own flag

See also: nodejs/node#58283 and nodejs/node#60507

  1. Slight change from Add Node.js 24 LTS to supported engines #905 to ensure that the unicode characters are all replaced within a chunk. The original attempt should have done so, but there was some misunderstanding on the documented behaviour of the pattern and it did not actually do it. The issue was only exposed intermittently depending on the chunks. Here switch to using replaceAll with the string instead.
  • fix: force escape unicode for Node.js 24 readline
    • Node.js 24 has a breaking change in readline adding U+2028 and U+2029 to the possible line endings - this has the effect of breaking the reading of jsonlines files (like the backup file) where any of the JSON values contain those characters.
    • Change the Liner PassThrough to a Transform that escapes those characters to e.g. \u2028 which can be safely read as U+2028 by JSON.parse (as JSON allows for any unicode character to be optionally escaped)
    • Includes also the fix: add liner sanitize option and use for restore from Add Node.js 24 LTS to supported engines #905 squashed in.
      • The content of e.g. log files is well-known and does not contain U+2028 or U+2029 so there is no need to waste effort doing the escaping when reading those files.
      • Add a sanitize option to the Liner constructor that defaults to false and set it to true for processing a backup file.

See also nodejs/node#57591 and nodejs/node#60606

  1. New fix to prevent write after destroy errors
  • fix: avoid write after destroy errors on Node 24
    • Check the targetWritable.destroyed property before writing
    • If it is destroyed then note it in the debug logs

See also nodejs/node#55270

Schema & API Changes

  • "No change"

Security and Privacy

  • "No change"

Testing

  • Added tests for the Liner sanitize option validating the different behaviours on different Node versions.
  • Write after destroy errors are exposed by existing tests
  • Readline pause/resume after close errors are exposed by existing tests

Monitoring and Logging

  • Added new debug log lines for write after destroy suppression.

@ricellis ricellis self-assigned this Nov 14, 2025
@ricellis
Copy link
Member Author

@eiri there was a failure in the bigger regression suite related to use of readline in another test - I have added 8920f3f for it

@eiri
Copy link
Contributor

eiri commented Nov 17, 2025

lgtm

@ricellis ricellis merged commit a977202 into main Nov 17, 2025
4 checks passed
@ricellis ricellis deleted the i508-node-24 branch November 17, 2025 14:38
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.

3 participants