Skip to content

Add nil checks for vm.Properties.TimeCreated with fallback to preserve GC grace period#1390

Merged
rakechill merged 7 commits intomainfrom
copilot/check-vm-properties-nil
Feb 3, 2026
Merged

Add nil checks for vm.Properties.TimeCreated with fallback to preserve GC grace period#1390
rakechill merged 7 commits intomainfrom
copilot/check-vm-properties-nil

Conversation

Copy link
Contributor

Copilot AI commented Jan 30, 2026

  • Understand the issue: Add nil checks for vm.Properties and vm.Properties.TimeCreated before accessing on line 535
  • Review existing code patterns for nil checks (line 541 shows the pattern)
  • Add nil check for vm.Properties and vm.Properties.TimeCreated on line 535
  • Add tests to validate the nil check behavior
  • Run tests to verify the changes work correctly
  • Run linter to ensure code quality (go vet passed, code formatted with gofmt)
  • Address feedback: Add fallback to time.Now() to preserve 5-minute garbage collection grace period
  • Add TODO and comment about risk if TimeCreated is never populated
  • Final code review

Summary: Added nil checks for vm.Properties.TimeCreated with a fallback to time.Now() to prevent nil pointer dereference while ensuring the garbage collection 5-minute grace period is always enforced.

TODO Added: Documented the potential risk that if vm.Properties.TimeCreated is NEVER populated, the VM might never be garbage collected since we call this helper every time we create an in-memory NodeClaim. The code assumes VMs that fail to come up should eventually stop appearing in Azure API responses.

Original prompt

On cloudprovider.go:499-499, Can you create a PR to check if vm.Properties or vm.Properties.TimeCreated are nil before setting this creationtimestamp? Create a PR based off of the main branch please.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits January 30, 2026 23:44
Co-authored-by: rakechill <30397998+rakechill@users.noreply.github.com>
Co-authored-by: rakechill <30397998+rakechill@users.noreply.github.com>
Copilot AI changed the title [WIP] Add nil check for vm.Properties and vm.Properties.TimeCreated Add nil checks for vm.Properties.TimeCreated access Jan 30, 2026
Copilot AI requested a review from rakechill January 30, 2026 23:50
@rakechill rakechill marked this pull request as ready for review January 30, 2026 23:52
…ce period

Co-authored-by: rakechill <30397998+rakechill@users.noreply.github.com>
Copilot AI changed the title Add nil checks for vm.Properties.TimeCreated access Add nil checks for vm.Properties.TimeCreated with fallback to preserve GC grace period Jan 31, 2026
Copilot AI requested a review from rakechill January 31, 2026 00:00
@rakechill rakechill requested a review from matthchr January 31, 2026 00:07
matthchr
matthchr previously approved these changes Jan 31, 2026
Copy link
Member

@matthchr matthchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I am fine with this as-is for now but based on investigation on what caused this panic i nthe first place we may need some additional logic here.

Copy link
Contributor

@rakechill rakechill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will approve once the requested TODO comment is added

… is never populated

Co-authored-by: rakechill <30397998+rakechill@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Jan 31, 2026

will approve once the requested TODO comment is added

The TODO comment has been added in commit 7e8e1be. It documents the risk if vm.Properties.TimeCreated is never populated and suggests investigating a more fail-safe approach.

@rakechill rakechill merged commit 924d83f into main Feb 3, 2026
13 checks passed
@rakechill rakechill deleted the copilot/check-vm-properties-nil branch February 3, 2026 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants