Fix/#325 incomplete debug message

[mpass99]

Closes #325

In order to fix the incomplete debug message we aim to ignore output of the runner after we have sent the SIGQUIT.

We could do so by:

• moving the SentryDebugWriter away from the debug command creation (in nomad.go) to the event processing (in nomad_runner.go).
• Let the SentryDebugWriter probe if it can write to the CodeOceanWriter before it processes the data.
• Passing a second context from nomad.go to the nomad_runner.go that is done when no output should be transferred (before the execution should be aborted).

I liked the second option the most which is now implemented in this PR.
The regression test is in a separate commit as it took more changes than the actual fix.

[codecov]

Codecov Report

Merging #417 (dd5117f) into main (0078b4c) will increase coverage by 0.11%.
The diff coverage is 94.33%.

@@            Coverage Diff             @@
##             main     #417      +/-   ##
==========================================
+ Coverage   75.59%   75.71%   +0.11%     
==========================================
  Files          37       37              
  Lines        3487     3508      +21     
==========================================
+ Hits         2636     2656      +20     
  Misses        686      686              
- Partials      165      166       +1     

Files Changed	Coverage Δ
internal/config/config.go	96.66% <ø> (ø)
pkg/dto/dto.go	52.38% <ø> (ø)
internal/nomad/sentry_debug_writer.go	76.05% <50.00%> (-2.41%)	⬇️
cmd/poseidon/main.go	62.50% <100.00%> (ø)
internal/api/websocket.go	94.36% <100.00%> (+0.08%)	⬆️
internal/api/ws/codeocean_writer.go	79.34% <100.00%> (+4.64%)	⬆️
pkg/logging/logging.go	80.00% <100.00%> (+2.00%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[MrSerth]

Awesome, this PR (and the knowledge gained to create it) is great news! Many thanks as well to separate the regression test in a dedicated commit and writing it first (that's real TDD)!

Your code changes look fine, and I recognize the probing you suggest here. There is just a minor thing I wasn't sure about: When & how is determined that the CodeOceanWriter isn't writable any longer? Is it through this context, that gets changed in this PR?

From my (limited) understanding, the second, or third option sound good, and I am fine to go with the second one you implemented. 👍

[mpass99]

When & how is determined that the CodeOceanWriter isn't writable any longer? Is it through this context, that gets changed in this PR?

Exactly, this context represents if the CodeOceanWriter is writable or not.

• When the client disconnects, the context is canceled here:
  

  poseidon/internal/api/websocket.go

  Lines 50 to 52 in ed1b83d

  	connection.SetCloseHandler(func(code int, text string) error {
  	log.WithContext(wsCtx).WithField("code", code).WithField("text", text).Debug("The client closed the connection.")
  	cancelWsCommunication()

  • Additionally, we cleanly stop the goroutine here:
    

    poseidon/internal/api/websocket.go

    Lines 65 to 68 in 8490e34

    	case <-wp.ctx.Done():
    	log.WithContext(wp.ctx).Info("Client closed the connection")
    	wp.Input.Stop()
    	wp.Output.Close(nil)

• When an (external) error appears when sending WebSocket messages, the context is cancelled (indirectly) here as done will be true:
  

  poseidon/internal/api/ws/codeocean_writer.go

  Lines 135 to 137 in ed1b83d

  	done := sendMessage(cw.connection, message.data, cw.ctx)
  	if done || message.done {
  	writingLoopDone()

• When the execution is done (the normal case), we Close the CodeOceanWriter here:
  

  poseidon/internal/api/websocket.go

  Lines 72 to 75 in 8490e34

  	case exitInfo = <-exit:
  	log.WithContext(wp.ctx).Info("Execution returned")
  	wp.Input.Stop()
  	wp.Output.Close(&exitInfo)

  • This leads to the context being cancelled (indirectly) here as message.done will be true:
    

    poseidon/internal/api/ws/codeocean_writer.go

    Lines 135 to 137 in ed1b83d

    	done := sendMessage(cw.connection, message.data, cw.ctx)
    	if done || message.done {
    	writingLoopDone()

[MrSerth]

Awesome, thanks for your detailed explanation. This makes sense to me, and I was now able to fully understand it :)