Skip to content

A till that answers nothing - #830

Merged
sridharkalaibala merged 2 commits into
developfrom
fix/a-till-that-answers-nothing
Sep 16, 2026
Merged

sridharkalaibala merged 2 commits into
developfrom
fix/a-till-that-answers-nothing

Conversation

@sridharkalaibala

Copy link
Copy Markdown
Contributor

A live shop's till was not answering its handsets. The diagnosis came from the session on that machine; this is the code half.

src/server.js required ./handset-slots inside the per-request path, and that path is an override of server.emit. So when the module was not there, the throw did not become a 500. It escaped as an uncaught exception and the request was never answered at all - the handset held an open socket until it timed out, nothing appeared on the till, and the shop reported that the captain app keeps disconnecting. Which is the same sentence this whole area exists because of.

[MobileTracker] External request: 192.168.1.2 -> GET /api
[FATAL] Uncaught Exception: Cannot find module './handset-slots'

Localhost stayed instant throughout, because the slots are only consulted for non-loopback callers (ip !== '127.0.0.1' && ip !== '::1'). So everything anybody tried on the machine itself worked perfectly.

Two changes

Required once, at the top of the file, inside a try. A require in the request path is evaluated per request, so a missing file becomes an invisible hang instead of one line in the log at startup.

The decision goes through admitHandset, which never throws and always answers. Module gone, module throwing, module returning nonsense: the handset is admitted and the log says once that the cap is not being applied.

Fail open is the deliberate choice. The cap is there so a shop is not silently serving a stranger's phone, which is worth having and is not a security boundary - the API's own authentication is. Against a till that stops answering its handsets mid-service, an uncapped device list is the smaller harm, and every outage handset-slots.js documents came from refusing a handset that should have been served.

Tests

  1. admitHandset is lifted out of the source and driven directly, because the caller is an emit override and cannot be. The source-level ones pin where the require lives and that nothing calls .admit inside the request path again - verified by putting that call back and watching it fail.

On the bad build itself, which this does not explain

The require and the module landed in the same commit (ec6077e0, 15 Sep, first tagged v1.6.1-beta.867) and the file has never been deleted since. So no clean checkout of any commit produces what is installed on that shop machine: server.js from after that commit, handset-slots.js from before it. Either it was built from a dirty tree, or something replaced server.js in place without adding its new module. Worth knowing which, because the second would be a much bigger problem than this one.

A fresh build from develop does contain the file (asar list shows src\handset-slots.js), and build.files lists it, so packaging is not dropping it today.

Branched on top of #826 so CI is green.

A build reached a shop with server.js asking for ./handset-slots and the
file not beside it. The require sat INSIDE the per-request path, and that
path is an override of server.emit, so the throw did not become a 500 -
it escaped as an uncaught exception and the request was never answered.
The handset held an open socket until it timed out. Nothing appeared on
the till. The shop was told the captain app keeps disconnecting, which is
the same sentence this whole area exists because of.

  [MobileTracker] External request: 192.168.1.2 -> GET /api
  [FATAL] Uncaught Exception: Cannot find module './handset-slots'

Localhost stayed instant throughout, because the slots are only consulted
for non-loopback callers. So everything anybody tried on the machine
itself worked.

Two changes.

The module is required ONCE, at the top of the file, inside a try. A
require in the request path is evaluated per request, and a failure there
is invisible; at the top it is one line in the log at startup.

The decision goes through admitHandset, which never throws and always
answers. Gone, broken, or answering nonsense: the handset is admitted.
The cap exists so a shop is not silently serving a stranger's phone; it
is not a security boundary, the API's authentication is. Against a till
that stops answering its handsets mid-service that is the smaller harm,
and every outage handset-slots.js documents came from refusing a handset
that should have been served.

11 tests. admitHandset is lifted out of the source and driven directly,
because the caller is an emit override and cannot be. The source-level
ones pin where the require lives and that nothing calls .admit inside the
request path again - checked by putting that call back and watching it
fail.

Not the cause of the shop's bad build: the require and the module landed
in the same commit (ec6077e) and the file has never been deleted, so no
clean checkout produces what is installed there. This makes that class of
failure answer something instead of hanging.
This is the actual cause of the till that answered nothing, and it is a
packaging fact rather than a code one.

src/server.js is not in build.files. It is in extraResources, copied to
the resources root and FLATTENED:

  { "from": "src/server.js", "to": "server.js" }

So at runtime it is resources/server.js, and require('./handset-slots')
resolves to resources/handset-slots.js - not to
resources/app.asar/src/handset-slots.js, which is a different directory a
relative require cannot reach.

src/handset-slots.js WAS listed, in build.files, so it went into the
asar. Beside main.js, which does not use it. Never beside server.js,
which does.

AND A TEST ALREADY CLAIMED TO PREVENT THIS:

  assert.ok(pkg.build.files.includes('src/handset-slots.js'))

True, meaningless, and green for the whole life of the bug it names.
Twelfth in the silent no-op family and the most expensive so far.

One line added to extraResources. The new test DERIVES the requirement
instead of restating a filename: for every file flattened out of src/ to
the resources root, every relative require it makes must land at that
same level. It also refuses a require that climbs out of the directory,
which cannot work once flattened, and asserts there are entries to check
at all. Verified by deleting the new line and watching it name the exact
failure the shop saw.

The old assertion now checks both places and says why they are not
interchangeable.

2779 desktop tests, 0 failures.
@sridharkalaibala

Copy link
Copy Markdown
Contributor Author

Correcting the last paragraph of the description, which was wrong, and found the real cause.

I wrote that a fresh build contains the file and "packaging is not dropping it today". The file is in the asar, but that is not where it needs to be, and I checked the wrong list.

src/server.js is not in build.files. It is in extraResources, copied to the resources root and flattened:

{ "from": "src/server.js", "to": "server.js" }

So at runtime it is resources/server.js, and require('./handset-slots') inside it resolves to resources/handset-slots.js - not to resources/app.asar/src/handset-slots.js, which is a different directory that a relative require cannot reach.

src/handset-slots.js was listed in build.files, so it went into the asar, beside main.js, which does not use it. Never beside server.js, which does. That is the whole bug, and it is reproducible from package.json alone.

So this was never a bad build from a dirty tree. Every build since ec6077e0 has shipped this way, including the two I made today, and any till on v1.6.1-beta.867 or later has a dead handset path the moment a non-loopback request arrives.

And a test already claimed to prevent it

assert.ok(pkg.build.files.includes('src/handset-slots.js'));

True, meaningless, and green for the entire life of the bug it was written to stop. Twelfth in the silent no-op family here and the most expensive so far.

What is now in this PR

  • one line in extraResources, which is the fix
  • tests/what-ships-beside-the-server.test.js, which derives the requirement rather than restating a filename: for every file flattened out of src/ to the resources root, every relative require it makes must land at that same level. It also refuses a require('../x'), which cannot work once flattened, and asserts there are entries to check at all. Verified by deleting the new line and watching it name the exact failure the shop saw.
  • the old assertion now checks both lists and says why they are not interchangeable

The admitHandset work in the first commit still stands on its own merit: with it, this class of failure admits the handset and logs it once, instead of leaving the socket open until it times out.

2779 desktop tests, 0 failures.

@sridharkalaibala
sridharkalaibala merged commit d2f1bb6 into develop Sep 16, 2026
9 checks passed
@github-actions github-actions Bot added the ready for QA Merged to develop and live on develop.posnic.io - anyone can test it label Sep 16, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Merged to develop. Anyone can test this - you do not need write access.

Try it at https://develop.posnic.io, or run it yourself:

git fetch origin develop && git checkout develop
npm install && npm --prefix api install
npm run dev   # then http://localhost:3000

When you have tested it, say what you did and what happened, and set
QA passed or QA failed. If you cannot set labels, just comment -
a maintainer will.

Reporting that something is broken is as useful as fixing it. It is
better found here than by a shopkeeper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for QA Merged to develop and live on develop.posnic.io - anyone can test it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant