Skip to content

fix(cli): stop false success, silent log following and hangs on daemon state changes - #255

Open
tomcasaburi wants to merge 7 commits into
masterfrom
fix/state-audit-039-rpc-client-commands-challenge-pa
Open

tomcasaburi wants to merge 7 commits into
masterfrom
fix/state-audit-039-rpc-client-commands-challenge-pa

Conversation

@tomcasaburi

@tomcasaburi tomcasaburi commented Nov 11, 2026 •

Copy link
Copy Markdown
Member

Fixes six places where a CLI command assumes something about the daemon's state that it never checks.

Why this PR exists

Each of these prints something untrue, ignores what the operator asked for, or waits forever:

  • logs -f goes silent after the daemon trims its log.
  • community stop typo.bso exits 0 like a real stop.
  • challenge install says "reloaded" when the daemon kept the old build.
  • A wrong auth key takes 20s to report.
  • community export hangs at its last percentage when the daemon dies.
  • bitsocial daemon --no-allowPrivateKeyExport next to any other flag leaves private-key export enabled.

They came from a state-machine audit of the CLI. Rinse12, close this if you disagree with the behavior changes listed below. Each finding has its own commit. Any of them can be dropped, except that the two later logs commits (new-file switch, and trim after the file already grew back) build on the first (trim), so dropping the trim fix means dropping all three.

Changes

logs -f after the daemon trims its log in place (src/cli/commands/logs.ts)

Once the daemon has written 20MB, it rewrites the same file to its last 15MB. The follower kept reading at its old offset, past the new end of the file: about 5MB was skipped after the first trim, and output stopped after the second.

Now, before each read, the follower checks that the last 4KB it read still sit right before its offset. It uses read() rather than stat(), which can return a stale size on Windows. When they do not, the file was rewritten. It can end before the offset, or already be longer again: the daemon appends up to 5MB of lines it buffered during the trim, and a busy follower polls late. The follower then finds those 4KB in the trimmed file and continues right after them, so no line is skipped or repeated. The offset and the 4KB come from the raw bytes read, so the check is exact.

If those bytes are gone (the file was emptied or replaced), it waits one more poll in case a trim is still rewriting the file. It then follows the file from its start and prints --- log file was truncated, following it from its start --- to stderr.

logs -f when it switches to a new log file (src/cli/commands/logs.ts)

The switch set the new file, awaited its read, printed it, and only then took the offset from a stat(), while the 300ms poll kept running on its own timer. Lines appended between the read and the stat were lost. A poll that ran inside that window read the new file at the old file's offset and printed part of it twice.

The switch now reads the file first, then sets the file, the offset (bytes read) and the anchor in one synchronous step. It runs from the poll loop every 10th poll (about 3s) instead of from a separate setInterval, so a read never overlaps a switch.

community stop for an address the daemon does not host (src/cli/commands/community/stop.ts)

pkc-js returns a remote instance for an address that is not in pkc.communities. It has no started, so the command skipped the stop, printed the address and exited 0. It now fails when the instance's resolved address is not listed by the daemon, and prints an error that starts with:

Error: Community plebit.bso is not hosted by the daemon at ws://localhost:9138/. Run "bitsocial community list" to see its communities

The check uses the address pkc-js resolved, not the typed string, so stopping by publicKey or by an equivalent .eth/.bso name keeps working.

Reload reports for challenge and page-sort install/remove (src/common-utils/package-registry.ts)

reloadPackagesInDaemon returned res.ok. The daemon answers ok with the list of packages it loaded, even when it kept the previous build because the new one failed to import, or when it reads another data path. It now returns:

{ status: "reloaded", loaded: string[] } | { status: "unreachable" } | { status: "failed", error: string }

install prints reloaded <name>@<version> ... only when that list has the installed version. Otherwise it warns:

Warning: The daemon at ws://localhost:9138/ reloaded its challenges without my-challenge@2.0.0, so it keeps serving what it had before. Either the package failed to import in the daemon (see `bitsocial logs`), or the daemon uses another --pkcOptions.dataPath.

remove warns when the daemon still lists the package or the reload failed. The page-sort commands share this code and get the same behavior. docs/challenges.md and docs/page-sorts.md are updated.

Refused auth key and an export the daemon lost (src/cli/base-command.ts, src/cli/commands/community/export.ts)

  • _connectToPkcRpc rejects as soon as pkc emits ERR_RPC_AUTH_REQUIRED, instead of waiting out the 20s timer. pkc-js has already stopped reconnecting at that point.
  • community export waited forever for its record. After a drop, pkc-js reconnects and re-subscribes to the exports, and the daemon answers with the exports it has, which outlive client connections. A short drop to a live daemon therefore finishes, and still does. A daemon that died never answers, though, and a restarted one has dropped the in-flight export. After a drop the command now waits up to 60s for that answer: pkc-js retries about 1, 3, 7, 15 and 31s after a drop. It fails when no answer comes, when the answer no longer has the export, or at once when pkc-js stops reconnecting (state failed, e.g. after a refused auth key):
Error: Lost the connection to the RPC server during the export, and did not get the export back within 60s. Check that the daemon is running, then export again
Error: The RPC server no longer has export <exportId> after the connection came back, so the daemon probably restarted. Export again

Dynamic flags: a single --flag=value, and --no-<flag> (src/cli/hooks/prerun/parse-dynamic-flags-hook.ts)

The hook skipped dynamic-flag registration whenever argv had at most one token, so community create --title=Hello failed with Nonexistent flag. It now skips only when no token starts with --.

The hook also no longer registers --no-<name> as a dynamic flag when <name> is a static boolean flag with allowNo. oclif matches an exact flag name before it tries negation, so a dynamic no-allowPrivateKeyExport flag shadowed the negation. On master, bitsocial daemon --no-allowPrivateKeyExport together with any other flag (for example --logPath <dir>) left private-key export over RPC enabled, and --no-enableIpfsGc was ignored the same way. Alone, the negation worked on master only because of the early return, so the single-token change needs this guard to keep the daemon's own example working. The check reuses the static flags the hook already skipped by name.

Behavior changes for consumers

  • community stop <address the daemon does not host> now exits 1 instead of 0. Scripts that pass stale addresses will start failing.
  • reloadPackagesInDaemon, reloadChallengesInDaemon and reloadPageSortsInDaemon now return PackageReloadResult instead of boolean. All in-repo callers are updated.
  • challenge/page-sort install and remove can print new warnings. That includes an answer at the reload URL that is not a reload (for example a 403 or 404), which used to be silent.
  • community export now exits 1 when the RPC connection drops and the daemon's exports do not come back within 60s, or come back without the export. Before, it waited until Ctrl+C. A short drop to a live daemon still finishes as before.
  • logs -f checks for a new log file every 10th poll (about 3s plus read time) instead of on an independent 3s timer. Each poll also reads the 4KB before its offset. It can print the new truncation notice on stderr.
  • daemon --no-allowPrivateKeyExport and daemon --no-enableIpfsGc now take effect when other flags are also passed. A daemon started that way on master ran with private-key export and IPFS GC on; after this change both are off, as the flags ask.
  • A single --flag=value (or single boolean flag) for community create, community edit and daemon is now registered and parsed instead of rejected.

Tests

  • test/cli/logs.test.ts:
    • new: the file is trimmed in place while being followed, with no skipped or repeated lines; the trimmed file is already longer than the old offset at the next poll, with no skipped or repeated lines; the file is emptied and followed from its start;
    • new: lines appended during a switch are printed exactly once, using a data: preload that delays stat();
    • runFollowUntil takes an optional env.
  • test/cli/stop.community.test.ts: the fake pkc now has communities, plus a new non-hosted address case. test/cli/stop.community.e2e.test.ts: the same case against a real daemon.
  • test/cli/challenge.test.ts: new install/remove cases against a fake reload endpoint. test/cli/challenge-daemon-reload-target.test.ts, test/cli/page-sort.test.ts and test/cli/challenge-integration.test.ts are updated for the new result shape.
  • test/cli/rpc-connection-timeout.test.ts: an immediate auth rejection with fake timers, and a spawned CLI using the real pkc-js client against a server that answers the upgrade with 403 (20.8s before, under 1s after).
  • test/cli/export.community.test.ts, new describe community export _runExport when the RPC connection drops (fake timers):
    • a short drop, then the daemon's exports with the export: the export finishes;
    • progress continues after a reconnect;
    • exports without the export after a reconnect: rejects;
    • no reconnect, or a reconnect with no exports, within the grace period: rejects;
    • state failed: rejects at once;
    • the connection is already down when the listener attaches;
    • the connection stays up: the export keeps waiting.
  • test/cli/create.community.test.ts: single --title=... and --suggested.language=... tokens.
  • test/cli/parse-dynamic-flags-hook.test.ts (new): loads the daemon command through oclif's Config, runs the prerun hook and then oclif's parser the way this.parse does, without calling run(), so no daemon starts. --no-allowPrivateKeyExport and --no-enableIpfsGc alone, before --logPath, after --logPath, and next to a dynamic --pkcOptions.dataPath=... must parse to false with no no-<name> flag; with no negation both stay true and dynamic flags still register. On master the multi-flag cases fail and the single-token cases pass; with only the single-token change, all 7 negation cases fail.

Every new test that covers a bug failed on the code before its fix, for the reason the issue describes. The single-token --no-<flag> cases pass on master and guard against the regression the single-token change would otherwise add. As one-off checks that are not committed:

  • I drove the real createDaemonFileLogger through two trims while logs -f followed it. Before the fix 1657 of 4499 lines were missing; after it 0 were missing and 0 duplicated.
  • I drove it again with bursts big enough that the lines buffered during each trim made the file longer than the follower's offset (200KB max, 150KB after a trim). A version that only detected a shorter file lost 510 of 27000 lines; the final code lost 0 and duplicated 0.
  • I ran community export against a real daemon through a TCP proxy that cut the connection right after the export started. When the proxy allowed reconnects again after 2.5s, the export finished and the snapshot downloaded. When it refused them, the command failed after 63s.

Checks run:

  • npm run build && npm run build:test pass.
  • The full suite (npx vitest run --config config/vitest.config.ts) passes: 71 files, 1183 passed, 4 skipped, 0 failed. One earlier run had a load flake in test/reachability/runtime.test.ts ("runs on a node setup manages too", a 1s vi.waitFor), a file this PR does not touch; it passes alone and in the rerun.
  • stop.community.e2e and challenge-integration pass against a real daemon.

Closes #254

…ip github-alerts]

The prerun hook returned early whenever argv had at most one token, so
`community create --title=Hello` was rejected as a nonexistent flag while
`--title Hello` worked. Skip registration only when no token is a flag.

Also stop registering `--no-<name>` as a dynamic flag when <name> is a
static boolean flag with allowNo. oclif matches an exact flag name before
it tries negation, so a dynamic `no-allowPrivateKeyExport` flag shadowed
the negation: `daemon --no-allowPrivateKeyExport --logPath <dir>` left
private-key export enabled, and `--no-enableIpfsGc` was ignored the same
way. That already happened on master whenever another flag was passed.
Without this guard, the change above would also break the single-token
form, which is the daemon's own example.
…not host [skip github-alerts]

pkc-js returns a remote instance for an address missing from pkc.communities (a
typo, or a community of another node). It has no `started`, so the command
skipped the stop, printed the address and exited 0, exactly like a real stop.
Now it reports the address and exits 1.
…ed version [skip github-alerts]

install printed "reloaded <name>@<version> in the daemon" on any ok answer, but
the daemon keeps the previous build when the new one fails to import, and it
reloads from its own data path. The reload now returns the list the daemon
answers with: install claims the reload only when that list has the version it
installed and warns otherwise; remove warns when the daemon still lists the
package or failed to reload. page-sort install/remove share the code and get
the same behavior.
…t [skip github-alerts]

pkc-js stops reconnecting after the RPC server refuses the auth key, yet the
connect helper only read errors when its 20s timer fired, so a wrong key took
20s to report. It now rejects on ERR_RPC_AUTH_REQUIRED right away.

`community export` settled only on an exportschange record or Ctrl+C. After a
drop, pkc-js reconnects and re-subscribes to the exports, and the daemon answers
with the exports it has, so a short drop to a live daemon still finishes. A
daemon that died never answers, and the command sat at its last percentage until
Ctrl+C. It now waits up to 60s after a drop for the daemon's exports, fails when
they no longer have the export (the daemon restarted), and fails at once when
pkc-js stops reconnecting.
…[skip github-alerts]

The daemon rewrites its log file to the last 15MB once it has written 20MB,
keeping the filename. `logs -f` kept reading at its old offset, past the new
end of the file, so it skipped about 5MB after the first trim and went silent
after the second. When the file ends before the offset, it now finds the last
bytes it read in the trimmed file and continues after them. A file that was
emptied or replaced is followed from its start, with a notice.
…ad [skip github-alerts]

The switch set the new file, awaited its read, printed it and only then took
the offset from a stat(), while the 300ms poll ran on its own timer. Lines
appended between the read and the stat were never printed, and a poll in that
window read the new file at the old file's offset and printed part of it twice.
The switch now reads the file first, sets file, offset and anchor together from
the bytes read, and runs from the poll loop so a read never overlaps it.
@coderabbitai

coderabbitai Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Pre-merge checks: Passed 1 · Inconclusive 1

Failed Failed checks: 1 inconclusive

Check name Status Explanation Resolution
Title check Pending This assessment has not completed. The check will be retried on the next pre-merge check run.
Passed Passed checks: 1 passed

Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-11T05:41:38.102364Z 6d4ffe4 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6d4ffe4fd7

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/cli/commands/logs.ts Outdated
Comment on lines +256 to +259
if (!chunk) {
// Nothing new. Nothing at `position - 1` either means the file is now shorter than what we read.
// read(), unlike stat(), sees the file's real end on Windows too (see above).
if (position > 0 && (await fd.read(readBuf, 0, 1, position - 1)).bytesRead === 0) await followShrunkFile();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Verify the anchor before trusting the old offset

When logging is busy during a trim, daemon-file-logger.ts buffers and then appends up to 5 MB—the same amount removed by the normal 20 MB-to-15 MB trim—so the file can regrow to or beyond position before this 300 ms poll. In that case the read at the old offset either returns later data or the one-byte probe succeeds, followShrunkFile() is never called, and logs -f silently skips the entire retained/buffered region. Detecting a trim only when chunk is empty and position - 1 is past EOF therefore leaves the original data-loss behavior under sustained log volume; the saved anchor must also be validated when the file has already regrown.

Useful? React with 👍 / 👎.

@claude

claude Bot commented Nov 11, 2026

Copy link
Copy Markdown

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

…t [skip github-alerts]

The trim check only ran when a poll read nothing and the byte before the
offset was gone. The daemon appends up to 5MB of lines buffered during a
trim right after it, and a busy or late follower polls after that, so
the trimmed file could already be longer than the old offset. The
follower then read on from that offset and skipped every line between
the retained part and it.

Each poll now checks that the last bytes it read still sit right before
its offset, and relocates them in the file when they do not. The initial
offset and anchor come from the raw bytes read, so the check is exact.
@tomcasaburi

Copy link
Copy Markdown
Member Author

Follow-up on review feedback:

Fixed

  • Codex P2 (logs.ts:259, trim not detected when the file already grew back past the old offset): fixed in 95027f0. A trim was only detected when a poll read nothing. The daemon appends up to 5MB of lines it buffered during the trim, and a busy follower polls late, so the file could be longer than the offset again by the next poll, and those lines were skipped. Each poll now checks that the last 4KB it read still sit right before its offset, and finds them in the file when they do not. The initial offset and those 4KB now come from the raw bytes read, so the check is exact. New test in test/cli/logs.test.ts ("keeps following a trimmed log that grew past the old offset before the next poll") failed on 6d4ffe4 and passes now. With the real file logger and bursty writes, 6d4ffe4 lost 510 of 27000 lines and 95027f0 lost 0 with no duplicates.

No action

  • Claude review: no issues found.
  • CodeRabbit: did not review (its free OSS review limit was reached).

CI / checks

  • All CI checks passed on 6d4ffe4. 95027f0 has not run in CI yet.
  • Locally on 95027f0: npm run build && npm run build:test pass, and the full suite passes (71 files, 1183 passed, 4 skipped). One earlier full run hit a load flake in test/reachability/runtime.test.ts, a file this PR does not touch. It passes on its own and in the rerun.
  • The PR body is updated for the new check, the new test and the commit order: both later logs commits depend on the first.

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.

logs -f, community stop/export, challenge install and RPC connect misreport or hang on daemon state

1 participant