Skip to content
This repository was archived by the owner on Sep 17, 2026. It is now read-only.

Raise the firehose scanner limit and stop discarding ErrTooLong - #28

Open
Peeja wants to merge 1 commit into
mainfrom
claude/swarf-firehose-scanner
Open

Peeja wants to merge 1 commit into
mainfrom
claude/swarf-firehose-scanner

Conversation

@Peeja

@Peeja Peeja commented Sep 17, 2026

Copy link
Copy Markdown

⏭️ Safe to merge without waiting for CI — no. Watch unit swarf, unit hilt, unit ingot, and both itest suites.

Refreshed on every push. Checks are not required yet, so this is advice, not a gate.

Derived, not asserted — swarf/pkg/client is imported in non-test code by:

$ grep -rln "swarf/pkg/client" --include='*.go' . | grep -v _test.go | grep -v '^./swarf/'
hilt/pkg/rpc/service/bucket/service.go
hilt/pkg/api/service/accesskey/service.go
hilt/pkg/fx/revocation.go
ingot/revocation/consumer.go
ingot/module.go
check verdict
unit swarf Load-bearing. The change and its new tests live here.
unit hilt, unit ingot Load-bearing. Both link this client in production code.
itest hilt, itest ingot Load-bearing. Both boot stacks that stream revocations.
ci → other 10 unit jobs, guards Skippable — no other module reaches swarf/pkg/client, and no workflow, guard script or compose file changed.
images ×8, e2e Skippable-ish. No Dockerfile changed, but hilt and ingot images embed this code. The weaker half of this block — if either goes red, believe it over me.

What this costs if it's wrong: the checks still run; we just don't wait. If an ignored check goes red after merge, main is red.


The third of the four swarf findings in MONOREPO_TODO.md, raised on #3 and deliberately left alone then — "changing behaviour inside a commit whose job is to move code makes a regression and a migration fault indistinguishable." The imports are long settled, so that reason has expired. Of the four, this is the only one that is a bug rather than a decision.

The bug

swarf/pkg/client/client.go built a default bufio.Scanner — 64 KiB token cap — and discarded the error:

scanner := bufio.NewScanner(response.Body)
...
// A read error means the stream was interrupted; the caller reconnects.
_ = scanner.Err()
return nil

A FirehoseRevocation carries a CID per delegation in its Path, so a long chain pushes one SSE event past the cap. Scan stops, ErrTooLong is dropped, streamConn returns nil, and Stream reconnects at the same cursor — forever. The record is never yielded and no error is ever returned.

It presents as a hang, not a failure, which is the worse of the two. And it lands in production revocation handling for two services, per the derivation above.

The fix already existed here, in the wrong place

cmd/swarf/stream.go has always done it right — scanner.Buffer(make([]byte, 64*1024), 4*1024*1024) and a real scanner.Err() check — while the library every other service consumes did neither. Same limits, now named as constants so the two can't drift silently.

The careful part: ErrTooLong is not a read error

The existing taxonomy is deliberate, and I kept it:

nil an established connection ended → caller reconnects. Right for a transient interruption.
corruptError Stream yields it to the consumer and stops.

ErrTooLong is not transient — the same event sits at the same cursor, so reconnecting is the bug. It now returns corruptError: an event too large to read belongs in the same family as one that can't be decoded. Every other read error still returns nil, so reconnection behaviour is unchanged.

Verified in both directions

Against the unfixed library, both new subtests fail by timing out at 30 seconds with context deadline exceeded — that is the hang itself reproduced, not a proxy for it — and pass in 1.1s with the fix:

--- FAIL: TestStreamLargeEvent/over_the_default_cap_is_delivered (30.00s)
--- FAIL: TestStreamLargeEvent/past_streamScanMax_errors_rather_than_hanging (30.00s)
        the stream hung instead of reporting the oversized event
FAIL	github.com/fil-forge/forge/swarf/pkg/client	60.015s

The delivery case asserts its fixture actually exceeds 64 KiB, so it cannot quietly stop testing anything. hilt and ingot build, vet, and pass their revocation tests; gofmt -s clean.

The other three findings are decisions, not fixes — left alone

  • The revocation lookup is served immutable for a year but is mutable. Either "newest matching" is the contract and the caching is wrong, or the endpoint should be content-addressed by cause CID and Get-by-delegation is the wrong route shape. It wants deciding together with the next one.
  • The memory and PostgreSQL stores disagree about what Get returns. Memory overwrites on repeated revocation; PostgreSQL keeps every row and returns the newest. A test passing against memory can be wrong about production.
  • streamSettleWindow = 10s assumes a bound on transaction duration that PostgreSQL does not give. The principled fix is a monotonic sequence, which is a schema change.

I'd rather you chose on those three than have me pick a contract and encode it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt


Generated by Claude Code

Recorded as a finding on #3 and deliberately left alone then: changing
behaviour inside a commit whose job is to move code makes a regression and a
migration fault indistinguishable. The imports are long settled, so that reason
has expired.

swarf/pkg/client/client.go:259 built a default bufio.Scanner, which caps a token
at 64 KiB, and line 286 discarded the error under a comment saying the caller
reconnects. A FirehoseRevocation carries a CID per delegation in its Path, so a
long chain pushes one SSE event past the cap. Scan then stops, ErrTooLong is
dropped, streamConn returns nil, and Stream reconnects at the same cursor --
forever. The record is never yielded and no error is ever returned, so it
presents as a hang rather than a failure, which is the worse of the two.

This is not hypothetical for two services: hilt/pkg/fx/revocation.go and
ingot/revocation/consumer.go both consume this client in non-test code, so the
failure lands in production revocation handling.

The fix already existed in this repository, in the wrong place:
cmd/swarf/stream.go:79 raises the buffer to 4 MiB and line 101 checks the
error, while the library every other service consumes did neither. Same limits
here, named as constants so the two cannot drift silently.

ErrTooLong is reported as corruptError rather than nil, and that distinction is
the careful part. The existing taxonomy is deliberate: nil means an established
connection ended and the caller should reconnect, which is right for a transient
interruption. ErrTooLong is not transient -- the same event sits at the same
cursor -- so reconnecting is precisely the bug. corruptError is what Stream
already surfaces to the consumer before stopping, and an event too large to read
belongs in the same family as one that cannot be decoded. Other read errors keep
returning nil, so reconnection behaviour is unchanged.

Verified in both directions rather than only the green one. Against the unfixed
library both new subtests fail by timing out at 30 seconds with context deadline
exceeded -- the hang itself, reproduced, not a proxy for it -- and pass in 1.1s
with the fix. The delivery case asserts the fixture actually exceeds 64 KiB, so
it cannot quietly stop testing anything. hilt and ingot build, vet and pass
their revocation tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Peeja pushed a commit that referenced this pull request Sep 17, 2026
MONOREPO_TODO.md's "Findings in the imported code" were left alone because
changing behaviour inside a commit whose job is to move code makes a regression
and a migration fault indistinguishable. The imports are settled, so that reason
has expired and the whole category is ordinary work again. Worth recording as a
category, not just as one PR.

Of swarf's four, one was a bug rather than a decision. #28 fixes it: the
firehose client used a default bufio.Scanner and discarded ErrTooLong, so an
event with a long delegation Path was never yielded and Stream reconnected at
the same cursor forever -- a hang rather than a failure, in code hilt and ingot
both link in production. The fix already existed in cmd/swarf/stream.go.

The other three are contract decisions and stay open, named on both pages so
they do not read as oversights: the immutable cache on a mutable route, the
memory-versus-PostgreSQL Get divergence, and the settle window that assumes a
bound PostgreSQL does not give.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Peeja pushed a commit that referenced this pull request Sep 17, 2026
Every load-bearing check in #28's own block passed -- unit swarf, unit hilt,
unit ingot, both itest suites -- along with e2e and all eight images.

Third e2e pass on the postgres healthcheck fix, and the arithmetic updated
rather than the conclusion: three consecutive passes had about an 86% chance at
the old 5% rate, so this still rules out very little. Counting them is the point;
declaring victory is not.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Peeja pushed a commit that referenced this pull request Sep 17, 2026
Checked rather than assumed: the fix and its test contain no module paths, the
patch applies cleanly to fil-forge/swarf at its head, and there it builds, vets
and passes both subtests under the upstream module path. Nothing is
monorepo-specific.

The deciding fact is that upstream hilt and ingot both require
github.com/fil-forge/swarf, pinned at v0.0.1-0.20260821142121 (2026-08-21),
while the bug arrived in 7520dac on 2026-08-18 -- so the pin has it. The
monorepo ships nothing, Phase 1's release machinery not existing yet. As it
stands #28 fixes the hang in the copy nobody deploys and leaves it in the copy
everyone deploys.

Landing the identical patch in both places is also cheapest for the final pull:
byte-identical ours and theirs merge with no conflict.

Two caveats recorded: fil-forge/swarf is not in this session's repository scope,
so an upstream PR needs the repo added; and fixing upstream swarf does not fix
its consumers, which each need a pin bump -- the same shape as the versitygw
lockWaitTime item.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants