fix(portal): close silent-gap paths in backfill floor + stream realtime#8
Conversation
d0f852e to
2d87c0e
Compare
dzhelezov
left a comment
There was a problem hiding this comment.
Request changes — all five findings are confirmed real on current main; please re-cut this onto post-refactor main rather than resolving conflicts in place.
This branch forks from 802ceed, before the invariant-first re-architecture (#6) and the tooling commit (#7). I re-verified every claim against main: none of the five gaps were fixed by the refactor — two were actually codified as documented contracts (INV-9 "warn + empty", INV-10 "gap resets"), so this PR changes invariant semantics and must update the catalog (portal/INVARIANTS.md), not just the code. Per-finding:
- Backfill floor (C10) — confirmed, but the fix targets moved code. The computation now lives in
compileFetchSpec,portal/portal-filters.ts:359-362(identicalfilter(b => b != null)+Math.minbug). Re-express the fix there; your C10 regression test ports as-is.⚠️ Since this is the one production-path (non-stream) gap and it blocks our validation campaign baseline, we are cherry-picking this floor fix onto main immediately, credited to this PR — please rebase over it and keep the four stream-realtime hardenings here. - Head-probe fatal (C11) — confirmed at both seams (
portal-realtime-wire.ts:114-117passthrough;portal.ts:417-423warn+empty). Keep the fix, but land it together with: the INV-9 row update, the shell test "INV-9: stream-realtime mode suppresses the RPC fallback (warn + empty, NOT delegated)" (its warn-message assertion breaks under your warn→debug demotion), the wire test "conservative passthrough", and the now-false comment atportal-realtime-wire.ts:114-115claiming the RPC finality-gap fallback is the safety net (it's suppressed in stream mode). assertStreamModeSupported— confirmed missing on main; good fail-loud guard. Three asks: (a) assert at startup too, not only at realtime-generator start after a multi-day backfill (the wiring'sgetLocalSyncProgresshasparams.filters); (b) consider exempting non-log filters withtoBlockbounded at/below startup finality (historical-only sources), or comment the accepted over-strictness; (c) surface theevent.transaction === undefinedstream-mode limitation in INTEGRATION.md, not only in a code comment.logsRevisionreconnect — confirmed (portal-realtime-wire.ts:305-308still "picked up on the next/streamreconnection"). The revision design and its interleaving are sound (bump lands whilestreamHotBlocksis suspended atyield; the resume cursor makes the reopen lossless). But main'sstreamHotBlocksnow reads viandjsonLines(portal-client.ts:44-70), which has nofinallyreleasing its reader — a straight port leaks the locked reader/connection on every discovery-triggered reopen. Add try/finally +reader.cancel()tondjsonLines(also fixes the same latent leak for every other early exit) or manage the reader locally. Please also comment the #4×#7 interaction: forced reopens widen the reconnect window in which a concurrent reorg surfaces as a fatal gap instead of a handled reorg — acceptable (crash beats silent corruption), but document the tradeoff.- Gap-fatal — confirmed (
portal-realtime.ts:218-221silently clears; the "finalized floor protects committed data" comment ignores already-indexed unfinalized blocks left on the wrong fork with no rollback event). Keep the throw; update the INV-10 row ("gapresets") and theReconciletype comment ("caller must re-sync"), and consider registering these as new G-numbered gaps in INVARIANTS.md so doc⟷code⟷test stay grep-able. (Fail-loud path verified end to end: a rejection escaping the realtime generator hitsunhandledRejection→exit(75)inponder start.)
Mechanics: drop the vite.portal.config.ts, coverage-diff.mjs, and sync-upstream.sh hunks entirely — #7 already landed your vite config byte-identically, and main's script versions are supersets (one-arg coverage-diff, Biome gate, glob copy); merging these would regress #7. New code must be single-quoted (sync-upstream.sh --test gates on biome check portal/ --diagnostic-level=error). Re-run scripts/sync-upstream.sh 0.16.6 --test and 0.15.17 --test against the full post-refactor suite, and re-verify each regression test still fails with its fix reverted, since the surrounding implementations changed.
All five fixes are worth landing — this is a valuable PR; it just predates the re-architecture.
2d87c0e to
d890aaf
Compare
|
Thanks for the incredibly thorough re-verification — the per-finding map to the post-refactor code was exactly what I needed. I've rebased onto post-refactor main ( Done / matches your asks:
Planned next pass (non-controversial):
One thing to confirm — findings 6 & 7. This push currently does not include them. Since both flip documented contracts (INV-9 warn+empty, INV-10 gap-resets) rather than filling a doc gap, I want your call on whether you'd prefer them landed here — with the |
…C10, ports #8) (#10) An undefined `fromBlock` means "from genesis" (ponder's runtime reads `filter.fromBlock ?? 0`), but `compileFetchSpec`'s `backfillStart` took `Math.min` over only the DEFINED fromBlocks. A config mixing one bounded source (e.g. fromBlock 15M) with one unbounded source therefore computed backfillStart=15M; `chunkRange` clamped every chunk fetch to >=15M, and the unbounded source's [0, 15M) intervals were assembled EMPTY and marked synced -- a permanent silent gap on the production historical path. Fix: if ANY filter omits fromBlock the floor is 0, symmetric with the existing backfillEnd rule (any undefined toBlock => unbounded). Re-expressed for the post-refactor compileFetchSpec; catalogued as INV-16. Ports the backfill-floor finding from PR #8 (@mo4islona); the other four stream-realtime findings stay with that PR. Tests (both mutation-verified to fail on the pre-fix source): - portal-filters.test.ts INV-16/C10: backfillStart is 0 for undefined, min for all-defined, 0 for mixed. - portal.test.ts C10 regression: two sources (one fromBlock 15M, one undefined) -- the undefined source's early-range log IS fetched/inserted. Gates green on both compat.tested versions (0.16.6, 0.15.17): 134 tests each. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
Heads-up on timing: the C10 floor fix is now on main (#10, credited). We're starting a 7-day Portal-native-realtime soak as soon as the four stream-mode fixes here land — they're exactly the paths the soak exercises. If a re-cut onto main isn't feasible within ~24h, we'll port them ourselves with credit (same as #10) so the soak clock can start; either way works, just flagging so we don't duplicate effort. |
…s 4–7) Rebased onto post-refactor main. The backfill-floor fix (finding 3 / C10) landed separately as #10 (INV-16), so this PR carries the four stream-realtime hardenings. Each ships a regression test; INVARIANTS.md is updated where a contract changes. - reconnect on child discovery (finding 4): a `logsRevision` bumped on each discovered child; `streamHotBlocks` snapshots it at open and re-opens with the widened server-side filter the moment it advances. `ndjsonLines` gained a `finally { void reader.cancel() }` (fire-and-forget) so the early break closes the old connection without slowing the historical hot path. - refuse unsupported stream mode (finding 5): `assertStreamModeSupported` rejects non-log or receipt-requiring sources under PORTAL_REALTIME=stream. Asserted at process start (wiring's getLocalSyncProgress) AND at realtime-generator start. - finalized-head probe fatal (finding 6, INV-9): in stream mode the RPC fallback is suppressed, so a persistently unknown Portal head can't locate the historical ↔realtime boundary. Both seams now fail loud — clampFinalizedToPortalHead retries then throws, and portal.ts throws on an unknown head — instead of passthrough / returning []. Changes the prior INV-9 "warn + empty" contract, deliberately. - unknown-parent gap fatal (finding 7, INV-10): a reorg deeper than the unfinalized window (or a skipped block) now throws instead of silently clearing the window, which left already-indexed unfinalized blocks on the wrong fork with no rollback. Changes the prior INV-10 "gap resets" contract, deliberately. INVARIANTS.md: INV-9/INV-10 rewritten; G4 (finding 6) and G5 (finding 7) added to the fixed-gaps list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
d890aaf to
fa51170
Compare
|
Pushed — all four stream-realtime fixes land here, rebased onto current main ( Addressed your per-finding asks:
One thing I deferred (flag if you want it in-PR vs follow-up): the INTEGRATION.md note documenting the log-only |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dzhelezov
left a comment
There was a problem hiding this comment.
Re-verified the re-cut independently: full gate green on both ponder versions (140/140 each), all four fixes mutation-verified (each new test fails with its fix reverted), INV-9/INV-10 catalog consistent, and the fatal paths traced end-to-end to ponder's exit(75) under start. Pushed one maintainer commit removing the stray .idea/ files (+ gitignore). The INTEGRATION.md stream-limitations note (log-only sources, no event.transaction at the tip, unknown-head/gap now fatal with exit-75 restart semantics) is fine as the follow-up you offered — we'll fold it into the docs pass if you don't get there first. Excellent work — merging. The stream soak starts on this build.
PR #8 makes unknown-head / reorg-gap FATAL in stream mode (exit 75, EX_TEMPFAIL) with restart as the designed recovery. soak-b.service now: Restart=on-failure (covers 75) + an ExecStartPre that appends a UTC timestamp to $SOAK_B_RESTART_LOG on every (re)start (ReadWritePaths widened to cover it). ab-diff.mjs reads that log (restartStats, unit-tested) and surfaces restartCount / lastRestartAt / restartsLastHour in soak-ab-status.json, raising a crash-loop alert at >3 restarts/hour. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
The four stream-realtime silent-gap hardenings, rebased onto post-refactor main. The production-path backfill-floor fix (finding 3 / C10) landed separately as #10 (INV-16, credited) and is dropped here. Each fix ships a regression test;
portal/INVARIANTS.mdis updated where a documented contract changes.portal-realtime.ts+ wire): alogsRevisionbumped on each discovered child;streamHotBlockssnapshots it at open and re-opens with the widened server-side filter the moment it advances — so a child's logs are caught on the next block, not only after an unrelated reconnect.ndjsonLinesgained afinally { void reader.cancel() }(fire-and-forget) so the early break closes the old connection without slowing the historical hot path.assertStreamModeSupportedrejects non-log or receipt-requiring sources underPORTAL_REALTIME=stream(they'd be silently skipped while marked synced). Asserted at process start (getLocalSyncProgress, via the wiring patch) and at realtime-generator start.clampFinalizedToPortalHeadretries then throws, andportal.tsthrows on an unknown head — instead of passthrough / returning[]. Deliberately changes the prior INV-9 "warn + empty" contract.INVARIANTS.md: INV-9 / INV-10 rewritten; G4 (finding 6) and G5 (finding 7) added to the fixed-gaps list, with the deliberate contract changes flagged.Test plan
scripts/sync-upstream.sh 0.16.6 --test— 140/140 greenscripts/sync-upstream.sh 0.15.17 --test— 140/140 green (wiring patch validated on bothcompat.testedversions)biome check portal/ --diagnostic-level=errorcleanportal-realtime.test.ts—streamHotBlocksre-opens with the widened filter when the logs revision advancesportal-realtime-wire.test.ts—assertStreamModeSupported(non-log refused, receipts refused, log-only ok)portal-shell.test.ts— stream-mode UNKNOWN head is FATAL;portal-realtime-wire.test.ts— clamp unknown head → FATALportal-realtime.test.ts— an unknown-parent gap is FATAL, not silently skippedOne deferred one-liner: the INTEGRATION.md note on the log-only (
event.transaction === undefined) stream-mode limitation — happy to add in a follow-up.🤖 Generated with Claude Code