Repository navigation
test: cover the session lifecycle, timeline, list and retention - #7360
Conversation
|
/review |
|
Claude finished @otavio's task in 5m 30s —— View job Code Review CompleteI reviewed all 10 changed files and posted 1 inline issue. Standards: 0 violations, 1 judgement call. Spec: did not run. Correctness: 0. StandardsThe worst finding is a judgement call for Duplicated Code at The code follows the rules otherwise:
SpecThe PR closes no issue and its commits carry no Additional notesThe correctness pass checked these areas against the server source and found no defects:
I dropped several smaller judgement calls because they did not clear the review bar:
To request another review round, comment |
ba759a6 to
c61ced1
Compare
|
/review |
|
Claude finished @otavio's task in 1m 51s —— View job Code Review CompleteI reviewed the 11 changed files for Standards (repo rules, Go conventions, test quality, code smells) and Correctness (security and runtime defects). I found no new issues, and the code looks good as it is. The Spec axis did not run. The PR closes no issue and its commits carry no |
c61ced1 to
7904bd1
Compare
The gateway registers a session before it reaches the device, so a password the device refuses still leaves a record behind. That record is what proves the authenticated flag is set by the login, not by the record's creation: it stays false and never joins the active set. The test agent sends an SSH keepalive every second and the gateway stamps the session on each one, on top of its own 30-second tick. The last_seen wait is 45 seconds so either source satisfies it. The web terminal case opens /ws/ssh with the agent's password, so it skips where the agent under test takes no passwords. The x/net websocket server refuses a handshake without an Origin, hence the base URL as origin. The terminal names its session in a message of kind 5 and reports a refusal in kind 4; the kinds are copied because server/ssh/web keeps them unexported. openSession, finishSession and awaitSession are shared with the list and retention tests that follow. requireSessionActive and sessionDetail now delegate to awaitSession, so the request for a session's detail is written once.
Every case now closes its connection and waits for the session to go inactive before reading the timeline. The gateway writes events in batches every 250ms and drains the queue when the session finishes, before it deactivates it, so an inactive session has its whole timeline stored. Waiting for the first event, as before, could read a timeline that still lacked the later ones. A window-change gets no reply, so the resize case waits until the shell reports the new size through stty before closing. The gateway records the event before forwarding the request, so a shell that saw the size means the event was queued. payload replaces payloadString so one accessor reads string and number fields alike.
The sessions are opened one after another, so the order they were created in is the order of their started_at, and the newest-first case can assert the exact sequence as well as the timestamps.
Some behaviour runs only from a cron job, such as session retention at 01:00. RunCron enqueues the job the server registered on a spec, so the handler runs in the server, through its own queue, without waiting for the tick. The asynq scheduler names each job by a UUID minted at boot, so a job is found by its spec in the scheduler entries asynq writes to redis. A spec two jobs share is refused rather than guessed. The jobs are enqueued on the "cron" queue, copied from pkg/worker/asynq, which keeps it unexported. The test reaches redis through a published port. A container address does not work: under rootless Docker the host network cannot route to it. pasta forwards only ports below the ephemeral range, so WithCronTrigger reserves one the way the HTTP and SSH ports are reserved. A stack that does not ask for it still publishes redis, on a port the daemon picks, because compose cannot publish a port conditionally.
The community compose file never passed SHELLHUB_SESSION_RETENTION_DAYS to the server, so the retention job was never registered on a test stack. The test compose file now passes it, at 0, which keeps retention off, unless a test asks for a window. The window is counted in whole days, so AgeSession moves a session's started_at back in the database, standing in for the days it would wait to expire. The rows are aged, not deleted: the deletion is the job's, fired through RunCron after the server logs that retention is enabled. SessionEventCount reads session_events directly because the API answers 404 for a deleted session, and the cascade is what it checks. The aged session that is still open survives because the job skips any session in the active set. Recordings are not covered: only the cloud registers a recording pruner, and without one the job deletes recorded sessions as well.
7904bd1 to
b132b0d
Compare
Summary
Covers Domain 14 (Sessions) of shellhub-io/team#243 with testcontainers tests. 14 of the 17 open items now have a test that asserts the outcome the item names. The other three are below, with the reason each is left open.
TestSessionLifecycleactiveis true while connected, false afterconn.Close(), with no close endpoint involvedweb=trueTestSessionLifecycle/ws/sshreadsweb: true; one opened over SSH readsfalseTestSessionLifecycleauthenticated: true; a password the device refuses leaves the sessionauthenticated: falseand never activewindow-changeTestSessionDetailSaysWhatTheSessionDidexit-statusTestSessionDetailSaysWhatTheSessionDidlast_seenTestSessionLifecyclelast_seenmoves past an earlier reading while the session stays activeTestSessionDetailSaysWhatTheSessionDid[0, 1], each command recorded on its own seatTestSessionLifecycledevice_uidTestSessionListeqandnelist exactly the expected sessionsTestSessionListactivetrue and falseTestSessionListclosedtrue and falsestarted_atdescendingTestSessionListstarted_atno later than the one beforeTestSessionRetentionTestSessionRetentionsession_eventsdrop from more than zero to zeroRetention runs the real job. The community compose file never passed
SHELLHUB_SESSION_RETENTION_DAYSto the server, and the job fires only at 01:00:TestSessionDetailSaysWhatTheSessionDidnow closes each connection and waits for the session to go inactive before it reads the timeline. The gateway drains its event batch when a session finishes, so the timeline is then complete. This applies to the four existing cases too.Not covered
server/api/services/service.go:128,geoip.NewNullGeoLite). The MaxMind locator is registered only bycloud/internal/cloud/init.go:45, and community CI runs the community stack. The test client also connects from a private address, which no GeoIP database places.cloud/internal/cloud/recordings.go:50. Without one,pruneRecordings(server/api/services/session-recording.go) hands every expired UID to the delete, recorded or not, so on community the behaviour this item names does not exist.GET /api/sessionswith an operator the field does not take answersc.NoContent(400)(server/api/routes/session.go:41-42). The OpenAPI spec declares a body for that 400, so the test stack's strict response validator replaces the response with a 500 (response header Content-Type has unexpected value: ""). That is OpenAPI declares a response body for statuses answered with no body #7122, which listsGET /api/sessionsamong its 400 cases. refactor(server): give the List shape the query contract it serves #7035 moves the route onto theListseam, which answers with the error envelope; feat(server): say what was wrong with a rejected filter #7061 adds a reason to it. Once either is on master the test can assert 400.#7349 (namespace-bounded session mutations) is already in this base. The namespace-scope test reads through the API, which that PR did not change.
Evidence
RunCronremoved,TestSessionRetentionfails:a finished session past the window is deleted,Condition never satisfied.testrunner:The full
tests/suite passes on the compose change (ok ... 1311.228s, 37 top-level tests).golangci-lintreports 0 issues ontests/.Merge Danger
Door: two-way
Test code and the test-only compose overlay. Nothing in the product changes.
Blast Radius: e2e-stacks
Every test stack now publishes redis on
127.0.0.1, on a port the daemon picks unless a test reserves one, because compose cannot publish a port conditionally.SHELLHUB_SESSION_RETENTION_DAYSdefaults to 0 there, which keeps retention off. Enterprise stacks pass their own value, the same 180 days.env.enterprisealready sets.