Repository navigation
fix(cli): stop false success, silent log following and hangs on daemon state changes - #255
tomcasaburi wants to merge 7 commits into
Conversation
…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.
Pre-merge checks:
|
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 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".
| 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(); |
There was a problem hiding this comment.
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 👍 / 👎.
Code reviewNo 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.
|
Follow-up on review feedback: Fixed
No action
CI / checks
|
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 -fgoes silent after the daemon trims its log.community stop typo.bsoexits 0 like a real stop.challenge installsays "reloaded" when the daemon kept the old build.community exporthangs at its last percentage when the daemon dies.bitsocial daemon --no-allowPrivateKeyExportnext 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 -fafter 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 thanstat(), 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 -fwhen 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 stopfor 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 nostarted, 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:The check uses the address pkc-js resolved, not the typed string, so stopping by publicKey or by an equivalent
.eth/.bsoname keeps working.Reload reports for
challengeandpage-sortinstall/remove (src/common-utils/package-registry.ts)reloadPackagesInDaemonreturnedres.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:installprintsreloaded <name>@<version> ...only when that list has the installed version. Otherwise it warns:removewarns when the daemon still lists the package or the reload failed. Thepage-sortcommands share this code and get the same behavior.docs/challenges.mdanddocs/page-sorts.mdare updated.Refused auth key and an export the daemon lost (
src/cli/base-command.ts,src/cli/commands/community/export.ts)_connectToPkcRpcrejects as soon as pkc emitsERR_RPC_AUTH_REQUIRED, instead of waiting out the 20s timer. pkc-js has already stopped reconnecting at that point.community exportwaited 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 (statefailed, e.g. after a refused auth key):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=Hellofailed withNonexistent 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 withallowNo. oclif matches an exact flag name before it tries negation, so a dynamicno-allowPrivateKeyExportflag shadowed the negation. On master,bitsocial daemon --no-allowPrivateKeyExporttogether with any other flag (for example--logPath <dir>) left private-key export over RPC enabled, and--no-enableIpfsGcwas 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 staticflagsthe 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,reloadChallengesInDaemonandreloadPageSortsInDaemonnow returnPackageReloadResultinstead ofboolean. All in-repo callers are updated.challenge/page-sortinstall 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 exportnow 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 -fchecks 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-allowPrivateKeyExportanddaemon --no-enableIpfsGcnow 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.--flag=value(or single boolean flag) forcommunity create,community editanddaemonis now registered and parsed instead of rejected.Tests
test/cli/logs.test.ts:data:preload that delaysstat();runFollowUntiltakes an optionalenv.test/cli/stop.community.test.ts: the fake pkc now hascommunities, 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.tsandtest/cli/challenge-integration.test.tsare 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 describecommunity export _runExport when the RPC connection drops(fake timers):failed: rejects at once;test/cli/create.community.test.ts: single--title=...and--suggested.language=...tokens.test/cli/parse-dynamic-flags-hook.test.ts(new): loads thedaemoncommand through oclif'sConfig, runs the prerun hook and then oclif's parser the waythis.parsedoes, without callingrun(), so no daemon starts.--no-allowPrivateKeyExportand--no-enableIpfsGcalone, before--logPath, after--logPath, and next to a dynamic--pkcOptions.dataPath=...must parse tofalsewith nono-<name>flag; with no negation both staytrueand 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:createDaemonFileLoggerthrough two trims whilelogs -ffollowed it. Before the fix 1657 of 4499 lines were missing; after it 0 were missing and 0 duplicated.community exportagainst 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:testpass.npx vitest run --config config/vitest.config.ts) passes: 71 files, 1183 passed, 4 skipped, 0 failed. One earlier run had a load flake intest/reachability/runtime.test.ts("runs on a node setup manages too", a 1svi.waitFor), a file this PR does not touch; it passes alone and in the rerun.stop.community.e2eandchallenge-integrationpass against a real daemon.Closes #254