22 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 11-jobs-realtime-and-search-infrastructure | 2026-09-30T13:16:12Z | standard | 84 |
|
|
issues_found |
Phase 11: Code Review Report
Reviewed: 2026-09-30T13:16:12Z Depth: standard Files Reviewed: 84 Status: issues_found
Summary
I reviewed the Phase 11 code: the River job layer (conga, the lagoon queue migrations, OnDatabase and AfterCommit), the scheduler, realtime (lighthouse, the Centrifugo driver, the subscribe proxy and the fonoteka authorizers), search (beachcomber and Typesense), Web Push (flare), the parity tooling (tide broadcasts) and the fonoteka.go bindings. For files that already existed, I reviewed only the Phase 11 diff (718a35c..HEAD, and 3d4895d^..HEAD in fonoteka.go). I compared behaviour against the PHP originals in /media/nvme/dev/golem15/fonoteka where parity was in question.
Several parts hold up:
- The subscribe proxy compares the secret in constant time and fails closed.
- Channel parsing and the collection and wishlist authorizers match the PHP rules exactly.
- Broadcast jobs are enqueued on the write's
*sql.Txinside a savepoint. - The River payload travels as a JSON string, so its key order survives JSONB.
- VAPID and Typesense keys are kept out of logs and error messages.
- The push endpoint allowlist rejects non-https URLs, URLs with user info and redirects.
The main defect is the order of commit and sync:
lagoon.AfterCommitruns its callback immediately inside a plaingormtransaction.- The framework's own admin CRUD (cabana) and the fonoteka
SaveAlbumboth use plain transactions. - As a result, Typesense is updated before the commit and before the belongs-to-many pivots are written. The index keeps stale
artist_idsafter a successful edit, and keeps changes the database rolled back.
Other issues:
- Framework API traps: a poisoned query handle passed to broadcast callbacks, identifier tokens that the proxy's PHP (int) cast turns into user ids, and the misleading nested
lagoon.Transactioncontract. - Scheduled commands that open their own database fail inside a running worker.
- The VAPID
--updateflow writes the private key into the app's repository tree, which is not gitignored. - The subscribe proxy shares its rate limit bucket with public traffic.
Critical Issues
CR-01: The search index syncs mid-transaction, before pivots are written, and diverges from committed data
File: modules/lagoon/transaction.go:100-117, modules/beachcomber/sync.go:86-90 (callers: modules/cabana/crud.go:311-373, ../fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service.go:55-67)
Issue: When lagoon.AfterCommit finds neither a lagoon.Transaction buffer nor a GORM-started transaction, it runs the callback immediately. That is always the case inside a plain gdb.Transaction(...). beachcomber relies on AfterCommit for every Searchable write, so in a plain transaction syncOne reloads the row, builds the document and sends the Typesense upsert or delete in the middle of the transaction.
The framework's own admin write path is exactly this shape: CRUDService in cabana calls s.DB.WithContext(ctx).Transaction(...). It then runs tx.Save(target) (sync fires here) and only afterwards syncBelongsToMany(...) and formAfterUpdate(...).
Consequences for the fonoteka album admin form, which has an artists relation field:
- After a successful edit that changes artists,
ToSearchableArrayhas already read the oldgolem15_fonoteka_album_artistsrows. The document keeps staleartist_ids, and nothing repairs it until the album is saved again. - When
syncBelongsToMany, the after hook, or a BulkDelete row fails after an earlier row was synced, the database rolls back, but Typesense already holds the upsert or has lost the document. - The Typesense HTTP call, bounded at 2s, runs while the transaction holds the row locks.
SaveAlbum has the same order: tx.Save(album) runs before syncArtists. Unlike PHP AlbumWriteService, it also never re-saves after the pivot sync; PHP calls $album->save() / touch() after syncing. The updated broadcast payload built in the same callback (realtime.go:112) also carries the pre-edit artist list.
This behaviour is documented and tested ("a plain gorm transaction syncs immediately"), but the documented design contradicts the beachcomber contract ("built from the committed row") on the framework's main write path.
Fix: Route framework and app writes through lagoon.Transaction so after-commit work waits for the commit:
// modules/cabana/crud.go (and BulkDelete/Delete/relation.go writes)
err := lagoon.Transaction(ctx, s.DB, func(ctx context.Context, tx *gorm.DB) error {
// ... unchanged body, using tx
})
// fonoteka classes/album_write_service.go
return lagoon.Transaction(ctx, gdb, func(ctx context.Context, tx *gorm.DB) error {
// validate, ResolveArtists, tx.Save(album), syncArtists(...)
})
Separately, make AfterCommit refuse to run inside a foreign *sql.Tx: log at Warn and skip, or return an error, instead of running immediately. This way a plain transaction can never push uncommitted state to an external system. Add a cabana admin-edit test that changes the artists and asserts the indexed artist_ids.
Warnings
WR-01: Broadcast channel and payload callbacks get a handle that continues the write's statement after WithContext
File: modules/lighthouse/broadcast.go:443-449
Issue: inSavepoint builds the handle it passes to Broadcastable.BroadcastChannels, BroadcastPayload and the Binding functions with db.Session(&gorm.Session{NewDB: true, Context: ctx}). lagoon/transaction.go:155-166 documents exactly why this is unsafe on a callback's handle:
- the
Contextoption clones the write's statement (table, WHERE and SET clauses); - the usual idiom
tx.WithContext(ctx)callsSession{Context}again withNewDBfalse (clone = 2), so the next chained call continues from that clone.
A channel function written as tx.WithContext(ctx).Model(&Collection{}).Where("id = ?", m.CollectionID).Take(&c) therefore queries the written model's table, with the write's primary key condition added. The broadcast is then silently skipped, or it computes the wrong channels. The fonoteka bindings avoid WithContext by chance; the framework API does not protect against it. beachcomber already uses the clean-handle fix.
Fix: Reuse the clean-handle construction:
tx := db.Session(&gorm.Session{NewDB: true, Context: ctx}).Clauses().Session(&gorm.Session{NewDB: true})
(or export lagoon.CleanHandle and use it in lighthouse and beachcomber). Add a test whose channel function starts with tx.WithContext(ctx).
WR-02: Scheduled commands that open their own database fail inside a running worker
File: modules/conga/commands.go:215-225, modules/conga/scheduler.go:147-161
Issue: The scheduler runs catalog commands in process (cat.Call) inside serve, queue:work or schedule:run. All three have already published *sql.DB and *gorm.DB. Commands built on the withDB pattern then run OpenFromApp (a second pool) followed by lagoon.Publish, and backpack.Registry.Publish returns duplicate provider for *sql.DB. This affects conga's own queue:clear, lagoon's migrate commands (lagoon/commands.go:78-88) and any plugin command copied from them, so scheduling one of them always fails. scheduleRunOnce works around this for itself with withAppDB, but the commands it calls do not.
Fix: Make withDB in conga (and lagoon) reuse a published handle, the way withAppDB and cabana.withAdminDB already do:
func withDB(ctx context.Context, app *backpack.App, fn func() error) error {
if gdb, ok := app.Lookup[*gorm.DB](); ok && gdb != nil {
return fn()
}
// ... open, publish, defer close
}
Add a scheduler test that runs a DB-using command while the database is published.
WR-03: A nested lagoon.Transaction over the root handle is an independent transaction, but its callbacks wait on the parent
File: modules/lagoon/transaction.go:54-65
Issue: The nested branch is chosen from the context buffer alone. If the inner call receives the app's root *gorm.DB rather than the outer tx, gdb.WithContext(childCtx).Transaction opens a new, independent transaction that commits on its own. This is common when a service takes (ctx, app DB). The child's AfterCommit callbacks are nevertheless moved into the parent buffer:
- if the parent rolls back, the callbacks are dropped although the child's writes are committed, so search sync or blob deletes are lost;
- the child's writes are not rolled back with the parent, contrary to the documented "a nested Transaction becomes a savepoint".
Fix: Take the nested path only when gdb is actually inside a transaction:
if parent, ok := ...; ok && parent != nil {
if _, inTx := gdb.Statement.ConnPool.(gorm.TxCommitter); inTx {
// savepoint path (current code)
}
// otherwise: treat as a top-level transaction with its own buffer
// (run its callbacks after its own commit), or return an error.
}
WR-04: websockets:generate-vapid-keys --update writes the VAPID private key into the application's repository tree
File: modules/flare/commands.go:182-193; ../fonoteka.go/main.go (compass.Load("config")); ../fonoteka.go/.gitignore
Issue: saveKeys persists push.private_key through compass.Persist to <config dir>/env/<env>/overrides.yaml. fonoteka.go loads config relative to the working directory, so running the command from the repository root writes the private key to fonoteka.go/config/env/<env>/overrides.yaml. Neither fonoteka.go nor the framework .gitignore ignores that path, so the next git add -A commits the key. config/push.yaml explicitly warns against committing it.
Fix: Add config/env/*/overrides.yaml to the application .gitignore (and to the scaffolded app template). The command could also print a warning, or refuse, when the destination is inside a git work tree and not ignored (git check-ignore).
WR-05: Identifier connection tokens are authorized as user ids by the subscribe proxy
File: modules/lighthouse/centrifugo/token.go:86-107, modules/lighthouse/centrifugo/handlers.go:147-152
Issue: ForIdentifier and SubscriptionForIdentifier sign tokens whose sub is an arbitrary non-user identifier. Centrifugo forwards that sub as user to the subscribe proxy, which converts it with lighthouse.PHPInt, the PHP (int) cast. An identifier such as "42-guest", "7abc" or a UUID that starts with digits ("42a1c9…") becomes user id 42 or 7. Every authorizer then evaluates the subscribe as that user, which lets a guest or device identifier subscribe to a real user's collection channels. The cast is PHP parity, but pairing it with a public identifier-token API is a privilege-confusion trap. There are no callers today.
Fix: Make the proxy accept only canonical decimal user ids: strconv.ParseUint(user, 10, 64) after checking the value round-trips. Deny or route any other sub to a separate identifier authorizer. Alternatively, require identifiers to carry a non-numeric prefix, validated in ForIdentifier. Document the rule in the README.
WR-06: The default broadcast payload serializes the in-memory model: zero values on partial updates, and every exported field
File: modules/lighthouse/broadcast.go:424-432
Issue: Without a payload override, prepare marshals t.model, the value GORM wrote.
- For
db.Model(&T{ID: 5}).Updates(map[string]any{"name": "x"})(a documented, non-zero-key path that is broadcast), the model holds only the id and the assigned columns. Subscribers receive"owner_id":0,"name":""and similar zero values as if they were the new state. - The struct is marshalled with
encoding/json, so every exported field withoutjson:"-"is published, for example a password hash or anEncryptedcolumn's JSON form. The WinterCMStoArray()honours$hidden.
The delete path already reloads the full row; the create and update paths do not.
Fix: For ActionUpdated (and ActionCreated), reload the row by primary key inside the savepoint before building the default payload, as snapshot does. Document that the default payload publishes every JSON-visible field, or require models that use the default payload to implement a serializer.
WR-07: The Centrifugo subscribe proxy shares an IP-keyed rate limit bucket with public traffic
File: ../fonoteka.go/plugins/golem15/fonoteka/routes.go:86, ../fonoteka.go/plugins/golem15/fonoteka/plugin.go:321-330, modules/lighthouse/route.go:571
Issue: Surfaces.Middleware (throttle:ws-api) is appended to the ServerToServer subscribe route too. The request has no principal, so it is keyed wsapi:ip:<ClientIP> with 120 per minute. The throttle runs before the proxy's secret check, so requests with a wrong secret count against the bucket. With the shipped trusted_proxies: [], every request that reaches the app through a local reverse proxy has the same RemoteAddr. Any anonymous client can then send 120 bogus POST /api/realtime/subscribe per minute and exhaust the bucket Centrifugo's legitimate calls share. Centrifugo treats a 429 as an internal error, so every user's subscribes fail. PHP has the same throttle, but the Go port is the place to stop copying a denial-of-service vector.
Fix: Do not apply the user-facing bucket to ServerToServer routes. Either give lighthouse.Surfaces a separate ServerToServerMiddleware, or let Mount apply Middleware only to UserAuth and Public routes. If the proxy needs a limit, key it after the secret check, and document that http.trusted_proxies must be set behind a proxy.
Info
IN-01: Parity tooling cannot detect key-order drift, and several payload maps are unordered
File: modules/tide/centrifugo_golden.go:197-235; ../fonoteka.go/plugins/golem15/fonoteka/realtime.go:96; modules/lighthouse/centrifugo/handlers.go:211
Issue: DiffPublications compares bodies structurally with key order ignored. As a result, the byte-for-byte order that BroadcastArgs carefully preserves is never asserted. albumBroadcast.Album and Result.Info are map[string]any, so encoding/json emits their keys alphabetically, whereas PHP emits insertion order (see fixtures/broadcasts/created.yaml).
Fix: Add an ordered comparison mode to DiffPublications for asserted goldens. When Phase 12 fills the album subtree, build it as an ordered struct.
IN-02: The fallback to the in-memory album in albumPayload is unreachable
File: ../fonoteka.go/plugins/golem15/fonoteka/realtime.go:112-116
Issue: The broadcast callback always runs inside a transaction, because GORM opens one for every write. A failed Preload…Take therefore aborts it, so the following enqueue fails and the savepoint is rolled back. SerializeAlbum(m) is never published.
Fix: Return the read error, or wrap the fresh read in its own savepoint if the fallback is intended.
IN-03: The River scheduler and schedule:run --once disagree on DST days and sub-minute intervals
File: modules/conga/schedule.go:272-286, modules/conga/commands.go:159-172
Issue: Every.Next adds absolute durations to local midnight, while dueAt uses wall-clock minutes:
- On a 23-hour or 25-hour day,
Every(6h)fires at 07:00, 13:00 and 19:00 local (or gains an extra 23:00 run that the 24hByPerioduniqueness may then drop). --oncefires at 06:00, 12:00 and 18:00.Every(90s)fires every 90 seconds under River but every 3 minutes under--once.
Fix: Compute Every on wall-clock time (time.Date(..., h, m, ...)), and document or reject intervals that are not whole minutes.
IN-04: summer_jobs rows can stay IN_PROGRESS forever
File: modules/conga/commands.go:185-211, modules/conga/worker.go:243-262
Issue: In these cases the row is never finalized:
queue:cleardeletes River jobs but leaves the linked rows at StatusInProgress;- a job that River's rescuer discards, or one that returns
river.JobCancelorJobSnoozeon a non-final attempt, is never recorded either.
Fix: Mark linked rows StatusStopped in clearQueue (join on river_job_id), and handle JobCancel errors in runAttempt.
IN-05: The centrifugo and typesense Config structs do not redact their secrets
File: modules/lighthouse/centrifugo/config.go:477-498, modules/beachcomber/typesense/config.go:24-38
Issue: Driver.Config() and Engine.Config() return structs holding APIKey, TokenSecret and ProxySecret, which %v or slog.Any print in full. flare.Config implements String, GoString and LogValue for exactly this reason. HS256 token_secret also has no minimum length.
Fix: Add the same redacting String, GoString and LogValue methods, and warn when token_secret is shorter than 32 bytes.
IN-06: parity:broadcasts --api-key puts a secret on the command line
File: cmd/summer/parity.go:84,112
Issue: Flag values show up in ps output and shell history. The environment variable alternative already exists.
Fix: Drop the flag, or document it as test-only and prefer PARITY_CENTRIFUGO_API_KEY.
IN-07: The removal harness leaves mutated security code behind on SIGTERM or SIGKILL
File: scripts/check-phase11.sh:344-390
Issue: The harness removes a protection from tracked source and restores it only in a Python finally. That block survives SIGINT and timeouts, but not SIGTERM (for example a CI job cancel) or SIGKILL. The file is then left with the protection removed.
Fix: Install a SIGTERM handler that restores the file (signal.signal(signal.SIGTERM, ...)), or mutate a scratch copy of the tree instead of the working tree.
IN-08: Service.Emit drops the caller's context when db is nil
File: modules/lighthouse/suppress.go:428-434, modules/lighthouse/job.go:83-93
Issue: enqueue takes its context only from db.Statement.Context. With a nil db, the insert runs on context.Background(), ignoring the caller's cancellation or deadline.
Fix: Pass ctx through to enqueue explicitly.
IN-09: The three modules detect a transaction in different ways
File: modules/lighthouse/broadcast.go:449, modules/beachcomber/sync.go:266, modules/conga/conga.go:494-500
Issue: lighthouse and conga test for *sql.Tx; beachcomber tests for gorm.TxCommitter. If PrepareStmt mode is ever enabled, the connection pool is *gorm.PreparedStmtTX, and lighthouse then inserts broadcast jobs outside the write's transaction, so rolled-back writes are published.
Fix: Share one helper that recognizes gorm.TxCommitter and unwraps PreparedStmtTX to its *sql.Tx.
IN-10: RecordBroadcasts returns before the recorder has released its port
File: modules/tide/centrifugo.go:229-233
Issue: The deferred cancel() starts Shutdown asynchronously. A second recording on the same --listen address in the same process can then fail with "address already in use".
Fix: After cancel(), wait on errCh (with a short timeout) before returning.
Reviewed: 2026-09-30T13:16:12Z Reviewer: Claude (gsd-code-reviewer) Depth: standard