fix: seven bugs from code audit (routing loop, drag overlap, resource leaks)#183
Open
pshenok wants to merge 1 commit into
Open
fix: seven bugs from code audit (routing loop, drag overlap, resource leaks)#183pshenok wants to merge 1 commit into
pshenok wants to merge 1 commit into
Conversation
Findings from a two-agent audit of routing, drag/drop, campaign, and resource cleanup: 1. ALB<->SQS routing loop + request leak. 'alb -> sqs' was the only bidirectional-with-its-reverse rule in the connection table. SQS's forwarding pushes to ALB (downstreamTypes=['alb']) while ALB's generic branch forwards to any connection with no backpressure, so an alb->sqs->alb pair bounced requests forever — never removed from STATE.requests, unbounded queue growth, silent money sink. Removed the alb->sqs rule; sqs->alb (the real path) stays. 2. Campaign burst callbacks leaked across retry/next. burstPattern schedules setTimeout spawns; the guard only checked this.active/ended, but Retry/Next go straight to loadLevel() (which resets ended=false) without exit(), so stale callbacks fired into the new attempt. Added a monotonic session id captured at schedule time. 3. Dropping a dragged node onto an occupied tile overlapped two services, making one permanently unselectable (raycast hits only the front mesh) while it kept charging upkeep. Reject the drop (revert to start tile) when the target tile is occupied — same guard createService already uses. 4. clearAllServices leaked connection geometry/material — every other removal path disposes them. Now disposes too. 5. Held keys weren't cleared on window blur, so a keyup missed during an alt-tab left the camera panning forever. Added a blur handler. 6. Dragging the Internet node never moved its ring (dead else branch: internetNode.mesh is always truthy). Branch on node identity instead. 7. _persistWin could poison best time with NaN if a completed[] entry lacked bestTimeSec. Guard with Number.isFinite. Verified in-browser: alb->sqs now rejected while sqs->alb works, clearAllServices runs clean, campaign session id increments per level load, _persistWin returns a finite best time from a malformed entry.
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.
A follow-up audit (two parallel agents over routing, drag/drop, campaign, and resource cleanup) surfaced seven distinct bugs. All verified in-browser before fixing.
1.
ALB → SQSrouting loop + permanent request leak (most severe)alb → sqswas the only rule in the connection-validity table whose reverse (sqs → alb) is also valid. SQS's forwarding pushes to ALB (Service.jsdownstreamTypes = ["alb"]), while ALB has no dedicated case and falls into the generic forwarding branch that pushes to any connection with no backpressure check. So analb ⇄ sqspair bounces a requestALB→SQS→ALB→SQS…forever — it's never handed tofinishRequest/failRequest/removeRequest, leaking it fromSTATE.requests, growing the queue unbounded, and charging upkeep for zero throughput. Fix: removed thealb → sqsrule;sqs → alb(the real forwarding direction) stays.2. Campaign burst callbacks leak across Retry / Next
burstPatternschedules up toburstSizesetTimeoutspawns spaced 20ms apart. The bail-out guard only checkedthis.active/ended, but Retry and Next callstartCampaignLevel → loadLeveldirectly (which resetsended = false) withoutexit(). Stale callbacks from the previous attempt then passed the guard and injected phantom requests into the freshly-loaded level, polluting its scoring before the player even pressed Play. Fix: monotonic session id captured at schedule time; a callback bails if the session changed.3. Drag-drop onto an occupied tile makes a service unremovable
The drag path never applied the overlap guard
createServiceuses. Dropping node B onto node A's tile left them sharing a position; the raycaster only ever returns the front mesh, so the other service became permanently unselectable/undeletable/un-upgradeable while still charging upkeep. Fix: reject the drop (revert to the drag-start tile) when the target tile is occupied.4.
clearAllServicesleaked connection GPU resourcesEvery other connection-removal path disposes
geometry/material; "Clear All" only removed the mesh from the scene. Fix: dispose them too.5. Held keys never cleared on window blur
A
keyupmissed during an alt-tab left the key stuck inkeysPressed, panning the camera forever. Fix:window blurhandler clears all held keys.6. Dragging the Internet node never moved its ring
if (draggedNode.mesh)— butinternetNode.meshis always set, so theelsebranch (the only one that also moves the ring) was dead code; the cyan ring stayed frozen at spawn. Fix: branch on node identity.7.
_persistWincould poison best time withNaNA
completed[]entry missingbestTimeSec(hand-edited/future schema) madeMath.min(undefined, elapsed) = NaN, permanently displaying "NaNs". Fix:Number.isFiniteguard.Verification
In-browser:
alb → sqsnow rejected whilesqs → albstill connects;clearAllServicesruns without error; campaign_sessionincrements on each level load (stale callbacks invalidated);_persistWinreturns a finite best time (42.5) and correct max stars from a malformed entry. No new console errors. No build step — served raw as before.