HarperFast / HarperFast/harper

logRotationTransport hand-wraps four manageThreads exports it does not customize

Open
#2,534 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
89
Forks
10
Avg merge
2d 2h
Merged PRs (30d)
205

Description

`server/threads/logRotationTransport.ts` exists to hand the log-rotation generation coordinator a thread mesh from the `server/threads/` side of a deliberate dependency edge — `utility/logging/` must never import `manageThreads.js`, because `manageThreads` already imports `harper_logger`.

The way it does that is a hand-written object literal that re-declares every method the coordinator's `RotationTransport` interface names, four of which are pure pass-throughs:

```ts
setRotationTransport({
threadId,
broadcast(message) { broadcast(message); },
sendToThread(target, message) { threads.sendToThread(target, message); },
onMessage(type, handler) { onMessageByType(type, handler); },
onThreadExit,
peerThreadIds() { /* the only real customization */ },
});
```

Only `peerThreadIds()` carries a decision — it deliberately includes job workers, which `isEligibleBroadcastRecipient` excludes from the schema broadcast but which do hold log descriptors. Everything else is boilerplate that has to be edited again every time the interface gains a member, and that silently goes stale if a `manageThreads` signature changes underneath it.

Raised by @dawsontoth reviewing https://github.com/HarperFast/harper/pull/2475: *"this smells like something that will be brittle later ... peerThreadIds is the real customization we're making."*

**On the suggested shape.** Subclassing is not available as written: `manageThreads.js` is a CommonJS module, not a class, and `threads` is the `connectedPorts` array with `sendToThread`/`onMessageByType` bolted onto it. The equivalent cleanup is for `manageThreads` to export the narrow mesh interface itself (or for the transport to close over `connectedPorts` directly) so this file only supplies `peerThreadIds` and the eligibility decision behind it. That is the change worth making; the wrapper is only the symptom.

Related: the file is loaded by a bare `require('./logRotationTransport.ts')` at the very bottom of `manageThreads.js` — load-order-sensitive coupling chosen to keep the dependency direction. An explicit init call from the server bootstrap would be testable without importing the whole thread manager, and belongs in the same cleanup.

No behavior change, no user-visible impact — this is maintainability of a module that shipped in #2475.

Contributor guide

Open the contributing guide

Research direction

Start by reading server/threads/logRotationTransport.ts and manageThreads.js, including the bare require at the bottom and the RotationTransport interface it adapts. Trace how the thread mesh is exported and initialized, then simplify the coupling so only peerThreadIds and its eligibility decision remain customized. Done means the dependency direction and behavior are unchanged, while initialization is explicit and testable.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js, typescript
Domain
backend, distributed-systems
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.