Claude/codebase review recommendations 3ixfnh - #2
Merged
Conversation
readPump overrode UserID, Username and RoomID unconditionally but filled MessageID and Timestamp only when the client left them empty, so both were client-controllable in practice. MessageID is the DynamoDB sort key and is broadcast to the whole room in every frame. Any client could read a peer's messageId off the wire, replay it, and silently overwrite that row via PutItem. Timestamp is the GSI sort key, so a client could set a far-future value and pin itself to the head of room history permanently. Both are now assigned unconditionally alongside the existing metadata overrides. hydrateHistory replays stored messages and is unaffected; NewChatMessage/NewSystemMessage set their own fields and are unaffected. TestClient_MetadataOverride already covered the UserID/Username overrides but set neither of these fields, so the gap was invisible. It now sends a forged messageId and a year-2099 timestamp and asserts a server-generated UUID and a timestamp inside the test window. Confirmed the new assertions fail against the pre-fix code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Register, Unregister and Broadcast sent on bounded channels with no done guard. Once Run returns nothing drains them, so every caller blocks forever. readPump calls Unregister from a defer, which means shutdown leaked every client goroutine in the process. Each send is now wrapped in the select/done pattern BroadcastAll already used. The type assertions are hoisted out of the if so the select reads cleanly; the any parameter type is left alone as out of scope. The post-shutdown path is deliberately a no-op rather than a Close. The shutdown branch in Run has already closed every client it knew about, and Client.Close is a bare close(c.send), so closing a late registrant would panic on a double close. A late registrant's writePump exits on its next failed ping write instead. TestHub_SendsDoNotBlockAfterShutdown covers all four send methods against a shut-down hub and fails if they do not return within 2s. Confirmed it hangs to that deadline against the pre-fix code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two views of the same bug: the sliding window rotating underneath a reader. rateWindow treated the final slot of MessagesPerMinute as the current rate, but snapshot() starts at idx+1, so that slot is the minute currently IN PROGRESS — a partial count that advanceIfNeeded zeroes on every rotation. The runner polls every 5s while the window rotates every 60s, so a busy server reported a traffic_dropout at the top of every minute. Debounce capped it at one per five minutes, which still means roughly 12 spurious briefings an hour, each one an ElevenLabs charge. Reproduced before the fix: a window rotating from 195 to 0 fires traffic_dropout severity=yellow observed=195. rateWindow now drops the in-progress slot and compares the most recent complete minute against the mean of the ones before it. The guard rises from len < 2 to len < 3, which a complete minute plus a baseline requires. This is not behaviour-neutral, and the trade is deliberate: detection is now at whole-minute granularity, so a genuine spike or dropout surfaces up to ~60s later, and dropout can only fire in the one poll following a rotation. Twelve false briefings an hour is the worse failure — it trains the operator to ignore the voice channel, and it bills for the privilege. slidingWindow.increment read idx under the mutex and did Add(1) after unlocking, so a rotation landing in that gap dropped the count into a freshly-zeroed slot. The lock now covers both, which is what makes the regression test above meaningful rather than incidentally passing. Test fixtures encoded the buggy semantics in two places: window() in detector_test.go and spikeMetrics() in briefing/runner_test.go, both of which put the value under test in slot 14. Both now use slot 13 and leave 14 at zero. All eight TestDetector_Check cases and the dropout, severity and debounce tests pass unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
EnsureTable read any DescribeTable error as "the table does not exist". A throttle, a transport blip or rejected credentials all fell through to CreateTable, which answers ResourceInUseException, which fails repo init and drops the server into "running without persistence" — while DynamoDB is perfectly healthy. This is a strong candidate for the Phase 2A table-init issue recorded in CLAUDE.md, though not yet confirmed as its cause. The DescribeTable result is now classified three ways: nil means the table exists, ResourceNotFoundException means create it, and anything else is returned wrapped rather than guessed at. EnsureTable also returned as soon as CreateTable was accepted, so on real AWS the first writes could hit a table still in CREATING. Both paths now wait via NewTableExistsWaiter with a 60s ceiling, inside the 90s init budget main.go allocates. The exists path waits too, since a previous start may have left the table mid-creation. DynamoDB Local is effectively instant, so this costs nothing locally. Classification is extracted as isTableNotFound and tested directly: bare and wrapped ResourceNotFoundException, a throughput exception, an unrelated transport error, and nil. The package has no AWS client mock by design, so this covers the actual defect without one; the waiter is left to the DynamoDB Local run. The table schema itself is untouched, per the repo convention that DynamoDB schemas are hand-written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four claims in the README were stale or newly load-bearing after this branch: - Server-authoritative fields: the README already claimed messageId/timestamp/ roomId were server-owned, which was only true of roomId until this branch. Now states all five overwritten fields and why the two sort keys matter. - Detector granularity: rate conditions are evaluated on complete minutes, and the ~60s detection lag that buys is worth stating rather than surprising someone tuning thresholds. - Storage init: absent vs unreachable is now distinguished, and startup waits for ACTIVE. - Hub shutdown: added a Clean shutdown bullet for the done-guarded sends. Also drops the note claiming TestClient_PingPong has a pre-existing race left in intentionally. That race was fixed when Voice Ops Briefings landed and the suite has run clean under -race since; CLAUDE.md was updated at the time and the README was missed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens correctness and operational behavior across the backend by hardening DynamoDB table initialization, preventing hub shutdown goroutine leaks, and making analytics rate-based anomaly detection robust to minute-boundary rotation artifacts. It also clarifies the server-authoritative message fields contract and updates tests/docs to reflect these behaviors.
Changes:
- DynamoDB init now distinguishes “table missing” from transient/transport errors and waits before accepting writes.
- Hub send paths (register/unregister/broadcast) are guarded to avoid blocking after shutdown, with a regression test.
- Analytics rate-window logic ignores the in-progress minute slot to prevent spurious dropouts, and the sliding-window increment/rotation race is fixed; tests updated accordingly.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents server-authoritative message fields, clean shutdown behavior, DynamoDB init semantics, and minute-slot handling in anomaly detection. |
| backend/pkg/storage/dynamodb.go | Improves table init by differentiating not-found vs other errors; adds a waiter intended to block until the table is usable. |
| backend/pkg/storage/dynamodb_test.go | Adds unit test coverage for the “table not found” error classification helper. |
| backend/pkg/hub/hub.go | Guards channel sends with done to prevent post-shutdown blocking; keeps BroadcastAll safe. |
| backend/pkg/hub/hub_test.go | Adds regression test ensuring hub send methods don’t block after shutdown. |
| backend/pkg/client/client.go | Forces server overwrite of message identity/order fields (MessageID/Timestamp/etc.) on inbound frames. |
| backend/pkg/client/client_test.go | Extends metadata override test to cover forged MessageID/Timestamp and validates server-generated UUID/time window. |
| backend/pkg/briefing/runner_test.go | Updates spike fixture to place the spike in the most recent complete minute. |
| backend/pkg/analytics/detector.go | Updates rateWindow to use the most recent complete minute and ignore the in-progress slot. |
| backend/pkg/analytics/detector_test.go | Updates helpers and adds a test to prevent false dropouts during minute rotation. |
| backend/pkg/analytics/aggregator.go | Fixes a rotation/increment race by holding the lock across slot selection and increment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+369
to
379
| func (r *DynamoDBRepository) waitForActive(ctx context.Context, tableName string) error { | ||
| waiter := dynamodb.NewTableExistsWaiter(r.client) | ||
| if err := waiter.Wait(ctx, &dynamodb.DescribeTableInput{ | ||
| TableName: aws.String(tableName), | ||
| }, 60*time.Second); err != nil { | ||
| return fmt.Errorf("table %s did not become active: %w", tableName, err) | ||
| } | ||
|
|
||
| r.logger.Info("DynamoDB table is active", slog.String("table", tableName)) | ||
| return nil | ||
| } |
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.
No description provided.