-
Notifications
You must be signed in to change notification settings - Fork 0
Update scanner layout and renderer alignment #20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| function removeAgent(state: LayoutState, id: string): void { | ||
| const coord = state.layout.get(id); | ||
| if (coord) { | ||
| unindexAgent(id, state.spatial); | ||
| } | ||
| state.layout.delete(id); | ||
|
|
||
| const groupKey = state.agentGroupKey.get(id); | ||
| if (groupKey && coord) { | ||
| const group = state.groups.get(groupKey); | ||
| if (group) { | ||
| group.freeStack.push(gridKeyFromWorld(coord)); | ||
| } | ||
| } | ||
| state.agentGroupKey.delete(id); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Memory leak: seenGen map entries never cleaned up when agents are removed
The seenGen map accumulates entries indefinitely because removeAgent does not delete entries from it.
Click to expand
How it happens
In updateLayout (public/src/lib/layout.ts:404), every agent gets an entry in seenGen:
state.seenGen.set(id, gen);When agents are removed via removeAgent (public/src/lib/layout.ts:354-369), the function cleans up layout, spatial, and agentGroupKey, but never removes the entry from seenGen:
function removeAgent(state: LayoutState, id: string): void {
// ... cleans layout, spatial, agentGroupKey
// BUT: state.seenGen.delete(id) is missing!
}Impact
Over time, as agents come and go (processes start and stop), the seenGen map will grow unboundedly. Each unique agent ID that has ever existed will remain in the map forever (until agents.length === 0 triggers a full clear at line 393). This causes gradual memory growth in long-running applications.
Actual vs Expected
- Actual:
seenGenretains entries for all agents that have ever existed - Expected:
seenGenshould only contain entries for currently active agents
Recommendation: Add state.seenGen.delete(id); to the removeAgent function to clean up the entry when an agent is removed.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80ada1db1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function findPlacement( | ||
| group: GroupState, | ||
| spatial: SpatialIndex, | ||
| maxAttempts = 256 | ||
| ): Coordinate | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid fixed 256-attempt placement cap
The new findPlacement hard-caps the spiral search at 256 attempts. In crowded layouts (or large agent counts in one group), it’s easy to exhaust 256 collisions, causing findPlacement to return null and addAgent to bail out without assigning a coordinate. That results in new agents silently not rendering. Previously the search radius scaled with layout size, so placement kept expanding. Consider scaling maxAttempts with current layout density or continuing the spiral until a free cell is found.
Useful? React with 👍 / 👎.
Summary
Testing
Ensure you read the contribution guide before submitting a commit and follow the exact guidelines.