fix(miner-logs): declare the real payload format on the log file leg - #203
Conversation
The file leg hardcoded `.log` and `application/octet-stream` for every download. The bytes are whatever the miner produced: some models stream a plain-text log, Whatsminers stream a gzipped tar of their log directory (<ip>.logs/board_stat0..3.log, power.log, miner-state.log, …). Clients that trusted the declared name saved an archive as .log and opened it as garbage; the frontend had already been flipped between .tar.gz and .log once each way because the constant can only ever be right for one fleet. The action result carries no format field (coreKey, byteLength, expiresAt, minerId), so read the leading bytes instead: - lib/payloadFormat.js detects gzip/zip/text and, for gzip, inflates only the first tar header to tell a gzipped tar from a bare .gz — never claiming a format it has not seen. Content-Type now follows the same detection. - The peeked chunk is re-emitted ahead of the rest via prependChunk(), which honours consumer backpressure. stream.unshift() cannot be used here: once the peek has consumed the source's last chunk it silently drops the head, truncating small logs. - A source failure before the first chunk returns 500 with a JSON body rather than a truncated stream under a confident content-type. Content-Length, Cache-Control and the ownership/expiry checks are unchanged, and the body is byte-identical to what the miner sent. Note: tests/unit/handlers/finance.handlers.test.js and the two admin_external cases in auth/users routes already fail on develop, unrelated to this change.
paragmore
left a comment
paragmore
left a comment
There was a problem hiding this comment.
nice fix — the detection logic is solid. left a few notes on the plumbing around it.
|
|
||
| const write = async (chunk) => { | ||
| if (out.write(chunk) === false) { | ||
| await new Promise((resolve) => out.once('drain', resolve)) |
There was a problem hiding this comment.
if the client aborts mid-download, fastify destroys out but nothing destroys source — the hyperswarm stream to the miner stays open, and the pump parks here forever on a drain that never fires (checked against streamx locally). The old reply.send(stream) let fastify destroy the source directly. out.on('close', () => source.destroy()) plus a destroyed check in write would cover it — or write the head then source.pipe(out), pipe ties the two lifecycles for free.
There was a problem hiding this comment.
Good catch, and the parked pump was the worse half — thanks.
Lifecycles are tied now: out.on('close', () => source.destroy()), plus write() returns false once out is destroyed so the pump returns instead of awaiting a drain that never comes (the backpressure wait settles on close as well as drain).
I didn't go with source.pipe(out) though: the peek can consume the source's last chunk, and piping an already-ended stream never ends the destination — that's exactly what made the first version of this (stream.unshift) deliver empty bodies for single-chunk logs. Left a note in the JSDoc so it doesn't get re-suggested.
Covered by a regression test: with the close handler removed it fails on its 2s timeout.
| 'use strict' | ||
|
|
||
| const zlib = require('zlib') | ||
| const { PassThrough } = require('streamx') |
There was a problem hiding this comment.
streamx isn't in package.json — it resolves through hoisting from a transitive dep today, so a bump elsewhere in the tree can break this route with no lockfile signal. Same story as the debug dep a while back; worth declaring it in dependencies.
There was a problem hiding this comment.
Right, and digging into it: the repo's convention is already Node's built-in streams (export/serializers.js, and log-downloader.test.js uses node:stream for the same kind of fixture) — my file was the only streamx require in the tree.
So the output is now require('stream').PassThrough and production code doesn't depend on streamx at all. The tests still use streamx.Readable on purpose, since that's what hypercore.createByteStream() actually returns and its peek/prefetch semantics differ from Node's, so streamx is declared as a pinned devDependency (2.28.0, matching brittle/standard style) with the matching lockfile root entry — no more resolving through hoisting.
| // block is expected to be truncated. Returns null when it cannot be inflated at all. | ||
| function inflateHead (head) { | ||
| try { | ||
| return zlib.gunzipSync(head, { finishFlush: zlib.constants.Z_SYNC_FLUSH }) |
There was a problem hiding this comment.
head is a device-supplied chunk and deflate expands up to ~1000x, so this can be a ~66MB sync inflate per request. Detection only needs the first 512 output bytes — inflating a slice like head.subarray(0, 4096) bounds it and keeps tar detection intact. (maxOutputLength doesn't fit here: it throws, which would turn a real .tar.gz into .gz.)
There was a problem hiding this comment.
Agreed, and the maxOutputLength note is exactly why I'd avoided it.
Bounded to head.subarray(0, 4096) now. Measured on a gzip of 64MB of zeros: 67MB allocated before, 4.2MB and 1.35ms after, still classified correctly — tar detection is unaffected since it only needs 512 output bytes.
inflateHead is exported so the bound is asserted on output size rather than by timing, which would be too noisy to catch a regression.
…robe Review follow-ups on the payload-format detection: - A client aborting mid-download left the transfer open: fastify destroys the response stream, but nothing destroyed the hypercore source, and the pump parked on a drain that would never fire. The two now share a lifecycle — destroying the output destroys the source, and a write to a gone consumer stops the pump instead of hanging it. source.pipe() would tie them for free but cannot be used here: the peek may already have consumed the source's last chunk, and piping an ended stream never ends the destination (that is what made the earlier unshift version drop single-chunk logs). - `head` is device-supplied and deflate expands up to ~1000x, so probing a whole chunk could inflate ~67MB synchronously per request. Only the first 4096 bytes are inflated now — 4.2MB and 1.35ms on a 64MB-of-zeros payload, with tar detection unchanged since it needs 512 output bytes. maxOutputLength is deliberately not used: it throws, which would demote a real .tar.gz to .gz. - The output stream is Node's stream.PassThrough rather than streamx, which matches the convention already used in export/serializers.js and drops the runtime dependency entirely. The tests keep streamx.Readable, since that is what hypercore.createByteStream() returns and its peek semantics differ, so streamx is now declared as a devDependency instead of resolving through hoisting from a transitive dep. Both fixes have regression coverage: removing the lifecycle handler fails the abort test on its timeout, and the inflate bound is asserted on output size rather than timing.
3 participants
Asana
Description
Companion to
tetherto/moria-app-ui#2926. The miner log file leg declared a name and type it could not know were true:The bytes are whatever the miner produced. Some models stream a single plain-text log; Whatsminers stream a gzipped tar of their log directory (
<ip>.logs/board_stat0..3.log,power.log,miner-state.log,temp0..3.xls, …). A production export was reported as "corrupted and illegible": the file was a complete, validtar.gz(gzip -tclean,tar -xzfextracts 20 entries / 17 MB) saved as.log, so the OS opened gzip bytes in a text viewer.A fixed extension can only ever be right for one fleet, whichever value it holds — which is why the client-side name has had to change direction before.
What changed
The action result carries no format field (
coreKey,byteLength,expiresAt,minerId), so the file leg reads the leading bytes:workers/lib/server/lib/payloadFormat.js(new)detectPayloadFormat(head)→{ extension, contentType }for gzip / zip / text. For gzip it inflates only the first tar header (zlib.gunzipSyncwithZ_SYNC_FLUSH, tolerating the truncated deflate block) and checksustarat offset 257, so a gzipped tar becomes.tar.gzand a bare gzip stays.gz— it never claims a format it has not seen.peekFirstChunk(stream)reads the first chunk and keeps anerrorlistener attached for the life of the stream (anerrorwith no listener is thrown by EventEmitter, and the source can fail in the gap before the pipe).prependChunk(source, head)re-emits the peeked bytes ahead of the rest, honouring consumer backpressure.stream.unshift()cannot be used here: once the peek has consumed the source's last chunk it silently drops the head, which truncated single-chunk logs — caught by the new test.downloadLogFilenow setsContent-Typeand theContent-Dispositionextension from the detected format, and returns 500 with a JSON body if the source fails before the first byte, rather than a truncated stream under a confident content-type.Content-Length,Cache-Control: no-store, the ownership check and the expiry check are unchanged, and the response body stays byte-identical to what the miner sent.Tests
tests/unit/lib/payloadFormat.test.js(new) — 13 tests: gzipped tar (including from a truncated 512-byte stream prefix), gzip-without-tar, zip, text, empty and 1-byte payloads; peek semantics;prependChunkbyte-integrity for 1/2/3-chunk payloads and a 4 MB payload through the high-water mark; post-peek source failure propagation.tests/unit/handlers/minerLogs.handlers.test.js— file-leg coverage:.tar.gz+application/gzipfor an archive,.log+text/plainfor text, byte-for-byte stream integrity (peek included), preservedContent-Length/Cache-Control, and 500 on an early stream error.makeMockReply()gained header capture.npm run lint(standard) clean. Full unit suite: my files pass;tests/unit/handlers/finance.handlers.test.jsand the twoadmin_externalcases inauth/users routesalready fail ondevelop(verified by stashing this branch) — untouched here.Type
Follow-up
The rack worker should report
fileName/contentTypein thedownloadLogsaction result data, andgetMinerLogDownloadStatusshould surface it — after which neither side needs to sniff bytes. Worth a ticket; the worker repo is outside this change.