app_record: Fix hangup handling before record loop

[silverjoe]

When a hangup is received while app_record is playing the
initial beep (or immediately after it), but before entering
the main recording loop, the application does not detect
the hangup and continues running until timeout.

This change adds an early hangup check before the
ast_waitfor() loop to ensure the application exits
immediately when the channel is already in a hung-up state.

Reproduction

Dialplan:

[incoming]
exten => h,1,Hangup(16)
exten => _.,1,Log(Verbose, Came the call from ${CALLERID(all)})
same => n,Playback(vm-intro)
same => n,Record(/tmp/test_vm.wav,10,60,k)
same => n,Log(Verbose, Status ${RECORD_STATUS})
same => n,Hangup(16)

Steps:

1. Place a call to this extension.
2. Wait for the prompt: "Please leave your message after the tone..."
3. Hang up around the end of the phrase "pound key".
4. Observe that app_record continues until timeout.

Expected behavior

The application should terminate immediately when the
channel is already in a hung-up state.

Fix

Add an early hangup check before the ast_waitfor() loop.

[sangoma-oss-cla]

All committers have signed the CLA.

[jcolp]

Per the AI policy you must disclose usage of it:

https://github.com/asterisk/documentation/blob/main/docs/Development/Policies-and-Procedures/AI-Policy.md

[silverjoe]

Per the AI policy you must disclose usage of it:

https://github.com/asterisk/documentation/blob/main/docs/Development/Policies-and-Procedures/AI-Policy.md

I used AI to help improve the wording of the PR description, since English is not my native language.

The issue analysis, debugging, and the actual code change were done by me and tested in my environment.

[github-actions]

Attention! This pull request may contain issues that could prevent it from being accepted. Please review the checklist below and take the recommended action. If you believe any of these are not applicable, just add a comment and let us know.

• The PR description does not match the commit message body. This can cause confusion for reviewers and future maintainers. GitHub doesn't automatically update the PR description when you update the commit message so if you've updated the commit with a force-push, please update the PR description to match the new commit message body.

Documentation:

• Asterisk Developer Documentation
  
  • Code Contribution
    
  • Commit Messages
    
  • Alembic Scripts
    

[github-actions]

Workflow PRCheck failed
20-ari1: FAILED TEST: rest_api/external_interaction/ami_bridge/stasis_app/non_stasis_app

[silverjoe]

cherry-pick-to: 22
cherry-pick-to: 23
cherry-pick-to: 20

[InterLinked1]

We should probably be handling the failure of ast_streamfile directly. Right now the code assumes the audio file didn't exist, but if a hangup occurred, that should probably be dealt with there.

Also, BYE is SIP terminology, so replace "BYE" with "hangup" in your commit message.

[phoneben]

cherry-pick-to: 21 cherry-pick-to: 22
cherry-pick-to: 21 cherry-pick-to: 22

why not 20?

[jcolp]

The current supported branches:

20, 22, 23, and master

This change will not go into 21.

[silverjoe]

We should probably be handling the failure of ast_streamfile directly. Right now the code assumes the audio file didn't exist, but if a hangup occurred, that should probably be dealt with there.

The audio file is created correctly. But no media frames arrive, actually no frames at all. So the loop just waits until ast_waitfor() finishes the waiting time.

Also, BYE is SIP terminology, so replace "BYE" with "hangup" in your commit message.

Fixed.

[silverjoe]

The current supported branches:

20, 22, 23, and master

This change will not go into 21.

I updated my comment with "cherry-pick-to".

[InterLinked1]

We should probably be handling the failure of ast_streamfile directly. Right now the code assumes the audio file didn't exist, but if a hangup occurred, that should probably be dealt with there.

The audio file is created correctly. But no media frames arrive, actually no frames at all. So the loop just waits until ast_waitfor() finishes the waiting time.

So what does ast_waitstream return in this case?

[silverjoe]

cherry-pick-to: 21 cherry-pick-to: 22
cherry-pick-to: 21 cherry-pick-to: 22

why not 20?

This PR is for branch 20.

[phoneben]

cherry-pick-to: 22
cherry-pick-to: 23
cherry-pick-to: master

it should look like this because the pr is against master, and we also cherry-picking to other branches

cherry-pick-to: 22
cherry-pick-to: 23
cherry-pick-to: 20

[silverjoe]

We should probably be handling the failure of ast_streamfile directly. Right now the code assumes the audio file didn't exist, but if a hangup occurred, that should probably be dealt with there.

The audio file is created correctly. But no media frames arrive, actually no frames at all. So the loop just waits until ast_waitfor() finishes the waiting time.

So what does ast_waitstream return in this case?

ast_waitstream returns the timeout value in milliseconds that we set in the maxduration argument.
You can try to reproduce it - it is really simple. The key point is that the hangup from your side must reach Asterisk at the 'beep' moment.

[silverjoe]

it should look like this because the pr is against master, and we also cherry-picking to other branches

O, thank you!

[InterLinked1]

We should probably be handling the failure of ast_streamfile directly. Right now the code assumes the audio file didn't exist, but if a hangup occurred, that should probably be dealt with there.

The audio file is created correctly. But no media frames arrive, actually no frames at all. So the loop just waits until ast_waitfor() finishes the waiting time.

So what does ast_waitstream return in this case?

ast_waitstream returns the timeout value in milliseconds that we set in the maxduration argument

Um, no, that's not what it's supposed to return:

asterisk/include/asterisk/file.h

Lines 211 to 215 in c2ee154

	* \retval 0 if the stream finishes
	* \retval character if it was interrupted by the channel.
	* \retval -1 on error
	*/
	int ast_waitstream(struct ast_channel *c, const char *breakon);

Did you even read the documentation?

My hunch is that some function that is already being called is returning -1 and we can simply catch that and abort, rather than doing an explicit check for hangup, which usually isn't needed.

[github-actions]

Workflow Check failed
20-ari1: FAILED TEST: rest_api/authentication_user_acl
20-pjs1: FAILED TEST: channels/pjsip/message/message_out_dialog
20-pjs2: FAILED TEST: channels/pjsip/transfers/attended_transfer/nominal/callee_local_semi_attended_transfer_record_route

[silverjoe]

We should probably be handling the failure of ast_streamfile directly. Right now the code assumes the audio file didn't exist, but if a hangup occurred, that should probably be dealt with there.

The audio file is created correctly. But no media frames arrive, actually no frames at all. So the loop just waits until ast_waitfor() finishes the waiting time.

So what does ast_waitstream return in this case?

ast_waitstream returns the timeout value in milliseconds that we set in the maxduration argument

Um, no, that's not what it's supposed to return:

asterisk/include/asterisk/file.h

Lines 211 to 215 in c2ee154

	* \retval 0 if the stream finishes
	* \retval character if it was interrupted by the channel.
	* \retval -1 on error
	*/
	int ast_waitstream(struct ast_channel *c, const char *breakon);

Did you even read the documentation?

I are talking about the ast_waitfor() function.

 * \brief Wait for input on a channel
 * \param chan channel to wait on
 * \param ms length of time to wait on the channel
 * \details
 * Wait for input on a channel for a given # of milliseconds (<0 for indefinite).
 * \retval < 0 on failure
 * \retval 0 if nothing ever arrived
 * \retval the # of ms remaining otherwise
 */
int ast_waitfor(struct ast_channel *chan, int ms);

My hunch is that some function that is already being called is returning -1 and we can simply catch that and abort, rather than doing an explicit check for hangup, which usually isn't needed.

I did not see any function returning -1 by the time the loop starts and the operation ms = ast_waitfor(chan, ms); is called.