fix(logs): address the #595 review — cancel the body, surface auth errors, poll for --since - #612
Merged
Merged
Conversation
Contributor
🚀 Package Preview Available!Install this PR's preview build with npm: npm i @base44-preview/cli@0.1.13-pr.612.385bd7aPrefer not to change any import paths? Install using npm alias so your code still imports npm i "base44@npm:@base44-preview/cli@0.1.13-pr.612.385bd7a"Or add it to your {
"dependencies": {
"base44": "npm:@base44-preview/cli@0.1.13-pr.612.385bd7a"
}
}
Preview published to npm registry — try new features instantly! |
…rors, poll for --since Three findings from the review of #595 that were never actioned before it merged, plus the review's one-line suggestion. - Cancel the response, don't just unlock the reader. readLines' finally called releaseLock() only, so exiting on a typed `end` left the body unconsumed, and an unconsumed body keeps its socket alive in the fetch pool -- a long tail's reconnects pile them up. The silence path's own cancel() is now redundant, so it just returns and lets the finally do it. This is client-side hygiene: the server ends its own generator after the end frame and detaches there, so a rollover does not leave a subscriber behind. - Build the URL and auth headers before the try. They were evaluated inside the fetch() arguments, so a logged-out user's readAuth() throw was caught as a transient failure and laddered 1+2+4+8s before the real error surfaced. A missing token is not something to retry. - `--since` with `--follow` now warns and polls instead of erroring. It worked before #595 (the first poll passed `since` through) and #595 turned it into a hard error, which is a break on a shipped flag pair. A stream only carries what happens next, so a run that asked for the past skips the stream entirely rather than opening one and backfilling around it. Also: `delay` is now `setTimeout` from node:timers/promises, per review.
davidsu
force-pushed
the
fix/logs-review-followups
branch
from
August 31, 2026 11:39
9203691 to
656f72d
Compare
netanelgilad
approved these changes
Aug 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows up #595, which merged with an unactioned review. Three findings from @guyofeck plus @netanelgilad's one-liner. Nothing here overlaps #611 in intent, but both touch
logs.ts— whichever lands second will need a trivial rebase.1. The response body was never cancelled
readLines'finallycalledreleaseLock()and nothing else, so exiting the generator on a typedendframe left the body unconsumed. An unconsumed body keeps its socket alive in the fetch pool, so a long-lived tail's reconnects pile up connections. Thefinallynow cancels; the silence path's owncancel()became redundant and just returns.Scope check, because the first version of this PR body overstated it: this is client-side hygiene, not a server-side subscriber leak. apper's
sse_log_streamreturns right after it yields theendframe, and its ownfinallyfiresdetach()andclose_stream()— so on the ordinary 5-minute rollover the server has already let go before the CLI reconnects. The narrow case where an abandoned socket could hold a server-side subscriber is a consumer that walks away from a still-live stream, and the silence path already cancelled explicitly for that. The apper side assessed it independently: bounded to ≤300s by the generator's own lifetime cap, whosefinallydetaches.2. Auth failures were classified as transient
buildStreamUrl()andbuildStreamAuthHeaders()were evaluated inside thefetch()arguments, i.e. inside thetry. A logged-out user'sreadAuth()throw was therefore caught as{kind: "transient"}and laddered 1+2+4+8s before the real error surfaced through the poll path. Both are now built before thetry, so a missing or unrefreshable token propagates immediately.Repro on
main:base44 logs -f --function foowhile logged out — a 15s pause before the login error.3.
--since+--followpolls instead of erroringBefore #595 this worked: the first poll passed
sincethrough, giving backfill-then-tail. #595 turned it into a hard error, and that shipped in v0.1.13.This restores the old behaviour as a deliberate compatibility concession, not as a feature. Anyone whose scripts already pair the two flags keeps working. The design intent is unchanged and stream-only: a live tail carries what happens next,
--sinceis not part of the streaming story, and the polling path it falls back to is transitional — it goes away once realtime covers every app. So the combination is accepted with the simplest semantics available and nothing more: a run that asks for the past never attempts the stream. No capability check, no backfill-then-stream handover, no silent gap between the two.--untiland--orderstay rejected with--follow.Docs note:
base44-troubleshooterin the skills repo says--sincecannot be combined with--follow. That is the right thing for the design and for where this is heading, so it is deliberately not being rewritten to advertise the pairing; at most it earns a one-line "accepted today, polls" aside. The dependency is recorded on the apper side's rollout file with the exact locations, so a future owner can decide.4.
delayis now nativesetTimeoutfromnode:timers/promises, replacing the hand-rolled promise wrapper.Tests
The old spec asserted the rejection. It is replaced with a live-process spec that runs
--follow --since, waits for the warning on stderr, and asserts the message is the--sinceone — the two stream-failure warnings say "not available for this app" or "Could not reach" instead, so that line is proof the stream was skipped rather than attempted and refused.Full suite 736 passed / 17 skipped; typecheck and lint clean.
🤖 Generated with Claude Code