Repository navigation
Bus: stop a stuck subscriber from freezing sst dev - #27
Merged
Merged
Conversation
Publish delivered to each subscriber with `ch <- event` while holding the bus read lock. A consumer that stopped draining filled its 10k buffer and parked the publisher on that send, so the read lock was never released. Subscribe and Unsubscribe then blocked on Lock(), and because a pending writer excludes new readers, every other Publish blocked behind them. One stuck consumer froze the entire bus. That path is reachable today: the socket handler writes to websockets inline with no write deadline, so a peer that stops reading stalls it and `sst dev` hangs with no error. Commit 9ec92b5 ("dev: potentially fixed freezing issue") added Unsubscribe to make abandoned subscribers rarer, but left the blocking send itself in place. Delivery is now non-blocking for every subscriber, so the read lock is only ever held for bounded work. Policy is expressed as buffer depth: typed subscribers carry control flow and get 16384 slots, while the firehose keeps 10000 and is cheap to lose. Overflow is counted per subscriber and surfaced through Stats(), so a wedged consumer can be named rather than merely detected. Also fixes Unsubscribe never removing typed subscribers: the inner loop reassigned a local slice and never wrote it back to the map. That path was unreachable from outside the package, which is why it went unnoticed. Callers that close their channel now unsubscribe first, matching the ordering diff.go already used. Benchmarks are included and report drops/op alongside ns/op, so a fast number produced by discarding events cannot pass unnoticed. Carried from anomalyco/sst#7012.
Subscribe returns <-chan interface{} but Unsubscribe took chan
interface{}, so a Subscribe handle could not be passed back. The typed
removal path was therefore unreachable from outside the package, which
is how it stayed broken from the day it was written.
Unsubscribe now takes a receive-only channel. A bidirectional channel
from SubscribeAll is assignable to that type, so every existing caller
compiles unchanged.
Carried from anomalyco/sst#7013.
Automatic review
SummaryThis makes 💡 Suggestions (5)
Release note (draft)
A first pass by opencode with the free |
github-actions
Bot
added
the
risk: high
1 of 2 tasks
the-human-mustachio
added a commit
that referenced
this pull request
Oct 8, 2026
…, 6 and 7 (#47) * astro-sst: import 3.1.4 from anomalyco/astro-sst, unchanged The adapter's source as of the astro-sst@3.1.4 tag (5c488cc), so the commits after this one show only the fork's changes. Every file under src/, tsconfig.json and CHANGELOG.md is byte-identical to the 3.1.4 tarball on npm. From upstream's repo, only packages/astro-sst is taken, plus the root README.md, which is the adapter's documentation. Upstream's master has a forged copy of the 3.1.4 merge (0018334) that added a malicious preinstall.js at the repo root, removed in 4b86ab3; nothing from the root's tooling is carried. Added: LICENSE, since upstream has no licence file and the package is MIT, and a .gitignore like svelte-kit-sst's. The package doesn't build yet: without upstream's lockfile, current @types/aws-lambda and @types/node don't type-check against it. The following commits pin its dev dependencies. * astro-sst: publish as @sst-community/astro-sst, and pin the build The package is renamed and points at this repo, publishes dist/ and the changelog only, and builds from a clean dist/ before it's packed. Its dev dependencies are pinned in a package-lock.json, to the versions upstream built 3.1.4 with: TypeScript 5.6.3, @types/aws-lambda 8.10.146 and @types/node 22.10.2. Built this way, the imported source gives back the 3.1.4 dist on npm byte for byte, .d.ts files included. Astro is the latest 5.x, which the source type-checks against unchanged. The rename means Astro can no longer find the server entrypoint by the package path "astro-sst/entrypoint": `sst init` will install the package under the alias "astro-sst", and an app can also install it by its own name. The adapter now gives Astro the entrypoint's file URL, which works either way. Astro 5, 6 and 7 all accept a URL there. * astro-sst: test the adapter against an Astro 5 app Like svelte-kit-sst's tests: the suite packs the adapter and installs the tarball into a fixture app, under the alias "astro-sst" that `sst init` writes. It builds the app in buffer and stream response modes, bundles each handler with the esbuild options SST uses (sharp left external, as sst.aws.Astro installs it), and calls it with API Gateway v1 and v2 and CloudFront events. No AWS account needed. It checks what sst.aws.Astro reads from the build (entry.mjs exporting the handler, the sst.buildMeta.json fields, 404.html in the server output), and the requests: pages, params, cookies, bodies, the client address, binary responses, redirects, errors, the prerendered 404, Astro's origin check and x-forwarded-host. One test is marked todo, for a bug in 3.1.4: a streamed response with no body, such as a redirect, writes the number 0 to the response stream, which Node streams reject with ERR_INVALID_ARG_TYPE. The new astro-sst workflow runs the tests on pull requests and pushes to main that touch the package. * astro-sst: stream a response with no body correctly In stream mode, a response with no body, such as a redirect or a 204, was labelled gzip and written as the number 0. Lambda's response stream writes a chunk that isn't a string or bytes as JSON, so the client got the body "0" with Content-Encoding: gzip, which fails to decode. The write was there because Lambda sends the status and headers just before the first write (HttpResponseStream.from in aws-lambda-nodejs-runtime-interface-client). The adapter now writes an empty string right after setting them, so they always go out, and only labels a body as gzip when there is one. The tests' stand-in for Lambda's stream now behaves the same way: it sends the status and headers on the first write and writes other values as JSON. A new endpoint checks a 204 in both modes. * astro-sst: support Astro 6 and 7, from anomalyco/astro-sst#27 Astro 6 removed applyPolyfills from astro/app/node, which 3.1.4's entrypoint imports, so the adapter couldn't build any Astro 6 or 7 app. Astro 6 and 7 now use the entrypoints from anomalyco/astro-sst#27 by Florian Lefebvre: one per response mode, each creating the app with createApp from astro/app/entrypoint, with entrypointResolution "auto". PR #27 drops Astro 5; here the 3.1.4 entrypoint stays, moved to entrypoint/astro5.ts, and the adapter picks by the app's Astro major version. Astro 5 still gets its Node polyfills, looked up rather than imported by name, since the package is now type-checked against Astro 7. Also: - The helpers the entrypoints share, and the `awslambda` global, are declared once. - responseMode defaults to "buffer" when it's left out of the options, not only when no options are passed. - edgeMiddleware isn't set: it's deprecated in Astro 7, and false was already the default. - astro is a peer dependency, ^5, ^6 or ^7, so pnpm links the app's copy for the adapter to read. - The ./entrypoint export goes: Astro finds the entrypoints by URL. The tests now also run on an Astro 6.4.8 app and an Astro 7.3.6 app, which imports the adapter by its own name, and check that Astro 5's polyfills put back crypto and File. Fully static sites build the same on Astro 5 and 7, which was checked by hand. Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev> * astro-sst: let Astro route every request, and serve 404.html from disk The entrypoints matched the route themselves before rendering, and for a URL with no route returned the bundled 404.html or "Not found". That bypassed what Astro does for such URLs: - trailing-slash redirects: with trailingSlash "never", /page/ got the 404 page instead of a 301 to /page; - a custom src/fetch.ts in Astro 6 and 7, which never saw a URL that matches no route. And when a page returned an empty 404, Astro fetched the custom 404 page over the network from the site's own URL, the default for prerenderedErrorPageFetch, so it took a round trip and failed where the function can't reach the site. The entrypoints now call render() without a route, and pass prerenderedErrorPageFetch, which serves 404.html from next to the handler, where sst.aws.Astro copies it. Other error pages, such as a prerendered 500, are fetched as before. Astro 5, 6 and 7 all have the option. A URL that matches no route, or only a prerendered page, still gets the custom 404 page. In stream mode the 404 page is no longer logged as an error. The test apps now set trailingSlash "never", and a blog page returns an empty 404 for one slug. The Astro 7 app is also built with a custom fetch handler, src/custom-fetch.ts. All the new tests failed before this change. * Astro: install the sst-community adapter from sst init, and stop SST's own on Astro 6+ `sst init` now installs @sst-community/astro-sst for an Astro app, under the name "astro-sst", so `import aws from "astro-sst"` keeps working. Its Astro list says it adds the adapter, which the template already did. SST's own astro-sst 3.x can't build an Astro 6 or 7 app: Astro 6 removed applyPolyfills, and the build fails with a "Missing export" error that doesn't say what to do. sst.aws.Astro now checks before the build and names the one-line package.json change instead. It only looks when the app's package.json lists "astro-sst", so another package's copy in a monorepo isn't picked up, and not in `sst dev`, which doesn't build. The check runs inside the build command's output, so it's done before the build starts. The README, the landing page and the Astro guide say to switch the adapter line too when switching a project, and the guide says which Astro versions each adapter supports. The adapter has to be on npm before a CLI with this mapping is released: `sst init` looks up its latest version. * Release: publish every adapter in packages/ The step that published svelte-kit-sst now loops over packages/, and publishes each package whose version isn't on npm yet, the same way. That covers astro-sst, and any adapter added later. MAINTAINING.md says how the adapters are versioned and installed, and that a new one is published by hand first, before the release whose CLI maps its name for `sst init`. * astro-sst: release 4.0.0 The first version as @sst-community/astro-sst, for Astro 5, 6 and 7. The changelog lists what changed from SST's 3.1.4, and the README says to install it under the name "astro-sst"; its `astro add` instructions installed SST's package. It starts at 4.0.0 because sst.aws.Astro requires 3.1.2 or later, and it's a major version because the package name changes. This version is published by hand, before the release that maps "astro-sst" to it in `sst init`; release.yml publishes the versions after it. * astro-sst: keep cookies in Lambda@Edge responses For a CloudFront event, the response built the set-cookie headers and then returned a second copy of the headers without them, so cookies the app set, such as a session, never reached the browser. It also labelled every status "OK". The response now returns the headers with set-cookie, only when there are cookies, and leaves out statusDescription, which is optional: CloudFront sends the standard text for the status. sst.aws.Astro doesn't run the server as Lambda@Edge, so this only affects a handler someone connects to CloudFront themselves. The tests now send a cookie with the CloudFront event and check a 404's status; both failed before this change. * astro-sst: fix cookie attributes, binary types and failing streams Three bugs from 3.1.4, found in review. A test for each failed before this change on Astro 5, 6 and 7. Cookies: the Set-Cookie values were parsed and written out again, which turned Max-Age into "maxAge"; browsers ignore that, so the cookie lasted only for the session. They're now passed on as they are. In stream mode they were also joined into one set-cookie header, so two cookies became one broken one; they now go in the prelude's `cookies`, which Lambda sends as separate headers, as OpenNext does. Binary types: a body was decoded as UTF-8 unless its type was on a list of binary types, so AVIF from Astro's image endpoint, MP4 or WASM was corrupted in buffer mode (the list also had "audio/wavaudio/webm" for two types). Now only text types are treated as text, and a response that's already encoded is passed on as bytes and not gzipped again. Failing streams: when the body failed partway in stream mode, the gzip stream was destroyed with the error after its listeners were removed, so the error was thrown out of the process, which crashes the Lambda runtime. The response stream is now ended with the error. The gzip loop also waits for drain before reading more, as the binary one did, and only a body that's actually gzipped is labelled gzip, without the app's uncompressed Content-Length. New fixture endpoints: cookies with attributes, an AVIF image, a body that fails partway and a 3 MB body. * astro-sst: require Astro 5.6 or later Astro added the prerenderedErrorPageFetch render option in 5.6.0. On 5.0 to 5.5 it's ignored, so when a request ends in the prerendered 404 page, Astro fetches it over the network from the site, and in those versions a failed fetch isn't caught, so the request fails. 3.1.4 served 404.html from disk before rendering, which the adapter no longer does. The peer range is now ^5.6.0 || ^6.0.0 || ^7.0.0, so npm refuses to install it next to an older Astro, and the adapter stops the build with a clear message if the range was bypassed. Upgrading Astro within 5.x is a minor update. Checked by hand against Astro 5.5.0. * Astro: only stop SST's own astro-sst 2.x and 3.x The check stopped any adapter named "astro-sst" on Astro 6 or later. If SST releases a version of its adapter for Astro 6, such as one from anomalyco/astro-sst#27, that would have been stopped too. It now only stops 2.x and 3.x, which import applyPolyfills. The Astro guide and the README say "SST's own astro-sst 3.x" for the same reason, and the guide says the sst-community adapter needs Astro 5.6 or later. MAINTAINING.md lists the adapter steps in the Astro and SvelteKit guides among the fork's edits under www/, for merging SST releases. * Release: check the adapters are on npm before releasing Trusted publishing can't create a package, so an adapter's first version is published by hand. If that was forgotten, the release used to fail at the adapter step, after the CLI release, which can't be changed, and without the Discord announcement. A step before the CLI release now checks that each adapter exists on npm, and stops the release if one doesn't. The publish loop also stops on an `npm view` error other than "no such version", instead of reading a network error as "not published", and skips folders without a package.json and private packages. * astro-sst: fix what testing in Lambda found Deploying Astro 5 and 7 apps with sst.aws.Astro, in buffer and stream modes, showed three things the local tests didn't. Each now has a test that failed before this change. A body failing partway in stream mode still crashed the runtime (Runtime.ExitError). The previous fix ended the response stream with the error, and Lambda's runtime then rejects a promise nothing awaits, so Node exits. The streaming result now settles when the response has been sent and rejects when the body fails, so the handler rejects and the runtime reports the error as the invocation's ("Invoke Error"), without crashing. The tests' stand-in for Lambda's stream now treats an error on the stream as the crash it is. Astro.clientAddress came from X-Forwarded-For. A client can send its own, and CloudFront passes it on unchanged: a request with a made-up value reached the app with exactly that value. It now comes from CloudFront-Viewer-Address, which CloudFront sets and replaces when a client sends one (both tried in AWS), and otherwise from the request's source IP. Streamed text responses were gzipped whatever the client accepted, so a client without gzip got bytes it couldn't read. They're now gzipped only when Accept-Encoding allows gzip. Buffered responses were already left to CloudFront. * astro-sst: type-check against the package's own types only CI failed to build the package: `new Error(message, { cause })` needs the ES2022 lib, which the ES2020 target doesn't include. It built here because TypeScript also loads @types packages from every enclosing node_modules, and the repo root's (@types/bun among them) declare it. CI installs only the package. tsconfig.json now sets "types": ["node"], as svelte-kit-sst's does, so only the package's own types load, and "lib": ["ES2022"]. The emitted JavaScript doesn't change. The full test suite passes in a copy of the package outside the repo, installed as CI installs it. * Astro: say which adapter sst.aws.Astro needs, and warn when sst init's install fails From the automatic review of #47. sst.aws.Astro's reference docs never said it needs the astro-sst adapter; only an error did. Its doc comment now says so, and how to install it. `sst init` ran the package manager's install and ignored the result, so a failed install still ended in "Done". With the adapter mapped, an app on Astro 5.0 to 5.5 now fails there with a peer dependency conflict. `sst init` now says the install failed and to run it to see why. Checked against an Astro 5.5.0 app. * Release: tell a missing adapter from an npm error before releasing From the automatic review of #47. The check before the CLI release read any `npm view` failure as "isn't on npm", so a registry error would stop a release with the wrong message. It now says that only for a 404, and otherwise shows npm's output. It still stops the release either way, before anything is published. MAINTAINING.md lists the check among what release.yml does. * Release: check the adapters are on npm first, before the image push From the second automatic review of #47. The check ran just before the CLI release, but the Build platform step before it had already pushed the bridge-task image, latest tag included, which `sst dev` pulls. A release stopped by the check still changed what `sst dev` runs. The check now runs right after Node is set up, before anything is published, which is what MAINTAINING.md says it does. * Astro: say which Astro versions each adapter supports in the docs From the second automatic review of #47. sst.aws.Astro's doc comment, which generates its reference page, names the adapter; it now also says version 4 supports Astro 5.6 and later, 6 and 7, and SST's own 3.x only Astro 5, so the docs say what the pre-build check enforces. * astro-sst: correct what the changelog says about empty 404s An Astro 5 site on 3.1.4 deployed with sst.aws.Astro already showed the custom 404 page when a page returned an empty 404: Astro fetched it through the site's own URL. What changed is that the function now serves its own copy, so it no longer depends on reaching that URL. --------- Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
TestTypedSubscriberIsLossless left its subscriber on the shared bus. Each later run then queued another 12,000 events on it, and from the third run it overflowed and the test's check of the bus-wide drop count failed, so `go test -count=3 ./pkg/bus/` failed. A single run, as CI does, passed.
This was referenced Oct 10, 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.
What this changes
Two fixes to the CLI's event bus (
pkg/bus):Publishsent each event to each subscriber with a blocking send, while holding the bus's read lock. A subscriber that stopped reading filled its buffer and parked the publisher, so every otherPublish,SubscribeandUnsubscribewaited behind it, andsst devhung with no error. Delivery now never blocks. A typed subscriber gets 16,384 slots,SubscribeAllkeeps 10,000, and an event past that is dropped and counted. A warning is logged at most once a second.Stats()andDropped()report it, though nothing calls them yet.deploy,refreshandremovenow unsubscribe before closing their channel, asdiffalready did.Unsubscribetook achan interface{}, butSubscribereturns a<-chan interface{}, so a typed subscription couldn't be passed back. Its removal loop also never wrote its result back to the map.Unsubscribenow takes a receive-only channel, which every existing caller still compiles against.A patch fix, internal to the CLI. One behavior changes: a subscriber thousands of events behind now loses events instead of stalling everything. For the terminal UI, that means missed output lines rather than a hang.
Why
A wedged consumer is reachable today. The socket handler writes to websockets inline with no write deadline, so a client that stops reading stalls it, and with it the bus.
Upstream
Carries anomalyco/sst#7013, which is anomalyco/sst#7012 plus the second fix. Both are open upstream, not merged. anomalyco/sst#7011 is an earlier, smaller version of the second fix. The code is unchanged, and both commits keep their author. Only the commit titles are reworded.
Originally submitted by @iliya-vidrush in the original repo.
How it was tested
go build ./...andgo vet ./pkg/bus/ ./cmd/sst/are clean.go test -race ./pkg/bus/passes, including the new tests and benchmarks.go test -race ./cmd/... ./pkg/...: every package passes exceptcmd/sst/mosaic/multiplexer/tcell-term. ItsTestAnywherehas a race inside the test itself, and it fails the same way onmain.sst devon an app.