test: cover device cleanup crons, team devices, MAC twins and re-pairing by otavio · Pull Request #7600 · shellhub-io/shellhub

otavio · 2026-10-17T23:42:14Z

Summary

End-to-end coverage for five device behaviours from the shellhub-io/team#243 audit (Domains 7 and 9)
that only had unit or service tests, or none.

tests/
├── device_cleanup_test.go                 TestDeviceCleanup
│     ephemeral device offline past its timeout deleted, online one kept, accepted count drops
│     device removed > 30 days ago purged, one removed 29 days ago kept, removed count drops
├── member_paired_devices_test.go          TestPairedDevicesFollowTheirMember
│     DELETE /api/devices/:uid/owner keeps the device through its member's removal
│     an operator (no ProvisioningKeyCreate) gets 403 and the owner stays
├── device_accept_mac_connected_test.go    TestDeviceAcceptMACTwinConnected
│     accept answers 409 on identity.mac while the twin's real agent is connected; both rows intact
│     automatic-key newcomer stays pending, accepted on a later auth once the twin is offline
├── device_removal_agent_test.go           TestRemovedPairedDeviceAgent
│     tunnel closed on removal; agent with a paired tenant pairs again without a restart
│     agent with SHELLHUB_TENANT_ID exits non-zero with the "Pair it again" message
└── environment/device.go                  AgeDeviceDisconnection, AgeDeviceRemoval

The crons run through RunCron instead of being waited for. The time each one measures is moved
back in the database.

Evidence

Before: none of these paths had an e2e test. Aging both devices inside their windows fails
both TestDeviceCleanup cases ("device … is still there"). Without the fix this PR stacks on,
TestRemovedPairedDeviceAgent/an_agent_that_learned_its_tenant_from_pairing_pairs_again fails
because the agent immediately re-persists the stale tenant.
After:
```
--- PASS: TestDeviceCleanup (36.02s)
--- PASS: TestPairedDevicesFollowTheirMember (27.16s)
--- PASS: TestDeviceAcceptMACTwinConnected (35.31s)
--- PASS: TestRemovedPairedDeviceAgent (28.86s)
```

Merge Danger

Door: two-way

Blast Radius: tests

Test-only. Adds about two minutes of stack time to the e2e job.

otavio · 2026-10-18T20:03:42Z

/review

otavio · 2026-10-18T20:20:19Z

/review

otavio · 2026-10-18T20:49:41Z

/review

otavio · 2026-10-18T23:15:42Z

/review

github-actions · 2026-10-18T23:16:13Z

Claude finished @otavio's task in 5m 13s —— View job

Code Review Complete

I reviewed all 10 files for Standards (repo rules, Go conventions, tests, code smells) and Correctness (security and runtime defects) and found no new issues. The code looks good as it is. The Spec axis did not run because the PR closes no issue and its commits cite none, so there is nothing to review against.

CreateDevicePairing reused the code cached for a public key for the whole code lifetime, including after the code was accepted, and then answered with the accepted status and tenant. An agent whose paired device is removed forgets its tenant and asks for a pairing again. If that happened within ten minutes of the pairing, it got the stale acceptance back, kept the tenant, authenticated, got 401 for the removed device, and started over: about 30 pairing and auth requests a second until the code expired. Only a pending code is reused now. AcceptDevicePairing now refuses a code that is no longer pending, the same not-found answer as an expired one. Without that, an accept page left open on the old code could put the removed device back in the namespace while the agent waits on its new code, which nobody accepts. The accepted code is also dropped from the cache when the new one is minted, but that alone does not close the hole: an accept already in flight on the old code writes it back after the drop. Refusing at accept time holds whichever side wins. Re-submitting an accepted code, a double click or a retry, now answers not found instead of succeeding again. The console's accept page resolves the code first, and the resolve used to answer for a pairing code whatever its status, so a reloaded or second-tab accept page offered the namespace picker and then failed with a generic error. The resolve now refuses a pairing code that is no longer pending too, and the page shows its usual "invalid or has expired" message. Dropping the accepted code made the accepted-device lookup load-bearing. Any lookup error used to read as "no accepted device", which was harmless while the cached code still answered accepted. Now it would discard a pairing that went through: an agent restarted before saving its tenant would be handed a fresh code nobody will accept, and it never asks again. Only a not-found result moves on to the cache; any other store error is returned, so the agent exits and retries. For the same reason, an accepted cached code triggers a second lookup before it is dropped: an accept can land between the first lookup and the cache read, as when `shellhub-agent login` runs while the daemon waits on the same code, and dropping it then would leave neither process with the tenant.

Both jobs are triggered through RunCron instead of waited for, and the stamp each one measures, disconnected_at or removed_at, is moved back in the database. Each case pairs a device past its window with one inside it, so a job that deleted too much fails as well as one that deleted nothing. The namespace counts are read before the stamp is moved: the server's own scheduler keeps running in the stack, and a tick landing after the move would delete the device before the snapshot. The ephemeral case also waits out the heartbeat flush interval after the device goes offline. A keep-alive queued before the disconnect is written later and clears disconnected_at once the moved stamp is older than the beat, so the device would never qualify for cleanup. Reading a namespace and waiting for a device by name move into the environment package, since other tests repeat both.

The twin is a real agent, so the server sees it online the way it does in production. The existing name-conflict test stops its agent first and never reaches this refusal. Each re-authentication of the pending newcomer ages its last enrollment attempt past the reconcile interval, which otherwise throttles the re-evaluation for a minute.

An agent that learned its tenant from pairing forgets it and pairs again without restarting. One given SHELLHUB_TENANT_ID exits. Reaching the second case needs a device enrolled by pairing and then served with a configured tenant, so a second container reuses the first one's key, identity and hostname, which gives it the same device UID. The test waits for the agent's own connected line instead of the API's online flag: right after a pairing accept the device reads online from its fresh last_seen before the agent has polled its code.

otavio requested a review from a team as a code owner October 17, 2026 23:42

otavio added this pull request to stack #7601 October 17, 2026 23:42

otavio changed the title ~~test/t243 devices~~ test: cover device cleanup crons, team devices, MAC twins and re-pairing Oct 17, 2026

This comment has been minimized.

Sign in to view